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

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

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.

Thanks.
--
Ashutosh

> Suggested-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@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
>