On Tue, 9 Apr 2024 at 00:08, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> 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@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. 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