[PATCH 0/7] drm/i915: Atomic sprites

[PATCH 0/7] drm/i915: Atomic sprites

[ville.syrjala]

Today I heard some grumblings about atomic updates again, so I decided
to move the thing forward a bit. This series only makes sprite
updates + the accompanying primary enable/disable happen atomically.
But it's a decent baby step towards full atomic updates. At least we
would get the vblank evade mechanism already into place if we push
this in.

The first two patches are just fixes, although the ILK sprite fix
doesn't help much since we don't handle sprite watermarks on ILK
at all. That means ILK sprites are pretty much unusable at the moment.
Perhaps we could just call sandybridge_update_sprite_wm() on ILK, but I'm
too lazy to double check it since I'm anyway going to nuke it soon.

Ville Syrjälä (7):
      drm/i915: Don't disable primary when color keying is used
      drm/i915: Fix non-scaled sprites for ILK
      drm/i915: Add i915_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      |  80 ++++++++++----
 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  | 203 ++++++++++++++++++++++++++---------
 5 files changed, 297 insertions(+), 69 deletions(-)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/7] drm/i915: Don't disable primary when color keying is used

[ville.syrjala]

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

When color keying is used, the primary may not be invisible even though
the sprite fully covers it. So check for color keying before deciding to
disable the primary plane.

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

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 07b13dc..74f6bd4 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -643,6 +643,15 @@ format_is_yuv(uint32_t format)
 	}
 }
 
+static bool colorkey_enabled(struct intel_plane *intel_plane)
+{
+	struct drm_intel_sprite_colorkey key;
+
+	intel_plane->get_colorkey(&intel_plane->base, &key);
+
+	return key.flags != I915_SET_COLORKEY_NONE;
+}
+
 static int
 intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		   struct drm_framebuffer *fb, int crtc_x, int crtc_y,
@@ -828,7 +837,7 @@ 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);
+	disable_primary = drm_rect_equals(&dst, &clip) && !colorkey_enabled(intel_plane);
 	WARN_ON(disable_primary && !visible && intel_crtc->active);
 
 	mutex_lock(&dev->struct_mutex);
-- 
1.8.1.5

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

[PATCH 2/7] drm/i915: Fix non-scaled sprites for ILK

[ville.syrjala]

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

For some reason we're enabling sprite scaling on ILK always. That
doesn't work well with the new watermark code that expects to use
LP1 watermarks with unscaled sprites.

Only enable sprite scaling on ILK when actually needed, just like
we do on SNB.

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

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 74f6bd4..0aa5911 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -470,7 +470,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	crtc_h--;
 
 	dvsscale = 0;
-	if (IS_GEN5(dev) || crtc_w != src_w || crtc_h != src_h)
+	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]);
-- 
1.8.1.5

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

[PATCH 3/7] drm/i915: Add i915_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.

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, 33 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 6b379de..49bcf4e 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -646,8 +646,8 @@ static bool intel_pipe_in_vblank(struct drm_device *dev, enum pipe pipe)
 	}
 }
 
-static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
-			     int *vpos, int *hpos)
+int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
+			     int *vpos, int *hpos, bool adjust)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
@@ -704,18 +704,20 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 		vtotal *= htotal;
 	}
 
-	in_vbl = position >= vbl_start && position < vbl_end;
+	if (adjust) {
+		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;
+		/*
+		 * 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;
@@ -732,6 +734,22 @@ 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)
+{
+	return i915_get_crtc_scanoutpos(dev, pipe, vpos, hpos, true);
+}
+
+int i915_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, false);
+
+	return vpos;
+}
+
 static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
 			      int *max_error,
 			      struct timeval *vblank_time,
@@ -3313,7 +3331,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 e33f387..7f5f74d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -580,6 +580,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 i915_get_crtc_scanline(struct drm_crtc *crtc);
 
 
 /* intel_crt.c */
-- 
1.8.1.5

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

[PATCH 4/7] 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.

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 0aa5911..c160e4f 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -118,9 +118,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,
@@ -128,6 +125,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
@@ -295,15 +295,15 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	} else
 		dev_priv->sprite_scaling_enabled &= ~(1 << pipe);
 
-	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))
@@ -473,15 +473,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.1.5

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

[PATCH 5/7] 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.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c      | 34 ++++++++++++++---
 drivers/gpu/drm/i915/intel_display.c |  2 +
 drivers/gpu/drm/i915/intel_drv.h     |  3 ++
 drivers/gpu/drm/i915/intel_sprite.c  | 72 ++++++++++++++++++++++++++++++++++++
 4 files changed, 105 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 49bcf4e..776fe2f 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1271,6 +1271,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;
@@ -1313,8 +1322,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_VBLANK_INTERRUPT_STATUS)
+			if (pipe_stats[pipe] & PIPE_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);
@@ -1509,11 +1520,15 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
 	if (de_iir & DE_GSE)
 		intel_opregion_asle_intr(dev);
 
-	if (de_iir & DE_PIPEA_VBLANK)
-		drm_handle_vblank(dev, 0);
+	if (de_iir & DE_PIPEA_VBLANK) {
+		drm_handle_vblank(dev, PIPE_A);
+		intel_pipe_handle_vblank(dev, PIPE_A);
+	}
 
-	if (de_iir & DE_PIPEB_VBLANK)
-		drm_handle_vblank(dev, 1);
+	if (de_iir & DE_PIPEB_VBLANK) {
+		drm_handle_vblank(dev, PIPE_B);
+		intel_pipe_handle_vblank(dev, PIPE_B);
+	}
 
 	if (de_iir & DE_POISON)
 		DRM_ERROR("Poison interrupt\n");
@@ -1568,8 +1583,11 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
 		intel_opregion_asle_intr(dev);
 
 	for (i = 0; i < 3; i++) {
-		if (de_iir & (DE_PIPEA_VBLANK_IVB << (5 * i)))
+		if (de_iir & (DE_PIPEA_VBLANK_IVB << (5 * i))) {
 			drm_handle_vblank(dev, i);
+			intel_pipe_handle_vblank(dev, i);
+		}
+
 		if (de_iir & (DE_PLANEA_FLIP_DONE_IVB << (5 * i))) {
 			intel_prepare_page_flip(dev, i);
 			intel_finish_page_flip_plane(dev, i);
@@ -2695,6 +2713,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;
 
@@ -2869,6 +2889,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 be056fd..34eb952 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9754,6 +9754,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 7f5f74d..83cb2a0 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -361,6 +361,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 c160e4f..ce4e4ec 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -37,6 +37,54 @@
 #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 = i915_get_crtc_scanline(crtc);
+
+	while (scanline >= min && scanline <= max && timeout > 0) {
+		local_irq_enable();
+		timeout = wait_event_timeout(intel_crtc->vbl_wait,
+					     intel_crtc->vbl_received,
+					     timeout);
+		local_irq_disable();
+
+		intel_crtc->vbl_received = false;
+		scanline = i915_get_crtc_scanline(crtc);
+	}
+
+	drm_vblank_put(dev, pipe);
+}
+
+static void intel_pipe_update_end(struct drm_crtc *crtc)
+{
+	local_irq_enable();
+}
+
 static void
 vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 		 struct drm_framebuffer *fb,
@@ -125,6 +173,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);
 
@@ -138,6 +188,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
@@ -149,12 +201,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);
 }
 
@@ -301,6 +357,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);
 
@@ -321,6 +379,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 			     i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
 	POSTING_READ(SPRSURF(pipe));
 
+	intel_pipe_update_end(crtc);
+
 	/* potentially re-enable LP watermarks */
 	if (scaling_was_enabled && !dev_priv->sprite_scaling_enabled)
 		intel_update_watermarks(crtc);
@@ -335,6 +395,8 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	int pipe = intel_plane->pipe;
 	bool scaling_was_enabled = dev_priv->sprite_scaling_enabled;
 
+	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)
@@ -343,6 +405,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);
+
 	dev_priv->sprite_scaling_enabled &= ~(1 << pipe);
 
 	intel_update_sprite_watermarks(plane, crtc, 0, 0, false, false);
@@ -479,6 +543,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);
 
@@ -493,6 +559,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
@@ -503,6 +571,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);
@@ -510,6 +580,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);
+
 	intel_update_sprite_watermarks(plane, crtc, 0, 0, false, false);
 }
 
-- 
1.8.1.5

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

[PATCH 6/7] 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.

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 ce4e4ec..f871b8f 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -85,6 +85,19 @@ static void intel_pipe_update_end(struct drm_crtc *crtc)
 	local_irq_enable();
 }
 
+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,
@@ -175,6 +188,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);
 
@@ -187,7 +202,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);
 }
@@ -203,11 +219,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);
 
@@ -359,6 +378,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);
 
@@ -377,7 +398,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);
 
@@ -397,13 +419,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);
 
@@ -545,6 +570,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);
 
@@ -558,7 +585,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);
 }
@@ -573,12 +601,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);
 
@@ -586,20 +617,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
@@ -618,17 +639,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)
@@ -642,9 +657,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
@@ -738,7 +750,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;
@@ -909,8 +921,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);
 
@@ -937,12 +949,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,
@@ -951,8 +963,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 */
@@ -990,8 +1002,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.1.5

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

[PATCH 7/7] 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.

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..9ffb232 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 = i915_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 = i915_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 = i915_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 f871b8f..874ffc1 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 = i915_get_crtc_scanline(crtc);
 
@@ -78,10 +80,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();
 }
 
-- 
1.8.1.5

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

Re: [PATCH 2/7] drm/i915: Fix non-scaled sprites for ILK

[Chris Wilson]

On Thu, Oct 17, 2013 at 10:53:14PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> For some reason we're enabling sprite scaling on ILK always. That
> doesn't work well with the new watermark code that expects to use
> LP1 watermarks with unscaled sprites.
> 
> Only enable sprite scaling on ILK when actually needed, just like
> we do on SNB.

The sprite plane simply didn't work on my ilk machine unless I
enabled the scaler. If you want to revert it, at least mention which
patch you are reverting with justification/demonstration that it works
now.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/7] drm/i915: Fix non-scaled sprites for ILK

[Ville Syrjälä]

On Thu, Oct 17, 2013 at 09:56:03PM +0100, Chris Wilson wrote:
> On Thu, Oct 17, 2013 at 10:53:14PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > For some reason we're enabling sprite scaling on ILK always. That
> > doesn't work well with the new watermark code that expects to use
> > LP1 watermarks with unscaled sprites.
> > 
> > Only enable sprite scaling on ILK when actually needed, just like
> > we do on SNB.
> 
> The sprite plane simply didn't work on my ilk machine unless I
> enabled the scaler. If you want to revert it, at least mention which
> patch you are reverting with justification/demonstration that it works
> now.

Hmm. Oh I see it actually does work when the scaler is enabled. I'm
going to assume the hardware automagically disables LP1+ watermarks in
that case or something, and then it just happens to work even though we
don't set up the watermarks correctly. Earlier I was trying with a >1k
wide 32bpp source w/o scaling, so it exceeded the scaler limits
(width_bytes > 4096) but since the code that does the check didn't
realize scaling was actually enabled it let it through and the
hardware didn't like that very much.

So since it kind of works currently with smaller source widths, I'm
going to leave it alone until I send out the watermark changes. And
I'll pimp up the commit message at that point.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/7] drm/i915: Don't disable primary when color keying is used

[Jesse Barnes]

On Thu, 17 Oct 2013 22:53:13 +0300
ville.syrjala@linux.intel.com wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> When color keying is used, the primary may not be invisible even though
> the sprite fully covers it. So check for color keying before deciding to
> disable the primary plane.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 07b13dc..74f6bd4 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -643,6 +643,15 @@ format_is_yuv(uint32_t format)
>  	}
>  }
>  
> +static bool colorkey_enabled(struct intel_plane *intel_plane)
> +{
> +	struct drm_intel_sprite_colorkey key;
> +
> +	intel_plane->get_colorkey(&intel_plane->base, &key);
> +
> +	return key.flags != I915_SET_COLORKEY_NONE;
> +}
> +
>  static int
>  intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  		   struct drm_framebuffer *fb, int crtc_x, int crtc_y,
> @@ -828,7 +837,7 @@ 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);
> +	disable_primary = drm_rect_equals(&dst, &clip) && !colorkey_enabled(intel_plane);
>  	WARN_ON(disable_primary && !visible && intel_crtc->active);
>  
>  	mutex_lock(&dev->struct_mutex);

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/7] drm/i915: Don't disable primary when color keying is used

[Daniel Vetter]

On Thu, Dec 12, 2013 at 9:56 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Thu, 17 Oct 2013 22:53:13 +0300
> ville.syrjala@linux.intel.com wrote:
>
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> When color keying is used, the primary may not be invisible even though
>> the sprite fully covers it. So check for color keying before deciding to
>> disable the primary plane.
>>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_sprite.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>> index 07b13dc..74f6bd4 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -643,6 +643,15 @@ format_is_yuv(uint32_t format)
>>       }
>>  }
>>
>> +static bool colorkey_enabled(struct intel_plane *intel_plane)
>> +{
>> +     struct drm_intel_sprite_colorkey key;
>> +
>> +     intel_plane->get_colorkey(&intel_plane->base, &key);
>> +
>> +     return key.flags != I915_SET_COLORKEY_NONE;
>> +}
>> +
>>  static int
>>  intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>>                  struct drm_framebuffer *fb, int crtc_x, int crtc_y,
>> @@ -828,7 +837,7 @@ 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);
>> +     disable_primary = drm_rect_equals(&dst, &clip) && !colorkey_enabled(intel_plane);
>>       WARN_ON(disable_primary && !visible && intel_crtc->active);
>>
>>       mutex_lock(&dev->struct_mutex);
>
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Ready your canisters with gallons full of sore acid and raw hatred,
maintainer party-crasher coming in: Where's the igt?

Should be fairly simple to exercise with those fancy CRCs we have ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/7] drm/i915: Add i915_get_crtc_scanline()

[Jesse Barnes]

On Thu, 17 Oct 2013 22:53:15 +0300
ville.syrjala@linux.intel.com wrote:

> 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.
> 
> 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, 33 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 6b379de..49bcf4e 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -646,8 +646,8 @@ static bool intel_pipe_in_vblank(struct drm_device *dev, enum pipe pipe)
>  	}
>  }
>  
> -static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
> -			     int *vpos, int *hpos)
> +int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
> +			     int *vpos, int *hpos, bool adjust)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> @@ -704,18 +704,20 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
>  		vtotal *= htotal;
>  	}
>  
> -	in_vbl = position >= vbl_start && position < vbl_end;
> +	if (adjust) {
> +		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;
> +		/*
> +		 * 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;
> @@ -732,6 +734,22 @@ 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)
> +{
> +	return i915_get_crtc_scanoutpos(dev, pipe, vpos, hpos, true);
> +}
> +
> +int i915_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, false);
> +
> +	return vpos;
> +}
> +
>  static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
>  			      int *max_error,
>  			      struct timeval *vblank_time,
> @@ -3313,7 +3331,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 e33f387..7f5f74d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -580,6 +580,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 i915_get_crtc_scanline(struct drm_crtc *crtc);
>  
>  
>  /* intel_crt.c */

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] drm/i915: Shuffle sprite register writes into a tighter group

[Jesse Barnes]

On Thu, 17 Oct 2013 22:53:16 +0300
ville.syrjala@linux.intel.com wrote:

> 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.
> 
> 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 0aa5911..c160e4f 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -118,9 +118,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,
> @@ -128,6 +125,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
> @@ -295,15 +295,15 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	} else
>  		dev_priv->sprite_scaling_enabled &= ~(1 << pipe);
>  
> -	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))
> @@ -473,15 +473,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

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jesse Barnes]

On Thu, 17 Oct 2013 22:53:17 +0300
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.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c      | 34 ++++++++++++++---
>  drivers/gpu/drm/i915/intel_display.c |  2 +
>  drivers/gpu/drm/i915/intel_drv.h     |  3 ++
>  drivers/gpu/drm/i915/intel_sprite.c  | 72 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 105 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 49bcf4e..776fe2f 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1271,6 +1271,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;
> @@ -1313,8 +1322,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_VBLANK_INTERRUPT_STATUS)
> +			if (pipe_stats[pipe] & PIPE_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);
> @@ -1509,11 +1520,15 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
>  	if (de_iir & DE_GSE)
>  		intel_opregion_asle_intr(dev);
>  
> -	if (de_iir & DE_PIPEA_VBLANK)
> -		drm_handle_vblank(dev, 0);
> +	if (de_iir & DE_PIPEA_VBLANK) {
> +		drm_handle_vblank(dev, PIPE_A);
> +		intel_pipe_handle_vblank(dev, PIPE_A);
> +	}
>  
> -	if (de_iir & DE_PIPEB_VBLANK)
> -		drm_handle_vblank(dev, 1);
> +	if (de_iir & DE_PIPEB_VBLANK) {
> +		drm_handle_vblank(dev, PIPE_B);
> +		intel_pipe_handle_vblank(dev, PIPE_B);
> +	}
>  
>  	if (de_iir & DE_POISON)
>  		DRM_ERROR("Poison interrupt\n");
> @@ -1568,8 +1583,11 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
>  		intel_opregion_asle_intr(dev);
>  
>  	for (i = 0; i < 3; i++) {
> -		if (de_iir & (DE_PIPEA_VBLANK_IVB << (5 * i)))
> +		if (de_iir & (DE_PIPEA_VBLANK_IVB << (5 * i))) {
>  			drm_handle_vblank(dev, i);
> +			intel_pipe_handle_vblank(dev, i);
> +		}
> +
>  		if (de_iir & (DE_PLANEA_FLIP_DONE_IVB << (5 * i))) {
>  			intel_prepare_page_flip(dev, i);
>  			intel_finish_page_flip_plane(dev, i);
> @@ -2695,6 +2713,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;
>  
> @@ -2869,6 +2889,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 be056fd..34eb952 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9754,6 +9754,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 7f5f74d..83cb2a0 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -361,6 +361,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 c160e4f..ce4e4ec 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -37,6 +37,54 @@
>  #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 = i915_get_crtc_scanline(crtc);
> +
> +	while (scanline >= min && scanline <= max && timeout > 0) {
> +		local_irq_enable();
> +		timeout = wait_event_timeout(intel_crtc->vbl_wait,
> +					     intel_crtc->vbl_received,
> +					     timeout);
> +		local_irq_disable();
> +
> +		intel_crtc->vbl_received = false;
> +		scanline = i915_get_crtc_scanline(crtc);
> +	}
> +
> +	drm_vblank_put(dev, pipe);
> +}
> +
> +static void intel_pipe_update_end(struct drm_crtc *crtc)
> +{
> +	local_irq_enable();
> +}
> +
>  static void
>  vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>  		 struct drm_framebuffer *fb,
> @@ -125,6 +173,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);
>  
> @@ -138,6 +188,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
> @@ -149,12 +201,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);
>  }
>  
> @@ -301,6 +357,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);
>  
> @@ -321,6 +379,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  			     i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
>  	POSTING_READ(SPRSURF(pipe));
>  
> +	intel_pipe_update_end(crtc);
> +
>  	/* potentially re-enable LP watermarks */
>  	if (scaling_was_enabled && !dev_priv->sprite_scaling_enabled)
>  		intel_update_watermarks(crtc);
> @@ -335,6 +395,8 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>  	int pipe = intel_plane->pipe;
>  	bool scaling_was_enabled = dev_priv->sprite_scaling_enabled;
>  
> +	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)
> @@ -343,6 +405,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);
> +
>  	dev_priv->sprite_scaling_enabled &= ~(1 << pipe);
>  
>  	intel_update_sprite_watermarks(plane, crtc, 0, 0, false, false);
> @@ -479,6 +543,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);
>  
> @@ -493,6 +559,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
> @@ -503,6 +571,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);
> @@ -510,6 +580,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);
> +
>  	intel_update_sprite_watermarks(plane, crtc, 0, 0, false, false);
>  }
>  

It looks like we may need to check for preemption again on
local_irq_enable() according to preempt-locking.txt?  Otherwise it
looks like this does the right thing.

With the above addressed or explained away:
Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/7] drm/i915: Perform primary enable/disable atomically with sprite updates

[Jesse Barnes]

On Thu, 17 Oct 2013 22:53:18 +0300
ville.syrjala@linux.intel.com wrote:

> 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.
> 
> 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 ce4e4ec..f871b8f 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -85,6 +85,19 @@ static void intel_pipe_update_end(struct drm_crtc *crtc)
>  	local_irq_enable();
>  }
>  
> +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,
> @@ -175,6 +188,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);
>  
> @@ -187,7 +202,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);
>  }
> @@ -203,11 +219,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);
>  
> @@ -359,6 +378,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);
>  
> @@ -377,7 +398,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);
>  
> @@ -397,13 +419,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);
>  
> @@ -545,6 +570,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);
>  
> @@ -558,7 +585,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);
>  }
> @@ -573,12 +601,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);
>  
> @@ -586,20 +617,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
> @@ -618,17 +639,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)
> @@ -642,9 +657,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
> @@ -738,7 +750,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;
> @@ -909,8 +921,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);
>  
> @@ -937,12 +949,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,
> @@ -951,8 +963,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 */
> @@ -990,8 +1002,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) {

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/7] drm/i915: Add pipe update trace points

[Jesse Barnes]

On Thu, 17 Oct 2013 22:53:19 +0300
ville.syrjala@linux.intel.com wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Add trace points for observing the atomic pipe update mechanism.
> 
> 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..9ffb232 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 = i915_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 = i915_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 = i915_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 f871b8f..874ffc1 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 = i915_get_crtc_scanline(crtc);
>  
> @@ -78,10 +80,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();
>  }
>  

Not sure about the "vblank_evaded" one.  Seems like you'd have enough
info from the start/end to figure out if you missed, since you'd have
the time and vblank seqno, right?   But that's really no biggie.

It does make me think we should start documenting these events though...

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jesse Barnes]

On Thu, 17 Oct 2013 22:53:17 +0300
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.

Oh and like I mentioned on irc, we need to remove the blocking vblank
wait from the fb switch case.  We've had patches for that almost since
the sprite bits went in, but for some reason they've never been
merged.  They're need to make the sprite stuff more usable by e.g.
Weston.

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/7] drm/i915: Don't disable primary when color keying is used

[Ville Syrjälä]

On Thu, Dec 12, 2013 at 09:58:25PM +0100, Daniel Vetter wrote:
> On Thu, Dec 12, 2013 at 9:56 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > On Thu, 17 Oct 2013 22:53:13 +0300
> > ville.syrjala@linux.intel.com wrote:
> >
> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>
> >> When color keying is used, the primary may not be invisible even though
> >> the sprite fully covers it. So check for color keying before deciding to
> >> disable the primary plane.
> >>
> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_sprite.c | 11 ++++++++++-
> >>  1 file changed, 10 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> >> index 07b13dc..74f6bd4 100644
> >> --- a/drivers/gpu/drm/i915/intel_sprite.c
> >> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> >> @@ -643,6 +643,15 @@ format_is_yuv(uint32_t format)
> >>       }
> >>  }
> >>
> >> +static bool colorkey_enabled(struct intel_plane *intel_plane)
> >> +{
> >> +     struct drm_intel_sprite_colorkey key;
> >> +
> >> +     intel_plane->get_colorkey(&intel_plane->base, &key);
> >> +
> >> +     return key.flags != I915_SET_COLORKEY_NONE;
> >> +}
> >> +
> >>  static int
> >>  intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >>                  struct drm_framebuffer *fb, int crtc_x, int crtc_y,
> >> @@ -828,7 +837,7 @@ 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);
> >> +     disable_primary = drm_rect_equals(&dst, &clip) && !colorkey_enabled(intel_plane);
> >>       WARN_ON(disable_primary && !visible && intel_crtc->active);
> >>
> >>       mutex_lock(&dev->struct_mutex);
> >
> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> Ready your canisters with gallons full of sore acid and raw hatred,
> maintainer party-crasher coming in: Where's the igt?
> 
> Should be fairly simple to exercise with those fancy CRCs we have ...

Whoever ends up writing the basic plane crc tests gets to add some
colorkey tests too. But this fixes a real bug, so my preference
would to to apply it asap, test or no test.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 7/7] drm/i915: Add pipe update trace points

[Ville Syrjälä]

On Thu, Dec 12, 2013 at 01:08:16PM -0800, Jesse Barnes wrote:
> On Thu, 17 Oct 2013 22:53:19 +0300
> ville.syrjala@linux.intel.com wrote:
> 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Add trace points for observing the atomic pipe update mechanism.
> > 
> > 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..9ffb232 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 = i915_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 = i915_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 = i915_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 f871b8f..874ffc1 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 = i915_get_crtc_scanline(crtc);
> >  
> > @@ -78,10 +80,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();
> >  }
> >  
> 
> Not sure about the "vblank_evaded" one.  Seems like you'd have enough
> info from the start/end to figure out if you missed, since you'd have
> the time and vblank seqno, right?   But that's really no biggie.

The evaded tracepoint is quite important since it allows us to verify
that the evaded->end sequence really took place during one frame.
start->end doesn't really tell us much because when the vblank gets
evaded those two tracepoints land on alternate frames.

> 
> It does make me think we should start documenting these events though...
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> -- 
> Jesse Barnes, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC