Re: [PATCH] drm/i915: Track when an object is pinned for use by the display engine

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

On Fri, Aug 09, 2013 at 12:25:09PM +0100, Chris Wilson wrote:
> The display engine has unique coherency rules such that it requires
> special handling to ensure that all writes to cursors, scanouts and
> sprites are clflushed. This patch introduces the infrastructure to
> simply track when an object is being accessed by the display engine.
> 
> v2: Explain the is_pin_display() magic as the sources for obj->pin_count
> and their individual rules is not obvious. (Ville)
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  |  2 ++
>  drivers/gpu/drm/i915/i915_drv.h      |  2 ++
>  drivers/gpu/drm/i915/i915_gem.c      | 36 ++++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_display.c |  8 ++++----
>  4 files changed, 42 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index b06e6ce..463c660 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -117,6 +117,8 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>  		seq_printf(m, " (name: %d)", obj->base.name);
>  	if (obj->pin_count)
>  		seq_printf(m, " (pinned x %d)", obj->pin_count);
> +	if (obj->pin_display)
> +		seq_printf(m, " (display)");
>  	if (obj->fence_reg != I915_FENCE_REG_NONE)
>  		seq_printf(m, " (fence: %d)", obj->fence_reg);
>  	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 957f22e..a6a1cc3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1379,6 +1379,7 @@ struct drm_i915_gem_object {
>  	 */
>  	unsigned int fault_mappable:1;
>  	unsigned int pin_mappable:1;
> +	unsigned int pin_display:1;
>  
>  	/*
>  	 * Is the GPU currently using a fence to access this buffer,
> @@ -1888,6 +1889,7 @@ int __must_check
>  i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  				     u32 alignment,
>  				     struct intel_ring_buffer *pipelined);
> +void i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj);
>  int i915_gem_attach_phys_object(struct drm_device *dev,
>  				struct drm_i915_gem_object *obj,
>  				int id,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 065f927..3880f05 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3583,6 +3583,22 @@ unlock:
>  	return ret;
>  }
>  
> +static bool is_pin_display(struct drm_i915_gem_object *obj)
> +{
> +	/* There are 3 sources that pin objects:
> +	 *   1. The display engine (scanouts, sprites, cursors);
> +	 *   2. Reservations for execbuffer;
> +	 *   3. The user.
> +	 *
> +	 * We can ignore reservations as we hold the struct_mutex and
> +	 * are only called outside of the reservation path.  The user
> +	 * can only increment pin_count once, and so if after
> +	 * subtracting the potential reference by the user, any pin_count
> +	 * remains, it must be due to another use by the display engine.
> +	 */
> +	return obj->pin_count - !!obj->user_pin_count;
> +}
> +
>  /*
>   * Prepare buffer for display plane (scanout, cursors, etc).
>   * Can be called from an uninterruptible phase (modesetting) and allows
> @@ -3602,6 +3618,11 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  			return ret;
>  	}
>  
> +	/* Mark the pin_display early so that we account for the
> +	 * display coherency whilst setting up the cache domains.
> +	 */
> +	obj->pin_display = true;
> +
>  	/* 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
>  	 * done with uncached PTEs. This is lowest common denominator for all
> @@ -3613,7 +3634,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  	 */
>  	ret = i915_gem_object_set_cache_level(obj, I915_CACHE_NONE);
>  	if (ret)
> -		return ret;
> +		goto err_unpin_display;
>  
>  	/* As the user may map the buffer once pinned in the display plane
>  	 * (e.g. libkms for the bootup splash), we have to ensure that we
> @@ -3621,7 +3642,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  	 */
>  	ret = i915_gem_obj_ggtt_pin(obj, alignment, true, false);
>  	if (ret)
> -		return ret;
> +		goto err_unpin_display;
>  
>  	i915_gem_object_flush_cpu_write_domain(obj);
>  
> @@ -3639,6 +3660,17 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  					    old_write_domain);
>  
>  	return 0;
> +
> +err_unpin_display:
> +	obj->pin_display = is_pin_display(obj);
> +	return ret;
> +}
> +
> +void
> +i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj)
> +{
> +	i915_gem_object_unpin(obj);
> +	obj->pin_display = is_pin_display(obj);
>  }
>  
>  int
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3bbbbe4..809b968 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1887,7 +1887,7 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
>  	return 0;
>  
>  err_unpin:
> -	i915_gem_object_unpin(obj);
> +	i915_gem_object_unpin_from_display_plane(obj);
>  err_interruptible:
>  	dev_priv->mm.interruptible = true;
>  	return ret;
> @@ -1896,7 +1896,7 @@ err_interruptible:
>  void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
>  {
>  	i915_gem_object_unpin_fence(obj);
> -	i915_gem_object_unpin(obj);
> +	i915_gem_object_unpin_from_display_plane(obj);
>  }
>  
>  /* Computes the linear offset to the base tile and adjusts x, y. bytes per pixel
> @@ -6769,7 +6769,7 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  			if (intel_crtc->cursor_bo != obj)
>  				i915_gem_detach_phys_object(dev, intel_crtc->cursor_bo);
>  		} else
> -			i915_gem_object_unpin(intel_crtc->cursor_bo);
> +			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
>  		drm_gem_object_unreference(&intel_crtc->cursor_bo->base);
>  	}
>  
> @@ -6784,7 +6784,7 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  
>  	return 0;
>  fail_unpin:
> -	i915_gem_object_unpin(obj);
> +	i915_gem_object_unpin_from_display_plane(obj);
>  fail_locked:
>  	mutex_unlock(&dev->struct_mutex);
>  fail:
> -- 
> 1.8.4.rc1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC