[PATCH] drm/xe/oa/uapi: Make bit masks unsigned

[Lucas De Marchi]

On Mon, Jul 29, 2024 at 02:54:58PM GMT, Ashutosh Dixit wrote:
>On Mon, 29 Jul 2024 06:21:20 -0700, Lucas De Marchi wrote:
>>
>
>Hi Lucas,
>
>> Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>
>>
>> That fixes the build, but question to Ashutosh: it's odd to tie the
>> format to a bspec. What happens on next platform if the HW changes?
>> Hopefully it doesn't change in an incompatible way, but looking at this
>> code it seems we could still keep the uapi by untying the HW from the
>> uapi in the documentation.
>
>IMO, in this case, it is not possible to decouple the formats from Bspec
>because that is where they are specified (in Bspec 52198/60942).
>
>In i915 the OA formats were specified by an enum (enum drm_i915_oa_format),
>but I would argue that enum is meaningful only when we refer back to Bspec.
>Also the i915 enum had to constantly updated when HW added new formats.
>
>For Xe, we changed the way the formats are specified in a way which we
>believe will make the uapi more robust and uapi header update much less
>frequent (hopefully we will never have to update the header and if at all
>we have to, we should be able to do it in a backwards compatible way since
>we have sufficient number of free bits). HW has followed this scheme for
>specifying the formats for years and only recently for Xe2 has added a
>couple of bits and introduced new PEC formats which I think it is not going
>to change now for some time.
>
>But as I said the formats have to refer back to Bspec since that is where
>there are specified and there are too many of them. Any description or enum
>is ambiguous unless it refers back to Bspec. So I don't see how not to
>refer to Bspec in the documentation. If anyone has any ideas about not
>referring to Bspec I am willing to consider it but I think the best way
>forward is to leave the documentation as is:
>
>	/*
>	 * OA_FORMAT's are specified the same way as in PRM/Bspec 52198/60942,
>	 * in terms of the following quantities: a. enum @drm_xe_oa_format_type
>	 * b. Counter select c. Counter size and d. BC report. Also refer to the
>	 * oa_formats array in drivers/gpu/drm/xe/xe_oa.c.
>	 */
>#define DRM_XE_OA_FORMAT_MASK_FMT_TYPE		(0xff << 0)
>#define DRM_XE_OA_FORMAT_MASK_COUNTER_SEL	(0xff << 8)
>#define DRM_XE_OA_FORMAT_MASK_COUNTER_SIZE	(0xff << 16)
>#define DRM_XE_OA_FORMAT_MASK_BC_REPORT		(0xff << 24)

I was under impression that these shifts were something coming from the
HW definition and we were exposing that raw to userspace, rather than
e.g.

struct drm_xe_oa_format {
	__u8 fmt_type;
	__u8 counter_sel;
	__u8 counter_size;
	__u8 bc_report;
	__u32 rsvd;
};

now I see the shifts are not tied to HW and it was only the chosen
format rather than declaring a struct.

Applied this patch to drm-xe-next.

Thanks
Lucas De Marchi

>
>Thanks.
>--
>Ashutosh