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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/vc4: inline flip in vc4_async_set_fence_cb() races with
  pending flips
--

commit 6426f110104db1e4dcc5dd10cee848879c7ef9a8
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 completion work that may sleep does not
run in interrupt context.

> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index c558b527c7e60..177ee27ab6d15 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c

[ ... ]

> @@ -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 vc4_async_page_flip_work() call here race with or overtake
an earlier async flip on the same CRTC?

flush_workqueue() only waits for work items already queued on
vc4_crtc->async_flip_wq.  If an earlier flip (flip1, to fb1) registered
vc4_async_page_flip_fence_cb() on an unsignaled fence, flip1->work is not
queued until that fence signals.  If a second async flip (flip2, to fb2)
on the same CRTC has no fence or an already-signaled fence:

CPU0                                    CPU1 (irq / kworker)
vc4_async_page_flip_common() [flip1]
  flip1->old_fb = fb0
  plane->state->fb = fb1
  dma_fence_add_callback(fence1, ...)

vc4_async_page_flip_common() [flip2]
  flip2->old_fb = fb1
  plane->state->fb = fb2
  flush_workqueue(async_flip_wq)
  vc4_async_page_flip_work(&flip2->work)
    vc4_plane_async_set_fb(plane, fb2)
    vc4_bo_dec_usecnt(fb1)
    drm_framebuffer_put(fb1)
                                        dma_fence_signal(fence1)
                                          queue_work(..., &flip1->work)
                                        vc4_async_page_flip_work(&flip1->work)
                                          vc4_plane_async_set_fb(plane, fb1)
                                          drm_framebuffer_put(fb1)
                                            <-- HVS scans out unreferenced fb1

flip2 completes before flip1 and drops the old_fb reference and BO usecnt
for fb1.  When flip1->work later runs, it overwrites the HVS scanout
address back to fb1 and drops flip1->fb, leaving the HVS scanning out fb1
after its flip references and BO usecnt have been released.

If fence1 signals while flip2 is past flush_workqueue(), both
vc4_async_page_flip_work() instances also run concurrently on the same
plane and update vc4_state->hw_dlist and vc4_state->dlist in
vc4_plane_async_set_fb() without locking.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-vc4-async-flip-fixes-v2-0-d33cbe99ed5d@igalia.com?part=2