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

[PATCH 1/5] 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/5] 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

[PATCH 3/5] 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.
v3: Add might_sleep() annotation

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 | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 53c8457869f6..f43121700f83 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1661,14 +1661,22 @@ 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);
+	might_sleep();
+
+	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

[PATCH 4/5] drm/i915: Use __intel_uncore_wait_for_register_fw for sandybride_pcode_read

[Chris Wilson]

Since the sandybridge_pcode_read() may be called from
skl_pcode_request() inside an atomic context (with preempt disabled), we
should avoid hitting any sleeping paths.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 55e1e88cd361..cacb65fa2dd5 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -8135,9 +8135,9 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val
 	I915_WRITE_FW(GEN6_PCODE_DATA1, 0);
 	I915_WRITE_FW(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox);
 
-	if (intel_wait_for_register_fw(dev_priv,
-				       GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
-				       500)) {
+	if (__intel_wait_for_register_fw(dev_priv,
+					 GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
+					 500, 0, NULL)) {
 		DRM_ERROR("timeout waiting for pcode read (%d) to finish\n", mbox);
 		return -ETIMEDOUT;
 	}
@@ -8180,9 +8180,9 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
 	I915_WRITE_FW(GEN6_PCODE_DATA1, 0);
 	I915_WRITE_FW(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox);
 
-	if (intel_wait_for_register_fw(dev_priv,
-				       GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
-				       500)) {
+	if (__intel_wait_for_register_fw(dev_priv,
+					 GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
+					 500, 0, NULL)) {
 		DRM_ERROR("timeout waiting for pcode write (%d) to finish\n", mbox);
 		return -ETIMEDOUT;
 	}
-- 
2.11.0

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

[PATCH 5/5] drm/i915: Use safer intel_uncore_wait_for_register in ring-init

[Chris Wilson]

While we do hold the forcewake for legacy ringbuffer initialisation, we
don't guard our access with the uncore.lock spinlock. In theory, we only
initialise when no others should be accessing the same mmio cachelines,
but in practice be safe as this is an infrequently used path and not
worth risky micro-optimisations.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 331da59a1eb5..97d5fcca8805 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -538,9 +538,9 @@ static int init_ring_common(struct intel_engine_cs *engine)
 	I915_WRITE_CTL(engine, RING_CTL_SIZE(ring->size) | RING_VALID);
 
 	/* If the head is still not zero, the ring is dead */
-	if (intel_wait_for_register_fw(dev_priv, RING_CTL(engine->mmio_base),
-				       RING_VALID, RING_VALID,
-				       50)) {
+	if (intel_wait_for_register(dev_priv, RING_CTL(engine->mmio_base),
+				    RING_VALID, RING_VALID,
+				    50)) {
 		DRM_ERROR("%s initialization failed "
 			  "ctl %08x (valid? %d) head %08x [%08x] tail %08x [%08x] start %08x [expected %08x]\n",
 			  engine->name,
-- 
2.11.0

_______________________________________________
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/5] drm/i915: Stop second guessing the caller for intel_uncore_wait_for_register()

[Patchwork]

== Series Details ==

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

== Summary ==

Series 22845v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/22845/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 -> PASS       (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
74c903cd3 drm/i915: Use safer intel_uncore_wait_for_register in ring-init
0c330db drm/i915: Use __intel_uncore_wait_for_register_fw for sandybride_pcode_read
ff4454d drm/i915: Acquire uncore.lock over intel_uncore_wait_for_register()
0de4038 drm/i915: Stop sleeping from inside gen6_bsd_submit_request()
c70bad3 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_4470/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Michal Wajdeczko]

On Tue, Apr 11, 2017 at 11:13:36AM +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.
>   *
>   * 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)

I still think that we should not try to silently drop fast timeouts longer than 20ms.
Maybe at least -EINVAL or GEM_WARN or something ?

-Michal

>  		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

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

[Michal Wajdeczko]

On Tue, Apr 11, 2017 at 11:13:37AM +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))

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 1/5] drm/i915: Stop second guessing the caller for intel_uncore_wait_for_register()

[Tvrtko Ursulin]

On 11/04/2017 11:13, 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.
>   *
>   * 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)

I agree with Michal here. Kerneldoc even says "must not be larger than 
20ms" so it would be better and completely fine in my opinion to:

	if (GEM_WARN_ON(fast_timeout_us > 20000))
		return -EINVAL;

Hm but it would break the bisectability of the series and break the 
sandybridge pcode.

So patch 4/5 looks broken since it changes the timeout from 500ms to 
500us. I don't see how to fix that without splitting the _fw and atomic 
concepts.

Regards,

Tvrtko

>  		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
>  }
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Tue, Apr 11, 2017 at 11:13:36AM +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.
>   *
>   * 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);

For whatever reason, replies are not in my mbox, so Tvrtko,Michal wrote:

> I agree with Michal here. Kerneldoc even says "must not be larger than 
> 20ms" so it would be better and completely fine in my opinion to:
>
>	if (GEM_WARN_ON(fast_timeout_us > 20000))
>		return -EINVAL;

Not EINVAL, ENODEV if we must. So GEM_BUG_ON(fast_timeout_us > 20000) as
documentation?

We need to keep the fast_timeout_us < X check for gcc.

> Hm but it would break the bisectability of the series and break the 
> sandybridge pcode.

No it doesn't, nobody is passing in such a large *fast_timeout_us*. What
have I missed?

> So patch 4/5 looks broken since it changes the timeout from 500ms to 
> 500us. I don't see how to fix that without splitting the _fw and atomic 
> concepts.

That's not being broken, thats part of the fix. 500ms timeout inside an
atomic section is insane. 500ms timeout outside of that is unwise and
deserves an error of its own. I am quite happy to add that error and
fight that battle later (as BAT is happy, EXT might not be, and users
never).
-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

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

[Tvrtko Ursulin]

On 11/04/2017 12:21, Chris Wilson wrote:
> On Tue, Apr 11, 2017 at 11:13:36AM +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.
>>   *
>>   * 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);
>
> For whatever reason, replies are not in my mbox, so Tvrtko,Michal wrote:
>
>> I agree with Michal here. Kerneldoc even says "must not be larger than
>> 20ms" so it would be better and completely fine in my opinion to:
>>
>> 	if (GEM_WARN_ON(fast_timeout_us > 20000))
>> 		return -EINVAL;
>
> Not EINVAL, ENODEV if we must. So GEM_BUG_ON(fast_timeout_us > 20000) as
> documentation?

Yeah thats fine.

> We need to keep the fast_timeout_us < X check for gcc.
>
>> Hm but it would break the bisectability of the series and break the
>> sandybridge pcode.
>
> No it doesn't, nobody is passing in such a large *fast_timeout_us*. What
> have I missed?

Sounds like I got confused, sorry.

>> So patch 4/5 looks broken since it changes the timeout from 500ms to
>> 500us. I don't see how to fix that without splitting the _fw and atomic
>> concepts.
>
> That's not being broken, thats part of the fix. 500ms timeout inside an
> atomic section is insane. 500ms timeout outside of that is unwise and
> deserves an error of its own. I am quite happy to add that error and
> fight that battle later (as BAT is happy, EXT might not be, and users
> never).

I see. Ok, then, with the GEM_BUG_ON:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko


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

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

[Tvrtko Ursulin]

On 11/04/2017 11:13, 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))
>  		DRM_ERROR("timed out waiting for the BSD ring to wake up\n");
>
>  	/* Now that the ring is fully powered up, update the tail */
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

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

[Tvrtko Ursulin]

On 11/04/2017 11:13, 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.
> v3: Add might_sleep() annotation
>
> 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 | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 53c8457869f6..f43121700f83 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1661,14 +1661,22 @@ 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);
> +	might_sleep();
> +
> +	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);
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

[PATCH v2] 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.

v2: Reinforce kerneldoc fast_timeout_us limit with a GEM_BUG_ON

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>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index eb38392a2435..dded42db880b 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,18 @@ 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);
+	GEM_BUG_ON(fast_timeout_us > 20000);
 
-	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

Re: [PATCH 4/5] drm/i915: Use __intel_uncore_wait_for_register_fw for sandybride_pcode_read

[Tvrtko Ursulin]

On 11/04/2017 11:13, Chris Wilson wrote:
> Since the sandybridge_pcode_read() may be called from
> skl_pcode_request() inside an atomic context (with preempt disabled), we
> should avoid hitting any sleeping paths.

Please update the commit to mention the timeout decrease and with that:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 55e1e88cd361..cacb65fa2dd5 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -8135,9 +8135,9 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val
>  	I915_WRITE_FW(GEN6_PCODE_DATA1, 0);
>  	I915_WRITE_FW(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox);
>
> -	if (intel_wait_for_register_fw(dev_priv,
> -				       GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
> -				       500)) {
> +	if (__intel_wait_for_register_fw(dev_priv,
> +					 GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
> +					 500, 0, NULL)) {
>  		DRM_ERROR("timeout waiting for pcode read (%d) to finish\n", mbox);
>  		return -ETIMEDOUT;
>  	}
> @@ -8180,9 +8180,9 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
>  	I915_WRITE_FW(GEN6_PCODE_DATA1, 0);
>  	I915_WRITE_FW(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox);
>
> -	if (intel_wait_for_register_fw(dev_priv,
> -				       GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
> -				       500)) {
> +	if (__intel_wait_for_register_fw(dev_priv,
> +					 GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
> +					 500, 0, NULL)) {
>  		DRM_ERROR("timeout waiting for pcode write (%d) to finish\n", mbox);
>  		return -ETIMEDOUT;
>  	}
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Use safer intel_uncore_wait_for_register in ring-init

[Tvrtko Ursulin]

On 11/04/2017 11:13, Chris Wilson wrote:
> While we do hold the forcewake for legacy ringbuffer initialisation, we
> don't guard our access with the uncore.lock spinlock. In theory, we only
> initialise when no others should be accessing the same mmio cachelines,
> but in practice be safe as this is an infrequently used path and not
> worth risky micro-optimisations.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 331da59a1eb5..97d5fcca8805 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -538,9 +538,9 @@ static int init_ring_common(struct intel_engine_cs *engine)
>  	I915_WRITE_CTL(engine, RING_CTL_SIZE(ring->size) | RING_VALID);
>
>  	/* If the head is still not zero, the ring is dead */
> -	if (intel_wait_for_register_fw(dev_priv, RING_CTL(engine->mmio_base),
> -				       RING_VALID, RING_VALID,
> -				       50)) {
> +	if (intel_wait_for_register(dev_priv, RING_CTL(engine->mmio_base),
> +				    RING_VALID, RING_VALID,
> +				    50)) {
>  		DRM_ERROR("%s initialization failed "
>  			  "ctl %08x (valid? %d) head %08x [%08x] tail %08x [%08x] start %08x [expected %08x]\n",
>  			  engine->name,
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

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

[Tvrtko Ursulin]

On 11/04/2017 12:27, 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.
>
> v2: Reinforce kerneldoc fast_timeout_us limit with a GEM_BUG_ON
>
> 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>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index eb38392a2435..dded42db880b 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,18 @@ 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);
> +	GEM_BUG_ON(fast_timeout_us > 20000);
>
> -	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
>  }
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

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

[Patchwork]

== Series Details ==

Series: series starting with [v2] drm/i915: Stop second guessing the caller for intel_uncore_wait_for_register() (rev2)
URL   : https://patchwork.freedesktop.org/series/22845/
State : failure

== Summary ==

Series 22845v2 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/22845/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 basic-render:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
        Subgroup gtt-bsd:
                dmesg-warn -> PASS       (fi-snb-2600) fdo#100643
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
        Subgroup gtt-default:
                dmesg-warn -> PASS       (fi-snb-2520m) fdo#100643
        Subgroup readonly-blt:
                dmesg-warn -> PASS       (fi-snb-2520m) 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-busy-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 -> PASS       (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:
WARNING: Long output truncated

a2de738b139a48433b46add849d29db167b7f3b5 drm-tip: 2017y-04m-11d-10h-20m-52s UTC integration manifest
10e6992 drm/i915: Use safer intel_uncore_wait_for_register in ring-init
11be5ec drm/i915: Use __intel_uncore_wait_for_register_fw for sandybride_pcode_read
92fe7b92 drm/i915: Acquire uncore.lock over intel_uncore_wait_for_register()
d0f1a63 drm/i915: Stop sleeping from inside gen6_bsd_submit_request()
5f6586e 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_4471/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Tue, Apr 11, 2017 at 10:30:00AM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [1/5] drm/i915: Stop second guessing the caller for intel_uncore_wait_for_register()
> URL   : https://patchwork.freedesktop.org/series/22845/
> State : success
> 
> == Summary ==
> 
> Series 22845v1 Series without cover letter
> https://patchwork.freedesktop.org/api/1.0/series/22845/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 -> PASS       (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

I am sorry for the fire, and the review is much appreciated. Thank you.

Pushed with the minor changes requested, let's hope the fire stays out.
-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