[Intel-xe] [PATCH 3/7] drm/xe: Use media base for GMD_ID access

[Michal Wajdeczko]

On 29.04.2023 08:23, Lucas De Marchi wrote:
> Instead of adding a hardcoded base, define GMD_ID() with a base
> argument and use it in all places.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> ---
>  drivers/gpu/drm/xe/regs/xe_gt_regs.h | 4 +++-
>  drivers/gpu/drm/xe/xe_pci.c          | 9 +++++----
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> index 4d87f1fe010d..da7b6d2c7e01 100644
> --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> @@ -8,6 +8,8 @@
>  
>  #include "regs/xe_reg_defs.h"
>  
> +#define MTL_MEDIA_GT_BASE			0x380000

maybe for completeness (and to avoid using anonymous 0 offset in other
places) we should define also:

#define GRAPHICS_GT_BASE			0x0

> +
>  /* RPM unit config (Gen8+) */
>  #define RPM_CONFIG0					XE_REG(0xd00)
>  #define   RPM_CONFIG0_CRYSTAL_CLOCK_FREQ_MASK		REG_GENMASK(5, 3)
> @@ -21,7 +23,7 @@
>  #define FORCEWAKE_ACK_MEDIA_VEBOX(n)		XE_REG(0xd70 + (n) * 4)
>  #define FORCEWAKE_ACK_RENDER			XE_REG(0xd84)
>  
> -#define GMD_ID					XE_REG(0xd8c)
> +#define GMD_ID(base)				XE_REG((base) + 0xd8c)

this register is not the only one that's has it's counterpart at this
0x38000 MEDIA offset, and other registers we are treating in automatic
way, without the need to explicitly pass the 'base', so I'm not sure we
should change that here

>  #define   GMD_ID_ARCH_MASK			REG_GENMASK(31, 22)
>  #define   GMD_ID_RELEASE_MASK			REG_GENMASK(21, 14)
>  #define   GMD_ID_STEP				REG_GENMASK(5, 0)

btw, is there a plan to s/REG_GENMASK/XE_REG_GENMASK or something ?

> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> index 35dcb8781f2a..8687e51cb0a4 100644
> --- a/drivers/gpu/drm/xe/xe_pci.c
> +++ b/drivers/gpu/drm/xe/xe_pci.c
> @@ -276,7 +276,7 @@ static const struct xe_gt_desc xelpmp_gts[] = {
>  		.type = XE_GT_TYPE_MEDIA,
>  		.vram_id = 0,
>  		.mmio_adj_limit = 0x40000,
> -		.mmio_adj_offset = 0x380000,
> +		.mmio_adj_offset = MTL_MEDIA_GT_BASE,
>  	},
>  };
>  
> @@ -391,8 +391,9 @@ find_subplatform(const struct xe_device *xe, const struct xe_device_desc *desc)
>  	return NULL;
>  }
>  
> -static u32 peek_gmdid(struct xe_device *xe, u32 gmdid_offset)
> +static u32 peek_gmdid(struct xe_device *xe, struct xe_reg gmdid_reg)

better to keep u32 but defined as new 'base' for the GMD_ID:

peek_gmdid(xe, GRAPHICS_GT_BASE)
peek_gmdid(xe, MTL_MEDIA_GT_BASE)

then since we parse the value according to the GMDID definition we will
prevent passing wrong register offset and always refer to the right
GMD_ID address inside the function:

static u32 peek_gmdid(struct xe_device *xe, u32 base)
{
	xe_reg gmdid = XE_REG(GMD_ID.addr + base);
...
	WARN_ON(base != GRAPHICS_GT_BASE && base != MTL_MEDIA_GT_BASE);
...
  	map = pci_iomap_range(pdev, 0, gmdid.addr, sizeof(u32));
...
	REG_FIELD_GET(GMD_ID_ARCH_MASK, value)


Michal

>  {
> +	u32 gmdid_offset = gmdid_reg.reg;
>  	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
>  	void __iomem *map = pci_iomap_range(pdev, 0, gmdid_offset, sizeof(u32));
>  	u32 ver;
> @@ -441,7 +442,7 @@ static void handle_gmdid(struct xe_device *xe,
>  {
>  	u32 ver;
>  
> -	ver = peek_gmdid(xe, GMD_ID.reg);
> +	ver = peek_gmdid(xe, GMD_ID(0));
>  	for (int i = 0; i < ARRAY_SIZE(graphics_ip_map); i++) {
>  		if (ver == graphics_ip_map[i].ver) {
>  			xe->info.graphics_verx100 = ver;
> @@ -456,7 +457,7 @@ static void handle_gmdid(struct xe_device *xe,
>  			ver / 100, ver % 100);
>  	}
>  
> -	ver = peek_gmdid(xe, GMD_ID.reg + 0x380000);
> +	ver = peek_gmdid(xe, GMD_ID(MTL_MEDIA_GT_BASE));
>  	for (int i = 0; i < ARRAY_SIZE(media_ip_map); i++) {
>  		if (ver == media_ip_map[i].ver) {
>  			xe->info.media_verx100 = ver;