[Intel-xe] [PATCH 21/21] drm/xe/uapi: Convert OA property key/value pairs to a struct

Thu Oct 5 05:37:09 UTC 2023

On Thu, 21 Sep 2023 16:53:20 -0700, Dixit, Ashutosh wrote:
>

Hi Umesh,

> On Tue, 19 Sep 2023 09:10:49 -0700, Ashutosh Dixit wrote:
> >
> > Change OA uapi to take a param struct rather than property key value
> > pairs. A param struct is simpler and param structs can be extenended in the
> > future using xe_user_extension so there seems to be no reason to use
> > property key value pairs.
>
> There are two ways of doing this:
>
> 1. In this patch we have collected all OA properties into a single
>    struct. The assumption is that any future changes would be handled via
>    'struct drm_xe_ext_set_property' chained structs (basically using
>    xe_user_extension):
>
>    https://patchwork.freedesktop.org/patch/558715/
>
> 2. The second way to do it would be to use chained 'struct
>    drm_xe_ext_set_property' from the beginning as is being done for
>    DRM_XE_EXEC_QUEUE_SET_PROPERTY. This is basically the same as the
>    earlier OA property key/value pairs except that the properties are now
>    input via chained structs.
>
>    This second way is a uniform way of specifying property values whereas
>    the first way in non-uniform.
>
> Just thought I'll point this out when we decide about this uapi during the
> code review.

Since we are almost certain that the OA uapi will need to be extended in
the future, for the sake of uniformity maybe we should go with the approach
2. above (this patch implements approach 1.)? Something to keep in mind for
the review of this patch. This is what I was referring to earlier today.

Thanks.
--
Ashutosh

>
> > Suggested-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa at intel.com>
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit at intel.com>
>
> /snip/
>
> > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > index c0018abee4052..8ba11c4eb36b5 100644
> > --- a/include/uapi/drm/xe_drm.h
> > +++ b/include/uapi/drm/xe_drm.h
> > @@ -1175,30 +1175,26 @@ struct drm_xe_query_oa_info {
> >	} oau[];
> >  };
> >
> > -enum drm_xe_oa_property_id {
> > -	/**
> > -	 * ID of the OA unit on which to open the OA stream, see
> > -	 * @oa_unit_id in 'struct drm_xe_engine_class_instance'. Defaults
> > -	 * to 0 if not provided.
> > -	 */
> > -	DRM_XE_OA_PROP_OA_UNIT_ID = 1,
> > +struct drm_xe_oa_open_param {
> > +	/** @extensions: Pointer to the first extension struct, if any */
> > +	__u64 extensions;
> >
> >	/**
> > -	 * A value of 1 requests the inclusion of raw OA unit reports as
> > -	 * part of stream samples.
> > +	 * @oa_unit_id: ID of the OA unit on which to open the OA stream,
> > +	 * see @oa_unit_id in struct @drm_xe_engine_class_instance
> >	 */
> > -	DRM_XE_OA_PROP_SAMPLE_OA,
> > +	__u32 oa_unit_id;
> >
> >	/**
> > -	 * The value specifies which set of OA unit metrics should be
> > -	 * configured, defining the contents of any OA unit reports.
> > +	 * @sample_oa: A value of 1 requests the inclusion of raw OA unit
> > +	 * reports as part of stream samples
> >	 */
> > -	DRM_XE_OA_PROP_OA_METRICS_SET,
> > +	__u32 sample_oa;
> >
> >	/**
> > -	 * The value specifies the size and layout of OA unit reports.
> > +	 * @oa_format: The value specifies the size and layout of OA unit reports
> >	 */
> > -	DRM_XE_OA_PROP_OA_FORMAT,
> > +	__u64 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
> > @@ -1210,86 +1206,79 @@ enum drm_xe_oa_property_id {
> >  #define XE_OA_MASK_BC_REPORT	(0xff << 24)
> >
> >	/**
> > -	 * Specifying this property implicitly requests periodic OA unit
> > -	 * sampling and (at least on Haswell) the sampling frequency is derived
> > -	 * from this exponent as follows:
> > -	 *
> > -	 *   80ns * 2^(period_exponent + 1)
> > +	 * @metric_set: specifies which set of OA unit metrics should be
> > +	 * configured, defining the contents of any OA unit reports. Metric
> > +	 * set ID is returned by the XE_PERF_ADD_CONFIG op of the PREF ioctl
> >	 */
> > -	DRM_XE_OA_PROP_OA_EXPONENT,
> > +	__u32 metric_set;
> >
> >	/**
> > -	 * Specifying this property is only valid when specify a context to
> > -	 * filter with DRM_XE_OA_PROP_ENGINE_ID. Specifying this property
> > -	 * will hold preemption of the particular engine we want to gather
> > -	 * performance data about.
> > +	 * @period_exponent: Specifying this property implicitly requests
> > +	 * periodic OA unit sampling. The sampling period is:
> > +	 *
> > +	 *   2^(period_exponent + 1) / @oa_timestamp_freq
> > +	 *
> > +	 * Set period_exponent *negative* to disable periodic sampling
> >	 */
> > -	DRM_XE_OA_PROP_HOLD_PREEMPTION,
> > +	__s32 period_exponent;
> >
> >	/**
> > -	 * Specify a global OA buffer size to be allocated in bytes. The
> > -	 * size specified must be supported by HW (powers of 2 ranging from
> > -	 * 128 KB to 128Mb depending on the platform)
> > +	 * @oa_buffer_size: Specify a global OA buffer size to be allocated
> > +	 * in bytes. The size specified must be supported by HW (powers of
> > +	 * 2 ranging from 128 KB to 128Mb depending on the platform). A
> > +	 * value of 0 will choose a default size of 16 MB.
> >	 */
> > -	DRM_XE_OA_PROP_OA_BUFFER_SIZE,
> > +	__u32 oa_buffer_size;
> >
> >	/**
> > -	 * This optional parameter specifies the timer interval in nanoseconds
> > -	 * at which the xe driver will check the OA buffer for available data.
> > -	 * Minimum allowed value is 100 microseconds. A default value is used by
> > -	 * the driver if this parameter is not specified. Note that larger timer
> > -	 * values will reduce cpu consumption during OA perf captures. However,
> > -	 * excessively large values would potentially result in OA buffer
> > -	 * overwrites as captures reach end of the OA buffer.
> > +	 * @poll_period: Specify timer interval in micro-seconds at which
> > +	 * the xe driver will check the OA buffer for available
> > +	 * data. Minimum allowed value is 100 microseconds. A value of 0
> > +	 * selects a default value is used by the driver. Note that larger
> > +	 * timer values will reduce cpu consumption during OA perf
> > +	 * captures. However, excessively large values would potentially
> > +	 * result in OA buffer overwrites as captures reach end of the OA
> > +	 * buffer.
> >	 */
> > -	DRM_XE_OA_PROP_POLL_OA_PERIOD,
> > +	__u32 poll_period_us;
> > +
> > +	/** @open_flags: Flags */
> > +	__u32 open_flags;
> > +#define XE_OA_FLAG_FD_CLOEXEC	(1 << 0)
> > +#define XE_OA_FLAG_FD_NONBLOCK	(1 << 1)
> > +#define XE_OA_FLAG_DISABLED	(1 << 2)
> >
> >	/**
> > -	 * Open the stream for a specific exec queue id (as used with
> > -	 * drm_xe_exec). A stream opened for a specific exec queue id this
> > -	 * way won't typically require root privileges.
> > +	 * @exec_queue_id: Open the stream for a specific exec queue id (as
> > +	 * used with drm_xe_exec). A stream opened for a specific exec
> > +	 * queue id this way won't typically require root
> > +	 * privileges. Pass a value <= 0 to not specify an exec queue id.
> >	 */
> > -	DRM_XE_OA_PROP_EXEC_QUEUE_ID,
> > +	__s32 exec_queue_id;
> >
> >	/**
> > -	 * This parameter specifies the engine instance and can be passed along
> > -	 * with DRM_XE_OA_PROP_EXEC_QUEUE_ID or will default to 0.
> > +	 * @engine_instance: engine instance to use with @exec_queue_id.
> >	 */
> > -	DRM_XE_OA_PROP_OA_ENGINE_INSTANCE,
> > +	__u32 engine_instance;
> >
> > -	DRM_XE_OA_PROP_MAX /* non-ABI */
> > -};
> > -
> > -struct drm_xe_oa_open_param {
> > -	/** @extensions: Pointer to the first extension struct, if any */
> > -	__u64 extensions;
> > +	/**
> > +	 * @hold_preemption: If true, this will disable preemption for the
> > +	 * exec queue selected with @exec_queue_id
> > +	 */
> > +	__u32 hold_preemption;
> >
> >	/**
> > -	 * @config_syncobj: (Output) handle to configuration syncobj
> > +	 * @config_syncobj: (output) handle to configuration syncobj
> >	 *
> >	 * Handle to a syncobj which the kernel will signal after stream
> >	 * configuration or re-configuration is complete (after return from
> >	 * the ioctl). This handle can be provided as a dependency to the
> > -	 * next XE exec ioctl.
> > +	 * next xe exec ioctl to synchronize xe exec with oa config changes
> >	 */
> >	__u32 config_syncobj;
> >
> > -	__u32 reserved;
> > -
> > -	/** @flags: Flags */
> > -	__u32 flags;
> > -#define XE_OA_FLAG_FD_CLOEXEC	(1 << 0)
> > -#define XE_OA_FLAG_FD_NONBLOCK	(1 << 1)
> > -#define XE_OA_FLAG_DISABLED	(1 << 2)
> > -
> > -	/** The number of u64 (id, value) pairs */
> > -	__u32 num_properties;
> > -
> > -	/**
> > -	 * Pointer to array of u64 (id, value) pairs configuring the stream
> > -	 * to open.
> > -	 */
> > -	__u64 properties_ptr;
> > +	/** @reserved: reserved (MBZ) */
> > +	__u64 reserved[4];
> >  };
> >
> >  struct drm_xe_oa_record_header {
> > --
> > 2.41.0
> >