[PATCH] drm/i915: Kill GEN_RANGE in favor of INTEL_GEN.

[PATCH] drm/i915: Kill GEN_RANGE in favor of INTEL_GEN.

[Rodrigo Vivi]

Instead of limiting the range with this unusual GEN_RANGE
let's assume following platforms would use same scheme
unless stated otherwise.

In our regular flow of platform enabling we check for
INTEL_GEN occurences, while GEN_RANGE had only this
specific usage and consequently got forgotten,
blocking userspace on CNL to read RCS Timestamps.

That case was later identified and fixed with:
f1294585d8e1 ("drm/i915/cnl: Allow the reg_read ioctl
to read the RCS TIMESTAMP register")

So this patch extend this a bit futher so we don't
end in similar situations in near future.

This patch reverts the last remaining usage of the
original patch where it was added: af76ae447d44 ("drm/i915:
Use a macro to express the range of valid gens for reg_read")

Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Nanley Chery <nanley.g.chery@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 0529af7cfbb8..5ff854b489a0 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1241,17 +1241,14 @@ 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) },
+	  .size = 8
+	},
 };
 
 int i915_reg_read_ioctl(struct drm_device *dev,
@@ -1266,7 +1263,7 @@ int i915_reg_read_ioctl(struct drm_device *dev,
 
 	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))
+		    (INTEL_GEN(dev_priv) >= 4))
 			break;
 	}
 
-- 
2.13.2

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

Re: [PATCH] drm/i915: Kill GEN_RANGE in favor of INTEL_GEN.

[Chris Wilson]

Quoting Rodrigo Vivi (2017-09-06 20:51:37)
> Instead of limiting the range with this unusual GEN_RANGE
> let's assume following platforms would use same scheme
> unless stated otherwise.

No. This is uabi that should indeed be checked before exposed and not
assumed that unprivileged access to a register of yesterday is still
safe tommorrow.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Kill GEN_RANGE in favor of INTEL_GEN.

[Vivi, Rodrigo]

On Wed, 2017-09-06 at 20:57 +0100, Chris Wilson wrote:
> Quoting Rodrigo Vivi (2017-09-06 20:51:37)
> > Instead of limiting the range with this unusual GEN_RANGE
> > let's assume following platforms would use same scheme
> > unless stated otherwise.
> 
> No. This is uabi that should indeed be checked before exposed and not
> assumed that unprivileged access to a register of yesterday is still
> safe tommorrow.

hm... makes sense..

can we at least move to 

INTEL_GEN >= 4 && INTEL_GEN <= 10

or some flag on platform definition?

I really don't like GEN_RANGE... 

> -Chris

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

Re: [PATCH] drm/i915: Kill GEN_RANGE in favor of INTEL_GEN.

[Chris Wilson]

Quoting Vivi, Rodrigo (2017-09-06 21:13:07)
> On Wed, 2017-09-06 at 20:57 +0100, Chris Wilson wrote:
> > Quoting Rodrigo Vivi (2017-09-06 20:51:37)
> > > Instead of limiting the range with this unusual GEN_RANGE
> > > let's assume following platforms would use same scheme
> > > unless stated otherwise.
> > 
> > No. This is uabi that should indeed be checked before exposed and not
> > assumed that unprivileged access to a register of yesterday is still
> > safe tommorrow.
> 
> hm... makes sense..
> 
> can we at least move to 
> 
> INTEL_GEN >= 4 && INTEL_GEN <= 10
> 
> or some flag on platform definition?
> 
> I really don't like GEN_RANGE... 

Something along the lines of

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6020a94daf81..b43a20bf2f25 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2843,24 +2843,30 @@ intel_info(const struct drm_i915_private *dev_priv)
 #define INTEL_REVID(dev_priv)  ((dev_priv)->drm.pdev->revision)
 
 #define GEN_FOREVER (0)
+
+#define __GEN_RANGE(S, E) \
+       GENMASK((E) == GEN_FOREVER ? BITS_PER_LONG - 1 : (E) - 1, \
+               (S) == GEN_FOREVER ? 0 : (S) - 1)
 /*
- * Returns true if Gen is in inclusive range [Start, End].
+ * Computes the generation mask for an 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 GEN_RANGE(S, E) ({ \
+       BUILD_BUG_ON(!__builtin_constant_p(S)); \
+       BUILD_BUG_ON(!__builtin_constant_p(E)); \
+       __GEN_RANGE(S, E); \
 })
 
+#define __IS_GEN(dev_priv, mask) (!!((dev_priv)->info.gen_mask & (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) __IS_GEN(dev_priv, GEN_RANGE(s, e))
+
 /*
  * Return true if revision is in range [since,until] inclusive.
  *
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 1d7b879cc68c..5f66313d628a 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1241,8 +1241,6 @@ 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;
@@ -1251,7 +1249,7 @@ static const struct register_whitelist {
 } whitelist[] = {
        { .offset_ldw = RING_TIMESTAMP(RENDER_RING_BASE),
          .offset_udw = RING_TIMESTAMP_UDW(RENDER_RING_BASE),
-         .size = 8, .gen_bitmask = GEN_RANGE(4, 9) },
+         .size = 8, .gen_bitmask = __GEN_RANGE(4, 9) },
 };
 
 int i915_reg_read_ioctl(struct drm_device *dev,
@@ -1266,7 +1264,7 @@ int i915_reg_read_ioctl(struct drm_device *dev,
 
        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))
+                   __IS_GEN(dev_priv, entry->gen_bitmask))
                        break;
        }
 

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

Re: [PATCH] drm/i915: Kill GEN_RANGE in favor of INTEL_GEN.

[Jani Nikula]

On Wed, 06 Sep 2017, "Vivi, Rodrigo" <rodrigo.vivi@intel.com> wrote:
> On Wed, 2017-09-06 at 20:57 +0100, Chris Wilson wrote:
>> Quoting Rodrigo Vivi (2017-09-06 20:51:37)
>> > Instead of limiting the range with this unusual GEN_RANGE
>> > let's assume following platforms would use same scheme
>> > unless stated otherwise.
>> 
>> No. This is uabi that should indeed be checked before exposed and not
>> assumed that unprivileged access to a register of yesterday is still
>> safe tommorrow.
>
> hm... makes sense..
>
> can we at least move to 
>
> INTEL_GEN >= 4 && INTEL_GEN <= 10
>
> or some flag on platform definition?
>
> I really don't like GEN_RANGE... 

Why?

My only problem with it is that gen N does not map to bit N in the
mask. I think it would be more important to optimize for developer brain
reading the code than saving one bit.

BR,
Jani.



>
>> -Chris
>

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

✓ Fi.CI.BAT: success for drm/i915: Kill GEN_RANGE in favor of INTEL_GEN.

[Patchwork]

== Series Details ==

Series: drm/i915: Kill GEN_RANGE in favor of INTEL_GEN.
URL   : https://patchwork.freedesktop.org/series/29903/
State : success

== Summary ==

Series 29903v1 drm/i915: Kill GEN_RANGE in favor of INTEL_GEN.
https://patchwork.freedesktop.org/api/1.0/series/29903/revisions/1/mbox/

Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-atomic:
                pass       -> FAIL       (fi-snb-2600) fdo#100215 +1
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> SKIP       (fi-cfl-s) fdo#102294

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

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:459s
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:365s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:552s
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:524s
fi-byt-j1900     total:289  pass:254  dwarn:1   dfail:0   fail:0   skip:34  time:519s
fi-byt-n2820     total:289  pass:250  dwarn:1   dfail:0   fail:0   skip:38  time:515s
fi-cfl-s         total:289  pass:249  dwarn:4   dfail:0   fail:0   skip:36  time:476s
fi-elk-e7500     total:289  pass:230  dwarn:0   dfail:0   fail:0   skip:59  time:438s
fi-glk-2a        total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:613s
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:426s
fi-ilk-650       total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:423s
fi-ivb-3520m     total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:506s
fi-ivb-3770      total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:479s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:513s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:602s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:604s
fi-pnv-d510      total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:526s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:474s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:535s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:515s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:446s
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:559s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:1   skip:39  time:405s

235edb916d56561e8e68ed0b119fb195c52eb25a drm-tip: 2017y-09m-06d-19h-31m-41s UTC integration manifest
f0ccd3123386 drm/i915: Kill GEN_RANGE in favor of INTEL_GEN.

== Logs ==

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

✗ Fi.CI.IGT: warning for drm/i915: Kill GEN_RANGE in favor of INTEL_GEN.

[Patchwork]

== Series Details ==

Series: drm/i915: Kill GEN_RANGE in favor of INTEL_GEN.
URL   : https://patchwork.freedesktop.org/series/29903/
State : warning

== Summary ==

Test kms_busy:
        Subgroup basic-modeset-C:
                pass       -> SKIP       (shard-hsw)
Test kms_plane:
        Subgroup plane-position-hole-dpms-pipe-B-planes:
                pass       -> SKIP       (shard-hsw)
Test kms_sysfs_edid_timing:
                warn       -> PASS       (shard-hsw) fdo#100047
Test kms_vblank:
        Subgroup accuracy-idle:
                fail       -> PASS       (shard-hsw)
Test kms_setmode:
        Subgroup basic:
                pass       -> FAIL       (shard-hsw) fdo#99912

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

shard-hsw        total:2174 pass:1180 dwarn:0   dfail:0   fail:16  skip:978 time:9268s

== Logs ==

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