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

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

[ville.syrjala]

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

Starting from ILK mmio flips can interfere with the detection of CS flip
completion. This is due to the fact that mmio flips also cause the flip done
interrupt to be raised.

This series tries to make sure we don't mistake a flip done interrupt
caused by an mmio flip as the flip done interrupt interrupt for a CS flip.
Well, that's just patch 2 actually and the rest is gravy.

Ville Syrjälä (4):
  drm/i915: Reduce the time we hold struct mutex in
    intel_pipe_set_base()
  drm/i915: Fix mmio vs. CS flip race on ILK+
  drm/i915: Drop the excessive vblank waits from modeset codepaths
  drm/i915: Wait for vblank in hsw_enable_ips()

 drivers/gpu/drm/i915/i915_reg.h      |   1 +
 drivers/gpu/drm/i915/intel_display.c | 102 ++++++++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_drv.h     |   2 +
 drivers/gpu/drm/i915/intel_sprite.c  |   5 +-
 4 files changed, 80 insertions(+), 30 deletions(-)

-- 
1.8.3.2

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

[PATCH 1/4] drm/i915: Reduce the time we hold struct mutex in intel_pipe_set_base()

[ville.syrjala]

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

We don't need to hold struct_mutex all through intel_pipe_set_base(),
just need to hold it while pinning/unpinning the buffers.

So reduce the struct_mutext usage in intel_pipe_set_base() just like we
did for the sprite code in:
 commit 82284b6becdbef6d8cd3fb43e8698510833a5129
 Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
 Date:   Tue Oct 1 18:02:12 2013 +0300

    drm/i915: Reduce the time we hold struct mutex in sprite update_plane code

The FBC and PSR locking is still entirely fubar. That stuff was
previouly done while holding struct_mutex, so leave it there for now.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2bccc68..37bc041 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2477,8 +2477,8 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 	ret = intel_pin_and_fence_fb_obj(dev,
 					 to_intel_framebuffer(fb)->obj,
 					 NULL);
+	mutex_unlock(&dev->struct_mutex);
 	if (ret != 0) {
-		mutex_unlock(&dev->struct_mutex);
 		DRM_ERROR("pin & fence failed\n");
 		return ret;
 	}
@@ -2516,6 +2516,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 
 	ret = dev_priv->display.update_plane(crtc, fb, x, y);
 	if (ret) {
+		mutex_lock(&dev->struct_mutex);
 		intel_unpin_fb_obj(to_intel_framebuffer(fb)->obj);
 		mutex_unlock(&dev->struct_mutex);
 		DRM_ERROR("failed to update base address\n");
@@ -2530,9 +2531,12 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 	if (old_fb) {
 		if (intel_crtc->active && old_fb != fb)
 			intel_wait_for_vblank(dev, intel_crtc->pipe);
+		mutex_lock(&dev->struct_mutex);
 		intel_unpin_fb_obj(to_intel_framebuffer(old_fb)->obj);
+		mutex_unlock(&dev->struct_mutex);
 	}
 
+	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
 	intel_edp_psr_update(dev);
 	mutex_unlock(&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 2/4] 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.

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 | 72 ++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_drv.h     |  2 +
 3 files changed, 68 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 146609a..0d82924 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3533,6 +3533,7 @@ enum punit_power_well {
 #define _PIPEA_FRMCOUNT_GM45	(dev_priv->info.display_mmio_offset + 0x70040)
 #define _PIPEA_FLIPCOUNT_GM45	(dev_priv->info.display_mmio_offset + 0x70044)
 #define PIPE_FRMCOUNT_GM45(pipe) _PIPE(pipe, _PIPEA_FRMCOUNT_GM45, _PIPEB_FRMCOUNT_GM45)
+#define PIPE_FLIPCOUNT_GM45(pipe) _PIPE(pipe, _PIPEA_FLIPCOUNT_GM45, _PIPEB_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 37bc041..aabc90c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8610,6 +8610,47 @@ 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->dspsurf &&
+		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)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
@@ -8622,7 +8663,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);
 }
@@ -8652,6 +8693,9 @@ static int intel_gen2_queue_flip(struct drm_device *dev,
 	if (ret)
 		goto err;
 
+	intel_crtc->unpin_work->dspsurf =
+		i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
+
 	ret = intel_ring_begin(ring, 6);
 	if (ret)
 		goto err_unpin;
@@ -8668,7 +8712,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->dspsurf);
 	intel_ring_emit(ring, 0); /* aux display base address, unused */
 
 	intel_mark_page_flip_active(intel_crtc);
@@ -8697,6 +8741,9 @@ static int intel_gen3_queue_flip(struct drm_device *dev,
 	if (ret)
 		goto err;
 
+	intel_crtc->unpin_work->dspsurf =
+		i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
+
 	ret = intel_ring_begin(ring, 6);
 	if (ret)
 		goto err_unpin;
@@ -8710,7 +8757,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->dspsurf);
 	intel_ring_emit(ring, MI_NOOP);
 
 	intel_mark_page_flip_active(intel_crtc);
@@ -8739,6 +8786,9 @@ static int intel_gen4_queue_flip(struct drm_device *dev,
 	if (ret)
 		goto err;
 
+	intel_crtc->unpin_work->dspsurf =
+		i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
+
 	ret = intel_ring_begin(ring, 4);
 	if (ret)
 		goto err_unpin;
@@ -8750,8 +8800,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->dspsurf |
 			obj->tiling_mode);
 
 	/* XXX Enabling the panel-fitter across page-flip is so far
@@ -8788,6 +8837,9 @@ static int intel_gen6_queue_flip(struct drm_device *dev,
 	if (ret)
 		goto err;
 
+	intel_crtc->unpin_work->dspsurf =
+		i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
+
 	ret = intel_ring_begin(ring, 4);
 	if (ret)
 		goto err_unpin;
@@ -8795,7 +8847,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->dspsurf);
 
 	/* Contrary to the suggestions in the documentation,
 	 * "Enable Panel Fitter" does not seem to be required when page
@@ -8837,6 +8889,9 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
 	if (ret)
 		goto err;
 
+	intel_crtc->unpin_work->dspsurf =
+		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;
@@ -8898,7 +8953,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->dspsurf);
 	intel_ring_emit(ring, (MI_NOOP));
 
 	intel_mark_page_flip_active(intel_crtc);
@@ -8996,6 +9051,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 5360d16..1513f4b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -566,6 +566,8 @@ struct intel_unpin_work {
 #define INTEL_FLIP_INACTIVE	0
 #define INTEL_FLIP_PENDING	1
 #define INTEL_FLIP_COMPLETE	2
+	u32 flip_count;
+	u32 dspsurf;
 	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 3/4] 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 aabc90c..6bfa357 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_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_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)
@@ -3711,16 +3709,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);
 }
 
 /* IPS only exists on ULT machines and is tied to pipe A. */
-- 
1.8.3.2

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

[PATCH 4/4] 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 6bfa357..2bddd41 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3550,17 +3550,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

Re: [PATCH 2/4] drm/i915: Fix mmio vs. CS flip race on ILK+

[Chris Wilson]

On Tue, Mar 11, 2014 at 07:37:34PM +0200, 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.
> 
> 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 | 72 ++++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_drv.h     |  2 +
>  3 files changed, 68 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 146609a..0d82924 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3533,6 +3533,7 @@ enum punit_power_well {
>  #define _PIPEA_FRMCOUNT_GM45	(dev_priv->info.display_mmio_offset + 0x70040)
>  #define _PIPEA_FLIPCOUNT_GM45	(dev_priv->info.display_mmio_offset + 0x70044)
>  #define PIPE_FRMCOUNT_GM45(pipe) _PIPE(pipe, _PIPEA_FRMCOUNT_GM45, _PIPEB_FRMCOUNT_GM45)
> +#define PIPE_FLIPCOUNT_GM45(pipe) _PIPE(pipe, _PIPEA_FLIPCOUNT_GM45, _PIPEB_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 37bc041..aabc90c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8610,6 +8610,47 @@ 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);
> +}

Couldn't we just use the idiom of casting the result to an int, please?

> +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->dspsurf &&
> +		flip_count_after_eq(I915_READ(PIPE_FLIPCOUNT_GM45(crtc->pipe)),
> +				    crtc->unpin_work->flip_count);

