[Intel-gfx] [PATCH v5 03/15] drm/i915/perf: avoid read back of head register

[Lionel Landwerlin]

From: Robert Bragg <robert at sixbynine.org>

There's no need for the driver to keep reading back the head pointer
from hardware since the hardware doesn't update it automatically. This
way we can treat any invalid head pointer value as a software/driver
bug instead of spurious hardware behaviour.

This change is also a small stepping stone towards re-working how
the head and tail state is managed as part of an improved workaround
for the tail register race condition.

Signed-off-by: Robert Bragg <robert at sixbynine.org>
Reviewed-by: Matthew Auld <matthew.auld at intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  | 11 ++++++++++
 drivers/gpu/drm/i915/i915_perf.c | 46 ++++++++++++++++++----------------------
 2 files changed, 32 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 357b6c6c2f04..b618a8bc54f6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2469,6 +2469,17 @@ struct drm_i915_private {
 				u8 *vaddr;
 				int format;
 				int format_size;
+
+				/**
+				 * Although we can always read back the head
+				 * pointer register, we prefer to avoid
+				 * trusting the HW state, just to avoid any
+				 * risk that some hardware condition could
+				 * somehow bump the head pointer unpredictably
+				 * and cause us to forward the wrong OA buffer
+				 * data to userspace.
+				 */
+				u32 head;
 			} oa_buffer;

 			u32 gen7_latched_oastatus1;
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index f59f6dd20922..f47d1cc2144b 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -322,9 +322,8 @@ struct perf_open_properties {
 static bool gen7_oa_buffer_is_empty_fop_unlocked(struct drm_i915_private *dev_priv)
 {
 	int report_size = dev_priv->perf.oa.oa_buffer.format_size;
-	u32 oastatus2 = I915_READ(GEN7_OASTATUS2);
 	u32 oastatus1 = I915_READ(GEN7_OASTATUS1);
-	u32 head = oastatus2 & GEN7_OASTATUS2_HEAD_MASK;
+	u32 head = dev_priv->perf.oa.oa_buffer.head;
 	u32 tail = oastatus1 & GEN7_OASTATUS1_TAIL_MASK;

 	return OA_TAKEN(tail, head) <
@@ -458,16 +457,24 @@ static int gen7_append_oa_reports(struct i915_perf_stream *stream,
 		return -EIO;

 	head = *head_ptr - gtt_offset;
+
+	/* An out of bounds or misaligned head pointer implies a driver bug
+	 * since we are in full control of head pointer which should only
+	 * be incremented by multiples of the report size (notably also
+	 * all a power of two).
+	 */
+	if (WARN_ONCE(head > OA_BUFFER_SIZE || head % report_size,
+		      "Inconsistent OA buffer head pointer = %u\n", head))
+		return -EIO;
+
 	tail -= gtt_offset;

 	/* The OA unit is expected to wrap the tail pointer according to the OA
-	 * buffer size and since we should never write a misaligned head
-	 * pointer we don't expect to read one back either...
+	 * buffer size
 	 */
-	if (tail > OA_BUFFER_SIZE || head > OA_BUFFER_SIZE ||
-	    head % report_size) {
-		DRM_ERROR("Inconsistent OA buffer pointer (head = %u, tail = %u): force restart\n",
-			  head, tail);
+	if (tail > OA_BUFFER_SIZE) {
+		DRM_ERROR("Inconsistent OA buffer tail pointer = %u: force restart\n",
+			  tail);
 		dev_priv->perf.oa.ops.oa_disable(dev_priv);
 		dev_priv->perf.oa.ops.oa_enable(dev_priv);
 		*head_ptr = I915_READ(GEN7_OASTATUS2) &
@@ -562,8 +569,6 @@ static int gen7_oa_read(struct i915_perf_stream *stream,
 			size_t *offset)
 {
 	struct drm_i915_private *dev_priv = stream->dev_priv;
-	int report_size = dev_priv->perf.oa.oa_buffer.format_size;
-	u32 oastatus2;
 	u32 oastatus1;
 	u32 head;
 	u32 tail;
@@ -572,10 +577,9 @@ static int gen7_oa_read(struct i915_perf_stream *stream,
 	if (WARN_ON(!dev_priv->perf.oa.oa_buffer.vaddr))
 		return -EIO;

-	oastatus2 = I915_READ(GEN7_OASTATUS2);
 	oastatus1 = I915_READ(GEN7_OASTATUS1);

-	head = oastatus2 & GEN7_OASTATUS2_HEAD_MASK;
+	head = dev_priv->perf.oa.oa_buffer.head;
 	tail = oastatus1 & GEN7_OASTATUS1_TAIL_MASK;

 	/* XXX: On Haswell we don't have a safe way to clear oastatus1
@@ -616,10 +620,9 @@ static int gen7_oa_read(struct i915_perf_stream *stream,
 		dev_priv->perf.oa.ops.oa_disable(dev_priv);
 		dev_priv->perf.oa.ops.oa_enable(dev_priv);

-		oastatus2 = I915_READ(GEN7_OASTATUS2);
 		oastatus1 = I915_READ(GEN7_OASTATUS1);

-		head = oastatus2 & GEN7_OASTATUS2_HEAD_MASK;
+		head = dev_priv->perf.oa.oa_buffer.head;
 		tail = oastatus1 & GEN7_OASTATUS1_TAIL_MASK;
 	}

@@ -635,17 +638,6 @@ static int gen7_oa_read(struct i915_perf_stream *stream,
 	ret = gen7_append_oa_reports(stream, buf, count, offset,
 				     &head, tail);

-	/* All the report sizes are a power of two and the
-	 * head should always be incremented by some multiple
-	 * of the report size.
-	 *
-	 * A warning here, but notably if we later read back a
-	 * misaligned pointer we will treat that as a bug since
-	 * it could lead to a buffer overrun.
-	 */
-	WARN_ONCE(head & (report_size - 1),
-		  "i915: Writing misaligned OA head pointer");
-
 	/* Note: we update the head pointer here even if an error
 	 * was returned since the error may represent a short read
 	 * where some some reports were successfully copied.
@@ -653,6 +645,7 @@ static int gen7_oa_read(struct i915_perf_stream *stream,
 	I915_WRITE(GEN7_OASTATUS2,
 		   ((head & GEN7_OASTATUS2_HEAD_MASK) |
 		    OA_MEM_SELECT_GGTT));
+	dev_priv->perf.oa.oa_buffer.head = head;

 	return ret;
 }
@@ -834,7 +827,10 @@ static void gen7_init_oa_buffer(struct drm_i915_private *dev_priv)
 	 * before OASTATUS1, but after OASTATUS2
 	 */
 	I915_WRITE(GEN7_OASTATUS2, gtt_offset | OA_MEM_SELECT_GGTT); /* head */
+	dev_priv->perf.oa.oa_buffer.head = gtt_offset;
+
 	I915_WRITE(GEN7_OABUFFER, gtt_offset);
+
 	I915_WRITE(GEN7_OASTATUS1, gtt_offset | OABUFFER_SIZE_16M); /* tail */

 	/* On Haswell we have to track which OASTATUS1 flags we've
--
2.11.0