[PATCH v13 4/4] drm/xe/vsec: Support BMG devices
Ruhl, Michael J
michael.j.ruhl at intel.com
Mon Sep 16 15:30:24 UTC 2024
> -----Original Message-----
> From: De Marchi, Lucas <lucas.demarchi at intel.com>
> Sent: Friday, September 13, 2024 8:02 PM
> To: Ruhl, Michael J <michael.j.ruhl at intel.com>
> Cc: intel-xe at lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi at intel.com>;
> thomas.hellstrom at linux.intel.com
> Subject: Re: [PATCH v13 4/4] drm/xe/vsec: Support BMG devices
>
> 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.
will do.
> >+ */
> >+#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.
Ok, I think xe_vsec.h makes the most sense. I will move them.
>
> >+
> >+static struct intel_vsec_header bmg_telemetry = {
>
> we prefer prefixes with the IP rather than the platform. Is it possible here?
I am ok with that but this is for the BMG devices. I am not clear on what the IP
mapping would be. If you have a suggestion?
> >+ .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
I think that it is the "value X" has 4 bits. (this was from the GUID definition
documentation.)
> >+ * 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.
I figure this out and get it cleaned up.
Thanks,
M
>
> >+ 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