Re: [PATCH 06/16] drm/xe/oa/uapi: Define and parse OA stream properties

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

On Thu, 08 Feb 2024 13:40:29 -0800, Lionel Landwerlin wrote:

Hi Lionel,

> +
> +	/** @DRM_XE_OA_PROPERTY_OA_FORMAT: Perf counter report format */
> +	DRM_XE_OA_PROPERTY_OA_FORMAT,
> +	/**
> +	 * OA_FORMAT's are specified the same way as in Bspec, in terms of
> +	 * the following quantities: a. enum @drm_xe_oa_format_type
> +	 * b. Counter select c. Counter size and d. BC report
> +	 */
> +#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)
>
> People outside of Intel don't have access to the BSpec.

Hmm, I was assuming Bspec is public, at least parts of it. Since we keep
dropping Bspec references in patch commit messages?

>
> And since there is no page number either

Page numbers are in the commit message, but you are right, they should be
added here.

> , it would just be easier for everybody to say :
>
>      "Refer to the oa_formats array in drivers/gpu/drm/xe/xe_oa.c"

Umesh, what do you think about this? I don't like the idea too much, of
referring to the internal implementation in the uapi, but if Bspec is not
public, and we want to keep this uapi, we'll probably need to do this.

Also, we are directly returning the oa_status register in response to
DRM_XE_PERF_IOCTL_STATUS ioctl (see 'struct drm_xe_oa_stream_status'), so
that also needs access to Bspec. But there I think we can just document the
relevant bits in xe_drm.h.

Thanks.
--
Ashutosh