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

[PATCH 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).

Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 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 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.

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[] = {{
+	.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

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

[Jani Nikula]

On Fri, 08 Sep 2017, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote:
> Split INTEL_GEN_MASK out of IS_GEN macro, and make it usable
> within static declarations (unlike combound statements).
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  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) \

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

but I'd really like a patch on top to remove the -1 from here and
info->gen_mask.

> +)
> +
>  /*
>   * 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))))

Actually, why do we even have info->gen_mask? It'll only ever have one
bit set, and it's duplication of information. Why don't we use
BIT((dev_priv)->info.gen) here?

BR,
Jani.

>  
>  /*
>   * Return true if revision is in range [since,until] inclusive.

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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?

> +       .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);

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)
>  
> -       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)?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915: Introduce INTEL_GEN_MASK

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Introduce INTEL_GEN_MASK
URL   : https://patchwork.freedesktop.org/series/30005/
State : failure

== Summary ==

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

Test gem_ringfill:
        Subgroup basic-default-hang:
                none       -> INCOMPLETE (fi-pnv-d510) fdo#101600
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-legacy:
                pass       -> FAIL       (fi-snb-2600) fdo#100215
        Subgroup basic-flip-before-cursor-legacy:
                pass       -> INCOMPLETE (fi-byt-j1900)
Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                pass       -> SKIP       (fi-skl-x1585l) fdo#101781

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

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:455s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:442s
fi-blb-e6850     total:289  pass:224  dwarn:1   dfail:0   fail:0   skip:64  time:358s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:559s
fi-bwr-2160      total:289  pass:184  dwarn:0   dfail:0   fail:0   skip:105 time:255s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:515s
fi-byt-j1900     total:215  pass:190  dwarn:0   dfail:0   fail:0   skip:24 
fi-byt-n2820     total:289  pass:250  dwarn:1   dfail:0   fail:0   skip:38  time:516s
fi-cfl-s         total:289  pass:250  dwarn:4   dfail:0   fail:0   skip:35  time:462s
fi-elk-e7500     total:289  pass:230  dwarn:0   dfail:0   fail:0   skip:59  time:441s
fi-glk-2a        total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:616s
fi-hsw-4770      total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:446s
fi-hsw-4770r     total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:423s
fi-ilk-650       total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:428s
fi-ivb-3520m     total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:502s
fi-ivb-3770      total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:473s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:509s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:599s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:607s
fi-pnv-d510      total:156  pass:113  dwarn:0   dfail:0   fail:0   skip:42 
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:472s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:538s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:522s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:448s
fi-skl-x1585l    total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:493s
fi-snb-2520m     total:289  pass:251  dwarn:0   dfail:0   fail:0   skip:38  time:564s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:1   skip:39  time:405s

16fef66706a3fcdd1d87009beab7e09de1f32807 drm-tip: 2017y-09m-08d-08h-41m-49s UTC integration manifest
023b58446065 drm/i915: Simplify i915_reg_read_ioctl
aea57c382ba3 drm/i915: Introduce INTEL_GEN_MASK

== Logs ==

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

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

[Joonas Lahtinen]

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

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

[Joonas Lahtinen]

On Fri, 2017-09-08 at 12:39 +0300, Jani Nikula wrote:
> On Fri, 08 Sep 2017, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote:
> > Split INTEL_GEN_MASK out of IS_GEN macro, and make it usable
> > within static declarations (unlike combound statements).
> > 
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > ---
> >  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) \
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> but I'd really like a patch on top to remove the -1 from here and
> info->gen_mask.

I'll add it.

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

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

[Tvrtko Ursulin]

On 08/09/2017 10:39, Jani Nikula wrote:
> On Fri, 08 Sep 2017, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote:
>> Split INTEL_GEN_MASK out of IS_GEN macro, and make it usable
>> within static declarations (unlike combound statements).
>>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> ---
>>   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) \
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> but I'd really like a patch on top to remove the -1 from here and
> info->gen_mask.

What is the objection on -1? It is hidden in the macros so you can only 
see it if looking at disassembly or even lower level.

We could even make it -2, but AFAIR Chris wanted to keep the option for 
supporting gen1 open. :)

Regards,

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

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

[Jani Nikula]

On Fri, 08 Sep 2017, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> On 08/09/2017 10:39, Jani Nikula wrote:
>> On Fri, 08 Sep 2017, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote:
>>> Split INTEL_GEN_MASK out of IS_GEN macro, and make it usable
>>> within static declarations (unlike combound statements).
>>>
>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> ---
>>>   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) \
>> 
>> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>> 
>> but I'd really like a patch on top to remove the -1 from here and
>> info->gen_mask.
>
> What is the objection on -1? It is hidden in the macros so you can only 
> see it if looking at disassembly or even lower level.

IMO it's unnecessary optimization that makes reviewing the patches at
hand a tiny bit harder. Multiply that by everyone who ever looks at the
guts of the macros, and wonders why gen_mask is really
gen_off_by_one_mask, and for what reason. Indeed I thought there must be
a reason, until I realized that in reality, there is none.

BR,
Jani.

>
> We could even make it -2, but AFAIR Chris wanted to keep the option for 
> supporting gen1 open. :)
>
> Regards,
>
> Tvrtko

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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

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

[Joonas Lahtinen]

On Fri, 2017-09-08 at 15:24 +0300, Ville Syrjälä wrote:
> On Fri, Sep 08, 2017 at 12:29:35PM +0300, Joonas Lahtinen wrote:
> > @@ -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.

It's actually the type GCC wants because GENMASK is used internally to
produce the mask, otherwise it complained. We should likely use
unsigned long in dev_priv too, with a BUILD_BUG_ON(max_gen >
BITS_PER_LONG) check.

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

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

[Ville Syrjälä]

On Mon, Sep 11, 2017 at 01:49:18PM +0300, Joonas Lahtinen wrote:
> On Fri, 2017-09-08 at 15:24 +0300, Ville Syrjälä wrote:
> > On Fri, Sep 08, 2017 at 12:29:35PM +0300, Joonas Lahtinen wrote:
> > > @@ -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.
> 
> It's actually the type GCC wants because GENMASK is used internally to
> produce the mask, otherwise it complained.

Hmm. Complained about what? We're not truncating anything with an
explicit smaller type, so I have a hard time seeing why gcc would
object to it.

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