[PATCH 13/16] drm/xe/oa/uapi: Query OA unit properties

[Dixit, Ashutosh]

On Tue, 06 Feb 2024 15:00:29 -0800, Umesh Nerlige Ramappa wrote:
>

Hi Umesh,

> On Fri, Jan 19, 2024 at 06:00:23PM -0800, Ashutosh Dixit wrote:
> > Implement query for properties of OA units present on a device.
> >
> > v2: Clean up reserved/pad fields (Umesh)
> >    Follow the same scheme as other query structs
> >
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit at intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_query.c | 81 +++++++++++++++++++++++++++++++++++
> > include/uapi/drm/xe_drm.h     | 55 ++++++++++++++++++++++++
> > 2 files changed, 136 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c
> > index 9b35673b286c8..eac266a3d66d5 100644
> > --- a/drivers/gpu/drm/xe/xe_query.c
> > +++ b/drivers/gpu/drm/xe/xe_query.c
> > @@ -520,6 +520,86 @@ static int query_gt_topology(struct xe_device *xe,
> >	return 0;
> > }
> >
> > +static size_t calc_oa_unit_query_size(struct xe_device *xe)
> > +{
> > +	size_t size = sizeof(struct drm_xe_query_oa_units);
> > +	struct xe_gt *gt;
> > +	int i, id;
> > +
> > +	for_each_gt(gt, xe, id) {
> > +		for (i = 0; i < gt->oa.num_oa_units; i++) {
> > +			size += sizeof(struct drm_xe_oa_unit);
> > +			size += gt->oa.oa_unit[i].num_engines *
> > +				sizeof(struct drm_xe_engine_class_instance);
> > +		}
> > +	}
> > +
> > +	return size;
> > +}
> > +
> > +static int query_oa_units(struct xe_device *xe,
> > +			  struct drm_xe_device_query *query)
> > +{
> > +	void __user *query_ptr = u64_to_user_ptr(query->data);
> > +	size_t size = calc_oa_unit_query_size(xe);
> > +	struct drm_xe_query_oa_units *qoa;
> > +	enum xe_hw_engine_id hwe_id;
> > +	struct drm_xe_oa_unit *du;
> > +	struct xe_hw_engine *hwe;
> > +	struct xe_oa_unit *u;
> > +	int gt_id, i, j, ret;
> > +	struct xe_gt *gt;
> > +	u8 *pdu;
> > +
> > +	if (query->size == 0) {
> > +		query->size = size;
> > +		return 0;
> > +	} else if (XE_IOCTL_DBG(xe, query->size != size)) {
> > +		return -EINVAL;
> > +	}
> > +
> > +	qoa = kzalloc(size, GFP_KERNEL);
> > +	if (!qoa)
> > +		return -ENOMEM;
> > +
> > +	pdu = (u8 *)&qoa->oa_units[0];
> > +	for_each_gt(gt, xe, gt_id) {
> > +		for (i = 0; i < gt->oa.num_oa_units; i++) {
> > +			u = &gt->oa.oa_unit[i];
> > +			du = (struct drm_xe_oa_unit *)pdu;
> > +
> > +			du->oa_unit_id = u->oa_unit_id;
> > +			du->oa_unit_type = u->type;
> > +			du->gt_id = gt->info.id;
> > +			du->open_stream = !!u->exclusive_stream;
> > +			du->oa_timestamp_freq = xe_oa_timestamp_frequency(gt);
> > +			du->oa_buf_size = XE_OA_BUFFER_SIZE;
> > +			du->num_engines = u->num_engines;
> > +
> > +			for (j = 1; j < DRM_XE_OA_PROPERTY_MAX; j++)
> > +				du->capabilities |= BIT(j);

Could you take a look at this too? So capabilities are just a bit mask of
the properties. This would mean that in the future any changes we make will
need to exposed via new properties, so that they get reflected in
capabilities.

> > +
> > +			j = 0;
> > +			for_each_hw_engine(hwe, gt, hwe_id) {
> > +				if (xe_oa_unit_id(hwe) == u->oa_unit_id) {
> > +					du->eci[j].engine_class =
> > +						xe_to_user_engine_class[hwe->class];
> > +					du->eci[j].engine_instance = hwe->logical_instance;
> > +					du->eci[j].gt_id = gt->info.id;
> > +					j++;
> > +				}
> > +			}
> > +			pdu += sizeof(*du) + j * sizeof(du->eci[0]);
> > +			qoa->num_oa_units++;
> > +		}
> > +	}
> > +
> > +	ret = copy_to_user(query_ptr, qoa, size);
> > +	kfree(qoa);
> > +
> > +	return ret ? -EFAULT : 0;
> > +}
> > +
> > static int (* const xe_query_funcs[])(struct xe_device *xe,
> >				      struct drm_xe_device_query *query) = {
> >	query_engines,
> > @@ -529,6 +609,7 @@ static int (* const xe_query_funcs[])(struct xe_device *xe,
> >	query_hwconfig,
> >	query_gt_topology,
> >	query_engine_cycles,
> > +	query_oa_units,
> > };
> >
> > int xe_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > index 42474629244d4..2c0949b248d9a 100644
> > --- a/include/uapi/drm/xe_drm.h
> > +++ b/include/uapi/drm/xe_drm.h
> > @@ -646,6 +646,7 @@ struct drm_xe_device_query {
> > #define DRM_XE_DEVICE_QUERY_HWCONFIG		4
> > #define DRM_XE_DEVICE_QUERY_GT_TOPOLOGY		5
> > #define DRM_XE_DEVICE_QUERY_ENGINE_CYCLES	6
> > +#define DRM_XE_DEVICE_QUERY_OA_UNITS		7
> >	/** @query: The type of data to query */
> >	__u32 query;
> >
> > @@ -1405,6 +1406,60 @@ enum drm_xe_oa_unit_type {
> >	DRM_XE_OA_UNIT_TYPE_OAM,
> > };
> >
> > +/**
> > + * struct drm_xe_oa_unit - describe OA unit
> > + */
> > +struct drm_xe_oa_unit {
> > +	/** @oa_unit_id: OA unit ID */
> > +	__u16 oa_unit_id;
> > +
> > +	/** @oa_unit_type: OA unit type of @drm_xe_oa_unit_type */
> > +	__u16 oa_unit_type;
> > +
> > +	/** @gt_id: GT ID for this OA unit */
> > +	__u16 gt_id;
> > +
> > +	/** @open_stream: True if a stream is open on the OA unit */
> > +	__u16 open_stream;
>
> How would the user end up using this? Ideally, I would think that user
> should only need to query all this info once and that would be before the
> user opens any streams. If the user did open a stream, then that info is
> already known to the user that a specific oa_unit_id is opened.
>
> Someone else or the same user trying to open the stream again would get an
> error anyways.
>
> > +
> > +	/** @capabilities: OA capabilities bit-mask */
> > +	__u64 capabilities;
> > +
> > +	/** @oa_timestamp_freq: OA timestamp freq */
> > +	__u64 oa_timestamp_freq;
> > +
> > +	/** @oa_buf_size: OA buffer size */
> > +	__u64 oa_buf_size;
>
> I think oa_buf_size value also changes based on whether the stream is
> opened or not. We could have a oa_max_buf_size and oa_buf_size separately.
>
> Also, I am reluctant having this query return different values based on
> whether the stream is opened of not. Determining the state/info of an open
> stream should be a different query/interface. I think the ioctl on the open
> fd was a good choice for that since that would return info on the open fd
> and would anyways be serialized with close(fd).
>
> If the user can somehow know that the stream is opened and also know the
> oa_buffer_size already, then we can do away with this (open_stream and
> oa_buf_size) altogether.

Agreed, this was kludgy. I have eliminated open_stream and oa_buf_size from
this struct in v9 and exposed it via a new DRM_XE_PERF_IOCTL_INFO ioctl on
the perf stream fd. Please take a look at "drm/xe/oa/uapi: Query OA unit
properties" in v9.

I have also eliminated gt_id from the above struct, since gt_id is available
in 'struct drm_xe_engine_class_instance' via the eci[] array below.

> > +
> > +	/** @reserved: MBZ */
> > +	__u64 reserved[4];
> > +
> > +	/** @num_engines: number of engines in @eci array */
> > +	__u64 num_engines;
> > +
> > +	/** @eci: engines attached to this OA unit */
> > +	struct drm_xe_engine_class_instance eci[];
> > +};
> > +
> > +/**
> > + * struct drm_xe_query_oa_units - describe OA units
> > + *
> > + * If a query is made with a struct drm_xe_device_query where .query
> > + * is equal to DRM_XE_DEVICE_QUERY_OA_UNITS, then the reply uses struct
> > + * drm_xe_query_oa_units in .data.
> > + *
> > + * When there is an @open_stream, the query returns properties specific to
> > + * that @open_stream. Else default properties are returned.

Removed this comment and added a code-block showing how to parse through
this variable length struct in v9.

> > + */
> > +struct drm_xe_query_oa_units {
> > +	/** @num_oa_units: number of OA units returned in oau[] */
> > +	__u32 num_oa_units;
> > +	/** @pad: MBZ */
> > +	__u32 pad;
> > +	/** @oa_units: OA units returned for this device */
> > +	struct drm_xe_oa_unit oa_units[];
> > +};
> > +
> > /** enum drm_xe_oa_format_type - OA format types */
> > enum drm_xe_oa_format_type {
> >	DRM_XE_OA_FMT_TYPE_OAG,
> > --
> > 2.41.0
> >

Thanks.
--
Ashutosh