[Intel-gfx] [PATCH 17/29] drm/i915: Make the high dword offset more explicit in i915_reg_read_ioctl

[Chris Wilson]

On Thu, Nov 05, 2015 at 01:41:25PM +0200, Ville Syrjälä wrote:
> On Thu, Nov 05, 2015 at 10:59:05AM +0000, Chris Wilson wrote:
> > On Wed, Nov 04, 2015 at 11:20:05PM +0200, ville.syrjala at linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > 
> > > Store the upper dword of the register offset in the whitelist as well.
> > > This would allow it to read register where the two halves aren't sitting
> > > right next to each other, and it'll make it easier to make register
> > > access type safe.
> > > 
> > > While at it change the register offsets to u32 from u64. Our register
> > > space isn't quite that big, yet :)
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h     |  1 +
> > >  drivers/gpu/drm/i915/intel_uncore.c | 10 ++++++----
> > >  2 files changed, 7 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 0510ca1..7cea51d 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -1567,6 +1567,7 @@ enum skl_disp_power_wells {
> > >  #define RING_IMR(base)		((base)+0xa8)
> > >  #define RING_HWSTAM(base)	((base)+0x98)
> > >  #define RING_TIMESTAMP(base)	((base)+0x358)
> > > +#define RING_TIMESTAMP_HI(base)	((base)+0x358 + 4)
> > >  #define   TAIL_ADDR		0x001FFFF8
> > >  #define   HEAD_WRAP_COUNT	0xFFE00000
> > >  #define   HEAD_WRAP_ONE		0x00200000
> > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > > index f0f97b2..ced494a 100644
> > > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > > @@ -1261,12 +1261,13 @@ void intel_uncore_fini(struct drm_device *dev)
> > >  #define GEN_RANGE(l, h) GENMASK(h, l)
> > >  
> > >  static const struct register_whitelist {
> > > -	uint64_t offset;
> > > +	uint32_t offset, offset_hi;
> > 
> > Hmm, fwiw I was confused here thinking that you were storing a 64bit
> > value split across the two u32. (I know that's silly but it has been
> > common enough in the past.) Maybe offset_ldw and offset_udw?
> 
> Hmm. Yeah, I suppose I've been rather inconsistent with the low/high
> dword stuff. Although some inconsistency was already there before I
> started I think. Should we try to standardize on ldw/udw everywhere?
> 
> And what about cases where we had the ldw only on olders gens, and
> the udw got added later, do we still want to put the _LDW suffix in
> there, or just have FOO and FOO_UDW?

I think the sensible approach is to try and be consistent going forwards
(and gradually drag the historical baggage into line). We can use the
convention that an unsuffixed REG is a plain 32bit, and that all 64bit
registers should be REG_LDW + REG_UDW (or, too late now, REG_L32 +
REG_U32). I don't think we need to add _LDW to new 32bit registers, as
it should be clear by implication and allows us to keep the name closer
to spec. (There is perhaps an argument that we should use REG_LO +
REG_HI as there are quite a few of those as well, but imho _LO / _HI are
a little too ambiguous - they may be part of the real register name.)

>From that pov, using u32 offset_ldw offset_udw; here is best.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre