diff mbox

[v7,04/20] drm/i915: Skip reset request if there is one already

Message ID 20170427231300.32841-5-michel.thierry@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Michel Thierry April 27, 2017, 11:12 p.m. UTC 
From: Mika Kuoppala <mika.kuoppala@linux.intel.com>

To perform engine reset we first disable engine to capture its state. This
is done by issuing a reset request. Because we are reusing existing
infrastructure, again when we actually reset an engine, reset function
checks engine mask and issues reset request again which is unnecessary. To
avoid this we check if the engine is already prepared, if so we just exit
from that point.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Chris Wilson April 29, 2017, 2:21 p.m. UTC | #1 
On Thu, Apr 27, 2017 at 04:12:44PM -0700, Michel Thierry wrote:
> From: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> 
> To perform engine reset we first disable engine to capture its state. This
> is done by issuing a reset request. Because we are reusing existing
> infrastructure, again when we actually reset an engine, reset function
> checks engine mask and issues reset request again which is unnecessary. To
> avoid this we check if the engine is already prepared, if so we just exit
> from that point.

Do we still need this? I am a bit dubious because it implies we have no
idea what we are doing, recursively calling resets.
-Chris
Michel Thierry May 1, 2017, 9:15 p.m. UTC | #2 
On 29/04/17 07:21, Chris Wilson wrote:
> On Thu, Apr 27, 2017 at 04:12:44PM -0700, Michel Thierry wrote:
>> From: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>>
>> To perform engine reset we first disable engine to capture its state. This
>> is done by issuing a reset request. Because we are reusing existing
>> infrastructure, again when we actually reset an engine, reset function
>> checks engine mask and issues reset request again which is unnecessary. To
>> avoid this we check if the engine is already prepared, if so we just exit
>> from that point.
>
> Do we still need this? I am a bit dubious because it implies we have no
> idea what we are doing, recursively calling resets.
> -Chris
>

I can drop this one. It isn't really needed (the 'shortcut' it refers is 
because we already set the bit in intel_reset_engine_start).

btw here it's only setting/querying "Ready-ness for Reset", and I've 
heard rumours that the register may not clear itself sometimes (but I 
haven't seen that behaviour myself).

-Michel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 3ebba6b2dd74..120fb440bb8b 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1686,10 +1686,15 @@  int intel_wait_for_register(struct drm_i915_private *dev_priv,
 static int gen8_reset_engine_start(struct intel_engine_cs *engine)
 {
 	struct drm_i915_private *dev_priv = engine->i915;
+	const i915_reg_t reset_ctrl = RING_RESET_CTL(engine->mmio_base);
+	const u32 ready = RESET_CTL_REQUEST_RESET | RESET_CTL_READY_TO_RESET;
 	int ret;
 
-	I915_WRITE_FW(RING_RESET_CTL(engine->mmio_base),
-		      _MASKED_BIT_ENABLE(RESET_CTL_REQUEST_RESET));
+	/* If engine has been already prepared, we can shortcut here */
+	if ((I915_READ_FW(reset_ctrl) & ready) == ready)
+		return 0;
+
+	I915_WRITE_FW(reset_ctrl, _MASKED_BIT_ENABLE(RESET_CTL_REQUEST_RESET));
 
 	ret = intel_wait_for_register_fw(dev_priv,
 					 RING_RESET_CTL(engine->mmio_base),