Re: [Intel-gfx] [PATCH dii-client 2/2] drm/i915/gt: Apply workaround 22016122933 correctly

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

On 21/07/2023 15:05, Jonathan Cavitt wrote:
> WA_22016122933 was recently applied to all MeteorLake engines, which is
> simultaneously too broad (should only apply to Media engines) and too
> specific (should apply to all platforms that use the same media engine
> as MeteorLake).  Correct this in cases where coherency settings are
> modified.
> 
> There were also two additional places where the workaround was applied
> unconditionally.  The change was confirmed as necessary for all
> platforms, so the workaround label was removed.
> 
> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> Suggested-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_pages.c | 5 +++--
>   drivers/gpu/drm/i915/gt/intel_gt.h        | 6 ++++++
>   drivers/gpu/drm/i915/gt/intel_lrc.c       | 7 ++++---
>   drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c | 4 ----
>   drivers/gpu/drm/i915/gt/uc/intel_guc.c    | 7 ++++---
>   drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 4 ----
>   6 files changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> index 44d93ead96ff..4acdd008d1d3 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> @@ -470,9 +470,10 @@ enum i915_map_type i915_coherent_map_type(struct intel_gt *gt,
>   					  bool always_coherent)
>   {
>   	/*
> -	 * Wa_22016122933: always return I915_MAP_WC for MTL
> +	 * Wa_22016122933: always return I915_MAP_WC for Media
> +	 * version 13.0 when the object is on the Media GT
>   	 */
> -	if (i915_gem_object_is_lmem(obj) || IS_METEORLAKE(gt->i915))
> +	if (i915_gem_object_is_lmem(obj) || intel_gt_needs_wa_22016122933(gt))
>   		return I915_MAP_WC;
>   	if (HAS_LLC(gt->i915) || always_coherent)
>   		return I915_MAP_WB;
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h b/drivers/gpu/drm/i915/gt/intel_gt.h
> index d2f4fbde5f9f..4eb41a3b6e8b 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.h
> @@ -6,6 +6,7 @@
>   #ifndef __INTEL_GT__
>   #define __INTEL_GT__
>   
> +#include "i915_drv.h"
>   #include "intel_engine_types.h"
>   #include "intel_gt_types.h"
>   #include "intel_reset.h"
> @@ -24,6 +25,11 @@ static inline bool gt_is_root(struct intel_gt *gt)
>   	return !gt->info.id;
>   }
>   
> +static inline bool intel_gt_needs_wa_22016122933(struct intel_gt *gt)
> +{
> +	return MEDIA_VER_FULL(gt->i915) == IP_VER(13, 0) && gt->type == GT_MEDIA;

Ah this is something Matt was recntly telling me is coming.

Would it make sense to store ipver in the gt? And then we could have:

	return gt->type == GT_MEDIA && gt->info.ipver == IP_VER(13,0);

?

(Give or take the gt->info part, and macro or not.)

Reads a bit better to me to ask the same component about it's type and 
version.

Regards,

Tvrtko

> +}
> +
>   static inline struct intel_gt *uc_to_gt(struct intel_uc *uc)
>   {
>   	return container_of(uc, struct intel_gt, uc);
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index c45e6d8cbaac..668ed3fc7076 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -1095,10 +1095,11 @@ __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
> -		 * as WC on CPU side and UC (PAT index 2) on GPU side
> +		 * Wa_22016122933: For Media version 13.0, all Media GT 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 (intel_gt_needs_wa_22016122933(engine->gt))
>   			i915_gem_object_set_cache_coherency(obj, I915_CACHE_NONE);
>   	}
>   	if (IS_ERR(obj))
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> index fb4933543f31..1093b47d3e06 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> @@ -284,10 +284,6 @@ static int gsc_fw_load_prepare(struct intel_gsc_uc *gsc)
>   	memcpy_toio(gsc->local_vaddr, src, gsc->fw.size);
>   	memset_io(gsc->local_vaddr + gsc->fw.size, 0, gsc->local->size - gsc->fw.size);
>   
> -	/*
> -	 * Wa_22016122933: Making sure the data in dst is
> -	 * visible to GSC right away
> -	 */
>   	intel_guc_write_barrier(&gt->uc.guc);
>   
>   	i915_gem_object_unpin_map(gsc->fw.obj);
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> index effb37727093..0000846f6029 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> @@ -745,10 +745,11 @@ 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
> -	 * as WC on CPU side and UC (PAT index 2) on GPU side
> +	 * Wa_22016122933: For Media version 13.0, all Media GT 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 (intel_gt_needs_wa_22016122933(gt))
>   		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/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> index f28a3a83742d..97eadd08181d 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> @@ -960,10 +960,6 @@ static int ct_read(struct intel_guc_ct *ct, struct ct_incoming_msg **msg)
>   	/* now update descriptor */
>   	WRITE_ONCE(desc->head, head);
>   
> -	/*
> -	 * Wa_22016122933: Making sure the head update is
> -	 * visible to GuC right away
> -	 */
>   	intel_guc_write_barrier(ct_to_guc(ct));
>   
>   	return available - len;