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@xxxxxxxxx>
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
