[PATCH v3] drm/xe/oa: Fix kernel doc warnings in xe_drm.h

[Dixit, Ashutosh]

On Mon, 24 Jun 2024 13:18:20 -0700, Souza, Jose wrote:
>
> On Mon, 2024-06-24 at 13:04 -0700, Dixit, Ashutosh wrote:
> > On Mon, 24 Jun 2024 05:30:37 -0700, Michal Wajdeczko wrote:
> > >
> >
> > Hi Michal,
> >
> > And Cc: Jose below.
> >
> > >
> > >
> > > On 23.06.2024 22:31, Ashutosh Dixit wrote:
> > > > Fix kernel doc warnings in xe_drm.h. Also eliminate private/non-abi enum
> > > > definitions.
> > > >
> > > > v2: Remove __DRM_XE_PERF_TYPE_MAX since it is unused (Michal)
> > > > v3: Also remove DRM_XE_OA_PROPERTY_MAX since it can also be eliminated (Michal)
> > > >
> > > > Suggested-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> > > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit at intel.com>
> > >
> > > some nits below, but in general LGTM, so
> > >
> > > Reviewed-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> >
> > Thanks.
> >
> > >
> > > and I'm assuming it's not too late for such uabi fixups, but better to
> > > wait for ack from Rodrigo
> >
> > Copying Jose: in case Mesa were using DRM_XE_OA_PROPERTY_MAX, they can
> > probably also use a value like 16 like we've done below in Xe (I will make
> > this change in IGT). Mesa PR is not merged yet so should be ok I think, but
> > they will need to make a tiny change. I am assuming as long as this all
> > gets into 6.11 it should be ok.
> >
> > Jose please confirm this is ok. Thanks.
>
> Mesa is not using and even if were we can change it until it gets
> merged(after KMD patches reaches drm-next).

OK, thanks for the confirmation Jose.

>
> While at it, do we have any UMD using drm_xe_query_oa_units?  Because
> Mesa don't use it, so I think it should be removed until a UMD makes use
> of it.  So drm_xe_query_oa_units and related UAPIs could be removed.

Actually we do use drm_xe_query_oa_units in gpuvis (actually
tools/xe-perf/xe_perf_recorder.c in IGT, which is used for gpuvis OA
recordings, see assign_oa_unit()).

drm_xe_query_oa_units is needed because it defines the mapping between an
OA unit and the hardware engines connected to that OA unit. Otherwise
userspace has no way of knowing that information.

In case of RCS, mostly you can assume that RCS is connected to OA unit
0. But for CCS etc. we need the query to correctly determine which OA unit
that engine is connected to.

Thanks.
--
Ashutosh


>
> >
> > >
> > > > ---
> > > >  drivers/gpu/drm/xe/xe_oa.c | 3 ++-
> > > >  include/uapi/drm/xe_drm.h  | 5 +----
> > > >  2 files changed, 3 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> > > > index 4168b51cf7b5..9263ae9a864e 100644
> > > > --- a/drivers/gpu/drm/xe/xe_oa.c
> > > > +++ b/drivers/gpu/drm/xe/xe_oa.c
> > > > @@ -1684,6 +1684,7 @@ static const xe_oa_user_extension_fn xe_oa_user_extension_funcs[] = {
> > > >		[DRM_XE_OA_EXTENSION_SET_PROPERTY] = xe_oa_user_ext_set_property,
> > > >  };
> > > >
> > > > +#define MAX_USER_EXTENSIONS	16
> > >
> > > nit: maybe it's worth to put small comment saying this is our choice to
> > > limit number of nested user extensions we want to support (or at least
> > > this is how I understood this)
> > >
> > > nit: and this doesn't really look like OA specific limitation, so maybe
> > > it's time to promote MAX_USER_EXTENSIONS to some shared location to make
> > > it unified across driver
> >
> > xe_exec_queue.c also uses a similar mechanism. But I think better to leave
> > them separate in different modules so that each module can tweak the value
> > to what is best for that module.
> >
> > Thanks.
> > --
> > Ashutosh
> >
> >
> > >
> > > >  static int xe_oa_user_extensions(struct xe_oa *oa, u64 extension, int ext_number,
> > > >					 struct xe_oa_open_param *param)
> > > >  {
> > > > @@ -1692,7 +1693,7 @@ static int xe_oa_user_extensions(struct xe_oa *oa, u64 extension, int ext_number
> > > >		int err;
> > > >		u32 idx;
> > > >
> > > > -	if (XE_IOCTL_DBG(oa->xe, ext_number >= DRM_XE_OA_PROPERTY_MAX))
> > > > +	if (XE_IOCTL_DBG(oa->xe, ext_number >= MAX_USER_EXTENSIONS))
> > > >			return -E2BIG;
> > > >
> > > >		err = __copy_from_user(&ext, address, sizeof(ext));
> > > > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > > > index 93e00be44b2d..b410553faa9b 100644
> > > > --- a/include/uapi/drm/xe_drm.h
> > > > +++ b/include/uapi/drm/xe_drm.h
> > > > @@ -1379,8 +1379,8 @@ struct drm_xe_wait_user_fence {
> > > >   * enum drm_xe_perf_type - Perf stream types
> > > >   */
> > > >  enum drm_xe_perf_type {
> > > > +	/** @DRM_XE_PERF_TYPE_OA: OA perf stream type */
> > > >		DRM_XE_PERF_TYPE_OA,
> > > > -	__DRM_XE_PERF_TYPE_MAX, /* non-ABI */
> > > >  };
> > > >
> > > >  /**
> > > > @@ -1611,9 +1611,6 @@ enum drm_xe_oa_property_id {
> > > >		 * pass along with @DRM_XE_OA_PROPERTY_EXEC_QUEUE_ID or will default to 0.
> > > >		 */
> > > >		DRM_XE_OA_PROPERTY_OA_ENGINE_INSTANCE,
> > > > -
> > > > -	/** @DRM_XE_OA_PROPERTY_MAX: non-ABI */
> > > > -	DRM_XE_OA_PROPERTY_MAX
> > > >  };
> > > >
> > > >  /**
>