diff mbox series

[7/9] drm/i915/perf: Allow dynamic reconfiguration of the OA stream

Message ID 20191009211947.6815-7-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show
Series [1/9] drm/i915/perf: store the associated engine of a stream | expand

Commit Message

Chris Wilson Oct. 9, 2019, 9:19 p.m. UTC 
From: Lionel Landwerlin <lionel.g.landwerlin@intel.com>

Introduce a new perf_ioctl command to change the OA configuration of the
active stream. This allows the OA stream to be reconfigured between
batch buffers, giving greater flexibility in sampling. We inject a
request into the OA context to reconfigure the stream asynchronously on
the GPU in between and ordered with execbuffer calls.

Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
 drivers/gpu/drm/i915/i915_perf.c | 34 +++++++++++++++++++++++++++++++-
 include/uapi/drm/i915_drm.h      | 10 ++++++++++
 2 files changed, 43 insertions(+), 1 deletion(-)

Comments

Lionel Landwerlin Oct. 10, 2019, 3:22 p.m. UTC | #1 
On 10/10/2019 00:19, Chris Wilson wrote:
> From: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>
> Introduce a new perf_ioctl command to change the OA configuration of the
> active stream. This allows the OA stream to be reconfigured between
> batch buffers, giving greater flexibility in sampling. We inject a
> request into the OA context to reconfigure the stream asynchronously on
> the GPU in between and ordered with execbuffer calls.
>
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>


So much simpler :)


> ---
>   drivers/gpu/drm/i915/i915_perf.c | 34 +++++++++++++++++++++++++++++++-
>   include/uapi/drm/i915_drm.h      | 10 ++++++++++
>   2 files changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 12cc47aece21..3b77db8995f3 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -2856,6 +2856,28 @@ static void i915_perf_disable_locked(struct i915_perf_stream *stream)
>   		stream->ops->disable(stream);
>   }
>   
> +static int i915_perf_config_locked(struct i915_perf_stream *stream,
> +				   unsigned long metrics_set)
> +{
> +	struct i915_oa_config *config;
> +	int err = 0;
> +
> +	config = i915_perf_get_oa_config(stream->perf, metrics_set);
> +	if (!config)
> +		return -EINVAL;
> +
> +	if (config != stream->oa_config) {
> +		if (stream->pinned_ctx)
> +			err = emit_oa_config(stream, stream->pinned_ctx);
> +		if (err == 0)
> +			config = xchg(&stream->oa_config, config);
> +	}
> +
> +	i915_oa_config_put(config);
> +
> +	return err;
> +}
> +
>   /**
>    * i915_perf_ioctl - support ioctl() usage with i915 perf stream FDs
>    * @stream: An i915 perf stream
> @@ -2879,6 +2901,8 @@ static long i915_perf_ioctl_locked(struct i915_perf_stream *stream,
>   	case I915_PERF_IOCTL_DISABLE:
>   		i915_perf_disable_locked(stream);
>   		return 0;
> +	case I915_PERF_IOCTL_CONFIG:
> +		return i915_perf_config_locked(stream, arg);

For REMOVE_CONFIG we passed a pointer to an u64, not sure whether we 
should reuse the same pattern here?

I don't mind the current version.


-Lionel

>   	}
>   
>   	return -EINVAL;
> @@ -4017,7 +4041,15 @@ void i915_perf_fini(struct drm_i915_private *i915)
>    */
>   int i915_perf_ioctl_version(void)
>   {
> -	return 1;
> +	/*
> +	 * 1: Initial version
> +	 *   I915_PERF_IOCTL_ENABLE
> +	 *   I915_PERF_IOCTL_DISABLE
> +	 *
> +	 * 2: Added runtime modification of OA config.
> +	 *   I915_PERF_IOCTL_CONFIG
> +	 */
> +	return 2;
>   }
>   
>   #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 0c7b2815fbf1..5e66f7c60261 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1932,6 +1932,16 @@ struct drm_i915_perf_open_param {
>    */
>   #define I915_PERF_IOCTL_DISABLE	_IO('i', 0x1)
>   
> +/**
> + * Change metrics_set captured by a stream.
> + *
> + * Will not take effect until the stream is restart, or upon the next
> + * execbuf when attached to a specific context.
> + *
> + * This ioctl is available in perf revision 2.
> + */
> +#define I915_PERF_IOCTL_CONFIG	_IO('i', 0x2)
> +
>   /**
>    * Common to all i915 perf records
>    */
Chris Wilson Oct. 10, 2019, 3:44 p.m. UTC | #2 
Quoting Lionel Landwerlin (2019-10-10 16:22:25)
> On 10/10/2019 00:19, Chris Wilson wrote:
> > From: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> >
> > Introduce a new perf_ioctl command to change the OA configuration of the
> > active stream. This allows the OA stream to be reconfigured between
> > batch buffers, giving greater flexibility in sampling. We inject a
> > request into the OA context to reconfigure the stream asynchronously on
> > the GPU in between and ordered with execbuffer calls.
> >
> > Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> 
> 
> So much simpler :)

Indeed, it all came together into a much more coherent story.

