On Thu, 9 Jan 2025 at 03:45, Rob Clark <robdcl...@gmail.com> wrote:
>
> On Wed, Jan 8, 2025 at 2:58 PM Jessica Zhang <quic_jessz...@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_jessz...@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_jessz...@quicinc.com>
> >
--
With best wishes
Dmitry
