Re: [PATCH v2 3/3] drm/xe: Add CLOS specific initializations

["Welty, Brian" <brian.welty@intel.com>]

On 1/9/2024 3:57 PM, Pallavi Mishra wrote:
> Handle CLOS specific initializations and PAT
> CLOS compatibility check.
> 
> Signed-off-by: Pallavi Mishra <pallavi.mishra@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_device.c | 15 ++++++++++++++
>   drivers/gpu/drm/xe/xe_pat.c    | 36 ++++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/xe/xe_pat.h    |  9 +++++++++
>   drivers/gpu/drm/xe/xe_vm.c     | 12 ++++++++++++
>   4 files changed, 72 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 004e65544e8d..4e3d4f2c0f9b 100644
[snip]
> diff --git a/drivers/gpu/drm/xe/xe_pat.c b/drivers/gpu/drm/xe/xe_pat.c
> index 1ff6bc79e7d4..ccdefba0f2f3 100644
> --- a/drivers/gpu/drm/xe/xe_pat.c
> +++ b/drivers/gpu/drm/xe/xe_pat.c
> @@ -45,6 +45,14 @@
>   #define XELP_PAT_WC				REG_FIELD_PREP(XELP_MEM_TYPE_MASK, 1)
>   #define XELP_PAT_UC				REG_FIELD_PREP(XELP_MEM_TYPE_MASK, 0)
>   
> +#define XE2_PAT_CLOS1                          ((1 << 20)|(1 << 21)|(1 << 22)|(1 << 23))
> +#define XE2_PAT_CLOS2                          ((1 << 24)|(1 << 25)|(1 << 26)|(1 << 27))
> +#define XE2_PAT_CLOS3                          ((1 << 28)|(1 << 29)|(1 << 30)|(1 << 31))
> +
> +#define XEPVC_PAT_CLOS1                                ((1 << 4)|(1 << 5))
> +#define XEPVC_PAT_CLOS2                                ((1 << 6)|(1 << 7))
> +
> +
>   static const char *XELP_MEM_TYPE_STR_MAP[] = { "UC", "WC", "WT", "WB" };
>   
>   struct xe_pat_ops {
> @@ -148,6 +156,34 @@ u16 xe_pat_index_get_coh_mode(struct xe_device *xe, u16 pat_index)
>   	return xe->pat.table[pat_index].coh_mode;
>   }
>   
> +int xe_pat_index_clos_check(struct xe_device *xe, u16 pat_index, u16 clos_index)
> +{
> +	WARN_ON(pat_index >= xe->pat.n_entries);
> +
> +	int err = 0;
> +
> +	switch (clos_index) {
> +	case 1:
> +		if (!(((1 << pat_index) & XE2_PAT_CLOS1)
> +			|| (1 << pat_index & XEPVC_PAT_CLOS1)))
> +			err = -EINVAL;
> +		break;
> +	case 2:
> +		if (!(((1 << pat_index) & XE2_PAT_CLOS2)
> +			|| (1 << pat_index & XEPVC_PAT_CLOS2)))
> +			err = -EINVAL;
> +		break;
> +	case 3:
> +		if (!((1 << pat_index) & XE2_PAT_CLOS3))
> +			err = -EINVAL;
> +		break;
> +	default:
> +		drm_err(&xe->drm, "Unsupported CLOS value\n");
> +			err = -EINVAL;
> +	}
> +	return err;
> +}
> +
>   static void program_pat(struct xe_gt *gt, const struct xe_pat_table_entry table[],
>   			int n_entries)
>   {
> diff --git a/drivers/gpu/drm/xe/xe_pat.h b/drivers/gpu/drm/xe/xe_pat.h
> index fa0dfbe525cd..afac06bc425f 100644
> --- a/drivers/gpu/drm/xe/xe_pat.h
> +++ b/drivers/gpu/drm/xe/xe_pat.h
> @@ -58,4 +58,13 @@ void xe_pat_dump(struct xe_gt *gt, struct drm_printer *p);
>    */
>   u16 xe_pat_index_get_coh_mode(struct xe_device *xe, u16 pat_index);
>   
> +/**
> + * xe_pat_index_clos_check - check whether clos has been reserved for
> + * chosen pat_index.
> + * @xe: xe device
> + * @pat_index: The pat_index to query
> + * @clos_index: clos index to compare
> + */
> +int xe_pat_index_clos_check(struct xe_device *xe, u16 pat_index, u16 clos_index);
> +
>   #endif
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 1ca917b8315c..8e8c0302c8a0 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -2800,6 +2800,18 @@ static int vm_bind_ioctl_check_args(struct xe_device *xe,
>   			err = -EINVAL;
>   			goto free_bind_ops;
>   		}
> +
> +		/* check whether Clos has been reserved for chosen pat */
> +		if ((GRAPHICS_VER(xe) >= 20 && (pat_index > 19)) || (xe->info.platform == XE_PVC && (pat_index > 3))) {
> +			mutex_lock(&xe->cache_resv.clos_mutex);
> +			err = xe_pat_index_clos_check(xe, pat_index, xe->cache_resv.clos_index);

Thanks for pointing out that xe->cache_resv.clos_index is used here.....
But this doesn't seem correct.  That is simply the most recently 
reserved clos_index.  It might have been reserved by some other user.

I believe you need to test using the set of CLOS indexes reserved to 
this user.   So you need something like:

     struct xe_file *xef = to_xe_file(file);

     for each clos_index in xef->clos_resv.clos_mask
         xe_pat_index_clos_check(xe, pat_index, clos_index);
     confirm pat_index is valid for one of the reserved clos indexes

Right?


> +			if (err) {
> +				mutex_unlock(&xe->cache_resv.clos_mutex);
> +				goto free_bind_ops;
> +			}
> +			mutex_unlock(&xe->cache_resv.clos_mutex);
> +		}
> +
>   	}
>   
>   	return 0;