[PATCH v13 4/4] drm/xe/vsec: Support BMG devices
Lucas De Marchi
lucas.demarchi at intel.com
Sat Sep 14 00:01:56 UTC 2024
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.
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 at linux.intel.com>
>Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>Signed-off-by: Michael J. Ruhl <michael.j.ruhl at 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
>
More information about the Intel-xe
mailing list