diff mbox

drm/i915/skl: Use correct use counters for force wakes

Message ID 1412086082-10144-1-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tvrtko Ursulin Sept. 30, 2014, 2:08 p.m. UTC 
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Write and reads following the block changed use engine specific use counters
and unless that is matched here force wake use counting goes bad. Same
force wake is attempted to be taken twice which leads to at least time outs.

NOTE: Depending on feedback from hardware designers it may not be necessary
to grab force wakes on Gen9 here. But for Gen8 it is needed due to a race
between RC6 and ELSP writes.

v2: Added blitter force wake engine and made more future proof.
    Added commit note.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

Comments

Lespiau, Damien Sept. 30, 2014, 2:19 p.m. UTC | #1 
On Tue, Sep 30, 2014 at 03:08:02PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Write and reads following the block changed use engine specific use counters
> and unless that is matched here force wake use counting goes bad. Same
> force wake is attempted to be taken twice which leads to at least time outs.
> 
> NOTE: Depending on feedback from hardware designers it may not be necessary
> to grab force wakes on Gen9 here. But for Gen8 it is needed due to a race
> between RC6 and ELSP writes.
> 
> v2: Added blitter force wake engine and made more future proof.
>     Added commit note.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> ---

This still has the issue of taking every fw engine, not looking at the
ring we're queuing the work for. Also I'll add the note in a comment
above the whole block. It does solve at least an error in current
kernels so:

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
Chris Wilson Sept. 30, 2014, 2:57 p.m. UTC | #2 
On Tue, Sep 30, 2014 at 03:08:02PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Write and reads following the block changed use engine specific use counters
> and unless that is matched here force wake use counting goes bad. Same
> force wake is attempted to be taken twice which leads to at least time outs.
> 
> NOTE: Depending on feedback from hardware designers it may not be necessary
> to grab force wakes on Gen9 here. But for Gen8 it is needed due to a race
> between RC6 and ELSP writes.
> 
> v2: Added blitter force wake engine and made more future proof.
>     Added commit note.

Speaking of futureproofing, what did you think of my patch to remove the
duplicated counting logic?
-Chris
Tvrtko Ursulin Sept. 30, 2014, 3:58 p.m. UTC | #3 
On 09/30/2014 03:57 PM, Chris Wilson wrote:
> On Tue, Sep 30, 2014 at 03:08:02PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Write and reads following the block changed use engine specific use counters
>> and unless that is matched here force wake use counting goes bad. Same
>> force wake is attempted to be taken twice which leads to at least time outs.
>>
>> NOTE: Depending on feedback from hardware designers it may not be necessary
>> to grab force wakes on Gen9 here. But for Gen8 it is needed due to a race
>> between RC6 and ELSP writes.
>>
>> v2: Added blitter force wake engine and made more future proof.
>>      Added commit note.
>
> Speaking of futureproofing, what did you think of my patch to remove the
> duplicated counting logic?

[For reference it is called "[PATCH] drm/i915: Reduce duplicated 
forcewake logic".]

Disclaimer: I don't know this code that well - only had to dig into it a 
few days back when I hit this bug which resulted in my patch.

But from a glance your patch does make it look cleaner and indeed more 
future proof (much better separation between platform specific and 
generic). Does it need a proper review to move it forward?

Regards,

Tvrtko
Daniel Vetter Sept. 30, 2014, 4:29 p.m. UTC | #4 
On Tue, Sep 30, 2014 at 04:58:54PM +0100, Tvrtko Ursulin wrote:
> 
> 
> On 09/30/2014 03:57 PM, Chris Wilson wrote:
> >On Tue, Sep 30, 2014 at 03:08:02PM +0100, Tvrtko Ursulin wrote:
> >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >>Write and reads following the block changed use engine specific use counters
> >>and unless that is matched here force wake use counting goes bad. Same
> >>force wake is attempted to be taken twice which leads to at least time outs.
> >>
> >>NOTE: Depending on feedback from hardware designers it may not be necessary
> >>to grab force wakes on Gen9 here. But for Gen8 it is needed due to a race
> >>between RC6 and ELSP writes.
> >>
> >>v2: Added blitter force wake engine and made more future proof.
> >>     Added commit note.
> >
> >Speaking of futureproofing, what did you think of my patch to remove the
> >duplicated counting logic?
> 
> [For reference it is called "[PATCH] drm/i915: Reduce duplicated forcewake
> logic".]
> 
> Disclaimer: I don't know this code that well - only had to dig into it a few
> days back when I hit this bug which resulted in my patch.
> 
> But from a glance your patch does make it look cleaner and indeed more
> future proof (much better separation between platform specific and generic).
> Does it need a proper review to move it forward?

Yes. Volunteered?

And when you review pls reply with a blurb in case the existing commit
message from Chris doesn't fully cover the bug you've stumbled over, so
that lazy me can just copy-paste that while applying ;-)

Thanks, Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 3048d78..0792d7a 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -276,7 +276,8 @@  static void execlists_elsp_write(struct intel_engine_cs *ring,
 				 struct drm_i915_gem_object *ctx_obj0,
 				 struct drm_i915_gem_object *ctx_obj1)
 {
-	struct drm_i915_private *dev_priv = ring->dev->dev_private;
+	struct drm_device *dev = ring->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint64_t temp = 0;
 	uint32_t desc[4];
 	unsigned long flags;
@@ -301,13 +302,18 @@  static void execlists_elsp_write(struct intel_engine_cs *ring,
 	 * Instead, we do the runtime_pm_get/put when creating/destroying requests.
 	 */
 	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
-	if (IS_CHERRYVIEW(dev_priv->dev)) {
+	if (IS_CHERRYVIEW(dev) || INTEL_INFO(dev)->gen >= 9) {
 		if (dev_priv->uncore.fw_rendercount++ == 0)
 			dev_priv->uncore.funcs.force_wake_get(dev_priv,
 							      FORCEWAKE_RENDER);
 		if (dev_priv->uncore.fw_mediacount++ == 0)
 			dev_priv->uncore.funcs.force_wake_get(dev_priv,
 							      FORCEWAKE_MEDIA);
+		if (INTEL_INFO(dev)->gen >= 9) {
+			if (dev_priv->uncore.fw_blittercount++ == 0)
+				dev_priv->uncore.funcs.force_wake_get(dev_priv,
+							FORCEWAKE_BLITTER);
+		}
 	} else {
 		if (dev_priv->uncore.forcewake_count++ == 0)
 			dev_priv->uncore.funcs.force_wake_get(dev_priv,
@@ -326,13 +332,18 @@  static void execlists_elsp_write(struct intel_engine_cs *ring,
 
 	/* Release Force Wakeup (see the big comment above). */
 	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
-	if (IS_CHERRYVIEW(dev_priv->dev)) {
+	if (IS_CHERRYVIEW(dev) || INTEL_INFO(dev)->gen >= 9) {
 		if (--dev_priv->uncore.fw_rendercount == 0)
 			dev_priv->uncore.funcs.force_wake_put(dev_priv,
 							      FORCEWAKE_RENDER);
 		if (--dev_priv->uncore.fw_mediacount == 0)
 			dev_priv->uncore.funcs.force_wake_put(dev_priv,
 							      FORCEWAKE_MEDIA);
+		if (INTEL_INFO(dev)->gen >= 9) {
+			if (--dev_priv->uncore.fw_blittercount == 0)
+				dev_priv->uncore.funcs.force_wake_put(dev_priv,
+							FORCEWAKE_BLITTER);
+		}
 	} else {
 		if (--dev_priv->uncore.forcewake_count == 0)
 			dev_priv->uncore.funcs.force_wake_put(dev_priv,