[PATCH 1/3] drm/i915: Use intel_engine_stop_cs when stopping ringbuffer

[PATCH 1/3] drm/i915: Use intel_engine_stop_cs when stopping ringbuffer

[Mika Kuoppala]

We have an exported function for stopping the engine before
disabling a ringbuffer. Take it into use.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_engine_cs.c  |  3 +++
 drivers/gpu/drm/i915/intel_ringbuffer.c | 31 +++++++++++--------------
 2 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 4f244019560d..3feb0f74c239 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -817,6 +817,9 @@ void intel_engine_cancel_stop_cs(struct intel_engine_cs *engine)
 {
 	struct drm_i915_private *dev_priv = engine->i915;
 
+	if (INTEL_GEN(dev_priv) < 3)
+		return;
+
 	GEM_TRACE("%s\n", engine->name);
 
 	I915_WRITE_FW(RING_MI_MODE(engine->mmio_base),
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 1b96b0960adc..5363dad1208d 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -607,23 +607,19 @@ static void ring_setup_status_page(struct intel_engine_cs *engine)
 static bool stop_ring(struct intel_engine_cs *engine)
 {
 	struct drm_i915_private *dev_priv = engine->i915;
+	int ret;
 
-	if (INTEL_GEN(dev_priv) > 2) {
-		I915_WRITE_MODE(engine, _MASKED_BIT_ENABLE(STOP_RING));
-		if (intel_wait_for_register(dev_priv,
-					    RING_MI_MODE(engine->mmio_base),
-					    MODE_IDLE,
-					    MODE_IDLE,
-					    1000)) {
-			DRM_ERROR("%s : timed out trying to stop ring\n",
-				  engine->name);
-			/* Sometimes we observe that the idle flag is not
-			 * set even though the ring is empty. So double
-			 * check before giving up.
-			 */
-			if (I915_READ_HEAD(engine) != I915_READ_TAIL(engine))
-				return false;
-		}
+	ret = intel_engine_stop_cs(engine);
+	if (ret == -ENODEV)
+		ret = 0;
+
+	if (ret) {
+		/* Sometimes we observe that the idle flag is not
+		 * set even though the ring is empty. So double
+		 * check before giving up.
+		 */
+		if (I915_READ_HEAD(engine) != I915_READ_TAIL(engine))
+			return false;
 	}
 
 	I915_WRITE_HEAD(engine, I915_READ_TAIL(engine));
@@ -718,8 +714,7 @@ static int init_ring_common(struct intel_engine_cs *engine)
 		goto out;
 	}
 
-	if (INTEL_GEN(dev_priv) > 2)
-		I915_WRITE_MODE(engine, _MASKED_BIT_DISABLE(STOP_RING));
+	intel_engine_cancel_stop_cs(engine);
 
 	/* Now awake, let it get started */
 	if (ring->tail != ring->head) {
-- 
2.17.1

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

[PATCH 2/3] drm/i915: Introduce intel_engine_stop_ringbuffer

[Mika Kuoppala]

We use identical sequence of stopping ringbuffer on reset
handing and on ring initialization. Make a function
to handle both cases.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reset.c       | 18 +---------------
 drivers/gpu/drm/i915/intel_engine_cs.c  | 28 +++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_ringbuffer.c | 10 ++-------
 drivers/gpu/drm/i915/intel_ringbuffer.h |  2 ++
 4 files changed, 33 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reset.c b/drivers/gpu/drm/i915/i915_reset.c
index 55d6123dbba4..4191398b5125 100644
--- a/drivers/gpu/drm/i915/i915_reset.c
+++ b/drivers/gpu/drm/i915/i915_reset.c
@@ -113,28 +113,12 @@ void i915_reset_request(struct i915_request *rq, bool guilty)
 
 static void gen3_stop_engine(struct intel_engine_cs *engine)
 {
-	struct drm_i915_private *dev_priv = engine->i915;
-	const u32 base = engine->mmio_base;
-
 	GEM_TRACE("%s\n", engine->name);
 
 	if (intel_engine_stop_cs(engine))
 		GEM_TRACE("%s: timed out on STOP_RING\n", engine->name);
 
-	I915_WRITE_FW(RING_HEAD(base), I915_READ_FW(RING_TAIL(base)));
-	POSTING_READ_FW(RING_HEAD(base)); /* paranoia */
-
-	I915_WRITE_FW(RING_HEAD(base), 0);
-	I915_WRITE_FW(RING_TAIL(base), 0);
-	POSTING_READ_FW(RING_TAIL(base));
-
-	/* The ring must be empty before it is disabled */
-	I915_WRITE_FW(RING_CTL(base), 0);
-
-	/* Check acts as a post */
-	if (I915_READ_FW(RING_HEAD(base)))
-		GEM_TRACE("%s: ring head [%x] not parked\n",
-			  engine->name, I915_READ_FW(RING_HEAD(base)));
+	intel_engine_stop_ringbuffer(engine);
 }
 
 static void i915_stop_engines(struct drm_i915_private *i915,
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 3feb0f74c239..a8e47cfa6e35 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -826,6 +826,34 @@ void intel_engine_cancel_stop_cs(struct intel_engine_cs *engine)
 		      _MASKED_BIT_DISABLE(STOP_RING));
 }
 
+int intel_engine_stop_ringbuffer(struct intel_engine_cs *engine)
+{
+	struct drm_i915_private *dev_priv = engine->i915;
+	const u32 base = engine->mmio_base;
+
+	assert_forcewakes_active(dev_priv, FORCEWAKE_ALL);
+	GEM_TRACE("%s\n", engine->name);
+
+	I915_WRITE_FW(RING_HEAD(base), I915_READ_FW(RING_TAIL(base)));
+	POSTING_READ_FW(RING_HEAD(base)); /* paranoia */
+
+	I915_WRITE_FW(RING_HEAD(base), 0);
+	I915_WRITE_FW(RING_TAIL(base), 0);
+	POSTING_READ_FW(RING_TAIL(base));
+
+	/* The ring must be empty before it is disabled */
+	I915_WRITE_FW(RING_CTL(base), 0);
+
+	/* Check acts as a post */
+	if (I915_READ_FW(RING_HEAD(base))) {
+		GEM_TRACE("%s: ring head [%x] not parked\n",
+			  engine->name, I915_READ_FW(RING_HEAD(base)));
+		return -EIO;
+	}
+
+	return 0;
+}
+
 const char *i915_cache_level_str(struct drm_i915_private *i915, int type)
 {
 	switch (type) {
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 5363dad1208d..8936a9051760 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -622,15 +622,9 @@ static bool stop_ring(struct intel_engine_cs *engine)
 			return false;
 	}
 
-	I915_WRITE_HEAD(engine, I915_READ_TAIL(engine));
+	ret = intel_engine_stop_ringbuffer(engine);
 
-	I915_WRITE_HEAD(engine, 0);
-	I915_WRITE_TAIL(engine, 0);
-
-	/* The ring must be empty before it is disabled */
-	I915_WRITE_CTL(engine, 0);
-
-	return (I915_READ_HEAD(engine) & HEAD_ADDR) == 0;
+	return ret == 0;
 }
 
 static int init_ring_common(struct intel_engine_cs *engine)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index de8dba7565b0..10eb0f9a1432 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -858,6 +858,8 @@ int intel_init_vebox_ring_buffer(struct intel_engine_cs *engine);
 int intel_engine_stop_cs(struct intel_engine_cs *engine);
 void intel_engine_cancel_stop_cs(struct intel_engine_cs *engine);
 
+int intel_engine_stop_ringbuffer(struct intel_engine_cs *engine);
+
 void intel_engine_set_hwsp_writemask(struct intel_engine_cs *engine, u32 mask);
 
 u64 intel_engine_get_active_head(const struct intel_engine_cs *engine);
-- 
2.17.1

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

[PATCH 3/3] drm/i915: Disable PSMI idle messaging when stopping ring

[Mika Kuoppala]

Hardware cannot be in a middle of idle flow messaging
when we pull the plug from ringbuffer. Disable idle
messaging before we do so to avoid potential deadlock
on engine initialization and reset.

v2: INVALID_MMIO_REG, unconditional enable (Chris)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_engine_cs.c | 56 +++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index a8e47cfa6e35..fe7fca392b63 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -725,7 +725,7 @@ int intel_engine_init_common(struct intel_engine_cs *engine)
 
 /**
  * intel_engines_cleanup_common - cleans up the engine state created by
- *                                the common initiailizers.
+ *                                the common initializers.
  * @engine: Engine to cleanup.
  *
  * This cleans up everything created by the common helpers.
@@ -826,6 +826,55 @@ void intel_engine_cancel_stop_cs(struct intel_engine_cs *engine)
 		      _MASKED_BIT_DISABLE(STOP_RING));
 }
 
+static i915_reg_t get_idle_poll_reg(const struct intel_engine_cs *engine)
+{
+	if (engine->id != RCS)
+		return INVALID_MMIO_REG;
+
+	if (IS_GEN(engine->i915, 9))
+		return GEN9_RCS_FE_FSM2;
+
+	if (IS_GEN(engine->i915, 8))
+		return GEN6_RCS_PWR_FSM;
+
+	return INVALID_MMIO_REG;
+}
+
+static void disable_idle_messaging(struct intel_engine_cs *engine)
+{
+	struct drm_i915_private *dev_priv = engine->i915;
+	i915_reg_t poll_reg;
+
+	poll_reg = get_idle_poll_reg(engine);
+	if (!i915_mmio_reg_valid(poll_reg))
+		return;
+
+	GEM_DEBUG_WARN_ON(I915_READ_FW(GEN6_RC_SLEEP_PSMI_CONTROL) &
+			  GEN6_PSMI_SLEEP_MSG_DISABLE);
+
+	I915_WRITE_FW(GEN6_RC_SLEEP_PSMI_CONTROL,
+		      _MASKED_BIT_ENABLE(GEN6_PSMI_SLEEP_MSG_DISABLE));
+
+	if (__intel_wait_for_register_fw(dev_priv, poll_reg,
+					 0x7f, 0x30,
+					 5000, 0,
+					 NULL))
+		DRM_DEBUG_DRIVER("psmi idle msg poll timeout\n");
+}
+
+static void enable_idle_messaging(struct intel_engine_cs *engine)
+{
+	struct drm_i915_private *dev_priv = engine->i915;
+	i915_reg_t poll_reg;
+
+	poll_reg = get_idle_poll_reg(engine);
+	if (!i915_mmio_reg_valid(poll_reg))
+		return;
+
+	I915_WRITE_FW(GEN6_RC_SLEEP_PSMI_CONTROL,
+		      _MASKED_BIT_DISABLE(GEN6_PSMI_SLEEP_MSG_DISABLE));
+}
+
 int intel_engine_stop_ringbuffer(struct intel_engine_cs *engine)
 {
 	struct drm_i915_private *dev_priv = engine->i915;
@@ -841,9 +890,14 @@ int intel_engine_stop_ringbuffer(struct intel_engine_cs *engine)
 	I915_WRITE_FW(RING_TAIL(base), 0);
 	POSTING_READ_FW(RING_TAIL(base));
 
+	/* Idle messaging needs to be off during ring disable */
+	disable_idle_messaging(engine);
+
 	/* The ring must be empty before it is disabled */
 	I915_WRITE_FW(RING_CTL(base), 0);
 
+	enable_idle_messaging(engine);
+
 	/* Check acts as a post */
 	if (I915_READ_FW(RING_HEAD(base))) {
 		GEM_TRACE("%s: ring head [%x] not parked\n",
-- 
2.17.1

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

Re: [PATCH 1/3] drm/i915: Use intel_engine_stop_cs when stopping ringbuffer

[Mika Kuoppala]

Mika Kuoppala <mika.kuoppala@linux.intel.com> writes:

> We have an exported function for stopping the engine before
> disabling a ringbuffer. Take it into use.
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_engine_cs.c  |  3 +++
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 31 +++++++++++--------------
>  2 files changed, 16 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 4f244019560d..3feb0f74c239 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -817,6 +817,9 @@ void intel_engine_cancel_stop_cs(struct intel_engine_cs *engine)
>  {
>  	struct drm_i915_private *dev_priv = engine->i915;
>  
> +	if (INTEL_GEN(dev_priv) < 3)
> +		return;
> +
>  	GEM_TRACE("%s\n", engine->name);
>  
>  	I915_WRITE_FW(RING_MI_MODE(engine->mmio_base),
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 1b96b0960adc..5363dad1208d 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -607,23 +607,19 @@ static void ring_setup_status_page(struct intel_engine_cs *engine)
>  static bool stop_ring(struct intel_engine_cs *engine)
>  {
>  	struct drm_i915_private *dev_priv = engine->i915;
> +	int ret;
>  
> -	if (INTEL_GEN(dev_priv) > 2) {
> -		I915_WRITE_MODE(engine, _MASKED_BIT_ENABLE(STOP_RING));
> -		if (intel_wait_for_register(dev_priv,
> -					    RING_MI_MODE(engine->mmio_base),
> -					    MODE_IDLE,
> -					    MODE_IDLE,
> -					    1000)) {
> -			DRM_ERROR("%s : timed out trying to stop ring\n",
> -				  engine->name);
> -			/* Sometimes we observe that the idle flag is not
> -			 * set even though the ring is empty. So double
> -			 * check before giving up.
> -			 */
> -			if (I915_READ_HEAD(engine) != I915_READ_TAIL(engine))
> -				return false;
> -		}
> +	ret = intel_engine_stop_cs(engine);
> +	if (ret == -ENODEV)
> +		ret = 0;
> +
> +	if (ret) {
> +		/* Sometimes we observe that the idle flag is not
> +		 * set even though the ring is empty. So double
> +		 * check before giving up.
> +		 */
> +		if (I915_READ_HEAD(engine) != I915_READ_TAIL(engine))

Hmm these could have been also converted to the _FW variants.
-Mika


> +			return false;
>  	}
>  
>  	I915_WRITE_HEAD(engine, I915_READ_TAIL(engine));
> @@ -718,8 +714,7 @@ static int init_ring_common(struct intel_engine_cs *engine)
>  		goto out;
>  	}
>  
> -	if (INTEL_GEN(dev_priv) > 2)
> -		I915_WRITE_MODE(engine, _MASKED_BIT_DISABLE(STOP_RING));
> +	intel_engine_cancel_stop_cs(engine);
>  
>  	/* Now awake, let it get started */
>  	if (ring->tail != ring->head) {
> -- 
> 2.17.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Use intel_engine_stop_cs when stopping ringbuffer

[Chris Wilson]

Quoting Mika Kuoppala (2019-02-27 16:58:48)
> We have an exported function for stopping the engine before
> disabling a ringbuffer. Take it into use.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_engine_cs.c  |  3 +++
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 31 +++++++++++--------------
>  2 files changed, 16 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 4f244019560d..3feb0f74c239 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -817,6 +817,9 @@ void intel_engine_cancel_stop_cs(struct intel_engine_cs *engine)
>  {
>         struct drm_i915_private *dev_priv = engine->i915;
>  
> +       if (INTEL_GEN(dev_priv) < 3)
> +               return;
> +
>         GEM_TRACE("%s\n", engine->name);
>  
>         I915_WRITE_FW(RING_MI_MODE(engine->mmio_base),
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 1b96b0960adc..5363dad1208d 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -607,23 +607,19 @@ static void ring_setup_status_page(struct intel_engine_cs *engine)
>  static bool stop_ring(struct intel_engine_cs *engine)
>  {
>         struct drm_i915_private *dev_priv = engine->i915;
> +       int ret;
>  
> -       if (INTEL_GEN(dev_priv) > 2) {
> -               I915_WRITE_MODE(engine, _MASKED_BIT_ENABLE(STOP_RING));
> -               if (intel_wait_for_register(dev_priv,
> -                                           RING_MI_MODE(engine->mmio_base),
> -                                           MODE_IDLE,
> -                                           MODE_IDLE,
> -                                           1000)) {
> -                       DRM_ERROR("%s : timed out trying to stop ring\n",
> -                                 engine->name);
> -                       /* Sometimes we observe that the idle flag is not
> -                        * set even though the ring is empty. So double
> -                        * check before giving up.
> -                        */
> -                       if (I915_READ_HEAD(engine) != I915_READ_TAIL(engine))
> -                               return false;
> -               }
> +       ret = intel_engine_stop_cs(engine);
> +       if (ret == -ENODEV)
> +               ret = 0;
> +
> +       if (ret) {
> +               /* Sometimes we observe that the idle flag is not

As you go past, gradually switch over to
/*
 * Sometimes...
style. The joys of a slow transition to uniformity and switching styles
half way through.

> +                * set even though the ring is empty. So double
> +                * check before giving up.
> +                */
> +               if (I915_READ_HEAD(engine) != I915_READ_TAIL(engine))
> +                       return false;
>         }
>  
>         I915_WRITE_HEAD(engine, I915_READ_TAIL(engine));
> @@ -718,8 +714,7 @@ static int init_ring_common(struct intel_engine_cs *engine)
>                 goto out;
>         }
>  
> -       if (INTEL_GEN(dev_priv) > 2)
> -               I915_WRITE_MODE(engine, _MASKED_BIT_DISABLE(STOP_RING));
> +       intel_engine_cancel_stop_cs(engine);
>  
>         /* Now awake, let it get started */
>         if (ring->tail != ring->head) {

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Introduce intel_engine_stop_ringbuffer

[Chris Wilson]

Quoting Mika Kuoppala (2019-02-27 16:58:49)
> We use identical sequence of stopping ringbuffer on reset
> handing and on ring initialization. Make a function
> to handle both cases.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reset.c       | 18 +---------------
>  drivers/gpu/drm/i915/intel_engine_cs.c  | 28 +++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 10 ++-------
>  drivers/gpu/drm/i915/intel_ringbuffer.h |  2 ++
>  4 files changed, 33 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reset.c b/drivers/gpu/drm/i915/i915_reset.c
> index 55d6123dbba4..4191398b5125 100644
> --- a/drivers/gpu/drm/i915/i915_reset.c
> +++ b/drivers/gpu/drm/i915/i915_reset.c
> @@ -113,28 +113,12 @@ void i915_reset_request(struct i915_request *rq, bool guilty)
>  
>  static void gen3_stop_engine(struct intel_engine_cs *engine)
>  {
> -       struct drm_i915_private *dev_priv = engine->i915;
> -       const u32 base = engine->mmio_base;
> -
>         GEM_TRACE("%s\n", engine->name);
>  
>         if (intel_engine_stop_cs(engine))
>                 GEM_TRACE("%s: timed out on STOP_RING\n", engine->name);
>  
> -       I915_WRITE_FW(RING_HEAD(base), I915_READ_FW(RING_TAIL(base)));
> -       POSTING_READ_FW(RING_HEAD(base)); /* paranoia */
> -
> -       I915_WRITE_FW(RING_HEAD(base), 0);
> -       I915_WRITE_FW(RING_TAIL(base), 0);
> -       POSTING_READ_FW(RING_TAIL(base));
> -
> -       /* The ring must be empty before it is disabled */
> -       I915_WRITE_FW(RING_CTL(base), 0);
> -
> -       /* Check acts as a post */
> -       if (I915_READ_FW(RING_HEAD(base)))
> -               GEM_TRACE("%s: ring head [%x] not parked\n",
> -                         engine->name, I915_READ_FW(RING_HEAD(base)));
> +       intel_engine_stop_ringbuffer(engine);
>  }
>  
>  static void i915_stop_engines(struct drm_i915_private *i915,
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 3feb0f74c239..a8e47cfa6e35 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -826,6 +826,34 @@ void intel_engine_cancel_stop_cs(struct intel_engine_cs *engine)
>                       _MASKED_BIT_DISABLE(STOP_RING));
>  }
>  
> +int intel_engine_stop_ringbuffer(struct intel_engine_cs *engine)

I think intel_engine_stop_ring() works better, since it is the set of
RING registers we are controlling and are not concerned here with the
backing buffer.

> +{
> +       struct drm_i915_private *dev_priv = engine->i915;
> +       const u32 base = engine->mmio_base;
> +
> +       assert_forcewakes_active(dev_priv, FORCEWAKE_ALL);
> +       GEM_TRACE("%s\n", engine->name);
> +
> +       I915_WRITE_FW(RING_HEAD(base), I915_READ_FW(RING_TAIL(base)));
> +       POSTING_READ_FW(RING_HEAD(base)); /* paranoia */
> +
> +       I915_WRITE_FW(RING_HEAD(base), 0);
> +       I915_WRITE_FW(RING_TAIL(base), 0);
> +       POSTING_READ_FW(RING_TAIL(base));
> +
> +       /* The ring must be empty before it is disabled */
> +       I915_WRITE_FW(RING_CTL(base), 0);
> +
> +       /* Check acts as a post */
> +       if (I915_READ_FW(RING_HEAD(base))) {
> +               GEM_TRACE("%s: ring head [%x] not parked\n",
> +                         engine->name, I915_READ_FW(RING_HEAD(base)));
> +               return -EIO;
> +       }
> +
> +       return 0;
> +}
> +
>  const char *i915_cache_level_str(struct drm_i915_private *i915, int type)
>  {
>         switch (type) {
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 5363dad1208d..8936a9051760 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -622,15 +622,9 @@ static bool stop_ring(struct intel_engine_cs *engine)
>                         return false;
>         }
>  
> -       I915_WRITE_HEAD(engine, I915_READ_TAIL(engine));
> +       ret = intel_engine_stop_ringbuffer(engine);

Maybe

if (intel_engine_stop_ring(engine))
	return false;

return false;

looks tidier?

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Disable PSMI idle messaging when stopping ring

[Chris Wilson]

Quoting Mika Kuoppala (2019-02-27 16:58:50)
> Hardware cannot be in a middle of idle flow messaging
> when we pull the plug from ringbuffer. Disable idle
> messaging before we do so to avoid potential deadlock
> on engine initialization and reset.
> 
> v2: INVALID_MMIO_REG, unconditional enable (Chris)
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_engine_cs.c | 56 +++++++++++++++++++++++++-
>  1 file changed, 55 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index a8e47cfa6e35..fe7fca392b63 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -725,7 +725,7 @@ int intel_engine_init_common(struct intel_engine_cs *engine)
>  
>  /**
>   * intel_engines_cleanup_common - cleans up the engine state created by
> - *                                the common initiailizers.
> + *                                the common initializers.
>   * @engine: Engine to cleanup.
>   *
>   * This cleans up everything created by the common helpers.
> @@ -826,6 +826,55 @@ void intel_engine_cancel_stop_cs(struct intel_engine_cs *engine)
>                       _MASKED_BIT_DISABLE(STOP_RING));
>  }
>  
> +static i915_reg_t get_idle_poll_reg(const struct intel_engine_cs *engine)
> +{
> +       if (engine->id != RCS)
> +               return INVALID_MMIO_REG;
> +
> +       if (IS_GEN(engine->i915, 9))
> +               return GEN9_RCS_FE_FSM2;
> +
> +       if (IS_GEN(engine->i915, 8))
> +               return GEN6_RCS_PWR_FSM;
> +
> +       return INVALID_MMIO_REG;
> +}
> +
> +static void disable_idle_messaging(struct intel_engine_cs *engine)
> +{
> +       struct drm_i915_private *dev_priv = engine->i915;
> +       i915_reg_t poll_reg;
> +
> +       poll_reg = get_idle_poll_reg(engine);
> +       if (!i915_mmio_reg_valid(poll_reg))
> +               return;
> +
> +       GEM_DEBUG_WARN_ON(I915_READ_FW(GEN6_RC_SLEEP_PSMI_CONTROL) &
> +                         GEN6_PSMI_SLEEP_MSG_DISABLE);
> +
> +       I915_WRITE_FW(GEN6_RC_SLEEP_PSMI_CONTROL,
> +                     _MASKED_BIT_ENABLE(GEN6_PSMI_SLEEP_MSG_DISABLE));
> +

Needs some reference at least. Or a synposis of what exactly we are
waiting for in a comment.

> +       if (__intel_wait_for_register_fw(dev_priv, poll_reg,
> +                                        0x7f, 0x30,
> +                                        5000, 0,
> +                                        NULL))
> +               DRM_DEBUG_DRIVER("psmi idle msg poll timeout\n");
> +}
> +
> +static void enable_idle_messaging(struct intel_engine_cs *engine)
> +{
> +       struct drm_i915_private *dev_priv = engine->i915;
> +       i915_reg_t poll_reg;
> +
> +       poll_reg = get_idle_poll_reg(engine);
> +       if (!i915_mmio_reg_valid(poll_reg))
> +               return;
> +
> +       I915_WRITE_FW(GEN6_RC_SLEEP_PSMI_CONTROL,
> +                     _MASKED_BIT_DISABLE(GEN6_PSMI_SLEEP_MSG_DISABLE));
> +}
> +
>  int intel_engine_stop_ringbuffer(struct intel_engine_cs *engine)
>  {
>         struct drm_i915_private *dev_priv = engine->i915;
> @@ -841,9 +890,14 @@ int intel_engine_stop_ringbuffer(struct intel_engine_cs *engine)
>         I915_WRITE_FW(RING_TAIL(base), 0);
>         POSTING_READ_FW(RING_TAIL(base));
>  
> +       /* Idle messaging needs to be off during ring disable */
> +       disable_idle_messaging(engine);
> +
>         /* The ring must be empty before it is disabled */
>         I915_WRITE_FW(RING_CTL(base), 0);
>  
> +       enable_idle_messaging(engine);
> +
>         /* Check acts as a post */
>         if (I915_READ_FW(RING_HEAD(base))) {
>                 GEM_TRACE("%s: ring head [%x] not parked\n",

Given the history SLEEP_MSG_DISABLE around RING_CTL is not surprising.

Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Use intel_engine_stop_cs when stopping ringbuffer

[Chris Wilson]

Quoting Mika Kuoppala (2019-02-27 17:03:38)
> Mika Kuoppala <mika.kuoppala@linux.intel.com> writes:
> 
> > We have an exported function for stopping the engine before
> > disabling a ringbuffer. Take it into use.
> >
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_engine_cs.c  |  3 +++
> >  drivers/gpu/drm/i915/intel_ringbuffer.c | 31 +++++++++++--------------
> >  2 files changed, 16 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> > index 4f244019560d..3feb0f74c239 100644
> > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > @@ -817,6 +817,9 @@ void intel_engine_cancel_stop_cs(struct intel_engine_cs *engine)
> >  {
> >       struct drm_i915_private *dev_priv = engine->i915;
> >  
> > +     if (INTEL_GEN(dev_priv) < 3)
> > +             return;
> > +
> >       GEM_TRACE("%s\n", engine->name);
> >  
> >       I915_WRITE_FW(RING_MI_MODE(engine->mmio_base),
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index 1b96b0960adc..5363dad1208d 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -607,23 +607,19 @@ static void ring_setup_status_page(struct intel_engine_cs *engine)
> >  static bool stop_ring(struct intel_engine_cs *engine)
> >  {
> >       struct drm_i915_private *dev_priv = engine->i915;
> > +     int ret;
> >  
> > -     if (INTEL_GEN(dev_priv) > 2) {
> > -             I915_WRITE_MODE(engine, _MASKED_BIT_ENABLE(STOP_RING));
> > -             if (intel_wait_for_register(dev_priv,
> > -                                         RING_MI_MODE(engine->mmio_base),
> > -                                         MODE_IDLE,
> > -                                         MODE_IDLE,
> > -                                         1000)) {
> > -                     DRM_ERROR("%s : timed out trying to stop ring\n",
> > -                               engine->name);
> > -                     /* Sometimes we observe that the idle flag is not
> > -                      * set even though the ring is empty. So double
> > -                      * check before giving up.
> > -                      */
> > -                     if (I915_READ_HEAD(engine) != I915_READ_TAIL(engine))
> > -                             return false;
> > -             }
> > +     ret = intel_engine_stop_cs(engine);
> > +     if (ret == -ENODEV)
> > +             ret = 0;
> > +
> > +     if (ret) {
> > +             /* Sometimes we observe that the idle flag is not
> > +              * set even though the ring is empty. So double
> > +              * check before giving up.
> > +              */
> > +             if (I915_READ_HEAD(engine) != I915_READ_TAIL(engine))
> 
> Hmm these could have been also converted to the _FW variants.

Sneak it in.
-Chris
_______________________________________________
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 [1/3] drm/i915: Use intel_engine_stop_cs when stopping ringbuffer

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: Use intel_engine_stop_cs when stopping ringbuffer
URL   : https://patchwork.freedesktop.org/series/57304/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5666 -> Patchwork_12316
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_12316 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_12316, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/57304/revisions/1/

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_12316:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live_active:
    - fi-apl-guc:         PASS -> DMESG-WARN

  
Known issues
------------

  Here are the changes found in Patchwork_12316 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_pm_rpm@basic-pci-d3-state:
    - fi-byt-j1900:       PASS -> SKIP [fdo#109271]

  * igt@i915_pm_rpm@basic-rte:
    - fi-byt-j1900:       PASS -> FAIL [fdo#108800]

  * igt@i915_selftest@live_contexts:
    - fi-icl-u2:          NOTRUN -> DMESG-FAIL [fdo#108569]

  * igt@kms_busy@basic-flip-a:
    - fi-kbl-7567u:       PASS -> SKIP [fdo#109271] / [fdo#109278] +2

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-kbl-7567u:       PASS -> WARN [fdo#109380]

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-c:
    - fi-kbl-7567u:       PASS -> SKIP [fdo#109271] +33

  * igt@kms_psr@primary_mmap_gtt:
    - fi-blb-e6850:       NOTRUN -> SKIP [fdo#109271] +27

  
#### Possible fixes ####

  * igt@i915_selftest@live_requests:
    - fi-icl-u2:          INCOMPLETE [fdo#109644] -> PASS

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - fi-blb-e6850:       INCOMPLETE [fdo#107718] -> PASS

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#108800]: https://bugs.freedesktop.org/show_bug.cgi?id=108800
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109380]: https://bugs.freedesktop.org/show_bug.cgi?id=109380
  [fdo#109644]: https://bugs.freedesktop.org/show_bug.cgi?id=109644


Participating hosts (44 -> 36)
------------------------------

  Missing    (8): fi-ilk-m540 fi-hsw-4200u fi-hsw-peppy fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-gdg-551 fi-ivb-3770 


Build changes
-------------

    * Linux: CI_DRM_5666 -> Patchwork_12316

  CI_DRM_5666: 358ab8acaabef3cef2a7ce9e5dd7c4196a0c30fc @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4860: b79007f9c575a538a63ce9301a890ed9e1a45f35 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12316: a3a61404858ebb091d4618c68a60513cfdedfc95 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

a3a61404858e drm/i915: Disable PSMI idle messaging when stopping ring
c29ce4adfdc3 drm/i915: Introduce intel_engine_stop_ringbuffer
8195837078fd drm/i915: Use intel_engine_stop_cs when stopping ringbuffer

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12316/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: failure for series starting with [1/3] drm/i915: Use intel_engine_stop_cs when stopping ringbuffer

[Chris Wilson]

Quoting Patchwork (2019-02-27 17:41:59)
> #### Possible regressions ####
> 
>   * igt@i915_selftest@live_active:
>     - fi-apl-guc:         PASS -> DMESG-WARN

Second flip due to "drm/i915: Avoid waking the engines just to check if
they are idle", both skl.

Suggests that its passing earlier was fluke.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/3] drm/i915: Use intel_engine_stop_cs when stopping ringbuffer

[Mika Kuoppala]

We have an exported function for stopping the engine before
disabling a ringbuffer. Take it into use.

v2: use fw on empty check

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_engine_cs.c  |  3 ++
 drivers/gpu/drm/i915/intel_ringbuffer.c | 41 ++++++++++++++-----------
 2 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index df8f88142f1d..e35dc0386bf6 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -856,6 +856,9 @@ void intel_engine_cancel_stop_cs(struct intel_engine_cs *engine)
 {
 	struct drm_i915_private *dev_priv = engine->i915;
 
+	if (INTEL_GEN(dev_priv) < 3)
+		return;
+
 	GEM_TRACE("%s\n", engine->name);
 
 	I915_WRITE_FW(RING_MI_MODE(engine->mmio_base),
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 1b96b0960adc..d7486f9a29a9 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -604,26 +604,32 @@ static void ring_setup_status_page(struct intel_engine_cs *engine)
 	flush_cs_tlb(engine);
 }
 
+static bool ring_is_empty(struct intel_engine_cs *engine)
+{
+	struct drm_i915_private *dev_priv = engine->i915;
+	const u32 base = engine->mmio_base;
+
+	return (I915_READ_FW(RING_HEAD(base)) & HEAD_ADDR) ==
+		(I915_READ_FW(RING_HEAD(base)) & TAIL_ADDR);
+}
+
 static bool stop_ring(struct intel_engine_cs *engine)
 {
 	struct drm_i915_private *dev_priv = engine->i915;
+	int ret;
 
-	if (INTEL_GEN(dev_priv) > 2) {
-		I915_WRITE_MODE(engine, _MASKED_BIT_ENABLE(STOP_RING));
-		if (intel_wait_for_register(dev_priv,
-					    RING_MI_MODE(engine->mmio_base),
-					    MODE_IDLE,
-					    MODE_IDLE,
-					    1000)) {
-			DRM_ERROR("%s : timed out trying to stop ring\n",
-				  engine->name);
-			/* Sometimes we observe that the idle flag is not
-			 * set even though the ring is empty. So double
-			 * check before giving up.
-			 */
-			if (I915_READ_HEAD(engine) != I915_READ_TAIL(engine))
-				return false;
-		}
+	ret = intel_engine_stop_cs(engine);
+	if (ret == -ENODEV)
+		ret = 0;
+
+	if (ret) {
+		/*
+		 * Sometimes we observe that the idle flag is not
+		 * set even though the ring is empty. So double
+		 * check before giving up.
+		 */
+		if (!ring_is_empty(engine))
+			return false;
 	}
 
 	I915_WRITE_HEAD(engine, I915_READ_TAIL(engine));
@@ -718,8 +724,7 @@ static int init_ring_common(struct intel_engine_cs *engine)
 		goto out;
 	}
 
-	if (INTEL_GEN(dev_priv) > 2)
-		I915_WRITE_MODE(engine, _MASKED_BIT_DISABLE(STOP_RING));
+	intel_engine_cancel_stop_cs(engine);
 
 	/* Now awake, let it get started */
 	if (ring->tail != ring->head) {
-- 
2.17.1

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

Re: [PATCH 1/3] drm/i915: Use intel_engine_stop_cs when stopping ringbuffer

[Mika Kuoppala]

Mika Kuoppala <mika.kuoppala@linux.intel.com> writes:

> We have an exported function for stopping the engine before
> disabling a ringbuffer. Take it into use.
>
> v2: use fw on empty check
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_engine_cs.c  |  3 ++
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 41 ++++++++++++++-----------
>  2 files changed, 26 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index df8f88142f1d..e35dc0386bf6 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -856,6 +856,9 @@ void intel_engine_cancel_stop_cs(struct intel_engine_cs *engine)
>  {
>  	struct drm_i915_private *dev_priv = engine->i915;
>  
> +	if (INTEL_GEN(dev_priv) < 3)
> +		return;
> +
>  	GEM_TRACE("%s\n", engine->name);
>  
>  	I915_WRITE_FW(RING_MI_MODE(engine->mmio_base),
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 1b96b0960adc..d7486f9a29a9 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -604,26 +604,32 @@ static void ring_setup_status_page(struct intel_engine_cs *engine)
>  	flush_cs_tlb(engine);
>  }
>  
> +static bool ring_is_empty(struct intel_engine_cs *engine)
> +{
> +	struct drm_i915_private *dev_priv = engine->i915;
> +	const u32 base = engine->mmio_base;
> +
> +	return (I915_READ_FW(RING_HEAD(base)) & HEAD_ADDR) ==
> +		(I915_READ_FW(RING_HEAD(base)) & TAIL_ADDR);

ARgh!
-Mika

> +}
> +
>  static bool stop_ring(struct intel_engine_cs *engine)
>  {
>  	struct drm_i915_private *dev_priv = engine->i915;
> +	int ret;
>  
> -	if (INTEL_GEN(dev_priv) > 2) {
> -		I915_WRITE_MODE(engine, _MASKED_BIT_ENABLE(STOP_RING));
> -		if (intel_wait_for_register(dev_priv,
> -					    RING_MI_MODE(engine->mmio_base),
> -					    MODE_IDLE,
> -					    MODE_IDLE,
> -					    1000)) {
> -			DRM_ERROR("%s : timed out trying to stop ring\n",
> -				  engine->name);
> -			/* Sometimes we observe that the idle flag is not
> -			 * set even though the ring is empty. So double
> -			 * check before giving up.
> -			 */
> -			if (I915_READ_HEAD(engine) != I915_READ_TAIL(engine))
> -				return false;
> -		}
> +	ret = intel_engine_stop_cs(engine);
> +	if (ret == -ENODEV)
> +		ret = 0;
> +
> +	if (ret) {
> +		/*
> +		 * Sometimes we observe that the idle flag is not
> +		 * set even though the ring is empty. So double
> +		 * check before giving up.
> +		 */
> +		if (!ring_is_empty(engine))
> +			return false;
>  	}
>  
>  	I915_WRITE_HEAD(engine, I915_READ_TAIL(engine));
> @@ -718,8 +724,7 @@ static int init_ring_common(struct intel_engine_cs *engine)
>  		goto out;
>  	}
>  
> -	if (INTEL_GEN(dev_priv) > 2)
> -		I915_WRITE_MODE(engine, _MASKED_BIT_DISABLE(STOP_RING));
> +	intel_engine_cancel_stop_cs(engine);
>  
>  	/* Now awake, let it get started */
>  	if (ring->tail != ring->head) {
> -- 
> 2.17.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/3] drm/i915: Use intel_engine_stop_cs when stopping ringbuffer

[Mika Kuoppala]

We have an exported function for stopping the engine before
disabling a ringbuffer. Take it into use.

v2: use fw on empty check
v3: tail is tail

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_engine_cs.c  |  3 ++
 drivers/gpu/drm/i915/intel_ringbuffer.c | 41 ++++++++++++++-----------
 2 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index df8f88142f1d..e35dc0386bf6 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -856,6 +856,9 @@ void intel_engine_cancel_stop_cs(struct intel_engine_cs *engine)
 {
 	struct drm_i915_private *dev_priv = engine->i915;
 
+	if (INTEL_GEN(dev_priv) < 3)
+		return;
+
 	GEM_TRACE("%s\n", engine->name);
 
 	I915_WRITE_FW(RING_MI_MODE(engine->mmio_base),
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 1b96b0960adc..5fe28d9087b7 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -604,26 +604,32 @@ static void ring_setup_status_page(struct intel_engine_cs *engine)
 	flush_cs_tlb(engine);
 }
 
+static bool ring_is_empty(struct intel_engine_cs *engine)
+{
+	struct drm_i915_private *dev_priv = engine->i915;
+	const u32 base = engine->mmio_base;
+
+	return (I915_READ_FW(RING_HEAD(base)) & HEAD_ADDR) ==
+		(I915_READ_FW(RING_TAIL(base)) & TAIL_ADDR);
+}
+
 static bool stop_ring(struct intel_engine_cs *engine)
 {
 	struct drm_i915_private *dev_priv = engine->i915;
+	int ret;
 
-	if (INTEL_GEN(dev_priv) > 2) {
-		I915_WRITE_MODE(engine, _MASKED_BIT_ENABLE(STOP_RING));
-		if (intel_wait_for_register(dev_priv,
-					    RING_MI_MODE(engine->mmio_base),
-					    MODE_IDLE,
-					    MODE_IDLE,
-					    1000)) {
-			DRM_ERROR("%s : timed out trying to stop ring\n",
-				  engine->name);
-			/* Sometimes we observe that the idle flag is not
-			 * set even though the ring is empty. So double
-			 * check before giving up.
-			 */
-			if (I915_READ_HEAD(engine) != I915_READ_TAIL(engine))
-				return false;
-		}
+	ret = intel_engine_stop_cs(engine);
+	if (ret == -ENODEV)
+		ret = 0;
+
+	if (ret) {
+		/*
+		 * Sometimes we observe that the idle flag is not
+		 * set even though the ring is empty. So double
+		 * check before giving up.
+		 */
+		if (!ring_is_empty(engine))
+			return false;
 	}
 
 	I915_WRITE_HEAD(engine, I915_READ_TAIL(engine));
@@ -718,8 +724,7 @@ static int init_ring_common(struct intel_engine_cs *engine)
 		goto out;
 	}
 
-	if (INTEL_GEN(dev_priv) > 2)
-		I915_WRITE_MODE(engine, _MASKED_BIT_DISABLE(STOP_RING));
+	intel_engine_cancel_stop_cs(engine);
 
 	/* Now awake, let it get started */
 	if (ring->tail != ring->head) {
-- 
2.17.1

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

Re: [PATCH 1/3] drm/i915: Use intel_engine_stop_cs when stopping ringbuffer

[Chris Wilson]

Quoting Mika Kuoppala (2019-02-28 16:14:11)
> We have an exported function for stopping the engine before
> disabling a ringbuffer. Take it into use.
> 
> v2: use fw on empty check
> v3: tail is tail
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_engine_cs.c  |  3 ++
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 41 ++++++++++++++-----------
>  2 files changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index df8f88142f1d..e35dc0386bf6 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -856,6 +856,9 @@ void intel_engine_cancel_stop_cs(struct intel_engine_cs *engine)
>  {
>         struct drm_i915_private *dev_priv = engine->i915;
>  
> +       if (INTEL_GEN(dev_priv) < 3)
> +               return;
> +
>         GEM_TRACE("%s\n", engine->name);
>  
>         I915_WRITE_FW(RING_MI_MODE(engine->mmio_base),
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 1b96b0960adc..5fe28d9087b7 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -604,26 +604,32 @@ static void ring_setup_status_page(struct intel_engine_cs *engine)
>         flush_cs_tlb(engine);
>  }
>  
> +static bool ring_is_empty(struct intel_engine_cs *engine)
> +{
> +       struct drm_i915_private *dev_priv = engine->i915;
> +       const u32 base = engine->mmio_base;
> +
> +       return (I915_READ_FW(RING_HEAD(base)) & HEAD_ADDR) ==
> +               (I915_READ_FW(RING_TAIL(base)) & TAIL_ADDR);
> +}
> +
>  static bool stop_ring(struct intel_engine_cs *engine)
>  {
>         struct drm_i915_private *dev_priv = engine->i915;
> +       int ret;
>  
> -       if (INTEL_GEN(dev_priv) > 2) {
> -               I915_WRITE_MODE(engine, _MASKED_BIT_ENABLE(STOP_RING));
> -               if (intel_wait_for_register(dev_priv,
> -                                           RING_MI_MODE(engine->mmio_base),
> -                                           MODE_IDLE,
> -                                           MODE_IDLE,
> -                                           1000)) {
> -                       DRM_ERROR("%s : timed out trying to stop ring\n",
> -                                 engine->name);
> -                       /* Sometimes we observe that the idle flag is not
> -                        * set even though the ring is empty. So double
> -                        * check before giving up.
> -                        */
> -                       if (I915_READ_HEAD(engine) != I915_READ_TAIL(engine))
> -                               return false;
> -               }
> +       ret = intel_engine_stop_cs(engine);
> +       if (ret == -ENODEV)
> +               ret = 0;
> +
> +       if (ret) {
> +               /*
> +                * Sometimes we observe that the idle flag is not
> +                * set even though the ring is empty. So double
> +                * check before giving up.
> +                */
> +               if (!ring_is_empty(engine))
> +                       return false;

Hmm, thinking more about this, shouldn't we push this down to stop_cs()?

If that's reporting an error in a situation where we can determine that
the ring is idle anyway, we can report the stop_cs succeeded.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Use intel_engine_stop_cs when stopping ringbuffer

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2019-02-28 16:14:11)
>> We have an exported function for stopping the engine before
>> disabling a ringbuffer. Take it into use.
>> 
>> v2: use fw on empty check
>> v3: tail is tail
>> 
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>  drivers/gpu/drm/i915/intel_engine_cs.c  |  3 ++
>>  drivers/gpu/drm/i915/intel_ringbuffer.c | 41 ++++++++++++++-----------
>>  2 files changed, 26 insertions(+), 18 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
>> index df8f88142f1d..e35dc0386bf6 100644
>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
>> @@ -856,6 +856,9 @@ void intel_engine_cancel_stop_cs(struct intel_engine_cs *engine)
>>  {
>>         struct drm_i915_private *dev_priv = engine->i915;
>>  
>> +       if (INTEL_GEN(dev_priv) < 3)
>> +               return;
>> +
>>         GEM_TRACE("%s\n", engine->name);
>>  
>>         I915_WRITE_FW(RING_MI_MODE(engine->mmio_base),
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> index 1b96b0960adc..5fe28d9087b7 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> @@ -604,26 +604,32 @@ static void ring_setup_status_page(struct intel_engine_cs *engine)
>>         flush_cs_tlb(engine);
>>  }
>>  
>> +static bool ring_is_empty(struct intel_engine_cs *engine)
>> +{
>> +       struct drm_i915_private *dev_priv = engine->i915;
>> +       const u32 base = engine->mmio_base;
>> +
>> +       return (I915_READ_FW(RING_HEAD(base)) & HEAD_ADDR) ==
>> +               (I915_READ_FW(RING_TAIL(base)) & TAIL_ADDR);
>> +}
>> +
>>  static bool stop_ring(struct intel_engine_cs *engine)
>>  {
>>         struct drm_i915_private *dev_priv = engine->i915;
>> +       int ret;
>>  
>> -       if (INTEL_GEN(dev_priv) > 2) {
>> -               I915_WRITE_MODE(engine, _MASKED_BIT_ENABLE(STOP_RING));
>> -               if (intel_wait_for_register(dev_priv,
>> -                                           RING_MI_MODE(engine->mmio_base),
>> -                                           MODE_IDLE,
>> -                                           MODE_IDLE,
>> -                                           1000)) {
>> -                       DRM_ERROR("%s : timed out trying to stop ring\n",
>> -                                 engine->name);
>> -                       /* Sometimes we observe that the idle flag is not
>> -                        * set even though the ring is empty. So double
>> -                        * check before giving up.
>> -                        */
>> -                       if (I915_READ_HEAD(engine) != I915_READ_TAIL(engine))
>> -                               return false;
>> -               }
>> +       ret = intel_engine_stop_cs(engine);
>> +       if (ret == -ENODEV)
>> +               ret = 0;
>> +
>> +       if (ret) {
>> +               /*
>> +                * Sometimes we observe that the idle flag is not
>> +                * set even though the ring is empty. So double
>> +                * check before giving up.
>> +                */
>> +               if (!ring_is_empty(engine))
>> +                       return false;
>
> Hmm, thinking more about this, shouldn't we push this down to stop_cs()?
>
> If that's reporting an error in a situation where we can determine that
> the ring is idle anyway, we can report the stop_cs succeeded.

Makes sense, I will take a look.

I felt small urge to deflate the 'stop'.

Would it be confusing if we just did
intel_engine_start|stop instead of stop_cs and
cancel_stop_cs?

So hmm:

intel_engine_start()
intel_engine_stop()

these would only toggle the STOP_RING

and for replacing stop_ring with:
intel_engine_empty_ring()

for zeroing the heads.

one 'stop' to rule the (ring)world!?

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

Re: [PATCH 1/3] drm/i915: Use intel_engine_stop_cs when stopping ringbuffer

[Chris Wilson]

Quoting Mika Kuoppala (2019-02-28 16:53:46)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Quoting Mika Kuoppala (2019-02-28 16:14:11)
> >> We have an exported function for stopping the engine before
> >> disabling a ringbuffer. Take it into use.
> >> 
> >> v2: use fw on empty check
> >> v3: tail is tail
> >> 
> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> >> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> ---
> >>  drivers/gpu/drm/i915/intel_engine_cs.c  |  3 ++
> >>  drivers/gpu/drm/i915/intel_ringbuffer.c | 41 ++++++++++++++-----------
> >>  2 files changed, 26 insertions(+), 18 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> >> index df8f88142f1d..e35dc0386bf6 100644
> >> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> >> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> >> @@ -856,6 +856,9 @@ void intel_engine_cancel_stop_cs(struct intel_engine_cs *engine)
> >>  {
> >>         struct drm_i915_private *dev_priv = engine->i915;
> >>  
> >> +       if (INTEL_GEN(dev_priv) < 3)
> >> +               return;
> >> +
> >>         GEM_TRACE("%s\n", engine->name);
> >>  
> >>         I915_WRITE_FW(RING_MI_MODE(engine->mmio_base),
> >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> index 1b96b0960adc..5fe28d9087b7 100644
> >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> @@ -604,26 +604,32 @@ static void ring_setup_status_page(struct intel_engine_cs *engine)
> >>         flush_cs_tlb(engine);
> >>  }
> >>  
> >> +static bool ring_is_empty(struct intel_engine_cs *engine)
> >> +{
> >> +       struct drm_i915_private *dev_priv = engine->i915;
> >> +       const u32 base = engine->mmio_base;
> >> +
> >> +       return (I915_READ_FW(RING_HEAD(base)) & HEAD_ADDR) ==
> >> +               (I915_READ_FW(RING_TAIL(base)) & TAIL_ADDR);
> >> +}
> >> +
> >>  static bool stop_ring(struct intel_engine_cs *engine)
> >>  {
> >>         struct drm_i915_private *dev_priv = engine->i915;
> >> +       int ret;
> >>  
> >> -       if (INTEL_GEN(dev_priv) > 2) {
> >> -               I915_WRITE_MODE(engine, _MASKED_BIT_ENABLE(STOP_RING));
> >> -               if (intel_wait_for_register(dev_priv,
> >> -                                           RING_MI_MODE(engine->mmio_base),
> >> -                                           MODE_IDLE,
> >> -                                           MODE_IDLE,
> >> -                                           1000)) {
> >> -                       DRM_ERROR("%s : timed out trying to stop ring\n",
> >> -                                 engine->name);
> >> -                       /* Sometimes we observe that the idle flag is not
> >> -                        * set even though the ring is empty. So double
> >> -                        * check before giving up.
> >> -                        */
> >> -                       if (I915_READ_HEAD(engine) != I915_READ_TAIL(engine))
> >> -                               return false;
> >> -               }
> >> +       ret = intel_engine_stop_cs(engine);
> >> +       if (ret == -ENODEV)
> >> +               ret = 0;
> >> +
> >> +       if (ret) {
> >> +               /*
> >> +                * Sometimes we observe that the idle flag is not
> >> +                * set even though the ring is empty. So double
> >> +                * check before giving up.
> >> +                */
> >> +               if (!ring_is_empty(engine))
> >> +                       return false;
> >
> > Hmm, thinking more about this, shouldn't we push this down to stop_cs()?
> >
> > If that's reporting an error in a situation where we can determine that
> > the ring is idle anyway, we can report the stop_cs succeeded.
> 
> Makes sense, I will take a look.
> 
> I felt small urge to deflate the 'stop'.
> 
> Would it be confusing if we just did
> intel_engine_start|stop instead of stop_cs and
> cancel_stop_cs?
> 
> So hmm:
> 
> intel_engine_start()
> intel_engine_stop()
> 
> these would only toggle the STOP_RING
> 
> and for replacing stop_ring with:
> intel_engine_empty_ring()

intel_engine_clear_ring() / reset_ring(). Hmm, clear_ring of those two.
> 
> for zeroing the heads.
> 
> one 'stop' to rule the (ring)world!?

The counter argument is that _start() is a little too broad. The appeal
of stop_cs() is that it describing what it is doing, poking at the bit
to stop the CS advancing and nothing more. It frequently doesn't
succeed...

So I think it's not a worthy change, but I never did feel totally
satisfied with stop_cs -- cs is too short, but we do have usage with
xCS.

intel_engine_stop_command_streamer,
intel_engine_halt_command_streamer,
intel_engine_pause_command_streamer,
?

But intel_engine_clear_ring() I could be sold on.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx