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,
> > + vc4_async_page_flip_fence_complete))
> > + vc4_async_page_flip_fence_complete(fence, &flip_state->cb.fence);
> > +
> > + return 0;
> > +}
> > +
> > static int
> > vc4_async_page_flip_common(struct drm_crtc *crtc,
> > struct drm_framebuffer *fb,
> > @@ -874,8 +912,7 @@ vc4_async_page_flip_common(struct drm_crtc *crtc,
> > */
> > drm_atomic_set_fb_for_plane(plane->state, fb);
> >
> > - vc4_queue_seqno_cb(dev, &flip_state->cb.seqno, bo->seqno,
> > - vc4_async_page_flip_seqno_complete);
> > + vc4_async_set_fence_cb(dev, flip_state);
>
> I tried to run igt kms_async_flip, but the test still seems very tied to
> the i915 driver. So, as far as I could check, I didn't see any red
> flags and sync page flip behaviour seems preserved.
>
> >
> > /* Driver takes ownership of state on successful async commit. */
> > return 0;
> > --
> > 2.35.1
> >
Attachment:
signature.asc
Description: PGP signature
