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

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

On Mon, 12 Feb 2024 11:08:26 -0800, Umesh Nerlige Ramappa wrote:
>

Hi Umesh,

> 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

This is what I ended up doing for this:

+	/** @oa_status: OA status register as specified in PRM/Bspec 46717/61226 */
 	__u64 oa_status;
+#define DRM_XE_OASTATUS_MMIO_TRG_Q_FULL		(1 << 6)
+#define DRM_XE_OASTATUS_COUNTER_OVERFLOW	(1 << 2)
+#define DRM_XE_OASTATUS_BUFFER_OVERFLOW		(1 << 1)
+#define DRM_XE_OASTATUS_REPORT_LOST		(1 << 0)

And for the formats:

+	/*
+	 * 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.
+ 	 */

Probably easier to see this here:

https://patchwork.freedesktop.org/patch/577943/?series=128993&rev=3

Thanks.
--
Ashutosh