> -----Original Message-----
> From: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> Sent: Monday, August 12, 2024 5:01 AM
> To: Ruhl, Michael J <michael.j.ruhl@xxxxxxxxx>
> Cc: intel-xe@xxxxxxxxxxxxxxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx;
> david.e.box@xxxxxxxxxxxxxxx; Brost, Matthew <matthew.brost@xxxxxxxxx>;
> Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>; Vivi, Rodrigo
> <rodrigo.vivi@xxxxxxxxx>
> Subject: Re: [PATCH v9 4/6] drm/xe/vsec: Support BMG devices
>
> 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().
Hi Ilpo,
Yup, the underlying pm_runtime_xxx call can return an error...and I missed the bool
conversion. I will update.
> 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>
The guard is new to me. I will get this updated.
Thank you!
M
>
> > + /* 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.
