Re: [Intel-gfx] [PATCH v2 4/4] drm/i915: Handle dma fences in dirtyfb callback

["Hogander, Jouni" <jouni.hogander@intel.com>]

On Tue, 2023-08-29 at 12:30 +0300, Ville Syrjälä wrote:
> On Thu, Jul 27, 2023 at 08:35:18AM +0300, Jouni Högander wrote:
> > Take into account dma fences in dirtyfb callback. If there is no
> > unsignaled dma fences perform flush immediately. If there are
> > unsignaled dma fences perform invalidate and add callback which
> > will
> > queue flush when the fence gets signaled.
> > 
> > v2: Use dma_resv_get_singleton
> > 
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_fb.c | 57
> > +++++++++++++++++++++++--
> >  1 file changed, 54 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_fb.c
> > b/drivers/gpu/drm/i915/display/intel_fb.c
> > index 446bbf7986b6..56a21377680d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fb.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> > @@ -7,6 +7,9 @@
> >  #include <drm/drm_framebuffer.h>
> >  #include <drm/drm_modeset_helper.h>
> >  
> > +#include <linux/dma-fence.h>
> > +#include <linux/dma-resv.h>
> > +
> >  #include "i915_drv.h"
> >  #include "intel_display.h"
> >  #include "intel_display_types.h"
> > @@ -1896,6 +1899,21 @@ static int
> > intel_user_framebuffer_create_handle(struct drm_framebuffer *fb,
> >         return drm_gem_handle_create(file, &obj->base, handle);
> >  }
> >  
> > +struct frontbuffer_fence_cb {
> > +       struct dma_fence_cb base;
> > +       struct intel_frontbuffer *front;
> > +};
> > +
> > +static void intel_user_framebuffer_fence_wake(struct dma_fence
> > *dma,
> > +                                             struct dma_fence_cb
> > *data)
> > +{
> > +       struct frontbuffer_fence_cb *cb = container_of(data,
> > typeof(*cb), base);
> > +
> > +       intel_frontbuffer_queue_flush(cb->front);
> > +       kfree(cb);
> > +       dma_fence_put(dma);
> > +}
> > +
> >  static int intel_user_framebuffer_dirty(struct drm_framebuffer
> > *fb,
> >                                         struct drm_file *file,
> >                                         unsigned int flags,
> > unsigned int color,
> > @@ -1903,11 +1921,44 @@ static int
> > intel_user_framebuffer_dirty(struct drm_framebuffer *fb,
> >                                         unsigned int num_clips)
> >  {
> >         struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> > +       struct intel_frontbuffer *front = to_intel_frontbuffer(fb);
> > +       struct dma_fence *fence;
> > +       struct frontbuffer_fence_cb *cb;
> > +       int ret = 0;
> > +
> > +       if (dma_resv_test_signaled(obj->base.resv,
> > dma_resv_usage_rw(false)))
> > +               goto flush;
> > +
> > +       intel_frontbuffer_invalidate(front, ORIGIN_DIRTYFB);
> 
> Looks like this could be deferred until dma_fence_add_callback()
> has informed us whether the fence has signalled in the meantime.
> That way we only do the invalidate if we succesfully add the
> callback. Could in theory drop the dma_resv_test_signaled() as well,
> but maybe it's worth keeping that around to avoid the kmalloc()/etc.
> whenever possible.
> 
> Apart from that and the few comments I gave to the other patch
> it all looks pretty reasonable to me.

Thank you for the review. I have sent a new version of the set where
I'm addressing your comments here and on that other patch. Please
check.

BR,

Jouni Högander

> 
> > +
> > +       ret = dma_resv_get_singleton(obj->base.resv,
> > dma_resv_usage_rw(false),
> > +                                    &fence);
> > +       if (ret || !fence)
> > +               goto flush;
> > +
> > +       cb = kmalloc(sizeof(*cb), GFP_KERNEL);
> > +       if (!cb) {
> > +               dma_fence_put(fence);
> > +               ret = -ENOMEM;
> > +               goto flush;
> > +       }
> >  
> > -       i915_gem_object_flush_if_display(obj);
> > -       intel_frontbuffer_flush(to_intel_frontbuffer(fb),
> > ORIGIN_DIRTYFB);
> > +       cb->front = front;
> >  
> > -       return 0;
> > +       ret = dma_fence_add_callback(fence, &cb->base,
> > +                                   
> > intel_user_framebuffer_fence_wake);
> > +       if (ret) {
> > +               intel_user_framebuffer_fence_wake(fence, &cb-
> > >base);
> > +               if (ret == -ENOENT)
> > +                       ret = 0;
> > +       }
> > +
> > +       return ret;
> > +
> > +flush:
> > +       i915_gem_object_flush_if_display(obj);
> > +       intel_frontbuffer_flush(front, ORIGIN_DIRTYFB);
> > +       return ret;
> >  }
> >  
> >  static const struct drm_framebuffer_funcs intel_fb_funcs = {
> > -- 
> > 2.34.1
>