[PATCH i915 v3 0/2] PRIME Synchronization

[PATCH i915 v3 0/2] PRIME Synchronization

[Alex Goins]

Hello all,

For a while now, I've been working to fix tearing with PRIME. This is my third
version of the solution, revised according to Chris Wilson and Maarten
Lankhorst's suggestions to remove the unnecessary lock on object_name_lock and
to move fence waiting from intel_atomic_commit() to intel_prepare_plane_fb().

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-v3)

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 | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

-- 
1.9.1

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

[PATCH i915 v3 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 | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b2270d5..acec108a 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[] = {
@@ -11170,10 +11172,19 @@ 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 drm_i915_gem_object *pending_flip_obj =
+		intel_crtc->unpin_work->pending_flip_obj;
 	u32 start_vbl_count;
 
 	intel_mark_page_flip_active(intel_crtc);
 
+	/* For framebuffer backed by dmabuf, wait for fence */
+	if (pending_flip_obj->base.dma_buf) {
+		reservation_object_wait_timeout_rcu(
+			pending_flip_obj->base.dma_buf->resv,
+			true, false, msecs_to_jiffies(96));
+	}
+
 	intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	if (INTEL_INFO(dev)->gen >= 9)
-- 
1.9.1

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

[PATCH i915 v3 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 acec108a..36c558a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13345,6 +13345,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,
+			true, false, msecs_to_jiffies(96));
+	}
+
 	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 v3 1/2] i915: wait for fences in mmio_flip()

[Chris Wilson]

On Thu, Nov 12, 2015 at 05:49:28PM -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 | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b2270d5..acec108a 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[] = {
> @@ -11170,10 +11172,19 @@ 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 drm_i915_gem_object *pending_flip_obj =
> +		intel_crtc->unpin_work->pending_flip_obj;
>  	u32 start_vbl_count;
>  
>  	intel_mark_page_flip_active(intel_crtc);
>  
> +	/* For framebuffer backed by dmabuf, wait for fence */
> +	if (pending_flip_obj->base.dma_buf) {
> +		reservation_object_wait_timeout_rcu(
> +			pending_flip_obj->base.dma_buf->resv,
> +			true, false, msecs_to_jiffies(96));
> +	}

This wait should be prior to marking the flip as waiting for the
flip-completion interrupt. My personal preference (aside from putting
this next to the other wait) would to have been to use crtc->primary->fb
to match the do_mmip_flips funcs (I expect that we will eliminate the
pending_flip_obj in the near future).

Also we are missing the addition of
if (obj->base.dma_buf && obj->base.dma_buf->resv->fence_excl)
  return true;
to use_mmio_flip().
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Stone]

Hi,

