On Sat, Sep 26, 2026 at 8:47 AM Rob Clark <[email protected]> wrote:
>
> On Sat, Sep 12, 2026 at 5:48 AM Dmitry Baryshkov
> <[email protected]> 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 <[email protected]>
> > ---
> > 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
> >
