Re: [PATCH 2/2] drm/i915: use semaphores for the display plane

[Chris Wilson <chris@chris-wilson.co.uk>]

On Wed, 21 Mar 2012 17:19:13 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> In theory this will have performance and power improvements. Performance
> because we don't need to stall when the scanout BO is busy, and power
> because we don't have to stall when the BO is busy ie. we can get the
> work done sooner and put the CPU to sleep (and one less interrupt
> required).
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem.c |    8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index ce2fee5..96ad162 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3041,9 +3041,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>   * any flushes to be pipelined (for pageflips).
>   *
>   * For the display plane, we want to be in the GTT but out of any write
> - * domains. So in many ways this looks like set_to_gtt_domain() apart from the
> - * ability to pipeline the waits, pinning and any additional subtleties
> - * that may differentiate the display plane from ordinary buffers.
> + * domains. So in many ways this looks like set_to_gtt_domain().
...apart from the whole pinning and pipelining. It looks less like
set-to-gtt-domain over time.

Not an improvement. I'll be the first to admit that it is not a great
comment, but at least it does try to capture why we don't just treat the
display plane as GTT. A better comment would explain our concept of
display plane and pipelining.

>   */
>  int
>  i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> @@ -3058,8 +3056,8 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  		return ret;
>  
>  	if (pipelined != obj->ring) {
> -		ret = i915_gem_object_wait_rendering(obj);
> -		if (ret == -ERESTARTSYS)
> +		ret = i915_gem_object_sync(obj, pipelined);
> +		if (ret)
>  			return ret;
>  	}

This looks eerily familiar.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre