Re: [PATCH i-g-t] lib/igt_fb/intel: Use correct MOCS for displayable surfaces

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Oct 03, 2025 at 09:37:46AM +0100, Tvrtko Ursulin wrote:
> Using the uncached MOCS for displayable surfaces is not always correct,
> especially when CCS compression is used with which some platforms require
> a special uncached entry, otherwise writes get unexpectedly cached.
> 
> Lets copy the knowledge of what is the correct MOCS for displayable
> surfaces from Mesa and add some new helpers to get it.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  lib/igt_fb.c       |  2 +-
>  lib/intel_bufops.c |  2 ++
>  lib/intel_mocs.c   | 21 +++++++++++++++++++--
>  lib/intel_mocs.h   |  2 ++
>  4 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index 03ede3a6fa20..b5a16f9cbe90 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -2712,7 +2712,7 @@ igt_fb_create_intel_buf(int fd, struct buf_ops *bops,
>  				    fb->strides[0],
>  				    region,
>  				    intel_get_pat_idx_uc(fd),
> -				    DEFAULT_MOCS_INDEX);
> +				    DISPLAYABLE_MOCS_INDEX);
>  	intel_buf_set_name(buf, name);
>  
>  	/* only really needed for proper CCS handling */
> diff --git a/lib/intel_bufops.c b/lib/intel_bufops.c
> index 475b0d1f7b10..1196069a500f 100644
> --- a/lib/intel_bufops.c
> +++ b/lib/intel_bufops.c
> @@ -1008,6 +1008,8 @@ static void __intel_buf_init(struct buf_ops *bops,
>  	buf->pat_index = pat_index;
>  	if (mocs_index == DEFAULT_MOCS_INDEX)
>  		mocs_index = intel_get_uc_mocs_index(bops->fd);
> +	else if (mocs_index == DISPLAYABLE_MOCS_INDEX)
> +		mocs_index = intel_get_displayable_mocs_index(bops->fd);
>  	buf->mocs_index = mocs_index;
>  	IGT_INIT_LIST_HEAD(&buf->link);
>  
> diff --git a/lib/intel_mocs.c b/lib/intel_mocs.c
> index e0c33c31c088..eefe83cf2116 100644
> --- a/lib/intel_mocs.c
> +++ b/lib/intel_mocs.c
> @@ -9,12 +9,14 @@
>  struct drm_intel_mocs_index {
>  	uint8_t uc_index;
>  	uint8_t wb_index;
> +	uint8_t external_index;

"external" doesn't mean anything for igt, so I would
just call it "displayable" or something.

>  	uint8_t defer_to_pat_index;
>  };
>  
>  static void get_mocs_index(int fd, struct drm_intel_mocs_index *mocs)
>  {
>  	uint16_t devid = intel_get_drm_devid(fd);
> +	unsigned int ip_ver = intel_graphics_ver(devid);
>  
>  	/*
>  	 * Gen >= 12 onwards don't have a setting for PTE,
> @@ -23,25 +25,31 @@ static void get_mocs_index(int fd, struct drm_intel_mocs_index *mocs)
>  	 * This helper function is providing current UC as well
>  	 * as WB MOCS index based on platform.
>  	 */
> -	if (intel_graphics_ver(devid) >= IP_VER(20, 0)) {
> +	if (ip_ver >= IP_VER(20, 0)) {
>  		mocs->uc_index = 3;
>  		mocs->wb_index = 4;
> +		mocs->external_index = 1;
>  		mocs->defer_to_pat_index = 0;
>  	} else if (IS_METEORLAKE(devid)) {
>  		mocs->uc_index = 5;
>  		mocs->wb_index = 1;
> +		mocs->external_index = 14;
>  	} else if (IS_DG2(devid)) {
>  		mocs->uc_index = 1;
>  		mocs->wb_index = 3;
> +		mocs->external_index = 3;
>  	} else if (IS_DG1(devid)) {
>  		mocs->uc_index = 1;
>  		mocs->wb_index = 5;
> -	} else if (IS_GEN12(devid)) {
> +		mocs->external_index = 5;
> +	} else if (ip_ver >= IP_VER(12, 0)) {
>  		mocs->uc_index = 3;
>  		mocs->wb_index = 2;
> +		mocs->external_index = 61;
>  	} else {
>  		mocs->uc_index = I915_MOCS_PTE;
>  		mocs->wb_index = I915_MOCS_CACHED;
> +		mocs->external_index = I915_MOCS_PTE;
>  	}
>  }
>  
> @@ -63,6 +71,15 @@ uint8_t intel_get_uc_mocs_index(int fd)
>  	return mocs.uc_index;
>  }
>  
> +uint8_t intel_get_displayable_mocs_index(int fd)
> +{
> +	struct drm_intel_mocs_index mocs;
> +
> +	get_mocs_index(fd, &mocs);
> +
> +	return mocs.external_index;
> +}
> +
>  uint8_t intel_get_defer_to_pat_mocs_index(int fd)
>  {
>  	struct drm_intel_mocs_index mocs;
> diff --git a/lib/intel_mocs.h b/lib/intel_mocs.h
> index 8597286d259d..394bb41be042 100644
> --- a/lib/intel_mocs.h
> +++ b/lib/intel_mocs.h
> @@ -9,9 +9,11 @@
>  #include <stdint.h>
>  
>  #define DEFAULT_MOCS_INDEX ((uint8_t)-1)
> +#define DISPLAYABLE_MOCS_INDEX ((uint8_t)-2)

Why do we even have these weird values? Ie. why can't we just
always provide the correct MOCS from the start?

And I really think this should handle *all* the platforms the
same way, but currently the MOCS for older platforms is handled
in some completely different way :(

But I suppose neither of those issues should be addressed in
this patch.

>  
>  uint8_t intel_get_wb_mocs_index(int fd);
>  uint8_t intel_get_uc_mocs_index(int fd);
> +uint8_t intel_get_displayable_mocs_index(int fd);
>  uint8_t intel_get_defer_to_pat_mocs_index(int fd);
>  
>  #endif /* _INTEL_MOCS_H */
> -- 
> 2.48.0

-- 
Ville Syrjälä
Intel