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@xxxxxxxxx>
> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> > ---
> > 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
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
