| Message ID | 1413801096-17706-1-git-send-email-nicholas.hoath@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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)
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(-)