| Message ID | 1446672017-24497-18-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? 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. -Chris
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.
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@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 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
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; uint32_t size; /* supported gens, 0x10 for 4, 0x30 for 4 and 5, etc. */ uint32_t gen_bitmask; } whitelist[] = { - { RING_TIMESTAMP(RENDER_RING_BASE), 8, GEN_RANGE(4, 9) }, + { .offset = RING_TIMESTAMP(RENDER_RING_BASE), .offset_hi = RING_TIMESTAMP_HI(RENDER_RING_BASE), + .size = 8, .gen_bitmask = GEN_RANGE(4, 9) }, }; int i915_reg_read_ioctl(struct drm_device *dev, @@ -1276,7 +1277,7 @@ int i915_reg_read_ioctl(struct drm_device *dev, struct drm_i915_reg_read *reg = data; struct register_whitelist const *entry = whitelist; unsigned size; - u64 offset; + uint32_t offset, offset_hi; int i, ret = 0; for (i = 0; i < ARRAY_SIZE(whitelist); i++, entry++) { @@ -1293,6 +1294,7 @@ int i915_reg_read_ioctl(struct drm_device *dev, * limit the available flags for that register). */ offset = entry->offset; + offset_hi = entry->offset_hi; size = entry->size; size |= reg->offset ^ offset; @@ -1300,7 +1302,7 @@ int i915_reg_read_ioctl(struct drm_device *dev, switch (size) { case 8 | 1: - reg->val = I915_READ64_2x32(offset, offset+4); + reg->val = I915_READ64_2x32(offset, offset_hi); break; case 8: reg->val = I915_READ64(offset);