On Tue, 19 Mar 2024 at 21:02, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote: > > > > On 3/19/2024 11:35 AM, Dmitry Baryshkov wrote: > > On Tue, 19 Mar 2024 at 20:15, Douglas Anderson <dianders@xxxxxxxxxxxx> 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 > >> > >> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx> > >> --- > >> 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(). > >> > >> include/drm/display/drm_dp_helper.h | 8 +++++++- > >> 1 file changed, 7 insertions(+), 1 deletion(-) > >> > >> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h > >> index a62fcd051d4d..b170efa1f5d2 100644 > >> --- a/include/drm/display/drm_dp_helper.h > >> +++ b/include/drm/display/drm_dp_helper.h > >> @@ -422,7 +422,13 @@ struct drm_dp_aux { > >> * @wait_hpd_asserted: wait for HPD to be asserted > >> * > >> * This is mainly useful for eDP panels drivers to wait for an eDP > >> - * panel to finish powering on. This is an optional function. > >> + * panel to finish powering on. It is optional for DP AUX controllers > >> + * to implement this function but required for DP AUX endpoints (panel > >> + * drivers) to call it after powering up but before doing AUX transfers. > >> + * If a DP AUX controller does not implement this function then it > >> + * may still support eDP panels that use the AUX controller's built-in > >> + * HPD signal by implementing a long wait for HPD in the transfer() > >> + * callback, though this is deprecated. > > > > It doesn't cover a valid case when the panel driver handles HPD signal > > on its own. > > > > This doc is only for wait_for_hpd_asserted(). If panel driver handles > HPD signal on its own, this will not be called. Do we need a doc for that? This comment declares that this callback must be called by the panel driver: '...but required for DP AUX endpoints [...] to call it after powering up but before doing AUX transfers.' If we were to follow documentation changes from this patch, we'd have to patch panel-edp to always call wait_for_hpd_asserted, even if HPD GPIO is used. However this is not correct from my POV. > >> * > >> * This function will efficiently wait for the HPD signal to be > >> * asserted. The `wait_us` parameter that is passed in says that we > >> -- > >> 2.44.0.291.gc1ea87d7ee-goog > >> > > > > -- With best wishes Dmitry
