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@xxxxxxxxxxx>
> > > [..]
> > > > 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