Re: [PATCH 4/4] drm/i915: Perform Sandybridge BSD tail write under the forcewake

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 30/06/16 12:30, Chris Wilson wrote:
> Since we have a sequence of register reads and writes, we can reduce the
> latency of starting the BSD ring by performing all the mmio operations
> under the same forcewake wakeref.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_drv.h         |  1 +
>   drivers/gpu/drm/i915/intel_ringbuffer.c | 28 ++++++++++++++++------------
>   2 files changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 48d30676455e..485ab1148181 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3882,6 +3882,7 @@ __raw_write(64, q)
>    */
>   #define I915_READ_FW(reg__) __raw_i915_read32(dev_priv, (reg__))
>   #define I915_WRITE_FW(reg__, val__) __raw_i915_write32(dev_priv, (reg__), (val__))
> +#define I915_WRITE64_FW(reg__, val__) __raw_i915_write64(dev_priv, (reg__), (val__))
>   #define POSTING_READ_FW(reg__) (void)I915_READ_FW(reg__)
>
>   /* "Broadcast RGB" property */
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index c4365cc1f133..7c93d4c210e5 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2684,34 +2684,38 @@ static void gen6_bsd_ring_write_tail(struct intel_engine_cs *engine,
>   {
>   	struct drm_i915_private *dev_priv = engine->i915;
>
> +	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
> +
>          /* Every tail move must follow the sequence below */
>
>   	/* Disable notification that the ring is IDLE. The GT
>   	 * will then assume that it is busy and bring it out of rc6.
>   	 */
> -	I915_WRITE(GEN6_BSD_SLEEP_PSMI_CONTROL,
> -		   _MASKED_BIT_ENABLE(GEN6_BSD_SLEEP_MSG_DISABLE));
> +	I915_WRITE_FW(GEN6_BSD_SLEEP_PSMI_CONTROL,
> +		      _MASKED_BIT_ENABLE(GEN6_BSD_SLEEP_MSG_DISABLE));
>
>   	/* Clear the context id. Here be magic! */
> -	I915_WRITE64(GEN6_BSD_RNCID, 0x0);
> +	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(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,
> +				       50))
>   		DRM_ERROR("timed out waiting for the BSD ring to wake up\n");
>
>   	/* Now that the ring is fully powered up, update the tail */
> -	I915_WRITE_TAIL(engine, value);
> -	POSTING_READ(RING_TAIL(engine->mmio_base));
> +	I915_WRITE_FW(RING_TAIL(engine->mmio_base), value);
> +	POSTING_READ_FW(RING_TAIL(engine->mmio_base));
>
>   	/* Let the ring send IDLE messages to the GT again,
>   	 * and so let it sleep to conserve power when idle.
>   	 */
> -	I915_WRITE(GEN6_BSD_SLEEP_PSMI_CONTROL,
> -		   _MASKED_BIT_DISABLE(GEN6_BSD_SLEEP_MSG_DISABLE));
> +	I915_WRITE_FW(GEN6_BSD_SLEEP_PSMI_CONTROL,
> +		      _MASKED_BIT_DISABLE(GEN6_BSD_SLEEP_MSG_DISABLE));
> +
> +	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>   }
>
>   static int gen6_bsd_ring_flush(struct drm_i915_gem_request *req,
>

Looks reasonable to me if you think mmio traces are not interesting here.

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