Re: [PATCH 3/6] drm/i915: Use the common register access functions for NOTRACE variants

[Ben Widawsky <ben@bwidawsk.net>]

On Fri, Jul 12, 2013 at 03:02:33PM +0100, Chris Wilson wrote:
> Detangle the confusion that NOTRACE variants of the register read/write
> routines were directly using the raw register access. We need for those
> routines to reuse the common code for serializing register access and
> ensuring the correct register power states. This is only possible now
> that the only routines that required raw access use their own API.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_drv.h | 28 ++++++++++++++--------------
>  drivers/gpu/drm/i915/intel_gt.c |  8 ++++----
>  2 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f140b04..1ab6d4e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2082,7 +2082,7 @@ int vlv_gpu_freq(int ddr_freq, int val);
>  int vlv_freq_opcode(int ddr_freq, int val);
>  
>  #define __i915_read(x, y) \
> -	u##x i915_read##x(struct drm_i915_private *dev_priv, u32 reg);
> +	u##x i915_read##x(struct drm_i915_private *dev_priv, u32 reg, bool trace);
>  
>  __i915_read(8, b)
>  __i915_read(16, w)
> @@ -2091,7 +2091,7 @@ __i915_read(64, q)
>  #undef __i915_read
>  
>  #define __i915_write(x, y) \
> -	void i915_write##x(struct drm_i915_private *dev_priv, u32 reg, u##x val);
> +	void i915_write##x(struct drm_i915_private *dev_priv, u32 reg, u##x val, bool trace);
>  
>  __i915_write(8, b)
>  __i915_write(16, w)
> @@ -2099,21 +2099,21 @@ __i915_write(32, l)
>  __i915_write(64, q)
>  #undef __i915_write
>  
> -#define I915_READ8(reg)		i915_read8(dev_priv, (reg))
> -#define I915_WRITE8(reg, val)	i915_write8(dev_priv, (reg), (val))
> +#define I915_READ8(reg)		i915_read8(dev_priv, (reg), true)
> +#define I915_WRITE8(reg, val)	i915_write8(dev_priv, (reg), (val), true)
>  
> -#define I915_READ16(reg)	i915_read16(dev_priv, (reg))
> -#define I915_WRITE16(reg, val)	i915_write16(dev_priv, (reg), (val))
> -#define I915_READ16_NOTRACE(reg)	readw(dev_priv->regs + (reg))
> -#define I915_WRITE16_NOTRACE(reg, val)	writew(val, dev_priv->regs + (reg))
> +#define I915_READ16(reg)	i915_read16(dev_priv, (reg), true)
> +#define I915_WRITE16(reg, val)	i915_write16(dev_priv, (reg), (val), true)
> +#define I915_READ16_NOTRACE(reg)	i915_read16(dev_priv, (reg), false)
> +#define I915_WRITE16_NOTRACE(reg, val)	i915_write16(dev_priv, (reg), (val), false)
>  
> -#define I915_READ(reg)		i915_read32(dev_priv, (reg))
> -#define I915_WRITE(reg, val)	i915_write32(dev_priv, (reg), (val))
> -#define I915_READ_NOTRACE(reg)		readl(dev_priv->regs + (reg))
> -#define I915_WRITE_NOTRACE(reg, val)	writel(val, dev_priv->regs + (reg))
> +#define I915_READ(reg)		i915_read32(dev_priv, (reg), true)
> +#define I915_WRITE(reg, val)	i915_write32(dev_priv, (reg), (val), true)
> +#define I915_READ_NOTRACE(reg)		i915_read32(dev_priv, (reg), false)
> +#define I915_WRITE_NOTRACE(reg, val)	i915_write32(dev_priv, (reg), (val), false)
>  
> -#define I915_WRITE64(reg, val)	i915_write64(dev_priv, (reg), (val))
> -#define I915_READ64(reg)	i915_read64(dev_priv, (reg))
> +#define I915_WRITE64(reg, val)	i915_write64(dev_priv, (reg), (val), true)
> +#define I915_READ64(reg)	i915_read64(dev_priv, (reg), true)
>  
>  #define POSTING_READ(reg)	(void)I915_READ_NOTRACE(reg)
>  #define POSTING_READ16(reg)	(void)I915_READ16_NOTRACE(reg)
> diff --git a/drivers/gpu/drm/i915/intel_gt.c b/drivers/gpu/drm/i915/intel_gt.c
> index cb3116c..d4bc7f4 100644
> --- a/drivers/gpu/drm/i915/intel_gt.c
> +++ b/drivers/gpu/drm/i915/intel_gt.c
> @@ -330,7 +330,7 @@ hsw_unclaimed_reg_check(struct drm_i915_private *dev_priv, u32 reg)
>  }
>  
>  #define __i915_read(x, y) \
> -u##x i915_read##x(struct drm_i915_private *dev_priv, u32 reg) { \
> +u##x i915_read##x(struct drm_i915_private *dev_priv, u32 reg, bool trace) { \
>  	u##x val = 0; \
>  	if (IS_GEN5(dev_priv->dev)) \
>  		ilk_dummy_write(dev_priv); \
> @@ -346,7 +346,7 @@ u##x i915_read##x(struct drm_i915_private *dev_priv, u32 reg) { \
>  	} else { \
>  		val = read##y(dev_priv->regs + reg); \
>  	} \
> -	trace_i915_reg_rw(false, reg, val, sizeof(val)); \
> +	if (trace) trace_i915_reg_rw(false, reg, val, sizeof(val)); \
>  	return val; \
>  }
>  
> @@ -357,9 +357,9 @@ __i915_read(64, q)
>  #undef __i915_read
>  
>  #define __i915_write(x, y) \
> -void i915_write##x(struct drm_i915_private *dev_priv, u32 reg, u##x val) { \
> +void i915_write##x(struct drm_i915_private *dev_priv, u32 reg, u##x val, bool trace) { \
>  	u32 __fifo_ret = 0; \
> -	trace_i915_reg_rw(true, reg, val, sizeof(val)); \
> +	if (trace) trace_i915_reg_rw(true, reg, val, sizeof(val)); \
>  	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
>  		__fifo_ret = __gen6_gt_wait_for_fifo(dev_priv); \
>  	} \


if (unlikely(trace))? taking a hit on the tracing case seems like what
you want... but I never know the status of such compiler flags.

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

-- 
Ben Widawsky, Intel Open Source Technology Center