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 22:25:40 -0800, Lionel Landwerlin wrote:
>
> 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

I know what you are saying, but seems counter_size and bc_report fields got
introduced (and are applicable) only to Xe2+. So to me the code looks
correct.

Compare Bspec:52198 (Xe1 and older) with Bspec:60942 (Xe2+).

I will add the Bspec page number and "Refer to the oa_formats array in
drivers/gpu/drm/xe/xe_oa.c" to xe_drm.h.

Thanks.
--
Ashutosh