[Intel-xe] [PATCH v3 10/20] drm/xe: Don't "peek" into GMD_ID

[Lucas De Marchi]

On Wed, Nov 15, 2023 at 01:51:00PM -0800, Matt Roper wrote:
>On Tue, Nov 14, 2023 at 02:02:21PM +0100, Michał Winiarski wrote:
>> Now that MMIO init got moved to device early, we can use regular
>> xe_mmio_read helpers to get to GMD_ID register.
>>
>> Signed-off-by: Michał Winiarski <michal.winiarski at intel.com>
>> ---
>>  drivers/gpu/drm/xe/regs/xe_gt_regs.h |  1 +
>>  drivers/gpu/drm/xe/xe_pci.c          | 35 ++++++++--------------------
>>  2 files changed, 11 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>> index cc27fe8fc3633..6b195115564ce 100644
>> --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>> +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>> @@ -38,6 +38,7 @@
>>  #define   GMD_ID_ARCH_MASK			REG_GENMASK(31, 22)
>>  #define   GMD_ID_RELEASE_MASK			REG_GENMASK(21, 14)
>>  #define   GMD_ID_REVID				REG_GENMASK(5, 0)
>> +#define MEDIA_GMD_ID				XE_REG(GMD_ID.addr + MEDIA_GT_GSI_OFFSET)
>>
>>  #define FORCEWAKE_ACK_GSC			XE_REG(0xdf8)
>>  #define FORCEWAKE_ACK_GT_MTL			XE_REG(0xdfc)
>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>> index a1026b7ee4727..a6c08577b01d2 100644
>> --- a/drivers/gpu/drm/xe/xe_pci.c
>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>> @@ -21,6 +21,7 @@
>>  #include "xe_drv.h"
>>  #include "xe_gt.h"
>>  #include "xe_macros.h"
>> +#include "xe_mmio.h"
>>  #include "xe_module.h"
>>  #include "xe_pci_types.h"
>>  #include "xe_pm.h"
>> @@ -441,29 +442,6 @@ find_subplatform(const struct xe_device *xe, const struct xe_device_desc *desc)
>>  	return NULL;
>>  }
>>
>> -static void peek_gmdid(struct xe_device *xe, u32 gmdid_offset, u32 *ver, u32 *revid)
>> -{
>> -	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
>> -	void __iomem *map = pci_iomap_range(pdev, 0, gmdid_offset, sizeof(u32));
>> -	u32 val;
>> -
>> -	if (!map) {
>> -		drm_err(&xe->drm, "Failed to read GMD_ID (%#x) from PCI BAR.\n",
>> -			gmdid_offset);
>> -		*ver = 0;
>> -		*revid = 0;
>> -
>> -		return;
>> -	}
>> -
>> -	val = ioread32(map);
>> -	pci_iounmap(pdev, map);
>> -
>> -	*ver = REG_FIELD_GET(GMD_ID_ARCH_MASK, val) * 100 +
>> -		REG_FIELD_GET(GMD_ID_RELEASE_MASK, val);
>> -	*revid = REG_FIELD_GET(GMD_ID_REVID, val);
>> -}
>> -
>>  /*
>>   * Pre-GMD_ID platform: device descriptor already points to the appropriate
>>   * graphics descriptor. Simply forward the description and calculate the version
>> @@ -495,9 +473,14 @@ static void handle_gmdid(struct xe_device *xe,
>>  			 u32 *graphics_revid,
>>  			 u32 *media_revid)
>>  {
>> +	struct xe_gt *gt = xe_root_mmio_gt(xe);
>> +	u32 val;
>>  	u32 ver;
>>
>> -	peek_gmdid(xe, GMD_ID.addr, &ver, graphics_revid);
>> +	val = xe_mmio_read32(gt, GMD_ID);
>> +	ver = REG_FIELD_GET(GMD_ID_ARCH_MASK, val) * 100 + REG_FIELD_GET(GMD_ID_RELEASE_MASK, val);
>> +	*graphics_revid = REG_FIELD_GET(GMD_ID_REVID, val);
>> +
>>  	for (int i = 0; i < ARRAY_SIZE(graphics_ip_map); i++) {
>>  		if (ver == graphics_ip_map[i].ver) {
>>  			xe->info.graphics_verx100 = ver;
>> @@ -512,7 +495,9 @@ static void handle_gmdid(struct xe_device *xe,
>>  			ver / 100, ver % 100);
>>  	}
>>
>> -	peek_gmdid(xe, GMD_ID.addr + 0x380000, &ver, media_revid);
>> +	val = xe_mmio_read32(gt, MEDIA_GMD_ID);
>
>It's unfortunate that we have to read the GMD_ID register through the
>wrong GT pointer, but I don't see a good way to avoid it.  Maybe rather
>than defining MEDIA_GMD_ID with the built-in offset (which people may
>try to copy/paste for other register definitions without realizing this
>is a one-time special case), we should just do something like
>
>        struct xe_reg media_gmdid_reg = GMD_ID;
>
>        /*
>         * Since we haven't initialized the media GT yet, we need to
>         * read media's GMD_ID register through the primary GT's
>         * mmio; that means we also need to explicitly add in the
>         * media GSI offset.
>         */
>         media_gmdid_reg.addr += MEDIA_GT_GSI_OFFSET;
>         val = xe_mmio_read32(gt, media_gmdid_reg);


since we already have a new use case for having a peek_gmdid
(https://patchwork.freedesktop.org/series/126609/), maybe rename the
function and leave this logic adding MEDIA_GT_GSI_OFFSET inside that
single function?

with this patch getting a r-b, we already have all the pre-requisites
reviewed so I'd be happy to rebase my series on top.

Lucas De Marchi

>
>
>Matt
>
>> +	ver = REG_FIELD_GET(GMD_ID_ARCH_MASK, val) * 100 + REG_FIELD_GET(GMD_ID_RELEASE_MASK, val);
>> +	*media_revid = REG_FIELD_GET(GMD_ID_REVID, val);
>>
>>  	/* Media may legitimately be fused off / not present */
>>  	if (ver == 0)
>> --
>> 2.42.1
>>
>
>-- 
>Matt Roper
>Graphics Software Engineer
>Linux GPU Platform Enablement
>Intel Corporation