[Intel-gfx] [PATCH v3 2/2] drm/i915: Simplify i915_reg_read_ioctl

[Chris Wilson]

Quoting Joonas Lahtinen (2017-09-11 08:57:19)
> 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.
> 
> v2:
> - Lift GEM_BUG_ONs and use is_power_of_2 (Chris)
> - Retain -EINVAL on bad flags behavior (Chris)
> 
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 93 +++++++++++++++++--------------------
>  include/uapi/drm/i915_drm.h         |  6 ++-
>  2 files changed, 46 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 1b38eb94d461..647bacd7c56b 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1292,72 +1292,63 @@ 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) {
> +               GEM_BUG_ON(!is_power_of_2(entry->size));
> +               GEM_BUG_ON(entry->size > 8);
> +
> +               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);
> +       flags = reg->offset & ~i915_mmio_reg_offset(entry->offset_ldw);

The mmio offset is not a mask, so ~not_a_mask is an interesting mix of
bits. So I still think ^ is clearer to extract the bits that differ in
the user offset. If you want to enunciate the interface clearly then
use flags = reg->offset & (entry->size - 1);

>  
>         intel_runtime_pm_get(dev_priv);
> -
> -       switch (size) {
> -       case 8 | 1:
> -               reg->val = I915_READ64_2x32(offset_ldw, offset_udw);
> -               break;
> -       case 8:
> -               reg->val = I915_READ64(offset_ldw);
> -               break;
> -       case 4:
> -               reg->val = I915_READ(offset_ldw);
> -               break;
> -       case 2:
> -               reg->val = I915_READ16(offset_ldw);
> -               break;
> -       case 1:
> -               reg->val = I915_READ8(offset_ldw);
> -               break;
> -       default:
> +       if (entry->size == 8 && flags == I915_REG_READ_8B_WA)

* sheds a tear for the loss of aesthetic beauty ;)

With a second take on flags = offset & blah,
Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
-Chris