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

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

On Mon, Feb 12, 2024 at 10:57:18AM -0800, Umesh Nerlige Ramappa wrote:
>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"


Missed your comment on the oa_status reg. In general, we should avoid 
exposing the register as is in the uApi. Instead we should just have a 
set of enums or bits that define a particular state. That would do away 
with the need to point to the spec.

If you disagree, then PRM reference should be good enough - OAG_OASTATUS 
register in the PRM or Hardware Specifications.

Specifically from this link:
https://www.intel.com/content/www/us/en/docs/graphics-for-linux/developer-reference/1-0/overview.html

Thanks,
Umesh

>
>Thanks,
>Umesh
>
>>
>>Thanks.
>>--
>>Ashutosh