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

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Wed, Oct 04, 2023 at 10:37:09PM -0700, Dixit, Ashutosh wrote:
>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.

Let's go with 2 then. I will skip reviewing this.

>
>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
>> >