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

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

[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.

Changes since v1 [2]:
* Just do the simple cacheline alignment trick Chris suggested

[1] https://bugs.freedesktop.org/show_bug.cgi?id=74053#
[2] http://lists.freedesktop.org/archives/intel-gfx/2014-February/039921.html

Ville Syrjälä (2):
  drm/i915: Add intel_ring_cachline_align()
  drm/i915: Prevent MI_DISPLAY_FLIP straddling two cachelines on IVB

 drivers/gpu/drm/i915/intel_display.c    | 14 ++++++++++++++
 drivers/gpu/drm/i915/intel_ringbuffer.c | 21 +++++++++++++++++++++
 drivers/gpu/drm/i915/intel_ringbuffer.h |  1 +
 3 files changed, 36 insertions(+)

-- 
1.8.3.2

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

[PATCH 1/2] drm/i915: Add intel_ring_cachline_align()

[ville.syrjala]

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

intel_ring_cachline_align() emits MI_NOOPs until the ring tail is
aligned to a cacheline boundary.

Cc: Bjoern C <lkml@call-home.ch>
Cc: Alexandru DAMIAN <alexandru.damian@intel.com>
Cc: Enrico Tagliavini <enrico.tagliavini@gmail.com>
Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 21 +++++++++++++++++++++
 drivers/gpu/drm/i915/intel_ringbuffer.h |  1 +
 2 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index ba686d7..8c1c0bc 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1638,6 +1638,27 @@ int intel_ring_begin(struct intel_ring_buffer *ring,
 	return 0;
 }
 
+/* Align the ring tail to a cacheline boundary */
+int intel_ring_cacheline_align(struct intel_ring_buffer *ring)
+{
+	int num_dwords = (64 - (ring->tail & 63)) / sizeof(uint32_t);
+	int ret;
+
+	if (num_dwords == 0)
+		return 0;
+
+	ret = intel_ring_begin(ring, num_dwords);
+	if (ret)
+		return ret;
+
+	while (num_dwords--)
+		intel_ring_emit(ring, MI_NOOP);
+
+	intel_ring_advance(ring);
+
+	return 0;
+}
+
 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..08b91c6 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -235,6 +235,7 @@ 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_cacheline_align(struct intel_ring_buffer *ring);
 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 v3 2/2] 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()
v3: Use intel_ring_cacheline_align() (Chris)

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 | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0c25310..37527d4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8609,6 +8609,20 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
 	if (ring->id == RCS)
 		len += 6;
 
+	/*
+	 * BSpec MI_DISPLAY_FLIP for IVB:
+	 * "The full packet must be contained within the same cache line."
+	 *
+	 * Currently the LRI+SRM+MI_DISPLAY_FLIP all fit within the same
+	 * cacheline, if we ever start emitting more commands before
+	 * the MI_DISPLAY_FLIP we may need to first emit everything else,
+	 * then do the cacheline alignment, and finally emit the
+	 * MI_DISPLAY_FLIP.
+	 */
+	ret = intel_ring_cacheline_align(ring);
+	if (ret)
+		goto err_unpin;
+
 	ret = intel_ring_begin(ring, len);
 	if (ret)
 		goto err_unpin;
-- 
1.8.3.2

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

Re: [PATCH v3 2/2] drm/i915: Prevent MI_DISPLAY_FLIP straddling two cachelines on IVB

[Chris Wilson]

On Tue, Feb 11, 2014 at 07:52:06PM +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()
> v3: Use intel_ring_cacheline_align() (Chris)
> 
> 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>

I would have used intel_ring_align_cacheline() as it seems more natural
for me to say...

Both,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v3 2/2] drm/i915: Prevent MI_DISPLAY_FLIP straddling two cachelines on IVB

[Ville Syrjälä]

On Tue, Feb 11, 2014 at 08:19:13PM +0000, Chris Wilson wrote:
> On Tue, Feb 11, 2014 at 07:52:06PM +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()
> > v3: Use intel_ring_cacheline_align() (Chris)
> > 
> > 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>
> 
> I would have used intel_ring_align_cacheline() as it seems more natural
> for me to say...

I had it as intel_ring_align_to_cacheline(), but then changed it when I
saw Daniel's suggestion for the name. So you can blame Daniel for this ;)
But I have no emotional attachment to the chosen name. So if you prefer
we rename it, I'm fine with that.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v3 2/2] drm/i915: Prevent MI_DISPLAY_FLIP straddling two cachelines on IVB

[Daniel Vetter]

On Tue, Feb 11, 2014 at 08:19:13PM +0000, Chris Wilson wrote:
> On Tue, Feb 11, 2014 at 07:52:06PM +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()
> > v3: Use intel_ring_cacheline_align() (Chris)
> > 
> > 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>
> 
> I would have used intel_ring_align_cacheline() as it seems more natural
> for me to say...
> 
> Both,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Both merged to -fixes with cc: stable slapped on them.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch