Motivated by my review in https://patchwork.freedesktop.org/patch/443857/?series=92135&rev=5 I went to look why we needed the additional hw_id fields. It turns out we don't, but we kept adding new IDs to keep it consistent. Now that with the extra media engines we would just leave than zero'ed, let's refactor the code so we don't keep them around: they aren't used since GRAPHICS_VER == 8.
I'd say last patch is a stretch due to the use of _PICK() and hardcoding the map, but to me it seems to avoid making it more complex elsewhere.
Lucas De Marchi (4): drm/i915/gt: fix platform prefix drm/i915/gt: nuke unused legacy engine hw_id drm/i915/gt: rename legacy engine->hw_id to engine->gen6_hw_id drm/i915/gt: nuke gen6_hw_id
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 10 ---------- drivers/gpu/drm/i915/gt/intel_engine_types.h | 12 ------------ drivers/gpu/drm/i915/gt/intel_gt.c | 4 ++-- drivers/gpu/drm/i915/i915_reg.h | 4 +++- 4 files changed, 5 insertions(+), 25 deletions(-)
gen8_clear_engine_error_register() is actually not used by GRAPHICS_VER >= 8, since for those we are using another register that is not engine-dependent. Fix the platform prefix, to make clear we are not using any GEN6_RING_FAULT_REG_* one GRAPHICS_VER >= 8.
Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com --- drivers/gpu/drm/i915/gt/intel_gt.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c index e714e21c0a4d..a8efdd44e9cf 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.c +++ b/drivers/gpu/drm/i915/gt/intel_gt.c @@ -205,7 +205,7 @@ static void clear_register(struct intel_uncore *uncore, i915_reg_t reg) intel_uncore_rmw(uncore, reg, 0, 0); }
-static void gen8_clear_engine_error_register(struct intel_engine_cs *engine) +static void gen6_clear_engine_error_register(struct intel_engine_cs *engine) { GEN6_RING_FAULT_REG_RMW(engine, RING_FAULT_VALID, 0); GEN6_RING_FAULT_REG_POSTING_READ(engine); @@ -251,7 +251,7 @@ intel_gt_clear_error_registers(struct intel_gt *gt, enum intel_engine_id id;
for_each_engine_masked(engine, gt, engine_mask, id) - gen8_clear_engine_error_register(engine); + gen6_clear_engine_error_register(engine); } }
On 21/07/2021 00:20, Lucas De Marchi wrote:
gen8_clear_engine_error_register() is actually not used by GRAPHICS_VER >= 8, since for those we are using another register that is not engine-dependent. Fix the platform prefix, to make clear we are not using any GEN6_RING_FAULT_REG_* one GRAPHICS_VER >= 8.
Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com
drivers/gpu/drm/i915/gt/intel_gt.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c index e714e21c0a4d..a8efdd44e9cf 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.c +++ b/drivers/gpu/drm/i915/gt/intel_gt.c @@ -205,7 +205,7 @@ static void clear_register(struct intel_uncore *uncore, i915_reg_t reg) intel_uncore_rmw(uncore, reg, 0, 0); }
-static void gen8_clear_engine_error_register(struct intel_engine_cs *engine) +static void gen6_clear_engine_error_register(struct intel_engine_cs *engine) { GEN6_RING_FAULT_REG_RMW(engine, RING_FAULT_VALID, 0); GEN6_RING_FAULT_REG_POSTING_READ(engine); @@ -251,7 +251,7 @@ intel_gt_clear_error_registers(struct intel_gt *gt, enum intel_engine_id id;
for_each_engine_masked(engine, gt, engine_mask, id)
gen8_clear_engine_error_register(engine);
gen6_clear_engine_error_register(engine);
Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@intel.com
Regards,
Tvrtko
On Tue, Jul 20, 2021 at 04:20:11PM -0700, Lucas De Marchi wrote:
gen8_clear_engine_error_register() is actually not used by GRAPHICS_VER >= 8, since for those we are using another register that is not engine-dependent. Fix the platform prefix, to make clear we are not using any GEN6_RING_FAULT_REG_* one GRAPHICS_VER >= 8.
Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com
Reviewed-by: Matt Roper matthew.d.roper@intel.com
drivers/gpu/drm/i915/gt/intel_gt.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c index e714e21c0a4d..a8efdd44e9cf 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.c +++ b/drivers/gpu/drm/i915/gt/intel_gt.c @@ -205,7 +205,7 @@ static void clear_register(struct intel_uncore *uncore, i915_reg_t reg) intel_uncore_rmw(uncore, reg, 0, 0); }
-static void gen8_clear_engine_error_register(struct intel_engine_cs *engine) +static void gen6_clear_engine_error_register(struct intel_engine_cs *engine) { GEN6_RING_FAULT_REG_RMW(engine, RING_FAULT_VALID, 0); GEN6_RING_FAULT_REG_POSTING_READ(engine); @@ -251,7 +251,7 @@ intel_gt_clear_error_registers(struct intel_gt *gt, enum intel_engine_id id;
for_each_engine_masked(engine, gt, engine_mask, id)
gen8_clear_engine_error_register(engine);
gen6_clear_engine_error_register(engine);}
-- 2.31.1
The engine hw_id is only used by RING_FAULT_REG(), which is not used since GRAPHICS_VER == 8. We tend to keep adding new defines just to be consistent, but let's try to remove them and let them defined to 0 when not used.
Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com --- drivers/gpu/drm/i915/gt/intel_engine_cs.c | 4 ---- drivers/gpu/drm/i915/gt/intel_engine_types.h | 4 ---- 2 files changed, 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index d561573ed98c..a11f69f2e46e 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -80,7 +80,6 @@ static const struct engine_info intel_engines[] = { }, }, [VCS1] = { - .hw_id = VCS1_HW, .class = VIDEO_DECODE_CLASS, .instance = 1, .mmio_bases = { @@ -89,7 +88,6 @@ static const struct engine_info intel_engines[] = { }, }, [VCS2] = { - .hw_id = VCS2_HW, .class = VIDEO_DECODE_CLASS, .instance = 2, .mmio_bases = { @@ -97,7 +95,6 @@ static const struct engine_info intel_engines[] = { }, }, [VCS3] = { - .hw_id = VCS3_HW, .class = VIDEO_DECODE_CLASS, .instance = 3, .mmio_bases = { @@ -114,7 +111,6 @@ static const struct engine_info intel_engines[] = { }, }, [VECS1] = { - .hw_id = VECS1_HW, .class = VIDEO_ENHANCEMENT_CLASS, .instance = 1, .mmio_bases = { diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h index 1cb9c3b70b29..a107eb58ffa2 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h @@ -34,10 +34,6 @@ #define VCS0_HW 1 #define BCS0_HW 2 #define VECS0_HW 3 -#define VCS1_HW 4 -#define VCS2_HW 6 -#define VCS3_HW 7 -#define VECS1_HW 12
/* Gen11+ HW Engine class + instance */ #define RENDER_CLASS 0
On 21/07/2021 00:20, Lucas De Marchi wrote:
The engine hw_id is only used by RING_FAULT_REG(), which is not used since GRAPHICS_VER == 8. We tend to keep adding new defines just to be consistent, but let's try to remove them and let them defined to 0 when not used.
Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 4 ---- drivers/gpu/drm/i915/gt/intel_engine_types.h | 4 ---- 2 files changed, 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index d561573ed98c..a11f69f2e46e 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -80,7 +80,6 @@ static const struct engine_info intel_engines[] = { }, }, [VCS1] = {
.hw_id = VCS1_HW,@@ -89,7 +88,6 @@ static const struct engine_info intel_engines[] = { }, }, [VCS2] = {
.hw_id = VCS2_HW,@@ -97,7 +95,6 @@ static const struct engine_info intel_engines[] = { }, }, [VCS3] = {
.hw_id = VCS3_HW,@@ -114,7 +111,6 @@ static const struct engine_info intel_engines[] = { }, }, [VECS1] = {
.hw_id = VECS1_HW,diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h index 1cb9c3b70b29..a107eb58ffa2 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h @@ -34,10 +34,6 @@ #define VCS0_HW 1 #define BCS0_HW 2 #define VECS0_HW 3 -#define VCS1_HW 4 -#define VCS2_HW 6 -#define VCS3_HW 7 -#define VECS1_HW 12
/* Gen11+ HW Engine class + instance */ #define RENDER_CLASS 0
Story checks out AFAICS.
Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@intel.com
Regards,
Tvrtko
On Tue, Jul 20, 2021 at 04:20:12PM -0700, Lucas De Marchi wrote:
The engine hw_id is only used by RING_FAULT_REG(), which is not used since GRAPHICS_VER == 8. We tend to keep adding new defines just to be consistent, but let's try to remove them and let them defined to 0 when not used.
s/when not used/for engines that only exist on gen8+ platforms/
Reviewed-by: Matt Roper matthew.d.roper@intel.com
For historical reference, we did use hw_id on gen8+ platforms too until relatively recently --- it was used to set the engine's guc_id as well up until:
commit c784e5249e773689e38d2bc1749f08b986621a26 Author: John Harrison John.C.Harrison@Intel.com Date: Wed Oct 28 07:58:24 2020 -0700
drm/i915/guc: Update to use firmware v49.0.1
Matt
Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 4 ---- drivers/gpu/drm/i915/gt/intel_engine_types.h | 4 ---- 2 files changed, 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index d561573ed98c..a11f69f2e46e 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -80,7 +80,6 @@ static const struct engine_info intel_engines[] = { }, }, [VCS1] = {
.hw_id = VCS1_HW,@@ -89,7 +88,6 @@ static const struct engine_info intel_engines[] = { }, }, [VCS2] = {
.hw_id = VCS2_HW,@@ -97,7 +95,6 @@ static const struct engine_info intel_engines[] = { }, }, [VCS3] = {
.hw_id = VCS3_HW,@@ -114,7 +111,6 @@ static const struct engine_info intel_engines[] = { }, }, [VECS1] = {
.hw_id = VECS1_HW,diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h index 1cb9c3b70b29..a107eb58ffa2 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h @@ -34,10 +34,6 @@ #define VCS0_HW 1 #define BCS0_HW 2 #define VECS0_HW 3 -#define VCS1_HW 4 -#define VCS2_HW 6 -#define VCS3_HW 7 -#define VECS1_HW 12
/* Gen11+ HW Engine class + instance */
#define RENDER_CLASS 0
2.31.1
On Wed, Jul 21, 2021 at 03:47:22PM -0700, Matt Roper wrote:
On Tue, Jul 20, 2021 at 04:20:12PM -0700, Lucas De Marchi wrote:
The engine hw_id is only used by RING_FAULT_REG(), which is not used since GRAPHICS_VER == 8. We tend to keep adding new defines just to be consistent, but let's try to remove them and let them defined to 0 when not used.
s/when not used/for engines that only exist on gen8+ platforms/
Reviewed-by: Matt Roper matthew.d.roper@intel.com
For historical reference, we did use hw_id on gen8+ platforms too until relatively recently --- it was used to set the engine's guc_id as well up until:
commit c784e5249e773689e38d2bc1749f08b986621a26 Author: John Harrison <John.C.Harrison@Intel.com> Date: Wed Oct 28 07:58:24 2020 -0700 drm/i915/guc: Update to use firmware v49.0.1
thanks for digging this, I will add that to the commit message as well.
Lucas De Marchi
We kept adding new engines and for that increasing hw_id unnecessarily: it's not used since GRAPHICS_VER == 8. Prepend "gen6" to the field and try to pack it in the structs to give a hint this field is actually not used in recent platforms.
Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com --- drivers/gpu/drm/i915/gt/intel_engine_cs.c | 12 ++++++------ drivers/gpu/drm/i915/gt/intel_engine_types.h | 2 +- drivers/gpu/drm/i915/i915_reg.h | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index a11f69f2e46e..508221de411c 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -42,7 +42,7 @@
#define MAX_MMIO_BASES 3 struct engine_info { - unsigned int hw_id; + u8 gen6_hw_id; u8 class; u8 instance; /* mmio bases table *must* be sorted in reverse graphics_ver order */ @@ -54,7 +54,7 @@ struct engine_info {
static const struct engine_info intel_engines[] = { [RCS0] = { - .hw_id = RCS0_HW, + .gen6_hw_id = RCS0_HW, .class = RENDER_CLASS, .instance = 0, .mmio_bases = { @@ -62,7 +62,7 @@ static const struct engine_info intel_engines[] = { }, }, [BCS0] = { - .hw_id = BCS0_HW, + .gen6_hw_id = BCS0_HW, .class = COPY_ENGINE_CLASS, .instance = 0, .mmio_bases = { @@ -70,7 +70,7 @@ static const struct engine_info intel_engines[] = { }, }, [VCS0] = { - .hw_id = VCS0_HW, + .gen6_hw_id = VCS0_HW, .class = VIDEO_DECODE_CLASS, .instance = 0, .mmio_bases = { @@ -102,7 +102,7 @@ static const struct engine_info intel_engines[] = { }, }, [VECS0] = { - .hw_id = VECS0_HW, + .gen6_hw_id = VECS0_HW, .class = VIDEO_ENHANCEMENT_CLASS, .instance = 0, .mmio_bases = { @@ -290,7 +290,7 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id) engine->i915 = i915; engine->gt = gt; engine->uncore = gt->uncore; - engine->hw_id = info->hw_id; + engine->gen6_hw_id = info->gen6_hw_id; guc_class = engine_class_to_guc_class(info->class); engine->guc_id = MAKE_GUC_ID(guc_class, info->instance); engine->mmio_base = __engine_mmio_base(i915, info->mmio_bases); diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h index a107eb58ffa2..266422d8d1b1 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h @@ -264,11 +264,11 @@ struct intel_engine_cs { enum intel_engine_id id; enum intel_engine_id legacy_idx;
- unsigned int hw_id; unsigned int guc_id;
intel_engine_mask_t mask;
+ u8 gen6_hw_id; u8 class; u8 instance;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 943fe485c662..8750ffce9d61 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -2572,7 +2572,7 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) #define ARB_MODE_BWGTLB_DISABLE (1 << 9) #define ARB_MODE_SWIZZLE_BDW (1 << 1) #define RENDER_HWS_PGA_GEN7 _MMIO(0x04080) -#define RING_FAULT_REG(engine) _MMIO(0x4094 + 0x100 * (engine)->hw_id) +#define RING_FAULT_REG(engine) _MMIO(0x4094 + 0x100 * (engine)->gen6_hw_id) #define GEN8_RING_FAULT_REG _MMIO(0x4094) #define GEN12_RING_FAULT_REG _MMIO(0xcec4) #define GEN8_RING_FAULT_ENGINE_ID(x) (((x) >> 12) & 0x7)
On 21/07/2021 00:20, Lucas De Marchi wrote:
We kept adding new engines and for that increasing hw_id unnecessarily: it's not used since GRAPHICS_VER == 8. Prepend "gen6" to the field and try to pack it in the structs to give a hint this field is actually not used in recent platforms.
Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 12 ++++++------ drivers/gpu/drm/i915/gt/intel_engine_types.h | 2 +- drivers/gpu/drm/i915/i915_reg.h | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index a11f69f2e46e..508221de411c 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -42,7 +42,7 @@
#define MAX_MMIO_BASES 3 struct engine_info {
- unsigned int hw_id;
- u8 gen6_hw_id; u8 class; u8 instance; /* mmio bases table *must* be sorted in reverse graphics_ver order */
@@ -54,7 +54,7 @@ struct engine_info {
static const struct engine_info intel_engines[] = { [RCS0] = {
.hw_id = RCS0_HW,
.gen6_hw_id = RCS0_HW,@@ -62,7 +62,7 @@ static const struct engine_info intel_engines[] = { }, }, [BCS0] = {
.hw_id = BCS0_HW,
.gen6_hw_id = BCS0_HW,@@ -70,7 +70,7 @@ static const struct engine_info intel_engines[] = { }, }, [VCS0] = {
.hw_id = VCS0_HW,
.gen6_hw_id = VCS0_HW,@@ -102,7 +102,7 @@ static const struct engine_info intel_engines[] = { }, }, [VECS0] = {
.hw_id = VECS0_HW,
.gen6_hw_id = VECS0_HW,@@ -290,7 +290,7 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id) engine->i915 = i915; engine->gt = gt; engine->uncore = gt->uncore;
- engine->hw_id = info->hw_id;
- engine->gen6_hw_id = info->gen6_hw_id; guc_class = engine_class_to_guc_class(info->class); engine->guc_id = MAKE_GUC_ID(guc_class, info->instance); engine->mmio_base = __engine_mmio_base(i915, info->mmio_bases);
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h index a107eb58ffa2..266422d8d1b1 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h @@ -264,11 +264,11 @@ struct intel_engine_cs { enum intel_engine_id id; enum intel_engine_id legacy_idx;
unsigned int hw_id; unsigned int guc_id;
intel_engine_mask_t mask;
- u8 gen6_hw_id; u8 class; u8 instance;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 943fe485c662..8750ffce9d61 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -2572,7 +2572,7 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) #define ARB_MODE_BWGTLB_DISABLE (1 << 9) #define ARB_MODE_SWIZZLE_BDW (1 << 1) #define RENDER_HWS_PGA_GEN7 _MMIO(0x04080) -#define RING_FAULT_REG(engine) _MMIO(0x4094 + 0x100 * (engine)->hw_id) +#define RING_FAULT_REG(engine) _MMIO(0x4094 + 0x100 * (engine)->gen6_hw_id) #define GEN8_RING_FAULT_REG _MMIO(0x4094) #define GEN12_RING_FAULT_REG _MMIO(0xcec4) #define GEN8_RING_FAULT_ENGINE_ID(x) (((x) >> 12) & 0x7)
Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@intel.com
Regards,
Tvrtko
On Tue, Jul 20, 2021 at 04:20:13PM -0700, Lucas De Marchi wrote:
We kept adding new engines and for that increasing hw_id unnecessarily: it's not used since GRAPHICS_VER == 8. Prepend "gen6" to the field and try to pack it in the structs to give a hint this field is actually not used in recent platforms.
Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com
Reviewed-by: Matt Roper matthew.d.roper@intel.com
although if we apply patch #4 we could probably drop this intermediate step.
Matt
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 12 ++++++------ drivers/gpu/drm/i915/gt/intel_engine_types.h | 2 +- drivers/gpu/drm/i915/i915_reg.h | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index a11f69f2e46e..508221de411c 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -42,7 +42,7 @@
#define MAX_MMIO_BASES 3 struct engine_info {
- unsigned int hw_id;
- u8 gen6_hw_id; u8 class; u8 instance; /* mmio bases table *must* be sorted in reverse graphics_ver order */
@@ -54,7 +54,7 @@ struct engine_info {
static const struct engine_info intel_engines[] = { [RCS0] = {
.hw_id = RCS0_HW,
.gen6_hw_id = RCS0_HW,@@ -62,7 +62,7 @@ static const struct engine_info intel_engines[] = { }, }, [BCS0] = {
.hw_id = BCS0_HW,
.gen6_hw_id = BCS0_HW,@@ -70,7 +70,7 @@ static const struct engine_info intel_engines[] = { }, }, [VCS0] = {
.hw_id = VCS0_HW,
.gen6_hw_id = VCS0_HW,@@ -102,7 +102,7 @@ static const struct engine_info intel_engines[] = { }, }, [VECS0] = {
.hw_id = VECS0_HW,
.gen6_hw_id = VECS0_HW,@@ -290,7 +290,7 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id) engine->i915 = i915; engine->gt = gt; engine->uncore = gt->uncore;
- engine->hw_id = info->hw_id;
- engine->gen6_hw_id = info->gen6_hw_id; guc_class = engine_class_to_guc_class(info->class); engine->guc_id = MAKE_GUC_ID(guc_class, info->instance); engine->mmio_base = __engine_mmio_base(i915, info->mmio_bases);
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h index a107eb58ffa2..266422d8d1b1 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h @@ -264,11 +264,11 @@ struct intel_engine_cs { enum intel_engine_id id; enum intel_engine_id legacy_idx;
unsigned int hw_id; unsigned int guc_id;
intel_engine_mask_t mask;
- u8 gen6_hw_id; u8 class; u8 instance;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 943fe485c662..8750ffce9d61 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -2572,7 +2572,7 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) #define ARB_MODE_BWGTLB_DISABLE (1 << 9) #define ARB_MODE_SWIZZLE_BDW (1 << 1) #define RENDER_HWS_PGA_GEN7 _MMIO(0x04080) -#define RING_FAULT_REG(engine) _MMIO(0x4094 + 0x100 * (engine)->hw_id) +#define RING_FAULT_REG(engine) _MMIO(0x4094 + 0x100 * (engine)->gen6_hw_id) #define GEN8_RING_FAULT_REG _MMIO(0x4094) #define GEN12_RING_FAULT_REG _MMIO(0xcec4)
#define GEN8_RING_FAULT_ENGINE_ID(x) (((x) >> 12) & 0x7)
2.31.1
On Wed, Jul 21, 2021 at 3:51 PM Matt Roper matthew.d.roper@intel.com wrote:
On Tue, Jul 20, 2021 at 04:20:13PM -0700, Lucas De Marchi wrote:
We kept adding new engines and for that increasing hw_id unnecessarily: it's not used since GRAPHICS_VER == 8. Prepend "gen6" to the field and try to pack it in the structs to give a hint this field is actually not used in recent platforms.
Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com
Reviewed-by: Matt Roper matthew.d.roper@intel.com
although if we apply patch #4 we could probably drop this intermediate
I was not so confident people would agree with that patch. Adding the macros to the header as suggested helps it being more palatable though.
thanks Lucas De Marchi
step.
Matt
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 12 ++++++------ drivers/gpu/drm/i915/gt/intel_engine_types.h | 2 +- drivers/gpu/drm/i915/i915_reg.h | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index a11f69f2e46e..508221de411c 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -42,7 +42,7 @@
#define MAX_MMIO_BASES 3 struct engine_info {
unsigned int hw_id;
u8 gen6_hw_id; u8 class; u8 instance; /* mmio bases table *must* be sorted in reverse graphics_ver order */@@ -54,7 +54,7 @@ struct engine_info {
static const struct engine_info intel_engines[] = { [RCS0] = {
.hw_id = RCS0_HW,
.gen6_hw_id = RCS0_HW, .class = RENDER_CLASS, .instance = 0, .mmio_bases = {@@ -62,7 +62,7 @@ static const struct engine_info intel_engines[] = { }, }, [BCS0] = {
.hw_id = BCS0_HW,
.gen6_hw_id = BCS0_HW, .class = COPY_ENGINE_CLASS, .instance = 0, .mmio_bases = {@@ -70,7 +70,7 @@ static const struct engine_info intel_engines[] = { }, }, [VCS0] = {
.hw_id = VCS0_HW,
.gen6_hw_id = VCS0_HW, .class = VIDEO_DECODE_CLASS, .instance = 0, .mmio_bases = {@@ -102,7 +102,7 @@ static const struct engine_info intel_engines[] = { }, }, [VECS0] = {
.hw_id = VECS0_HW,
.gen6_hw_id = VECS0_HW, .class = VIDEO_ENHANCEMENT_CLASS, .instance = 0, .mmio_bases = {@@ -290,7 +290,7 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id) engine->i915 = i915; engine->gt = gt; engine->uncore = gt->uncore;
engine->hw_id = info->hw_id;
engine->gen6_hw_id = info->gen6_hw_id; guc_class = engine_class_to_guc_class(info->class); engine->guc_id = MAKE_GUC_ID(guc_class, info->instance); engine->mmio_base = __engine_mmio_base(i915, info->mmio_bases);diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h index a107eb58ffa2..266422d8d1b1 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h @@ -264,11 +264,11 @@ struct intel_engine_cs { enum intel_engine_id id; enum intel_engine_id legacy_idx;
unsigned int hw_id; unsigned int guc_id; intel_engine_mask_t mask;
u8 gen6_hw_id; u8 class; u8 instance;diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 943fe485c662..8750ffce9d61 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -2572,7 +2572,7 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) #define ARB_MODE_BWGTLB_DISABLE (1 << 9) #define ARB_MODE_SWIZZLE_BDW (1 << 1) #define RENDER_HWS_PGA_GEN7 _MMIO(0x04080) -#define RING_FAULT_REG(engine) _MMIO(0x4094 + 0x100 * (engine)->hw_id) +#define RING_FAULT_REG(engine) _MMIO(0x4094 + 0x100 * (engine)->gen6_hw_id) #define GEN8_RING_FAULT_REG _MMIO(0x4094) #define GEN12_RING_FAULT_REG _MMIO(0xcec4)
#define GEN8_RING_FAULT_ENGINE_ID(x) (((x) >> 12) & 0x7)
2.31.1
-- Matt Roper Graphics Software Engineer VTT-OSGC Platform Enablement Intel Corporation (916) 356-2795 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
This is only used by GRAPHICS_VER == 6 and GRAPHICS_VER == 7. All other recent platforms do not depend on this field, so it doesn't make much sense to keep it generic like that. Instead, just do a mapping from engine class to HW ID in the single place that is needed.
Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com --- drivers/gpu/drm/i915/gt/intel_engine_cs.c | 6 ------ drivers/gpu/drm/i915/gt/intel_engine_types.h | 8 -------- drivers/gpu/drm/i915/i915_reg.h | 4 +++- 3 files changed, 3 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index 508221de411c..0a04e8d90e9e 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -42,7 +42,6 @@
#define MAX_MMIO_BASES 3 struct engine_info { - u8 gen6_hw_id; u8 class; u8 instance; /* mmio bases table *must* be sorted in reverse graphics_ver order */ @@ -54,7 +53,6 @@ struct engine_info {
static const struct engine_info intel_engines[] = { [RCS0] = { - .gen6_hw_id = RCS0_HW, .class = RENDER_CLASS, .instance = 0, .mmio_bases = { @@ -62,7 +60,6 @@ static const struct engine_info intel_engines[] = { }, }, [BCS0] = { - .gen6_hw_id = BCS0_HW, .class = COPY_ENGINE_CLASS, .instance = 0, .mmio_bases = { @@ -70,7 +67,6 @@ static const struct engine_info intel_engines[] = { }, }, [VCS0] = { - .gen6_hw_id = VCS0_HW, .class = VIDEO_DECODE_CLASS, .instance = 0, .mmio_bases = { @@ -102,7 +98,6 @@ static const struct engine_info intel_engines[] = { }, }, [VECS0] = { - .gen6_hw_id = VECS0_HW, .class = VIDEO_ENHANCEMENT_CLASS, .instance = 0, .mmio_bases = { @@ -290,7 +285,6 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id) engine->i915 = i915; engine->gt = gt; engine->uncore = gt->uncore; - engine->gen6_hw_id = info->gen6_hw_id; guc_class = engine_class_to_guc_class(info->class); engine->guc_id = MAKE_GUC_ID(guc_class, info->instance); engine->mmio_base = __engine_mmio_base(i915, info->mmio_bases); diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h index 266422d8d1b1..64330bfb7641 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h @@ -28,13 +28,6 @@ #include "intel_wakeref.h" #include "intel_workarounds_types.h"
-/* Legacy HW Engine ID */ - -#define RCS0_HW 0 -#define VCS0_HW 1 -#define BCS0_HW 2 -#define VECS0_HW 3 - /* Gen11+ HW Engine class + instance */ #define RENDER_CLASS 0 #define VIDEO_DECODE_CLASS 1 @@ -268,7 +261,6 @@ struct intel_engine_cs {
intel_engine_mask_t mask;
- u8 gen6_hw_id; u8 class; u8 instance;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 8750ffce9d61..d91386f4828e 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -2572,7 +2572,9 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) #define ARB_MODE_BWGTLB_DISABLE (1 << 9) #define ARB_MODE_SWIZZLE_BDW (1 << 1) #define RENDER_HWS_PGA_GEN7 _MMIO(0x04080) -#define RING_FAULT_REG(engine) _MMIO(0x4094 + 0x100 * (engine)->gen6_hw_id) + +#define _GEN6_ENGINE_CLASS_TO_ID(class) _PICK((class), 0, 1, 3, 2) +#define RING_FAULT_REG(engine) _MMIO(0x4094 + 0x100 * _GEN6_ENGINE_CLASS_TO_ID((engine)->class)) #define GEN8_RING_FAULT_REG _MMIO(0x4094) #define GEN12_RING_FAULT_REG _MMIO(0xcec4) #define GEN8_RING_FAULT_ENGINE_ID(x) (((x) >> 12) & 0x7)
On 21/07/2021 00:20, Lucas De Marchi wrote:
This is only used by GRAPHICS_VER == 6 and GRAPHICS_VER == 7. All other recent platforms do not depend on this field, so it doesn't make much sense to keep it generic like that. Instead, just do a mapping from engine class to HW ID in the single place that is needed.
Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 6 ------ drivers/gpu/drm/i915/gt/intel_engine_types.h | 8 -------- drivers/gpu/drm/i915/i915_reg.h | 4 +++- 3 files changed, 3 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index 508221de411c..0a04e8d90e9e 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -42,7 +42,6 @@
#define MAX_MMIO_BASES 3 struct engine_info {
- u8 gen6_hw_id; u8 class; u8 instance; /* mmio bases table *must* be sorted in reverse graphics_ver order */
@@ -54,7 +53,6 @@ struct engine_info {
static const struct engine_info intel_engines[] = { [RCS0] = {
.gen6_hw_id = RCS0_HW,@@ -62,7 +60,6 @@ static const struct engine_info intel_engines[] = { }, }, [BCS0] = {
.gen6_hw_id = BCS0_HW,@@ -70,7 +67,6 @@ static const struct engine_info intel_engines[] = { }, }, [VCS0] = {
.gen6_hw_id = VCS0_HW,@@ -102,7 +98,6 @@ static const struct engine_info intel_engines[] = { }, }, [VECS0] = {
.gen6_hw_id = VECS0_HW,@@ -290,7 +285,6 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id) engine->i915 = i915; engine->gt = gt; engine->uncore = gt->uncore;
- engine->gen6_hw_id = info->gen6_hw_id; guc_class = engine_class_to_guc_class(info->class); engine->guc_id = MAKE_GUC_ID(guc_class, info->instance); engine->mmio_base = __engine_mmio_base(i915, info->mmio_bases);
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h index 266422d8d1b1..64330bfb7641 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h @@ -28,13 +28,6 @@ #include "intel_wakeref.h" #include "intel_workarounds_types.h"
-/* Legacy HW Engine ID */
-#define RCS0_HW 0 -#define VCS0_HW 1 -#define BCS0_HW 2 -#define VECS0_HW 3
- /* Gen11+ HW Engine class + instance */ #define RENDER_CLASS 0 #define VIDEO_DECODE_CLASS 1
@@ -268,7 +261,6 @@ struct intel_engine_cs {
intel_engine_mask_t mask;
- u8 gen6_hw_id; u8 class; u8 instance;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 8750ffce9d61..d91386f4828e 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -2572,7 +2572,9 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) #define ARB_MODE_BWGTLB_DISABLE (1 << 9) #define ARB_MODE_SWIZZLE_BDW (1 << 1) #define RENDER_HWS_PGA_GEN7 _MMIO(0x04080) -#define RING_FAULT_REG(engine) _MMIO(0x4094 + 0x100 * (engine)->gen6_hw_id)
+#define _GEN6_ENGINE_CLASS_TO_ID(class) _PICK((class), 0, 1, 3, 2) +#define RING_FAULT_REG(engine) _MMIO(0x4094 + 0x100 * _GEN6_ENGINE_CLASS_TO_ID((engine)->class))
Makes sense to me.
Maybe HW_ID and HW_CLASS in the macro name? Not sure.
Only open I have is why the "Gen11+ HW Engine class + instance" comment and now we would tie that, allegedly Gen11 concept, with Gen6-7. Care to do some digging?
Regards,
Tvrtko
#define GEN8_RING_FAULT_REG _MMIO(0x4094) #define GEN12_RING_FAULT_REG _MMIO(0xcec4) #define GEN8_RING_FAULT_ENGINE_ID(x) (((x) >> 12) & 0x7)
On Wed, Jul 21, 2021 at 10:25:59AM +0100, Tvrtko Ursulin wrote:
On 21/07/2021 00:20, Lucas De Marchi wrote:
This is only used by GRAPHICS_VER == 6 and GRAPHICS_VER == 7. All other recent platforms do not depend on this field, so it doesn't make much sense to keep it generic like that. Instead, just do a mapping from engine class to HW ID in the single place that is needed.
Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 6 ------ drivers/gpu/drm/i915/gt/intel_engine_types.h | 8 -------- drivers/gpu/drm/i915/i915_reg.h | 4 +++- 3 files changed, 3 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index 508221de411c..0a04e8d90e9e 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -42,7 +42,6 @@ #define MAX_MMIO_BASES 3 struct engine_info {
- u8 gen6_hw_id; u8 class; u8 instance; /* mmio bases table *must* be sorted in reverse graphics_ver order */
@@ -54,7 +53,6 @@ struct engine_info { static const struct engine_info intel_engines[] = { [RCS0] = {
.gen6_hw_id = RCS0_HW,@@ -62,7 +60,6 @@ static const struct engine_info intel_engines[] = { }, }, [BCS0] = {
.gen6_hw_id = BCS0_HW,@@ -70,7 +67,6 @@ static const struct engine_info intel_engines[] = { }, }, [VCS0] = {
.gen6_hw_id = VCS0_HW,@@ -102,7 +98,6 @@ static const struct engine_info intel_engines[] = { }, }, [VECS0] = {
.gen6_hw_id = VECS0_HW,@@ -290,7 +285,6 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id) engine->i915 = i915; engine->gt = gt; engine->uncore = gt->uncore;
- engine->gen6_hw_id = info->gen6_hw_id; guc_class = engine_class_to_guc_class(info->class); engine->guc_id = MAKE_GUC_ID(guc_class, info->instance); engine->mmio_base = __engine_mmio_base(i915, info->mmio_bases);
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h index 266422d8d1b1..64330bfb7641 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h @@ -28,13 +28,6 @@ #include "intel_wakeref.h" #include "intel_workarounds_types.h" -/* Legacy HW Engine ID */
-#define RCS0_HW 0 -#define VCS0_HW 1 -#define BCS0_HW 2 -#define VECS0_HW 3
/* Gen11+ HW Engine class + instance */ #define RENDER_CLASS 0 #define VIDEO_DECODE_CLASS 1 @@ -268,7 +261,6 @@ struct intel_engine_cs { intel_engine_mask_t mask;
- u8 gen6_hw_id; u8 class; u8 instance;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 8750ffce9d61..d91386f4828e 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -2572,7 +2572,9 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) #define ARB_MODE_BWGTLB_DISABLE (1 << 9) #define ARB_MODE_SWIZZLE_BDW (1 << 1) #define RENDER_HWS_PGA_GEN7 _MMIO(0x04080) -#define RING_FAULT_REG(engine) _MMIO(0x4094 + 0x100 * (engine)->gen6_hw_id)
+#define _GEN6_ENGINE_CLASS_TO_ID(class) _PICK((class), 0, 1, 3, 2) +#define RING_FAULT_REG(engine) _MMIO(0x4094 + 0x100 * _GEN6_ENGINE_CLASS_TO_ID((engine)->class))
Makes sense to me.
Maybe HW_ID and HW_CLASS in the macro name? Not sure.
I can do that... I think I avoided it because it makes the macro very big. Anyway, this should be called in just one place, so it doesn't matter much... I can add it.
Only open I have is why the "Gen11+ HW Engine class + instance" comment and now we would tie that, allegedly Gen11 concept, with Gen6-7. Care to do some digging?
Not sure. This comes from 3d7b3039741d ("drm/i915: Move engine IDs out of i915_reg.h") that I reviewed :-o
Cc'ing Daniele. I don't see "class" as a Gen11+ thing. Is it just that those numbers started to make sense for gen11? Since
a) we are using the class even for GRAPHICS_VER < 11 b) the legacy HW IDs shouldn't be used anywhere else anymore
we could
1) move the legacy defines back to i915_reg.h 2) use them in the macro above (IMO would slightly improve the readability of that _PICK() call) 3) Remove the "Gen11+" comment in the _CLASS macros to avoid misunderstandings
Thoughts?
thanks Lucas De Marchi
Regards,
Tvrtko
#define GEN8_RING_FAULT_REG _MMIO(0x4094) #define GEN12_RING_FAULT_REG _MMIO(0xcec4) #define GEN8_RING_FAULT_ENGINE_ID(x) (((x) >> 12) & 0x7)
On 21/07/2021 19:44, Lucas De Marchi wrote:
On Wed, Jul 21, 2021 at 10:25:59AM +0100, Tvrtko Ursulin wrote:
On 21/07/2021 00:20, Lucas De Marchi wrote:
This is only used by GRAPHICS_VER == 6 and GRAPHICS_VER == 7. All other recent platforms do not depend on this field, so it doesn't make much sense to keep it generic like that. Instead, just do a mapping from engine class to HW ID in the single place that is needed.
Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 6 ------ drivers/gpu/drm/i915/gt/intel_engine_types.h | 8 -------- drivers/gpu/drm/i915/i915_reg.h | 4 +++- 3 files changed, 3 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index 508221de411c..0a04e8d90e9e 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -42,7 +42,6 @@ #define MAX_MMIO_BASES 3 struct engine_info { - u8 gen6_hw_id; u8 class; u8 instance; /* mmio bases table *must* be sorted in reverse graphics_ver order */ @@ -54,7 +53,6 @@ struct engine_info { static const struct engine_info intel_engines[] = { [RCS0] = { - .gen6_hw_id = RCS0_HW, .class = RENDER_CLASS, .instance = 0, .mmio_bases = { @@ -62,7 +60,6 @@ static const struct engine_info intel_engines[] = { }, }, [BCS0] = { - .gen6_hw_id = BCS0_HW, .class = COPY_ENGINE_CLASS, .instance = 0, .mmio_bases = { @@ -70,7 +67,6 @@ static const struct engine_info intel_engines[] = { }, }, [VCS0] = { - .gen6_hw_id = VCS0_HW, .class = VIDEO_DECODE_CLASS, .instance = 0, .mmio_bases = { @@ -102,7 +98,6 @@ static const struct engine_info intel_engines[] = { }, }, [VECS0] = { - .gen6_hw_id = VECS0_HW, .class = VIDEO_ENHANCEMENT_CLASS, .instance = 0, .mmio_bases = { @@ -290,7 +285,6 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id) engine->i915 = i915; engine->gt = gt; engine->uncore = gt->uncore; - engine->gen6_hw_id = info->gen6_hw_id; guc_class = engine_class_to_guc_class(info->class); engine->guc_id = MAKE_GUC_ID(guc_class, info->instance); engine->mmio_base = __engine_mmio_base(i915, info->mmio_bases); diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h index 266422d8d1b1..64330bfb7641 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h @@ -28,13 +28,6 @@ #include "intel_wakeref.h" #include "intel_workarounds_types.h" -/* Legacy HW Engine ID */
-#define RCS0_HW 0 -#define VCS0_HW 1 -#define BCS0_HW 2 -#define VECS0_HW 3
/* Gen11+ HW Engine class + instance */ #define RENDER_CLASS 0 #define VIDEO_DECODE_CLASS 1 @@ -268,7 +261,6 @@ struct intel_engine_cs { intel_engine_mask_t mask; - u8 gen6_hw_id; u8 class; u8 instance; diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 8750ffce9d61..d91386f4828e 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -2572,7 +2572,9 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) #define ARB_MODE_BWGTLB_DISABLE (1 << 9) #define ARB_MODE_SWIZZLE_BDW (1 << 1) #define RENDER_HWS_PGA_GEN7 _MMIO(0x04080) -#define RING_FAULT_REG(engine) _MMIO(0x4094 + 0x100 * (engine)->gen6_hw_id)
+#define _GEN6_ENGINE_CLASS_TO_ID(class) _PICK((class), 0, 1, 3, 2) +#define RING_FAULT_REG(engine) _MMIO(0x4094 + 0x100 * _GEN6_ENGINE_CLASS_TO_ID((engine)->class))
Makes sense to me.
Maybe HW_ID and HW_CLASS in the macro name? Not sure.
I can do that... I think I avoided it because it makes the macro very big. Anyway, this should be called in just one place, so it doesn't matter much... I can add it.
Only open I have is why the "Gen11+ HW Engine class + instance" comment and now we would tie that, allegedly Gen11 concept, with Gen6-7. Care to do some digging?
Not sure. This comes from 3d7b3039741d ("drm/i915: Move engine IDs out of i915_reg.h") that I reviewed :-o
Cc'ing Daniele. I don't see "class" as a Gen11+ thing. Is it just that those numbers started to make sense for gen11? Since
a) we are using the class even for GRAPHICS_VER < 11 b) the legacy HW IDs shouldn't be used anywhere else anymore
we could
- move the legacy defines back to i915_reg.h
- use them in the macro above (IMO would slightly improve the
readability of that _PICK() call) 3) Remove the "Gen11+" comment in the _CLASS macros to avoid misunderstandings
Thoughts?
What Matt suggested sounds fine to me (using _PICK with _RING_FAULT_REG_RCS etc). It retires the concept of hw id from the code base completely and hopefully the need for it does not re-surface.
Regards,
Tvrtko
On Tue, Jul 20, 2021 at 04:20:14PM -0700, Lucas De Marchi wrote:
This is only used by GRAPHICS_VER == 6 and GRAPHICS_VER == 7. All other recent platforms do not depend on this field, so it doesn't make much sense to keep it generic like that. Instead, just do a mapping from engine class to HW ID in the single place that is needed.
Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 6 ------ drivers/gpu/drm/i915/gt/intel_engine_types.h | 8 -------- drivers/gpu/drm/i915/i915_reg.h | 4 +++- 3 files changed, 3 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index 508221de411c..0a04e8d90e9e 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -42,7 +42,6 @@
#define MAX_MMIO_BASES 3 struct engine_info {
- u8 gen6_hw_id; u8 class; u8 instance; /* mmio bases table *must* be sorted in reverse graphics_ver order */
@@ -54,7 +53,6 @@ struct engine_info {
static const struct engine_info intel_engines[] = { [RCS0] = {
.gen6_hw_id = RCS0_HW,@@ -62,7 +60,6 @@ static const struct engine_info intel_engines[] = { }, }, [BCS0] = {
.gen6_hw_id = BCS0_HW,@@ -70,7 +67,6 @@ static const struct engine_info intel_engines[] = { }, }, [VCS0] = {
.gen6_hw_id = VCS0_HW,@@ -102,7 +98,6 @@ static const struct engine_info intel_engines[] = { }, }, [VECS0] = {
.gen6_hw_id = VECS0_HW,@@ -290,7 +285,6 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id) engine->i915 = i915; engine->gt = gt; engine->uncore = gt->uncore;
- engine->gen6_hw_id = info->gen6_hw_id; guc_class = engine_class_to_guc_class(info->class); engine->guc_id = MAKE_GUC_ID(guc_class, info->instance); engine->mmio_base = __engine_mmio_base(i915, info->mmio_bases);
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h index 266422d8d1b1..64330bfb7641 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h @@ -28,13 +28,6 @@ #include "intel_wakeref.h" #include "intel_workarounds_types.h"
-/* Legacy HW Engine ID */
-#define RCS0_HW 0 -#define VCS0_HW 1 -#define BCS0_HW 2 -#define VECS0_HW 3
/* Gen11+ HW Engine class + instance */ #define RENDER_CLASS 0 #define VIDEO_DECODE_CLASS 1 @@ -268,7 +261,6 @@ struct intel_engine_cs {
intel_engine_mask_t mask;
- u8 gen6_hw_id; u8 class; u8 instance;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 8750ffce9d61..d91386f4828e 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -2572,7 +2572,9 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) #define ARB_MODE_BWGTLB_DISABLE (1 << 9) #define ARB_MODE_SWIZZLE_BDW (1 << 1) #define RENDER_HWS_PGA_GEN7 _MMIO(0x04080) -#define RING_FAULT_REG(engine) _MMIO(0x4094 + 0x100 * (engine)->gen6_hw_id)
+#define _GEN6_ENGINE_CLASS_TO_ID(class) _PICK((class), 0, 1, 3, 2) +#define RING_FAULT_REG(engine) _MMIO(0x4094 + 0x100 * _GEN6_ENGINE_CLASS_TO_ID((engine)->class))
If you want to make this more clear to someone reading it down the road, you could always do something explicit like:
#define _RING_FAULT_REG_RCS 0x4094 #define _RING_FAULT_REG_VCS 0x4194 #define _RING_FAULT_REG_BCS 0x4294 #define _RING_FAULT_REG_VECS 0x4394 #define RING_FAULT_REG(engine) _MMIO(_PICK((engine)->class, \ _RING_FAULT_REG_RCS, \ _RING_FAULT_REG_VCS, \ _RING_FAULT_REG_VECS, \ _RING_FAULT_REG_BCS))
But in general,
Reviewed-by: Matt Roper matthew.d.roper@intel.com
#define GEN8_RING_FAULT_REG _MMIO(0x4094) #define GEN12_RING_FAULT_REG _MMIO(0xcec4)
#define GEN8_RING_FAULT_ENGINE_ID(x) (((x) >> 12) & 0x7)
2.31.1
dri-devel@lists.freedesktop.org
-
Lucas De Marchi -
Lucas De Marchi -
Matt Roper -
Tvrtko Ursulin