[PATCH 0/5] i915 atomic plane helper conversion (v4)

[PATCH 0/5] i915 atomic plane helper conversion (v4)

[Matt Roper]

Updated with some minor cleanups and fixes spotted by Ander.  The largest
change between this series and the last was that I've now squashed the "prepare
for atomic plane helpers" patch that added the new entrypoints with the
"transition to atomic helpers" patch which actually makes the transition over.
With earlier revisions of the patch series, there was still enough subtle
changes to semantics during the transition that I kept them separate to help
keep it clear what was changing.  But we've reworked this patchset enough now
that both the preparation and transition patches are much simpler and
straightforward, so I think it makes sense to just combine them (although I'm
happy to split them back out if reviewers disagree).

Previous patchset available at:
   http://lists.freedesktop.org/archives/intel-gfx/2014-December/057136.html

Matt Roper (5):
  drm/i915: Refactor work that can sleep out of commit (v5)
  drm/i915: Move vblank evasion to commit (v3)
  drm/i915: Clarify sprite plane function names (v3)
  drm/i915: Move to atomic plane helpers (v8)
  drm/i915: Drop unused position fields (v2)

 Documentation/DocBook/drm.tmpl            |   5 +
 drivers/gpu/drm/i915/Makefile             |   1 +
 drivers/gpu/drm/i915/intel_atomic_plane.c | 150 +++++++++++++
 drivers/gpu/drm/i915/intel_display.c      | 358 +++++++++++++++---------------
 drivers/gpu/drm/i915/intel_drv.h          |  50 ++++-
 drivers/gpu/drm/i915/intel_sprite.c       | 183 ++++++---------
 6 files changed, 438 insertions(+), 309 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_atomic_plane.c

-- 
1.8.5.1

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

[PATCH 1/5] drm/i915: Refactor work that can sleep out of commit (v5)

[Matt Roper]

Once we integrate our work into the atomic pipeline, plane commit
operations will need to happen with interrupts disabled, due to vblank
evasion.  Our commit functions today include sleepable work, so those
operations need to be split out and run either before or after the
atomic register programming.

The solution here calculates which of those operations will need to be
performed during the 'check' phase and sets flags in an intel_crtc
sub-struct.  New intel_begin_crtc_commit() and
intel_finish_crtc_commit() functions are added before and after the
actual register programming; these will eventually be called from the
atomic plane helper's .atomic_begin() and .atomic_end() entrypoints.

v2: Fix broken sprite code split

v3: Make the pre/post commit work crtc-based to match how we eventually
    want this to be called from the atomic plane helpers.

v4: Some platforms that haven't had their watermark code reworked were
    waiting for vblank, then calling update_sprite_watermarks in their
    platform-specific disable code.  These also need to be flagged out
    of the critical section.

v5: Sprite plane test for primary show/hide should just set the flag to
    wait for pending flips, not actually perform the wait.  (Ander)

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 180 ++++++++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_drv.h     |  31 ++++++
 drivers/gpu/drm/i915/intel_sprite.c  |  78 ++++++---------
 3 files changed, 178 insertions(+), 111 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3044af5..5d90114 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11737,7 +11737,11 @@ static int
 intel_check_primary_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
+	struct drm_device *dev = plane->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc = state->base.crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_rect *dest = &state->dst;
 	struct drm_rect *src = &state->src;
@@ -11758,6 +11762,40 @@ intel_check_primary_plane(struct drm_plane *plane,
 		return -EBUSY;
 	}
 
+	if (intel_crtc->active) {
+		/*
+		 * FBC does not work on some platforms for rotated
+		 * planes, so disable it when rotation is not 0 and
+		 * update it when rotation is set back to 0.
+		 *
+		 * FIXME: This is redundant with the fbc update done in
+		 * the primary plane enable function except that that
+		 * one is done too late. We eventually need to unify
+		 * this.
+		 */
+		if (intel_crtc->primary_enabled &&
+		    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
+		    dev_priv->fbc.plane == intel_crtc->plane &&
+		    intel_plane->rotation != BIT(DRM_ROTATE_0)) {
+			intel_crtc->atomic.disable_fbc = true;
+		}
+
+		if (state->visible) {
+			/*
+			 * BDW signals flip done immediately if the plane
+			 * is disabled, even if the plane enable is already
+			 * armed to occur at the next vblank :(
+			 */
+			if (IS_BROADWELL(dev) && !intel_crtc->primary_enabled)
+				intel_crtc->atomic.wait_vblank = true;
+		}
+
+		intel_crtc->atomic.fb_bits |=
+			INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
+
+		intel_crtc->atomic.update_fbc = true;
+	}
+
 	return 0;
 }
 
@@ -11773,18 +11811,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_rect *src = &state->src;
-	enum pipe pipe = intel_plane->pipe;
-
-	if (!fb) {
-		/*
-		 * 'prepare' is never called when plane is being disabled, so
-		 * we need to handle frontbuffer tracking here
-		 */
-		mutex_lock(&dev->struct_mutex);
-		i915_gem_track_fb(intel_fb_obj(plane->fb), NULL,
-				  INTEL_FRONTBUFFER_PRIMARY(pipe));
-		mutex_unlock(&dev->struct_mutex);
-	}
 
 	plane->fb = fb;
 	crtc->x = src->x1 >> 16;
@@ -11801,26 +11827,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
 	intel_plane->obj = obj;
 
 	if (intel_crtc->active) {
-		/*
-		 * FBC does not work on some platforms for rotated
-		 * planes, so disable it when rotation is not 0 and
-		 * update it when rotation is set back to 0.
-		 *
-		 * FIXME: This is redundant with the fbc update done in
-		 * the primary plane enable function except that that
-		 * one is done too late. We eventually need to unify
-		 * this.
-		 */
-		if (intel_crtc->primary_enabled &&
-		    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
-		    dev_priv->fbc.plane == intel_crtc->plane &&
-		    intel_plane->rotation != BIT(DRM_ROTATE_0)) {
-			intel_fbc_disable(dev);
-		}
-
 		if (state->visible) {
-			bool was_enabled = intel_crtc->primary_enabled;
-
 			/* FIXME: kill this fastboot hack */
 			intel_update_pipe_size(intel_crtc);
 
@@ -11828,14 +11835,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
 
 			dev_priv->display.update_primary_plane(crtc, plane->fb,
 					crtc->x, crtc->y);
-
-			/*
-			 * BDW signals flip done immediately if the plane
-			 * is disabled, even if the plane enable is already
-			 * armed to occur at the next vblank :(
-			 */
-			if (IS_BROADWELL(dev) && !was_enabled)
-				intel_wait_for_vblank(dev, intel_crtc->pipe);
 		} else {
 			/*
 			 * If clipping results in a non-visible primary plane,
@@ -11846,13 +11845,50 @@ intel_commit_primary_plane(struct drm_plane *plane,
 			 */
 			intel_disable_primary_hw_plane(plane, crtc);
 		}
+	}
+}
 
-		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
+static void intel_begin_crtc_commit(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+	if (intel_crtc->atomic.disable_fbc)
+		intel_fbc_disable(dev);
+
+	if (intel_crtc->atomic.pre_disable_primary)
+		intel_pre_disable_primary(crtc);
+
+	if (intel_crtc->atomic.update_wm)
+		intel_update_watermarks(crtc);
+}
 
+static void intel_finish_crtc_commit(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_plane *p;
+
+	if (intel_crtc->atomic.wait_vblank)
+		intel_wait_for_vblank(dev, intel_crtc->pipe);
+
+	intel_frontbuffer_flip(dev, intel_crtc->atomic.fb_bits);
+
+	if (intel_crtc->atomic.update_fbc) {
 		mutex_lock(&dev->struct_mutex);
 		intel_fbc_update(dev);
 		mutex_unlock(&dev->struct_mutex);
 	}
+
+	if (intel_crtc->atomic.post_enable_primary)
+		intel_post_enable_primary(crtc);
+
+	drm_for_each_legacy_plane(p, &dev->mode_config.plane_list)
+		if (intel_crtc->atomic.update_sprite_watermarks & drm_plane_index(p))
+			intel_update_sprite_watermarks(p, crtc, 0, 0, 0,
+						       false, false);
+
+	memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
 }
 
 int
@@ -11864,7 +11900,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 {
 	struct drm_device *dev = plane->dev;
 	struct drm_framebuffer *old_fb = plane->fb;
-	struct intel_plane_state state;
+	struct intel_plane_state state = {{ 0 }};
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int ret;
@@ -11902,7 +11938,33 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 			return ret;
 	}
 
+	if (!state.base.fb) {
+		unsigned fb_bits = 0;
+
+		switch (plane->type) {
+		case DRM_PLANE_TYPE_PRIMARY:
+			fb_bits = INTEL_FRONTBUFFER_PRIMARY(intel_plane->pipe);
+			break;
+		case DRM_PLANE_TYPE_CURSOR:
+			fb_bits = INTEL_FRONTBUFFER_CURSOR(intel_plane->pipe);
+			break;
+		case DRM_PLANE_TYPE_OVERLAY:
+			fb_bits = INTEL_FRONTBUFFER_SPRITE(intel_plane->pipe);
+			break;
+		}
+
+		/*
+		 * 'prepare' is never called when plane is being disabled, so
+		 * we need to handle frontbuffer tracking here
+		 */
+		mutex_lock(&dev->struct_mutex);
+		i915_gem_track_fb(intel_fb_obj(plane->fb), NULL, fb_bits);
+		mutex_unlock(&dev->struct_mutex);
+	}
+
+	intel_begin_crtc_commit(crtc);
 	intel_plane->commit_plane(plane, &state);
+	intel_finish_crtc_commit(crtc);
 
 	if (fb != old_fb && old_fb) {
 		if (intel_crtc->active)
@@ -12009,6 +12071,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
 	struct drm_rect *src = &state->src;
 	const struct drm_rect *clip = &state->clip;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int crtc_w, crtc_h;
 	unsigned stride;
 	int ret;
@@ -12024,7 +12087,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
 
 	/* if we want to turn off the cursor ignore width and height */
 	if (!obj)
-		return 0;
+		goto finish;
 
 	/* Check for which cursor types we support */
 	crtc_w = drm_rect_width(&state->orig_dst);
@@ -12051,6 +12114,16 @@ intel_check_cursor_plane(struct drm_plane *plane,
 	}
 	mutex_unlock(&dev->struct_mutex);
 
+finish:
+	if (intel_crtc->active) {
+		if (intel_crtc->cursor_width !=
+		    drm_rect_width(&state->orig_dst))
+			intel_crtc->atomic.update_wm = true;
+
+		intel_crtc->atomic.fb_bits |=
+			INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe);
+	}
+
 	return ret;
 }
 
@@ -12063,9 +12136,6 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
-	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
-	enum pipe pipe = intel_crtc->pipe;
-	unsigned old_width;
 	uint32_t addr;
 
 	plane->fb = state->base.fb;
@@ -12085,17 +12155,6 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 	if (intel_crtc->cursor_bo == obj)
 		goto update;
 
-	/*
-	 * 'prepare' is only called when fb != NULL; we still need to update
-	 * frontbuffer tracking for the 'disable' case here.
-	 */
-	if (!obj) {
-		mutex_lock(&dev->struct_mutex);
-		i915_gem_track_fb(old_obj, NULL,
-				  INTEL_FRONTBUFFER_CURSOR(pipe));
-		mutex_unlock(&dev->struct_mutex);
-	}
-
 	if (!obj)
 		addr = 0;
 	else if (!INTEL_INFO(dev)->cursor_needs_physical)
@@ -12106,18 +12165,11 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 	intel_crtc->cursor_addr = addr;
 	intel_crtc->cursor_bo = obj;
 update:
-	old_width = intel_crtc->cursor_width;
-
 	intel_crtc->cursor_width = drm_rect_width(&state->orig_dst);
 	intel_crtc->cursor_height = drm_rect_height(&state->orig_dst);
 
-	if (intel_crtc->active) {
-		if (old_width != intel_crtc->cursor_width)
-			intel_update_watermarks(crtc);
+	if (intel_crtc->active)
 		intel_crtc_update_cursor(crtc, state->visible);
-
-		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
-	}
 }
 
 static const struct drm_plane_funcs intel_cursor_plane_funcs = {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 588b618..a03bd72 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -251,6 +251,12 @@ struct intel_plane_state {
 	struct drm_rect orig_src;
 	struct drm_rect orig_dst;
 	bool visible;
+
+	/*
+	 * used only for sprite planes to determine when to implicitly
+	 * enable/disable the primary plane
+	 */
+	bool hides_primary;
 };
 
 struct intel_plane_config {
@@ -415,6 +421,27 @@ struct skl_pipe_wm {
 	uint32_t linetime;
 };
 
+/*
+ * Tracking of operations that need to be performed at the beginning/end of an
+ * atomic commit, outside the atomic section where interrupts are disabled.
+ * These are generally operations that grab mutexes or might otherwise sleep
+ * and thus can't be run with interrupts disabled.
+ */
+struct intel_crtc_atomic_commit {
+	/* Sleepable operations to perform before commit */
+	bool wait_for_flips;
+	bool disable_fbc;
+	bool pre_disable_primary;
+	bool update_wm;
+
+	/* Sleepable operations to perform after commit */
+	unsigned fb_bits;
+	bool wait_vblank;
+	bool update_fbc;
+	bool post_enable_primary;
+	unsigned update_sprite_watermarks;
+};
+
 struct intel_crtc {
 	struct drm_crtc base;
 	enum pipe pipe;
@@ -468,6 +495,8 @@ struct intel_crtc {
 
 	int scanline_offset;
 	struct intel_mmio_flip mmio_flip;
+
+	struct intel_crtc_atomic_commit atomic;
 };
 
 struct intel_plane_wm_parameters {
@@ -1213,6 +1242,8 @@ int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
 bool intel_pipe_update_start(struct intel_crtc *crtc,
 			     uint32_t *start_vbl_count);
 void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count);
+void intel_post_enable_primary(struct drm_crtc *crtc);
+void intel_pre_disable_primary(struct drm_crtc *crtc);
 
 /* intel_tv.c */
 void intel_tv_init(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index c18e57d..ff7d6a1 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -771,9 +771,8 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	 * Avoid underruns when disabling the sprite.
 	 * FIXME remove once watermark updates are done properly.
 	 */
-	intel_wait_for_vblank(dev, pipe);
-
-	intel_update_sprite_watermarks(plane, crtc, 0, 0, 0, false, false);
+	intel_crtc->atomic.wait_vblank = true;
+	intel_crtc->atomic.update_sprite_watermarks |= (1 << drm_plane_index(plane));
 }
 
 static int
@@ -976,12 +975,11 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	 * Avoid underruns when disabling the sprite.
 	 * FIXME remove once watermark updates are done properly.
 	 */
-	intel_wait_for_vblank(dev, pipe);
-
-	intel_update_sprite_watermarks(plane, crtc, 0, 0, 0, false, false);
+	intel_crtc->atomic.wait_vblank = true;
+	intel_crtc->atomic.update_sprite_watermarks |= (1 << drm_plane_index(plane));
 }
 
-static void
+void
 intel_post_enable_primary(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -1008,7 +1006,7 @@ intel_post_enable_primary(struct drm_crtc *crtc)
 	mutex_unlock(&dev->struct_mutex);
 }
 
-static void
+void
 intel_pre_disable_primary(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -1113,7 +1111,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
 
 	if (!fb) {
 		state->visible = false;
-		return 0;
+		goto finish;
 	}
 
 	/* Don't modify another pipe's plane */
@@ -1260,6 +1258,29 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	dst->y1 = crtc_y;
 	dst->y2 = crtc_y + crtc_h;
 
+finish:
+	/*
+	 * If the sprite is completely covering the primary plane,
+	 * we can disable the primary and save power.
+	 */
+	state->hides_primary = drm_rect_equals(dst, clip) &&
+		!colorkey_enabled(intel_plane);
+	WARN_ON(state->hides_primary && !state->visible && intel_crtc->active);
+
+	if (intel_crtc->active) {
+		if (intel_crtc->primary_enabled == state->hides_primary)
+			intel_crtc->atomic.wait_for_flips = true;
+
+		if (intel_crtc->primary_enabled && state->hides_primary)
+			intel_crtc->atomic.pre_disable_primary = true;
+
+		intel_crtc->atomic.fb_bits |=
+			INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe);
+
+		if (!intel_crtc->primary_enabled && !state->hides_primary)
+			intel_crtc->atomic.post_enable_primary = true;
+	}
+
 	return 0;
 }
 
@@ -1267,37 +1288,14 @@ static void
 intel_commit_sprite_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
-	struct drm_device *dev = plane->dev;
 	struct drm_crtc *crtc = state->base.crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
-	enum pipe pipe = intel_crtc->pipe;
 	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	int crtc_x, crtc_y;
 	unsigned int crtc_w, crtc_h;
 	uint32_t src_x, src_y, src_w, src_h;
-	struct drm_rect *dst = &state->dst;
-	const struct drm_rect *clip = &state->clip;
-	bool primary_enabled;
-
-	/*
-	 * 'prepare' is never called when plane is being disabled, so we need
-	 * to handle frontbuffer tracking here
-	 */
-	if (!fb) {
-		mutex_lock(&dev->struct_mutex);
-		i915_gem_track_fb(intel_fb_obj(plane->fb), NULL,
-				  INTEL_FRONTBUFFER_SPRITE(pipe));
-		mutex_unlock(&dev->struct_mutex);
-	}
-
-	/*
-	 * If the sprite is completely covering the primary plane,
-	 * we can disable the primary and save power.
-	 */
-	primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane);
-	WARN_ON(!primary_enabled && !state->visible && intel_crtc->active);
 
 	intel_plane->crtc_x = state->orig_dst.x1;
 	intel_plane->crtc_y = state->orig_dst.y1;
@@ -1310,15 +1308,7 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 	intel_plane->obj = obj;
 
 	if (intel_crtc->active) {
-		bool primary_was_enabled = intel_crtc->primary_enabled;
-
-		intel_crtc->primary_enabled = primary_enabled;
-
-		if (primary_was_enabled != primary_enabled)
-			intel_crtc_wait_for_pending_flips(crtc);
-
-		if (primary_was_enabled && !primary_enabled)
-			intel_pre_disable_primary(crtc);
+		intel_crtc->primary_enabled = !state->hides_primary;
 
 		if (state->visible) {
 			crtc_x = state->dst.x1;
@@ -1335,12 +1325,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 		} else {
 			intel_plane->disable_plane(plane, crtc);
 		}
-
-
-		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_SPRITE(pipe));
-
-		if (!primary_was_enabled && primary_enabled)
-			intel_post_enable_primary(crtc);
 	}
 }
 
-- 
1.8.5.1

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

Re: [PATCH 1/5] drm/i915: Refactor work that can sleep out of commit (v5)

[Ander Conselvan de Oliveira]

On 12/16/2014 02:23 AM, Matt Roper wrote:
> Once we integrate our work into the atomic pipeline, plane commit
> operations will need to happen with interrupts disabled, due to vblank
> evasion.  Our commit functions today include sleepable work, so those
> operations need to be split out and run either before or after the
> atomic register programming.
>
> The solution here calculates which of those operations will need to be
> performed during the 'check' phase and sets flags in an intel_crtc
> sub-struct.  New intel_begin_crtc_commit() and
> intel_finish_crtc_commit() functions are added before and after the
> actual register programming; these will eventually be called from the
> atomic plane helper's .atomic_begin() and .atomic_end() entrypoints.
>
> v2: Fix broken sprite code split
>
> v3: Make the pre/post commit work crtc-based to match how we eventually
>      want this to be called from the atomic plane helpers.
>
> v4: Some platforms that haven't had their watermark code reworked were
>      waiting for vblank, then calling update_sprite_watermarks in their
>      platform-specific disable code.  These also need to be flagged out
>      of the critical section.
>
> v5: Sprite plane test for primary show/hide should just set the flag to
>      wait for pending flips, not actually perform the wait.  (Ander)
>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 180 ++++++++++++++++++++++-------------
>   drivers/gpu/drm/i915/intel_drv.h     |  31 ++++++
>   drivers/gpu/drm/i915/intel_sprite.c  |  78 ++++++---------
>   3 files changed, 178 insertions(+), 111 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3044af5..5d90114 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11737,7 +11737,11 @@ static int
>   intel_check_primary_plane(struct drm_plane *plane,
>   			  struct intel_plane_state *state)
>   {
> +	struct drm_device *dev = plane->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct drm_crtc *crtc = state->base.crtc;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
>   	struct drm_framebuffer *fb = state->base.fb;
>   	struct drm_rect *dest = &state->dst;
>   	struct drm_rect *src = &state->src;
> @@ -11758,6 +11762,40 @@ intel_check_primary_plane(struct drm_plane *plane,
>   		return -EBUSY;
>   	}
>
> +	if (intel_crtc->active) {
> +		/*
> +		 * FBC does not work on some platforms for rotated
> +		 * planes, so disable it when rotation is not 0 and
> +		 * update it when rotation is set back to 0.
> +		 *
> +		 * FIXME: This is redundant with the fbc update done in
> +		 * the primary plane enable function except that that
> +		 * one is done too late. We eventually need to unify
> +		 * this.
> +		 */
> +		if (intel_crtc->primary_enabled &&
> +		    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
> +		    dev_priv->fbc.plane == intel_crtc->plane &&
> +		    intel_plane->rotation != BIT(DRM_ROTATE_0)) {
> +			intel_crtc->atomic.disable_fbc = true;
> +		}
> +
> +		if (state->visible) {
> +			/*
> +			 * BDW signals flip done immediately if the plane
> +			 * is disabled, even if the plane enable is already
> +			 * armed to occur at the next vblank :(
> +			 */
> +			if (IS_BROADWELL(dev) && !intel_crtc->primary_enabled)
> +				intel_crtc->atomic.wait_vblank = true;
> +		}
> +
> +		intel_crtc->atomic.fb_bits |=
> +			INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
> +
> +		intel_crtc->atomic.update_fbc = true;
> +	}
> +
>   	return 0;
>   }
>
> @@ -11773,18 +11811,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
>   	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>   	struct intel_plane *intel_plane = to_intel_plane(plane);
>   	struct drm_rect *src = &state->src;
> -	enum pipe pipe = intel_plane->pipe;
> -
> -	if (!fb) {
> -		/*
> -		 * 'prepare' is never called when plane is being disabled, so
> -		 * we need to handle frontbuffer tracking here
> -		 */
> -		mutex_lock(&dev->struct_mutex);
> -		i915_gem_track_fb(intel_fb_obj(plane->fb), NULL,
> -				  INTEL_FRONTBUFFER_PRIMARY(pipe));
> -		mutex_unlock(&dev->struct_mutex);
> -	}
>
>   	plane->fb = fb;
>   	crtc->x = src->x1 >> 16;
> @@ -11801,26 +11827,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
>   	intel_plane->obj = obj;
>
>   	if (intel_crtc->active) {
> -		/*
> -		 * FBC does not work on some platforms for rotated
> -		 * planes, so disable it when rotation is not 0 and
> -		 * update it when rotation is set back to 0.
> -		 *
> -		 * FIXME: This is redundant with the fbc update done in
> -		 * the primary plane enable function except that that
> -		 * one is done too late. We eventually need to unify
> -		 * this.
> -		 */
> -		if (intel_crtc->primary_enabled &&
> -		    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
> -		    dev_priv->fbc.plane == intel_crtc->plane &&
> -		    intel_plane->rotation != BIT(DRM_ROTATE_0)) {
> -			intel_fbc_disable(dev);
> -		}
> -
>   		if (state->visible) {
> -			bool was_enabled = intel_crtc->primary_enabled;
> -
>   			/* FIXME: kill this fastboot hack */
>   			intel_update_pipe_size(intel_crtc);
>
> @@ -11828,14 +11835,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
>
>   			dev_priv->display.update_primary_plane(crtc, plane->fb,
>   					crtc->x, crtc->y);
> -
> -			/*
> -			 * BDW signals flip done immediately if the plane
> -			 * is disabled, even if the plane enable is already
> -			 * armed to occur at the next vblank :(
> -			 */
> -			if (IS_BROADWELL(dev) && !was_enabled)
> -				intel_wait_for_vblank(dev, intel_crtc->pipe);
>   		} else {
>   			/*
>   			 * If clipping results in a non-visible primary plane,
> @@ -11846,13 +11845,50 @@ intel_commit_primary_plane(struct drm_plane *plane,
>   			 */
>   			intel_disable_primary_hw_plane(plane, crtc);
>   		}
> +	}
> +}
>
> -		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
> +static void intel_begin_crtc_commit(struct drm_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +
> +	if (intel_crtc->atomic.disable_fbc)
> +		intel_fbc_disable(dev);
> +
> +	if (intel_crtc->atomic.pre_disable_primary)
> +		intel_pre_disable_primary(crtc);
> +
> +	if (intel_crtc->atomic.update_wm)
> +		intel_update_watermarks(crtc);
> +}
>
> +static void intel_finish_crtc_commit(struct drm_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct drm_plane *p;
> +
> +	if (intel_crtc->atomic.wait_vblank)
> +		intel_wait_for_vblank(dev, intel_crtc->pipe);
> +
> +	intel_frontbuffer_flip(dev, intel_crtc->atomic.fb_bits);
> +
> +	if (intel_crtc->atomic.update_fbc) {
>   		mutex_lock(&dev->struct_mutex);
>   		intel_fbc_update(dev);
>   		mutex_unlock(&dev->struct_mutex);
>   	}
> +
> +	if (intel_crtc->atomic.post_enable_primary)
> +		intel_post_enable_primary(crtc);
> +
> +	drm_for_each_legacy_plane(p, &dev->mode_config.plane_list)
> +		if (intel_crtc->atomic.update_sprite_watermarks & drm_plane_index(p))
> +			intel_update_sprite_watermarks(p, crtc, 0, 0, 0,
> +						       false, false);
> +
> +	memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
>   }
>
>   int
> @@ -11864,7 +11900,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>   {
>   	struct drm_device *dev = plane->dev;
>   	struct drm_framebuffer *old_fb = plane->fb;
> -	struct intel_plane_state state;
> +	struct intel_plane_state state = {{ 0 }};
>   	struct intel_plane *intel_plane = to_intel_plane(plane);
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   	int ret;
> @@ -11902,7 +11938,33 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>   			return ret;
>   	}
>
> +	if (!state.base.fb) {
> +		unsigned fb_bits = 0;
> +
> +		switch (plane->type) {
> +		case DRM_PLANE_TYPE_PRIMARY:
> +			fb_bits = INTEL_FRONTBUFFER_PRIMARY(intel_plane->pipe);
> +			break;
> +		case DRM_PLANE_TYPE_CURSOR:
> +			fb_bits = INTEL_FRONTBUFFER_CURSOR(intel_plane->pipe);
> +			break;
> +		case DRM_PLANE_TYPE_OVERLAY:
> +			fb_bits = INTEL_FRONTBUFFER_SPRITE(intel_plane->pipe);
> +			break;
> +		}
> +
> +		/*
> +		 * 'prepare' is never called when plane is being disabled, so
> +		 * we need to handle frontbuffer tracking here
> +		 */
> +		mutex_lock(&dev->struct_mutex);
> +		i915_gem_track_fb(intel_fb_obj(plane->fb), NULL, fb_bits);
> +		mutex_unlock(&dev->struct_mutex);
> +	}
> +
> +	intel_begin_crtc_commit(crtc);
>   	intel_plane->commit_plane(plane, &state);
> +	intel_finish_crtc_commit(crtc);

This needs to be rebased on top of the patch that added the 
intel_runtime_pm_get() and _put() calls here.


>   	if (fb != old_fb && old_fb) {
>   		if (intel_crtc->active)
> @@ -12009,6 +12071,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
>   	struct drm_rect *src = &state->src;
>   	const struct drm_rect *clip = &state->clip;
>   	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   	int crtc_w, crtc_h;
>   	unsigned stride;
>   	int ret;
> @@ -12024,7 +12087,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
>
>   	/* if we want to turn off the cursor ignore width and height */
>   	if (!obj)
> -		return 0;
> +		goto finish;
>
>   	/* Check for which cursor types we support */
>   	crtc_w = drm_rect_width(&state->orig_dst);
> @@ -12051,6 +12114,16 @@ intel_check_cursor_plane(struct drm_plane *plane,
>   	}
>   	mutex_unlock(&dev->struct_mutex);
>
> +finish:
> +	if (intel_crtc->active) {
> +		if (intel_crtc->cursor_width !=
> +		    drm_rect_width(&state->orig_dst))
> +			intel_crtc->atomic.update_wm = true;
> +
> +		intel_crtc->atomic.fb_bits |=
> +			INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe);
> +	}
> +
>   	return ret;
>   }
>
> @@ -12063,9 +12136,6 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   	struct intel_plane *intel_plane = to_intel_plane(plane);
>   	struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
> -	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
> -	enum pipe pipe = intel_crtc->pipe;
> -	unsigned old_width;
>   	uint32_t addr;
>
>   	plane->fb = state->base.fb;
> @@ -12085,17 +12155,6 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>   	if (intel_crtc->cursor_bo == obj)
>   		goto update;
>
> -	/*
> -	 * 'prepare' is only called when fb != NULL; we still need to update
> -	 * frontbuffer tracking for the 'disable' case here.
> -	 */
> -	if (!obj) {
> -		mutex_lock(&dev->struct_mutex);
> -		i915_gem_track_fb(old_obj, NULL,
> -				  INTEL_FRONTBUFFER_CURSOR(pipe));
> -		mutex_unlock(&dev->struct_mutex);
> -	}
> -
>   	if (!obj)
>   		addr = 0;
>   	else if (!INTEL_INFO(dev)->cursor_needs_physical)
> @@ -12106,18 +12165,11 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>   	intel_crtc->cursor_addr = addr;
>   	intel_crtc->cursor_bo = obj;
>   update:
> -	old_width = intel_crtc->cursor_width;
> -
>   	intel_crtc->cursor_width = drm_rect_width(&state->orig_dst);
>   	intel_crtc->cursor_height = drm_rect_height(&state->orig_dst);
>
> -	if (intel_crtc->active) {
> -		if (old_width != intel_crtc->cursor_width)
> -			intel_update_watermarks(crtc);
> +	if (intel_crtc->active)
>   		intel_crtc_update_cursor(crtc, state->visible);
> -
> -		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
> -	}
>   }
>
>   static const struct drm_plane_funcs intel_cursor_plane_funcs = {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 588b618..a03bd72 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -251,6 +251,12 @@ struct intel_plane_state {
>   	struct drm_rect orig_src;
>   	struct drm_rect orig_dst;
>   	bool visible;
> +
> +	/*
> +	 * used only for sprite planes to determine when to implicitly
> +	 * enable/disable the primary plane
> +	 */
> +	bool hides_primary;
>   };
>
>   struct intel_plane_config {
> @@ -415,6 +421,27 @@ struct skl_pipe_wm {
>   	uint32_t linetime;
>   };
>
> +/*
> + * Tracking of operations that need to be performed at the beginning/end of an
> + * atomic commit, outside the atomic section where interrupts are disabled.
> + * These are generally operations that grab mutexes or might otherwise sleep
> + * and thus can't be run with interrupts disabled.
> + */
> +struct intel_crtc_atomic_commit {
> +	/* Sleepable operations to perform before commit */
> +	bool wait_for_flips;

In intel_begin_crtc_commit(), the three fields below are handled, but 
not the one above.

> +	bool disable_fbc;
> +	bool pre_disable_primary;
> +	bool update_wm;
> +
> +	/* Sleepable operations to perform after commit */
> +	unsigned fb_bits;
> +	bool wait_vblank;
> +	bool update_fbc;
> +	bool post_enable_primary;
> +	unsigned update_sprite_watermarks;
> +};
> +
>   struct intel_crtc {
>   	struct drm_crtc base;
>   	enum pipe pipe;
> @@ -468,6 +495,8 @@ struct intel_crtc {
>
>   	int scanline_offset;
>   	struct intel_mmio_flip mmio_flip;
> +
> +	struct intel_crtc_atomic_commit atomic;
>   };
>
>   struct intel_plane_wm_parameters {
> @@ -1213,6 +1242,8 @@ int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
>   bool intel_pipe_update_start(struct intel_crtc *crtc,
>   			     uint32_t *start_vbl_count);
>   void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count);
> +void intel_post_enable_primary(struct drm_crtc *crtc);
> +void intel_pre_disable_primary(struct drm_crtc *crtc);
>
>   /* intel_tv.c */
>   void intel_tv_init(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index c18e57d..ff7d6a1 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -771,9 +771,8 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>   	 * Avoid underruns when disabling the sprite.
>   	 * FIXME remove once watermark updates are done properly.
>   	 */
> -	intel_wait_for_vblank(dev, pipe);
> -
> -	intel_update_sprite_watermarks(plane, crtc, 0, 0, 0, false, false);
> +	intel_crtc->atomic.wait_vblank = true;
> +	intel_crtc->atomic.update_sprite_watermarks |= (1 << drm_plane_index(plane));
>   }
>
>   static int
> @@ -976,12 +975,11 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>   	 * Avoid underruns when disabling the sprite.
>   	 * FIXME remove once watermark updates are done properly.
>   	 */
> -	intel_wait_for_vblank(dev, pipe);
> -
> -	intel_update_sprite_watermarks(plane, crtc, 0, 0, 0, false, false);
> +	intel_crtc->atomic.wait_vblank = true;
> +	intel_crtc->atomic.update_sprite_watermarks |= (1 << drm_plane_index(plane));
>   }
>
> -static void
> +void
>   intel_post_enable_primary(struct drm_crtc *crtc)
>   {
>   	struct drm_device *dev = crtc->dev;
> @@ -1008,7 +1006,7 @@ intel_post_enable_primary(struct drm_crtc *crtc)
>   	mutex_unlock(&dev->struct_mutex);
>   }
>
> -static void
> +void

Should this have kernel doc?

>   intel_pre_disable_primary(struct drm_crtc *crtc)
>   {
>   	struct drm_device *dev = crtc->dev;
> @@ -1113,7 +1111,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
>
>   	if (!fb) {
>   		state->visible = false;
> -		return 0;
> +		goto finish;
>   	}
>
>   	/* Don't modify another pipe's plane */
> @@ -1260,6 +1258,29 @@ intel_check_sprite_plane(struct drm_plane *plane,
>   	dst->y1 = crtc_y;
>   	dst->y2 = crtc_y + crtc_h;
>
> +finish:
> +	/*
> +	 * If the sprite is completely covering the primary plane,
> +	 * we can disable the primary and save power.
> +	 */
> +	state->hides_primary = drm_rect_equals(dst, clip) &&
> +		!colorkey_enabled(intel_plane);
> +	WARN_ON(state->hides_primary && !state->visible && intel_crtc->active);
> +
> +	if (intel_crtc->active) {
> +		if (intel_crtc->primary_enabled == state->hides_primary)
> +			intel_crtc->atomic.wait_for_flips = true;
> +
> +		if (intel_crtc->primary_enabled && state->hides_primary)
> +			intel_crtc->atomic.pre_disable_primary = true;
> +
> +		intel_crtc->atomic.fb_bits |=
> +			INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe);
> +
> +		if (!intel_crtc->primary_enabled && !state->hides_primary)
> +			intel_crtc->atomic.post_enable_primary = true;
> +	}
> +
>   	return 0;
>   }
>
> @@ -1267,37 +1288,14 @@ static void
>   intel_commit_sprite_plane(struct drm_plane *plane,
>   			  struct intel_plane_state *state)
>   {
> -	struct drm_device *dev = plane->dev;
>   	struct drm_crtc *crtc = state->base.crtc;
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   	struct intel_plane *intel_plane = to_intel_plane(plane);
> -	enum pipe pipe = intel_crtc->pipe;
>   	struct drm_framebuffer *fb = state->base.fb;
>   	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>   	int crtc_x, crtc_y;
>   	unsigned int crtc_w, crtc_h;
>   	uint32_t src_x, src_y, src_w, src_h;
> -	struct drm_rect *dst = &state->dst;
> -	const struct drm_rect *clip = &state->clip;
> -	bool primary_enabled;
> -
> -	/*
> -	 * 'prepare' is never called when plane is being disabled, so we need
> -	 * to handle frontbuffer tracking here
> -	 */
> -	if (!fb) {
> -		mutex_lock(&dev->struct_mutex);
> -		i915_gem_track_fb(intel_fb_obj(plane->fb), NULL,
> -				  INTEL_FRONTBUFFER_SPRITE(pipe));
> -		mutex_unlock(&dev->struct_mutex);
> -	}
> -
> -	/*
> -	 * If the sprite is completely covering the primary plane,
> -	 * we can disable the primary and save power.
> -	 */
> -	primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane);
> -	WARN_ON(!primary_enabled && !state->visible && intel_crtc->active);
>
>   	intel_plane->crtc_x = state->orig_dst.x1;
>   	intel_plane->crtc_y = state->orig_dst.y1;
> @@ -1310,15 +1308,7 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>   	intel_plane->obj = obj;
>
>   	if (intel_crtc->active) {
> -		bool primary_was_enabled = intel_crtc->primary_enabled;
> -
> -		intel_crtc->primary_enabled = primary_enabled;
> -
> -		if (primary_was_enabled != primary_enabled)
> -			intel_crtc_wait_for_pending_flips(crtc);

As noted above, it seems that this patch removes the wait for pending flips.


Ander

> -
> -		if (primary_was_enabled && !primary_enabled)
> -			intel_pre_disable_primary(crtc);
> +		intel_crtc->primary_enabled = !state->hides_primary;
>
>   		if (state->visible) {
>   			crtc_x = state->dst.x1;
> @@ -1335,12 +1325,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>   		} else {
>   			intel_plane->disable_plane(plane, crtc);
>   		}
> -
> -
> -		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_SPRITE(pipe));
> -
> -		if (!primary_was_enabled && primary_enabled)
> -			intel_post_enable_primary(crtc);
>   	}
>   }
>
>

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

[PATCH 2/5] drm/i915: Move vblank evasion to commit (v3)

[Matt Roper]

Move the vblank evasion up from the low-level, hw-specific
update_plane() handlers to the general plane commit operation.
Everything inside commit should now be non-sleeping, so this brings us
closer to how vblank evasion will behave once we move over to atomic.

v2:
 - Restore lost intel_crtc->active check on vblank evasion

v3:
 - Replace assert_pipe_enabled() in intel_disable_primary_hw_plane()
   with an intel_crtc->active test; it turns out assert_pipe_enabled()
   grabs some mutexes and can sleep, which we can't do with interrupts
   disabled.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 13 ++++++++++-
 drivers/gpu/drm/i915/intel_drv.h     |  4 ++++
 drivers/gpu/drm/i915/intel_sprite.c  | 42 ------------------------------------
 3 files changed, 16 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5d90114..ce552d1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2165,7 +2165,8 @@ static void intel_disable_primary_hw_plane(struct drm_plane *plane,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
-	assert_pipe_enabled(dev_priv, intel_crtc->pipe);
+	if (WARN_ON(!intel_crtc->active))
+		return;
 
 	if (!intel_crtc->primary_enabled)
 		return;
@@ -11861,6 +11862,12 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 
 	if (intel_crtc->atomic.update_wm)
 		intel_update_watermarks(crtc);
+
+	/* Perform vblank evasion around commit operation */
+	if (intel_crtc->active)
+		intel_crtc->atomic.evade =
+			intel_pipe_update_start(intel_crtc,
+						&intel_crtc->atomic.start_vbl_count);
 }
 
 static void intel_finish_crtc_commit(struct drm_crtc *crtc)
@@ -11869,6 +11876,10 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_plane *p;
 
+	if (intel_crtc->atomic.evade)
+		intel_pipe_update_end(intel_crtc,
+				      intel_crtc->atomic.start_vbl_count);
+
 	if (intel_crtc->atomic.wait_vblank)
 		intel_wait_for_vblank(dev, intel_crtc->pipe);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a03bd72..1934156 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -428,6 +428,10 @@ struct skl_pipe_wm {
  * and thus can't be run with interrupts disabled.
  */
 struct intel_crtc_atomic_commit {
+	/* vblank evasion */
+	bool evade;
+	unsigned start_vbl_count;
+
 	/* Sleepable operations to perform before commit */
 	bool wait_for_flips;
 	bool disable_fbc;
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index ff7d6a1..2520748 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -412,8 +412,6 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	u32 sprctl;
 	unsigned long sprsurf_offset, linear_offset;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
-	u32 start_vbl_count;
-	bool atomic_update;
 
 	sprctl = I915_READ(SPCNTR(pipe, plane));
 
@@ -502,8 +500,6 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 		linear_offset += src_h * fb->pitches[0] + src_w * pixel_size;
 	}
 
-	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
-
 	intel_update_primary_plane(intel_crtc);
 
 	if (IS_CHERRYVIEW(dev) && pipe == PIPE_B)
@@ -525,9 +521,6 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 		   sprsurf_offset);
 
 	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
-
-	if (atomic_update)
-		intel_pipe_update_end(intel_crtc, start_vbl_count);
 }
 
 static void
@@ -539,10 +532,6 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	int plane = intel_plane->plane;
-	u32 start_vbl_count;
-	bool atomic_update;
-
-	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	intel_update_primary_plane(intel_crtc);
 
@@ -553,9 +542,6 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 
 	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
-	if (atomic_update)
-		intel_pipe_update_end(intel_crtc, start_vbl_count);
-
 	intel_update_sprite_watermarks(dplane, crtc, 0, 0, 0, false, false);
 }
 
@@ -626,8 +612,6 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	u32 sprctl, sprscale = 0;
 	unsigned long sprsurf_offset, linear_offset;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
-	u32 start_vbl_count;
-	bool atomic_update;
 
 	sprctl = I915_READ(SPRCTL(pipe));
 
@@ -711,8 +695,6 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		}
 	}
 
-	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
-
 	intel_update_primary_plane(intel_crtc);
 
 	I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
@@ -735,9 +717,6 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		   i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
 
 	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
-
-	if (atomic_update)
-		intel_pipe_update_end(intel_crtc, start_vbl_count);
 }
 
 static void
@@ -748,10 +727,6 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
-	u32 start_vbl_count;
-	bool atomic_update;
-
-	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	intel_update_primary_plane(intel_crtc);
 
@@ -764,9 +739,6 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 
 	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
-	if (atomic_update)
-		intel_pipe_update_end(intel_crtc, start_vbl_count);
-
 	/*
 	 * Avoid underruns when disabling the sprite.
 	 * FIXME remove once watermark updates are done properly.
@@ -845,8 +817,6 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	unsigned long dvssurf_offset, linear_offset;
 	u32 dvscntr, dvsscale;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
-	u32 start_vbl_count;
-	bool atomic_update;
 
 	dvscntr = I915_READ(DVSCNTR(pipe));
 
@@ -921,8 +891,6 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		linear_offset += src_h * fb->pitches[0] + src_w * pixel_size;
 	}
 
-	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
-
 	intel_update_primary_plane(intel_crtc);
 
 	I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
@@ -940,9 +908,6 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		   i915_gem_obj_ggtt_offset(obj) + dvssurf_offset);
 
 	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
-
-	if (atomic_update)
-		intel_pipe_update_end(intel_crtc, start_vbl_count);
 }
 
 static void
@@ -953,10 +918,6 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
-	u32 start_vbl_count;
-	bool atomic_update;
-
-	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	intel_update_primary_plane(intel_crtc);
 
@@ -968,9 +929,6 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 
 	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
-	if (atomic_update)
-		intel_pipe_update_end(intel_crtc, start_vbl_count);
-
 	/*
 	 * Avoid underruns when disabling the sprite.
 	 * FIXME remove once watermark updates are done properly.
-- 
1.8.5.1

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

Re: [PATCH 2/5] drm/i915: Move vblank evasion to commit (v3)

[Ander Conselvan de Oliveira]

On 12/16/2014 02:23 AM, Matt Roper wrote:
> Move the vblank evasion up from the low-level, hw-specific
> update_plane() handlers to the general plane commit operation.
> Everything inside commit should now be non-sleeping, so this brings us
> closer to how vblank evasion will behave once we move over to atomic.
>
> v2:
>   - Restore lost intel_crtc->active check on vblank evasion
>
> v3:
>   - Replace assert_pipe_enabled() in intel_disable_primary_hw_plane()
>     with an intel_crtc->active test; it turns out assert_pipe_enabled()
>     grabs some mutexes and can sleep, which we can't do with interrupts
>     disabled.

Sounds like this should have gone in the previous patch, or a separate one.

Ander

>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 13 ++++++++++-
>   drivers/gpu/drm/i915/intel_drv.h     |  4 ++++
>   drivers/gpu/drm/i915/intel_sprite.c  | 42 ------------------------------------
>   3 files changed, 16 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5d90114..ce552d1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2165,7 +2165,8 @@ static void intel_disable_primary_hw_plane(struct drm_plane *plane,
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>
> -	assert_pipe_enabled(dev_priv, intel_crtc->pipe);
> +	if (WARN_ON(!intel_crtc->active))
> +		return;
>
>   	if (!intel_crtc->primary_enabled)
>   		return;
> @@ -11861,6 +11862,12 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
>
>   	if (intel_crtc->atomic.update_wm)
>   		intel_update_watermarks(crtc);
> +
> +	/* Perform vblank evasion around commit operation */
> +	if (intel_crtc->active)
> +		intel_crtc->atomic.evade =
> +			intel_pipe_update_start(intel_crtc,
> +						&intel_crtc->atomic.start_vbl_count);
>   }
>
>   static void intel_finish_crtc_commit(struct drm_crtc *crtc)
> @@ -11869,6 +11876,10 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   	struct drm_plane *p;
>
> +	if (intel_crtc->atomic.evade)
> +		intel_pipe_update_end(intel_crtc,
> +				      intel_crtc->atomic.start_vbl_count);
> +
>   	if (intel_crtc->atomic.wait_vblank)
>   		intel_wait_for_vblank(dev, intel_crtc->pipe);
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a03bd72..1934156 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -428,6 +428,10 @@ struct skl_pipe_wm {
>    * and thus can't be run with interrupts disabled.
>    */
>   struct intel_crtc_atomic_commit {
> +	/* vblank evasion */
> +	bool evade;
> +	unsigned start_vbl_count;
> +
>   	/* Sleepable operations to perform before commit */
>   	bool wait_for_flips;
>   	bool disable_fbc;
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index ff7d6a1..2520748 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -412,8 +412,6 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>   	u32 sprctl;
>   	unsigned long sprsurf_offset, linear_offset;
>   	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> -	u32 start_vbl_count;
> -	bool atomic_update;
>
>   	sprctl = I915_READ(SPCNTR(pipe, plane));
>
> @@ -502,8 +500,6 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>   		linear_offset += src_h * fb->pitches[0] + src_w * pixel_size;
>   	}
>
> -	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
> -
>   	intel_update_primary_plane(intel_crtc);
>
>   	if (IS_CHERRYVIEW(dev) && pipe == PIPE_B)
> @@ -525,9 +521,6 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>   		   sprsurf_offset);
>
>   	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
> -
> -	if (atomic_update)
> -		intel_pipe_update_end(intel_crtc, start_vbl_count);
>   }
>
>   static void
> @@ -539,10 +532,6 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   	int pipe = intel_plane->pipe;
>   	int plane = intel_plane->plane;
> -	u32 start_vbl_count;
> -	bool atomic_update;
> -
> -	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
>
>   	intel_update_primary_plane(intel_crtc);
>
> @@ -553,9 +542,6 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
>
>   	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
>
> -	if (atomic_update)
> -		intel_pipe_update_end(intel_crtc, start_vbl_count);
> -
>   	intel_update_sprite_watermarks(dplane, crtc, 0, 0, 0, false, false);
>   }
>
> @@ -626,8 +612,6 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>   	u32 sprctl, sprscale = 0;
>   	unsigned long sprsurf_offset, linear_offset;
>   	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> -	u32 start_vbl_count;
> -	bool atomic_update;
>
>   	sprctl = I915_READ(SPRCTL(pipe));
>
> @@ -711,8 +695,6 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>   		}
>   	}
>
> -	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
> -
>   	intel_update_primary_plane(intel_crtc);
>
>   	I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
> @@ -735,9 +717,6 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>   		   i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
>
>   	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
> -
> -	if (atomic_update)
> -		intel_pipe_update_end(intel_crtc, start_vbl_count);
>   }
>
>   static void
> @@ -748,10 +727,6 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>   	struct intel_plane *intel_plane = to_intel_plane(plane);
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   	int pipe = intel_plane->pipe;
> -	u32 start_vbl_count;
> -	bool atomic_update;
> -
> -	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
>
>   	intel_update_primary_plane(intel_crtc);
>
> @@ -764,9 +739,6 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>
>   	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
>
> -	if (atomic_update)
> -		intel_pipe_update_end(intel_crtc, start_vbl_count);
> -
>   	/*
>   	 * Avoid underruns when disabling the sprite.
>   	 * FIXME remove once watermark updates are done properly.
> @@ -845,8 +817,6 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>   	unsigned long dvssurf_offset, linear_offset;
>   	u32 dvscntr, dvsscale;
>   	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> -	u32 start_vbl_count;
> -	bool atomic_update;
>
>   	dvscntr = I915_READ(DVSCNTR(pipe));
>
> @@ -921,8 +891,6 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>   		linear_offset += src_h * fb->pitches[0] + src_w * pixel_size;
>   	}
>
> -	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
> -
>   	intel_update_primary_plane(intel_crtc);
>
>   	I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
> @@ -940,9 +908,6 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>   		   i915_gem_obj_ggtt_offset(obj) + dvssurf_offset);
>
>   	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
> -
> -	if (atomic_update)
> -		intel_pipe_update_end(intel_crtc, start_vbl_count);
>   }
>
>   static void
> @@ -953,10 +918,6 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>   	struct intel_plane *intel_plane = to_intel_plane(plane);
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   	int pipe = intel_plane->pipe;
> -	u32 start_vbl_count;
> -	bool atomic_update;
> -
> -	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
>
>   	intel_update_primary_plane(intel_crtc);
>
> @@ -968,9 +929,6 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>
>   	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
>
> -	if (atomic_update)
> -		intel_pipe_update_end(intel_crtc, start_vbl_count);
> -
>   	/*
>   	 * Avoid underruns when disabling the sprite.
>   	 * FIXME remove once watermark updates are done properly.
>

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

[PATCH 3/5] drm/i915: Clarify sprite plane function names (v3)

[Matt Roper]

A few of the sprite-related function names in i915 are very similar
(e.g., intel_enable_planes() vs intel_crtc_enable_planes()) and don't
make it clear whether they only operate on sprite planes, or whether
they also apply to all universal plane types.  Rename a few functions to
be more consistent with our function naming for primary/cursor planes or
to clarify that they apply specifically to sprite planes:

 - s/intel_disable_planes/intel_disable_sprite_planes/
 - s/intel_enable_planes/intel_enable_sprite_planes/

Also, drop the sprite-specific intel_destroy_plane() and just use
the type-agnostic intel_plane_destroy() function.  The extra 'disable'
call that intel_destroy_plane() did is unnecessary since the plane will
already be disabled due to framebuffer destruction by the point it gets
called.

v2: Earlier consolidation patches have reduced the number of functions
    we need to rename here.

v3: Also rename intel_plane_funcs vtable to intel_sprite_plane_funcs
    for consistency with primary/cursor.  (Ander)

Reviewed-by(v1): Bob Paauwe <bob.j.paauwe@intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 10 +++++-----
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_sprite.c  | 14 +++-----------
 3 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ce552d1..030cf93 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4037,7 +4037,7 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc)
 	}
 }
 
-static void intel_enable_planes(struct drm_crtc *crtc)
+static void intel_enable_sprite_planes(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	enum pipe pipe = to_intel_crtc(crtc)->pipe;
@@ -4051,7 +4051,7 @@ static void intel_enable_planes(struct drm_crtc *crtc)
 	}
 }
 
-static void intel_disable_planes(struct drm_crtc *crtc)
+static void intel_disable_sprite_planes(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	enum pipe pipe = to_intel_crtc(crtc)->pipe;
@@ -4195,7 +4195,7 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
 	int pipe = intel_crtc->pipe;
 
 	intel_enable_primary_hw_plane(crtc->primary, crtc);
-	intel_enable_planes(crtc);
+	intel_enable_sprite_planes(crtc);
 	intel_crtc_update_cursor(crtc, true);
 	intel_crtc_dpms_overlay(intel_crtc, true);
 
@@ -4230,7 +4230,7 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc)
 
 	intel_crtc_dpms_overlay(intel_crtc, false);
 	intel_crtc_update_cursor(crtc, false);
-	intel_disable_planes(crtc);
+	intel_disable_sprite_planes(crtc);
 	intel_disable_primary_hw_plane(crtc->primary, crtc);
 
 	/*
@@ -12008,7 +12008,7 @@ intel_disable_plane(struct drm_plane *plane)
 }
 
 /* Common destruction function for both primary and cursor planes */
-static void intel_plane_destroy(struct drm_plane *plane)
+void intel_plane_destroy(struct drm_plane *plane)
 {
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	drm_plane_cleanup(plane);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1934156..2523315 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1053,6 +1053,7 @@ int intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		       uint32_t src_x, uint32_t src_y,
 		       uint32_t src_w, uint32_t src_h);
 int intel_disable_plane(struct drm_plane *plane);
+void intel_plane_destroy(struct drm_plane *plane);
 
 /* intel_dp_mst.c */
 int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 2520748..a689c73 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1286,14 +1286,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 	}
 }
 
-static void intel_destroy_plane(struct drm_plane *plane)
-{
-	struct intel_plane *intel_plane = to_intel_plane(plane);
-	intel_disable_plane(plane);
-	drm_plane_cleanup(plane);
-	kfree(intel_plane);
-}
-
 int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
 			      struct drm_file *file_priv)
 {
@@ -1393,10 +1385,10 @@ int intel_plane_restore(struct drm_plane *plane)
 				  intel_plane->src_w, intel_plane->src_h);
 }
 
-static const struct drm_plane_funcs intel_plane_funcs = {
+static const struct drm_plane_funcs intel_sprite_plane_funcs = {
 	.update_plane = intel_update_plane,
 	.disable_plane = intel_disable_plane,
-	.destroy = intel_destroy_plane,
+	.destroy = intel_plane_destroy,
 	.set_property = intel_plane_set_property,
 };
 
@@ -1533,7 +1525,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 	intel_plane->commit_plane = intel_commit_sprite_plane;
 	possible_crtcs = (1 << pipe);
 	ret = drm_universal_plane_init(dev, &intel_plane->base, possible_crtcs,
-				       &intel_plane_funcs,
+				       &intel_sprite_plane_funcs,
 				       plane_formats, num_plane_formats,
 				       DRM_PLANE_TYPE_OVERLAY);
 	if (ret) {
-- 
1.8.5.1

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

Re: [PATCH 3/5] drm/i915: Clarify sprite plane function names (v3)

[Ander Conselvan de Oliveira]

On 12/16/2014 02:23 AM, Matt Roper wrote:
> A few of the sprite-related function names in i915 are very similar
> (e.g., intel_enable_planes() vs intel_crtc_enable_planes()) and don't
> make it clear whether they only operate on sprite planes, or whether
> they also apply to all universal plane types.  Rename a few functions to
> be more consistent with our function naming for primary/cursor planes or
> to clarify that they apply specifically to sprite planes:
>
>   - s/intel_disable_planes/intel_disable_sprite_planes/
>   - s/intel_enable_planes/intel_enable_sprite_planes/
>
> Also, drop the sprite-specific intel_destroy_plane() and just use
> the type-agnostic intel_plane_destroy() function.  The extra 'disable'
> call that intel_destroy_plane() did is unnecessary since the plane will
> already be disabled due to framebuffer destruction by the point it gets
> called.
>
> v2: Earlier consolidation patches have reduced the number of functions
>      we need to rename here.
>
> v3: Also rename intel_plane_funcs vtable to intel_sprite_plane_funcs
>      for consistency with primary/cursor.  (Ander)
>
> Reviewed-by(v1): Bob Paauwe <bob.j.paauwe@intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 10 +++++-----
>   drivers/gpu/drm/i915/intel_drv.h     |  1 +
>   drivers/gpu/drm/i915/intel_sprite.c  | 14 +++-----------
>   3 files changed, 9 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ce552d1..030cf93 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4037,7 +4037,7 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc)
>   	}
>   }
>
> -static void intel_enable_planes(struct drm_crtc *crtc)
> +static void intel_enable_sprite_planes(struct drm_crtc *crtc)
>   {
>   	struct drm_device *dev = crtc->dev;
>   	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> @@ -4051,7 +4051,7 @@ static void intel_enable_planes(struct drm_crtc *crtc)
>   	}
>   }
>
> -static void intel_disable_planes(struct drm_crtc *crtc)
> +static void intel_disable_sprite_planes(struct drm_crtc *crtc)
>   {
>   	struct drm_device *dev = crtc->dev;
>   	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> @@ -4195,7 +4195,7 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
>   	int pipe = intel_crtc->pipe;
>
>   	intel_enable_primary_hw_plane(crtc->primary, crtc);
> -	intel_enable_planes(crtc);
> +	intel_enable_sprite_planes(crtc);
>   	intel_crtc_update_cursor(crtc, true);
>   	intel_crtc_dpms_overlay(intel_crtc, true);
>
> @@ -4230,7 +4230,7 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc)
>
>   	intel_crtc_dpms_overlay(intel_crtc, false);
>   	intel_crtc_update_cursor(crtc, false);
> -	intel_disable_planes(crtc);
> +	intel_disable_sprite_planes(crtc);
>   	intel_disable_primary_hw_plane(crtc->primary, crtc);
>
>   	/*
> @@ -12008,7 +12008,7 @@ intel_disable_plane(struct drm_plane *plane)
>   }
>
>   /* Common destruction function for both primary and cursor planes */
> -static void intel_plane_destroy(struct drm_plane *plane)
> +void intel_plane_destroy(struct drm_plane *plane)

Do we need kernel doc for this?

Ander

>   {
>   	struct intel_plane *intel_plane = to_intel_plane(plane);
>   	drm_plane_cleanup(plane);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1934156..2523315 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1053,6 +1053,7 @@ int intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>   		       uint32_t src_x, uint32_t src_y,
>   		       uint32_t src_w, uint32_t src_h);
>   int intel_disable_plane(struct drm_plane *plane);
> +void intel_plane_destroy(struct drm_plane *plane);
>
>   /* intel_dp_mst.c */
>   int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 2520748..a689c73 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1286,14 +1286,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>   	}
>   }
>
> -static void intel_destroy_plane(struct drm_plane *plane)
> -{
> -	struct intel_plane *intel_plane = to_intel_plane(plane);
> -	intel_disable_plane(plane);
> -	drm_plane_cleanup(plane);
> -	kfree(intel_plane);
> -}
> -
>   int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
>   			      struct drm_file *file_priv)
>   {
> @@ -1393,10 +1385,10 @@ int intel_plane_restore(struct drm_plane *plane)
>   				  intel_plane->src_w, intel_plane->src_h);
>   }
>
> -static const struct drm_plane_funcs intel_plane_funcs = {
> +static const struct drm_plane_funcs intel_sprite_plane_funcs = {
>   	.update_plane = intel_update_plane,
>   	.disable_plane = intel_disable_plane,
> -	.destroy = intel_destroy_plane,
> +	.destroy = intel_plane_destroy,
>   	.set_property = intel_plane_set_property,
>   };
>
> @@ -1533,7 +1525,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>   	intel_plane->commit_plane = intel_commit_sprite_plane;
>   	possible_crtcs = (1 << pipe);
>   	ret = drm_universal_plane_init(dev, &intel_plane->base, possible_crtcs,
> -				       &intel_plane_funcs,
> +				       &intel_sprite_plane_funcs,
>   				       plane_formats, num_plane_formats,
>   				       DRM_PLANE_TYPE_OVERLAY);
>   	if (ret) {
>

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

Re: [PATCH 3/5] drm/i915: Clarify sprite plane function names (v3)

[Daniel Vetter]

On Tue, Dec 23, 2014 at 12:08:29PM +0200, Ander Conselvan de Oliveira wrote:
> On 12/16/2014 02:23 AM, Matt Roper wrote:
> >A few of the sprite-related function names in i915 are very similar
> >(e.g., intel_enable_planes() vs intel_crtc_enable_planes()) and don't
> >make it clear whether they only operate on sprite planes, or whether
> >they also apply to all universal plane types.  Rename a few functions to
> >be more consistent with our function naming for primary/cursor planes or
> >to clarify that they apply specifically to sprite planes:
> >
> >  - s/intel_disable_planes/intel_disable_sprite_planes/
> >  - s/intel_enable_planes/intel_enable_sprite_planes/
> >
> >Also, drop the sprite-specific intel_destroy_plane() and just use
> >the type-agnostic intel_plane_destroy() function.  The extra 'disable'
> >call that intel_destroy_plane() did is unnecessary since the plane will
> >already be disabled due to framebuffer destruction by the point it gets
> >called.
> >
> >v2: Earlier consolidation patches have reduced the number of functions
> >     we need to rename here.
> >
> >v3: Also rename intel_plane_funcs vtable to intel_sprite_plane_funcs
> >     for consistency with primary/cursor.  (Ander)
> >
> >Reviewed-by(v1): Bob Paauwe <bob.j.paauwe@intel.com>
> >Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> >---
> >  drivers/gpu/drm/i915/intel_display.c | 10 +++++-----
> >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> >  drivers/gpu/drm/i915/intel_sprite.c  | 14 +++-----------
> >  3 files changed, 9 insertions(+), 16 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >index ce552d1..030cf93 100644
> >--- a/drivers/gpu/drm/i915/intel_display.c
> >+++ b/drivers/gpu/drm/i915/intel_display.c
> >@@ -4037,7 +4037,7 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc)
> >  	}
> >  }
> >
> >-static void intel_enable_planes(struct drm_crtc *crtc)
> >+static void intel_enable_sprite_planes(struct drm_crtc *crtc)
> >  {
> >  	struct drm_device *dev = crtc->dev;
> >  	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> >@@ -4051,7 +4051,7 @@ static void intel_enable_planes(struct drm_crtc *crtc)
> >  	}
> >  }
> >
> >-static void intel_disable_planes(struct drm_crtc *crtc)
> >+static void intel_disable_sprite_planes(struct drm_crtc *crtc)
> >  {
> >  	struct drm_device *dev = crtc->dev;
> >  	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> >@@ -4195,7 +4195,7 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
> >  	int pipe = intel_crtc->pipe;
> >
> >  	intel_enable_primary_hw_plane(crtc->primary, crtc);
> >-	intel_enable_planes(crtc);
> >+	intel_enable_sprite_planes(crtc);
> >  	intel_crtc_update_cursor(crtc, true);
> >  	intel_crtc_dpms_overlay(intel_crtc, true);
> >
> >@@ -4230,7 +4230,7 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc)
> >
> >  	intel_crtc_dpms_overlay(intel_crtc, false);
> >  	intel_crtc_update_cursor(crtc, false);
> >-	intel_disable_planes(crtc);
> >+	intel_disable_sprite_planes(crtc);
> >  	intel_disable_primary_hw_plane(crtc->primary, crtc);
> >
> >  	/*
> >@@ -12008,7 +12008,7 @@ intel_disable_plane(struct drm_plane *plane)
> >  }
> >
> >  /* Common destruction function for both primary and cursor planes */
> >-static void intel_plane_destroy(struct drm_plane *plane)
> >+void intel_plane_destroy(struct drm_plane *plane)
> 
> Do we need kernel doc for this?

Not sure kerneldoc for all the plane stuff is worth it yet while
everything is in-flux. Probably better to extract the higher-level stuff
into a new file/regroup functions a bit once the dust has settled and then
kerneldoc everything.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/5] drm/i915: Move to atomic plane helpers (v8)

[Matt Roper]

Switch plane handling to use the atomic plane helpers.  This means that
rather than provide our own implementations of .update_plane() and
.disable_plane(), we expose the lower-level check/prepare/commit/cleanup
entrypoints and let the DRM core implement update/disable for us using
those entrypoints.

The other main change that falls out of this patch is that our
drm_plane's will now always have a valid plane->state that contains the
relevant plane state (initial state is allocated at plane creation).
The base drm_plane_state pointed to holds the requested source/dest
coordinates, and the subclassed intel_plane_state holds the adjusted
values that our driver actually uses.

v2:
 - Renamed file from intel_atomic.c to intel_atomic_plane.c (Daniel)
 - Fix a copy/paste comment mistake (Bob)

v3:
 - Use prepare/cleanup functions that we've already factored out
 - Use newly refactored pre_commit/commit/post_commit to avoid sleeping
   during vblank evasion

v4:
 - Rebase to latest di-nightly requires adding an 'old_state' parameter
   to atomic_update;

v5:
 - Must have botched a rebase somewhere and lost some work.  Restore
   state 'dirty' flag to let begin/end code know which planes to
   run the pre_commit/post_commit hooks for.  This would have actually
   shown up as broken in the next commit rather than this one.

v6:
 - Squash kerneldoc patch into this one.
 - Previous patches have now already taken care of most of the
   infrastructure that used to be in this patch.  All we're adding here
   now is some thin wrappers.

v7:
 - Check return of intel_plane_duplicate_state() for allocation
   failures.

v8:
 - Drop unused drm_plane_state -> intel_plane_state cast.  (Ander)
 - Squash in actual transition to plane helpers.  Significant
   refactoring earlier in the patchset has made the combined
   prep+transition much easier to swallow than it was in earlier
   iterations. (Ander)

Testcase: igt/kms_plane
Testcase: igt/kms_universal_plane
Testcase: igt/kms_cursor_crc
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 Documentation/DocBook/drm.tmpl            |   5 +
 drivers/gpu/drm/i915/Makefile             |   1 +
 drivers/gpu/drm/i915/intel_atomic_plane.c | 150 ++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c      | 250 ++++++++++++------------------
 drivers/gpu/drm/i915/intel_drv.h          |  10 +-
 drivers/gpu/drm/i915/intel_sprite.c       |  50 ++++--
 6 files changed, 301 insertions(+), 165 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_atomic_plane.c

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 995dfb2..c033a0d 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -3932,6 +3932,11 @@ int num_ioctls;</synopsis>
         </para>
       </sect2>
       <sect2>
+        <title>Atomic Plane Helpers</title>
+!Pdrivers/gpu/drm/i915/intel_atomic_plane.c atomic plane helpers
+!Idrivers/gpu/drm/i915/intel_atomic_plane.c
+      </sect2>
+      <sect2>
         <title>Output Probing</title>
         <para>
 	  This section covers output probing and related infrastructure like the
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 1849ffa..16e3dc3 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -66,6 +66,7 @@ i915-y += dvo_ch7017.o \
 	  dvo_ns2501.o \
 	  dvo_sil164.o \
 	  dvo_tfp410.o \
+	  intel_atomic_plane.o \
 	  intel_crt.o \
 	  intel_ddi.o \
 	  intel_dp.o \
diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
new file mode 100644
index 0000000..286fec8
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -0,0 +1,150 @@
+/*
+ * Copyright © 2014 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+/**
+ * DOC: atomic plane helper support
+ *
+ * The functions here are used by the atomic plane helper functions to
+ * implement legacy plane updates (i.e., drm_plane->update_plane() and
+ * drm_plane->disable_plane()).  This allows plane updates to use the
+ * atomic state infrastructure and perform plane updates as separate
+ * prepare/check/commit/cleanup steps.
+ */
+
+#include <drm/drmP.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_plane_helper.h>
+#include "intel_drv.h"
+
+/**
+ * intel_plane_duplicate_state - duplicate plane state
+ * @plane: drm plane
+ *
+ * Allocates and returns a copy of the plane state (both common and
+ * Intel-specific) for the specified plane.
+ *
+ * Returns: The newly allocated plane state, or NULL or failure.
+ */
+struct drm_plane_state *
+intel_plane_duplicate_state(struct drm_plane *plane)
+{
+	struct intel_plane_state *state;
+
+	if (plane->state)
+		state = kmemdup(plane->state, sizeof(*state), GFP_KERNEL);
+	else
+		state = kzalloc(sizeof(*state), GFP_KERNEL);
+
+	if (!state)
+		return NULL;
+
+	if (state->base.fb)
+		drm_framebuffer_reference(state->base.fb);
+
+	return &state->base;
+}
+
+/**
+ * intel_plane_destroy_state - destroy plane state
+ * @plane: drm plane
+ *
+ * Destroys the plane state (both common and Intel-specific) for the
+ * specified plane.
+ */
+void
+intel_plane_destroy_state(struct drm_plane *plane,
+			  struct drm_plane_state *state)
+{
+	drm_atomic_helper_plane_destroy_state(plane, state);
+}
+
+static int intel_plane_atomic_check(struct drm_plane *plane,
+				    struct drm_plane_state *state)
+{
+	struct drm_crtc *crtc = state->crtc;
+	struct intel_crtc *intel_crtc;
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_plane_state *intel_state = to_intel_plane_state(state);
+
+	crtc = crtc ? crtc : plane->crtc;
+	intel_crtc = to_intel_crtc(crtc);
+
+	/*
+	 * The original src/dest coordinates are stored in state->base, but
+	 * we want to keep another copy internal to our driver that we can
+	 * clip/modify ourselves.
+	 */
+	intel_state->src.x1 = state->src_x;
+	intel_state->src.y1 = state->src_y;
+	intel_state->src.x2 = state->src_x + state->src_w;
+	intel_state->src.y2 = state->src_y + state->src_h;
+	intel_state->dst.x1 = state->crtc_x;
+	intel_state->dst.y1 = state->crtc_y;
+	intel_state->dst.x2 = state->crtc_x + state->crtc_w;
+	intel_state->dst.y2 = state->crtc_y + state->crtc_h;
+
+	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
+	intel_state->clip.x1 = 0;
+	intel_state->clip.y1 = 0;
+	intel_state->clip.x2 =
+		intel_crtc->active ? intel_crtc->config.pipe_src_w : 0;
+	intel_state->clip.y2 =
+		intel_crtc->active ? intel_crtc->config.pipe_src_h : 0;
+
+	/*
+	 * Disabling a plane is always okay; we just need to update
+	 * fb tracking in a special way since cleanup_fb() won't
+	 * get called by the plane helpers.
+	 */
+	if (state->fb == NULL && plane->state->fb != NULL) {
+		/*
+		 * 'prepare' is never called when plane is being disabled, so
+		 * we need to handle frontbuffer tracking as a special case
+		 */
+		intel_crtc->atomic.track_fbs |= (1 << drm_plane_index(plane));
+	}
+
+	return intel_plane->check_plane(plane, intel_state);
+}
+
+static void intel_plane_atomic_update(struct drm_plane *plane,
+				      struct drm_plane_state *old_state)
+{
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_plane_state *intel_state =
+		to_intel_plane_state(plane->state);
+
+	/* Don't disable an already disabled plane */
+	if (!plane->state->fb && !old_state->fb)
+		return;
+
+	intel_plane->commit_plane(plane, intel_state);
+}
+
+const struct drm_plane_helper_funcs intel_plane_helper_funcs = {
+	.prepare_fb = intel_prepare_plane_fb,
+	.cleanup_fb = intel_cleanup_plane_fb,
+	.atomic_check = intel_plane_atomic_check,
+	.atomic_update = intel_plane_atomic_update,
+};
+
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 030cf93..934e6a8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -98,6 +98,8 @@ static void vlv_prepare_pll(struct intel_crtc *crtc,
 			    const struct intel_crtc_config *pipe_config);
 static void chv_prepare_pll(struct intel_crtc *crtc,
 			    const struct intel_crtc_config *pipe_config);
+static void intel_begin_crtc_commit(struct drm_crtc *crtc);
+static void intel_finish_crtc_commit(struct drm_crtc *crtc);
 
 static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
 {
@@ -9794,6 +9796,8 @@ out_hang:
 static struct drm_crtc_helper_funcs intel_helper_funcs = {
 	.mode_set_base_atomic = intel_pipe_set_base_atomic,
 	.load_lut = intel_crtc_load_lut,
+	.atomic_begin = intel_begin_crtc_commit,
+	.atomic_flush = intel_finish_crtc_commit,
 };
 
 /**
@@ -11674,7 +11678,7 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 	unsigned frontbuffer_bits = 0;
 	int ret = 0;
 
-	if (WARN_ON(fb == plane->fb || !obj))
+	if (!obj)
 		return 0;
 
 	switch (plane->type) {
@@ -11741,7 +11745,7 @@ intel_check_primary_plane(struct drm_plane *plane,
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc = state->base.crtc;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_crtc *intel_crtc;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_rect *dest = &state->dst;
@@ -11749,6 +11753,9 @@ intel_check_primary_plane(struct drm_plane *plane,
 	const struct drm_rect *clip = &state->clip;
 	int ret;
 
+	crtc = crtc ? crtc : plane->crtc;
+	intel_crtc = to_intel_crtc(crtc);
+
 	ret = drm_plane_helper_check_update(plane, crtc, fb,
 					    src, dest, clip,
 					    DRM_PLANE_HELPER_NO_SCALING,
@@ -11808,23 +11815,26 @@ intel_commit_primary_plane(struct drm_plane *plane,
 	struct drm_framebuffer *fb = state->base.fb;
 	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_crtc *intel_crtc;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_rect *src = &state->src;
 
+	crtc = crtc ? crtc : plane->crtc;
+	intel_crtc = to_intel_crtc(crtc);
+
 	plane->fb = fb;
 	crtc->x = src->x1 >> 16;
 	crtc->y = src->y1 >> 16;
 
-	intel_plane->crtc_x = state->orig_dst.x1;
-	intel_plane->crtc_y = state->orig_dst.y1;
-	intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
-	intel_plane->crtc_h = drm_rect_height(&state->orig_dst);
-	intel_plane->src_x = state->orig_src.x1;
-	intel_plane->src_y = state->orig_src.y1;
-	intel_plane->src_w = drm_rect_width(&state->orig_src);
-	intel_plane->src_h = drm_rect_height(&state->orig_src);
+	intel_plane->crtc_x = state->base.crtc_x;
+	intel_plane->crtc_y = state->base.crtc_y;
+	intel_plane->crtc_w = state->base.crtc_w;
+	intel_plane->crtc_h = state->base.crtc_h;
+	intel_plane->src_x = state->base.src_x;
+	intel_plane->src_y = state->base.src_y;
+	intel_plane->src_w = state->base.src_w;
+	intel_plane->src_h = state->base.src_h;
 	intel_plane->obj = obj;
 
 	if (intel_crtc->active) {
@@ -11853,6 +11863,32 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_plane *intel_plane;
+	struct drm_plane *p;
+	unsigned fb_bits = 0;
+
+	/* Track fb's for any planes being disabled */
+	list_for_each_entry(p, &dev->mode_config.plane_list, head) {
+		intel_plane = to_intel_plane(p);
+
+		if (intel_crtc->atomic.track_fbs & (1 << drm_plane_index(p))) {
+			switch (p->type) {
+			case DRM_PLANE_TYPE_PRIMARY:
+				fb_bits = INTEL_FRONTBUFFER_PRIMARY(intel_plane->pipe);
+				break;
+			case DRM_PLANE_TYPE_CURSOR:
+				fb_bits = INTEL_FRONTBUFFER_CURSOR(intel_plane->pipe);
+				break;
+			case DRM_PLANE_TYPE_OVERLAY:
+				fb_bits = INTEL_FRONTBUFFER_SPRITE(intel_plane->pipe);
+				break;
+			}
+
+			mutex_lock(&dev->struct_mutex);
+			i915_gem_track_fb(intel_fb_obj(p->fb), NULL, fb_bits);
+			mutex_unlock(&dev->struct_mutex);
+		}
+	}
 
 	if (intel_crtc->atomic.disable_fbc)
 		intel_fbc_disable(dev);
@@ -11902,124 +11938,23 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 	memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
 }
 
-int
-intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
-		   struct drm_framebuffer *fb, int crtc_x, int crtc_y,
-		   unsigned int crtc_w, unsigned int crtc_h,
-		   uint32_t src_x, uint32_t src_y,
-		   uint32_t src_w, uint32_t src_h)
-{
-	struct drm_device *dev = plane->dev;
-	struct drm_framebuffer *old_fb = plane->fb;
-	struct intel_plane_state state = {{ 0 }};
-	struct intel_plane *intel_plane = to_intel_plane(plane);
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int ret;
-
-	state.base.crtc = crtc ? crtc : plane->crtc;
-	state.base.fb = fb;
-
-	/* sample coordinates in 16.16 fixed point */
-	state.src.x1 = src_x;
-	state.src.x2 = src_x + src_w;
-	state.src.y1 = src_y;
-	state.src.y2 = src_y + src_h;
-
-	/* integer pixels */
-	state.dst.x1 = crtc_x;
-	state.dst.x2 = crtc_x + crtc_w;
-	state.dst.y1 = crtc_y;
-	state.dst.y2 = crtc_y + crtc_h;
-
-	state.clip.x1 = 0;
-	state.clip.y1 = 0;
-	state.clip.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0;
-	state.clip.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0;
-
-	state.orig_src = state.src;
-	state.orig_dst = state.dst;
-
-	ret = intel_plane->check_plane(plane, &state);
-	if (ret)
-		return ret;
-
-	if (fb != old_fb && fb) {
-		ret = intel_prepare_plane_fb(plane, fb);
-		if (ret)
-			return ret;
-	}
-
-	if (!state.base.fb) {
-		unsigned fb_bits = 0;
-
-		switch (plane->type) {
-		case DRM_PLANE_TYPE_PRIMARY:
-			fb_bits = INTEL_FRONTBUFFER_PRIMARY(intel_plane->pipe);
-			break;
-		case DRM_PLANE_TYPE_CURSOR:
-			fb_bits = INTEL_FRONTBUFFER_CURSOR(intel_plane->pipe);
-			break;
-		case DRM_PLANE_TYPE_OVERLAY:
-			fb_bits = INTEL_FRONTBUFFER_SPRITE(intel_plane->pipe);
-			break;
-		}
-
-		/*
-		 * 'prepare' is never called when plane is being disabled, so
-		 * we need to handle frontbuffer tracking here
-		 */
-		mutex_lock(&dev->struct_mutex);
-		i915_gem_track_fb(intel_fb_obj(plane->fb), NULL, fb_bits);
-		mutex_unlock(&dev->struct_mutex);
-	}
-
-	intel_begin_crtc_commit(crtc);
-	intel_plane->commit_plane(plane, &state);
-	intel_finish_crtc_commit(crtc);
-
-	if (fb != old_fb && old_fb) {
-		if (intel_crtc->active)
-			intel_wait_for_vblank(dev, intel_crtc->pipe);
-		intel_cleanup_plane_fb(plane, old_fb);
-	}
-
-	plane->fb = fb;
-
-	return 0;
-}
-
-/**
- * intel_disable_plane - disable a plane
- * @plane: plane to disable
- *
- * General disable handler for all plane types.
- */
-int
-intel_disable_plane(struct drm_plane *plane)
-{
-	if (!plane->fb)
-		return 0;
-
-	if (WARN_ON(!plane->crtc))
-		return -EINVAL;
-
-	return plane->funcs->update_plane(plane, plane->crtc, NULL,
-					  0, 0, 0, 0, 0, 0, 0, 0);
-}
-
 /* Common destruction function for both primary and cursor planes */
 void intel_plane_destroy(struct drm_plane *plane)
 {
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	intel_plane_destroy_state(plane, plane->state);
 	drm_plane_cleanup(plane);
 	kfree(intel_plane);
 }
 
 static const struct drm_plane_funcs intel_primary_plane_funcs = {
-	.update_plane = intel_update_plane,
-	.disable_plane = intel_disable_plane,
+	.update_plane = drm_plane_helper_update,
+	.disable_plane = drm_plane_helper_disable,
 	.destroy = intel_plane_destroy,
-	.set_property = intel_plane_set_property
+	.set_property = intel_plane_set_property,
+	.atomic_duplicate_state = intel_plane_duplicate_state,
+	.atomic_destroy_state = intel_plane_destroy_state,
+
 };
 
 static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
@@ -12033,6 +11968,12 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
 	if (primary == NULL)
 		return NULL;
 
+	primary->base.state = intel_plane_duplicate_state(&primary->base);
+	if (primary->base.state == NULL) {
+		kfree(primary);
+		return NULL;
+	}
+
 	primary->can_scale = false;
 	primary->max_downscale = 1;
 	primary->pipe = pipe;
@@ -12068,6 +12009,8 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
 				primary->rotation);
 	}
 
+	drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
+
 	return &primary->base;
 }
 
@@ -12076,17 +12019,19 @@ intel_check_cursor_plane(struct drm_plane *plane,
 			 struct intel_plane_state *state)
 {
 	struct drm_crtc *crtc = state->base.crtc;
-	struct drm_device *dev = crtc->dev;
+	struct drm_device *dev = plane->dev;
 	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_rect *dest = &state->dst;
 	struct drm_rect *src = &state->src;
 	const struct drm_rect *clip = &state->clip;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int crtc_w, crtc_h;
+	struct intel_crtc *intel_crtc;
 	unsigned stride;
 	int ret;
 
+	crtc = crtc ? crtc : plane->crtc;
+	intel_crtc = to_intel_crtc(crtc);
+
 	ret = drm_plane_helper_check_update(plane, crtc, fb,
 					    src, dest, clip,
 					    DRM_PLANE_HELPER_NO_SCALING,
@@ -12101,15 +12046,14 @@ intel_check_cursor_plane(struct drm_plane *plane,
 		goto finish;
 
 	/* Check for which cursor types we support */
-	crtc_w = drm_rect_width(&state->orig_dst);
-	crtc_h = drm_rect_height(&state->orig_dst);
-	if (!cursor_size_ok(dev, crtc_w, crtc_h)) {
-		DRM_DEBUG("Cursor dimension not supported\n");
+	if (!cursor_size_ok(dev, state->base.crtc_w, state->base.crtc_h)) {
+		DRM_DEBUG("Cursor dimension %dx%d not supported\n",
+			  state->base.crtc_w, state->base.crtc_h);
 		return -EINVAL;
 	}
 
-	stride = roundup_pow_of_two(crtc_w) * 4;
-	if (obj->base.size < stride * crtc_h) {
+	stride = roundup_pow_of_two(state->base.crtc_w) * 4;
+	if (obj->base.size < stride * state->base.crtc_h) {
 		DRM_DEBUG_KMS("buffer is too small\n");
 		return -ENOMEM;
 	}
@@ -12127,8 +12071,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
 
 finish:
 	if (intel_crtc->active) {
-		if (intel_crtc->cursor_width !=
-		    drm_rect_width(&state->orig_dst))
+		if (intel_crtc->cursor_width != state->base.crtc_w)
 			intel_crtc->atomic.update_wm = true;
 
 		intel_crtc->atomic.fb_bits |=
@@ -12143,24 +12086,27 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
 	struct drm_crtc *crtc = state->base.crtc;
-	struct drm_device *dev = crtc->dev;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_device *dev = plane->dev;
+	struct intel_crtc *intel_crtc;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
 	uint32_t addr;
 
+	crtc = crtc ? crtc : plane->crtc;
+	intel_crtc = to_intel_crtc(crtc);
+
 	plane->fb = state->base.fb;
-	crtc->cursor_x = state->orig_dst.x1;
-	crtc->cursor_y = state->orig_dst.y1;
-
-	intel_plane->crtc_x = state->orig_dst.x1;
-	intel_plane->crtc_y = state->orig_dst.y1;
-	intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
-	intel_plane->crtc_h = drm_rect_height(&state->orig_dst);
-	intel_plane->src_x = state->orig_src.x1;
-	intel_plane->src_y = state->orig_src.y1;
-	intel_plane->src_w = drm_rect_width(&state->orig_src);
-	intel_plane->src_h = drm_rect_height(&state->orig_src);
+	crtc->cursor_x = state->base.crtc_x;
+	crtc->cursor_y = state->base.crtc_y;
+
+	intel_plane->crtc_x = state->base.crtc_x;
+	intel_plane->crtc_y = state->base.crtc_y;
+	intel_plane->crtc_w = state->base.crtc_w;
+	intel_plane->crtc_h = state->base.crtc_h;
+	intel_plane->src_x = state->base.src_x;
+	intel_plane->src_y = state->base.src_y;
+	intel_plane->src_w = state->base.src_w;
+	intel_plane->src_h = state->base.src_h;
 	intel_plane->obj = obj;
 
 	if (intel_crtc->cursor_bo == obj)
@@ -12176,18 +12122,20 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 	intel_crtc->cursor_addr = addr;
 	intel_crtc->cursor_bo = obj;
 update:
-	intel_crtc->cursor_width = drm_rect_width(&state->orig_dst);
-	intel_crtc->cursor_height = drm_rect_height(&state->orig_dst);
+	intel_crtc->cursor_width = state->base.crtc_w;
+	intel_crtc->cursor_height = state->base.crtc_h;
 
 	if (intel_crtc->active)
 		intel_crtc_update_cursor(crtc, state->visible);
 }
 
 static const struct drm_plane_funcs intel_cursor_plane_funcs = {
-	.update_plane = intel_update_plane,
-	.disable_plane = intel_disable_plane,
+	.update_plane = drm_plane_helper_update,
+	.disable_plane = drm_plane_helper_disable,
 	.destroy = intel_plane_destroy,
 	.set_property = intel_plane_set_property,
+	.atomic_duplicate_state = intel_plane_duplicate_state,
+	.atomic_destroy_state = intel_plane_destroy_state,
 };
 
 static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
@@ -12199,6 +12147,12 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
 	if (cursor == NULL)
 		return NULL;
 
+	cursor->base.state = intel_plane_duplicate_state(&cursor->base);
+	if (cursor->base.state == NULL) {
+		kfree(cursor);
+		return NULL;
+	}
+
 	cursor->can_scale = false;
 	cursor->max_downscale = 1;
 	cursor->pipe = pipe;
@@ -12225,6 +12179,8 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
 				cursor->rotation);
 	}
 
+	drm_plane_helper_add(&cursor->base, &intel_plane_helper_funcs);
+
 	return &cursor->base;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 2523315..ab23190 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -248,8 +248,6 @@ struct intel_plane_state {
 	struct drm_rect src;
 	struct drm_rect dst;
 	struct drm_rect clip;
-	struct drm_rect orig_src;
-	struct drm_rect orig_dst;
 	bool visible;
 
 	/*
@@ -437,6 +435,7 @@ struct intel_crtc_atomic_commit {
 	bool disable_fbc;
 	bool pre_disable_primary;
 	bool update_wm;
+	unsigned track_fbs;
 
 	/* Sleepable operations to perform after commit */
 	unsigned fb_bits;
@@ -575,6 +574,7 @@ struct cxsr_latency {
 #define to_intel_encoder(x) container_of(x, struct intel_encoder, base)
 #define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base)
 #define to_intel_plane(x) container_of(x, struct intel_plane, base)
+#define to_intel_plane_state(x) container_of(x, struct intel_plane_state, base)
 #define intel_fb_obj(x) (x ? to_intel_framebuffer(x)->obj : NULL)
 
 struct intel_hdmi {
@@ -1253,4 +1253,10 @@ void intel_pre_disable_primary(struct drm_crtc *crtc);
 /* intel_tv.c */
 void intel_tv_init(struct drm_device *dev);
 
+/* intel_atomic.c */
+struct drm_plane_state *intel_plane_duplicate_state(struct drm_plane *plane);
+void intel_plane_destroy_state(struct drm_plane *plane,
+			       struct drm_plane_state *state);
+extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
+
 #endif /* __INTEL_DRV_H__ */
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index a689c73..f8efcfd 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -33,6 +33,7 @@
 #include <drm/drm_crtc.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_rect.h>
+#include <drm/drm_plane_helper.h>
 #include "intel_drv.h"
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
@@ -1061,12 +1062,13 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	uint32_t src_x, src_y, src_w, src_h;
 	struct drm_rect *src = &state->src;
 	struct drm_rect *dst = &state->dst;
-	struct drm_rect *orig_src = &state->orig_src;
 	const struct drm_rect *clip = &state->clip;
 	int hscale, vscale;
 	int max_scale, min_scale;
 	int pixel_size;
 
+	intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
+
 	if (!fb) {
 		state->visible = false;
 		goto finish;
@@ -1147,10 +1149,10 @@ intel_check_sprite_plane(struct drm_plane *plane,
 				    intel_plane->rotation);
 
 		/* sanity check to make sure the src viewport wasn't enlarged */
-		WARN_ON(src->x1 < (int) orig_src->x1 ||
-			src->y1 < (int) orig_src->y1 ||
-			src->x2 > (int) orig_src->x2 ||
-			src->y2 > (int) orig_src->y2);
+		WARN_ON(src->x1 < (int) state->base.src_x ||
+			src->y1 < (int) state->base.src_y ||
+			src->x2 > (int) state->base.src_x + state->base.src_w ||
+			src->y2 > (int) state->base.src_y + state->base.src_h);
 
 		/*
 		 * Hardware doesn't handle subpixel coordinates.
@@ -1247,7 +1249,7 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
 	struct drm_crtc *crtc = state->base.crtc;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_crtc *intel_crtc;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
@@ -1255,14 +1257,19 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 	unsigned int crtc_w, crtc_h;
 	uint32_t src_x, src_y, src_w, src_h;
 
-	intel_plane->crtc_x = state->orig_dst.x1;
-	intel_plane->crtc_y = state->orig_dst.y1;
-	intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
-	intel_plane->crtc_h = drm_rect_height(&state->orig_dst);
-	intel_plane->src_x = state->orig_src.x1;
-	intel_plane->src_y = state->orig_src.y1;
-	intel_plane->src_w = drm_rect_width(&state->orig_src);
-	intel_plane->src_h = drm_rect_height(&state->orig_src);
+	intel_plane->crtc_x = state->base.crtc_x;
+	intel_plane->crtc_y = state->base.crtc_y;
+	intel_plane->crtc_w = state->base.crtc_w;
+	intel_plane->crtc_h = state->base.crtc_h;
+	intel_plane->src_x = state->base.src_x;
+	intel_plane->src_y = state->base.src_y;
+	intel_plane->src_w = state->base.src_w;
+	intel_plane->src_h = state->base.src_h;
+
+	crtc = crtc ? crtc : plane->crtc;
+	intel_crtc = to_intel_crtc(crtc);
+
+	plane->fb = state->base.fb;
 	intel_plane->obj = obj;
 
 	if (intel_crtc->active) {
@@ -1386,10 +1393,12 @@ int intel_plane_restore(struct drm_plane *plane)
 }
 
 static const struct drm_plane_funcs intel_sprite_plane_funcs = {
-	.update_plane = intel_update_plane,
-	.disable_plane = intel_disable_plane,
+	.update_plane = drm_plane_helper_update,
+	.disable_plane = drm_plane_helper_disable,
 	.destroy = intel_plane_destroy,
 	.set_property = intel_plane_set_property,
+	.atomic_duplicate_state = intel_plane_duplicate_state,
+	.atomic_destroy_state = intel_plane_destroy_state,
 };
 
 static uint32_t ilk_plane_formats[] = {
@@ -1451,6 +1460,13 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 	if (!intel_plane)
 		return -ENOMEM;
 
+	intel_plane->base.state =
+		intel_plane_duplicate_state(&intel_plane->base);
+	if (intel_plane->base.state == NULL) {
+		kfree(intel_plane);
+		return -ENOMEM;
+	}
+
 	switch (INTEL_INFO(dev)->gen) {
 	case 5:
 	case 6:
@@ -1544,6 +1560,8 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 					   dev->mode_config.rotation_property,
 					   intel_plane->rotation);
 
+	drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
+
  out:
 	return ret;
 }
-- 
1.8.5.1

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

Re: [PATCH 4/5] drm/i915: Move to atomic plane helpers (v8)

[Ander Conselvan de Oliveira]

On 12/16/2014 02:23 AM, Matt Roper wrote:
> Switch plane handling to use the atomic plane helpers.  This means that
> rather than provide our own implementations of .update_plane() and
> .disable_plane(), we expose the lower-level check/prepare/commit/cleanup
> entrypoints and let the DRM core implement update/disable for us using
> those entrypoints.
>
> The other main change that falls out of this patch is that our
> drm_plane's will now always have a valid plane->state that contains the
> relevant plane state (initial state is allocated at plane creation).
> The base drm_plane_state pointed to holds the requested source/dest
> coordinates, and the subclassed intel_plane_state holds the adjusted
> values that our driver actually uses.
>
> v2:
>   - Renamed file from intel_atomic.c to intel_atomic_plane.c (Daniel)
>   - Fix a copy/paste comment mistake (Bob)
>
> v3:
>   - Use prepare/cleanup functions that we've already factored out
>   - Use newly refactored pre_commit/commit/post_commit to avoid sleeping
>     during vblank evasion
>
> v4:
>   - Rebase to latest di-nightly requires adding an 'old_state' parameter
>     to atomic_update;
>
> v5:
>   - Must have botched a rebase somewhere and lost some work.  Restore
>     state 'dirty' flag to let begin/end code know which planes to
>     run the pre_commit/post_commit hooks for.  This would have actually
>     shown up as broken in the next commit rather than this one.
>
> v6:
>   - Squash kerneldoc patch into this one.
>   - Previous patches have now already taken care of most of the
>     infrastructure that used to be in this patch.  All we're adding here
>     now is some thin wrappers.
>
> v7:
>   - Check return of intel_plane_duplicate_state() for allocation
>     failures.
>
> v8:
>   - Drop unused drm_plane_state -> intel_plane_state cast.  (Ander)
>   - Squash in actual transition to plane helpers.  Significant
>     refactoring earlier in the patchset has made the combined
>     prep+transition much easier to swallow than it was in earlier
>     iterations. (Ander)

Patch looks good overall. Just a little bike shedding below.
>
> Testcase: igt/kms_plane
> Testcase: igt/kms_universal_plane
> Testcase: igt/kms_cursor_crc
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   Documentation/DocBook/drm.tmpl            |   5 +
>   drivers/gpu/drm/i915/Makefile             |   1 +
>   drivers/gpu/drm/i915/intel_atomic_plane.c | 150 ++++++++++++++++++
>   drivers/gpu/drm/i915/intel_display.c      | 250 ++++++++++++------------------
>   drivers/gpu/drm/i915/intel_drv.h          |  10 +-
>   drivers/gpu/drm/i915/intel_sprite.c       |  50 ++++--
>   6 files changed, 301 insertions(+), 165 deletions(-)
>   create mode 100644 drivers/gpu/drm/i915/intel_atomic_plane.c
>
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 995dfb2..c033a0d 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -3932,6 +3932,11 @@ int num_ioctls;</synopsis>
>           </para>
>         </sect2>
>         <sect2>
> +        <title>Atomic Plane Helpers</title>
> +!Pdrivers/gpu/drm/i915/intel_atomic_plane.c atomic plane helpers
> +!Idrivers/gpu/drm/i915/intel_atomic_plane.c
> +      </sect2>
> +      <sect2>
>           <title>Output Probing</title>
>           <para>
>   	  This section covers output probing and related infrastructure like the
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 1849ffa..16e3dc3 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -66,6 +66,7 @@ i915-y += dvo_ch7017.o \
>   	  dvo_ns2501.o \
>   	  dvo_sil164.o \
>   	  dvo_tfp410.o \
> +	  intel_atomic_plane.o \
>   	  intel_crt.o \
>   	  intel_ddi.o \
>   	  intel_dp.o \
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> new file mode 100644
> index 0000000..286fec8
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -0,0 +1,150 @@
> +/*
> + * Copyright © 2014 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +/**
> + * DOC: atomic plane helper support
> + *
> + * The functions here are used by the atomic plane helper functions to
> + * implement legacy plane updates (i.e., drm_plane->update_plane() and
> + * drm_plane->disable_plane()).  This allows plane updates to use the
> + * atomic state infrastructure and perform plane updates as separate
> + * prepare/check/commit/cleanup steps.
> + */
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_plane_helper.h>
> +#include "intel_drv.h"
> +
> +/**
> + * intel_plane_duplicate_state - duplicate plane state
> + * @plane: drm plane
> + *
> + * Allocates and returns a copy of the plane state (both common and
> + * Intel-specific) for the specified plane.
> + *
> + * Returns: The newly allocated plane state, or NULL or failure.
> + */
> +struct drm_plane_state *
> +intel_plane_duplicate_state(struct drm_plane *plane)
> +{
> +	struct intel_plane_state *state;
> +
> +	if (plane->state)
> +		state = kmemdup(plane->state, sizeof(*state), GFP_KERNEL);
> +	else
> +		state = kzalloc(sizeof(*state), GFP_KERNEL);
> +
> +	if (!state)
> +		return NULL;
> +
> +	if (state->base.fb)
> +		drm_framebuffer_reference(state->base.fb);
> +
> +	return &state->base;
> +}
> +
> +/**
> + * intel_plane_destroy_state - destroy plane state
> + * @plane: drm plane
> + *
> + * Destroys the plane state (both common and Intel-specific) for the
> + * specified plane.
> + */
> +void
> +intel_plane_destroy_state(struct drm_plane *plane,
> +			  struct drm_plane_state *state)
> +{
> +	drm_atomic_helper_plane_destroy_state(plane, state);
> +}
> +
> +static int intel_plane_atomic_check(struct drm_plane *plane,
> +				    struct drm_plane_state *state)
> +{
> +	struct drm_crtc *crtc = state->crtc;
> +	struct intel_crtc *intel_crtc;
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_plane_state *intel_state = to_intel_plane_state(state);
> +
> +	crtc = crtc ? crtc : plane->crtc;
> +	intel_crtc = to_intel_crtc(crtc);
> +
> +	/*
> +	 * The original src/dest coordinates are stored in state->base, but
> +	 * we want to keep another copy internal to our driver that we can
> +	 * clip/modify ourselves.
> +	 */
> +	intel_state->src.x1 = state->src_x;
> +	intel_state->src.y1 = state->src_y;
> +	intel_state->src.x2 = state->src_x + state->src_w;
> +	intel_state->src.y2 = state->src_y + state->src_h;
> +	intel_state->dst.x1 = state->crtc_x;
> +	intel_state->dst.y1 = state->crtc_y;
> +	intel_state->dst.x2 = state->crtc_x + state->crtc_w;
> +	intel_state->dst.y2 = state->crtc_y + state->crtc_h;
> +
> +	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
> +	intel_state->clip.x1 = 0;
> +	intel_state->clip.y1 = 0;
> +	intel_state->clip.x2 =
> +		intel_crtc->active ? intel_crtc->config.pipe_src_w : 0;
> +	intel_state->clip.y2 =
> +		intel_crtc->active ? intel_crtc->config.pipe_src_h : 0;
> +
> +	/*
> +	 * Disabling a plane is always okay; we just need to update
> +	 * fb tracking in a special way since cleanup_fb() won't
> +	 * get called by the plane helpers.
> +	 */
> +	if (state->fb == NULL && plane->state->fb != NULL) {
> +		/*
> +		 * 'prepare' is never called when plane is being disabled, so
> +		 * we need to handle frontbuffer tracking as a special case
> +		 */
> +		intel_crtc->atomic.track_fbs |= (1 << drm_plane_index(plane));
> +	}
> +
> +	return intel_plane->check_plane(plane, intel_state);
> +}
> +
> +static void intel_plane_atomic_update(struct drm_plane *plane,
> +				      struct drm_plane_state *old_state)
> +{
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_plane_state *intel_state =
> +		to_intel_plane_state(plane->state);
> +
> +	/* Don't disable an already disabled plane */
> +	if (!plane->state->fb && !old_state->fb)
> +		return;
> +
> +	intel_plane->commit_plane(plane, intel_state);
> +}
> +
> +const struct drm_plane_helper_funcs intel_plane_helper_funcs = {
> +	.prepare_fb = intel_prepare_plane_fb,
> +	.cleanup_fb = intel_cleanup_plane_fb,
> +	.atomic_check = intel_plane_atomic_check,
> +	.atomic_update = intel_plane_atomic_update,
> +};
> +
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 030cf93..934e6a8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -98,6 +98,8 @@ static void vlv_prepare_pll(struct intel_crtc *crtc,
>   			    const struct intel_crtc_config *pipe_config);
>   static void chv_prepare_pll(struct intel_crtc *crtc,
>   			    const struct intel_crtc_config *pipe_config);
> +static void intel_begin_crtc_commit(struct drm_crtc *crtc);
> +static void intel_finish_crtc_commit(struct drm_crtc *crtc);
>
>   static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
>   {
> @@ -9794,6 +9796,8 @@ out_hang:
>   static struct drm_crtc_helper_funcs intel_helper_funcs = {
>   	.mode_set_base_atomic = intel_pipe_set_base_atomic,
>   	.load_lut = intel_crtc_load_lut,
> +	.atomic_begin = intel_begin_crtc_commit,
> +	.atomic_flush = intel_finish_crtc_commit,
>   };
>
>   /**
> @@ -11674,7 +11678,7 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>   	unsigned frontbuffer_bits = 0;
>   	int ret = 0;
>
> -	if (WARN_ON(fb == plane->fb || !obj))
> +	if (!obj)
>   		return 0;
>
>   	switch (plane->type) {
> @@ -11741,7 +11745,7 @@ intel_check_primary_plane(struct drm_plane *plane,
>   	struct drm_device *dev = plane->dev;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct drm_crtc *crtc = state->base.crtc;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_crtc *intel_crtc;
>   	struct intel_plane *intel_plane = to_intel_plane(plane);
>   	struct drm_framebuffer *fb = state->base.fb;
>   	struct drm_rect *dest = &state->dst;
> @@ -11749,6 +11753,9 @@ intel_check_primary_plane(struct drm_plane *plane,
>   	const struct drm_rect *clip = &state->clip;
>   	int ret;
>
> +	crtc = crtc ? crtc : plane->crtc;
> +	intel_crtc = to_intel_crtc(crtc);
> +
>   	ret = drm_plane_helper_check_update(plane, crtc, fb,
>   					    src, dest, clip,
>   					    DRM_PLANE_HELPER_NO_SCALING,
> @@ -11808,23 +11815,26 @@ intel_commit_primary_plane(struct drm_plane *plane,
>   	struct drm_framebuffer *fb = state->base.fb;
>   	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_crtc *intel_crtc;
>   	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>   	struct intel_plane *intel_plane = to_intel_plane(plane);
>   	struct drm_rect *src = &state->src;
>
> +	crtc = crtc ? crtc : plane->crtc;
> +	intel_crtc = to_intel_crtc(crtc);
> +
>   	plane->fb = fb;
>   	crtc->x = src->x1 >> 16;
>   	crtc->y = src->y1 >> 16;
>
> -	intel_plane->crtc_x = state->orig_dst.x1;
> -	intel_plane->crtc_y = state->orig_dst.y1;
> -	intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
> -	intel_plane->crtc_h = drm_rect_height(&state->orig_dst);
> -	intel_plane->src_x = state->orig_src.x1;
> -	intel_plane->src_y = state->orig_src.y1;
> -	intel_plane->src_w = drm_rect_width(&state->orig_src);
> -	intel_plane->src_h = drm_rect_height(&state->orig_src);
> +	intel_plane->crtc_x = state->base.crtc_x;
> +	intel_plane->crtc_y = state->base.crtc_y;
> +	intel_plane->crtc_w = state->base.crtc_w;
> +	intel_plane->crtc_h = state->base.crtc_h;
> +	intel_plane->src_x = state->base.src_x;
> +	intel_plane->src_y = state->base.src_y;
> +	intel_plane->src_w = state->base.src_w;
> +	intel_plane->src_h = state->base.src_h;
>   	intel_plane->obj = obj;
>
>   	if (intel_crtc->active) {
> @@ -11853,6 +11863,32 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
>   {
>   	struct drm_device *dev = crtc->dev;
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_plane *intel_plane;
> +	struct drm_plane *p;
> +	unsigned fb_bits = 0;
> +
> +	/* Track fb's for any planes being disabled */
> +	list_for_each_entry(p, &dev->mode_config.plane_list, head) {
> +		intel_plane = to_intel_plane(p);
> +
> +		if (intel_crtc->atomic.track_fbs & (1 << drm_plane_index(p))) {
> +			switch (p->type) {
> +			case DRM_PLANE_TYPE_PRIMARY:
> +				fb_bits = INTEL_FRONTBUFFER_PRIMARY(intel_plane->pipe);
> +				break;
> +			case DRM_PLANE_TYPE_CURSOR:
> +				fb_bits = INTEL_FRONTBUFFER_CURSOR(intel_plane->pipe);
> +				break;
> +			case DRM_PLANE_TYPE_OVERLAY:
> +				fb_bits = INTEL_FRONTBUFFER_SPRITE(intel_plane->pipe);
> +				break;
> +			}
> +
> +			mutex_lock(&dev->struct_mutex);
> +			i915_gem_track_fb(intel_fb_obj(p->fb), NULL, fb_bits);
> +			mutex_unlock(&dev->struct_mutex);
> +		}
> +	}
>
>   	if (intel_crtc->atomic.disable_fbc)
>   		intel_fbc_disable(dev);
> @@ -11902,124 +11938,23 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
>   	memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
>   }
>
> -int
> -intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> -		   struct drm_framebuffer *fb, int crtc_x, int crtc_y,
> -		   unsigned int crtc_w, unsigned int crtc_h,
> -		   uint32_t src_x, uint32_t src_y,
> -		   uint32_t src_w, uint32_t src_h)
> -{
> -	struct drm_device *dev = plane->dev;
> -	struct drm_framebuffer *old_fb = plane->fb;
> -	struct intel_plane_state state = {{ 0 }};
> -	struct intel_plane *intel_plane = to_intel_plane(plane);
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	int ret;
> -
> -	state.base.crtc = crtc ? crtc : plane->crtc;
> -	state.base.fb = fb;
> -
> -	/* sample coordinates in 16.16 fixed point */
> -	state.src.x1 = src_x;
> -	state.src.x2 = src_x + src_w;
> -	state.src.y1 = src_y;
> -	state.src.y2 = src_y + src_h;
> -
> -	/* integer pixels */
> -	state.dst.x1 = crtc_x;
> -	state.dst.x2 = crtc_x + crtc_w;
> -	state.dst.y1 = crtc_y;
> -	state.dst.y2 = crtc_y + crtc_h;
> -
> -	state.clip.x1 = 0;
> -	state.clip.y1 = 0;
> -	state.clip.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0;
> -	state.clip.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0;
> -
> -	state.orig_src = state.src;
> -	state.orig_dst = state.dst;
> -
> -	ret = intel_plane->check_plane(plane, &state);
> -	if (ret)
> -		return ret;
> -
> -	if (fb != old_fb && fb) {
> -		ret = intel_prepare_plane_fb(plane, fb);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	if (!state.base.fb) {
> -		unsigned fb_bits = 0;
> -
> -		switch (plane->type) {
> -		case DRM_PLANE_TYPE_PRIMARY:
> -			fb_bits = INTEL_FRONTBUFFER_PRIMARY(intel_plane->pipe);
> -			break;
> -		case DRM_PLANE_TYPE_CURSOR:
> -			fb_bits = INTEL_FRONTBUFFER_CURSOR(intel_plane->pipe);
> -			break;
> -		case DRM_PLANE_TYPE_OVERLAY:
> -			fb_bits = INTEL_FRONTBUFFER_SPRITE(intel_plane->pipe);
> -			break;
> -		}
> -
> -		/*
> -		 * 'prepare' is never called when plane is being disabled, so
> -		 * we need to handle frontbuffer tracking here
> -		 */
> -		mutex_lock(&dev->struct_mutex);
> -		i915_gem_track_fb(intel_fb_obj(plane->fb), NULL, fb_bits);
> -		mutex_unlock(&dev->struct_mutex);
> -	}
> -
> -	intel_begin_crtc_commit(crtc);
> -	intel_plane->commit_plane(plane, &state);
> -	intel_finish_crtc_commit(crtc);
> -
> -	if (fb != old_fb && old_fb) {
> -		if (intel_crtc->active)
> -			intel_wait_for_vblank(dev, intel_crtc->pipe);
> -		intel_cleanup_plane_fb(plane, old_fb);
> -	}
> -
> -	plane->fb = fb;
> -
> -	return 0;
> -}
> -
> -/**
> - * intel_disable_plane - disable a plane
> - * @plane: plane to disable
> - *
> - * General disable handler for all plane types.
> - */
> -int
> -intel_disable_plane(struct drm_plane *plane)
> -{
> -	if (!plane->fb)
> -		return 0;
> -
> -	if (WARN_ON(!plane->crtc))
> -		return -EINVAL;
> -
> -	return plane->funcs->update_plane(plane, plane->crtc, NULL,
> -					  0, 0, 0, 0, 0, 0, 0, 0);
> -}
> -
>   /* Common destruction function for both primary and cursor planes */
>   void intel_plane_destroy(struct drm_plane *plane)
>   {
>   	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	intel_plane_destroy_state(plane, plane->state);
>   	drm_plane_cleanup(plane);
>   	kfree(intel_plane);
>   }
>
>   static const struct drm_plane_funcs intel_primary_plane_funcs = {
> -	.update_plane = intel_update_plane,
> -	.disable_plane = intel_disable_plane,
> +	.update_plane = drm_plane_helper_update,
> +	.disable_plane = drm_plane_helper_disable,
>   	.destroy = intel_plane_destroy,
> -	.set_property = intel_plane_set_property
> +	.set_property = intel_plane_set_property,
> +	.atomic_duplicate_state = intel_plane_duplicate_state,
> +	.atomic_destroy_state = intel_plane_destroy_state,
> +
>   };
>
>   static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> @@ -12033,6 +11968,12 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>   	if (primary == NULL)
>   		return NULL;
>
> +	primary->base.state = intel_plane_duplicate_state(&primary->base);
> +	if (primary->base.state == NULL) {
> +		kfree(primary);
> +		return NULL;
> +	}
> +
>   	primary->can_scale = false;
>   	primary->max_downscale = 1;
>   	primary->pipe = pipe;
> @@ -12068,6 +12009,8 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>   				primary->rotation);
>   	}
>
> +	drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
> +
>   	return &primary->base;
>   }
>
> @@ -12076,17 +12019,19 @@ intel_check_cursor_plane(struct drm_plane *plane,
>   			 struct intel_plane_state *state)
>   {
>   	struct drm_crtc *crtc = state->base.crtc;
> -	struct drm_device *dev = crtc->dev;
> +	struct drm_device *dev = plane->dev;
>   	struct drm_framebuffer *fb = state->base.fb;
>   	struct drm_rect *dest = &state->dst;
>   	struct drm_rect *src = &state->src;
>   	const struct drm_rect *clip = &state->clip;
>   	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	int crtc_w, crtc_h;
> +	struct intel_crtc *intel_crtc;
>   	unsigned stride;
>   	int ret;
>
> +	crtc = crtc ? crtc : plane->crtc;
> +	intel_crtc = to_intel_crtc(crtc);
> +
>   	ret = drm_plane_helper_check_update(plane, crtc, fb,
>   					    src, dest, clip,
>   					    DRM_PLANE_HELPER_NO_SCALING,
> @@ -12101,15 +12046,14 @@ intel_check_cursor_plane(struct drm_plane *plane,
>   		goto finish;
>
>   	/* Check for which cursor types we support */
> -	crtc_w = drm_rect_width(&state->orig_dst);
> -	crtc_h = drm_rect_height(&state->orig_dst);
> -	if (!cursor_size_ok(dev, crtc_w, crtc_h)) {
> -		DRM_DEBUG("Cursor dimension not supported\n");
> +	if (!cursor_size_ok(dev, state->base.crtc_w, state->base.crtc_h)) {
> +		DRM_DEBUG("Cursor dimension %dx%d not supported\n",
> +			  state->base.crtc_w, state->base.crtc_h);
>   		return -EINVAL;
>   	}
>
> -	stride = roundup_pow_of_two(crtc_w) * 4;
> -	if (obj->base.size < stride * crtc_h) {
> +	stride = roundup_pow_of_two(state->base.crtc_w) * 4;
> +	if (obj->base.size < stride * state->base.crtc_h) {
>   		DRM_DEBUG_KMS("buffer is too small\n");
>   		return -ENOMEM;
>   	}
> @@ -12127,8 +12071,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
>
>   finish:
>   	if (intel_crtc->active) {
> -		if (intel_crtc->cursor_width !=
> -		    drm_rect_width(&state->orig_dst))
> +		if (intel_crtc->cursor_width != state->base.crtc_w)
>   			intel_crtc->atomic.update_wm = true;
>
>   		intel_crtc->atomic.fb_bits |=
> @@ -12143,24 +12086,27 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>   			  struct intel_plane_state *state)
>   {
>   	struct drm_crtc *crtc = state->base.crtc;
> -	struct drm_device *dev = crtc->dev;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct drm_device *dev = plane->dev;
> +	struct intel_crtc *intel_crtc;
>   	struct intel_plane *intel_plane = to_intel_plane(plane);
>   	struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
>   	uint32_t addr;
>
> +	crtc = crtc ? crtc : plane->crtc;
> +	intel_crtc = to_intel_crtc(crtc);
> +
>   	plane->fb = state->base.fb;
> -	crtc->cursor_x = state->orig_dst.x1;
> -	crtc->cursor_y = state->orig_dst.y1;
> -
> -	intel_plane->crtc_x = state->orig_dst.x1;
> -	intel_plane->crtc_y = state->orig_dst.y1;
> -	intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
> -	intel_plane->crtc_h = drm_rect_height(&state->orig_dst);
> -	intel_plane->src_x = state->orig_src.x1;
> -	intel_plane->src_y = state->orig_src.y1;
> -	intel_plane->src_w = drm_rect_width(&state->orig_src);
> -	intel_plane->src_h = drm_rect_height(&state->orig_src);
> +	crtc->cursor_x = state->base.crtc_x;
> +	crtc->cursor_y = state->base.crtc_y;
> +
> +	intel_plane->crtc_x = state->base.crtc_x;
> +	intel_plane->crtc_y = state->base.crtc_y;
> +	intel_plane->crtc_w = state->base.crtc_w;
> +	intel_plane->crtc_h = state->base.crtc_h;
> +	intel_plane->src_x = state->base.src_x;
> +	intel_plane->src_y = state->base.src_y;
> +	intel_plane->src_w = state->base.src_w;
> +	intel_plane->src_h = state->base.src_h;
>   	intel_plane->obj = obj;
>
>   	if (intel_crtc->cursor_bo == obj)
> @@ -12176,18 +12122,20 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>   	intel_crtc->cursor_addr = addr;
>   	intel_crtc->cursor_bo = obj;
>   update:
> -	intel_crtc->cursor_width = drm_rect_width(&state->orig_dst);
> -	intel_crtc->cursor_height = drm_rect_height(&state->orig_dst);
> +	intel_crtc->cursor_width = state->base.crtc_w;
> +	intel_crtc->cursor_height = state->base.crtc_h;
>
>   	if (intel_crtc->active)
>   		intel_crtc_update_cursor(crtc, state->visible);
>   }
>
>   static const struct drm_plane_funcs intel_cursor_plane_funcs = {
> -	.update_plane = intel_update_plane,
> -	.disable_plane = intel_disable_plane,
> +	.update_plane = drm_plane_helper_update,
> +	.disable_plane = drm_plane_helper_disable,
>   	.destroy = intel_plane_destroy,
>   	.set_property = intel_plane_set_property,
> +	.atomic_duplicate_state = intel_plane_duplicate_state,
> +	.atomic_destroy_state = intel_plane_destroy_state,
>   };
>
>   static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
> @@ -12199,6 +12147,12 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
>   	if (cursor == NULL)
>   		return NULL;
>
> +	cursor->base.state = intel_plane_duplicate_state(&cursor->base);
> +	if (cursor->base.state == NULL) {
> +		kfree(cursor);
> +		return NULL;
> +	}
> +
>   	cursor->can_scale = false;
>   	cursor->max_downscale = 1;
>   	cursor->pipe = pipe;
> @@ -12225,6 +12179,8 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
>   				cursor->rotation);
>   	}
>
> +	drm_plane_helper_add(&cursor->base, &intel_plane_helper_funcs);
> +
>   	return &cursor->base;
>   }
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 2523315..ab23190 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -248,8 +248,6 @@ struct intel_plane_state {
>   	struct drm_rect src;
>   	struct drm_rect dst;
>   	struct drm_rect clip;
> -	struct drm_rect orig_src;
> -	struct drm_rect orig_dst;
>   	bool visible;
>
>   	/*
> @@ -437,6 +435,7 @@ struct intel_crtc_atomic_commit {
>   	bool disable_fbc;
>   	bool pre_disable_primary;
>   	bool update_wm;
> +	unsigned track_fbs;

I think track_fbs is a poor name for this, since this is actually a mask 
of planes being disabled. Maybe call it disabled_planes.

Ander


>
>   	/* Sleepable operations to perform after commit */
>   	unsigned fb_bits;
> @@ -575,6 +574,7 @@ struct cxsr_latency {
>   #define to_intel_encoder(x) container_of(x, struct intel_encoder, base)
>   #define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base)
>   #define to_intel_plane(x) container_of(x, struct intel_plane, base)
> +#define to_intel_plane_state(x) container_of(x, struct intel_plane_state, base)
>   #define intel_fb_obj(x) (x ? to_intel_framebuffer(x)->obj : NULL)
>
>   struct intel_hdmi {
> @@ -1253,4 +1253,10 @@ void intel_pre_disable_primary(struct drm_crtc *crtc);
>   /* intel_tv.c */
>   void intel_tv_init(struct drm_device *dev);
>
> +/* intel_atomic.c */
> +struct drm_plane_state *intel_plane_duplicate_state(struct drm_plane *plane);
> +void intel_plane_destroy_state(struct drm_plane *plane,
> +			       struct drm_plane_state *state);
> +extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
> +
>   #endif /* __INTEL_DRV_H__ */
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index a689c73..f8efcfd 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -33,6 +33,7 @@
>   #include <drm/drm_crtc.h>
>   #include <drm/drm_fourcc.h>
>   #include <drm/drm_rect.h>
> +#include <drm/drm_plane_helper.h>
>   #include "intel_drv.h"
>   #include <drm/i915_drm.h>
>   #include "i915_drv.h"
> @@ -1061,12 +1062,13 @@ intel_check_sprite_plane(struct drm_plane *plane,
>   	uint32_t src_x, src_y, src_w, src_h;
>   	struct drm_rect *src = &state->src;
>   	struct drm_rect *dst = &state->dst;
> -	struct drm_rect *orig_src = &state->orig_src;
>   	const struct drm_rect *clip = &state->clip;
>   	int hscale, vscale;
>   	int max_scale, min_scale;
>   	int pixel_size;
>
> +	intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
> +
>   	if (!fb) {
>   		state->visible = false;
>   		goto finish;
> @@ -1147,10 +1149,10 @@ intel_check_sprite_plane(struct drm_plane *plane,
>   				    intel_plane->rotation);
>
>   		/* sanity check to make sure the src viewport wasn't enlarged */
> -		WARN_ON(src->x1 < (int) orig_src->x1 ||
> -			src->y1 < (int) orig_src->y1 ||
> -			src->x2 > (int) orig_src->x2 ||
> -			src->y2 > (int) orig_src->y2);
> +		WARN_ON(src->x1 < (int) state->base.src_x ||
> +			src->y1 < (int) state->base.src_y ||
> +			src->x2 > (int) state->base.src_x + state->base.src_w ||
> +			src->y2 > (int) state->base.src_y + state->base.src_h);
>
>   		/*
>   		 * Hardware doesn't handle subpixel coordinates.
> @@ -1247,7 +1249,7 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>   			  struct intel_plane_state *state)
>   {
>   	struct drm_crtc *crtc = state->base.crtc;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_crtc *intel_crtc;
>   	struct intel_plane *intel_plane = to_intel_plane(plane);
>   	struct drm_framebuffer *fb = state->base.fb;
>   	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> @@ -1255,14 +1257,19 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>   	unsigned int crtc_w, crtc_h;
>   	uint32_t src_x, src_y, src_w, src_h;
>
> -	intel_plane->crtc_x = state->orig_dst.x1;
> -	intel_plane->crtc_y = state->orig_dst.y1;
> -	intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
> -	intel_plane->crtc_h = drm_rect_height(&state->orig_dst);
> -	intel_plane->src_x = state->orig_src.x1;
> -	intel_plane->src_y = state->orig_src.y1;
> -	intel_plane->src_w = drm_rect_width(&state->orig_src);
> -	intel_plane->src_h = drm_rect_height(&state->orig_src);
> +	intel_plane->crtc_x = state->base.crtc_x;
> +	intel_plane->crtc_y = state->base.crtc_y;
> +	intel_plane->crtc_w = state->base.crtc_w;
> +	intel_plane->crtc_h = state->base.crtc_h;
> +	intel_plane->src_x = state->base.src_x;
> +	intel_plane->src_y = state->base.src_y;
> +	intel_plane->src_w = state->base.src_w;
> +	intel_plane->src_h = state->base.src_h;
> +
> +	crtc = crtc ? crtc : plane->crtc;
> +	intel_crtc = to_intel_crtc(crtc);
> +
> +	plane->fb = state->base.fb;
>   	intel_plane->obj = obj;
>
>   	if (intel_crtc->active) {
> @@ -1386,10 +1393,12 @@ int intel_plane_restore(struct drm_plane *plane)
>   }
>
>   static const struct drm_plane_funcs intel_sprite_plane_funcs = {
> -	.update_plane = intel_update_plane,
> -	.disable_plane = intel_disable_plane,
> +	.update_plane = drm_plane_helper_update,
> +	.disable_plane = drm_plane_helper_disable,
>   	.destroy = intel_plane_destroy,
>   	.set_property = intel_plane_set_property,
> +	.atomic_duplicate_state = intel_plane_duplicate_state,
> +	.atomic_destroy_state = intel_plane_destroy_state,
>   };
>
>   static uint32_t ilk_plane_formats[] = {
> @@ -1451,6 +1460,13 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>   	if (!intel_plane)
>   		return -ENOMEM;
>
> +	intel_plane->base.state =
> +		intel_plane_duplicate_state(&intel_plane->base);
> +	if (intel_plane->base.state == NULL) {
> +		kfree(intel_plane);
> +		return -ENOMEM;
> +	}
> +
>   	switch (INTEL_INFO(dev)->gen) {
>   	case 5:
>   	case 6:
> @@ -1544,6 +1560,8 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>   					   dev->mode_config.rotation_property,
>   					   intel_plane->rotation);
>
> +	drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
> +
>    out:
>   	return ret;
>   }
>

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

[PATCH 5/5] drm/i915: Drop unused position fields (v2)

[Matt Roper]

The userspace-requested plane coordinates are now always available via
plane->state.base (and the i915-adjusted values are stored in
plane->state), so we no longer use the coordinate fields in intel_plane
and can drop them.

Also, note that the error case for pageflip calls update_plane() to
program the values from plane->state; it's simpler to just call
intel_plane_restore() which does the same thing.

v2: Replace manual update_plane() with intel_plane_restore() in pageflip
    error handler.

Reviewed-by(v1): Bob Paauwe <bob.j.paauwe@intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 27 +--------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  4 ----
 drivers/gpu/drm/i915/intel_sprite.c  | 19 ++++---------------
 3 files changed, 5 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 934e6a8..d664104 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9616,7 +9616,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_plane *primary = crtc->primary;
-	struct intel_plane *intel_plane = to_intel_plane(primary);
 	enum pipe pipe = intel_crtc->pipe;
 	struct intel_unpin_work *work;
 	struct intel_engine_cs *ring;
@@ -9775,15 +9774,7 @@ free_work:
 
 	if (ret == -EIO) {
 out_hang:
-		ret = primary->funcs->update_plane(primary, crtc, fb,
-						   intel_plane->crtc_x,
-						   intel_plane->crtc_y,
-						   intel_plane->crtc_h,
-						   intel_plane->crtc_w,
-						   intel_plane->src_x,
-						   intel_plane->src_y,
-						   intel_plane->src_h,
-						   intel_plane->src_w);
+		ret = intel_plane_restore(primary);
 		if (ret == 0 && event) {
 			spin_lock_irq(&dev->event_lock);
 			drm_send_vblank_event(dev, pipe, event);
@@ -11827,14 +11818,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
 	crtc->x = src->x1 >> 16;
 	crtc->y = src->y1 >> 16;
 
-	intel_plane->crtc_x = state->base.crtc_x;
-	intel_plane->crtc_y = state->base.crtc_y;
-	intel_plane->crtc_w = state->base.crtc_w;
-	intel_plane->crtc_h = state->base.crtc_h;
-	intel_plane->src_x = state->base.src_x;
-	intel_plane->src_y = state->base.src_y;
-	intel_plane->src_w = state->base.src_w;
-	intel_plane->src_h = state->base.src_h;
 	intel_plane->obj = obj;
 
 	if (intel_crtc->active) {
@@ -12099,14 +12082,6 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 	crtc->cursor_x = state->base.crtc_x;
 	crtc->cursor_y = state->base.crtc_y;
 
-	intel_plane->crtc_x = state->base.crtc_x;
-	intel_plane->crtc_y = state->base.crtc_y;
-	intel_plane->crtc_w = state->base.crtc_w;
-	intel_plane->crtc_h = state->base.crtc_h;
-	intel_plane->src_x = state->base.src_x;
-	intel_plane->src_y = state->base.src_y;
-	intel_plane->src_w = state->base.src_w;
-	intel_plane->src_h = state->base.src_h;
 	intel_plane->obj = obj;
 
 	if (intel_crtc->cursor_bo == obj)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ab23190..dbf04dc 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -517,10 +517,6 @@ struct intel_plane {
 	struct drm_i915_gem_object *obj;
 	bool can_scale;
 	int max_downscale;
-	int crtc_x, crtc_y;
-	unsigned int crtc_w, crtc_h;
-	uint32_t src_x, src_y;
-	uint32_t src_w, src_h;
 	unsigned int rotation;
 
 	/* Since we need to change the watermarks before/after
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index f8efcfd..d874ce0 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1257,15 +1257,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 	unsigned int crtc_w, crtc_h;
 	uint32_t src_x, src_y, src_w, src_h;
 
-	intel_plane->crtc_x = state->base.crtc_x;
-	intel_plane->crtc_y = state->base.crtc_y;
-	intel_plane->crtc_w = state->base.crtc_w;
-	intel_plane->crtc_h = state->base.crtc_h;
-	intel_plane->src_x = state->base.src_x;
-	intel_plane->src_y = state->base.src_y;
-	intel_plane->src_w = state->base.src_w;
-	intel_plane->src_h = state->base.src_h;
-
 	crtc = crtc ? crtc : plane->crtc;
 	intel_crtc = to_intel_crtc(crtc);
 
@@ -1380,16 +1371,14 @@ int intel_plane_set_property(struct drm_plane *plane,
 
 int intel_plane_restore(struct drm_plane *plane)
 {
-	struct intel_plane *intel_plane = to_intel_plane(plane);
-
 	if (!plane->crtc || !plane->fb)
 		return 0;
 
 	return plane->funcs->update_plane(plane, plane->crtc, plane->fb,
-				  intel_plane->crtc_x, intel_plane->crtc_y,
-				  intel_plane->crtc_w, intel_plane->crtc_h,
-				  intel_plane->src_x, intel_plane->src_y,
-				  intel_plane->src_w, intel_plane->src_h);
+				  plane->state->crtc_x, plane->state->crtc_y,
+				  plane->state->crtc_w, plane->state->crtc_h,
+				  plane->state->src_x, plane->state->src_y,
+				  plane->state->src_w, plane->state->src_h);
 }
 
 static const struct drm_plane_funcs intel_sprite_plane_funcs = {
-- 
1.8.5.1

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

Re: [PATCH 5/5] drm/i915: Drop unused position fields (v2)

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  364/364              364/364
ILK              +4-3              360/366              361/366
SNB                 -2              448/450              446/450
IVB                 -2              497/498              495/498
BYT                                  289/289              289/289
HSW                 -1              563/564              562/564
BDW                                  417/417              417/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt_kms_flip_nonexisting-fb      PASS(2, M26)      DMESG_WARN(1, M26)
 ILK  igt_kms_flip_rcs-flip-vs-panning-interruptible      DMESG_WARN(1, M26)PASS(2, M26)      DMESG_WARN(1, M26)
 ILK  igt_drv_suspend_fence-restore-untiled      DMESG_WARN(1, M26)PASS(6, M37M26)      PASS(1, M26)
 ILK  igt_kms_flip_bcs-flip-vs-modeset-interruptible      DMESG_WARN(1, M26)PASS(6, M37M26)      PASS(1, M26)
*ILK  igt_kms_flip_busy-flip-interruptible      DMESG_WARN(2, M26)PASS(5, M37M26)      NSPT(1, M26)
 ILK  igt_kms_flip_flip-vs-rmfb-interruptible      DMESG_WARN(1, M26)PASS(6, M37M26)      PASS(1, M26)
 ILK  igt_kms_flip_plain-flip-ts-check-interruptible      DMESG_WARN(1, M26)PASS(1, M26)      DMESG_WARN(1, M26)
 ILK  igt_kms_flip_rcs-flip-vs-dpms      DMESG_WARN(1, M26)PASS(5, M37M26)      PASS(1, M26)
*SNB  igt_kms_mmio_vs_cs_flip_setplane_vs_cs_flip      PASS(2, M35)      DMESG_WARN(1, M35)
*SNB  igt_kms_rotation_crc_sprite-rotation      PASS(2, M35)      DMESG_WARN(1, M35)
*IVB  igt_kms_mmio_vs_cs_flip_setplane_vs_cs_flip      PASS(2, M34)      DMESG_WARN(1, M34)
*IVB  igt_kms_rotation_crc_sprite-rotation      PASS(2, M34)      DMESG_WARN(1, M34)
*HSW  igt_kms_mmio_vs_cs_flip_setplane_vs_cs_flip      PASS(2, M40)      DMESG_WARN(1, M40)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Drop unused position fields (v2)

[Ander Conselvan de Oliveira]

On 12/16/2014 02:23 AM, Matt Roper wrote:
> The userspace-requested plane coordinates are now always available via
> plane->state.base (and the i915-adjusted values are stored in
> plane->state), so we no longer use the coordinate fields in intel_plane
> and can drop them.
> 
> Also, note that the error case for pageflip calls update_plane() to
> program the values from plane->state; it's simpler to just call
> intel_plane_restore() which does the same thing.
> 
> v2: Replace manual update_plane() with intel_plane_restore() in pageflip
>      error handler.
> 
> Reviewed-by(v1): Bob Paauwe <bob.j.paauwe@intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Reviewed-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

> ---
>   drivers/gpu/drm/i915/intel_display.c | 27 +--------------------------
>   drivers/gpu/drm/i915/intel_drv.h     |  4 ----
>   drivers/gpu/drm/i915/intel_sprite.c  | 19 ++++---------------
>   3 files changed, 5 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 934e6a8..d664104 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9616,7 +9616,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>   	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   	struct drm_plane *primary = crtc->primary;
> -	struct intel_plane *intel_plane = to_intel_plane(primary);
>   	enum pipe pipe = intel_crtc->pipe;
>   	struct intel_unpin_work *work;
>   	struct intel_engine_cs *ring;
> @@ -9775,15 +9774,7 @@ free_work:
>   
>   	if (ret == -EIO) {
>   out_hang:
> -		ret = primary->funcs->update_plane(primary, crtc, fb,
> -						   intel_plane->crtc_x,
> -						   intel_plane->crtc_y,
> -						   intel_plane->crtc_h,
> -						   intel_plane->crtc_w,
> -						   intel_plane->src_x,
> -						   intel_plane->src_y,
> -						   intel_plane->src_h,
> -						   intel_plane->src_w);
> +		ret = intel_plane_restore(primary);
>   		if (ret == 0 && event) {
>   			spin_lock_irq(&dev->event_lock);
>   			drm_send_vblank_event(dev, pipe, event);
> @@ -11827,14 +11818,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
>   	crtc->x = src->x1 >> 16;
>   	crtc->y = src->y1 >> 16;
>   
> -	intel_plane->crtc_x = state->base.crtc_x;
> -	intel_plane->crtc_y = state->base.crtc_y;
> -	intel_plane->crtc_w = state->base.crtc_w;
> -	intel_plane->crtc_h = state->base.crtc_h;
> -	intel_plane->src_x = state->base.src_x;
> -	intel_plane->src_y = state->base.src_y;
> -	intel_plane->src_w = state->base.src_w;
> -	intel_plane->src_h = state->base.src_h;
>   	intel_plane->obj = obj;
>   
>   	if (intel_crtc->active) {
> @@ -12099,14 +12082,6 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>   	crtc->cursor_x = state->base.crtc_x;
>   	crtc->cursor_y = state->base.crtc_y;
>   
> -	intel_plane->crtc_x = state->base.crtc_x;
> -	intel_plane->crtc_y = state->base.crtc_y;
> -	intel_plane->crtc_w = state->base.crtc_w;
> -	intel_plane->crtc_h = state->base.crtc_h;
> -	intel_plane->src_x = state->base.src_x;
> -	intel_plane->src_y = state->base.src_y;
> -	intel_plane->src_w = state->base.src_w;
> -	intel_plane->src_h = state->base.src_h;
>   	intel_plane->obj = obj;
>   
>   	if (intel_crtc->cursor_bo == obj)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index ab23190..dbf04dc 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -517,10 +517,6 @@ struct intel_plane {
>   	struct drm_i915_gem_object *obj;
>   	bool can_scale;
>   	int max_downscale;
> -	int crtc_x, crtc_y;
> -	unsigned int crtc_w, crtc_h;
> -	uint32_t src_x, src_y;
> -	uint32_t src_w, src_h;
>   	unsigned int rotation;
>   
>   	/* Since we need to change the watermarks before/after
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index f8efcfd..d874ce0 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1257,15 +1257,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>   	unsigned int crtc_w, crtc_h;
>   	uint32_t src_x, src_y, src_w, src_h;
>   
> -	intel_plane->crtc_x = state->base.crtc_x;
> -	intel_plane->crtc_y = state->base.crtc_y;
> -	intel_plane->crtc_w = state->base.crtc_w;
> -	intel_plane->crtc_h = state->base.crtc_h;
> -	intel_plane->src_x = state->base.src_x;
> -	intel_plane->src_y = state->base.src_y;
> -	intel_plane->src_w = state->base.src_w;
> -	intel_plane->src_h = state->base.src_h;
> -
>   	crtc = crtc ? crtc : plane->crtc;
>   	intel_crtc = to_intel_crtc(crtc);
>   
> @@ -1380,16 +1371,14 @@ int intel_plane_set_property(struct drm_plane *plane,
>   
>   int intel_plane_restore(struct drm_plane *plane)
>   {
> -	struct intel_plane *intel_plane = to_intel_plane(plane);
> -
>   	if (!plane->crtc || !plane->fb)
>   		return 0;
>   
>   	return plane->funcs->update_plane(plane, plane->crtc, plane->fb,
> -				  intel_plane->crtc_x, intel_plane->crtc_y,
> -				  intel_plane->crtc_w, intel_plane->crtc_h,
> -				  intel_plane->src_x, intel_plane->src_y,
> -				  intel_plane->src_w, intel_plane->src_h);
> +				  plane->state->crtc_x, plane->state->crtc_y,
> +				  plane->state->crtc_w, plane->state->crtc_h,
> +				  plane->state->src_x, plane->state->src_y,
> +				  plane->state->src_w, plane->state->src_h);
>   }
>   
>   static const struct drm_plane_funcs intel_sprite_plane_funcs = {
> 

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

[PATCH 0/5] i915 atomic plane helper conversion (v5)

[Matt Roper]

Updated based on review feedback from Ander.

Previous series is available here:
  http://lists.freedesktop.org/archives/intel-gfx/2014-December/057598.html

Matt Roper (5):
  drm/i915: Refactor work that can sleep out of commit (v6)
  drm/i915: Move vblank evasion to commit (v4)
  drm/i915: Clarify sprite plane function names (v4)
  drm/i915: Move to atomic plane helpers (v9)
  drm/i915: Drop unused position fields (v2)

 Documentation/DocBook/drm.tmpl            |   5 +
 drivers/gpu/drm/i915/Makefile             |   1 +
 drivers/gpu/drm/i915/intel_atomic_plane.c | 151 ++++++++++++
 drivers/gpu/drm/i915/intel_display.c      | 377 +++++++++++++++---------------
 drivers/gpu/drm/i915/intel_drv.h          |  50 +++-
 drivers/gpu/drm/i915/intel_sprite.c       | 203 +++++++---------
 6 files changed, 473 insertions(+), 314 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_atomic_plane.c

-- 
1.8.5.1

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

[PATCH 5/5] drm/i915: Drop unused position fields (v2)

[Matt Roper]

The userspace-requested plane coordinates are now always available via
plane->state.base (and the i915-adjusted values are stored in
plane->state), so we no longer use the coordinate fields in intel_plane
and can drop them.

Also, note that the error case for pageflip calls update_plane() to
program the values from plane->state; it's simpler to just call
intel_plane_restore() which does the same thing.

v2: Replace manual update_plane() with intel_plane_restore() in pageflip
    error handler.

Reviewed-by(v1): Bob Paauwe <bob.j.paauwe@intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 27 +--------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  4 ----
 drivers/gpu/drm/i915/intel_sprite.c  | 19 ++++---------------
 3 files changed, 5 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e7b51db..75a514d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9616,7 +9616,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_plane *primary = crtc->primary;
-	struct intel_plane *intel_plane = to_intel_plane(primary);
 	enum pipe pipe = intel_crtc->pipe;
 	struct intel_unpin_work *work;
 	struct intel_engine_cs *ring;
@@ -9775,15 +9774,7 @@ free_work:
 
 	if (ret == -EIO) {
 out_hang:
-		ret = primary->funcs->update_plane(primary, crtc, fb,
-						   intel_plane->crtc_x,
-						   intel_plane->crtc_y,
-						   intel_plane->crtc_h,
-						   intel_plane->crtc_w,
-						   intel_plane->src_x,
-						   intel_plane->src_y,
-						   intel_plane->src_h,
-						   intel_plane->src_w);
+		ret = intel_plane_restore(primary);
 		if (ret == 0 && event) {
 			spin_lock_irq(&dev->event_lock);
 			drm_send_vblank_event(dev, pipe, event);
@@ -11823,14 +11814,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
 	crtc->x = src->x1 >> 16;
 	crtc->y = src->y1 >> 16;
 
-	intel_plane->crtc_x = state->base.crtc_x;
-	intel_plane->crtc_y = state->base.crtc_y;
-	intel_plane->crtc_w = state->base.crtc_w;
-	intel_plane->crtc_h = state->base.crtc_h;
-	intel_plane->src_x = state->base.src_x;
-	intel_plane->src_y = state->base.src_y;
-	intel_plane->src_w = state->base.src_w;
-	intel_plane->src_h = state->base.src_h;
 	intel_plane->obj = obj;
 
 	if (intel_crtc->active) {
@@ -12111,14 +12094,6 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 	crtc->cursor_x = state->base.crtc_x;
 	crtc->cursor_y = state->base.crtc_y;
 
-	intel_plane->crtc_x = state->base.crtc_x;
-	intel_plane->crtc_y = state->base.crtc_y;
-	intel_plane->crtc_w = state->base.crtc_w;
-	intel_plane->crtc_h = state->base.crtc_h;
-	intel_plane->src_x = state->base.src_x;
-	intel_plane->src_y = state->base.src_y;
-	intel_plane->src_w = state->base.src_w;
-	intel_plane->src_h = state->base.src_h;
 	intel_plane->obj = obj;
 
 	if (intel_crtc->cursor_bo == obj)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 604bd22..db3fde3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -517,10 +517,6 @@ struct intel_plane {
 	struct drm_i915_gem_object *obj;
 	bool can_scale;
 	int max_downscale;
-	int crtc_x, crtc_y;
-	unsigned int crtc_w, crtc_h;
-	uint32_t src_x, src_y;
-	uint32_t src_w, src_h;
 	unsigned int rotation;
 
 	/* Since we need to change the watermarks before/after
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 16e7c96..37079f1 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1277,15 +1277,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 	unsigned int crtc_w, crtc_h;
 	uint32_t src_x, src_y, src_w, src_h;
 
-	intel_plane->crtc_x = state->base.crtc_x;
-	intel_plane->crtc_y = state->base.crtc_y;
-	intel_plane->crtc_w = state->base.crtc_w;
-	intel_plane->crtc_h = state->base.crtc_h;
-	intel_plane->src_x = state->base.src_x;
-	intel_plane->src_y = state->base.src_y;
-	intel_plane->src_w = state->base.src_w;
-	intel_plane->src_h = state->base.src_h;
-
 	crtc = crtc ? crtc : plane->crtc;
 	intel_crtc = to_intel_crtc(crtc);
 
@@ -1400,16 +1391,14 @@ int intel_plane_set_property(struct drm_plane *plane,
 
 int intel_plane_restore(struct drm_plane *plane)
 {
-	struct intel_plane *intel_plane = to_intel_plane(plane);
-
 	if (!plane->crtc || !plane->fb)
 		return 0;
 
 	return plane->funcs->update_plane(plane, plane->crtc, plane->fb,
-				  intel_plane->crtc_x, intel_plane->crtc_y,
-				  intel_plane->crtc_w, intel_plane->crtc_h,
-				  intel_plane->src_x, intel_plane->src_y,
-				  intel_plane->src_w, intel_plane->src_h);
+				  plane->state->crtc_x, plane->state->crtc_y,
+				  plane->state->crtc_w, plane->state->crtc_h,
+				  plane->state->src_x, plane->state->src_y,
+				  plane->state->src_w, plane->state->src_h);
 }
 
 static const struct drm_plane_funcs intel_sprite_plane_funcs = {
-- 
1.8.5.1

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

Re: [PATCH 5/5] drm/i915: Drop unused position fields (v2)

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  363/364              363/364
ILK                 -1              364/366              363/366
SNB                 -2              447/450              445/450
IVB                 -2              496/498              494/498
BYT                                  288/289              288/289
HSW                 -1              562/564              561/564
BDW                                  415/417              415/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt_gem_reloc_vs_gpu_forked-faulting-reloc-thrash-inactive      PASS(2, M37)      DMESG_WARN(1, M37)
*SNB  igt_kms_mmio_vs_cs_flip_setplane_vs_cs_flip      PASS(2, M35)      DMESG_WARN(1, M35)
*SNB  igt_kms_rotation_crc_sprite-rotation      PASS(2, M35)      DMESG_WARN(1, M35)
*IVB  igt_kms_mmio_vs_cs_flip_setplane_vs_cs_flip      PASS(2, M34)      DMESG_WARN(1, M34)
*IVB  igt_kms_rotation_crc_sprite-rotation      PASS(2, M34)      DMESG_WARN(1, M34)
*HSW  igt_kms_mmio_vs_cs_flip_setplane_vs_cs_flip      PASS(2, M40)      DMESG_WARN(1, M40)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx