Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD
From: Abhinav Kumar
Date: Mon Apr 08 2024 - 17:18:08 EST
On 4/8/2024 2:13 PM, Dmitry Baryshkov wrote:
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.
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