Re: [PATCH 13/20] drm/i915: Teach execbuffer to take the engine wakeref not GT

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

On 18/07/2019 08:00, Chris Wilson wrote:
> In the next patch, we would like to couple into the engine wakeref to
> free the batch pool on idling. The caveat here is that we therefore want
> to track the engine wakeref more precisely and to hold it instead of the
> broader GT wakeref as we process the ioctl.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   .../gpu/drm/i915/gem/i915_gem_execbuffer.c    | 36 ++++++++++++-------
>   drivers/gpu/drm/i915/gt/intel_context.h       |  7 ++++
>   2 files changed, 31 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index cbd7c6e3a1f8..8768d436e54b 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -2138,13 +2138,35 @@ static int eb_pin_context(struct i915_execbuffer *eb, struct intel_context *ce)
>   	if (err)
>   		return err;
>   
> +	/*
> +	 * Take a local wakeref for preparing to dispatch the execbuf as
> +	 * we expect to access the hardware fairly frequently in the
> +	 * process. Upon first dispatch, we acquire another prolonged
> +	 * wakeref that we hold until the GPU has been idle for at least
> +	 * 100ms.

Old comment but slightly not business of this layer to talk about 100ms.

> +	 */
> +	err = intel_context_timeline_lock(ce);
> +	if (err)
> +		goto err_unpin;
> +
> +	intel_context_enter(ce);
> +	intel_context_timeline_unlock(ce);
> +
>   	eb->engine = ce->engine;
>   	eb->context = ce;
>   	return 0;
> +
> +err_unpin:
> +	intel_context_unpin(ce);
> +	return err;
>   }
>   
>   static void eb_unpin_context(struct i915_execbuffer *eb)
>   {
> +	__intel_context_timeline_lock(eb->context);

Slight misuse of established semantics for underscores. (I'd expect no 
locking, or something along those lines, not a difference between 
interruptible and non-interruptible.)

> +	intel_context_exit(eb->context);
> +	intel_context_timeline_unlock(eb->context);
> +
>   	intel_context_unpin(eb->context);
>   }
>   
> @@ -2425,18 +2447,9 @@ i915_gem_do_execbuffer(struct drm_device *dev,
>   	if (unlikely(err))
>   		goto err_destroy;
>   
> -	/*
> -	 * Take a local wakeref for preparing to dispatch the execbuf as
> -	 * we expect to access the hardware fairly frequently in the
> -	 * process. Upon first dispatch, we acquire another prolonged
> -	 * wakeref that we hold until the GPU has been idle for at least
> -	 * 100ms.
> -	 */
> -	intel_gt_pm_get(&eb.i915->gt);
> -
>   	err = i915_mutex_lock_interruptible(dev);
>   	if (err)
> -		goto err_rpm;
> +		goto err_context;
>   
>   	err = eb_select_engine(&eb, file, args);
>   	if (unlikely(err))
> @@ -2601,8 +2614,7 @@ i915_gem_do_execbuffer(struct drm_device *dev,
>   	eb_unpin_context(&eb);
>   err_unlock:
>   	mutex_unlock(&dev->struct_mutex);
> -err_rpm:
> -	intel_gt_pm_put(&eb.i915->gt);
> +err_context:
>   	i915_gem_context_put(eb.gem_context);
>   err_destroy:
>   	eb_destroy(&eb);
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h
> index 23c7e4c0ce7c..485886d84a56 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context.h
> @@ -127,6 +127,13 @@ static inline void intel_context_put(struct intel_context *ce)
>   	kref_put(&ce->ref, ce->ops->destroy);
>   }
>   
> +static inline void
> +__intel_context_timeline_lock(struct intel_context *ce)
> +	__acquires(&ce->ring->timeline->mutex)
> +{
> +	mutex_lock(&ce->ring->timeline->mutex);
> +}
> +
>   static inline int __must_check
>   intel_context_timeline_lock(struct intel_context *ce)
>   	__acquires(&ce->ring->timeline->mutex)
> 

But passable.

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