[PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD

[PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD

[Abhinav Kumar]

From: Kuogee Hsieh <quic_khsieh@quicinc.com>

For HPD events coming from external modules using drm_bridge_hpd_notify(),
the sequence of calls leading to dp_bridge_hpd_notify() is like below:

dp_bridge_hpd_notify+0x18/0x70 [msm]
drm_bridge_connector_detect+0x60/0xe8 [drm_kms_helper]
drm_helper_probe_detect+0x94/0xc0 [drm_kms_helper]
drm_helper_probe_single_connector_modes+0x43c/0x53c [drm_kms_helper]
drm_client_modeset_probe+0x240/0x1114 [drm]
drm_fb_helper_hotplug_event.part.26+0x9c/0xe8 [drm_kms_helper]
drm_fb_helper_hotplug_event+0x24/0x38 [drm_kms_helper]
msm_fbdev_client_hotplug+0x24/0xd4 [msm]
drm_client_dev_hotplug+0xd8/0x148 [drm]
drm_kms_helper_connector_hotplug_event+0x30/0x3c [drm_kms_helper]
drm_bridge_connector_handle_hpd+0x84/0x94 [drm_kms_helper]
drm_bridge_connector_hpd_cb+0xc/0x14 [drm_kms_helper]
drm_bridge_hpd_notify+0x38/0x50 [drm]
drm_aux_hpd_bridge_notify+0x14/0x20 [aux_hpd_bridge]
pmic_glink_altmode_worker+0xec/0x27c [pmic_glink_altmode]
process_scheduled_works+0x17c/0x2cc
worker_thread+0x2ac/0x2d0
kthread+0xfc/0x120

There are three notifications delivered to DP driver for each notification event.

1) From the drm_aux_hpd_bridge_notify() itself as shown above

2) From output_poll_execute() thread which arises due to
drm_helper_probe_single_connector_modes() call of the above stacktrace
as shown in more detail here.

dp_bridge_hpd_notify+0x18/0x70 [msm]
drm_bridge_connector_detect+0x60/0xe8 [drm_kms_helper]
drm_helper_probe_detect+0x94/0xc0 [drm_kms_helper]
drm_helper_probe_single_connector_modes+0x43c/0x53c [drm_kms_helper]
drm_client_modeset_probe+0x240/0x1114 [drm]
drm_fb_helper_hotplug_event.part.26+0x9c/0xe8 [drm_kms_helper]
drm_fb_helper_hotplug_event+0x24/0x38 [drm_kms_helper]
msm_fbdev_client_hotplug+0x24/0xd4 [msm]
drm_client_dev_hotplug+0xd8/0x148 [drm]
drm_kms_helper_hotplug_event+0x30/0x3c [drm_kms_helper]
output_poll_execute+0xe0/0x210 [drm_kms_helper]

3) From the DP driver as the dp_bridge_hpd_notify() callback today triggers
the hpd_event_thread for connect and disconnect events respectively via below stack

dp_bridge_hpd_notify+0x18/0x70 [msm]
drm_bridge_connector_detect+0x60/0xe8 [drm_kms_helper]
drm_helper_probe_detect_ctx+0x98/0x110 [drm_kms_helper]
check_connector_changed+0x4c/0x20c [drm_kms_helper]
drm_helper_hpd_irq_event+0x98/0x120 [drm_kms_helper]
hpd_event_thread+0x478/0x5bc [msm]

dp_bridge_hpd_notify() delivered from output_poll_execute() thread
returns the incorrect HPD status as the MSM DP driver returns the value
of link_ready and not the HPD status currently in the .detect() callback.

And because the HPD event thread has not run yet, this results in two complementary
events.

To address this, fix dp_bridge_hpd_notify() to call dp_hpd_plug_handle/unplug_handle()
directly to return consistent values for the above scenarios.

changes in v3:
	- Fix the commit message as per submitting guidelines.
	- remove extra line added

changes in v2:
	- Fix the commit message to explain the scenario
	- Fix the subject a little as well

Fixes: 542b37efc20e ("drm/msm/dp: Implement hpd_notify()")
Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index d80f89581760..bfb6dfff27e8 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1665,7 +1665,7 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
 		return;
 
 	if (!dp_display->link_ready && status == connector_status_connected)
-		dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
+		dp_hpd_plug_handle(dp, 0);
 	else if (dp_display->link_ready && status == connector_status_disconnected)
-		dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
+		dp_hpd_unplug_handle(dp, 0);
 }
-- 
2.43.2

Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD

[Dmitry Baryshkov]

On Sat, 6 Apr 2024 at 06:16, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> From: Kuogee Hsieh <quic_khsieh@quicinc.com>
>
> For HPD events coming from external modules using drm_bridge_hpd_notify(),
> the sequence of calls leading to dp_bridge_hpd_notify() is like below:
>
> dp_bridge_hpd_notify+0x18/0x70 [msm]
> drm_bridge_connector_detect+0x60/0xe8 [drm_kms_helper]
> drm_helper_probe_detect+0x94/0xc0 [drm_kms_helper]
> drm_helper_probe_single_connector_modes+0x43c/0x53c [drm_kms_helper]
> drm_client_modeset_probe+0x240/0x1114 [drm]
> drm_fb_helper_hotplug_event.part.26+0x9c/0xe8 [drm_kms_helper]
> drm_fb_helper_hotplug_event+0x24/0x38 [drm_kms_helper]
> msm_fbdev_client_hotplug+0x24/0xd4 [msm]
> drm_client_dev_hotplug+0xd8/0x148 [drm]
> drm_kms_helper_connector_hotplug_event+0x30/0x3c [drm_kms_helper]
> drm_bridge_connector_handle_hpd+0x84/0x94 [drm_kms_helper]
> drm_bridge_connector_hpd_cb+0xc/0x14 [drm_kms_helper]
> drm_bridge_hpd_notify+0x38/0x50 [drm]
> drm_aux_hpd_bridge_notify+0x14/0x20 [aux_hpd_bridge]
> pmic_glink_altmode_worker+0xec/0x27c [pmic_glink_altmode]
> process_scheduled_works+0x17c/0x2cc
> worker_thread+0x2ac/0x2d0
> kthread+0xfc/0x120
>
> There are three notifications delivered to DP driver for each notification event.
>
> 1) From the drm_aux_hpd_bridge_notify() itself as shown above
>
> 2) From output_poll_execute() thread which arises due to
> drm_helper_probe_single_connector_modes() call of the above stacktrace
> as shown in more detail here.
>
> dp_bridge_hpd_notify+0x18/0x70 [msm]
> drm_bridge_connector_detect+0x60/0xe8 [drm_kms_helper]
> drm_helper_probe_detect+0x94/0xc0 [drm_kms_helper]
> drm_helper_probe_single_connector_modes+0x43c/0x53c [drm_kms_helper]
> drm_client_modeset_probe+0x240/0x1114 [drm]
> drm_fb_helper_hotplug_event.part.26+0x9c/0xe8 [drm_kms_helper]
> drm_fb_helper_hotplug_event+0x24/0x38 [drm_kms_helper]
> msm_fbdev_client_hotplug+0x24/0xd4 [msm]
> drm_client_dev_hotplug+0xd8/0x148 [drm]
> drm_kms_helper_hotplug_event+0x30/0x3c [drm_kms_helper]
> output_poll_execute+0xe0/0x210 [drm_kms_helper]
>
> 3) From the DP driver as the dp_bridge_hpd_notify() callback today triggers
> the hpd_event_thread for connect and disconnect events respectively via below stack
>
> dp_bridge_hpd_notify+0x18/0x70 [msm]
> drm_bridge_connector_detect+0x60/0xe8 [drm_kms_helper]
> drm_helper_probe_detect_ctx+0x98/0x110 [drm_kms_helper]
> check_connector_changed+0x4c/0x20c [drm_kms_helper]
> drm_helper_hpd_irq_event+0x98/0x120 [drm_kms_helper]
> hpd_event_thread+0x478/0x5bc [msm]
>
> dp_bridge_hpd_notify() delivered from output_poll_execute() thread
> returns the incorrect HPD status as the MSM DP driver returns the value
> of link_ready and not the HPD status currently in the .detect() callback.
>
> And because the HPD event thread has not run yet, this results in two complementary
> events.
>
> To address this, fix dp_bridge_hpd_notify() to call dp_hpd_plug_handle/unplug_handle()
> directly to return consistent values for the above scenarios.
>
> changes in v3:
>         - Fix the commit message as per submitting guidelines.
>         - remove extra line added
>
> changes in v2:
>         - Fix the commit message to explain the scenario
>         - Fix the subject a little as well
>
> Fixes: 542b37efc20e ("drm/msm/dp: Implement hpd_notify()")
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

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

-- 
With best wishes
Dmitry

Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD

[Bjorn Andersson]

On Fri, Apr 05, 2024 at 08:15:47PM -0700, Abhinav Kumar wrote:
> From: Kuogee Hsieh <quic_khsieh@quicinc.com>
[..]
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index d80f89581760..bfb6dfff27e8 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1665,7 +1665,7 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
>  		return;
>  
>  	if (!dp_display->link_ready && status == connector_status_connected)
> -		dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
> +		dp_hpd_plug_handle(dp, 0);

If I read the code correctly, and we get an external connect event
inbetween a previous disconnect and the related disable call, this
should result in a PLUG_INT being injected into the queue still.

Will that not cause the same problem?

Regards,
Bjorn

>  	else if (dp_display->link_ready && status == connector_status_disconnected)
> -		dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
> +		dp_hpd_unplug_handle(dp, 0);
>  }
> -- 
> 2.43.2
> 

Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD

[Abhinav Kumar]

On 4/7/2024 11:48 AM, Bjorn Andersson wrote:
> On Fri, Apr 05, 2024 at 08:15:47PM -0700, Abhinav Kumar wrote:
>> From: Kuogee Hsieh <quic_khsieh@quicinc.com>
> [..]
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index d80f89581760..bfb6dfff27e8 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -1665,7 +1665,7 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
>>   		return;
>>   
>>   	if (!dp_display->link_ready && status == connector_status_connected)
>> -		dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
>> +		dp_hpd_plug_handle(dp, 0);
> 
> If I read the code correctly, and we get an external connect event
> inbetween a previous disconnect and the related disable call, this
> should result in a PLUG_INT being injected into the queue still.
> 
> Will that not cause the same problem?
> 
> Regards,
> Bjorn
>

Yes, your observation is correct and I had asked the same question to 
kuogee before taking over this change and posting.

We will have to handle that case separately. I don't have a good 
solution yet for it without requiring further rework or we drop the 
below snippet.

         if (state == ST_DISCONNECT_PENDING) {
                 /* wait until ST_DISCONNECTED */
                 dp_add_event(dp, EV_HPD_PLUG_INT, 0, 1); /* delay = 1 */
                 mutex_unlock(&dp->event_mutex);
                 return 0;
         }

I will need sometime to address that use-case as I need to see if we can 
handle that better and then drop the the DISCONNECT_PENDING state to 
address this fully. But it needs more testing.

But, we will need this patch anyway because without this we will not be 
able to fix even the most regular and commonly seen case of basic 
connect/disconnect receiving complementary events.


>>   	else if (dp_display->link_ready && status == connector_status_disconnected)
>> -		dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
>> +		dp_hpd_unplug_handle(dp, 0);
>>   }
>> -- 
>> 2.43.2
>>

Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD

[Bjorn Andersson]

On Mon, Apr 08, 2024 at 12:43:34PM -0700, Abhinav Kumar wrote:
> 
> 
> On 4/7/2024 11:48 AM, Bjorn Andersson wrote:
> > On Fri, Apr 05, 2024 at 08:15:47PM -0700, Abhinav Kumar wrote:
> > > From: Kuogee Hsieh <quic_khsieh@quicinc.com>
> > [..]
> > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > > index d80f89581760..bfb6dfff27e8 100644
> > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > @@ -1665,7 +1665,7 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
> > >   		return;
> > >   	if (!dp_display->link_ready && status == connector_status_connected)
> > > -		dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
> > > +		dp_hpd_plug_handle(dp, 0);
> > 
> > If I read the code correctly, and we get an external connect event
> > inbetween a previous disconnect and the related disable call, this
> > should result in a PLUG_INT being injected into the queue still.
> > 
> > Will that not cause the same problem?
> > 
> > Regards,
> > Bjorn
> > 
> 
> Yes, your observation is correct and I had asked the same question to kuogee
> before taking over this change and posting.
> 
> We will have to handle that case separately. I don't have a good solution
> yet for it without requiring further rework or we drop the below snippet.
> 
>         if (state == ST_DISCONNECT_PENDING) {
>                 /* wait until ST_DISCONNECTED */
>                 dp_add_event(dp, EV_HPD_PLUG_INT, 0, 1); /* delay = 1 */
>                 mutex_unlock(&dp->event_mutex);
>                 return 0;
>         }
> 
> I will need sometime to address that use-case as I need to see if we can
> handle that better and then drop the the DISCONNECT_PENDING state to address
> this fully. But it needs more testing.
> 
> But, we will need this patch anyway because without this we will not be able
> to fix even the most regular and commonly seen case of basic
> connect/disconnect receiving complementary events.
> 

I did some more testing on this patch. Connecting and disconnecting the
cable while in fbcon works reliably, except for:
- edid/modes are not read before we bring up the link so I always end up
  with 640x480

- if I run modetest -s <id>:<mode> the link is brought up with the new
  resolution and I get my test image on the screen.
  But as we're shutting down the link for the resolution chance I end up
  in dp_bridge_hpd_notify() with link_ready && state = disconnected.
  This triggers an unplug which hangs on the event_mutex, such that as
  soon as I get the test image, the state machine enters
  DISCONNECT_PENDING. This is immediately followed by another
  !link_ready && status = connected, which attempts to perform the plug
  operation, but as we're in DISCONNECT_PENDING this is posted on the
  event queue. From there I get a log entry from my PLUG_INT, every
  100ms stating that we're still in DISCONNECT_PENDING. If I exit
  modetest the screen goes black, and no new mode can be selected,
  because we're in DISCONNECT_PENDING. The only way out is to disconnect
  the cable to complete the DISCONNECT_PENDING.

Regards,
Bjorn

> 
> > >   	else if (dp_display->link_ready && status == connector_status_disconnected)
> > > -		dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
> > > +		dp_hpd_unplug_handle(dp, 0);
> > >   }
> > > -- 
> > > 2.43.2
> > > 

Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD

[Abhinav Kumar]

On 4/8/2024 1:41 PM, Bjorn Andersson wrote:
> On Mon, Apr 08, 2024 at 12:43:34PM -0700, Abhinav Kumar wrote:
>>
>>
>> On 4/7/2024 11:48 AM, Bjorn Andersson wrote:
>>> On Fri, Apr 05, 2024 at 08:15:47PM -0700, Abhinav Kumar wrote:
>>>> From: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>> [..]
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> index d80f89581760..bfb6dfff27e8 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> @@ -1665,7 +1665,7 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
>>>>    		return;
>>>>    	if (!dp_display->link_ready && status == connector_status_connected)
>>>> -		dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
>>>> +		dp_hpd_plug_handle(dp, 0);
>>>
>>> If I read the code correctly, and we get an external connect event
>>> inbetween a previous disconnect and the related disable call, this
>>> should result in a PLUG_INT being injected into the queue still.
>>>
>>> Will that not cause the same problem?
>>>
>>> Regards,
>>> Bjorn
>>>
>>
>> Yes, your observation is correct and I had asked the same question to kuogee
>> before taking over this change and posting.
>>
>> We will have to handle that case separately. I don't have a good solution
>> yet for it without requiring further rework or we drop the below snippet.
>>
>>          if (state == ST_DISCONNECT_PENDING) {
>>                  /* wait until ST_DISCONNECTED */
>>                  dp_add_event(dp, EV_HPD_PLUG_INT, 0, 1); /* delay = 1 */
>>                  mutex_unlock(&dp->event_mutex);
>>                  return 0;
>>          }
>>
>> I will need sometime to address that use-case as I need to see if we can
>> handle that better and then drop the the DISCONNECT_PENDING state to address
>> this fully. But it needs more testing.
>>
>> But, we will need this patch anyway because without this we will not be able
>> to fix even the most regular and commonly seen case of basic
>> connect/disconnect receiving complementary events.
>>
> 
> I did some more testing on this patch. Connecting and disconnecting the
> cable while in fbcon works reliably, except for:

Thanks for the tests !

> - edid/modes are not read before we bring up the link so I always end up
>    with 640x480
> 

hmmm, I wonder why this should be affected due to this patch. We always 
read the EDID during hpd_connect() and the selected resolution will be 
programmed with the modeset. We will retry this with our x1e80100 and see.

> - if I run modetest -s <id>:<mode> the link is brought up with the new
>    resolution and I get my test image on the screen.
>    But as we're shutting down the link for the resolution chance I end up
>    in dp_bridge_hpd_notify() with link_ready && state = disconnected.
>    This triggers an unplug which hangs on the event_mutex, such that as
>    soon as I get the test image, the state machine enters
>    DISCONNECT_PENDING. This is immediately followed by another
>    !link_ready && status = connected, which attempts to perform the plug
>    operation, but as we're in DISCONNECT_PENDING this is posted on the
>    event queue. From there I get a log entry from my PLUG_INT, every
>    100ms stating that we're still in DISCONNECT_PENDING. If I exit
>    modetest the screen goes black, and no new mode can be selected,
>    because we're in DISCONNECT_PENDING. The only way out is to disconnect
>    the cable to complete the DISCONNECT_PENDING.
> 

I am going to run this test-case and see what we can do.

> Regards,
> Bjorn
> 
>>
>>>>    	else if (dp_display->link_ready && status == connector_status_disconnected)
>>>> -		dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
>>>> +		dp_hpd_unplug_handle(dp, 0);
>>>>    }
>>>> -- 
>>>> 2.43.2
>>>>

Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD

[Dmitry Baryshkov]

On Tue, 9 Apr 2024 at 00:08, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 4/8/2024 1:41 PM, Bjorn Andersson wrote:
> > On Mon, Apr 08, 2024 at 12:43:34PM -0700, Abhinav Kumar wrote:
> >>
> >>
> >> On 4/7/2024 11:48 AM, Bjorn Andersson wrote:
> >>> On Fri, Apr 05, 2024 at 08:15:47PM -0700, Abhinav Kumar wrote:
> >>>> From: Kuogee Hsieh <quic_khsieh@quicinc.com>
> >>> [..]
> >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> index d80f89581760..bfb6dfff27e8 100644
> >>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> @@ -1665,7 +1665,7 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
> >>>>                    return;
> >>>>            if (!dp_display->link_ready && status == connector_status_connected)
> >>>> -          dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
> >>>> +          dp_hpd_plug_handle(dp, 0);
> >>>
> >>> If I read the code correctly, and we get an external connect event
> >>> inbetween a previous disconnect and the related disable call, this
> >>> should result in a PLUG_INT being injected into the queue still.
> >>>
> >>> Will that not cause the same problem?
> >>>
> >>> Regards,
> >>> Bjorn
> >>>
> >>
> >> Yes, your observation is correct and I had asked the same question to kuogee
> >> before taking over this change and posting.
> >>
> >> We will have to handle that case separately. I don't have a good solution
> >> yet for it without requiring further rework or we drop the below snippet.
> >>
> >>          if (state == ST_DISCONNECT_PENDING) {
> >>                  /* wait until ST_DISCONNECTED */
> >>                  dp_add_event(dp, EV_HPD_PLUG_INT, 0, 1); /* delay = 1 */
> >>                  mutex_unlock(&dp->event_mutex);
> >>                  return 0;
> >>          }
> >>
> >> I will need sometime to address that use-case as I need to see if we can
> >> handle that better and then drop the the DISCONNECT_PENDING state to address
> >> this fully. But it needs more testing.
> >>
> >> But, we will need this patch anyway because without this we will not be able
> >> to fix even the most regular and commonly seen case of basic
> >> connect/disconnect receiving complementary events.
> >>
> >
> > I did some more testing on this patch. Connecting and disconnecting the
> > cable while in fbcon works reliably, except for:
>
> Thanks for the tests !
>
> > - edid/modes are not read before we bring up the link so I always end up
> >    with 640x480
> >
>
> hmmm, I wonder why this should be affected due to this patch. We always
> read the EDID during hpd_connect() and the selected resolution will be
> programmed with the modeset. We will retry this with our x1e80100 and see.

BTW, why is EDID read during HPD handling? I always supposed that it
can be read much later, when the DRM framework calls the get_modes /
get_edid callbacks.

>
> > - if I run modetest -s <id>:<mode> the link is brought up with the new
> >    resolution and I get my test image on the screen.
> >    But as we're shutting down the link for the resolution chance I end up
> >    in dp_bridge_hpd_notify() with link_ready && state = disconnected.
> >    This triggers an unplug which hangs on the event_mutex, such that as
> >    soon as I get the test image, the state machine enters
> >    DISCONNECT_PENDING. This is immediately followed by another
> >    !link_ready && status = connected, which attempts to perform the plug
> >    operation, but as we're in DISCONNECT_PENDING this is posted on the
> >    event queue. From there I get a log entry from my PLUG_INT, every
> >    100ms stating that we're still in DISCONNECT_PENDING. If I exit
> >    modetest the screen goes black, and no new mode can be selected,
> >    because we're in DISCONNECT_PENDING. The only way out is to disconnect
> >    the cable to complete the DISCONNECT_PENDING.
> >
>
> I am going to run this test-case and see what we can do.
>
> > Regards,
> > Bjorn
> >
> >>
> >>>>            else if (dp_display->link_ready && status == connector_status_disconnected)
> >>>> -          dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
> >>>> +          dp_hpd_unplug_handle(dp, 0);
> >>>>    }
> >>>> --
> >>>> 2.43.2
> >>>>



-- 
With best wishes
Dmitry

Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD

[Abhinav Kumar]

On 4/8/2024 2:13 PM, Dmitry Baryshkov wrote:
> On Tue, 9 Apr 2024 at 00:08, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 4/8/2024 1:41 PM, Bjorn Andersson wrote:
>>> On Mon, Apr 08, 2024 at 12:43:34PM -0700, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 4/7/2024 11:48 AM, Bjorn Andersson wrote:
>>>>> On Fri, Apr 05, 2024 at 08:15:47PM -0700, Abhinav Kumar wrote:
>>>>>> From: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>>> [..]
>>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>>>>>> index d80f89581760..bfb6dfff27e8 100644
>>>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>>>> @@ -1665,7 +1665,7 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
>>>>>>                     return;
>>>>>>             if (!dp_display->link_ready && status == connector_status_connected)
>>>>>> -          dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
>>>>>> +          dp_hpd_plug_handle(dp, 0);
>>>>>
>>>>> If I read the code correctly, and we get an external connect event
>>>>> inbetween a previous disconnect and the related disable call, this
>>>>> should result in a PLUG_INT being injected into the queue still.
>>>>>
>>>>> Will that not cause the same problem?
>>>>>
>>>>> Regards,
>>>>> Bjorn
>>>>>
>>>>
>>>> Yes, your observation is correct and I had asked the same question to kuogee
>>>> before taking over this change and posting.
>>>>
>>>> We will have to handle that case separately. I don't have a good solution
>>>> yet for it without requiring further rework or we drop the below snippet.
>>>>
>>>>           if (state == ST_DISCONNECT_PENDING) {
>>>>                   /* wait until ST_DISCONNECTED */
>>>>                   dp_add_event(dp, EV_HPD_PLUG_INT, 0, 1); /* delay = 1 */
>>>>                   mutex_unlock(&dp->event_mutex);
>>>>                   return 0;
>>>>           }
>>>>
>>>> I will need sometime to address that use-case as I need to see if we can
>>>> handle that better and then drop the the DISCONNECT_PENDING state to address
>>>> this fully. But it needs more testing.
>>>>
>>>> But, we will need this patch anyway because without this we will not be able
>>>> to fix even the most regular and commonly seen case of basic
>>>> connect/disconnect receiving complementary events.
>>>>
>>>
>>> I did some more testing on this patch. Connecting and disconnecting the
>>> cable while in fbcon works reliably, except for:
>>
>> Thanks for the tests !
>>
>>> - edid/modes are not read before we bring up the link so I always end up
>>>     with 640x480
>>>
>>
>> hmmm, I wonder why this should be affected due to this patch. We always
>> read the EDID during hpd_connect() and the selected resolution will be
>> programmed with the modeset. We will retry this with our x1e80100 and see.
> 
> BTW, why is EDID read during HPD handling? I always supposed that it
> can be read much later, when the DRM framework calls the get_modes /
> get_edid callbacks.
> 

Well, dp_panel_read_sink_caps() is in dp_display_process_hpd_high() 
currently. We read the edid there.

get_modes(), parses the EDID and adds the modes using drm_add_edid_modes().

>>
>>> - if I run modetest -s <id>:<mode> the link is brought up with the new
>>>     resolution and I get my test image on the screen.
>>>     But as we're shutting down the link for the resolution chance I end up
>>>     in dp_bridge_hpd_notify() with link_ready && state = disconnected.
>>>     This triggers an unplug which hangs on the event_mutex, such that as
>>>     soon as I get the test image, the state machine enters
>>>     DISCONNECT_PENDING. This is immediately followed by another
>>>     !link_ready && status = connected, which attempts to perform the plug
>>>     operation, but as we're in DISCONNECT_PENDING this is posted on the
>>>     event queue. From there I get a log entry from my PLUG_INT, every
>>>     100ms stating that we're still in DISCONNECT_PENDING. If I exit
>>>     modetest the screen goes black, and no new mode can be selected,
>>>     because we're in DISCONNECT_PENDING. The only way out is to disconnect
>>>     the cable to complete the DISCONNECT_PENDING.
>>>
>>
>> I am going to run this test-case and see what we can do.
>>
>>> Regards,
>>> Bjorn
>>>
>>>>
>>>>>>             else if (dp_display->link_ready && status == connector_status_disconnected)
>>>>>> -          dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
>>>>>> +          dp_hpd_unplug_handle(dp, 0);
>>>>>>     }
>>>>>> --
>>>>>> 2.43.2
>>>>>>
> 
> 
> 

Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD

[Dmitry Baryshkov]

On Tue, 9 Apr 2024 at 00:17, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 4/8/2024 2:13 PM, Dmitry Baryshkov wrote:
> > On Tue, 9 Apr 2024 at 00:08, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 4/8/2024 1:41 PM, Bjorn Andersson wrote:
> >>> On Mon, Apr 08, 2024 at 12:43:34PM -0700, Abhinav Kumar wrote:
> >>>>
> >>>>
> >>>> On 4/7/2024 11:48 AM, Bjorn Andersson wrote:
> >>>>> On Fri, Apr 05, 2024 at 08:15:47PM -0700, Abhinav Kumar wrote:
> >>>>>> From: Kuogee Hsieh <quic_khsieh@quicinc.com>
> >>>>> [..]
> >>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> >>>>>> index d80f89581760..bfb6dfff27e8 100644
> >>>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >>>>>> @@ -1665,7 +1665,7 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
> >>>>>>                     return;
> >>>>>>             if (!dp_display->link_ready && status == connector_status_connected)
> >>>>>> -          dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
> >>>>>> +          dp_hpd_plug_handle(dp, 0);
> >>>>>
> >>>>> If I read the code correctly, and we get an external connect event
> >>>>> inbetween a previous disconnect and the related disable call, this
> >>>>> should result in a PLUG_INT being injected into the queue still.
> >>>>>
> >>>>> Will that not cause the same problem?
> >>>>>
> >>>>> Regards,
> >>>>> Bjorn
> >>>>>
> >>>>
> >>>> Yes, your observation is correct and I had asked the same question to kuogee
> >>>> before taking over this change and posting.
> >>>>
> >>>> We will have to handle that case separately. I don't have a good solution
> >>>> yet for it without requiring further rework or we drop the below snippet.
> >>>>
> >>>>           if (state == ST_DISCONNECT_PENDING) {
> >>>>                   /* wait until ST_DISCONNECTED */
> >>>>                   dp_add_event(dp, EV_HPD_PLUG_INT, 0, 1); /* delay = 1 */
> >>>>                   mutex_unlock(&dp->event_mutex);
> >>>>                   return 0;
> >>>>           }
> >>>>
> >>>> I will need sometime to address that use-case as I need to see if we can
> >>>> handle that better and then drop the the DISCONNECT_PENDING state to address
> >>>> this fully. But it needs more testing.
> >>>>
> >>>> But, we will need this patch anyway because without this we will not be able
> >>>> to fix even the most regular and commonly seen case of basic
> >>>> connect/disconnect receiving complementary events.
> >>>>
> >>>
> >>> I did some more testing on this patch. Connecting and disconnecting the
> >>> cable while in fbcon works reliably, except for:
> >>
> >> Thanks for the tests !
> >>
> >>> - edid/modes are not read before we bring up the link so I always end up
> >>>     with 640x480
> >>>
> >>
> >> hmmm, I wonder why this should be affected due to this patch. We always
> >> read the EDID during hpd_connect() and the selected resolution will be
> >> programmed with the modeset. We will retry this with our x1e80100 and see.
> >
> > BTW, why is EDID read during HPD handling? I always supposed that it
> > can be read much later, when the DRM framework calls the get_modes /
> > get_edid callbacks.
> >
>
> Well, dp_panel_read_sink_caps() is in dp_display_process_hpd_high()
> currently. We read the edid there.

My question was, why is it done this way? Can it be dropped? There is
no need to store EDID in the driver data,  is it?

>
> get_modes(), parses the EDID and adds the modes using drm_add_edid_modes().
>
> >>
> >>> - if I run modetest -s <id>:<mode> the link is brought up with the new
> >>>     resolution and I get my test image on the screen.
> >>>     But as we're shutting down the link for the resolution chance I end up
> >>>     in dp_bridge_hpd_notify() with link_ready && state = disconnected.
> >>>     This triggers an unplug which hangs on the event_mutex, such that as
> >>>     soon as I get the test image, the state machine enters
> >>>     DISCONNECT_PENDING. This is immediately followed by another
> >>>     !link_ready && status = connected, which attempts to perform the plug
> >>>     operation, but as we're in DISCONNECT_PENDING this is posted on the
> >>>     event queue. From there I get a log entry from my PLUG_INT, every
> >>>     100ms stating that we're still in DISCONNECT_PENDING. If I exit
> >>>     modetest the screen goes black, and no new mode can be selected,
> >>>     because we're in DISCONNECT_PENDING. The only way out is to disconnect
> >>>     the cable to complete the DISCONNECT_PENDING.
> >>>
> >>
> >> I am going to run this test-case and see what we can do.
> >>
> >>> Regards,
> >>> Bjorn
> >>>
> >>>>
> >>>>>>             else if (dp_display->link_ready && status == connector_status_disconnected)
> >>>>>> -          dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
> >>>>>> +          dp_hpd_unplug_handle(dp, 0);
> >>>>>>     }
> >>>>>> --
> >>>>>> 2.43.2
> >>>>>>
> >
> >
> >



-- 
With best wishes
Dmitry

Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD

[Dmitry Baryshkov]

On Mon, 8 Apr 2024 at 22:43, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 4/7/2024 11:48 AM, Bjorn Andersson wrote:
> > On Fri, Apr 05, 2024 at 08:15:47PM -0700, Abhinav Kumar wrote:
> >> From: Kuogee Hsieh <quic_khsieh@quicinc.com>
> > [..]
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> >> index d80f89581760..bfb6dfff27e8 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >> @@ -1665,7 +1665,7 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
> >>              return;
> >>
> >>      if (!dp_display->link_ready && status == connector_status_connected)
> >> -            dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
> >> +            dp_hpd_plug_handle(dp, 0);
> >
> > If I read the code correctly, and we get an external connect event
> > inbetween a previous disconnect and the related disable call, this
> > should result in a PLUG_INT being injected into the queue still.
> >
> > Will that not cause the same problem?
> >
> > Regards,
> > Bjorn
> >
>
> Yes, your observation is correct and I had asked the same question to
> kuogee before taking over this change and posting.

Should it then have the Co-developed-by trailers?

> We will have to handle that case separately. I don't have a good
> solution yet for it without requiring further rework or we drop the
> below snippet.
>
>          if (state == ST_DISCONNECT_PENDING) {
>                  /* wait until ST_DISCONNECTED */
>                  dp_add_event(dp, EV_HPD_PLUG_INT, 0, 1); /* delay = 1 */
>                  mutex_unlock(&dp->event_mutex);
>                  return 0;
>          }
>
> I will need sometime to address that use-case as I need to see if we can
> handle that better and then drop the the DISCONNECT_PENDING state to
> address this fully. But it needs more testing.
>
> But, we will need this patch anyway because without this we will not be
> able to fix even the most regular and commonly seen case of basic
> connect/disconnect receiving complementary events.

Hmm, no. We need to drop the HPD state machine, not to patch it. Once
the driver has proper detect() callback, there will be no
complementary events. That is a proper way to fix the code, not these
kinds of band-aids patches.

> >>      else if (dp_display->link_ready && status == connector_status_disconnected)
> >> -            dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
> >> +            dp_hpd_unplug_handle(dp, 0);
> >>   }
> >> --
> >> 2.43.2
> >>



-- 
With best wishes
Dmitry

Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD

[Abhinav Kumar]

On 4/8/2024 2:12 PM, Dmitry Baryshkov wrote:
> On Mon, 8 Apr 2024 at 22:43, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 4/7/2024 11:48 AM, Bjorn Andersson wrote:
>>> On Fri, Apr 05, 2024 at 08:15:47PM -0700, Abhinav Kumar wrote:
>>>> From: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>> [..]
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> index d80f89581760..bfb6dfff27e8 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> @@ -1665,7 +1665,7 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
>>>>               return;
>>>>
>>>>       if (!dp_display->link_ready && status == connector_status_connected)
>>>> -            dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
>>>> +            dp_hpd_plug_handle(dp, 0);
>>>
>>> If I read the code correctly, and we get an external connect event
>>> inbetween a previous disconnect and the related disable call, this
>>> should result in a PLUG_INT being injected into the queue still.
>>>
>>> Will that not cause the same problem?
>>>
>>> Regards,
>>> Bjorn
>>>
>>
>> Yes, your observation is correct and I had asked the same question to
>> kuogee before taking over this change and posting.
> 
> Should it then have the Co-developed-by trailers?
> 

hmmm, perhaps but that didnt result in any code change between v2 and 
v3, so I didnt add it.

>> We will have to handle that case separately. I don't have a good
>> solution yet for it without requiring further rework or we drop the
>> below snippet.
>>
>>           if (state == ST_DISCONNECT_PENDING) {
>>                   /* wait until ST_DISCONNECTED */
>>                   dp_add_event(dp, EV_HPD_PLUG_INT, 0, 1); /* delay = 1 */
>>                   mutex_unlock(&dp->event_mutex);
>>                   return 0;
>>           }
>>
>> I will need sometime to address that use-case as I need to see if we can
>> handle that better and then drop the the DISCONNECT_PENDING state to
>> address this fully. But it needs more testing.
>>
>> But, we will need this patch anyway because without this we will not be
>> able to fix even the most regular and commonly seen case of basic
>> connect/disconnect receiving complementary events.
> 
> Hmm, no. We need to drop the HPD state machine, not to patch it. Once
> the driver has proper detect() callback, there will be no
> complementary events. That is a proper way to fix the code, not these
> kinds of band-aids patches.
> 

I had discussed this part too :)

I totally agree we should fix .detect()'s behavior to just match cable 
connect/disconnect and not link_ready status.

But that alone would not have fixed this issue. If HPD thread does not 
get scheduled and plug_handle() was not executed, .detect() would have 
still returned old status as we will update the cable status only in 
plug_handle() / unplug_handle() to have a common API between internal 
and external hpd execution.

So we need to do both, make .detect() return correct status AND drop hpd 
event thread processing.

But, dropping the hpd event thread processing alone was fixing the 
complimentary events issue. So kuogee had only this part in this patch.


>>>>       else if (dp_display->link_ready && status == connector_status_disconnected)
>>>> -            dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
>>>> +            dp_hpd_unplug_handle(dp, 0);
>>>>    }
>>>> --
>>>> 2.43.2
>>>>
> 
> 
> 

Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD

[Dmitry Baryshkov]

On Tue, 9 Apr 2024 at 00:23, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 4/8/2024 2:12 PM, Dmitry Baryshkov wrote:
> > On Mon, 8 Apr 2024 at 22:43, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 4/7/2024 11:48 AM, Bjorn Andersson wrote:
> >>> On Fri, Apr 05, 2024 at 08:15:47PM -0700, Abhinav Kumar wrote:
> >>>> From: Kuogee Hsieh <quic_khsieh@quicinc.com>
> >>> [..]
> >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> index d80f89581760..bfb6dfff27e8 100644
> >>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> @@ -1665,7 +1665,7 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
> >>>>               return;
> >>>>
> >>>>       if (!dp_display->link_ready && status == connector_status_connected)
> >>>> -            dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
> >>>> +            dp_hpd_plug_handle(dp, 0);
> >>>
> >>> If I read the code correctly, and we get an external connect event
> >>> inbetween a previous disconnect and the related disable call, this
> >>> should result in a PLUG_INT being injected into the queue still.
> >>>
> >>> Will that not cause the same problem?
> >>>
> >>> Regards,
> >>> Bjorn
> >>>
> >>
> >> Yes, your observation is correct and I had asked the same question to
> >> kuogee before taking over this change and posting.
> >
> > Should it then have the Co-developed-by trailers?
> >
>
> hmmm, perhaps but that didnt result in any code change between v2 and
> v3, so I didnt add it.

So who authored v0 of it? From your text I had an impression that it
was Kuogee. Please excuse me if I was wrong.

>
> >> We will have to handle that case separately. I don't have a good
> >> solution yet for it without requiring further rework or we drop the
> >> below snippet.
> >>
> >>           if (state == ST_DISCONNECT_PENDING) {
> >>                   /* wait until ST_DISCONNECTED */
> >>                   dp_add_event(dp, EV_HPD_PLUG_INT, 0, 1); /* delay = 1 */
> >>                   mutex_unlock(&dp->event_mutex);
> >>                   return 0;
> >>           }
> >>
> >> I will need sometime to address that use-case as I need to see if we can
> >> handle that better and then drop the the DISCONNECT_PENDING state to
> >> address this fully. But it needs more testing.
> >>
> >> But, we will need this patch anyway because without this we will not be
> >> able to fix even the most regular and commonly seen case of basic
> >> connect/disconnect receiving complementary events.
> >
> > Hmm, no. We need to drop the HPD state machine, not to patch it. Once
> > the driver has proper detect() callback, there will be no
> > complementary events. That is a proper way to fix the code, not these
> > kinds of band-aids patches.
> >
>
> I had discussed this part too :)
>
> I totally agree we should fix .detect()'s behavior to just match cable
> connect/disconnect and not link_ready status.
>
> But that alone would not have fixed this issue. If HPD thread does not
> get scheduled and plug_handle() was not executed, .detect() would have
> still returned old status as we will update the cable status only in
> plug_handle() / unplug_handle() to have a common API between internal
> and external hpd execution.

I think there should be just hpd_notify, which if the HPD is up,
attempts to read the DPCD. No need for separate plug/unplug_handle.
The detect() can be as simple as !drm_dp_is_branch() || sink_count != 0.

>
> So we need to do both, make .detect() return correct status AND drop hpd
> event thread processing.
>
> But, dropping the hpd event thread processing alone was fixing the
> complimentary events issue. So kuogee had only this part in this patch.

I'd prefer to wait for the final patchset then. Which has the HPD
thread completely removed.

>
>
> >>>>       else if (dp_display->link_ready && status == connector_status_disconnected)
> >>>> -            dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
> >>>> +            dp_hpd_unplug_handle(dp, 0);
> >>>>    }
> >>>> --
> >>>> 2.43.2
> >>>>
> >
> >
> >



--
With best wishes
Dmitry

Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD

[Bjorn Andersson]

On Tue, Apr 09, 2024 at 01:07:57AM +0300, Dmitry Baryshkov wrote:
> On Tue, 9 Apr 2024 at 00:23, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> > On 4/8/2024 2:12 PM, Dmitry Baryshkov wrote:
> > > On Mon, 8 Apr 2024 at 22:43, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> > >> On 4/7/2024 11:48 AM, Bjorn Andersson wrote:
> > >>> On Fri, Apr 05, 2024 at 08:15:47PM -0700, Abhinav Kumar wrote:
> > >>>> From: Kuogee Hsieh <quic_khsieh@quicinc.com>
> > >>> [..]
> > >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
[..]
> > >>
> > >> I will need sometime to address that use-case as I need to see if we can
> > >> handle that better and then drop the the DISCONNECT_PENDING state to
> > >> address this fully. But it needs more testing.
> > >>
> > >> But, we will need this patch anyway because without this we will not be
> > >> able to fix even the most regular and commonly seen case of basic
> > >> connect/disconnect receiving complementary events.
> > >
> > > Hmm, no. We need to drop the HPD state machine, not to patch it. Once
> > > the driver has proper detect() callback, there will be no
> > > complementary events. That is a proper way to fix the code, not these
> > > kinds of band-aids patches.
> > >
> >
> > I had discussed this part too :)
> >
> > I totally agree we should fix .detect()'s behavior to just match cable
> > connect/disconnect and not link_ready status.
> >
> > But that alone would not have fixed this issue. If HPD thread does not
> > get scheduled and plug_handle() was not executed, .detect() would have
> > still returned old status as we will update the cable status only in
> > plug_handle() / unplug_handle() to have a common API between internal
> > and external hpd execution.
> 
> I think there should be just hpd_notify, which if the HPD is up,
> attempts to read the DPCD. No need for separate plug/unplug_handle.
> The detect() can be as simple as !drm_dp_is_branch() || sink_count != 0.
> 

What is detect() supposed to return in the event that we have external
HPD handler? The link state? While the external HPD bridge would return
the HPD state?

If a driver only drives the link inbetween atomic_enable() and
atomic_disable() will the "connected state" then ever be reported as
"connected"? (I'm sure I'm still missing pieces of this puzzle).

Regards,
Bjorn

Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD

[Dmitry Baryshkov]

On Mon, Apr 08, 2024 at 09:33:01PM -0500, Bjorn Andersson wrote:
> On Tue, Apr 09, 2024 at 01:07:57AM +0300, Dmitry Baryshkov wrote:
> > On Tue, 9 Apr 2024 at 00:23, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> > > On 4/8/2024 2:12 PM, Dmitry Baryshkov wrote:
> > > > On Mon, 8 Apr 2024 at 22:43, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> > > >> On 4/7/2024 11:48 AM, Bjorn Andersson wrote:
> > > >>> On Fri, Apr 05, 2024 at 08:15:47PM -0700, Abhinav Kumar wrote:
> > > >>>> From: Kuogee Hsieh <quic_khsieh@quicinc.com>
> > > >>> [..]
> > > >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> [..]
> > > >>
> > > >> I will need sometime to address that use-case as I need to see if we can
> > > >> handle that better and then drop the the DISCONNECT_PENDING state to
> > > >> address this fully. But it needs more testing.
> > > >>
> > > >> But, we will need this patch anyway because without this we will not be
> > > >> able to fix even the most regular and commonly seen case of basic
> > > >> connect/disconnect receiving complementary events.
> > > >
> > > > Hmm, no. We need to drop the HPD state machine, not to patch it. Once
> > > > the driver has proper detect() callback, there will be no
> > > > complementary events. That is a proper way to fix the code, not these
> > > > kinds of band-aids patches.
> > > >
> > >
> > > I had discussed this part too :)
> > >
> > > I totally agree we should fix .detect()'s behavior to just match cable
> > > connect/disconnect and not link_ready status.
> > >
> > > But that alone would not have fixed this issue. If HPD thread does not
> > > get scheduled and plug_handle() was not executed, .detect() would have
> > > still returned old status as we will update the cable status only in
> > > plug_handle() / unplug_handle() to have a common API between internal
> > > and external hpd execution.
> > 
> > I think there should be just hpd_notify, which if the HPD is up,
> > attempts to read the DPCD. No need for separate plug/unplug_handle.
> > The detect() can be as simple as !drm_dp_is_branch() || sink_count != 0.
> > 
> 
> What is detect() supposed to return in the event that we have external
> HPD handler? The link state? While the external HPD bridge would return
> the HPD state?

It should return the same: there is a sensible display attached. Other
drivers (and drm/msm/dp internally) use !branch || (sink_count > 0).

> If a driver only drives the link inbetween atomic_enable() and
> atomic_disable() will the "connected state" then ever be reported as
> "connected"? (I'm sure I'm still missing pieces of this puzzle).

I don't probably get the question. Nothing stops the driver from
accessing the AUX bus outside of the atomic_enable/disable() brackets.

> 
> Regards,
> Bjorn

-- 
With best wishes
Dmitry