* [PATCH] drm/i915: Split GAM and MSLICE steering
@ 2022-09-16 1:43 Matt Roper
2022-09-16 9:02 ` [Intel-gfx] " Tvrtko Ursulin
0 siblings, 1 reply; 6+ messages in thread
From: Matt Roper @ 2022-09-16 1:43 UTC (permalink / raw)
To: intel-gfx; +Cc: dri-devel
Although the bspec lists several MMIO ranges as "MSLICE," it turns out
that a subset of these are of a "GAM" subclass that has unique rules and
doesn't followed regular mslice steering behavior.
* Xe_HP SDV: GAM ranges must always be steered to 0,0. These
registers share the regular steering control register (0xFDC) with
other steering types
* DG2: GAM ranges must always be steered to 1,0. GAM registers have a
dedicated steering control register (0xFE0) so we can set the value
once at startup and rely on implicit steering. Technically the
hardware default should already be set to 1,0 properly, but it never
hurts to ensure that in the driver.
Bspec: 66534
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 24 +++++++++++++++++++--
drivers/gpu/drm/i915/gt/intel_gt_regs.h | 1 +
drivers/gpu/drm/i915/gt/intel_gt_types.h | 1 +
drivers/gpu/drm/i915/gt/intel_workarounds.c | 10 +++++++++
4 files changed, 34 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
index e79405a45312..a2047a68ea7a 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
@@ -40,6 +40,7 @@ static const char * const intel_steering_types[] = {
"L3BANK",
"MSLICE",
"LNCF",
+ "GAM",
"INSTANCE 0",
};
@@ -48,14 +49,23 @@ static const struct intel_mmio_range icl_l3bank_steering_table[] = {
{},
};
+/*
+ * Although the bspec lists more "MSLICE" ranges than shown here, some of those
+ * are of a "GAM" subclass that has special rules. Thus we use a separate
+ * GAM table farther down for those.
+ */
static const struct intel_mmio_range xehpsdv_mslice_steering_table[] = {
- { 0x004000, 0x004AFF },
- { 0x00C800, 0x00CFFF },
{ 0x00DD00, 0x00DDFF },
{ 0x00E900, 0x00FFFF }, /* 0xEA00 - OxEFFF is unused */
{},
};
+static const struct intel_mmio_range xehpsdv_gam_steering_table[] = {
+ { 0x004000, 0x004AFF },
+ { 0x00C800, 0x00CFFF },
+ {},
+};
+
static const struct intel_mmio_range xehpsdv_lncf_steering_table[] = {
{ 0x00B000, 0x00B0FF },
{ 0x00D800, 0x00D8FF },
@@ -114,9 +124,15 @@ void intel_gt_mcr_init(struct intel_gt *gt)
} else if (IS_DG2(i915)) {
gt->steering_table[MSLICE] = xehpsdv_mslice_steering_table;
gt->steering_table[LNCF] = dg2_lncf_steering_table;
+ /*
+ * No need to hook up the GAM table since it has a dedicated
+ * steering control register on DG2 and can use implicit
+ * steering.
+ */
} else if (IS_XEHPSDV(i915)) {
gt->steering_table[MSLICE] = xehpsdv_mslice_steering_table;
gt->steering_table[LNCF] = xehpsdv_lncf_steering_table;
+ gt->steering_table[GAM] = xehpsdv_gam_steering_table;
} else if (GRAPHICS_VER(i915) >= 11 &&
GRAPHICS_VER_FULL(i915) < IP_VER(12, 50)) {
gt->steering_table[L3BANK] = icl_l3bank_steering_table;
@@ -351,6 +367,10 @@ static void get_nonterminated_steering(struct intel_gt *gt,
*group = __ffs(gt->info.mslice_mask) << 1;
*instance = 0; /* unused */
break;
+ case GAM:
+ *group = IS_DG2(gt->i915) ? 1 : 0;
+ *instance = 0;
+ break;
case INSTANCE0:
/*
* There are a lot of MCR types for which instance (0, 0)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
index 2275ee47da95..2343b26e0e21 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
@@ -42,6 +42,7 @@
#define MCFG_MCR_SELECTOR _MMIO(0xfd0)
#define SF_MCR_SELECTOR _MMIO(0xfd8)
#define GEN8_MCR_SELECTOR _MMIO(0xfdc)
+#define GAM_MCR_SELECTOR _MMIO(0xfe0)
#define GEN8_MCR_SLICE(slice) (((slice) & 3) << 26)
#define GEN8_MCR_SLICE_MASK GEN8_MCR_SLICE(3)
#define GEN8_MCR_SUBSLICE(subslice) (((subslice) & 3) << 24)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
index f19c2de77ff6..30003d68fd51 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
@@ -59,6 +59,7 @@ enum intel_steering_type {
L3BANK,
MSLICE,
LNCF,
+ GAM,
/*
* On some platforms there are multiple types of MCR registers that
diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index 6d2003d598e6..d04652a3b4e5 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -1181,6 +1181,9 @@ xehp_init_mcr(struct intel_gt *gt, struct i915_wa_list *wal)
gt->steering_table[MSLICE] = NULL;
}
+ if (IS_XEHPSDV(gt->i915) && slice_mask & BIT(0))
+ gt->steering_table[GAM] = NULL;
+
slice = __ffs(slice_mask);
subslice = intel_sseu_find_first_xehp_dss(sseu, GEN_DSS_PER_GSLICE, slice) %
GEN_DSS_PER_GSLICE;
@@ -1198,6 +1201,13 @@ xehp_init_mcr(struct intel_gt *gt, struct i915_wa_list *wal)
*/
__set_mcr_steering(wal, MCFG_MCR_SELECTOR, 0, 2);
__set_mcr_steering(wal, SF_MCR_SELECTOR, 0, 2);
+
+ /*
+ * On DG2, GAM registers have a dedicated steering control register
+ * and must always be programmed to a hardcoded groupid of "1."
+ */
+ if (IS_DG2(gt->i915))
+ __set_mcr_steering(wal, GAM_MCR_SELECTOR, 1, 0);
}
static void
--
2.37.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Split GAM and MSLICE steering
2022-09-16 1:43 [PATCH] drm/i915: Split GAM and MSLICE steering Matt Roper
@ 2022-09-16 9:02 ` Tvrtko Ursulin
2022-09-16 14:53 ` Matt Roper
0 siblings, 1 reply; 6+ messages in thread
From: Tvrtko Ursulin @ 2022-09-16 9:02 UTC (permalink / raw)
To: Matt Roper, intel-gfx; +Cc: dri-devel
On 16/09/2022 02:43, Matt Roper wrote:
> Although the bspec lists several MMIO ranges as "MSLICE," it turns out
> that a subset of these are of a "GAM" subclass that has unique rules and
> doesn't followed regular mslice steering behavior.
>
> * Xe_HP SDV: GAM ranges must always be steered to 0,0. These
> registers share the regular steering control register (0xFDC) with
> other steering types
>
> * DG2: GAM ranges must always be steered to 1,0. GAM registers have a
> dedicated steering control register (0xFE0) so we can set the value
> once at startup and rely on implicit steering. Technically the
> hardware default should already be set to 1,0 properly, but it never
> hurts to ensure that in the driver.
Do you have any data on whether the "technically should" holds in
practice? What would be the consequences of some platform/machine
surprising us here?
Regards,
Tvrtko
>
> Bspec: 66534
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
> drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 24 +++++++++++++++++++--
> drivers/gpu/drm/i915/gt/intel_gt_regs.h | 1 +
> drivers/gpu/drm/i915/gt/intel_gt_types.h | 1 +
> drivers/gpu/drm/i915/gt/intel_workarounds.c | 10 +++++++++
> 4 files changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> index e79405a45312..a2047a68ea7a 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> @@ -40,6 +40,7 @@ static const char * const intel_steering_types[] = {
> "L3BANK",
> "MSLICE",
> "LNCF",
> + "GAM",
> "INSTANCE 0",
> };
>
> @@ -48,14 +49,23 @@ static const struct intel_mmio_range icl_l3bank_steering_table[] = {
> {},
> };
>
> +/*
> + * Although the bspec lists more "MSLICE" ranges than shown here, some of those
> + * are of a "GAM" subclass that has special rules. Thus we use a separate
> + * GAM table farther down for those.
> + */
> static const struct intel_mmio_range xehpsdv_mslice_steering_table[] = {
> - { 0x004000, 0x004AFF },
> - { 0x00C800, 0x00CFFF },
> { 0x00DD00, 0x00DDFF },
> { 0x00E900, 0x00FFFF }, /* 0xEA00 - OxEFFF is unused */
> {},
> };
>
> +static const struct intel_mmio_range xehpsdv_gam_steering_table[] = {
> + { 0x004000, 0x004AFF },
> + { 0x00C800, 0x00CFFF },
> + {},
> +};
> +
> static const struct intel_mmio_range xehpsdv_lncf_steering_table[] = {
> { 0x00B000, 0x00B0FF },
> { 0x00D800, 0x00D8FF },
> @@ -114,9 +124,15 @@ void intel_gt_mcr_init(struct intel_gt *gt)
> } else if (IS_DG2(i915)) {
> gt->steering_table[MSLICE] = xehpsdv_mslice_steering_table;
> gt->steering_table[LNCF] = dg2_lncf_steering_table;
> + /*
> + * No need to hook up the GAM table since it has a dedicated
> + * steering control register on DG2 and can use implicit
> + * steering.
> + */
> } else if (IS_XEHPSDV(i915)) {
> gt->steering_table[MSLICE] = xehpsdv_mslice_steering_table;
> gt->steering_table[LNCF] = xehpsdv_lncf_steering_table;
> + gt->steering_table[GAM] = xehpsdv_gam_steering_table;
> } else if (GRAPHICS_VER(i915) >= 11 &&
> GRAPHICS_VER_FULL(i915) < IP_VER(12, 50)) {
> gt->steering_table[L3BANK] = icl_l3bank_steering_table;
> @@ -351,6 +367,10 @@ static void get_nonterminated_steering(struct intel_gt *gt,
> *group = __ffs(gt->info.mslice_mask) << 1;
> *instance = 0; /* unused */
> break;
> + case GAM:
> + *group = IS_DG2(gt->i915) ? 1 : 0;
> + *instance = 0;
> + break;
> case INSTANCE0:
> /*
> * There are a lot of MCR types for which instance (0, 0)
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> index 2275ee47da95..2343b26e0e21 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> @@ -42,6 +42,7 @@
> #define MCFG_MCR_SELECTOR _MMIO(0xfd0)
> #define SF_MCR_SELECTOR _MMIO(0xfd8)
> #define GEN8_MCR_SELECTOR _MMIO(0xfdc)
> +#define GAM_MCR_SELECTOR _MMIO(0xfe0)
> #define GEN8_MCR_SLICE(slice) (((slice) & 3) << 26)
> #define GEN8_MCR_SLICE_MASK GEN8_MCR_SLICE(3)
> #define GEN8_MCR_SUBSLICE(subslice) (((subslice) & 3) << 24)
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> index f19c2de77ff6..30003d68fd51 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> @@ -59,6 +59,7 @@ enum intel_steering_type {
> L3BANK,
> MSLICE,
> LNCF,
> + GAM,
>
> /*
> * On some platforms there are multiple types of MCR registers that
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 6d2003d598e6..d04652a3b4e5 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -1181,6 +1181,9 @@ xehp_init_mcr(struct intel_gt *gt, struct i915_wa_list *wal)
> gt->steering_table[MSLICE] = NULL;
> }
>
> + if (IS_XEHPSDV(gt->i915) && slice_mask & BIT(0))
> + gt->steering_table[GAM] = NULL;
> +
> slice = __ffs(slice_mask);
> subslice = intel_sseu_find_first_xehp_dss(sseu, GEN_DSS_PER_GSLICE, slice) %
> GEN_DSS_PER_GSLICE;
> @@ -1198,6 +1201,13 @@ xehp_init_mcr(struct intel_gt *gt, struct i915_wa_list *wal)
> */
> __set_mcr_steering(wal, MCFG_MCR_SELECTOR, 0, 2);
> __set_mcr_steering(wal, SF_MCR_SELECTOR, 0, 2);
> +
> + /*
> + * On DG2, GAM registers have a dedicated steering control register
> + * and must always be programmed to a hardcoded groupid of "1."
> + */
> + if (IS_DG2(gt->i915))
> + __set_mcr_steering(wal, GAM_MCR_SELECTOR, 1, 0);
> }
>
> static void
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Split GAM and MSLICE steering
2022-09-16 9:02 ` [Intel-gfx] " Tvrtko Ursulin
@ 2022-09-16 14:53 ` Matt Roper
2022-09-21 16:58 ` Kumar Valsan, Prathap
0 siblings, 1 reply; 6+ messages in thread
From: Matt Roper @ 2022-09-16 14:53 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx, dri-devel
On Fri, Sep 16, 2022 at 10:02:32AM +0100, Tvrtko Ursulin wrote:
>
> On 16/09/2022 02:43, Matt Roper wrote:
> > Although the bspec lists several MMIO ranges as "MSLICE," it turns out
> > that a subset of these are of a "GAM" subclass that has unique rules and
> > doesn't followed regular mslice steering behavior.
> >
> > * Xe_HP SDV: GAM ranges must always be steered to 0,0. These
> > registers share the regular steering control register (0xFDC) with
> > other steering types
> >
> > * DG2: GAM ranges must always be steered to 1,0. GAM registers have a
> > dedicated steering control register (0xFE0) so we can set the value
> > once at startup and rely on implicit steering. Technically the
> > hardware default should already be set to 1,0 properly, but it never
> > hurts to ensure that in the driver.
>
> Do you have any data on whether the "technically should" holds in practice?
> What would be the consequences of some platform/machine surprising us here?
The bspec indicates the hardware default value is already the necessary
1,0 value; I'm mostly paranoid about some kind of boot firmware wiping
it to 0,0 by accident. I don't have any evidence that has ever actually
happened, but explicitly re-programming it to 1,0 in the patch here is a
defensive measure just to be safe.
If we didn't have this patch _and_ some firmware screwed up the GAM
steering target, then presumably we might read back garbage or 0 from
GAM registers in places where we should have received a real value.
Matt
>
> Regards,
>
> Tvrtko
>
> >
> > Bspec: 66534
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> > drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 24 +++++++++++++++++++--
> > drivers/gpu/drm/i915/gt/intel_gt_regs.h | 1 +
> > drivers/gpu/drm/i915/gt/intel_gt_types.h | 1 +
> > drivers/gpu/drm/i915/gt/intel_workarounds.c | 10 +++++++++
> > 4 files changed, 34 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> > index e79405a45312..a2047a68ea7a 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> > @@ -40,6 +40,7 @@ static const char * const intel_steering_types[] = {
> > "L3BANK",
> > "MSLICE",
> > "LNCF",
> > + "GAM",
> > "INSTANCE 0",
> > };
> > @@ -48,14 +49,23 @@ static const struct intel_mmio_range icl_l3bank_steering_table[] = {
> > {},
> > };
> > +/*
> > + * Although the bspec lists more "MSLICE" ranges than shown here, some of those
> > + * are of a "GAM" subclass that has special rules. Thus we use a separate
> > + * GAM table farther down for those.
> > + */
> > static const struct intel_mmio_range xehpsdv_mslice_steering_table[] = {
> > - { 0x004000, 0x004AFF },
> > - { 0x00C800, 0x00CFFF },
> > { 0x00DD00, 0x00DDFF },
> > { 0x00E900, 0x00FFFF }, /* 0xEA00 - OxEFFF is unused */
> > {},
> > };
> > +static const struct intel_mmio_range xehpsdv_gam_steering_table[] = {
> > + { 0x004000, 0x004AFF },
> > + { 0x00C800, 0x00CFFF },
> > + {},
> > +};
> > +
> > static const struct intel_mmio_range xehpsdv_lncf_steering_table[] = {
> > { 0x00B000, 0x00B0FF },
> > { 0x00D800, 0x00D8FF },
> > @@ -114,9 +124,15 @@ void intel_gt_mcr_init(struct intel_gt *gt)
> > } else if (IS_DG2(i915)) {
> > gt->steering_table[MSLICE] = xehpsdv_mslice_steering_table;
> > gt->steering_table[LNCF] = dg2_lncf_steering_table;
> > + /*
> > + * No need to hook up the GAM table since it has a dedicated
> > + * steering control register on DG2 and can use implicit
> > + * steering.
> > + */
> > } else if (IS_XEHPSDV(i915)) {
> > gt->steering_table[MSLICE] = xehpsdv_mslice_steering_table;
> > gt->steering_table[LNCF] = xehpsdv_lncf_steering_table;
> > + gt->steering_table[GAM] = xehpsdv_gam_steering_table;
> > } else if (GRAPHICS_VER(i915) >= 11 &&
> > GRAPHICS_VER_FULL(i915) < IP_VER(12, 50)) {
> > gt->steering_table[L3BANK] = icl_l3bank_steering_table;
> > @@ -351,6 +367,10 @@ static void get_nonterminated_steering(struct intel_gt *gt,
> > *group = __ffs(gt->info.mslice_mask) << 1;
> > *instance = 0; /* unused */
> > break;
> > + case GAM:
> > + *group = IS_DG2(gt->i915) ? 1 : 0;
> > + *instance = 0;
> > + break;
> > case INSTANCE0:
> > /*
> > * There are a lot of MCR types for which instance (0, 0)
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > index 2275ee47da95..2343b26e0e21 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > @@ -42,6 +42,7 @@
> > #define MCFG_MCR_SELECTOR _MMIO(0xfd0)
> > #define SF_MCR_SELECTOR _MMIO(0xfd8)
> > #define GEN8_MCR_SELECTOR _MMIO(0xfdc)
> > +#define GAM_MCR_SELECTOR _MMIO(0xfe0)
> > #define GEN8_MCR_SLICE(slice) (((slice) & 3) << 26)
> > #define GEN8_MCR_SLICE_MASK GEN8_MCR_SLICE(3)
> > #define GEN8_MCR_SUBSLICE(subslice) (((subslice) & 3) << 24)
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > index f19c2de77ff6..30003d68fd51 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > @@ -59,6 +59,7 @@ enum intel_steering_type {
> > L3BANK,
> > MSLICE,
> > LNCF,
> > + GAM,
> > /*
> > * On some platforms there are multiple types of MCR registers that
> > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > index 6d2003d598e6..d04652a3b4e5 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > @@ -1181,6 +1181,9 @@ xehp_init_mcr(struct intel_gt *gt, struct i915_wa_list *wal)
> > gt->steering_table[MSLICE] = NULL;
> > }
> > + if (IS_XEHPSDV(gt->i915) && slice_mask & BIT(0))
> > + gt->steering_table[GAM] = NULL;
> > +
> > slice = __ffs(slice_mask);
> > subslice = intel_sseu_find_first_xehp_dss(sseu, GEN_DSS_PER_GSLICE, slice) %
> > GEN_DSS_PER_GSLICE;
> > @@ -1198,6 +1201,13 @@ xehp_init_mcr(struct intel_gt *gt, struct i915_wa_list *wal)
> > */
> > __set_mcr_steering(wal, MCFG_MCR_SELECTOR, 0, 2);
> > __set_mcr_steering(wal, SF_MCR_SELECTOR, 0, 2);
> > +
> > + /*
> > + * On DG2, GAM registers have a dedicated steering control register
> > + * and must always be programmed to a hardcoded groupid of "1."
> > + */
> > + if (IS_DG2(gt->i915))
> > + __set_mcr_steering(wal, GAM_MCR_SELECTOR, 1, 0);
> > }
> > static void
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Split GAM and MSLICE steering
2022-09-16 14:53 ` Matt Roper
@ 2022-09-21 16:58 ` Kumar Valsan, Prathap
2022-09-21 19:26 ` Matt Roper
0 siblings, 1 reply; 6+ messages in thread
From: Kumar Valsan, Prathap @ 2022-09-21 16:58 UTC (permalink / raw)
To: Matt Roper; +Cc: Tvrtko Ursulin, intel-gfx, dri-devel
On Fri, Sep 16, 2022 at 07:53:40AM -0700, Matt Roper wrote:
> On Fri, Sep 16, 2022 at 10:02:32AM +0100, Tvrtko Ursulin wrote:
> >
> > On 16/09/2022 02:43, Matt Roper wrote:
> > > Although the bspec lists several MMIO ranges as "MSLICE," it turns out
> > > that a subset of these are of a "GAM" subclass that has unique rules and
> > > doesn't followed regular mslice steering behavior.
> > >
> > > * Xe_HP SDV: GAM ranges must always be steered to 0,0. These
> > > registers share the regular steering control register (0xFDC) with
> > > other steering types
> > >
> > > * DG2: GAM ranges must always be steered to 1,0. GAM registers have a
> > > dedicated steering control register (0xFE0) so we can set the value
> > > once at startup and rely on implicit steering. Technically the
> > > hardware default should already be set to 1,0 properly, but it never
> > > hurts to ensure that in the driver.
> >
> > Do you have any data on whether the "technically should" holds in practice?
> > What would be the consequences of some platform/machine surprising us here?
>
> The bspec indicates the hardware default value is already the necessary
> 1,0 value; I'm mostly paranoid about some kind of boot firmware wiping
> it to 0,0 by accident. I don't have any evidence that has ever actually
> happened, but explicitly re-programming it to 1,0 in the patch here is a
> defensive measure just to be safe.
>
> If we didn't have this patch _and_ some firmware screwed up the GAM
> steering target, then presumably we might read back garbage or 0 from
> GAM registers in places where we should have received a real value.
Will firmware ever touch the steering target registers. As i was going
through the respective hsd. The software driver impact is marked as none
so wondering if this change is really required ?
Thanks,
Prathap
>
>
> Matt
>
> >
> > Regards,
> >
> > Tvrtko
> >
> > >
> > > Bspec: 66534
> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 24 +++++++++++++++++++--
> > > drivers/gpu/drm/i915/gt/intel_gt_regs.h | 1 +
> > > drivers/gpu/drm/i915/gt/intel_gt_types.h | 1 +
> > > drivers/gpu/drm/i915/gt/intel_workarounds.c | 10 +++++++++
> > > 4 files changed, 34 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> > > index e79405a45312..a2047a68ea7a 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> > > @@ -40,6 +40,7 @@ static const char * const intel_steering_types[] = {
> > > "L3BANK",
> > > "MSLICE",
> > > "LNCF",
> > > + "GAM",
> > > "INSTANCE 0",
> > > };
> > > @@ -48,14 +49,23 @@ static const struct intel_mmio_range icl_l3bank_steering_table[] = {
> > > {},
> > > };
> > > +/*
> > > + * Although the bspec lists more "MSLICE" ranges than shown here, some of those
> > > + * are of a "GAM" subclass that has special rules. Thus we use a separate
> > > + * GAM table farther down for those.
> > > + */
> > > static const struct intel_mmio_range xehpsdv_mslice_steering_table[] = {
> > > - { 0x004000, 0x004AFF },
> > > - { 0x00C800, 0x00CFFF },
> > > { 0x00DD00, 0x00DDFF },
> > > { 0x00E900, 0x00FFFF }, /* 0xEA00 - OxEFFF is unused */
> > > {},
> > > };
> > > +static const struct intel_mmio_range xehpsdv_gam_steering_table[] = {
> > > + { 0x004000, 0x004AFF },
> > > + { 0x00C800, 0x00CFFF },
> > > + {},
> > > +};
> > > +
> > > static const struct intel_mmio_range xehpsdv_lncf_steering_table[] = {
> > > { 0x00B000, 0x00B0FF },
> > > { 0x00D800, 0x00D8FF },
> > > @@ -114,9 +124,15 @@ void intel_gt_mcr_init(struct intel_gt *gt)
> > > } else if (IS_DG2(i915)) {
> > > gt->steering_table[MSLICE] = xehpsdv_mslice_steering_table;
> > > gt->steering_table[LNCF] = dg2_lncf_steering_table;
> > > + /*
> > > + * No need to hook up the GAM table since it has a dedicated
> > > + * steering control register on DG2 and can use implicit
> > > + * steering.
> > > + */
> > > } else if (IS_XEHPSDV(i915)) {
> > > gt->steering_table[MSLICE] = xehpsdv_mslice_steering_table;
> > > gt->steering_table[LNCF] = xehpsdv_lncf_steering_table;
> > > + gt->steering_table[GAM] = xehpsdv_gam_steering_table;
> > > } else if (GRAPHICS_VER(i915) >= 11 &&
> > > GRAPHICS_VER_FULL(i915) < IP_VER(12, 50)) {
> > > gt->steering_table[L3BANK] = icl_l3bank_steering_table;
> > > @@ -351,6 +367,10 @@ static void get_nonterminated_steering(struct intel_gt *gt,
> > > *group = __ffs(gt->info.mslice_mask) << 1;
> > > *instance = 0; /* unused */
> > > break;
> > > + case GAM:
> > > + *group = IS_DG2(gt->i915) ? 1 : 0;
> > > + *instance = 0;
> > > + break;
> > > case INSTANCE0:
> > > /*
> > > * There are a lot of MCR types for which instance (0, 0)
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > index 2275ee47da95..2343b26e0e21 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > @@ -42,6 +42,7 @@
> > > #define MCFG_MCR_SELECTOR _MMIO(0xfd0)
> > > #define SF_MCR_SELECTOR _MMIO(0xfd8)
> > > #define GEN8_MCR_SELECTOR _MMIO(0xfdc)
> > > +#define GAM_MCR_SELECTOR _MMIO(0xfe0)
> > > #define GEN8_MCR_SLICE(slice) (((slice) & 3) << 26)
> > > #define GEN8_MCR_SLICE_MASK GEN8_MCR_SLICE(3)
> > > #define GEN8_MCR_SUBSLICE(subslice) (((subslice) & 3) << 24)
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > > index f19c2de77ff6..30003d68fd51 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > > @@ -59,6 +59,7 @@ enum intel_steering_type {
> > > L3BANK,
> > > MSLICE,
> > > LNCF,
> > > + GAM,
> > > /*
> > > * On some platforms there are multiple types of MCR registers that
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > index 6d2003d598e6..d04652a3b4e5 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > @@ -1181,6 +1181,9 @@ xehp_init_mcr(struct intel_gt *gt, struct i915_wa_list *wal)
> > > gt->steering_table[MSLICE] = NULL;
> > > }
> > > + if (IS_XEHPSDV(gt->i915) && slice_mask & BIT(0))
> > > + gt->steering_table[GAM] = NULL;
> > > +
> > > slice = __ffs(slice_mask);
> > > subslice = intel_sseu_find_first_xehp_dss(sseu, GEN_DSS_PER_GSLICE, slice) %
> > > GEN_DSS_PER_GSLICE;
> > > @@ -1198,6 +1201,13 @@ xehp_init_mcr(struct intel_gt *gt, struct i915_wa_list *wal)
> > > */
> > > __set_mcr_steering(wal, MCFG_MCR_SELECTOR, 0, 2);
> > > __set_mcr_steering(wal, SF_MCR_SELECTOR, 0, 2);
> > > +
> > > + /*
> > > + * On DG2, GAM registers have a dedicated steering control register
> > > + * and must always be programmed to a hardcoded groupid of "1."
> > > + */
> > > + if (IS_DG2(gt->i915))
> > > + __set_mcr_steering(wal, GAM_MCR_SELECTOR, 1, 0);
> > > }
> > > static void
>
> --
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Split GAM and MSLICE steering
2022-09-21 16:58 ` Kumar Valsan, Prathap
@ 2022-09-21 19:26 ` Matt Roper
2022-09-21 19:46 ` Kumar Valsan, Prathap
0 siblings, 1 reply; 6+ messages in thread
From: Matt Roper @ 2022-09-21 19:26 UTC (permalink / raw)
To: Kumar Valsan, Prathap; +Cc: Tvrtko Ursulin, intel-gfx, dri-devel
On Wed, Sep 21, 2022 at 12:58:08PM -0400, Kumar Valsan, Prathap wrote:
> On Fri, Sep 16, 2022 at 07:53:40AM -0700, Matt Roper wrote:
> > On Fri, Sep 16, 2022 at 10:02:32AM +0100, Tvrtko Ursulin wrote:
> > >
> > > On 16/09/2022 02:43, Matt Roper wrote:
> > > > Although the bspec lists several MMIO ranges as "MSLICE," it turns out
> > > > that a subset of these are of a "GAM" subclass that has unique rules and
> > > > doesn't followed regular mslice steering behavior.
> > > >
> > > > * Xe_HP SDV: GAM ranges must always be steered to 0,0. These
> > > > registers share the regular steering control register (0xFDC) with
> > > > other steering types
> > > >
> > > > * DG2: GAM ranges must always be steered to 1,0. GAM registers have a
> > > > dedicated steering control register (0xFE0) so we can set the value
> > > > once at startup and rely on implicit steering. Technically the
> > > > hardware default should already be set to 1,0 properly, but it never
> > > > hurts to ensure that in the driver.
> > >
> > > Do you have any data on whether the "technically should" holds in practice?
> > > What would be the consequences of some platform/machine surprising us here?
> >
> > The bspec indicates the hardware default value is already the necessary
> > 1,0 value; I'm mostly paranoid about some kind of boot firmware wiping
> > it to 0,0 by accident. I don't have any evidence that has ever actually
> > happened, but explicitly re-programming it to 1,0 in the patch here is a
> > defensive measure just to be safe.
> >
> > If we didn't have this patch _and_ some firmware screwed up the GAM
> > steering target, then presumably we might read back garbage or 0 from
> > GAM registers in places where we should have received a real value.
> Will firmware ever touch the steering target registers. As i was going
> through the respective hsd. The software driver impact is marked as none
> so wondering if this change is really required ?
The GAM only has a dedicated steering register on DG2; on XEHPSDV it
shares 0xFDC with all the other kinds of steering, so it is important to
handle this range independently of the MSLICE range and make sure we
properly re-steer GAM accesses to the primary instance (and not just any
random MSLICE) there.
On DG2, if we assume firmware behaves properly, the dedicated steering
register is initialized properly and we don't need to explicitly
re-steer. However this patch will ensure that we don't needlessly
re-program 0xFDC according to MSLICE rules when accessing a GAM
register.
There's also the worry that firmware may try to "sanitize" the registers
at startup by programming them to what it thinks are appropriate default
values. Given that DG2's primary GAM is unusual (instance 1, instead of
instance 0 as on other platforms), this feels like a place where
firmware bugs could creep in. They hopefully/probably won't, but
ensuring we forcefully initialize 0xFE0 to the proper value just ensures
that we don't even have to worry about it.
Finally, splitting the GAM from MSLICE ensures we get more accurate
debug messages from the drm_printer in dmesg and debugfs.
Matt
>
> Thanks,
> Prathap
> >
> >
> > Matt
> >
> > >
> > > Regards,
> > >
> > > Tvrtko
> > >
> > > >
> > > > Bspec: 66534
> > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 24 +++++++++++++++++++--
> > > > drivers/gpu/drm/i915/gt/intel_gt_regs.h | 1 +
> > > > drivers/gpu/drm/i915/gt/intel_gt_types.h | 1 +
> > > > drivers/gpu/drm/i915/gt/intel_workarounds.c | 10 +++++++++
> > > > 4 files changed, 34 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> > > > index e79405a45312..a2047a68ea7a 100644
> > > > --- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> > > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> > > > @@ -40,6 +40,7 @@ static const char * const intel_steering_types[] = {
> > > > "L3BANK",
> > > > "MSLICE",
> > > > "LNCF",
> > > > + "GAM",
> > > > "INSTANCE 0",
> > > > };
> > > > @@ -48,14 +49,23 @@ static const struct intel_mmio_range icl_l3bank_steering_table[] = {
> > > > {},
> > > > };
> > > > +/*
> > > > + * Although the bspec lists more "MSLICE" ranges than shown here, some of those
> > > > + * are of a "GAM" subclass that has special rules. Thus we use a separate
> > > > + * GAM table farther down for those.
> > > > + */
> > > > static const struct intel_mmio_range xehpsdv_mslice_steering_table[] = {
> > > > - { 0x004000, 0x004AFF },
> > > > - { 0x00C800, 0x00CFFF },
> > > > { 0x00DD00, 0x00DDFF },
> > > > { 0x00E900, 0x00FFFF }, /* 0xEA00 - OxEFFF is unused */
> > > > {},
> > > > };
> > > > +static const struct intel_mmio_range xehpsdv_gam_steering_table[] = {
> > > > + { 0x004000, 0x004AFF },
> > > > + { 0x00C800, 0x00CFFF },
> > > > + {},
> > > > +};
> > > > +
> > > > static const struct intel_mmio_range xehpsdv_lncf_steering_table[] = {
> > > > { 0x00B000, 0x00B0FF },
> > > > { 0x00D800, 0x00D8FF },
> > > > @@ -114,9 +124,15 @@ void intel_gt_mcr_init(struct intel_gt *gt)
> > > > } else if (IS_DG2(i915)) {
> > > > gt->steering_table[MSLICE] = xehpsdv_mslice_steering_table;
> > > > gt->steering_table[LNCF] = dg2_lncf_steering_table;
> > > > + /*
> > > > + * No need to hook up the GAM table since it has a dedicated
> > > > + * steering control register on DG2 and can use implicit
> > > > + * steering.
> > > > + */
> > > > } else if (IS_XEHPSDV(i915)) {
> > > > gt->steering_table[MSLICE] = xehpsdv_mslice_steering_table;
> > > > gt->steering_table[LNCF] = xehpsdv_lncf_steering_table;
> > > > + gt->steering_table[GAM] = xehpsdv_gam_steering_table;
> > > > } else if (GRAPHICS_VER(i915) >= 11 &&
> > > > GRAPHICS_VER_FULL(i915) < IP_VER(12, 50)) {
> > > > gt->steering_table[L3BANK] = icl_l3bank_steering_table;
> > > > @@ -351,6 +367,10 @@ static void get_nonterminated_steering(struct intel_gt *gt,
> > > > *group = __ffs(gt->info.mslice_mask) << 1;
> > > > *instance = 0; /* unused */
> > > > break;
> > > > + case GAM:
> > > > + *group = IS_DG2(gt->i915) ? 1 : 0;
> > > > + *instance = 0;
> > > > + break;
> > > > case INSTANCE0:
> > > > /*
> > > > * There are a lot of MCR types for which instance (0, 0)
> > > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > > index 2275ee47da95..2343b26e0e21 100644
> > > > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > > @@ -42,6 +42,7 @@
> > > > #define MCFG_MCR_SELECTOR _MMIO(0xfd0)
> > > > #define SF_MCR_SELECTOR _MMIO(0xfd8)
> > > > #define GEN8_MCR_SELECTOR _MMIO(0xfdc)
> > > > +#define GAM_MCR_SELECTOR _MMIO(0xfe0)
> > > > #define GEN8_MCR_SLICE(slice) (((slice) & 3) << 26)
> > > > #define GEN8_MCR_SLICE_MASK GEN8_MCR_SLICE(3)
> > > > #define GEN8_MCR_SUBSLICE(subslice) (((subslice) & 3) << 24)
> > > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > > > index f19c2de77ff6..30003d68fd51 100644
> > > > --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > > > @@ -59,6 +59,7 @@ enum intel_steering_type {
> > > > L3BANK,
> > > > MSLICE,
> > > > LNCF,
> > > > + GAM,
> > > > /*
> > > > * On some platforms there are multiple types of MCR registers that
> > > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > > index 6d2003d598e6..d04652a3b4e5 100644
> > > > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > > @@ -1181,6 +1181,9 @@ xehp_init_mcr(struct intel_gt *gt, struct i915_wa_list *wal)
> > > > gt->steering_table[MSLICE] = NULL;
> > > > }
> > > > + if (IS_XEHPSDV(gt->i915) && slice_mask & BIT(0))
> > > > + gt->steering_table[GAM] = NULL;
> > > > +
> > > > slice = __ffs(slice_mask);
> > > > subslice = intel_sseu_find_first_xehp_dss(sseu, GEN_DSS_PER_GSLICE, slice) %
> > > > GEN_DSS_PER_GSLICE;
> > > > @@ -1198,6 +1201,13 @@ xehp_init_mcr(struct intel_gt *gt, struct i915_wa_list *wal)
> > > > */
> > > > __set_mcr_steering(wal, MCFG_MCR_SELECTOR, 0, 2);
> > > > __set_mcr_steering(wal, SF_MCR_SELECTOR, 0, 2);
> > > > +
> > > > + /*
> > > > + * On DG2, GAM registers have a dedicated steering control register
> > > > + * and must always be programmed to a hardcoded groupid of "1."
> > > > + */
> > > > + if (IS_DG2(gt->i915))
> > > > + __set_mcr_steering(wal, GAM_MCR_SELECTOR, 1, 0);
> > > > }
> > > > static void
> >
> > --
> > Matt Roper
> > Graphics Software Engineer
> > VTT-OSGC Platform Enablement
> > Intel Corporation
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Split GAM and MSLICE steering
2022-09-21 19:26 ` Matt Roper
@ 2022-09-21 19:46 ` Kumar Valsan, Prathap
0 siblings, 0 replies; 6+ messages in thread
From: Kumar Valsan, Prathap @ 2022-09-21 19:46 UTC (permalink / raw)
To: Matt Roper; +Cc: Tvrtko Ursulin, intel-gfx, dri-devel
On Wed, Sep 21, 2022 at 12:26:17PM -0700, Matt Roper wrote:
> On Wed, Sep 21, 2022 at 12:58:08PM -0400, Kumar Valsan, Prathap wrote:
> > On Fri, Sep 16, 2022 at 07:53:40AM -0700, Matt Roper wrote:
> > > On Fri, Sep 16, 2022 at 10:02:32AM +0100, Tvrtko Ursulin wrote:
> > > >
> > > > On 16/09/2022 02:43, Matt Roper wrote:
> > > > > Although the bspec lists several MMIO ranges as "MSLICE," it turns out
> > > > > that a subset of these are of a "GAM" subclass that has unique rules and
> > > > > doesn't followed regular mslice steering behavior.
> > > > >
> > > > > * Xe_HP SDV: GAM ranges must always be steered to 0,0. These
> > > > > registers share the regular steering control register (0xFDC) with
> > > > > other steering types
> > > > >
> > > > > * DG2: GAM ranges must always be steered to 1,0. GAM registers have a
> > > > > dedicated steering control register (0xFE0) so we can set the value
> > > > > once at startup and rely on implicit steering. Technically the
> > > > > hardware default should already be set to 1,0 properly, but it never
> > > > > hurts to ensure that in the driver.
> > > >
> > > > Do you have any data on whether the "technically should" holds in practice?
> > > > What would be the consequences of some platform/machine surprising us here?
> > >
> > > The bspec indicates the hardware default value is already the necessary
> > > 1,0 value; I'm mostly paranoid about some kind of boot firmware wiping
> > > it to 0,0 by accident. I don't have any evidence that has ever actually
> > > happened, but explicitly re-programming it to 1,0 in the patch here is a
> > > defensive measure just to be safe.
> > >
> > > If we didn't have this patch _and_ some firmware screwed up the GAM
> > > steering target, then presumably we might read back garbage or 0 from
> > > GAM registers in places where we should have received a real value.
> > Will firmware ever touch the steering target registers. As i was going
> > through the respective hsd. The software driver impact is marked as none
> > so wondering if this change is really required ?
>
> The GAM only has a dedicated steering register on DG2; on XEHPSDV it
> shares 0xFDC with all the other kinds of steering, so it is important to
> handle this range independently of the MSLICE range and make sure we
> properly re-steer GAM accesses to the primary instance (and not just any
> random MSLICE) there.
Ok. I missed that part.
>
> On DG2, if we assume firmware behaves properly, the dedicated steering
> register is initialized properly and we don't need to explicitly
> re-steer. However this patch will ensure that we don't needlessly
> re-program 0xFDC according to MSLICE rules when accessing a GAM
> register.
>
> There's also the worry that firmware may try to "sanitize" the registers
> at startup by programming them to what it thinks are appropriate default
> values. Given that DG2's primary GAM is unusual (instance 1, instead of
> instance 0 as on other platforms), this feels like a place where
> firmware bugs could creep in. They hopefully/probably won't, but
> ensuring we forcefully initialize 0xFE0 to the proper value just ensures
> that we don't even have to worry about it.
Got it.
>
> Finally, splitting the GAM from MSLICE ensures we get more accurate
> debug messages from the drm_printer in dmesg and debugfs.
>
Looks good to me.
Reviewed-by: Prathap Kumar Valsan <prathap.kumar.valsan@intel.com>
>
> Matt
>
> >
> > Thanks,
> > Prathap
> > >
> > >
> > > Matt
> > >
> > > >
> > > > Regards,
> > > >
> > > > Tvrtko
> > > >
> > > > >
> > > > > Bspec: 66534
> > > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > > > ---
> > > > > drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 24 +++++++++++++++++++--
> > > > > drivers/gpu/drm/i915/gt/intel_gt_regs.h | 1 +
> > > > > drivers/gpu/drm/i915/gt/intel_gt_types.h | 1 +
> > > > > drivers/gpu/drm/i915/gt/intel_workarounds.c | 10 +++++++++
> > > > > 4 files changed, 34 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> > > > > index e79405a45312..a2047a68ea7a 100644
> > > > > --- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> > > > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> > > > > @@ -40,6 +40,7 @@ static const char * const intel_steering_types[] = {
> > > > > "L3BANK",
> > > > > "MSLICE",
> > > > > "LNCF",
> > > > > + "GAM",
> > > > > "INSTANCE 0",
> > > > > };
> > > > > @@ -48,14 +49,23 @@ static const struct intel_mmio_range icl_l3bank_steering_table[] = {
> > > > > {},
> > > > > };
> > > > > +/*
> > > > > + * Although the bspec lists more "MSLICE" ranges than shown here, some of those
> > > > > + * are of a "GAM" subclass that has special rules. Thus we use a separate
> > > > > + * GAM table farther down for those.
> > > > > + */
> > > > > static const struct intel_mmio_range xehpsdv_mslice_steering_table[] = {
> > > > > - { 0x004000, 0x004AFF },
> > > > > - { 0x00C800, 0x00CFFF },
> > > > > { 0x00DD00, 0x00DDFF },
> > > > > { 0x00E900, 0x00FFFF }, /* 0xEA00 - OxEFFF is unused */
> > > > > {},
> > > > > };
> > > > > +static const struct intel_mmio_range xehpsdv_gam_steering_table[] = {
> > > > > + { 0x004000, 0x004AFF },
> > > > > + { 0x00C800, 0x00CFFF },
> > > > > + {},
> > > > > +};
> > > > > +
> > > > > static const struct intel_mmio_range xehpsdv_lncf_steering_table[] = {
> > > > > { 0x00B000, 0x00B0FF },
> > > > > { 0x00D800, 0x00D8FF },
> > > > > @@ -114,9 +124,15 @@ void intel_gt_mcr_init(struct intel_gt *gt)
> > > > > } else if (IS_DG2(i915)) {
> > > > > gt->steering_table[MSLICE] = xehpsdv_mslice_steering_table;
> > > > > gt->steering_table[LNCF] = dg2_lncf_steering_table;
> > > > > + /*
> > > > > + * No need to hook up the GAM table since it has a dedicated
> > > > > + * steering control register on DG2 and can use implicit
> > > > > + * steering.
> > > > > + */
> > > > > } else if (IS_XEHPSDV(i915)) {
> > > > > gt->steering_table[MSLICE] = xehpsdv_mslice_steering_table;
> > > > > gt->steering_table[LNCF] = xehpsdv_lncf_steering_table;
> > > > > + gt->steering_table[GAM] = xehpsdv_gam_steering_table;
> > > > > } else if (GRAPHICS_VER(i915) >= 11 &&
> > > > > GRAPHICS_VER_FULL(i915) < IP_VER(12, 50)) {
> > > > > gt->steering_table[L3BANK] = icl_l3bank_steering_table;
> > > > > @@ -351,6 +367,10 @@ static void get_nonterminated_steering(struct intel_gt *gt,
> > > > > *group = __ffs(gt->info.mslice_mask) << 1;
> > > > > *instance = 0; /* unused */
> > > > > break;
> > > > > + case GAM:
> > > > > + *group = IS_DG2(gt->i915) ? 1 : 0;
> > > > > + *instance = 0;
> > > > > + break;
> > > > > case INSTANCE0:
> > > > > /*
> > > > > * There are a lot of MCR types for which instance (0, 0)
> > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > > > index 2275ee47da95..2343b26e0e21 100644
> > > > > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > > > @@ -42,6 +42,7 @@
> > > > > #define MCFG_MCR_SELECTOR _MMIO(0xfd0)
> > > > > #define SF_MCR_SELECTOR _MMIO(0xfd8)
> > > > > #define GEN8_MCR_SELECTOR _MMIO(0xfdc)
> > > > > +#define GAM_MCR_SELECTOR _MMIO(0xfe0)
> > > > > #define GEN8_MCR_SLICE(slice) (((slice) & 3) << 26)
> > > > > #define GEN8_MCR_SLICE_MASK GEN8_MCR_SLICE(3)
> > > > > #define GEN8_MCR_SUBSLICE(subslice) (((subslice) & 3) << 24)
> > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > > > > index f19c2de77ff6..30003d68fd51 100644
> > > > > --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > > > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > > > > @@ -59,6 +59,7 @@ enum intel_steering_type {
> > > > > L3BANK,
> > > > > MSLICE,
> > > > > LNCF,
> > > > > + GAM,
> > > > > /*
> > > > > * On some platforms there are multiple types of MCR registers that
> > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > > > index 6d2003d598e6..d04652a3b4e5 100644
> > > > > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > > > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > > > @@ -1181,6 +1181,9 @@ xehp_init_mcr(struct intel_gt *gt, struct i915_wa_list *wal)
> > > > > gt->steering_table[MSLICE] = NULL;
> > > > > }
> > > > > + if (IS_XEHPSDV(gt->i915) && slice_mask & BIT(0))
> > > > > + gt->steering_table[GAM] = NULL;
> > > > > +
> > > > > slice = __ffs(slice_mask);
> > > > > subslice = intel_sseu_find_first_xehp_dss(sseu, GEN_DSS_PER_GSLICE, slice) %
> > > > > GEN_DSS_PER_GSLICE;
> > > > > @@ -1198,6 +1201,13 @@ xehp_init_mcr(struct intel_gt *gt, struct i915_wa_list *wal)
> > > > > */
> > > > > __set_mcr_steering(wal, MCFG_MCR_SELECTOR, 0, 2);
> > > > > __set_mcr_steering(wal, SF_MCR_SELECTOR, 0, 2);
> > > > > +
> > > > > + /*
> > > > > + * On DG2, GAM registers have a dedicated steering control register
> > > > > + * and must always be programmed to a hardcoded groupid of "1."
> > > > > + */
> > > > > + if (IS_DG2(gt->i915))
> > > > > + __set_mcr_steering(wal, GAM_MCR_SELECTOR, 1, 0);
> > > > > }
> > > > > static void
> > >
> > > --
> > > Matt Roper
> > > Graphics Software Engineer
> > > VTT-OSGC Platform Enablement
> > > Intel Corporation
>
> --
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-09-21 19:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-16 1:43 [PATCH] drm/i915: Split GAM and MSLICE steering Matt Roper
2022-09-16 9:02 ` [Intel-gfx] " Tvrtko Ursulin
2022-09-16 14:53 ` Matt Roper
2022-09-21 16:58 ` Kumar Valsan, Prathap
2022-09-21 19:26 ` Matt Roper
2022-09-21 19:46 ` Kumar Valsan, Prathap
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).