* [PATCH] drm/dp: Clarify that wait_hpd_asserted() is not optional for panels
@ 2024-03-19 18:14 Douglas Anderson
2024-03-19 18:26 ` Abhinav Kumar
2024-03-19 18:35 ` Dmitry Baryshkov
0 siblings, 2 replies; 8+ messages in thread
From: Douglas Anderson @ 2024-03-19 18:14 UTC (permalink / raw
To: dri-devel, Abhinav Kumar, Dmitry Baryshkov
Cc: linux-tegra, Mikko Perttunen, Jonathan Hunter, Thierry Reding,
Douglas Anderson, Ankit Nautiyal, Daniel Vetter, David Airlie,
Imre Deak, Jani Nikula, Maxime Ripard, linux-kernel
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@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().
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.
*
* 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/dp: Clarify that wait_hpd_asserted() is not optional for panels
2024-03-19 18:14 [PATCH] drm/dp: Clarify that wait_hpd_asserted() is not optional for panels Douglas Anderson
@ 2024-03-19 18:26 ` Abhinav Kumar
2024-03-19 18:35 ` Dmitry Baryshkov
1 sibling, 0 replies; 8+ messages in thread
From: Abhinav Kumar @ 2024-03-19 18:26 UTC (permalink / raw
To: Douglas Anderson, dri-devel, Dmitry Baryshkov
Cc: linux-tegra, Mikko Perttunen, Jonathan Hunter, Thierry Reding,
Ankit Nautiyal, Daniel Vetter, David Airlie, Imre Deak,
Jani Nikula, Maxime Ripard, linux-kernel
On 3/19/2024 11:14 AM, Douglas Anderson 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@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().
>
> include/drm/display/drm_dp_helper.h | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
Thanks for the change,
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/dp: Clarify that wait_hpd_asserted() is not optional for panels
2024-03-19 18:14 [PATCH] drm/dp: Clarify that wait_hpd_asserted() is not optional for panels Douglas Anderson
2024-03-19 18:26 ` Abhinav Kumar
@ 2024-03-19 18:35 ` Dmitry Baryshkov
2024-03-19 19:02 ` Abhinav Kumar
1 sibling, 1 reply; 8+ messages in thread
From: Dmitry Baryshkov @ 2024-03-19 18:35 UTC (permalink / raw
To: Douglas Anderson
Cc: dri-devel, Abhinav Kumar, linux-tegra, Mikko Perttunen,
Jonathan Hunter, Thierry Reding, Ankit Nautiyal, Daniel Vetter,
David Airlie, Imre Deak, Jani Nikula, Maxime Ripard, linux-kernel
On Tue, 19 Mar 2024 at 20:15, 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
>
> 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().
>
> 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 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/dp: Clarify that wait_hpd_asserted() is not optional for panels
2024-03-19 18:35 ` Dmitry Baryshkov
@ 2024-03-19 19:02 ` Abhinav Kumar
2024-03-19 20:16 ` Dmitry Baryshkov
0 siblings, 1 reply; 8+ messages in thread
From: Abhinav Kumar @ 2024-03-19 19:02 UTC (permalink / raw
To: Dmitry Baryshkov, Douglas Anderson
Cc: dri-devel, linux-tegra, Mikko Perttunen, Jonathan Hunter,
Thierry Reding, Ankit Nautiyal, Daniel Vetter, David Airlie,
Imre Deak, Jani Nikula, Maxime Ripard, linux-kernel
On 3/19/2024 11:35 AM, Dmitry Baryshkov wrote:
> On Tue, 19 Mar 2024 at 20:15, 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
>>
>> 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().
>>
>> 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 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
>>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/dp: Clarify that wait_hpd_asserted() is not optional for panels
2024-03-19 19:02 ` Abhinav Kumar
@ 2024-03-19 20:16 ` Dmitry Baryshkov
2024-03-19 20:38 ` Abhinav Kumar
0 siblings, 1 reply; 8+ messages in thread
From: Dmitry Baryshkov @ 2024-03-19 20:16 UTC (permalink / raw
To: Abhinav Kumar
Cc: Douglas Anderson, dri-devel, linux-tegra, Mikko Perttunen,
Jonathan Hunter, Thierry Reding, Ankit Nautiyal, Daniel Vetter,
David Airlie, Imre Deak, Jani Nikula, Maxime Ripard, linux-kernel
On Tue, 19 Mar 2024 at 21:02, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 3/19/2024 11:35 AM, Dmitry Baryshkov wrote:
> > On Tue, 19 Mar 2024 at 20:15, 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
> >>
> >> 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().
> >>
> >> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/dp: Clarify that wait_hpd_asserted() is not optional for panels
2024-03-19 20:16 ` Dmitry Baryshkov
@ 2024-03-19 20:38 ` Abhinav Kumar
2024-03-19 20:54 ` Dmitry Baryshkov
0 siblings, 1 reply; 8+ messages in thread
From: Abhinav Kumar @ 2024-03-19 20:38 UTC (permalink / raw
To: Dmitry Baryshkov
Cc: Douglas Anderson, dri-devel, linux-tegra, Mikko Perttunen,
Jonathan Hunter, Thierry Reding, Ankit Nautiyal, Daniel Vetter,
David Airlie, Imre Deak, Jani Nikula, Maxime Ripard, linux-kernel
On 3/19/2024 1:16 PM, Dmitry Baryshkov wrote:
> On Tue, 19 Mar 2024 at 21:02, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 3/19/2024 11:35 AM, Dmitry Baryshkov wrote:
>>> On Tue, 19 Mar 2024 at 20:15, 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
>>>>
>>>> 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().
>>>>
>>>> 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 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
>>>>
>>>
>>>
>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/dp: Clarify that wait_hpd_asserted() is not optional for panels
2024-03-19 20:38 ` Abhinav Kumar
@ 2024-03-19 20:54 ` Dmitry Baryshkov
2024-03-19 21:00 ` Doug Anderson
0 siblings, 1 reply; 8+ messages in thread
From: Dmitry Baryshkov @ 2024-03-19 20:54 UTC (permalink / raw
To: Abhinav Kumar
Cc: Douglas Anderson, dri-devel, linux-tegra, Mikko Perttunen,
Jonathan Hunter, Thierry Reding, Ankit Nautiyal, Daniel Vetter,
David Airlie, Imre Deak, Jani Nikula, Maxime Ripard, linux-kernel
On Tue, 19 Mar 2024 at 22:39, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 3/19/2024 1:16 PM, Dmitry Baryshkov wrote:
> > On Tue, 19 Mar 2024 at 21:02, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 3/19/2024 11:35 AM, Dmitry Baryshkov wrote:
> >>> On Tue, 19 Mar 2024 at 20:15, 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
> >>>>
> >>>> 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().
> >>>>
> >>>> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/dp: Clarify that wait_hpd_asserted() is not optional for panels
2024-03-19 20:54 ` Dmitry Baryshkov
@ 2024-03-19 21:00 ` Doug Anderson
0 siblings, 0 replies; 8+ messages in thread
From: Doug Anderson @ 2024-03-19 21:00 UTC (permalink / raw
To: Dmitry Baryshkov
Cc: Abhinav Kumar, dri-devel, linux-tegra, Mikko Perttunen,
Jonathan Hunter, Thierry Reding, Ankit Nautiyal, Daniel Vetter,
David Airlie, Imre Deak, Jani Nikula, Maxime Ripard, linux-kernel
Hi,
On Tue, Mar 19, 2024 at 1:55 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> > >>>> - * 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.
Adjusted wording in v2. Kept Abhniav's R-b. PTAL.
https://lore.kernel.org/r/20240319135836.v2.1.I521dad0693cc24fe4dd14cba0c7048d94f5b6b41@changeid
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-03-19 21:00 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-19 18:14 [PATCH] drm/dp: Clarify that wait_hpd_asserted() is not optional for panels Douglas Anderson
2024-03-19 18:26 ` Abhinav Kumar
2024-03-19 18:35 ` Dmitry Baryshkov
2024-03-19 19:02 ` Abhinav Kumar
2024-03-19 20:16 ` Dmitry Baryshkov
2024-03-19 20:38 ` Abhinav Kumar
2024-03-19 20:54 ` Dmitry Baryshkov
2024-03-19 21:00 ` Doug Anderson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.