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@xxxxxxxxx> > Signed-off-by: Michael J. Ruhl <michael.j.ruhl@xxxxxxxxx> > --- > 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@xxxxxxxxxxxxxxx> > + /* 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.