[PATCH] drm/i915: Simplify and fix object to display tracking

[PATCH] drm/i915: Simplify and fix object to display tracking

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Purpose of this tracking is to know when to flush the cache between the
non-coherent display engine. Previously to:

   commit 121920faf2ccce9aa66a7e2588415c9647b66104
   Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
   Date:   Mon Mar 23 11:10:37 2015 +0000

       drm/i915/skl: Query display address through a wrapper

This worked by a mix of direct flag manipulation and checking for
existence of a pinned GGTT VMA.

With the introduction of rotated display mappings this approach is
no longer correct.

New simpler approach is to just keep this count over calls which pin and
unpin objects to and from display.

(Inspired and extracted code from a larger rework by Chris Wilson.)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h |  3 ++-
 drivers/gpu/drm/i915/i915_gem.c | 33 +++++++++------------------------
 2 files changed, 11 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4ef320c..12388dd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1969,7 +1969,8 @@ struct drm_i915_gem_object {
 	 */
 	unsigned int fault_mappable:1;
 	unsigned int pin_mappable:1;
-	unsigned int pin_display:1;
+	unsigned int pin_display:3;
+#define I915_MAX_PIN_DISPLAY 7
 
 	/*
 	 * Is the object to be mapped as read-only to the GPU
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 3a91365..b8579d4 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3849,24 +3849,6 @@ unlock:
 	return ret;
 }
 
-static bool is_pin_display(struct drm_i915_gem_object *obj)
-{
-	struct i915_vma *vma;
-
-	vma = i915_gem_obj_to_ggtt(obj);
-	if (!vma)
-		return false;
-
-	/* There are 2 sources that pin objects:
-	 *   1. The display engine (scanouts, sprites, cursors);
-	 *   2. Reservations for execbuffer;
-	 *
-	 * We can ignore reservations as we hold the struct_mutex and
-	 * are only called outside of the reservation path.
-	 */
-	return vma->pin_count;
-}
-
 /*
  * Prepare buffer for display plane (scanout, cursors, etc).
  * Can be called from an uninterruptible phase (modesetting) and allows
@@ -3879,9 +3861,11 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 				     const struct i915_ggtt_view *view)
 {
 	u32 old_read_domains, old_write_domain;
-	bool was_pin_display;
 	int ret;
 
+	if (WARN_ON(obj->pin_display == I915_MAX_PIN_DISPLAY))
+		return -ENODEV;
+
 	if (pipelined != i915_gem_request_get_ring(obj->last_read_req)) {
 		ret = i915_gem_object_sync(obj, pipelined);
 		if (ret)
@@ -3891,8 +3875,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 	/* Mark the pin_display early so that we account for the
 	 * display coherency whilst setting up the cache domains.
 	 */
-	was_pin_display = obj->pin_display;
-	obj->pin_display = true;
+	obj->pin_display++;
 
 	/* The display engine is not coherent with the LLC cache on gen6.  As
 	 * a result, we make sure that the pinning that is about to occur is
@@ -3936,8 +3919,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 	return 0;
 
 err_unpin_display:
-	WARN_ON(was_pin_display != is_pin_display(obj));
-	obj->pin_display = was_pin_display;
+	obj->pin_display--;
 	return ret;
 }
 
@@ -3945,9 +3927,12 @@ void
 i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj,
 					 const struct i915_ggtt_view *view)
 {
+	if (WARN_ON(obj->pin_display == 0))
+		return;
+
 	i915_gem_object_ggtt_unpin_view(obj, view);
 
-	obj->pin_display = is_pin_display(obj);
+	obj->pin_display--;
 }
 
 int
-- 
2.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Simplify and fix object to display tracking

[Ville Syrjälä]

On Tue, Mar 31, 2015 at 12:10:04PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Purpose of this tracking is to know when to flush the cache between the
> non-coherent display engine. Previously to:
> 
>    commit 121920faf2ccce9aa66a7e2588415c9647b66104
>    Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>    Date:   Mon Mar 23 11:10:37 2015 +0000
> 
>        drm/i915/skl: Query display address through a wrapper
> 
> This worked by a mix of direct flag manipulation and checking for
> existence of a pinned GGTT VMA.
> 
> With the introduction of rotated display mappings this approach is
> no longer correct.
> 
> New simpler approach is to just keep this count over calls which pin and
> unpin objects to and from display.
> 
> (Inspired and extracted code from a larger rework by Chris Wilson.)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  3 ++-
>  drivers/gpu/drm/i915/i915_gem.c | 33 +++++++++------------------------
>  2 files changed, 11 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4ef320c..12388dd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1969,7 +1969,8 @@ struct drm_i915_gem_object {
>  	 */
>  	unsigned int fault_mappable:1;
>  	unsigned int pin_mappable:1;
> -	unsigned int pin_display:1;
> +	unsigned int pin_display:3;
> +#define I915_MAX_PIN_DISPLAY 7

