Re: [PATCH 2/2] drm/i915: Simplify i915_reg_read_ioctl

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx