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

[Lionel Landwerlin]

On 09/02/2024 00:26, Dixit, Ashutosh wrote:
> 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.


What got me confused is the BC field, which I expected would take some 
non-zero value so Gfx8+ formats (since some of them has B/C counters).

But actually it's zero in xe_oa.c


-Lionel


>
> Thanks.
> --
> Ashutosh