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@xxxxxxxxx> > > --- > > 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 >