Instead of dspsurf, I'd prefer if this was just a more generic gtt_offset
so that it isn't peculiar to gen4+, and can be used for the gen3 stall
checks.

Other than those two nitpicks,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

A follow-on request would be to update the pageflip stall logic, but I
can rebase my patches on to this.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/4] drm/i915: Reduce the time we hold struct mutex in intel_pipe_set_base()

[Chris Wilson]

On Tue, Mar 11, 2014 at 07:37:33PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We don't need to hold struct_mutex all through intel_pipe_set_base(),
> just need to hold it while pinning/unpinning the buffers.
> 
> So reduce the struct_mutext usage in intel_pipe_set_base() just like we
> did for the sprite code in:
>  commit 82284b6becdbef6d8cd3fb43e8698510833a5129
>  Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
>  Date:   Tue Oct 1 18:02:12 2013 +0300
> 
>     drm/i915: Reduce the time we hold struct mutex in sprite update_plane code
> 
> The FBC and PSR locking is still entirely fubar. That stuff was
> previouly done while holding struct_mutex, so leave it there for now.

Yup. I am amazed we enabled FBC when it has known deadlocks...

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

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Chris Wilson]

On Tue, Mar 11, 2014 at 07:37:35PM +0200, 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.

Hmm, could we not add an assert(DSPSURFLIVE ==
intel_crtc->dspsurf)? And maybe start tracking the frame counters?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Chris Wilson]

On Tue, Mar 11, 2014 at 07:37:36PM +0200, 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.

This should be done before removing the wait from
intel_enable_primary_plane() for correctness?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/4] drm/i915: Fix mmio vs. CS flip race on ILK+

[Ville Syrjälä]

On Wed, Mar 12, 2014 at 08:30:08AM +0000, Chris Wilson wrote:
> On Tue, Mar 11, 2014 at 07:37:34PM +0200, 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.
> > 
> > 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 | 72 ++++++++++++++++++++++++++++++++----
> >  drivers/gpu/drm/i915/intel_drv.h     |  2 +
> >  3 files changed, 68 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 146609a..0d82924 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -3533,6 +3533,7 @@ enum punit_power_well {
> >  #define _PIPEA_FRMCOUNT_GM45	(dev_priv->info.display_mmio_offset + 0x70040)
> >  #define _PIPEA_FLIPCOUNT_GM45	(dev_priv->info.display_mmio_offset + 0x70044)
> >  #define PIPE_FRMCOUNT_GM45(pipe) _PIPE(pipe, _PIPEA_FRMCOUNT_GM45, _PIPEB_FRMCOUNT_GM45)
> > +#define PIPE_FLIPCOUNT_GM45(pipe) _PIPE(pipe, _PIPEA_FLIPCOUNT_GM45, _PIPEB_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 37bc041..aabc90c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -8610,6 +8610,47 @@ 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);
> > +}
> 
> Couldn't we just use the idiom of casting the result to an int, please?

That only works for 32bit counters, which these are, but if you look at
my latest watermark series I have a similar function for the vblank
counter comparison where I also need to deal w/ 24bit counters. So in
order to be consistent I used the same approach here.

> 
> > +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->dspsurf &&
> > +		flip_count_after_eq(I915_READ(PIPE_FLIPCOUNT_GM45(crtc->pipe)),
> > +				    crtc->unpin_work->flip_count);
> 
> Instead of dspsurf, I'd prefer if this was just a more generic gtt_offset
> so that it isn't peculiar to gen4+, and can be used for the gen3 stall
> checks.

You'll note that I did fill it in for gen2/3 too. I did cringe at the
name a bit there, but didn't bother changing it to anything else. I
can rename it.

> 
> Other than those two nitpicks,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> A follow-on request would be to update the pageflip stall logic, but I
> can rebase my patches on to this.
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Ville Syrjälä
Intel OTC

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

[Ville Syrjälä]

On Wed, Mar 12, 2014 at 08:36:37AM +0000, Chris Wilson wrote:
> On Tue, Mar 11, 2014 at 07:37:36PM +0200, 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.
> 
> This should be done before removing the wait from
> intel_enable_primary_plane() for correctness?

Yeah reordering the patches would be better. I just realized this
issue after I had already done the other patches and being a bit
lazy I just slapped the fix on top.

