Messages in this thread | | | Date | Mon, 8 Apr 2024 14:17:29 -0700 | Subject | Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD | From | Abhinav Kumar <> |
| |
On 4/8/2024 2:13 PM, Dmitry Baryshkov wrote: > On Tue, 9 Apr 2024 at 00:08, Abhinav Kumar <quic_abhinavk@quicinc.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@quicinc.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.
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 >>>>>> > > >
| |