[13/20] drm/i915: Teach execbuffer to take the engine wakeref not GT
diff mbox series

Message ID 20190718070024.21781-13-chris@chris-wilson.co.uk
State New
Headers show
Series
  • [01/20] drm/i915: Move aliasing_ppgtt underneath its i915_ggtt
Related show

Commit Message

Chris Wilson July 18, 2019, 7 a.m. UTC
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(-)

Comments

Tvrtko Ursulin July 22, 2019, 4:03 p.m. UTC | #1
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

Patch
diff mbox series

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.
+	 */
+	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);
+	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)