| Message ID | 20170908092935.1278-2-joonas.lahtinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Joonas Lahtinen (2017-09-08 10:29:35) > Convert to use the freshly available made INTEL_GEN_MASK for easier > grepping and improve function readability and clarify the UABI > documentation. > > No functional changes. > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_uncore.c | 81 ++++++++++++++++++------------------- > include/uapi/drm/i915_drm.h | 6 ++- > 2 files changed, 44 insertions(+), 43 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index 1b38eb94d461..74f135d247a1 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -1292,72 +1292,71 @@ void intel_uncore_fini(struct drm_i915_private *dev_priv) > intel_uncore_forcewake_reset(dev_priv, false); > } > > -#define GEN_RANGE(l, h) GENMASK((h) - 1, (l) - 1) > - > -static const struct register_whitelist { > - i915_reg_t offset_ldw, offset_udw; > - uint32_t size; > - /* supported gens, 0x10 for 4, 0x30 for 4 and 5, etc. */ > - uint32_t gen_bitmask; > -} whitelist[] = { > - { .offset_ldw = RING_TIMESTAMP(RENDER_RING_BASE), > - .offset_udw = RING_TIMESTAMP_UDW(RENDER_RING_BASE), > - .size = 8, .gen_bitmask = GEN_RANGE(4, 10) }, > -}; > +static const struct reg_whitelist { > + i915_reg_t offset_ldw; > + i915_reg_t offset_udw; > + unsigned long gen_mask; > + u8 size; > +} reg_read_whitelist[] = {{ Hmm, Won't {{ look unusual if we ever say add all the other ring timestamps to the white list? Or problem for another day? > + .offset_ldw = RING_TIMESTAMP(RENDER_RING_BASE), > + .offset_udw = RING_TIMESTAMP_UDW(RENDER_RING_BASE), > + .gen_mask = INTEL_GEN_MASK(4, 10), > + .size = 8 > +}}; > > int i915_reg_read_ioctl(struct drm_device *dev, > void *data, struct drm_file *file) > { > struct drm_i915_private *dev_priv = to_i915(dev); > struct drm_i915_reg_read *reg = data; > - struct register_whitelist const *entry = whitelist; > - unsigned size; > - i915_reg_t offset_ldw, offset_udw; > - int i, ret = 0; > - > - for (i = 0; i < ARRAY_SIZE(whitelist); i++, entry++) { > - if (i915_mmio_reg_offset(entry->offset_ldw) == (reg->offset & -entry->size) && > - (INTEL_INFO(dev_priv)->gen_mask & entry->gen_bitmask)) > + struct reg_whitelist const *entry; > + unsigned flags; > + int remain; > + int ret = 0; > + > + entry = reg_read_whitelist; > + remain = ARRAY_SIZE(reg_read_whitelist); > + while (remain) { > + if (INTEL_INFO(dev_priv)->gen_mask & entry->gen_mask && > + i915_mmio_reg_offset(entry->offset_ldw) == > + (reg->offset & -entry->size)) > break; > + entry++; > + remain--; > } > > - if (i == ARRAY_SIZE(whitelist)) > + if (!remain) > return -EINVAL; > > - /* We use the low bits to encode extra flags as the register should > - * be naturally aligned (and those that are not so aligned merely > - * limit the available flags for that register). > - */ > - offset_ldw = entry->offset_ldw; > - offset_udw = entry->offset_udw; > - size = entry->size; > - size |= reg->offset ^ i915_mmio_reg_offset(offset_ldw); > + GEM_BUG_ON(hweight8(entry->size) != 1); > + GEM_BUG_ON(entry->size > 8); Sensible assertions, but we already depending on entry->size being well defined to get to here. So move it up. Also hweight8(x) != 1 is !is_power_of_2(x) > > - intel_runtime_pm_get(dev_priv); > + flags = reg->offset & ~i915_mmio_reg_offset(entry->offset_ldw); > > - switch (size) { > - case 8 | 1: > - reg->val = I915_READ64_2x32(offset_ldw, offset_udw); > - break; > + intel_runtime_pm_get(dev_priv); > + switch (entry->size) { > case 8: > - reg->val = I915_READ64(offset_ldw); > + if (flags & I915_REG_READ_8B_WA) We're losing -EINVAL for the invalid flag combinations. Can I tempt you to use (entry->size | flags)? -Chris
On Fri, 2017-09-08 at 11:13 +0100, Chris Wilson wrote: > Quoting Joonas Lahtinen (2017-09-08 10:29:35) > > Convert to use the freshly available made INTEL_GEN_MASK for easier > > grepping and improve function readability and clarify the UABI > > documentation. > > > > No functional changes. > > > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_uncore.c | 81 ++++++++++++++++++------------------- > > include/uapi/drm/i915_drm.h | 6 ++- > > 2 files changed, 44 insertions(+), 43 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > > index 1b38eb94d461..74f135d247a1 100644 > > --- a/drivers/gpu/drm/i915/intel_uncore.c > > +++ b/drivers/gpu/drm/i915/intel_uncore.c > > @@ -1292,72 +1292,71 @@ void intel_uncore_fini(struct drm_i915_private *dev_priv) > > intel_uncore_forcewake_reset(dev_priv, false); > > } > > > > -#define GEN_RANGE(l, h) GENMASK((h) - 1, (l) - 1) > > - > > -static const struct register_whitelist { > > - i915_reg_t offset_ldw, offset_udw; > > - uint32_t size; > > - /* supported gens, 0x10 for 4, 0x30 for 4 and 5, etc. */ > > - uint32_t gen_bitmask; > > -} whitelist[] = { > > - { .offset_ldw = RING_TIMESTAMP(RENDER_RING_BASE), > > - .offset_udw = RING_TIMESTAMP_UDW(RENDER_RING_BASE), > > - .size = 8, .gen_bitmask = GEN_RANGE(4, 10) }, > > -}; > > +static const struct reg_whitelist { > > + i915_reg_t offset_ldw; > > + i915_reg_t offset_udw; > > + unsigned long gen_mask; > > + u8 size; > > +} reg_read_whitelist[] = {{ > > Hmm, Won't {{ look unusual if we ever say add all the other ring > timestamps to the white list? Or problem for another day? Hmm? whitelist[] = {{ .a = x, .b = y }, { .a = w, .b = z }}; > > > + .offset_ldw = RING_TIMESTAMP(RENDER_RING_BASE), > > + .offset_udw = RING_TIMESTAMP_UDW(RENDER_RING_BASE), > > + .gen_mask = INTEL_GEN_MASK(4, 10), > > + .size = 8 > > +}}; <SNIP> > > - /* We use the low bits to encode extra flags as the register should > > - * be naturally aligned (and those that are not so aligned merely > > - * limit the available flags for that register). > > - */ > > - offset_ldw = entry->offset_ldw; > > - offset_udw = entry->offset_udw; > > - size = entry->size; > > - size |= reg->offset ^ i915_mmio_reg_offset(offset_ldw); > > + GEM_BUG_ON(hweight8(entry->size) != 1); > > + GEM_BUG_ON(entry->size > 8); > > Sensible assertions, but we already depending on entry->size being well > defined to get to here. So move it up. Also hweight8(x) != 1 is > !is_power_of_2(x) Yeah, makes sense. > > > > - intel_runtime_pm_get(dev_priv); > > + flags = reg->offset & ~i915_mmio_reg_offset(entry->offset_ldw); > > > > - switch (size) { > > - case 8 | 1: > > - reg->val = I915_READ64_2x32(offset_ldw, offset_udw); > > - break; > > + intel_runtime_pm_get(dev_priv); > > + switch (entry->size) { > > case 8: > > - reg->val = I915_READ64(offset_ldw); > > + if (flags & I915_REG_READ_8B_WA) > > We're losing -EINVAL for the invalid flag combinations. Can I tempt you > to use (entry->size | flags)? Hmm, I wanted to avoid the masking with 1 and 2 if we get more than one flag. Of course if we assume they won't need flags, we could keep it. switch (entry->size | (flags << 4)) + case 8 | (I915_REG_READ_8B_WA << 4) feels bit like a hack, too. Which one is less confusing? Regards, Joonas
On Fri, Sep 08, 2017 at 12:29:35PM +0300, Joonas Lahtinen wrote: > Convert to use the freshly available made INTEL_GEN_MASK for easier > grepping and improve function readability and clarify the UABI > documentation. > > No functional changes. > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_uncore.c | 81 ++++++++++++++++++------------------- > include/uapi/drm/i915_drm.h | 6 ++- > 2 files changed, 44 insertions(+), 43 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index 1b38eb94d461..74f135d247a1 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -1292,72 +1292,71 @@ void intel_uncore_fini(struct drm_i915_private *dev_priv) > intel_uncore_forcewake_reset(dev_priv, false); > } > > -#define GEN_RANGE(l, h) GENMASK((h) - 1, (l) - 1) > - > -static const struct register_whitelist { > - i915_reg_t offset_ldw, offset_udw; > - uint32_t size; > - /* supported gens, 0x10 for 4, 0x30 for 4 and 5, etc. */ > - uint32_t gen_bitmask; > -} whitelist[] = { > - { .offset_ldw = RING_TIMESTAMP(RENDER_RING_BASE), > - .offset_udw = RING_TIMESTAMP_UDW(RENDER_RING_BASE), > - .size = 8, .gen_bitmask = GEN_RANGE(4, 10) }, > -}; > +static const struct reg_whitelist { > + i915_reg_t offset_ldw; > + i915_reg_t offset_udw; > + unsigned long gen_mask; 'long' seems like a bad type for something like this. Changes size on 32 vs 64 bit. Also we could make do with 16 bits for now, though with the single whitelist entry this only has a small impact. > + u8 size; > +} reg_read_whitelist[] = {{ > + .offset_ldw = RING_TIMESTAMP(RENDER_RING_BASE), > + .offset_udw = RING_TIMESTAMP_UDW(RENDER_RING_BASE), > + .gen_mask = INTEL_GEN_MASK(4, 10), > + .size = 8 > +}}; > > int i915_reg_read_ioctl(struct drm_device *dev, > void *data, struct drm_file *file) > { > struct drm_i915_private *dev_priv = to_i915(dev); > struct drm_i915_reg_read *reg = data; > - struct register_whitelist const *entry = whitelist; > - unsigned size; > - i915_reg_t offset_ldw, offset_udw; > - int i, ret = 0; > - > - for (i = 0; i < ARRAY_SIZE(whitelist); i++, entry++) { > - if (i915_mmio_reg_offset(entry->offset_ldw) == (reg->offset & -entry->size) && > - (INTEL_INFO(dev_priv)->gen_mask & entry->gen_bitmask)) > + struct reg_whitelist const *entry; > + unsigned flags; > + int remain; > + int ret = 0; > + > + entry = reg_read_whitelist; > + remain = ARRAY_SIZE(reg_read_whitelist); > + while (remain) { > + if (INTEL_INFO(dev_priv)->gen_mask & entry->gen_mask && > + i915_mmio_reg_offset(entry->offset_ldw) == > + (reg->offset & -entry->size)) > break; > + entry++; > + remain--; > } > > - if (i == ARRAY_SIZE(whitelist)) > + if (!remain) > return -EINVAL; > > - /* We use the low bits to encode extra flags as the register should > - * be naturally aligned (and those that are not so aligned merely > - * limit the available flags for that register). > - */ > - offset_ldw = entry->offset_ldw; > - offset_udw = entry->offset_udw; > - size = entry->size; > - size |= reg->offset ^ i915_mmio_reg_offset(offset_ldw); > + GEM_BUG_ON(hweight8(entry->size) != 1); > + GEM_BUG_ON(entry->size > 8); > > - intel_runtime_pm_get(dev_priv); > + flags = reg->offset & ~i915_mmio_reg_offset(entry->offset_ldw); > > - switch (size) { > - case 8 | 1: > - reg->val = I915_READ64_2x32(offset_ldw, offset_udw); > - break; > + intel_runtime_pm_get(dev_priv); > + switch (entry->size) { > case 8: > - reg->val = I915_READ64(offset_ldw); > + if (flags & I915_REG_READ_8B_WA) > + reg->val = I915_READ64_2x32(entry->offset_ldw, > + entry->offset_udw); > + else > + reg->val = I915_READ64(entry->offset_ldw); > break; > case 4: > - reg->val = I915_READ(offset_ldw); > + reg->val = I915_READ(entry->offset_ldw); > break; > case 2: > - reg->val = I915_READ16(offset_ldw); > + reg->val = I915_READ16(entry->offset_ldw); > break; > case 1: > - reg->val = I915_READ8(offset_ldw); > + reg->val = I915_READ8(entry->offset_ldw); > break; > default: > ret = -EINVAL; > - goto out; > + break; > } > - > -out: > intel_runtime_pm_put(dev_priv); > + > return ret; > } > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index d8d10d932759..b4505d55990d 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -1308,14 +1308,16 @@ struct drm_i915_reg_read { > * be specified > */ > __u64 offset; > +#define I915_REG_READ_8B_WA BIT(0) > + > __u64 val; /* Return value */ > }; > /* Known registers: > * > * Render engine timestamp - 0x2358 + 64bit - gen7+ > * - Note this register returns an invalid value if using the default > - * single instruction 8byte read, in order to workaround that use > - * offset (0x2538 | 1) instead. > + * single instruction 8byte read, in order to workaround that pass > + * flag I915_REG_READ_8B_WA in offset field. > * > */ > > -- > 2.13.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, 2017-09-08 at 15:24 +0300, Ville Syrjälä wrote: > On Fri, Sep 08, 2017 at 12:29:35PM +0300, Joonas Lahtinen wrote: > > @@ -1292,72 +1292,71 @@ void intel_uncore_fini(struct drm_i915_private *dev_priv) > > intel_uncore_forcewake_reset(dev_priv, false); > > } > > > > -#define GEN_RANGE(l, h) GENMASK((h) - 1, (l) - 1) > > - > > -static const struct register_whitelist { > > - i915_reg_t offset_ldw, offset_udw; > > - uint32_t size; > > - /* supported gens, 0x10 for 4, 0x30 for 4 and 5, etc. */ > > - uint32_t gen_bitmask; > > -} whitelist[] = { > > - { .offset_ldw = RING_TIMESTAMP(RENDER_RING_BASE), > > - .offset_udw = RING_TIMESTAMP_UDW(RENDER_RING_BASE), > > - .size = 8, .gen_bitmask = GEN_RANGE(4, 10) }, > > -}; > > +static const struct reg_whitelist { > > + i915_reg_t offset_ldw; > > + i915_reg_t offset_udw; > > + unsigned long gen_mask; > > 'long' seems like a bad type for something like this. Changes size on 32 > vs 64 bit. Also we could make do with 16 bits for now, though with > the single whitelist entry this only has a small impact. It's actually the type GCC wants because GENMASK is used internally to produce the mask, otherwise it complained. We should likely use unsigned long in dev_priv too, with a BUILD_BUG_ON(max_gen > BITS_PER_LONG) check. Regards, Joonas
On Mon, Sep 11, 2017 at 01:49:18PM +0300, Joonas Lahtinen wrote: > On Fri, 2017-09-08 at 15:24 +0300, Ville Syrjälä wrote: > > On Fri, Sep 08, 2017 at 12:29:35PM +0300, Joonas Lahtinen wrote: > > > @@ -1292,72 +1292,71 @@ void intel_uncore_fini(struct drm_i915_private *dev_priv) > > > intel_uncore_forcewake_reset(dev_priv, false); > > > } > > > > > > -#define GEN_RANGE(l, h) GENMASK((h) - 1, (l) - 1) > > > - > > > -static const struct register_whitelist { > > > - i915_reg_t offset_ldw, offset_udw; > > > - uint32_t size; > > > - /* supported gens, 0x10 for 4, 0x30 for 4 and 5, etc. */ > > > - uint32_t gen_bitmask; > > > -} whitelist[] = { > > > - { .offset_ldw = RING_TIMESTAMP(RENDER_RING_BASE), > > > - .offset_udw = RING_TIMESTAMP_UDW(RENDER_RING_BASE), > > > - .size = 8, .gen_bitmask = GEN_RANGE(4, 10) }, > > > -}; > > > +static const struct reg_whitelist { > > > + i915_reg_t offset_ldw; > > > + i915_reg_t offset_udw; > > > + unsigned long gen_mask; > > > > 'long' seems like a bad type for something like this. Changes size on 32 > > vs 64 bit. Also we could make do with 16 bits for now, though with > > the single whitelist entry this only has a small impact. > > It's actually the type GCC wants because GENMASK is used internally to > produce the mask, otherwise it complained. Hmm. Complained about what? We're not truncating anything with an explicit smaller type, so I have a hard time seeing why gcc would object to it.
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 1b38eb94d461..74f135d247a1 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -1292,72 +1292,71 @@ void intel_uncore_fini(struct drm_i915_private *dev_priv) intel_uncore_forcewake_reset(dev_priv, false); } -#define GEN_RANGE(l, h) GENMASK((h) - 1, (l) - 1) - -static const struct register_whitelist { - i915_reg_t offset_ldw, offset_udw; - uint32_t size; - /* supported gens, 0x10 for 4, 0x30 for 4 and 5, etc. */ - uint32_t gen_bitmask; -} whitelist[] = { - { .offset_ldw = RING_TIMESTAMP(RENDER_RING_BASE), - .offset_udw = RING_TIMESTAMP_UDW(RENDER_RING_BASE), - .size = 8, .gen_bitmask = GEN_RANGE(4, 10) }, -}; +static const struct reg_whitelist { + i915_reg_t offset_ldw; + i915_reg_t offset_udw; + unsigned long gen_mask; + u8 size; +} reg_read_whitelist[] = {{ + .offset_ldw = RING_TIMESTAMP(RENDER_RING_BASE), + .offset_udw = RING_TIMESTAMP_UDW(RENDER_RING_BASE), + .gen_mask = INTEL_GEN_MASK(4, 10), + .size = 8 +}}; int i915_reg_read_ioctl(struct drm_device *dev, void *data, struct drm_file *file) { struct drm_i915_private *dev_priv = to_i915(dev); struct drm_i915_reg_read *reg = data; - struct register_whitelist const *entry = whitelist; - unsigned size; - i915_reg_t offset_ldw, offset_udw; - int i, ret = 0; - - for (i = 0; i < ARRAY_SIZE(whitelist); i++, entry++) { - if (i915_mmio_reg_offset(entry->offset_ldw) == (reg->offset & -entry->size) && - (INTEL_INFO(dev_priv)->gen_mask & entry->gen_bitmask)) + struct reg_whitelist const *entry; + unsigned flags; + int remain; + int ret = 0; + + entry = reg_read_whitelist; + remain = ARRAY_SIZE(reg_read_whitelist); + while (remain) { + if (INTEL_INFO(dev_priv)->gen_mask & entry->gen_mask && + i915_mmio_reg_offset(entry->offset_ldw) == + (reg->offset & -entry->size)) break; + entry++; + remain--; } - if (i == ARRAY_SIZE(whitelist)) + if (!remain) return -EINVAL; - /* We use the low bits to encode extra flags as the register should - * be naturally aligned (and those that are not so aligned merely - * limit the available flags for that register). - */ - offset_ldw = entry->offset_ldw; - offset_udw = entry->offset_udw; - size = entry->size; - size |= reg->offset ^ i915_mmio_reg_offset(offset_ldw); + GEM_BUG_ON(hweight8(entry->size) != 1); + GEM_BUG_ON(entry->size > 8); - intel_runtime_pm_get(dev_priv); + flags = reg->offset & ~i915_mmio_reg_offset(entry->offset_ldw); - switch (size) { - case 8 | 1: - reg->val = I915_READ64_2x32(offset_ldw, offset_udw); - break; + intel_runtime_pm_get(dev_priv); + switch (entry->size) { case 8: - reg->val = I915_READ64(offset_ldw); + if (flags & I915_REG_READ_8B_WA) + reg->val = I915_READ64_2x32(entry->offset_ldw, + entry->offset_udw); + else + reg->val = I915_READ64(entry->offset_ldw); break; case 4: - reg->val = I915_READ(offset_ldw); + reg->val = I915_READ(entry->offset_ldw); break; case 2: - reg->val = I915_READ16(offset_ldw); + reg->val = I915_READ16(entry->offset_ldw); break; case 1: - reg->val = I915_READ8(offset_ldw); + reg->val = I915_READ8(entry->offset_ldw); break; default: ret = -EINVAL; - goto out; + break; } - -out: intel_runtime_pm_put(dev_priv); + return ret; } diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index d8d10d932759..b4505d55990d 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -1308,14 +1308,16 @@ struct drm_i915_reg_read { * be specified */ __u64 offset; +#define I915_REG_READ_8B_WA BIT(0) + __u64 val; /* Return value */ }; /* Known registers: * * Render engine timestamp - 0x2358 + 64bit - gen7+ * - Note this register returns an invalid value if using the default - * single instruction 8byte read, in order to workaround that use - * offset (0x2538 | 1) instead. + * single instruction 8byte read, in order to workaround that pass + * flag I915_REG_READ_8B_WA in offset field. * */
Convert to use the freshly available made INTEL_GEN_MASK for easier grepping and improve function readability and clarify the UABI documentation. No functional changes. Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> --- drivers/gpu/drm/i915/intel_uncore.c | 81 ++++++++++++++++++------------------- include/uapi/drm/i915_drm.h | 6 ++- 2 files changed, 44 insertions(+), 43 deletions(-)