Re: [PATCH 1/2] drm/i915: Move releasing of the GEM request from free to retire/cancel

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 19/04/16 13:59, Chris Wilson wrote:
> If we move the release of the GEM request (i.e. decoupling it from the
> various lists used for client and context tracking) after it is complete
> (either by the GPU retiring the request, or by the caller cancelling the
> request), we can remove the requirement that the final unreference of
> the GEM request need to be under the struct_mutex.
>
> v2,v3: Rebalance execlists by moving the context unpinning.
> v4: Rebase onto -nightly
> v5: Avoid trying to rebalance execlist/GuC context pinning, leave that
> to the next step
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_drv.h      | 14 --------------
>   drivers/gpu/drm/i915/i915_gem.c      | 23 +++++++++--------------
>   drivers/gpu/drm/i915/intel_display.c |  2 +-
>   drivers/gpu/drm/i915/intel_pm.c      |  2 +-
>   4 files changed, 11 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c0a6fbff4678..c59b2670cc36 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2370,23 +2370,9 @@ i915_gem_request_reference(struct drm_i915_gem_request *req)
>   static inline void
>   i915_gem_request_unreference(struct drm_i915_gem_request *req)
>   {
> -	WARN_ON(!mutex_is_locked(&req->engine->dev->struct_mutex));
>   	kref_put(&req->ref, i915_gem_request_free);
>   }
>
> -static inline void
> -i915_gem_request_unreference__unlocked(struct drm_i915_gem_request *req)
> -{
> -	struct drm_device *dev;
> -
> -	if (!req)
> -		return;
> -
> -	dev = req->engine->dev;
> -	if (kref_put_mutex(&req->ref, i915_gem_request_free, &dev->struct_mutex))
> -		mutex_unlock(&dev->struct_mutex);
> -}
> -
>   static inline void i915_gem_request_assign(struct drm_i915_gem_request **pdst,
>   					   struct drm_i915_gem_request *src)
>   {
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 261185281e78..9b4854a17264 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1413,6 +1413,13 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request)
>   	list_del_init(&request->list);
>   	i915_gem_request_remove_from_client(request);
>
> +	if (request->ctx) {
> +		if (i915.enable_execlists)
> +			intel_lr_context_unpin(request->ctx, request->engine);
> +
> +		i915_gem_context_unreference(request->ctx);
> +	}
> +
>   	i915_gem_request_unreference(request);
>   }
>
> @@ -2713,18 +2720,6 @@ void i915_gem_request_free(struct kref *req_ref)
>   {
>   	struct drm_i915_gem_request *req = container_of(req_ref,
>   						 typeof(*req), ref);
> -	struct intel_context *ctx = req->ctx;
> -
> -	if (req->file_priv)
> -		i915_gem_request_remove_from_client(req);
> -
> -	if (ctx) {
> -		if (i915.enable_execlists)
> -			intel_lr_context_unpin(ctx, req->engine);
> -
> -		i915_gem_context_unreference(ctx);
> -	}
> -
>   	kmem_cache_free(req->i915->requests, req);
>   }
>
> @@ -3186,7 +3181,7 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>   			ret = __i915_wait_request(req[i], true,
>   						  args->timeout_ns > 0 ? &args->timeout_ns : NULL,
>   						  to_rps_client(file));
> -		i915_gem_request_unreference__unlocked(req[i]);
> +		i915_gem_request_unreference(req[i]);
>   	}
>   	return ret;
>
> @@ -4199,7 +4194,7 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file)
>   	if (ret == 0)
>   		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, 0);
>
> -	i915_gem_request_unreference__unlocked(target);
> +	i915_gem_request_unreference(target);
>
>   	return ret;
>   }
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ff60241b1f76..f5bf46f99cc2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11399,7 +11399,7 @@ static void intel_mmio_flip_work_func(struct work_struct *work)
>   		WARN_ON(__i915_wait_request(mmio_flip->req,
>   					    false, NULL,
>   					    &mmio_flip->i915->rps.mmioflips));
> -		i915_gem_request_unreference__unlocked(mmio_flip->req);
> +		i915_gem_request_unreference(mmio_flip->req);
>   	}
>
>   	/* For framebuffer backed by dmabuf, wait for fence */
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index b7c218602c6e..ed3797bf41aa 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -7366,7 +7366,7 @@ static void __intel_rps_boost_work(struct work_struct *work)
>   		gen6_rps_boost(to_i915(req->engine->dev), NULL,
>   			       req->emitted_jiffies);
>
> -	i915_gem_request_unreference__unlocked(req);
> +	i915_gem_request_unreference(req);
>   	kfree(boost);
>   }
>
>

If I am not too close to this:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx