[PATCH 00/12] drm/i915: Some cleanups and fixes to the sprite code v2

[PATCH 00/12] drm/i915: Some cleanups and fixes to the sprite code v2

[ville.syrjala]

Chris asked for some renames and assertions during v1. While adding those I
noticed that what I did in the original patch 02 didn't match quite so well
with the assertions. So I modified patch 02 a bit, and that caused quite a bit
of bit of rebase issues for most of the other patches, so I figured it's better
to repost the whole thing.

Changes from v1:
- Move the primary disable/enable calls inside intel_crtc->active checks
  in intel_update_plane/intel_disable_plane. That also ate up patch 03 from
  the original series.
- Add primary_disabled WARNs
- Rename primary plane funcs
- Flush primary plane changes from sprite code
- Add a POSTING_READ() to intel_flush_primary_plane. This shouldn't really
  be necessary now that I think about it some more. So we might want to drop
  that change...

[PATCH 01/12] drm/i915: Set primary_disabled in intel_{enable, disable}_plane

[ville.syrjala]

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

If the primary gets marked as disabled while the pipe is off for
instance, we should still re-enable it when the pipe is turned on,
unless the sprite covers it fully also in that configuration.
Unfortunately we do the plane visibility checks only in the sprite code,
which is executed after the primary enabling when turning the pipe off.

Ideally we should compute the plane visibility before touching the
hardware at all, but for now just set the primary_disabld flag
in intel_{enable,disable}_plane.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 29b9387..261e043 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1809,12 +1809,16 @@ void intel_flush_display_plane(struct drm_i915_private *dev_priv,
 static void intel_enable_plane(struct drm_i915_private *dev_priv,
 			       enum plane plane, enum pipe pipe)
 {
+	struct intel_crtc *intel_crtc =
+		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
 	int reg;
 	u32 val;
 
 	/* If the pipe isn't enabled, we can't pump pixels and may hang */
 	assert_pipe_enabled(dev_priv, pipe);
 
+	intel_crtc->primary_disabled = false;
+
 	reg = DSPCNTR(plane);
 	val = I915_READ(reg);
 	if (val & DISPLAY_PLANE_ENABLE)
@@ -1836,9 +1840,13 @@ static void intel_enable_plane(struct drm_i915_private *dev_priv,
 static void intel_disable_plane(struct drm_i915_private *dev_priv,
 				enum plane plane, enum pipe pipe)
 {
+	struct intel_crtc *intel_crtc =
+		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
 	int reg;
 	u32 val;
 
+	intel_crtc->primary_disabled = true;
+
 	reg = DSPCNTR(plane);
 	val = I915_READ(reg);
 	if ((val & DISPLAY_PLANE_ENABLE) == 0)
-- 
1.8.1.5

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

[PATCH v2 02/12] drm/i915: Allow sprites to be configured on a disabled pipe

[ville.syrjala]

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

We allow cursors to be set up when the pipe is disabled. Do the same for
sprites as well.

We need to be somewhat careful with the primary disable logic as we
don't want to accidentally enable the primary plane on a disabled pipe.

v2: Skip primary enable/disable and plane registers
    writes on disabled pipe

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

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index cae10bc..9161a1d 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -623,14 +623,10 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		   uint32_t src_w, uint32_t src_h)
 {
 	struct drm_device *dev = plane->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct intel_framebuffer *intel_fb;
 	struct drm_i915_gem_object *obj, *old_obj;
-	int pipe = intel_plane->pipe;
-	enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
-								      pipe);
 	int ret = 0;
 	bool disable_primary = false;
 	bool visible;
@@ -652,8 +648,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		.y2 = crtc_y + crtc_h,
 	};
 	const struct drm_rect clip = {
-		.x2 = intel_crtc->config.pipe_src_w,
-		.y2 = intel_crtc->config.pipe_src_h,
+		.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
+		.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
 	};
 
 	intel_fb = to_intel_framebuffer(fb);
@@ -670,12 +666,6 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	intel_plane->src_w = src_w;
 	intel_plane->src_h = src_h;
 
-	/* Pipe must be running... */
-	if (!(I915_READ(PIPECONF(cpu_transcoder)) & PIPECONF_ENABLE)) {
-		DRM_DEBUG_KMS("Pipe disabled\n");
-		return -EINVAL;
-	}
-
 	/* Don't modify another pipe's plane */
 	if (intel_plane->pipe != intel_crtc->pipe) {
 		DRM_DEBUG_KMS("Wrong plane <-> crtc mapping\n");
@@ -810,7 +800,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	 * we can disable the primary and save power.
 	 */
 	disable_primary = drm_rect_equals(&dst, &clip);
-	WARN_ON(disable_primary && !visible);
+	WARN_ON(disable_primary && !visible && intel_crtc->active);
 
 	mutex_lock(&dev->struct_mutex);
 
@@ -825,22 +815,24 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 
 	intel_plane->obj = obj;
 
-	/*
-	 * Be sure to re-enable the primary before the sprite is no longer
-	 * covering it fully.
-	 */
-	if (!disable_primary)
-		intel_enable_primary(crtc);
-
-	if (visible)
-		intel_plane->update_plane(plane, crtc, fb, obj,
-					  crtc_x, crtc_y, crtc_w, crtc_h,
-					  src_x, src_y, src_w, src_h);
-	else
-		intel_plane->disable_plane(plane, crtc);
-
-	if (disable_primary)
-		intel_disable_primary(crtc);
+	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);
+
+		if (visible)
+			intel_plane->update_plane(plane, crtc, fb, obj,
+						  crtc_x, crtc_y, crtc_w, crtc_h,
+						  src_x, src_y, src_w, src_h);
+		else
+			intel_plane->disable_plane(plane, crtc);
+
+		if (disable_primary)
+			intel_disable_primary(crtc);
+	}
 
 	/* Unpin old obj after new one is active to avoid ugliness */
 	if (old_obj) {
@@ -852,7 +844,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		 */
 		if (old_obj != obj) {
 			mutex_unlock(&dev->struct_mutex);
-			intel_wait_for_vblank(dev, to_intel_crtc(crtc)->pipe);
+			if (intel_crtc->active)
+				intel_wait_for_vblank(dev, to_intel_crtc(crtc)->pipe);
 			mutex_lock(&dev->struct_mutex);
 		}
 		intel_unpin_fb_obj(old_obj);
@@ -868,6 +861,7 @@ intel_disable_plane(struct drm_plane *plane)
 {
 	struct drm_device *dev = plane->dev;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc;
 	int ret = 0;
 
 	if (!plane->fb)
@@ -876,13 +870,18 @@ intel_disable_plane(struct drm_plane *plane)
 	if (WARN_ON(!plane->crtc))
 		return -EINVAL;
 
-	intel_enable_primary(plane->crtc);
-	intel_plane->disable_plane(plane, plane->crtc);
+	intel_crtc = to_intel_crtc(plane->crtc);
+
+	if (intel_crtc->active) {
+		intel_enable_primary(plane->crtc);
+		intel_plane->disable_plane(plane, plane->crtc);
+	}
 
 	if (!intel_plane->obj)
 		goto out;
 
-	intel_wait_for_vblank(dev, intel_plane->pipe);
+	if (intel_crtc->active)
+		intel_wait_for_vblank(dev, intel_plane->pipe);
 
 	mutex_lock(&dev->struct_mutex);
 	intel_unpin_fb_obj(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 03/12] drm/i915: Reduce the time we hold struct mutex in sprite update_plane code

[ville.syrjala]

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

We used to call the entire intel specific update_plane hook while
holding struct_mutex. Actually we only need to hold struct_mutex while
pinning/unpinning the obj. The plane state itself is protected by the
kms locks, and as the object is pinned we can dig out the offset and
tiling information from it without fearing that it would change
underneath us.

So now we don't need to drop and reacquire the lock around the
wait_for_vblank. Also we will need another wait_for_vblank in the IVB
specific update_plane hook, and this way we don't need to worry about
struct_mutex there either.

Also move the intel_plane->obj=NULL assignment outside strut_mutex in
disable_plane to make it clear that it's not protected by struct_mutex.

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

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 9161a1d..b859f94 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -525,7 +525,10 @@ intel_enable_primary(struct drm_crtc *crtc)
 		return;
 
 	intel_crtc->primary_disabled = false;
+
+	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
+	mutex_unlock(&dev->struct_mutex);
 
 	I915_WRITE(reg, I915_READ(reg) | DISPLAY_PLANE_ENABLE);
 }
@@ -544,7 +547,10 @@ intel_disable_primary(struct drm_crtc *crtc)
 	I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE);
 
 	intel_crtc->primary_disabled = true;
+
+	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
+	mutex_unlock(&dev->struct_mutex);
 }
 
 static int
@@ -810,8 +816,11 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	 * the sprite planes only require 128KiB alignment and 32 PTE padding.
 	 */
 	ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
+
+	mutex_unlock(&dev->struct_mutex);
+
 	if (ret)
-		goto out_unlock;
+		return ret;
 
 	intel_plane->obj = obj;
 
@@ -842,18 +851,15 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		 * wait for vblank to avoid ugliness, we only need to
 		 * do the pin & ref bookkeeping.
 		 */
-		if (old_obj != obj) {
-			mutex_unlock(&dev->struct_mutex);
-			if (intel_crtc->active)
-				intel_wait_for_vblank(dev, to_intel_crtc(crtc)->pipe);
-			mutex_lock(&dev->struct_mutex);
-		}
+		if (old_obj != obj && intel_crtc->active)
+			intel_wait_for_vblank(dev, to_intel_crtc(crtc)->pipe);
+
+		mutex_lock(&dev->struct_mutex);
 		intel_unpin_fb_obj(old_obj);
+		mutex_unlock(&dev->struct_mutex);
 	}
 
-out_unlock:
-	mutex_unlock(&dev->struct_mutex);
-	return ret;
+	return 0;
 }
 
 static int
@@ -885,8 +891,9 @@ intel_disable_plane(struct drm_plane *plane)
 
 	mutex_lock(&dev->struct_mutex);
 	intel_unpin_fb_obj(intel_plane->obj);
-	intel_plane->obj = NULL;
 	mutex_unlock(&dev->struct_mutex);
+
+	intel_plane->obj = NULL;
 out:
 
 	return ret;
-- 
1.8.1.5

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

[PATCH 04/12] drm/i915: Kill a goto from sprite disable code

[ville.syrjala]

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

Let's not use goto when a simple if suffices. This is not error handling
code or anything, so the goto looks out of place.

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

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index b859f94..b5e30b1 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -868,7 +868,6 @@ intel_disable_plane(struct drm_plane *plane)
 	struct drm_device *dev = plane->dev;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct intel_crtc *intel_crtc;
-	int ret = 0;
 
 	if (!plane->fb)
 		return 0;
@@ -883,20 +882,18 @@ intel_disable_plane(struct drm_plane *plane)
 		intel_plane->disable_plane(plane, plane->crtc);
 	}
 
-	if (!intel_plane->obj)
-		goto out;
-
-	if (intel_crtc->active)
-		intel_wait_for_vblank(dev, intel_plane->pipe);
+	if (intel_plane->obj) {
+		if (intel_crtc->active)
+			intel_wait_for_vblank(dev, intel_plane->pipe);
 
-	mutex_lock(&dev->struct_mutex);
-	intel_unpin_fb_obj(intel_plane->obj);
-	mutex_unlock(&dev->struct_mutex);
+		mutex_lock(&dev->struct_mutex);
+		intel_unpin_fb_obj(intel_plane->obj);
+		mutex_unlock(&dev->struct_mutex);
 
-	intel_plane->obj = NULL;
-out:
+		intel_plane->obj = NULL;
+	}
 
-	return ret;
+	return 0;
 }
 
 static void intel_destroy_plane(struct drm_plane *plane)
-- 
1.8.1.5

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

[PATCH 05/12] drm/i915: Do a bit of cleanup in the sprite code

[ville.syrjala]

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

Move the variable initialization to where the variables are declared,
and kill a pointless to_intel_crtc() cast when we already have the
casted pointer.

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

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index b5e30b1..549243a 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -631,9 +631,10 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = plane->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
-	struct intel_framebuffer *intel_fb;
-	struct drm_i915_gem_object *obj, *old_obj;
-	int ret = 0;
+	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
+	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 visible;
 	int hscale, vscale;
@@ -658,11 +659,6 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
 	};
 
-	intel_fb = to_intel_framebuffer(fb);
-	obj = intel_fb->obj;
-
-	old_obj = intel_plane->obj;
-
 	intel_plane->crtc_x = crtc_x;
 	intel_plane->crtc_y = crtc_y;
 	intel_plane->crtc_w = crtc_w;
@@ -852,7 +848,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		 * do the pin & ref bookkeeping.
 		 */
 		if (old_obj != obj && intel_crtc->active)
-			intel_wait_for_vblank(dev, to_intel_crtc(crtc)->pipe);
+			intel_wait_for_vblank(dev, intel_crtc->pipe);
 
 		mutex_lock(&dev->struct_mutex);
 		intel_unpin_fb_obj(old_obj);
-- 
1.8.1.5

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

[PATCH 06/12] drm/i915: Save user requested plane coordinates only on success

[ville.syrjala]

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

If the setplane operation fails, we shouldn't save the user's requested
plane coordinates. Since we adjust the coordinates during the clipping
process, make a copy of the originals, and once the operation has
succeeded save them for later reuse when the plane gets re-enabled.

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

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 549243a..276c3a6 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -658,15 +658,20 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
 		.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
 	};
-
-	intel_plane->crtc_x = crtc_x;
-	intel_plane->crtc_y = crtc_y;
-	intel_plane->crtc_w = crtc_w;
-	intel_plane->crtc_h = crtc_h;
-	intel_plane->src_x = src_x;
-	intel_plane->src_y = src_y;
-	intel_plane->src_w = src_w;
-	intel_plane->src_h = src_h;
+	const struct {
+		int crtc_x, crtc_y;
+		unsigned int crtc_w, crtc_h;
+		uint32_t src_x, src_y, src_w, src_h;
+	} orig = {
+		.crtc_x = crtc_x,
+		.crtc_y = crtc_y,
+		.crtc_w = crtc_w,
+		.crtc_h = crtc_h,
+		.src_x = src_x,
+		.src_y = src_y,
+		.src_w = src_w,
+		.src_h = src_h,
+	};
 
 	/* Don't modify another pipe's plane */
 	if (intel_plane->pipe != intel_crtc->pipe) {
@@ -818,6 +823,14 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (ret)
 		return ret;
 
+	intel_plane->crtc_x = orig.crtc_x;
+	intel_plane->crtc_y = orig.crtc_y;
+	intel_plane->crtc_w = orig.crtc_w;
+	intel_plane->crtc_h = orig.crtc_h;
+	intel_plane->src_x = orig.src_x;
+	intel_plane->src_y = orig.src_y;
+	intel_plane->src_w = orig.src_w;
+	intel_plane->src_h = orig.src_h;
 	intel_plane->obj = obj;
 
 	if (intel_crtc->active) {
-- 
1.8.1.5

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

[PATCH 07/12] drm/i915: Do the fbc vs. primary plane enable/disable in the right order

[ville.syrjala]

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

Disable fbc before disabling the primary plane, and enable fbc after
the primary plane has been enabled again.

Also use intel_disable_fbc() to disable FBC to avoid the pointless
overhead of intel_update_fbc(), and especially avoid having to clean
up and set up the stolen mem compressed buffer again.

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

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 276c3a6..d191469 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -526,11 +526,11 @@ intel_enable_primary(struct drm_crtc *crtc)
 
 	intel_crtc->primary_disabled = false;
 
+	I915_WRITE(reg, I915_READ(reg) | DISPLAY_PLANE_ENABLE);
+
 	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
 	mutex_unlock(&dev->struct_mutex);
-
-	I915_WRITE(reg, I915_READ(reg) | DISPLAY_PLANE_ENABLE);
 }
 
 static void
@@ -544,13 +544,14 @@ intel_disable_primary(struct drm_crtc *crtc)
 	if (intel_crtc->primary_disabled)
 		return;
 
-	I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE);
-
 	intel_crtc->primary_disabled = true;
 
 	mutex_lock(&dev->struct_mutex);
-	intel_update_fbc(dev);
+	if (dev_priv->fbc.plane == intel_crtc->plane)
+		intel_disable_fbc(dev);
 	mutex_unlock(&dev->struct_mutex);
+
+	I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE);
 }
 
 static int
-- 
1.8.1.5

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

[PATCH 08/12] drm/i915: Enable/disable IPS when primary is enabled/disabled

[ville.syrjala]

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

IPS should be OK as long as one plane is enabled on the pipe, but
it does seem to cause problems when going between primary only and
sprite only.

This needs more investigations, but for now just disable IPS whenever
the primary plane is disabled.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 261e043..eef4fdc 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3291,7 +3291,7 @@ static void intel_disable_planes(struct drm_crtc *crtc)
 			intel_plane_disable(&intel_plane->base);
 }
 
-static void hsw_enable_ips(struct intel_crtc *crtc)
+void hsw_enable_ips(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 
@@ -3306,7 +3306,7 @@ static void hsw_enable_ips(struct intel_crtc *crtc)
 	I915_WRITE(IPS_CTL, IPS_ENABLE);
 }
 
-static void hsw_disable_ips(struct intel_crtc *crtc)
+void hsw_disable_ips(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e6db0c6..1ba1a92 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -679,6 +679,8 @@ ironlake_check_encoder_dotclock(const struct intel_crtc_config *pipe_config,
 				int dotclock);
 bool intel_crtc_active(struct drm_crtc *crtc);
 void i915_disable_vga_mem(struct drm_device *dev);
+void hsw_enable_ips(struct intel_crtc *crtc);
+void hsw_disable_ips(struct intel_crtc *crtc);
 
 
 /* intel_dp.c */
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index d191469..76d0e2f 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -528,6 +528,17 @@ intel_enable_primary(struct drm_crtc *crtc)
 
 	I915_WRITE(reg, I915_READ(reg) | DISPLAY_PLANE_ENABLE);
 
+	/*
+	 * FIXME IPS should be fine as long as one plane is
+	 * enabled, but in practice it seems to have problems
+	 * when going from primary only to sprite only and vice
+	 * versa.
+	 */
+	if (intel_crtc->config.ips_enabled) {
+		intel_wait_for_vblank(dev, intel_crtc->pipe);
+		hsw_enable_ips(intel_crtc);
+	}
+
 	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
 	mutex_unlock(&dev->struct_mutex);
@@ -551,6 +562,14 @@ intel_disable_primary(struct drm_crtc *crtc)
 		intel_disable_fbc(dev);
 	mutex_unlock(&dev->struct_mutex);
 
+	/*
+	 * FIXME IPS should be fine as long as one plane is
+	 * enabled, but in practice it seems to have problems
+	 * when going from primary only to sprite only and vice
+	 * versa.
+	 */
+	hsw_disable_ips(intel_crtc);
+
 	I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE);
 }
 
-- 
1.8.1.5

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

[PATCH 09/12] drm/i915: Rename intel_flush_display_plane to intel_flush_primary_plane

[ville.syrjala]

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

The intel_flush_primary_plane name actually tells us which plane
we're talking about.

Also reorganize the internals a bit and add a missing POSTING_READ()
to make sure the hardware has seen the changes by the time we
return from the function.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 16 ++++++++--------
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 drivers/gpu/drm/i915/intel_pm.c      |  2 +-
 drivers/gpu/drm/i915/intel_tv.c      |  4 ++--
 4 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index eef4fdc..be6a19d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1789,13 +1789,13 @@ static void intel_disable_pipe(struct drm_i915_private *dev_priv,
  * Plane regs are double buffered, going from enabled->disabled needs a
  * trigger in order to latch.  The display address reg provides this.
  */
-void intel_flush_display_plane(struct drm_i915_private *dev_priv,
-				      enum plane plane)
+void intel_flush_primary_plane(struct drm_i915_private *dev_priv,
+			       enum plane plane)
 {
-	if (dev_priv->info->gen >= 4)
-		I915_WRITE(DSPSURF(plane), I915_READ(DSPSURF(plane)));
-	else
-		I915_WRITE(DSPADDR(plane), I915_READ(DSPADDR(plane)));
+	u32 reg = dev_priv->info->gen >= 4 ? DSPSURF(plane) : DSPADDR(plane);
+
+	I915_WRITE(reg, I915_READ(reg));
+	POSTING_READ(reg);
 }
 
 /**
@@ -1825,7 +1825,7 @@ static void intel_enable_plane(struct drm_i915_private *dev_priv,
 		return;
 
 	I915_WRITE(reg, val | DISPLAY_PLANE_ENABLE);
-	intel_flush_display_plane(dev_priv, plane);
+	intel_flush_primary_plane(dev_priv, plane);
 	intel_wait_for_vblank(dev_priv->dev, pipe);
 }
 
@@ -1853,7 +1853,7 @@ static void intel_disable_plane(struct drm_i915_private *dev_priv,
 		return;
 
 	I915_WRITE(reg, val & ~DISPLAY_PLANE_ENABLE);
-	intel_flush_display_plane(dev_priv, plane);
+	intel_flush_primary_plane(dev_priv, plane);
 	intel_wait_for_vblank(dev_priv->dev, pipe);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1ba1a92..99977b6 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -814,7 +814,7 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob);
 
 /* intel_sprite.c */
 int intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane);
-void intel_flush_display_plane(struct drm_i915_private *dev_priv,
+void intel_flush_primary_plane(struct drm_i915_private *dev_priv,
 			       enum plane plane);
 void intel_plane_restore(struct drm_plane *plane);
 void intel_plane_disable(struct drm_plane *plane);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 698257c..43babe9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4704,7 +4704,7 @@ static void g4x_disable_trickle_feed(struct drm_device *dev)
 		I915_WRITE(DSPCNTR(pipe),
 			   I915_READ(DSPCNTR(pipe)) |
 			   DISPPLANE_TRICKLE_FEED_DISABLE);
-		intel_flush_display_plane(dev_priv, pipe);
+		intel_flush_primary_plane(dev_priv, pipe);
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 92895f9..1f7e8ca 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1094,7 +1094,7 @@ static void intel_tv_mode_set(struct intel_encoder *encoder)
 		unsigned int xsize, ysize;
 		/* Pipe must be off here */
 		I915_WRITE(dspcntr_reg, dspcntr & ~DISPLAY_PLANE_ENABLE);
-		intel_flush_display_plane(dev_priv, intel_crtc->plane);
+		intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 		/* Wait for vblank for the disable to take effect */
 		if (IS_GEN2(dev))
@@ -1123,7 +1123,7 @@ static void intel_tv_mode_set(struct intel_encoder *encoder)
 
 		I915_WRITE(pipeconf_reg, pipeconf);
 		I915_WRITE(dspcntr_reg, dspcntr);
-		intel_flush_display_plane(dev_priv, intel_crtc->plane);
+		intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 	}
 
 	j = 0;
-- 
1.8.1.5

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

[PATCH 10/12] drm/i915: Rename intel_{enable, disable}_plane to intel_{enable, disable}_primary_plane

[ville.syrjala]

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

The new names make it clearer which plane we're talking about.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index be6a19d..76a77f7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1799,15 +1799,15 @@ void intel_flush_primary_plane(struct drm_i915_private *dev_priv,
 }
 
 /**
- * intel_enable_plane - enable a display plane on a given pipe
+ * intel_enable_primary_plane - enable the primary plane on a given pipe
  * @dev_priv: i915 private structure
  * @plane: plane to enable
  * @pipe: pipe being fed
  *
  * Enable @plane on @pipe, making sure that @pipe is running first.
  */
-static void intel_enable_plane(struct drm_i915_private *dev_priv,
-			       enum plane plane, enum pipe pipe)
+static void intel_enable_primary_plane(struct drm_i915_private *dev_priv,
+				       enum plane plane, enum pipe pipe)
 {
 	struct intel_crtc *intel_crtc =
 		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
@@ -1830,15 +1830,15 @@ static void intel_enable_plane(struct drm_i915_private *dev_priv,
 }
 
 /**
- * intel_disable_plane - disable a display plane
+ * intel_disable_primary_plane - disable the primary plane
  * @dev_priv: i915 private structure
  * @plane: plane to disable
  * @pipe: pipe consuming the data
  *
  * Disable @plane; should be an independent operation.
  */
-static void intel_disable_plane(struct drm_i915_private *dev_priv,
-				enum plane plane, enum pipe pipe)
+static void intel_disable_primary_plane(struct drm_i915_private *dev_priv,
+					enum plane plane, enum pipe pipe)
 {
 	struct intel_crtc *intel_crtc =
 		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
@@ -3413,7 +3413,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	intel_update_watermarks(crtc);
 	intel_enable_pipe(dev_priv, pipe,
 			  intel_crtc->config.has_pch_encoder, false);
-	intel_enable_plane(dev_priv, plane, pipe);
+	intel_enable_primary_plane(dev_priv, plane, pipe);
 	intel_enable_planes(crtc);
 	intel_crtc_update_cursor(crtc, true);
 
@@ -3490,7 +3490,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	intel_update_watermarks(crtc);
 	intel_enable_pipe(dev_priv, pipe,
 			  intel_crtc->config.has_pch_encoder, false);
-	intel_enable_plane(dev_priv, plane, pipe);
+	intel_enable_primary_plane(dev_priv, plane, pipe);
 	intel_enable_planes(crtc);
 	intel_crtc_update_cursor(crtc, true);
 
@@ -3559,7 +3559,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 
 	intel_crtc_update_cursor(crtc, false);
 	intel_disable_planes(crtc);
-	intel_disable_plane(dev_priv, plane, pipe);
+	intel_disable_primary_plane(dev_priv, plane, pipe);
 
 	if (intel_crtc->config.has_pch_encoder)
 		intel_set_pch_fifo_underrun_reporting(dev, pipe, false);
@@ -3636,7 +3636,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 
 	intel_crtc_update_cursor(crtc, false);
 	intel_disable_planes(crtc);
-	intel_disable_plane(dev_priv, plane, pipe);
+	intel_disable_primary_plane(dev_priv, plane, pipe);
 
 	if (intel_crtc->config.has_pch_encoder)
 		intel_set_pch_fifo_underrun_reporting(dev, TRANSCODER_A, false);
@@ -3779,7 +3779,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 
 	intel_update_watermarks(crtc);
 	intel_enable_pipe(dev_priv, pipe, false, is_dsi);
-	intel_enable_plane(dev_priv, plane, pipe);
+	intel_enable_primary_plane(dev_priv, plane, pipe);
 	intel_enable_planes(crtc);
 	intel_crtc_update_cursor(crtc, true);
 
@@ -3817,7 +3817,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 
 	intel_update_watermarks(crtc);
 	intel_enable_pipe(dev_priv, pipe, false, false);
-	intel_enable_plane(dev_priv, plane, pipe);
+	intel_enable_primary_plane(dev_priv, plane, pipe);
 	intel_enable_planes(crtc);
 	/* The fixup needs to happen before cursor is enabled */
 	if (IS_G4X(dev))
@@ -3873,7 +3873,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	intel_crtc_dpms_overlay(intel_crtc, false);
 	intel_crtc_update_cursor(crtc, false);
 	intel_disable_planes(crtc);
-	intel_disable_plane(dev_priv, plane, pipe);
+	intel_disable_primary_plane(dev_priv, plane, pipe);
 
 	intel_disable_pipe(dev_priv, pipe);
 
-- 
1.8.1.5

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

[PATCH 11/12] drm/i915: WARN if primary plane state doesn't match expectations

[ville.syrjala]

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

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 76a77f7..a0d0202 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1817,6 +1817,8 @@ static void intel_enable_primary_plane(struct drm_i915_private *dev_priv,
 	/* If the pipe isn't enabled, we can't pump pixels and may hang */
 	assert_pipe_enabled(dev_priv, pipe);
 
+	WARN(!intel_crtc->primary_disabled, "Primary plane already enabled\n");
+
 	intel_crtc->primary_disabled = false;
 
 	reg = DSPCNTR(plane);
@@ -1845,6 +1847,8 @@ static void intel_disable_primary_plane(struct drm_i915_private *dev_priv,
 	int reg;
 	u32 val;
 
+	WARN(intel_crtc->primary_disabled, "Primary plane already disabled\n");
+
 	intel_crtc->primary_disabled = true;
 
 	reg = DSPCNTR(plane);
-- 
1.8.1.5

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

[PATCH 12/12] drm/i915: Flush primary plane changes in sprite code

[ville.syrjala]

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

Flush the primary plane changes when enabling/disabling the primary
plane in response to sprite visibility.

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

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 76d0e2f..e001d2c 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -527,6 +527,7 @@ intel_enable_primary(struct drm_crtc *crtc)
 	intel_crtc->primary_disabled = false;
 
 	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
@@ -571,6 +572,7 @@ intel_disable_primary(struct drm_crtc *crtc)
 	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
-- 
1.8.1.5

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

Re: [PATCH 03/12] drm/i915: Reduce the time we hold struct mutex in sprite update_plane code

[Chris Wilson]

On Tue, Oct 01, 2013 at 06:02:12PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We used to call the entire intel specific update_plane hook while
> holding struct_mutex. Actually we only need to hold struct_mutex while
> pinning/unpinning the obj. The plane state itself is protected by the
> kms locks, and as the object is pinned we can dig out the offset and
> tiling information from it without fearing that it would change
> underneath us.
> 
> So now we don't need to drop and reacquire the lock around the
> wait_for_vblank. Also we will need another wait_for_vblank in the IVB
> specific update_plane hook, and this way we don't need to worry about
> struct_mutex there either.
> 
> Also move the intel_plane->obj=NULL assignment outside strut_mutex in
> disable_plane to make it clear that it's not protected by struct_mutex.

intel_update_fbc() needs to be taken out and shot. It needs the mode
lock, crtc lock and the struct_mutex.

This patch looks fine, but anything touching fbc just makes me want to
curl up in a corner and whimper. Friends don't let friends enable fbc!
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 07/12] drm/i915: Do the fbc vs. primary plane enable/disable in the right order

[Chris Wilson]

On Tue, Oct 01, 2013 at 06:02:16PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Disable fbc before disabling the primary plane, and enable fbc after
> the primary plane has been enabled again.
> 
> Also use intel_disable_fbc() to disable FBC to avoid the pointless
> overhead of intel_update_fbc(), and especially avoid having to clean
> up and set up the stolen mem compressed buffer again.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

+1
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 00/12] drm/i915: Some cleanups and fixes to the sprite code v2

[Chris Wilson]

On Tue, Oct 01, 2013 at 06:02:09PM +0300, ville.syrjala@linux.intel.com wrote:
> Chris asked for some renames and assertions during v1. While adding those I
> noticed that what I did in the original patch 02 didn't match quite so well
> with the assertions. So I modified patch 02 a bit, and that caused quite a bit
> of bit of rebase issues for most of the other patches, so I figured it's better
> to repost the whole thing.
> 
> Changes from v1:
> - Move the primary disable/enable calls inside intel_crtc->active checks
>   in intel_update_plane/intel_disable_plane. That also ate up patch 03 from
>   the original series.
> - Add primary_disabled WARNs
> - Rename primary plane funcs
> - Flush primary plane changes from sprite code
> - Add a POSTING_READ() to intel_flush_primary_plane. This shouldn't really
>   be necessary now that I think about it some more. So we might want to drop
>   that change...

Looks good, very good, to me.

Even with throwing up over FBC,

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

except for

08/12: drm/i915: Enable/disable IPS when primary is
       enabled/disabled

For which the code looks ok, but only merits an
Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 03/12] drm/i915: Reduce the time we hold struct mutex in sprite update_plane code

[Ville Syrjälä]

On Fri, Oct 04, 2013 at 11:21:20AM +0100, Chris Wilson wrote:
> On Tue, Oct 01, 2013 at 06:02:12PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We used to call the entire intel specific update_plane hook while
> > holding struct_mutex. Actually we only need to hold struct_mutex while
> > pinning/unpinning the obj. The plane state itself is protected by the
> > kms locks, and as the object is pinned we can dig out the offset and
> > tiling information from it without fearing that it would change
> > underneath us.
> > 
> > So now we don't need to drop and reacquire the lock around the
> > wait_for_vblank. Also we will need another wait_for_vblank in the IVB
> > specific update_plane hook, and this way we don't need to worry about
> > struct_mutex there either.
> > 
> > Also move the intel_plane->obj=NULL assignment outside strut_mutex in
> > disable_plane to make it clear that it's not protected by struct_mutex.
> 
> intel_update_fbc() needs to be taken out and shot. It needs the mode
> lock, crtc lock and the struct_mutex.
> 
> This patch looks fine, but anything touching fbc just makes me want to
> curl up in a corner and whimper. Friends don't let friends enable fbc!

Yeah, I have to fight the urge to beat that guy into shape every time I
come across it. I fear it's going to be another rathole, which is why
I'm trying to hold off until I've managed to clear my board of other
tasks a bit.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 03/12] drm/i915: Reduce the time we hold struct mutex in sprite update_plane code

[Daniel Vetter]

On Fri, Oct 04, 2013 at 01:40:08PM +0300, Ville Syrjälä wrote:
> On Fri, Oct 04, 2013 at 11:21:20AM +0100, Chris Wilson wrote:
> > On Tue, Oct 01, 2013 at 06:02:12PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > We used to call the entire intel specific update_plane hook while
> > > holding struct_mutex. Actually we only need to hold struct_mutex while
> > > pinning/unpinning the obj. The plane state itself is protected by the
> > > kms locks, and as the object is pinned we can dig out the offset and
> > > tiling information from it without fearing that it would change
> > > underneath us.
> > > 
> > > So now we don't need to drop and reacquire the lock around the
> > > wait_for_vblank. Also we will need another wait_for_vblank in the IVB
> > > specific update_plane hook, and this way we don't need to worry about
> > > struct_mutex there either.
> > > 
> > > Also move the intel_plane->obj=NULL assignment outside strut_mutex in
> > > disable_plane to make it clear that it's not protected by struct_mutex.
> > 
> > intel_update_fbc() needs to be taken out and shot. It needs the mode
> > lock, crtc lock and the struct_mutex.
> > 
> > This patch looks fine, but anything touching fbc just makes me want to
> > curl up in a corner and whimper. Friends don't let friends enable fbc!
> 
> Yeah, I have to fight the urge to beat that guy into shape every time I
> come across it. I fear it's going to be another rathole, which is why
> I'm trying to hold off until I've managed to clear my board of other
> tasks a bit.

Wrt cleaning up the fbc rathole (and it's one, same applies to psr btw):
We need to have some illusion of testcases first:
- Checks that screen updates work using the crc stuff.
- Functional checks that we actually enter psr/use compressed buffers. And
  they _must_ fail if QA botches their setup somehow (not like the current
  pc8+ test which is too polite and just skips).
- Integration into the pipe config and plane config stuff for atomic
  modesets and nuclear pageflips and what not else.

This can easily cost us a few souls I think ;-)

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

Re: [PATCH 00/12] drm/i915: Some cleanups and fixes to the sprite code v2

[Daniel Vetter]

On Fri, Oct 04, 2013 at 11:28:16AM +0100, Chris Wilson wrote:
> On Tue, Oct 01, 2013 at 06:02:09PM +0300, ville.syrjala@linux.intel.com wrote:
> > Chris asked for some renames and assertions during v1. While adding those I
> > noticed that what I did in the original patch 02 didn't match quite so well
> > with the assertions. So I modified patch 02 a bit, and that caused quite a bit
> > of bit of rebase issues for most of the other patches, so I figured it's better
> > to repost the whole thing.
> > 
> > Changes from v1:
> > - Move the primary disable/enable calls inside intel_crtc->active checks
> >   in intel_update_plane/intel_disable_plane. That also ate up patch 03 from
> >   the original series.
> > - Add primary_disabled WARNs
> > - Rename primary plane funcs
> > - Flush primary plane changes from sprite code
> > - Add a POSTING_READ() to intel_flush_primary_plane. This shouldn't really
> >   be necessary now that I think about it some more. So we might want to drop
> >   that change...
> 
> Looks good, very good, to me.
> 
> Even with throwing up over FBC,
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> except for
> 
> 08/12: drm/i915: Enable/disable IPS when primary is
>        enabled/disabled
> 
> For which the code looks ok, but only merits an
> Acked-by: Chris Wilson <chris@chris-wilson.co.uk>

All merged, thanks for patches&review. Now my problem here is that I'm
really uneasy with our complete lack of testcoverage for all these
corner-cases. I know that we can't really get full functional testing
going for sprites/planes/cursors before we have the CRC stuff all merged,
but at least exercising all this code would be great.

So can we please move "polish/create testcases for cursor/sprite/plane
corner-cases and push the to igt" to the top-spot for all things planes?
At least before we start to wreak utter havoc with atomic
pageflips/modeset I want to have some assurance that we don't break all
the low-level functions we've painstakingly beaten into shape in the past
few months ...

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