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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Sep 13, 2024 at 07:01:56PM -0500, Lucas De Marchi wrote:
> I was looking at some possible way to split this so we could have it
> for 6.12, but I don't see a way. Even if this was 1 or 2 weeks ago, it'd
> be hard since the first 3 patches are going through the tree.
> 
> So... I think this will have to wait 6.12 to be backmerged in
> drm-xe-next. Cc'ing Rodrigo and Thomas to know if they see it
> differently.

I believe this could be added along with the vsec patches in the next
merge window.
I even had acked the patch to be there. But not sure if it was picked by
the vsec maintainers or not.

> 
> Some additional review below.
> 
> On Thu, Sep 12, 2024 at 02:42:09PM GMT, 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.
> > 
> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> > ---
> > drivers/gpu/drm/xe/Makefile          |   1 +
> > drivers/gpu/drm/xe/xe_device.c       |   5 +
> > drivers/gpu/drm/xe/xe_device_types.h |   6 +
> > drivers/gpu/drm/xe/xe_vsec.c         | 223 +++++++++++++++++++++++++++
> > drivers/gpu/drm/xe/xe_vsec.h         |  13 ++
> > 5 files changed, 248 insertions(+)
> > create mode 100644 drivers/gpu/drm/xe/xe_vsec.c
> > create mode 100644 drivers/gpu/drm/xe/xe_vsec.h
> > 
> > diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> > index edfd812e0f41..ce4da2b30db3 100644
> > --- a/drivers/gpu/drm/xe/Makefile
> > +++ b/drivers/gpu/drm/xe/Makefile
> > @@ -109,6 +109,7 @@ xe-y += xe_bb.o \
> > 	xe_vm.o \
> > 	xe_vram.o \
> > 	xe_vram_freq.o \
> > +	xe_vsec.o \
> > 	xe_wait_user_fence.o \
> > 	xe_wa.o \
> > 	xe_wopcm.o
> > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> > index 4d3c794f134c..c3edb0f36d05 100644
> > --- a/drivers/gpu/drm/xe/xe_device.c
> > +++ b/drivers/gpu/drm/xe/xe_device.c
> > @@ -54,6 +54,7 @@
> > #include "xe_ttm_sys_mgr.h"
> > #include "xe_vm.h"
> > #include "xe_vram.h"
> > +#include "xe_vsec.h"
> > #include "xe_wait_user_fence.h"
> > #include "xe_wa.h"
> > 
> > @@ -373,6 +374,8 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
> > 		goto err;
> > 	}
> > 
> > +	drmm_mutex_init(&xe->drm, &xe->pmt.lock);
> > +
> > 	err = xe_display_create(xe);
> > 	if (WARN_ON(err))
> > 		goto err;
> > @@ -759,6 +762,8 @@ int xe_device_probe(struct xe_device *xe)
> > 	for_each_gt(gt, xe, id)
> > 		xe_gt_sanitize_freq(gt);
> > 
> > +	xe_vsec_init(xe);
> > +
> > 	return devm_add_action_or_reset(xe->drm.dev, xe_device_sanitize, xe);
> > 
> > err_fini_display:
> > diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> > index c92df0a2423f..cf37af207210 100644
> > --- a/drivers/gpu/drm/xe/xe_device_types.h
> > +++ b/drivers/gpu/drm/xe/xe_device_types.h
> > @@ -487,6 +487,12 @@ struct xe_device {
> > 		struct mutex lock;
> > 	} d3cold;
> > 
> > +	/** @pmt: Support the PMT driver callback interface */
> > +	struct {
> > +		/** @pmt.lock: protect access for telemetry data */
> > +		struct mutex lock;
> > +	} pmt;
> > +
> > 	/**
> > 	 * @pm_callback_task: Track the active task that is running in either
> > 	 * the runtime_suspend or runtime_resume callbacks.
> > diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> > new file mode 100644
> > index 000000000000..1d82ef980b80
> > --- /dev/null
> > +++ b/drivers/gpu/drm/xe/xe_vsec.c
> > @@ -0,0 +1,223 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright © 2022 - 2024 Intel Corporation
> 
> just 2024 should be fine.
> 
> > + */
> > +#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>
> > +
> > +#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_OFFSET	0xDB000
> > +#define BMG_DISCOVERY_OFFSET	(SOC_BASE + BMG_PMT_BASE_OFFSET)
> > +
> > +#define BMG_TELEMETRY_BASE_OFFSET	0xE0000
> > +#define BMG_TELEMETRY_OFFSET		(SOC_BASE + BMG_TELEMETRY_BASE_OFFSET)
> > +
> > +#define BMG_DEVICE_ID 0xE2F8
> > +
> > +#define SG_REMAP_INDEX1		XE_REG(SOC_BASE + 0x08)
> > +#define SG_REMAP_BITS		GENMASK(31, 24)
> 
> we should probably move these to regs/xe_soc.h or regs/xe_vsec.h or
> regs/xe_pmt.h. Then please follow the same format with added spaces for
> the bit definition.
> 
> I don't like the defines here since it's hidden and it's often accessing
> the register in a weird way.
> 
> 
> > +
> > +static struct intel_vsec_header bmg_telemetry = {
> 
> we prefer prefixes with the IP rather than the platform. Is it possible
> here?
> 
> > +	.length = 0x10,
> > +	.id = VSEC_ID_TELEMETRY,
> > +	.num_entries = 2,
> > +	.entry_size = 4,
> > +	.tbir = 0,
> > +	.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,..)}
> 
> what's the meaning of the X here and in the lines below?
> Maybe just document line by line below
> 
> > + * 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 (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;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	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;
> > +	struct xe_mmio *mmio;
> > +	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 */
> > +	mmio = &xe_device_get_root_tile(xe)->primary_gt->mmio;
> 
> this doesn't look right. We recently moved away from using gt as the
> target of all mmio operations. This seems more like tile->mmio.
> Cc Matt Roper.
> 
> 
> > +	xe_mmio_rmw32(mmio, SG_REMAP_INDEX1, SG_REMAP_BITS,
> > +		      FIELD_PREP(SG_REMAP_BITS, mem_region));
> > +
> > +	memcpy_fromio(data, telem_addr, count);
> > +	xe_pm_runtime_put(xe);
> > +
> > +	return count;
> > +}
> > +
> > +struct pmt_callbacks xe_pmt_cb = {
> > +	.read_telem = xe_pmt_telem_read,
> > +};
> > +
> > +static const int vsec_platforms[] = {
> > +	[XE_BATTLEMAGE] = XE_VSEC_BMG,
> > +};
> > +
> > +static enum xe_vsec get_platform_info(struct xe_device *xe)
> > +{
> > +	if (xe->info.platform > XE_BATTLEMAGE)
> > +		return XE_VSEC_UNKNOWN;
> > +
> > +	return vsec_platforms[xe->info.platform];
> > +}
> > +
> > +/**
> > + * xe_vsec_init - Initialize resources and add intel_vsec auxiliary
> > + * interface
> > + * @xe: valid xe instance
> > + */
> > +void xe_vsec_init(struct xe_device *xe)
> > +{
> > +	struct intel_vsec_platform_info *info;
> > +	struct device *dev = xe->drm.dev;
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +	enum xe_vsec platform;
> > +
> > +	platform = get_platform_info(xe);
> > +	if (platform == XE_VSEC_UNKNOWN)
> > +		return;
> > +
> > +	info = &xe_vsec_info[platform];
> > +	if (!info->headers)
> > +		return;
> > +
> > +	switch (platform) {
> > +	case XE_VSEC_BMG:
> > +		info->priv_data = &xe_pmt_cb;
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	/*
> > +	 * Register a VSEC. Cleanup is handled using device managed
> > +	 * resources.
> > +	 */
> > +	intel_vsec_register(pdev, info);
> > +}
> > +MODULE_IMPORT_NS(INTEL_VSEC);
> > diff --git a/drivers/gpu/drm/xe/xe_vsec.h b/drivers/gpu/drm/xe/xe_vsec.h
> > new file mode 100644
> > index 000000000000..3fd29a21cad6
> > --- /dev/null
> > +++ b/drivers/gpu/drm/xe/xe_vsec.h
> > @@ -0,0 +1,13 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright © 2022 - 2024 Intel Corporation
> 
> just 2024 should be fine.
> 
> Lucas DE Marchi
> 
> > + */
> > +
> > +#ifndef _XE_VSEC_H_
> > +#define _XE_VSEC_H_
> > +
> > +struct xe_device;
> > +
> > +void xe_vsec_init(struct xe_device *xe);
> > +
> > +#endif
> > -- 
> > 2.44.0
> >