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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@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@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'm not sure though if this is something that we are going to be reading
> enough that taking an extra few seconds to trace usage of
> offset/offset_hi is going to matter much.
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> We should probably also document that when passing a reg_read for a u64
> register (that may or may not be split depending on gen) we always
> specify the offset of the lower 32bits.

I'll see about adding a note somewhere.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx