Re: [PATCH 40/55] drm/i915: Refactor blocking waits

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On ma, 2016-07-25 at 18:32 +0100, Chris Wilson wrote:
> Tidy up the for loops that handle waiting for read/write vs read-only
> access.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 158 +++++++++++++++++++---------------------
>  1 file changed, 75 insertions(+), 83 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 3f6b69dcaccb..2d86a0c3f295 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1339,6 +1339,23 @@ put_rpm:
>  	return ret;
>  }
>  
> +static void
> +i915_gem_object_retire_request(struct drm_i915_gem_object *obj,
> +			       struct drm_i915_gem_request *req)
> +{
> +	int idx = req->engine->id;
> +
> +	if (i915_gem_active_peek(&obj->last_read[idx],
> +				 &obj->base.dev->struct_mutex) == req)
> +		i915_gem_object_retire__read(obj, idx);
> +	else if (i915_gem_active_peek(&obj->last_write,
> +				      &obj->base.dev->struct_mutex) == req)
> +		i915_gem_object_retire__write(obj);

If these functions will use same mutex (be it different than
struct_mutex) in all invocations, I'd make an alias for it.

> +
> +	if (!i915_reset_in_progress(&req->i915->gpu_error))
> +		i915_gem_request_retire_upto(req);
> +}
> +
>  /**
>   * Ensures that all rendering to the object has completed and the object is
>   * safe to unbind from the GTT or access from the CPU.
> @@ -1349,39 +1366,34 @@ int
>  i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj,
>  			       bool readonly)
>  {
> -	struct drm_i915_gem_request *request;
>  	struct reservation_object *resv;
> -	int ret, i;
> +	struct i915_gem_active *active;
> +	unsigned long active_mask;
> +	int idx, ret;
>  

We could do early exit here based on the active_mask, like with the
nonblocking version.

> -	if (readonly) {
> -		request = i915_gem_active_peek(&obj->last_write,
> -					       &obj->base.dev->struct_mutex);
> -		if (request) {
> -			ret = i915_wait_request(request);
> -			if (ret)
> -				return ret;
> +	lockdep_assert_held(&obj->base.dev->struct_mutex);
>  
> -			i = request->engine->id;
> -			if (i915_gem_active_peek(&obj->last_read[i],
> -						 &obj->base.dev->struct_mutex) == request)
> -				i915_gem_object_retire__read(obj, i);
> -			else
> -				i915_gem_object_retire__write(obj);
> -		}
> +	if (!readonly) {

Not sure why not just swap and keep this if (readonly)...

> +		active = obj->last_read;
> +		active_mask = obj->active;
>  	} else {
> -		for (i = 0; i < I915_NUM_ENGINES; i++) {
> -			request = i915_gem_active_peek(&obj->last_read[i],
> -						       &obj->base.dev->struct_mutex);
> -			if (!request)
> -				continue;
> +		active_mask = 1;

Wouldn't we have RENDER_RING define for this and other instances?

With above addressed,

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx