[PATCH 1/3] drm/i915: Stop second guessing the caller for intel_uncore_wait_for_register()

[PATCH 1/3] drm/i915: Stop second guessing the caller for intel_uncore_wait_for_register()

[Chris Wilson]

Allow the caller to use the fast_timeout_us to specify how long to wait
within the atomic section, rather than transparently switching to a
sleeping loop for larger values. This is required as some callsites may
need a long wait and are in an atomic section.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index eb38392a2435..53c8457869f6 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1601,7 +1601,7 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
  *
  * Otherwise, the wait will timeout after @slow_timeout_ms milliseconds.
  * For atomic context @slow_timeout_ms must be zero and @fast_timeout_us
- * must be not larger than 10 microseconds.
+ * must be not larger than 20,0000 microseconds.
  *
  * Note that this routine assumes the caller holds forcewake asserted, it is
  * not suitable for very long waits. See intel_wait_for_register() if you
@@ -1623,16 +1623,17 @@ int __intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
 	int ret;
 
 	/* Catch any overuse of this function */
-	might_sleep_if(fast_timeout_us > 10 || slow_timeout_ms);
+	might_sleep_if(slow_timeout_ms);
 
-	if (fast_timeout_us > 10)
-		ret = _wait_for(done, fast_timeout_us, 10);
-	else
+	ret = -ETIMEDOUT;
+	if (fast_timeout_us && fast_timeout_us < 20000)
 		ret = _wait_for_atomic(done, fast_timeout_us, 0);
 	if (ret)
 		ret = wait_for(done, slow_timeout_ms);
+
 	if (out_value)
 		*out_value = reg_value;
+
 	return ret;
 #undef done
 }
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm/i915: Stop sleeping from inside gen6_bsd_submit_request()

[Chris Wilson]

submit_request() is called from an atomic context, it's not allowed to
sleep. We have to be careful in our parameters to
intel_uncore_wait_for_register() to limit ourselves to the atomic wait
loop and not incur the wrath of our warnings.

Fixes: 6976e74b5fa1 ("drm/i915: Don't allow overuse of __intel_wait_for_register_fw()")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/20170410143807.22725-1-chris@chris-wilson.co.uk
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index c98acc27279a..331da59a1eb5 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1729,11 +1729,11 @@ static void gen6_bsd_submit_request(struct drm_i915_gem_request *request)
 	I915_WRITE64_FW(GEN6_BSD_RNCID, 0x0);
 
 	/* Wait for the ring not to be idle, i.e. for it to wake up. */
-	if (intel_wait_for_register_fw(dev_priv,
-				       GEN6_BSD_SLEEP_PSMI_CONTROL,
-				       GEN6_BSD_SLEEP_INDICATOR,
-				       0,
-				       50))
+	if (__intel_wait_for_register_fw(dev_priv,
+					 GEN6_BSD_SLEEP_PSMI_CONTROL,
+					 GEN6_BSD_SLEEP_INDICATOR,
+					 0,
+					 1000, 0, NULL))
 		DRM_ERROR("timed out waiting for the BSD ring to wake up\n");
 
 	/* Now that the ring is fully powered up, update the tail */
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Stop sleeping from inside gen6_bsd_submit_request()

[Michal Wajdeczko]

On Mon, Apr 10, 2017 at 04:02:06PM +0100, Chris Wilson wrote:
> submit_request() is called from an atomic context, it's not allowed to
> sleep. We have to be careful in our parameters to
> intel_uncore_wait_for_register() to limit ourselves to the atomic wait
> loop and not incur the wrath of our warnings.
> 
> Fixes: 6976e74b5fa1 ("drm/i915: Don't allow overuse of __intel_wait_for_register_fw()")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Link: http://patchwork.freedesktop.org/patch/msgid/20170410143807.22725-1-chris@chris-wilson.co.uk
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index c98acc27279a..331da59a1eb5 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1729,11 +1729,11 @@ static void gen6_bsd_submit_request(struct drm_i915_gem_request *request)
>  	I915_WRITE64_FW(GEN6_BSD_RNCID, 0x0);
>  
>  	/* Wait for the ring not to be idle, i.e. for it to wake up. */
> -	if (intel_wait_for_register_fw(dev_priv,
> -				       GEN6_BSD_SLEEP_PSMI_CONTROL,
> -				       GEN6_BSD_SLEEP_INDICATOR,
> -				       0,
> -				       50))
> +	if (__intel_wait_for_register_fw(dev_priv,
> +					 GEN6_BSD_SLEEP_PSMI_CONTROL,
> +					 GEN6_BSD_SLEEP_INDICATOR,
> +					 0,
> +					 1000, 0, NULL))

Hmm, it's now 50x smaller timeout, but let's hope it's still enough, hence

Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>

-Michal

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Stop sleeping from inside gen6_bsd_submit_request()

[Chris Wilson]

On Mon, Apr 10, 2017 at 05:23:35PM +0200, Michal Wajdeczko wrote:
> On Mon, Apr 10, 2017 at 04:02:06PM +0100, Chris Wilson wrote:
> > submit_request() is called from an atomic context, it's not allowed to
> > sleep. We have to be careful in our parameters to
> > intel_uncore_wait_for_register() to limit ourselves to the atomic wait
> > loop and not incur the wrath of our warnings.
> > 
> > Fixes: 6976e74b5fa1 ("drm/i915: Don't allow overuse of __intel_wait_for_register_fw()")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Link: http://patchwork.freedesktop.org/patch/msgid/20170410143807.22725-1-chris@chris-wilson.co.uk
> > ---
> >  drivers/gpu/drm/i915/intel_ringbuffer.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index c98acc27279a..331da59a1eb5 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -1729,11 +1729,11 @@ static void gen6_bsd_submit_request(struct drm_i915_gem_request *request)
> >  	I915_WRITE64_FW(GEN6_BSD_RNCID, 0x0);
> >  
> >  	/* Wait for the ring not to be idle, i.e. for it to wake up. */
> > -	if (intel_wait_for_register_fw(dev_priv,
> > -				       GEN6_BSD_SLEEP_PSMI_CONTROL,
> > -				       GEN6_BSD_SLEEP_INDICATOR,
> > -				       0,
> > -				       50))
> > +	if (__intel_wait_for_register_fw(dev_priv,
> > +					 GEN6_BSD_SLEEP_PSMI_CONTROL,
> > +					 GEN6_BSD_SLEEP_INDICATOR,
> > +					 0,
> > +					 1000, 0, NULL))
> 
> Hmm, it's now 50x smaller timeout, but let's hope it's still enough, hence

We have already told the device to leave rc6 (we hold forcewake) so this
should be a formality. For the last few months we have been running with
a sleep after 2us, and have never seen the warning about sleeping from
here in CI -- so I think a 1ms (i.e. 500x longer) timeout is safe.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] drm/i915: Acquire uncore.lock over intel_uncore_wait_for_register()

[Chris Wilson]

We acquire the forcewake and use I915_READ_FW instead for the atomic
wait within intel_uncore_wait_for_register. However, this still leaves
us vulnerable to concurrent mmio access to the register, which can cause
system hangs on gen7. The protection is to acquire uncore.lock around
each register, so lets add it back.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 53c8457869f6..d2ba3011c2f0 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1661,14 +1661,18 @@ int intel_wait_for_register(struct drm_i915_private *dev_priv,
 			    u32 value,
 			    unsigned int timeout_ms)
 {
-
 	unsigned fw =
 		intel_uncore_forcewake_for_reg(dev_priv, reg, FW_REG_READ);
 	int ret;
 
-	intel_uncore_forcewake_get(dev_priv, fw);
+	spin_lock_irq(&dev_priv->uncore.lock);
+	intel_uncore_forcewake_get__locked(dev_priv, fw);
+
 	ret = wait_for_us((I915_READ_FW(reg) & mask) == value, 2);
-	intel_uncore_forcewake_put(dev_priv, fw);
+
+	intel_uncore_forcewake_put__locked(dev_priv, fw);
+	spin_unlock_irq(&dev_priv->uncore.lock);
+
 	if (ret)
 		ret = wait_for((I915_READ_NOTRACE(reg) & mask) == value,
 			       timeout_ms);
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Acquire uncore.lock over intel_uncore_wait_for_register()

[Michal Wajdeczko]

On Mon, Apr 10, 2017 at 04:02:07PM +0100, Chris Wilson wrote:
> We acquire the forcewake and use I915_READ_FW instead for the atomic
> wait within intel_uncore_wait_for_register. However, this still leaves
> us vulnerable to concurrent mmio access to the register, which can cause
> system hangs on gen7. The protection is to acquire uncore.lock around
> each register, so lets add it back.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 53c8457869f6..d2ba3011c2f0 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1661,14 +1661,18 @@ int intel_wait_for_register(struct drm_i915_private *dev_priv,
>  			    u32 value,
>  			    unsigned int timeout_ms)
>  {
> -
>  	unsigned fw =
>  		intel_uncore_forcewake_for_reg(dev_priv, reg, FW_REG_READ);
>  	int ret;
>  
> -	intel_uncore_forcewake_get(dev_priv, fw);
> +	spin_lock_irq(&dev_priv->uncore.lock);
> +	intel_uncore_forcewake_get__locked(dev_priv, fw);
> +
>  	ret = wait_for_us((I915_READ_FW(reg) & mask) == value, 2);

To further decrease driver size we may try to call here our helper function:

	ret = __intel_wait_for_register_fw(dev_priv, reg, mask, value, 2, 0, NULL);

-Michal

> -	intel_uncore_forcewake_put(dev_priv, fw);
> +
> +	intel_uncore_forcewake_put__locked(dev_priv, fw);
> +	spin_unlock_irq(&dev_priv->uncore.lock);
> +
>  	if (ret)
>  		ret = wait_for((I915_READ_NOTRACE(reg) & mask) == value,
>  			       timeout_ms);
> -- 
> 2.11.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915: Acquire uncore.lock over intel_uncore_wait_for_register()

[Chris Wilson]

We acquire the forcewake and use I915_READ_FW instead for the atomic
wait within intel_uncore_wait_for_register. However, this still leaves
us vulnerable to concurrent mmio access to the register, which can cause
system hangs on gen7. The protection is to acquire uncore.lock around
each register, so lets add it back.

v2: Wrap __intel_wait_for_register_fw() to re-use its atomic wait_for
loop and spare adding another for ourselves.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 53c8457869f6..01cea3b7a704 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1661,14 +1661,20 @@ int intel_wait_for_register(struct drm_i915_private *dev_priv,
 			    u32 value,
 			    unsigned int timeout_ms)
 {
-
 	unsigned fw =
 		intel_uncore_forcewake_for_reg(dev_priv, reg, FW_REG_READ);
 	int ret;
 
-	intel_uncore_forcewake_get(dev_priv, fw);
-	ret = wait_for_us((I915_READ_FW(reg) & mask) == value, 2);
-	intel_uncore_forcewake_put(dev_priv, fw);
+	spin_lock_irq(&dev_priv->uncore.lock);
+	intel_uncore_forcewake_get__locked(dev_priv, fw);
+
+	ret = __intel_wait_for_register_fw(dev_priv,
+					   reg, mask, value,
+					   2, 0, NULL);
+
+	intel_uncore_forcewake_put__locked(dev_priv, fw);
+	spin_unlock_irq(&dev_priv->uncore.lock);
+
 	if (ret)
 		ret = wait_for((I915_READ_NOTRACE(reg) & mask) == value,
 			       timeout_ms);
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Acquire uncore.lock over intel_uncore_wait_for_register()

[Michal Wajdeczko]

On Mon, Apr 10, 2017 at 04:55:31PM +0100, Chris Wilson wrote:
> We acquire the forcewake and use I915_READ_FW instead for the atomic
> wait within intel_uncore_wait_for_register. However, this still leaves
> us vulnerable to concurrent mmio access to the register, which can cause
> system hangs on gen7. The protection is to acquire uncore.lock around
> each register, so lets add it back.
> 
> v2: Wrap __intel_wait_for_register_fw() to re-use its atomic wait_for
> loop and spare adding another for ourselves.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 53c8457869f6..01cea3b7a704 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1661,14 +1661,20 @@ int intel_wait_for_register(struct drm_i915_private *dev_priv,
>  			    u32 value,
>  			    unsigned int timeout_ms)
>  {
> -
>  	unsigned fw =
>  		intel_uncore_forcewake_for_reg(dev_priv, reg, FW_REG_READ);
>  	int ret;
>  
> -	intel_uncore_forcewake_get(dev_priv, fw);
> -	ret = wait_for_us((I915_READ_FW(reg) & mask) == value, 2);
> -	intel_uncore_forcewake_put(dev_priv, fw);
> +	spin_lock_irq(&dev_priv->uncore.lock);
> +	intel_uncore_forcewake_get__locked(dev_priv, fw);
> +
> +	ret = __intel_wait_for_register_fw(dev_priv,
> +					   reg, mask, value,
> +					   2, 0, NULL);
> +
> +	intel_uncore_forcewake_put__locked(dev_priv, fw);
> +	spin_unlock_irq(&dev_priv->uncore.lock);

Hmm, shouldn't we use spin_lock_irqsave/spin_unlock_irqrestore here?
Like in GEN6_READ_HEADER/GEN6_READ_FOOTER.

-Michal

> +
>  	if (ret)
>  		ret = wait_for((I915_READ_NOTRACE(reg) & mask) == value,
>  			       timeout_ms);
> -- 
> 2.11.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Stop second guessing the caller for intel_uncore_wait_for_register()

[Michal Wajdeczko]

On Mon, Apr 10, 2017 at 04:02:05PM +0100, Chris Wilson wrote:
> Allow the caller to use the fast_timeout_us to specify how long to wait
> within the atomic section, rather than transparently switching to a
> sleeping loop for larger values. This is required as some callsites may
> need a long wait and are in an atomic section.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index eb38392a2435..53c8457869f6 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1601,7 +1601,7 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
>   *
>   * Otherwise, the wait will timeout after @slow_timeout_ms milliseconds.
>   * For atomic context @slow_timeout_ms must be zero and @fast_timeout_us
> - * must be not larger than 10 microseconds.
> + * must be not larger than 20,0000 microseconds.

So we don't enforce this any more ?

>   *
>   * Note that this routine assumes the caller holds forcewake asserted, it is
>   * not suitable for very long waits. See intel_wait_for_register() if you
> @@ -1623,16 +1623,17 @@ int __intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
>  	int ret;
>  
>  	/* Catch any overuse of this function */
> -	might_sleep_if(fast_timeout_us > 10 || slow_timeout_ms);
> +	might_sleep_if(slow_timeout_ms);
>  
> -	if (fast_timeout_us > 10)
> -		ret = _wait_for(done, fast_timeout_us, 10);
> -	else
> +	ret = -ETIMEDOUT;
> +	if (fast_timeout_us && fast_timeout_us < 20000)
>  		ret = _wait_for_atomic(done, fast_timeout_us, 0);
>  	if (ret)
>  		ret = wait_for(done, slow_timeout_ms);

What if someone passes fast=20001us and slow=0ms ?
Maybe like this:

	might_sleep_if(fast_timeout_us > 20000 || slow_timeout_ms);

	if (fast_timeout_us && fast_timeout_us < 20000)
		ret = _wait_for_atomic(done, fast_timeout_us, 0);
	else
		slow_timeout_ms += fast_timeout_us/1000;

	if (slow_timeout_ms)
		ret = wait_for(done, slow_timeout_ms);


-Michal
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Stop second guessing the caller for intel_uncore_wait_for_register()

[Chris Wilson]

On Mon, Apr 10, 2017 at 05:20:32PM +0200, Michal Wajdeczko wrote:
> On Mon, Apr 10, 2017 at 04:02:05PM +0100, Chris Wilson wrote:
> > Allow the caller to use the fast_timeout_us to specify how long to wait
> > within the atomic section, rather than transparently switching to a
> > sleeping loop for larger values. This is required as some callsites may
> > need a long wait and are in an atomic section.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_uncore.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index eb38392a2435..53c8457869f6 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -1601,7 +1601,7 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
> >   *
> >   * Otherwise, the wait will timeout after @slow_timeout_ms milliseconds.
> >   * For atomic context @slow_timeout_ms must be zero and @fast_timeout_us
> > - * must be not larger than 10 microseconds.
> > + * must be not larger than 20,0000 microseconds.
> 
> So we don't enforce this any more ?

I considered adding a GEM_BUG_ON / GEM_WARN_ON, but really just not
listening to them and hitting the sleep was my answer to sillyness.

Atomic spinning for 20ms is not acceptable. :|

> >   * Note that this routine assumes the caller holds forcewake asserted, it is
> >   * not suitable for very long waits. See intel_wait_for_register() if you
> > @@ -1623,16 +1623,17 @@ int __intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
> >  	int ret;
> >  
> >  	/* Catch any overuse of this function */
> > -	might_sleep_if(fast_timeout_us > 10 || slow_timeout_ms);
> > +	might_sleep_if(slow_timeout_ms);
> >  
> > -	if (fast_timeout_us > 10)
> > -		ret = _wait_for(done, fast_timeout_us, 10);
> > -	else
> > +	ret = -ETIMEDOUT;
> > +	if (fast_timeout_us && fast_timeout_us < 20000)
> >  		ret = _wait_for_atomic(done, fast_timeout_us, 0);
> >  	if (ret)
> >  		ret = wait_for(done, slow_timeout_ms);
> 
> What if someone passes fast=20001us and slow=0ms ?
> Maybe like this:
> 
> 	might_sleep_if(fast_timeout_us > 20000 || slow_timeout_ms);
> 
> 	if (fast_timeout_us && fast_timeout_us < 20000)
> 		ret = _wait_for_atomic(done, fast_timeout_us, 0);
> 	else
> 		slow_timeout_ms += fast_timeout_us/1000;

No. Just shoot it down in review.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Stop second guessing the caller for intel_uncore_wait_for_register()

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: Stop second guessing the caller for intel_uncore_wait_for_register()
URL   : https://patchwork.freedesktop.org/series/22786/
State : success

== Summary ==

Series 22786v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/22786/revisions/1/mbox/

Test core_auth:
        Subgroup basic-auth:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
Test drv_getparams_basic:
        Subgroup basic-subslice-total:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
Test drv_module_reload:
        Subgroup basic-no-display:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
        Subgroup basic-reload:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
        Subgroup basic-reload-final:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
Test gem_basic:
        Subgroup create-close:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
Test gem_busy:
        Subgroup basic-hang-default:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
Test gem_close_race:
        Subgroup basic-threads:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
Test gem_cs_tlb:
        Subgroup basic-default:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
Test gem_ctx_basic:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
Test gem_ctx_create:
        Subgroup basic-files:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
Test gem_ctx_param:
        Subgroup basic-default:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
Test gem_ctx_switch:
        Subgroup basic-default:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
        Subgroup basic-default-heavy:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
Test gem_exec_basic:
        Subgroup gtt-render:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
        Subgroup readonly-bsd:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
        Subgroup readonly-default:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
        Subgroup readonly-render:
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
Test gem_exec_create:
        Subgroup basic:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
Test gem_exec_fence:
        Subgroup await-hang-default:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
        Subgroup basic-await-default:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
        Subgroup basic-wait-default:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
        Subgroup nb-await-default:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
                dmesg-warn -> FAIL       (fi-snb-2600) fdo#100643
        Subgroup basic-batch-kernel-default-wb:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
        Subgroup basic-uc-pro-default:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
        Subgroup basic-uc-prw-default:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
        Subgroup basic-uc-ro-default:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
        Subgroup basic-uc-rw-default:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
        Subgroup basic-uc-set-default:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
        Subgroup basic-wb-pro-default:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
WARNING: Long output truncated

e461ecfd413fb930d00f44f3de0019e528b4731f drm-tip: 2017y-04m-10d-14h-25m-24s UTC integration manifest
757edaa drm/i915: Acquire uncore.lock over intel_uncore_wait_for_register()
b6cd508 drm/i915: Stop sleeping from inside gen6_bsd_submit_request()
1558d0d drm/i915: Stop second guessing the caller for intel_uncore_wait_for_register()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4463/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for series starting with [1/3] drm/i915: Stop second guessing the caller for intel_uncore_wait_for_register() (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: Stop second guessing the caller for intel_uncore_wait_for_register() (rev2)
URL   : https://patchwork.freedesktop.org/series/22786/
State : warning

== Summary ==

Series 22786v2 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/22786/revisions/2/mbox/

Test core_auth:
        Subgroup basic-auth:
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
Test drv_getparams_basic:
        Subgroup basic-subslice-total:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
Test drv_module_reload:
        Subgroup basic-no-display:
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
        Subgroup basic-reload:
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
        Subgroup basic-reload-final:
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
Test gem_basic:
        Subgroup create-close:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
Test gem_busy:
        Subgroup basic-hang-default:
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
Test gem_close_race:
        Subgroup basic-threads:
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
Test gem_cs_tlb:
        Subgroup basic-default:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
Test gem_ctx_basic:
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
Test gem_ctx_create:
        Subgroup basic-files:
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
Test gem_ctx_param:
        Subgroup basic-default:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
Test gem_ctx_switch:
        Subgroup basic-default:
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
        Subgroup basic-default-heavy:
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
Test gem_exec_basic:
        Subgroup gtt-render:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
        Subgroup readonly-bsd:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
        Subgroup readonly-default:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
        Subgroup readonly-render:
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
Test gem_exec_create:
        Subgroup basic:
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
Test gem_exec_fence:
        Subgroup await-hang-default:
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
        Subgroup basic-await-default:
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
        Subgroup basic-wait-default:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
        Subgroup nb-await-default:
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                dmesg-warn -> FAIL       (fi-snb-2600) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
        Subgroup basic-batch-kernel-default-wb:
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
        Subgroup basic-uc-pro-default:
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
        Subgroup basic-uc-prw-default:
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
        Subgroup basic-uc-ro-default:
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
        Subgroup basic-uc-rw-default:
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
        Subgroup basic-uc-set-default:
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
        Subgroup basic-wb-pro-default:
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
WARNING: Long output truncated

e461ecfd413fb930d00f44f3de0019e528b4731f drm-tip: 2017y-04m-10d-14h-25m-24s UTC integration manifest
bd1d3ec drm/i915: Acquire uncore.lock over intel_uncore_wait_for_register()
0bdd1c1 drm/i915: Stop sleeping from inside gen6_bsd_submit_request()
c83f560 drm/i915: Stop second guessing the caller for intel_uncore_wait_for_register()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4465/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: warning for series starting with [1/3] drm/i915: Stop second guessing the caller for intel_uncore_wait_for_register() (rev2)

[Chris Wilson]

On Mon, Apr 10, 2017 at 04:23:15PM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [1/3] drm/i915: Stop second guessing the caller for intel_uncore_wait_for_register() (rev2)
> URL   : https://patchwork.freedesktop.org/series/22786/
> State : warning
> 
> == Summary ==
> 
> Series 22786v2 Series without cover letter
> https://patchwork.freedesktop.org/api/1.0/series/22786/revisions/2/mbox/
> 
> Test core_auth:
>         Subgroup basic-auth:
>                 dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
>                 dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
> Test drv_getparams_basic:
>         Subgroup basic-subslice-total:
>                 dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
> Test drv_module_reload:
>         Subgroup basic-no-display:
>                 dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
>                 dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
>         Subgroup basic-reload:
>                 dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
>                 dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
>         Subgroup basic-reload-final:
>                 dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
>                 dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
> Test gem_basic:
>         Subgroup create-close:
>                 dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
> Test gem_busy:
>         Subgroup basic-hang-default:
>                 dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
>                 dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
> Test gem_close_race:
>         Subgroup basic-threads:
>                 dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
>                 dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
> Test gem_cs_tlb:
>         Subgroup basic-default:
>                 dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
> Test gem_ctx_basic:
>                 dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
>                 dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
> Test gem_ctx_create:
>         Subgroup basic-files:
>                 dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
>                 dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
> Test gem_ctx_param:
>         Subgroup basic-default:
>                 dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
> Test gem_ctx_switch:
>         Subgroup basic-default:
>                 dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
>                 dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
>         Subgroup basic-default-heavy:
>                 dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
>                 dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
> Test gem_exec_basic:
>         Subgroup gtt-render:
>                 dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
>         Subgroup readonly-bsd:
>                 dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
>         Subgroup readonly-default:
>                 dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
>         Subgroup readonly-render:
>                 dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
> Test gem_exec_create:
>         Subgroup basic:
>                 dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
>                 dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
> Test gem_exec_fence:
>         Subgroup await-hang-default:
>                 dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
>                 dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
>         Subgroup basic-await-default:
>                 dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
>                 dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
>         Subgroup basic-wait-default:
>                 dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
>         Subgroup nb-await-default:
>                 dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
>                 dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
> Test gem_exec_flush:
>         Subgroup basic-batch-kernel-default-uc:
>                 dmesg-warn -> FAIL       (fi-snb-2600) fdo#100643
>                 dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
>         Subgroup basic-batch-kernel-default-wb:
>                 dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
>                 dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
>         Subgroup basic-uc-pro-default:
>                 dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
>                 dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
>         Subgroup basic-uc-prw-default:
>                 dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
>                 dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
>         Subgroup basic-uc-ro-default:
>                 dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
>                 dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
>         Subgroup basic-uc-rw-default:
>                 dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
>                 dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
>         Subgroup basic-uc-set-default:
>                 dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
>                 dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
>         Subgroup basic-wb-pro-default:
>                 dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
> WARNING: Long output truncated

Ping?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx