Re: [Intel-gfx] [PATCH 1/4] drm/i915: Store gen_mask inside the static device info
Chris Wilson writes:
> Rather than deriving the gen_mask from the static intel_device_info->gen
> at runtime, prefill it in the static data.
>
> Signed-off-by: Chris Wilson
Reviewed-by: Rodrigo Vivi
> ---
> drivers/gpu/drm/i915/i915_drv.c | 2 --
> drivers/gpu/drm/i915/i915_pci.c | 39 ---
> 2 files changed, 24 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index daa9060bdfcb..90f4adbbff28 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -902,8 +902,6 @@ static int i915_driver_init_early(struct drm_i915_private
> *dev_priv,
> device_info->platform_mask = BIT(device_info->platform);
>
> BUG_ON(device_info->gen > sizeof(device_info->gen_mask) *
> BITS_PER_BYTE);
> - device_info->gen_mask = BIT(device_info->gen - 1);
> -
> 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_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index 4e7a10c89782..3b4516d7f9a4 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -29,6 +29,8 @@
> #include "i915_drv.h"
> #include "i915_selftest.h"
>
> +#define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1)
> +
> #define GEN_DEFAULT_PIPEOFFSETS \
> .pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \
> PIPE_C_OFFSET, PIPE_EDP_OFFSET }, \
> @@ -63,7 +65,8 @@
> .page_sizes = I915_GTT_PAGE_SIZE_4K
>
> #define GEN2_FEATURES \
> - .gen = 2, .num_pipes = 1, \
> + GEN(2), \
> + .num_pipes = 1, \
> .has_overlay = 1, .overlay_needs_physical = 1, \
> .has_gmch_display = 1, \
> .hws_needs_physical = 1, \
> @@ -100,7 +103,8 @@ static const struct intel_device_info intel_i865g_info = {
> };
>
> #define GEN3_FEATURES \
> - .gen = 3, .num_pipes = 2, \
> + GEN(3), \
> + .num_pipes = 2, \
> .has_gmch_display = 1, \
> .ring_mask = RENDER_RING, \
> .has_snoop = true, \
> @@ -163,7 +167,8 @@ static const struct intel_device_info intel_pineview_info
> = {
> };
>
> #define GEN4_FEATURES \
> - .gen = 4, .num_pipes = 2, \
> + GEN(4), \
> + .num_pipes = 2, \
> .has_hotplug = 1, \
> .has_gmch_display = 1, \
> .ring_mask = RENDER_RING, \
> @@ -205,7 +210,8 @@ static const struct intel_device_info intel_gm45_info = {
> };
>
> #define GEN5_FEATURES \
> - .gen = 5, .num_pipes = 2, \
> + GEN(5), \
> + .num_pipes = 2, \
> .has_hotplug = 1, \
> .ring_mask = RENDER_RING | BSD_RING, \
> .has_snoop = true, \
> @@ -227,7 +233,8 @@ static const struct intel_device_info
> intel_ironlake_m_info = {
> };
>
> #define GEN6_FEATURES \
> - .gen = 6, .num_pipes = 2, \
> + GEN(6), \
> + .num_pipes = 2, \
> .has_hotplug = 1, \
> .has_fbc = 1, \
> .ring_mask = RENDER_RING | BSD_RING | BLT_RING, \
> @@ -270,7 +277,8 @@ static const struct intel_device_info
> intel_sandybridge_m_gt2_info = {
> };
>
> #define GEN7_FEATURES \
> - .gen = 7, .num_pipes = 3, \
> + GEN(7), \
> + .num_pipes = 3, \
> .has_hotplug = 1, \
> .has_fbc = 1, \
> .ring_mask = RENDER_RING | BSD_RING | BLT_RING, \
> @@ -324,7 +332,7 @@ static const struct intel_device_info
> intel_ivybridge_q_info = {
>
> static const struct intel_device_info intel_valleyview_info = {
> .platform = INTEL_VALLEYVIEW,
> - .gen = 7,
> + GEN(7),
> .is_lp = 1,
> .num_pipes = 2,
> .has_psr = 1,
> @@ -385,7 +393,7 @@ static const struct intel_device_info
> intel_haswell_gt3_info = {
>
> #define BDW_PLATFORM \
> GEN8_FEATURES, \
> - .gen = 8, \
> + GEN(8), \
> .platform = INTEL_BROADWELL
>
> static const struct intel_device_info intel_broadwell_gt1_info = {
> @@ -413,7 +421,8 @@ static const struct intel_device_info
> intel_broadwell_gt3_info = {
> };
>
> static const struct intel_device_info intel_cherryview_info = {
> - .gen = 8, .num_pipes = 3,
> + GEN(8),
> + .num_pipes = 3,
> .has_hotplug = 1,
> .is_lp = 1,
> .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
> @@ -452,7 +461,7 @@ static const struct intel_device_info
> intel_cherryview_info = {
>
> #define SKL_PLATFORM \
> GEN9_FEATURES, \
> - .gen = 9, \
> + GEN(9), \
> .platform = INTEL_SKYLAKE
>
> static const struct intel_device_info intel_skylake_gt1_info = {
> @@ -481,7 +490,7 @@ static const struct intel_device_info
> intel_skylake_gt4_info = {
> };
>
> #define GEN9_LP_FEATURES \
> - .gen = 9, \
> + GEN(9), \
> .is_lp = 1, \
> .has_hotplug = 1, \
> .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING, \
> @@ -526,7 +535,7 @@ static const struct intel_device_info
> intel_geminilake_info = {
Re: [Intel-gfx] [PATCH 1/4] drm/i915: Store gen_mask inside the static device info
Quoting Jani Nikula (2018-02-12 09:58:20) > On Fri, 09 Feb 2018, Chris Wilson wrote: > > Rather than deriving the gen_mask from the static intel_device_info->gen > > at runtime, prefill it in the static data. > > > > Signed-off-by: Chris Wilson > > --- > > drivers/gpu/drm/i915/i915_drv.c | 2 -- > > drivers/gpu/drm/i915/i915_pci.c | 39 > > --- > > 2 files changed, 24 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > > b/drivers/gpu/drm/i915/i915_drv.c > > index daa9060bdfcb..90f4adbbff28 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -902,8 +902,6 @@ static int i915_driver_init_early(struct > > drm_i915_private *dev_priv, > > device_info->platform_mask = BIT(device_info->platform); > > > > BUG_ON(device_info->gen > sizeof(device_info->gen_mask) * > > BITS_PER_BYTE); > > - device_info->gen_mask = BIT(device_info->gen - 1); > > - > > 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_pci.c > > b/drivers/gpu/drm/i915/i915_pci.c > > index 4e7a10c89782..3b4516d7f9a4 100644 > > --- a/drivers/gpu/drm/i915/i915_pci.c > > +++ b/drivers/gpu/drm/i915/i915_pci.c > > @@ -29,6 +29,8 @@ > > #include "i915_drv.h" > > #include "i915_selftest.h" > > > > +#define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1) > > Please #undef GEN afterwards, ditto for the other macros in the other > patches. I was working on this being local to the file, but sure :) > Does the compiler warn if you overflow .gen_mask? Can we drop the > runtime overflow check now? Or better be safe than sorry with the > BUG_ON? I know clang emits a warn, and I've vague memories about gcc warning for large constants. What I did contemplate is that given the macro, we could use BUILD_BUG_ON_OR_ZERO() and incorporate the overflow checks into the macro. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm/i915: Store gen_mask inside the static device info
On Fri, 09 Feb 2018, Chris Wilson wrote:
> Rather than deriving the gen_mask from the static intel_device_info->gen
> at runtime, prefill it in the static data.
>
> Signed-off-by: Chris Wilson
> ---
> drivers/gpu/drm/i915/i915_drv.c | 2 --
> drivers/gpu/drm/i915/i915_pci.c | 39 ---
> 2 files changed, 24 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index daa9060bdfcb..90f4adbbff28 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -902,8 +902,6 @@ static int i915_driver_init_early(struct drm_i915_private
> *dev_priv,
> device_info->platform_mask = BIT(device_info->platform);
>
> BUG_ON(device_info->gen > sizeof(device_info->gen_mask) *
> BITS_PER_BYTE);
> - device_info->gen_mask = BIT(device_info->gen - 1);
> -
> 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_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index 4e7a10c89782..3b4516d7f9a4 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -29,6 +29,8 @@
> #include "i915_drv.h"
> #include "i915_selftest.h"
>
> +#define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1)
Please #undef GEN afterwards, ditto for the other macros in the other
patches.
Does the compiler warn if you overflow .gen_mask? Can we drop the
runtime overflow check now? Or better be safe than sorry with the
BUG_ON?
I like the destination in the series.
BR,
Jani.
> +
> #define GEN_DEFAULT_PIPEOFFSETS \
> .pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \
> PIPE_C_OFFSET, PIPE_EDP_OFFSET }, \
> @@ -63,7 +65,8 @@
> .page_sizes = I915_GTT_PAGE_SIZE_4K
>
> #define GEN2_FEATURES \
> - .gen = 2, .num_pipes = 1, \
> + GEN(2), \
> + .num_pipes = 1, \
> .has_overlay = 1, .overlay_needs_physical = 1, \
> .has_gmch_display = 1, \
> .hws_needs_physical = 1, \
> @@ -100,7 +103,8 @@ static const struct intel_device_info intel_i865g_info = {
> };
>
> #define GEN3_FEATURES \
> - .gen = 3, .num_pipes = 2, \
> + GEN(3), \
> + .num_pipes = 2, \
> .has_gmch_display = 1, \
> .ring_mask = RENDER_RING, \
> .has_snoop = true, \
> @@ -163,7 +167,8 @@ static const struct intel_device_info intel_pineview_info
> = {
> };
>
> #define GEN4_FEATURES \
> - .gen = 4, .num_pipes = 2, \
> + GEN(4), \
> + .num_pipes = 2, \
> .has_hotplug = 1, \
> .has_gmch_display = 1, \
> .ring_mask = RENDER_RING, \
> @@ -205,7 +210,8 @@ static const struct intel_device_info intel_gm45_info = {
> };
>
> #define GEN5_FEATURES \
> - .gen = 5, .num_pipes = 2, \
> + GEN(5), \
> + .num_pipes = 2, \
> .has_hotplug = 1, \
> .ring_mask = RENDER_RING | BSD_RING, \
> .has_snoop = true, \
> @@ -227,7 +233,8 @@ static const struct intel_device_info
> intel_ironlake_m_info = {
> };
>
> #define GEN6_FEATURES \
> - .gen = 6, .num_pipes = 2, \
> + GEN(6), \
> + .num_pipes = 2, \
> .has_hotplug = 1, \
> .has_fbc = 1, \
> .ring_mask = RENDER_RING | BSD_RING | BLT_RING, \
> @@ -270,7 +277,8 @@ static const struct intel_device_info
> intel_sandybridge_m_gt2_info = {
> };
>
> #define GEN7_FEATURES \
> - .gen = 7, .num_pipes = 3, \
> + GEN(7), \
> + .num_pipes = 3, \
> .has_hotplug = 1, \
> .has_fbc = 1, \
> .ring_mask = RENDER_RING | BSD_RING | BLT_RING, \
> @@ -324,7 +332,7 @@ static const struct intel_device_info
> intel_ivybridge_q_info = {
>
> static const struct intel_device_info intel_valleyview_info = {
> .platform = INTEL_VALLEYVIEW,
> - .gen = 7,
> + GEN(7),
> .is_lp = 1,
> .num_pipes = 2,
> .has_psr = 1,
> @@ -385,7 +393,7 @@ static const struct intel_device_info
> intel_haswell_gt3_info = {
>
> #define BDW_PLATFORM \
> GEN8_FEATURES, \
> - .gen = 8, \
> + GEN(8), \
> .platform = INTEL_BROADWELL
>
> static const struct intel_device_info intel_broadwell_gt1_info = {
> @@ -413,7 +421,8 @@ static const struct intel_device_info
> intel_broadwell_gt3_info = {
> };
>
> static const struct intel_device_info intel_cherryview_info = {
> - .gen = 8, .num_pipes = 3,
> + GEN(8),
> + .num_pipes = 3,
> .has_hotplug = 1,
> .is_lp = 1,
> .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
> @@ -452,7 +461,7 @@ static const struct intel_device_info
> intel_cherryview_info = {
>
> #define SKL_PLATFORM \
> GEN9_FEATURES, \
> - .gen = 9, \
> + GEN(9), \
> .platform = INTEL_SKYLAKE
>
> static const struct intel_device_info intel_skylake_gt1_info = {
> @@ -481,7 +490,7 @@ static const struct intel_device_info
> intel_skylake_gt4_info = {
> };
>
> #define GEN9
[Intel-gfx] [PATCH 1/4] drm/i915: Store gen_mask inside the static device info
Rather than deriving the gen_mask from the static intel_device_info->gen
at runtime, prefill it in the static data.
Signed-off-by: Chris Wilson
---
drivers/gpu/drm/i915/i915_drv.c | 2 --
drivers/gpu/drm/i915/i915_pci.c | 39 ---
2 files changed, 24 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index daa9060bdfcb..90f4adbbff28 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -902,8 +902,6 @@ static int i915_driver_init_early(struct drm_i915_private
*dev_priv,
device_info->platform_mask = BIT(device_info->platform);
BUG_ON(device_info->gen > sizeof(device_info->gen_mask) *
BITS_PER_BYTE);
- device_info->gen_mask = BIT(device_info->gen - 1);
-
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_pci.c b/drivers/gpu/drm/i915/i915_pci.c
index 4e7a10c89782..3b4516d7f9a4 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -29,6 +29,8 @@
#include "i915_drv.h"
#include "i915_selftest.h"
+#define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1)
+
#define GEN_DEFAULT_PIPEOFFSETS \
.pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \
PIPE_C_OFFSET, PIPE_EDP_OFFSET }, \
@@ -63,7 +65,8 @@
.page_sizes = I915_GTT_PAGE_SIZE_4K
#define GEN2_FEATURES \
- .gen = 2, .num_pipes = 1, \
+ GEN(2), \
+ .num_pipes = 1, \
.has_overlay = 1, .overlay_needs_physical = 1, \
.has_gmch_display = 1, \
.hws_needs_physical = 1, \
@@ -100,7 +103,8 @@ static const struct intel_device_info intel_i865g_info = {
};
#define GEN3_FEATURES \
- .gen = 3, .num_pipes = 2, \
+ GEN(3), \
+ .num_pipes = 2, \
.has_gmch_display = 1, \
.ring_mask = RENDER_RING, \
.has_snoop = true, \
@@ -163,7 +167,8 @@ static const struct intel_device_info intel_pineview_info =
{
};
#define GEN4_FEATURES \
- .gen = 4, .num_pipes = 2, \
+ GEN(4), \
+ .num_pipes = 2, \
.has_hotplug = 1, \
.has_gmch_display = 1, \
.ring_mask = RENDER_RING, \
@@ -205,7 +210,8 @@ static const struct intel_device_info intel_gm45_info = {
};
#define GEN5_FEATURES \
- .gen = 5, .num_pipes = 2, \
+ GEN(5), \
+ .num_pipes = 2, \
.has_hotplug = 1, \
.ring_mask = RENDER_RING | BSD_RING, \
.has_snoop = true, \
@@ -227,7 +233,8 @@ static const struct intel_device_info intel_ironlake_m_info
= {
};
#define GEN6_FEATURES \
- .gen = 6, .num_pipes = 2, \
+ GEN(6), \
+ .num_pipes = 2, \
.has_hotplug = 1, \
.has_fbc = 1, \
.ring_mask = RENDER_RING | BSD_RING | BLT_RING, \
@@ -270,7 +277,8 @@ static const struct intel_device_info
intel_sandybridge_m_gt2_info = {
};
#define GEN7_FEATURES \
- .gen = 7, .num_pipes = 3, \
+ GEN(7), \
+ .num_pipes = 3, \
.has_hotplug = 1, \
.has_fbc = 1, \
.ring_mask = RENDER_RING | BSD_RING | BLT_RING, \
@@ -324,7 +332,7 @@ static const struct intel_device_info
intel_ivybridge_q_info = {
static const struct intel_device_info intel_valleyview_info = {
.platform = INTEL_VALLEYVIEW,
- .gen = 7,
+ GEN(7),
.is_lp = 1,
.num_pipes = 2,
.has_psr = 1,
@@ -385,7 +393,7 @@ static const struct intel_device_info
intel_haswell_gt3_info = {
#define BDW_PLATFORM \
GEN8_FEATURES, \
- .gen = 8, \
+ GEN(8), \
.platform = INTEL_BROADWELL
static const struct intel_device_info intel_broadwell_gt1_info = {
@@ -413,7 +421,8 @@ static const struct intel_device_info
intel_broadwell_gt3_info = {
};
static const struct intel_device_info intel_cherryview_info = {
- .gen = 8, .num_pipes = 3,
+ GEN(8),
+ .num_pipes = 3,
.has_hotplug = 1,
.is_lp = 1,
.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
@@ -452,7 +461,7 @@ static const struct intel_device_info intel_cherryview_info
= {
#define SKL_PLATFORM \
GEN9_FEATURES, \
- .gen = 9, \
+ GEN(9), \
.platform = INTEL_SKYLAKE
static const struct intel_device_info intel_skylake_gt1_info = {
@@ -481,7 +490,7 @@ static const struct intel_device_info
intel_skylake_gt4_info = {
};
#define GEN9_LP_FEATURES \
- .gen = 9, \
+ GEN(9), \
.is_lp = 1, \
.has_hotplug = 1, \
.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING, \
@@ -526,7 +535,7 @@ static const struct intel_device_info intel_geminilake_info
= {
#define KBL_PLATFORM \
GEN9_FEATURES, \
- .gen = 9, \
+ GEN(9), \
.platform = INTEL_KABYLAKE
static const struct intel_device_info intel_kabylake_gt1_info = {
@@ -547,7 +556,7 @@ st
