[PATCH v2 0/5] drm/i915: mmio vs. CS flip race fix

[PATCH v2 0/5] drm/i915: mmio vs. CS flip race fix

[ville.syrjala]

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

This is the second version of the mmio vs. CS flip race fix.

Since v1 I fixed one if Chris's complaints. I still left the
flip_count_after_eq() untouched so that it's reasonably consistent
with the vbl_count_after_eq() I have in my watermark series.

The IPS vs. vblank wait removal patches got reordered.

And I realized we have another race with the primary enable/disable
logic in the sprite code, so I added a fix for that as well.

And finally I tried to kill off some code duplication in the .queue_flip()
functions.

I also wrote a few testcases that exercise these bugs.

Ville Syrjälä (5):
  drm/i915: Fix mmio vs. CS flip race on ILK+
  drm/i915: Wait for vblank in hsw_enable_ips()
  drm/i915: Drop the excessive vblank waits from modeset codepaths
  drm/i915: Wait for pending page flips before enabling/disabling the
    primary plane
  drm/i915: Move buffer pinning and ring selection to
    intel_crtc_page_flip()

 drivers/gpu/drm/i915/i915_drv.h      |   1 +
 drivers/gpu/drm/i915/i915_reg.h      |   1 +
 drivers/gpu/drm/i915/intel_display.c | 184 +++++++++++++++++------------------
 drivers/gpu/drm/i915/intel_drv.h     |   3 +
 drivers/gpu/drm/i915/intel_sprite.c  |   9 +-
 5 files changed, 102 insertions(+), 96 deletions(-)

-- 
1.8.3.2

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

[PATCH v2 1/5] drm/i915: Fix mmio vs. CS flip race on ILK+

[ville.syrjala]

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

Starting from ILK, mmio flips also cause a flip done interrupt to be
signalled. This means if we first do a set_base and follow it
immediately with the CS flip, we might mistake the flip done interrupt
caused by the set_base as the flip done interrupt caused by the CS
flip.

The hardware has a flip counter which increments every time a mmio or
CS flip is issued. It basically counts the number of DSPSURF register
writes. This means we can sample the counter before we put the CS
flip into the ring, and then when we get a flip done interrupt we can
check whether the CS flip has actually performed the surface address
update, or if the interrupt was caused by a previous but yet
unfinished mmio flip.

Even with the flip counter we still have a race condition of the CS flip
base address update happens after the mmio flip done interrupt was
raised but not yet processed by the driver. When the interrupt is
eventually processed, the flip counter will already indicate that the
CS flip has been executed, but it would not actually complete until the
next start of vblank. We can use the DSPSURFLIVE register to check
whether the hardware is actually scanning out of the buffer we expect,
or if we managed hit this race window.

This covers all the cases where the CS flip actually changes the base
address. If the base address remains unchanged, we might still complete
the CS flip before it has actually completed. But since the address
didn't change anyway, the premature flip completion can't result in
userspace overwriting data that's still being scanned out.

CTG already has the flip counter and DSPSURFLIVE registers, and
although the flip done interrupt is still limited to CS flips alone,
the code now also checks the flip counter on CTG as well.

v2: s/dspsurf/gtt_offset/ (Chris)

Testcase: igt/kms_mmio_vs_cs_flip/setcrtc_vs_cs_flip
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=73027
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      |  1 +
 drivers/gpu/drm/i915/intel_display.c | 73 ++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_drv.h     |  2 +
 3 files changed, 69 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8f84555..c28169c 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3638,6 +3638,7 @@ enum punit_power_well {
 #define _PIPEA_FRMCOUNT_GM45	0x70040
 #define _PIPEA_FLIPCOUNT_GM45	0x70044
 #define PIPE_FRMCOUNT_GM45(pipe) _PIPE2(pipe, _PIPEA_FRMCOUNT_GM45)
+#define PIPE_FLIPCOUNT_GM45(pipe) _PIPE2(pipe, _PIPEA_FLIPCOUNT_GM45)
 
 /* Cursor A & B regs */
 #define _CURACNTR		(dev_priv->info.display_mmio_offset + 0x70080)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1390ab5..32c6c16 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8577,6 +8577,48 @@ void intel_finish_page_flip_plane(struct drm_device *dev, int plane)
 	do_intel_finish_page_flip(dev, crtc);
 }
 
+/* Is 'a' after or equal to 'b'? */
+static bool flip_count_after_eq(u32 a, u32 b)
+{
+	return !((a - b) & 0x80000000);
+}
+
+static bool page_flip_finished(struct intel_crtc *crtc)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	/*
+	 * The relevant registers doen't exist on pre-ctg.
+	 * As the flip done interrupt doesn't trigger for mmio
+	 * flips on gmch platforms, a flip count check isn't
+	 * really needed there. But since ctg has the registers,
+	 * include it in the check anyway.
+	 */
+	if (INTEL_INFO(dev)->gen < 5 && !IS_G4X(dev))
+		return true;
+
+	/*
+	 * A DSPSURFLIVE check isn't enough in case the mmio and CS flips
+	 * used the same base address. In that case the mmio flip might
+	 * have completed, but the CS hasn't even executed the flip yet.
+	 *
+	 * A flip count check isn't enough as the CS might have updated
+	 * the base address just after start of vblank, but before we
+	 * managed to process the interrupt. This means we'd complete the
+	 * CS flip too soon.
+	 *
+	 * Combining both checks should get us a good enough result. It may
+	 * still happen that the CS flip has been executed, but has not
+	 * yet actually completed. But in case the base address is the same
+	 * anyway, we don't really care.
+	 */
+	return (I915_READ(DSPSURFLIVE(crtc->plane)) & ~0xfff) ==
+		crtc->unpin_work->gtt_offset &&
+		flip_count_after_eq(I915_READ(PIPE_FLIPCOUNT_GM45(crtc->pipe)),
+				    crtc->unpin_work->flip_count);
+}
+
 void intel_prepare_page_flip(struct drm_device *dev, int plane)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -8589,7 +8631,7 @@ void intel_prepare_page_flip(struct drm_device *dev, int plane)
 	 * is also accompanied by a spurious intel_prepare_page_flip().
 	 */
 	spin_lock_irqsave(&dev->event_lock, flags);
-	if (intel_crtc->unpin_work)
+	if (intel_crtc->unpin_work && page_flip_finished(intel_crtc))
 		atomic_inc_not_zero(&intel_crtc->unpin_work->pending);
 	spin_unlock_irqrestore(&dev->event_lock, flags);
 }
@@ -8619,6 +8661,9 @@ static int intel_gen2_queue_flip(struct drm_device *dev,
 	if (ret)
 		goto err;
 
+	intel_crtc->unpin_work->gtt_offset =
+		i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
+
 	ret = intel_ring_begin(ring, 6);
 	if (ret)
 		goto err_unpin;
@@ -8635,7 +8680,7 @@ static int intel_gen2_queue_flip(struct drm_device *dev,
 	intel_ring_emit(ring, MI_DISPLAY_FLIP |
 			MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
 	intel_ring_emit(ring, fb->pitches[0]);
-	intel_ring_emit(ring, i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset);
+	intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset);
 	intel_ring_emit(ring, 0); /* aux display base address, unused */
 
 	intel_mark_page_flip_active(intel_crtc);
@@ -8664,6 +8709,9 @@ static int intel_gen3_queue_flip(struct drm_device *dev,
 	if (ret)
 		goto err;
 
+	intel_crtc->unpin_work->gtt_offset =
+		i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
+
 	ret = intel_ring_begin(ring, 6);
 	if (ret)
 		goto err_unpin;
@@ -8677,7 +8725,7 @@ static int intel_gen3_queue_flip(struct drm_device *dev,
 	intel_ring_emit(ring, MI_DISPLAY_FLIP_I915 |
 			MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
 	intel_ring_emit(ring, fb->pitches[0]);
-	intel_ring_emit(ring, i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset);
+	intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset);
 	intel_ring_emit(ring, MI_NOOP);
 
 	intel_mark_page_flip_active(intel_crtc);
@@ -8706,6 +8754,9 @@ static int intel_gen4_queue_flip(struct drm_device *dev,
 	if (ret)
 		goto err;
 
+	intel_crtc->unpin_work->gtt_offset =
+		i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
+
 	ret = intel_ring_begin(ring, 4);
 	if (ret)
 		goto err_unpin;
@@ -8717,8 +8768,7 @@ static int intel_gen4_queue_flip(struct drm_device *dev,
 	intel_ring_emit(ring, MI_DISPLAY_FLIP |
 			MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
 	intel_ring_emit(ring, fb->pitches[0]);
-	intel_ring_emit(ring,
-			(i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset) |
+	intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset |
 			obj->tiling_mode);
 
 	/* XXX Enabling the panel-fitter across page-flip is so far
@@ -8755,6 +8805,9 @@ static int intel_gen6_queue_flip(struct drm_device *dev,
 	if (ret)
 		goto err;
 
+	intel_crtc->unpin_work->gtt_offset =
+		i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
+
 	ret = intel_ring_begin(ring, 4);
 	if (ret)
 		goto err_unpin;
@@ -8762,7 +8815,7 @@ static int intel_gen6_queue_flip(struct drm_device *dev,
 	intel_ring_emit(ring, MI_DISPLAY_FLIP |
 			MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
 	intel_ring_emit(ring, fb->pitches[0] | obj->tiling_mode);
-	intel_ring_emit(ring, i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset);
+	intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset);
 
 	/* Contrary to the suggestions in the documentation,
 	 * "Enable Panel Fitter" does not seem to be required when page
@@ -8804,6 +8857,9 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
 	if (ret)
 		goto err;
 
+	intel_crtc->unpin_work->gtt_offset =
+		i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
+
 	switch(intel_crtc->plane) {
 	case PLANE_A:
 		plane_bit = MI_DISPLAY_FLIP_IVB_PLANE_A;
@@ -8881,7 +8937,7 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
 
 	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);
+	intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset);
 	intel_ring_emit(ring, (MI_NOOP));
 
 	intel_mark_page_flip_active(intel_crtc);
@@ -8979,6 +9035,9 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	atomic_inc(&intel_crtc->unpin_work_count);
 	intel_crtc->reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
 
+	if (INTEL_INFO(dev)->gen >= 5 || IS_G4X(dev))
+		work->flip_count = I915_READ(PIPE_FLIPCOUNT_GM45(intel_crtc->pipe)) + 1;
+
 	ret = dev_priv->display.queue_flip(dev, crtc, fb, obj, page_flip_flags);
 	if (ret)
 		goto cleanup_pending;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c551472..edef34e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -590,6 +590,8 @@ struct intel_unpin_work {
 #define INTEL_FLIP_INACTIVE	0
 #define INTEL_FLIP_PENDING	1
 #define INTEL_FLIP_COMPLETE	2
+	u32 flip_count;
+	u32 gtt_offset;
 	bool enable_stall_check;
 };
 
-- 
1.8.3.2

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

[PATCH 2/5] drm/i915: Wait for vblank in hsw_enable_ips()

[ville.syrjala]

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

Now that the vblank wait is gone from intel_enable_primary_plane(),
hsw_enable_ips() needs to do the vblank wait itself.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 12 ++++++------
 drivers/gpu/drm/i915/intel_sprite.c  |  5 +----
 2 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 32c6c16..33d21bf 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3543,17 +3543,17 @@ static void intel_disable_planes(struct drm_crtc *crtc)
 
 void hsw_enable_ips(struct intel_crtc *crtc)
 {
-	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	if (!crtc->config.ips_enabled)
 		return;
 
-	/* We can only enable IPS after we enable a plane and wait for a vblank.
-	 * We guarantee that the plane is enabled by calling intel_enable_ips
-	 * only after intel_enable_plane. And intel_enable_plane already waits
-	 * for a vblank, so all we need to do here is to enable the IPS bit. */
+	/* We can only enable IPS after we enable a plane and wait for a vblank */
+	intel_wait_for_vblank(dev, crtc->pipe);
+
 	assert_plane_enabled(dev_priv, crtc->plane);
-	if (IS_BROADWELL(crtc->base.dev)) {
+	if (IS_BROADWELL(dev)) {
 		mutex_lock(&dev_priv->rps.hw_lock);
 		WARN_ON(sandybridge_pcode_write(dev_priv, DISPLAY_IPS_CONTROL, 0xc0000000));
 		mutex_unlock(&dev_priv->rps.hw_lock);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 336ae6c..4df7245 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -540,10 +540,7 @@ intel_enable_primary(struct drm_crtc *crtc)
 	 * when going from primary only to sprite only and vice
 	 * versa.
 	 */
-	if (intel_crtc->config.ips_enabled) {
-		intel_wait_for_vblank(dev, intel_crtc->pipe);
-		hsw_enable_ips(intel_crtc);
-	}
+	hsw_enable_ips(intel_crtc);
 
 	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
-- 
1.8.3.2

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

[PATCH 3/5] drm/i915: Drop the excessive vblank waits from modeset codepaths

[ville.syrjala]

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

Now that we've plugged the mmio vs. ring flip race, we shouldn't need
these vblank waits in the modeset codepaths anymore. So get rid of
them.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 33d21bf..9b181fc 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1901,7 +1901,6 @@ static void intel_enable_primary_hw_plane(struct drm_i915_private *dev_priv,
 
 	I915_WRITE(reg, val | DISPLAY_PLANE_ENABLE);
 	intel_flush_primary_plane(dev_priv, plane);
-	intel_wait_for_vblank(dev_priv->dev, pipe);
 }
 
 /**
@@ -1931,7 +1930,6 @@ static void intel_disable_primary_hw_plane(struct drm_i915_private *dev_priv,
 
 	I915_WRITE(reg, val & ~DISPLAY_PLANE_ENABLE);
 	intel_flush_primary_plane(dev_priv, plane);
-	intel_wait_for_vblank(dev_priv->dev, pipe);
 }
 
 static bool need_vtd_wa(struct drm_device *dev)
@@ -3706,16 +3704,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	if (HAS_PCH_CPT(dev))
 		cpt_verify_modeset(dev, intel_crtc->pipe);
 
-	/*
-	 * There seems to be a race in PCH platform hw (at least on some
-	 * outputs) where an enabled pipe still completes any pageflip right
-	 * away (as if the pipe is off) instead of waiting for vblank. As soon
-	 * as the first vblank happend, everything works as expected. Hence just
-	 * wait for one vblank before returning to avoid strange things
-	 * happening.
-	 */
-	intel_wait_for_vblank(dev, intel_crtc->pipe);
-
 	drm_vblank_on(dev, pipe);
 }
 
-- 
1.8.3.2

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

[PATCH 4/5] drm/i915: Wait for pending page flips before enabling/disabling the primary plane

[ville.syrjala]

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

We have to write to the primary plane base address registrer when we
enable/disable the primary plane in response to sprite coverage. Those
writes will cause the flip counter to increment which could interfere
with the detection of CS flip completion. We could end up completing
CS flips before the CS has even executed the commands from the ring.

To avoid such issues, wait for CS flips to finish before we toggle the
primary plane on/off.

Testcase: igt/kms_mmio_vs_cs_flip/setplane_vs_cs_flip
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 drivers/gpu/drm/i915/intel_drv.h     | 1 +
 drivers/gpu/drm/i915/intel_sprite.c  | 4 ++++
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9b181fc..c0fe5dd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3096,7 +3096,7 @@ bool intel_has_pending_fb_unpin(struct drm_device *dev)
 	return false;
 }
 
-static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
+void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index edef34e..f578e42 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -775,6 +775,7 @@ int valleyview_get_vco(struct drm_i915_private *dev_priv);
 void intel_mode_from_pipe_config(struct drm_display_mode *mode,
 				 struct intel_crtc_config *pipe_config);
 int intel_format_to_fourcc(int format);
+void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
 
 /* intel_dp.c */
 void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 4df7245..36a8f5e 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -529,6 +529,8 @@ intel_enable_primary(struct drm_crtc *crtc)
 	if (intel_crtc->primary_enabled)
 		return;
 
+	intel_crtc_wait_for_pending_flips(crtc);
+
 	intel_crtc->primary_enabled = true;
 
 	I915_WRITE(reg, I915_READ(reg) | DISPLAY_PLANE_ENABLE);
@@ -558,6 +560,8 @@ intel_disable_primary(struct drm_crtc *crtc)
 	if (!intel_crtc->primary_enabled)
 		return;
 
+	intel_crtc_wait_for_pending_flips(crtc);
+
 	intel_crtc->primary_enabled = false;
 
 	mutex_lock(&dev->struct_mutex);
-- 
1.8.3.2

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

[PATCH 5/5] drm/i915: Move buffer pinning and ring selection to intel_crtc_page_flip()

[ville.syrjala]

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

All of the .queue_flip() callbacks duplicate the same code to pin the
buffers and calculate the gtt_offset. Move that code to
intel_crtc_page_flip(). In order to do that we must also move the ring
selection logic there.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |   1 +
 drivers/gpu/drm/i915/intel_display.c | 115 +++++++++++------------------------
 2 files changed, 35 insertions(+), 81 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7d6acb4..5efc435 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -462,6 +462,7 @@ struct drm_i915_display_funcs {
 	int (*queue_flip)(struct drm_device *dev, struct drm_crtc *crtc,
 			  struct drm_framebuffer *fb,
 			  struct drm_i915_gem_object *obj,
+			  struct intel_ring_buffer *ring,
 			  uint32_t flags);
 	int (*update_primary_plane)(struct drm_crtc *crtc,
 				    struct drm_framebuffer *fb,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c0fe5dd..e844a6b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8637,24 +8637,16 @@ static int intel_gen2_queue_flip(struct drm_device *dev,
 				 struct drm_crtc *crtc,
 				 struct drm_framebuffer *fb,
 				 struct drm_i915_gem_object *obj,
+				 struct intel_ring_buffer *ring,
 				 uint32_t flags)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	u32 flip_mask;
-	struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
 	int ret;
 
-	ret = intel_pin_and_fence_fb_obj(dev, obj, ring);
-	if (ret)
-		goto err;
-
-	intel_crtc->unpin_work->gtt_offset =
-		i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
-
 	ret = intel_ring_begin(ring, 6);
 	if (ret)
-		goto err_unpin;
+		return ret;
 
 	/* Can't queue multiple flips, so wait for the previous
 	 * one to finish before executing the next.
@@ -8674,35 +8666,22 @@ static int intel_gen2_queue_flip(struct drm_device *dev,
 	intel_mark_page_flip_active(intel_crtc);
 	__intel_ring_advance(ring);
 	return 0;
-
-err_unpin:
-	intel_unpin_fb_obj(obj);
-err:
-	return ret;
 }
 
 static int intel_gen3_queue_flip(struct drm_device *dev,
 				 struct drm_crtc *crtc,
 				 struct drm_framebuffer *fb,
 				 struct drm_i915_gem_object *obj,
+				 struct intel_ring_buffer *ring,
 				 uint32_t flags)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	u32 flip_mask;
-	struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
 	int ret;
 
-	ret = intel_pin_and_fence_fb_obj(dev, obj, ring);
-	if (ret)
-		goto err;
-
-	intel_crtc->unpin_work->gtt_offset =
-		i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
-
 	ret = intel_ring_begin(ring, 6);
 	if (ret)
-		goto err_unpin;
+		return ret;
 
 	if (intel_crtc->plane)
 		flip_mask = MI_WAIT_FOR_PLANE_B_FLIP;
@@ -8719,35 +8698,23 @@ static int intel_gen3_queue_flip(struct drm_device *dev,
 	intel_mark_page_flip_active(intel_crtc);
 	__intel_ring_advance(ring);
 	return 0;
-
-err_unpin:
-	intel_unpin_fb_obj(obj);
-err:
-	return ret;
 }
 
 static int intel_gen4_queue_flip(struct drm_device *dev,
 				 struct drm_crtc *crtc,
 				 struct drm_framebuffer *fb,
 				 struct drm_i915_gem_object *obj,
+				 struct intel_ring_buffer *ring,
 				 uint32_t flags)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	uint32_t pf, pipesrc;
-	struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
 	int ret;
 
-	ret = intel_pin_and_fence_fb_obj(dev, obj, ring);
-	if (ret)
-		goto err;
-
-	intel_crtc->unpin_work->gtt_offset =
-		i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
-
 	ret = intel_ring_begin(ring, 4);
 	if (ret)
-		goto err_unpin;
+		return ret;
 
 	/* i965+ uses the linear or tiled offsets from the
 	 * Display Registers (which do not change across a page-flip)
@@ -8770,35 +8737,23 @@ static int intel_gen4_queue_flip(struct drm_device *dev,
 	intel_mark_page_flip_active(intel_crtc);
 	__intel_ring_advance(ring);
 	return 0;
-
-err_unpin:
-	intel_unpin_fb_obj(obj);
-err:
-	return ret;
 }
 
 static int intel_gen6_queue_flip(struct drm_device *dev,
 				 struct drm_crtc *crtc,
 				 struct drm_framebuffer *fb,
 				 struct drm_i915_gem_object *obj,
+				 struct intel_ring_buffer *ring,
 				 uint32_t flags)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
 	uint32_t pf, pipesrc;
 	int ret;
 
-	ret = intel_pin_and_fence_fb_obj(dev, obj, ring);
-	if (ret)
-		goto err;
-
-	intel_crtc->unpin_work->gtt_offset =
-		i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
-
 	ret = intel_ring_begin(ring, 4);
 	if (ret)
-		goto err_unpin;
+		return ret;
 
 	intel_ring_emit(ring, MI_DISPLAY_FLIP |
 			MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
@@ -8818,36 +8773,19 @@ static int intel_gen6_queue_flip(struct drm_device *dev,
 	intel_mark_page_flip_active(intel_crtc);
 	__intel_ring_advance(ring);
 	return 0;
-
-err_unpin:
-	intel_unpin_fb_obj(obj);
-err:
-	return ret;
 }
 
 static int intel_gen7_queue_flip(struct drm_device *dev,
 				 struct drm_crtc *crtc,
 				 struct drm_framebuffer *fb,
 				 struct drm_i915_gem_object *obj,
+				 struct intel_ring_buffer *ring,
 				 uint32_t flags)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_ring_buffer *ring;
 	uint32_t plane_bit = 0;
 	int len, ret;
 
-	ring = obj->ring;
-	if (IS_VALLEYVIEW(dev) || ring == NULL || ring->id != RCS)
-		ring = &dev_priv->ring[BCS];
-
-	ret = intel_pin_and_fence_fb_obj(dev, obj, ring);
-	if (ret)
-		goto err;
-
-	intel_crtc->unpin_work->gtt_offset =
-		i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
-
 	switch(intel_crtc->plane) {
 	case PLANE_A:
 		plane_bit = MI_DISPLAY_FLIP_IVB_PLANE_A;
@@ -8860,8 +8798,7 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
 		break;
 	default:
 		WARN_ONCE(1, "unknown plane in flip command\n");
-		ret = -ENODEV;
-		goto err_unpin;
+		return -ENODEV;
 	}
 
 	len = 4;
@@ -8888,11 +8825,11 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
 	 */
 	ret = intel_ring_cacheline_align(ring);
 	if (ret)
-		goto err_unpin;
+		return ret;
 
 	ret = intel_ring_begin(ring, len);
 	if (ret)
-		goto err_unpin;
+		return ret;
 
 	/* Unmask the flip-done completion message. Note that the bspec says that
 	 * we should do this for both the BCS and RCS, and that we must not unmask
@@ -8931,17 +8868,13 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
 	intel_mark_page_flip_active(intel_crtc);
 	__intel_ring_advance(ring);
 	return 0;
-
-err_unpin:
-	intel_unpin_fb_obj(obj);
-err:
-	return ret;
 }
 
 static int intel_default_queue_flip(struct drm_device *dev,
 				    struct drm_crtc *crtc,
 				    struct drm_framebuffer *fb,
 				    struct drm_i915_gem_object *obj,
+				    struct intel_ring_buffer *ring,
 				    uint32_t flags)
 {
 	return -ENODEV;
@@ -8958,6 +8891,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	struct drm_i915_gem_object *obj = to_intel_framebuffer(fb)->obj;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_unpin_work *work;
+	struct intel_ring_buffer *ring;
 	unsigned long flags;
 	int ret;
 
@@ -9026,10 +8960,27 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	if (INTEL_INFO(dev)->gen >= 5 || IS_G4X(dev))
 		work->flip_count = I915_READ(PIPE_FLIPCOUNT_GM45(intel_crtc->pipe)) + 1;
 
-	ret = dev_priv->display.queue_flip(dev, crtc, fb, obj, page_flip_flags);
+	if (IS_VALLEYVIEW(dev)) {
+		ring = &dev_priv->ring[BCS];
+	} else if (INTEL_INFO(dev)->gen >= 7) {
+		ring = obj->ring;
+		if (ring == NULL || ring->id != RCS)
+			ring = &dev_priv->ring[BCS];
+	} else {
+		ring = &dev_priv->ring[RCS];
+	}
+
+	ret = intel_pin_and_fence_fb_obj(dev, obj, ring);
 	if (ret)
 		goto cleanup_pending;
 
+	work->gtt_offset =
+		i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
+
+	ret = dev_priv->display.queue_flip(dev, crtc, fb, obj, ring, page_flip_flags);
+	if (ret)
+		goto cleanup_unpin;
+
 	intel_disable_fbc(dev);
 	intel_mark_fb_busy(obj, NULL);
 	mutex_unlock(&dev->struct_mutex);
@@ -9038,6 +8989,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 
 	return 0;
 
+cleanup_unpin:
+	intel_unpin_fb_obj(obj);
 cleanup_pending:
 	atomic_dec(&intel_crtc->unpin_work_count);
 	crtc->primary->fb = old_fb;
-- 
1.8.3.2

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

[PATCH igt] tests/kms_mmio_vs_cs_flip: Add a test case to exercise mmio vs. CS flip races

[ville.syrjala]

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

kms_mmio_vs_cs_flip has two subtests:
- setplane_vs_cs_flip tests the interaction between
  fullscreen sprites and CS flips
- setcrtc_vs_cs_flip tests the interaction between
  primary plane panning and CS flips

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 tests/Makefile.sources      |   1 +
 tests/kms_mmio_vs_cs_flip.c | 519 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 520 insertions(+)
 create mode 100644 tests/kms_mmio_vs_cs_flip.c

diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index c957ace..6ff0a35 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -60,6 +60,7 @@ TESTS_progs_M = \
 	kms_fbc_crc \
 	kms_flip \
 	kms_flip_tiling \
+	kms_mmio_vs_cs_flip \
 	kms_pipe_crc_basic \
 	kms_plane \
 	kms_render \
diff --git a/tests/kms_mmio_vs_cs_flip.c b/tests/kms_mmio_vs_cs_flip.c
new file mode 100644
index 0000000..9d9b02a
--- /dev/null
+++ b/tests/kms_mmio_vs_cs_flip.c
@@ -0,0 +1,519 @@
+/*
+ * Copyright © 2014 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include <errno.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <string.h>
+#include <time.h>
+
+#include "drmtest.h"
+#include "igt_debugfs.h"
+#include "igt_kms.h"
+#include "intel_chipset.h"
+#include "ioctl_wrappers.h"
+
+typedef struct {
+	int drm_fd;
+	igt_display_t display;
+	igt_pipe_crc_t *pipe_crc;
+	drm_intel_bufmgr *bufmgr;
+	drm_intel_bo *busy_bo;
+	uint32_t devid;
+	bool flip_done;
+} data_t;
+
+static void exec_nop(data_t *data, uint32_t handle, unsigned int ring)
+{
+	struct intel_batchbuffer *batch;
+	drm_intel_bo *bo;
+
+	batch = intel_batchbuffer_alloc(data->bufmgr, data->devid);
+	igt_assert(batch);
+
+	bo = gem_handle_to_libdrm_bo(data->bufmgr, data->drm_fd, "", handle);
+	igt_assert(bo);
+
+	/* add relocs to make sure the kernel will think we write to dst */
+	BEGIN_BATCH(4);
+	OUT_BATCH(MI_BATCH_BUFFER_END);
+	OUT_BATCH(MI_NOOP);
+	OUT_RELOC(bo, I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER, 0);
+	OUT_BATCH(MI_NOOP);
+	ADVANCE_BATCH();
+
+	intel_batchbuffer_flush_on_ring(batch, ring);
+	intel_batchbuffer_free(batch);
+}
+
+static void exec_blt(data_t *data)
+{
+	struct intel_batchbuffer *batch;
+	int w, h, pitch, i;
+
+	batch = intel_batchbuffer_alloc(data->bufmgr, data->devid);
+	igt_assert(batch);
+
+	w = 8192;
+	h = data->busy_bo->size / (8192 * 4);
+	pitch = w * 4;
+
+	for (i = 0; i < 20; i++) {
+		BLIT_COPY_BATCH_START(data->devid, 0);
+		OUT_BATCH((3 << 24) | /* 32 bits */
+			  (0xcc << 16) | /* copy ROP */
+			  pitch);
+		OUT_BATCH(0 << 16 | 0);
+		OUT_BATCH(h << 16 | w);
+		OUT_RELOC(data->busy_bo, I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER, 0);
+		BLIT_RELOC_UDW(data->devid);
+		OUT_BATCH(0 << 16 | 0);
+		OUT_BATCH(pitch);
+		OUT_RELOC(data->busy_bo, I915_GEM_DOMAIN_RENDER, 0, 0);
+		BLIT_RELOC_UDW(data->devid);
+		ADVANCE_BATCH();
+	}
+
+	intel_batchbuffer_flush(batch);
+	intel_batchbuffer_free(batch);
+}
+
+static void page_flip_handler(int fd, unsigned int frame, unsigned int sec,
+			      unsigned int usec, void *_data)
+{
+	data_t *data = _data;
+
+	data->flip_done = true;
+}
+
+static void wait_for_flip(data_t *data, uint32_t flip_handle)
+{
+	struct timeval timeout = {
+		.tv_sec = 3,
+		.tv_usec = 0,
+	};
+	drmEventContext evctx = {
+		.version = DRM_EVENT_CONTEXT_VERSION,
+		.page_flip_handler = page_flip_handler,
+	};
+	fd_set fds;
+
+	FD_ZERO(&fds);
+	FD_SET(data->drm_fd, &fds);
+
+	while (!data->flip_done) {
+		int ret = select(data->drm_fd + 1, &fds, NULL, NULL, &timeout);
+
+		if (ret < 0 && errno == EINTR)
+			continue;
+
+		igt_assert(ret >= 0);
+
+		do_or_die(drmHandleEvent(data->drm_fd, &evctx));
+	}
+
+	/*
+	 * The flip completion may have been signalled prematurely, so
+	 * also submit another nop batch and wait for it to make sure
+	 * the ring has really been drained.
+	 */
+	if (IS_GEN7(data->devid) || IS_GEN8(data->devid))
+		exec_nop(data, flip_handle, I915_EXEC_BLT);
+	else
+		exec_nop(data, flip_handle, I915_EXEC_RENDER);
+	gem_sync(data->drm_fd, flip_handle);
+}
+
+static void make_gpu_busy(data_t *data, uint32_t flip_handle)
+{
+	/*
+	 * Make sure flip_handle has been used on the blt ring.
+	 * This should make the flip use the same ring on gen7+.
+	 */
+	if (IS_GEN7(data->devid) || IS_GEN8(data->devid))
+		exec_nop(data, flip_handle, I915_EXEC_BLT);
+
+	/*
+	 * Add a pile commands to the ring.  The flip will be
+	 * stuck behing these commands and hence gets delayed
+	 * significantly.
+	 */
+	exec_blt(data);
+
+	/*
+	 * Make sure the render ring will block until the blt ring is clear.
+	 * This is in case the flip will execute on the render ring and the
+	 * blits were on the blt ring (this will be the case on gen6 at least).
+	 *
+	 * We can't add an explicit dependency between flip_handle and the
+	 * blits since that would cause the driver to block until the blits
+	 * have completed before it will perform a subsequent mmio flip,
+	 * and so the test would fail to exercise the mmio vs. CS flip race.
+	 */
+	if (HAS_BLT_RING(data->devid))
+		exec_nop(data, data->busy_bo->handle, I915_EXEC_RENDER);
+}
+
+/*
+ * 1. set primary plane to full red
+ * 2. grab a reference crc
+ * 3. set primary plane to full blue
+ * 4. queue lots of GPU activity to delay the subsequent page flip
+ * 5. queue a page flip to the same blue fb
+ * 6. toggle a fullscreen sprite (green) on and back off again
+ * 7. set primary plane to red fb
+ * 8. wait for GPU to finish
+ * 9. compare current crc with reference crc
+ *
+ * We expect the primary plane to display full red at the end.
+ * If the sprite operations have interfered with the page flip,
+ * the driver may have mistakenly completed the flip before
+ * it was executed by the CS, and hence the subsequent mmio
+ * flips may have overtaken it. So once we've finished everything
+ * the CS flip may have been the last thing to occur, which means
+ * the primary plane may be full blue instead of the red it's
+ * supposed to be.
+ */
+static void
+test_plane(data_t *data, igt_output_t *output, enum pipe pipe, enum igt_plane plane)
+{
+	struct igt_fb red_fb, green_fb, blue_fb;
+	drmModeModeInfo *mode;
+	igt_plane_t *primary, *sprite;
+	igt_crc_t ref_crc, crc;
+	int ret;
+
+	if (data->pipe_crc)
+		igt_pipe_crc_free(data->pipe_crc);
+	data->pipe_crc = igt_pipe_crc_new(pipe, INTEL_PIPE_CRC_SOURCE_AUTO);
+
+	igt_output_set_pipe(output, pipe);
+	primary = igt_output_get_plane(output, 0);
+	sprite = igt_output_get_plane(output, plane);
+	igt_display_commit(&data->display);
+
+	mode = igt_output_get_mode(output);
+	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
+			    DRM_FORMAT_XRGB8888,
+			    false, /* tiled */
+			    1.0, 0.0, 0.0,
+			    &red_fb);
+	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
+			    DRM_FORMAT_XRGB8888,
+			    false, /* tiled */
+			    0.0, 1.0, 0.0,
+			    &green_fb);
+	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
+			    DRM_FORMAT_XRGB8888,
+			    false, /* tiled */
+			    0.0, 0.0, 1.0,
+			    &blue_fb);
+
+	/*
+	 * Make sure these buffers are suited for display use
+	 * because most of the modeset operations must be fast
+	 * later on.
+	 */
+	igt_plane_set_fb(primary, &blue_fb);
+	igt_display_commit(&data->display);
+	igt_plane_set_fb(sprite, &green_fb);
+	igt_display_commit(&data->display);
+	igt_plane_set_fb(sprite, NULL);
+	igt_display_commit(&data->display);
+
+	/* set red fb and grab reference crc */
+	igt_plane_set_fb(primary, &red_fb);
+	igt_display_commit(&data->display);
+	igt_pipe_crc_collect_crc(data->pipe_crc, &ref_crc);
+
+	ret = drmModeSetCrtc(data->drm_fd, output->config.crtc->crtc_id,
+			     blue_fb.fb_id, 0, 0, &output->id, 1,
+			     mode);
+	igt_assert(ret == 0);
+
+	make_gpu_busy(data, blue_fb.gem_handle);
+
+	data->flip_done = false;
+	ret = drmModePageFlip(data->drm_fd, output->config.crtc->crtc_id,
+			      blue_fb.fb_id, DRM_MODE_PAGE_FLIP_EVENT, data);
+	igt_assert(ret == 0);
+
+	/*
+	 * Toggle a fullscreen sprite on and back off. This will result
+	 * in the primary plane getting disabled and re-enbled, and that
+	 * leads to mmio flips. The driver may then mistake the flip done
+	 * interrupts from the mmio flips as the flip done interrupts for
+	 * the CS flip, and hence subsequent mmio flips won't wait for the
+	 * CS flips like they should.
+	 */
+	ret = drmModeSetPlane(data->drm_fd,
+			      sprite->drm_plane->plane_id,
+			      output->config.crtc->crtc_id,
+			      green_fb.fb_id, 0,
+			      0, 0, mode->hdisplay, mode->vdisplay,
+			      0, 0, mode->hdisplay << 16, mode->vdisplay << 16);
+	igt_assert(ret == 0);
+	ret = drmModeSetPlane(data->drm_fd,
+			      sprite->drm_plane->plane_id,
+			      output->config.crtc->crtc_id,
+			      0, 0,
+			      0, 0, 0, 0,
+			      0, 0, 0, 0);
+	igt_assert(ret == 0);
+
+	/*
+	 * Set primary plane to red fb. This should wait for the CS flip
+	 * to complete. But if the kernel mistook the flip done interrupt
+	 * from the mmio flip as the flip done from the CS flip, this will
+	 * not wait for anything. And hence the the CS flip will actually
+	 * occur after this mmio flip.
+	 */
+	ret = drmModeSetCrtc(data->drm_fd, output->config.crtc->crtc_id,
+			     red_fb.fb_id, 0, 0, &output->id, 1,
+			     mode);
+	igt_assert(ret == 0);
+
+	/* Make sure the flip has been executed */
+	wait_for_flip(data, blue_fb.gem_handle);
+
+	/* Grab crc and compare with the extected result */
+	igt_pipe_crc_collect_crc(data->pipe_crc, &crc);
+
+	igt_plane_set_fb(primary, NULL);
+	igt_display_commit(&data->display);
+
+	igt_remove_fb(data->drm_fd, &red_fb);
+	igt_remove_fb(data->drm_fd, &green_fb);
+	igt_remove_fb(data->drm_fd, &blue_fb);
+
+	igt_pipe_crc_free(data->pipe_crc);
+	data->pipe_crc = NULL;
+
+	igt_output_set_pipe(output, PIPE_ANY);
+	igt_display_commit(&data->display);
+
+	igt_assert(igt_crc_equal(&ref_crc, &crc));
+}
+
+/*
+ * 1. set primary plane to full red
+ * 2. grab a reference crc
+ * 3. set primary plane to full green
+ * 4. wait for vblank
+ * 5. pan primary plane a bit (to cause a mmio flip w/o vblank wait)
+ * 6. queue lots of GPU activity to delay the subsequent page flip
+ * 6. queue a page flip to a blue fb
+ * 7. set primary plane to red fb
+ * 8. wait for GPU to finish
+ * 9. compare current crc with reference crc
+ *
+ * We expect the primary plane to display full red at the end.
+ * If the previously schedule primary plane pan operation has interfered
+ * with the following page flip, the driver may have mistakenly completed
+ * the flip before it was executed by the CS, and hence the subsequent mmio
+ * flips may have overtaken it. So once we've finished everything
+ * the CS flip may have been the last thing to occur, which means
+ * the primary plane may be full blue instead of the red it's
+ * supposed to be.
+ */
+static void
+test_crtc(data_t *data, igt_output_t *output, enum pipe pipe)
+{
+	struct igt_fb red_fb, green_fb, blue_fb;
+	drmModeModeInfo *mode;
+	igt_plane_t *primary;
+	igt_crc_t ref_crc, crc;
+	int ret;
+
+	if (data->pipe_crc)
+		igt_pipe_crc_free(data->pipe_crc);
+	data->pipe_crc = igt_pipe_crc_new(pipe, INTEL_PIPE_CRC_SOURCE_AUTO);
+
+	igt_output_set_pipe(output, pipe);
+	primary = igt_output_get_plane(output, 0);
+
+	mode = igt_output_get_mode(output);
+	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay+1,
+			    DRM_FORMAT_XRGB8888,
+			    false, /* tiled */
+			    1.0, 0.0, 0.0,
+			    &red_fb);
+	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay+1,
+			    DRM_FORMAT_XRGB8888,
+			    false, /* tiled */
+			    0.0, 0.0, 1.0,
+			    &blue_fb);
+	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay+1,
+			    DRM_FORMAT_XRGB8888,
+			    false, /* tiled */
+			    0.0, 1.0, 0.0,
+			    &green_fb);
+
+	/*
+	 * Make sure these buffers are suited for display use
+	 * because most of the modeset operations must be fast
+	 * later on.
+	 */
+	igt_plane_set_fb(primary, &green_fb);
+	igt_display_commit(&data->display);
+	igt_plane_set_fb(primary, &blue_fb);
+	igt_display_commit(&data->display);
+
+	/* set red fb and grab reference crc */
+	igt_plane_set_fb(primary, &red_fb);
+	igt_display_commit(&data->display);
+	igt_pipe_crc_collect_crc(data->pipe_crc, &ref_crc);
+
+	/*
+	 * Further down we need to issue an mmio flip w/o the kernel
+	 * waiting for vblank. The easiest way is to just pan within
+	 * the same FB. So pan away a bit here, and later we undo this
+	 * with another pan which will result in the desired mmio flip.
+	 */
+	ret = drmModeSetCrtc(data->drm_fd, output->config.crtc->crtc_id,
+			     green_fb.fb_id, 0, 1, &output->id, 1,
+			     mode);
+	igt_assert(ret == 0);
+
+	/*
+	 * Make it more likely that the CS flip has been submitted into the
+	 * ring by the time the mmio flip from the drmModeSetCrtc() below
+	 * completes. The driver will then mistake the flip done interrupt
+	 * from the mmio flip as the flip done interrupt from the CS flip.
+	 */
+	igt_wait_for_vblank(data->drm_fd, pipe);
+
+	/* now issue the mmio flip w/o vblank waits in the kernel, ie. pan a bit */
+	ret = drmModeSetCrtc(data->drm_fd, output->config.crtc->crtc_id,
+			     green_fb.fb_id, 0, 0, &output->id, 1,
+			     mode);
+	igt_assert(ret == 0);
+
+	make_gpu_busy(data, blue_fb.gem_handle);
+
+	/*
+	 * Submit the CS flip. The commands must be emitted into the ring
+	 * before the mmio flip from the panning operation completes.
+	 */
+	data->flip_done = false;
+	ret = drmModePageFlip(data->drm_fd, output->config.crtc->crtc_id,
+			      blue_fb.fb_id, DRM_MODE_PAGE_FLIP_EVENT, data);
+	igt_assert(ret == 0);
+
+	/*
+	 * Set primary plane to red fb. This should wait for the CS flip
+	 * to complete. But if the kernel mistook the flip done interrupt
+	 * from the mmio flip as the flip done from the CS flip, this will
+	 * not wait for anything. And hence the the CS flip will actually
+	 * occur after this mmio flip.
+	 */
+	ret = drmModeSetCrtc(data->drm_fd, output->config.crtc->crtc_id,
+			     red_fb.fb_id, 0, 0, &output->id, 1,
+			     mode);
+	igt_assert(ret == 0);
+
+	/* Make sure the flip has been executed */
+	wait_for_flip(data, blue_fb.gem_handle);
+
+	/* Grab crc and compare with the extected result */
+	igt_pipe_crc_collect_crc(data->pipe_crc, &crc);
+
+	igt_plane_set_fb(primary, NULL);
+	igt_display_commit(&data->display);
+
+	igt_remove_fb(data->drm_fd, &red_fb);
+	igt_remove_fb(data->drm_fd, &green_fb);
+	igt_remove_fb(data->drm_fd, &blue_fb);
+
+	igt_pipe_crc_free(data->pipe_crc);
+	data->pipe_crc = NULL;
+
+	igt_output_set_pipe(output, PIPE_ANY);
+	igt_display_commit(&data->display);
+
+	igt_assert(igt_crc_equal(&ref_crc, &crc));
+}
+
+static void
+run_plane_test_for_pipe(data_t *data, enum pipe pipe)
+{
+	igt_output_t *output;
+	enum igt_plane plane = 1; /* testing with one sprite is enough */
+
+	igt_skip_on(plane >= data->display.pipes[pipe].n_planes);
+
+	for_each_connected_output(&data->display, output)
+		test_plane(data, output, pipe, plane);
+}
+
+static void
+run_crtc_test_for_pipe(data_t *data, enum pipe pipe)
+{
+	igt_output_t *output;
+
+	for_each_connected_output(&data->display, output)
+		test_crtc(data, output, pipe);
+}
+
+static data_t data;
+
+igt_main
+{
+	int pipe;
+
+	igt_skip_on_simulation();
+
+	igt_fixture {
+		data.drm_fd = drm_open_any();
+
+		igt_set_vt_graphics_mode();
+
+		data.devid = intel_get_drm_devid(data.drm_fd);
+
+		igt_require_pipe_crc();
+		igt_display_init(&data.display, data.drm_fd);
+
+		data.bufmgr = drm_intel_bufmgr_gem_init(data.drm_fd, 4096);
+		igt_assert(data.bufmgr);
+		drm_intel_bufmgr_gem_enable_reuse(data.bufmgr);
+
+		data.busy_bo = drm_intel_bo_alloc(data.bufmgr, "bo",
+						  128*1024*1024, 4096);
+		gem_set_tiling(data.drm_fd, data.busy_bo->handle, 0, 4096);
+	}
+
+	igt_subtest_f("setplane_vs_cs_flip") {
+		for (pipe = 0; pipe < data.display.n_pipes; pipe++)
+			run_plane_test_for_pipe(&data, pipe);
+	}
+
+	igt_subtest_f("setcrtc_vs_cs_flip") {
+		for (pipe = 0; pipe < data.display.n_pipes; pipe++)
+			run_crtc_test_for_pipe(&data, pipe);
+	}
+
+	igt_fixture {
+		drm_intel_bufmgr_destroy(data.bufmgr);
+		igt_display_fini(&data.display);
+	}
+}
-- 
1.8.3.2

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

Re: [PATCH 5/5] drm/i915: Move buffer pinning and ring selection to intel_crtc_page_flip()

[Chris Wilson]

On Tue, Apr 15, 2014 at 09:41:38PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> All of the .queue_flip() callbacks duplicate the same code to pin the
> buffers and calculate the gtt_offset. Move that code to
> intel_crtc_page_flip(). In order to do that we must also move the ring
> selection logic there.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I was a little uneasy about this given the fun-and-games we play with
ring selection and the historic issues we have had with bugs in
unpinning. However, this looks to be worth it, especially with the new
piece of common code,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v2 3/5] drm/i915: Drop the excessive vblank waits from modeset codepaths

[ville.syrjala]

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

Now that we've plugged the mmio vs. ring flip race, we shouldn't need
these vblank waits in the modeset codepaths anymore. So get rid of
them.

v2: gen2 needs to wait for planes to turn off before disabling pipe

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 33d21bf..17258fe5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1901,7 +1901,6 @@ static void intel_enable_primary_hw_plane(struct drm_i915_private *dev_priv,
 
 	I915_WRITE(reg, val | DISPLAY_PLANE_ENABLE);
 	intel_flush_primary_plane(dev_priv, plane);
-	intel_wait_for_vblank(dev_priv->dev, pipe);
 }
 
 /**
@@ -1931,7 +1930,6 @@ static void intel_disable_primary_hw_plane(struct drm_i915_private *dev_priv,
 
 	I915_WRITE(reg, val & ~DISPLAY_PLANE_ENABLE);
 	intel_flush_primary_plane(dev_priv, plane);
-	intel_wait_for_vblank(dev_priv->dev, pipe);
 }
 
 static bool need_vtd_wa(struct drm_device *dev)
@@ -3706,16 +3704,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	if (HAS_PCH_CPT(dev))
 		cpt_verify_modeset(dev, intel_crtc->pipe);
 
-	/*
-	 * There seems to be a race in PCH platform hw (at least on some
-	 * outputs) where an enabled pipe still completes any pageflip right
-	 * away (as if the pipe is off) instead of waiting for vblank. As soon
-	 * as the first vblank happend, everything works as expected. Hence just
-	 * wait for one vblank before returning to avoid strange things
-	 * happening.
-	 */
-	intel_wait_for_vblank(dev, intel_crtc->pipe);
-
 	drm_vblank_on(dev, pipe);
 }
 
@@ -4475,6 +4463,13 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	intel_disable_planes(crtc);
 	intel_disable_primary_hw_plane(dev_priv, plane, pipe);
 
+	/*
+	 * On gen2 planes are double buffered but the pipe isn't, so we must
+	 * wait for planes to fully turn off before disabling the pipe.
+	 */
+	if (IS_GEN2(dev))
+		intel_wait_for_vblank(dev, pipe);
+
 	intel_set_cpu_fifo_underrun_reporting(dev, pipe, false);
 	intel_disable_pipe(dev_priv, pipe);
 
-- 
1.8.3.2

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

Re: [PATCH v2 1/5] drm/i915: Fix mmio vs. CS flip race on ILK+

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 11881 bytes --]

needs a small rebase but everything looks good for me so
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>


On Tue, Apr 15, 2014 at 11:41 AM, <ville.syrjala@linux.intel.com> wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Starting from ILK, mmio flips also cause a flip done interrupt to be
> signalled. This means if we first do a set_base and follow it
> immediately with the CS flip, we might mistake the flip done interrupt
> caused by the set_base as the flip done interrupt caused by the CS
> flip.
>
> The hardware has a flip counter which increments every time a mmio or
> CS flip is issued. It basically counts the number of DSPSURF register
> writes. This means we can sample the counter before we put the CS
> flip into the ring, and then when we get a flip done interrupt we can
> check whether the CS flip has actually performed the surface address
> update, or if the interrupt was caused by a previous but yet
> unfinished mmio flip.
>
> Even with the flip counter we still have a race condition of the CS flip
> base address update happens after the mmio flip done interrupt was
> raised but not yet processed by the driver. When the interrupt is
> eventually processed, the flip counter will already indicate that the
> CS flip has been executed, but it would not actually complete until the
> next start of vblank. We can use the DSPSURFLIVE register to check
> whether the hardware is actually scanning out of the buffer we expect,
> or if we managed hit this race window.
>
> This covers all the cases where the CS flip actually changes the base
> address. If the base address remains unchanged, we might still complete
> the CS flip before it has actually completed. But since the address
> didn't change anyway, the premature flip completion can't result in
> userspace overwriting data that's still being scanned out.
>
> CTG already has the flip counter and DSPSURFLIVE registers, and
> although the flip done interrupt is still limited to CS flips alone,
> the code now also checks the flip counter on CTG as well.
>
> v2: s/dspsurf/gtt_offset/ (Chris)
>
> Testcase: igt/kms_mmio_vs_cs_flip/setcrtc_vs_cs_flip
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=73027
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |  1 +
>  drivers/gpu/drm/i915/intel_display.c | 73
> ++++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_drv.h     |  2 +
>  3 files changed, 69 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index 8f84555..c28169c 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3638,6 +3638,7 @@ enum punit_power_well {
>  #define _PIPEA_FRMCOUNT_GM45   0x70040
>  #define _PIPEA_FLIPCOUNT_GM45  0x70044
>  #define PIPE_FRMCOUNT_GM45(pipe) _PIPE2(pipe, _PIPEA_FRMCOUNT_GM45)
> +#define PIPE_FLIPCOUNT_GM45(pipe) _PIPE2(pipe, _PIPEA_FLIPCOUNT_GM45)
>
>  /* Cursor A & B regs */
>  #define _CURACNTR              (dev_priv->info.display_mmio_offset +
> 0x70080)
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 1390ab5..32c6c16 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8577,6 +8577,48 @@ void intel_finish_page_flip_plane(struct drm_device
> *dev, int plane)
>         do_intel_finish_page_flip(dev, crtc);
>  }
>
> +/* Is 'a' after or equal to 'b'? */
> +static bool flip_count_after_eq(u32 a, u32 b)
> +{
> +       return !((a - b) & 0x80000000);
> +}
> +
> +static bool page_flip_finished(struct intel_crtc *crtc)
> +{
> +       struct drm_device *dev = crtc->base.dev;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +       /*
> +        * The relevant registers doen't exist on pre-ctg.
> +        * As the flip done interrupt doesn't trigger for mmio
> +        * flips on gmch platforms, a flip count check isn't
> +        * really needed there. But since ctg has the registers,
> +        * include it in the check anyway.
> +        */
> +       if (INTEL_INFO(dev)->gen < 5 && !IS_G4X(dev))
> +               return true;
> +
> +       /*
> +        * A DSPSURFLIVE check isn't enough in case the mmio and CS flips
> +        * used the same base address. In that case the mmio flip might
> +        * have completed, but the CS hasn't even executed the flip yet.
> +        *
> +        * A flip count check isn't enough as the CS might have updated
> +        * the base address just after start of vblank, but before we
> +        * managed to process the interrupt. This means we'd complete the
> +        * CS flip too soon.
> +        *
> +        * Combining both checks should get us a good enough result. It may
> +        * still happen that the CS flip has been executed, but has not
> +        * yet actually completed. But in case the base address is the same
> +        * anyway, we don't really care.
> +        */
> +       return (I915_READ(DSPSURFLIVE(crtc->plane)) & ~0xfff) ==
> +               crtc->unpin_work->gtt_offset &&
> +
> flip_count_after_eq(I915_READ(PIPE_FLIPCOUNT_GM45(crtc->pipe)),
> +                                   crtc->unpin_work->flip_count);
> +}
> +
>  void intel_prepare_page_flip(struct drm_device *dev, int plane)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -8589,7 +8631,7 @@ void intel_prepare_page_flip(struct drm_device *dev,
> int plane)
>          * is also accompanied by a spurious intel_prepare_page_flip().
>          */
>         spin_lock_irqsave(&dev->event_lock, flags);
> -       if (intel_crtc->unpin_work)
> +       if (intel_crtc->unpin_work && page_flip_finished(intel_crtc))
>                 atomic_inc_not_zero(&intel_crtc->unpin_work->pending);
>         spin_unlock_irqrestore(&dev->event_lock, flags);
>  }
> @@ -8619,6 +8661,9 @@ static int intel_gen2_queue_flip(struct drm_device
> *dev,
>         if (ret)
>                 goto err;
>
> +       intel_crtc->unpin_work->gtt_offset =
> +               i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
> +
>         ret = intel_ring_begin(ring, 6);
>         if (ret)
>                 goto err_unpin;
> @@ -8635,7 +8680,7 @@ static int intel_gen2_queue_flip(struct drm_device
> *dev,
>         intel_ring_emit(ring, MI_DISPLAY_FLIP |
>                         MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
>         intel_ring_emit(ring, fb->pitches[0]);
> -       intel_ring_emit(ring, i915_gem_obj_ggtt_offset(obj) +
> intel_crtc->dspaddr_offset);
> +       intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset);
>         intel_ring_emit(ring, 0); /* aux display base address, unused */
>
>         intel_mark_page_flip_active(intel_crtc);
> @@ -8664,6 +8709,9 @@ static int intel_gen3_queue_flip(struct drm_device
> *dev,
>         if (ret)
>                 goto err;
>
> +       intel_crtc->unpin_work->gtt_offset =
> +               i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
> +
>         ret = intel_ring_begin(ring, 6);
>         if (ret)
>                 goto err_unpin;
> @@ -8677,7 +8725,7 @@ static int intel_gen3_queue_flip(struct drm_device
> *dev,
>         intel_ring_emit(ring, MI_DISPLAY_FLIP_I915 |
>                         MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
>         intel_ring_emit(ring, fb->pitches[0]);
> -       intel_ring_emit(ring, i915_gem_obj_ggtt_offset(obj) +
> intel_crtc->dspaddr_offset);
> +       intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset);
>         intel_ring_emit(ring, MI_NOOP);
>
>         intel_mark_page_flip_active(intel_crtc);
> @@ -8706,6 +8754,9 @@ static int intel_gen4_queue_flip(struct drm_device
> *dev,
>         if (ret)
>                 goto err;
>
> +       intel_crtc->unpin_work->gtt_offset =
> +               i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
> +
>         ret = intel_ring_begin(ring, 4);
>         if (ret)
>                 goto err_unpin;
> @@ -8717,8 +8768,7 @@ static int intel_gen4_queue_flip(struct drm_device
> *dev,
>         intel_ring_emit(ring, MI_DISPLAY_FLIP |
>                         MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
>         intel_ring_emit(ring, fb->pitches[0]);
> -       intel_ring_emit(ring,
> -                       (i915_gem_obj_ggtt_offset(obj) +
> intel_crtc->dspaddr_offset) |
> +       intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset |
>                         obj->tiling_mode);
>
>         /* XXX Enabling the panel-fitter across page-flip is so far
> @@ -8755,6 +8805,9 @@ static int intel_gen6_queue_flip(struct drm_device
> *dev,
>         if (ret)
>                 goto err;
>
> +       intel_crtc->unpin_work->gtt_offset =
> +               i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
> +
>         ret = intel_ring_begin(ring, 4);
>         if (ret)
>                 goto err_unpin;
> @@ -8762,7 +8815,7 @@ static int intel_gen6_queue_flip(struct drm_device
> *dev,
>         intel_ring_emit(ring, MI_DISPLAY_FLIP |
>                         MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
>         intel_ring_emit(ring, fb->pitches[0] | obj->tiling_mode);
> -       intel_ring_emit(ring, i915_gem_obj_ggtt_offset(obj) +
> intel_crtc->dspaddr_offset);
> +       intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset);
>
>         /* Contrary to the suggestions in the documentation,
>          * "Enable Panel Fitter" does not seem to be required when page
> @@ -8804,6 +8857,9 @@ static int intel_gen7_queue_flip(struct drm_device
> *dev,
>         if (ret)
>                 goto err;
>
> +       intel_crtc->unpin_work->gtt_offset =
> +               i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
> +
>         switch(intel_crtc->plane) {
>         case PLANE_A:
>                 plane_bit = MI_DISPLAY_FLIP_IVB_PLANE_A;
> @@ -8881,7 +8937,7 @@ static int intel_gen7_queue_flip(struct drm_device
> *dev,
>
>         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);
> +       intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset);
>         intel_ring_emit(ring, (MI_NOOP));
>
>         intel_mark_page_flip_active(intel_crtc);
> @@ -8979,6 +9035,9 @@ static int intel_crtc_page_flip(struct drm_crtc
> *crtc,
>         atomic_inc(&intel_crtc->unpin_work_count);
>         intel_crtc->reset_counter =
> atomic_read(&dev_priv->gpu_error.reset_counter);
>
> +       if (INTEL_INFO(dev)->gen >= 5 || IS_G4X(dev))
> +               work->flip_count =
> I915_READ(PIPE_FLIPCOUNT_GM45(intel_crtc->pipe)) + 1;
> +
>         ret = dev_priv->display.queue_flip(dev, crtc, fb, obj,
> page_flip_flags);
>         if (ret)
>                 goto cleanup_pending;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index c551472..edef34e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -590,6 +590,8 @@ struct intel_unpin_work {
>  #define INTEL_FLIP_INACTIVE    0
>  #define INTEL_FLIP_PENDING     1
>  #define INTEL_FLIP_COMPLETE    2
> +       u32 flip_count;
> +       u32 gtt_offset;
>         bool enable_stall_check;
>  };
>
> --
> 1.8.3.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[-- Attachment #1.2: Type: text/html, Size: 14161 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 2/5] drm/i915: Wait for vblank in hsw_enable_ips()

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 2962 bytes --]

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>


On Tue, Apr 15, 2014 at 11:41 AM, <ville.syrjala@linux.intel.com> wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Now that the vblank wait is gone from intel_enable_primary_plane(),
> hsw_enable_ips() needs to do the vblank wait itself.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 12 ++++++------
>  drivers/gpu/drm/i915/intel_sprite.c  |  5 +----
>  2 files changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 32c6c16..33d21bf 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3543,17 +3543,17 @@ static void intel_disable_planes(struct drm_crtc
> *crtc)
>
>  void hsw_enable_ips(struct intel_crtc *crtc)
>  {
> -       struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> +       struct drm_device *dev = crtc->base.dev;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
>
>         if (!crtc->config.ips_enabled)
>                 return;
>
> -       /* We can only enable IPS after we enable a plane and wait for a
> vblank.
> -        * We guarantee that the plane is enabled by calling
> intel_enable_ips
> -        * only after intel_enable_plane. And intel_enable_plane already
> waits
> -        * for a vblank, so all we need to do here is to enable the IPS
> bit. */
> +       /* We can only enable IPS after we enable a plane and wait for a
> vblank */
> +       intel_wait_for_vblank(dev, crtc->pipe);
> +
>         assert_plane_enabled(dev_priv, crtc->plane);
> -       if (IS_BROADWELL(crtc->base.dev)) {
> +       if (IS_BROADWELL(dev)) {
>                 mutex_lock(&dev_priv->rps.hw_lock);
>                 WARN_ON(sandybridge_pcode_write(dev_priv,
> DISPLAY_IPS_CONTROL, 0xc0000000));
>                 mutex_unlock(&dev_priv->rps.hw_lock);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 336ae6c..4df7245 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -540,10 +540,7 @@ intel_enable_primary(struct drm_crtc *crtc)
>          * when going from primary only to sprite only and vice
>          * versa.
>          */
> -       if (intel_crtc->config.ips_enabled) {
> -               intel_wait_for_vblank(dev, intel_crtc->pipe);
> -               hsw_enable_ips(intel_crtc);
> -       }
> +       hsw_enable_ips(intel_crtc);
>
>         mutex_lock(&dev->struct_mutex);
>         intel_update_fbc(dev);
> --
> 1.8.3.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[-- Attachment #1.2: Type: text/html, Size: 4027 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH v2 3/5] drm/i915: Drop the excessive vblank waits from modeset codepaths

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 3044 bytes --]

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>


On Fri, Apr 25, 2014 at 3:30 AM, <ville.syrjala@linux.intel.com> wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Now that we've plugged the mmio vs. ring flip race, we shouldn't need
> these vblank waits in the modeset codepaths anymore. So get rid of
> them.
>
> v2: gen2 needs to wait for planes to turn off before disabling pipe
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 33d21bf..17258fe5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1901,7 +1901,6 @@ static void intel_enable_primary_hw_plane(struct
> drm_i915_private *dev_priv,
>
>         I915_WRITE(reg, val | DISPLAY_PLANE_ENABLE);
>         intel_flush_primary_plane(dev_priv, plane);
> -       intel_wait_for_vblank(dev_priv->dev, pipe);
>  }
>
>  /**
> @@ -1931,7 +1930,6 @@ static void intel_disable_primary_hw_plane(struct
> drm_i915_private *dev_priv,
>
>         I915_WRITE(reg, val & ~DISPLAY_PLANE_ENABLE);
>         intel_flush_primary_plane(dev_priv, plane);
> -       intel_wait_for_vblank(dev_priv->dev, pipe);
>  }
>
>  static bool need_vtd_wa(struct drm_device *dev)
> @@ -3706,16 +3704,6 @@ static void ironlake_crtc_enable(struct drm_crtc
> *crtc)
>         if (HAS_PCH_CPT(dev))
>                 cpt_verify_modeset(dev, intel_crtc->pipe);
>
> -       /*
> -        * There seems to be a race in PCH platform hw (at least on some
> -        * outputs) where an enabled pipe still completes any pageflip
> right
> -        * away (as if the pipe is off) instead of waiting for vblank. As
> soon
> -        * as the first vblank happend, everything works as expected.
> Hence just
> -        * wait for one vblank before returning to avoid strange things
> -        * happening.
> -        */
> -       intel_wait_for_vblank(dev, intel_crtc->pipe);
> -
>         drm_vblank_on(dev, pipe);
>  }
>
> @@ -4475,6 +4463,13 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>         intel_disable_planes(crtc);
>         intel_disable_primary_hw_plane(dev_priv, plane, pipe);
>
> +       /*
> +        * On gen2 planes are double buffered but the pipe isn't, so we
> must
> +        * wait for planes to fully turn off before disabling the pipe.
> +        */
> +       if (IS_GEN2(dev))
> +               intel_wait_for_vblank(dev, pipe);
> +
>         intel_set_cpu_fifo_underrun_reporting(dev, pipe, false);
>         intel_disable_pipe(dev_priv, pipe);
>
> --
> 1.8.3.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[-- Attachment #1.2: Type: text/html, Size: 4180 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 4/5] drm/i915: Wait for pending page flips before enabling/disabling the primary plane

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 3462 bytes --]

It seems it needs a rebase...
I couldn't figure out where exactly you will put the wait_for_pending so
I'm going to wait a v2.

But this is my fault that took a century to review it... I'm sorry...


On Tue, Apr 15, 2014 at 11:41 AM, <ville.syrjala@linux.intel.com> wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> We have to write to the primary plane base address registrer when we
> enable/disable the primary plane in response to sprite coverage. Those
> writes will cause the flip counter to increment which could interfere
> with the detection of CS flip completion. We could end up completing
> CS flips before the CS has even executed the commands from the ring.
>
> To avoid such issues, wait for CS flips to finish before we toggle the
> primary plane on/off.
>
> Testcase: igt/kms_mmio_vs_cs_flip/setplane_vs_cs_flip
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>  drivers/gpu/drm/i915/intel_drv.h     | 1 +
>  drivers/gpu/drm/i915/intel_sprite.c  | 4 ++++
>  3 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 9b181fc..c0fe5dd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3096,7 +3096,7 @@ bool intel_has_pending_fb_unpin(struct drm_device
> *dev)
>         return false;
>  }
>
> -static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
> +void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
>  {
>         struct drm_device *dev = crtc->dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index edef34e..f578e42 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -775,6 +775,7 @@ int valleyview_get_vco(struct drm_i915_private
> *dev_priv);
>  void intel_mode_from_pipe_config(struct drm_display_mode *mode,
>                                  struct intel_crtc_config *pipe_config);
>  int intel_format_to_fourcc(int format);
> +void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
>
>  /* intel_dp.c */
>  void intel_dp_init(struct drm_device *dev, int output_reg, enum port
> port);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 4df7245..36a8f5e 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -529,6 +529,8 @@ intel_enable_primary(struct drm_crtc *crtc)
>         if (intel_crtc->primary_enabled)
>                 return;
>
> +       intel_crtc_wait_for_pending_flips(crtc);
> +
>         intel_crtc->primary_enabled = true;
>
>         I915_WRITE(reg, I915_READ(reg) | DISPLAY_PLANE_ENABLE);
> @@ -558,6 +560,8 @@ intel_disable_primary(struct drm_crtc *crtc)
>         if (!intel_crtc->primary_enabled)
>                 return;
>
> +       intel_crtc_wait_for_pending_flips(crtc);
> +
>         intel_crtc->primary_enabled = false;
>
>         mutex_lock(&dev->struct_mutex);
> --
> 1.8.3.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[-- Attachment #1.2: Type: text/html, Size: 4484 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH v2 1/5] drm/i915: Fix mmio vs. CS flip race on ILK+

[Daniel Vetter]

On Tue, Apr 15, 2014 at 09:41:34PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Starting from ILK, mmio flips also cause a flip done interrupt to be
> signalled. This means if we first do a set_base and follow it
> immediately with the CS flip, we might mistake the flip done interrupt
> caused by the set_base as the flip done interrupt caused by the CS
> flip.
> 
> The hardware has a flip counter which increments every time a mmio or
> CS flip is issued. It basically counts the number of DSPSURF register
> writes. This means we can sample the counter before we put the CS
> flip into the ring, and then when we get a flip done interrupt we can
> check whether the CS flip has actually performed the surface address
> update, or if the interrupt was caused by a previous but yet
> unfinished mmio flip.
> 
> Even with the flip counter we still have a race condition of the CS flip
> base address update happens after the mmio flip done interrupt was
> raised but not yet processed by the driver. When the interrupt is
> eventually processed, the flip counter will already indicate that the
> CS flip has been executed, but it would not actually complete until the
> next start of vblank. We can use the DSPSURFLIVE register to check
> whether the hardware is actually scanning out of the buffer we expect,
> or if we managed hit this race window.
> 
> This covers all the cases where the CS flip actually changes the base
> address. If the base address remains unchanged, we might still complete
> the CS flip before it has actually completed. But since the address
> didn't change anyway, the premature flip completion can't result in
> userspace overwriting data that's still being scanned out.
> 
> CTG already has the flip counter and DSPSURFLIVE registers, and
> although the flip done interrupt is still limited to CS flips alone,
> the code now also checks the flip counter on CTG as well.
> 
> v2: s/dspsurf/gtt_offset/ (Chris)
> 
> Testcase: igt/kms_mmio_vs_cs_flip/setcrtc_vs_cs_flip
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=73027
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |  1 +
>  drivers/gpu/drm/i915/intel_display.c | 73 ++++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_drv.h     |  2 +
>  3 files changed, 69 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8f84555..c28169c 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3638,6 +3638,7 @@ enum punit_power_well {
>  #define _PIPEA_FRMCOUNT_GM45	0x70040
>  #define _PIPEA_FLIPCOUNT_GM45	0x70044
>  #define PIPE_FRMCOUNT_GM45(pipe) _PIPE2(pipe, _PIPEA_FRMCOUNT_GM45)
> +#define PIPE_FLIPCOUNT_GM45(pipe) _PIPE2(pipe, _PIPEA_FLIPCOUNT_GM45)
>  
>  /* Cursor A & B regs */
>  #define _CURACNTR		(dev_priv->info.display_mmio_offset + 0x70080)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1390ab5..32c6c16 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8577,6 +8577,48 @@ void intel_finish_page_flip_plane(struct drm_device *dev, int plane)
>  	do_intel_finish_page_flip(dev, crtc);
>  }
>  
> +/* Is 'a' after or equal to 'b'? */
> +static bool flip_count_after_eq(u32 a, u32 b)

I've added a g4x prefix here. The wrap-around semantics on earlier chips
(yeah I know doesn't exist, but the vblank counter does) wouldn't work.
-Daniel

> +{
> +	return !((a - b) & 0x80000000);
> +}
> +
> +static bool page_flip_finished(struct intel_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	/*
> +	 * The relevant registers doen't exist on pre-ctg.
> +	 * As the flip done interrupt doesn't trigger for mmio
> +	 * flips on gmch platforms, a flip count check isn't
> +	 * really needed there. But since ctg has the registers,
> +	 * include it in the check anyway.
> +	 */
> +	if (INTEL_INFO(dev)->gen < 5 && !IS_G4X(dev))
> +		return true;
> +
> +	/*
> +	 * A DSPSURFLIVE check isn't enough in case the mmio and CS flips
> +	 * used the same base address. In that case the mmio flip might
> +	 * have completed, but the CS hasn't even executed the flip yet.
> +	 *
> +	 * A flip count check isn't enough as the CS might have updated
> +	 * the base address just after start of vblank, but before we
> +	 * managed to process the interrupt. This means we'd complete the
> +	 * CS flip too soon.
> +	 *
> +	 * Combining both checks should get us a good enough result. It may
> +	 * still happen that the CS flip has been executed, but has not
> +	 * yet actually completed. But in case the base address is the same
> +	 * anyway, we don't really care.
> +	 */
> +	return (I915_READ(DSPSURFLIVE(crtc->plane)) & ~0xfff) ==
> +		crtc->unpin_work->gtt_offset &&
> +		flip_count_after_eq(I915_READ(PIPE_FLIPCOUNT_GM45(crtc->pipe)),
> +				    crtc->unpin_work->flip_count);
> +}
> +
>  void intel_prepare_page_flip(struct drm_device *dev, int plane)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -8589,7 +8631,7 @@ void intel_prepare_page_flip(struct drm_device *dev, int plane)
>  	 * is also accompanied by a spurious intel_prepare_page_flip().
>  	 */
>  	spin_lock_irqsave(&dev->event_lock, flags);
> -	if (intel_crtc->unpin_work)
> +	if (intel_crtc->unpin_work && page_flip_finished(intel_crtc))
>  		atomic_inc_not_zero(&intel_crtc->unpin_work->pending);
>  	spin_unlock_irqrestore(&dev->event_lock, flags);
>  }
> @@ -8619,6 +8661,9 @@ static int intel_gen2_queue_flip(struct drm_device *dev,
>  	if (ret)
>  		goto err;
>  
> +	intel_crtc->unpin_work->gtt_offset =
> +		i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
> +
>  	ret = intel_ring_begin(ring, 6);
>  	if (ret)
>  		goto err_unpin;
> @@ -8635,7 +8680,7 @@ static int intel_gen2_queue_flip(struct drm_device *dev,
>  	intel_ring_emit(ring, MI_DISPLAY_FLIP |
>  			MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
>  	intel_ring_emit(ring, fb->pitches[0]);
> -	intel_ring_emit(ring, i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset);
> +	intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset);
>  	intel_ring_emit(ring, 0); /* aux display base address, unused */
>  
>  	intel_mark_page_flip_active(intel_crtc);
> @@ -8664,6 +8709,9 @@ static int intel_gen3_queue_flip(struct drm_device *dev,
>  	if (ret)
>  		goto err;
>  
> +	intel_crtc->unpin_work->gtt_offset =
> +		i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
> +
>  	ret = intel_ring_begin(ring, 6);
>  	if (ret)
>  		goto err_unpin;
> @@ -8677,7 +8725,7 @@ static int intel_gen3_queue_flip(struct drm_device *dev,
>  	intel_ring_emit(ring, MI_DISPLAY_FLIP_I915 |
>  			MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
>  	intel_ring_emit(ring, fb->pitches[0]);
> -	intel_ring_emit(ring, i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset);
> +	intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset);
>  	intel_ring_emit(ring, MI_NOOP);
>  
>  	intel_mark_page_flip_active(intel_crtc);
> @@ -8706,6 +8754,9 @@ static int intel_gen4_queue_flip(struct drm_device *dev,
>  	if (ret)
>  		goto err;
>  
> +	intel_crtc->unpin_work->gtt_offset =
> +		i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
> +
>  	ret = intel_ring_begin(ring, 4);
>  	if (ret)
>  		goto err_unpin;
> @@ -8717,8 +8768,7 @@ static int intel_gen4_queue_flip(struct drm_device *dev,
>  	intel_ring_emit(ring, MI_DISPLAY_FLIP |
>  			MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
>  	intel_ring_emit(ring, fb->pitches[0]);
> -	intel_ring_emit(ring,
> -			(i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset) |
> +	intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset |
>  			obj->tiling_mode);
>  
>  	/* XXX Enabling the panel-fitter across page-flip is so far
> @@ -8755,6 +8805,9 @@ static int intel_gen6_queue_flip(struct drm_device *dev,
>  	if (ret)
>  		goto err;
>  
> +	intel_crtc->unpin_work->gtt_offset =
> +		i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
> +
>  	ret = intel_ring_begin(ring, 4);
>  	if (ret)
>  		goto err_unpin;
> @@ -8762,7 +8815,7 @@ static int intel_gen6_queue_flip(struct drm_device *dev,
>  	intel_ring_emit(ring, MI_DISPLAY_FLIP |
>  			MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
>  	intel_ring_emit(ring, fb->pitches[0] | obj->tiling_mode);
> -	intel_ring_emit(ring, i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset);
> +	intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset);
>  
>  	/* Contrary to the suggestions in the documentation,
>  	 * "Enable Panel Fitter" does not seem to be required when page
> @@ -8804,6 +8857,9 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
>  	if (ret)
>  		goto err;
>  
> +	intel_crtc->unpin_work->gtt_offset =
> +		i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
> +
>  	switch(intel_crtc->plane) {
>  	case PLANE_A:
>  		plane_bit = MI_DISPLAY_FLIP_IVB_PLANE_A;
> @@ -8881,7 +8937,7 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
>  
>  	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);
> +	intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset);
>  	intel_ring_emit(ring, (MI_NOOP));
>  
>  	intel_mark_page_flip_active(intel_crtc);
> @@ -8979,6 +9035,9 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	atomic_inc(&intel_crtc->unpin_work_count);
>  	intel_crtc->reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
>  
> +	if (INTEL_INFO(dev)->gen >= 5 || IS_G4X(dev))
> +		work->flip_count = I915_READ(PIPE_FLIPCOUNT_GM45(intel_crtc->pipe)) + 1;
> +
>  	ret = dev_priv->display.queue_flip(dev, crtc, fb, obj, page_flip_flags);
>  	if (ret)
>  		goto cleanup_pending;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index c551472..edef34e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -590,6 +590,8 @@ struct intel_unpin_work {
>  #define INTEL_FLIP_INACTIVE	0
>  #define INTEL_FLIP_PENDING	1
>  #define INTEL_FLIP_COMPLETE	2
> +	u32 flip_count;
> +	u32 gtt_offset;
>  	bool enable_stall_check;
>  };
>  
> -- 
> 1.8.3.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH v2 4/5] drm/i915: Wait for pending page flips before enabling/disabling the primary plane

[ville.syrjala]

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

We have to write to the primary plane base address registrer when we
enable/disable the primary plane in response to sprite coverage. Those
writes will cause the flip counter to increment which could interfere
with the detection of CS flip completion. We could end up completing
CS flips before the CS has even executed the commands from the ring.

To avoid such issues, wait for CS flips to finish before we toggle the
primary plane on/off.

v2: Rebased due to atomic sprite update changes

Testcase: igt/kms_mmio_vs_cs_flip/setplane_vs_cs_flip
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 drivers/gpu/drm/i915/intel_drv.h     | 2 ++
 drivers/gpu/drm/i915/intel_sprite.c  | 3 +++
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8ca8b2f..849bc71 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3291,7 +3291,7 @@ bool intel_has_pending_fb_unpin(struct drm_device *dev)
 	return false;
 }
 
-static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
+void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a78eb75..b9b13eb 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -801,6 +801,8 @@ int valleyview_get_vco(struct drm_i915_private *dev_priv);
 void intel_mode_from_pipe_config(struct drm_display_mode *mode,
 				 struct intel_crtc_config *pipe_config);
 int intel_format_to_fourcc(int format);
+void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
+
 
 /* intel_dp.c */
 void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 7780f6c..d6acd6b 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1018,6 +1018,9 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 
 		intel_crtc->primary_enabled = primary_enabled;
 
+		if (primary_was_enabled != primary_enabled)
+			intel_crtc_wait_for_pending_flips(crtc);
+
 		if (primary_was_enabled && !primary_enabled)
 			intel_pre_disable_primary(crtc);
 
-- 
1.8.5.5

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

[PATCH igt v2] tests/kms_mmio_vs_cs_flip: Add a test case to exercise mmio vs. CS flip races

[ville.syrjala]

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

kms_mmio_vs_cs_flip has two subtests:
- setplane_vs_cs_flip tests the interaction between
  fullscreen sprites and CS flips
- setcrtc_vs_cs_flip tests the interaction between
  primary plane panning and CS flips

v2: Skip sprite test when there are no sprites
    Reduce busy_bo to 64MB (now works on my gen2)
    Handle pipe vs. port incompatibility

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 tests/Makefile.sources      |   1 +
 tests/kms_mmio_vs_cs_flip.c | 537 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 538 insertions(+)
 create mode 100644 tests/kms_mmio_vs_cs_flip.c

diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 383a209..1f7931d 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -64,6 +64,7 @@ TESTS_progs_M = \
 	kms_fence_pin_leak \
 	kms_flip \
 	kms_flip_tiling \
+	kms_mmio_vs_cs_flip \
 	kms_pipe_crc_basic \
 	kms_plane \
 	kms_render \
diff --git a/tests/kms_mmio_vs_cs_flip.c b/tests/kms_mmio_vs_cs_flip.c
new file mode 100644
index 0000000..a724721
--- /dev/null
+++ b/tests/kms_mmio_vs_cs_flip.c
@@ -0,0 +1,537 @@
+/*
+ * Copyright © 2014 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include <errno.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <string.h>
+#include <time.h>
+
+#include "drmtest.h"
+#include "igt_debugfs.h"
+#include "igt_kms.h"
+#include "intel_chipset.h"
+#include "ioctl_wrappers.h"
+
+typedef struct {
+	int drm_fd;
+	igt_display_t display;
+	igt_pipe_crc_t *pipe_crc;
+	drm_intel_bufmgr *bufmgr;
+	drm_intel_bo *busy_bo;
+	uint32_t devid;
+	bool flip_done;
+} data_t;
+
+static void exec_nop(data_t *data, uint32_t handle, unsigned int ring)
+{
+	struct intel_batchbuffer *batch;
+	drm_intel_bo *bo;
+
+	batch = intel_batchbuffer_alloc(data->bufmgr, data->devid);
+	igt_assert(batch);
+
+	bo = gem_handle_to_libdrm_bo(data->bufmgr, data->drm_fd, "", handle);
+	igt_assert(bo);
+
+	/* add relocs to make sure the kernel will think we write to dst */
+	BEGIN_BATCH(4);
+	OUT_BATCH(MI_BATCH_BUFFER_END);
+	OUT_BATCH(MI_NOOP);
+	OUT_RELOC(bo, I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER, 0);
+	OUT_BATCH(MI_NOOP);
+	ADVANCE_BATCH();
+
+	intel_batchbuffer_flush_on_ring(batch, ring);
+	intel_batchbuffer_free(batch);
+
+	drm_intel_bo_unreference(bo);
+}
+
+static void exec_blt(data_t *data)
+{
+	struct intel_batchbuffer *batch;
+	int w, h, pitch, i;
+
+	batch = intel_batchbuffer_alloc(data->bufmgr, data->devid);
+	igt_assert(batch);
+
+	w = 8192;
+	h = data->busy_bo->size / (8192 * 4);
+	pitch = w * 4;
+
+	for (i = 0; i < 40; i++) {
+		BLIT_COPY_BATCH_START(data->devid, 0);
+		OUT_BATCH((3 << 24) | /* 32 bits */
+			  (0xcc << 16) | /* copy ROP */
+			  pitch);
+		OUT_BATCH(0 << 16 | 0);
+		OUT_BATCH(h << 16 | w);
+		OUT_RELOC(data->busy_bo, I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER, 0);
+		BLIT_RELOC_UDW(data->devid);
+		OUT_BATCH(0 << 16 | 0);
+		OUT_BATCH(pitch);
+		OUT_RELOC(data->busy_bo, I915_GEM_DOMAIN_RENDER, 0, 0);
+		BLIT_RELOC_UDW(data->devid);
+		ADVANCE_BATCH();
+	}
+
+	intel_batchbuffer_flush(batch);
+	intel_batchbuffer_free(batch);
+}
+
+static void page_flip_handler(int fd, unsigned int frame, unsigned int sec,
+			      unsigned int usec, void *_data)
+{
+	data_t *data = _data;
+
+	data->flip_done = true;
+}
+
+static void wait_for_flip(data_t *data, uint32_t flip_handle)
+{
+	struct timeval timeout = {
+		.tv_sec = 3,
+		.tv_usec = 0,
+	};
+	drmEventContext evctx = {
+		.version = DRM_EVENT_CONTEXT_VERSION,
+		.page_flip_handler = page_flip_handler,
+	};
+	fd_set fds;
+
+	FD_ZERO(&fds);
+	FD_SET(data->drm_fd, &fds);
+
+	while (!data->flip_done) {
+		int ret = select(data->drm_fd + 1, &fds, NULL, NULL, &timeout);
+
+		if (ret < 0 && errno == EINTR)
+			continue;
+
+		igt_assert(ret >= 0);
+
+		do_or_die(drmHandleEvent(data->drm_fd, &evctx));
+	}
+
+	/*
+	 * The flip completion may have been signalled prematurely, so
+	 * also submit another nop batch and wait for it to make sure
+	 * the ring has really been drained.
+	 */
+	if (IS_GEN7(data->devid) || IS_GEN8(data->devid))
+		exec_nop(data, flip_handle, I915_EXEC_BLT);
+	else
+		exec_nop(data, flip_handle, I915_EXEC_RENDER);
+	gem_sync(data->drm_fd, flip_handle);
+}
+
+static void make_gpu_busy(data_t *data, uint32_t flip_handle)
+{
+	/*
+	 * Make sure flip_handle has been used on the blt ring.
+	 * This should make the flip use the same ring on gen7+.
+	 */
+	if (IS_GEN7(data->devid) || IS_GEN8(data->devid))
+		exec_nop(data, flip_handle, I915_EXEC_BLT);
+
+	/*
+	 * Add a pile commands to the ring.  The flip will be
+	 * stuck behing these commands and hence gets delayed
+	 * significantly.
+	 */
+	exec_blt(data);
+
+	/*
+	 * Make sure the render ring will block until the blt ring is clear.
+	 * This is in case the flip will execute on the render ring and the
+	 * blits were on the blt ring (this will be the case on gen6 at least).
+	 *
+	 * We can't add an explicit dependency between flip_handle and the
+	 * blits since that would cause the driver to block until the blits
+	 * have completed before it will perform a subsequent mmio flip,
+	 * and so the test would fail to exercise the mmio vs. CS flip race.
+	 */
+	if (HAS_BLT_RING(data->devid))
+		exec_nop(data, data->busy_bo->handle, I915_EXEC_RENDER);
+}
+
+/*
+ * 1. set primary plane to full red
+ * 2. grab a reference crc
+ * 3. set primary plane to full blue
+ * 4. queue lots of GPU activity to delay the subsequent page flip
+ * 5. queue a page flip to the same blue fb
+ * 6. toggle a fullscreen sprite (green) on and back off again
+ * 7. set primary plane to red fb
+ * 8. wait for GPU to finish
+ * 9. compare current crc with reference crc
+ *
+ * We expect the primary plane to display full red at the end.
+ * If the sprite operations have interfered with the page flip,
+ * the driver may have mistakenly completed the flip before
+ * it was executed by the CS, and hence the subsequent mmio
+ * flips may have overtaken it. So once we've finished everything
+ * the CS flip may have been the last thing to occur, which means
+ * the primary plane may be full blue instead of the red it's
+ * supposed to be.
+ */
+static void
+test_plane(data_t *data, igt_output_t *output, enum pipe pipe, enum igt_plane plane)
+{
+	struct igt_fb red_fb, green_fb, blue_fb;
+	drmModeModeInfo *mode;
+	igt_plane_t *primary, *sprite;
+	igt_crc_t ref_crc, crc;
+	int ret;
+
+	igt_output_set_pipe(output, pipe);
+	igt_display_commit(&data->display);
+
+	if (!output->valid) {
+		igt_output_set_pipe(output, PIPE_ANY);
+		igt_display_commit(&data->display);
+		return;
+	}
+
+	primary = igt_output_get_plane(output, 0);
+	sprite = igt_output_get_plane(output, plane);
+
+	mode = igt_output_get_mode(output);
+	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
+			    DRM_FORMAT_XRGB8888,
+			    false, /* tiled */
+			    1.0, 0.0, 0.0,
+			    &red_fb);
+	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
+			    DRM_FORMAT_XRGB8888,
+			    false, /* tiled */
+			    0.0, 1.0, 0.0,
+			    &green_fb);
+	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
+			    DRM_FORMAT_XRGB8888,
+			    false, /* tiled */
+			    0.0, 0.0, 1.0,
+			    &blue_fb);
+
+	/*
+	 * Make sure these buffers are suited for display use
+	 * because most of the modeset operations must be fast
+	 * later on.
+	 */
+	igt_plane_set_fb(primary, &blue_fb);
+	igt_display_commit(&data->display);
+	igt_plane_set_fb(sprite, &green_fb);
+	igt_display_commit(&data->display);
+	igt_plane_set_fb(sprite, NULL);
+	igt_display_commit(&data->display);
+
+	if (data->pipe_crc)
+		igt_pipe_crc_free(data->pipe_crc);
+	data->pipe_crc = igt_pipe_crc_new(pipe, INTEL_PIPE_CRC_SOURCE_AUTO);
+
+	/* set red fb and grab reference crc */
+	igt_plane_set_fb(primary, &red_fb);
+	igt_display_commit(&data->display);
+	igt_pipe_crc_collect_crc(data->pipe_crc, &ref_crc);
+
+	ret = drmModeSetCrtc(data->drm_fd, output->config.crtc->crtc_id,
+			     blue_fb.fb_id, 0, 0, &output->id, 1,
+			     mode);
+	igt_assert(ret == 0);
+
+	make_gpu_busy(data, blue_fb.gem_handle);
+
+	data->flip_done = false;
+	ret = drmModePageFlip(data->drm_fd, output->config.crtc->crtc_id,
+			      blue_fb.fb_id, DRM_MODE_PAGE_FLIP_EVENT, data);
+	igt_assert(ret == 0);
+
+	/*
+	 * Toggle a fullscreen sprite on and back off. This will result
+	 * in the primary plane getting disabled and re-enbled, and that
+	 * leads to mmio flips. The driver may then mistake the flip done
+	 * interrupts from the mmio flips as the flip done interrupts for
+	 * the CS flip, and hence subsequent mmio flips won't wait for the
+	 * CS flips like they should.
+	 */
+	ret = drmModeSetPlane(data->drm_fd,
+			      sprite->drm_plane->plane_id,
+			      output->config.crtc->crtc_id,
+			      green_fb.fb_id, 0,
+			      0, 0, mode->hdisplay, mode->vdisplay,
+			      0, 0, mode->hdisplay << 16, mode->vdisplay << 16);
+	igt_assert(ret == 0);
+	ret = drmModeSetPlane(data->drm_fd,
+			      sprite->drm_plane->plane_id,
+			      output->config.crtc->crtc_id,
+			      0, 0,
+			      0, 0, 0, 0,
+			      0, 0, 0, 0);
+	igt_assert(ret == 0);
+
+	/*
+	 * Set primary plane to red fb. This should wait for the CS flip
+	 * to complete. But if the kernel mistook the flip done interrupt
+	 * from the mmio flip as the flip done from the CS flip, this will
+	 * not wait for anything. And hence the the CS flip will actually
+	 * occur after this mmio flip.
+	 */
+	ret = drmModeSetCrtc(data->drm_fd, output->config.crtc->crtc_id,
+			     red_fb.fb_id, 0, 0, &output->id, 1,
+			     mode);
+	igt_assert(ret == 0);
+
+	/* Make sure the flip has been executed */
+	wait_for_flip(data, blue_fb.gem_handle);
+
+	/* Grab crc and compare with the extected result */
+	igt_pipe_crc_collect_crc(data->pipe_crc, &crc);
+
+	igt_plane_set_fb(primary, NULL);
+	igt_display_commit(&data->display);
+
+	igt_remove_fb(data->drm_fd, &red_fb);
+	igt_remove_fb(data->drm_fd, &green_fb);
+	igt_remove_fb(data->drm_fd, &blue_fb);
+
+	igt_pipe_crc_free(data->pipe_crc);
+	data->pipe_crc = NULL;
+
+	igt_output_set_pipe(output, PIPE_ANY);
+	igt_display_commit(&data->display);
+
+	igt_assert(igt_crc_equal(&ref_crc, &crc));
+}
+
+/*
+ * 1. set primary plane to full red
+ * 2. grab a reference crc
+ * 3. set primary plane to full green
+ * 4. wait for vblank
+ * 5. pan primary plane a bit (to cause a mmio flip w/o vblank wait)
+ * 6. queue lots of GPU activity to delay the subsequent page flip
+ * 6. queue a page flip to a blue fb
+ * 7. set primary plane to red fb
+ * 8. wait for GPU to finish
+ * 9. compare current crc with reference crc
+ *
+ * We expect the primary plane to display full red at the end.
+ * If the previously schedule primary plane pan operation has interfered
+ * with the following page flip, the driver may have mistakenly completed
+ * the flip before it was executed by the CS, and hence the subsequent mmio
+ * flips may have overtaken it. So once we've finished everything
+ * the CS flip may have been the last thing to occur, which means
+ * the primary plane may be full blue instead of the red it's
+ * supposed to be.
+ */
+static void
+test_crtc(data_t *data, igt_output_t *output, enum pipe pipe)
+{
+	struct igt_fb red_fb, green_fb, blue_fb;
+	drmModeModeInfo *mode;
+	igt_plane_t *primary;
+	igt_crc_t ref_crc, crc;
+	int ret;
+
+	igt_output_set_pipe(output, pipe);
+	igt_display_commit(&data->display);
+
+	if (!output->valid) {
+		igt_output_set_pipe(output, PIPE_ANY);
+		igt_display_commit(&data->display);
+		return;
+	}
+
+	primary = igt_output_get_plane(output, 0);
+
+	mode = igt_output_get_mode(output);
+	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay+1,
+			    DRM_FORMAT_XRGB8888,
+			    false, /* tiled */
+			    1.0, 0.0, 0.0,
+			    &red_fb);
+	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay+1,
+			    DRM_FORMAT_XRGB8888,
+			    false, /* tiled */
+			    0.0, 0.0, 1.0,
+			    &blue_fb);
+	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay+1,
+			    DRM_FORMAT_XRGB8888,
+			    false, /* tiled */
+			    0.0, 1.0, 0.0,
+			    &green_fb);
+
+	/*
+	 * Make sure these buffers are suited for display use
+	 * because most of the modeset operations must be fast
+	 * later on.
+	 */
+	igt_plane_set_fb(primary, &green_fb);
+	igt_display_commit(&data->display);
+	igt_plane_set_fb(primary, &blue_fb);
+	igt_display_commit(&data->display);
+
+	if (data->pipe_crc)
+		igt_pipe_crc_free(data->pipe_crc);
+	data->pipe_crc = igt_pipe_crc_new(pipe, INTEL_PIPE_CRC_SOURCE_AUTO);
+
+	/* set red fb and grab reference crc */
+	igt_plane_set_fb(primary, &red_fb);
+	igt_display_commit(&data->display);
+	igt_pipe_crc_collect_crc(data->pipe_crc, &ref_crc);
+
+	/*
+	 * Further down we need to issue an mmio flip w/o the kernel
+	 * waiting for vblank. The easiest way is to just pan within
+	 * the same FB. So pan away a bit here, and later we undo this
+	 * with another pan which will result in the desired mmio flip.
+	 */
+	ret = drmModeSetCrtc(data->drm_fd, output->config.crtc->crtc_id,
+			     green_fb.fb_id, 0, 1, &output->id, 1,
+			     mode);
+	igt_assert(ret == 0);
+
+	/*
+	 * Make it more likely that the CS flip has been submitted into the
+	 * ring by the time the mmio flip from the drmModeSetCrtc() below
+	 * completes. The driver will then mistake the flip done interrupt
+	 * from the mmio flip as the flip done interrupt from the CS flip.
+	 */
+	igt_wait_for_vblank(data->drm_fd, pipe);
+
+	/* now issue the mmio flip w/o vblank waits in the kernel, ie. pan a bit */
+	ret = drmModeSetCrtc(data->drm_fd, output->config.crtc->crtc_id,
+			     green_fb.fb_id, 0, 0, &output->id, 1,
+			     mode);
+	igt_assert(ret == 0);
+
+	make_gpu_busy(data, blue_fb.gem_handle);
+
+	/*
+	 * Submit the CS flip. The commands must be emitted into the ring
+	 * before the mmio flip from the panning operation completes.
+	 */
+	data->flip_done = false;
+	ret = drmModePageFlip(data->drm_fd, output->config.crtc->crtc_id,
+			      blue_fb.fb_id, DRM_MODE_PAGE_FLIP_EVENT, data);
+	igt_assert(ret == 0);
+
+	/*
+	 * Set primary plane to red fb. This should wait for the CS flip
+	 * to complete. But if the kernel mistook the flip done interrupt
+	 * from the mmio flip as the flip done from the CS flip, this will
+	 * not wait for anything. And hence the the CS flip will actually
+	 * occur after this mmio flip.
+	 */
+	ret = drmModeSetCrtc(data->drm_fd, output->config.crtc->crtc_id,
+			     red_fb.fb_id, 0, 0, &output->id, 1,
+			     mode);
+	igt_assert(ret == 0);
+
+	/* Make sure the flip has been executed */
+	wait_for_flip(data, blue_fb.gem_handle);
+
+	/* Grab crc and compare with the extected result */
+	igt_pipe_crc_collect_crc(data->pipe_crc, &crc);
+
+	igt_plane_set_fb(primary, NULL);
+	igt_display_commit(&data->display);
+
+	igt_remove_fb(data->drm_fd, &red_fb);
+	igt_remove_fb(data->drm_fd, &green_fb);
+	igt_remove_fb(data->drm_fd, &blue_fb);
+
+	igt_pipe_crc_free(data->pipe_crc);
+	data->pipe_crc = NULL;
+
+	igt_output_set_pipe(output, PIPE_ANY);
+	igt_display_commit(&data->display);
+
+	igt_assert(igt_crc_equal(&ref_crc, &crc));
+}
+
+static void
+run_plane_test_for_pipe(data_t *data, enum pipe pipe)
+{
+	igt_output_t *output;
+	enum igt_plane plane = 1; /* testing with one sprite is enough */
+
+	igt_require(data->display.pipes[pipe].n_planes > 2);
+
+	for_each_connected_output(&data->display, output)
+		test_plane(data, output, pipe, plane);
+}
+
+static void
+run_crtc_test_for_pipe(data_t *data, enum pipe pipe)
+{
+	igt_output_t *output;
+
+	for_each_connected_output(&data->display, output)
+		test_crtc(data, output, pipe);
+}
+
+static data_t data;
+
+igt_main
+{
+	int pipe;
+
+	igt_skip_on_simulation();
+
+	igt_fixture {
+		data.drm_fd = drm_open_any();
+
+		igt_set_vt_graphics_mode();
+
+		data.devid = intel_get_drm_devid(data.drm_fd);
+
+		igt_require_pipe_crc();
+		igt_display_init(&data.display, data.drm_fd);
+
+		data.bufmgr = drm_intel_bufmgr_gem_init(data.drm_fd, 4096);
+		igt_assert(data.bufmgr);
+		drm_intel_bufmgr_gem_enable_reuse(data.bufmgr);
+
+		data.busy_bo = drm_intel_bo_alloc(data.bufmgr, "bo",
+						  64*1024*1024, 4096);
+		gem_set_tiling(data.drm_fd, data.busy_bo->handle, 0, 4096);
+	}
+
+	igt_subtest_f("setplane_vs_cs_flip") {
+		for (pipe = 0; pipe < data.display.n_pipes; pipe++)
+			run_plane_test_for_pipe(&data, pipe);
+	}
+
+	igt_subtest_f("setcrtc_vs_cs_flip") {
+		for (pipe = 0; pipe < data.display.n_pipes; pipe++)
+			run_crtc_test_for_pipe(&data, pipe);
+	}
+
+	igt_fixture {
+		drm_intel_bo_unreference(data.busy_bo);
+		drm_intel_bufmgr_destroy(data.bufmgr);
+		igt_display_fini(&data.display);
+	}
+}
-- 
1.8.5.5

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

Re: [PATCH v2 4/5] drm/i915: Wait for pending page flips before enabling/disabling the primary plane

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 3189 bytes --]

Reviewed-by Rodrigo Vivi <rodrigo.vivi@gmail.com>


On Wed, May 21, 2014 at 4:04 AM, <ville.syrjala@linux.intel.com> wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> We have to write to the primary plane base address registrer when we
> enable/disable the primary plane in response to sprite coverage. Those
> writes will cause the flip counter to increment which could interfere
> with the detection of CS flip completion. We could end up completing
> CS flips before the CS has even executed the commands from the ring.
>
> To avoid such issues, wait for CS flips to finish before we toggle the
> primary plane on/off.
>
> v2: Rebased due to atomic sprite update changes
>
> Testcase: igt/kms_mmio_vs_cs_flip/setplane_vs_cs_flip
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>  drivers/gpu/drm/i915/intel_drv.h     | 2 ++
>  drivers/gpu/drm/i915/intel_sprite.c  | 3 +++
>  3 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 8ca8b2f..849bc71 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3291,7 +3291,7 @@ bool intel_has_pending_fb_unpin(struct drm_device
> *dev)
>         return false;
>  }
>
> -static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
> +void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
>  {
>         struct drm_device *dev = crtc->dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index a78eb75..b9b13eb 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -801,6 +801,8 @@ int valleyview_get_vco(struct drm_i915_private
> *dev_priv);
>  void intel_mode_from_pipe_config(struct drm_display_mode *mode,
>                                  struct intel_crtc_config *pipe_config);
>  int intel_format_to_fourcc(int format);
> +void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
> +
>
>  /* intel_dp.c */
>  void intel_dp_init(struct drm_device *dev, int output_reg, enum port
> port);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 7780f6c..d6acd6b 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1018,6 +1018,9 @@ intel_update_plane(struct drm_plane *plane, struct
> drm_crtc *crtc,
>
>                 intel_crtc->primary_enabled = primary_enabled;
>
> +               if (primary_was_enabled != primary_enabled)
> +                       intel_crtc_wait_for_pending_flips(crtc);
> +
>                 if (primary_was_enabled && !primary_enabled)
>                         intel_pre_disable_primary(crtc);
>
> --
> 1.8.5.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[-- Attachment #1.2: Type: text/html, Size: 4346 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH igt v2] tests/kms_mmio_vs_cs_flip: Add a test case to exercise mmio vs. CS flip races

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 23263 bytes --]

tests also look good for me so:
Reviewed-by Rodrigo Vivi <rodrigo.vivi@gmail.com>


On Wed, May 21, 2014 at 5:17 AM, <ville.syrjala@linux.intel.com> wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> kms_mmio_vs_cs_flip has two subtests:
> - setplane_vs_cs_flip tests the interaction between
>   fullscreen sprites and CS flips
> - setcrtc_vs_cs_flip tests the interaction between
>   primary plane panning and CS flips
>
> v2: Skip sprite test when there are no sprites
>     Reduce busy_bo to 64MB (now works on my gen2)
>     Handle pipe vs. port incompatibility
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  tests/Makefile.sources      |   1 +
>  tests/kms_mmio_vs_cs_flip.c | 537
> ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 538 insertions(+)
>  create mode 100644 tests/kms_mmio_vs_cs_flip.c
>
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index 383a209..1f7931d 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -64,6 +64,7 @@ TESTS_progs_M = \
>         kms_fence_pin_leak \
>         kms_flip \
>         kms_flip_tiling \
> +       kms_mmio_vs_cs_flip \
>         kms_pipe_crc_basic \
>         kms_plane \
>         kms_render \
> diff --git a/tests/kms_mmio_vs_cs_flip.c b/tests/kms_mmio_vs_cs_flip.c
> new file mode 100644
> index 0000000..a724721
> --- /dev/null
> +++ b/tests/kms_mmio_vs_cs_flip.c
> @@ -0,0 +1,537 @@
> +/*
> + * Copyright © 2014 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the
> "Software"),
> + * to deal in the Software without restriction, including without
> limitation
> + * the rights to use, copy, modify, merge, publish, distribute,
> sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> next
> + * paragraph) shall be included in all copies or substantial portions of
> the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT
> SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#include <errno.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <time.h>
> +
> +#include "drmtest.h"
> +#include "igt_debugfs.h"
> +#include "igt_kms.h"
> +#include "intel_chipset.h"
> +#include "ioctl_wrappers.h"
> +
> +typedef struct {
> +       int drm_fd;
> +       igt_display_t display;
> +       igt_pipe_crc_t *pipe_crc;
> +       drm_intel_bufmgr *bufmgr;
> +       drm_intel_bo *busy_bo;
> +       uint32_t devid;
> +       bool flip_done;
> +} data_t;
> +
> +static void exec_nop(data_t *data, uint32_t handle, unsigned int ring)
> +{
> +       struct intel_batchbuffer *batch;
> +       drm_intel_bo *bo;
> +
> +       batch = intel_batchbuffer_alloc(data->bufmgr, data->devid);
> +       igt_assert(batch);
> +
> +       bo = gem_handle_to_libdrm_bo(data->bufmgr, data->drm_fd, "",
> handle);
> +       igt_assert(bo);
> +
> +       /* add relocs to make sure the kernel will think we write to dst */
> +       BEGIN_BATCH(4);
> +       OUT_BATCH(MI_BATCH_BUFFER_END);
> +       OUT_BATCH(MI_NOOP);
> +       OUT_RELOC(bo, I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER, 0);
> +       OUT_BATCH(MI_NOOP);
> +       ADVANCE_BATCH();
> +
> +       intel_batchbuffer_flush_on_ring(batch, ring);
> +       intel_batchbuffer_free(batch);
> +
> +       drm_intel_bo_unreference(bo);
> +}
> +
> +static void exec_blt(data_t *data)
> +{
> +       struct intel_batchbuffer *batch;
> +       int w, h, pitch, i;
> +
> +       batch = intel_batchbuffer_alloc(data->bufmgr, data->devid);
> +       igt_assert(batch);
> +
> +       w = 8192;
> +       h = data->busy_bo->size / (8192 * 4);
> +       pitch = w * 4;
> +
> +       for (i = 0; i < 40; i++) {
> +               BLIT_COPY_BATCH_START(data->devid, 0);
> +               OUT_BATCH((3 << 24) | /* 32 bits */
> +                         (0xcc << 16) | /* copy ROP */
> +                         pitch);
> +               OUT_BATCH(0 << 16 | 0);
> +               OUT_BATCH(h << 16 | w);
> +               OUT_RELOC(data->busy_bo, I915_GEM_DOMAIN_RENDER,
> I915_GEM_DOMAIN_RENDER, 0);
> +               BLIT_RELOC_UDW(data->devid);
> +               OUT_BATCH(0 << 16 | 0);
> +               OUT_BATCH(pitch);
> +               OUT_RELOC(data->busy_bo, I915_GEM_DOMAIN_RENDER, 0, 0);
> +               BLIT_RELOC_UDW(data->devid);
> +               ADVANCE_BATCH();
> +       }
> +
> +       intel_batchbuffer_flush(batch);
> +       intel_batchbuffer_free(batch);
> +}
> +
> +static void page_flip_handler(int fd, unsigned int frame, unsigned int
> sec,
> +                             unsigned int usec, void *_data)
> +{
> +       data_t *data = _data;
> +
> +       data->flip_done = true;
> +}
> +
> +static void wait_for_flip(data_t *data, uint32_t flip_handle)
> +{
> +       struct timeval timeout = {
> +               .tv_sec = 3,
> +               .tv_usec = 0,
> +       };
> +       drmEventContext evctx = {
> +               .version = DRM_EVENT_CONTEXT_VERSION,
> +               .page_flip_handler = page_flip_handler,
> +       };
> +       fd_set fds;
> +
> +       FD_ZERO(&fds);
> +       FD_SET(data->drm_fd, &fds);
> +
> +       while (!data->flip_done) {
> +               int ret = select(data->drm_fd + 1, &fds, NULL, NULL,
> &timeout);
> +
> +               if (ret < 0 && errno == EINTR)
> +                       continue;
> +
> +               igt_assert(ret >= 0);
> +
> +               do_or_die(drmHandleEvent(data->drm_fd, &evctx));
> +       }
> +
> +       /*
> +        * The flip completion may have been signalled prematurely, so
> +        * also submit another nop batch and wait for it to make sure
> +        * the ring has really been drained.
> +        */
> +       if (IS_GEN7(data->devid) || IS_GEN8(data->devid))
> +               exec_nop(data, flip_handle, I915_EXEC_BLT);
> +       else
> +               exec_nop(data, flip_handle, I915_EXEC_RENDER);
> +       gem_sync(data->drm_fd, flip_handle);
> +}
> +
> +static void make_gpu_busy(data_t *data, uint32_t flip_handle)
> +{
> +       /*
> +        * Make sure flip_handle has been used on the blt ring.
> +        * This should make the flip use the same ring on gen7+.
> +        */
> +       if (IS_GEN7(data->devid) || IS_GEN8(data->devid))
> +               exec_nop(data, flip_handle, I915_EXEC_BLT);
> +
> +       /*
> +        * Add a pile commands to the ring.  The flip will be
> +        * stuck behing these commands and hence gets delayed
> +        * significantly.
> +        */
> +       exec_blt(data);
> +
> +       /*
> +        * Make sure the render ring will block until the blt ring is
> clear.
> +        * This is in case the flip will execute on the render ring and the
> +        * blits were on the blt ring (this will be the case on gen6 at
> least).
> +        *
> +        * We can't add an explicit dependency between flip_handle and the
> +        * blits since that would cause the driver to block until the blits
> +        * have completed before it will perform a subsequent mmio flip,
> +        * and so the test would fail to exercise the mmio vs. CS flip
> race.
> +        */
> +       if (HAS_BLT_RING(data->devid))
> +               exec_nop(data, data->busy_bo->handle, I915_EXEC_RENDER);
> +}
> +
> +/*
> + * 1. set primary plane to full red
> + * 2. grab a reference crc
> + * 3. set primary plane to full blue
> + * 4. queue lots of GPU activity to delay the subsequent page flip
> + * 5. queue a page flip to the same blue fb
> + * 6. toggle a fullscreen sprite (green) on and back off again
> + * 7. set primary plane to red fb
> + * 8. wait for GPU to finish
> + * 9. compare current crc with reference crc
> + *
> + * We expect the primary plane to display full red at the end.
> + * If the sprite operations have interfered with the page flip,
> + * the driver may have mistakenly completed the flip before
> + * it was executed by the CS, and hence the subsequent mmio
> + * flips may have overtaken it. So once we've finished everything
> + * the CS flip may have been the last thing to occur, which means
> + * the primary plane may be full blue instead of the red it's
> + * supposed to be.
> + */
> +static void
> +test_plane(data_t *data, igt_output_t *output, enum pipe pipe, enum
> igt_plane plane)
> +{
> +       struct igt_fb red_fb, green_fb, blue_fb;
> +       drmModeModeInfo *mode;
> +       igt_plane_t *primary, *sprite;
> +       igt_crc_t ref_crc, crc;
> +       int ret;
> +
> +       igt_output_set_pipe(output, pipe);
> +       igt_display_commit(&data->display);
> +
> +       if (!output->valid) {
> +               igt_output_set_pipe(output, PIPE_ANY);
> +               igt_display_commit(&data->display);
> +               return;
> +       }
> +
> +       primary = igt_output_get_plane(output, 0);
> +       sprite = igt_output_get_plane(output, plane);
> +
> +       mode = igt_output_get_mode(output);
> +       igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
> +                           DRM_FORMAT_XRGB8888,
> +                           false, /* tiled */
> +                           1.0, 0.0, 0.0,
> +                           &red_fb);
> +       igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
> +                           DRM_FORMAT_XRGB8888,
> +                           false, /* tiled */
> +                           0.0, 1.0, 0.0,
> +                           &green_fb);
> +       igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
> +                           DRM_FORMAT_XRGB8888,
> +                           false, /* tiled */
> +                           0.0, 0.0, 1.0,
> +                           &blue_fb);
> +
> +       /*
> +        * Make sure these buffers are suited for display use
> +        * because most of the modeset operations must be fast
> +        * later on.
> +        */
> +       igt_plane_set_fb(primary, &blue_fb);
> +       igt_display_commit(&data->display);
> +       igt_plane_set_fb(sprite, &green_fb);
> +       igt_display_commit(&data->display);
> +       igt_plane_set_fb(sprite, NULL);
> +       igt_display_commit(&data->display);
> +
> +       if (data->pipe_crc)
> +               igt_pipe_crc_free(data->pipe_crc);
> +       data->pipe_crc = igt_pipe_crc_new(pipe,
> INTEL_PIPE_CRC_SOURCE_AUTO);
> +
> +       /* set red fb and grab reference crc */
> +       igt_plane_set_fb(primary, &red_fb);
> +       igt_display_commit(&data->display);
> +       igt_pipe_crc_collect_crc(data->pipe_crc, &ref_crc);
> +
> +       ret = drmModeSetCrtc(data->drm_fd, output->config.crtc->crtc_id,
> +                            blue_fb.fb_id, 0, 0, &output->id, 1,
> +                            mode);
> +       igt_assert(ret == 0);
> +
> +       make_gpu_busy(data, blue_fb.gem_handle);
> +
> +       data->flip_done = false;
> +       ret = drmModePageFlip(data->drm_fd, output->config.crtc->crtc_id,
> +                             blue_fb.fb_id, DRM_MODE_PAGE_FLIP_EVENT,
> data);
> +       igt_assert(ret == 0);
> +
> +       /*
> +        * Toggle a fullscreen sprite on and back off. This will result
> +        * in the primary plane getting disabled and re-enbled, and that
> +        * leads to mmio flips. The driver may then mistake the flip done
> +        * interrupts from the mmio flips as the flip done interrupts for
> +        * the CS flip, and hence subsequent mmio flips won't wait for the
> +        * CS flips like they should.
> +        */
> +       ret = drmModeSetPlane(data->drm_fd,
> +                             sprite->drm_plane->plane_id,
> +                             output->config.crtc->crtc_id,
> +                             green_fb.fb_id, 0,
> +                             0, 0, mode->hdisplay, mode->vdisplay,
> +                             0, 0, mode->hdisplay << 16, mode->vdisplay
> << 16);
> +       igt_assert(ret == 0);
> +       ret = drmModeSetPlane(data->drm_fd,
> +                             sprite->drm_plane->plane_id,
> +                             output->config.crtc->crtc_id,
> +                             0, 0,
> +                             0, 0, 0, 0,
> +                             0, 0, 0, 0);
> +       igt_assert(ret == 0);
> +
> +       /*
> +        * Set primary plane to red fb. This should wait for the CS flip
> +        * to complete. But if the kernel mistook the flip done interrupt
> +        * from the mmio flip as the flip done from the CS flip, this will
> +        * not wait for anything. And hence the the CS flip will actually
> +        * occur after this mmio flip.
> +        */
> +       ret = drmModeSetCrtc(data->drm_fd, output->config.crtc->crtc_id,
> +                            red_fb.fb_id, 0, 0, &output->id, 1,
> +                            mode);
> +       igt_assert(ret == 0);
> +
> +       /* Make sure the flip has been executed */
> +       wait_for_flip(data, blue_fb.gem_handle);
> +
> +       /* Grab crc and compare with the extected result */
> +       igt_pipe_crc_collect_crc(data->pipe_crc, &crc);
> +
> +       igt_plane_set_fb(primary, NULL);
> +       igt_display_commit(&data->display);
> +
> +       igt_remove_fb(data->drm_fd, &red_fb);
> +       igt_remove_fb(data->drm_fd, &green_fb);
> +       igt_remove_fb(data->drm_fd, &blue_fb);
> +
> +       igt_pipe_crc_free(data->pipe_crc);
> +       data->pipe_crc = NULL;
> +
> +       igt_output_set_pipe(output, PIPE_ANY);
> +       igt_display_commit(&data->display);
> +
> +       igt_assert(igt_crc_equal(&ref_crc, &crc));
> +}
> +
> +/*
> + * 1. set primary plane to full red
> + * 2. grab a reference crc
> + * 3. set primary plane to full green
> + * 4. wait for vblank
> + * 5. pan primary plane a bit (to cause a mmio flip w/o vblank wait)
> + * 6. queue lots of GPU activity to delay the subsequent page flip
> + * 6. queue a page flip to a blue fb
> + * 7. set primary plane to red fb
> + * 8. wait for GPU to finish
> + * 9. compare current crc with reference crc
> + *
> + * We expect the primary plane to display full red at the end.
> + * If the previously schedule primary plane pan operation has interfered
> + * with the following page flip, the driver may have mistakenly completed
> + * the flip before it was executed by the CS, and hence the subsequent
> mmio
> + * flips may have overtaken it. So once we've finished everything
> + * the CS flip may have been the last thing to occur, which means
> + * the primary plane may be full blue instead of the red it's
> + * supposed to be.
> + */
> +static void
> +test_crtc(data_t *data, igt_output_t *output, enum pipe pipe)
> +{
> +       struct igt_fb red_fb, green_fb, blue_fb;
> +       drmModeModeInfo *mode;
> +       igt_plane_t *primary;
> +       igt_crc_t ref_crc, crc;
> +       int ret;
> +
> +       igt_output_set_pipe(output, pipe);
> +       igt_display_commit(&data->display);
> +
> +       if (!output->valid) {
> +               igt_output_set_pipe(output, PIPE_ANY);
> +               igt_display_commit(&data->display);
> +               return;
> +       }
> +
> +       primary = igt_output_get_plane(output, 0);
> +
> +       mode = igt_output_get_mode(output);
> +       igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay+1,
> +                           DRM_FORMAT_XRGB8888,
> +                           false, /* tiled */
> +                           1.0, 0.0, 0.0,
> +                           &red_fb);
> +       igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay+1,
> +                           DRM_FORMAT_XRGB8888,
> +                           false, /* tiled */
> +                           0.0, 0.0, 1.0,
> +                           &blue_fb);
> +       igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay+1,
> +                           DRM_FORMAT_XRGB8888,
> +                           false, /* tiled */
> +                           0.0, 1.0, 0.0,
> +                           &green_fb);
> +
> +       /*
> +        * Make sure these buffers are suited for display use
> +        * because most of the modeset operations must be fast
> +        * later on.
> +        */
> +       igt_plane_set_fb(primary, &green_fb);
> +       igt_display_commit(&data->display);
> +       igt_plane_set_fb(primary, &blue_fb);
> +       igt_display_commit(&data->display);
> +
> +       if (data->pipe_crc)
> +               igt_pipe_crc_free(data->pipe_crc);
> +       data->pipe_crc = igt_pipe_crc_new(pipe,
> INTEL_PIPE_CRC_SOURCE_AUTO);
> +
> +       /* set red fb and grab reference crc */
> +       igt_plane_set_fb(primary, &red_fb);
> +       igt_display_commit(&data->display);
> +       igt_pipe_crc_collect_crc(data->pipe_crc, &ref_crc);
> +
> +       /*
> +        * Further down we need to issue an mmio flip w/o the kernel
> +        * waiting for vblank. The easiest way is to just pan within
> +        * the same FB. So pan away a bit here, and later we undo this
> +        * with another pan which will result in the desired mmio flip.
> +        */
> +       ret = drmModeSetCrtc(data->drm_fd, output->config.crtc->crtc_id,
> +                            green_fb.fb_id, 0, 1, &output->id, 1,
> +                            mode);
> +       igt_assert(ret == 0);
> +
> +       /*
> +        * Make it more likely that the CS flip has been submitted into the
> +        * ring by the time the mmio flip from the drmModeSetCrtc() below
> +        * completes. The driver will then mistake the flip done interrupt
> +        * from the mmio flip as the flip done interrupt from the CS flip.
> +        */
> +       igt_wait_for_vblank(data->drm_fd, pipe);
> +
> +       /* now issue the mmio flip w/o vblank waits in the kernel, ie. pan
> a bit */
> +       ret = drmModeSetCrtc(data->drm_fd, output->config.crtc->crtc_id,
> +                            green_fb.fb_id, 0, 0, &output->id, 1,
> +                            mode);
> +       igt_assert(ret == 0);
> +
> +       make_gpu_busy(data, blue_fb.gem_handle);
> +
> +       /*
> +        * Submit the CS flip. The commands must be emitted into the ring
> +        * before the mmio flip from the panning operation completes.
> +        */
> +       data->flip_done = false;
> +       ret = drmModePageFlip(data->drm_fd, output->config.crtc->crtc_id,
> +                             blue_fb.fb_id, DRM_MODE_PAGE_FLIP_EVENT,
> data);
> +       igt_assert(ret == 0);
> +
> +       /*
> +        * Set primary plane to red fb. This should wait for the CS flip
> +        * to complete. But if the kernel mistook the flip done interrupt
> +        * from the mmio flip as the flip done from the CS flip, this will
> +        * not wait for anything. And hence the the CS flip will actually
> +        * occur after this mmio flip.
> +        */
> +       ret = drmModeSetCrtc(data->drm_fd, output->config.crtc->crtc_id,
> +                            red_fb.fb_id, 0, 0, &output->id, 1,
> +                            mode);
> +       igt_assert(ret == 0);
> +
> +       /* Make sure the flip has been executed */
> +       wait_for_flip(data, blue_fb.gem_handle);
> +
> +       /* Grab crc and compare with the extected result */
> +       igt_pipe_crc_collect_crc(data->pipe_crc, &crc);
> +
> +       igt_plane_set_fb(primary, NULL);
> +       igt_display_commit(&data->display);
> +
> +       igt_remove_fb(data->drm_fd, &red_fb);
> +       igt_remove_fb(data->drm_fd, &green_fb);
> +       igt_remove_fb(data->drm_fd, &blue_fb);
> +
> +       igt_pipe_crc_free(data->pipe_crc);
> +       data->pipe_crc = NULL;
> +
> +       igt_output_set_pipe(output, PIPE_ANY);
> +       igt_display_commit(&data->display);
> +
> +       igt_assert(igt_crc_equal(&ref_crc, &crc));
> +}
> +
> +static void
> +run_plane_test_for_pipe(data_t *data, enum pipe pipe)
> +{
> +       igt_output_t *output;
> +       enum igt_plane plane = 1; /* testing with one sprite is enough */
> +
> +       igt_require(data->display.pipes[pipe].n_planes > 2);
> +
> +       for_each_connected_output(&data->display, output)
> +               test_plane(data, output, pipe, plane);
> +}
> +
> +static void
> +run_crtc_test_for_pipe(data_t *data, enum pipe pipe)
> +{
> +       igt_output_t *output;
> +
> +       for_each_connected_output(&data->display, output)
> +               test_crtc(data, output, pipe);
> +}
> +
> +static data_t data;
> +
> +igt_main
> +{
> +       int pipe;
> +
> +       igt_skip_on_simulation();
> +
> +       igt_fixture {
> +               data.drm_fd = drm_open_any();
> +
> +               igt_set_vt_graphics_mode();
> +
> +               data.devid = intel_get_drm_devid(data.drm_fd);
> +
> +               igt_require_pipe_crc();
> +               igt_display_init(&data.display, data.drm_fd);
> +
> +               data.bufmgr = drm_intel_bufmgr_gem_init(data.drm_fd, 4096);
> +               igt_assert(data.bufmgr);
> +               drm_intel_bufmgr_gem_enable_reuse(data.bufmgr);
> +
> +               data.busy_bo = drm_intel_bo_alloc(data.bufmgr, "bo",
> +                                                 64*1024*1024, 4096);
> +               gem_set_tiling(data.drm_fd, data.busy_bo->handle, 0, 4096);
> +       }
> +
> +       igt_subtest_f("setplane_vs_cs_flip") {
> +               for (pipe = 0; pipe < data.display.n_pipes; pipe++)
> +                       run_plane_test_for_pipe(&data, pipe);
> +       }
> +
> +       igt_subtest_f("setcrtc_vs_cs_flip") {
> +               for (pipe = 0; pipe < data.display.n_pipes; pipe++)
> +                       run_crtc_test_for_pipe(&data, pipe);
> +       }
> +
> +       igt_fixture {
> +               drm_intel_bo_unreference(data.busy_bo);
> +               drm_intel_bufmgr_destroy(data.bufmgr);
> +               igt_display_fini(&data.display);
> +       }
> +}
> --
> 1.8.5.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[-- Attachment #1.2: Type: text/html, Size: 28309 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH v2 1/5] drm/i915: Fix mmio vs. CS flip race on ILK+

[sourab gupta]

[-- Attachment #1.1: Type: text/plain, Size: 12564 bytes --]

Reviewed the code. Everything looks good, so
Reviewed-by: Sourab Gupta <sourabgupta@gmail.com>


On Wed, May 21, 2014 at 6:09 AM, Rodrigo Vivi <rodrigo.vivi@gmail.com>wrote:

> needs a small rebase but everything looks good for me so
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>
>
> On Tue, Apr 15, 2014 at 11:41 AM, <ville.syrjala@linux.intel.com> wrote:
>
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> Starting from ILK, mmio flips also cause a flip done interrupt to be
>> signalled. This means if we first do a set_base and follow it
>> immediately with the CS flip, we might mistake the flip done interrupt
>> caused by the set_base as the flip done interrupt caused by the CS
>> flip.
>>
>> The hardware has a flip counter which increments every time a mmio or
>> CS flip is issued. It basically counts the number of DSPSURF register
>> writes. This means we can sample the counter before we put the CS
>> flip into the ring, and then when we get a flip done interrupt we can
>> check whether the CS flip has actually performed the surface address
>> update, or if the interrupt was caused by a previous but yet
>> unfinished mmio flip.
>>
>> Even with the flip counter we still have a race condition of the CS flip
>> base address update happens after the mmio flip done interrupt was
>> raised but not yet processed by the driver. When the interrupt is
>> eventually processed, the flip counter will already indicate that the
>> CS flip has been executed, but it would not actually complete until the
>> next start of vblank. We can use the DSPSURFLIVE register to check
>> whether the hardware is actually scanning out of the buffer we expect,
>> or if we managed hit this race window.
>>
>> This covers all the cases where the CS flip actually changes the base
>> address. If the base address remains unchanged, we might still complete
>> the CS flip before it has actually completed. But since the address
>> didn't change anyway, the premature flip completion can't result in
>> userspace overwriting data that's still being scanned out.
>>
>> CTG already has the flip counter and DSPSURFLIVE registers, and
>> although the flip done interrupt is still limited to CS flips alone,
>> the code now also checks the flip counter on CTG as well.
>>
>> v2: s/dspsurf/gtt_offset/ (Chris)
>>
>> Testcase: igt/kms_mmio_vs_cs_flip/setcrtc_vs_cs_flip
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=73027
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h      |  1 +
>>  drivers/gpu/drm/i915/intel_display.c | 73
>> ++++++++++++++++++++++++++++++++----
>>  drivers/gpu/drm/i915/intel_drv.h     |  2 +
>>  3 files changed, 69 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h
>> index 8f84555..c28169c 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -3638,6 +3638,7 @@ enum punit_power_well {
>>  #define _PIPEA_FRMCOUNT_GM45   0x70040
>>  #define _PIPEA_FLIPCOUNT_GM45  0x70044
>>  #define PIPE_FRMCOUNT_GM45(pipe) _PIPE2(pipe, _PIPEA_FRMCOUNT_GM45)
>> +#define PIPE_FLIPCOUNT_GM45(pipe) _PIPE2(pipe, _PIPEA_FLIPCOUNT_GM45)
>>
>>  /* Cursor A & B regs */
>>  #define _CURACNTR              (dev_priv->info.display_mmio_offset +
>> 0x70080)
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 1390ab5..32c6c16 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -8577,6 +8577,48 @@ void intel_finish_page_flip_plane(struct
>> drm_device *dev, int plane)
>>         do_intel_finish_page_flip(dev, crtc);
>>  }
>>
>> +/* Is 'a' after or equal to 'b'? */
>> +static bool flip_count_after_eq(u32 a, u32 b)
>> +{
>> +       return !((a - b) & 0x80000000);
>> +}
>> +
>> +static bool page_flip_finished(struct intel_crtc *crtc)
>> +{
>> +       struct drm_device *dev = crtc->base.dev;
>> +       struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +       /*
>> +        * The relevant registers doen't exist on pre-ctg.
>> +        * As the flip done interrupt doesn't trigger for mmio
>> +        * flips on gmch platforms, a flip count check isn't
>> +        * really needed there. But since ctg has the registers,
>> +        * include it in the check anyway.
>> +        */
>> +       if (INTEL_INFO(dev)->gen < 5 && !IS_G4X(dev))
>> +               return true;
>> +
>> +       /*
>> +        * A DSPSURFLIVE check isn't enough in case the mmio and CS flips
>> +        * used the same base address. In that case the mmio flip might
>> +        * have completed, but the CS hasn't even executed the flip yet.
>> +        *
>> +        * A flip count check isn't enough as the CS might have updated
>> +        * the base address just after start of vblank, but before we
>> +        * managed to process the interrupt. This means we'd complete the
>> +        * CS flip too soon.
>> +        *
>> +        * Combining both checks should get us a good enough result. It
>> may
>> +        * still happen that the CS flip has been executed, but has not
>> +        * yet actually completed. But in case the base address is the
>> same
>> +        * anyway, we don't really care.
>> +        */
>> +       return (I915_READ(DSPSURFLIVE(crtc->plane)) & ~0xfff) ==
>> +               crtc->unpin_work->gtt_offset &&
>> +
>> flip_count_after_eq(I915_READ(PIPE_FLIPCOUNT_GM45(crtc->pipe)),
>> +                                   crtc->unpin_work->flip_count);
>> +}
>> +
>>  void intel_prepare_page_flip(struct drm_device *dev, int plane)
>>  {
>>         struct drm_i915_private *dev_priv = dev->dev_private;
>> @@ -8589,7 +8631,7 @@ void intel_prepare_page_flip(struct drm_device
>> *dev, int plane)
>>          * is also accompanied by a spurious intel_prepare_page_flip().
>>          */
>>         spin_lock_irqsave(&dev->event_lock, flags);
>> -       if (intel_crtc->unpin_work)
>> +       if (intel_crtc->unpin_work && page_flip_finished(intel_crtc))
>>                 atomic_inc_not_zero(&intel_crtc->unpin_work->pending);
>>         spin_unlock_irqrestore(&dev->event_lock, flags);
>>  }
>> @@ -8619,6 +8661,9 @@ static int intel_gen2_queue_flip(struct drm_device
>> *dev,
>>         if (ret)
>>                 goto err;
>>
>> +       intel_crtc->unpin_work->gtt_offset =
>> +               i915_gem_obj_ggtt_offset(obj) +
>> intel_crtc->dspaddr_offset;
>> +
>>         ret = intel_ring_begin(ring, 6);
>>         if (ret)
>>                 goto err_unpin;
>> @@ -8635,7 +8680,7 @@ static int intel_gen2_queue_flip(struct drm_device
>> *dev,
>>         intel_ring_emit(ring, MI_DISPLAY_FLIP |
>>                         MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
>>         intel_ring_emit(ring, fb->pitches[0]);
>> -       intel_ring_emit(ring, i915_gem_obj_ggtt_offset(obj) +
>> intel_crtc->dspaddr_offset);
>> +       intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset);
>>         intel_ring_emit(ring, 0); /* aux display base address, unused */
>>
>>         intel_mark_page_flip_active(intel_crtc);
>> @@ -8664,6 +8709,9 @@ static int intel_gen3_queue_flip(struct drm_device
>> *dev,
>>         if (ret)
>>                 goto err;
>>
>> +       intel_crtc->unpin_work->gtt_offset =
>> +               i915_gem_obj_ggtt_offset(obj) +
>> intel_crtc->dspaddr_offset;
>> +
>>         ret = intel_ring_begin(ring, 6);
>>         if (ret)
>>                 goto err_unpin;
>> @@ -8677,7 +8725,7 @@ static int intel_gen3_queue_flip(struct drm_device
>> *dev,
>>         intel_ring_emit(ring, MI_DISPLAY_FLIP_I915 |
>>                         MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
>>         intel_ring_emit(ring, fb->pitches[0]);
>> -       intel_ring_emit(ring, i915_gem_obj_ggtt_offset(obj) +
>> intel_crtc->dspaddr_offset);
>> +       intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset);
>>         intel_ring_emit(ring, MI_NOOP);
>>
>>         intel_mark_page_flip_active(intel_crtc);
>> @@ -8706,6 +8754,9 @@ static int intel_gen4_queue_flip(struct drm_device
>> *dev,
>>         if (ret)
>>                 goto err;
>>
>> +       intel_crtc->unpin_work->gtt_offset =
>> +               i915_gem_obj_ggtt_offset(obj) +
>> intel_crtc->dspaddr_offset;
>> +
>>         ret = intel_ring_begin(ring, 4);
>>         if (ret)
>>                 goto err_unpin;
>> @@ -8717,8 +8768,7 @@ static int intel_gen4_queue_flip(struct drm_device
>> *dev,
>>         intel_ring_emit(ring, MI_DISPLAY_FLIP |
>>                         MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
>>         intel_ring_emit(ring, fb->pitches[0]);
>> -       intel_ring_emit(ring,
>> -                       (i915_gem_obj_ggtt_offset(obj) +
>> intel_crtc->dspaddr_offset) |
>> +       intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset |
>>                         obj->tiling_mode);
>>
>>         /* XXX Enabling the panel-fitter across page-flip is so far
>> @@ -8755,6 +8805,9 @@ static int intel_gen6_queue_flip(struct drm_device
>> *dev,
>>         if (ret)
>>                 goto err;
>>
>> +       intel_crtc->unpin_work->gtt_offset =
>> +               i915_gem_obj_ggtt_offset(obj) +
>> intel_crtc->dspaddr_offset;
>> +
>>         ret = intel_ring_begin(ring, 4);
>>         if (ret)
>>                 goto err_unpin;
>> @@ -8762,7 +8815,7 @@ static int intel_gen6_queue_flip(struct drm_device
>> *dev,
>>         intel_ring_emit(ring, MI_DISPLAY_FLIP |
>>                         MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
>>         intel_ring_emit(ring, fb->pitches[0] | obj->tiling_mode);
>> -       intel_ring_emit(ring, i915_gem_obj_ggtt_offset(obj) +
>> intel_crtc->dspaddr_offset);
>> +       intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset);
>>
>>         /* Contrary to the suggestions in the documentation,
>>          * "Enable Panel Fitter" does not seem to be required when page
>> @@ -8804,6 +8857,9 @@ static int intel_gen7_queue_flip(struct drm_device
>> *dev,
>>         if (ret)
>>                 goto err;
>>
>> +       intel_crtc->unpin_work->gtt_offset =
>> +               i915_gem_obj_ggtt_offset(obj) +
>> intel_crtc->dspaddr_offset;
>> +
>>         switch(intel_crtc->plane) {
>>         case PLANE_A:
>>                 plane_bit = MI_DISPLAY_FLIP_IVB_PLANE_A;
>> @@ -8881,7 +8937,7 @@ static int intel_gen7_queue_flip(struct drm_device
>> *dev,
>>
>>         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);
>> +       intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset);
>>         intel_ring_emit(ring, (MI_NOOP));
>>
>>         intel_mark_page_flip_active(intel_crtc);
>> @@ -8979,6 +9035,9 @@ static int intel_crtc_page_flip(struct drm_crtc
>> *crtc,
>>         atomic_inc(&intel_crtc->unpin_work_count);
>>         intel_crtc->reset_counter =
>> atomic_read(&dev_priv->gpu_error.reset_counter);
>>
>> +       if (INTEL_INFO(dev)->gen >= 5 || IS_G4X(dev))
>> +               work->flip_count =
>> I915_READ(PIPE_FLIPCOUNT_GM45(intel_crtc->pipe)) + 1;
>> +
>>         ret = dev_priv->display.queue_flip(dev, crtc, fb, obj,
>> page_flip_flags);
>>         if (ret)
>>                 goto cleanup_pending;
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index c551472..edef34e 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -590,6 +590,8 @@ struct intel_unpin_work {
>>  #define INTEL_FLIP_INACTIVE    0
>>  #define INTEL_FLIP_PENDING     1
>>  #define INTEL_FLIP_COMPLETE    2
>> +       u32 flip_count;
>> +       u32 gtt_offset;
>>         bool enable_stall_check;
>>  };
>>
>> --
>> 1.8.3.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>

[-- Attachment #1.2: Type: text/html, Size: 15306 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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