On Tue, 19 Mar 2024 at 22:39, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote: > > > > On 3/19/2024 1:16 PM, Dmitry Baryshkov wrote: > > 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. > > > > hmmm I dont mind explicitly saying "unless the panel can independently > check the HPD state" but not required in my opinion because if panel was > capable of checking the HPD gpio (its self-capable) why would it even > call wait_for_hpd_asserted? I'm fine with the proposed change. Doug? > > I will let you and Doug discuss this but fwiw, I am fine without this > additional clarification. So the R-b stands with or without this > additional clause. > > >>>> * > >>>> * 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
