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
>
