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