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

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Thu, Feb 08, 2024 at 02:26:39PM -0800, 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.

If that's the case, then based on the PRM documents, I would just put 
this in the comment - 

"Refer to performance counter report formats under the Observability 
section in the PRM"

Thanks,
Umesh

>
>Thanks.
>--
>Ashutosh