Re: [PATCH 2/4] drm/i915: Convert sandybridge_pcode_*() to use intel_wait_for_register()

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

On 30/06/16 12:30, Chris Wilson wrote:
> We want to replace the inline wait_for() with an out-of-line hybrid
> busy/sleep wait_for() in the hopes of speeding up the communication wit
> the PCode unit.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_pm.c | 43 +++++++++++++++++++++++++++--------------
>   1 file changed, 28 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d7f8ba89d2a5..f84bf253d04a 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -7623,46 +7623,59 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val
>   {
>   	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
>
> -	if (I915_READ(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) {
> +	/* GEN6_PCODE_* are outside of the forcewake domain, we can
> +	 * use te fw I915_READ variants to reduce the amount of work
> +	 * required when reading/writing.
> +	 */
> +
> +	if (I915_READ_FW(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) {
>   		DRM_DEBUG_DRIVER("warning: pcode (read) mailbox access failed\n");
>   		return -EAGAIN;
>   	}
>
> -	I915_WRITE(GEN6_PCODE_DATA, *val);
> -	I915_WRITE(GEN6_PCODE_DATA1, 0);
> -	I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox);
> +	I915_WRITE_FW(GEN6_PCODE_DATA, *val);
> +	I915_WRITE_FW(GEN6_PCODE_DATA1, 0);
> +	I915_WRITE_FW(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox);
>
> -	if (wait_for((I915_READ(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) == 0,
> -		     500)) {
> +	if (intel_wait_for_register(dev_priv,
> +				    GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
> +				    500)) {

Why not the _fw version?

Actually brings me to the point I glanced over in the previous patch - 
why would intel_wait_for_register_fw be not suitable for long waits? It 
looks like a concern for the calling code grabbing the fw outside it for 
a long period and not for the function itself. This call site being a 
case in point for that.

>   		DRM_ERROR("timeout waiting for pcode read (%d) to finish\n", mbox);
>   		return -ETIMEDOUT;
>   	}
>
> -	*val = I915_READ(GEN6_PCODE_DATA);
> -	I915_WRITE(GEN6_PCODE_DATA, 0);
> +	*val = I915_READ_FW(GEN6_PCODE_DATA);
> +	I915_WRITE_FW(GEN6_PCODE_DATA, 0);
>
>   	return 0;
>   }
>
> -int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u32 mbox, u32 val)
> +int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
> +			       u32 mbox, u32 val)
>   {
>   	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
>
> -	if (I915_READ(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) {
> +	/* GEN6_PCODE_* are outside of the forcewake domain, we can
> +	 * use te fw I915_READ variants to reduce the amount of work
> +	 * required when reading/writing.
> +	 */
> +
> +	if (I915_READ_FW(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) {
>   		DRM_DEBUG_DRIVER("warning: pcode (write) mailbox access failed\n");
>   		return -EAGAIN;
>   	}
>
> -	I915_WRITE(GEN6_PCODE_DATA, val);
> -	I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox);
> +	I915_WRITE_FW(GEN6_PCODE_DATA, val);
> +	I915_WRITE_FW(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox);
>
> -	if (wait_for((I915_READ(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) == 0,
> -		     500)) {
> +	if (intel_wait_for_register_fw(dev_priv,
> +				       GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
> +				       500)) {

Here you use the _fw variant.

>   		DRM_ERROR("timeout waiting for pcode write (%d) to finish\n", mbox);
>   		return -ETIMEDOUT;
>   	}
>
> -	I915_WRITE(GEN6_PCODE_DATA, 0);
> +	I915_WRITE_FW(GEN6_PCODE_DATA, 0);
>
>   	return 0;
>   }
>

Regards,

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