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?
> 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
