Re: [Intel-gfx] [PATCH v2 14/14] drm/i915/mtl: Add multicast steering for media GT

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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