[PATCH v2 0/5] drm/i915: Atomic sprites v2

[PATCH v2 0/5] drm/i915: Atomic sprites v2

[ville.syrjala]

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

I dug up my atomic sprite update + primary enable/disable patches, did
a few small updates, and rebased the lot on top of nightly

I left Jesse's r-bs in place since I only made fairly minor changes. The
preempt_check_resched() addition being the only thing of substance. So
maybe someone wants to double check that one.

Ville Syrjälä (5):
  drm/i915: Add intel_get_crtc_scanline()
  drm/i915: Shuffle sprite register writes into a tighter group
  drm/i915: Make sprite updates atomic
  drm/i915: Perform primary enable/disable atomically with sprite
    updates
  drm/i915: Add pipe update trace points

 drivers/gpu/drm/i915/i915_irq.c      |  73 ++++++++++---
 drivers/gpu/drm/i915/i915_trace.h    |  77 ++++++++++++++
 drivers/gpu/drm/i915/intel_display.c |   2 +
 drivers/gpu/drm/i915/intel_drv.h     |   4 +
 drivers/gpu/drm/i915/intel_sprite.c  | 196 ++++++++++++++++++++++++++---------
 5 files changed, 288 insertions(+), 64 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: Add intel_get_crtc_scanline()

[ville.syrjala]

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

Refactor the i915_get_crtc_scanoutpos() code a bit to make the
"negative values during vblank" adjustment optional. For most uses
the raw scanline number without such adjustments is easier to use.

v2: Rebase after vblank timestamp changes.
    Use intel_ prefix instead of i915_ as is more customary for
    display related functions.
    Include DRM_SCANOUTPOS_INVBL in the return value even w/o
    adjustments, for a bit of extra consistency.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c  | 46 +++++++++++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 2 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 6e1c2ff..da761a6a 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -675,7 +675,9 @@ static bool intel_pipe_in_vblank_locked(struct drm_device *dev, enum pipe pipe)
 }
 
 static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
-			     int *vpos, int *hpos, ktime_t *stime, ktime_t *etime)
+				    int *vpos, int *hpos,
+				    ktime_t *stime, ktime_t *etime,
+				    bool adjust)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
@@ -756,16 +758,18 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 
 	in_vbl = position >= vbl_start && position < vbl_end;
 
-	/*
-	 * While in vblank, position will be negative
-	 * counting up towards 0 at vbl_end. And outside
-	 * vblank, position will be positive counting
-	 * up since vbl_end.
-	 */
-	if (position >= vbl_start)
-		position -= vbl_end;
-	else
-		position += vtotal - vbl_end;
+	if (adjust) {
+		/*
+		 * While in vblank, position will be negative
+		 * counting up towards 0 at vbl_end. And outside
+		 * vblank, position will be positive counting
+		 * up since vbl_end.
+		 */
+		if (position >= vbl_start)
+			position -= vbl_end;
+		else
+			position += vtotal - vbl_end;
+	}
 
 	if (IS_GEN2(dev) || IS_G4X(dev) || INTEL_INFO(dev)->gen >= 5) {
 		*vpos = position;
@@ -782,6 +786,24 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 	return ret;
 }
 
+static int i915_get_scanout_position(struct drm_device *dev, int pipe,
+				     int *vpos, int *hpos,
+				     ktime_t *stime, ktime_t *etime)
+{
+	return i915_get_crtc_scanoutpos(dev, pipe, vpos, hpos,
+					stime, etime, true);
+}
+
+int intel_get_crtc_scanline(struct drm_crtc *crtc)
+{
+	int vpos = 0, hpos = 0;
+
+	i915_get_crtc_scanoutpos(crtc->dev, to_intel_crtc(crtc)->pipe,
+				 &vpos, &hpos, NULL, NULL, false);
+
+	return vpos;
+}
+
 static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
 			      int *max_error,
 			      struct timeval *vblank_time,
@@ -3805,7 +3827,7 @@ void intel_irq_init(struct drm_device *dev)
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp;
-		dev->driver->get_scanout_position = i915_get_crtc_scanoutpos;
+		dev->driver->get_scanout_position = i915_get_scanout_position;
 	}
 
 	if (IS_VALLEYVIEW(dev)) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 2b72b1d..6df4b69 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -599,6 +599,7 @@ void snb_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 void snb_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 void hsw_pc8_disable_interrupts(struct drm_device *dev);
 void hsw_pc8_restore_interrupts(struct drm_device *dev);
+int intel_get_crtc_scanline(struct drm_crtc *crtc);
 
 
 /* intel_crt.c */
-- 
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: Shuffle sprite register writes into a tighter group

[ville.syrjala]

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

Group the sprite register writes a bit tighter. We want to write
the registers atomically, and so doing the base address/offset
artihmetic within the critical section is pointless when it can
all be done beforehand.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index fe4de89..ed9fa7c 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -124,9 +124,6 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	crtc_w--;
 	crtc_h--;
 
-	I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
-	I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
-
 	linear_offset = y * fb->pitches[0] + x * pixel_size;
 	sprsurf_offset = intel_gen4_compute_page_offset(&x, &y,
 							obj->tiling_mode,
@@ -134,6 +131,9 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 							fb->pitches[0]);
 	linear_offset -= sprsurf_offset;
 
+	I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
+	I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
+
 	if (obj->tiling_mode != I915_TILING_NONE)
 		I915_WRITE(SPTILEOFF(pipe, plane), (y << 16) | x);
 	else
@@ -293,15 +293,15 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (crtc_w != src_w || crtc_h != src_h)
 		sprscale = SPRITE_SCALE_ENABLE | (src_w << 16) | src_h;
 
-	I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
-	I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
-
 	linear_offset = y * fb->pitches[0] + x * pixel_size;
 	sprsurf_offset =
 		intel_gen4_compute_page_offset(&x, &y, obj->tiling_mode,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= sprsurf_offset;
 
+	I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
+	I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
+
 	/* HSW consolidates SPRTILEOFF and SPRLINOFF into a single SPROFFSET
 	 * register */
 	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
@@ -472,15 +472,15 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (crtc_w != src_w || crtc_h != src_h)
 		dvsscale = DVS_SCALE_ENABLE | (src_w << 16) | src_h;
 
-	I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
-	I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
-
 	linear_offset = y * fb->pitches[0] + x * pixel_size;
 	dvssurf_offset =
 		intel_gen4_compute_page_offset(&x, &y, obj->tiling_mode,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= dvssurf_offset;
 
+	I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
+	I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
+
 	if (obj->tiling_mode != I915_TILING_NONE)
 		I915_WRITE(DVSTILEOFF(pipe), (y << 16) | x);
 	else
-- 
1.8.3.2

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

[PATCH v2 3/5] drm/i915: Make sprite updates atomic

[ville.syrjala]

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

Add a mechanism by which we can evade the leading edge of vblank. This
guarantees that no two sprite register writes will straddle on either
side of the vblank start, and that means all the writes will be latched
together in one atomic operation.

We do the vblank evade by checking the scanline counter, and if it's too
close to the start of vblank (too close has been hardcoded to 100usec
for now), we will wait for the vblank start to pass. In order to
eliminate random delayes from the rest of the system, we operate with
interrupts disabled, except when waiting for the vblank obviously.

v2: preempt_check_resched() calls after local_irq_enable() (Jesse)
    Hook up the vblank irq stuff on BDW as well

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c      | 29 ++++++++++++--
 drivers/gpu/drm/i915/intel_display.c |  2 +
 drivers/gpu/drm/i915/intel_drv.h     |  3 ++
 drivers/gpu/drm/i915/intel_sprite.c  | 76 ++++++++++++++++++++++++++++++++++++
 4 files changed, 106 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index da761a6a..888fb45 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1434,6 +1434,15 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
 	}
 }
 
+static void intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
+{
+	struct intel_crtc *intel_crtc =
+		to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
+
+	intel_crtc->vbl_received = true;
+	wake_up(&intel_crtc->vbl_wait);
+}
+
 static irqreturn_t valleyview_irq_handler(int irq, void *arg)
 {
 	struct drm_device *dev = (struct drm_device *) arg;
@@ -1476,8 +1485,10 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
 		spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
 		for_each_pipe(pipe) {
-			if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
+			if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS) {
 				drm_handle_vblank(dev, pipe);
+				intel_pipe_handle_vblank(dev, pipe);
+			}
 
 			if (pipe_stats[pipe] & PLANE_FLIPDONE_INT_STATUS_VLV) {
 				intel_prepare_page_flip(dev, pipe);
@@ -1676,8 +1687,10 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
 		DRM_ERROR("Poison interrupt\n");
 
 	for_each_pipe(pipe) {
-		if (de_iir & DE_PIPE_VBLANK(pipe))
+		if (de_iir & DE_PIPE_VBLANK(pipe)) {
 			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
+		}
 
 		if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe))
 			if (intel_set_cpu_fifo_underrun_reporting(dev, pipe, false))
@@ -1726,8 +1739,10 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
 		intel_opregion_asle_intr(dev);
 
 	for_each_pipe(i) {
-		if (de_iir & (DE_PIPE_VBLANK_IVB(i)))
+		if (de_iir & (DE_PIPE_VBLANK_IVB(i))) {
 			drm_handle_vblank(dev, i);
+			intel_pipe_handle_vblank(dev, i);
+		}
 
 		/* plane/pipes map 1:1 on ilk+ */
 		if (de_iir & DE_PLANE_FLIP_DONE_IVB(i)) {
@@ -1873,8 +1888,10 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 			continue;
 
 		pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
-		if (pipe_iir & GEN8_PIPE_VBLANK)
+		if (pipe_iir & GEN8_PIPE_VBLANK) {
 			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
+		}
 
 		if (pipe_iir & GEN8_PIPE_FLIP_DONE) {
 			intel_prepare_page_flip(dev, pipe);
@@ -3172,6 +3189,8 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
 	if (!drm_handle_vblank(dev, pipe))
 		return false;
 
+	intel_pipe_handle_vblank(dev, pipe);
+
 	if ((iir & flip_pending) == 0)
 		return false;
 
@@ -3359,6 +3378,8 @@ static bool i915_handle_vblank(struct drm_device *dev,
 	if (!drm_handle_vblank(dev, pipe))
 		return false;
 
+	intel_pipe_handle_vblank(dev, pipe);
+
 	if ((iir & flip_pending) == 0)
 		return false;
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 15f55e8..31d1d4d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10297,6 +10297,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 		intel_crtc->plane = !pipe;
 	}
 
+	init_waitqueue_head(&intel_crtc->vbl_wait);
+
 	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
 	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
 	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 6df4b69..ff97ea4 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -376,6 +376,9 @@ struct intel_crtc {
 		/* watermarks currently being used  */
 		struct intel_pipe_wm active;
 	} wm;
+
+	wait_queue_head_t vbl_wait;
+	bool vbl_received;
 };
 
 struct intel_plane_wm_parameters {
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index ed9fa7c..198a056 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -37,6 +37,58 @@
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 
+static unsigned int usecs_to_scanlines(const struct drm_display_mode *mode, unsigned int usecs)
+{
+	/* paranoia */
+	if (!mode->crtc_htotal)
+		return 1;
+
+	return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal);
+}
+
+static void intel_pipe_update_start(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	const struct drm_display_mode *mode = &intel_crtc->config.adjusted_mode;
+	enum pipe pipe = intel_crtc->pipe;
+	/* FIXME needs to be calibrated sensibly */
+	unsigned int min = mode->crtc_vblank_start - usecs_to_scanlines(mode, 100);
+	unsigned int max = mode->crtc_vblank_start - 1;
+	long timeout = msecs_to_jiffies_timeout(1);
+	unsigned int scanline;
+
+	if (WARN_ON(drm_vblank_get(dev, pipe)))
+		return;
+
+	local_irq_disable();
+
+	intel_crtc->vbl_received = false;
+	scanline = intel_get_crtc_scanline(crtc);
+
+	while (scanline >= min && scanline <= max && timeout > 0) {
+		local_irq_enable();
+		preempt_check_resched();
+
+		timeout = wait_event_timeout(intel_crtc->vbl_wait,
+					     intel_crtc->vbl_received,
+					     timeout);
+
+		local_irq_disable();
+
+		intel_crtc->vbl_received = false;
+		scanline = intel_get_crtc_scanline(crtc);
+	}
+
+	drm_vblank_put(dev, pipe);
+}
+
+static void intel_pipe_update_end(struct drm_crtc *crtc)
+{
+	local_irq_enable();
+	preempt_check_resched();
+}
+
 static void
 vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 		 struct drm_framebuffer *fb,
@@ -131,6 +183,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 							fb->pitches[0]);
 	linear_offset -= sprsurf_offset;
 
+	intel_pipe_update_start(crtc);
+
 	I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
 	I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
 
@@ -144,6 +198,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	I915_MODIFY_DISPBASE(SPSURF(pipe, plane), i915_gem_obj_ggtt_offset(obj) +
 			     sprsurf_offset);
 	POSTING_READ(SPSURF(pipe, plane));
+
+	intel_pipe_update_end(crtc);
 }
 
 static void
@@ -155,12 +211,16 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	int pipe = intel_plane->pipe;
 	int plane = intel_plane->plane;
 
+	intel_pipe_update_start(crtc);
+
 	I915_WRITE(SPCNTR(pipe, plane), I915_READ(SPCNTR(pipe, plane)) &
 		   ~SP_ENABLE);
 	/* Activate double buffered register update */
 	I915_MODIFY_DISPBASE(SPSURF(pipe, plane), 0);
 	POSTING_READ(SPSURF(pipe, plane));
 
+	intel_pipe_update_end(crtc);
+
 	intel_update_sprite_watermarks(dplane, crtc, 0, 0, false, false);
 }
 
@@ -299,6 +359,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= sprsurf_offset;
 
+	intel_pipe_update_start(crtc);
+
 	I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -318,6 +380,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_MODIFY_DISPBASE(SPRSURF(pipe),
 			     i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
 	POSTING_READ(SPRSURF(pipe));
+
+	intel_pipe_update_end(crtc);
 }
 
 static void
@@ -328,6 +392,8 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	int pipe = intel_plane->pipe;
 
+	intel_pipe_update_start(crtc);
+
 	I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE);
 	/* Can't leave the scaler enabled... */
 	if (intel_plane->can_scale)
@@ -336,6 +402,8 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	I915_MODIFY_DISPBASE(SPRSURF(pipe), 0);
 	POSTING_READ(SPRSURF(pipe));
 
+	intel_pipe_update_end(crtc);
+
 	/*
 	 * Avoid underruns when disabling the sprite.
 	 * FIXME remove once watermark updates are done properly.
@@ -478,6 +546,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= dvssurf_offset;
 
+	intel_pipe_update_start(crtc);
+
 	I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -492,6 +562,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_MODIFY_DISPBASE(DVSSURF(pipe),
 			     i915_gem_obj_ggtt_offset(obj) + dvssurf_offset);
 	POSTING_READ(DVSSURF(pipe));
+
+	intel_pipe_update_end(crtc);
 }
 
 static void
@@ -502,6 +574,8 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	int pipe = intel_plane->pipe;
 
+	intel_pipe_update_start(crtc);
+
 	I915_WRITE(DVSCNTR(pipe), I915_READ(DVSCNTR(pipe)) & ~DVS_ENABLE);
 	/* Disable the scaler */
 	I915_WRITE(DVSSCALE(pipe), 0);
@@ -509,6 +583,8 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	I915_MODIFY_DISPBASE(DVSSURF(pipe), 0);
 	POSTING_READ(DVSSURF(pipe));
 
+	intel_pipe_update_end(crtc);
+
 	/*
 	 * Avoid underruns when disabling the sprite.
 	 * FIXME remove once watermark updates are done properly.
-- 
1.8.3.2

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

Re: [PATCH v2 3/5] drm/i915: Make sprite updates atomic

[Daniel Vetter]

On Fri, Jan 17, 2014 at 08:09:04PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Add a mechanism by which we can evade the leading edge of vblank. This
> guarantees that no two sprite register writes will straddle on either
> side of the vblank start, and that means all the writes will be latched
> together in one atomic operation.
> 
> We do the vblank evade by checking the scanline counter, and if it's too
> close to the start of vblank (too close has been hardcoded to 100usec
> for now), we will wait for the vblank start to pass. In order to
> eliminate random delayes from the rest of the system, we operate with
> interrupts disabled, except when waiting for the vblank obviously.
> 
> v2: preempt_check_resched() calls after local_irq_enable() (Jesse)
>     Hook up the vblank irq stuff on BDW as well
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Two nitpicks below. Otherwise I'm ok (and I actually started to pull in
the entire series already).
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_irq.c      | 29 ++++++++++++--
>  drivers/gpu/drm/i915/intel_display.c |  2 +
>  drivers/gpu/drm/i915/intel_drv.h     |  3 ++
>  drivers/gpu/drm/i915/intel_sprite.c  | 76 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 106 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index da761a6a..888fb45 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1434,6 +1434,15 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
>  	}
>  }
>  
> +static void intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
> +{
> +	struct intel_crtc *intel_crtc =
> +		to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
> +
> +	intel_crtc->vbl_received = true;

vbl_received needs a comment in the header about how the access rules work
and probably some comments about barriers and stuff in the code. You
access this both from process context and irq context without locks, so
the code must come with comments explaining that all the right barriers
and access restrictions (like ACCESS_ONCE) are enforced.

> +	wake_up(&intel_crtc->vbl_wait);
> +}
> +
>  static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>  {
>  	struct drm_device *dev = (struct drm_device *) arg;
> @@ -1476,8 +1485,10 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>  		spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  
>  		for_each_pipe(pipe) {
> -			if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
> +			if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS) {
>  				drm_handle_vblank(dev, pipe);
> +				intel_pipe_handle_vblank(dev, pipe);
> +			}
>  
>  			if (pipe_stats[pipe] & PLANE_FLIPDONE_INT_STATUS_VLV) {
>  				intel_prepare_page_flip(dev, pipe);
> @@ -1676,8 +1687,10 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
>  		DRM_ERROR("Poison interrupt\n");
>  
>  	for_each_pipe(pipe) {
> -		if (de_iir & DE_PIPE_VBLANK(pipe))
> +		if (de_iir & DE_PIPE_VBLANK(pipe)) {
>  			drm_handle_vblank(dev, pipe);
> +			intel_pipe_handle_vblank(dev, pipe);
> +		}
>  
>  		if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe))
>  			if (intel_set_cpu_fifo_underrun_reporting(dev, pipe, false))
> @@ -1726,8 +1739,10 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
>  		intel_opregion_asle_intr(dev);
>  
>  	for_each_pipe(i) {
> -		if (de_iir & (DE_PIPE_VBLANK_IVB(i)))
> +		if (de_iir & (DE_PIPE_VBLANK_IVB(i))) {
>  			drm_handle_vblank(dev, i);
> +			intel_pipe_handle_vblank(dev, i);
> +		}
>  
>  		/* plane/pipes map 1:1 on ilk+ */
>  		if (de_iir & DE_PLANE_FLIP_DONE_IVB(i)) {
> @@ -1873,8 +1888,10 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
>  			continue;
>  
>  		pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
> -		if (pipe_iir & GEN8_PIPE_VBLANK)
> +		if (pipe_iir & GEN8_PIPE_VBLANK) {
>  			drm_handle_vblank(dev, pipe);
> +			intel_pipe_handle_vblank(dev, pipe);
> +		}
>  
>  		if (pipe_iir & GEN8_PIPE_FLIP_DONE) {
>  			intel_prepare_page_flip(dev, pipe);
> @@ -3172,6 +3189,8 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
>  	if (!drm_handle_vblank(dev, pipe))
>  		return false;
>  
> +	intel_pipe_handle_vblank(dev, pipe);
> +
>  	if ((iir & flip_pending) == 0)
>  		return false;
>  
> @@ -3359,6 +3378,8 @@ static bool i915_handle_vblank(struct drm_device *dev,
>  	if (!drm_handle_vblank(dev, pipe))
>  		return false;
>  
> +	intel_pipe_handle_vblank(dev, pipe);
> +
>  	if ((iir & flip_pending) == 0)
>  		return false;
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 15f55e8..31d1d4d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10297,6 +10297,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  		intel_crtc->plane = !pipe;
>  	}
>  
> +	init_waitqueue_head(&intel_crtc->vbl_wait);
> +
>  	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
>  	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
>  	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 6df4b69..ff97ea4 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -376,6 +376,9 @@ struct intel_crtc {
>  		/* watermarks currently being used  */
>  		struct intel_pipe_wm active;
>  	} wm;
> +
> +	wait_queue_head_t vbl_wait;
> +	bool vbl_received;
>  };
>  
>  struct intel_plane_wm_parameters {
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index ed9fa7c..198a056 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -37,6 +37,58 @@
>  #include <drm/i915_drm.h>
>  #include "i915_drv.h"
>  
> +static unsigned int usecs_to_scanlines(const struct drm_display_mode *mode, unsigned int usecs)
> +{
> +	/* paranoia */
> +	if (!mode->crtc_htotal)
> +		return 1;
> +
> +	return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal);
> +}
> +
> +static void intel_pipe_update_start(struct drm_crtc *crtc)

Since this is an internal helper in our driver I prefer struct intel_crtc
*crtc as the parameter. Long term (and we're slowly getting there) this
should lead to tigther code and less downcasting all over the place - the
usual foo and intel_foo duo is a bit annoying ;-)


> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	const struct drm_display_mode *mode = &intel_crtc->config.adjusted_mode;
> +	enum pipe pipe = intel_crtc->pipe;
> +	/* FIXME needs to be calibrated sensibly */
> +	unsigned int min = mode->crtc_vblank_start - usecs_to_scanlines(mode, 100);
> +	unsigned int max = mode->crtc_vblank_start - 1;
> +	long timeout = msecs_to_jiffies_timeout(1);
> +	unsigned int scanline;
> +
> +	if (WARN_ON(drm_vblank_get(dev, pipe)))
> +		return;
> +
> +	local_irq_disable();
> +
> +	intel_crtc->vbl_received = false;
> +	scanline = intel_get_crtc_scanline(crtc);
> +
> +	while (scanline >= min && scanline <= max && timeout > 0) {
> +		local_irq_enable();
> +		preempt_check_resched();
> +
> +		timeout = wait_event_timeout(intel_crtc->vbl_wait,
> +					     intel_crtc->vbl_received,
> +					     timeout);
> +
> +		local_irq_disable();
> +
> +		intel_crtc->vbl_received = false;
> +		scanline = intel_get_crtc_scanline(crtc);
> +	}
> +
> +	drm_vblank_put(dev, pipe);
> +}
> +
> +static void intel_pipe_update_end(struct drm_crtc *crtc)
> +{
> +	local_irq_enable();
> +	preempt_check_resched();
> +}
> +
>  static void
>  vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>  		 struct drm_framebuffer *fb,
> @@ -131,6 +183,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>  							fb->pitches[0]);
>  	linear_offset -= sprsurf_offset;
>  
> +	intel_pipe_update_start(crtc);
> +
>  	I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
>  	I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
>  
> @@ -144,6 +198,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>  	I915_MODIFY_DISPBASE(SPSURF(pipe, plane), i915_gem_obj_ggtt_offset(obj) +
>  			     sprsurf_offset);
>  	POSTING_READ(SPSURF(pipe, plane));
> +
> +	intel_pipe_update_end(crtc);
>  }
>  
>  static void
> @@ -155,12 +211,16 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
>  	int pipe = intel_plane->pipe;
>  	int plane = intel_plane->plane;
>  
> +	intel_pipe_update_start(crtc);
> +
>  	I915_WRITE(SPCNTR(pipe, plane), I915_READ(SPCNTR(pipe, plane)) &
>  		   ~SP_ENABLE);
>  	/* Activate double buffered register update */
>  	I915_MODIFY_DISPBASE(SPSURF(pipe, plane), 0);
>  	POSTING_READ(SPSURF(pipe, plane));
>  
> +	intel_pipe_update_end(crtc);
> +
>  	intel_update_sprite_watermarks(dplane, crtc, 0, 0, false, false);
>  }
>  
> @@ -299,6 +359,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  					       pixel_size, fb->pitches[0]);
>  	linear_offset -= sprsurf_offset;
>  
> +	intel_pipe_update_start(crtc);
> +
>  	I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
>  	I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
>  
> @@ -318,6 +380,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	I915_MODIFY_DISPBASE(SPRSURF(pipe),
>  			     i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
>  	POSTING_READ(SPRSURF(pipe));
> +
> +	intel_pipe_update_end(crtc);
>  }
>  
>  static void
> @@ -328,6 +392,8 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	int pipe = intel_plane->pipe;
>  
> +	intel_pipe_update_start(crtc);
> +
>  	I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE);
>  	/* Can't leave the scaler enabled... */
>  	if (intel_plane->can_scale)
> @@ -336,6 +402,8 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>  	I915_MODIFY_DISPBASE(SPRSURF(pipe), 0);
>  	POSTING_READ(SPRSURF(pipe));
>  
> +	intel_pipe_update_end(crtc);
> +
>  	/*
>  	 * Avoid underruns when disabling the sprite.
>  	 * FIXME remove once watermark updates are done properly.
> @@ -478,6 +546,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  					       pixel_size, fb->pitches[0]);
>  	linear_offset -= dvssurf_offset;
>  
> +	intel_pipe_update_start(crtc);
> +
>  	I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
>  	I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
>  
> @@ -492,6 +562,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	I915_MODIFY_DISPBASE(DVSSURF(pipe),
>  			     i915_gem_obj_ggtt_offset(obj) + dvssurf_offset);
>  	POSTING_READ(DVSSURF(pipe));
> +
> +	intel_pipe_update_end(crtc);
>  }
>  
>  static void
> @@ -502,6 +574,8 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	int pipe = intel_plane->pipe;
>  
> +	intel_pipe_update_start(crtc);
> +
>  	I915_WRITE(DVSCNTR(pipe), I915_READ(DVSCNTR(pipe)) & ~DVS_ENABLE);
>  	/* Disable the scaler */
>  	I915_WRITE(DVSSCALE(pipe), 0);
> @@ -509,6 +583,8 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>  	I915_MODIFY_DISPBASE(DVSSURF(pipe), 0);
>  	POSTING_READ(DVSSURF(pipe));
>  
> +	intel_pipe_update_end(crtc);
> +
>  	/*
>  	 * Avoid underruns when disabling the sprite.
>  	 * FIXME remove once watermark updates are done properly.
> -- 
> 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

Re: [PATCH v2 3/5] drm/i915: Make sprite updates atomic

[Ville Syrjälä]

On Mon, Jan 20, 2014 at 05:23:39PM +0100, Daniel Vetter wrote:
> On Fri, Jan 17, 2014 at 08:09:04PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Add a mechanism by which we can evade the leading edge of vblank. This
> > guarantees that no two sprite register writes will straddle on either
> > side of the vblank start, and that means all the writes will be latched
> > together in one atomic operation.
> > 
> > We do the vblank evade by checking the scanline counter, and if it's too
> > close to the start of vblank (too close has been hardcoded to 100usec
> > for now), we will wait for the vblank start to pass. In order to
> > eliminate random delayes from the rest of the system, we operate with
> > interrupts disabled, except when waiting for the vblank obviously.
> > 
> > v2: preempt_check_resched() calls after local_irq_enable() (Jesse)
> >     Hook up the vblank irq stuff on BDW as well
> > 
> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Two nitpicks below. Otherwise I'm ok (and I actually started to pull in
> the entire series already).
> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c      | 29 ++++++++++++--
> >  drivers/gpu/drm/i915/intel_display.c |  2 +
> >  drivers/gpu/drm/i915/intel_drv.h     |  3 ++
> >  drivers/gpu/drm/i915/intel_sprite.c  | 76 ++++++++++++++++++++++++++++++++++++
> >  4 files changed, 106 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index da761a6a..888fb45 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1434,6 +1434,15 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
> >  	}
> >  }
> >  
> > +static void intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
> > +{
> > +	struct intel_crtc *intel_crtc =
> > +		to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
> > +
> > +	intel_crtc->vbl_received = true;
> 
> vbl_received needs a comment in the header about how the access rules work
> and probably some comments about barriers and stuff in the code. You
> access this both from process context and irq context without locks, so
> the code must come with comments explaining that all the right barriers
> and access restrictions (like ACCESS_ONCE) are enforced.

I can add a note saying something about wake_up() and wait_event()
implying memory barriers.

> 
> > +	wake_up(&intel_crtc->vbl_wait);
> > +}
> > +
> >  static irqreturn_t valleyview_irq_handler(int irq, void *arg)
> >  {
> >  	struct drm_device *dev = (struct drm_device *) arg;
> > @@ -1476,8 +1485,10 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
> >  		spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> >  
> >  		for_each_pipe(pipe) {
> > -			if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
> > +			if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS) {
> >  				drm_handle_vblank(dev, pipe);
> > +				intel_pipe_handle_vblank(dev, pipe);
> > +			}
> >  
> >  			if (pipe_stats[pipe] & PLANE_FLIPDONE_INT_STATUS_VLV) {
> >  				intel_prepare_page_flip(dev, pipe);
> > @@ -1676,8 +1687,10 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
> >  		DRM_ERROR("Poison interrupt\n");
> >  
> >  	for_each_pipe(pipe) {
> > -		if (de_iir & DE_PIPE_VBLANK(pipe))
> > +		if (de_iir & DE_PIPE_VBLANK(pipe)) {
> >  			drm_handle_vblank(dev, pipe);
> > +			intel_pipe_handle_vblank(dev, pipe);
> > +		}
> >  
> >  		if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe))
> >  			if (intel_set_cpu_fifo_underrun_reporting(dev, pipe, false))
> > @@ -1726,8 +1739,10 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
> >  		intel_opregion_asle_intr(dev);
> >  
> >  	for_each_pipe(i) {
> > -		if (de_iir & (DE_PIPE_VBLANK_IVB(i)))
> > +		if (de_iir & (DE_PIPE_VBLANK_IVB(i))) {
> >  			drm_handle_vblank(dev, i);
> > +			intel_pipe_handle_vblank(dev, i);
> > +		}
> >  
> >  		/* plane/pipes map 1:1 on ilk+ */
> >  		if (de_iir & DE_PLANE_FLIP_DONE_IVB(i)) {
> > @@ -1873,8 +1888,10 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
> >  			continue;
> >  
> >  		pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
> > -		if (pipe_iir & GEN8_PIPE_VBLANK)
> > +		if (pipe_iir & GEN8_PIPE_VBLANK) {
> >  			drm_handle_vblank(dev, pipe);
> > +			intel_pipe_handle_vblank(dev, pipe);
> > +		}
> >  
> >  		if (pipe_iir & GEN8_PIPE_FLIP_DONE) {
> >  			intel_prepare_page_flip(dev, pipe);
> > @@ -3172,6 +3189,8 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
> >  	if (!drm_handle_vblank(dev, pipe))
> >  		return false;
> >  
> > +	intel_pipe_handle_vblank(dev, pipe);
> > +
> >  	if ((iir & flip_pending) == 0)
> >  		return false;
> >  
> > @@ -3359,6 +3378,8 @@ static bool i915_handle_vblank(struct drm_device *dev,
> >  	if (!drm_handle_vblank(dev, pipe))
> >  		return false;
> >  
> > +	intel_pipe_handle_vblank(dev, pipe);
> > +
> >  	if ((iir & flip_pending) == 0)
> >  		return false;
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 15f55e8..31d1d4d 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -10297,6 +10297,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> >  		intel_crtc->plane = !pipe;
> >  	}
> >  
> > +	init_waitqueue_head(&intel_crtc->vbl_wait);
> > +
> >  	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
> >  	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
> >  	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 6df4b69..ff97ea4 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -376,6 +376,9 @@ struct intel_crtc {
> >  		/* watermarks currently being used  */
> >  		struct intel_pipe_wm active;
> >  	} wm;
> > +
> > +	wait_queue_head_t vbl_wait;
> > +	bool vbl_received;
> >  };
> >  
> >  struct intel_plane_wm_parameters {
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index ed9fa7c..198a056 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -37,6 +37,58 @@
> >  #include <drm/i915_drm.h>
> >  #include "i915_drv.h"
> >  
> > +static unsigned int usecs_to_scanlines(const struct drm_display_mode *mode, unsigned int usecs)
> > +{
> > +	/* paranoia */
> > +	if (!mode->crtc_htotal)
> > +		return 1;
> > +
> > +	return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal);
> > +}
> > +
> > +static void intel_pipe_update_start(struct drm_crtc *crtc)
> 
> Since this is an internal helper in our driver I prefer struct intel_crtc
> *crtc as the parameter. Long term (and we're slowly getting there) this
> should lead to tigther code and less downcasting all over the place - the
> usual foo and intel_foo duo is a bit annoying ;-)

I can change it.

> 
> 
> > +{
> > +	struct drm_device *dev = crtc->dev;
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +	const struct drm_display_mode *mode = &intel_crtc->config.adjusted_mode;
> > +	enum pipe pipe = intel_crtc->pipe;
> > +	/* FIXME needs to be calibrated sensibly */
> > +	unsigned int min = mode->crtc_vblank_start - usecs_to_scanlines(mode, 100);
> > +	unsigned int max = mode->crtc_vblank_start - 1;
> > +	long timeout = msecs_to_jiffies_timeout(1);
> > +	unsigned int scanline;
> > +
> > +	if (WARN_ON(drm_vblank_get(dev, pipe)))
> > +		return;
> > +
> > +	local_irq_disable();
> > +
> > +	intel_crtc->vbl_received = false;

Now that you got me thinking about barriers again, I wonder if I should
add an explicit compiler barrier here. The intel_get_crtc_scanline() call
should act as a compiler barrier though, so it shouldn't be needed. So
maybe I should add a comment here too?

> > +	scanline = intel_get_crtc_scanline(crtc);
> > +
> > +	while (scanline >= min && scanline <= max && timeout > 0) {
> > +		local_irq_enable();
> > +		preempt_check_resched();
> > +
> > +		timeout = wait_event_timeout(intel_crtc->vbl_wait,
> > +					     intel_crtc->vbl_received,
> > +					     timeout);
> > +
> > +		local_irq_disable();
> > +
> > +		intel_crtc->vbl_received = false;
> > +		scanline = intel_get_crtc_scanline(crtc);
> > +	}
> > +
> > +	drm_vblank_put(dev, pipe);
> > +}
> > +
> > +static void intel_pipe_update_end(struct drm_crtc *crtc)
> > +{
> > +	local_irq_enable();
> > +	preempt_check_resched();
> > +}
> > +
> >  static void
> >  vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> >  		 struct drm_framebuffer *fb,
> > @@ -131,6 +183,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> >  							fb->pitches[0]);
> >  	linear_offset -= sprsurf_offset;
> >  
> > +	intel_pipe_update_start(crtc);
> > +
> >  	I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
> >  	I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
> >  
> > @@ -144,6 +198,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> >  	I915_MODIFY_DISPBASE(SPSURF(pipe, plane), i915_gem_obj_ggtt_offset(obj) +
> >  			     sprsurf_offset);
> >  	POSTING_READ(SPSURF(pipe, plane));
> > +
> > +	intel_pipe_update_end(crtc);
> >  }
> >  
> >  static void
> > @@ -155,12 +211,16 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
> >  	int pipe = intel_plane->pipe;
> >  	int plane = intel_plane->plane;
> >  
> > +	intel_pipe_update_start(crtc);
> > +
> >  	I915_WRITE(SPCNTR(pipe, plane), I915_READ(SPCNTR(pipe, plane)) &
> >  		   ~SP_ENABLE);
> >  	/* Activate double buffered register update */
> >  	I915_MODIFY_DISPBASE(SPSURF(pipe, plane), 0);
> >  	POSTING_READ(SPSURF(pipe, plane));
> >  
> > +	intel_pipe_update_end(crtc);
> > +
> >  	intel_update_sprite_watermarks(dplane, crtc, 0, 0, false, false);
> >  }
> >  
> > @@ -299,6 +359,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  					       pixel_size, fb->pitches[0]);
> >  	linear_offset -= sprsurf_offset;
> >  
> > +	intel_pipe_update_start(crtc);
> > +
> >  	I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
> >  	I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
> >  
> > @@ -318,6 +380,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  	I915_MODIFY_DISPBASE(SPRSURF(pipe),
> >  			     i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
> >  	POSTING_READ(SPRSURF(pipe));
> > +
> > +	intel_pipe_update_end(crtc);
> >  }
> >  
> >  static void
> > @@ -328,6 +392,8 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
> >  	struct intel_plane *intel_plane = to_intel_plane(plane);
> >  	int pipe = intel_plane->pipe;
> >  
> > +	intel_pipe_update_start(crtc);
> > +
> >  	I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE);
> >  	/* Can't leave the scaler enabled... */
> >  	if (intel_plane->can_scale)
> > @@ -336,6 +402,8 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
> >  	I915_MODIFY_DISPBASE(SPRSURF(pipe), 0);
> >  	POSTING_READ(SPRSURF(pipe));
> >  
> > +	intel_pipe_update_end(crtc);
> > +
> >  	/*
> >  	 * Avoid underruns when disabling the sprite.
> >  	 * FIXME remove once watermark updates are done properly.
> > @@ -478,6 +546,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  					       pixel_size, fb->pitches[0]);
> >  	linear_offset -= dvssurf_offset;
> >  
> > +	intel_pipe_update_start(crtc);
> > +
> >  	I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
> >  	I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
> >  
> > @@ -492,6 +562,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  	I915_MODIFY_DISPBASE(DVSSURF(pipe),
> >  			     i915_gem_obj_ggtt_offset(obj) + dvssurf_offset);
> >  	POSTING_READ(DVSSURF(pipe));
> > +
> > +	intel_pipe_update_end(crtc);
> >  }
> >  
> >  static void
> > @@ -502,6 +574,8 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
> >  	struct intel_plane *intel_plane = to_intel_plane(plane);
> >  	int pipe = intel_plane->pipe;
> >  
> > +	intel_pipe_update_start(crtc);
> > +
> >  	I915_WRITE(DVSCNTR(pipe), I915_READ(DVSCNTR(pipe)) & ~DVS_ENABLE);
> >  	/* Disable the scaler */
> >  	I915_WRITE(DVSSCALE(pipe), 0);
> > @@ -509,6 +583,8 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
> >  	I915_MODIFY_DISPBASE(DVSSURF(pipe), 0);
> >  	POSTING_READ(DVSSURF(pipe));
> >  
> > +	intel_pipe_update_end(crtc);
> > +
> >  	/*
> >  	 * Avoid underruns when disabling the sprite.
> >  	 * FIXME remove once watermark updates are done properly.
> > -- 
> > 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

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v2 3/5] drm/i915: Make sprite updates atomic

[Daniel Vetter]

On Mon, Jan 20, 2014 at 5:56 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>> > +{
>> > +   struct drm_device *dev = crtc->dev;
>> > +   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> > +   const struct drm_display_mode *mode = &intel_crtc->config.adjusted_mode;
>> > +   enum pipe pipe = intel_crtc->pipe;
>> > +   /* FIXME needs to be calibrated sensibly */
>> > +   unsigned int min = mode->crtc_vblank_start - usecs_to_scanlines(mode, 100);
>> > +   unsigned int max = mode->crtc_vblank_start - 1;
>> > +   long timeout = msecs_to_jiffies_timeout(1);
>> > +   unsigned int scanline;
>> > +
>> > +   if (WARN_ON(drm_vblank_get(dev, pipe)))
>> > +           return;
>> > +
>> > +   local_irq_disable();
>> > +
>> > +   intel_crtc->vbl_received = false;
>
> Now that you got me thinking about barriers again, I wonder if I should
> add an explicit compiler barrier here. The intel_get_crtc_scanline() call
> should act as a compiler barrier though, so it shouldn't be needed. So
> maybe I should add a comment here too?

This piece of code here was the actual reason I've asked for barrier
comments ;-) Ofc document the wake_up/wait_even barriers for the irq
write -> read here is also good, but this write here is imo the
crucial piece.

Also I think we should have a check here that the caller is holding
the crtc lock, to make sure that only one thread is using this
facility. Oh, and one more while I ponder this: We enable interrupt
processing before crtcs are fully set up, so chasing the pipe->crtc
mapping from the irq handling either needs to be done carefully (i.e.
a small analysis of why we won't ever get an vblank interrupt before
the crtc is set up) or needs to use something statically allocated in
dev_priv.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v2 3/5] drm/i915: Make sprite updates atomic

[Ville Syrjälä]

On Mon, Jan 20, 2014 at 06:43:42PM +0100, Daniel Vetter wrote:
> On Mon, Jan 20, 2014 at 5:56 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >> > +{
> >> > +   struct drm_device *dev = crtc->dev;
> >> > +   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >> > +   const struct drm_display_mode *mode = &intel_crtc->config.adjusted_mode;
> >> > +   enum pipe pipe = intel_crtc->pipe;
> >> > +   /* FIXME needs to be calibrated sensibly */
> >> > +   unsigned int min = mode->crtc_vblank_start - usecs_to_scanlines(mode, 100);
> >> > +   unsigned int max = mode->crtc_vblank_start - 1;
> >> > +   long timeout = msecs_to_jiffies_timeout(1);
> >> > +   unsigned int scanline;
> >> > +
> >> > +   if (WARN_ON(drm_vblank_get(dev, pipe)))
> >> > +           return;
> >> > +
> >> > +   local_irq_disable();
> >> > +
> >> > +   intel_crtc->vbl_received = false;
> >
> > Now that you got me thinking about barriers again, I wonder if I should
> > add an explicit compiler barrier here. The intel_get_crtc_scanline() call
> > should act as a compiler barrier though, so it shouldn't be needed. So
> > maybe I should add a comment here too?
> 
> This piece of code here was the actual reason I've asked for barrier
> comments ;-) Ofc document the wake_up/wait_even barriers for the irq
> write -> read here is also good, but this write here is imo the
> crucial piece.
> 
> Also I think we should have a check here that the caller is holding
> the crtc lock, to make sure that only one thread is using this
> facility. Oh, and one more while I ponder this: We enable interrupt
> processing before crtcs are fully set up, so chasing the pipe->crtc
> mapping from the irq handling either needs to be done carefully (i.e.
> a small analysis of why we won't ever get an vblank interrupt before
> the crtc is set up) or needs to use something statically allocated in
> dev_priv.

We have the same problem already w/ underrun interrupts at least, no?

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v2 3/5] drm/i915: Make sprite updates atomic

[Daniel Vetter]

On Mon, Jan 20, 2014 at 7:38 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> We have the same problem already w/ underrun interrupts at least, no?

Yeah, but iirc we've hand-waved over the issue with "hopefully the
bios wm setup never underruns while we take over". If it does we're
screwed ;-)

Wrt vblank interrupts I think we're better off since we initially
disable them, and only enable them on-demand. Presuming that our
interrupt setup code is race-free and properly clears out old
interrupts we should be fine here. So I'm ok with just mentioning that
in the commit message.

For the pipe underrun stuff I guess we could to disable the relevant
interrupt sources at driver bring up first, but maybe we can just wait
with that until the first bug report hits us ;-)

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

[PATCH v3 3/5] drm/i915: Make sprite updates atomic

[ville.syrjala]

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

Add a mechanism by which we can evade the leading edge of vblank. This
guarantees that no two sprite register writes will straddle on either
side of the vblank start, and that means all the writes will be latched
together in one atomic operation.

We do the vblank evade by checking the scanline counter, and if it's too
close to the start of vblank (too close has been hardcoded to 100usec
for now), we will wait for the vblank start to pass. In order to
eliminate random delayes from the rest of the system, we operate with
interrupts disabled, except when waiting for the vblank obviously.

Note that we now go digging through pipe_to_crtc_mapping[] in the
vblank interrupt handler, which is a bit dangerous since we set up
interrupts before the crtcs. However in this case since it's the vblank
interrupt, we don't actually unmask it until some piece of code
requests it.

v2: preempt_check_resched() calls after local_irq_enable() (Jesse)
    Hook up the vblank irq stuff on BDW as well
v3: Pass intel_crtc instead of drm_crtc (Daniel)
    Warn if crtc.mutex isn't locked (Daniel)
    Add an explicit compiler barrier and document the barriers (Daniel)
    Note the irq vs. modeset setup madness in the commit message (Daniel)

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c      |  29 ++++++++--
 drivers/gpu/drm/i915/intel_display.c |   2 +
 drivers/gpu/drm/i915/intel_drv.h     |   3 +
 drivers/gpu/drm/i915/intel_sprite.c  | 103 +++++++++++++++++++++++++++++++++++
 4 files changed, 133 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index ffb56a9..8ef9edb 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1439,6 +1439,15 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
 	}
 }
 
+static void intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
+{
+	struct intel_crtc *intel_crtc =
+		to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
+
+	intel_crtc->vbl_received = true;
+	wake_up(&intel_crtc->vbl_wait);
+}
+
 static irqreturn_t valleyview_irq_handler(int irq, void *arg)
 {
 	struct drm_device *dev = (struct drm_device *) arg;
@@ -1479,8 +1488,10 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
 		spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
 		for_each_pipe(pipe) {
-			if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
+			if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS) {
 				drm_handle_vblank(dev, pipe);
+				intel_pipe_handle_vblank(dev, pipe);
+			}
 
 			if (pipe_stats[pipe] & PLANE_FLIPDONE_INT_STATUS_VLV) {
 				intel_prepare_page_flip(dev, pipe);
@@ -1679,8 +1690,10 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
 		DRM_ERROR("Poison interrupt\n");
 
 	for_each_pipe(pipe) {
-		if (de_iir & DE_PIPE_VBLANK(pipe))
+		if (de_iir & DE_PIPE_VBLANK(pipe)) {
 			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
+		}
 
 		if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe))
 			if (intel_set_cpu_fifo_underrun_reporting(dev, pipe, false))
@@ -1729,8 +1742,10 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
 		intel_opregion_asle_intr(dev);
 
 	for_each_pipe(i) {
-		if (de_iir & (DE_PIPE_VBLANK_IVB(i)))
+		if (de_iir & (DE_PIPE_VBLANK_IVB(i))) {
 			drm_handle_vblank(dev, i);
+			intel_pipe_handle_vblank(dev, i);
+		}
 
 		/* plane/pipes map 1:1 on ilk+ */
 		if (de_iir & DE_PLANE_FLIP_DONE_IVB(i)) {
@@ -1872,8 +1887,10 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 			continue;
 
 		pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
-		if (pipe_iir & GEN8_PIPE_VBLANK)
+		if (pipe_iir & GEN8_PIPE_VBLANK) {
 			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
+		}
 
 		if (pipe_iir & GEN8_PIPE_FLIP_DONE) {
 			intel_prepare_page_flip(dev, pipe);
@@ -3161,6 +3178,8 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
 	if (!drm_handle_vblank(dev, pipe))
 		return false;
 
+	intel_pipe_handle_vblank(dev, pipe);
+
 	if ((iir & flip_pending) == 0)
 		return false;
 
@@ -3344,6 +3363,8 @@ static bool i915_handle_vblank(struct drm_device *dev,
 	if (!drm_handle_vblank(dev, pipe))
 		return false;
 
+	intel_pipe_handle_vblank(dev, pipe);
+
 	if ((iir & flip_pending) == 0)
 		return false;
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d1cc7ea..b39e445 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10297,6 +10297,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 		intel_crtc->plane = !pipe;
 	}
 
+	init_waitqueue_head(&intel_crtc->vbl_wait);
+
 	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
 	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
 	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 318bb4c..e539550 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -376,6 +376,9 @@ struct intel_crtc {
 		/* watermarks currently being used  */
 		struct intel_pipe_wm active;
 	} wm;
+
+	wait_queue_head_t vbl_wait;
+	bool vbl_received;
 };
 
 struct intel_plane_wm_parameters {
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index ed9fa7c..1ffa7ba 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -37,6 +37,79 @@
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 
+static unsigned int usecs_to_scanlines(const struct drm_display_mode *mode, unsigned int usecs)
+{
+	/* paranoia */
+	if (!mode->crtc_htotal)
+		return 1;
+
+	return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal);
+}
+
+static void intel_pipe_update_start(struct intel_crtc *crtc)
+{
+	struct drm_device *dev = crtc->base.dev;
+	const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
+	enum pipe pipe = crtc->pipe;
+	/* FIXME needs to be calibrated sensibly */
+	unsigned int min = mode->crtc_vblank_start - usecs_to_scanlines(mode, 100);
+	unsigned int max = mode->crtc_vblank_start - 1;
+	long timeout = msecs_to_jiffies_timeout(1);
+	unsigned int scanline;
+
+	WARN_ON(!mutex_is_locked(&crtc->base.mutex));
+
+	if (WARN_ON(drm_vblank_get(dev, pipe)))
+		return;
+
+	local_irq_disable();
+
+	/*
+	 * Explicit compiler barrier is used to make sure that vbl_received
+	 * store happens before reading the current scanline, otherwise
+	 * something like this this might happen:
+	 *
+	 * 1. scanline = intel_get_crtc_scanline();
+	 * 2. vblank irq -> vbl_received = true
+	 * 3. vbl_received = false
+	 *
+	 * Which could make us miss the fact that we already crossed into
+	 * vblank even though the scanline indicates that we're somewhere
+	 * just before the start of vblank.
+	 *
+	 * wake_up() vs. wait_event_timeout() imply the necessary memory
+	 * barries to make sure wait_event_timeout() sees the correct
+	 * value of vbl_received after waking up.
+	 */
+	crtc->vbl_received = false;
+	barrier();
+	scanline = intel_get_crtc_scanline(&crtc->base);
+
+	while (scanline >= min && scanline <= max && timeout > 0) {
+		local_irq_enable();
+		preempt_check_resched();
+
+		timeout = wait_event_timeout(crtc->vbl_wait,
+					     crtc->vbl_received,
+					     timeout);
+
+		local_irq_disable();
+
+		/* ditto */
+		crtc->vbl_received = false;
+		barrier();
+		scanline = intel_get_crtc_scanline(&crtc->base);
+	}
+
+	drm_vblank_put(dev, pipe);
+}
+
+static void intel_pipe_update_end(struct intel_crtc *crtc)
+{
+	local_irq_enable();
+	preempt_check_resched();
+}
+
 static void
 vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 		 struct drm_framebuffer *fb,
@@ -48,6 +121,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	int plane = intel_plane->plane;
 	u32 sprctl;
@@ -131,6 +205,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 							fb->pitches[0]);
 	linear_offset -= sprsurf_offset;
 
+	intel_pipe_update_start(intel_crtc);
+
 	I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
 	I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
 
@@ -144,6 +220,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	I915_MODIFY_DISPBASE(SPSURF(pipe, plane), i915_gem_obj_ggtt_offset(obj) +
 			     sprsurf_offset);
 	POSTING_READ(SPSURF(pipe, plane));
+
+	intel_pipe_update_end(intel_crtc);
 }
 
 static void
@@ -152,15 +230,20 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	int plane = intel_plane->plane;
 
+	intel_pipe_update_start(intel_crtc);
+
 	I915_WRITE(SPCNTR(pipe, plane), I915_READ(SPCNTR(pipe, plane)) &
 		   ~SP_ENABLE);
 	/* Activate double buffered register update */
 	I915_MODIFY_DISPBASE(SPSURF(pipe, plane), 0);
 	POSTING_READ(SPSURF(pipe, plane));
 
+	intel_pipe_update_end(intel_crtc);
+
 	intel_update_sprite_watermarks(dplane, crtc, 0, 0, false, false);
 }
 
@@ -226,6 +309,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	u32 sprctl, sprscale = 0;
 	unsigned long sprsurf_offset, linear_offset;
@@ -299,6 +383,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= sprsurf_offset;
 
+	intel_pipe_update_start(intel_crtc);
+
 	I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -318,6 +404,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_MODIFY_DISPBASE(SPRSURF(pipe),
 			     i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
 	POSTING_READ(SPRSURF(pipe));
+
+	intel_pipe_update_end(intel_crtc);
 }
 
 static void
@@ -326,8 +414,11 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 
+	intel_pipe_update_start(intel_crtc);
+
 	I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE);
 	/* Can't leave the scaler enabled... */
 	if (intel_plane->can_scale)
@@ -336,6 +427,8 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	I915_MODIFY_DISPBASE(SPRSURF(pipe), 0);
 	POSTING_READ(SPRSURF(pipe));
 
+	intel_pipe_update_end(intel_crtc);
+
 	/*
 	 * Avoid underruns when disabling the sprite.
 	 * FIXME remove once watermark updates are done properly.
@@ -410,6 +503,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	unsigned long dvssurf_offset, linear_offset;
 	u32 dvscntr, dvsscale;
@@ -478,6 +572,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= dvssurf_offset;
 
+	intel_pipe_update_start(intel_crtc);
+
 	I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -492,6 +588,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_MODIFY_DISPBASE(DVSSURF(pipe),
 			     i915_gem_obj_ggtt_offset(obj) + dvssurf_offset);
 	POSTING_READ(DVSSURF(pipe));
+
+	intel_pipe_update_end(intel_crtc);
 }
 
 static void
@@ -500,8 +598,11 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 
+	intel_pipe_update_start(intel_crtc);
+
 	I915_WRITE(DVSCNTR(pipe), I915_READ(DVSCNTR(pipe)) & ~DVS_ENABLE);
 	/* Disable the scaler */
 	I915_WRITE(DVSSCALE(pipe), 0);
@@ -509,6 +610,8 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	I915_MODIFY_DISPBASE(DVSSURF(pipe), 0);
 	POSTING_READ(DVSSURF(pipe));
 
+	intel_pipe_update_end(intel_crtc);
+
 	/*
 	 * Avoid underruns when disabling the sprite.
 	 * FIXME remove once watermark updates are done properly.
-- 
1.8.3.2

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

Re: [PATCH v3 3/5] drm/i915: Make sprite updates atomic

[Daniel Vetter]

On Tue, Jan 21, 2014 at 04:12:38PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Add a mechanism by which we can evade the leading edge of vblank. This
> guarantees that no two sprite register writes will straddle on either
> side of the vblank start, and that means all the writes will be latched
> together in one atomic operation.
>
> We do the vblank evade by checking the scanline counter, and if it's too
> close to the start of vblank (too close has been hardcoded to 100usec
> for now), we will wait for the vblank start to pass. In order to
> eliminate random delayes from the rest of the system, we operate with
> interrupts disabled, except when waiting for the vblank obviously.
>
> Note that we now go digging through pipe_to_crtc_mapping[] in the
> vblank interrupt handler, which is a bit dangerous since we set up
> interrupts before the crtcs. However in this case since it's the vblank
> interrupt, we don't actually unmask it until some piece of code
> requests it.
>
> v2: preempt_check_resched() calls after local_irq_enable() (Jesse)
>     Hook up the vblank irq stuff on BDW as well
> v3: Pass intel_crtc instead of drm_crtc (Daniel)
>     Warn if crtc.mutex isn't locked (Daniel)
>     Add an explicit compiler barrier and document the barriers (Daniel)
>     Note the irq vs. modeset setup madness in the commit message (Daniel)
>
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c      |  29 ++++++++--
>  drivers/gpu/drm/i915/intel_display.c |   2 +
>  drivers/gpu/drm/i915/intel_drv.h     |   3 +
>  drivers/gpu/drm/i915/intel_sprite.c  | 103 +++++++++++++++++++++++++++++++++++
>  4 files changed, 133 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index ffb56a9..8ef9edb 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1439,6 +1439,15 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
>   }
>  }
>  
> +static void intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
> +{
> + struct intel_crtc *intel_crtc =
> + to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
> +
> + intel_crtc->vbl_received = true;
> + wake_up(&intel_crtc->vbl_wait);
> +}
> +
>  static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>  {
>   struct drm_device *dev = (struct drm_device *) arg;
> @@ -1479,8 +1488,10 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>   spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  
>   for_each_pipe(pipe) {
> - if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
> + if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS) {
>   drm_handle_vblank(dev, pipe);
> + intel_pipe_handle_vblank(dev, pipe);
> + }
>  
>   if (pipe_stats[pipe] & PLANE_FLIPDONE_INT_STATUS_VLV) {
>   intel_prepare_page_flip(dev, pipe);
> @@ -1679,8 +1690,10 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
>   DRM_ERROR("Poison interrupt\n");
>  
>   for_each_pipe(pipe) {
> - if (de_iir & DE_PIPE_VBLANK(pipe))
> + if (de_iir & DE_PIPE_VBLANK(pipe)) {
>   drm_handle_vblank(dev, pipe);
> + intel_pipe_handle_vblank(dev, pipe);
> + }
>  
>   if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe))
>   if (intel_set_cpu_fifo_underrun_reporting(dev, pipe, false))
> @@ -1729,8 +1742,10 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
>   intel_opregion_asle_intr(dev);
>  
>   for_each_pipe(i) {
> - if (de_iir & (DE_PIPE_VBLANK_IVB(i)))
> + if (de_iir & (DE_PIPE_VBLANK_IVB(i))) {
>   drm_handle_vblank(dev, i);
> + intel_pipe_handle_vblank(dev, i);
> + }
>  
>   /* plane/pipes map 1:1 on ilk+ */
>   if (de_iir & DE_PLANE_FLIP_DONE_IVB(i)) {
> @@ -1872,8 +1887,10 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
>   continue;
>  
>   pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
> - if (pipe_iir & GEN8_PIPE_VBLANK)
> + if (pipe_iir & GEN8_PIPE_VBLANK) {
>   drm_handle_vblank(dev, pipe);
> + intel_pipe_handle_vblank(dev, pipe);
> + }
>  
>   if (pipe_iir & GEN8_PIPE_FLIP_DONE) {
>   intel_prepare_page_flip(dev, pipe);
> @@ -3161,6 +3178,8 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
>   if (!drm_handle_vblank(dev, pipe))
>   return false;
>  
> + intel_pipe_handle_vblank(dev, pipe);
> +
>   if ((iir & flip_pending) == 0)
>   return false;
>  
> @@ -3344,6 +3363,8 @@ static bool i915_handle_vblank(struct drm_device *dev,
>   if (!drm_handle_vblank(dev, pipe))
>   return false;
>  
> + intel_pipe_handle_vblank(dev, pipe);
> +
>   if ((iir & flip_pending) == 0)
>   return false;
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d1cc7ea..b39e445 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10297,6 +10297,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>   intel_crtc->plane = !pipe;
>   }
>  
> + init_waitqueue_head(&intel_crtc->vbl_wait);
> +
>   BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
>         dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
>   dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 318bb4c..e539550 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -376,6 +376,9 @@ struct intel_crtc {
>   /* watermarks currently being used  */
>   struct intel_pipe_wm active;
>   } wm;
> +
> + wait_queue_head_t vbl_wait;
> + bool vbl_received;
>  };
>  
>  struct intel_plane_wm_parameters {
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index ed9fa7c..1ffa7ba 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -37,6 +37,79 @@
>  #include <drm/i915_drm.h>
>  #include "i915_drv.h"
>  
> +static unsigned int usecs_to_scanlines(const struct drm_display_mode *mode, unsigned int usecs)
> +{
> + /* paranoia */
> + if (!mode->crtc_htotal)
> + return 1;
> +
> + return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal);
> +}
> +
> +static void intel_pipe_update_start(struct intel_crtc *crtc)
> +{
> + struct drm_device *dev = crtc->base.dev;
> + const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
> + enum pipe pipe = crtc->pipe;
> + /* FIXME needs to be calibrated sensibly */
> + unsigned int min = mode->crtc_vblank_start - usecs_to_scanlines(mode, 100);
> + unsigned int max = mode->crtc_vblank_start - 1;
> + long timeout = msecs_to_jiffies_timeout(1);
> + unsigned int scanline;
> +
> + WARN_ON(!mutex_is_locked(&crtc->base.mutex));
> +
> + if (WARN_ON(drm_vblank_get(dev, pipe)))
> + return;
> +
> + local_irq_disable();
> +
> + /*
> + * Explicit compiler barrier is used to make sure that vbl_received
> + * store happens before reading the current scanline, otherwise
> + * something like this this might happen:
> + *
> + * 1. scanline = intel_get_crtc_scanline();
> + * 2. vblank irq -> vbl_received = true
> + * 3. vbl_received = false
> + *
> + * Which could make us miss the fact that we already crossed into
> + * vblank even though the scanline indicates that we're somewhere
> + * just before the start of vblank.
> + *
> + * wake_up() vs. wait_event_timeout() imply the necessary memory
> + * barries to make sure wait_event_timeout() sees the correct
> + * value of vbl_received after waking up.
> + */
> + crtc->vbl_received = false;
> + barrier();
> + scanline = intel_get_crtc_scanline(&crtc->base);

Ok, took a bit longer but here's it. I think we need a bit more polish
with this one. My thoughts about all this:

- Definitely an s/barrier/smp_*/ required. Yeah, on x86 with the total
  store order it's ok, but a big reason for adding barriers and comments
  is documentation. And a big reason for that is to deter people from too
  many lockless tricks ;-)

- You still write from both process context and irq context, which makes
  thinking about anything lockless much harder. I prefer a strict
  consumer/producer relationship for lockless tricks. We can get there by
  tracking the absolute framecounter instead of the vbl_received edge
  signal.

- Finally I don't trust our hw to get the coherency right between all
  this. Especially since the actual vblank interrupt is so neatly
  ill-defined. So I think we should actually base our decisions whether we
  need to block still or whether we've just missed the interrupt on the
  hardware frame counter. Which means we also need a seqlock-like loop to
  grab the scanline:
  1. read hw framecounter
  2. read scanline
  3. read hw framecounter again, if they mismatch go back to 1.

  1-3. could be done with irqs disabled for better behaviour

  Then if we are in the danger scanline zone we can just add 1 to the
  read-out framecounter - since the hw guarantees to update all registers
  atomically it's hopefully save ...

- A better plan on more recent platforms might actually be to read out the
  vblank timestamp register and compare that with the reference clock. If
  it's too near we'll block for the vblank. That way we also address the
  neat Bspec language that the vblank update is done ... somewhere not
  quite specified exactly.

So with my ignorant opinion dumped, what's your stance on all this? ;-)

Cheers, Daniel

> +
> + while (scanline >= min && scanline <= max && timeout > 0) {
> + local_irq_enable();
> + preempt_check_resched();
> +
> + timeout = wait_event_timeout(crtc->vbl_wait,
> +     crtc->vbl_received,
> +     timeout);
> +
> + local_irq_disable();
> +
> + /* ditto */
> + crtc->vbl_received = false;
> + barrier();
> + scanline = intel_get_crtc_scanline(&crtc->base);
> + }
> +
> + drm_vblank_put(dev, pipe);
> +}
> +
> +static void intel_pipe_update_end(struct intel_crtc *crtc)
> +{
> + local_irq_enable();
> + preempt_check_resched();
> +}
> +
>  static void
>  vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>   struct drm_framebuffer *fb,
> @@ -48,6 +121,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>   struct drm_device *dev = dplane->dev;
>   struct drm_i915_private *dev_priv = dev->dev_private;
>   struct intel_plane *intel_plane = to_intel_plane(dplane);
> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   int pipe = intel_plane->pipe;
>   int plane = intel_plane->plane;
>   u32 sprctl;
> @@ -131,6 +205,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>   fb->pitches[0]);
>   linear_offset -= sprsurf_offset;
>  
> + intel_pipe_update_start(intel_crtc);
> +
>   I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
>   I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
>  
> @@ -144,6 +220,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>   I915_MODIFY_DISPBASE(SPSURF(pipe, plane), i915_gem_obj_ggtt_offset(obj) +
>       sprsurf_offset);
>   POSTING_READ(SPSURF(pipe, plane));
> +
> + intel_pipe_update_end(intel_crtc);
>  }
>  
>  static void
> @@ -152,15 +230,20 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
>   struct drm_device *dev = dplane->dev;
>   struct drm_i915_private *dev_priv = dev->dev_private;
>   struct intel_plane *intel_plane = to_intel_plane(dplane);
> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   int pipe = intel_plane->pipe;
>   int plane = intel_plane->plane;
>  
> + intel_pipe_update_start(intel_crtc);
> +
>   I915_WRITE(SPCNTR(pipe, plane), I915_READ(SPCNTR(pipe, plane)) &
>     ~SP_ENABLE);
>   /* Activate double buffered register update */
>   I915_MODIFY_DISPBASE(SPSURF(pipe, plane), 0);
>   POSTING_READ(SPSURF(pipe, plane));
>  
> + intel_pipe_update_end(intel_crtc);
> +
>   intel_update_sprite_watermarks(dplane, crtc, 0, 0, false, false);
>  }
>  
> @@ -226,6 +309,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>   struct drm_device *dev = plane->dev;
>   struct drm_i915_private *dev_priv = dev->dev_private;
>   struct intel_plane *intel_plane = to_intel_plane(plane);
> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   int pipe = intel_plane->pipe;
>   u32 sprctl, sprscale = 0;
>   unsigned long sprsurf_offset, linear_offset;
> @@ -299,6 +383,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>         pixel_size, fb->pitches[0]);
>   linear_offset -= sprsurf_offset;
>  
> + intel_pipe_update_start(intel_crtc);
> +
>   I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
>   I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
>  
> @@ -318,6 +404,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>   I915_MODIFY_DISPBASE(SPRSURF(pipe),
>       i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
>   POSTING_READ(SPRSURF(pipe));
> +
> + intel_pipe_update_end(intel_crtc);
>  }
>  
>  static void
> @@ -326,8 +414,11 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>   struct drm_device *dev = plane->dev;
>   struct drm_i915_private *dev_priv = dev->dev_private;
>   struct intel_plane *intel_plane = to_intel_plane(plane);
> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   int pipe = intel_plane->pipe;
>  
> + intel_pipe_update_start(intel_crtc);
> +
>   I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE);
>   /* Can't leave the scaler enabled... */
>   if (intel_plane->can_scale)
> @@ -336,6 +427,8 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>   I915_MODIFY_DISPBASE(SPRSURF(pipe), 0);
>   POSTING_READ(SPRSURF(pipe));
>  
> + intel_pipe_update_end(intel_crtc);
> +
>   /*
>   * Avoid underruns when disabling the sprite.
>   * FIXME remove once watermark updates are done properly.
> @@ -410,6 +503,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>   struct drm_device *dev = plane->dev;
>   struct drm_i915_private *dev_priv = dev->dev_private;
>   struct intel_plane *intel_plane = to_intel_plane(plane);
> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   int pipe = intel_plane->pipe;
>   unsigned long dvssurf_offset, linear_offset;
>   u32 dvscntr, dvsscale;
> @@ -478,6 +572,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>         pixel_size, fb->pitches[0]);
>   linear_offset -= dvssurf_offset;
>  
> + intel_pipe_update_start(intel_crtc);
> +
>   I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
>   I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
>  
> @@ -492,6 +588,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>   I915_MODIFY_DISPBASE(DVSSURF(pipe),
>       i915_gem_obj_ggtt_offset(obj) + dvssurf_offset);
>   POSTING_READ(DVSSURF(pipe));
> +
> + intel_pipe_update_end(intel_crtc);
>  }
>  
>  static void
> @@ -500,8 +598,11 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>   struct drm_device *dev = plane->dev;
>   struct drm_i915_private *dev_priv = dev->dev_private;
>   struct intel_plane *intel_plane = to_intel_plane(plane);
> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   int pipe = intel_plane->pipe;
>  
> + intel_pipe_update_start(intel_crtc);
> +
>   I915_WRITE(DVSCNTR(pipe), I915_READ(DVSCNTR(pipe)) & ~DVS_ENABLE);
>   /* Disable the scaler */
>   I915_WRITE(DVSSCALE(pipe), 0);
> @@ -509,6 +610,8 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>   I915_MODIFY_DISPBASE(DVSSURF(pipe), 0);
>   POSTING_READ(DVSSURF(pipe));
>  
> + intel_pipe_update_end(intel_crtc);
> +
>   /*
>   * Avoid underruns when disabling the sprite.
>   * FIXME remove once watermark updates are done properly.
> --
> 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

Re: [PATCH v3 3/5] drm/i915: Make sprite updates atomic

[Ville Syrjälä]

On Sun, Jan 26, 2014 at 06:29:06PM +0100, Daniel Vetter wrote:
> On Tue, Jan 21, 2014 at 04:12:38PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Add a mechanism by which we can evade the leading edge of vblank. This
> > guarantees that no two sprite register writes will straddle on either
> > side of the vblank start, and that means all the writes will be latched
> > together in one atomic operation.
> >
> > We do the vblank evade by checking the scanline counter, and if it's too
> > close to the start of vblank (too close has been hardcoded to 100usec
> > for now), we will wait for the vblank start to pass. In order to
> > eliminate random delayes from the rest of the system, we operate with
> > interrupts disabled, except when waiting for the vblank obviously.
> >
> > Note that we now go digging through pipe_to_crtc_mapping[] in the
> > vblank interrupt handler, which is a bit dangerous since we set up
> > interrupts before the crtcs. However in this case since it's the vblank
> > interrupt, we don't actually unmask it until some piece of code
> > requests it.
> >
> > v2: preempt_check_resched() calls after local_irq_enable() (Jesse)
> >     Hook up the vblank irq stuff on BDW as well
> > v3: Pass intel_crtc instead of drm_crtc (Daniel)
> >     Warn if crtc.mutex isn't locked (Daniel)
> >     Add an explicit compiler barrier and document the barriers (Daniel)
> >     Note the irq vs. modeset setup madness in the commit message (Daniel)
> >
> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c      |  29 ++++++++--
> >  drivers/gpu/drm/i915/intel_display.c |   2 +
> >  drivers/gpu/drm/i915/intel_drv.h     |   3 +
> >  drivers/gpu/drm/i915/intel_sprite.c  | 103 +++++++++++++++++++++++++++++++++++
> >  4 files changed, 133 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index ffb56a9..8ef9edb 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1439,6 +1439,15 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
> >   }
> >  }
> >  
> > +static void intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
> > +{
> > + struct intel_crtc *intel_crtc =
> > + to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
> > +
> > + intel_crtc->vbl_received = true;
> > + wake_up(&intel_crtc->vbl_wait);
> > +}
> > +
> >  static irqreturn_t valleyview_irq_handler(int irq, void *arg)
> >  {
> >   struct drm_device *dev = (struct drm_device *) arg;
> > @@ -1479,8 +1488,10 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
> >   spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> >  
> >   for_each_pipe(pipe) {
> > - if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
> > + if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS) {
> >   drm_handle_vblank(dev, pipe);
> > + intel_pipe_handle_vblank(dev, pipe);
> > + }
> >  
> >   if (pipe_stats[pipe] & PLANE_FLIPDONE_INT_STATUS_VLV) {
> >   intel_prepare_page_flip(dev, pipe);
> > @@ -1679,8 +1690,10 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
> >   DRM_ERROR("Poison interrupt\n");
> >  
> >   for_each_pipe(pipe) {
> > - if (de_iir & DE_PIPE_VBLANK(pipe))
> > + if (de_iir & DE_PIPE_VBLANK(pipe)) {
> >   drm_handle_vblank(dev, pipe);
> > + intel_pipe_handle_vblank(dev, pipe);
> > + }
> >  
> >   if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe))
> >   if (intel_set_cpu_fifo_underrun_reporting(dev, pipe, false))
> > @@ -1729,8 +1742,10 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
> >   intel_opregion_asle_intr(dev);
> >  
> >   for_each_pipe(i) {
> > - if (de_iir & (DE_PIPE_VBLANK_IVB(i)))
> > + if (de_iir & (DE_PIPE_VBLANK_IVB(i))) {
> >   drm_handle_vblank(dev, i);
> > + intel_pipe_handle_vblank(dev, i);
> > + }
> >  
> >   /* plane/pipes map 1:1 on ilk+ */
> >   if (de_iir & DE_PLANE_FLIP_DONE_IVB(i)) {
> > @@ -1872,8 +1887,10 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
> >   continue;
> >  
> >   pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
> > - if (pipe_iir & GEN8_PIPE_VBLANK)
> > + if (pipe_iir & GEN8_PIPE_VBLANK) {
> >   drm_handle_vblank(dev, pipe);
> > + intel_pipe_handle_vblank(dev, pipe);
> > + }
> >  
> >   if (pipe_iir & GEN8_PIPE_FLIP_DONE) {
> >   intel_prepare_page_flip(dev, pipe);
> > @@ -3161,6 +3178,8 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
> >   if (!drm_handle_vblank(dev, pipe))
> >   return false;
> >  
> > + intel_pipe_handle_vblank(dev, pipe);
> > +
> >   if ((iir & flip_pending) == 0)
> >   return false;
> >  
> > @@ -3344,6 +3363,8 @@ static bool i915_handle_vblank(struct drm_device *dev,
> >   if (!drm_handle_vblank(dev, pipe))
> >   return false;
> >  
> > + intel_pipe_handle_vblank(dev, pipe);
> > +
> >   if ((iir & flip_pending) == 0)
> >   return false;
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index d1cc7ea..b39e445 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -10297,6 +10297,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> >   intel_crtc->plane = !pipe;
> >   }
> >  
> > + init_waitqueue_head(&intel_crtc->vbl_wait);
> > +
> >   BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
> >         dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
> >   dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 318bb4c..e539550 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -376,6 +376,9 @@ struct intel_crtc {
> >   /* watermarks currently being used  */
> >   struct intel_pipe_wm active;
> >   } wm;
> > +
> > + wait_queue_head_t vbl_wait;
> > + bool vbl_received;
> >  };
> >  
> >  struct intel_plane_wm_parameters {
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index ed9fa7c..1ffa7ba 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -37,6 +37,79 @@
> >  #include <drm/i915_drm.h>
> >  #include "i915_drv.h"
> >  
> > +static unsigned int usecs_to_scanlines(const struct drm_display_mode *mode, unsigned int usecs)
> > +{
> > + /* paranoia */
> > + if (!mode->crtc_htotal)
> > + return 1;
> > +
> > + return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal);
> > +}
> > +
> > +static void intel_pipe_update_start(struct intel_crtc *crtc)
> > +{
> > + struct drm_device *dev = crtc->base.dev;
> > + const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
> > + enum pipe pipe = crtc->pipe;
> > + /* FIXME needs to be calibrated sensibly */
> > + unsigned int min = mode->crtc_vblank_start - usecs_to_scanlines(mode, 100);
> > + unsigned int max = mode->crtc_vblank_start - 1;
> > + long timeout = msecs_to_jiffies_timeout(1);
> > + unsigned int scanline;
> > +
> > + WARN_ON(!mutex_is_locked(&crtc->base.mutex));
> > +
> > + if (WARN_ON(drm_vblank_get(dev, pipe)))
> > + return;
> > +
> > + local_irq_disable();
> > +
> > + /*
> > + * Explicit compiler barrier is used to make sure that vbl_received
> > + * store happens before reading the current scanline, otherwise
> > + * something like this this might happen:
> > + *
> > + * 1. scanline = intel_get_crtc_scanline();
> > + * 2. vblank irq -> vbl_received = true
> > + * 3. vbl_received = false
> > + *
> > + * Which could make us miss the fact that we already crossed into
> > + * vblank even though the scanline indicates that we're somewhere
> > + * just before the start of vblank.
> > + *
> > + * wake_up() vs. wait_event_timeout() imply the necessary memory
> > + * barries to make sure wait_event_timeout() sees the correct
> > + * value of vbl_received after waking up.
> > + */
> > + crtc->vbl_received = false;
> > + barrier();
> > + scanline = intel_get_crtc_scanline(&crtc->base);
> 
> Ok, took a bit longer but here's it. I think we need a bit more polish
> with this one. My thoughts about all this:
> 
> - Definitely an s/barrier/smp_*/ required. Yeah, on x86 with the total
>   store order it's ok, but a big reason for adding barriers and comments
>   is documentation. And a big reason for that is to deter people from too
>   many lockless tricks ;-)

I still don't see why you'd need an smp_ memory barrier here. It's just
one regular store vs. mmio read ordering issue we have here.

If we'd really need to worry about hardware reordering the operations,
then it would seem that we'd need a mandatory memory barrier instead
of an smp one.

> 
> - You still write from both process context and irq context, which makes
>   thinking about anything lockless much harder. I prefer a strict
>   consumer/producer relationship for lockless tricks. We can get there by
>   tracking the absolute framecounter instead of the vbl_received edge
>   signal.
> 
> - Finally I don't trust our hw to get the coherency right between all
>   this. Especially since the actual vblank interrupt is so neatly
>   ill-defined.

My observation is that the vblank ISR behaves very well on PCH platforms.
On earlier platforms it's a bit more unclear whether you get the start
of vblank, or a slightly delayed version. But that should not matter
anyway, since if we end up waiting, the delay is not a problem, and if
we don't end up waiting, well then it doesn't matter when the event
would really trigger. The only concern in the latter case would be where
the hardware would really latch the new register values. Ie. what
happens if the write lands in the short window between the start of
vblank, and when the vblank event triggers.

Hmm. In fact the current code might not do the right thing on platforms
where we have the scanling=vbl-1 issue w/o a working ISR workaround
(gen2, ctg, vlv). That's a bit sad. Not sure if there's any other fix than
to add a slight delay to double check the scanline in that case case :(
ATM it might fall through to the to 1 ms timeout, which should actually
be OK as long as it doesn't get scheculed back in exactly in the danger
zone for the next vblank. Anyway the code is relying on that even in the
case where it decides to wait for the vblank interrupt. Of course it
would be possible to retry until we're guaranteed to be in the clear,
but then there's no guarantee of forward progress, which doesn't sound
very nice. I guess one option would to do a interrupts off polling wait
the second time around to make sure we get it right then.

> So I think we should actually base our decisions whether we
>   need to block still or whether we've just missed the interrupt on the
>   hardware frame counter. Which means we also need a seqlock-like loop to
>   grab the scanline:
>   1. read hw framecounter
>   2. read scanline
>   3. read hw framecounter again, if they mismatch go back to 1.

Or just figure out which side of vblank start we're at. Well, that
might only work on PCH platforms since on CTG we can't know which side
we're at if the scanline counter says vbl_start-1 (due to ISR being
useless). On gen3/4 we already have an atomic read of the pixel counter
with the frame counter (with another kind of seq lock thingy due to
the high+low frame counter split).

> 
>   1-3. could be done with irqs disabled for better behaviour
> 
>   Then if we are in the danger scanline zone we can just add 1 to the
>   read-out framecounter - since the hw guarantees to update all registers
>   atomically it's hopefully save ...

We could use the frame counter sure, except on gen2 where we'd need to
cook up a software frame counter then. Well, basically I'd just have to
replace vbl_received w/ an atomic counter.

> 
> - A better plan on more recent platforms might actually be to read out the
>   vblank timestamp register and compare that with the reference clock. If
>   it's too near we'll block for the vblank. That way we also address the
>   neat Bspec language that the vblank update is done ... somewhere not
>   quite specified exactly.

More codepaths and more bugs. Also as the scanline counter is reliable
on PCH platforms I don't see any benefit at this point. It can be done
later if deemed useful.

> 
> So with my ignorant opinion dumped, what's your stance on all this? ;-)
> 
> Cheers, Daniel
> 
> > +
> > + while (scanline >= min && scanline <= max && timeout > 0) {
> > + local_irq_enable();
> > + preempt_check_resched();
> > +
> > + timeout = wait_event_timeout(crtc->vbl_wait,
> > +     crtc->vbl_received,
> > +     timeout);
> > +
> > + local_irq_disable();
> > +
> > + /* ditto */
> > + crtc->vbl_received = false;
> > + barrier();
> > + scanline = intel_get_crtc_scanline(&crtc->base);
> > + }
> > +
> > + drm_vblank_put(dev, pipe);
> > +}
> > +
> > +static void intel_pipe_update_end(struct intel_crtc *crtc)
> > +{
> > + local_irq_enable();
> > + preempt_check_resched();
> > +}
> > +
> >  static void
> >  vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> >   struct drm_framebuffer *fb,
> > @@ -48,6 +121,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> >   struct drm_device *dev = dplane->dev;
> >   struct drm_i915_private *dev_priv = dev->dev_private;
> >   struct intel_plane *intel_plane = to_intel_plane(dplane);
> > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >   int pipe = intel_plane->pipe;
> >   int plane = intel_plane->plane;
> >   u32 sprctl;
> > @@ -131,6 +205,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> >   fb->pitches[0]);
> >   linear_offset -= sprsurf_offset;
> >  
> > + intel_pipe_update_start(intel_crtc);
> > +
> >   I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
> >   I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
> >  
> > @@ -144,6 +220,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> >   I915_MODIFY_DISPBASE(SPSURF(pipe, plane), i915_gem_obj_ggtt_offset(obj) +
> >       sprsurf_offset);
> >   POSTING_READ(SPSURF(pipe, plane));
> > +
> > + intel_pipe_update_end(intel_crtc);
> >  }
> >  
> >  static void
> > @@ -152,15 +230,20 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
> >   struct drm_device *dev = dplane->dev;
> >   struct drm_i915_private *dev_priv = dev->dev_private;
> >   struct intel_plane *intel_plane = to_intel_plane(dplane);
> > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >   int pipe = intel_plane->pipe;
> >   int plane = intel_plane->plane;
> >  
> > + intel_pipe_update_start(intel_crtc);
> > +
> >   I915_WRITE(SPCNTR(pipe, plane), I915_READ(SPCNTR(pipe, plane)) &
> >     ~SP_ENABLE);
> >   /* Activate double buffered register update */
> >   I915_MODIFY_DISPBASE(SPSURF(pipe, plane), 0);
> >   POSTING_READ(SPSURF(pipe, plane));
> >  
> > + intel_pipe_update_end(intel_crtc);
> > +
> >   intel_update_sprite_watermarks(dplane, crtc, 0, 0, false, false);
> >  }
> >  
> > @@ -226,6 +309,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >   struct drm_device *dev = plane->dev;
> >   struct drm_i915_private *dev_priv = dev->dev_private;
> >   struct intel_plane *intel_plane = to_intel_plane(plane);
> > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >   int pipe = intel_plane->pipe;
> >   u32 sprctl, sprscale = 0;
> >   unsigned long sprsurf_offset, linear_offset;
> > @@ -299,6 +383,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >         pixel_size, fb->pitches[0]);
> >   linear_offset -= sprsurf_offset;
> >  
> > + intel_pipe_update_start(intel_crtc);
> > +
> >   I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
> >   I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
> >  
> > @@ -318,6 +404,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >   I915_MODIFY_DISPBASE(SPRSURF(pipe),
> >       i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
> >   POSTING_READ(SPRSURF(pipe));
> > +
> > + intel_pipe_update_end(intel_crtc);
> >  }
> >  
> >  static void
> > @@ -326,8 +414,11 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
> >   struct drm_device *dev = plane->dev;
> >   struct drm_i915_private *dev_priv = dev->dev_private;
> >   struct intel_plane *intel_plane = to_intel_plane(plane);
> > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >   int pipe = intel_plane->pipe;
> >  
> > + intel_pipe_update_start(intel_crtc);
> > +
> >   I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE);
> >   /* Can't leave the scaler enabled... */
> >   if (intel_plane->can_scale)
> > @@ -336,6 +427,8 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
> >   I915_MODIFY_DISPBASE(SPRSURF(pipe), 0);
> >   POSTING_READ(SPRSURF(pipe));
> >  
> > + intel_pipe_update_end(intel_crtc);
> > +
> >   /*
> >   * Avoid underruns when disabling the sprite.
> >   * FIXME remove once watermark updates are done properly.
> > @@ -410,6 +503,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >   struct drm_device *dev = plane->dev;
> >   struct drm_i915_private *dev_priv = dev->dev_private;
> >   struct intel_plane *intel_plane = to_intel_plane(plane);
> > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >   int pipe = intel_plane->pipe;
> >   unsigned long dvssurf_offset, linear_offset;
> >   u32 dvscntr, dvsscale;
> > @@ -478,6 +572,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >         pixel_size, fb->pitches[0]);
> >   linear_offset -= dvssurf_offset;
> >  
> > + intel_pipe_update_start(intel_crtc);
> > +
> >   I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
> >   I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
> >  
> > @@ -492,6 +588,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >   I915_MODIFY_DISPBASE(DVSSURF(pipe),
> >       i915_gem_obj_ggtt_offset(obj) + dvssurf_offset);
> >   POSTING_READ(DVSSURF(pipe));
> > +
> > + intel_pipe_update_end(intel_crtc);
> >  }
> >  
> >  static void
> > @@ -500,8 +598,11 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
> >   struct drm_device *dev = plane->dev;
> >   struct drm_i915_private *dev_priv = dev->dev_private;
> >   struct intel_plane *intel_plane = to_intel_plane(plane);
> > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >   int pipe = intel_plane->pipe;
> >  
> > + intel_pipe_update_start(intel_crtc);
> > +
> >   I915_WRITE(DVSCNTR(pipe), I915_READ(DVSCNTR(pipe)) & ~DVS_ENABLE);
> >   /* Disable the scaler */
> >   I915_WRITE(DVSSCALE(pipe), 0);
> > @@ -509,6 +610,8 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
> >   I915_MODIFY_DISPBASE(DVSSURF(pipe), 0);
> >   POSTING_READ(DVSSURF(pipe));
> >  
> > + intel_pipe_update_end(intel_crtc);
> > +
> >   /*
> >   * Avoid underruns when disabling the sprite.
> >   * FIXME remove once watermark updates are done properly.
> > --
> > 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

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v3 3/5] drm/i915: Make sprite updates atomic

[Daniel Vetter]

On Mon, Jan 27, 2014 at 01:06:33PM +0200, Ville Syrjälä wrote:
> On Sun, Jan 26, 2014 at 06:29:06PM +0100, Daniel Vetter wrote:
> > On Tue, Jan 21, 2014 at 04:12:38PM +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Add a mechanism by which we can evade the leading edge of vblank. This
> > > guarantees that no two sprite register writes will straddle on either
> > > side of the vblank start, and that means all the writes will be latched
> > > together in one atomic operation.
> > >
> > > We do the vblank evade by checking the scanline counter, and if it's too
> > > close to the start of vblank (too close has been hardcoded to 100usec
> > > for now), we will wait for the vblank start to pass. In order to
> > > eliminate random delayes from the rest of the system, we operate with
> > > interrupts disabled, except when waiting for the vblank obviously.
> > >
> > > Note that we now go digging through pipe_to_crtc_mapping[] in the
> > > vblank interrupt handler, which is a bit dangerous since we set up
> > > interrupts before the crtcs. However in this case since it's the vblank
> > > interrupt, we don't actually unmask it until some piece of code
> > > requests it.
> > >
> > > v2: preempt_check_resched() calls after local_irq_enable() (Jesse)
> > >     Hook up the vblank irq stuff on BDW as well
> > > v3: Pass intel_crtc instead of drm_crtc (Daniel)
> > >     Warn if crtc.mutex isn't locked (Daniel)
> > >     Add an explicit compiler barrier and document the barriers (Daniel)
> > >     Note the irq vs. modeset setup madness in the commit message (Daniel)
> > >
> > > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_irq.c      |  29 ++++++++--
> > >  drivers/gpu/drm/i915/intel_display.c |   2 +
> > >  drivers/gpu/drm/i915/intel_drv.h     |   3 +
> > >  drivers/gpu/drm/i915/intel_sprite.c  | 103 +++++++++++++++++++++++++++++++++++
> > >  4 files changed, 133 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > index ffb56a9..8ef9edb 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -1439,6 +1439,15 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
> > >   }
> > >  }
> > >  
> > > +static void intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
> > > +{
> > > + struct intel_crtc *intel_crtc =
> > > + to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
> > > +
> > > + intel_crtc->vbl_received = true;
> > > + wake_up(&intel_crtc->vbl_wait);
> > > +}
> > > +
> > >  static irqreturn_t valleyview_irq_handler(int irq, void *arg)
> > >  {
> > >   struct drm_device *dev = (struct drm_device *) arg;
> > > @@ -1479,8 +1488,10 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
> > >   spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> > >  
> > >   for_each_pipe(pipe) {
> > > - if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
> > > + if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS) {
> > >   drm_handle_vblank(dev, pipe);
> > > + intel_pipe_handle_vblank(dev, pipe);
> > > + }
> > >  
> > >   if (pipe_stats[pipe] & PLANE_FLIPDONE_INT_STATUS_VLV) {
> > >   intel_prepare_page_flip(dev, pipe);
> > > @@ -1679,8 +1690,10 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
> > >   DRM_ERROR("Poison interrupt\n");
> > >  
> > >   for_each_pipe(pipe) {
> > > - if (de_iir & DE_PIPE_VBLANK(pipe))
> > > + if (de_iir & DE_PIPE_VBLANK(pipe)) {
> > >   drm_handle_vblank(dev, pipe);
> > > + intel_pipe_handle_vblank(dev, pipe);
> > > + }
> > >  
> > >   if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe))
> > >   if (intel_set_cpu_fifo_underrun_reporting(dev, pipe, false))
> > > @@ -1729,8 +1742,10 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
> > >   intel_opregion_asle_intr(dev);
> > >  
> > >   for_each_pipe(i) {
> > > - if (de_iir & (DE_PIPE_VBLANK_IVB(i)))
> > > + if (de_iir & (DE_PIPE_VBLANK_IVB(i))) {
> > >   drm_handle_vblank(dev, i);
> > > + intel_pipe_handle_vblank(dev, i);
> > > + }
> > >  
> > >   /* plane/pipes map 1:1 on ilk+ */
> > >   if (de_iir & DE_PLANE_FLIP_DONE_IVB(i)) {
> > > @@ -1872,8 +1887,10 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
> > >   continue;
> > >  
> > >   pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
> > > - if (pipe_iir & GEN8_PIPE_VBLANK)
> > > + if (pipe_iir & GEN8_PIPE_VBLANK) {
> > >   drm_handle_vblank(dev, pipe);
> > > + intel_pipe_handle_vblank(dev, pipe);
> > > + }
> > >  
> > >   if (pipe_iir & GEN8_PIPE_FLIP_DONE) {
> > >   intel_prepare_page_flip(dev, pipe);
> > > @@ -3161,6 +3178,8 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
> > >   if (!drm_handle_vblank(dev, pipe))
> > >   return false;
> > >  
> > > + intel_pipe_handle_vblank(dev, pipe);
> > > +
> > >   if ((iir & flip_pending) == 0)
> > >   return false;
> > >  
> > > @@ -3344,6 +3363,8 @@ static bool i915_handle_vblank(struct drm_device *dev,
> > >   if (!drm_handle_vblank(dev, pipe))
> > >   return false;
> > >  
> > > + intel_pipe_handle_vblank(dev, pipe);
> > > +
> > >   if ((iir & flip_pending) == 0)
> > >   return false;
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index d1cc7ea..b39e445 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -10297,6 +10297,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> > >   intel_crtc->plane = !pipe;
> > >   }
> > >  
> > > + init_waitqueue_head(&intel_crtc->vbl_wait);
> > > +
> > >   BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
> > >         dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
> > >   dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 318bb4c..e539550 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -376,6 +376,9 @@ struct intel_crtc {
> > >   /* watermarks currently being used  */
> > >   struct intel_pipe_wm active;
> > >   } wm;
> > > +
> > > + wait_queue_head_t vbl_wait;
> > > + bool vbl_received;
> > >  };
> > >  
> > >  struct intel_plane_wm_parameters {
> > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > index ed9fa7c..1ffa7ba 100644
> > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > @@ -37,6 +37,79 @@
> > >  #include <drm/i915_drm.h>
> > >  #include "i915_drv.h"
> > >  
> > > +static unsigned int usecs_to_scanlines(const struct drm_display_mode *mode, unsigned int usecs)
> > > +{
> > > + /* paranoia */
> > > + if (!mode->crtc_htotal)
> > > + return 1;
> > > +
> > > + return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal);
> > > +}
> > > +
> > > +static void intel_pipe_update_start(struct intel_crtc *crtc)
> > > +{
> > > + struct drm_device *dev = crtc->base.dev;
> > > + const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
> > > + enum pipe pipe = crtc->pipe;
> > > + /* FIXME needs to be calibrated sensibly */
> > > + unsigned int min = mode->crtc_vblank_start - usecs_to_scanlines(mode, 100);
> > > + unsigned int max = mode->crtc_vblank_start - 1;
> > > + long timeout = msecs_to_jiffies_timeout(1);
> > > + unsigned int scanline;
> > > +
> > > + WARN_ON(!mutex_is_locked(&crtc->base.mutex));
> > > +
> > > + if (WARN_ON(drm_vblank_get(dev, pipe)))
> > > + return;
> > > +
> > > + local_irq_disable();
> > > +
> > > + /*
> > > + * Explicit compiler barrier is used to make sure that vbl_received
> > > + * store happens before reading the current scanline, otherwise
> > > + * something like this this might happen:
> > > + *
> > > + * 1. scanline = intel_get_crtc_scanline();
> > > + * 2. vblank irq -> vbl_received = true
> > > + * 3. vbl_received = false
> > > + *
> > > + * Which could make us miss the fact that we already crossed into
> > > + * vblank even though the scanline indicates that we're somewhere
> > > + * just before the start of vblank.
> > > + *
> > > + * wake_up() vs. wait_event_timeout() imply the necessary memory
> > > + * barries to make sure wait_event_timeout() sees the correct
> > > + * value of vbl_received after waking up.
> > > + */
> > > + crtc->vbl_received = false;
> > > + barrier();
> > > + scanline = intel_get_crtc_scanline(&crtc->base);
> > 
> > Ok, took a bit longer but here's it. I think we need a bit more polish
> > with this one. My thoughts about all this:
> > 
> > - Definitely an s/barrier/smp_*/ required. Yeah, on x86 with the total
> >   store order it's ok, but a big reason for adding barriers and comments
> >   is documentation. And a big reason for that is to deter people from too
> >   many lockless tricks ;-)
> 
> I still don't see why you'd need an smp_ memory barrier here. It's just
> one regular store vs. mmio read ordering issue we have here.
> 
> If we'd really need to worry about hardware reordering the operations,
> then it would seem that we'd need a mandatory memory barrier instead
> of an smp one.

Hm yeah, full barrier makes more sense. I'd also throw a full barrier in
_before_ we reset vbl_received in the interrupt handling code.

Also a style nitpick with barrier comments is to have it a both places and
reference the place of the other one. So I think we should throw in an
another comment in intel_pipe_handle_vblank. Actually two to mention that
the wake_up implicit parrier syncs with the implicit barrier in wait_event
in intel_pipe_update_start. With that I'll shut up about my nagging ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v3 3/5] drm/i915: Make sprite updates atomic

[Ville Syrjälä]

On Mon, Jan 27, 2014 at 05:03:39PM +0100, Daniel Vetter wrote:
> On Mon, Jan 27, 2014 at 01:06:33PM +0200, Ville Syrjälä wrote:
> > On Sun, Jan 26, 2014 at 06:29:06PM +0100, Daniel Vetter wrote:
> > > On Tue, Jan 21, 2014 at 04:12:38PM +0200, ville.syrjala@linux.intel.com wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > >
> > > > Add a mechanism by which we can evade the leading edge of vblank. This
> > > > guarantees that no two sprite register writes will straddle on either
> > > > side of the vblank start, and that means all the writes will be latched
> > > > together in one atomic operation.
> > > >
> > > > We do the vblank evade by checking the scanline counter, and if it's too
> > > > close to the start of vblank (too close has been hardcoded to 100usec
> > > > for now), we will wait for the vblank start to pass. In order to
> > > > eliminate random delayes from the rest of the system, we operate with
> > > > interrupts disabled, except when waiting for the vblank obviously.
> > > >
> > > > Note that we now go digging through pipe_to_crtc_mapping[] in the
> > > > vblank interrupt handler, which is a bit dangerous since we set up
> > > > interrupts before the crtcs. However in this case since it's the vblank
> > > > interrupt, we don't actually unmask it until some piece of code
> > > > requests it.
> > > >
> > > > v2: preempt_check_resched() calls after local_irq_enable() (Jesse)
> > > >     Hook up the vblank irq stuff on BDW as well
> > > > v3: Pass intel_crtc instead of drm_crtc (Daniel)
> > > >     Warn if crtc.mutex isn't locked (Daniel)
> > > >     Add an explicit compiler barrier and document the barriers (Daniel)
> > > >     Note the irq vs. modeset setup madness in the commit message (Daniel)
> > > >
> > > > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_irq.c      |  29 ++++++++--
> > > >  drivers/gpu/drm/i915/intel_display.c |   2 +
> > > >  drivers/gpu/drm/i915/intel_drv.h     |   3 +
> > > >  drivers/gpu/drm/i915/intel_sprite.c  | 103 +++++++++++++++++++++++++++++++++++
> > > >  4 files changed, 133 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > > index ffb56a9..8ef9edb 100644
> > > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > > @@ -1439,6 +1439,15 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
> > > >   }
> > > >  }
> > > >  
> > > > +static void intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
> > > > +{
> > > > + struct intel_crtc *intel_crtc =
> > > > + to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
> > > > +
> > > > + intel_crtc->vbl_received = true;
> > > > + wake_up(&intel_crtc->vbl_wait);
> > > > +}
> > > > +
> > > >  static irqreturn_t valleyview_irq_handler(int irq, void *arg)
> > > >  {
> > > >   struct drm_device *dev = (struct drm_device *) arg;
> > > > @@ -1479,8 +1488,10 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
> > > >   spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> > > >  
> > > >   for_each_pipe(pipe) {
> > > > - if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
> > > > + if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS) {
> > > >   drm_handle_vblank(dev, pipe);
> > > > + intel_pipe_handle_vblank(dev, pipe);
> > > > + }
> > > >  
> > > >   if (pipe_stats[pipe] & PLANE_FLIPDONE_INT_STATUS_VLV) {
> > > >   intel_prepare_page_flip(dev, pipe);
> > > > @@ -1679,8 +1690,10 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
> > > >   DRM_ERROR("Poison interrupt\n");
> > > >  
> > > >   for_each_pipe(pipe) {
> > > > - if (de_iir & DE_PIPE_VBLANK(pipe))
> > > > + if (de_iir & DE_PIPE_VBLANK(pipe)) {
> > > >   drm_handle_vblank(dev, pipe);
> > > > + intel_pipe_handle_vblank(dev, pipe);
> > > > + }
> > > >  
> > > >   if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe))
> > > >   if (intel_set_cpu_fifo_underrun_reporting(dev, pipe, false))
> > > > @@ -1729,8 +1742,10 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
> > > >   intel_opregion_asle_intr(dev);
> > > >  
> > > >   for_each_pipe(i) {
> > > > - if (de_iir & (DE_PIPE_VBLANK_IVB(i)))
> > > > + if (de_iir & (DE_PIPE_VBLANK_IVB(i))) {
> > > >   drm_handle_vblank(dev, i);
> > > > + intel_pipe_handle_vblank(dev, i);
> > > > + }
> > > >  
> > > >   /* plane/pipes map 1:1 on ilk+ */
> > > >   if (de_iir & DE_PLANE_FLIP_DONE_IVB(i)) {
> > > > @@ -1872,8 +1887,10 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
> > > >   continue;
> > > >  
> > > >   pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
> > > > - if (pipe_iir & GEN8_PIPE_VBLANK)
> > > > + if (pipe_iir & GEN8_PIPE_VBLANK) {
> > > >   drm_handle_vblank(dev, pipe);
> > > > + intel_pipe_handle_vblank(dev, pipe);
> > > > + }
> > > >  
> > > >   if (pipe_iir & GEN8_PIPE_FLIP_DONE) {
> > > >   intel_prepare_page_flip(dev, pipe);
> > > > @@ -3161,6 +3178,8 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
> > > >   if (!drm_handle_vblank(dev, pipe))
> > > >   return false;
> > > >  
> > > > + intel_pipe_handle_vblank(dev, pipe);
> > > > +
> > > >   if ((iir & flip_pending) == 0)
> > > >   return false;
> > > >  
> > > > @@ -3344,6 +3363,8 @@ static bool i915_handle_vblank(struct drm_device *dev,
> > > >   if (!drm_handle_vblank(dev, pipe))
> > > >   return false;
> > > >  
> > > > + intel_pipe_handle_vblank(dev, pipe);
> > > > +
> > > >   if ((iir & flip_pending) == 0)
> > > >   return false;
> > > >  
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index d1cc7ea..b39e445 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -10297,6 +10297,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> > > >   intel_crtc->plane = !pipe;
> > > >   }
> > > >  
> > > > + init_waitqueue_head(&intel_crtc->vbl_wait);
> > > > +
> > > >   BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
> > > >         dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
> > > >   dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 318bb4c..e539550 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -376,6 +376,9 @@ struct intel_crtc {
> > > >   /* watermarks currently being used  */
> > > >   struct intel_pipe_wm active;
> > > >   } wm;
> > > > +
> > > > + wait_queue_head_t vbl_wait;
> > > > + bool vbl_received;
> > > >  };
> > > >  
> > > >  struct intel_plane_wm_parameters {
> > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > > index ed9fa7c..1ffa7ba 100644
> > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > > @@ -37,6 +37,79 @@
> > > >  #include <drm/i915_drm.h>
> > > >  #include "i915_drv.h"
> > > >  
> > > > +static unsigned int usecs_to_scanlines(const struct drm_display_mode *mode, unsigned int usecs)
> > > > +{
> > > > + /* paranoia */
> > > > + if (!mode->crtc_htotal)
> > > > + return 1;
> > > > +
> > > > + return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal);
> > > > +}
> > > > +
> > > > +static void intel_pipe_update_start(struct intel_crtc *crtc)
> > > > +{
> > > > + struct drm_device *dev = crtc->base.dev;
> > > > + const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
> > > > + enum pipe pipe = crtc->pipe;
> > > > + /* FIXME needs to be calibrated sensibly */
> > > > + unsigned int min = mode->crtc_vblank_start - usecs_to_scanlines(mode, 100);
> > > > + unsigned int max = mode->crtc_vblank_start - 1;
> > > > + long timeout = msecs_to_jiffies_timeout(1);
> > > > + unsigned int scanline;
> > > > +
> > > > + WARN_ON(!mutex_is_locked(&crtc->base.mutex));
> > > > +
> > > > + if (WARN_ON(drm_vblank_get(dev, pipe)))
> > > > + return;
> > > > +
> > > > + local_irq_disable();
> > > > +
> > > > + /*
> > > > + * Explicit compiler barrier is used to make sure that vbl_received
> > > > + * store happens before reading the current scanline, otherwise
> > > > + * something like this this might happen:
> > > > + *
> > > > + * 1. scanline = intel_get_crtc_scanline();
> > > > + * 2. vblank irq -> vbl_received = true
> > > > + * 3. vbl_received = false
> > > > + *
> > > > + * Which could make us miss the fact that we already crossed into
> > > > + * vblank even though the scanline indicates that we're somewhere
> > > > + * just before the start of vblank.
> > > > + *
> > > > + * wake_up() vs. wait_event_timeout() imply the necessary memory
> > > > + * barries to make sure wait_event_timeout() sees the correct
> > > > + * value of vbl_received after waking up.
> > > > + */
> > > > + crtc->vbl_received = false;
> > > > + barrier();
> > > > + scanline = intel_get_crtc_scanline(&crtc->base);
> > > 
> > > Ok, took a bit longer but here's it. I think we need a bit more polish
> > > with this one. My thoughts about all this:
> > > 
> > > - Definitely an s/barrier/smp_*/ required. Yeah, on x86 with the total
> > >   store order it's ok, but a big reason for adding barriers and comments
> > >   is documentation. And a big reason for that is to deter people from too
> > >   many lockless tricks ;-)
> > 
> > I still don't see why you'd need an smp_ memory barrier here. It's just
> > one regular store vs. mmio read ordering issue we have here.
> > 
> > If we'd really need to worry about hardware reordering the operations,
> > then it would seem that we'd need a mandatory memory barrier instead
> > of an smp one.
> 
> Hm yeah, full barrier makes more sense. I'd also throw a full barrier in
> _before_ we reset vbl_received in the interrupt handling code.
> 
> Also a style nitpick with barrier comments is to have it a both places and
> reference the place of the other one. So I think we should throw in an
> another comment in intel_pipe_handle_vblank. Actually two to mention that
> the wake_up implicit parrier syncs with the implicit barrier in wait_event
> in intel_pipe_update_start. With that I'll shut up about my nagging ;-)

I still think a compiler barrier is all we need here. We only have one
piece of shared data, and any ordering restriction we have is strictly
within one CPU (vbl_received=false store vs. scanline read), so normal
cache coherency is enough to handle the irq handler vs. waiter situation.

I could be convinced to use mb() between the vbl_received store and
scanline read for paranoia's sake, but adding a mb() to the irq handler
I don't get at all. I suppose I could add it there just for fun, but then
then the comment would have to be:

/* danvet's memory barrier */

since I wouldn't know what else to write ;)

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v3 3/5] drm/i915: Make sprite updates atomic

[Daniel Vetter]

On Thu, Feb 06, 2014 at 11:25:17AM +0200, Ville Syrjälä wrote:
> On Mon, Jan 27, 2014 at 05:03:39PM +0100, Daniel Vetter wrote:
> > On Mon, Jan 27, 2014 at 01:06:33PM +0200, Ville Syrjälä wrote:
> > > On Sun, Jan 26, 2014 at 06:29:06PM +0100, Daniel Vetter wrote:
> > > > On Tue, Jan 21, 2014 at 04:12:38PM +0200, ville.syrjala@linux.intel.com wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > >
> > > > > Add a mechanism by which we can evade the leading edge of vblank. This
> > > > > guarantees that no two sprite register writes will straddle on either
> > > > > side of the vblank start, and that means all the writes will be latched
> > > > > together in one atomic operation.
> > > > >
> > > > > We do the vblank evade by checking the scanline counter, and if it's too
> > > > > close to the start of vblank (too close has been hardcoded to 100usec
> > > > > for now), we will wait for the vblank start to pass. In order to
> > > > > eliminate random delayes from the rest of the system, we operate with
> > > > > interrupts disabled, except when waiting for the vblank obviously.
> > > > >
> > > > > Note that we now go digging through pipe_to_crtc_mapping[] in the
> > > > > vblank interrupt handler, which is a bit dangerous since we set up
> > > > > interrupts before the crtcs. However in this case since it's the vblank
> > > > > interrupt, we don't actually unmask it until some piece of code
> > > > > requests it.
> > > > >
> > > > > v2: preempt_check_resched() calls after local_irq_enable() (Jesse)
> > > > >     Hook up the vblank irq stuff on BDW as well
> > > > > v3: Pass intel_crtc instead of drm_crtc (Daniel)
> > > > >     Warn if crtc.mutex isn't locked (Daniel)
> > > > >     Add an explicit compiler barrier and document the barriers (Daniel)
> > > > >     Note the irq vs. modeset setup madness in the commit message (Daniel)
> > > > >
> > > > > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_irq.c      |  29 ++++++++--
> > > > >  drivers/gpu/drm/i915/intel_display.c |   2 +
> > > > >  drivers/gpu/drm/i915/intel_drv.h     |   3 +
> > > > >  drivers/gpu/drm/i915/intel_sprite.c  | 103 +++++++++++++++++++++++++++++++++++
> > > > >  4 files changed, 133 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > > > index ffb56a9..8ef9edb 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > > > @@ -1439,6 +1439,15 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
> > > > >   }
> > > > >  }
> > > > >  
> > > > > +static void intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
> > > > > +{
> > > > > + struct intel_crtc *intel_crtc =
> > > > > + to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
> > > > > +
> > > > > + intel_crtc->vbl_received = true;
> > > > > + wake_up(&intel_crtc->vbl_wait);
> > > > > +}
> > > > > +
> > > > >  static irqreturn_t valleyview_irq_handler(int irq, void *arg)
> > > > >  {
> > > > >   struct drm_device *dev = (struct drm_device *) arg;
> > > > > @@ -1479,8 +1488,10 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
> > > > >   spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> > > > >  
> > > > >   for_each_pipe(pipe) {
> > > > > - if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
> > > > > + if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS) {
> > > > >   drm_handle_vblank(dev, pipe);
> > > > > + intel_pipe_handle_vblank(dev, pipe);
> > > > > + }
> > > > >  
> > > > >   if (pipe_stats[pipe] & PLANE_FLIPDONE_INT_STATUS_VLV) {
> > > > >   intel_prepare_page_flip(dev, pipe);
> > > > > @@ -1679,8 +1690,10 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
> > > > >   DRM_ERROR("Poison interrupt\n");
> > > > >  
> > > > >   for_each_pipe(pipe) {
> > > > > - if (de_iir & DE_PIPE_VBLANK(pipe))
> > > > > + if (de_iir & DE_PIPE_VBLANK(pipe)) {
> > > > >   drm_handle_vblank(dev, pipe);
> > > > > + intel_pipe_handle_vblank(dev, pipe);
> > > > > + }
> > > > >  
> > > > >   if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe))
> > > > >   if (intel_set_cpu_fifo_underrun_reporting(dev, pipe, false))
> > > > > @@ -1729,8 +1742,10 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
> > > > >   intel_opregion_asle_intr(dev);
> > > > >  
> > > > >   for_each_pipe(i) {
> > > > > - if (de_iir & (DE_PIPE_VBLANK_IVB(i)))
> > > > > + if (de_iir & (DE_PIPE_VBLANK_IVB(i))) {
> > > > >   drm_handle_vblank(dev, i);
> > > > > + intel_pipe_handle_vblank(dev, i);
> > > > > + }
> > > > >  
> > > > >   /* plane/pipes map 1:1 on ilk+ */
> > > > >   if (de_iir & DE_PLANE_FLIP_DONE_IVB(i)) {
> > > > > @@ -1872,8 +1887,10 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
> > > > >   continue;
> > > > >  
> > > > >   pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
> > > > > - if (pipe_iir & GEN8_PIPE_VBLANK)
> > > > > + if (pipe_iir & GEN8_PIPE_VBLANK) {
> > > > >   drm_handle_vblank(dev, pipe);
> > > > > + intel_pipe_handle_vblank(dev, pipe);
> > > > > + }
> > > > >  
> > > > >   if (pipe_iir & GEN8_PIPE_FLIP_DONE) {
> > > > >   intel_prepare_page_flip(dev, pipe);
> > > > > @@ -3161,6 +3178,8 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
> > > > >   if (!drm_handle_vblank(dev, pipe))
> > > > >   return false;
> > > > >  
> > > > > + intel_pipe_handle_vblank(dev, pipe);
> > > > > +
> > > > >   if ((iir & flip_pending) == 0)
> > > > >   return false;
> > > > >  
> > > > > @@ -3344,6 +3363,8 @@ static bool i915_handle_vblank(struct drm_device *dev,
> > > > >   if (!drm_handle_vblank(dev, pipe))
> > > > >   return false;
> > > > >  
> > > > > + intel_pipe_handle_vblank(dev, pipe);
> > > > > +
> > > > >   if ((iir & flip_pending) == 0)
> > > > >   return false;
> > > > >  
> > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > > index d1cc7ea..b39e445 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > @@ -10297,6 +10297,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> > > > >   intel_crtc->plane = !pipe;
> > > > >   }
> > > > >  
> > > > > + init_waitqueue_head(&intel_crtc->vbl_wait);
> > > > > +
> > > > >   BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
> > > > >         dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
> > > > >   dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
> > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > > index 318bb4c..e539550 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > @@ -376,6 +376,9 @@ struct intel_crtc {
> > > > >   /* watermarks currently being used  */
> > > > >   struct intel_pipe_wm active;
> > > > >   } wm;
> > > > > +
> > > > > + wait_queue_head_t vbl_wait;
> > > > > + bool vbl_received;
> > > > >  };
> > > > >  
> > > > >  struct intel_plane_wm_parameters {
> > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > > > index ed9fa7c..1ffa7ba 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > > > @@ -37,6 +37,79 @@
> > > > >  #include <drm/i915_drm.h>
> > > > >  #include "i915_drv.h"
> > > > >  
> > > > > +static unsigned int usecs_to_scanlines(const struct drm_display_mode *mode, unsigned int usecs)
> > > > > +{
> > > > > + /* paranoia */
> > > > > + if (!mode->crtc_htotal)
> > > > > + return 1;
> > > > > +
> > > > > + return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal);
> > > > > +}
> > > > > +
> > > > > +static void intel_pipe_update_start(struct intel_crtc *crtc)
> > > > > +{
> > > > > + struct drm_device *dev = crtc->base.dev;
> > > > > + const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
> > > > > + enum pipe pipe = crtc->pipe;
> > > > > + /* FIXME needs to be calibrated sensibly */
> > > > > + unsigned int min = mode->crtc_vblank_start - usecs_to_scanlines(mode, 100);
> > > > > + unsigned int max = mode->crtc_vblank_start - 1;
> > > > > + long timeout = msecs_to_jiffies_timeout(1);
> > > > > + unsigned int scanline;
> > > > > +
> > > > > + WARN_ON(!mutex_is_locked(&crtc->base.mutex));
> > > > > +
> > > > > + if (WARN_ON(drm_vblank_get(dev, pipe)))
> > > > > + return;
> > > > > +
> > > > > + local_irq_disable();
> > > > > +
> > > > > + /*
> > > > > + * Explicit compiler barrier is used to make sure that vbl_received
> > > > > + * store happens before reading the current scanline, otherwise
> > > > > + * something like this this might happen:
> > > > > + *
> > > > > + * 1. scanline = intel_get_crtc_scanline();
> > > > > + * 2. vblank irq -> vbl_received = true
> > > > > + * 3. vbl_received = false
> > > > > + *
> > > > > + * Which could make us miss the fact that we already crossed into
> > > > > + * vblank even though the scanline indicates that we're somewhere
> > > > > + * just before the start of vblank.
> > > > > + *
> > > > > + * wake_up() vs. wait_event_timeout() imply the necessary memory
> > > > > + * barries to make sure wait_event_timeout() sees the correct
> > > > > + * value of vbl_received after waking up.
> > > > > + */
> > > > > + crtc->vbl_received = false;
> > > > > + barrier();
> > > > > + scanline = intel_get_crtc_scanline(&crtc->base);
> > > > 
> > > > Ok, took a bit longer but here's it. I think we need a bit more polish
> > > > with this one. My thoughts about all this:
> > > > 
> > > > - Definitely an s/barrier/smp_*/ required. Yeah, on x86 with the total
> > > >   store order it's ok, but a big reason for adding barriers and comments
> > > >   is documentation. And a big reason for that is to deter people from too
> > > >   many lockless tricks ;-)
> > > 
> > > I still don't see why you'd need an smp_ memory barrier here. It's just
> > > one regular store vs. mmio read ordering issue we have here.
> > > 
> > > If we'd really need to worry about hardware reordering the operations,
> > > then it would seem that we'd need a mandatory memory barrier instead
> > > of an smp one.
> > 
> > Hm yeah, full barrier makes more sense. I'd also throw a full barrier in
> > _before_ we reset vbl_received in the interrupt handling code.
> > 
> > Also a style nitpick with barrier comments is to have it a both places and
> > reference the place of the other one. So I think we should throw in an
> > another comment in intel_pipe_handle_vblank. Actually two to mention that
> > the wake_up implicit parrier syncs with the implicit barrier in wait_event
> > in intel_pipe_update_start. With that I'll shut up about my nagging ;-)
> 
> I still think a compiler barrier is all we need here. We only have one
> piece of shared data, and any ordering restriction we have is strictly
> within one CPU (vbl_received=false store vs. scanline read), so normal
> cache coherency is enough to handle the irq handler vs. waiter situation.
> 
> I could be convinced to use mb() between the vbl_received store and
> scanline read for paranoia's sake, but adding a mb() to the irq handler
> I don't get at all. I suppose I could add it there just for fun, but then
> then the comment would have to be:
> 
> /* danvet's memory barrier */
> 
> since I wouldn't know what else to write ;)

Wrt the compiler barrier: The subsequent mmio write already clobbers state
and so serves as a compiler barrier iirc. So adding another one won't
change anything. But we not only need to order the write against the mmio
read, we also need to make sure that the write lands in-order wrt the
write from the interrupt handler. Hence smp barriers in both places.

I know that on x86 the hw can't reorder stuff that badly anyway, so this
is purely documentation. Or maybe I'm just paranoid. In any case I think
either no barrier (and the comment still there) or more barriers ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 4/5] drm/i915: Perform primary enable/disable atomically with sprite updates

[ville.syrjala]

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

Move the primary plane enable/disable to occur atomically with the
sprite update that caused the primary plane visibility to change.

FBC and IPS enable/disable is left to happen well before or after
the primary plane change.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 96 ++++++++++++++++++++++---------------
 1 file changed, 57 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 198a056..c3034ad 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -89,6 +89,19 @@ static void intel_pipe_update_end(struct drm_crtc *crtc)
 	preempt_check_resched();
 }
 
+static void intel_update_primary_plane(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	int reg = DSPCNTR(intel_crtc->plane);
+
+	if (intel_crtc->primary_enabled)
+		I915_WRITE(reg, I915_READ(reg) | DISPLAY_PLANE_ENABLE);
+	else
+		I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE);
+}
+
 static void
 vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 		 struct drm_framebuffer *fb,
@@ -185,6 +198,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 
 	intel_pipe_update_start(crtc);
 
+	intel_update_primary_plane(crtc);
+
 	I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
 	I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
 
@@ -197,7 +212,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	I915_WRITE(SPCNTR(pipe, plane), sprctl);
 	I915_MODIFY_DISPBASE(SPSURF(pipe, plane), i915_gem_obj_ggtt_offset(obj) +
 			     sprsurf_offset);
-	POSTING_READ(SPSURF(pipe, plane));
+
+	intel_flush_primary_plane(dev_priv, to_intel_crtc(crtc)->plane);
 
 	intel_pipe_update_end(crtc);
 }
@@ -213,11 +229,14 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 
 	intel_pipe_update_start(crtc);
 
+	intel_update_primary_plane(crtc);
+
 	I915_WRITE(SPCNTR(pipe, plane), I915_READ(SPCNTR(pipe, plane)) &
 		   ~SP_ENABLE);
 	/* Activate double buffered register update */
 	I915_MODIFY_DISPBASE(SPSURF(pipe, plane), 0);
-	POSTING_READ(SPSURF(pipe, plane));
+
+	intel_flush_primary_plane(dev_priv, to_intel_crtc(crtc)->plane);
 
 	intel_pipe_update_end(crtc);
 
@@ -361,6 +380,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 
 	intel_pipe_update_start(crtc);
 
+	intel_update_primary_plane(crtc);
+
 	I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -379,7 +400,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_WRITE(SPRCTL(pipe), sprctl);
 	I915_MODIFY_DISPBASE(SPRSURF(pipe),
 			     i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
-	POSTING_READ(SPRSURF(pipe));
+
+	intel_flush_primary_plane(dev_priv, to_intel_crtc(crtc)->plane);
 
 	intel_pipe_update_end(crtc);
 }
@@ -394,13 +416,16 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 
 	intel_pipe_update_start(crtc);
 
+	intel_update_primary_plane(crtc);
+
 	I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE);
 	/* Can't leave the scaler enabled... */
 	if (intel_plane->can_scale)
 		I915_WRITE(SPRSCALE(pipe), 0);
 	/* Activate double buffered register update */
 	I915_MODIFY_DISPBASE(SPRSURF(pipe), 0);
-	POSTING_READ(SPRSURF(pipe));
+
+	intel_flush_primary_plane(dev_priv, to_intel_crtc(crtc)->plane);
 
 	intel_pipe_update_end(crtc);
 
@@ -548,6 +573,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 
 	intel_pipe_update_start(crtc);
 
+	intel_update_primary_plane(crtc);
+
 	I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -561,7 +588,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_WRITE(DVSCNTR(pipe), dvscntr);
 	I915_MODIFY_DISPBASE(DVSSURF(pipe),
 			     i915_gem_obj_ggtt_offset(obj) + dvssurf_offset);
-	POSTING_READ(DVSSURF(pipe));
+
+	intel_flush_primary_plane(dev_priv, to_intel_crtc(crtc)->plane);
 
 	intel_pipe_update_end(crtc);
 }
@@ -576,12 +604,15 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 
 	intel_pipe_update_start(crtc);
 
+	intel_update_primary_plane(crtc);
+
 	I915_WRITE(DVSCNTR(pipe), I915_READ(DVSCNTR(pipe)) & ~DVS_ENABLE);
 	/* Disable the scaler */
 	I915_WRITE(DVSSCALE(pipe), 0);
 	/* Flush double buffered register updates */
 	I915_MODIFY_DISPBASE(DVSSURF(pipe), 0);
-	POSTING_READ(DVSSURF(pipe));
+
+	intel_flush_primary_plane(dev_priv, to_intel_crtc(crtc)->plane);
 
 	intel_pipe_update_end(crtc);
 
@@ -595,20 +626,10 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 }
 
 static void
-intel_enable_primary(struct drm_crtc *crtc)
+intel_post_enable_primary(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int reg = DSPCNTR(intel_crtc->plane);
-
-	if (intel_crtc->primary_enabled)
-		return;
-
-	intel_crtc->primary_enabled = true;
-
-	I915_WRITE(reg, I915_READ(reg) | DISPLAY_PLANE_ENABLE);
-	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 	/*
 	 * FIXME IPS should be fine as long as one plane is
@@ -627,17 +648,11 @@ intel_enable_primary(struct drm_crtc *crtc)
 }
 
 static void
-intel_disable_primary(struct drm_crtc *crtc)
+intel_pre_disable_primary(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int reg = DSPCNTR(intel_crtc->plane);
-
-	if (!intel_crtc->primary_enabled)
-		return;
-
-	intel_crtc->primary_enabled = false;
 
 	mutex_lock(&dev->struct_mutex);
 	if (dev_priv->fbc.plane == intel_crtc->plane)
@@ -651,9 +666,6 @@ intel_disable_primary(struct drm_crtc *crtc)
 	 * versa.
 	 */
 	hsw_disable_ips(intel_crtc);
-
-	I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE);
-	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 }
 
 static int
@@ -747,7 +759,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_i915_gem_object *obj = intel_fb->obj;
 	struct drm_i915_gem_object *old_obj = intel_plane->obj;
 	int ret;
-	bool disable_primary = false;
+	bool primary_enabled;
 	bool visible;
 	int hscale, vscale;
 	int max_scale, min_scale;
@@ -918,8 +930,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	 * If the sprite is completely covering the primary plane,
 	 * we can disable the primary and save power.
 	 */
-	disable_primary = drm_rect_equals(&dst, &clip) && !colorkey_enabled(intel_plane);
-	WARN_ON(disable_primary && !visible && intel_crtc->active);
+	primary_enabled = !drm_rect_equals(&dst, &clip) || colorkey_enabled(intel_plane);
+	WARN_ON(!primary_enabled && !visible && intel_crtc->active);
 
 	mutex_lock(&dev->struct_mutex);
 
@@ -946,12 +958,12 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	intel_plane->obj = obj;
 
 	if (intel_crtc->active) {
-		/*
-		 * Be sure to re-enable the primary before the sprite is no longer
-		 * covering it fully.
-		 */
-		if (!disable_primary)
-			intel_enable_primary(crtc);
+		bool primary_was_enabled = intel_crtc->primary_enabled;
+
+		intel_crtc->primary_enabled = primary_enabled;
+
+		if (primary_was_enabled && !primary_enabled)
+			intel_pre_disable_primary(crtc);
 
 		if (visible)
 			intel_plane->update_plane(plane, crtc, fb, obj,
@@ -960,8 +972,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		else
 			intel_plane->disable_plane(plane, crtc);
 
-		if (disable_primary)
-			intel_disable_primary(crtc);
+		if (!primary_was_enabled && primary_enabled)
+			intel_post_enable_primary(crtc);
 	}
 
 	/* Unpin old obj after new one is active to avoid ugliness */
@@ -999,8 +1011,14 @@ intel_disable_plane(struct drm_plane *plane)
 	intel_crtc = to_intel_crtc(plane->crtc);
 
 	if (intel_crtc->active) {
-		intel_enable_primary(plane->crtc);
+		bool primary_was_enabled = intel_crtc->primary_enabled;
+
+		intel_crtc->primary_enabled = true;
+
 		intel_plane->disable_plane(plane, plane->crtc);
+
+		if (!primary_was_enabled && intel_crtc->primary_enabled)
+			intel_post_enable_primary(plane->crtc);
 	}
 
 	if (intel_plane->obj) {
-- 
1.8.3.2

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

[PATCH v2 4/5] drm/i915: Perform primary enable/disable atomically with sprite updates

[ville.syrjala]

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

Move the primary plane enable/disable to occur atomically with the
sprite update that caused the primary plane visibility to change.

FBC and IPS enable/disable is left to happen well before or after
the primary plane change.

v2: Pass intel_crtc instead of drm_crtc (Daniel)

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 94 ++++++++++++++++++++++---------------
 1 file changed, 55 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 1ffa7ba..8bf3be8 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -110,6 +110,17 @@ static void intel_pipe_update_end(struct intel_crtc *crtc)
 	preempt_check_resched();
 }
 
+static void intel_update_primary_plane(struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	int reg = DSPCNTR(crtc->plane);
+
+	if (crtc->primary_enabled)
+		I915_WRITE(reg, I915_READ(reg) | DISPLAY_PLANE_ENABLE);
+	else
+		I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE);
+}
+
 static void
 vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 		 struct drm_framebuffer *fb,
@@ -207,6 +218,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 
 	intel_pipe_update_start(intel_crtc);
 
+	intel_update_primary_plane(intel_crtc);
+
 	I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
 	I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
 
@@ -219,7 +232,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	I915_WRITE(SPCNTR(pipe, plane), sprctl);
 	I915_MODIFY_DISPBASE(SPSURF(pipe, plane), i915_gem_obj_ggtt_offset(obj) +
 			     sprsurf_offset);
-	POSTING_READ(SPSURF(pipe, plane));
+
+	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 	intel_pipe_update_end(intel_crtc);
 }
@@ -236,11 +250,14 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 
 	intel_pipe_update_start(intel_crtc);
 
+	intel_update_primary_plane(intel_crtc);
+
 	I915_WRITE(SPCNTR(pipe, plane), I915_READ(SPCNTR(pipe, plane)) &
 		   ~SP_ENABLE);
 	/* Activate double buffered register update */
 	I915_MODIFY_DISPBASE(SPSURF(pipe, plane), 0);
-	POSTING_READ(SPSURF(pipe, plane));
+
+	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 	intel_pipe_update_end(intel_crtc);
 
@@ -385,6 +402,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 
 	intel_pipe_update_start(intel_crtc);
 
+	intel_update_primary_plane(intel_crtc);
+
 	I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -403,7 +422,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_WRITE(SPRCTL(pipe), sprctl);
 	I915_MODIFY_DISPBASE(SPRSURF(pipe),
 			     i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
-	POSTING_READ(SPRSURF(pipe));
+
+	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 	intel_pipe_update_end(intel_crtc);
 }
@@ -419,13 +439,16 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 
 	intel_pipe_update_start(intel_crtc);
 
+	intel_update_primary_plane(intel_crtc);
+
 	I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE);
 	/* Can't leave the scaler enabled... */
 	if (intel_plane->can_scale)
 		I915_WRITE(SPRSCALE(pipe), 0);
 	/* Activate double buffered register update */
 	I915_MODIFY_DISPBASE(SPRSURF(pipe), 0);
-	POSTING_READ(SPRSURF(pipe));
+
+	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 	intel_pipe_update_end(intel_crtc);
 
@@ -574,6 +597,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 
 	intel_pipe_update_start(intel_crtc);
 
+	intel_update_primary_plane(intel_crtc);
+
 	I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -587,7 +612,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_WRITE(DVSCNTR(pipe), dvscntr);
 	I915_MODIFY_DISPBASE(DVSSURF(pipe),
 			     i915_gem_obj_ggtt_offset(obj) + dvssurf_offset);
-	POSTING_READ(DVSSURF(pipe));
+
+	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 	intel_pipe_update_end(intel_crtc);
 }
@@ -603,12 +629,15 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 
 	intel_pipe_update_start(intel_crtc);
 
+	intel_update_primary_plane(intel_crtc);
+
 	I915_WRITE(DVSCNTR(pipe), I915_READ(DVSCNTR(pipe)) & ~DVS_ENABLE);
 	/* Disable the scaler */
 	I915_WRITE(DVSSCALE(pipe), 0);
 	/* Flush double buffered register updates */
 	I915_MODIFY_DISPBASE(DVSSURF(pipe), 0);
-	POSTING_READ(DVSSURF(pipe));
+
+	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 	intel_pipe_update_end(intel_crtc);
 
@@ -622,20 +651,10 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 }
 
 static void
-intel_enable_primary(struct drm_crtc *crtc)
+intel_post_enable_primary(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int reg = DSPCNTR(intel_crtc->plane);
-
-	if (intel_crtc->primary_enabled)
-		return;
-
-	intel_crtc->primary_enabled = true;
-
-	I915_WRITE(reg, I915_READ(reg) | DISPLAY_PLANE_ENABLE);
-	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 	/*
 	 * FIXME IPS should be fine as long as one plane is
@@ -654,17 +673,11 @@ intel_enable_primary(struct drm_crtc *crtc)
 }
 
 static void
-intel_disable_primary(struct drm_crtc *crtc)
+intel_pre_disable_primary(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int reg = DSPCNTR(intel_crtc->plane);
-
-	if (!intel_crtc->primary_enabled)
-		return;
-
-	intel_crtc->primary_enabled = false;
 
 	mutex_lock(&dev->struct_mutex);
 	if (dev_priv->fbc.plane == intel_crtc->plane)
@@ -678,9 +691,6 @@ intel_disable_primary(struct drm_crtc *crtc)
 	 * versa.
 	 */
 	hsw_disable_ips(intel_crtc);
-
-	I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE);
-	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 }
 
 static int
@@ -774,7 +784,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_i915_gem_object *obj = intel_fb->obj;
 	struct drm_i915_gem_object *old_obj = intel_plane->obj;
 	int ret;
-	bool disable_primary = false;
+	bool primary_enabled;
 	bool visible;
 	int hscale, vscale;
 	int max_scale, min_scale;
@@ -945,8 +955,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	 * If the sprite is completely covering the primary plane,
 	 * we can disable the primary and save power.
 	 */
-	disable_primary = drm_rect_equals(&dst, &clip) && !colorkey_enabled(intel_plane);
-	WARN_ON(disable_primary && !visible && intel_crtc->active);
+	primary_enabled = !drm_rect_equals(&dst, &clip) || colorkey_enabled(intel_plane);
+	WARN_ON(!primary_enabled && !visible && intel_crtc->active);
 
 	mutex_lock(&dev->struct_mutex);
 
@@ -973,12 +983,12 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	intel_plane->obj = obj;
 
 	if (intel_crtc->active) {
-		/*
-		 * Be sure to re-enable the primary before the sprite is no longer
-		 * covering it fully.
-		 */
-		if (!disable_primary)
-			intel_enable_primary(crtc);
+		bool primary_was_enabled = intel_crtc->primary_enabled;
+
+		intel_crtc->primary_enabled = primary_enabled;
+
+		if (primary_was_enabled && !primary_enabled)
+			intel_pre_disable_primary(crtc);
 
 		if (visible)
 			intel_plane->update_plane(plane, crtc, fb, obj,
@@ -987,8 +997,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		else
 			intel_plane->disable_plane(plane, crtc);
 
-		if (disable_primary)
-			intel_disable_primary(crtc);
+		if (!primary_was_enabled && primary_enabled)
+			intel_post_enable_primary(crtc);
 	}
 
 	/* Unpin old obj after new one is active to avoid ugliness */
@@ -1026,8 +1036,14 @@ intel_disable_plane(struct drm_plane *plane)
 	intel_crtc = to_intel_crtc(plane->crtc);
 
 	if (intel_crtc->active) {
-		intel_enable_primary(plane->crtc);
+		bool primary_was_enabled = intel_crtc->primary_enabled;
+
+		intel_crtc->primary_enabled = true;
+
 		intel_plane->disable_plane(plane, plane->crtc);
+
+		if (!primary_was_enabled && intel_crtc->primary_enabled)
+			intel_post_enable_primary(plane->crtc);
 	}
 
 	if (intel_plane->obj) {
-- 
1.8.3.2

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

[PATCH v2 5/5] drm/i915: Add pipe update trace points

[ville.syrjala]

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

Add trace points for observing the atomic pipe update mechanism.

v2: Rebased due to earlier changes

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_trace.h   | 77 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_sprite.c |  6 +++
 2 files changed, 83 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index 6e580c9..58d9a7b 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -7,6 +7,7 @@
 
 #include <drm/drmP.h>
 #include "i915_drv.h"
+#include "intel_drv.h"
 #include "intel_ringbuffer.h"
 
 #undef TRACE_SYSTEM
@@ -14,6 +15,82 @@
 #define TRACE_SYSTEM_STRING __stringify(TRACE_SYSTEM)
 #define TRACE_INCLUDE_FILE i915_trace
 
+/* pipe updates */
+
+TRACE_EVENT(i915_pipe_update_start,
+	    TP_PROTO(struct drm_crtc *crtc, u32 min, u32 max),
+	    TP_ARGS(crtc, min, max),
+
+	    TP_STRUCT__entry(
+			     __field(enum pipe, pipe)
+			     __field(u32, frame)
+			     __field(u32, scanline)
+			     __field(u32, min)
+			     __field(u32, max)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->pipe = to_intel_crtc(crtc)->pipe;
+			   __entry->frame = crtc->dev->driver->get_vblank_counter(crtc->dev,
+										  to_intel_crtc(crtc)->pipe);
+			   __entry->scanline = intel_get_crtc_scanline(crtc);
+			   __entry->min = min;
+			   __entry->max = max;
+			   ),
+
+	    TP_printk("pipe %c, frame=%u, scanline=%u, min=%u, max=%u",
+		      pipe_name(__entry->pipe), __entry->frame,
+		       __entry->scanline, __entry->min, __entry->max)
+);
+
+TRACE_EVENT(i915_pipe_update_vblank_evaded,
+	    TP_PROTO(struct drm_crtc *crtc, u32 min, u32 max),
+	    TP_ARGS(crtc, min, max),
+
+	    TP_STRUCT__entry(
+			     __field(enum pipe, pipe)
+			     __field(u32, frame)
+			     __field(u32, scanline)
+			     __field(u32, min)
+			     __field(u32, max)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->pipe = to_intel_crtc(crtc)->pipe;
+			   __entry->frame = crtc->dev->driver->get_vblank_counter(crtc->dev,
+										  to_intel_crtc(crtc)->pipe);
+			   __entry->scanline = intel_get_crtc_scanline(crtc);
+			   __entry->min = min;
+			   __entry->max = max;
+			   ),
+
+	    TP_printk("pipe %c, frame=%u, scanline=%u, min=%u, max=%u",
+		      pipe_name(__entry->pipe), __entry->frame,
+		       __entry->scanline, __entry->min, __entry->max)
+);
+
+TRACE_EVENT(i915_pipe_update_end,
+	    TP_PROTO(struct drm_crtc *crtc),
+	    TP_ARGS(crtc),
+
+	    TP_STRUCT__entry(
+			     __field(enum pipe, pipe)
+			     __field(u32, frame)
+			     __field(u32, scanline)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->pipe = to_intel_crtc(crtc)->pipe;
+			   __entry->frame = crtc->dev->driver->get_vblank_counter(crtc->dev,
+										  to_intel_crtc(crtc)->pipe);
+			   __entry->scanline = intel_get_crtc_scanline(crtc);
+			   ),
+
+	    TP_printk("pipe %c, frame=%u, scanline=%u",
+		      pipe_name(__entry->pipe), __entry->frame,
+		      __entry->scanline)
+);
+
 /* object tracking */
 
 TRACE_EVENT(i915_gem_object_create,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index c3034ad..c033af7 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -63,6 +63,8 @@ static void intel_pipe_update_start(struct drm_crtc *crtc)
 
 	local_irq_disable();
 
+	trace_i915_pipe_update_start(crtc, min, max);
+
 	intel_crtc->vbl_received = false;
 	scanline = intel_get_crtc_scanline(crtc);
 
@@ -81,10 +83,14 @@ static void intel_pipe_update_start(struct drm_crtc *crtc)
 	}
 
 	drm_vblank_put(dev, pipe);
+
+	trace_i915_pipe_update_vblank_evaded(crtc, min, max);
 }
 
 static void intel_pipe_update_end(struct drm_crtc *crtc)
 {
+	trace_i915_pipe_update_end(crtc);
+
 	local_irq_enable();
 	preempt_check_resched();
 }
-- 
1.8.3.2

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

[PATCH v3 5/5] drm/i915: Add pipe update trace points

[ville.syrjala]

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

Add trace points for observing the atomic pipe update mechanism.

v2: Rebased due to earlier changes
v3: Pass intel_crtc instead of drm_crtc (Daniel)

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_trace.h   | 77 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_sprite.c |  6 +++
 2 files changed, 83 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index 6e580c9..b903f8e 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -7,6 +7,7 @@
 
 #include <drm/drmP.h>
 #include "i915_drv.h"
+#include "intel_drv.h"
 #include "intel_ringbuffer.h"
 
 #undef TRACE_SYSTEM
@@ -14,6 +15,82 @@
 #define TRACE_SYSTEM_STRING __stringify(TRACE_SYSTEM)
 #define TRACE_INCLUDE_FILE i915_trace
 
+/* pipe updates */
+
+TRACE_EVENT(i915_pipe_update_start,
+	    TP_PROTO(struct intel_crtc *crtc, u32 min, u32 max),
+	    TP_ARGS(crtc, min, max),
+
+	    TP_STRUCT__entry(
+			     __field(enum pipe, pipe)
+			     __field(u32, frame)
+			     __field(u32, scanline)
+			     __field(u32, min)
+			     __field(u32, max)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->pipe = crtc->pipe;
+			   __entry->frame = crtc->base.dev->driver->get_vblank_counter(crtc->base.dev,
+										       crtc->pipe);
+			   __entry->scanline = intel_get_crtc_scanline(&crtc->base);
+			   __entry->min = min;
+			   __entry->max = max;
+			   ),
+
+	    TP_printk("pipe %c, frame=%u, scanline=%u, min=%u, max=%u",
+		      pipe_name(__entry->pipe), __entry->frame,
+		       __entry->scanline, __entry->min, __entry->max)
+);
+
+TRACE_EVENT(i915_pipe_update_vblank_evaded,
+	    TP_PROTO(struct intel_crtc *crtc, u32 min, u32 max),
+	    TP_ARGS(crtc, min, max),
+
+	    TP_STRUCT__entry(
+			     __field(enum pipe, pipe)
+			     __field(u32, frame)
+			     __field(u32, scanline)
+			     __field(u32, min)
+			     __field(u32, max)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->pipe = crtc->pipe;
+			   __entry->frame = crtc->base.dev->driver->get_vblank_counter(crtc->base.dev,
+										       crtc->pipe);
+			   __entry->scanline = intel_get_crtc_scanline(&crtc->base);
+			   __entry->min = min;
+			   __entry->max = max;
+			   ),
+
+	    TP_printk("pipe %c, frame=%u, scanline=%u, min=%u, max=%u",
+		      pipe_name(__entry->pipe), __entry->frame,
+		       __entry->scanline, __entry->min, __entry->max)
+);
+
+TRACE_EVENT(i915_pipe_update_end,
+	    TP_PROTO(struct intel_crtc *crtc),
+	    TP_ARGS(crtc),
+
+	    TP_STRUCT__entry(
+			     __field(enum pipe, pipe)
+			     __field(u32, frame)
+			     __field(u32, scanline)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->pipe = crtc->pipe;
+			   __entry->frame = crtc->base.dev->driver->get_vblank_counter(crtc->base.dev,
+										       crtc->pipe);
+			   __entry->scanline = intel_get_crtc_scanline(&crtc->base);
+			   ),
+
+	    TP_printk("pipe %c, frame=%u, scanline=%u",
+		      pipe_name(__entry->pipe), __entry->frame,
+		      __entry->scanline)
+);
+
 /* object tracking */
 
 TRACE_EVENT(i915_gem_object_create,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 8bf3be8..47e1721 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -64,6 +64,8 @@ static void intel_pipe_update_start(struct intel_crtc *crtc)
 
 	local_irq_disable();
 
+	trace_i915_pipe_update_start(crtc, min, max);
+
 	/*
 	 * Explicit compiler barrier is used to make sure that vbl_received
 	 * store happens before reading the current scanline, otherwise
@@ -102,10 +104,14 @@ static void intel_pipe_update_start(struct intel_crtc *crtc)
 	}
 
 	drm_vblank_put(dev, pipe);
+
+	trace_i915_pipe_update_vblank_evaded(crtc, min, max);
 }
 
 static void intel_pipe_update_end(struct intel_crtc *crtc)
 {
+	trace_i915_pipe_update_end(crtc);
+
 	local_irq_enable();
 	preempt_check_resched();
 }
-- 
1.8.3.2

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