diff mbox series

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

Message ID 20190228160133.819-3-mika.kuoppala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series [1/3] drm/i915: Use intel_engine_stop_cs when stopping ringbuffer | expand

Commit Message

Mika Kuoppala Feb. 28, 2019, 4:01 p.m. UTC 
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)
v3: comment (Chris), bspec reference

References: bspec/11751
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Acked-by: 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 | 64 +++++++++++++++++++++++++-
 1 file changed, 63 insertions(+), 1 deletion(-)

Comments

Chris Wilson Feb. 28, 2019, 4:22 p.m. UTC | #1 
Quoting Mika Kuoppala (2019-02-28 16:01:33)
> 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)
> v3: comment (Chris), bspec reference
> 
> References: bspec/11751
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Acked-by: 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 | 64 +++++++++++++++++++++++++-
>  1 file changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 2a94b92cfcd3..10cb6b22fe92 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -764,7 +764,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.
> @@ -865,6 +865,63 @@ 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);
> +       /*
> +        * Hardware must not be in middle of idle flow signalling
> +        * when the RING_CTL is zeroed. In order to prevent this
> +        * we disable the messaging temporarily.
> +        */
> +       I915_WRITE_FW(GEN6_RC_SLEEP_PSMI_CONTROL,
> +                     _MASKED_BIT_ENABLE(GEN6_PSMI_SLEEP_MSG_DISABLE));
> +
> +       /*
> +        * Poll the magic bits to ensure that engine state machine
> +        * is in valid state for stopping the ring.
> +        */
> +       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_ring(struct intel_engine_cs *engine)
>  {
>         struct drm_i915_private *dev_priv = engine->i915;
> @@ -880,9 +937,14 @@ int intel_engine_stop_ring(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 */

Ok, that's ott :)

The previous pair are a nice why, peeling back some of the layers of the
HW to see what's going on. This one is just repeating
disable_idle_messing().
-Chris
Mika Kuoppala Feb. 28, 2019, 4:33 p.m. UTC | #2 
Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2019-02-28 16:01:33)
>> 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)
>> v3: comment (Chris), bspec reference
>> 
>> References: bspec/11751
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Acked-by: 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 | 64 +++++++++++++++++++++++++-
>>  1 file changed, 63 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
>> index 2a94b92cfcd3..10cb6b22fe92 100644
>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
>> @@ -764,7 +764,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.
>> @@ -865,6 +865,63 @@ 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);
>> +       /*
>> +        * Hardware must not be in middle of idle flow signalling
>> +        * when the RING_CTL is zeroed. In order to prevent this
>> +        * we disable the messaging temporarily.
>> +        */
>> +       I915_WRITE_FW(GEN6_RC_SLEEP_PSMI_CONTROL,
>> +                     _MASKED_BIT_ENABLE(GEN6_PSMI_SLEEP_MSG_DISABLE));
>> +
>> +       /*
>> +        * Poll the magic bits to ensure that engine state machine
>> +        * is in valid state for stopping the ring.
>> +        */
>> +       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_ring(struct intel_engine_cs *engine)
>>  {
>>         struct drm_i915_private *dev_priv = engine->i915;
>> @@ -880,9 +937,14 @@ int intel_engine_stop_ring(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 */
>
> Ok, that's ott :)

Rest assured, Captain Obvious will scrap this before merging.

>
> The previous pair are a nice why, peeling back some of the layers of the
> HW to see what's going on. This one is just repeating
> disable_idle_messing().

There is this dream I had where hardware engineers
did review patches.

But it should be close enough of 'what's going on'

Thanks,
-Mika
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 2a94b92cfcd3..10cb6b22fe92 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -764,7 +764,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.
@@ -865,6 +865,63 @@  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);
+	/*
+	 * Hardware must not be in middle of idle flow signalling
+	 * when the RING_CTL is zeroed. In order to prevent this
+	 * we disable the messaging temporarily.
+	 */
+	I915_WRITE_FW(GEN6_RC_SLEEP_PSMI_CONTROL,
+		      _MASKED_BIT_ENABLE(GEN6_PSMI_SLEEP_MSG_DISABLE));
+
+	/*
+	 * Poll the magic bits to ensure that engine state machine
+	 * is in valid state for stopping the ring.
+	 */
+	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_ring(struct intel_engine_cs *engine)
 {
 	struct drm_i915_private *dev_priv = engine->i915;
@@ -880,9 +937,14 @@  int intel_engine_stop_ring(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",