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) Thanks. -- Ashutosh
