[v2] drm/dp: Clarify that wait_hpd_asserted() is not optional for panels

In response to my patch removing the "wait for HPD" logic at the
beginning of the MSM DP transfer() callback [1], we had some debate
about what the "This is an optional function" meant in the
documentation of the wait_hpd_asserted() callback. Let's clarify.

As talked about in the MSM DP patch [1], before wait_hpd_asserted()
was introduced there was no great way for panel drivers to wait for
HPD in the case that the "built-in" HPD signal was used. Panel drivers
could only wait for HPD if a GPIO was used. At the time, we ended up
just saying that if we were using the "built-in" HPD signal that DP
AUX controllers needed to wait for HPD themselves at the beginning of
their transfer() callback. The fact that the wait for HPD at the
beginning of transfer() was awkward/problematic was the whole reason
wait_hpd_asserted() was added.

Let's make it obvious that if a DP AUX controller implements
wait_hpd_asserted() that they don't need a loop waiting for HPD at the
start of their transfer() function. We'll still allow DP controllers
to work the old way but mark it as deprecated.

[1] https://lore.kernel.org/r/20240315143621.v2.3.I535606f6d4f7e3e5588bb75c55996f61980183cd@changeid

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
I would consider changing the docs to say that implementing
wait_hpd_asserted() is actually _required_ for any DP controllers that
want to support eDP panels parented on the DP AUX bus. The issue is
that one DP controller (tegra/dpaux.c, found by looking for those that
include display/drm_dp_aux_bus.h) does populate the DP AUX bus but
doesn't implement wait_hpd_asserted(). I'm actually not sure how/if
this work on tegra since I also don't see any delay loop for HPD in
tegra's transfer() callback. For now, I've left wait_hpd_asserted() as
optional and described the old/deprecated way things used to work
before wait_hpd_asserted().

Changes in v2:
- Make it clear that panels don't need to call if HPD is a GPIO.

 include/drm/display/drm_dp_helper.h | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

On Tue, 19 Mar 2024 at 22:58, Douglas Anderson <dianders@chromium.org> wrote:
>
> In response to my patch removing the "wait for HPD" logic at the
> beginning of the MSM DP transfer() callback [1], we had some debate
> about what the "This is an optional function" meant in the
> documentation of the wait_hpd_asserted() callback. Let's clarify.
>
> As talked about in the MSM DP patch [1], before wait_hpd_asserted()
> was introduced there was no great way for panel drivers to wait for
> HPD in the case that the "built-in" HPD signal was used. Panel drivers
> could only wait for HPD if a GPIO was used. At the time, we ended up
> just saying that if we were using the "built-in" HPD signal that DP
> AUX controllers needed to wait for HPD themselves at the beginning of
> their transfer() callback. The fact that the wait for HPD at the
> beginning of transfer() was awkward/problematic was the whole reason
> wait_hpd_asserted() was added.
>
> Let's make it obvious that if a DP AUX controller implements
> wait_hpd_asserted() that they don't need a loop waiting for HPD at the
> start of their transfer() function. We'll still allow DP controllers
> to work the old way but mark it as deprecated.
>
> [1] https://lore.kernel.org/r/20240315143621.v2.3.I535606f6d4f7e3e5588bb75c55996f61980183cd@changeid
>
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> I would consider changing the docs to say that implementing
> wait_hpd_asserted() is actually _required_ for any DP controllers that
> want to support eDP panels parented on the DP AUX bus. The issue is
> that one DP controller (tegra/dpaux.c, found by looking for those that
> include display/drm_dp_aux_bus.h) does populate the DP AUX bus but
> doesn't implement wait_hpd_asserted(). I'm actually not sure how/if
> this work on tegra since I also don't see any delay loop for HPD in
> tegra's transfer() callback. For now, I've left wait_hpd_asserted() as
> optional and described the old/deprecated way things used to work
> before wait_hpd_asserted().
>
> Changes in v2:
> - Make it clear that panels don't need to call if HPD is a GPIO.
>
>  include/drm/display/drm_dp_helper.h | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Hi,

On Tue, Mar 19, 2024 at 3:45 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Tue, 19 Mar 2024 at 22:58, Douglas Anderson <dianders@chromium.org> wrote:
> >
> > In response to my patch removing the "wait for HPD" logic at the
> > beginning of the MSM DP transfer() callback [1], we had some debate
> > about what the "This is an optional function" meant in the
> > documentation of the wait_hpd_asserted() callback. Let's clarify.
> >
> > As talked about in the MSM DP patch [1], before wait_hpd_asserted()
> > was introduced there was no great way for panel drivers to wait for
> > HPD in the case that the "built-in" HPD signal was used. Panel drivers
> > could only wait for HPD if a GPIO was used. At the time, we ended up
> > just saying that if we were using the "built-in" HPD signal that DP
> > AUX controllers needed to wait for HPD themselves at the beginning of
> > their transfer() callback. The fact that the wait for HPD at the
> > beginning of transfer() was awkward/problematic was the whole reason
> > wait_hpd_asserted() was added.
> >
> > Let's make it obvious that if a DP AUX controller implements
> > wait_hpd_asserted() that they don't need a loop waiting for HPD at the
> > start of their transfer() function. We'll still allow DP controllers
> > to work the old way but mark it as deprecated.
> >
> > [1] https://lore.kernel.org/r/20240315143621.v2.3.I535606f6d4f7e3e5588bb75c55996f61980183cd@changeid
> >
> > Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > I would consider changing the docs to say that implementing
> > wait_hpd_asserted() is actually _required_ for any DP controllers that
> > want to support eDP panels parented on the DP AUX bus. The issue is
> > that one DP controller (tegra/dpaux.c, found by looking for those that
> > include display/drm_dp_aux_bus.h) does populate the DP AUX bus but
> > doesn't implement wait_hpd_asserted(). I'm actually not sure how/if
> > this work on tegra since I also don't see any delay loop for HPD in
> > tegra's transfer() callback. For now, I've left wait_hpd_asserted() as
> > optional and described the old/deprecated way things used to work
> > before wait_hpd_asserted().
> >
> > Changes in v2:
> > - Make it clear that panels don't need to call if HPD is a GPIO.
> >
> >  include/drm/display/drm_dp_helper.h | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

I don't think this is controversial and I've been involved / written
most of the code for eDP panel interactions, so I think I can land
this myself with Dmitry and Abhinav's review (thanks!). If someone
later comes back with additional thoughts I'm happy to update things
more.

Pushed to drm-misc-next:

6376eb8b9115 drm/dp: Clarify that wait_hpd_asserted() is not optional for panels