Re: [PATCH 1/3] drm/xe: Introduced needs_scratch bit in device descriptor

[Matthew Brost <matthew.brost@intel.com>]

On Wed, Feb 12, 2025 at 09:23:29PM -0500, Oak Zeng wrote:

Again, replied to wrong version earilier. Please include version number
in patches.

Copy / pasting my reply here. 

> On some platform, scratch page is needed for out of bound prefetch
> to work. Introduce a bit in device descriptor to specify whether
> this device needs scratch page to work.
> 
> v2: introduce a needs_scratch bit in device info (Thomas, Jonathan)
> 
> Signed-off-by: Oak Zeng <oak.zeng@intel.com>
> Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
>  drivers/gpu/drm/xe/xe_device_types.h | 3 +++
>  drivers/gpu/drm/xe/xe_pci.c          | 5 +++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index 833c29fed3a3..a5ba9e4e470f 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -46,6 +46,7 @@ struct xe_pxp;
>  #define GRAPHICS_VERx100(xe) ((xe)->info.graphics_verx100)
>  #define MEDIA_VERx100(xe) ((xe)->info.media_verx100)
>  #define IS_DGFX(xe) ((xe)->info.is_dgfx)
> +#define NEEDS_SCRATCH(xe) ((xe)->info.needs_scratch)
>  

I'd drop this macro. These are leftovers from the i915 code and I see
little value in a macro rather than looking directly at a field. In the
existing cases these macros are used all over the code so they make a
little more sense so if change the variable we only have change this
macro, but in this new case the macro is used 2 places in the code so
I'd say just look at the variable.

Matt


>  #define XE_VRAM_FLAGS_NEED64K		BIT(0)
>  
> @@ -318,6 +319,8 @@ struct xe_device {
>  		u8 has_usm:1;
>  		/** @info.is_dgfx: is discrete device */
>  		u8 is_dgfx:1;
> +		/** @info.needs_scratch: needs scratch page for oob prefetch to work */
> +		u8 needs_scratch:1;
>  		/**
>  		 * @info.probe_display: Probe display hardware.  If set to
>  		 * false, the driver will behave as if there is no display
> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> index 6a8e82aff385..c614f673efa5 100644
> --- a/drivers/gpu/drm/xe/xe_pci.c
> +++ b/drivers/gpu/drm/xe/xe_pci.c
> @@ -67,6 +67,7 @@ struct xe_device_desc {
>  	u8 has_llc:1;
>  	u8 has_pxp:1;
>  	u8 has_sriov:1;
> +	u8 needs_scratch:1;
>  	u8 skip_guc_pc:1;
>  	u8 skip_mtcfg:1;
>  	u8 skip_pcode:1;
> @@ -353,6 +354,7 @@ static const struct xe_device_desc lnl_desc = {
>  	.dma_mask_size = 46,
>  	.has_display = true,
>  	.has_pxp = true,
> +	.needs_scratch = true,
>  };
>  
>  static const struct xe_device_desc bmg_desc = {
> @@ -361,6 +363,7 @@ static const struct xe_device_desc bmg_desc = {
>  	.dma_mask_size = 46,
>  	.has_display = true,
>  	.has_heci_cscfi = 1,
> +	.needs_scratch = true,
>  };
>  
>  static const struct xe_device_desc ptl_desc = {
> @@ -369,6 +372,7 @@ static const struct xe_device_desc ptl_desc = {
>  	.has_display = true,
>  	.has_sriov = true,
>  	.require_force_probe = true,
> +	.needs_scratch = true,
>  };
>  
>  #undef PLATFORM
> @@ -644,6 +648,7 @@ static int xe_info_init_early(struct xe_device *xe,
>  	xe->info.skip_guc_pc = desc->skip_guc_pc;
>  	xe->info.skip_mtcfg = desc->skip_mtcfg;
>  	xe->info.skip_pcode = desc->skip_pcode;
> +	xe->info.needs_scratch = desc->needs_scratch;
>  
>  	xe->info.probe_display = IS_ENABLED(CONFIG_DRM_XE_DISPLAY) &&
>  				 xe_modparam.probe_display &&
> -- 
> 2.26.3
>