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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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?

...

> +#define BMG_DEVICE_ID 0xE2F8

Is this defined in any specification? I mean is the format the same as PCI device ID?

...

> +#define GFX_BAR			0

Do you need a separate definition for this?

...

> +enum record_id {
> +	PUNIT,
> +	OOBMSM_0,
> +	OOBMSM_1

Trailing comma?

> +};
> +
> +enum capability {
> +	CRASHLOG,
> +	TELEMETRY,
> +	WATCHER

Ditto?

> +};

...

> +	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?

> +	}

...

> +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.

> +	return ret;
> +}

-- 
With Best Regards,
Andy Shevchenko