Re: [PATCH 8/8] drm/i915: Replace several IS_METEORLAKE with proper IP version checks

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 19/07/2023 16:22, Matt Roper wrote:
> On Wed, Jul 19, 2023 at 08:57:15AM +0100, Tvrtko Ursulin wrote:
> > On 18/07/2023 23:28, Matt Roper wrote:
> > > Many of the IS_METEORLAKE conditions throughout the driver are supposed
> > > to be checks for Xe_LPG and/or Xe_LPM+ IP, not for the MTL platform
> > > specifically.  Update those checks to ensure that the code will still
> > > operate properly if/when these IP versions show up on future platforms.
> > >
> > > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > > ---
> > >    drivers/gpu/drm/i915/gem/i915_gem_create.c | 4 ++--
> > >    drivers/gpu/drm/i915/gem/i915_gem_pages.c  | 4 ++--
> > >    drivers/gpu/drm/i915/gt/intel_engine_pm.c  | 2 +-
> > >    drivers/gpu/drm/i915/gt/intel_lrc.c        | 4 ++--
> > >    drivers/gpu/drm/i915/gt/intel_mocs.c       | 2 +-
> > >    drivers/gpu/drm/i915/gt/intel_reset.c      | 2 +-
> > >    drivers/gpu/drm/i915/gt/intel_rps.c        | 2 +-
> > >    drivers/gpu/drm/i915/gt/uc/intel_guc.c     | 4 ++--
> > >    drivers/gpu/drm/i915/i915_debugfs.c        | 2 +-
> > >    drivers/gpu/drm/i915/i915_perf.c           | 8 +++++---
> > >    10 files changed, 18 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c
> > > index d24c0ce8805c..19156ba4b9ef 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c
> > > @@ -405,8 +405,8 @@ static int ext_set_pat(struct i915_user_extension __user *base, void *data)
> > >    	BUILD_BUG_ON(sizeof(struct drm_i915_gem_create_ext_set_pat) !=
> > >    		     offsetofend(struct drm_i915_gem_create_ext_set_pat, rsvd));
> > > -	/* Limiting the extension only to Meteor Lake */
> > > -	if (!IS_METEORLAKE(i915))
> > > +	/* Limiting the extension only to Xe_LPG and beyond */
> > > +	if (GRAPHICS_VER_FULL(i915) < IP_VER(12, 70))
> > >    		return -ENODEV;
> > >    	if (copy_from_user(&ext, base, sizeof(ext)))
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> > > index 89fc8ea6bcfc..4b003925cc3e 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> > > @@ -470,9 +470,9 @@ enum i915_map_type i915_coherent_map_type(struct drm_i915_private *i915,
> > >    					  bool always_coherent)
> > >    {
> > >    	/*
> > > -	 * Wa_22016122933: always return I915_MAP_WC for MTL
> > > +	 * Wa_22016122933: always return I915_MAP_WC for Xe_LPM+
> > >    	 */
> > > -	if (i915_gem_object_is_lmem(obj) || IS_METEORLAKE(i915))
> > > +	if (i915_gem_object_is_lmem(obj) || MEDIA_VER_FULL(i915) == IP_VER(13, 0))
> >
> > The helper is global for the driver/device but this makes the response based
> > on just one part of the driver which reads dodgy. There is another one in
> > __lrc_alloc_state.
> >
> > For instance, do these workarounds need to be applied to an object used from
> > the render tile? Yes it also was before these change, but now it is not
> > obvious to me if and how the ipver of the media tile affects the other tile.
>
> The current driver implementation is a bit overkill and goes beyond
> what's actually requested by the workaround.  There are only coherency
> issues between the CPU and media, so if an object is only used on the
> primary GT (which would be the case for a lot of stuff like GuC0 CTBs
> and such), the workaround is not necessary.
>
> The workaround database marks this one as relevant to Xe_LPM+ and _not_
> relevant to Xe_LPG.  Also, the hardware team's description even
> explicitly notes that this is only necessary for media and that the
> primary GT is not impacted.
>
> I pointed this out recently in one of the internal reviews for some of
> this code, so I think there may be some optimizations to the
> implementation of this workaround coming soon.

Okay, good to know improvements are on the radar. Would it make sense to 
put a comment in i915_coherent_map_type, explaining the scope is 
currently too wide, I mean in this patch when you replace the check with 
MEDIA_VER_FULL?

Regards,

Tvrtko

> Matt
>
> > Regards,
> >
> > Tvrtko
> >
> > >    		return I915_MAP_WC;
> > >    	if (HAS_LLC(i915) || always_coherent)
> > >    		return I915_MAP_WB;
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> > > index 21af0ec52223..24060278e7a2 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> > > @@ -21,7 +21,7 @@ static void intel_gsc_idle_msg_enable(struct intel_engine_cs *engine)
> > >    {
> > >    	struct drm_i915_private *i915 = engine->i915;
> > > -	if (IS_METEORLAKE(i915) && engine->id == GSC0) {
> > > +	if (MEDIA_VER(i915) >= 13 && engine->id == GSC0) {
> > >    		intel_uncore_write(engine->gt->uncore,
> > >    				   RC_PSMI_CTRL_GSCCS,
> > >    				   _MASKED_BIT_DISABLE(IDLE_MSG_DISABLE));
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > > index 4fefa67d285f..a125c3284bab 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > > @@ -1095,10 +1095,10 @@ __lrc_alloc_state(struct intel_context *ce, struct intel_engine_cs *engine)
> > >    	if (IS_ERR(obj)) {
> > >    		obj = i915_gem_object_create_shmem(engine->i915, context_size);
> > >    		/*
> > > -		 * Wa_22016122933: For MTL the shared memory needs to be mapped
> > > +		 * Wa_22016122933: For Xe_LPM+ the shared memory needs to be mapped
> > >    		 * as WC on CPU side and UC (PAT index 2) on GPU side
> > >    		 */
> > > -		if (IS_METEORLAKE(engine->i915))
> > > +		if (MEDIA_VER_FULL(engine->i915) == IP_VER(13, 0))
> > >    			i915_gem_object_set_cache_coherency(obj, I915_CACHE_NONE);
> > >    	}
> > >    	if (IS_ERR(obj))
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_mocs.c b/drivers/gpu/drm/i915/gt/intel_mocs.c
> > > index 2c014407225c..830ad2c10761 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_mocs.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_mocs.c
> > > @@ -507,7 +507,7 @@ static unsigned int get_mocs_settings(const struct drm_i915_private *i915,
> > >    	memset(table, 0, sizeof(struct drm_i915_mocs_table));
> > >    	table->unused_entries_index = I915_MOCS_PTE;
> > > -	if (IS_METEORLAKE(i915)) {
> > > +	if (GT_GRAPHICS_RANGE(&i915->gt0, IP_VER(12, 70), IP_VER(12, 71))) {
> > >    		table->size = ARRAY_SIZE(mtl_mocs_table);
> > >    		table->table = mtl_mocs_table;
> > >    		table->n_entries = MTL_NUM_MOCS_ENTRIES;
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
> > > index 403f0d9caadf..0714584dd83d 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> > > @@ -705,7 +705,7 @@ static int __reset_guc(struct intel_gt *gt)
> > >    static bool needs_wa_14015076503(struct intel_gt *gt, intel_engine_mask_t engine_mask)
> > >    {
> > > -	if (!IS_METEORLAKE(gt->i915) || !HAS_ENGINE(gt, GSC0))
> > > +	if (MEDIA_VER_FULL(gt->i915) != IP_VER(13, 0) || !HAS_ENGINE(gt, GSC0))
> > >    		return false;
> > >    	if (!__HAS_ENGINE(engine_mask, GSC0))
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
> > > index e92e626d4994..89775328440e 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_rps.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_rps.c
> > > @@ -1160,7 +1160,7 @@ void gen6_rps_get_freq_caps(struct intel_rps *rps, struct intel_rps_freq_caps *c
> > >    {
> > >    	struct drm_i915_private *i915 = rps_to_i915(rps);
> > > -	if (IS_METEORLAKE(i915))
> > > +	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 70))
> > >    		return mtl_get_freq_caps(rps, caps);
> > >    	else
> > >    		return __gen6_rps_get_freq_caps(rps, caps);
> > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> > > index 300b968e6de8..310d12dc7077 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> > > @@ -743,10 +743,10 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
> > >    		return ERR_CAST(obj);
> > >    	/*
> > > -	 * Wa_22016122933: For MTL the shared memory needs to be mapped
> > > +	 * Wa_22016122933: For Xe_LPM+ the shared memory needs to be mapped
> > >    	 * as WC on CPU side and UC (PAT index 2) on GPU side
> > >    	 */
> > > -	if (IS_METEORLAKE(gt->i915))
> > > +	if (MEDIA_VER_FULL(gt->i915) == IP_VER(13, 0))
> > >    		i915_gem_object_set_cache_coherency(obj, I915_CACHE_NONE);
> > >    	vma = i915_vma_instance(obj, &gt->ggtt->vm, NULL);
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index 4de44cf1026d..0a0217271b76 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -144,7 +144,7 @@ static const char *i915_cache_level_str(struct drm_i915_gem_object *obj)
> > >    {
> > >    	struct drm_i915_private *i915 = obj_to_i915(obj);
> > > -	if (IS_METEORLAKE(i915)) {
> > > +	if (GT_GRAPHICS_RANGE(to_gt(i915), IP_VER(12, 70), IP_VER(12, 71))) {
> > >    		switch (obj->pat_index) {
> > >    		case 0: return " WB";
> > >    		case 1: return " WT";
> > > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> > > index ff95f2cdf2b0..cc9592d4de23 100644
> > > --- a/drivers/gpu/drm/i915/i915_perf.c
> > > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > > @@ -3227,11 +3227,13 @@ get_sseu_config(struct intel_sseu *out_sseu,
> > >     */
> > >    u32 i915_perf_oa_timestamp_frequency(struct drm_i915_private *i915)
> > >    {
> > > +	struct intel_gt *gt = to_gt(i915);
> > > +
> > >    	/*
> > >    	 * Wa_18013179988:dg2
> > > -	 * Wa_14015846243:mtl
> > > +	 * Wa_14015846243:xelpg
> > >    	 */
> > > -	if (IS_DG2(i915) || IS_METEORLAKE(i915)) {
> > > +	if (IS_DG2(i915) || GT_GRAPHICS_RANGE(gt, IP_VER(12, 70), IP_VER(12, 71))) {
> > >    		intel_wakeref_t wakeref;
> > >    		u32 reg, shift;
> > > @@ -4539,7 +4541,7 @@ static bool xehp_is_valid_b_counter_addr(struct i915_perf *perf, u32 addr)
> > >    static bool gen12_is_valid_mux_addr(struct i915_perf *perf, u32 addr)
> > >    {
> > > -	if (IS_METEORLAKE(perf->i915))
> > > +	if (GRAPHICS_VER_FULL(perf->i915) >= IP_VER(12, 70))
> > >    		return reg_in_range_table(addr, mtl_oa_mux_regs);
> > >    	else
> > >    		return reg_in_range_table(addr, gen12_oa_mux_regs);