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
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
>