-- 
Ville Syrjälä
Intel OTC

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

[Ville Syrjälä]

On Wed, Mar 12, 2014 at 08:35:39AM +0000, Chris Wilson wrote:
> On Tue, Mar 11, 2014 at 07:37:35PM +0200, 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.
> 
> Hmm, could we not add an assert(DSPSURFLIVE ==
> intel_crtc->dspsurf)?

Where would you want the assert?

> And maybe start tracking the frame counters?

Tracking them for what purpose?

-- 
Ville Syrjälä
Intel OTC

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

[Chris Wilson]

On Wed, Mar 12, 2014 at 11:16:59AM +0200, Ville Syrjälä wrote:
> On Wed, Mar 12, 2014 at 08:35:39AM +0000, Chris Wilson wrote:
> > On Tue, Mar 11, 2014 at 07:37:35PM +0200, 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.
> > 
> > Hmm, could we not add an assert(DSPSURFLIVE ==
> > intel_crtc->dspsurf)?
> 
> Where would you want the assert?

assert_plane_is_bound(new_fb) in crtc_enable() and
assert_plane_is_bound(old_fb) in crt_disable(). i.e. add them to our set
of plane/pipe checks through modeset.

Extra paranoia would be to double check dspsurflive against dspsurf
during KMS init.
 
> > And maybe start tracking the frame counters?
> 
> Tracking them for what purpose?

Just general debug, checking modesets trigger an update. Fun stats.
No clear purpose yet, but if we had it exposed in a debugfs file (say
i915_display_info) we are more likely to remember it is available when
required.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/4] drm/i915: Reduce the time we hold struct mutex in intel_pipe_set_base()

[Daniel Vetter]

On Wed, Mar 12, 2014 at 08:32:41AM +0000, Chris Wilson wrote:
> On Tue, Mar 11, 2014 at 07:37:33PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We don't need to hold struct_mutex all through intel_pipe_set_base(),
> > just need to hold it while pinning/unpinning the buffers.
> > 
> > So reduce the struct_mutext usage in intel_pipe_set_base() just like we
> > did for the sprite code in:
> >  commit 82284b6becdbef6d8cd3fb43e8698510833a5129
> >  Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >  Date:   Tue Oct 1 18:02:12 2013 +0300
> > 
> >     drm/i915: Reduce the time we hold struct mutex in sprite update_plane code
> > 
> > The FBC and PSR locking is still entirely fubar. That stuff was
> > previouly done while holding struct_mutex, so leave it there for now.
> 
> Yup. I am amazed we enabled FBC when it has known deadlocks...

Well I occasional decide to retreat from certain fronts to fighther the
bigger war. But I've resolved that any psr/fbc stuff will only be merged
enabled by default now. We might need to postpone the locking fixes past
merging byt psr though I fear ...

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

Merged this one here for now, I'll let you two duke it out over the
remaining patches ;-)

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

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

[Ville Syrjälä]

On Wed, Mar 12, 2014 at 09:25:42AM +0000, Chris Wilson wrote:
> On Wed, Mar 12, 2014 at 11:16:59AM +0200, Ville Syrjälä wrote:
> > On Wed, Mar 12, 2014 at 08:35:39AM +0000, Chris Wilson wrote:
> > > On Tue, Mar 11, 2014 at 07:37:35PM +0200, 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.
> > > 
> > > Hmm, could we not add an assert(DSPSURFLIVE ==
> > > intel_crtc->dspsurf)?
> > 
> > Where would you want the assert?
> 
> assert_plane_is_bound(new_fb) in crtc_enable() and
> assert_plane_is_bound(old_fb) in crt_disable(). i.e. add them to our set
> of plane/pipe checks through modeset.

We can't really add live base address checks to such places w/o adding
extra vblank waits. For example if we first do a set_base w/o waiting for
vblank and then disable the crtc. By the time we get to crtc_disable()
there's no guarantee that the mmio flip from set_base has completed, and
hence the live surface address may still have the old value.

For the atomic page flip stuff the live address can serve as a decent
debug tool to make sure the flip has really completed or not completed
when we expect. IIRC I had such a debug patch in my atomic branch.

-- 
Ville Syrjälä
Intel OTC