diff mbox series

[v2,14/14] drm/i915/mtl: Add multicast steering for media GT

Message ID 20221001004550.3031431-15-matthew.d.roper@intel.com (mailing list archive)
State New, archived
Headers show
Series Explicit MCR handling and MTL steering | expand

Commit Message

Matt Roper Oct. 1, 2022, 12:45 a.m. UTC 
MTL's media GT only has a single type of steering ("OAADDRM") which
selects between media slice 0 and media slice 1.  We'll always steer to
media slice 0 unless it is fused off (which is the case when VD0, VE0,
and SFC0 are all reported as unavailable).

Bspec: 67789
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gt_mcr.c      | 19 +++++++++++++++++--
 drivers/gpu/drm/i915/gt/intel_gt_types.h    |  1 +
 drivers/gpu/drm/i915/gt/intel_workarounds.c | 18 +++++++++++++++++-
 3 files changed, 35 insertions(+), 3 deletions(-)

Comments

Tvrtko Ursulin Oct. 3, 2022, 8:56 a.m. UTC | #1 
Hi Matt,

On 01/10/2022 01:45, Matt Roper wrote:
> MTL's media GT only has a single type of steering ("OAADDRM") which
> selects between media slice 0 and media slice 1.  We'll always steer to
> media slice 0 unless it is fused off (which is the case when VD0, VE0,
> and SFC0 are all reported as unavailable).
> 
> Bspec: 67789
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_gt_mcr.c      | 19 +++++++++++++++++--
>   drivers/gpu/drm/i915/gt/intel_gt_types.h    |  1 +
>   drivers/gpu/drm/i915/gt/intel_workarounds.c | 18 +++++++++++++++++-
>   3 files changed, 35 insertions(+), 3 deletions(-)

[snip]

> +static void
> +mtl_media_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list *wal)
> +{
> +	/*
> +	 * Unlike older platforms, we no longer setup implicit steering here;
> +	 * all MCR accesses are explicitly steered.
> +	 */
> +	if (drm_debug_enabled(DRM_UT_DRIVER)) {
> +		struct drm_printer p = drm_debug_printer("MCR Steering:");
> +
> +		intel_gt_mcr_report_steering(&p, gt, false);
> +	}
> +}
> +
>   static void
>   gt_init_workarounds(struct intel_gt *gt, struct i915_wa_list *wal)
>   {
>   	struct drm_i915_private *i915 = gt->i915;
>   
> -	if (IS_METEORLAKE(i915) && gt->type == GT_PRIMARY)
> +	if (IS_METEORLAKE(i915) && gt->type == GT_MEDIA)
> +		mtl_media_gt_workarounds_init(gt, wal);
> +	else if (IS_METEORLAKE(i915) && gt->type == GT_PRIMARY)
>   		mtl_3d_gt_workarounds_init(gt, wal);
>   	else if (IS_PONTEVECCHIO(i915))
>   		pvc_gt_workarounds_init(gt, wal);

Casually reading only - wouldn't it be nicer if the if-ladder in here 
(gt_init_workarounds) would have a single case per platform, and then 
you'd fork further (3d vs media) in MTL specific function?

Also, series ends up with mtl_media_gt_workarounds_init and 
mtl_3d_gt_workarounds_init apparently 100% identical. You will need two 
copies in the future?

Regards,

Tvrtko
Matt Roper Oct. 3, 2022, 7:32 p.m. UTC | #2 
On Mon, Oct 03, 2022 at 09:56:18AM +0100, Tvrtko Ursulin wrote:
> 
> Hi Matt,
> 
> On 01/10/2022 01:45, Matt Roper wrote:
> > MTL's media GT only has a single type of steering ("OAADDRM") which
> > selects between media slice 0 and media slice 1.  We'll always steer to
> > media slice 0 unless it is fused off (which is the case when VD0, VE0,
> > and SFC0 are all reported as unavailable).
> > 
> > Bspec: 67789
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_gt_mcr.c      | 19 +++++++++++++++++--
> >   drivers/gpu/drm/i915/gt/intel_gt_types.h    |  1 +
> >   drivers/gpu/drm/i915/gt/intel_workarounds.c | 18 +++++++++++++++++-
> >   3 files changed, 35 insertions(+), 3 deletions(-)
> 
> [snip]
> 
> > +static void
> > +mtl_media_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list *wal)
> > +{
> > +	/*
> > +	 * Unlike older platforms, we no longer setup implicit steering here;
> > +	 * all MCR accesses are explicitly steered.
> > +	 */
> > +	if (drm_debug_enabled(DRM_UT_DRIVER)) {
> > +		struct drm_printer p = drm_debug_printer("MCR Steering:");
> > +
> > +		intel_gt_mcr_report_steering(&p, gt, false);
> > +	}
> > +}
> > +
> >   static void
> >   gt_init_workarounds(struct intel_gt *gt, struct i915_wa_list *wal)
> >   {
> >   	struct drm_i915_private *i915 = gt->i915;
> > -	if (IS_METEORLAKE(i915) && gt->type == GT_PRIMARY)
> > +	if (IS_METEORLAKE(i915) && gt->type == GT_MEDIA)
> > +		mtl_media_gt_workarounds_init(gt, wal);
> > +	else if (IS_METEORLAKE(i915) && gt->type == GT_PRIMARY)
> >   		mtl_3d_gt_workarounds_init(gt, wal);
> >   	else if (IS_PONTEVECCHIO(i915))
> >   		pvc_gt_workarounds_init(gt, wal);
> 
> Casually reading only - wouldn't it be nicer if the if-ladder in here
> (gt_init_workarounds) would have a single case per platform, and then you'd
> fork further (3d vs media) in MTL specific function?

Actually, that reminds me that we probably need to change this in a
different direction --- starting with MTL, we should stop tying
workarounds to the platform (IS_METEORLAKE) but rather tie them to the
IP version (i.e., GRAPHICS_VER or MEDIA_VER) since in the future the
same chiplets can potentially be re-used on multiple platforms.
Conversely, you could also potentially have variants of the same
"platform" (e.g., MTL) that incorporate chiplets with different IP
versions (and thus need distinct lists of workarounds and such).

At the moment MTL is the only platform we have with the standalone media
design so there's no potential for mix-and-match of chiplets yet, and
IS_METEORLAKE works fine in the short term, but we do need to start
planning ahead and moving off of platform checks in areas of the driver
like this.

> 
> Also, series ends up with mtl_media_gt_workarounds_init and
> mtl_3d_gt_workarounds_init apparently 100% identical. You will need two
> copies in the future?

Yes, the two GTs are expected to end up with completely different sets
of workarounds once the platform is enabled.  We've just been delaying
on actually sending the new MTL workarounds upstream to give the
workaround database a bit more time to settle.


Matt

> 
> Regards,
> 
> Tvrtko
Tvrtko Ursulin Oct. 4, 2022, 10:13 a.m. UTC | #3 
On 03/10/2022 20:32, Matt Roper wrote:
> On Mon, Oct 03, 2022 at 09:56:18AM +0100, Tvrtko Ursulin wrote:
>>
>> Hi Matt,
>>
>> On 01/10/2022 01:45, Matt Roper wrote:
>>> MTL's media GT only has a single type of steering ("OAADDRM") which
>>> selects between media slice 0 and media slice 1.  We'll always steer to
>>> media slice 0 unless it is fused off (which is the case when VD0, VE0,
>>> and SFC0 are all reported as unavailable).
>>>
>>> Bspec: 67789
>>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/gt/intel_gt_mcr.c      | 19 +++++++++++++++++--
>>>    drivers/gpu/drm/i915/gt/intel_gt_types.h    |  1 +
>>>    drivers/gpu/drm/i915/gt/intel_workarounds.c | 18 +++++++++++++++++-
>>>    3 files changed, 35 insertions(+), 3 deletions(-)
>>
>> [snip]
>>
>>> +static void
>>> +mtl_media_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list *wal)
>>> +{
>>> +	/*
>>> +	 * Unlike older platforms, we no longer setup implicit steering here;
>>> +	 * all MCR accesses are explicitly steered.
>>> +	 */
>>> +	if (drm_debug_enabled(DRM_UT_DRIVER)) {
>>> +		struct drm_printer p = drm_debug_printer("MCR Steering:");
>>> +
>>> +		intel_gt_mcr_report_steering(&p, gt, false);
>>> +	}
>>> +}
>>> +
>>>    static void
>>>    gt_init_workarounds(struct intel_gt *gt, struct i915_wa_list *wal)
>>>    {
>>>    	struct drm_i915_private *i915 = gt->i915;
>>> -	if (IS_METEORLAKE(i915) && gt->type == GT_PRIMARY)
>>> +	if (IS_METEORLAKE(i915) && gt->type == GT_MEDIA)
>>> +		mtl_media_gt_workarounds_init(gt, wal);
>>> +	else if (IS_METEORLAKE(i915) && gt->type == GT_PRIMARY)
>>>    		mtl_3d_gt_workarounds_init(gt, wal);
>>>    	else if (IS_PONTEVECCHIO(i915))
>>>    		pvc_gt_workarounds_init(gt, wal);
>>
>> Casually reading only - wouldn't it be nicer if the if-ladder in here
>> (gt_init_workarounds) would have a single case per platform, and then you'd
>> fork further (3d vs media) in MTL specific function?
> 
> Actually, that reminds me that we probably need to change this in a
> different direction --- starting with MTL, we should stop tying
> workarounds to the platform (IS_METEORLAKE) but rather tie them to the
> IP version (i.e., GRAPHICS_VER or MEDIA_VER) since in the future the
> same chiplets can potentially be re-used on multiple platforms.
> Conversely, you could also potentially have variants of the same
> "platform" (e.g., MTL) that incorporate chiplets with different IP
> versions (and thus need distinct lists of workarounds and such).
> 
> At the moment MTL is the only platform we have with the standalone media
> design so there's no potential for mix-and-match of chiplets yet, and
> IS_METEORLAKE works fine in the short term, but we do need to start
> planning ahead and moving off of platform checks in areas of the driver
> like this.
> 
>>
>> Also, series ends up with mtl_media_gt_workarounds_init and
>> mtl_3d_gt_workarounds_init apparently 100% identical. You will need two
>> copies in the future?
> 
> Yes, the two GTs are expected to end up with completely different sets
> of workarounds once the platform is enabled.  We've just been delaying
> on actually sending the new MTL workarounds upstream to give the
> workaround database a bit more time to settle.

Ah yes, I misread the banner printed from those two "as no workaround 
will be programmed from here" and thought why you'd need two copies of a 
nearly empty function and two identical comments. My bad.

You will end up with three instances of "if debug report steering" so 
could in theory add a helper for that. For some minimal value of 
consolidation.. up to you.

Regards,

Tvrtko
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
index a1fa71b47831..a580723bdd04 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
@@ -42,6 +42,7 @@  static const char * const intel_steering_types[] = {
 	"LNCF",
 	"GAM",
 	"DSS",
+	"OADDRM",
 	"INSTANCE 0",
 };
 
@@ -129,6 +130,12 @@  static const struct intel_mmio_range mtl3d_dss_steering_table[] = {
 	{ 0x00DE80, 0x00E8FF },		/* DSS (0xE000-0xE0FF reserved) */
 };
 
+static const struct intel_mmio_range xelpmp_oaddrm_steering_table[] = {
+	{ 0x393200, 0x39323F },
+	{ 0x393400, 0x3934FF },
+};
+
+
 void intel_gt_mcr_init(struct intel_gt *gt)
 {
 	struct drm_i915_private *i915 = gt->i915;
@@ -151,8 +158,9 @@  void intel_gt_mcr_init(struct intel_gt *gt)
 			drm_warn(&i915->drm, "mslice mask all zero!\n");
 	}
 
-	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 70) &&
-	    gt->type == GT_PRIMARY) {
+	if (MEDIA_VER(i915) >= 13 && gt->type == GT_MEDIA) {
+		gt->steering_table[OADDRM] = xelpmp_oaddrm_steering_table;
+	} else if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 70)) {
 		fuse = REG_FIELD_GET(GT_L3_EXC_MASK,
 				     intel_uncore_read(gt->uncore, XEHP_FUSE4));
 
@@ -479,6 +487,13 @@  static void get_nonterminated_steering(struct intel_gt *gt,
 		*group = 0;
 		*instance = 0;
 		break;
+	case OADDRM:
+		if ((VDBOX_MASK(gt) | VEBOX_MASK(gt) | gt->info.sfc_mask) & BIT(0))
+			*group = 0;
+		else
+			*group = 1;
+		*instance = 0;
+		break;
 	default:
 		MISSING_CASE(type);
 		*group = 0;
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
index 77ecbd6be331..5d049e0c66f5 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
@@ -61,6 +61,7 @@  enum intel_steering_type {
 	LNCF,
 	GAM,
 	DSS,
+	OADDRM,
 
 	/*
 	 * 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 9be048da7fb3..744d83e5bb1f 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -1595,12 +1595,28 @@  mtl_3d_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list *wal)
 	}
 }
 
+static void
+mtl_media_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list *wal)
+{
+	/*
+	 * Unlike older platforms, we no longer setup implicit steering here;
+	 * all MCR accesses are explicitly steered.
+	 */
+	if (drm_debug_enabled(DRM_UT_DRIVER)) {
+		struct drm_printer p = drm_debug_printer("MCR Steering:");
+
+		intel_gt_mcr_report_steering(&p, gt, false);
+	}
+}
+
 static void
 gt_init_workarounds(struct intel_gt *gt, struct i915_wa_list *wal)
 {
 	struct drm_i915_private *i915 = gt->i915;
 
-	if (IS_METEORLAKE(i915) && gt->type == GT_PRIMARY)
+	if (IS_METEORLAKE(i915) && gt->type == GT_MEDIA)
+		mtl_media_gt_workarounds_init(gt, wal);
+	else if (IS_METEORLAKE(i915) && gt->type == GT_PRIMARY)
 		mtl_3d_gt_workarounds_init(gt, wal);
 	else if (IS_PONTEVECCHIO(i915))
 		pvc_gt_workarounds_init(gt, wal);