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

[Dmitry Baryshkov]

On Thu, 9 Jan 2025 at 03:45, Rob Clark <robdclark at gmail.com> wrote:
>
> On Wed, Jan 8, 2025 at 2:58 PM Jessica Zhang <quic_jesszhan at quicinc.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 at quicinc.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?

I think a more correct approach would be to run a worker, waiting for
the commit to happen and then freeing the FBs.

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



-- 
With best wishes
Dmitry