[PATCH i915 v4 0/2] PRIME Synchronization

[PATCH i915 v4 0/2] PRIME Synchronization

[Alex Goins]

Hello all,

For a while now, I've been working to fix tearing with PRIME. This is my
fourth version of the solution, revised according to Daniel Vetter's and
Chris Wilson's suggestions. It now uses crtc->primary->fb to get the GEM
object instead of pending_flip_obj, waits for fence BEFORE marking page
flip active, and modifies use_mmio_flip() to ensure that mmio_flip is used
for objects backed by dma_buf with an exclusive fence attached. Calls to
reservation_object_wait_timeout_rcu have been changed to only wait for
exclusive fences, as well as to be interruptible and without a timeout.

Repeat of overview below:

I have two patches, one that implements fencing for i915's legacy mmio_flip
path, and one for atomic modesetting for futureproofing. Currently the
mmio_flip path is the one ultimately used by the X patches, due to the lack
of asynchronous atomic modesetting support in i915.

With my synchronization patches to X, it is possible to export two shared
buffers per crtc instead of just one. The sink driver uses the legacy
drmModePageFlip() to flip between the buffers, as the rest of the driver
has yet to be ported to atomics. In the pageflip/vblank event handler, the
sink driver requests a present from the source using the new X ABI function
pScreen->PresentTrackedFlippingPixmap(). If the call returns successfully,
it uses drmModePageFlip() to flip to the updated buffer, otherwise it waits
until the next vblank and tries again.

When the source driver presents on a given buffer, it first attaches a
fence.  The source driver is responsible for either using software
signaling or hardware semaphore-backed fences to ensure the fence is
signaled when the present is finished. If the sink's DRM driver implements
fencing in the flipping path, it will guarantee that that flip won't occur
until the present has finished.

This means that DRM drivers that don't implement fencing in their flipping
paths won't be able to guarantee 100% tear-free PRIME with my X patches.
However, the good news is that even without fencing, tearing is rare.
Generally presenting finishes before the next vblank, so there is no need
to wait on the fence. The X patches are a drastic improvement with or
without fencing, but the fencing is nonetheless important to guarantee
tear-free under all conditions.

To give some greater context, I've uploaded my branches for DRM and the X
server to Github. I'll move forward with upstreaming the X changes if and
when these DRM patches go in.

DRM Tree:    https://github.com/GoinsWithTheWind/drm-prime-sync
X Tree:      https://github.com/GoinsWithTheWind/xserver-prime-sync

(branch agoins-prime-v4)

Thanks, Alex @ NVIDIA Linux Driver Team

Alex Goins (2):
  i915: wait for fences in mmio_flip()
  i915: wait for fence in prepare_plane_fb

 drivers/gpu/drm/i915/intel_display.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH i915 v4 1/2] i915: wait for fences in mmio_flip()

[Alex Goins]

If a buffer is backed by dmabuf, wait on its reservation object's fences
before flipping.

Signed-off-by: Alex Goins <agoins@nvidia.com>
---
 drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b2270d5..4867ff0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -44,6 +44,8 @@
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_rect.h>
 #include <linux/dma_remapping.h>
+#include <linux/reservation.h>
+#include <linux/dma-buf.h>
 
 /* Primary plane formats for gen <= 3 */
 static const uint32_t i8xx_primary_formats[] = {
@@ -11088,6 +11090,8 @@ static bool use_mmio_flip(struct intel_engine_cs *ring,
 		return true;
 	else if (i915.enable_execlists)
 		return true;
+	else if (obj->base.dma_buf && obj->base.dma_buf->resv->fence_excl)
+		return true;
 	else
 		return ring != i915_gem_request_get_ring(obj->last_write_req);
 }
@@ -11170,8 +11174,18 @@ static void ilk_do_mmio_flip(struct intel_crtc *intel_crtc)
 static void intel_do_mmio_flip(struct intel_crtc *intel_crtc)
 {
 	struct drm_device *dev = intel_crtc->base.dev;
+	struct intel_framebuffer *intel_fb =
+		to_intel_framebuffer(intel_crtc->base.primary->fb);
+	struct drm_i915_gem_object *obj = intel_fb->obj;
 	u32 start_vbl_count;
 
+	/* For framebuffer backed by dmabuf, wait for fence */
+	if (obj->base.dma_buf) {
+		reservation_object_wait_timeout_rcu(
+			obj->base.dma_buf->resv,
+			false, true, MAX_SCHEDULE_TIMEOUT);
+	}
+
 	intel_mark_page_flip_active(intel_crtc);
 
 	intel_pipe_update_start(intel_crtc, &start_vbl_count);
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH i915 v4 2/2] i915: wait for fence in prepare_plane_fb

[Alex Goins]

In intel_prepare_plane_fb, if fb is backed by dma-buf, wait for fence.

Signed-off-by: Alex Goins <agoins@nvidia.com>
---
 drivers/gpu/drm/i915/intel_display.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4867ff0..4fb811d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13348,6 +13348,13 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 	if (!obj)
 		return 0;
 
+	/* For framebuffer backed by dmabuf, wait for fence */
+	if (obj->base.dma_buf) {
+		reservation_object_wait_timeout_rcu(
+			obj->base.dma_buf->resv,
+			false, true, MAX_SCHEDULE_TIMEOUT);
+	}
+
 	mutex_lock(&dev->struct_mutex);
 
 	if (plane->type == DRM_PLANE_TYPE_CURSOR &&
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH i915 v4 1/2] i915: wait for fences in mmio_flip()

[Daniel Vetter]

On Thu, Nov 19, 2015 at 07:51:25PM -0800, Alex Goins wrote:
> If a buffer is backed by dmabuf, wait on its reservation object's fences
> before flipping.
> 
> Signed-off-by: Alex Goins <agoins@nvidia.com>

When resending patches please add a per-revision changelog to each patch
with notes what changed. Otherwise reviewers have to recollect all the
context themselves by digging through old threads.

Can you please resend with that added?

Thanks, Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b2270d5..4867ff0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -44,6 +44,8 @@
>  #include <drm/drm_plane_helper.h>
>  #include <drm/drm_rect.h>
>  #include <linux/dma_remapping.h>
> +#include <linux/reservation.h>
> +#include <linux/dma-buf.h>
>  
>  /* Primary plane formats for gen <= 3 */
>  static const uint32_t i8xx_primary_formats[] = {
> @@ -11088,6 +11090,8 @@ static bool use_mmio_flip(struct intel_engine_cs *ring,
>  		return true;
>  	else if (i915.enable_execlists)
>  		return true;
> +	else if (obj->base.dma_buf && obj->base.dma_buf->resv->fence_excl)
> +		return true;
>  	else
>  		return ring != i915_gem_request_get_ring(obj->last_write_req);
>  }
> @@ -11170,8 +11174,18 @@ static void ilk_do_mmio_flip(struct intel_crtc *intel_crtc)
>  static void intel_do_mmio_flip(struct intel_crtc *intel_crtc)
>  {
>  	struct drm_device *dev = intel_crtc->base.dev;
> +	struct intel_framebuffer *intel_fb =
> +		to_intel_framebuffer(intel_crtc->base.primary->fb);
> +	struct drm_i915_gem_object *obj = intel_fb->obj;
>  	u32 start_vbl_count;
>  
> +	/* For framebuffer backed by dmabuf, wait for fence */
> +	if (obj->base.dma_buf) {
> +		reservation_object_wait_timeout_rcu(
> +			obj->base.dma_buf->resv,
> +			false, true, MAX_SCHEDULE_TIMEOUT);
> +	}
> +
>  	intel_mark_page_flip_active(intel_crtc);
>  
>  	intel_pipe_update_start(intel_crtc, &start_vbl_count);
> -- 
> 1.9.1
> 
> 
> -----------------------------------------------------------------------------------
> This email message is for the sole use of the intended recipient(s) and may contain
> confidential information.  Any unauthorized review, use, disclosure or distribution
> is prohibited.  If you are not the intended recipient, please contact the sender by
> reply email and destroy all copies of the original message.
> -----------------------------------------------------------------------------------

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH i915 v4 1/2] i915: wait for fences in mmio_flip()

[Ville Syrjälä]

On Thu, Nov 19, 2015 at 07:51:25PM -0800, Alex Goins wrote:
> If a buffer is backed by dmabuf, wait on its reservation object's fences
> before flipping.
> 
> Signed-off-by: Alex Goins <agoins@nvidia.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b2270d5..4867ff0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -44,6 +44,8 @@
>  #include <drm/drm_plane_helper.h>
>  #include <drm/drm_rect.h>
>  #include <linux/dma_remapping.h>
> +#include <linux/reservation.h>
> +#include <linux/dma-buf.h>
>  
>  /* Primary plane formats for gen <= 3 */
>  static const uint32_t i8xx_primary_formats[] = {
> @@ -11088,6 +11090,8 @@ static bool use_mmio_flip(struct intel_engine_cs *ring,
>  		return true;
>  	else if (i915.enable_execlists)
>  		return true;
> +	else if (obj->base.dma_buf && obj->base.dma_buf->resv->fence_excl)
> +		return true;
>  	else
>  		return ring != i915_gem_request_get_ring(obj->last_write_req);
>  }
> @@ -11170,8 +11174,18 @@ static void ilk_do_mmio_flip(struct intel_crtc *intel_crtc)
>  static void intel_do_mmio_flip(struct intel_crtc *intel_crtc)
>  {
>  	struct drm_device *dev = intel_crtc->base.dev;
> +	struct intel_framebuffer *intel_fb =
> +		to_intel_framebuffer(intel_crtc->base.primary->fb);
> +	struct drm_i915_gem_object *obj = intel_fb->obj;
>  	u32 start_vbl_count;
>  
> +	/* For framebuffer backed by dmabuf, wait for fence */
> +	if (obj->base.dma_buf) {
> +		reservation_object_wait_timeout_rcu(
> +			obj->base.dma_buf->resv,
> +			false, true, MAX_SCHEDULE_TIMEOUT);
> +	}
> +

Why is this in do_mmio_flip() rather than next to the normal seqno wait
at the previous level?

>  	intel_mark_page_flip_active(intel_crtc);
>  
>  	intel_pipe_update_start(intel_crtc, &start_vbl_count);
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH i915 v4 1/2] i915: wait for fences in mmio_flip()

[Alexander Goins]

Now that the wait happens before marking page flip active, I guess there's no reason not to.

Thanks,
Alex

-----Original Message-----
From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
Sent: Friday, November 20, 2015 3:31 AM
To: Alexander Goins
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH i915 v4 1/2] i915: wait for fences in mmio_flip()

On Thu, Nov 19, 2015 at 07:51:25PM -0800, Alex Goins wrote:
> If a buffer is backed by dmabuf, wait on its reservation object's 
> fences before flipping.
> 
> Signed-off-by: Alex Goins <agoins@nvidia.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index b2270d5..4867ff0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -44,6 +44,8 @@
>  #include <drm/drm_plane_helper.h>
>  #include <drm/drm_rect.h>
>  #include <linux/dma_remapping.h>
> +#include <linux/reservation.h>
> +#include <linux/dma-buf.h>
>  
>  /* Primary plane formats for gen <= 3 */  static const uint32_t 
> i8xx_primary_formats[] = { @@ -11088,6 +11090,8 @@ static bool 
> use_mmio_flip(struct intel_engine_cs *ring,
>  		return true;
>  	else if (i915.enable_execlists)
>  		return true;
> +	else if (obj->base.dma_buf && obj->base.dma_buf->resv->fence_excl)
> +		return true;
>  	else
>  		return ring != i915_gem_request_get_ring(obj->last_write_req);
>  }
> @@ -11170,8 +11174,18 @@ static void ilk_do_mmio_flip(struct 
> intel_crtc *intel_crtc)  static void intel_do_mmio_flip(struct 
> intel_crtc *intel_crtc)  {
>  	struct drm_device *dev = intel_crtc->base.dev;
> +	struct intel_framebuffer *intel_fb =
> +		to_intel_framebuffer(intel_crtc->base.primary->fb);
> +	struct drm_i915_gem_object *obj = intel_fb->obj;
>  	u32 start_vbl_count;
>  
> +	/* For framebuffer backed by dmabuf, wait for fence */
> +	if (obj->base.dma_buf) {
> +		reservation_object_wait_timeout_rcu(
> +			obj->base.dma_buf->resv,
> +			false, true, MAX_SCHEDULE_TIMEOUT);
> +	}
> +

Why is this in do_mmio_flip() rather than next to the normal seqno wait at the previous level?

>  	intel_mark_page_flip_active(intel_crtc);
>  
>  	intel_pipe_update_start(intel_crtc, &start_vbl_count);
> --
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH i915 v4 1/2] i915: wait for fences in mmio_flip()

[Alexander Goins]

Alright, will do.

Thanks,
Alex

-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Friday, November 20, 2015 12:21 AM
To: Alexander Goins
Cc: dri-devel@lists.freedesktop.org; daniel@fooishbar.org; daniel@ffwll.ch; maarten.lankhorst@linux.intel.com; chris@chris-wilson.co.uk
Subject: Re: [PATCH i915 v4 1/2] i915: wait for fences in mmio_flip()

On Thu, Nov 19, 2015 at 07:51:25PM -0800, Alex Goins wrote:
> If a buffer is backed by dmabuf, wait on its reservation object's 
> fences before flipping.
> 
> Signed-off-by: Alex Goins <agoins@nvidia.com>

When resending patches please add a per-revision changelog to each patch with notes what changed. Otherwise reviewers have to recollect all the context themselves by digging through old threads.

Can you please resend with that added?

Thanks, Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index b2270d5..4867ff0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -44,6 +44,8 @@
>  #include <drm/drm_plane_helper.h>
>  #include <drm/drm_rect.h>
>  #include <linux/dma_remapping.h>
> +#include <linux/reservation.h>
> +#include <linux/dma-buf.h>
>  
>  /* Primary plane formats for gen <= 3 */  static const uint32_t 
> i8xx_primary_formats[] = { @@ -11088,6 +11090,8 @@ static bool 
> use_mmio_flip(struct intel_engine_cs *ring,
>  		return true;
>  	else if (i915.enable_execlists)
>  		return true;
> +	else if (obj->base.dma_buf && obj->base.dma_buf->resv->fence_excl)
> +		return true;
>  	else
>  		return ring != i915_gem_request_get_ring(obj->last_write_req);
>  }
> @@ -11170,8 +11174,18 @@ static void ilk_do_mmio_flip(struct 
> intel_crtc *intel_crtc)  static void intel_do_mmio_flip(struct 
> intel_crtc *intel_crtc)  {
>  	struct drm_device *dev = intel_crtc->base.dev;
> +	struct intel_framebuffer *intel_fb =
> +		to_intel_framebuffer(intel_crtc->base.primary->fb);
> +	struct drm_i915_gem_object *obj = intel_fb->obj;
>  	u32 start_vbl_count;
>  
> +	/* For framebuffer backed by dmabuf, wait for fence */
> +	if (obj->base.dma_buf) {
> +		reservation_object_wait_timeout_rcu(
> +			obj->base.dma_buf->resv,
> +			false, true, MAX_SCHEDULE_TIMEOUT);
> +	}
> +
>  	intel_mark_page_flip_active(intel_crtc);
>  
>  	intel_pipe_update_start(intel_crtc, &start_vbl_count);
> --
> 1.9.1
> 
> 
> ----------------------------------------------------------------------
> ------------- This email message is for the sole use of the intended 
> recipient(s) and may contain confidential information.  Any 
> unauthorized review, use, disclosure or distribution is prohibited.  
> If you are not the intended recipient, please contact the sender by 
> reply email and destroy all copies of the original message.
> ----------------------------------------------------------------------
> -------------

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel