Message-ID: <CAA8EJppw0jkkHKNucgOg56OD_eH2Afx875m0oRBCtj=EOSJr4g@mail.gmail.com>
Date: Tue, 9 Apr 2024 01:09:09 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Abhinav Kumar <quic_abhinavk@...cinc.com>
Cc: Bjorn Andersson <andersson@...nel.org>, freedreno@...ts.freedesktop.org, 
	Rob Clark <robdclark@...il.com>, Sean Paul <sean@...rly.run>, 
	Marijn Suijten <marijn.suijten@...ainline.org>, David Airlie <airlied@...il.com>, 
	Daniel Vetter <daniel@...ll.ch>, Kuogee Hsieh <quic_khsieh@...cinc.com>, 
	dri-devel@...ts.freedesktop.org, seanpaul@...omium.org, swboyd@...omium.org, 
	quic_jesszhan@...cinc.com, quic_bjorande@...cinc.com, johan@...nel.org, 
	linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle()
 directly for external HPD

On Tue, 9 Apr 2024 at 00:17, Abhinav Kumar <quic_abhinavk@...cinc.com> wrote:
>
>
>
> On 4/8/2024 2:13 PM, Dmitry Baryshkov wrote:
> > On Tue, 9 Apr 2024 at 00:08, Abhinav Kumar <quic_abhinavk@...cinc.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@...cinc.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

Powered by blists - more mailing lists