[Intel-gfx] [PATCH v2 5/5] drm/i915/perf: improve tail race workaround

[Robert Bragg]

There's a HW race condition between OA unit tail pointer register
updates and writes to memory whereby the tail pointer can sometimes get
ahead of what's been written out to the OA buffer so far (in terms of
what's visible to the CPU).

Although this can be observed explicitly while copying reports to
userspace by checking for a zeroed report-id field in tail reports, we
want to account for this earlier, as part of the _oa_buffer_check to
avoid lots of redundant read() attempts.

Previously the driver used to define an effective tail pointer that
lagged the real pointer by a 'tail margin' measured in bytes derived
from OA_TAIL_MARGIN_NSEC and the configured sampling frequency.
Unfortunately this was flawed considering that the OA unit may also
automatically generate non-periodic reports (such as on context switch)
or the OA unit may be enabled without any periodic sampling.

This improves how we define a tail pointer for reading that lags the
real tail pointer by at least %OA_TAIL_MARGIN_NSEC nanoseconds, which
gives enough time for the corresponding reports to become visible to the
CPU.

The driver now maintains two tail pointers:
 1) An 'aging' tail with an associated timestamp that is tracked until we
can trust the corresponding data is visible to the CPU; at which point
it is considered 'aged'.
 2) An 'aged' tail that can be used for read()ing.

The two separate pointers let us decouple read()s from tail pointer aging.

The tail pointers are checked and updated at a limited rate within a
hrtimer callback (the same callback that is used for delivering POLLIN
events) and since we're now measuring the wall clock time elapsed since
a given tail pointer was read the mechanism no longer cares about
the OA unit's periodic sampling frequency.

The natural place to handle the tail pointer updates was in
gen7_oa_buffer_is_empty() which is called as part of blocking reads and
the hrtimer callback used for polling, and so this was renamed to
oa_buffer_check() considering the added side effect while checking
whether the buffer contains data.

Signed-off-by: Robert Bragg 
Reviewed-by: Matthew Auld 
---
 drivers/gpu/drm/i915/i915_drv.h  |  60 -
 drivers/gpu/drm/i915/i915_perf.c | 277 ++-
 2 files changed, 241 insertions(+), 96 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b990549cfe65..dfd4d9b299b7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2051,7 +2051,7 @@ struct i915_oa_ops {
size_t *offset);
 
/**
-* @oa_buffer_is_empty: Check if OA buffer empty (false positives OK)
+* @oa_buffer_check: Check for OA buffer data + update tail
 *
 * This is either called via fops or the poll check hrtimer (atomic
 * ctx) without any locks taken.
@@ -2064,7 +2064,7 @@ struct i915_oa_ops {
 * here, which will be handled gracefully - likely resulting in an
 * %EAGAIN error for userspace.
 */
-   bool (*oa_buffer_is_empty)(struct drm_i915_private *dev_priv);
+   bool (*oa_buffer_check)(struct drm_i915_private *dev_priv);
 };
 
 struct intel_cdclk_state {
@@ -2412,9 +2412,6 @@ struct drm_i915_private {
 
bool periodic;
int period_exponent;
-   int timestamp_frequency;
-
-   int tail_margin;
 
int metrics_set;
 
@@ -2430,6 +2427,59 @@ struct drm_i915_private {
int format_size;
 
/**
+* Locks reads and writes to all head/tail state
+*
+* Consider: the head and tail pointer state
+* needs to be read consistently from a hrtimer
+* callback (atomic context) and read() fop
+* (user context) with tail pointer updates
+* happening in atomic context and head updates
+* in user context and the (unlikely)
+* possibility of read() errors needing to
+* reset all head/tail state.
+*
+* Note: Contention or performance aren't
+* currently a significant concern here
+* considering the relatively low frequency of
+* hrtimer callbacks (5ms period) and that
+* reads typically only happen in response to a
+* hrtimer event and likely complete before the
+* next callback.
+*
+ 

[Intel-gfx] [PATCH v2 5/5] drm/i915/perf: improve tail race workaround

[Robert Bragg]

There's a HW race condition between OA unit tail pointer register
updates and writes to memory whereby the tail pointer can sometimes get
ahead of what's been written out to the OA buffer so far (in terms of
what's visible to the CPU).

Although this can be observed explicitly while copying reports to
userspace by checking for a zeroed report-id field in tail reports, we
want to account for this earlier, as part of the _oa_buffer_check to
avoid lots of redundant read() attempts.

Previously the driver used to define an effective tail pointer that
lagged the real pointer by a 'tail margin' measured in bytes derived
from OA_TAIL_MARGIN_NSEC and the configured sampling frequency.
Unfortunately this was flawed considering that the OA unit may also
automatically generate non-periodic reports (such as on context switch)
or the OA unit may be enabled without any periodic sampling.

This improves how we define a tail pointer for reading that lags the
real tail pointer by at least %OA_TAIL_MARGIN_NSEC nanoseconds, which
gives enough time for the corresponding reports to become visible to the
CPU.

The driver now maintains two tail pointers:
 1) An 'aging' tail with an associated timestamp that is tracked until we
can trust the corresponding data is visible to the CPU; at which point
it is considered 'aged'.
 2) An 'aged' tail that can be used for read()ing.

The two separate pointers let us decouple read()s from tail pointer aging.

The tail pointers are checked and updated at a limited rate within a
hrtimer callback (the same callback that is used for delivering POLLIN
events) and since we're now measuring the wall clock time elapsed since
a given tail pointer was read the mechanism no longer cares about
the OA unit's periodic sampling frequency.

The natural place to handle the tail pointer updates was in
gen7_oa_buffer_is_empty() which is called as part of blocking reads and
the hrtimer callback used for polling, and so this was renamed to
oa_buffer_check() considering the added side effect while checking
whether the buffer contains data.

Signed-off-by: Robert Bragg 
Reviewed-by: Matthew Auld 
---
 drivers/gpu/drm/i915/i915_drv.h  |  60 -
 drivers/gpu/drm/i915/i915_perf.c | 277 ++-
 2 files changed, 241 insertions(+), 96 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a7e8936ce0b0..f34b7f5022fc 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2038,7 +2038,7 @@ struct i915_oa_ops {
size_t *offset);
 
/**
-* @oa_buffer_is_empty: Check if OA buffer empty (false positives OK)
+* @oa_buffer_check: Check for OA buffer data + update tail
 *
 * This is either called via fops or the poll check hrtimer (atomic
 * ctx) without any locks taken.
@@ -2051,7 +2051,7 @@ struct i915_oa_ops {
 * here, which will be handled gracefully - likely resulting in an
 * %EAGAIN error for userspace.
 */
-   bool (*oa_buffer_is_empty)(struct drm_i915_private *dev_priv);
+   bool (*oa_buffer_check)(struct drm_i915_private *dev_priv);
 };
 
 struct drm_i915_private {
@@ -2381,9 +2381,6 @@ struct drm_i915_private {
 
bool periodic;
int period_exponent;
-   int timestamp_frequency;
-
-   int tail_margin;
 
int metrics_set;
 
@@ -2399,6 +2396,59 @@ struct drm_i915_private {
int format_size;
 
/**
+* Locks reads and writes to all head/tail state
+*
+* Consider: the head and tail pointer state
+* needs to be read consistently from a hrtimer
+* callback (atomic context) and read() fop
+* (user context) with tail pointer updates
+* happening in atomic context and head updates
+* in user context and the (unlikely)
+* possibility of read() errors needing to
+* reset all head/tail state.
+*
+* Note: Contention or performance aren't
+* currently a significant concern here
+* considering the relatively low frequency of
+* hrtimer callbacks (5ms period) and that
+* reads typically only happen in response to a
+* hrtimer event and likely complete before the
+* next callback.
+*
+