diff mbox series

[v2,4/4] drm/i915: Handle dma fences in dirtyfb callback

Message ID 20230727053518.709345-5-jouni.hogander@intel.com (mailing list archive)
State New, archived
Headers show
Series Handle dma fences in dirtyfb ioctl | expand

Commit Message

Hogander, Jouni July 27, 2023, 5:35 a.m. UTC 
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(-)

Comments

Ville Syrjälä Aug. 29, 2023, 9:30 a.m. UTC | #1 
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.

> +
> +	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
Hogander, Jouni Aug. 30, 2023, 8:06 a.m. UTC | #2 
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
>
diff mbox series

Patch

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);
+
+	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 = {