Re: [PATCH 2/2] drm/vc4: Complete async page flips from an ordered workqueue

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] drm/vc4: inline flip in vc4_async_set_fence_cb() overtakes
  pending flips
- [Medium] drm/vc4: NULL data->name and inverted drmm order in
  __vc4_crtc_init()
--

commit 350614eb18304f31f6b7fcd5f4826c9bda20f32b
Author: Maíra Canal <mcanal@igalia.com>

drm/vc4: Complete async page flips from an ordered workqueue

This commit defers async page flip completion from the dma_fence callback to
a per-CRTC ordered workqueue so that sleeping operations do not run in
atomic context.

> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index c558b527c7e60..623b636a03652 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -884,15 +884,18 @@ struct vc4_async_flip_state {
>  	struct drm_framebuffer *fb;
>  	struct drm_framebuffer *old_fb;
>  	struct drm_pending_vblank_event *event;
> +	struct dma_fence *fence;
>  	struct dma_fence_cb cb;
> +	struct work_struct work;
>  };
>  
> -/* Called when the V3D execution for the BO being flipped to is done, so that
> - * we can actually update the plane's address to point to it.
> +/* Executed when the V3D execution for the BO being flipped to is done, so
> + * that we can actually update the plane's address to point to it.
>   */
> -static void
> -vc4_async_page_flip_complete(struct vc4_async_flip_state *flip_state)
> +static void vc4_async_page_flip_work(struct work_struct *work)
>  {
> +	struct vc4_async_flip_state *flip_state =
> +		container_of(work, struct vc4_async_flip_state, work);
>  	struct drm_crtc *crtc = flip_state->crtc;
>  	struct drm_device *dev = crtc->dev;
>  	struct vc4_dev *vc4 = to_vc4_dev(dev);
> @@ -930,17 +933,19 @@ vc4_async_page_flip_complete(struct vc4_async_flip_state *flip_state)
>  		drm_framebuffer_put(flip_state->old_fb);
>  	}
>  
> +	dma_fence_put(flip_state->fence);
>  	kfree(flip_state);
>  }
>  
> -static void vc4_async_page_flip_fence_complete(struct dma_fence *fence,
> -					       struct dma_fence_cb *cb)
> +static void vc4_async_page_flip_fence_cb(struct dma_fence *fence,
> +					 struct dma_fence_cb *cb)
>  {
>  	struct vc4_async_flip_state *flip_state =
>  		container_of(cb, struct vc4_async_flip_state, cb);
> +	struct vc4_crtc *vc4_crtc = to_vc4_crtc(flip_state->crtc);
>  
> -	vc4_async_page_flip_complete(flip_state);
> -	dma_fence_put(fence);
> +	/* Completing the flip may sleep, so defer it to a worker. */
> +	queue_work(vc4_crtc->async_flip_wq, &flip_state->work);
>  }
>  
>  static int vc4_async_set_fence_cb(struct drm_device *dev,
> @@ -948,23 +953,26 @@ static int vc4_async_set_fence_cb(struct drm_device *dev,
>  {
>  	struct drm_framebuffer *fb = flip_state->fb;
>  	struct drm_gem_dma_object *dma_bo = drm_fb_dma_get_gem_obj(fb, 0);
> -	struct dma_fence *fence;
> +	struct vc4_crtc *vc4_crtc = to_vc4_crtc(flip_state->crtc);
>  	int ret;
>  
> -	ret = dma_resv_get_singleton(dma_bo->base.resv, DMA_RESV_USAGE_READ, &fence);
> +	INIT_WORK(&flip_state->work, vc4_async_page_flip_work);
> +
> +	ret = dma_resv_get_singleton(dma_bo->base.resv, DMA_RESV_USAGE_READ,
> +				     &flip_state->fence);
>  	if (ret)
>  		return ret;
>  
> -	/* If there's no fence, complete the page flip immediately */
> -	if (!fence) {
> -		vc4_async_page_flip_fence_complete(fence, &flip_state->cb);
> +	if (flip_state->fence &&
> +	    !dma_fence_add_callback(flip_state->fence, &flip_state->cb,
> +				    vc4_async_page_flip_fence_cb))
>  		return 0;
> -	}
>  
> -	/* If the fence has already been completed, complete the page flip */
> -	if (dma_fence_add_callback(fence, &flip_state->cb,
> -				   vc4_async_page_flip_fence_complete))
> -		vc4_async_page_flip_fence_complete(fence, &flip_state->cb);
> +	/* No fence or the fence has already signaled. Complete the flip
> +	 * immediately, after earlier queued flips.
> +	 */
> +	flush_workqueue(vc4_crtc->async_flip_wq);
> +	vc4_async_page_flip_work(&flip_state->work);

[Severity: High]
Can an inline flip in vc4_async_set_fence_cb() overtake or race with an
earlier flip whose fence has not yet queued its work item?

flush_workqueue() only waits for work items already on async_flip_wq.  If
flip 1 (to fb1) is still waiting on an unsignaled fence when flip 2 (to fb2,
with no fence or an already-signaled fence) runs, flip 2 completes inline
first:

CPU0 (ioctl)                              CPU1 (kworker)
vc4_async_page_flip_common() [flip 1, fb1]
  plane->state->fb = fb1
  dma_fence_add_callback(fence1, ...)

vc4_async_page_flip_common() [flip 2, fb2]
  flip2->old_fb = fb1
  plane->state->fb = fb2
  flush_workqueue(async_flip_wq)
  vc4_async_page_flip_work(flip2)
    vc4_plane_async_set_fb(plane, fb2)
    vc4_bo_dec_usecnt(fb1)
    drm_framebuffer_put(fb1)
                                          <fence1 signals, queues work>
                                          vc4_async_page_flip_work(flip1)
                                            vc4_plane_async_set_fb(plane, fb1)
                                            drm_framebuffer_put(fb1)
                                              <-- HVS scans out unreferenced fb1

If fence1 signals around the time of flip 2, both vc4_async_page_flip_work()
calls can also run concurrently and race in vc4_plane_async_set_fb() on
vc4_state->dlist and vc4_state->hw_dlist without locking.  When flip 1
finishes after flip 2, it overwrites the display list with the older fb1 and
drops flip 1's reference to fb1 after flip 2 already dropped fb1's old_fb
reference and BO usecnt, leaving the HVS scanning out a framebuffer whose
references and usecnt have been released.

[ ... ]

> @@ -1387,7 +1395,13 @@ int __vc4_crtc_init(struct drm_device *drm,
>  	vc4_crtc->data = data;
>  	vc4_crtc->pdev = pdev;
>  	vc4_crtc->feeds_txp = feeds_txp;
> +	vc4_crtc->async_flip_wq = drmm_alloc_ordered_workqueue(drm, "%s-async-flip",
> +							       0, data->name);
> +	if (IS_ERR(vc4_crtc->async_flip_wq))
> +		return PTR_ERR(vc4_crtc->async_flip_wq);
> +
>  	spin_lock_init(&vc4_crtc->irq_lock);
> +
>  	ret = drmm_crtc_init_with_planes(drm, crtc, primary_plane, NULL,
>  					 crtc_funcs, data->name);

[Severity: Medium]
Does allocating async_flip_wq before drmm_crtc_init_with_planes() pass a
NULL data->name on BCM2712 and invert the drmm teardown order?

bcm2712_pv0_data and bcm2712_pv1_data leave .base.name as NULL and rely on
drmm_crtc_init_with_planes() to assign a default crtc->name, so data->name
formats the workqueue name as "(null)-async-flip" on Raspberry Pi 5.

In addition, drmm actions run in reverse registration order, so
drmm_crtc_init_with_planes_cleanup() calls drm_crtc_cleanup() and zeroes
*crtc (including crtc->dev and crtc->primary) before
__drmm_workqueue_release() drains and destroys async_flip_wq.  Allocating
the workqueue after drmm_crtc_init_with_planes() using crtc->name avoids
both issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-vc4-async-flip-fixes-v1-0-42449a703840@igalia.com?part=2