linux-kernel - Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 8 Apr 2024 15:41:54 -0500
From: Bjorn Andersson <andersson@...nel.org>
To: Abhinav Kumar <quic_abhinavk@...cinc.com>
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 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:
- edid/modes are not read before we bring up the link so I always end up
  with 640x480

- 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.

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
> > >