linux-kernel - Re: [PATCH] drm/msm/dpu: Force disabling commits to take non-async path

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: <CAF6AEGtksLq5jpizGMj4t248rbNScgDvrxpxv09==b3DmpkLuQ@mail.gmail.com>
Date: Wed, 8 Jan 2025 17:45:34 -0800
From: Rob Clark <robdclark@...il.com>
To: Jessica Zhang <quic_jesszhan@...cinc.com>
Cc: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>, Sean Paul <sean@...rly.run>, 
	Marijn Suijten <marijn.suijten@...ainline.org>, David Airlie <airlied@...il.com>, 
	Simona Vetter <simona@...ll.ch>, quic_abhinavk@...cinc.com, 
	Rob Clark <robdclark@...omium.org>, linux-arm-msm@...r.kernel.org, 
	dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/msm/dpu: Force disabling commits to take non-async path

On Wed, Jan 8, 2025 at 2:58 PM Jessica Zhang <quic_jesszhan@...cinc.com> wrote:
>
> Force commit that are disabling a plane in the async_crtc to take the
> non-async commit tail path.
>
> In cases where there are two consecutive async cursor updates (one
> regular non-NULL update followed by a disabling NULL FB update), it is
> possible for the second NULL update to not be queued (due to the
> pending_crtc_mask check) or otherwise not be run before the cursor FB is
> deallocated by drm_atomic_helper_cleanup_planes(). This would cause a
> context fault since the hardware would try to fetch the old plane state
> with the stale FB address.
>
> Avoid this issue by forcing cursor updates that will disable the cursor
> plane to be blocking commits. This will ensure that hardware clears and
> stops fetching the FB source address before the driver deallocates the FB
>
> Fixes: 2d99ced787e3 ("drm/msm: async commit support")
> Signed-off-by: Jessica Zhang <quic_jesszhan@...cinc.com>
> ---
>  drivers/gpu/drm/msm/msm_atomic.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
> index 9c45d641b5212c11078ab38c13a519663d85e10a..ddc74c68148c643d34ca631dd28d4cdc2b8c7dc0 100644
> --- a/drivers/gpu/drm/msm/msm_atomic.c
> +++ b/drivers/gpu/drm/msm/msm_atomic.c
> @@ -142,6 +142,7 @@ static bool can_do_async(struct drm_atomic_state *state,
>         struct drm_connector_state *connector_state;
>         struct drm_connector *connector;
>         struct drm_crtc_state *crtc_state;
> +       struct drm_plane_state *plane_state;
>         struct drm_crtc *crtc;
>         int i, num_crtcs = 0;
>
> @@ -162,6 +163,18 @@ static bool can_do_async(struct drm_atomic_state *state,
>                 *async_crtc = crtc;
>         }
>
> +       /*
> +        * Force a blocking commit if the cursor is being disabled. This is to
> +        * ensure that the registers are cleared and hardware doesn't try to
> +        * fetch from a stale address.
> +        */
> +       if (*async_crtc) {
> +               plane_state = drm_atomic_get_new_plane_state(state,
> +                                                            (*async_crtc)->cursor);
> +               if (plane_state && !plane_state->fb)
> +                       return false;

hmm, I suppose we want the same even if the fb changes?  Or
alternatively somewhere hold an extra ref to the backing obj until hw
has finished scanout?

BR,
-R

> +       }
> +
>         return true;
>  }
>
>
> ---
> base-commit: 866e43b945bf98f8e807dfa45eca92f931f3a032
> change-id: 20250108-async-disable-fix-cc1b9a1d5b19
>
> Best regards,
> --
> Jessica Zhang <quic_jesszhan@...cinc.com>
>