Re: [Intel-xe] [PATCH 1/2] drm/xe/uapi: "Perf" layer to support multiple perf counter stream types

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

On Thu, 12 Oct 2023 19:05:24 -0700, Umesh Nerlige Ramappa wrote:
>

Hi Umesh,

> >> I would just squash the 2 patches here since this is a new interface and we
> >> don't care much about what existed earlier.
> >
> > OK, I'll squash the two patches.

Done: https://patchwork.freedesktop.org/series/125724/

> >> > +/**
> >> > + * struct drm_xe_perf_param - XE perf layer param
> >> > + *
> >> > + * The perf layer enables multiplexing perf counter streams of multiple
> >> > + * types. The actual params for a particular stream operation are supplied
> >> > + * via the @param pointer (use __copy_from_user to get these params).
> >> > + */
> >> > +struct drm_xe_perf_param {
> >> > +	/** @extensions: Pointer to the first extension struct, if any */
> >> > +	__u64 extensions;
> >> > +	/** @perf_type: Perf stream type, of enum @drm_xe_perf_type */
> >> > +	__u64 perf_type;
> >> > +	/** @param_size: size of data struct pointed to by @param */
> >> > +	__u64 param_size;
> >>
> >> Since this structure is expandable using extensions, maybe we don't need to
> >> worry about the param_size. We can drop it. The size would matter if the
> >> only way to extend a structure was to add members to the end.
> >
> > Hmm, note param_size is not the size of this struct, it is the "size of
> > data struct pointed to by @param". Even the struct pointed to by @param
> > probably will also have an extensions member.
> >
> > Even then I am thinking no harm in having the param_size member since it
> > helps in cases where UMD and KMD versions are different and the struct size
> > has changed, so each has a different notion of what the struct size
> > is. Having param_size available enables the kernel to resolve such cases if
> > the need arises in the future (as is done in drm_ioctl). UMD just needs to
> > fill in param_size as sizeof(struct) it is passing in via @param, so
> > doesn't look too horrible. So I'm thinking of leaving it in just in case.
> >
> > What do you think?
>
> IMO, if extensions are members of all structs in our uApi, then we should
> just use that for extending the structs. I think the size is added burden
> on UMDs to populate. In case of drm_ioctl, the UMD does not see this, so
> it's probably okay.

I have dropped param_size for now in the above series to allow for the
basic patch to get merged first. I might submit a separate patch for
param_size later with additional justification and then we can debate that.

Thanks.
--
Ashutosh