Why is that enough?

>  
>  	/*
>  	 * Is the object to be mapped as read-only to the GPU
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 3a91365..b8579d4 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3849,24 +3849,6 @@ unlock:
>  	return ret;
>  }
>  
> -static bool is_pin_display(struct drm_i915_gem_object *obj)
> -{
> -	struct i915_vma *vma;
> -
> -	vma = i915_gem_obj_to_ggtt(obj);
> -	if (!vma)
> -		return false;
> -
> -	/* There are 2 sources that pin objects:
> -	 *   1. The display engine (scanouts, sprites, cursors);
> -	 *   2. Reservations for execbuffer;
> -	 *
> -	 * We can ignore reservations as we hold the struct_mutex and
> -	 * are only called outside of the reservation path.
> -	 */
> -	return vma->pin_count;
> -}
> -
>  /*
>   * Prepare buffer for display plane (scanout, cursors, etc).
>   * Can be called from an uninterruptible phase (modesetting) and allows
> @@ -3879,9 +3861,11 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  				     const struct i915_ggtt_view *view)
>  {
>  	u32 old_read_domains, old_write_domain;
> -	bool was_pin_display;
>  	int ret;
>  
> +	if (WARN_ON(obj->pin_display == I915_MAX_PIN_DISPLAY))
> +		return -ENODEV;
> +
>  	if (pipelined != i915_gem_request_get_ring(obj->last_read_req)) {
>  		ret = i915_gem_object_sync(obj, pipelined);
>  		if (ret)
> @@ -3891,8 +3875,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  	/* Mark the pin_display early so that we account for the
>  	 * display coherency whilst setting up the cache domains.
>  	 */
> -	was_pin_display = obj->pin_display;
> -	obj->pin_display = true;
> +	obj->pin_display++;
>  
>  	/* The display engine is not coherent with the LLC cache on gen6.  As
>  	 * a result, we make sure that the pinning that is about to occur is
> @@ -3936,8 +3919,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  	return 0;
>  
>  err_unpin_display:
> -	WARN_ON(was_pin_display != is_pin_display(obj));
> -	obj->pin_display = was_pin_display;
> +	obj->pin_display--;
>  	return ret;
>  }
>  
> @@ -3945,9 +3927,12 @@ void
>  i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj,
>  					 const struct i915_ggtt_view *view)
>  {
> +	if (WARN_ON(obj->pin_display == 0))
> +		return;
> +
>  	i915_gem_object_ggtt_unpin_view(obj, view);
>  
> -	obj->pin_display = is_pin_display(obj);
> +	obj->pin_display--;
>  }
>  
>  int
> -- 
> 2.3.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH] drm/i915: Simplify and fix object to display tracking

[Chris Wilson]

On Tue, Mar 31, 2015 at 12:10:04PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Purpose of this tracking is to know when to flush the cache between the
> non-coherent display engine. Previously to:
> 
>    commit 121920faf2ccce9aa66a7e2588415c9647b66104
>    Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>    Date:   Mon Mar 23 11:10:37 2015 +0000
> 
>        drm/i915/skl: Query display address through a wrapper
> 
> This worked by a mix of direct flag manipulation and checking for
> existence of a pinned GGTT VMA.
> 
> With the introduction of rotated display mappings this approach is
> no longer correct.
> 
> New simpler approach is to just keep this count over calls which pin and
> unpin objects to and from display.
> 
> (Inspired and extracted code from a larger rework by Chris Wilson.)

Hmm, given a stuck pageflip worker we can easily end up with the same
object pinned into the display plane several times. (If that is not enough
to convince, think about async pageflips and a low priority worker.)
Multiply that over each CRTC doing its own pinning and it is quite easy
to run afoul of I915_MAX_PIN_DISPLAY. I was thinking that having the
counter on the vma was cleaner, but in retrospect I think we just want a
full counter on the object.
-Chris

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

Re: [PATCH] drm/i915: Simplify and fix object to display tracking

[Chris Wilson]

On Tue, Mar 31, 2015 at 03:00:30PM +0300, Ville Syrjälä wrote:
> On Tue, Mar 31, 2015 at 12:10:04PM +0100, Tvrtko Ursulin wrote:
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 4ef320c..12388dd 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1969,7 +1969,8 @@ struct drm_i915_gem_object {
> >  	 */
> >  	unsigned int fault_mappable:1;
> >  	unsigned int pin_mappable:1;
> > -	unsigned int pin_display:1;
> > +	unsigned int pin_display:3;
> > +#define I915_MAX_PIN_DISPLAY 7
> 
> Why is that enough?

I had it split across two counters, which limited the number of times an
object could pinned to the display plane NUM_PLANES+1. I thought I was
saving space by using a few bits from a hole in the object and putting
the full counter on the vma.
-Chris

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

[PATCH v2] drm/i915: Simplify and fix object to display tracking

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Purpose of this tracking is to know when to flush the cache between the
non-coherent display engine. Previously to:

   commit 121920faf2ccce9aa66a7e2588415c9647b66104
   Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
   Date:   Mon Mar 23 11:10:37 2015 +0000

       drm/i915/skl: Query display address through a wrapper

This worked by a mix of direct flag manipulation and checking for
existence of a pinned GGTT VMA.

With the introduction of rotated display mappings this approach is
no longer correct.

New simpler approach is to just keep this count over calls which pin and
unpin objects to and from display.

(Inspired and extracted code from a larger rework by Chris Wilson.)

v2: Remove the limit since it is not well defined.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h |  3 ++-
 drivers/gpu/drm/i915/i915_gem.c | 30 ++++++------------------------
 2 files changed, 8 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4ef320c..37abd58 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1969,7 +1969,6 @@ struct drm_i915_gem_object {
 	 */
 	unsigned int fault_mappable:1;
 	unsigned int pin_mappable:1;
-	unsigned int pin_display:1;
 
 	/*
 	 * Is the object to be mapped as read-only to the GPU
@@ -1983,6 +1982,8 @@ struct drm_i915_gem_object {
 
 	unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS;
 
+	unsigned int pin_display;
+
 	struct sg_table *pages;
 	int pages_pin_count;
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 3a91365..8b75dab 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3849,24 +3849,6 @@ unlock:
 	return ret;
 }
 
-static bool is_pin_display(struct drm_i915_gem_object *obj)
-{
-	struct i915_vma *vma;
-
-	vma = i915_gem_obj_to_ggtt(obj);
-	if (!vma)
-		return false;
-
-	/* There are 2 sources that pin objects:
-	 *   1. The display engine (scanouts, sprites, cursors);
-	 *   2. Reservations for execbuffer;
-	 *
-	 * We can ignore reservations as we hold the struct_mutex and
-	 * are only called outside of the reservation path.
-	 */
-	return vma->pin_count;
-}
-
 /*
  * Prepare buffer for display plane (scanout, cursors, etc).
  * Can be called from an uninterruptible phase (modesetting) and allows
@@ -3879,7 +3861,6 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 				     const struct i915_ggtt_view *view)
 {
 	u32 old_read_domains, old_write_domain;
-	bool was_pin_display;
 	int ret;
 
 	if (pipelined != i915_gem_request_get_ring(obj->last_read_req)) {
@@ -3891,8 +3872,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 	/* Mark the pin_display early so that we account for the
 	 * display coherency whilst setting up the cache domains.
 	 */
-	was_pin_display = obj->pin_display;
-	obj->pin_display = true;
+	obj->pin_display++;
 
 	/* The display engine is not coherent with the LLC cache on gen6.  As
 	 * a result, we make sure that the pinning that is about to occur is
@@ -3936,8 +3916,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 	return 0;
 
 err_unpin_display:
-	WARN_ON(was_pin_display != is_pin_display(obj));
-	obj->pin_display = was_pin_display;
+	obj->pin_display--;
 	return ret;
 }
 
@@ -3945,9 +3924,12 @@ void
 i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj,
 					 const struct i915_ggtt_view *view)
 {
+	if (WARN_ON(obj->pin_display == 0))
+		return;
+
 	i915_gem_object_ggtt_unpin_view(obj, view);
 
-	obj->pin_display = is_pin_display(obj);
+	obj->pin_display--;
 }
 
 int
-- 
2.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Simplify and fix object to display tracking

[Chris Wilson]

On Tue, Mar 31, 2015 at 01:23:10PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Purpose of this tracking is to know when to flush the cache between the

CPU and the

> non-coherent display engine. Previously to:

s/Previously/Prior/

> 
>    commit 121920faf2ccce9aa66a7e2588415c9647b66104
>    Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>    Date:   Mon Mar 23 11:10:37 2015 +0000
> 
>        drm/i915/skl: Query display address through a wrapper
> 
> This worked by a mix of direct flag manipulation and checking for
> existence of a pinned GGTT VMA.
> 
> With the introduction of rotated display mappings this approach is
> no longer correct.
> 
> New simpler approach is to just keep this count over calls which pin and
> unpin objects to and from display.

at the slight cost of extra space in every bo.

> 
> (Inspired and extracted code from a larger rework by Chris Wilson.)
> 
> v2: Remove the limit since it is not well defined.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>

Proof reading someone else's changelog is much simpler than coming up
with one from scratch.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

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

Re: [PATCH v2] drm/i915: Simplify and fix object to display tracking

[Tvrtko Ursulin]

On 03/31/2015 01:32 PM, Chris Wilson wrote:
> On Tue, Mar 31, 2015 at 01:23:10PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Purpose of this tracking is to know when to flush the cache between the
>
> CPU and the
>
>> non-coherent display engine. Previously to:
>
> s/Previously/Prior/
>
>>
>>     commit 121920faf2ccce9aa66a7e2588415c9647b66104
>>     Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>     Date:   Mon Mar 23 11:10:37 2015 +0000
>>
>>         drm/i915/skl: Query display address through a wrapper
>>
>> This worked by a mix of direct flag manipulation and checking for
>> existence of a pinned GGTT VMA.
>>
>> With the introduction of rotated display mappings this approach is
>> no longer correct.
>>
>> New simpler approach is to just keep this count over calls which pin and
>> unpin objects to and from display.
>
> at the slight cost of extra space in every bo.

Is space is a concern, how about just a flag then? Counter kind of lost 
its usefulness at the moment.

>>
>> (Inspired and extracted code from a larger rework by Chris Wilson.)
>>
>> v2: Remove the limit since it is not well defined.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>
> Proof reading someone else's changelog is much simpler than coming up
> with one from scratch.
>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks!

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Simplify and fix object to display tracking

[Chris Wilson]

On Tue, Mar 31, 2015 at 01:41:42PM +0100, Tvrtko Ursulin wrote:
> 
> On 03/31/2015 01:32 PM, Chris Wilson wrote:
> >On Tue, Mar 31, 2015 at 01:23:10PM +0100, Tvrtko Ursulin wrote:
> >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >>Purpose of this tracking is to know when to flush the cache between the
> >
> >CPU and the
> >
> >>non-coherent display engine. Previously to:
> >
> >s/Previously/Prior/
> >
> >>
> >>    commit 121920faf2ccce9aa66a7e2588415c9647b66104
> >>    Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>    Date:   Mon Mar 23 11:10:37 2015 +0000
> >>
> >>        drm/i915/skl: Query display address through a wrapper
> >>
> >>This worked by a mix of direct flag manipulation and checking for
> >>existence of a pinned GGTT VMA.
> >>
> >>With the introduction of rotated display mappings this approach is
> >>no longer correct.
> >>
> >>New simpler approach is to just keep this count over calls which pin and
> >>unpin objects to and from display.
> >
> >at the slight cost of extra space in every bo.
> 
> Is space is a concern, how about just a flag then? Counter kind of
> lost its usefulness at the moment.

Space is always a concern with something that we allocate in the tens of
thousands - however, we round object sizes up to a cacheline, so
keeping the object trim is quite hard and our objects are already quite
large. Every now and again we try and go on a diet, but it doesn't last.

What's the shortcoming of the counter?
-Chris

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

[PATCH v3] drm/i915: Simplify and fix object to display tracking

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Purpose of this tracking is to know when to flush the cache between
the CPU and the non-coherent display engine. Prior to:

   commit 121920faf2ccce9aa66a7e2588415c9647b66104
   Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
   Date:   Mon Mar 23 11:10:37 2015 +0000

       drm/i915/skl: Query display address through a wrapper

This worked by a mix of direct flag manipulation and checking for
existence of a pinned GGTT VMA.

With the introduction of rotated display mappings this approach is
no longer correct.

New simpler approach is to just keep this count over calls which pin
and unpin objects to and from display, at the slight cost of extra
space in every bo.

(Inspired and extracted code from a larger rework by Chris Wilson.)

v2: Remove the limit since it is not well defined. (Chris Wilson, Ville Syrjälä)
v3: Commit message corrections. (Chris Wilson)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h |  3 ++-
 drivers/gpu/drm/i915/i915_gem.c | 30 ++++++------------------------
 2 files changed, 8 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4ef320c..37abd58 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1969,7 +1969,6 @@ struct drm_i915_gem_object {
 	 */
 	unsigned int fault_mappable:1;
 	unsigned int pin_mappable:1;
-	unsigned int pin_display:1;
 
 	/*
 	 * Is the object to be mapped as read-only to the GPU
@@ -1983,6 +1982,8 @@ struct drm_i915_gem_object {
 
 	unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS;
 
+	unsigned int pin_display;
+
 	struct sg_table *pages;
 	int pages_pin_count;
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 3a91365..8b75dab 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3849,24 +3849,6 @@ unlock:
 	return ret;
 }
 
-static bool is_pin_display(struct drm_i915_gem_object *obj)
-{
-	struct i915_vma *vma;
-
-	vma = i915_gem_obj_to_ggtt(obj);
-	if (!vma)
-		return false;
-
-	/* There are 2 sources that pin objects:
-	 *   1. The display engine (scanouts, sprites, cursors);
-	 *   2. Reservations for execbuffer;
-	 *
-	 * We can ignore reservations as we hold the struct_mutex and
-	 * are only called outside of the reservation path.
-	 */
-	return vma->pin_count;
-}
-
 /*
  * Prepare buffer for display plane (scanout, cursors, etc).
  * Can be called from an uninterruptible phase (modesetting) and allows
@@ -3879,7 +3861,6 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 				     const struct i915_ggtt_view *view)
 {
 	u32 old_read_domains, old_write_domain;
-	bool was_pin_display;
 	int ret;
 
 	if (pipelined != i915_gem_request_get_ring(obj->last_read_req)) {
@@ -3891,8 +3872,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 	/* Mark the pin_display early so that we account for the
 	 * display coherency whilst setting up the cache domains.
 	 */
-	was_pin_display = obj->pin_display;
-	obj->pin_display = true;
+	obj->pin_display++;
 
 	/* The display engine is not coherent with the LLC cache on gen6.  As
 	 * a result, we make sure that the pinning that is about to occur is
@@ -3936,8 +3916,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 	return 0;
 
 err_unpin_display:
-	WARN_ON(was_pin_display != is_pin_display(obj));
-	obj->pin_display = was_pin_display;
+	obj->pin_display--;
 	return ret;
 }
 
@@ -3945,9 +3924,12 @@ void
 i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj,
 					 const struct i915_ggtt_view *view)
 {
+	if (WARN_ON(obj->pin_display == 0))
+		return;
+
 	i915_gem_object_ggtt_unpin_view(obj, view);
 
-	obj->pin_display = is_pin_display(obj);
+	obj->pin_display--;
 }
 
 int
-- 
2.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Simplify and fix object to display tracking

[Joonas Lahtinen]

On ti, 2015-03-31 at 13:55 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Purpose of this tracking is to know when to flush the cache between
> the CPU and the non-coherent display engine. Prior to:
> 
>    commit 121920faf2ccce9aa66a7e2588415c9647b66104
>    Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>    Date:   Mon Mar 23 11:10:37 2015 +0000
> 
>        drm/i915/skl: Query display address through a wrapper
> 
> This worked by a mix of direct flag manipulation and checking for
> existence of a pinned GGTT VMA.
> 
> With the introduction of rotated display mappings this approach is
> no longer correct.
> 
> New simpler approach is to just keep this count over calls which pin
> and unpin objects to and from display, at the slight cost of extra
> space in every bo.
> 
> (Inspired and extracted code from a larger rework by Chris Wilson.)
> 
> v2: Remove the limit since it is not well defined. (Chris Wilson, Ville Syrjälä)
> v3: Commit message corrections. (Chris Wilson)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  3 ++-
>  drivers/gpu/drm/i915/i915_gem.c | 30 ++++++------------------------
>  2 files changed, 8 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4ef320c..37abd58 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1969,7 +1969,6 @@ struct drm_i915_gem_object {
>  	 */
>  	unsigned int fault_mappable:1;
>  	unsigned int pin_mappable:1;
> -	unsigned int pin_display:1;
>  
>  	/*
>  	 * Is the object to be mapped as read-only to the GPU
> @@ -1983,6 +1982,8 @@ struct drm_i915_gem_object {
>  
>  	unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS;
>  
> +	unsigned int pin_display;
> +

Here. Comment below.

>  	struct sg_table *pages;
>  	int pages_pin_count;
>  

How about naming it like the variable just below it (pages_pin_count).
So let it be "display_pin_count", and make it just "int" too, I don't
think the 1 bit reduction in pin count matters really, but having sign
makes it useful for detecting negative pin count errors? At least I'd
make it signed integer and make some of the spots WARN_ON(xxx--- < 0).

Regards, Joonas

> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 3a91365..8b75dab 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3849,24 +3849,6 @@ unlock:
>  	return ret;
>  }
>  
> -static bool is_pin_display(struct drm_i915_gem_object *obj)
> -{
> -	struct i915_vma *vma;
> -
> -	vma = i915_gem_obj_to_ggtt(obj);
> -	if (!vma)
> -		return false;
> -
> -	/* There are 2 sources that pin objects:
> -	 *   1. The display engine (scanouts, sprites, cursors);
> -	 *   2. Reservations for execbuffer;
> -	 *
> -	 * We can ignore reservations as we hold the struct_mutex and
> -	 * are only called outside of the reservation path.
> -	 */
> -	return vma->pin_count;
> -}
> -
>  /*
>   * Prepare buffer for display plane (scanout, cursors, etc).
>   * Can be called from an uninterruptible phase (modesetting) and allows
> @@ -3879,7 +3861,6 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  				     const struct i915_ggtt_view *view)
>  {
>  	u32 old_read_domains, old_write_domain;
> -	bool was_pin_display;
>  	int ret;
>  
>  	if (pipelined != i915_gem_request_get_ring(obj->last_read_req)) {
> @@ -3891,8 +3872,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  	/* Mark the pin_display early so that we account for the
>  	 * display coherency whilst setting up the cache domains.
>  	 */
> -	was_pin_display = obj->pin_display;
> -	obj->pin_display = true;
> +	obj->pin_display++;
>  
>  	/* The display engine is not coherent with the LLC cache on gen6.  As
>  	 * a result, we make sure that the pinning that is about to occur is
> @@ -3936,8 +3916,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  	return 0;
>  
>  err_unpin_display:
> -	WARN_ON(was_pin_display != is_pin_display(obj));
> -	obj->pin_display = was_pin_display;
> +	obj->pin_display--;
>  	return ret;
>  }
>  
> @@ -3945,9 +3924,12 @@ void
>  i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj,
>  					 const struct i915_ggtt_view *view)
>  {
> +	if (WARN_ON(obj->pin_display == 0))
> +		return;
> +
>  	i915_gem_object_ggtt_unpin_view(obj, view);
>  
> -	obj->pin_display = is_pin_display(obj);
> +	obj->pin_display--;
>  }
>  
>  int


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Simplify and fix object to display tracking

[Tvrtko Ursulin]

Hi,

On 03/31/2015 02:10 PM, Joonas Lahtinen wrote:
> On ti, 2015-03-31 at 13:55 +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Purpose of this tracking is to know when to flush the cache between
>> the CPU and the non-coherent display engine. Prior to:
>>
>>     commit 121920faf2ccce9aa66a7e2588415c9647b66104
>>     Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>     Date:   Mon Mar 23 11:10:37 2015 +0000
>>
>>         drm/i915/skl: Query display address through a wrapper
>>
>> This worked by a mix of direct flag manipulation and checking for
>> existence of a pinned GGTT VMA.
>>
>> With the introduction of rotated display mappings this approach is
>> no longer correct.
>>
>> New simpler approach is to just keep this count over calls which pin
>> and unpin objects to and from display, at the slight cost of extra
>> space in every bo.
>>
>> (Inspired and extracted code from a larger rework by Chris Wilson.)
>>
>> v2: Remove the limit since it is not well defined. (Chris Wilson, Ville Syrjälä)
>> v3: Commit message corrections. (Chris Wilson)
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h |  3 ++-
>>   drivers/gpu/drm/i915/i915_gem.c | 30 ++++++------------------------
>>   2 files changed, 8 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 4ef320c..37abd58 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1969,7 +1969,6 @@ struct drm_i915_gem_object {
>>   	 */
>>   	unsigned int fault_mappable:1;
>>   	unsigned int pin_mappable:1;
>> -	unsigned int pin_display:1;
>>
>>   	/*
>>   	 * Is the object to be mapped as read-only to the GPU
>> @@ -1983,6 +1982,8 @@ struct drm_i915_gem_object {
>>
>>   	unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS;
>>
>> +	unsigned int pin_display;
>> +
>
> Here. Comment below.
>
>>   	struct sg_table *pages;
>>   	int pages_pin_count;
>>
>
> How about naming it like the variable just below it (pages_pin_count).
> So let it be "display_pin_count", and make it just "int" too, I don't
> think the 1 bit reduction in pin count matters really, but having sign
> makes it useful for detecting negative pin count errors? At least I'd
> make it signed integer and make some of the spots WARN_ON(xxx--- < 0).

It can't go negative due check in i915_gem_object_unpin_from_display_plane.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Simplify and fix object to display tracking

[Chris Wilson]

On Tue, Mar 31, 2015 at 04:10:05PM +0300, Joonas Lahtinen wrote:
> On ti, 2015-03-31 at 13:55 +0100, Tvrtko Ursulin wrote:
> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > 
> > Purpose of this tracking is to know when to flush the cache between
> > the CPU and the non-coherent display engine. Prior to:
> > 
> >    commit 121920faf2ccce9aa66a7e2588415c9647b66104
> >    Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >    Date:   Mon Mar 23 11:10:37 2015 +0000
> > 
> >        drm/i915/skl: Query display address through a wrapper
> > 
> > This worked by a mix of direct flag manipulation and checking for
> > existence of a pinned GGTT VMA.
> > 
> > With the introduction of rotated display mappings this approach is
> > no longer correct.
> > 
> > New simpler approach is to just keep this count over calls which pin
> > and unpin objects to and from display, at the slight cost of extra
> > space in every bo.
> > 
> > (Inspired and extracted code from a larger rework by Chris Wilson.)
> > 
> > v2: Remove the limit since it is not well defined. (Chris Wilson, Ville Syrjälä)
> > v3: Commit message corrections. (Chris Wilson)
> > 
> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h |  3 ++-
> >  drivers/gpu/drm/i915/i915_gem.c | 30 ++++++------------------------
> >  2 files changed, 8 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 4ef320c..37abd58 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1969,7 +1969,6 @@ struct drm_i915_gem_object {
> >  	 */
> >  	unsigned int fault_mappable:1;
> >  	unsigned int pin_mappable:1;
> > -	unsigned int pin_display:1;
> >  
> >  	/*
> >  	 * Is the object to be mapped as read-only to the GPU
> > @@ -1983,6 +1982,8 @@ struct drm_i915_gem_object {
> >  
> >  	unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS;
> >  
> > +	unsigned int pin_display;
> > +
> 
> Here. Comment below.
> 
> >  	struct sg_table *pages;
> >  	int pages_pin_count;
> >  
> 
> How about naming it like the variable just below it (pages_pin_count).
> So let it be "display_pin_count", and make it just "int" too, I don't
> think the 1 bit reduction in pin count matters really, but having sign
> makes it useful for detecting negative pin count errors? At least I'd
> make it signed integer and make some of the spots WARN_ON(xxx--- < 0).

We already have the counter underflow detection. We could just as well
make pages_pin_count unsigned. I blame my own laziness.
-Chris

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

Re: [PATCH] drm/i915: Simplify and fix object to display tracking

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6103
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  303/303              303/303
SNB                                  304/304              304/304
IVB                                  338/338              338/338
BYT                                  287/287              287/287
HSW                                  361/361              361/361
BDW                                  309/309              309/309
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Simplify and fix object to display tracking

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6104
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -4              272/272              268/272
ILK                                  302/302              302/302
SNB                                  303/303              303/303
IVB                                  338/338              338/338
BYT                                  287/287              287/287
HSW                                  361/361              361/361
BDW                                  308/308              308/308
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 PNV  igt@gem_userptr_blits@coherency-sync      CRASH(2)PASS(3)      CRASH(2)
*PNV  igt@gem_fence_thrash@bo-write-verify-threaded-none      PASS(3)      CRASH(1)PASS(1)
 PNV  igt@gem_tiled_pread_pwrite      FAIL(3)PASS(2)      FAIL(2)
 PNV  igt@gen3_render_tiledx_blits      FAIL(3)PASS(1)      FAIL(2)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Simplify and fix object to display tracking

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6104
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -4              272/272              268/272
ILK                                  302/302              302/302
SNB                                  303/303              303/303
IVB                                  338/338              338/338
BYT                                  287/287              287/287
HSW                                  361/361              361/361
BDW                                  308/308              308/308
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 PNV  igt@gem_userptr_blits@coherency-sync      CRASH(2)PASS(3)      CRASH(2)
*PNV  igt@gem_fence_thrash@bo-write-verify-threaded-none      PASS(3)      CRASH(1)PASS(1)
 PNV  igt@gem_tiled_pread_pwrite      FAIL(3)PASS(2)      FAIL(2)
 PNV  igt@gen3_render_tiledx_blits      FAIL(3)PASS(1)      FAIL(2)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx