[PATCH] drm/i915: Remove unecessary per execlist queue item pm reference

[PATCH] drm/i915: Remove unecessary per execlist queue item pm reference

[Nick Hoath]

The execlist code was holding a pm reference for the lifetime of an
execlist queue item. Having this pm reference is required when getting
force wakes during execlists_elsp_write. As this happens inside an interrupt
handler and retrieving the reference can sleep, it can't be done more fine
grained. However, the matching execbuffer already holds a pm reference which
covers this timeframe.

Issue: VIZ-4274
Signed-off-by: Nick Hoath <nicholas.hoath@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 1be836a..dc096de 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -297,7 +297,7 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
 	 *
 	 * The other problem is that we can't just call gen6_gt_force_wake_get()
 	 * because that function calls intel_runtime_pm_get(), which might sleep.
-	 * Instead, we do the runtime_pm_get/put when creating/destroying requests.
+	 * Instead, we rely on the requests pm get/put.
 	 */
 	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
 	if (IS_CHERRYVIEW(dev_priv->dev)) {
@@ -519,8 +519,6 @@ static void execlists_free_request_task(struct work_struct *work)
 	struct drm_device *dev = req->ring->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	intel_runtime_pm_put(dev_priv);
-
 	mutex_lock(&dev->struct_mutex);
 	i915_gem_context_unreference(req->ctx);
 	mutex_unlock(&dev->struct_mutex);
@@ -546,8 +544,6 @@ static int execlists_context_queue(struct intel_engine_cs *ring,
 	req->tail = tail;
 	INIT_WORK(&req->work, execlists_free_request_task);
 
-	intel_runtime_pm_get(dev_priv);
-
 	spin_lock_irqsave(&ring->execlist_lock, flags);
 
 	list_for_each_entry(cursor, &ring->execlist_queue, execlist_link)
-- 
2.1.1

Re: [PATCH] drm/i915: Remove unecessary per execlist queue item pm reference

[Daniel Vetter]

On Mon, Oct 20, 2014 at 11:31:36AM +0100, Nick Hoath wrote:
> The execlist code was holding a pm reference for the lifetime of an
> execlist queue item. Having this pm reference is required when getting
> force wakes during execlists_elsp_write. As this happens inside an interrupt
> handler and retrieving the reference can sleep, it can't be done more fine
> grained. However, the matching execbuffer already holds a pm reference which
> covers this timeframe.

That's not entirely accurate. We keep the gpu out of runtime pm as long as
it's busy, which is done by the intel_mark_busy/idle functions. The
requests themselves don't actually hold a runtime pm reference.

Can you please update commit message and comments?

Also, I don't think this works without also reworking the way we retire
execlist items since atm they're retired in a complete separate work item.
So you can't actually rely on the overal gpu business runtime pm.

I think we need to assemeble all the different pieces of the
request/execlist puzzle into one patch series. Looking at each individual
piece doesn't make a lot of sense to me.

Thanks, Daniel

> 
> Issue: VIZ-4274
> Signed-off-by: Nick Hoath <nicholas.hoath@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 1be836a..dc096de 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -297,7 +297,7 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
>  	 *
>  	 * The other problem is that we can't just call gen6_gt_force_wake_get()
>  	 * because that function calls intel_runtime_pm_get(), which might sleep.
> -	 * Instead, we do the runtime_pm_get/put when creating/destroying requests.
> +	 * Instead, we rely on the requests pm get/put.
>  	 */
>  	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
>  	if (IS_CHERRYVIEW(dev_priv->dev)) {
> @@ -519,8 +519,6 @@ static void execlists_free_request_task(struct work_struct *work)
>  	struct drm_device *dev = req->ring->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	intel_runtime_pm_put(dev_priv);
> -
>  	mutex_lock(&dev->struct_mutex);
>  	i915_gem_context_unreference(req->ctx);
>  	mutex_unlock(&dev->struct_mutex);
> @@ -546,8 +544,6 @@ static int execlists_context_queue(struct intel_engine_cs *ring,
>  	req->tail = tail;
>  	INIT_WORK(&req->work, execlists_free_request_task);
>  
> -	intel_runtime_pm_get(dev_priv);
> -
>  	spin_lock_irqsave(&ring->execlist_lock, flags);
>  
>  	list_for_each_entry(cursor, &ring->execlist_queue, execlist_link)
> -- 
> 2.1.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch