Re: [Intel-gfx] [PATCH 4/7] drm/i915/perf: add new open param to configure polling of OA buffer
On Thu, Mar 12, 2020 at 10:37:12PM +0200, Lionel Landwerlin wrote: On 12/03/2020 21:27, Dixit, Ashutosh wrote: On Tue, 03 Mar 2020 14:19:02 -0800, Umesh Nerlige Ramappa wrote: From: Lionel Landwerlin This new parameter let's the application choose how often the OA buffer should be checked on the CPU side for data availability. Longer polling period tend to reduce CPU overhead if the application does not care about somewhat real time data collection. v2: Allow disabling polling completely with 0 value (Lionel) v3: Version the new parameter (Joonas) v4: Rebase (Umesh) Hi Lionel, I was thinking that one way to keep things simple for now (and fix the high cpu usage issue) would be to expose _ONLY_ this OA poll period parameter to user space. That is we don't expose the interrupt or the flush ioctl to user space at this time. This way the user will be able to configure the hrtimer frequency to a lower value to bring down the cpu usage. Also we would disallow disabling the timer (and internally also not use the interrupt). So everything will happen in exactly the same way as it used to (no other changes needed) but at a lower rate if the user so chooses. What do you think about this? Thanks! -- Ashutosh Sure, just make sure the users know about this. The fact that they can now select timer values that will potentially lead to the loss of the buffer's data because it was overridden. posted v2: Kernel patches - https://patchwork.freedesktop.org/series/54280/#rev6 IGT - https://patchwork.freedesktop.org/series/74655/ Thanks, Umesh Thanks, -Lionel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/7] drm/i915/perf: add new open param to configure polling of OA buffer
On Thu, 12 Mar 2020 13:37:12 -0700, Lionel Landwerlin wrote: > On 12/03/2020 21:27, Dixit, Ashutosh wrote: > > On Tue, 03 Mar 2020 14:19:02 -0800, Umesh Nerlige Ramappa wrote: > >> From: Lionel Landwerlin > >> > >> This new parameter let's the application choose how often the OA > >> buffer should be checked on the CPU side for data availability. Longer > >> polling period tend to reduce CPU overhead if the application does not > >> care about somewhat real time data collection. > >> > >> v2: Allow disabling polling completely with 0 value (Lionel) > >> v3: Version the new parameter (Joonas) > >> v4: Rebase (Umesh) > > Hi Lionel, I was thinking that one way to keep things simple for now (and > > fix the high cpu usage issue) would be to expose _ONLY_ this OA poll period > > parameter to user space. That is we don't expose the interrupt or the flush > > ioctl to user space at this time. This way the user will be able to > > configure the hrtimer frequency to a lower value to bring down the cpu > > usage. > > > > Also we would disallow disabling the timer (and internally also not use the > > interrupt). So everything will happen in exactly the same way as it used to > > (no other changes needed) but at a lower rate if the user so chooses. > > > > What do you think about this? > > > > Thanks! > > -- > > Ashutosh > > Sure, just make sure the users know about this. Ok, so the plan now is to just post and review/merge the first 4 patches mostly as is, except that the poll timer cannot be disabled. IMO this should solve the cpu usage issue. Then we can take up the remaining 3 interrupt and flush patches and see if they are really needed and move them forward if they are. > The fact that they can now select timer values that will potentially lead > to the loss of the buffer's data because it was overridden. I think you mean over-written. You are right but I think there is way around this and we can post that patch soon which I think will avoid this issue too. Thanks! -- Ashutosh ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/7] drm/i915/perf: add new open param to configure polling of OA buffer
On 12/03/2020 21:27, Dixit, Ashutosh wrote: On Tue, 03 Mar 2020 14:19:02 -0800, Umesh Nerlige Ramappa wrote: From: Lionel Landwerlin This new parameter let's the application choose how often the OA buffer should be checked on the CPU side for data availability. Longer polling period tend to reduce CPU overhead if the application does not care about somewhat real time data collection. v2: Allow disabling polling completely with 0 value (Lionel) v3: Version the new parameter (Joonas) v4: Rebase (Umesh) Hi Lionel, I was thinking that one way to keep things simple for now (and fix the high cpu usage issue) would be to expose _ONLY_ this OA poll period parameter to user space. That is we don't expose the interrupt or the flush ioctl to user space at this time. This way the user will be able to configure the hrtimer frequency to a lower value to bring down the cpu usage. Also we would disallow disabling the timer (and internally also not use the interrupt). So everything will happen in exactly the same way as it used to (no other changes needed) but at a lower rate if the user so chooses. What do you think about this? Thanks! -- Ashutosh Sure, just make sure the users know about this. The fact that they can now select timer values that will potentially lead to the loss of the buffer's data because it was overridden. Thanks, -Lionel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/7] drm/i915/perf: add new open param to configure polling of OA buffer
On Tue, 03 Mar 2020 14:19:02 -0800, Umesh Nerlige Ramappa wrote: > > From: Lionel Landwerlin > > This new parameter let's the application choose how often the OA > buffer should be checked on the CPU side for data availability. Longer > polling period tend to reduce CPU overhead if the application does not > care about somewhat real time data collection. > > v2: Allow disabling polling completely with 0 value (Lionel) > v3: Version the new parameter (Joonas) > v4: Rebase (Umesh) Hi Lionel, I was thinking that one way to keep things simple for now (and fix the high cpu usage issue) would be to expose _ONLY_ this OA poll period parameter to user space. That is we don't expose the interrupt or the flush ioctl to user space at this time. This way the user will be able to configure the hrtimer frequency to a lower value to bring down the cpu usage. Also we would disallow disabling the timer (and internally also not use the interrupt). So everything will happen in exactly the same way as it used to (no other changes needed) but at a lower rate if the user so chooses. What do you think about this? Thanks! -- Ashutosh ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 4/7] drm/i915/perf: add new open param to configure polling of OA buffer
From: Lionel Landwerlin
This new parameter let's the application choose how often the OA
buffer should be checked on the CPU side for data availability. Longer
polling period tend to reduce CPU overhead if the application does not
care about somewhat real time data collection.
v2: Allow disabling polling completely with 0 value (Lionel)
v3: Version the new parameter (Joonas)
v4: Rebase (Umesh)
Signed-off-by: Lionel Landwerlin
Signed-off-by: Umesh Nerlige Ramappa
---
drivers/gpu/drm/i915/i915_perf.c | 48 +-
drivers/gpu/drm/i915/i915_perf_types.h | 6
include/uapi/drm/i915_drm.h| 10 ++
3 files changed, 56 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 21a63644846f..cf4176f4d11d 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -262,11 +262,11 @@
*/
#define OA_TAIL_MARGIN_NSEC10ULL
-/* frequency for checking whether the OA unit has written new reports to the
- * circular OA buffer...
+/* The default frequency for checking whether the OA unit has written new
+ * reports to the circular OA buffer...
*/
-#define POLL_FREQUENCY 200
-#define POLL_PERIOD (NSEC_PER_SEC / POLL_FREQUENCY)
+#define DEFAULT_POLL_FREQUENCY 200
+#define DEFAULT_POLL_PERIOD (NSEC_PER_SEC / DEFAULT_POLL_FREQUENCY)
/* for sysctl proc_dointvec_minmax of dev.i915.perf_stream_paranoid */
static u32 i915_perf_stream_paranoid = true;
@@ -349,6 +349,8 @@ static const struct i915_oa_format
gen12_oa_formats[I915_OA_FORMAT_MAX] = {
* @oa_periodic: Whether to enable periodic OA unit sampling
* @oa_period_exponent: The OA unit sampling period is derived from this
* @engine: The engine (typically rcs0) being monitored by the OA unit
+ * @poll_oa_period: The period at which the CPU will check for OA data
+ * availability
*
* As read_properties_unlocked() enumerates and validates the properties given
* to open a stream of metrics the configuration is built up in the structure
@@ -368,6 +370,7 @@ struct perf_open_properties {
int oa_period_exponent;
struct intel_engine_cs *engine;
+ u64 poll_oa_period;
};
struct i915_oa_config_bo {
@@ -2642,9 +2645,9 @@ static void i915_oa_stream_enable(struct i915_perf_stream
*stream)
stream->perf->ops.oa_enable(stream);
- if (stream->periodic)
+ if (stream->periodic && stream->poll_oa_period)
hrtimer_start(>poll_check_timer,
- ns_to_ktime(POLL_PERIOD),
+ ns_to_ktime(stream->poll_oa_period),
HRTIMER_MODE_REL_PINNED);
}
@@ -3044,7 +3047,8 @@ static enum hrtimer_restart oa_poll_check_timer_cb(struct
hrtimer *hrtimer)
wake_up(>poll_wq);
}
- hrtimer_forward_now(hrtimer, ns_to_ktime(POLL_PERIOD));
+ hrtimer_forward_now(hrtimer,
+ ns_to_ktime(stream->poll_oa_period));
return HRTIMER_RESTART;
}
@@ -3425,6 +3429,7 @@ i915_perf_open_ioctl_locked(struct i915_perf *perf,
stream->perf = perf;
stream->ctx = specific_ctx;
+ stream->poll_oa_period = props->poll_oa_period;
ret = i915_oa_stream_init(stream, param, props);
if (ret)
@@ -3481,6 +3486,7 @@ static u64 oa_exponent_to_ns(struct i915_perf *perf, int
exponent)
/**
* read_properties_unlocked - validate + copy userspace stream open properties
* @perf: i915 perf instance
+ * @open_flags: Flags set by userspace for the opening of the stream
* @uprops: The array of u64 key value pairs given by userspace
* @n_props: The number of key value pairs expected in @uprops
* @props: The stream configuration built up while validating properties
@@ -3494,6 +3500,7 @@ static u64 oa_exponent_to_ns(struct i915_perf *perf, int
exponent)
* rule out defining new properties with ordering requirements in the future.
*/
static int read_properties_unlocked(struct i915_perf *perf,
+ u32 open_flags,
u64 __user *uprops,
u32 n_props,
struct perf_open_properties *props)
@@ -3502,6 +3509,7 @@ static int read_properties_unlocked(struct i915_perf
*perf,
u32 i;
memset(props, 0, sizeof(struct perf_open_properties));
+ props->poll_oa_period = DEFAULT_POLL_PERIOD;
if (!n_props) {
DRM_DEBUG("No i915 perf properties given\n");
@@ -3617,6 +3625,14 @@ static int read_properties_unlocked(struct i915_perf
*perf,
case DRM_I915_PERF_PROP_HOLD_PREEMPTION:
props->hold_preemption = !!value;
break;
+ case DRM_I915_PERF_PROP_POLL_OA_DELAY:
+ if (value > 0 && value < 10 /* 100us */) {
+ DRM_DEBUG("OA availability timer too
[Intel-gfx] [PATCH 4/7] drm/i915/perf: add new open param to configure polling of OA buffer
This new parameter let's the application choose how often the OA
buffer should be checked on the CPU side for data availability. Longer
polling period tend to reduce CPU overhead if the application does not
care about somewhat real time data collection.
v2: Allow disabling polling completely with 0 value (Lionel)
Signed-off-by: Lionel Landwerlin
---
drivers/gpu/drm/i915/i915_drv.h | 6 +
drivers/gpu/drm/i915/i915_perf.c | 43 ++--
include/uapi/drm/i915_drm.h | 8 ++
3 files changed, 50 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8cb93718224f..d535df8f7d0a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1379,6 +1379,12 @@ struct i915_perf_stream {
* @oa_config: The OA configuration used by the stream.
*/
struct i915_oa_config *oa_config;
+
+ /**
+* @poll_oa_period: The period in nanoseconds at which the OA
+* buffer should be checked for available data.
+*/
+ u64 poll_oa_period;
};
/**
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index b37c7ad0cde6..9ad45ad31b43 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -254,11 +254,11 @@
*/
#define OA_TAIL_MARGIN_NSEC10ULL
-/* frequency for checking whether the OA unit has written new reports to the
- * circular OA buffer...
+/* The default frequency for checking whether the OA unit has written new
+ * reports to the circular OA buffer...
*/
-#define POLL_FREQUENCY 200
-#define POLL_PERIOD (NSEC_PER_SEC / POLL_FREQUENCY)
+#define DEFAULT_POLL_FREQUENCY 200
+#define DEFAULT_POLL_PERIOD (NSEC_PER_SEC / DEFAULT_POLL_FREQUENCY)
/* for sysctl proc_dointvec_minmax of dev.i915.perf_stream_paranoid */
static int zero;
@@ -335,6 +335,8 @@ static const struct i915_oa_format
gen8_plus_oa_formats[I915_OA_FORMAT_MAX] = {
* @oa_format: An OA unit HW report format
* @oa_periodic: Whether to enable periodic OA unit sampling
* @oa_period_exponent: The OA unit sampling period is derived from this
+ * @poll_oa_period: The period at which the CPU will check for OA data
+ * availability
*
* As read_properties_unlocked() enumerates and validates the properties given
* to open a stream of metrics the configuration is built up in the structure
@@ -351,6 +353,7 @@ struct perf_open_properties {
int oa_format;
bool oa_periodic;
int oa_period_exponent;
+ u64 poll_oa_period;
};
static void free_oa_config(struct drm_i915_private *dev_priv,
@@ -1894,9 +1897,9 @@ static void i915_oa_stream_enable(struct i915_perf_stream
*stream)
dev_priv->perf.oa.ops.oa_enable(stream);
- if (dev_priv->perf.oa.periodic)
+ if (dev_priv->perf.oa.periodic && stream->poll_oa_period)
hrtimer_start(_priv->perf.oa.poll_check_timer,
- ns_to_ktime(POLL_PERIOD),
+ ns_to_ktime(stream->poll_oa_period),
HRTIMER_MODE_REL_PINNED);
}
@@ -2260,13 +2263,15 @@ static enum hrtimer_restart
oa_poll_check_timer_cb(struct hrtimer *hrtimer)
struct drm_i915_private *dev_priv =
container_of(hrtimer, typeof(*dev_priv),
perf.oa.poll_check_timer);
+ struct i915_perf_stream *stream = dev_priv->perf.oa.exclusive_stream;
if (oa_buffer_check_unlocked(dev_priv)) {
dev_priv->perf.oa.pollin = true;
wake_up(_priv->perf.oa.poll_wq);
}
- hrtimer_forward_now(hrtimer, ns_to_ktime(POLL_PERIOD));
+ hrtimer_forward_now(hrtimer,
+ ns_to_ktime(stream->poll_oa_period));
return HRTIMER_RESTART;
}
@@ -2587,6 +2592,7 @@ i915_perf_open_ioctl_locked(struct drm_i915_private
*dev_priv,
stream->dev_priv = dev_priv;
stream->ctx = specific_ctx;
+ stream->poll_oa_period = props->poll_oa_period;
ret = i915_oa_stream_init(stream, param, props);
if (ret)
@@ -2642,6 +2648,7 @@ static u64 oa_exponent_to_ns(struct drm_i915_private
*dev_priv, int exponent)
/**
* read_properties_unlocked - validate + copy userspace stream open properties
* @dev_priv: i915 device instance
+ * @open_flags: Flags set by userspace for the opening of the stream
* @uprops: The array of u64 key value pairs given by userspace
* @n_props: The number of key value pairs expected in @uprops
* @props: The stream configuration built up while validating properties
@@ -2655,6 +2662,7 @@ static u64 oa_exponent_to_ns(struct drm_i915_private
*dev_priv, int exponent)
* rule out defining new properties with ordering requirements in the future.
*/
static int read_properties_unlocked(struct drm_i915_private *dev_priv,
+ u32 open_flags,
u64 __user
