Received-SPF: pass (google.com: domain of linux-kernel+bounces-9594-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20231221135548.1.I10f326a9305d57ad32cee7f8d9c60518c8be20fb@changeid>
In-Reply-To: <20231221135548.1.I10f326a9305d57ad32cee7f8d9c60518c8be20fb@changeid>
From: Pin-yen Lin <treapking@chromium.org>
Date: Fri, 22 Dec 2023 18:29:22 +0800
Message-ID: <CAEXTbpdUjCvLE+m3d1vSvsE2njRSk1Ou3bZZGEvD_7oYt4+k4Q@mail.gmail.com>
Subject: Re: [PATCH] drm/bridge: parade-ps8640: Wait for HPD when doing an AUX transfer
To: Douglas Anderson <dianders@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 Douglas,

On Fri, Dec 22, 2023 at 5:56=E2=80=AFAM Douglas Anderson <dianders@chromium=
.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_asserte=
d() 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/drm/bri=
dge/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 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.

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.

Regards,
Pin-yen