[PATCH v11] drm/xe/vsec: Support BMG devices

[Ruhl, Michael J]

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko at linux.intel.com>
> Sent: Tuesday, August 13, 2024 10:11 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; ilpo.jarvinen at linux.intel.com; Brost, Matthew
> <matthew.brost at intel.com>; hdegoede at redhat.com; Vivi, Rodrigo
> <rodrigo.vivi at intel.com>
> Subject: Re: [PATCH v11] drm/xe/vsec: Support BMG devices
> 
> On Mon, Aug 12, 2024 at 04:04:22PM -0400, Michael J. Ruhl wrote:
> > The Battlemage (BMG) discrete graphics card supports the Platform,
> > Monitoring Technology (PMT) feature directly on the primary PCI
> > device.
> >
> > Utilize the PMT callback API to add support for the BMG devices.
> 
> ...
> 
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> > +#include <linux/cleanup.h>
> > +#include <linux/intel_vsec.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/pci.h>
> 
> ...
> 
> > +#define SOC_BASE		0x280000
> > +
> > +#define BMG_PMT_BASE		0xDB000
> > +#define BMG_DISCOVERY_OFFSET	(SOC_BASE + BMG_PMT_BASE)
> 
> > +#define BMG_TELEMETRY_BASE	0xE0000
> > +#define BMG_TELEMETRY_OFFSET	(SOC_BASE + BMG_TELEMETRY_BASE)
> 
> This looks like double indirection.
> Wouldn't suffix _BASE_OFFSET be better for PMT and TELEMETRY cases?

I am not sure I understand.

Are  you saying rename BMG_PMT_BASE to BMG_PMT_BASE_OFFSET?

> 
> ...
> 
> > +#define BMG_DEVICE_ID 0xE2F8
> 
> Is this defined in any specification? I mean is the format the same as PCI device
> ID?

I think that this is defined in BMG PMT yaml definition.  It is provide in the PMT discovery
data, so it is defined by the specific device. 


> ...
> 
> > +#define GFX_BAR			0
> 
> Do you need a separate definition for this?

Guess not.  Will remove. 😊

> ...
> 
> > +enum record_id {
> > +	PUNIT,
> > +	OOBMSM_0,
> > +	OOBMSM_1
> 
> Trailing comma?
> 
> > +};
> > +
> > +enum capability {
> > +	CRASHLOG,
> > +	TELEMETRY,
> > +	WATCHER
> 
> Ditto?

Will update.
 
> > +};
> 
> ...
> 
> > +	switch (record_id) {
> > +	case PUNIT:
> > +		*index = 0;
> > +		if (cap_type == TELEMETRY)
> > +			*offset = PUNIT_TELEMETRY_OFFSET;
> > +		else
> > +			*offset = PUNIT_WATCHER_OFFSET;
> > +		break;
> > +
> > +	case OOBMSM_0:
> > +		*index = 1;
> > +		if (cap_type == WATCHER)
> > +			*offset = OOBMSM_0_WATCHER_OFFSET;
> > +		break;
> > +
> > +	case OOBMSM_1:
> > +		*index = 1;
> > +		if (cap_type == TELEMETRY)
> > +			*offset = OOBMSM_1_TELEMETRY_OFFSET;
> > +		break;
> 
> default case?

I validate the record_id and cap_type values at the beginning of the function,
so default would be redundant.

The goal was to validate, then set data.

So adding the default will remove the record_id check from the if.  Do you prefer
that path?
 
> > +	}
> 
> ...
> 
> > +static int xe_pmt_telem_read(struct pci_dev *pdev, u32 guid, u64
> > +*data, u32 count) {
> > +	struct xe_device *xe = pdev_to_xe_device(pdev);
> > +	void __iomem *telem_addr = xe->mmio.regs +
> BMG_TELEMETRY_OFFSET;
> > +	u32 mem_region;
> > +	u32 offset;
> > +	int ret;
> > +
> > +	ret = guid_decode(guid, &mem_region, &offset);
> > +	if (ret)
> > +		return ret;
> > +
> > +	telem_addr += offset;
> > +
> > +	guard(mutex)(&xe->pmt.lock);
> > +
> > +	/* indicate that we are not at an appropriate power level */
> > +	if (!xe_pm_runtime_get_if_active(xe))
> > +		return -ENODATA;
> > +
> > +	/* set SoC re-mapper index register based on GUID memory region */
> > +	xe_mmio_rmw32(xe->tiles[0].primary_gt, SG_REMAP_INDEX1,
> SG_REMAP_BITS,
> > +		      FIELD_PREP(SG_REMAP_BITS, mem_region));
> > +
> > +	memcpy_fromio(data, telem_addr, count);
> 
> > +	ret = count;
> > +	xe_pm_runtime_put(xe);
> 
> Does this have a side effect on count? If yes, a comment, if no, you may return
> count directly.

It does not.  I was looking at that yesterday (when I updated with the guard()) and 
thought about updating.  I will update.

Thank you for your comments!

M

> 
> > +	return ret;
> > +}
> 
> --
> With Best Regards,
> Andy Shevchenko
>