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@xxxxxxxxxxxxxxx wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
