[PATCH 13/14] drm/vc4: crtc: Fix out of order frames during asynchronous page flips

[Melissa Wen]

On 06/06, Maxime Ripard wrote:
> Hi,
> 
> On Thu, May 12, 2022 at 09:44:42AM -0100, Melissa Wen wrote:
> > On 05/09, Melissa Wen wrote:
> > > O 05/09, Melissa Wen wrote:
> > > > On 05/03, Maxime Ripard wrote:
> > > > > When doing an asynchronous page flip (PAGE_FLIP ioctl with the
> > > > > DRM_MODE_PAGE_FLIP_ASYNC flag set), the current code waits for the
> > > > > possible GPU buffer being rendered through a call to
> > > > > vc4_queue_seqno_cb().
> > > > > 
> > > > > On the BCM2835-37, the GPU driver is part of the vc4 driver and that
> > > > > function is defined in vc4_gem.c to wait for the buffer to be rendered,
> > > > > and once it's done, call a callback.
> > > > > 
> > > > > However, on the BCM2711 used on the RaspberryPi4, the GPU driver is
> > > > > separate (v3d) and that function won't do anything. This was working
> > > > > because we were going into a path, due to uninitialized variables, that
> > > > > was always scheduling the callback.
> > > > > 
> > > > > However, we were never actually waiting for the buffer to be rendered
> > > > > which was resulting in frames being displayed out of order.
> > > > > 
> > > > > The generic API to signal those kind of completion in the kernel are the
> > > > > DMA fences, and fortunately the v3d drivers supports them and signal
> > > > > when its job is done. That API also provides an equivalent function that
> > > > > allows to have a callback being executed when the fence is signalled as
> > > > > done.
> > > > > 
> > > > > Let's change our driver a bit to rely on the previous function for the
> > > > > older SoCs, and on DMA fences for the BCM2711.
> > > > > 
> > > > > Signed-off-by: Maxime Ripard <maxime at cerno.tech>
> > > > > ---
> > > > >  drivers/gpu/drm/vc4/vc4_crtc.c | 41 ++++++++++++++++++++++++++++++++--
> > > > >  1 file changed, 39 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> > > > > index e0ae7bef08fa..8e1369fca937 100644
> > > > > --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> > > > > +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> > > > > @@ -776,6 +776,7 @@ struct vc4_async_flip_state {
> > > > >  	struct drm_pending_vblank_event *event;
> > > > >  
> > > > >  	union {
> > > > > +		struct dma_fence_cb fence;
> > > > >  		struct vc4_seqno_cb seqno;
> > > > >  	} cb;
> > > > >  };
> > > > > @@ -835,6 +836,43 @@ static void vc4_async_page_flip_seqno_complete(struct vc4_seqno_cb *cb)
> > > > >  		vc4_bo_dec_usecnt(bo);
> > > > >  }
> > > > >  
> > > > > +static void vc4_async_page_flip_fence_complete(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.fence);
> > > > > +
> > > > > +	vc4_async_page_flip_complete(flip_state);
> > > > > +	dma_fence_put(fence);
> > > > > +}
> > > > > +
> > > > > +static int vc4_async_set_fence_cb(struct drm_device *dev,
> > > > > +				  struct vc4_async_flip_state *flip_state)
> > > > > +{
> > > > > +	struct drm_framebuffer *fb = flip_state->fb;
> > > > > +	struct drm_gem_cma_object *cma_bo = drm_fb_cma_get_gem_obj(fb, 0);
> > > > > +	struct vc4_dev *vc4 = to_vc4_dev(dev);
> > > > > +	struct dma_fence *fence;
> > > > > +	int ret;
> > > > > +
> > > > > +	if (!vc4->is_vc5) {
> > > > > +		struct vc4_bo *bo = to_vc4_bo(&cma_bo->base);
> > > > > +
> > > > > +		return vc4_queue_seqno_cb(dev, &flip_state->cb.seqno, bo->seqno,
> > > > > +					  vc4_async_page_flip_seqno_complete);
> > > > > +	}
> > > > > +
> > > > > +	ret = dma_resv_get_singleton(cma_bo->base.resv, false, &fence);
> > > + for kernel bot complaint, I replaced false with `DMA_RESV_USAGE_READ`
> > > to run some tests
> > > 
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	if (dma_fence_add_callback(fence, &flip_state->cb.fence,
> > me again :)
> > 
> > I was thinking if we should add a check here for !fence and just complete the page flip,
> > instead of letting `dma_fence_add_callback` warns whenever fence is NULL.
> > I think there are situation in which fence is NULL and it is not an
> > issue, right? Does it make sense?
> 
> I'm not sure. What situation do you have in mind?

I mean, if no implicity fence was attached to this bo, it's safe to just
do the page flip. This behaviour will happen anyway, after
dma_fence_add_callback() checks fence is NULL and return -EINVAL. But
this check will also trigger a warning for something that it's not an
issue here, I think. So, if we just check `fence` before calling
dma_fence_add_callback(), we keep the same behaviour and prevent the
warning.

Melissa
> 
> Maxime


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20220608/15869c2d/attachment-0001.sig>