[Intel-gfx] [PATCH 14/53] drm/i915: Update pin_to_display_plane() to do explicit request management

[Tomas Elf]

On 19/02/2015 17:17, John.C.Harrison at Intel.com wrote:
> From: John Harrison <John.C.Harrison at Intel.com>
>
> Added explicit creation creation and submission of the request structure to the

Nitpick: "creation creation"

> display object pinning code. This removes any reliance on the OLR keeping track
> of the request and the unknown randomness that can ensue with other work
> becoming part of the same request.
>
> v2: Added semaphore enabled check to prevent allocating a pointless request
> structure in the case where the sync just calls wait_rendering().
>
> For: VIZ-5115
> Signed-off-by: John Harrison <John.C.Harrison at Intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c |   21 ++++++++++++++++++---
>   1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 4c29177..5897d54 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3943,9 +3943,24 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>   	int ret;
>
>   	if (pipelined != i915_gem_request_get_ring(obj->last_read_req)) {
> -		ret = i915_gem_object_sync(obj, pipelined);
> -		if (ret)
> -			return ret;
> +		if (!pipelined || !i915_semaphore_is_enabled(obj->base.dev)) {
> +			ret = i915_gem_object_wait_rendering(obj, false);
> +		} else {

The call to i915_gem_object_wait_rendering() was taken from the 
implementation of i915_gem_object_sync() below. Ripping out code from 
the function you're calling just to do it in advance is not very nice. 
Just imagine a scenario where the i915_gem_object_sync() implementation 
were to change but the code outside the function would not. We need to 
figure out a better way of doing this. In fact, allocating and managing 
a request that never gets to be used inside the i915_gem_object_sync() 
function could be a better way to go since it poses less potential 
future problem even though it's more wasteful. But there are probably 
better ways of doing this.

Thanks,
Tomas

> +			struct drm_i915_private *dev_priv = pipelined->dev->dev_private;
> +			struct drm_i915_gem_request *req;
> +
> +			ret = dev_priv->gt.alloc_request(pipelined, pipelined->default_context, &req);
> +			if (ret)
> +				return ret;
> +
> +			ret = i915_gem_object_sync(obj, req->ring);
> +			if (ret)
> +				return ret;
> +
> +			ret = i915_add_request_no_flush(req->ring);
> +			if (ret)
> +				return ret;
> +		}
>   	}
>
>   	/* Mark the pin_display early so that we account for the
>