Re: [PATCH v3 1/8] drm/msm: serialise framebuffer pin state

[Rob Clark]

On Sat, Sep 26, 2026 at 8:47 AM Rob Clark <rob.clark@xxxxxxxxxxxxxxxx> wrote:
>
> On Sat, Sep 12, 2026 at 5:48 AM Dmitry Baryshkov
> <dmitry.baryshkov@xxxxxxxxxxxxxxxx> wrote:
> >
> > prepare_count, iova[] and the pins they describe are updated locklessly,
> > but drm_atomic_helper_commit() prepares a new state in parallel with the
> > completion of the previous one -- stall_checks() only stalls on the second
> > previous commit.  Commit N+1's ->prepare_fb() thus runs while commit N is
> > in ->cleanup_fb(), and for a double-buffered flip that is the same
> > framebuffer:
> >
> >   cleanup: prepare_count 1 -> 0
> >   prepare: prepare_count 0 -> 1, pins, stores iova[]
> >   cleanup: memset(iova, 0)
> >
> > leaving the plane programmed with a NULL base address:
> >
> >   arm-smmu 15000000.iommu: Unhandled context fault: fsr=0x402,
> >       iova=0x00000100, fsynr=0x3e0023, cbfrsynra=0x1c00, cb=11
> >
> > The opposite order is broken too since commit 8ac37c88f991 ("drm/msm:
> > Refcount framebuffer pins"): a prepare which finds the count non-zero
> > returns at once, assuming iova[] is populated.
> >
> > Both callbacks may sleep, so a mutex will do.  drm_framebuffer_init()
> > adds the framebuffer to the object idr, from where userspace can reach it
> > before msm_framebuffer_init() returns, so take the private state out of
> > its way and initialise it first.
> >
> > Fixes: 8ac37c88f991 ("drm/msm: Refcount framebuffer pins")
> > Assisted-by: LLM
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/msm/msm_fb.c | 44 +++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 33 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_fb.c b/drivers/gpu/drm/msm/msm_fb.c
> > index 60c108d35d2a..77415302e6d8 100644
> > --- a/drivers/gpu/drm/msm/msm_fb.c
> > +++ b/drivers/gpu/drm/msm/msm_fb.c
> > @@ -22,9 +22,12 @@ struct msm_framebuffer {
> >         /* Count of # of attached planes which need dirtyfb: */
> >         refcount_t dirtyfb;
> >
> > +       /* Protects the pin state below: */
> > +       struct mutex lock;
> > +
> >         /* Framebuffer per-plane address, if pinned, else zero: */
> >         uint64_t iova[DRM_FORMAT_MAX_PLANES];
> > -       atomic_t prepare_count;
> > +       unsigned int prepare_count;
> >  };
> >  #define to_msm_framebuffer(x) container_of(x, struct msm_framebuffer, base)
> >
> > @@ -45,9 +48,17 @@ static int msm_framebuffer_dirtyfb(struct drm_framebuffer *fb,
> >                                          clips, num_clips);
> >  }
> >
> > +static void msm_framebuffer_destroy(struct drm_framebuffer *fb)
> > +{
> > +       struct msm_framebuffer *msm_fb = to_msm_framebuffer(fb);
> > +
> > +       mutex_destroy(&msm_fb->lock);
> > +       drm_gem_fb_destroy(fb);
> > +}
> > +
> >  static const struct drm_framebuffer_funcs msm_framebuffer_funcs = {
> >         .create_handle = drm_gem_fb_create_handle,
> > -       .destroy = drm_gem_fb_destroy,
> > +       .destroy = msm_framebuffer_destroy,
> >         .dirty = msm_framebuffer_dirtyfb,
> >  };
> >
> > @@ -76,13 +87,15 @@ int msm_framebuffer_prepare(struct drm_framebuffer *fb, bool needs_dirtyfb)
> >         struct msm_drm_private *priv = fb->dev->dev_private;
> >         struct drm_gpuvm *vm = priv->kms->vm;
> >         struct msm_framebuffer *msm_fb = to_msm_framebuffer(fb);
> > -       int ret, i, n = fb->format->num_planes;
> > +       int ret = 0, i, n = fb->format->num_planes;
> >
> >         if (needs_dirtyfb)
> >                 refcount_inc(&msm_fb->dirtyfb);
> >
> > -       if (atomic_inc_return(&msm_fb->prepare_count) > 1)
> > -               return 0;
> > +       mutex_lock(&msm_fb->lock);
>
> guard(mutex)(&msm_fb->lock) ??
>
> here and below that would simplify the error path

or perhaps scoped_guard() if needed for the next patch

BR,
-R


> BR,
> -R
>
> > +
> > +       if (msm_fb->prepare_count++)
> > +               goto out;
> >
> >         for (i = 0; i < n; i++) {
> >                 msm_gem_vma_get(fb->obj[i]);
> > @@ -90,10 +103,13 @@ int msm_framebuffer_prepare(struct drm_framebuffer *fb, bool needs_dirtyfb)
> >                 drm_dbg_state(fb->dev, "FB[%u]: iova[%d]: %08llx (%d)\n",
> >                               fb->base.id, i, msm_fb->iova[i], ret);
> >                 if (ret)
> > -                       return ret;
> > +                       break;
> >         }
> >
> > -       return 0;
> > +out:
> > +       mutex_unlock(&msm_fb->lock);
> > +
> > +       return ret;
> >  }
> >
> >  void msm_framebuffer_cleanup(struct drm_framebuffer *fb, bool needed_dirtyfb)
> > @@ -106,8 +122,10 @@ void msm_framebuffer_cleanup(struct drm_framebuffer *fb, bool needed_dirtyfb)
> >         if (needed_dirtyfb)
> >                 refcount_dec(&msm_fb->dirtyfb);
> >
> > -       if (atomic_dec_return(&msm_fb->prepare_count))
> > -               return;
> > +       mutex_lock(&msm_fb->lock);
> > +
> > +       if (--msm_fb->prepare_count)
> > +               goto out;
> >
> >         memset(msm_fb->iova, 0, sizeof(msm_fb->iova));
> >
> > @@ -115,6 +133,9 @@ void msm_framebuffer_cleanup(struct drm_framebuffer *fb, bool needed_dirtyfb)
> >                 msm_gem_unpin_iova(fb->obj[i], vm);
> >                 msm_gem_vma_put(fb->obj[i]);
> >         }
> > +
> > +out:
> > +       mutex_unlock(&msm_fb->lock);
> >  }
> >
> >  uint32_t msm_framebuffer_iova(struct drm_framebuffer *fb, int plane)
> > @@ -199,14 +220,15 @@ msm_framebuffer_init(struct drm_device *dev, const struct drm_format_info *info,
> >
> >         drm_helper_mode_fill_fb_struct(dev, fb, info, mode_cmd);
> >
> > +       refcount_set(&msm_fb->dirtyfb, 1);
> > +       mutex_init(&msm_fb->lock);
> > +
> >         ret = drm_framebuffer_init(dev, fb, &msm_framebuffer_funcs);
> >         if (ret) {
> >                 DRM_DEV_ERROR(dev->dev, "framebuffer init failed: %d\n", ret);
> >                 goto fail;
> >         }
> >
> > -       refcount_set(&msm_fb->dirtyfb, 1);
> > -
> >         drm_dbg_state(dev, "create: FB ID: %d (%p)\n", fb->base.id, fb);
> >
> >         return fb;
> >
> > --
> > 2.47.3
> >