On 13 November 2015 at 10:08, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, Nov 12, 2015 at 05:49:28PM -0800, Alex Goins wrote:
>>  static const uint32_t i8xx_primary_formats[] = {
>> @@ -11170,10 +11172,19 @@ 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 drm_i915_gem_object *pending_flip_obj =
>> +             intel_crtc->unpin_work->pending_flip_obj;
>>       u32 start_vbl_count;
>>
>>       intel_mark_page_flip_active(intel_crtc);
>>
>> +     /* For framebuffer backed by dmabuf, wait for fence */
>> +     if (pending_flip_obj->base.dma_buf) {
>> +             reservation_object_wait_timeout_rcu(
>> +                     pending_flip_obj->base.dma_buf->resv,
>> +                     true, false, msecs_to_jiffies(96));
>> +     }
>
> This wait should be prior to marking the flip as waiting for the
> flip-completion interrupt. My personal preference (aside from putting
> this next to the other wait) would to have been to use crtc->primary->fb
> to match the do_mmip_flips funcs (I expect that we will eliminate the
> pending_flip_obj in the near future).

No, don't use crtc->primary->fb for anything.

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

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

[Chris Wilson]

On Fri, Nov 13, 2015 at 10:38:07AM +0000, Daniel Stone wrote:
> Hi,
> 
> On 13 November 2015 at 10:08, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Thu, Nov 12, 2015 at 05:49:28PM -0800, Alex Goins wrote:
> >>  static const uint32_t i8xx_primary_formats[] = {
> >> @@ -11170,10 +11172,19 @@ 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 drm_i915_gem_object *pending_flip_obj =
> >> +             intel_crtc->unpin_work->pending_flip_obj;
> >>       u32 start_vbl_count;
> >>
> >>       intel_mark_page_flip_active(intel_crtc);
> >>
> >> +     /* For framebuffer backed by dmabuf, wait for fence */
> >> +     if (pending_flip_obj->base.dma_buf) {
> >> +             reservation_object_wait_timeout_rcu(
> >> +                     pending_flip_obj->base.dma_buf->resv,
> >> +                     true, false, msecs_to_jiffies(96));
> >> +     }
> >
> > This wait should be prior to marking the flip as waiting for the
> > flip-completion interrupt. My personal preference (aside from putting
> > this next to the other wait) would to have been to use crtc->primary->fb
> > to match the do_mmip_flips funcs (I expect that we will eliminate the
> > pending_flip_obj in the near future).
> 
> No, don't use crtc->primary->fb for anything.

s/crtc->primary->fb/whatever is actually used by the ilk_do_mmio_flip
and co which is certainly not pending_flip_obj/
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Alexander Goins]

Sorry; needless to say I'm not super familiar with the Intel driver. ilk_do_mmio_flip() uses crtc->primary->fb to fetch the gem object:

         struct intel_framebuffer *intel_fb =
                 to_intel_framebuffer(intel_crtc->base.primary->fb);
         struct drm_i915_gem_object *obj = intel_fb->obj;

Given that, would it be okay for me to do the same?

Thanks,
Alex

-----Original Message-----
From: Chris Wilson [mailto:chris@chris-wilson.co.uk] 
Sent: Friday, November 13, 2015 2:46 AM
To: Daniel Stone
Cc: Alexander Goins; dri-devel; Daniel Vetter; Maarten Lankhorst
Subject: Re: [PATCH i915 v3 1/2] i915: wait for fences in mmio_flip()

On Fri, Nov 13, 2015 at 10:38:07AM +0000, Daniel Stone wrote:
> Hi,
> 
> On 13 November 2015 at 10:08, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Thu, Nov 12, 2015 at 05:49:28PM -0800, Alex Goins wrote:
> >>  static const uint32_t i8xx_primary_formats[] = { @@ -11170,10 
> >> +11172,19 @@ 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 drm_i915_gem_object *pending_flip_obj =
> >> +             intel_crtc->unpin_work->pending_flip_obj;
> >>       u32 start_vbl_count;
> >>
> >>       intel_mark_page_flip_active(intel_crtc);
> >>
> >> +     /* For framebuffer backed by dmabuf, wait for fence */
> >> +     if (pending_flip_obj->base.dma_buf) {
> >> +             reservation_object_wait_timeout_rcu(
> >> +                     pending_flip_obj->base.dma_buf->resv,
> >> +                     true, false, msecs_to_jiffies(96));
> >> +     }
> >
> > This wait should be prior to marking the flip as waiting for the 
> > flip-completion interrupt. My personal preference (aside from 
> > putting this next to the other wait) would to have been to use 
> > crtc->primary->fb to match the do_mmip_flips funcs (I expect that we 
> > will eliminate the pending_flip_obj in the near future).
> 
> No, don't use crtc->primary->fb for anything.

s/crtc->primary->fb/whatever is actually used by the ilk_do_mmio_flip and co which is certainly not pending_flip_obj/ -Chris

--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Alexander Goins]

Okay, implemented these suggestions. Will send out v4 patch set once the crtc->primary->fb thing is cleared up.

-----Original Message-----
From: Chris Wilson [mailto:chris@chris-wilson.co.uk] 
Sent: Friday, November 13, 2015 2:08 AM
To: Alexander Goins
Cc: dri-devel@lists.freedesktop.org; daniel@fooishbar.org; daniel@ffwll.ch; maarten.lankhorst@linux.intel.com
Subject: Re: [PATCH i915 v3 1/2] i915: wait for fences in mmio_flip()

On Thu, Nov 12, 2015 at 05:49:28PM -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 | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index b2270d5..acec108a 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[] = { @@ -11170,10 +11172,19 @@ 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 drm_i915_gem_object *pending_flip_obj =
> +		intel_crtc->unpin_work->pending_flip_obj;
>  	u32 start_vbl_count;
>  
>  	intel_mark_page_flip_active(intel_crtc);
>  
> +	/* For framebuffer backed by dmabuf, wait for fence */
> +	if (pending_flip_obj->base.dma_buf) {
> +		reservation_object_wait_timeout_rcu(
> +			pending_flip_obj->base.dma_buf->resv,
> +			true, false, msecs_to_jiffies(96));
> +	}

This wait should be prior to marking the flip as waiting for the flip-completion interrupt. My personal preference (aside from putting this next to the other wait) would to have been to use crtc->primary->fb to match the do_mmip_flips funcs (I expect that we will eliminate the pending_flip_obj in the near future).

Also we are missing the addition of
if (obj->base.dma_buf && obj->base.dma_buf->resv->fence_excl)
  return true;
to use_mmio_flip().
-Chris

--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Fri, Nov 13, 2015 at 07:38:21PM +0000, Alexander Goins wrote:
> Sorry; needless to say I'm not super familiar with the Intel driver. ilk_do_mmio_flip() uses crtc->primary->fb to fetch the gem object:
> 
>          struct intel_framebuffer *intel_fb =
>                  to_intel_framebuffer(intel_crtc->base.primary->fb);
>          struct drm_i915_gem_object *obj = intel_fb->obj;
> 
> Given that, would it be okay for me to do the same?

I think so, since this is the legacy page_flip. Which will hopefully
disappear real soon now ...
-Daniel

> 
> Thanks,
> Alex
> 
> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk] 
> Sent: Friday, November 13, 2015 2:46 AM
> To: Daniel Stone
> Cc: Alexander Goins; dri-devel; Daniel Vetter; Maarten Lankhorst
> Subject: Re: [PATCH i915 v3 1/2] i915: wait for fences in mmio_flip()
> 
> On Fri, Nov 13, 2015 at 10:38:07AM +0000, Daniel Stone wrote:
> > Hi,
> > 
> > On 13 November 2015 at 10:08, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > On Thu, Nov 12, 2015 at 05:49:28PM -0800, Alex Goins wrote:
> > >>  static const uint32_t i8xx_primary_formats[] = { @@ -11170,10 
> > >> +11172,19 @@ 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 drm_i915_gem_object *pending_flip_obj =
> > >> +             intel_crtc->unpin_work->pending_flip_obj;
> > >>       u32 start_vbl_count;
> > >>
> > >>       intel_mark_page_flip_active(intel_crtc);
> > >>
> > >> +     /* For framebuffer backed by dmabuf, wait for fence */
> > >> +     if (pending_flip_obj->base.dma_buf) {
> > >> +             reservation_object_wait_timeout_rcu(
> > >> +                     pending_flip_obj->base.dma_buf->resv,
> > >> +                     true, false, msecs_to_jiffies(96));
> > >> +     }
> > >
> > > This wait should be prior to marking the flip as waiting for the 
> > > flip-completion interrupt. My personal preference (aside from 
> > > putting this next to the other wait) would to have been to use 
> > > crtc->primary->fb to match the do_mmip_flips funcs (I expect that we 
> > > will eliminate the pending_flip_obj in the near future).
> > 
> > No, don't use crtc->primary->fb for anything.
> 
> s/crtc->primary->fb/whatever is actually used by the ilk_do_mmio_flip and co which is certainly not pending_flip_obj/ -Chris
> 
> --
> Chris Wilson, Intel Open Source Technology Centre
> -----------------------------------------------------------------------------------
> 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