On Sat, 08 Jun 2024 03:54:16 -0700, Michal Wajdeczko wrote:
>
Hi Michal,
> On 07.06.2024 22:43, Ashutosh Dixit wrote:
> > Add and initialize supported OA data formats for various platforms
> > (including Xe2). User can request OA data in any supported format.
> >
> > Bspec: 52198, 60942, 61101
> >
> > v2: Start 'xe_oa_format_name' enum from 0 (Umesh)
> > Fix error rewind with OA (Umesh)
> > v3: Use graphics versions rather than absolute platform names
> >
> > Acked-by: José Roberto de Souza <jose.souza at intel.com>
> > Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa at intel.com>
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit at intel.com>
> > ---
> > drivers/gpu/drm/xe/Makefile | 1 +
> > drivers/gpu/drm/xe/xe_device.c | 12 +++-
> > drivers/gpu/drm/xe/xe_device_types.h | 4 ++
> > drivers/gpu/drm/xe/xe_module.c | 1 +
> > drivers/gpu/drm/xe/xe_oa.c | 101 +++++++++++++++++++++++++++
> > drivers/gpu/drm/xe/xe_oa.h | 16 +++++
> > drivers/gpu/drm/xe/xe_oa_types.h | 76 ++++++++++++++++++++
> > include/uapi/drm/xe_drm.h | 10 +++
> > 8 files changed, 220 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/gpu/drm/xe/xe_oa.c
> > create mode 100644 drivers/gpu/drm/xe/xe_oa.h
> > create mode 100644 drivers/gpu/drm/xe/xe_oa_types.h
> >
> > diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> > index 7a91b318efa5..c3127f10f886 100644
> > --- a/drivers/gpu/drm/xe/Makefile
> > +++ b/drivers/gpu/drm/xe/Makefile
> > @@ -92,6 +92,7 @@ xe-y += xe_bb.o \
> > xe_mmio.o \
> > xe_mocs.o \
> > xe_module.o \
> > + xe_oa.o \
> > xe_pat.o \
> > xe_pci.o \
> > xe_pcode.o \
> > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> > index 31ab04b4bd5c..e225eb09fc17 100644
> > --- a/drivers/gpu/drm/xe/xe_device.c
> > +++ b/drivers/gpu/drm/xe/xe_device.c
> > @@ -41,6 +41,7 @@
> > #include "xe_memirq.h"
> > #include "xe_mmio.h"
> > #include "xe_module.h"
> > +#include "xe_oa.h"
> > #include "xe_pat.h"
> > #include "xe_pcode.h"
> > #include "xe_perf.h"
> > @@ -655,10 +656,14 @@ int xe_device_probe(struct xe_device *xe)
> >
> > xe_heci_gsc_init(xe);
> >
> > - err = xe_display_init(xe);
> > + err = xe_oa_init(xe);
> > if (err)
> > goto err_fini_gt;
> >
> > + err = xe_display_init(xe);
> > + if (err)
> > + goto err_fini_oa;
> > +
> > err = drm_dev_register(&xe->drm, 0);
> > if (err)
> > goto err_fini_display;
> > @@ -674,6 +679,9 @@ int xe_device_probe(struct xe_device *xe)
> > err_fini_display:
> > xe_display_driver_remove(xe);
> >
> > +err_fini_oa:
> > + xe_oa_fini(xe);
> > +
> > err_fini_gt:
> > for_each_gt(gt, xe, id) {
> > if (id < last_gt)
> > @@ -706,6 +714,8 @@ void xe_device_remove(struct xe_device *xe)
> >
> > xe_display_fini(xe);
> >
> > + xe_oa_fini(xe);
> > +
> > xe_heci_gsc_fini(xe);
> >
> > for_each_gt(gt, xe, id)
> > diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> > index 52bc461171d5..185986e1d586 100644
> > --- a/drivers/gpu/drm/xe/xe_device_types.h
> > +++ b/drivers/gpu/drm/xe/xe_device_types.h
> > @@ -17,6 +17,7 @@
> > #include "xe_gt_types.h"
> > #include "xe_lmtt_types.h"
> > #include "xe_memirq_types.h"
> > +#include "xe_oa.h"
> > #include "xe_platform_types.h"
> > #include "xe_pt_types.h"
> > #include "xe_sriov_types.h"
> > @@ -462,6 +463,9 @@ struct xe_device {
> > /** @heci_gsc: graphics security controller */
> > struct xe_heci_gsc heci_gsc;
> >
> > + /** @oa: oa perf counter subsystem */
> > + struct xe_oa oa;
> > +
> > /** @needs_flr_on_fini: requests function-reset on fini */
> > bool needs_flr_on_fini;
> >
> > diff --git a/drivers/gpu/drm/xe/xe_module.c b/drivers/gpu/drm/xe/xe_module.c
> > index 893858a2eea0..4e9f31f11920 100644
> > --- a/drivers/gpu/drm/xe/xe_module.c
> > +++ b/drivers/gpu/drm/xe/xe_module.c
> > @@ -10,6 +10,7 @@
> >
> > #include "xe_drv.h"
> > #include "xe_hw_fence.h"
> > +#include "xe_oa.h"
> > #include "xe_pci.h"
> > #include "xe_perf.h"
> > #include "xe_sched_job.h"
> > diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> > new file mode 100644
> > index 000000000000..3349e645cb72
> > --- /dev/null
> > +++ b/drivers/gpu/drm/xe/xe_oa.c
> > @@ -0,0 +1,101 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright © 2023 Intel Corporation
> > + */
> > +
> > +#include <drm/xe_drm.h>
> > +
> > +#include "xe_assert.h"
> > +#include "xe_device.h"
> > +#include "xe_macros.h"
> > +#include "xe_oa.h"
> > +
> > +#define DRM_FMT(x) DRM_XE_OA_FMT_TYPE_##x
> > +
> > +static const struct xe_oa_format oa_formats[] = {
> > + [XE_OA_FORMAT_C4_B8] = { 7, 64, DRM_FMT(OAG) },
> > + [XE_OA_FORMAT_A12] = { 0, 64, DRM_FMT(OAG) },
> > + [XE_OA_FORMAT_A12_B8_C8] = { 2, 128, DRM_FMT(OAG) },
> > + [XE_OA_FORMAT_A32u40_A4u32_B8_C8] = { 5, 256, DRM_FMT(OAG) },
> > + [XE_OAR_FORMAT_A32u40_A4u32_B8_C8] = { 5, 256, DRM_FMT(OAR) },
> > + [XE_OA_FORMAT_A24u40_A14u32_B8_C8] = { 5, 256, DRM_FMT(OAG) },
> > + [XE_OAC_FORMAT_A24u64_B8_C8] = { 1, 320, DRM_FMT(OAC), HDR_64_BIT },
> > + [XE_OAC_FORMAT_A22u32_R2u32_B8_C8] = { 2, 192, DRM_FMT(OAC), HDR_64_BIT },
> > + [XE_OAM_FORMAT_MPEC8u64_B8_C8] = { 1, 192, DRM_FMT(OAM_MPEC), HDR_64_BIT },
> > + [XE_OAM_FORMAT_MPEC8u32_B8_C8] = { 2, 128, DRM_FMT(OAM_MPEC), HDR_64_BIT },
> > + [XE_OA_FORMAT_PEC64u64] = { 1, 576, DRM_FMT(PEC), HDR_64_BIT, 1, 0 },
> > + [XE_OA_FORMAT_PEC64u64_B8_C8] = { 1, 640, DRM_FMT(PEC), HDR_64_BIT, 1, 1 },
> > + [XE_OA_FORMAT_PEC64u32] = { 1, 320, DRM_FMT(PEC), HDR_64_BIT },
> > + [XE_OA_FORMAT_PEC32u64_G1] = { 5, 320, DRM_FMT(PEC), HDR_64_BIT, 1, 0 },
> > + [XE_OA_FORMAT_PEC32u32_G1] = { 5, 192, DRM_FMT(PEC), HDR_64_BIT },
> > + [XE_OA_FORMAT_PEC32u64_G2] = { 6, 320, DRM_FMT(PEC), HDR_64_BIT, 1, 0 },
> > + [XE_OA_FORMAT_PEC32u32_G2] = { 6, 192, DRM_FMT(PEC), HDR_64_BIT },
> > + [XE_OA_FORMAT_PEC36u64_G1_32_G2_4] = { 3, 320, DRM_FMT(PEC), HDR_64_BIT, 1, 0 },
> > + [XE_OA_FORMAT_PEC36u64_G1_4_G2_32] = { 4, 320, DRM_FMT(PEC), HDR_64_BIT, 1, 0 },
> > +};
> > +
> > +static void oa_format_add(struct xe_oa *oa, enum xe_oa_format_name format)
> > +{
> > + __set_bit(format, oa->format_mask);
> > +}
> > +
> > +static void xe_oa_init_supported_formats(struct xe_oa *oa)
> > +{
> > + if (GRAPHICS_VER(oa->xe) >= 20) {
> > + /* Xe2+ */
> > + oa_format_add(oa, XE_OAM_FORMAT_MPEC8u64_B8_C8);
> > + oa_format_add(oa, XE_OAM_FORMAT_MPEC8u32_B8_C8);
> > + oa_format_add(oa, XE_OA_FORMAT_PEC64u64);
> > + oa_format_add(oa, XE_OA_FORMAT_PEC64u64_B8_C8);
> > + oa_format_add(oa, XE_OA_FORMAT_PEC64u32);
> > + oa_format_add(oa, XE_OA_FORMAT_PEC32u64_G1);
> > + oa_format_add(oa, XE_OA_FORMAT_PEC32u32_G1);
> > + oa_format_add(oa, XE_OA_FORMAT_PEC32u64_G2);
> > + oa_format_add(oa, XE_OA_FORMAT_PEC32u32_G2);
> > + oa_format_add(oa, XE_OA_FORMAT_PEC36u64_G1_32_G2_4);
> > + oa_format_add(oa, XE_OA_FORMAT_PEC36u64_G1_4_G2_32);
> > + } else if (GRAPHICS_VERx100(oa->xe) >= 1270) {
> > + /* XE_METEORLAKE */
> > + oa_format_add(oa, XE_OAR_FORMAT_A32u40_A4u32_B8_C8);
> > + oa_format_add(oa, XE_OA_FORMAT_A24u40_A14u32_B8_C8);
> > + oa_format_add(oa, XE_OAC_FORMAT_A24u64_B8_C8);
> > + oa_format_add(oa, XE_OAC_FORMAT_A22u32_R2u32_B8_C8);
> > + oa_format_add(oa, XE_OAM_FORMAT_MPEC8u64_B8_C8);
> > + oa_format_add(oa, XE_OAM_FORMAT_MPEC8u32_B8_C8);
> > + } else if (GRAPHICS_VERx100(oa->xe) >= 1255) {
> > + /* XE_DG2, XE_PVC */
> > + oa_format_add(oa, XE_OAR_FORMAT_A32u40_A4u32_B8_C8);
> > + oa_format_add(oa, XE_OA_FORMAT_A24u40_A14u32_B8_C8);
> > + oa_format_add(oa, XE_OAC_FORMAT_A24u64_B8_C8);
> > + oa_format_add(oa, XE_OAC_FORMAT_A22u32_R2u32_B8_C8);
> > + } else {
> > + /* Gen12+ */
> > + xe_assert(oa->xe, GRAPHICS_VER(oa->xe) >= 12);
> > + oa_format_add(oa, XE_OA_FORMAT_A12);
> > + oa_format_add(oa, XE_OA_FORMAT_A12_B8_C8);
> > + oa_format_add(oa, XE_OA_FORMAT_A32u40_A4u32_B8_C8);
> > + oa_format_add(oa, XE_OA_FORMAT_C4_B8);
> > + }
> > +}
> > +
> > +int xe_oa_init(struct xe_device *xe)
> > +{
> > + struct xe_oa *oa = &xe->oa;
> > +
> > + /* Support OA only with GuC submission and Gen12+ */
> > + if (XE_WARN_ON(!xe_device_uc_enabled(xe)) || XE_WARN_ON(GRAPHICS_VER(xe) < 12))
> > + return 0;
> > +
> > + oa->xe = xe;
> > + oa->oa_formats = oa_formats;
> > +
> > + xe_oa_init_supported_formats(oa);
> > + return 0;
> > +}
> > +
> > +void xe_oa_fini(struct xe_device *xe)
> > +{
> > + struct xe_oa *oa = &xe->oa;
> > +
> > + oa->xe = NULL;
> > +}
> > diff --git a/drivers/gpu/drm/xe/xe_oa.h b/drivers/gpu/drm/xe/xe_oa.h
> > new file mode 100644
> > index 000000000000..a2f301e2be57
> > --- /dev/null
> > +++ b/drivers/gpu/drm/xe/xe_oa.h
> > @@ -0,0 +1,16 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright © 2023 Intel Corporation
> > + */
> > +
> > +#ifndef _XE_OA_H_
> > +#define _XE_OA_H_
> > +
> > +#include "xe_oa_types.h"
>
> don't include full header if you already have required forward decl
This one is unrelated. xe_oa.h is included in other files such as
xe_device.c and those files need declarations in xe_oa_types.h. So I have
left this as is. This is the same pattern as followed e.g. in
xe_bb.h/xe_bb_types.h, xe_bo.h/xe_bo_types.h, xe_device.h/xe_device_types.h
etc.
>
> > +
> > +struct xe_device;
> > +
> > +int xe_oa_init(struct xe_device *xe);
> > +void xe_oa_fini(struct xe_device *xe);
> > +
> > +#endif
> > diff --git a/drivers/gpu/drm/xe/xe_oa_types.h b/drivers/gpu/drm/xe/xe_oa_types.h
> > new file mode 100644
> > index 000000000000..1e339090c90d
> > --- /dev/null
> > +++ b/drivers/gpu/drm/xe/xe_oa_types.h
> > @@ -0,0 +1,76 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright © 2023 Intel Corporation
> > + */
> > +
> > +#ifndef _XE_OA_TYPES_H_
> > +#define _XE_OA_TYPES_H_
> > +
> > +#include <linux/math.h>
> > +#include <linux/types.h>
> > +
> > +enum xe_oa_report_header {
> > + HDR_32_BIT = 0,
> > + HDR_64_BIT,
> > +};
> > +
> > +enum xe_oa_format_name {
> > + XE_OA_FORMAT_C4_B8,
> > +
> > + /* Gen8+ */
> > + XE_OA_FORMAT_A12,
> > + XE_OA_FORMAT_A12_B8_C8,
> > + XE_OA_FORMAT_A32u40_A4u32_B8_C8,
> > +
> > + /* DG2 */
> > + XE_OAR_FORMAT_A32u40_A4u32_B8_C8,
> > + XE_OA_FORMAT_A24u40_A14u32_B8_C8,
> > +
> > + /* DG2/MTL OAC */
> > + XE_OAC_FORMAT_A24u64_B8_C8,
> > + XE_OAC_FORMAT_A22u32_R2u32_B8_C8,
> > +
> > + /* MTL OAM */
> > + XE_OAM_FORMAT_MPEC8u64_B8_C8,
> > + XE_OAM_FORMAT_MPEC8u32_B8_C8,
> > +
> > + /* Xe2+ */
> > + XE_OA_FORMAT_PEC64u64,
> > + XE_OA_FORMAT_PEC64u64_B8_C8,
> > + XE_OA_FORMAT_PEC64u32,
> > + XE_OA_FORMAT_PEC32u64_G1,
> > + XE_OA_FORMAT_PEC32u32_G1,
> > + XE_OA_FORMAT_PEC32u64_G2,
> > + XE_OA_FORMAT_PEC32u32_G2,
> > + XE_OA_FORMAT_PEC36u64_G1_32_G2_4,
> > + XE_OA_FORMAT_PEC36u64_G1_4_G2_32,
> > +
> > + XE_OA_FORMAT_MAX,
>
> maybe add some prefix like __ to clearly indicate that this is a special
> enumerator not to be used like other above
Done.
>
> > +};
> > +
> > +/** struct xe_oa_format - Format fields for supported OA formats */
> > +struct xe_oa_format {
> > + u32 counter_select;
> > + int size;
> > + int type;
> > + enum xe_oa_report_header header;
> > + u16 counter_size;
> > + u16 bc_report;
>
> missing kernel-docs for members
Added.
>
> > +};
> > +
> > +/**
> > + * struct xe_oa - OA device level information
> > + */
> > +struct xe_oa {
> > + /** @xe: back pointer to xe device */
> > + struct xe_device *xe;
> > +
> > + /** @oa_formats: tracks all OA formats across platforms */
> > + const struct xe_oa_format *oa_formats;
> > +
> > +#define FORMAT_MASK_SIZE DIV_ROUND_UP(XE_OA_FORMAT_MAX - 1, BITS_PER_LONG)
>
> hmm, are you sure this -1 is correct ?
> and maybe all you need is BITS_TO_LONGS ?
Changed to BITS_TO_LONGS(__XE_OA_FORMAT_MAX).
>
> > +
> > + /** @format_mask: tracks valid OA formats for a platform */
> > + unsigned long format_mask[FORMAT_MASK_SIZE];
> > +};
> > +#endif
> > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > index 03df64600aa0..bb87bf0c96f9 100644
> > --- a/include/uapi/drm/xe_drm.h
> > +++ b/include/uapi/drm/xe_drm.h
> > @@ -1433,6 +1433,16 @@ enum drm_xe_perf_ioctls {
> > DRM_XE_PERF_IOCTL_INFO = _IO('i', 0x4),
> > };
> >
> > +/** enum drm_xe_oa_format_type - OA format types */
>
> it doesn't look like rest of top-level kernel-doc
>
> and all below enumerators are undocumented
Done.
>
> > +enum drm_xe_oa_format_type {
> > + DRM_XE_OA_FMT_TYPE_OAG,
> > + DRM_XE_OA_FMT_TYPE_OAR,
> > + DRM_XE_OA_FMT_TYPE_OAM,
> > + DRM_XE_OA_FMT_TYPE_OAC,
> > + DRM_XE_OA_FMT_TYPE_OAM_MPEC,
> > + DRM_XE_OA_FMT_TYPE_PEC,
> > +};
> > +
> > #if defined(__cplusplus)
> > }
> > #endif
Thanks.
--
Ashutosh