linux-kernel - Re: [PATCH] drm/bridge: parade-ps8640: Wait for HPD when doing an AUX transfer

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]

Message-ID: <CAD=FV=WDb7y-9dRgb0D=VxVB6EjUkcOJ+9D0Mp0-vw6wuKUHEg@mail.gmail.com>
Date: Fri, 22 Dec 2023 07:33:48 -0800
From: Doug Anderson <dianders@...omium.org>
To: Pin-yen Lin <treapking@...omium.org>
Cc: dri-devel@...ts.freedesktop.org, hsinyi@...omium.org, 
	Andrzej Hajda <andrzej.hajda@...el.com>, Daniel Vetter <daniel@...ll.ch>, 
	David Airlie <airlied@...il.com>, Dmitry Baryshkov <dmitry.baryshkov@...aro.org>, 
	Jernej Skrabec <jernej.skrabec@...il.com>, Jonas Karlman <jonas@...boo.se>, 
	Laurent Pinchart <Laurent.pinchart@...asonboard.com>, 
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, 
	Neil Armstrong <neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>, 
	Thomas Zimmermann <tzimmermann@...e.de>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/bridge: parade-ps8640: Wait for HPD when doing an AUX transfer

Hi,

On Fri, Dec 22, 2023 at 2:29 AM Pin-yen Lin <treapking@...omium.org> wrote:
>
> Hi Douglas,
>
> On Fri, Dec 22, 2023 at 5:56 AM Douglas Anderson <dianders@...omium.org> wrote:
> >
> > Unlike what is claimed in commit f5aa7d46b0ee ("drm/bridge:
> > parade-ps8640: Provide wait_hpd_asserted() in struct drm_dp_aux"), if
> > someone manually tries to do an AUX transfer (like via `i2cdump ${bus}
> > 0x50 i`) while the panel is off we don't just get a simple transfer
> > error. Instead, the whole ps8640 gets thrown for a loop and goes into
> > a bad state.
> >
> > Let's put the function to wait for the HPD (and the magical 50 ms
> > after first reset) back in when we're doing an AUX transfer. This
> > shouldn't actually make things much slower (assuming the panel is on)
> > because we should immediately poll and see the HPD high. Mostly this
> > is just an extra i2c transfer to the bridge.
> >
> > Fixes: f5aa7d46b0ee ("drm/bridge: parade-ps8640: Provide wait_hpd_asserted() in struct drm_dp_aux")
> > Signed-off-by: Douglas Anderson <dianders@...omium.org>
> > ---
> >
> >  drivers/gpu/drm/bridge/parade-ps8640.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> > index 541e4f5afc4c..fb5e9ae9ad81 100644
> > --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> > @@ -346,6 +346,11 @@ static ssize_t ps8640_aux_transfer(struct drm_dp_aux *aux,
> >         int ret;
> >
> >         pm_runtime_get_sync(dev);
> > +       ret = _ps8640_wait_hpd_asserted(ps_bridge, 200 * 1000);
> > +       if (ret) {
> > +               pm_runtime_put_sync_suspend(dev);
> > +               return ret;
> > +       }
> >         ret = ps8640_aux_transfer_msg(aux, msg);
> >         pm_runtime_mark_last_busy(dev);
> >         pm_runtime_put_autosuspend(dev);
> > --
> > 2.43.0.472.g3155946c3a-goog
> >
>
> I think commit 9294914dd550 ("drm/bridge: parade-ps8640: Link device
> to ensure suspend/resume order")  is trying to address the same
> problem, but we see this issue here because the device link is missing
> DL_FLAG_PM_RUNTIME. I prefer to add DL_FLAG_PM_RUNTIME here so we
> don't need to add a _ps8640_wait_hpd_asserted() after every
> pm_runtime_get_*() call.

I disagree. We've had several discussions on the lists about this
topic before, though since I'm technically on vacation right now I'm
not going to go look them up. In general "pm_runtime" is not
sufficient for powering up DRM components. While DRM components can
use pm_runtime themselves (as we are doing here), powering up another
DRM component by grabbing a pm_runtime reference isn't right. There is
a reason for the complexity of the DRM prepare/enable and all the
current debates about the right order to call components in prepare()
just demonstrates further that a simple pm_runtime reference isn't
enough.

It can be noted that, with ${SUBJECT} patch we _aren't_ powering up
the panel. I actually tested two cases on sc7180-lazor. In one case I
just closed the lid, which powered off the panel, but the touchscreen
kept the panel power rail on. In this case with my patch I could still
read the panel EDID. I then hacked the touchscreen off. Now when I
closed the lid I'd get a timeout. This is different than if we tried
to get a pm_runtime reference to the panel.


> As a side note, I've verified both this patch and DL_FLAG_PM_RUNTIME
> in our downstream v5.15 kernel and panel-edp driver. Both of them
> successfully wait for HPD asserted when the timeout used to happen,
> but the panel is black in that situation. That being said, this patch
> still brings us to a better state. Originally, panel_edp_resume()
> would return an error when the timeout occurs, so the panel-edp driver
> is stuck at an unexpected state. With this patch or
> DL_FLAG_PM_RUNTIME, the runtime PM callbacks won't fail and a system
> suspend/resume brings the panel back.

OK. I'm going to shut off email for real this time while I enjoy some
time off. Hopefully the above convinces you. Otherwise I guess we can
continue to debate in mid-January.

-Doug