[PATCH v5 1/2] drm/i915: Introduce INTEL_GEN_MASK

[PATCH v5 1/2] drm/i915: Introduce INTEL_GEN_MASK

[Joonas Lahtinen]

Split INTEL_GEN_MASK out of IS_GEN macro, and make it usable
within static declarations (unlike combound statements).

Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 63ca2ffcafef..c3f9d7d7b146 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2873,23 +2873,21 @@ intel_info(const struct drm_i915_private *dev_priv)
 #define INTEL_REVID(dev_priv)	((dev_priv)->drm.pdev->revision)
 
 #define GEN_FOREVER (0)
+
+#define INTEL_GEN_MASK(s, e) ( \
+        BUILD_BUG_ON_ZERO(!__builtin_constant_p(s)) + \
+        BUILD_BUG_ON_ZERO(!__builtin_constant_p(e)) + \
+        GENMASK((e) != GEN_FOREVER ? (e) - 1 : BITS_PER_LONG - 1, \
+	        (s) != GEN_FOREVER ? (s) - 1 : 0) \
+)
+
 /*
  * Returns true if Gen is in inclusive range [Start, End].
  *
  * Use GEN_FOREVER for unbound start and or end.
  */
-#define IS_GEN(dev_priv, s, e) ({ \
-	unsigned int __s = (s), __e = (e); \
-	BUILD_BUG_ON(!__builtin_constant_p(s)); \
-	BUILD_BUG_ON(!__builtin_constant_p(e)); \
-	if ((__s) != GEN_FOREVER) \
-		__s = (s) - 1; \
-	if ((__e) == GEN_FOREVER) \
-		__e = BITS_PER_LONG - 1; \
-	else \
-		__e = (e) - 1; \
-	!!((dev_priv)->info.gen_mask & GENMASK((__e), (__s))); \
-})
+#define IS_GEN(dev_priv, s, e) \
+	(!!((dev_priv)->info.gen_mask & INTEL_GEN_MASK((s), (e))))
 
 /*
  * Return true if revision is in range [since,until] inclusive.
-- 
2.13.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Joonas Lahtinen]

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)

v3:
- Extract flags with 'entry->size - 1' (Chris)

v4:
- Add GEM_BUG_ON on for flags vs entry offset (Chris)

Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_uncore.c | 95 +++++++++++++++++--------------------
 include/uapi/drm/i915_drm.h         |  6 ++-
 2 files changed, 48 insertions(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 1b38eb94d461..92420e0efa04 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1292,72 +1292,65 @@ 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) {
+		u32 entry_offset = i915_mmio_reg_offset(entry->offset_ldw);
+
+		GEM_BUG_ON(!is_power_of_2(entry->size));
+		GEM_BUG_ON(entry->size > 8);
+		GEM_BUG_ON(entry_offset & (entry->size - 1));
+
+		if (INTEL_INFO(dev_priv)->gen_mask & entry->gen_mask &&
+		    entry_offset == (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 & (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)
+		reg->val = I915_READ64_2x32(entry->offset_ldw,
+					    entry->offset_udw);
+	else if (entry->size == 8 && flags == 0)
+		reg->val = I915_READ64(entry->offset_ldw);
+	else if (entry->size == 4 && flags == 0)
+		reg->val = I915_READ(entry->offset_ldw);
+	else if (entry->size == 2 && flags == 0)
+		reg->val = I915_READ16(entry->offset_ldw);
+	else if (entry->size == 1 && flags == 0)
+		reg->val = I915_READ8(entry->offset_ldw);
+	else
 		ret = -EINVAL;
-		goto out;
-	}
-
-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

✓ Fi.CI.BAT: success for series starting with [v5,1/2] drm/i915: Introduce INTEL_GEN_MASK

[Patchwork]

== Series Details ==

Series: series starting with [v5,1/2] drm/i915: Introduce INTEL_GEN_MASK
URL   : https://patchwork.freedesktop.org/series/30113/
State : success

== Summary ==

Series 30113v1 series starting with [v5,1/2] drm/i915: Introduce INTEL_GEN_MASK
https://patchwork.freedesktop.org/api/1.0/series/30113/revisions/1/mbox/

Test chamelium:
        Subgroup dp-crc-fast:
                pass       -> FAIL       (fi-kbl-7500u) fdo#102514
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-legacy:
                pass       -> FAIL       (fi-snb-2600) fdo#100215
Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                pass       -> SKIP       (fi-skl-x1585l) fdo#101781
Test kms_frontbuffer_tracking:
        Subgroup basic:
                pass       -> INCOMPLETE (fi-bsw-n3050) fdo#101707

fdo#102514 https://bugs.freedesktop.org/show_bug.cgi?id=102514
fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215
fdo#101781 https://bugs.freedesktop.org/show_bug.cgi?id=101781
fdo#101707 https://bugs.freedesktop.org/show_bug.cgi?id=101707

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:444s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:455s
fi-blb-e6850     total:289  pass:224  dwarn:1   dfail:0   fail:0   skip:64  time:376s
fi-bsw-n3050     total:225  pass:195  dwarn:0   dfail:0   fail:0   skip:29 
fi-bwr-2160      total:289  pass:184  dwarn:0   dfail:0   fail:0   skip:105 time:271s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:504s
fi-byt-j1900     total:289  pass:254  dwarn:1   dfail:0   fail:0   skip:34  time:500s
fi-byt-n2820     total:289  pass:250  dwarn:1   dfail:0   fail:0   skip:38  time:485s
fi-cfl-s         total:289  pass:250  dwarn:4   dfail:0   fail:0   skip:35  time:448s
fi-elk-e7500     total:289  pass:230  dwarn:0   dfail:0   fail:0   skip:59  time:453s
fi-glk-2a        total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:594s
fi-hsw-4770      total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:426s
fi-hsw-4770r     total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:410s
fi-ilk-650       total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:434s
fi-ivb-3520m     total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:490s
fi-ivb-3770      total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:464s
fi-kbl-7500u     total:289  pass:263  dwarn:1   dfail:0   fail:1   skip:24  time:480s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:582s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:587s
fi-pnv-d510      total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:560s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:465s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:519s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:499s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:461s
fi-skl-x1585l    total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:487s
fi-snb-2520m     total:289  pass:251  dwarn:0   dfail:0   fail:0   skip:38  time:572s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:1   skip:39  time:428s

9ef5732ddf6924e98a5c5d034a3535cff14f72bf drm-tip: 2017y-09m-10d-21h-59m-53s UTC integration manifest
889533e10397 drm/i915: Simplify i915_reg_read_ioctl
1fc08e72da29 drm/i915: Introduce INTEL_GEN_MASK

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5638/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

Quoting Joonas Lahtinen (2017-09-11 10:59:01)
> 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)
> 
> v3:
> - Extract flags with 'entry->size - 1' (Chris)
> 
> v4:
> - Add GEM_BUG_ON on for flags vs entry offset (Chris)
> 
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 95 +++++++++++++++++--------------------
>  include/uapi/drm/i915_drm.h         |  6 ++-
>  2 files changed, 48 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 1b38eb94d461..92420e0efa04 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1292,72 +1292,65 @@ 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) {
> +               u32 entry_offset = i915_mmio_reg_offset(entry->offset_ldw);
> +
> +               GEM_BUG_ON(!is_power_of_2(entry->size));
> +               GEM_BUG_ON(entry->size > 8);
> +               GEM_BUG_ON(entry_offset & (entry->size - 1));
> +
> +               if (INTEL_INFO(dev_priv)->gen_mask & entry->gen_mask &&
> +                   entry_offset == (reg->offset & -entry->size))
>                         break;
> +               entry++;
> +               remain--;
>         }
>  
> -       if (i == ARRAY_SIZE(whitelist))
> +       if (!remain)
>                 return -EINVAL;

Thinking about the abi this would be nicer if it was ENOENT (continuing
our abuse) show that the caller can distinguish between a register not
existing. Bonus marks for -ENODEV.

e.g.:

ret = -ENOENT;
while (remain) {
	if (entry_offset != (reg->offset & -entry->size))
		continue;
	
	if (!INTEL_INFO(dev_priv)->gen_mask & entry->gem_mask) {
		ret = -ENODEV; /* known register, but bad gen */
		continue;
	}

	ret = 0;
	break;
}
if (ret)
	return ret;

Just thinking how to write an agnostic igt, you would start with

	igt_require(read_timestamp(fd) != -ENODEV);

so the test would naturally only run on the right hw. But there may be
some subtlety if registers are reassigned between gen...
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [v5,1/2] drm/i915: Introduce INTEL_GEN_MASK

[Patchwork]

== Series Details ==

Series: series starting with [v5,1/2] drm/i915: Introduce INTEL_GEN_MASK
URL   : https://patchwork.freedesktop.org/series/30113/
State : success

== Summary ==

Test perf:
        Subgroup blocking:
                pass       -> FAIL       (shard-hsw) fdo#102252
Test kms_setmode:
        Subgroup basic:
                pass       -> FAIL       (shard-hsw) fdo#99912

fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912

shard-hsw        total:2302 pass:1237 dwarn:0   dfail:0   fail:13  skip:1052 time:9463s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5638/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx