[Intel-gfx] [PATCH 28/71] drm/i915/chv: Added CHV specific register read and write

[Ville Syrjälä]

On Wed, Apr 09, 2014 at 02:16:04PM +0100, Chris Wilson wrote:
> On Wed, Apr 09, 2014 at 01:28:26PM +0300, ville.syrjala at linux.intel.com wrote:
> > From: Deepak S <deepak.s at 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)
> > 
> > Signed-off-by: Deepak S <deepak.s at intel.com>
> > [vsyrjala: Move the register range macros into intel_uncore.c]
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> >  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 823d699..8e3c686 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))
> 
> You could write this as a single stanza of
> 
> if (reg < foo) fwengine = BAR; 
> else if (reg < baz) ...
> 
> which I think would not only help with us reviewing it, but wouldn't
> generate so nearly as bad code.

For me that looks harder to read as you can't see each range neatly on
one line anymore. But you may be right about getting better code out of
it. I must admit I've never checked what kind of code gcc generates
for these things.

Another idea would be to use a switch statement with case ranges, but
that doesn't seem really different to the current approach.

-- 
Ville Syrjälä
Intel OTC