> > ---
> >   /**
> >    * i915_perf_ioctl - support ioctl() usage with i915 perf stream FDs
> >    * @stream: An i915 perf stream
> > @@ -2879,6 +2901,8 @@ static long i915_perf_ioctl_locked(struct i915_perf_stream *stream,
> >       case I915_PERF_IOCTL_DISABLE:
> >               i915_perf_disable_locked(stream);
> >               return 0;
> > +     case I915_PERF_IOCTL_CONFIG:
> > +             return i915_perf_config_locked(stream, arg);
> 
> For REMOVE_CONFIG we passed a pointer to an u64, not sure whether we 
> should reuse the same pattern here?

Aiui, the user creates oa-config handles, and/or queries them. If we are
simpler talking handles that fit inside unsigned long (so assume u32)
then I don't see the harm in passing an id rather than a pointer.

The alternative is this takes an uuid string? Or you want to always use
u64 handles?

I guess you will have a better idea what works better after playing
around with userspace.
-Chris
Lionel Landwerlin Oct. 10, 2019, 4:07 p.m. UTC | #3 
On 10/10/2019 18:44, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2019-10-10 16:22:25)
>> On 10/10/2019 00:19, Chris Wilson wrote:
>>> From: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>>>
>>> Introduce a new perf_ioctl command to change the OA configuration of the
>>> active stream. This allows the OA stream to be reconfigured between
>>> batch buffers, giving greater flexibility in sampling. We inject a
>>> request into the OA context to reconfigure the stream asynchronously on
>>> the GPU in between and ordered with execbuffer calls.
>>>
>>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>>
>> So much simpler :)
> Indeed, it all came together into a much more coherent story.
>
>>> ---
>>>    /**
>>>     * i915_perf_ioctl - support ioctl() usage with i915 perf stream FDs
>>>     * @stream: An i915 perf stream
>>> @@ -2879,6 +2901,8 @@ static long i915_perf_ioctl_locked(struct i915_perf_stream *stream,
>>>        case I915_PERF_IOCTL_DISABLE:
>>>                i915_perf_disable_locked(stream);
>>>                return 0;
>>> +     case I915_PERF_IOCTL_CONFIG:
>>> +             return i915_perf_config_locked(stream, arg);
>> For REMOVE_CONFIG we passed a pointer to an u64, not sure whether we
>> should reuse the same pattern here?
> Aiui, the user creates oa-config handles, and/or queries them. If we are
> simpler talking handles that fit inside unsigned long (so assume u32)
> then I don't see the harm in passing an id rather than a pointer.
>
> The alternative is this takes an uuid string? Or you want to always use
> u64 handles?
>
> I guess you will have a better idea what works better after playing
> around with userspace.
> -Chris
>
Yeah, that's a fine interface actually.

This should really have you as the author.

Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Chris Wilson Oct. 10, 2019, 7:50 p.m. UTC | #4 
Quoting Lionel Landwerlin (2019-10-10 17:07:11)
> Yeah, that's a fine interface actually.

One last thought for the interface, should we return the previous
config-id?

That limits the config-id space to long (min s32) so that we can report
the negative error code (or exclude the top 4095 values).
-Chris
Lionel Landwerlin Oct. 10, 2019, 9:08 p.m. UTC | #5 
On 10/10/2019 22:50, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2019-10-10 17:07:11)
>> Yeah, that's a fine interface actually.
> One last thought for the interface, should we return the previous
> config-id?
>
> That limits the config-id space to long (min s32) so that we can report
> the negative error code (or exclude the top 4095 values).
> -Chris
>
No real use for it at the moment. I leave this up to your uAPI designer 
experience :)


-Lionel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 12cc47aece21..3b77db8995f3 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -2856,6 +2856,28 @@  static void i915_perf_disable_locked(struct i915_perf_stream *stream)
 		stream->ops->disable(stream);
 }
 
+static int i915_perf_config_locked(struct i915_perf_stream *stream,
+				   unsigned long metrics_set)
+{
+	struct i915_oa_config *config;
+	int err = 0;
+
+	config = i915_perf_get_oa_config(stream->perf, metrics_set);
+	if (!config)
+		return -EINVAL;
+
+	if (config != stream->oa_config) {
+		if (stream->pinned_ctx)
+			err = emit_oa_config(stream, stream->pinned_ctx);
+		if (err == 0)
+			config = xchg(&stream->oa_config, config);
+	}
+
+	i915_oa_config_put(config);
+
+	return err;
+}
+
 /**
  * i915_perf_ioctl - support ioctl() usage with i915 perf stream FDs
  * @stream: An i915 perf stream
@@ -2879,6 +2901,8 @@  static long i915_perf_ioctl_locked(struct i915_perf_stream *stream,
 	case I915_PERF_IOCTL_DISABLE:
 		i915_perf_disable_locked(stream);
 		return 0;
+	case I915_PERF_IOCTL_CONFIG:
+		return i915_perf_config_locked(stream, arg);
 	}
 
 	return -EINVAL;
@@ -4017,7 +4041,15 @@  void i915_perf_fini(struct drm_i915_private *i915)
  */
 int i915_perf_ioctl_version(void)
 {
-	return 1;
+	/*
+	 * 1: Initial version
+	 *   I915_PERF_IOCTL_ENABLE
+	 *   I915_PERF_IOCTL_DISABLE
+	 *
+	 * 2: Added runtime modification of OA config.
+	 *   I915_PERF_IOCTL_CONFIG
+	 */
+	return 2;
 }
 
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 0c7b2815fbf1..5e66f7c60261 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1932,6 +1932,16 @@  struct drm_i915_perf_open_param {
  */
 #define I915_PERF_IOCTL_DISABLE	_IO('i', 0x1)
 
+/**
+ * Change metrics_set captured by a stream.
+ *
+ * Will not take effect until the stream is restart, or upon the next
+ * execbuf when attached to a specific context.
+ *
+ * This ioctl is available in perf revision 2.
+ */
+#define I915_PERF_IOCTL_CONFIG	_IO('i', 0x2)
+
 /**
  * Common to all i915 perf records
  */