Re: [PATCH 1/7] drm/xe: Modify stepping info directly in xe_step_*_get()

[Matt Roper <matthew.d.roper@intel.com>]

On Thu, Mar 05, 2026 at 09:02:28AM -0300, Gustavo Sousa wrote:
> In an upcoming change, we will add a member to struct xe_step_info to
> represent the platform-level stepping.  As such, we should stop assigning
> the value returned by functions xe_step_pre_gmdid_get() and
> xe_step_gmdid_get() directly to xe->info.step.
> 
> Since there are no other users for those functions, let's simply update
> them to modify xe->info.step directly.
> 
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/xe/xe_pci.c  |  6 ++---
>  drivers/gpu/drm/xe/xe_step.c | 52 +++++++++++++++++++++++++-------------------
>  drivers/gpu/drm/xe/xe_step.h |  8 +++----
>  3 files changed, 36 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> index 29f976e66848..72d4131e9775 100644
> --- a/drivers/gpu/drm/xe/xe_pci.c
> +++ b/drivers/gpu/drm/xe/xe_pci.c
> @@ -914,7 +914,7 @@ static int xe_info_init(struct xe_device *xe,
>  	if (desc->pre_gmdid_graphics_ip) {
>  		graphics_ip = desc->pre_gmdid_graphics_ip;
>  		media_ip = desc->pre_gmdid_media_ip;
> -		xe->info.step = xe_step_pre_gmdid_get(xe);
> +		xe_step_pre_gmdid_get(xe);
>  	} else {
>  		xe_assert(xe, !desc->pre_gmdid_media_ip);
>  		ret = handle_gmdid(xe, &graphics_ip, &media_ip,
> @@ -922,9 +922,7 @@ static int xe_info_init(struct xe_device *xe,
>  		if (ret)
>  			return ret;
>  
> -		xe->info.step = xe_step_gmdid_get(xe,
> -						  graphics_gmdid_revid,
> -						  media_gmdid_revid);
> +		xe_step_gmdid_get(xe, graphics_gmdid_revid, media_gmdid_revid);
>  	}
>  
>  	/*
> diff --git a/drivers/gpu/drm/xe/xe_step.c b/drivers/gpu/drm/xe/xe_step.c
> index 2860986f82f7..064b604b5b94 100644
> --- a/drivers/gpu/drm/xe/xe_step.c
> +++ b/drivers/gpu/drm/xe/xe_step.c
> @@ -115,15 +115,17 @@ __diag_pop();
>   * Convert the PCI revid into proper IP steppings.  This should only be
>   * used on platforms that do not have GMD_ID support.
>   */
> -struct xe_step_info xe_step_pre_gmdid_get(struct xe_device *xe)
> +void xe_step_pre_gmdid_get(struct xe_device *xe)
>  {
>  	const struct xe_step_info *revids = NULL;
> -	struct xe_step_info step = {};
>  	u16 revid = xe->info.revid;
>  	int size = 0;
>  	const int *basedie_info = NULL;
>  	int basedie_size = 0;
>  	int baseid = 0;
> +	u8 graphics = STEP_NONE;
> +	u8 media = STEP_NONE;
> +	u8 basedie = STEP_NONE;
>  
>  	if (xe->info.platform == XE_PVC) {
>  		baseid = FIELD_GET(GENMASK(5, 3), xe->info.revid);
> @@ -166,10 +168,12 @@ struct xe_step_info xe_step_pre_gmdid_get(struct xe_device *xe)
>  
>  	/* Not using the stepping scheme for the platform yet. */
>  	if (!revids)
> -		return step;
> +		goto done;
>  
>  	if (revid < size && revids[revid].graphics != STEP_NONE) {
> -		step = revids[revid];
> +		graphics = revids[revid].graphics;
> +		media = revids[revid].media;
> +		basedie = revids[revid].basedie;
>  	} else {
>  		drm_warn(&xe->drm, "Unknown revid 0x%02x\n", revid);
>  
> @@ -187,25 +191,30 @@ struct xe_step_info xe_step_pre_gmdid_get(struct xe_device *xe)
>  		if (revid < size) {
>  			drm_dbg(&xe->drm, "Using steppings for revid 0x%02x\n",
>  				revid);
> -			step = revids[revid];
> +			graphics = revids[revid].graphics;
> +			media = revids[revid].media;
> +			basedie = revids[revid].basedie;
>  		} else {
>  			drm_dbg(&xe->drm, "Using future steppings\n");
> -			step.graphics = STEP_FUTURE;
> +			graphics = STEP_FUTURE;
>  		}
>  	}
>  
> -	drm_WARN_ON(&xe->drm, step.graphics == STEP_NONE);
> +	drm_WARN_ON(&xe->drm, graphics == STEP_NONE);
>  
>  	if (basedie_info && basedie_size) {
>  		if (baseid < basedie_size && basedie_info[baseid] != STEP_NONE) {
> -			step.basedie = basedie_info[baseid];
> +			basedie = basedie_info[baseid];
>  		} else {
>  			drm_warn(&xe->drm, "Unknown baseid 0x%02x\n", baseid);
> -			step.basedie = STEP_FUTURE;
> +			basedie = STEP_FUTURE;
>  		}
>  	}
>  
> -	return step;
> +done:
> +	xe->info.step.graphics = graphics;
> +	xe->info.step.media = media;
> +	xe->info.step.basedie = basedie;
>  }
>  
>  /**
> @@ -220,28 +229,27 @@ struct xe_step_info xe_step_pre_gmdid_get(struct xe_device *xe)
>   * all platforms:  major steppings (A0, B0, etc.) are 4 apart, with minor
>   * steppings (A1, A2, etc.) taking the values in between.
>   */
> -struct xe_step_info xe_step_gmdid_get(struct xe_device *xe,
> -				      u32 graphics_gmdid_revid,
> -				      u32 media_gmdid_revid)
> +void xe_step_gmdid_get(struct xe_device *xe,
> +		       u32 graphics_gmdid_revid,
> +		       u32 media_gmdid_revid)
>  {
> -	struct xe_step_info step = {
> -		.graphics = STEP_A0 + graphics_gmdid_revid,
> -		.media = STEP_A0 + media_gmdid_revid,
> -	};
> +	u8 graphics = STEP_A0 + graphics_gmdid_revid;
> +	u8 media = STEP_A0 + media_gmdid_revid;
>  
> -	if (step.graphics >= STEP_FUTURE) {
> -		step.graphics = STEP_FUTURE;
> +	if (graphics >= STEP_FUTURE) {
> +		graphics = STEP_FUTURE;
>  		drm_dbg(&xe->drm, "Graphics GMD_ID revid value %d treated as future stepping\n",
>  			graphics_gmdid_revid);
>  	}
>  
> -	if (step.media >= STEP_FUTURE) {
> -		step.media = STEP_FUTURE;
> +	if (media >= STEP_FUTURE) {
> +		media = STEP_FUTURE;
>  		drm_dbg(&xe->drm, "Media GMD_ID revid value %d treated as future stepping\n",
>  			media_gmdid_revid);
>  	}
>  
> -	return step;
> +	xe->info.step.graphics = graphics;
> +	xe->info.step.media = media;
>  }
>  
>  #define STEP_NAME_CASE(name)	\
> diff --git a/drivers/gpu/drm/xe/xe_step.h b/drivers/gpu/drm/xe/xe_step.h
> index 686cb59200c2..6febb7fac476 100644
> --- a/drivers/gpu/drm/xe/xe_step.h
> +++ b/drivers/gpu/drm/xe/xe_step.h
> @@ -12,10 +12,10 @@
>  
>  struct xe_device;
>  
> -struct xe_step_info xe_step_pre_gmdid_get(struct xe_device *xe);
> -struct xe_step_info xe_step_gmdid_get(struct xe_device *xe,
> -				      u32 graphics_gmdid_revid,
> -				      u32 media_gmdid_revid);
> +void xe_step_pre_gmdid_get(struct xe_device *xe);
> +void xe_step_gmdid_get(struct xe_device *xe,
> +		       u32 graphics_gmdid_revid,
> +		       u32 media_gmdid_revid);
>  static inline u32 xe_step_to_gmdid(enum xe_step step) { return step - STEP_A0; }
>  
>  const char *xe_step_name(enum xe_step step);
> 
> -- 
> 2.52.0
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation