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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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