Re: [PATCH 2/4] drm/xe: Disambiguate GMDID-based IP names

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

On Thu, Feb 20, 2025 at 02:25:09PM -0300, Gustavo Sousa wrote:
> The name of an IP is a function of its version. As such, given an IP
> version, it should be clear to identify the name of that IP release.
> 
> With the current code, we keep that mapping clear for pre-GMDID IPs, but
> ambiguous for GMDID-based ones. That causes two types of inconveniences:
> 
>  1. The end user, who might not have all the necessary mapping at hand,
>     might be confused when seeing different possible IP names in the
>     dmesg log.
> 
>  2. It makes a developer who is not familiar with the "IP version" to
>     "Release name" need to resort to looking at the specs to understand
>     see what version maps to what. While the specs should be the
>     authority on the mapping, we should make our lives easier by
>     reflecting that mapping in the source code.
> 
> Thus, since the IP name is tied to the version, let's  remove the
> ambiguity by using a "name" field in struct gmdid_map instead of
> accumulating names in the descriptor instances.
> 
> This does result in the code having IP name being defined in
> different structs (gmdid_map, xe_graphics_desc, xe_media_desc), but that
> will be resolved in upcoming changes.
> 
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_pci.c       | 31 +++++++++++++------------------
>  drivers/gpu/drm/xe/xe_pci_types.h |  1 +
>  2 files changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> index feea897f130d..a61a8982ab67 100644
> --- a/drivers/gpu/drm/xe/xe_pci.c
> +++ b/drivers/gpu/drm/xe/xe_pci.c
> @@ -149,7 +149,6 @@ static const struct xe_graphics_desc graphics_xehpc = {
>  };
>  
>  static const struct xe_graphics_desc graphics_xelpg = {
> -	.name = "Xe_LPG",
>  	.hw_engine_mask =
>  		BIT(XE_HW_ENGINE_RCS0) | BIT(XE_HW_ENGINE_BCS0) |
>  		BIT(XE_HW_ENGINE_CCS0),
> @@ -172,8 +171,6 @@ static const struct xe_graphics_desc graphics_xelpg = {
>  		GENMASK(XE_HW_ENGINE_CCS3, XE_HW_ENGINE_CCS0)
>  
>  static const struct xe_graphics_desc graphics_xe2 = {
> -	.name = "Xe2_LPG / Xe2_HPG / Xe3_LPG",
> -
>  	XE2_GFX_FEATURES,
>  };
>  
> @@ -198,7 +195,6 @@ static const struct xe_media_desc media_xehpm = {
>  };
>  
>  static const struct xe_media_desc media_xelpmp = {
> -	.name = "Xe_LPM+",
>  	.hw_engine_mask =
>  		GENMASK(XE_HW_ENGINE_VCS7, XE_HW_ENGINE_VCS0) |
>  		GENMASK(XE_HW_ENGINE_VECS3, XE_HW_ENGINE_VECS0) |
> @@ -206,7 +202,6 @@ static const struct xe_media_desc media_xelpmp = {
>  };
>  
>  static const struct xe_media_desc media_xe2 = {
> -	.name = "Xe2_LPM / Xe2_HPM / Xe3_LPM",
>  	.hw_engine_mask =
>  		GENMASK(XE_HW_ENGINE_VCS7, XE_HW_ENGINE_VCS0) |
>  		GENMASK(XE_HW_ENGINE_VECS3, XE_HW_ENGINE_VECS0) |
> @@ -376,21 +371,21 @@ __diag_pop();
>  
>  /* Map of GMD_ID values to graphics IP */
>  static const struct gmdid_map graphics_ip_map[] = {
> -	{ 1270, &graphics_xelpg },
> -	{ 1271, &graphics_xelpg },
> -	{ 1274, &graphics_xelpg },	/* Xe_LPG+ */
> -	{ 2001, &graphics_xe2 },
> -	{ 2004, &graphics_xe2 },
> -	{ 3000, &graphics_xe2 },
> -	{ 3001, &graphics_xe2 },
> +	{ 1270, "Xe_LPG", &graphics_xelpg },
> +	{ 1271, "Xe_LPG", &graphics_xelpg },
> +	{ 1274, "Xe_LPG+", &graphics_xelpg },
> +	{ 2001, "Xe2_HPG", &graphics_xe2 },
> +	{ 2004, "Xe2_LPG", &graphics_xe2 },
> +	{ 3000, "Xe3_LPG", &graphics_xe2 },
> +	{ 3001, "Xe3_LPG", &graphics_xe2 },
>  };
>  
>  /* Map of GMD_ID values to media IP */
>  static const struct gmdid_map media_ip_map[] = {
> -	{ 1300, &media_xelpmp },
> -	{ 1301, &media_xe2 },
> -	{ 2000, &media_xe2 },
> -	{ 3000, &media_xe2 },
> +	{ 1300, "Xe_LPM+", &media_xelpmp },
> +	{ 1301, "Xe2_HPM", &media_xe2 },
> +	{ 2000, "Xe2_LPM", &media_xe2 },
> +	{ 3000, "Xe3_LPM", &media_xe2 },

Can we actually point all of these at media_xelpmp now?  I think early
on there was a difference related to GSC, which is why we had two
different descriptors, but now that Xe2 and beyond have the GSC enabled
I believe the two structures are identical.

Aside from that,

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

>  };
>  
>  /*
> @@ -592,8 +587,8 @@ static void handle_gmdid(struct xe_device *xe,
>  	for (int i = 0; i < ARRAY_SIZE(graphics_ip_map); i++) {
>  		if (ver == graphics_ip_map[i].ver) {
>  			xe->info.graphics_verx100 = ver;
> +			xe->info.graphics_name = graphics_ip_map[i].name;
>  			*graphics = graphics_ip_map[i].ip;
> -			xe->info.graphics_name = (*graphics)->name;
>  
>  			break;
>  		}
> @@ -614,8 +609,8 @@ static void handle_gmdid(struct xe_device *xe,
>  	for (int i = 0; i < ARRAY_SIZE(media_ip_map); i++) {
>  		if (ver == media_ip_map[i].ver) {
>  			xe->info.media_verx100 = ver;
> +			xe->info.media_name = media_ip_map[i].name;
>  			*media = media_ip_map[i].ip;
> -			xe->info.media_name = (*media)->name;
>  
>  			break;
>  		}
> diff --git a/drivers/gpu/drm/xe/xe_pci_types.h b/drivers/gpu/drm/xe/xe_pci_types.h
> index b96423844952..8e586d02d089 100644
> --- a/drivers/gpu/drm/xe/xe_pci_types.h
> +++ b/drivers/gpu/drm/xe/xe_pci_types.h
> @@ -39,6 +39,7 @@ struct xe_media_desc {
>  
>  struct gmdid_map {
>  	unsigned int ver;
> +	const char *name;
>  	const void *ip;
>  };
>  
> -- 
> 2.48.1
> 

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