On Wed, Jan 8, 2025 at 2:58 PM Jessica Zhang <quic_jesszhan@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxx>
>
