Re: [PATCH v9 4/6] drm/xe/vsec: Support BMG devices

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 5801 bytes --]

On Thu, 25 Jul 2024, Michael J. Ruhl wrote:

> Utilize the PMT callback API to add support for the BMG
> devices.

The shortlog and commit message are a bit terse on details what this 
change is about, it's all hidden into the acronyms :-).

> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---

> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> new file mode 100644
> index 000000000000..2c967aaa4072
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_vsec.c
> @@ -0,0 +1,222 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright © 2022 - 2024 Intel Corporation
> + */
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/intel_vsec.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/pci.h>
> +
> +#include "xe_device.h"
> +#include "xe_device_types.h"
> +#include "xe_drv.h"
> +#include "xe_mmio.h"
> +#include "xe_platform_types.h"
> +#include "xe_pm.h"
> +#include "xe_vsec.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)
> +
> +#define BMG_DEVICE_ID 0xE2F8
> +
> +#define GFX_BAR			0
> +
> +#define SG_REMAP_INDEX1		XE_REG(SOC_BASE + 0x08)
> +#define SG_REMAP_BITS		GENMASK(31, 24)
> +
> +static struct intel_vsec_header bmg_telemetry = {
> +	.length = 0x10,
> +	.id = VSEC_ID_TELEMETRY,
> +	.num_entries = 2,
> +	.entry_size = 4,
> +	.tbir = GFX_BAR,
> +	.offset = BMG_DISCOVERY_OFFSET,
> +};
> +
> +static struct intel_vsec_header *bmg_capabilities[] = {
> +	&bmg_telemetry,
> +	NULL
> +};
> +
> +enum xe_vsec {
> +	XE_VSEC_UNKNOWN = 0,
> +	XE_VSEC_BMG,
> +};
> +
> +static struct intel_vsec_platform_info xe_vsec_info[] = {
> +	[XE_VSEC_BMG] = {
> +		.caps = VSEC_CAP_TELEMETRY,
> +		.headers = bmg_capabilities,
> +	},
> +	{ }
> +};
> +
> +/*
> + * The GUID will have the following bits to decode:
> + *
> + * X(4bits) - {Telemetry space iteration number (0,1,..)}
> + * X(4bits) - Segment (SEGMENT_INDEPENDENT-0, Client-1, Server-2)
> + * X(4bits) - SOC_SKU
> + * XXXX(16bits)– Device ID – changes for each down bin SKU’s
> + * X(2bits) - Capability Type (Crashlog-0, Telemetry Aggregator-1, Watcher-2)
> + * X(2bits) - Record-ID (0-PUNIT, 1-OOBMSM_0, 2-OOBMSM_1)
> + */
> +#define GUID_TELEM_ITERATION	GENMASK(3, 0)
> +#define GUID_SEGMENT		GENMASK(7, 4)
> +#define GUID_SOC_SKU		GENMASK(11, 8)
> +#define GUID_DEVICE_ID		GENMASK(27, 12)
> +#define GUID_CAP_TYPE		GENMASK(29, 28)
> +#define GUID_RECORD_ID		GENMASK(31, 30)
> +
> +#define PUNIT_TELEMETRY_OFFSET		0x0200
> +#define PUNIT_WATCHER_OFFSET		0x14A0
> +#define OOBMSM_0_WATCHER_OFFSET		0x18D8
> +#define OOBMSM_1_TELEMETRY_OFFSET	0x1000
> +
> +enum record_id {
> +	PUNIT,
> +	OOBMSM_0,
> +	OOBMSM_1
> +};
> +
> +enum capability {
> +	CRASHLOG,
> +	TELEMETRY,
> +	WATCHER
> +};
> +
> +static int guid_decode(u32 guid, int *index, u32 *offset)
> +{
> +	u32 record_id = FIELD_GET(GUID_RECORD_ID, guid);
> +	u32 cap_type  = FIELD_GET(GUID_CAP_TYPE, guid);
> +	u32 device_id = FIELD_GET(GUID_DEVICE_ID, guid);
> +
> +	if (device_id != BMG_DEVICE_ID)
> +		return -ENODEV;
> +
> +	if (record_id > OOBMSM_1 || cap_type > WATCHER)
> +		return -EINVAL;
> +
> +	*offset = 0;
> +
> +	if (cap_type == CRASHLOG) {
> +		*index = record_id == PUNIT ? 2 : 4;
> +		return 0;
> +	}
> +
> +	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;
> +	}
> +
> +	return 0;
> +}
> +
> +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;
> +
> +	mutex_lock(&xe->pmt.lock);
> +
> +	/* indicate that we are not at an appropriate power level */
> +	ret = -ENODATA;
> +	if (xe_pm_runtime_get_if_active(xe) > 0) {

xe_pm_runtime_get_if_active() returns bool so > 0 looks odd. In fact, 
active > 0 compare is already done by that called function so perhaps you 
mixed up what kind of value is returned by xe_pm_runtime_get_if_active().

Also, I'd restructure this logic with guard & use of reverse logic.

	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;

	... the rest of the code de-indented by one level (minus the 
	mutex_unlock() which is no longer needed because guard() is used).

With those fixed, I think this one is ready to go so after fixing, please 
add:

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>


> +		/* 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);
> +	}
> +	mutex_unlock(&xe->pmt.lock);
> +
> +	return ret;
> +}

-- 
 i.