[PATCH v6 6/6] drm/xe/vsec: Add support for DG2

Thu Jul 11 17:44:44 UTC 2024

>-----Original Message-----
>From: Ilpo Järvinen <ilpo.jarvinen at linux.intel.com>
>Sent: Thursday, July 11, 2024 7:45 AM
>To: Ruhl, Michael J <michael.j.ruhl at intel.com>
>Cc: intel-xe at lists.freedesktop.org; platform-driver-x86 at vger.kernel.org;
>david.e.box at linux.intel.com; Brost, Matthew <matthew.brost at intel.com>
>Subject: Re: [PATCH v6 6/6] drm/xe/vsec: Add support for DG2
>
>On Wed, 10 Jul 2024, Michael J. Ruhl wrote:
>
>> DG2 needs to adjust the discovery offset WRT the GT BAR not the
>
>WRT = w.r.t. ?
>
>> P2SB bar so add the base_adjust value to allow for the difference
>> to be used.
>
>Could you please try to write out the problem in clearer terms. Currently
>the reasonale given in patch 5 doesn't even match the one given here or
>it is written in so unclear terms I cannot make the connection.

Yes.  I will right this up better.

>> Update xe_vsec.c to include DG2 header information.
>>
>> Signed-off-by: Michael J. Ruhl <michael.j.ruhl at intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_vsec.c | 81
>++++++++++++++++++++++++++++++++++++
>>  1 file changed, 81 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
>> index 98999d467db1..531ddd32a1a6 100644
>> --- a/drivers/gpu/drm/xe/xe_vsec.c
>> +++ b/drivers/gpu/drm/xe/xe_vsec.c
>> @@ -19,6 +19,16 @@
>>
>>  #define SOC_BASE		0x280000
>>
>> +/* from drivers/platform/x86/intel/pmt/telemetry.c */
>> +#define TELEM_BASE_OFFSET	0x8
>> +
>> +#define DG2_PMT_BASE		0xE8000
>> +#define DG2_DISCOVERY_START	0x6000
>> +#define DG2_TELEM_START		0x4000
>> +
>> +#define DG2_DISCOVERY_OFFSET	(SOC_BASE + DG2_PMT_BASE +
>DG2_DISCOVERY_START)
>> +#define DG2_TELEM_OFFSET	(SOC_BASE + DG2_PMT_BASE +
>DG2_TELEM_START)
>> +
>>  #define BMG_PMT_BASE		0xDB000
>>  #define BMG_DISCOVERY_OFFSET	(SOC_BASE + BMG_PMT_BASE)
>>
>> @@ -32,6 +42,20 @@
>>  #define SG_REMAP_INDEX1		XE_REG(SOC_BASE + 0x08)
>>  #define SG_REMAP_BITS		GENMASK(31, 24)
>>
>> +static struct intel_vsec_header dg2_telemetry = {
>> +	.length = 0x10,
>> +	.id = VSEC_ID_TELEMETRY,
>> +	.num_entries = 1,
>> +	.entry_size = 3,
>> +	.tbir = GFX_BAR,
>> +	.offset = DG2_DISCOVERY_OFFSET,
>> +};
>> +
>> +static struct intel_vsec_header *dg2_capabilities[] = {
>> +	&dg2_telemetry,
>> +	NULL
>> +};
>> +
>>  static struct intel_vsec_header bmg_telemetry = {
>>  	.length = 0x10,
>>  	.id = VSEC_ID_TELEMETRY,
>> @@ -48,10 +72,16 @@ static struct intel_vsec_header *bmg_capabilities[] =
>{
>>
>>  enum xe_vsec {
>>  	XE_VSEC_UNKNOWN = 0,
>> +	XE_VSEC_DG2,
>>  	XE_VSEC_BMG,
>>  };
>>
>>  static struct intel_vsec_platform_info xe_vsec_info[] = {
>> +	[XE_VSEC_DG2] = {
>> +		.caps = VSEC_CAP_TELEMETRY,
>> +		.headers = dg2_capabilities,
>> +		.quirks = VSEC_QUIRK_EARLY_HW,
>> +	},
>>  	[XE_VSEC_BMG] = {
>>  		.caps = VSEC_CAP_TELEMETRY,
>>  		.headers = bmg_capabilities,
>> @@ -174,6 +204,7 @@ struct pmt_callbacks xe_pmt_cb = {
>>  };
>>
>>  static const int vsec_platforms[] = {
>> +	[XE_DG2] = XE_VSEC_DG2,
>>  	[XE_BATTLEMAGE] = XE_VSEC_BMG,
>>  };
>>
>> @@ -185,6 +216,49 @@ static enum xe_vsec get_platform_info(struct
>xe_device *xe)
>>  	return vsec_platforms[xe->info.platform];
>>  }
>>
>> +/*
>> + * Access the DG2 PMT MMIO discovery table
>> + *
>> + * The intel_vsec driver does not typically access the discovery table.
>> + * Instead, it creates a memory resource for the table and passes it
>> + * to the PMT telemetry driver. Each discovery table contains 3 items,
>> + *    - GUID
>> + *    - Telemetry size
>> + *    - Telemetry offset (offset from P2SB BAR, not GT)
>> + *
>> + * For DG2 we know what the telemetry offset is, but we still need to
>> + * use the discovery table to pass the GUID and the size. So figure
>> + * out the difference between the P2SB offset and the GT offset and
>> + * save this so that the telemetry driver can use it to adjust the
>> + * value.
>> + */
>> +static int dg2_adjust_offset(struct pci_dev *pdev, struct device *dev,
>> +			     struct intel_vsec_platform_info *info)
>> +{
>> +	void __iomem *base;
>> +	u32 telem_offset;
>> +	u64 addr;
>> +
>> +	/* compile check to verify that quirk has P2SB quirk added */
>
>I don't know what "quirk" the first instance refers to (I think I
>probably know what the P2SB quirk is).
>
>What is the purpose/meaning of this comment? Is it some leftover for
>code that no longer exists as it talks about "compile check"??

Artifact from my first attempt.

I will remove this and update the comment below (another quirk reference).

Thanks!

M

>--
> i.
>
>> +
>> +	addr = pci_resource_start(pdev, GFX_BAR) + info->headers[0]->offset;
>> +	base = ioremap_wc(addr, 16);
>> +	if (!base)
>> +		return -ENOMEM;
>> +
>> +	telem_offset = readl(base + TELEM_BASE_OFFSET);
>> +
>> +	/* Use the base_addr + P2SB quirk to pass this info */
>> +	if (telem_offset < DG2_TELEM_OFFSET)
>> +		info->base_adjust = -(DG2_TELEM_OFFSET - telem_offset);
>> +	else
>> +		info->base_adjust = -(telem_offset - DG2_TELEM_OFFSET);
>> +
>> +	iounmap(base);
>> +
>> +	return 0;
>> +}
>> +
>>  /**
>>   * xe_vsec_init - Initialize resources and add intel_vsec auxiliary
>>   * interface
>> @@ -196,6 +270,7 @@ void xe_vsec_init(struct xe_device *xe)
>>  	struct device *dev = xe->drm.dev;
>>  	struct pci_dev *pdev = to_pci_dev(dev);
>>  	enum xe_vsec platform;
>> +	u32 ret;
>>
>>  	platform = get_platform_info(xe);
>>  	if (platform == XE_VSEC_UNKNOWN)
>> @@ -206,6 +281,12 @@ void xe_vsec_init(struct xe_device *xe)
>>  		return;
>>
>>  	switch (platform) {
>> +	case XE_VSEC_DG2:
>> +		ret = dg2_adjust_offset(pdev, dev, info);
>> +		if (ret)
>> +			return;
>> +		break;
>> +
>>  	case XE_VSEC_BMG:
>>  		info->priv_data = &xe_pmt_cb;
>>  		break;
>>