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@xxxxxxxxxx> > > > > --- > > > > 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? Maxime
Attachment:
signature.asc
Description: PGP signature