drm/msm/dp: ST_DISPLAY_OFF hpd cleanup

[PATCH 2/4] drm/msm/dp: Return early from atomic_enable() if ST_DISCONNECT_PENDING

Posted by Jessica Zhang 8 months, 2 weeks ago

From: Abhinav Kumar <quic_abhinavk@quicinc.com>

The checks in msm_dp_bridge_atomic_enable() for making sure that we are in
ST_DISPLAY_OFF OR ST_MAINLINK_READY seem redundant.

DRM fwk shall not issue any commits if state is not ST_MAINLINK_READY as
msm_dp's atomic_check callback returns a failure if state is not
ST_MAINLINK_READY.

For the ST_DISPLAY_OFF check, its mainly to guard against a scenario that
there is an atomic_enable() without a prior atomic_disable() which once
again should not really happen.

Since it's still possible for the state machine to transition to
ST_DISCONNECT_PENDING between atomic_check() and atomic_commit(), change
this check to return early if hpd_state is ST_DISCONNECT_PENDING.

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Signed-off-by: Jessica Zhang <jessica.zhang@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 1d7cda62d5fb..f2820f06f5dc 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1512,7 +1512,7 @@ void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
 	}
 
 	hpd_state = msm_dp_display->hpd_state;
-	if (hpd_state != ST_DISPLAY_OFF && hpd_state != ST_MAINLINK_READY) {
+	if (hpd_state == ST_DISCONNECT_PENDING) {
 		mutex_unlock(&msm_dp_display->event_mutex);
 		return;
 	}

-- 
2.49.0

Re: [PATCH 2/4] drm/msm/dp: Return early from atomic_enable() if ST_DISCONNECT_PENDING

Posted by Dmitry Baryshkov 8 months, 2 weeks ago

On Fri, 30 May 2025 at 02:15, Jessica Zhang
<jessica.zhang@oss.qualcomm.com> wrote:
>
> From: Abhinav Kumar <quic_abhinavk@quicinc.com>
>
> The checks in msm_dp_bridge_atomic_enable() for making sure that we are in
> ST_DISPLAY_OFF OR ST_MAINLINK_READY seem redundant.
>
> DRM fwk shall not issue any commits if state is not ST_MAINLINK_READY as
> msm_dp's atomic_check callback returns a failure if state is not
> ST_MAINLINK_READY.

What if the state changes between atomic_check() and atomic_enable()?
There are no locks, cable unplugging is async, so it's perfectly
possible.

>
> For the ST_DISPLAY_OFF check, its mainly to guard against a scenario that
> there is an atomic_enable() without a prior atomic_disable() which once
> again should not really happen.
>
> Since it's still possible for the state machine to transition to
> ST_DISCONNECT_PENDING between atomic_check() and atomic_commit(), change
> this check to return early if hpd_state is ST_DISCONNECT_PENDING.

Can we really, please, drop the state machine? I had other plans for
the next week, but maybe I should just do it, so that by the end of
6.17 cycle we can have a merged, stable and working solution? This
topic has been lingering for months without any actual change.

>
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> Signed-off-by: Jessica Zhang <jessica.zhang@oss.qualcomm.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 1d7cda62d5fb..f2820f06f5dc 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1512,7 +1512,7 @@ void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
>         }
>
>         hpd_state = msm_dp_display->hpd_state;
> -       if (hpd_state != ST_DISPLAY_OFF && hpd_state != ST_MAINLINK_READY) {
> +       if (hpd_state == ST_DISCONNECT_PENDING) {



>                 mutex_unlock(&msm_dp_display->event_mutex);
>                 return;
>         }
>
> --
> 2.49.0
>


-- 
With best wishes
Dmitry

Re: [PATCH 2/4] drm/msm/dp: Return early from atomic_enable() if ST_DISCONNECT_PENDING

Posted by Jessica Zhang 8 months, 1 week ago


On 5/30/2025 9:04 AM, Dmitry Baryshkov wrote:
> On Fri, 30 May 2025 at 02:15, Jessica Zhang
> <jessica.zhang@oss.qualcomm.com> wrote:
>>
>> From: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>
>> The checks in msm_dp_bridge_atomic_enable() for making sure that we are in
>> ST_DISPLAY_OFF OR ST_MAINLINK_READY seem redundant.
>>
>> DRM fwk shall not issue any commits if state is not ST_MAINLINK_READY as
>> msm_dp's atomic_check callback returns a failure if state is not
>> ST_MAINLINK_READY.
> 
> What if the state changes between atomic_check() and atomic_enable()?
> There are no locks, cable unplugging is async, so it's perfectly
> possible.
> 
>>
>> For the ST_DISPLAY_OFF check, its mainly to guard against a scenario that
>> there is an atomic_enable() without a prior atomic_disable() which once
>> again should not really happen.
>>
>> Since it's still possible for the state machine to transition to
>> ST_DISCONNECT_PENDING between atomic_check() and atomic_commit(), change
>> this check to return early if hpd_state is ST_DISCONNECT_PENDING.
> 
> Can we really, please, drop the state machine? I had other plans for
> the next week, but maybe I should just do it, so that by the end of
> 6.17 cycle we can have a merged, stable and working solution? This
> topic has been lingering for months without any actual change.

FWIW, I'm currently working on the state machine rework by the middle of 
next week.

I'm anticipating that the rework itself will take some time to get 
merged, so didn't want MST to get delayed more by this series.

Thanks,

Jessica Zhang

> 
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> Signed-off-by: Jessica Zhang <jessica.zhang@oss.qualcomm.com>
>> ---
>>   drivers/gpu/drm/msm/dp/dp_display.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 1d7cda62d5fb..f2820f06f5dc 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -1512,7 +1512,7 @@ void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
>>          }
>>
>>          hpd_state = msm_dp_display->hpd_state;
>> -       if (hpd_state != ST_DISPLAY_OFF && hpd_state != ST_MAINLINK_READY) {
>> +       if (hpd_state == ST_DISCONNECT_PENDING) {
> 
> 
> 
>>                  mutex_unlock(&msm_dp_display->event_mutex);
>>                  return;
>>          }
>>
>> --
>> 2.49.0
>>
> 
>

Re: [PATCH 2/4] drm/msm/dp: Return early from atomic_enable() if ST_DISCONNECT_PENDING

Posted by OSS 8 months, 1 week ago


On 2 June 2025 18:54:12 BST, Jessica Zhang <jessica.zhang@oss.qualcomm.com> wrote:
>
>
>On 5/30/2025 9:04 AM, Dmitry Baryshkov wrote:
>> On Fri, 30 May 2025 at 02:15, Jessica Zhang
>> <jessica.zhang@oss.qualcomm.com> wrote:
>>> 
>>> From: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>> 
>>> The checks in msm_dp_bridge_atomic_enable() for making sure that we are in
>>> ST_DISPLAY_OFF OR ST_MAINLINK_READY seem redundant.
>>> 
>>> DRM fwk shall not issue any commits if state is not ST_MAINLINK_READY as
>>> msm_dp's atomic_check callback returns a failure if state is not
>>> ST_MAINLINK_READY.
>> 
>> What if the state changes between atomic_check() and atomic_enable()?
>> There are no locks, cable unplugging is async, so it's perfectly
>> possible.
>> 
>>> 
>>> For the ST_DISPLAY_OFF check, its mainly to guard against a scenario that
>>> there is an atomic_enable() without a prior atomic_disable() which once
>>> again should not really happen.
>>> 
>>> Since it's still possible for the state machine to transition to
>>> ST_DISCONNECT_PENDING between atomic_check() and atomic_commit(), change
>>> this check to return early if hpd_state is ST_DISCONNECT_PENDING.
>> 
>> Can we really, please, drop the state machine? I had other plans for
>> the next week, but maybe I should just do it, so that by the end of
>> 6.17 cycle we can have a merged, stable and working solution? This
>> topic has been lingering for months without any actual change.
>
>FWIW, I'm currently working on the state machine rework by the middle of next week.
>
>I'm anticipating that the rework itself will take some time to get merged, so didn't want MST to get delayed more by this series.

Yes, that's fine. I really want HPD to be merged before MST. And if I wasn't explicit, the state machine must be gone. Link training should happen from atomic_enable, detect should be reporting whether there is an actual display plugged, etc.  Current code must be dropped.


>
>Thanks,
>
>Jessica Zhang
>
>> 
>>> 
>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>> Signed-off-by: Jessica Zhang <jessica.zhang@oss.qualcomm.com>
>>> ---
>>>   drivers/gpu/drm/msm/dp/dp_display.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>>> index 1d7cda62d5fb..f2820f06f5dc 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>> @@ -1512,7 +1512,7 @@ void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
>>>          }
>>> 
>>>          hpd_state = msm_dp_display->hpd_state;
>>> -       if (hpd_state != ST_DISPLAY_OFF && hpd_state != ST_MAINLINK_READY) {
>>> +       if (hpd_state == ST_DISCONNECT_PENDING) {
>> 
>> 
>> 
>>>                  mutex_unlock(&msm_dp_display->event_mutex);
>>>                  return;
>>>          }
>>> 
>>> --
>>> 2.49.0
>>> 
>> 
>> 
>

With best wishes, 
Dmitry