[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b7ed299d-4cf7-8829-fd7f-e518aef05fb2@quicinc.com>
Date: Mon, 8 Apr 2024 14:08:47 -0700
From: Abhinav Kumar <quic_abhinavk@...cinc.com>
To: Bjorn Andersson <andersson@...nel.org>
CC: <freedreno@...ts.freedesktop.org>, Rob Clark <robdclark@...il.com>,
"Dmitry Baryshkov" <dmitry.baryshkov@...aro.org>,
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 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.
> - 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
>>>>
Powered by blists - more mailing lists