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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx