Re: [PATCH 04/10] drm/i915/chv: Added CHV specific register read and write

[Ben Widawsky <ben@bwidawsk.net>]

On Mon, Apr 21, 2014 at 01:34:08PM +0530, deepak.s@linux.intel.com wrote:
> From: Deepak S <deepak.s@linux.intel.com>
> 
> Support to individually control Media/Render well based on the register access.
> Add CHV specific write function to habdle difference between registers
> that are sadowed vs those that need forcewake even for writes.
> 
> v2: Drop write FIFO for CHV and add comman well forcewake (Ville)
> 
> v3: Fix for decrementing fw count in chv read/write. (Deepak)
> 
> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> [vsyrjala: Move the register range macros into intel_uncore.c]
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I left some comments on the first sending of this patch, and it's not
clear if you ignored them intentionally or not. Inquiring minds would
like to know.

I'll repeat some of the ones I feel are more important below.

> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 139 +++++++++++++++++++++++++++++++++---
>  1 file changed, 131 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 2a72bab..11741e4 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -495,6 +495,31 @@ void assert_force_wake_inactive(struct drm_i915_private *dev_priv)
>  	((reg) >= 0x22000 && (reg) < 0x24000) ||\
>  	((reg) >= 0x30000 && (reg) < 0x40000))
>  
> +#define FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg) \
> +	(((reg) >= 0x2000 && (reg) < 0x4000) ||\
> +	((reg) >= 0x5000 && (reg) < 0x8000) ||\
> +	((reg) >= 0x8300 && (reg) < 0x8500) ||\
> +	((reg) >= 0xB000 && (reg) < 0xC000) ||\
> +	((reg) >= 0xE000 && (reg) < 0xE800))
> +
> +#define FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)\
> +	(((reg) >= 0x8800 && (reg) < 0x8900) ||\
> +	((reg) >= 0xD000 && (reg) < 0xD800) ||\
> +	((reg) >= 0x12000 && (reg) < 0x14000) ||\
> +	((reg) >= 0x1A000 && (reg) < 0x1C000) ||\
> +	((reg) >= 0x1E800 && (reg) < 0x1EA00) ||\
> +	((reg) >= 0x30000 && (reg) < 0x40000))
> +
> +#define FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)\
> +	(((reg) >= 0x4000 && (reg) < 0x5000) ||\
> +	((reg) >= 0x8000 && (reg) < 0x8300) ||\
> +	((reg) >= 0x8500 && (reg) < 0x8600) ||\
> +	((reg) >= 0x9000 && (reg) < 0xB000) ||\
> +	((reg) >= 0xC000 && (reg) < 0xc800) ||\
> +	((reg) >= 0xF000 && (reg) < 0x10000) ||\
> +	((reg) >= 0x14000 && (reg) < 0x14400) ||\
> +	((reg) >= 0x22000 && (reg) < 0x24000))
> +
>  static void
>  ilk_dummy_write(struct drm_i915_private *dev_priv)
>  {
> @@ -588,7 +613,48 @@ vlv_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
>  	REG_READ_FOOTER; \
>  }
>  
> +#define __chv_read(x) \
> +static u##x \
> +chv_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
> +	unsigned fwengine = 0; \
> +	REG_READ_HEADER(x); \
> +	if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg)) { \
> +		fwengine = FORCEWAKE_RENDER; \
> +	} \
> +	else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)) { \
> +		fwengine = FORCEWAKE_MEDIA; \
> +	} \
> +	else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)) { \
> +		fwengine = FORCEWAKE_ALL; \
> +	} \

These don't following linux kernel coding style

> +	if (FORCEWAKE_RENDER & fwengine) { \
> +		if (dev_priv->uncore.fw_rendercount++ == 0) \
> +			(dev_priv)->uncore.funcs.force_wake_get(dev_priv, \
> +								fwengine); \
> +	} \
> +	if (FORCEWAKE_MEDIA & fwengine) { \
> +		if (dev_priv->uncore.fw_mediacount++ == 0) \
> +			(dev_priv)->uncore.funcs.force_wake_get(dev_priv, \
> +								fwengine); \
> +	} \
> +	val = __raw_i915_read##x(dev_priv, reg); \
> +	if (FORCEWAKE_RENDER & fwengine) { \
> +		if (--dev_priv->uncore.fw_rendercount == 0) \
> +			(dev_priv)->uncore.funcs.force_wake_put(dev_priv, \
> +								fwengine); \
> +	} \
> +	if (FORCEWAKE_MEDIA & fwengine) { \
> +		if (--dev_priv->uncore.fw_mediacount == 0) \
> +			(dev_priv)->uncore.funcs.force_wake_put(dev_priv, \
> +								fwengine); \
> +	} \
> +	REG_READ_FOOTER; \
> +}

It seems like it makes a lot more sense to pass register offset to
force_wake_put() and let the logic occur there instead of making a big
ugly macro. We can cheat and use the existing functions since fw_engine
was defined as an int, and the register range fits within that.

>  
> +__chv_read(8)
> +__chv_read(16)
> +__chv_read(32)
> +__chv_read(64)
>  __vlv_read(8)
>  __vlv_read(16)
>  __vlv_read(32)
> @@ -606,6 +672,7 @@ __gen4_read(16)
>  __gen4_read(32)
>  __gen4_read(64)
>  
> +#undef __chv_read
>  #undef __vlv_read
>  #undef __gen6_read
>  #undef __gen5_read
> @@ -710,6 +777,49 @@ gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace
>  	REG_WRITE_FOOTER; \
>  }
>  
> +#define __chv_write(x) \
> +static void \
> +chv_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
> +	unsigned fwengine = 0; \
> +	bool __needs_put = !is_gen8_shadowed(dev_priv, reg); \
> +	REG_WRITE_HEADER; \
> +	if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg)) { \
> +		fwengine = FORCEWAKE_RENDER; \
> +	} \
> +	else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)) { \
> +		fwengine = FORCEWAKE_MEDIA; \
> +	} \
> +	else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)) { \
> +		fwengine = FORCEWAKE_ALL; \
> +	} \

coding convention again

> +	if (__needs_put && (FORCEWAKE_RENDER & fwengine)) { \
> +			if (dev_priv->uncore.fw_rendercount++ == 0) \
> +				(dev_priv)->uncore.funcs.force_wake_get(dev_priv, \
> +									fwengine); \
> +	} \
> +	if (__needs_put && (FORCEWAKE_MEDIA & fwengine)) { \
> +		if (dev_priv->uncore.fw_mediacount++ == 0) \
> +			(dev_priv)->uncore.funcs.force_wake_get(dev_priv, \
> +								fwengine); \
> +	} \
> +	__raw_i915_write##x(dev_priv, reg, val); \
> +	if (__needs_put && (FORCEWAKE_RENDER & fwengine)) { \
> +			if (--dev_priv->uncore.fw_rendercount == 0) \
> +				(dev_priv)->uncore.funcs.force_wake_put(dev_priv, \
> +									fwengine); \
> +	} \
> +	if (__needs_put && (FORCEWAKE_MEDIA & fwengine)) { \
> +		if (--dev_priv->uncore.fw_mediacount == 0) \
> +			(dev_priv)->uncore.funcs.force_wake_put(dev_priv, \
> +								fwengine); \
> +	} \

Same comment above

> +	REG_WRITE_FOOTER; \
> +}
> +
> +__chv_write(8)
> +__chv_write(16)
> +__chv_write(32)
> +__chv_write(64)
>  __gen8_write(8)
>  __gen8_write(16)
>  __gen8_write(32)
> @@ -731,6 +841,7 @@ __gen4_write(16)
>  __gen4_write(32)
>  __gen4_write(64)
>  
> +#undef __chv_write
>  #undef __gen8_write
>  #undef __hsw_write
>  #undef __gen6_write
> @@ -794,14 +905,26 @@ void intel_uncore_init(struct drm_device *dev)
>  
>  	switch (INTEL_INFO(dev)->gen) {
>  	default:
> -		dev_priv->uncore.funcs.mmio_writeb  = gen8_write8;
> -		dev_priv->uncore.funcs.mmio_writew  = gen8_write16;
> -		dev_priv->uncore.funcs.mmio_writel  = gen8_write32;
> -		dev_priv->uncore.funcs.mmio_writeq  = gen8_write64;
> -		dev_priv->uncore.funcs.mmio_readb  = gen6_read8;
> -		dev_priv->uncore.funcs.mmio_readw  = gen6_read16;
> -		dev_priv->uncore.funcs.mmio_readl  = gen6_read32;
> -		dev_priv->uncore.funcs.mmio_readq  = gen6_read64;
> +		if (IS_CHERRYVIEW(dev)) {
> +			dev_priv->uncore.funcs.mmio_writeb  = chv_write8;
> +			dev_priv->uncore.funcs.mmio_writew  = chv_write16;
> +			dev_priv->uncore.funcs.mmio_writel  = chv_write32;
> +			dev_priv->uncore.funcs.mmio_writeq  = chv_write64;
> +			dev_priv->uncore.funcs.mmio_readb  = chv_read8;
> +			dev_priv->uncore.funcs.mmio_readw  = chv_read16;
> +			dev_priv->uncore.funcs.mmio_readl  = chv_read32;
> +			dev_priv->uncore.funcs.mmio_readq  = chv_read64;
> +
> +		} else {
> +			dev_priv->uncore.funcs.mmio_writeb  = gen8_write8;
> +			dev_priv->uncore.funcs.mmio_writew  = gen8_write16;
> +			dev_priv->uncore.funcs.mmio_writel  = gen8_write32;
> +			dev_priv->uncore.funcs.mmio_writeq  = gen8_write64;
> +			dev_priv->uncore.funcs.mmio_readb  = gen6_read8;
> +			dev_priv->uncore.funcs.mmio_readw  = gen6_read16;
> +			dev_priv->uncore.funcs.mmio_readl  = gen6_read32;
> +			dev_priv->uncore.funcs.mmio_readq  = gen6_read64;
> +		}
>  		break;
>  	case 7:
>  	case 6:

I think if you make CHV have it's own forcewake get/put, then you can
just use the the same MMIO functions.

Like the previous patch, I am having trouble finding where the register
ranges exist. Assuming those are correct (and I'm guessing Ville checked
thoroughly), the patch looks correct.

So I'd prefer to at least explore my suggestions, but it's
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx