Received-SPF: pass (google.com: domain of linux-kernel+bounces-20140-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20231221135548.1.I10f326a9305d57ad32cee7f8d9c60518c8be20fb@changeid>
 <CAEXTbpdUjCvLE+m3d1vSvsE2njRSk1Ou3bZZGEvD_7oYt4+k4Q@mail.gmail.com>
 <CAD=FV=WDb7y-9dRgb0D=VxVB6EjUkcOJ+9D0Mp0-vw6wuKUHEg@mail.gmail.com> <CAEXTbpdWPcc9==xb-_+2wY29aNdieLThmn_7JD4KV8U1LWgB2g@mail.gmail.com>
In-Reply-To: <CAEXTbpdWPcc9==xb-_+2wY29aNdieLThmn_7JD4KV8U1LWgB2g@mail.gmail.com>
From: Doug Anderson <dianders@chromium.org>
Date: Mon, 8 Jan 2024 13:59:30 -0800
Message-ID: <CAD=FV=W4dkqhDbt6z=+C-pkU4HKd+NkzjRaa6P3Dxn-+cy3rCA@mail.gmail.com>
Subject: Re: [PATCH] drm/bridge: parade-ps8640: Wait for HPD when doing an AUX transfer
To: Pin-yen Lin <treapking@chromium.org>
Cc: dri-devel@lists.freedesktop.org, hsinyi@chromium.org, 
	Andrzej Hajda <andrzej.hajda@intel.com>, Daniel Vetter <daniel@ffwll.ch>, 
	David Airlie <airlied@gmail.com>, Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, 
	Jernej Skrabec <jernej.skrabec@gmail.com>, Jonas Karlman <jonas@kwiboo.se>, 
	Laurent Pinchart <Laurent.pinchart@ideasonboard.com>, 
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard <mripard@kernel.org>, 
	Neil Armstrong <neil.armstrong@linaro.org>, Robert Foss <rfoss@kernel.org>, 
	Thomas Zimmermann <tzimmermann@suse.de>, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,

On Mon, Dec 25, 2023 at 1:08=E2=80=AFAM Pin-yen Lin <treapking@chromium.org=
> wrote:
>
> Hi,
>
> On Fri, Dec 22, 2023 at 11:34=E2=80=AFPM Doug Anderson <dianders@chromium=
org> wrote:
> >
> > Hi,
> >
> > On Fri, Dec 22, 2023 at 2:29=E2=80=AFAM Pin-yen Lin <treapking@chromium=
org> wrote:
> > >
> > > Hi Douglas,
> > >
> > > On Fri, Dec 22, 2023 at 5:56=E2=80=AFAM Douglas Anderson <dianders@ch=
romium.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 ${b=
us}
> > > > 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 in=
to
> > > > 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 o=
n)
> > > > because we should immediately poll and see the HPD high. Mostly thi=
s
> > > > is just an extra i2c transfer to the bridge.
> > > >
> > > > Fixes: f5aa7d46b0ee ("drm/bridge: parade-ps8640: Provide wait_hpd_a=
sserted() in struct drm_dp_aux")
> > > > Signed-off-by: Douglas Anderson <dianders@chromium.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/d=
rm/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 =3D _ps8640_wait_hpd_asserted(ps_bridge, 200 * 1000);
> > > > +       if (ret) {
> > > > +               pm_runtime_put_sync_suspend(dev);
> > > > +               return ret;
> > > > +       }
> > > >         ret =3D 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 missin=
g
> > > 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.
> >
> Okay, thanks for the detailed explanation. Then, let's go with the
> approach in this patch. So,
>
> Tested-by: Pin-yen Lin <treapking@chromium.org>
> Reviewed-by: Pin-yen Lin <treapking@chromium.org>

Thanks for the tags. I've pushed this to drm-misc-fixes:

024b32db43a3 drm/bridge: parade-ps8640: Wait for HPD when doing an AUX tran=
sfer