[PATCH 0/3] drm/i915: IVB MI_DISPLAY_FLIP cacheline trick

[PATCH 0/3] drm/i915: IVB MI_DISPLAY_FLIP cacheline trick

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

BSpec tells us that the entire MI_DISPLAY_FLIP packet must be contained
within a single cacheline on IVB. This series achieves that.

Changes since my original patch [1]:
* Move the logic into a new intel_ring_begin_cacheline_safe() function
  (as suggested by Daniel).
* Actually handle the case when the ring would wrap due to the extra
  dwords. With the original patch, the MI_DISPLAY_FLIP packet might still
  end up straddling two cachelines in this case.

[1] https://bugs.freedesktop.org/show_bug.cgi?id=74053#c8

Ville Syrjälä (3):
  drm/i915: Rework ring wrap detection
  drm/i915: Introduce intel_ring_begin_cacheline_safe()
  drm/i915: Prevent MI_DISPLAY_FLIP straddling two cachelines on IVB

 drivers/gpu/drm/i915/intel_display.c    | 12 +++--
 drivers/gpu/drm/i915/intel_ringbuffer.c | 88 ++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_ringbuffer.h |  5 ++
 3 files changed, 96 insertions(+), 9 deletions(-)

-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/3] drm/i915: Rework ring wrap detection

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Move the ring wrap detection out from __intel_ring_prepare() all the way
up to intel_ring_begin(). Also convert intel_ring_begin() into a thin
wrapper that just does the wrap detection, and calls
__intel_ring_begin() which contains the code from the original
intel_ring_begin().

This will be useful when we introduce the cacheline safe ring begin
function where the wrap detection won't be quite so simple.

Cc: Bjoern C <lkml@call-home.ch>
Cc: Alexandru DAMIAN <alexandru.damian@intel.com>
Cc: Enrico Tagliavini <enrico.tagliavini@gmail.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index ba686d7..a2bd533 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1595,11 +1595,11 @@ intel_ring_alloc_seqno(struct intel_ring_buffer *ring)
 }
 
 static int __intel_ring_prepare(struct intel_ring_buffer *ring,
-				int bytes)
+				int bytes, bool wrap)
 {
 	int ret;
 
-	if (unlikely(ring->tail + bytes > ring->effective_size)) {
+	if (unlikely(wrap)) {
 		ret = intel_wrap_ring_buffer(ring);
 		if (unlikely(ret))
 			return ret;
@@ -1614,8 +1614,8 @@ static int __intel_ring_prepare(struct intel_ring_buffer *ring,
 	return 0;
 }
 
-int intel_ring_begin(struct intel_ring_buffer *ring,
-		     int num_dwords)
+static int __intel_ring_begin(struct intel_ring_buffer *ring,
+			      int bytes, bool wrap)
 {
 	drm_i915_private_t *dev_priv = ring->dev->dev_private;
 	int ret;
@@ -1625,7 +1625,7 @@ int intel_ring_begin(struct intel_ring_buffer *ring,
 	if (ret)
 		return ret;
 
-	ret = __intel_ring_prepare(ring, num_dwords * sizeof(uint32_t));
+	ret = __intel_ring_prepare(ring, bytes, wrap);
 	if (ret)
 		return ret;
 
@@ -1634,10 +1634,19 @@ int intel_ring_begin(struct intel_ring_buffer *ring,
 	if (ret)
 		return ret;
 
-	ring->space -= num_dwords * sizeof(uint32_t);
+	ring->space -= bytes;
 	return 0;
 }
 
+int intel_ring_begin(struct intel_ring_buffer *ring,
+		     int num_dwords)
+{
+	int bytes = num_dwords * sizeof(uint32_t);
+
+	return __intel_ring_begin(ring, bytes,
+				  ring->tail + bytes > ring->effective_size);
+}
+
 void intel_ring_init_seqno(struct intel_ring_buffer *ring, u32 seqno)
 {
 	struct drm_i915_private *dev_priv = ring->dev->dev_private;
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm/i915: Introduce intel_ring_begin_cacheline_safe()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add a special variant of intel_ring_begin() which allows the caller to
specify that a certain subset of the emitted dwords must fall within
a single cacheline.

intel_ring_begin_cacheline_safe() will return the number of extra dwords
that need to be emitted by the caller to produce the desired result.

Cc: Bjoern C <lkml@call-home.ch>
Cc: Alexandru DAMIAN <alexandru.damian@intel.com>
Cc: Enrico Tagliavini <enrico.tagliavini@gmail.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 67 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_ringbuffer.h |  5 +++
 2 files changed, 72 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index a2bd533..87f5cee 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1647,6 +1647,73 @@ int intel_ring_begin(struct intel_ring_buffer *ring,
 				  ring->tail + bytes > ring->effective_size);
 }
 
+static bool range_within_cacheline(int offset, int start, int end)
+{
+	return ((offset + start) & ~63) == ((offset + end - 1) & ~63);
+}
+
+/**
+ * intel_ring_begin_cacheline_safe - Allocate ring space while
+ * making sure a specific range of dwords lands inside a single
+ * cacheline.
+ *
+ * @ring: the ring buffer
+ * @first_cl_dword: first special dword
+ * @num_cl_dwords: how many dwords after @first_cl_dword must
+ *                 be within the same cacheline.
+ * @num_dwords: total number of dwords
+ * @num_extra_dwords: return number of extra dwords
+ *
+ * Allocate ring space line intel_ring_begin but make sure
+ * @num_cl_dwords dwords starting at @first_cl_dword all
+ * end up within the same cacheline. @num_extra_dwords will
+ * indicate how many extra dwords have to emitted. The extra
+ * dwords must be emitted at or before the original
+ * @first_cl_dword position.
+ */
+int intel_ring_begin_cacheline_safe(struct intel_ring_buffer *ring,
+				    int first_cl_dword,
+				    int num_cl_dwords,
+				    int num_dwords,
+				    int *num_extra_dwords)
+{
+	int start = first_cl_dword * sizeof(uint32_t);
+	int end = start + num_cl_dwords * sizeof(uint32_t);
+	int bytes = num_dwords * sizeof(uint32_t);
+	int tail = ring->tail;
+	int extra = 0;
+
+	if (WARN_ON(end - start > 64))
+		return -EINVAL;
+
+	/* Need to wrap in any case? */
+	if (tail + bytes > ring->effective_size)
+		tail = 0;
+
+	if (!range_within_cacheline(tail, start, end)) {
+		extra = 64 - ((tail + start) & 63);
+
+		/* Need to wrap due to the extra dwords? */
+		if (tail + extra + bytes > ring->effective_size) {
+			tail = 0;
+
+			/* Recalculate extra after the wrap */
+			extra = 0;
+			if (!range_within_cacheline(tail, start, end))
+				extra = 64 - ((tail + start) & 63);
+		}
+	}
+
+	/* Sanity checks. These should never happen. */
+	if (WARN_ON(!range_within_cacheline(tail + extra, start, end) ||
+		    tail + extra + bytes > ring->effective_size))
+		return -EINVAL;
+
+	*num_extra_dwords = extra / sizeof(uint32_t);
+
+	return __intel_ring_begin(ring, extra + bytes, tail != ring->tail);
+}
+
 void intel_ring_init_seqno(struct intel_ring_buffer *ring, u32 seqno)
 {
 	struct drm_i915_private *dev_priv = ring->dev->dev_private;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 38c757e..e48e81e 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -235,6 +235,11 @@ intel_write_status_page(struct intel_ring_buffer *ring,
 void intel_cleanup_ring_buffer(struct intel_ring_buffer *ring);
 
 int __must_check intel_ring_begin(struct intel_ring_buffer *ring, int n);
+int __must_check intel_ring_begin_cacheline_safe(struct intel_ring_buffer *ring,
+						 int first_cl_dword,
+						 int num_cl_dwords,
+						 int num_dwords,
+						 int *num_extra_dwords);
 static inline void intel_ring_emit(struct intel_ring_buffer *ring,
 				   u32 data)
 {
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 3/3] drm/i915: Prevent MI_DISPLAY_FLIP straddling two cachelines on IVB

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

According to BSpec the entire MI_DISPLAY_FLIP packet must be contained
in a single cacheline. Make sure that happens.

v2: Use intel_ring_begin_cacheline_safe()

Cc: Bjoern C <lkml@call-home.ch>
Cc: Alexandru DAMIAN <alexandru.damian@intel.com>
Cc: Enrico Tagliavini <enrico.tagliavini@gmail.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=74053
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0c25310..361a9a9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8579,7 +8579,7 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_ring_buffer *ring;
 	uint32_t plane_bit = 0;
-	int len, ret;
+	int extra, len = 0, ret;
 
 	ring = obj->ring;
 	if (IS_VALLEYVIEW(dev) || ring == NULL || ring->id != RCS)
@@ -8605,11 +8605,14 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
 		goto err_unpin;
 	}
 
-	len = 4;
 	if (ring->id == RCS)
 		len += 6;
 
-	ret = intel_ring_begin(ring, len);
+	/*
+	 * BSpec MI_DISPLAY_FLIP for IVB:
+	 * "The full packet must be contained within the same cache line."
+	 */
+	ret = intel_ring_begin_cacheline_safe(ring, len, 4, len + 4, &extra);
 	if (ret)
 		goto err_unpin;
 
@@ -8634,6 +8637,9 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
 		intel_ring_emit(ring, ring->scratch.gtt_offset + 256);
 	}
 
+	while (extra--)
+		intel_ring_emit(ring, MI_NOOP);
+
 	intel_ring_emit(ring, MI_DISPLAY_FLIP_I915 | plane_bit);
 	intel_ring_emit(ring, (fb->pitches[0] | obj->tiling_mode));
 	intel_ring_emit(ring, i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset);
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/3] drm/i915: Prevent MI_DISPLAY_FLIP straddling two cachelines on IVB

[Chris Wilson]

On Tue, Feb 11, 2014 at 03:55:50PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> According to BSpec the entire MI_DISPLAY_FLIP packet must be contained
> in a single cacheline. Make sure that happens.
> 
> v2: Use intel_ring_begin_cacheline_safe()

Ugh, no. Let's not make intel_ring_begin() any more complicated and just
introduce a function to align the current head in the ringbuffer to a
cacheline. Especially with such an interface that is hard to get right.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v2 3/3] drm/i915: Prevent MI_DISPLAY_FLIP straddling two cachelines on IVB

[Ville Syrjälä]

On Tue, Feb 11, 2014 at 02:14:28PM +0000, Chris Wilson wrote:
> On Tue, Feb 11, 2014 at 03:55:50PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > According to BSpec the entire MI_DISPLAY_FLIP packet must be contained
> > in a single cacheline. Make sure that happens.
> > 
> > v2: Use intel_ring_begin_cacheline_safe()
> 
> Ugh, no. Let's not make intel_ring_begin() any more complicated and just
> introduce a function to align the current head in the ringbuffer to a
> cacheline. Especially with such an interface that is hard to get right.

This doesn't make intel_ring_begin() itself more complicated, but I
guess you meant that the new special version is too complicated for
your taste?

So I guess you want somehting like this:

int ring_align()
{
	nops = (64 - (tail & 63)) / 4;

	ret = ring_begin(nops);
	if (ret)
		return ret;
	while (nops--)
		ring_emit(MI_NOOP);
	ring_advance()

	return 0;
}

int queue_flip()
{
	ret = ring_align();
	if (ret)
		return ret;

	ret = ring_begin(len);
	if (ret)
		return ret;
	if (RCS)
		emit LRI DERRMR;
	emit MI_DISPLAY_FLIP;
	ring_advance()

	return 0;
}

So we end up relying on the fact that the entire
LRI+MI_DISPLAY_FLIP sequence will fit within one cacheline. Although if
that would be problem I suppose we could always emit the LRI before
aligning the tail.

If no one is concerned about the useless MI_NOOPs we'll be emitting for
most flips, I guess it's a good enough solution.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v2 3/3] drm/i915: Prevent MI_DISPLAY_FLIP straddling two cachelines on IVB

[Daniel Vetter]

On Tue, Feb 11, 2014 at 05:50:05PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 11, 2014 at 02:14:28PM +0000, Chris Wilson wrote:
> > On Tue, Feb 11, 2014 at 03:55:50PM +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > According to BSpec the entire MI_DISPLAY_FLIP packet must be contained
> > > in a single cacheline. Make sure that happens.
> > > 
> > > v2: Use intel_ring_begin_cacheline_safe()
> > 
> > Ugh, no. Let's not make intel_ring_begin() any more complicated and just
> > introduce a function to align the current head in the ringbuffer to a
> > cacheline. Especially with such an interface that is hard to get right.
> 
> This doesn't make intel_ring_begin() itself more complicated, but I
> guess you meant that the new special version is too complicated for
> your taste?
> 
> So I guess you want somehting like this:
> 
> int ring_align()
> {
> 	nops = (64 - (tail & 63)) / 4;
> 
> 	ret = ring_begin(nops);
> 	if (ret)
> 		return ret;
> 	while (nops--)
> 		ring_emit(MI_NOOP);
> 	ring_advance()
> 
> 	return 0;
> }
> 
> int queue_flip()
> {
> 	ret = ring_align();
> 	if (ret)
> 		return ret;
> 
> 	ret = ring_begin(len);
> 	if (ret)
> 		return ret;
> 	if (RCS)
> 		emit LRI DERRMR;
> 	emit MI_DISPLAY_FLIP;
> 	ring_advance()
> 
> 	return 0;
> }
> 
> So we end up relying on the fact that the entire
> LRI+MI_DISPLAY_FLIP sequence will fit within one cacheline. Although if
> that would be problem I suppose we could always emit the LRI before
> aligning the tail.
> 
> If no one is concerned about the useless MI_NOOPs we'll be emitting for
> most flips, I guess it's a good enough solution.

Yeah Chris&me discussed this quickly on irc and I concur with him that an
intel_ring_cacheline_align(ring) helper is sufficient and much less crazy
;-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v2 3/3] drm/i915: Prevent MI_DISPLAY_FLIP straddling two cachelines on IVB

[Chris Wilson]

On Tue, Feb 11, 2014 at 05:50:05PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 11, 2014 at 02:14:28PM +0000, Chris Wilson wrote:
> > On Tue, Feb 11, 2014 at 03:55:50PM +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > According to BSpec the entire MI_DISPLAY_FLIP packet must be contained
> > > in a single cacheline. Make sure that happens.
> > > 
> > > v2: Use intel_ring_begin_cacheline_safe()
> > 
> > Ugh, no. Let's not make intel_ring_begin() any more complicated and just
> > introduce a function to align the current head in the ringbuffer to a
> > cacheline. Especially with such an interface that is hard to get right.
> 
> This doesn't make intel_ring_begin() itself more complicated, but I
> guess you meant that the new special version is too complicated for
> your taste?

Yes. But it also raises the question of whether I want to use
intel_ring_begin() or intel_ring_begin_safe() everytime I touch the
ring. The slight loss of efficiency is well worth the clarity of
saying:
  ring_align();
  ring_begin()
for those special cases.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre