Re: [PATCH 05/17] drm/xe/oa/uapi: Initialize OA units

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Tue, 19 Dec 2023 08:11:58 -0800, Umesh Nerlige Ramappa wrote:
>

Hi Umesh,

> > diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> > index 11662a81ef6d8..5ad3c9c78b4e9 100644
> > --- a/drivers/gpu/drm/xe/xe_oa.c
> > +++ b/drivers/gpu/drm/xe/xe_oa.c
> > @@ -5,7 +5,10 @@
> >
> > #include <linux/sysctl.h>
> >
> > +#include "regs/xe_oa_regs.h"
> > #include "xe_device.h"
> > +#include "xe_gt.h"
> > +#include "xe_mmio.h"
> > #include "xe_oa.h"
> >
> > static int xe_oa_sample_rate_hard_limit;
> > @@ -13,6 +16,13 @@ static u32 xe_oa_max_sample_rate = 100000;
> >
> > static struct ctl_table_header *sysctl_header;
> >
> > +enum {
> > +	XE_OA_UNIT_OAG = 0,
> > +	XE_OA_UNIT_OAM_SAMEDIA_0 = 0,
> > +	XE_OA_UNIT_MAX,
> > +	XE_OA_UNIT_INVALID = U32_MAX,
> > +};
>
> Right now, I think the enum is not needed since we are only defining 0.

Removed in v8. There was my spiel on this part in i915 too:

https://patchwork.freedesktop.org/patch/527601/?series=115330&rev=1#comment_955388

> > +
> > #define DRM_FMT(x) DRM_XE_OA_FMT_TYPE_##x
> >
> > static const struct xe_oa_format oa_formats[] = {
> > @@ -37,6 +47,143 @@ static const struct xe_oa_format oa_formats[] = {
> >	[XE_OA_FORMAT_PEC36u64_G1_4_G2_32]	= { 4, 320, DRM_FMT(PEC), HDR_64_BIT, 1, 0 },
> > };
> >
> > +static u32 num_oa_units_per_gt(struct xe_gt *gt)
> > +{
> > +	return 1;
> > +}
> > +
> > +static u32 __hwe_oam_unit(struct xe_hw_engine *hwe)
> > +{
> > +	if (GRAPHICS_VERx100(gt_to_xe(hwe->gt)) >= 1270) {
> > +		/*
> > +		 * There's 1 SAMEDIA gt and 1 OAM per SAMEDIA gt. All media slices
> > +		 * within the gt use the same OAM. All MTL/LNL SKUs list 1 SA MEDIA
> > +		 */
> > +		drm_WARN_ON(&gt_to_xe(hwe->gt)->drm,
> > +			    hwe->gt->info.type != XE_GT_TYPE_MEDIA);
> > +
> > +		return XE_OA_UNIT_OAM_SAMEDIA_0;
> > +	}
> > +
> > +	return XE_OA_UNIT_INVALID;
> > +}
> > +
> > +static u32 __hwe_oa_unit(struct xe_hw_engine *hwe)
> > +{
> > +	switch (hwe->class) {
> > +	case XE_ENGINE_CLASS_RENDER:
> > +	case XE_ENGINE_CLASS_COMPUTE:
> > +		return XE_OA_UNIT_OAG;
> > +
> > +	case XE_ENGINE_CLASS_VIDEO_DECODE:
> > +	case XE_ENGINE_CLASS_VIDEO_ENHANCE:
> > +		return __hwe_oam_unit(hwe);
> > +
> > +	default:
> > +		return XE_OA_UNIT_INVALID;
> > +	}
> > +}
> > +
> > +static struct xe_oa_regs __oam_regs(u32 base)
> > +{
> > +	return (struct xe_oa_regs) {
> > +		base,
> > +		OAM_HEAD_POINTER(base),
> > +		OAM_TAIL_POINTER(base),
> > +		OAM_BUFFER(base),
> > +		OAM_CONTEXT_CONTROL(base),
> > +		OAM_CONTROL(base),
> > +		OAM_DEBUG(base),
> > +		OAM_STATUS(base),
> > +		OAM_CONTROL_COUNTER_SEL_MASK,
> > +	};
> > +}
> > +
> > +static struct xe_oa_regs __oag_regs(void)
> > +{
> > +	return (struct xe_oa_regs) {
> > +		0,
> > +		OAG_OAHEADPTR,
> > +		OAG_OATAILPTR,
> > +		OAG_OABUFFER,
> > +		OAG_OAGLBCTXCTRL,
> > +		OAG_OACONTROL,
> > +		OAG_OA_DEBUG,
> > +		OAG_OASTATUS,
> > +		OAG_OACONTROL_OA_COUNTER_SEL_MASK,
> > +	};
> > +}
> > +
> > +static void __xe_oa_init_oa_units(struct xe_gt *gt)
> > +{
> > +	const u32 mtl_oa_base[] = {
> > +		[XE_OA_UNIT_OAM_SAMEDIA_0] = 0x393000,
>
> The base can also be 0x13000 because intel_uncore will automagically add
> 0x380000. I prefer 0x13000 so that the media related mmio adjustments
> happen in one place - intel_uncore. For functionality, it doesn't matter.

Changed to 0x13000, since you are right, GSI steering will take care of it.

> > +	};
> > +	int i, num_units = gt->oa.num_oa_units;
> > +
> > +	for (i = 0; i < num_units; i++) {
> > +		struct xe_oa_unit *u = &gt->oa.oa_unit[i];
> > +
> > +		if (i == XE_OA_UNIT_OAG && gt->info.type != XE_GT_TYPE_MEDIA) {
>
> This is where I feel enum can be dropped since decision can solely be made
> with gt->info.type.

Done.

>
> > +			u->regs = __oag_regs();
> > +			u->type = DRM_XE_OA_UNIT_TYPE_OAG;
> > +		} else if (GRAPHICS_VERx100(gt_to_xe(gt)) >= 1270) {
> > +			u->regs = __oam_regs(mtl_oa_base[i]);
> > +			u->type = DRM_XE_OA_UNIT_TYPE_OAM;
> > +		}
> > +
> > +		/* Set oa_unit_ids now to ensure ids remain contiguous */
> > +		u->oa_unit_id = gt_to_xe(gt)->oa.oa_unit_ids++;
> > +	}
> > +}
> > +
>
> All the above are minor comments, so with or without those addressed, this
> is
>
> Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

Thanks.
--
Ashutosh