Re: [Intel-xe] [PATCH 01/10] drm/xe/oa: Introduce OA uapi

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

On Mon, 21 Aug 2023 09:48:33 -0700, Dixit, Ashutosh wrote:
>

Hi Umesh,

About just the 'enum drm_xe_oa_format' below:

> On Tue, 15 Aug 2023 19:14:40 -0700, Umesh Nerlige Ramappa wrote:
> > On Tue, Aug 15, 2023 at 12:29:59PM -0700, Dixit, Ashutosh wrote:
> > > On Tue, 08 Aug 2023 15:59:24 -0700, Umesh Nerlige Ramappa wrote:
> > >>
> > >> On Mon, Aug 07, 2023 at 06:31:50PM -0700, Ashutosh Dixit wrote:
> > >> > +enum drm_xe_oa_format {
> > >> > +	XE_OA_FORMAT_C4_B8 = 7,
> > >> > +
> > >> > +	/* 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,
> > >> > +
> > >> > +	/* MTL OAM */
> > >> > +	XE_OAM_FORMAT_MPEC8u64_B8_C8,
> > >> > +	XE_OAM_FORMAT_MPEC8u32_B8_C8,
> > >> > +
> > >> > +	XE_OA_FORMAT_MAX	    /* non-ABI */
> > >> > +};
> > >>
> > >> If we support a query to get the supported OA formats, we can do away with
> > >> this.
> > >
> > > OK, yeah get using user_ext's.
> > >
> > >> Also thinking if we can provide the format definition in such a way
> > >> that UMD can just decode the counters from it with some generic code. That
> > >> way, we don't have to constantly keep updating UMD code for every new
> > >> platforms (may be far-fetched, but could save us a lot of
> > >> time). Thoughts?
> > >
> > > Ok, but I am not sure how to do this.
> >
> > Just a thought, we can ignore that for now.
>
> I am not sure if exposing valid counter_select values has much use since
> userland has metrics files so already knows supported counter_select
> values. How about not exposing anything at all regarding this in xe_drm.h?
> Userland can just send the counter_select values it already knows about and
> XE can just validate these. LNL etc. need a couple of other fields, so we
> can just define the bitfields XE is expecting and have userspace fill these
> in. XE will validate all of these.
>
> I think we need a little bit of encapsulation (provided the OA format) if
> we remove this, but if it causes problems in uapi updates etc. we can
> probably get rid of it.
>
> Again, assuming we'll change the python scripts etc. for this. Maybe we
> /should/ prefer a clean interface to these userspace modifications?

I think instead of including the enum above as we've been doing, it is
possible to describe the formats in terms of a few quantities. Looking at
Bspec, these would be something like:

- Format Type: OAG/OAR/OAC/OAM/PEC
- Counter Select
- Counter Size (LNL+)
- BC Report (LNL+)

So I think from these (together with the platform (MTL/LNL etc.) we can
figure out what the report format (the oa_formats[] array) should be.

So it is possible I think to replace 'enum drm_xe_oa_format' above with a
struct or a bitfield with these quantities.

I think this will be a little more resilient than the enum above, we won't
have to change xe_drm.h each time a new format is introduced. We will only
have to change xe_drm.h when HW format representation in the OACONTROL
register changes basically. So e.g. if we had this struct/bitfield in
i915_drm.h we would have been ok till LNL at which point we'd have to make
some changes (introduce 'Counter Size' and 'BC Report').

So what do you think? Should we switch to this way of getting the formats
through the uapi and discontinue the enum? Or do you just want to see the
patch? Or any other ideas?

Thanks.
--
Ashutosh