* [PATCH] drm/i915: Use bitmask for IS_REVID checking
@ 2016-05-10 13:02 Tvrtko Ursulin
2016-05-10 13:33 ` ✗ Ro.CI.BAT: warning for " Patchwork
` (4 more replies)
0 siblings, 5 replies; 9+ messages in thread
From: Tvrtko Ursulin @ 2016-05-10 13:02 UTC (permalink / raw)
To: Intel-gfx; +Cc: Jani Nikula
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
With this scheme all call sites of IS_SKL_REVID and IS_BXT_REVID
result in a maximum of one conditional jump instruction (was
three before) and overall reduction in code size.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jani Nikula <jani.nikula@intel.com>
---
drivers/gpu/drm/i915/i915_dma.c | 11 +++++++++++
drivers/gpu/drm/i915/i915_drv.h | 24 +++++++++++++++++++-----
2 files changed, 30 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 46ac1da64a09..1ae812436827 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1072,6 +1072,17 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
memcpy(device_info, info, sizeof(dev_priv->info));
device_info->device_id = dev->pdev->device;
+ /* For unknown revisions mask stays at zero which is correct. */
+ if (dev->pdev->revision <
+ sizeof(device_info->revision_mask) * BITS_PER_BYTE)
+ device_info->revision_mask = BIT(dev->pdev->revision);
+
+ if (IS_SKYLAKE(dev_priv))
+ device_info->skl_rev_mask = ~0;
+
+ if (IS_BROXTON(dev_priv))
+ device_info->bxt_rev_mask = ~0;
+
spin_lock_init(&dev_priv->irq_lock);
spin_lock_init(&dev_priv->gpu_error.lock);
mutex_init(&dev_priv->backlight_lock);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 26e7de415966..fbeb68e9ec96 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -757,6 +757,9 @@ struct intel_csr {
struct intel_device_info {
u32 display_mmio_offset;
u16 device_id;
+ u8 revision_mask;
+ u8 skl_rev_mask;
+ u8 bxt_rev_mask;
u8 num_pipes:3;
u8 num_sprites[I915_MAX_PIPES];
u8 gen;
@@ -2519,14 +2522,23 @@ struct drm_i915_cmd_table {
#define INTEL_DEVID(p) (INTEL_INFO(p)->device_id)
#define INTEL_REVID(p) (__I915__(p)->dev->pdev->revision)
-#define REVID_FOREVER 0xff
+#define REVID_FOREVER 0
+
/*
* Return true if revision is in range [since,until] inclusive.
*
* Use 0 for open-ended since, and REVID_FOREVER for open-ended until.
*/
-#define IS_REVID(p, since, until) \
- (INTEL_REVID(p) >= (since) && INTEL_REVID(p) <= (until))
+#define IS_REVID(p, mask, since, until) ({\
+ unsigned int __s = (since), __e = (until); \
+ BUILD_BUG_ON(!__builtin_constant_p(since)); \
+ BUILD_BUG_ON(!__builtin_constant_p(until)); \
+ BUILD_BUG_ON(since >= sizeof(INTEL_INFO(p)->revision_mask) * BITS_PER_BYTE); \
+ BUILD_BUG_ON(until >= sizeof(INTEL_INFO(p)->revision_mask) * BITS_PER_BYTE); \
+ if ((__e) == REVID_FOREVER) \
+ __e = BITS_PER_LONG - 1; \
+ !!(INTEL_INFO(p)->revision_mask & (mask) & GENMASK((__e), (__s))); \
+})
#define IS_I830(dev) (INTEL_DEVID(dev) == 0x3577)
#define IS_845G(dev) (INTEL_DEVID(dev) == 0x2562)
@@ -2605,14 +2617,16 @@ struct drm_i915_cmd_table {
#define SKL_REVID_E0 0x4
#define SKL_REVID_F0 0x5
-#define IS_SKL_REVID(p, since, until) (IS_SKYLAKE(p) && IS_REVID(p, since, until))
+#define IS_SKL_REVID(p, since, until) IS_REVID(p, INTEL_INFO(p)->skl_rev_mask, \
+ since, until)
#define BXT_REVID_A0 0x0
#define BXT_REVID_A1 0x1
#define BXT_REVID_B0 0x3
#define BXT_REVID_C0 0x9
-#define IS_BXT_REVID(p, since, until) (IS_BROXTON(p) && IS_REVID(p, since, until))
+#define IS_BXT_REVID(p, since, until) IS_REVID(p, INTEL_INFO(p)->bxt_rev_mask, \
+ since, until)
/*
* The genX designation typically refers to the render engine, so render
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* ✗ Ro.CI.BAT: warning for drm/i915: Use bitmask for IS_REVID checking
2016-05-10 13:02 [PATCH] drm/i915: Use bitmask for IS_REVID checking Tvrtko Ursulin
@ 2016-05-10 13:33 ` Patchwork
2016-05-10 13:33 ` [PATCH] " Jani Nikula
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2016-05-10 13:33 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Use bitmask for IS_REVID checking
URL : https://patchwork.freedesktop.org/series/6972/
State : warning
== Summary ==
Series 6972v1 drm/i915: Use bitmask for IS_REVID checking
http://patchwork.freedesktop.org/api/1.0/series/6972/revisions/1/mbox
Test kms_pipe_crc_basic:
Subgroup hang-read-crc-pipe-a:
pass -> DMESG-WARN (ro-ivb2-i7-3770)
Subgroup suspend-read-crc-pipe-b:
dmesg-warn -> PASS (ro-bdw-i7-5557U)
Test kms_sink_crc_basic:
pass -> SKIP (ro-skl-i7-6700hq)
ro-bdw-i5-5250u total:219 pass:202 dwarn:4 dfail:0 fail:0 skip:13
ro-bdw-i7-5557U total:219 pass:205 dwarn:0 dfail:0 fail:0 skip:14
ro-bdw-i7-5600u total:219 pass:187 dwarn:0 dfail:0 fail:0 skip:32
ro-bsw-n3050 total:219 pass:175 dwarn:0 dfail:0 fail:2 skip:42
ro-byt-n2820 total:218 pass:174 dwarn:0 dfail:0 fail:3 skip:41
ro-hsw-i3-4010u total:218 pass:193 dwarn:0 dfail:0 fail:0 skip:25
ro-hsw-i7-4770r total:219 pass:194 dwarn:0 dfail:0 fail:0 skip:25
ro-ilk-i7-620lm total:219 pass:151 dwarn:0 dfail:0 fail:1 skip:67
ro-ilk1-i5-650 total:214 pass:152 dwarn:0 dfail:0 fail:1 skip:61
ro-ivb-i7-3770 total:219 pass:183 dwarn:0 dfail:0 fail:0 skip:36
ro-ivb2-i7-3770 total:219 pass:186 dwarn:1 dfail:0 fail:0 skip:32
ro-skl-i7-6700hq total:214 pass:189 dwarn:0 dfail:0 fail:0 skip:25
ro-snb-i7-2620M total:219 pass:177 dwarn:0 dfail:0 fail:1 skip:41
Results at /archive/results/CI_IGT_test/RO_Patchwork_847/
2d4abf3 drm-intel-nightly: 2016y-05m-10d-09h-36m-54s UTC integration manifest
0a5a626 drm/i915: Use bitmask for IS_REVID checking
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Use bitmask for IS_REVID checking
2016-05-10 13:02 [PATCH] drm/i915: Use bitmask for IS_REVID checking Tvrtko Ursulin
2016-05-10 13:33 ` ✗ Ro.CI.BAT: warning for " Patchwork
@ 2016-05-10 13:33 ` Jani Nikula
2016-05-10 13:59 ` Tvrtko Ursulin
2016-05-10 13:48 ` ✗ Ro.CI.BAT: warning for " Patchwork
` (2 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Jani Nikula @ 2016-05-10 13:33 UTC (permalink / raw)
To: Tvrtko Ursulin, Intel-gfx
On Tue, 10 May 2016, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> With this scheme all call sites of IS_SKL_REVID and IS_BXT_REVID
> result in a maximum of one conditional jump instruction (was
> three before) and overall reduction in code size.
IMO we really shouldn't bother with this. The end goal is that we remove
all of these when we decide we no longer care about early
(non-production) revisions for a platform. For Skylake I'd argue we are
beyond this point.
BR,
Jani.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jani Nikula <jani.nikula@intel.com>
> ---
> drivers/gpu/drm/i915/i915_dma.c | 11 +++++++++++
> drivers/gpu/drm/i915/i915_drv.h | 24 +++++++++++++++++++-----
> 2 files changed, 30 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 46ac1da64a09..1ae812436827 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1072,6 +1072,17 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
> memcpy(device_info, info, sizeof(dev_priv->info));
> device_info->device_id = dev->pdev->device;
>
> + /* For unknown revisions mask stays at zero which is correct. */
> + if (dev->pdev->revision <
> + sizeof(device_info->revision_mask) * BITS_PER_BYTE)
> + device_info->revision_mask = BIT(dev->pdev->revision);
> +
> + if (IS_SKYLAKE(dev_priv))
> + device_info->skl_rev_mask = ~0;
> +
> + if (IS_BROXTON(dev_priv))
> + device_info->bxt_rev_mask = ~0;
> +
> spin_lock_init(&dev_priv->irq_lock);
> spin_lock_init(&dev_priv->gpu_error.lock);
> mutex_init(&dev_priv->backlight_lock);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 26e7de415966..fbeb68e9ec96 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -757,6 +757,9 @@ struct intel_csr {
> struct intel_device_info {
> u32 display_mmio_offset;
> u16 device_id;
> + u8 revision_mask;
> + u8 skl_rev_mask;
> + u8 bxt_rev_mask;
> u8 num_pipes:3;
> u8 num_sprites[I915_MAX_PIPES];
> u8 gen;
> @@ -2519,14 +2522,23 @@ struct drm_i915_cmd_table {
> #define INTEL_DEVID(p) (INTEL_INFO(p)->device_id)
> #define INTEL_REVID(p) (__I915__(p)->dev->pdev->revision)
>
> -#define REVID_FOREVER 0xff
> +#define REVID_FOREVER 0
> +
> /*
> * Return true if revision is in range [since,until] inclusive.
> *
> * Use 0 for open-ended since, and REVID_FOREVER for open-ended until.
> */
> -#define IS_REVID(p, since, until) \
> - (INTEL_REVID(p) >= (since) && INTEL_REVID(p) <= (until))
> +#define IS_REVID(p, mask, since, until) ({\
> + unsigned int __s = (since), __e = (until); \
> + BUILD_BUG_ON(!__builtin_constant_p(since)); \
> + BUILD_BUG_ON(!__builtin_constant_p(until)); \
> + BUILD_BUG_ON(since >= sizeof(INTEL_INFO(p)->revision_mask) * BITS_PER_BYTE); \
> + BUILD_BUG_ON(until >= sizeof(INTEL_INFO(p)->revision_mask) * BITS_PER_BYTE); \
> + if ((__e) == REVID_FOREVER) \
> + __e = BITS_PER_LONG - 1; \
> + !!(INTEL_INFO(p)->revision_mask & (mask) & GENMASK((__e), (__s))); \
> +})
>
> #define IS_I830(dev) (INTEL_DEVID(dev) == 0x3577)
> #define IS_845G(dev) (INTEL_DEVID(dev) == 0x2562)
> @@ -2605,14 +2617,16 @@ struct drm_i915_cmd_table {
> #define SKL_REVID_E0 0x4
> #define SKL_REVID_F0 0x5
>
> -#define IS_SKL_REVID(p, since, until) (IS_SKYLAKE(p) && IS_REVID(p, since, until))
> +#define IS_SKL_REVID(p, since, until) IS_REVID(p, INTEL_INFO(p)->skl_rev_mask, \
> + since, until)
>
> #define BXT_REVID_A0 0x0
> #define BXT_REVID_A1 0x1
> #define BXT_REVID_B0 0x3
> #define BXT_REVID_C0 0x9
>
> -#define IS_BXT_REVID(p, since, until) (IS_BROXTON(p) && IS_REVID(p, since, until))
> +#define IS_BXT_REVID(p, since, until) IS_REVID(p, INTEL_INFO(p)->bxt_rev_mask, \
> + since, until)
>
> /*
> * The genX designation typically refers to the render engine, so render
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* ✗ Ro.CI.BAT: warning for drm/i915: Use bitmask for IS_REVID checking
2016-05-10 13:02 [PATCH] drm/i915: Use bitmask for IS_REVID checking Tvrtko Ursulin
2016-05-10 13:33 ` ✗ Ro.CI.BAT: warning for " Patchwork
2016-05-10 13:33 ` [PATCH] " Jani Nikula
@ 2016-05-10 13:48 ` Patchwork
2016-05-10 13:53 ` [PATCH v2] " Tvrtko Ursulin
2016-05-10 14:26 ` ✗ Ro.CI.BAT: failure for drm/i915: Use bitmask for IS_REVID checking (rev2) Patchwork
4 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2016-05-10 13:48 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Use bitmask for IS_REVID checking
URL : https://patchwork.freedesktop.org/series/6972/
State : warning
== Summary ==
Series 6972v1 drm/i915: Use bitmask for IS_REVID checking
http://patchwork.freedesktop.org/api/1.0/series/6972/revisions/1/mbox
Test kms_pipe_crc_basic:
Subgroup hang-read-crc-pipe-a:
pass -> DMESG-WARN (ro-ivb2-i7-3770)
Subgroup suspend-read-crc-pipe-b:
dmesg-warn -> PASS (ro-bdw-i7-5557U)
Test kms_sink_crc_basic:
pass -> SKIP (ro-skl-i7-6700hq)
ro-bdw-i5-5250u total:219 pass:202 dwarn:4 dfail:0 fail:0 skip:13
ro-bdw-i7-5557U total:219 pass:205 dwarn:0 dfail:0 fail:0 skip:14
ro-bdw-i7-5600u total:219 pass:187 dwarn:0 dfail:0 fail:0 skip:32
ro-bsw-n3050 total:219 pass:175 dwarn:0 dfail:0 fail:2 skip:42
ro-byt-n2820 total:218 pass:174 dwarn:0 dfail:0 fail:3 skip:41
ro-hsw-i3-4010u total:218 pass:193 dwarn:0 dfail:0 fail:0 skip:25
ro-hsw-i7-4770r total:219 pass:194 dwarn:0 dfail:0 fail:0 skip:25
ro-ilk-i7-620lm total:219 pass:151 dwarn:0 dfail:0 fail:1 skip:67
ro-ilk1-i5-650 total:214 pass:152 dwarn:0 dfail:0 fail:1 skip:61
ro-ivb-i7-3770 total:219 pass:183 dwarn:0 dfail:0 fail:0 skip:36
ro-ivb2-i7-3770 total:219 pass:186 dwarn:1 dfail:0 fail:0 skip:32
ro-skl-i7-6700hq total:214 pass:189 dwarn:0 dfail:0 fail:0 skip:25
ro-snb-i7-2620M total:219 pass:177 dwarn:0 dfail:0 fail:1 skip:41
Results at /archive/results/CI_IGT_test/RO_Patchwork_847/
2d4abf3 drm-intel-nightly: 2016y-05m-10d-09h-36m-54s UTC integration manifest
0a5a626 drm/i915: Use bitmask for IS_REVID checking
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] drm/i915: Use bitmask for IS_REVID checking
2016-05-10 13:02 [PATCH] drm/i915: Use bitmask for IS_REVID checking Tvrtko Ursulin
` (2 preceding siblings ...)
2016-05-10 13:48 ` ✗ Ro.CI.BAT: warning for " Patchwork
@ 2016-05-10 13:53 ` Tvrtko Ursulin
2016-05-10 14:06 ` Tvrtko Ursulin
2016-05-10 14:26 ` ✗ Ro.CI.BAT: failure for drm/i915: Use bitmask for IS_REVID checking (rev2) Patchwork
4 siblings, 1 reply; 9+ messages in thread
From: Tvrtko Ursulin @ 2016-05-10 13:53 UTC (permalink / raw)
To: Intel-gfx; +Cc: Jani Nikula
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
With this scheme all call sites of IS_SKL_REVID and IS_BXT_REVID
result in a maximum of one conditional jump instruction (was
three before) and overall reduction in code size.
v2: Simplified, now saves ~880 bytes of text.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jani Nikula <jani.nikula@intel.com>
---
drivers/gpu/drm/i915/i915_dma.c | 8 ++++++++
drivers/gpu/drm/i915/i915_drv.h | 21 ++++++++++++++++-----
2 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 46ac1da64a09..f2ad78d8296f 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1072,6 +1072,14 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
memcpy(device_info, info, sizeof(dev_priv->info));
device_info->device_id = dev->pdev->device;
+ /* For unknown revisions mask stays at zero which is correct. */
+ if (IS_SKYLAKE(dev_priv) && dev->pdev->revision <
+ sizeof(device_info->skl_rev_mask) * BITS_PER_BYTE)
+ device_info->skl_rev_mask = BIT(dev->pdev->revision);
+ else if (IS_BROXTON(dev_priv) && dev->pdev->revision <
+ sizeof(device_info->bxt_rev_mask) * BITS_PER_BYTE)
+ device_info->bxt_rev_mask = BIT(dev->pdev->revision);
+
spin_lock_init(&dev_priv->irq_lock);
spin_lock_init(&dev_priv->gpu_error.lock);
mutex_init(&dev_priv->backlight_lock);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 26e7de415966..18eee575b9ae 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -757,6 +757,8 @@ struct intel_csr {
struct intel_device_info {
u32 display_mmio_offset;
u16 device_id;
+ u8 skl_rev_mask;
+ u8 bxt_rev_mask;
u8 num_pipes:3;
u8 num_sprites[I915_MAX_PIPES];
u8 gen;
@@ -2519,14 +2521,23 @@ struct drm_i915_cmd_table {
#define INTEL_DEVID(p) (INTEL_INFO(p)->device_id)
#define INTEL_REVID(p) (__I915__(p)->dev->pdev->revision)
-#define REVID_FOREVER 0xff
+#define REVID_FOREVER 0
+
/*
* Return true if revision is in range [since,until] inclusive.
*
* Use 0 for open-ended since, and REVID_FOREVER for open-ended until.
*/
-#define IS_REVID(p, since, until) \
- (INTEL_REVID(p) >= (since) && INTEL_REVID(p) <= (until))
+#define IS_REVID(p, mask, since, until) ({\
+ unsigned int __s = (since), __e = (until); \
+ BUILD_BUG_ON(!__builtin_constant_p(since)); \
+ BUILD_BUG_ON(!__builtin_constant_p(until)); \
+ BUILD_BUG_ON(since >= sizeof(INTEL_INFO(p)->mask) * BITS_PER_BYTE); \
+ BUILD_BUG_ON(until >= sizeof(INTEL_INFO(p)->mask) * BITS_PER_BYTE); \
+ if ((__e) == REVID_FOREVER) \
+ __e = BITS_PER_LONG - 1; \
+ !!(INTEL_INFO(p)->mask & GENMASK((__e), (__s))); \
+})
#define IS_I830(dev) (INTEL_DEVID(dev) == 0x3577)
#define IS_845G(dev) (INTEL_DEVID(dev) == 0x2562)
@@ -2605,14 +2616,14 @@ struct drm_i915_cmd_table {
#define SKL_REVID_E0 0x4
#define SKL_REVID_F0 0x5
-#define IS_SKL_REVID(p, since, until) (IS_SKYLAKE(p) && IS_REVID(p, since, until))
+#define IS_SKL_REVID(p, since, until) IS_REVID(p, skl_rev_mask, since, until)
#define BXT_REVID_A0 0x0
#define BXT_REVID_A1 0x1
#define BXT_REVID_B0 0x3
#define BXT_REVID_C0 0x9
-#define IS_BXT_REVID(p, since, until) (IS_BROXTON(p) && IS_REVID(p, since, until))
+#define IS_BXT_REVID(p, since, until) IS_REVID(p, bxt_rev_mask, since, until)
/*
* The genX designation typically refers to the render engine, so render
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Use bitmask for IS_REVID checking
2016-05-10 13:33 ` [PATCH] " Jani Nikula
@ 2016-05-10 13:59 ` Tvrtko Ursulin
2016-05-11 11:58 ` Jani Nikula
0 siblings, 1 reply; 9+ messages in thread
From: Tvrtko Ursulin @ 2016-05-10 13:59 UTC (permalink / raw)
To: Jani Nikula, Intel-gfx
Hi,
On 10/05/16 14:33, Jani Nikula wrote:
> On Tue, 10 May 2016, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> With this scheme all call sites of IS_SKL_REVID and IS_BXT_REVID
>> result in a maximum of one conditional jump instruction (was
>> three before) and overall reduction in code size.
>
> IMO we really shouldn't bother with this. The end goal is that we remove
> all of these when we decide we no longer care about early
> (non-production) revisions for a platform. For Skylake I'd argue we are
> beyond this point.
Thought that simply removing everything guarded by IS_SKL_REVID might be
possible never crossed my mind. :) Which one is the first production
stepping then?
Regards,
Tvrtko.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_dma.c | 11 +++++++++++
>> drivers/gpu/drm/i915/i915_drv.h | 24 +++++++++++++++++++-----
>> 2 files changed, 30 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>> index 46ac1da64a09..1ae812436827 100644
>> --- a/drivers/gpu/drm/i915/i915_dma.c
>> +++ b/drivers/gpu/drm/i915/i915_dma.c
>> @@ -1072,6 +1072,17 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
>> memcpy(device_info, info, sizeof(dev_priv->info));
>> device_info->device_id = dev->pdev->device;
>>
>> + /* For unknown revisions mask stays at zero which is correct. */
>> + if (dev->pdev->revision <
>> + sizeof(device_info->revision_mask) * BITS_PER_BYTE)
>> + device_info->revision_mask = BIT(dev->pdev->revision);
>> +
>> + if (IS_SKYLAKE(dev_priv))
>> + device_info->skl_rev_mask = ~0;
>> +
>> + if (IS_BROXTON(dev_priv))
>> + device_info->bxt_rev_mask = ~0;
>> +
>> spin_lock_init(&dev_priv->irq_lock);
>> spin_lock_init(&dev_priv->gpu_error.lock);
>> mutex_init(&dev_priv->backlight_lock);
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 26e7de415966..fbeb68e9ec96 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -757,6 +757,9 @@ struct intel_csr {
>> struct intel_device_info {
>> u32 display_mmio_offset;
>> u16 device_id;
>> + u8 revision_mask;
>> + u8 skl_rev_mask;
>> + u8 bxt_rev_mask;
>> u8 num_pipes:3;
>> u8 num_sprites[I915_MAX_PIPES];
>> u8 gen;
>> @@ -2519,14 +2522,23 @@ struct drm_i915_cmd_table {
>> #define INTEL_DEVID(p) (INTEL_INFO(p)->device_id)
>> #define INTEL_REVID(p) (__I915__(p)->dev->pdev->revision)
>>
>> -#define REVID_FOREVER 0xff
>> +#define REVID_FOREVER 0
>> +
>> /*
>> * Return true if revision is in range [since,until] inclusive.
>> *
>> * Use 0 for open-ended since, and REVID_FOREVER for open-ended until.
>> */
>> -#define IS_REVID(p, since, until) \
>> - (INTEL_REVID(p) >= (since) && INTEL_REVID(p) <= (until))
>> +#define IS_REVID(p, mask, since, until) ({\
>> + unsigned int __s = (since), __e = (until); \
>> + BUILD_BUG_ON(!__builtin_constant_p(since)); \
>> + BUILD_BUG_ON(!__builtin_constant_p(until)); \
>> + BUILD_BUG_ON(since >= sizeof(INTEL_INFO(p)->revision_mask) * BITS_PER_BYTE); \
>> + BUILD_BUG_ON(until >= sizeof(INTEL_INFO(p)->revision_mask) * BITS_PER_BYTE); \
>> + if ((__e) == REVID_FOREVER) \
>> + __e = BITS_PER_LONG - 1; \
>> + !!(INTEL_INFO(p)->revision_mask & (mask) & GENMASK((__e), (__s))); \
>> +})
>>
>> #define IS_I830(dev) (INTEL_DEVID(dev) == 0x3577)
>> #define IS_845G(dev) (INTEL_DEVID(dev) == 0x2562)
>> @@ -2605,14 +2617,16 @@ struct drm_i915_cmd_table {
>> #define SKL_REVID_E0 0x4
>> #define SKL_REVID_F0 0x5
>>
>> -#define IS_SKL_REVID(p, since, until) (IS_SKYLAKE(p) && IS_REVID(p, since, until))
>> +#define IS_SKL_REVID(p, since, until) IS_REVID(p, INTEL_INFO(p)->skl_rev_mask, \
>> + since, until)
>>
>> #define BXT_REVID_A0 0x0
>> #define BXT_REVID_A1 0x1
>> #define BXT_REVID_B0 0x3
>> #define BXT_REVID_C0 0x9
>>
>> -#define IS_BXT_REVID(p, since, until) (IS_BROXTON(p) && IS_REVID(p, since, until))
>> +#define IS_BXT_REVID(p, since, until) IS_REVID(p, INTEL_INFO(p)->bxt_rev_mask, \
>> + since, until)
>>
>> /*
>> * The genX designation typically refers to the render engine, so render
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] drm/i915: Use bitmask for IS_REVID checking
2016-05-10 13:53 ` [PATCH v2] " Tvrtko Ursulin
@ 2016-05-10 14:06 ` Tvrtko Ursulin
0 siblings, 0 replies; 9+ messages in thread
From: Tvrtko Ursulin @ 2016-05-10 14:06 UTC (permalink / raw)
To: Intel-gfx; +Cc: Jani Nikula
On 10/05/16 14:53, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> With this scheme all call sites of IS_SKL_REVID and IS_BXT_REVID
> result in a maximum of one conditional jump instruction (was
> three before) and overall reduction in code size.
>
> v2: Simplified, now saves ~880 bytes of text.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jani Nikula <jani.nikula@intel.com>
> ---
> drivers/gpu/drm/i915/i915_dma.c | 8 ++++++++
> drivers/gpu/drm/i915/i915_drv.h | 21 ++++++++++++++++-----
> 2 files changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 46ac1da64a09..f2ad78d8296f 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1072,6 +1072,14 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
> memcpy(device_info, info, sizeof(dev_priv->info));
> device_info->device_id = dev->pdev->device;
>
> + /* For unknown revisions mask stays at zero which is correct. */
> + if (IS_SKYLAKE(dev_priv) && dev->pdev->revision <
> + sizeof(device_info->skl_rev_mask) * BITS_PER_BYTE)
> + device_info->skl_rev_mask = BIT(dev->pdev->revision);
> + else if (IS_BROXTON(dev_priv) && dev->pdev->revision <
> + sizeof(device_info->bxt_rev_mask) * BITS_PER_BYTE)
> + device_info->bxt_rev_mask = BIT(dev->pdev->revision);
> +
No this is wrong, > /dev/null please, sorry for the noise. :)
Tvrtko
> spin_lock_init(&dev_priv->irq_lock);
> spin_lock_init(&dev_priv->gpu_error.lock);
> mutex_init(&dev_priv->backlight_lock);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 26e7de415966..18eee575b9ae 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -757,6 +757,8 @@ struct intel_csr {
> struct intel_device_info {
> u32 display_mmio_offset;
> u16 device_id;
> + u8 skl_rev_mask;
> + u8 bxt_rev_mask;
> u8 num_pipes:3;
> u8 num_sprites[I915_MAX_PIPES];
> u8 gen;
> @@ -2519,14 +2521,23 @@ struct drm_i915_cmd_table {
> #define INTEL_DEVID(p) (INTEL_INFO(p)->device_id)
> #define INTEL_REVID(p) (__I915__(p)->dev->pdev->revision)
>
> -#define REVID_FOREVER 0xff
> +#define REVID_FOREVER 0
> +
> /*
> * Return true if revision is in range [since,until] inclusive.
> *
> * Use 0 for open-ended since, and REVID_FOREVER for open-ended until.
> */
> -#define IS_REVID(p, since, until) \
> - (INTEL_REVID(p) >= (since) && INTEL_REVID(p) <= (until))
> +#define IS_REVID(p, mask, since, until) ({\
> + unsigned int __s = (since), __e = (until); \
> + BUILD_BUG_ON(!__builtin_constant_p(since)); \
> + BUILD_BUG_ON(!__builtin_constant_p(until)); \
> + BUILD_BUG_ON(since >= sizeof(INTEL_INFO(p)->mask) * BITS_PER_BYTE); \
> + BUILD_BUG_ON(until >= sizeof(INTEL_INFO(p)->mask) * BITS_PER_BYTE); \
> + if ((__e) == REVID_FOREVER) \
> + __e = BITS_PER_LONG - 1; \
> + !!(INTEL_INFO(p)->mask & GENMASK((__e), (__s))); \
> +})
>
> #define IS_I830(dev) (INTEL_DEVID(dev) == 0x3577)
> #define IS_845G(dev) (INTEL_DEVID(dev) == 0x2562)
> @@ -2605,14 +2616,14 @@ struct drm_i915_cmd_table {
> #define SKL_REVID_E0 0x4
> #define SKL_REVID_F0 0x5
>
> -#define IS_SKL_REVID(p, since, until) (IS_SKYLAKE(p) && IS_REVID(p, since, until))
> +#define IS_SKL_REVID(p, since, until) IS_REVID(p, skl_rev_mask, since, until)
>
> #define BXT_REVID_A0 0x0
> #define BXT_REVID_A1 0x1
> #define BXT_REVID_B0 0x3
> #define BXT_REVID_C0 0x9
>
> -#define IS_BXT_REVID(p, since, until) (IS_BROXTON(p) && IS_REVID(p, since, until))
> +#define IS_BXT_REVID(p, since, until) IS_REVID(p, bxt_rev_mask, since, until)
>
> /*
> * The genX designation typically refers to the render engine, so render
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* ✗ Ro.CI.BAT: failure for drm/i915: Use bitmask for IS_REVID checking (rev2)
2016-05-10 13:02 [PATCH] drm/i915: Use bitmask for IS_REVID checking Tvrtko Ursulin
` (3 preceding siblings ...)
2016-05-10 13:53 ` [PATCH v2] " Tvrtko Ursulin
@ 2016-05-10 14:26 ` Patchwork
4 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2016-05-10 14:26 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Use bitmask for IS_REVID checking (rev2)
URL : https://patchwork.freedesktop.org/series/6972/
State : failure
== Summary ==
Series 6972v2 drm/i915: Use bitmask for IS_REVID checking
http://patchwork.freedesktop.org/api/1.0/series/6972/revisions/2/mbox
Test drv_hangman:
Subgroup error-state-basic:
pass -> FAIL (ro-ilk1-i5-650)
incomplete -> PASS (fi-snb-i7-2600)
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-b:
dmesg-warn -> PASS (ro-bdw-i7-5557U)
Test kms_sink_crc_basic:
pass -> SKIP (ro-skl-i7-6700hq)
fi-bdw-i7-5557u total:219 pass:206 dwarn:0 dfail:0 fail:0 skip:13
fi-byt-n2820 total:219 pass:175 dwarn:0 dfail:0 fail:3 skip:41
fi-hsw-i7-4770k total:219 pass:197 dwarn:0 dfail:0 fail:1 skip:21
fi-skl-i5-6260u total:219 pass:207 dwarn:0 dfail:0 fail:0 skip:12
fi-skl-i7-6700k total:219 pass:191 dwarn:0 dfail:0 fail:0 skip:28
fi-snb-i7-2600 total:219 pass:176 dwarn:0 dfail:0 fail:0 skip:43
ro-bdw-i5-5250u total:219 pass:202 dwarn:4 dfail:0 fail:0 skip:13
ro-bdw-i7-5557U total:219 pass:205 dwarn:0 dfail:0 fail:0 skip:14
ro-bdw-i7-5600u total:219 pass:187 dwarn:0 dfail:0 fail:0 skip:32
ro-bsw-n3050 total:219 pass:174 dwarn:0 dfail:0 fail:3 skip:42
ro-byt-n2820 total:218 pass:174 dwarn:0 dfail:0 fail:3 skip:41
ro-hsw-i3-4010u total:218 pass:193 dwarn:0 dfail:0 fail:0 skip:25
ro-hsw-i7-4770r total:219 pass:194 dwarn:0 dfail:0 fail:0 skip:25
ro-ilk1-i5-650 total:214 pass:151 dwarn:0 dfail:0 fail:2 skip:61
ro-ivb-i7-3770 total:219 pass:183 dwarn:0 dfail:0 fail:0 skip:36
ro-ivb2-i7-3770 total:219 pass:187 dwarn:0 dfail:0 fail:0 skip:32
ro-skl-i7-6700hq total:214 pass:189 dwarn:0 dfail:0 fail:0 skip:25
ro-snb-i7-2620M total:219 pass:177 dwarn:0 dfail:0 fail:1 skip:41
fi-hsw-i7-4770r failed to connect after reboot
fi-kbl-y failed to connect after reboot
ro-ilk-i7-620lm failed to connect after reboot
Results at /archive/results/CI_IGT_test/RO_Patchwork_849/
2d4abf3 drm-intel-nightly: 2016y-05m-10d-09h-36m-54s UTC integration manifest
4e1a637 drm/i915: Use bitmask for IS_REVID checking
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Use bitmask for IS_REVID checking
2016-05-10 13:59 ` Tvrtko Ursulin
@ 2016-05-11 11:58 ` Jani Nikula
0 siblings, 0 replies; 9+ messages in thread
From: Jani Nikula @ 2016-05-11 11:58 UTC (permalink / raw)
To: Tvrtko Ursulin, Intel-gfx; +Cc: Syrjala, Ville
On Tue, 10 May 2016, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> Hi,
>
> On 10/05/16 14:33, Jani Nikula wrote:
>> On Tue, 10 May 2016, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> With this scheme all call sites of IS_SKL_REVID and IS_BXT_REVID
>>> result in a maximum of one conditional jump instruction (was
>>> three before) and overall reduction in code size.
>>
>> IMO we really shouldn't bother with this. The end goal is that we remove
>> all of these when we decide we no longer care about early
>> (non-production) revisions for a platform. For Skylake I'd argue we are
>> beyond this point.
>
> Thought that simply removing everything guarded by IS_SKL_REVID might be
> possible never crossed my mind. :) Which one is the first production
> stepping then?
None of the ones we list and check for i.e. we should replace all of the
IS_SKL_REVID() uses with either 0 or IS_SKYLAKE(), and simplify from
there.
Maybe we should check the revid once on driver load, and whine about
early hardware. We've had such checks in the past on earlier
platforms. This might save our precious time, as we shouldn't be
debugging issues reported against pre-production hardware.
Ville here says dropping the checks is a good time to verify the
workarounds we do have in place are correct going forward.
BR,
Jani.
>
> Regards,
>
> Tvrtko.
>
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>> ---
>>> drivers/gpu/drm/i915/i915_dma.c | 11 +++++++++++
>>> drivers/gpu/drm/i915/i915_drv.h | 24 +++++++++++++++++++-----
>>> 2 files changed, 30 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>>> index 46ac1da64a09..1ae812436827 100644
>>> --- a/drivers/gpu/drm/i915/i915_dma.c
>>> +++ b/drivers/gpu/drm/i915/i915_dma.c
>>> @@ -1072,6 +1072,17 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
>>> memcpy(device_info, info, sizeof(dev_priv->info));
>>> device_info->device_id = dev->pdev->device;
>>>
>>> + /* For unknown revisions mask stays at zero which is correct. */
>>> + if (dev->pdev->revision <
>>> + sizeof(device_info->revision_mask) * BITS_PER_BYTE)
>>> + device_info->revision_mask = BIT(dev->pdev->revision);
>>> +
>>> + if (IS_SKYLAKE(dev_priv))
>>> + device_info->skl_rev_mask = ~0;
>>> +
>>> + if (IS_BROXTON(dev_priv))
>>> + device_info->bxt_rev_mask = ~0;
>>> +
>>> spin_lock_init(&dev_priv->irq_lock);
>>> spin_lock_init(&dev_priv->gpu_error.lock);
>>> mutex_init(&dev_priv->backlight_lock);
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 26e7de415966..fbeb68e9ec96 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -757,6 +757,9 @@ struct intel_csr {
>>> struct intel_device_info {
>>> u32 display_mmio_offset;
>>> u16 device_id;
>>> + u8 revision_mask;
>>> + u8 skl_rev_mask;
>>> + u8 bxt_rev_mask;
>>> u8 num_pipes:3;
>>> u8 num_sprites[I915_MAX_PIPES];
>>> u8 gen;
>>> @@ -2519,14 +2522,23 @@ struct drm_i915_cmd_table {
>>> #define INTEL_DEVID(p) (INTEL_INFO(p)->device_id)
>>> #define INTEL_REVID(p) (__I915__(p)->dev->pdev->revision)
>>>
>>> -#define REVID_FOREVER 0xff
>>> +#define REVID_FOREVER 0
>>> +
>>> /*
>>> * Return true if revision is in range [since,until] inclusive.
>>> *
>>> * Use 0 for open-ended since, and REVID_FOREVER for open-ended until.
>>> */
>>> -#define IS_REVID(p, since, until) \
>>> - (INTEL_REVID(p) >= (since) && INTEL_REVID(p) <= (until))
>>> +#define IS_REVID(p, mask, since, until) ({\
>>> + unsigned int __s = (since), __e = (until); \
>>> + BUILD_BUG_ON(!__builtin_constant_p(since)); \
>>> + BUILD_BUG_ON(!__builtin_constant_p(until)); \
>>> + BUILD_BUG_ON(since >= sizeof(INTEL_INFO(p)->revision_mask) * BITS_PER_BYTE); \
>>> + BUILD_BUG_ON(until >= sizeof(INTEL_INFO(p)->revision_mask) * BITS_PER_BYTE); \
>>> + if ((__e) == REVID_FOREVER) \
>>> + __e = BITS_PER_LONG - 1; \
>>> + !!(INTEL_INFO(p)->revision_mask & (mask) & GENMASK((__e), (__s))); \
>>> +})
>>>
>>> #define IS_I830(dev) (INTEL_DEVID(dev) == 0x3577)
>>> #define IS_845G(dev) (INTEL_DEVID(dev) == 0x2562)
>>> @@ -2605,14 +2617,16 @@ struct drm_i915_cmd_table {
>>> #define SKL_REVID_E0 0x4
>>> #define SKL_REVID_F0 0x5
>>>
>>> -#define IS_SKL_REVID(p, since, until) (IS_SKYLAKE(p) && IS_REVID(p, since, until))
>>> +#define IS_SKL_REVID(p, since, until) IS_REVID(p, INTEL_INFO(p)->skl_rev_mask, \
>>> + since, until)
>>>
>>> #define BXT_REVID_A0 0x0
>>> #define BXT_REVID_A1 0x1
>>> #define BXT_REVID_B0 0x3
>>> #define BXT_REVID_C0 0x9
>>>
>>> -#define IS_BXT_REVID(p, since, until) (IS_BROXTON(p) && IS_REVID(p, since, until))
>>> +#define IS_BXT_REVID(p, since, until) IS_REVID(p, INTEL_INFO(p)->bxt_rev_mask, \
>>> + since, until)
>>>
>>> /*
>>> * The genX designation typically refers to the render engine, so render
>>
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-05-11 11:58 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-10 13:02 [PATCH] drm/i915: Use bitmask for IS_REVID checking Tvrtko Ursulin
2016-05-10 13:33 ` ✗ Ro.CI.BAT: warning for " Patchwork
2016-05-10 13:33 ` [PATCH] " Jani Nikula
2016-05-10 13:59 ` Tvrtko Ursulin
2016-05-11 11:58 ` Jani Nikula
2016-05-10 13:48 ` ✗ Ro.CI.BAT: warning for " Patchwork
2016-05-10 13:53 ` [PATCH v2] " Tvrtko Ursulin
2016-05-10 14:06 ` Tvrtko Ursulin
2016-05-10 14:26 ` ✗ Ro.CI.BAT: failure for drm/i915: Use bitmask for IS_REVID checking (rev2) Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.