[RFC][PATCH] Atomic page flip WIP

[RFC][PATCH] Atomic page flip WIP

[ville.syrjala]

I'm posting this as rather raw just to get a bit more substance to the
discussions.

The drm_flip thingy may seem a bit too mid-layerish for people's taste,
but at least it is almost completely driven by explicit function calls
from the driver (the wq side is the exception naturally).

I originally wrote drm_flip for Medfield, where I used it syncronize the
pipe's primary layer and one or two video overlays (gen3 style overlays).
So it's a fairly well proven tehnology. I can post a version of the video
overlay code too in case someone would be interested in having drm_planes
on gen3 hardware.

This set applies on top of my earlier drm_atomic_4 branch:
https://gitorious.org/vsyrjala/linux/commits/drm_atomic_4

[RFC][PATCH 1/4] drm/i915: Try to commit single pipe in one go

[ville.syrjala]

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

---
 drivers/gpu/drm/i915/intel_atomic.c |   23 ++++++++++++-----------
 1 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index e439c04..e9eaa8a 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -907,6 +907,7 @@ static int apply_config(struct drm_device *dev,
 	for (i = 0; i < dev->mode_config.num_crtc; i++) {
 		struct intel_crtc_state *st = &s->crtc[i];
 		struct intel_crtc *intel_crtc = to_intel_crtc(st->crtc);
+		int j;
 
 		if (st->mode_dirty) {
 			ret = crtc_mode_set(st->crtc);
@@ -925,21 +926,21 @@ static int apply_config(struct drm_device *dev,
 						 intel_crtc->cursor_height,
 						 intel_crtc->cursor_bo,
 						 intel_crtc->cursor_addr);
-	}
 
-	for (i = 0; i < dev->mode_config.num_plane; i++) {
-		struct intel_plane_state *st = &s->plane[i];
-		struct drm_plane *plane = st->plane;
+		for (j = 0; j < dev->mode_config.num_plane; j++) {
+			struct intel_plane_state *pst = &s->plane[j];
+			struct drm_plane *plane = pst->plane;
 
-		if (!s->plane[i].dirty)
-			continue;
+			if (!pst->dirty)
+				continue;
 
-		if (!plane->crtc)
-			continue;
+			if (plane->crtc != st->crtc)
+				continue;
 
-		ret = intel_commit_plane(plane, plane->crtc, plane->fb, &st->coords, false);
-		if (ret)
-			return ret;
+			ret = intel_commit_plane(plane, plane->crtc, plane->fb, &pst->coords, false);
+			if (ret)
+				return ret;
+		}
 	}
 
 	for (i = 0; i < dev->mode_config.num_crtc; i++) {
-- 
1.7.8.6

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC][PATCH 2/4] drm: Add drm_flip helper

[ville.syrjala]

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

The drm_flip mechanism can be used to implement robust page flipping
support, and also to synchronize the flips on multiple hardware
scanout engines (eg. CRTCs and overlays).

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/Makefile   |    2 +-
 drivers/gpu/drm/drm_flip.c |  374 ++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_flip.h     |  244 +++++++++++++++++++++++++++++
 3 files changed, 619 insertions(+), 1 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_flip.c
 create mode 100644 include/drm/drm_flip.h

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index f65f65e..72ce54a 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -12,7 +12,7 @@ drm-y       :=	drm_auth.o drm_buffer.o drm_bufs.o drm_cache.o \
 		drm_platform.o drm_sysfs.o drm_hashtab.o drm_mm.o \
 		drm_crtc.o drm_modes.o drm_edid.o \
 		drm_info.o drm_debugfs.o drm_encoder_slave.o \
-		drm_trace_points.o drm_global.o drm_prime.o
+		drm_trace_points.o drm_global.o drm_prime.o drm_flip.o
 
 drm-$(CONFIG_COMPAT) += drm_ioc32.o
 
diff --git a/drivers/gpu/drm/drm_flip.c b/drivers/gpu/drm/drm_flip.c
new file mode 100644
index 0000000..4e716a3
--- /dev/null
+++ b/drivers/gpu/drm/drm_flip.c
@@ -0,0 +1,374 @@
+/*
+ * Copyright (C) 2012 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.
+ *
+ * Authors:
+ * Ville Syrjälä <ville.syrjala@linux.intel.com>
+ */
+
+#include <drm/drm_flip.h>
+
+static void drm_flip_driver_cleanup(struct work_struct *work)
+{
+	struct drm_flip *flip, *next;
+	struct drm_flip_driver *driver =
+		container_of(work, struct drm_flip_driver, cleanup_work);
+	LIST_HEAD(list);
+
+	spin_lock_irq(&driver->lock);
+
+	list_cut_position(&list,
+			  &driver->cleanup_list,
+			  driver->cleanup_list.prev);
+
+	spin_unlock_irq(&driver->lock);
+
+	if (list_empty(&list))
+		return;
+
+	list_for_each_entry_safe(flip, next, &list, list) {
+		struct drm_flip_helper *helper = flip->helper;
+
+		WARN_ON(!flip->finished);
+
+		helper->funcs->cleanup(flip);
+	}
+}
+
+static void drm_flip_driver_finish(struct work_struct *work)
+{
+	struct drm_flip *flip, *next;
+	struct drm_flip_driver *driver =
+		container_of(work, struct drm_flip_driver, finish_work);
+	LIST_HEAD(list);
+	bool need_cleanup = false;
+
+	spin_lock_irq(&driver->lock);
+
+	list_cut_position(&list,
+			  &driver->finish_list,
+			  driver->finish_list.prev);
+
+	spin_unlock_irq(&driver->lock);
+
+	if (list_empty(&list))
+		return;
+
+	list_for_each_entry_safe(flip, next, &list, list) {
+		struct drm_flip_helper *helper = flip->helper;
+
+		helper->funcs->finish(flip);
+
+		spin_lock_irq(&driver->lock);
+
+		flip->finished = true;
+
+		/*
+		 * It's possible that drm_flip_set_scanout() was called after we
+		 * pulled this flip from finish_list, in which case the flip
+		 * could be in need of cleanup, but not on cleanup_list.
+		 */
+		if (flip == helper->scanout_flip) {
+			list_del_init(&flip->list);
+		} else {
+			need_cleanup = true;
+			list_move_tail(&flip->list, &driver->cleanup_list);
+		}
+
+		spin_unlock_irq(&driver->lock);
+	}
+
+	if (need_cleanup)
+		queue_work(driver->wq, &driver->cleanup_work);
+}
+
+static bool drm_flip_set_scanout(struct drm_flip_helper *helper,
+				 struct drm_flip *flip)
+{
+	struct drm_flip_driver *driver = helper->driver;
+	struct drm_flip *old = helper->scanout_flip;
+
+	helper->scanout_flip = flip;
+
+	if (old && old->finished)
+		list_move_tail(&old->list, &driver->cleanup_list);
+
+	return old != NULL;
+}
+
+static bool drm_flip_complete(struct drm_flip *flip)
+{
+	struct drm_flip_helper *helper = flip->helper;
+	struct drm_flip_driver *driver = helper->driver;
+	bool need_cleanup = false;
+
+	helper->funcs->complete(flip);
+
+	if (flip->flipped) {
+		if (drm_flip_set_scanout(helper, flip))
+			need_cleanup = true;
+	}
+
+	list_add_tail(&flip->list, &driver->finish_list);
+
+	return need_cleanup;
+}
+
+void drm_flip_helper_init(struct drm_flip_helper *helper,
+			  struct drm_flip_driver *driver,
+			  const struct drm_flip_helper_funcs *funcs)
+{
+	helper->pending_flip = NULL;
+	helper->scanout_flip = NULL;
+	helper->driver = driver;
+	helper->funcs = funcs;
+}
+
+void drm_flip_helper_clear(struct drm_flip_helper *helper)
+{
+	unsigned long flags;
+	struct drm_flip_driver *driver = helper->driver;
+	struct drm_flip *pending_flip;
+	bool need_finish = false;
+	bool need_cleanup = false;
+
+	spin_lock_irqsave(&driver->lock, flags);
+
+	pending_flip = helper->pending_flip;
+
+	if (pending_flip) {
+		need_finish = true;
+
+		if (drm_flip_complete(pending_flip))
+			need_cleanup = true;
+
+		helper->pending_flip = NULL;
+	}
+
+	if (drm_flip_set_scanout(helper, NULL))
+		need_cleanup = true;
+
+	spin_unlock_irqrestore(&driver->lock, flags);
+
+	if (need_finish)
+		queue_work(driver->wq, &driver->finish_work);
+
+	if (need_cleanup)
+		queue_work(driver->wq, &driver->cleanup_work);
+}
+
+void drm_flip_helper_fini(struct drm_flip_helper *helper)
+{
+	struct drm_flip_driver *driver = helper->driver;
+
+	drm_flip_helper_clear(helper);
+
+	flush_work_sync(&driver->finish_work);
+	flush_work_sync(&driver->cleanup_work);
+}
+
+void drm_flip_helper_vblank(struct drm_flip_helper *helper)
+{
+	struct drm_flip_driver *driver = helper->driver;
+	struct drm_flip *pending_flip;
+	unsigned long flags;
+	bool need_finish = false;
+	bool need_cleanup = false;
+
+	spin_lock_irqsave(&driver->lock, flags);
+
+	pending_flip = helper->pending_flip;
+
+	if (pending_flip) {
+		BUG_ON(pending_flip->helper != helper);
+
+		if (helper->funcs->vblank(pending_flip))
+			pending_flip->flipped = true;
+
+		if (pending_flip->flipped) {
+			need_finish = true;
+
+			if (drm_flip_complete(pending_flip))
+				need_cleanup = true;
+
+			helper->pending_flip = NULL;
+		}
+	}
+
+	spin_unlock_irqrestore(&driver->lock, flags);
+
+	if (need_finish)
+		queue_work(driver->wq, &driver->finish_work);
+
+	if (need_cleanup)
+		queue_work(driver->wq, &driver->cleanup_work);
+}
+
+void drm_flip_driver_init(struct drm_flip_driver *driver,
+			  const struct drm_flip_driver_funcs *funcs)
+{
+	spin_lock_init(&driver->lock);
+
+	INIT_LIST_HEAD(&driver->finish_list);
+	INIT_LIST_HEAD(&driver->cleanup_list);
+
+	INIT_WORK(&driver->finish_work, drm_flip_driver_finish);
+	INIT_WORK(&driver->cleanup_work, drm_flip_driver_cleanup);
+
+	driver->funcs = funcs;
+
+	driver->wq = create_singlethread_workqueue("drm_flip");
+}
+
+void drm_flip_driver_fini(struct drm_flip_driver *driver)
+{
+	destroy_workqueue(driver->wq);
+
+	/* All the scheduled flips should be cleaned up by now. */
+	WARN_ON(!list_empty(&driver->finish_list));
+	WARN_ON(!list_empty(&driver->cleanup_list));
+}
+
+void drm_flip_driver_schedule_flips(struct drm_flip_driver *driver,
+				    struct list_head *flips)
+{
+	unsigned long flags;
+	struct drm_flip *flip, *next;
+	bool need_finish = false;
+	bool need_cleanup = false;
+
+	spin_lock_irqsave(&driver->lock, flags);
+
+	list_for_each_entry(flip, flips, list) {
+		struct drm_flip_helper *helper = flip->helper;
+		struct drm_flip *pending_flip = helper->pending_flip;
+
+		if (helper->funcs->flip(flip, pending_flip))
+			pending_flip->flipped = true;
+	}
+
+	if (driver->funcs->flush)
+		driver->funcs->flush(driver);
+
+	/* Complete all flips that got overridden */
+	list_for_each_entry_safe(flip, next, flips, list) {
+		struct drm_flip_helper *helper = flip->helper;
+		struct drm_flip *pending_flip = helper->pending_flip;
+
+		BUG_ON(helper->driver != driver);
+
+		if (pending_flip) {
+			BUG_ON(pending_flip->helper != helper);
+
+			need_finish = true;
+
+			if (drm_flip_complete(pending_flip))
+				need_cleanup = true;
+		}
+
+		list_del_init(&flip->list);
+		helper->pending_flip = flip;
+	}
+
+	spin_unlock_irqrestore(&driver->lock, flags);
+
+	if (need_finish)
+		queue_work(driver->wq, &driver->finish_work);
+
+	if (need_cleanup)
+		queue_work(driver->wq, &driver->cleanup_work);
+}
+
+void drm_flip_driver_prepare_flips(struct drm_flip_driver *driver,
+				   struct list_head *flips)
+{
+	struct drm_flip *flip;
+
+	list_for_each_entry(flip, flips, list) {
+		struct drm_flip_helper *helper = flip->helper;
+
+		if (helper->funcs->prepare)
+			helper->funcs->prepare(flip);
+	}
+}
+
+void drm_flip_driver_complete_flips(struct drm_flip_driver *driver,
+				    struct list_head *flips)
+{
+	unsigned long flags;
+	struct drm_flip *flip, *next;
+	bool need_finish = false;
+	bool need_cleanup = false;
+
+	spin_lock_irqsave(&driver->lock, flags);
+
+	/* first complete all pending flips */
+	list_for_each_entry(flip, flips, list) {
+		struct drm_flip_helper *helper = flip->helper;
+		struct drm_flip *pending_flip = helper->pending_flip;
+
+		BUG_ON(helper->driver != driver);
+
+		if (pending_flip) {
+			BUG_ON(pending_flip->helper != helper);
+
+			need_finish = true;
+
+			if (drm_flip_complete(pending_flip))
+				need_cleanup = true;
+
+			helper->pending_flip = NULL;
+		}
+	}
+
+	/* then complete all new flips as well */
+	list_for_each_entry_safe(flip, next, flips, list) {
+		list_del_init(&flip->list);
+
+		/*
+		 * This is the flip that gets scanned out
+		 * next time the hardware is fired up.
+		 */
+		flip->flipped = true;
+
+		need_finish = true;
+
+		if (drm_flip_complete(flip))
+			need_cleanup = true;
+	}
+
+	spin_unlock_irqrestore(&driver->lock, flags);
+
+	if (need_finish)
+		queue_work(driver->wq, &driver->finish_work);
+
+	if (need_cleanup)
+		queue_work(driver->wq, &driver->cleanup_work);
+}
+
+void drm_flip_init(struct drm_flip *flip,
+		   struct drm_flip_helper *helper)
+{
+	flip->helper = helper;
+	flip->flipped = false;
+	flip->finished = false;
+	INIT_LIST_HEAD(&flip->list);
+}
diff --git a/include/drm/drm_flip.h b/include/drm/drm_flip.h
new file mode 100644
index 0000000..4172d6e
--- /dev/null
+++ b/include/drm/drm_flip.h
@@ -0,0 +1,244 @@
+/*
+ * Copyright (C) 2012 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and iated 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.
+ *
+ * Authors:
+ * Ville Syrjälä <ville.syrjala@linux.intel.com>
+ */
+
+#ifndef DRM_FLIP_H
+#define DRM_FLIP_H
+
+#include <linux/list.h>
+#include <linux/spinlock.h>
+#include <linux/workqueue.h>
+
+struct drm_flip;
+struct drm_flip_helper;
+struct drm_flip_driver;
+
+/* Driver callbacks for drm_flip_driver */
+struct drm_flip_driver_funcs {
+	/*
+	 * Optional callback, called after drm_flip_driver_schedule_flips()
+	 * has called drm_flip_helper::flip() for all the provided flips.
+	 * Can be used to:
+	 * - commit the flips atomically to the hardware, if the
+	 *   hardware provides some mechanism to do that.
+	 * - flush posted writes to make sure all the flips have reached
+	 *   the hardware
+	 * Called with drm_flip_driver::lock held.
+	 */
+	void (*flush)(struct drm_flip_driver *driver);
+};
+
+/*
+ * The driver needs one drm_flip_driver to
+ * coordinates the drm_flip mechanism.
+ */
+struct drm_flip_driver {
+	/* protects drm_flip_driver, drm_flip_helper, and drm_flip internals. */
+	spinlock_t lock;
+
+	/* list of drm_flips waiting to be finished, protected by 'lock' */
+	struct list_head finish_list;
+
+	/* list of drm_flips waiting to be cleaned up, protected by 'lock' */
+	struct list_head cleanup_list;
+
+	/* work used to finish the drm_flips */
+	struct work_struct finish_work;
+
+	/* work used to clean up the drm_flips */
+	struct work_struct cleanup_work;
+
+	/* driver provided callback functions */
+	const struct drm_flip_driver_funcs *funcs;
+
+	/* work queue for finish_work and cleanup_work */
+	struct workqueue_struct *wq;
+};
+
+/* Driver callbacks for drm_flip_helper */
+struct drm_flip_helper_funcs {
+	/*
+	 * Optional function to perform heavy but non-timing
+	 * critial preparations for the flip.
+	 * Called from drm_flip_driver_prepare_flips() with
+	 * no extra locks being held.
+	 */
+	void (*prepare)(struct drm_flip *flip);
+	/*
+	 * Instruct the hardware to flip on the next vblank.
+	 * Must return true, iff pending_flip exists, and has
+	 * actually flipped (ie. now being scanned out).
+	 * Otherwise must return false.
+	 * Called with drm_flip_driver::lock held.
+	 */
+	bool (*flip)(struct drm_flip *flip,
+		     struct drm_flip *pending_flip);
+	/*
+	 * Called from drm_flip_helper_vblank() if
+	 * pending_flip exists. Must return true, iff
+	 * pending_flip has actually flipped (ie. now
+	 * being scanned out). Otherwise must return false.
+	 * Called with drm_flip_driver::lock held.
+	 */
+	bool (*vblank)(struct drm_flip *pending_flip);
+
+	/*
+	 * The flip has just occured, or it got overwritten
+	 * by a more recent flip. If the flip occured, it is
+	 * now being scanned out, otherwise it is scheduled
+	 * for cleanup.
+	 * Can be called from drm_flip_driver_schedule_flips(),
+	 * drm_flip_driver_complete_flips(), or from
+	 * drm_flip_helper_vblank().
+	 * Called with drm_flip_driver::lock held.
+	 */
+	void (*complete)(struct drm_flip *flip);
+
+	/*
+	 * Perform finishing steps on the flip. Called from a workqueue
+	 * soon after the flip has completed. The flip's buffer may be
+	 * actively scanned out.
+	 * Called with no locks being held.
+	 */
+	void (*finish)(struct drm_flip *flip);
+
+	/*
+	 * Perform final cleanup on the flip. Called from a workqueue
+	 * after the flip's buffer is no longer being scanned out.
+	 * Called with no locks being held.
+	 */
+	void (*cleanup)(struct drm_flip *flip);
+
+};
+
+/*
+ * The driver needs one drm_flip_helper for each scanout engine it
+ * wants to operate through the drm_flip mechanism.
+ */
+struct drm_flip_helper {
+	/* drm_flip from the previous drm_flip_schedule() call */
+	struct drm_flip *pending_flip;
+	/* drm_flip whose buffer is being scanned out */
+	struct drm_flip *scanout_flip;
+	/* associated drm_flip_driver */
+	struct drm_flip_driver *driver;
+	/* driver provided callback functions */
+	const struct drm_flip_helper_funcs *funcs;
+};
+
+/*
+ * This structure represents a single page flip operation.
+ */
+struct drm_flip {
+	/* associated drm_flip_helper */
+	struct drm_flip_helper *helper;
+	/* has this flip occured? */
+	bool flipped;
+	/* has the finish work been executed for this flip? */
+	bool finished;
+	/* used to keep this flip on various lists */
+	struct list_head list;
+};
+
+/*
+ * Initialize the flip driver.
+ */
+void drm_flip_driver_init(struct drm_flip_driver *driver,
+			  const struct drm_flip_driver_funcs *funcs);
+
+/*
+ * Finalize the flip driver. This will block until all the
+ * pending finish and cleanup work has been completed.
+ */
+void drm_flip_driver_fini(struct drm_flip_driver *driver);
+
+/*
+ * Initialize flip helper.
+ */
+void drm_flip_helper_init(struct drm_flip_helper *helper,
+			  struct drm_flip_driver *driver,
+			  const struct drm_flip_helper_funcs *funcs);
+
+/*
+ * Clear flip helper state. This will forcefully complete the
+ * helper's pending flip (if any).
+ */
+void drm_flip_helper_clear(struct drm_flip_helper *helper);
+
+/*
+ * Finalize the flip helper. This will forcefully complete the
+ * helper's pending flip (if any), and wait for the finish and
+ * cleanup works to finish.
+ */
+void drm_flip_helper_fini(struct drm_flip_helper *helper);
+
+/*
+ * Call this from the driver's vblank handler for the scanout engine
+ * associated with this helper.
+ */
+void drm_flip_helper_vblank(struct drm_flip_helper *helper);
+
+/*
+ * This will call drm_flip_helper::prepare() (if provided) for all the
+ * drm_flips on the list. The purpose is to perform any non-timing critical
+ * preparation steps for the flips before taking locks or disabling interrupts.
+ */
+void drm_flip_driver_prepare_flips(struct drm_flip_driver *driver,
+				   struct list_head *flips);
+
+/*
+ * Schedule the flips on the list to occur on the next vblank.
+ *
+ * This will call drm_flip_helper::flip() for all the drm_flips on the list.
+ * It will then call drm_flip_driver::flush(), after which it will complete
+ * any pending_flip that got overridden by the new flips.
+ *
+ * Unless the hardware provides some mechanism to synchronize the flips, the
+ * time spent until drm_flip_driver::flush() is timing critical and the driver
+ * must somehow make sure it can complete the operation in a seemingly atomic
+ * fashion.
+ */
+void drm_flip_driver_schedule_flips(struct drm_flip_driver *driver,
+				    struct list_head *flips);
+
+/*
+ * This will complete any pending_flip and also all the flips
+ * on the provided list (in that order).
+ *
+ * Call this instead of drm_flip_driver_schedule_flips()
+ * eg. if the hardware powered down, and you just want to keep
+ * the drm_flip mechanim's state consistent w/o waking up the
+ * hardware.
+ */
+void drm_flip_driver_complete_flips(struct drm_flip_driver *driver,
+				    struct list_head *flips);
+
+/*
+ * Initialize the flip structure members.
+ */
+void drm_flip_init(struct drm_flip *flip,
+		   struct drm_flip_helper *helper);
+
+#endif
-- 
1.7.8.6

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC][PATCH 3/4] drm: i915: Pass gem object to intel_finish_fb()

[ville.syrjala]

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

intel_finish_fb() has no use for the drm_framebuffer metadata, so pass
the gem object directly.

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

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index e9eaa8a..0a96d15 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -797,7 +797,7 @@ static void unpin_work_func(struct work_struct *work)
 				intel_wait_for_vblank(dev, pipe);
 				vblank_waited |= 1 << pipe;
 			}
-			intel_finish_fb(fb);
+			intel_finish_fb(obj);
 			intel_unpin_fb_obj(obj);
 
 			mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 46fb4d1..01c1a19 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2188,9 +2188,8 @@ intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 }
 
 int
-intel_finish_fb(struct drm_framebuffer *old_fb)
+intel_finish_fb(struct drm_i915_gem_object *obj)
 {
-	struct drm_i915_gem_object *obj = to_intel_framebuffer(old_fb)->obj;
 	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
 	bool was_interruptible = dev_priv->mm.interruptible;
 	int ret;
@@ -2250,7 +2249,7 @@ _intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 	}
 
 	if (old_fb)
-		intel_finish_fb(old_fb);
+		intel_finish_fb(to_intel_framebuffer(old_fb)->obj);
 
 	ret = dev_priv->display.update_plane(crtc, crtc->fb, x, y);
 	if (ret) {
@@ -2862,7 +2861,7 @@ static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
 		return;
 
 	mutex_lock(&dev->struct_mutex);
-	intel_finish_fb(crtc->fb);
+	intel_finish_fb(to_intel_framebuffer(crtc->fb)->obj);
 	mutex_unlock(&dev->struct_mutex);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9deb3f4..fa81676 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -440,7 +440,7 @@ extern int intel_pin_and_fence_fb_obj(struct drm_device *dev,
 				      struct drm_i915_gem_object *obj,
 				      struct intel_ring_buffer *pipelined);
 extern void intel_unpin_fb_obj(struct drm_i915_gem_object *obj);
-extern int intel_finish_fb(struct drm_framebuffer *fb);
+extern int intel_finish_fb(struct drm_i915_gem_object *obj);
 
 extern int intel_framebuffer_init(struct drm_device *dev,
 				  struct intel_framebuffer *ifb,
-- 
1.7.8.6

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC][PATCH 4/4] drm: i915: Atomic pageflip WIP

[ville.syrjala]

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

Use the drm_flip helper to implement atomic page flipping.

Work in progress. Ignore the huge mess in intel_sprite.c for now.
---
 drivers/gpu/drm/i915/i915_drv.h      |    4 +
 drivers/gpu/drm/i915/i915_irq.c      |   10 +-
 drivers/gpu/drm/i915/intel_atomic.c  |  449 +++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_display.c |  171 +++++++++----
 drivers/gpu/drm/i915/intel_drv.h     |   29 +++
 drivers/gpu/drm/i915/intel_sprite.c  |  386 ++++++++++++++++++-----------
 6 files changed, 841 insertions(+), 208 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 57e4894..e29994f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -41,6 +41,8 @@
 #include <linux/intel-iommu.h>
 #include <linux/kref.h>
 
+#include "drm_flip.h"
+
 /* General customization:
  */
 
@@ -845,6 +847,8 @@ typedef struct drm_i915_private {
 	struct work_struct parity_error_work;
 	bool hw_contexts_disabled;
 	uint32_t hw_context_size;
+
+	struct drm_flip_driver flip_driver;
 } drm_i915_private_t;
 
 /* Iterate over initialised rings */
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 23f2ea0..78ff3e0 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -37,6 +37,8 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
+void intel_handle_vblank(struct drm_device *dev, int pipe);
+
 /* For display hotplug interrupt */
 static void
 ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
@@ -548,7 +550,7 @@ static irqreturn_t valleyview_irq_handler(DRM_IRQ_ARGS)
 
 		for_each_pipe(pipe) {
 			if (pipe_stats[pipe] & PIPE_VBLANK_INTERRUPT_STATUS)
-				drm_handle_vblank(dev, pipe);
+				intel_handle_vblank(dev, pipe);
 
 			if (pipe_stats[pipe] & PLANE_FLIPDONE_INT_STATUS_VLV) {
 				intel_prepare_page_flip(dev, pipe);
@@ -686,7 +688,7 @@ static irqreturn_t ivybridge_irq_handler(DRM_IRQ_ARGS)
 				intel_finish_page_flip_plane(dev, i);
 			}
 			if (de_iir & (DE_PIPEA_VBLANK_IVB << (5 * i)))
-				drm_handle_vblank(dev, i);
+				intel_handle_vblank(dev, i);
 		}
 
 		/* check event from PCH */
@@ -779,10 +781,10 @@ static irqreturn_t ironlake_irq_handler(DRM_IRQ_ARGS)
 	}
 
 	if (de_iir & DE_PIPEA_VBLANK)
-		drm_handle_vblank(dev, 0);
+		intel_handle_vblank(dev, 0);
 
 	if (de_iir & DE_PIPEB_VBLANK)
-		drm_handle_vblank(dev, 1);
+		intel_handle_vblank(dev, 1);
 
 	/* check event from PCH */
 	if (de_iir & DE_PCH_EVENT) {
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 0a96d15..6d8d7d3 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -3,6 +3,7 @@
 
 #include <drm/drmP.h>
 #include <drm/drm_crtc.h>
+#include <drm/drm_flip.h>
 
 #include "intel_drv.h"
 
@@ -1309,6 +1310,46 @@ static void update_props(const struct drm_device *dev,
 	}
 }
 
+static int atomic_pipe_commit(struct drm_device *dev,
+			      struct intel_atomic_state *state,
+			      int pipe);
+
+static int try_atomic_commit(struct drm_device *dev, struct intel_atomic_state *s)
+{
+	int ret;
+	int i;
+	u32 pipes = 0;
+
+	for (i = 0; i < dev->mode_config.num_crtc; i++) {
+		struct intel_crtc_state *st = &s->crtc[i];
+
+		if (st->mode_dirty)
+			return -EAGAIN;
+
+		if (st->fb_dirty)
+			pipes |= 1 << to_intel_crtc(st->crtc)->pipe;
+	}
+
+	for (i = 0; i < dev->mode_config.num_plane; i++) {
+		struct intel_plane_state *st = &s->plane[i];
+
+		if (st->dirty)
+			pipes |= 1 << to_intel_plane(st->plane)->pipe;
+	}
+
+	if (hweight32(pipes) != 1)
+		return -EAGAIN;
+
+	ret = atomic_pipe_commit(dev, s, ffs(pipes) - 1);
+	if (ret)
+		return ret;
+
+	/* don't restore the old state in end() */
+	s->dirty = false;
+
+	return 0;
+}
+
 static int intel_atomic_commit(struct drm_device *dev, void *state)
 {
 	struct intel_atomic_state *s = state;
@@ -1325,17 +1366,23 @@ static int intel_atomic_commit(struct drm_device *dev, void *state)
 	if (ret)
 		return ret;
 
-	ret = apply_config(dev, s);
+	/* try to apply asynchronously and atomically */
+	ret = try_atomic_commit(dev, s);
+
+	/* fall back to sync/non-atomic */
 	if (ret) {
-		unpin_fbs(dev, s);
-		unpin_cursors(dev, s);
-		s->restore_hw = true;
-		return ret;
-	}
+		ret = apply_config(dev, s);
+		if (ret) {
+			unpin_fbs(dev, s);
+			unpin_cursors(dev, s);
+			s->restore_hw = true;
+			return ret;
+		}
 
-	unpin_old_fbs(dev, s);
+		unpin_old_fbs(dev, s);
 
-	unpin_old_cursors(dev, s);
+		unpin_old_cursors(dev, s);
+	}
 
 	update_plane_obj(dev, s);
 
@@ -1390,6 +1437,9 @@ static struct {
 	{ &prop_cursor_y, "CURSOR_Y", INT_MIN, INT_MAX },
 };
 
+static void intel_flip_init(struct drm_device *dev);
+static void intel_flip_fini(struct drm_device *dev);
+
 int intel_atomic_init(struct drm_device *dev)
 {
 	struct drm_crtc *crtc;
@@ -1452,6 +1502,8 @@ int intel_atomic_init(struct drm_device *dev)
 
 	unpin_workqueue = create_workqueue("i915_fb_unpin");
 
+	intel_flip_init(dev);
+
 	return 0;
 
  out:
@@ -1465,6 +1517,8 @@ void intel_atomic_fini(struct drm_device *dev)
 {
 	struct drm_crtc *crtc;
 
+	intel_flip_fini(dev);
+
 	destroy_workqueue(unpin_workqueue);
 
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
@@ -1485,3 +1539,382 @@ void intel_atomic_fini(struct drm_device *dev)
 	drm_property_destroy(dev, prop_src_y);
 	drm_property_destroy(dev, prop_src_x);
 }
+
+void intel_plane_calc(struct drm_crtc *crtc, struct drm_framebuffer *fb, int x, int y);
+void intel_plane_prepare(struct drm_crtc *crtc);
+void intel_plane_commit(struct drm_crtc *crtc);
+void intel_sprite_calc(struct drm_plane *plane, struct drm_framebuffer *fb, const struct intel_plane_coords *coords);
+void intel_sprite_prepare(struct drm_plane *plane);
+void intel_sprite_commit(struct drm_plane *plane);
+
+enum {
+	/* somwehat arbitrary value */
+	INTEL_VBL_CNT_TIMEOUT = 5,
+};
+
+struct intel_flip
+{
+	struct drm_flip base;
+	u32 vbl_count;
+	bool vblank_ref;
+	struct drm_crtc *crtc;
+	struct drm_plane *plane;
+	struct drm_i915_gem_object *old_bo;
+};
+
+static void intel_flip_complete(struct drm_flip *flip)
+{
+	struct intel_flip *intel_flip =
+		container_of(flip, struct intel_flip, base);
+	struct drm_device *dev = intel_flip->crtc->dev;
+	int pipe = to_intel_crtc(intel_flip->crtc)->pipe;
+
+	// send flip event
+
+	if (intel_flip->vblank_ref)
+		drm_vblank_put(dev, pipe);
+
+	// allow GPU to render to old_obj
+}
+
+static void intel_flip_finish(struct drm_flip *flip)
+{
+	struct intel_flip *intel_flip =
+		container_of(flip, struct intel_flip, base);
+	struct drm_device *dev = intel_flip->crtc->dev;
+
+	if (intel_flip->old_bo) {
+		mutex_lock(&dev->struct_mutex);
+
+		intel_finish_fb(intel_flip->old_bo);
+		intel_unpin_fb_obj(intel_flip->old_bo);
+
+		mutex_unlock(&dev->struct_mutex);
+	}
+
+}
+
+static void intel_flip_cleanup(struct drm_flip *flip)
+{
+	struct intel_flip *intel_flip =
+		container_of(flip, struct intel_flip, base);
+
+	kfree(intel_flip);
+}
+
+static void intel_flip_driver_flush(struct drm_flip_driver *driver)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(driver, struct drm_i915_private, flip_driver);
+
+	/* Flush posted writes */
+	I915_READ(PIPESTAT(PIPE_A));
+}
+
+static u32 get_vbl_count(struct drm_crtc *crtc)
+{
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
+	int pipe = intel_crtc->pipe;
+	u32 high, low1, low2, dsl;
+	unsigned int timeout = 0;
+
+	/*
+	 * FIXME check where the frame counter increments, and if
+	 * it happens in the middle of some line, take appropriate
+	 * measures to get a sensible reading.
+	 */
+
+	/* All reads must be satisfied during the same frame */
+	do {
+		low1 = I915_READ(PIPEFRAMEPIXEL(pipe)) >> PIPE_FRAME_LOW_SHIFT;
+		high = I915_READ(PIPEFRAME(pipe)) << 8;
+		dsl = I915_READ(PIPEDSL(pipe));
+		low2 = I915_READ(PIPEFRAMEPIXEL(pipe)) >> PIPE_FRAME_LOW_SHIFT;
+	} while (low1 != low2 && timeout++ < INTEL_VBL_CNT_TIMEOUT);
+
+	if (timeout >= INTEL_VBL_CNT_TIMEOUT)
+		dev_warn(crtc->dev->dev,
+			 "Timed out while determining VBL count for pipe %d\n",
+			 intel_crtc->pipe);
+
+	return ((high | low2) +
+		((dsl >= crtc->hwmode.crtc_vdisplay) &&
+		 (dsl < crtc->hwmode.crtc_vtotal - 1))) & 0xffffff;
+}
+
+static unsigned int usecs_to_scanlines(struct drm_crtc *crtc,
+				       unsigned int usecs)
+{
+	/* paranoia */
+	if (!crtc->hwmode.crtc_htotal)
+		return 1;
+
+	return DIV_ROUND_UP(usecs * crtc->hwmode.clock,
+			    1000 * crtc->hwmode.crtc_htotal);
+}
+
+static void intel_pipe_vblank_evade(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	int pipe = intel_crtc->pipe;
+	/* FIXME needs to be calibrated sensibly */
+	u32 min = crtc->hwmode.crtc_vdisplay - usecs_to_scanlines(crtc, 60);
+	u32 max = crtc->hwmode.crtc_vdisplay - 1;
+	long timeout = msecs_to_jiffies(1);
+	u32 val;
+
+	bool vblank_ref = drm_vblank_get(dev, pipe) == 0;
+
+	intel_crtc->vbl_received = false;
+
+	val = I915_READ(PIPEDSL(pipe));
+
+	while (val >= min && val <= max && timeout > 0) {
+		local_irq_enable();
+
+		timeout = wait_event_timeout(intel_crtc->vbl_wait,
+					     intel_crtc->vbl_received,
+					     timeout);
+
+		local_irq_disable();
+
+		intel_crtc->vbl_received = false;
+
+		val = I915_READ(PIPEDSL(pipe));
+	}
+
+	if (vblank_ref)
+		drm_vblank_put(dev, pipe);
+
+	if (val >= min && val <= max)
+		dev_warn(dev->dev,
+			 "Page flipping close to vblank start (DSL=%u, VBL=%u)\n",
+			 val, crtc->hwmode.crtc_vdisplay);
+}
+
+static bool vbl_count_after_eq(u32 a, u32 b)
+{
+	return !((a - b) & 0x800000);
+}
+
+static bool intel_vbl_check(struct drm_flip *pending_flip, u32 vbl_count)
+{
+	struct intel_flip *old_intel_flip =
+		container_of(pending_flip, struct intel_flip, base);
+
+	return vbl_count_after_eq(vbl_count, old_intel_flip->vbl_count);
+}
+
+static void intel_flip_prepare(struct drm_flip *flip)
+{
+	struct intel_flip *intel_flip =
+		container_of(flip, struct intel_flip, base);
+
+	/* FIXME some other pipe/pf stuff could be performed here as well. */
+
+	/* stage double buffer updates which need arming by something else */
+	if (intel_flip->plane)
+		intel_sprite_prepare(intel_flip->plane);
+	else
+		intel_plane_prepare(intel_flip->crtc);
+}
+
+static bool intel_flip_flip(struct drm_flip *flip,
+			    struct drm_flip *pending_flip)
+{
+	struct intel_flip *intel_flip = container_of(flip, struct intel_flip, base);
+	struct drm_crtc *crtc = intel_flip->crtc;
+	struct drm_device *dev = crtc->dev;
+	int pipe = to_intel_crtc(crtc)->pipe;
+	u32 vbl_count;
+
+	struct intel_flip *old_intel_flip = NULL;
+	if (pending_flip)
+		old_intel_flip = container_of(pending_flip, struct intel_flip, base);
+
+	intel_flip->vblank_ref = drm_vblank_get(dev, pipe) == 0;
+
+	vbl_count = get_vbl_count(crtc);
+
+	/* arm all the double buffer registers */
+	if (intel_flip->plane)
+		intel_sprite_commit(intel_flip->plane);
+	else
+		intel_plane_commit(crtc);
+
+	/* This flip will happen on the next vblank */
+	intel_flip->vbl_count = (vbl_count + 1) & 0xffffff;
+
+	/* FIXME oops in unpin when swapping old bos */
+#if 0
+	if (pending_flip) {
+		struct intel_flip *old_intel_flip =
+			container_of(pending_flip, struct intel_flip, base);
+		bool flipped = intel_vbl_check(pending_flip, vbl_count);
+
+		if (!flipped)
+			swap(intel_flip->old_bo, old_intel_flip->old_bo);
+
+		return flipped;
+	}
+#endif
+
+	return false;
+}
+
+static bool intel_flip_vblank(struct drm_flip *pending_flip)
+{
+	struct intel_flip *old_intel_flip =
+		container_of(pending_flip, struct intel_flip, base);
+
+	u32 vbl_count = get_vbl_count(old_intel_flip->crtc);
+
+	return intel_vbl_check(pending_flip, vbl_count);
+}
+
+static const struct drm_flip_helper_funcs intel_flip_funcs = {
+	.prepare = intel_flip_prepare,
+	.flip = intel_flip_flip,
+	.vblank = intel_flip_vblank,
+	.complete = intel_flip_complete,
+	.finish = intel_flip_finish,
+	.cleanup = intel_flip_cleanup,
+};
+
+static const struct drm_flip_driver_funcs intel_flip_driver_funcs = {
+	.flush = intel_flip_driver_flush,
+};
+
+static void intel_flip_init(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc;
+	struct intel_plane *intel_plane;
+
+	drm_flip_driver_init(&dev_priv->flip_driver, &intel_flip_driver_funcs);
+
+	list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head) {
+		init_waitqueue_head(&intel_crtc->vbl_wait);
+
+		drm_flip_helper_init(&intel_crtc->flip_helper,
+				     &dev_priv->flip_driver, &intel_flip_funcs);
+	}
+
+	list_for_each_entry(intel_plane, &dev->mode_config.plane_list, base.head)
+		drm_flip_helper_init(&intel_plane->flip_helper,
+				     &dev_priv->flip_driver, &intel_flip_funcs);
+}
+
+static void intel_flip_fini(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc;
+	struct drm_plane *plane;
+
+	list_for_each_entry(plane, &dev->mode_config.plane_list, head)
+		drm_flip_helper_fini(&to_intel_plane(plane)->flip_helper);
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
+		drm_flip_helper_fini(&to_intel_crtc(crtc)->flip_helper);
+
+	drm_flip_driver_fini(&dev_priv->flip_driver);
+}
+
+static int atomic_pipe_commit(struct drm_device *dev,
+			      struct intel_atomic_state *state,
+			      int pipe)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_flip *intel_flip, *next;
+	LIST_HEAD(flips);
+	int i;
+
+	for (i = 0; i < dev->mode_config.num_crtc; i++) {
+		struct intel_crtc_state *st = &state->crtc[i];
+		struct drm_crtc *crtc = st->crtc;
+		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+		if (!st->fb_dirty)
+			continue;
+
+		if (intel_crtc->pipe != pipe)
+			continue;
+
+		intel_flip = kzalloc(sizeof *intel_flip, GFP_KERNEL);
+		if (!intel_flip)
+			goto out;
+
+		intel_plane_calc(crtc, crtc->fb, crtc->x, crtc->y);
+
+		drm_flip_init(&intel_flip->base, &intel_crtc->flip_helper);
+		intel_flip->crtc = crtc;
+		if (st->old_fb)
+			intel_flip->old_bo = to_intel_framebuffer(st->old_fb)->obj;
+
+		list_add_tail(&intel_flip->base.list, &flips);
+	}
+
+	for (i = 0; i < dev->mode_config.num_plane; i++) {
+		struct intel_plane_state *st = &state->plane[i];
+		struct drm_plane *plane = st->plane;
+		struct intel_plane *intel_plane = to_intel_plane(plane);
+
+		if (!st->dirty)
+			continue;
+
+		if (intel_plane->pipe != pipe)
+			continue;
+
+		intel_flip = kzalloc(sizeof *intel_flip, GFP_KERNEL);
+		if (!intel_flip)
+			goto out;
+
+		intel_sprite_calc(plane, plane->fb, &st->coords);
+
+		drm_flip_init(&intel_flip->base, &intel_plane->flip_helper);
+		intel_flip->crtc = intel_get_crtc_for_pipe(dev, pipe);
+		intel_flip->plane = plane;
+		if (st->old_fb)
+			intel_flip->old_bo = to_intel_framebuffer(st->old_fb)->obj;
+
+		list_add_tail(&intel_flip->base.list, &flips);
+	}
+
+	/* FIXME if pipe is not enabled complete flip immediately */
+
+	drm_flip_driver_prepare_flips(&dev_priv->flip_driver, &flips);
+
+	local_irq_disable();
+
+	intel_pipe_vblank_evade(intel_get_crtc_for_pipe(dev, pipe));
+
+	drm_flip_driver_schedule_flips(&dev_priv->flip_driver, &flips);
+
+	local_irq_enable();
+
+	return 0;
+
+ out:
+	list_for_each_entry_safe(intel_flip, next, &flips, base.list)
+		kfree(intel_flip);
+
+	return -ENOMEM;
+}
+
+void intel_handle_vblank(struct drm_device *dev, int pipe)
+{
+	struct intel_crtc *intel_crtc = to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
+	struct intel_plane *intel_plane;
+
+	drm_handle_vblank(dev, pipe);
+
+	drm_flip_helper_vblank(&intel_crtc->flip_helper);
+
+	list_for_each_entry(intel_plane, &dev->mode_config.plane_list, base.head) {
+		if (intel_plane->pipe == pipe)
+			drm_flip_helper_vblank(&intel_plane->flip_helper);
+	}
+}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 01c1a19..23fbc0b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1988,8 +1988,36 @@ static unsigned long gen4_compute_dspaddr_offset_xtiled(int *x, int *y,
 	return tile_rows * pitch * 8 + tiles * 4096;
 }
 
-static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
-			     int x, int y)
+void intel_plane_prepare(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	int plane = intel_crtc->plane;
+	const struct intel_plane_regs *regs = &intel_crtc->primary_regs;
+
+	I915_WRITE(DSPCNTR(plane), regs->cntr);
+	I915_WRITE(DSPSTRIDE(plane), regs->stride);
+}
+
+void intel_plane_commit(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	int plane = intel_crtc->plane;
+	const struct intel_plane_regs *regs = &intel_crtc->primary_regs;
+
+	if (INTEL_INFO(dev)->gen >= 4) {
+		I915_WRITE(DSPSURF(plane), regs->surf);
+		I915_WRITE(DSPTILEOFF(plane), regs->tileoff);
+		I915_WRITE(DSPLINOFF(plane), regs->linoff);
+	} else
+		I915_WRITE(DSPADDR(plane), regs->addr);
+}
+
+static int i9xx_calc_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
+			   int x, int y)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1998,9 +2026,8 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 	struct drm_i915_gem_object *obj;
 	int plane = intel_crtc->plane;
 	unsigned long linear_offset;
-	u32 dspcntr;
-	u32 reg;
 	unsigned int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
+	struct intel_plane_regs *regs = &intel_crtc->primary_regs;
 
 	switch (plane) {
 	case 0:
@@ -2014,36 +2041,35 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 	intel_fb = to_intel_framebuffer(fb);
 	obj = intel_fb->obj;
 
-	reg = DSPCNTR(plane);
-	dspcntr = I915_READ(reg);
+	regs->cntr = I915_READ(DSPCNTR(plane));
 	/* Mask out pixel format bits in case we change it */
-	dspcntr &= ~DISPPLANE_PIXFORMAT_MASK;
+	regs->cntr &= ~DISPPLANE_PIXFORMAT_MASK;
 	switch (fb->pixel_format) {
 	case DRM_FORMAT_C8:
-		dspcntr |= DISPPLANE_8BPP;
+		regs->cntr |= DISPPLANE_8BPP;
 		break;
 	case DRM_FORMAT_XRGB1555:
 	case DRM_FORMAT_ARGB1555:
-		dspcntr |= DISPPLANE_BGRX555;
+		regs->cntr |= DISPPLANE_BGRX555;
 		break;
 	case DRM_FORMAT_RGB565:
-		dspcntr |= DISPPLANE_BGRX565;
+		regs->cntr |= DISPPLANE_BGRX565;
 		break;
 	case DRM_FORMAT_XRGB8888:
 	case DRM_FORMAT_ARGB8888:
-		dspcntr |= DISPPLANE_BGRX888;
+		regs->cntr |= DISPPLANE_BGRX888;
 		break;
 	case DRM_FORMAT_XBGR8888:
 	case DRM_FORMAT_ABGR8888:
-		dspcntr |= DISPPLANE_RGBX888;
+		regs->cntr |= DISPPLANE_RGBX888;
 		break;
 	case DRM_FORMAT_XRGB2101010:
 	case DRM_FORMAT_ARGB2101010:
-		dspcntr |= DISPPLANE_BGRX101010;
+		regs->cntr |= DISPPLANE_BGRX101010;
 		break;
 	case DRM_FORMAT_XBGR2101010:
 	case DRM_FORMAT_ABGR2101010:
-		dspcntr |= DISPPLANE_RGBX101010;
+		regs->cntr |= DISPPLANE_RGBX101010;
 		break;
 	default:
 		DRM_ERROR("Unknown pixel format 0x%08x\n", fb->pixel_format);
@@ -2052,13 +2078,11 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 
 	if (INTEL_INFO(dev)->gen >= 4) {
 		if (obj->tiling_mode != I915_TILING_NONE)
-			dspcntr |= DISPPLANE_TILED;
+			regs->cntr |= DISPPLANE_TILED;
 		else
-			dspcntr &= ~DISPPLANE_TILED;
+			regs->cntr &= ~DISPPLANE_TILED;
 	}
 
-	I915_WRITE(reg, dspcntr);
-
 	linear_offset = fb->offsets[0] + y * fb->pitches[0] + x * cpp;
 
 	if (INTEL_INFO(dev)->gen >= 4) {
@@ -2072,21 +2096,42 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 
 	DRM_DEBUG_KMS("Writing base %08X %08lX %d %d %d\n",
 		      obj->gtt_offset, linear_offset, x, y, fb->pitches[0]);
-	I915_WRITE(DSPSTRIDE(plane), fb->pitches[0]);
+
+	regs->stride = fb->pitches[0];
+
 	if (INTEL_INFO(dev)->gen >= 4) {
-		I915_MODIFY_DISPBASE(DSPSURF(plane),
-				     obj->gtt_offset + intel_crtc->dspaddr_offset);
-		I915_WRITE(DSPTILEOFF(plane), (y << 16) | x);
-		I915_WRITE(DSPLINOFF(plane), linear_offset);
+		regs->surf = I915_LO_DISPBASE(I915_READ(DSPSURF(plane)));
+		regs->surf |= obj->gtt_offset + intel_crtc->dspaddr_offset;
+		regs->tileoff = (y << 16) | x;
+		regs->linoff = linear_offset;
 	} else
-		I915_WRITE(DSPADDR(plane), obj->gtt_offset + linear_offset);
-	POSTING_READ(reg);
+		regs->addr = obj->gtt_offset + linear_offset;
 
 	return 0;
 }
 
-static int ironlake_update_plane(struct drm_crtc *crtc,
-				 struct drm_framebuffer *fb, int x, int y)
+static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
+			     int x, int y)
+{
+	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	int plane = intel_crtc->plane;
+	int ret;
+
+	ret = i9xx_calc_plane(crtc, fb, x, y);
+	if (ret)
+		return ret;
+
+	intel_plane_prepare(crtc);
+	intel_plane_commit(crtc);
+
+	POSTING_READ(DSPCNTR(plane));
+
+	return 0;
+}
+
+static int ironlake_calc_plane(struct drm_crtc *crtc,
+			       struct drm_framebuffer *fb, int x, int y)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -2095,9 +2140,8 @@ static int ironlake_update_plane(struct drm_crtc *crtc,
 	struct drm_i915_gem_object *obj;
 	int plane = intel_crtc->plane;
 	unsigned long linear_offset;
-	u32 dspcntr;
-	u32 reg;
 	unsigned int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
+	struct intel_plane_regs *regs = &intel_crtc->primary_regs;
 
 	switch (plane) {
 	case 0:
@@ -2112,32 +2156,31 @@ static int ironlake_update_plane(struct drm_crtc *crtc,
 	intel_fb = to_intel_framebuffer(fb);
 	obj = intel_fb->obj;
 
-	reg = DSPCNTR(plane);
-	dspcntr = I915_READ(reg);
+	regs->cntr = I915_READ(DSPCNTR(plane));
 	/* Mask out pixel format bits in case we change it */
-	dspcntr &= ~DISPPLANE_PIXFORMAT_MASK;
+	regs->cntr &= ~DISPPLANE_PIXFORMAT_MASK;
 	switch (fb->pixel_format) {
 	case DRM_FORMAT_C8:
-		dspcntr |= DISPPLANE_8BPP;
+		regs->cntr |= DISPPLANE_8BPP;
 		break;
 	case DRM_FORMAT_RGB565:
-		dspcntr |= DISPPLANE_BGRX565;
+		regs->cntr |= DISPPLANE_BGRX565;
 		break;
 	case DRM_FORMAT_XRGB8888:
 	case DRM_FORMAT_ARGB8888:
-		dspcntr |= DISPPLANE_BGRX888;
+		regs->cntr |= DISPPLANE_BGRX888;
 		break;
 	case DRM_FORMAT_XBGR8888:
 	case DRM_FORMAT_ABGR8888:
-		dspcntr |= DISPPLANE_RGBX888;
+		regs->cntr |= DISPPLANE_RGBX888;
 		break;
 	case DRM_FORMAT_XRGB2101010:
 	case DRM_FORMAT_ARGB2101010:
-		dspcntr |= DISPPLANE_BGRX101010;
+		regs->cntr |= DISPPLANE_BGRX101010;
 		break;
 	case DRM_FORMAT_XBGR2101010:
 	case DRM_FORMAT_ABGR2101010:
-		dspcntr |= DISPPLANE_RGBX101010;
+		regs->cntr |= DISPPLANE_RGBX101010;
 		break;
 	default:
 		DRM_ERROR("Unknown pixel format 0x%08x\n", fb->pixel_format);
@@ -2145,33 +2188,57 @@ static int ironlake_update_plane(struct drm_crtc *crtc,
 	}
 
 	if (obj->tiling_mode != I915_TILING_NONE)
-		dspcntr |= DISPPLANE_TILED;
+		regs->cntr |= DISPPLANE_TILED;
 	else
-		dspcntr &= ~DISPPLANE_TILED;
+		regs->cntr &= ~DISPPLANE_TILED;
 
 	/* must disable */
-	dspcntr |= DISPPLANE_TRICKLE_FEED_DISABLE;
-
-	I915_WRITE(reg, dspcntr);
+	regs->cntr |= DISPPLANE_TRICKLE_FEED_DISABLE;
 
 	linear_offset = fb->offsets[0] + y * fb->pitches[0] + x * (fb->bits_per_pixel / 8);
-	intel_crtc->dspaddr_offset =
-		gen4_compute_dspaddr_offset_xtiled(&x, &y,
-						   cpp, fb->pitches[0]);
+	intel_crtc->dspaddr_offset = gen4_compute_dspaddr_offset_xtiled(&x, &y, cpp, fb->pitches[0]);
 	linear_offset -= intel_crtc->dspaddr_offset;
 
 	DRM_DEBUG_KMS("Writing base %08X %08lX %d %d %d\n",
 		      obj->gtt_offset, linear_offset, x, y, fb->pitches[0]);
-	I915_WRITE(DSPSTRIDE(plane), fb->pitches[0]);
-	I915_MODIFY_DISPBASE(DSPSURF(plane),
-			     obj->gtt_offset + intel_crtc->dspaddr_offset);
-	I915_WRITE(DSPTILEOFF(plane), (y << 16) | x);
-	I915_WRITE(DSPLINOFF(plane), linear_offset);
-	POSTING_READ(reg);
+	regs->stride = fb->pitches[0];
+	regs->surf = I915_LO_DISPBASE(I915_READ(DSPSURF(plane)));
+	regs->surf |= obj->gtt_offset + intel_crtc->dspaddr_offset;
+	regs->tileoff = (y << 16) | x;
+	regs->linoff = linear_offset;
 
 	return 0;
 }
 
+static int ironlake_update_plane(struct drm_crtc *crtc,
+				 struct drm_framebuffer *fb, int x, int y)
+{
+	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	int plane = intel_crtc->plane;
+	int ret;
+
+	ret = ironlake_calc_plane(crtc, fb, x, y);
+	if (ret)
+		return ret;
+
+	intel_plane_prepare(crtc);
+	intel_plane_commit(crtc);
+
+	POSTING_READ(DSPCNTR(plane));
+
+	return 0;
+}
+
+int intel_plane_calc(struct drm_crtc *crtc,
+		     struct drm_framebuffer *fb, int x, int y)
+{
+	if (HAS_PCH_SPLIT(crtc->dev))
+		return ironlake_calc_plane(crtc, fb, x, y);
+	else
+		return i9xx_calc_plane(crtc, fb, x, y);
+}
+
 /* Assume fb object is pinned & idle & fenced and just update base pointers */
 static int
 intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index fa81676..52b0cc7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -31,6 +31,7 @@
 #include "drm_crtc.h"
 #include "drm_crtc_helper.h"
 #include "drm_fb_helper.h"
+#include "drm_flip.h"
 
 #define _wait_for(COND, MS, W) ({ \
 	unsigned long timeout__ = jiffies + msecs_to_jiffies(MS);	\
@@ -162,6 +163,21 @@ struct intel_connector {
 	struct intel_encoder *encoder;
 };
 
+struct intel_plane_regs {
+	u32 cntr;
+	u32 addr;
+	u32 linoff;
+	u32 stride;
+	u32 pos;
+	u32 size;
+	u32 keyval;
+	u32 keymsk;
+	u32 surf;
+	u32 keymaxval;
+	u32 tileoff;
+	u32 scale;
+};
+
 struct intel_crtc {
 	struct drm_crtc base;
 	enum pipe pipe;
@@ -191,6 +207,12 @@ struct intel_crtc {
 
 	/* We can share PLLs across outputs if the timings match */
 	struct intel_pch_pll *pch_pll;
+
+	struct intel_plane_regs primary_regs;
+
+	struct drm_flip_helper flip_helper;
+	wait_queue_head_t vbl_wait;
+	bool vbl_received;
 };
 
 struct intel_plane_coords {
@@ -208,6 +230,7 @@ struct intel_plane {
 	struct drm_i915_gem_object *obj;
 	int max_downscale;
 	u32 lut_r[1024], lut_g[1024], lut_b[1024];
+	struct intel_plane_regs regs;
 	void (*update_plane)(struct drm_plane *plane,
 			     struct drm_framebuffer *fb,
 			     const struct intel_plane_coords *clip);
@@ -216,6 +239,7 @@ struct intel_plane {
 			       struct drm_intel_sprite_colorkey *key);
 	void (*get_colorkey)(struct drm_plane *plane,
 			     struct drm_intel_sprite_colorkey *key);
+	struct drm_flip_helper flip_helper;
 };
 
 struct intel_watermark_params {
@@ -525,4 +549,9 @@ extern void intel_crtc_cursor_commit(struct drm_crtc *crtc, uint32_t handle,
 				     struct drm_i915_gem_object *obj,
 				     uint32_t addr);
 
+extern int intel_calc_primary(struct drm_crtc *crtc,
+			    struct drm_framebuffer *fb, int x, int y);
+extern void intel_prepare_primary(struct drm_crtc *crtc);
+extern void intel_commit_primary(struct drm_crtc *crtc);
+
 #endif /* __INTEL_DRV_H__ */
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index d456cd2..3049a62 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -187,12 +187,10 @@ int intel_check_plane(const struct drm_plane *plane,
 }
 
 static void
-ivb_update_plane(struct drm_plane *plane,
-		 struct drm_framebuffer *fb,
-		 const struct intel_plane_coords *coords)
+ivb_calc_plane(struct drm_plane *plane,
+	       struct drm_framebuffer *fb,
+	       const struct intel_plane_coords *coords)
 {
-	struct drm_device *dev = plane->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	const struct drm_i915_gem_object *obj = to_intel_framebuffer(fb)->obj;
 	int crtc_x = coords->crtc_x;
@@ -203,47 +201,48 @@ ivb_update_plane(struct drm_plane *plane,
 	uint32_t y = coords->src_y;
 	uint32_t src_w = coords->src_w;
 	uint32_t src_h = coords->src_h;
-	int pipe = intel_plane->pipe;
-	u32 sprctl, sprscale = 0;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
-
-	sprctl = I915_READ(SPRCTL(pipe));
+	struct intel_plane_regs *regs = &intel_plane->regs;
 
 	/* Mask out pixel format bits in case we change it */
-	sprctl &= ~SPRITE_PIXFORMAT_MASK;
-	sprctl &= ~SPRITE_RGB_ORDER_RGBX;
-	sprctl &= ~SPRITE_YUV_BYTE_ORDER_MASK;
-	sprctl &= ~SPRITE_TILED;
+	regs->cntr &= ~(SPRITE_PIXFORMAT_MASK |
+			SPRITE_RGB_ORDER_RGBX |
+			SPRITE_YUV_BYTE_ORDER_MASK |
+			SPRITE_TRICKLE_FEED_DISABLE |
+			SPRITE_TILED |
+			SPRITE_ENABLE);
 
 	switch (fb->pixel_format) {
 	case DRM_FORMAT_XBGR8888:
-		sprctl |= SPRITE_FORMAT_RGBX888;
+		regs->cntr |= SPRITE_FORMAT_RGBX888;
 		break;
 	case DRM_FORMAT_XRGB8888:
-		sprctl |= SPRITE_FORMAT_RGBX888 | SPRITE_RGB_ORDER_RGBX;
+		regs->cntr |= SPRITE_FORMAT_RGBX888 | SPRITE_RGB_ORDER_RGBX;
 		break;
 	case DRM_FORMAT_YUYV:
-		sprctl |= SPRITE_FORMAT_YUV422 | SPRITE_YUV_ORDER_YUYV;
+		regs->cntr |= SPRITE_FORMAT_YUV422 | SPRITE_YUV_ORDER_YUYV;
 		break;
 	case DRM_FORMAT_YVYU:
-		sprctl |= SPRITE_FORMAT_YUV422 | SPRITE_YUV_ORDER_YVYU;
+		regs->cntr |= SPRITE_FORMAT_YUV422 | SPRITE_YUV_ORDER_YVYU;
 		break;
 	case DRM_FORMAT_UYVY:
-		sprctl |= SPRITE_FORMAT_YUV422 | SPRITE_YUV_ORDER_UYVY;
+		regs->cntr |= SPRITE_FORMAT_YUV422 | SPRITE_YUV_ORDER_UYVY;
 		break;
 	case DRM_FORMAT_VYUY:
-		sprctl |= SPRITE_FORMAT_YUV422 | SPRITE_YUV_ORDER_VYUY;
+		regs->cntr |= SPRITE_FORMAT_YUV422 | SPRITE_YUV_ORDER_VYUY;
 		break;
 	default:
 		BUG();
 	}
 
 	if (obj->tiling_mode != I915_TILING_NONE)
-		sprctl |= SPRITE_TILED;
+		regs->cntr |= SPRITE_TILED;
 
 	/* must disable */
-	sprctl |= SPRITE_TRICKLE_FEED_DISABLE;
-	sprctl |= SPRITE_ENABLE;
+	regs->cntr |= SPRITE_TRICKLE_FEED_DISABLE;
+
+	if (coords->visible)
+		regs->cntr |= SPRITE_ENABLE;
 
 	/* Sizes are 0 based */
 	src_w--;
@@ -251,20 +250,54 @@ ivb_update_plane(struct drm_plane *plane,
 	crtc_w--;
 	crtc_h--;
 
-	intel_update_sprite_watermarks(dev, pipe, crtc_w, pixel_size);
+	/*
+	 * IVB workaround: must disable low power watermarks for at least
+	 * one frame before enabling scaling.  LP watermarks can be re-enabled
+	 * when scaling is disabled.
+	 */
+	if (coords->visible && (crtc_w != src_w || crtc_h != src_h))
+		regs->scale = SPRITE_SCALE_ENABLE | (src_w << 16) | src_h;
+	else
+		regs->scale = 0;
+
+	regs->stride = fb->pitches[0];
+	regs->pos =  (crtc_y << 16) | crtc_x;
+
+	if (obj->tiling_mode != I915_TILING_NONE) {
+		y += fb->offsets[0] / fb->pitches[0];
+		x += fb->offsets[0] % fb->pitches[0] / pixel_size;
+
+		regs->tileoff = (y << 16) | x;
+	} else
+		regs->linoff = fb->offsets[0] + y * fb->pitches[0] + x * pixel_size;
+
+	regs->size = (crtc_h << 16) | crtc_w;
+	regs->surf = I915_LO_DISPBASE(regs->surf) | obj->gtt_offset;
+}
+
+static void
+ivb_commit_plane(struct drm_plane *plane, struct drm_framebuffer *fb)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	int pipe = intel_plane->pipe;
+	int pixel_size = fb ? drm_format_plane_cpp(fb->pixel_format, 0) : 0;
+	const struct intel_plane_regs *regs = &intel_plane->regs;
+
+	intel_update_sprite_watermarks(dev, pipe, regs->size & 0xffff, pixel_size);
 
 	/*
 	 * IVB workaround: must disable low power watermarks for at least
 	 * one frame before enabling scaling.  LP watermarks can be re-enabled
 	 * when scaling is disabled.
 	 */
-	if (crtc_w != src_w || crtc_h != src_h) {
+	if (regs->scale & SPRITE_SCALE_ENABLE) {
 		if (!dev_priv->sprite_scaling_enabled) {
 			dev_priv->sprite_scaling_enabled = true;
 			intel_update_watermarks(dev);
 			intel_wait_for_vblank(dev, pipe);
 		}
-		sprscale = SPRITE_SCALE_ENABLE | (src_w << 16) | src_h;
 	} else {
 		if (dev_priv->sprite_scaling_enabled) {
 			dev_priv->sprite_scaling_enabled = false;
@@ -273,23 +306,31 @@ ivb_update_plane(struct drm_plane *plane,
 		}
 	}
 
-	I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
-	I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
-	if (obj->tiling_mode != I915_TILING_NONE) {
-		y += fb->offsets[0] / fb->pitches[0];
-		x += fb->offsets[0] % fb->pitches[0] / pixel_size;
+	I915_WRITE(SPRKEYVAL(pipe), regs->keyval);
+	I915_WRITE(SPRKEYMAX(pipe), regs->keymaxval);
+	I915_WRITE(SPRKEYMSK(pipe), regs->keymsk);
+	I915_WRITE(SPRSTRIDE(pipe), regs->stride);
+	I915_WRITE(SPRPOS(pipe), regs->pos);
+	I915_WRITE(SPRTILEOFF(pipe), regs->tileoff);
+	I915_WRITE(SPRLINOFF(pipe), regs->linoff);
+	I915_WRITE(SPRSIZE(pipe), regs->size);
+	I915_WRITE(SPRSCALE(pipe), regs->scale);
+	I915_WRITE(SPRCTL(pipe), regs->cntr);
+	I915_WRITE(SPRSURF(pipe), regs->surf);
+}
 
-		I915_WRITE(SPRTILEOFF(pipe), (y << 16) | x);
-	} else {
-		unsigned long offset;
+static void
+ivb_update_plane(struct drm_plane *plane,
+		 struct drm_framebuffer *fb,
+		 const struct intel_plane_coords *coords)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int pipe = to_intel_plane(plane)->pipe;
+
+	ivb_calc_plane(plane, fb, coords);
+	ivb_commit_plane(plane, fb);
 
-		offset = fb->offsets[0] + y * fb->pitches[0] + x * pixel_size;
-		I915_WRITE(SPRLINOFF(pipe), offset);
-	}
-	I915_WRITE(SPRSIZE(pipe), (crtc_h << 16) | crtc_w);
-	I915_WRITE(SPRSCALE(pipe), sprscale);
-	I915_WRITE(SPRCTL(pipe), sprctl);
-	I915_MODIFY_DISPBASE(SPRSURF(pipe), obj->gtt_offset);
 	POSTING_READ(SPRSURF(pipe));
 }
 
@@ -300,12 +341,13 @@ ivb_disable_plane(struct drm_plane *plane)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	int pipe = intel_plane->pipe;
+	struct intel_plane_regs *regs = &intel_plane->regs;
 
-	I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE);
+	regs->cntr &= ~SPRITE_ENABLE;
 	/* Can't leave the scaler enabled... */
-	I915_WRITE(SPRSCALE(pipe), 0);
-	/* Activate double buffered register update */
-	I915_MODIFY_DISPBASE(SPRSURF(pipe), 0);
+	regs->scale = 0;
+
+	ivb_commit_plane(plane, plane->fb);
 	POSTING_READ(SPRSURF(pipe));
 
 	dev_priv->sprite_scaling_enabled = false;
@@ -318,61 +360,50 @@ ivb_update_colorkey(struct drm_plane *plane,
 {
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_plane *intel_plane;
-	u32 sprctl;
-	int ret = 0;
-
-	intel_plane = to_intel_plane(plane);
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_plane_regs *regs = &intel_plane->regs;
 
-	I915_WRITE(SPRKEYVAL(intel_plane->pipe), key->min_value);
-	I915_WRITE(SPRKEYMAX(intel_plane->pipe), key->max_value);
-	I915_WRITE(SPRKEYMSK(intel_plane->pipe), key->channel_mask);
+	regs->keyval = key->min_value;
+	regs->keymaxval = key->max_value;
+	regs->keymsk = key->channel_mask;
 
-	sprctl = I915_READ(SPRCTL(intel_plane->pipe));
-	sprctl &= ~(SPRITE_SOURCE_KEY | SPRITE_DEST_KEY);
+	regs->cntr &= ~(SPRITE_SOURCE_KEY | SPRITE_DEST_KEY);
 	if (key->flags & I915_SET_COLORKEY_DESTINATION)
-		sprctl |= SPRITE_DEST_KEY;
+		regs->cntr |= SPRITE_DEST_KEY;
 	else if (key->flags & I915_SET_COLORKEY_SOURCE)
-		sprctl |= SPRITE_SOURCE_KEY;
-	I915_WRITE(SPRCTL(intel_plane->pipe), sprctl);
+		regs->cntr |= SPRITE_SOURCE_KEY;
 
+	ivb_commit_plane(plane, plane->fb);
 	POSTING_READ(SPRKEYMSK(intel_plane->pipe));
 
-	return ret;
+	return 0;
 }
 
 static void
 ivb_get_colorkey(struct drm_plane *plane, struct drm_intel_sprite_colorkey *key)
 {
-	struct drm_device *dev = plane->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_plane *intel_plane;
-	u32 sprctl;
-
-	intel_plane = to_intel_plane(plane);
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	const struct intel_plane_regs *regs = &intel_plane->regs;
 
-	key->min_value = I915_READ(SPRKEYVAL(intel_plane->pipe));
-	key->max_value = I915_READ(SPRKEYMAX(intel_plane->pipe));
-	key->channel_mask = I915_READ(SPRKEYMSK(intel_plane->pipe));
+	key->min_value = regs->keyval;
+	key->max_value = regs->keymaxval;
+	key->channel_mask = regs->keymsk;
 	key->flags = 0;
 
-	sprctl = I915_READ(SPRCTL(intel_plane->pipe));
-
-	if (sprctl & SPRITE_DEST_KEY)
+	if (regs->cntr & SPRITE_DEST_KEY)
 		key->flags = I915_SET_COLORKEY_DESTINATION;
-	else if (sprctl & SPRITE_SOURCE_KEY)
+	else if (regs->cntr & SPRITE_SOURCE_KEY)
 		key->flags = I915_SET_COLORKEY_SOURCE;
 	else
 		key->flags = I915_SET_COLORKEY_NONE;
 }
 
 static void
-ilk_update_plane(struct drm_plane *plane,
-		 struct drm_framebuffer *fb,
-		 const struct intel_plane_coords *coords)
+ilk_calc_plane(struct drm_plane *plane,
+	       struct drm_framebuffer *fb,
+	       const struct intel_plane_coords *coords)
 {
 	struct drm_device *dev = plane->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	const struct drm_i915_gem_object *obj = to_intel_framebuffer(fb)->obj;
 	int crtc_x = coords->crtc_x;
@@ -383,46 +414,48 @@ ilk_update_plane(struct drm_plane *plane,
 	uint32_t y = coords->src_y;
 	uint32_t src_w = coords->src_w;
 	uint32_t src_h = coords->src_h;
-	int pipe = intel_plane->pipe;
-	u32 dvscntr, dvsscale;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
-
-	dvscntr = I915_READ(DVSCNTR(pipe));
+	struct intel_plane_regs *regs = &intel_plane->regs;
 
 	/* Mask out pixel format bits in case we change it */
-	dvscntr &= ~DVS_PIXFORMAT_MASK;
-	dvscntr &= ~DVS_RGB_ORDER_XBGR;
-	dvscntr &= ~DVS_YUV_BYTE_ORDER_MASK;
+	regs->cntr &= ~(DVS_PIXFORMAT_MASK |
+			DVS_RGB_ORDER_XBGR |
+			DVS_YUV_BYTE_ORDER_MASK |
+			DVS_TRICKLE_FEED_DISABLE |
+			DVS_TILED |
+			DVS_ENABLE);
 
 	switch (fb->pixel_format) {
 	case DRM_FORMAT_XBGR8888:
-		dvscntr |= DVS_FORMAT_RGBX888 | DVS_RGB_ORDER_XBGR;
+		regs->cntr |= DVS_FORMAT_RGBX888 | DVS_RGB_ORDER_XBGR;
 		break;
 	case DRM_FORMAT_XRGB8888:
-		dvscntr |= DVS_FORMAT_RGBX888;
+		regs->cntr |= DVS_FORMAT_RGBX888;
 		break;
 	case DRM_FORMAT_YUYV:
-		dvscntr |= DVS_FORMAT_YUV422 | DVS_YUV_ORDER_YUYV;
+		regs->cntr |= DVS_FORMAT_YUV422 | DVS_YUV_ORDER_YUYV;
 		break;
 	case DRM_FORMAT_YVYU:
-		dvscntr |= DVS_FORMAT_YUV422 | DVS_YUV_ORDER_YVYU;
+		regs->cntr |= DVS_FORMAT_YUV422 | DVS_YUV_ORDER_YVYU;
 		break;
 	case DRM_FORMAT_UYVY:
-		dvscntr |= DVS_FORMAT_YUV422 | DVS_YUV_ORDER_UYVY;
+		regs->cntr |= DVS_FORMAT_YUV422 | DVS_YUV_ORDER_UYVY;
 		break;
 	case DRM_FORMAT_VYUY:
-		dvscntr |= DVS_FORMAT_YUV422 | DVS_YUV_ORDER_VYUY;
+		regs->cntr |= DVS_FORMAT_YUV422 | DVS_YUV_ORDER_VYUY;
 		break;
 	default:
 		BUG();
 	}
 
 	if (obj->tiling_mode != I915_TILING_NONE)
-		dvscntr |= DVS_TILED;
+		regs->cntr |= DVS_TILED;
 
 	if (IS_GEN6(dev))
-		dvscntr |= DVS_TRICKLE_FEED_DISABLE; /* must disable */
-	dvscntr |= DVS_ENABLE;
+		regs->cntr |= DVS_TRICKLE_FEED_DISABLE; /* must disable */
+
+	if (coords->visible)
+		regs->cntr |= DVS_ENABLE;
 
 	/* Sizes are 0 based */
 	src_w--;
@@ -430,29 +463,64 @@ ilk_update_plane(struct drm_plane *plane,
 	crtc_w--;
 	crtc_h--;
 
-	intel_update_sprite_watermarks(dev, pipe, crtc_w, pixel_size);
+	if (coords->visible && (IS_GEN5(dev) || crtc_w != src_w || crtc_h != src_h))
+		regs->scale = DVS_SCALE_ENABLE | (src_w << 16) | src_h;
+	else
+		regs->scale = 0;
 
-	dvsscale = 0;
-	if (IS_GEN5(dev) || crtc_w != src_w || crtc_h != src_h)
-		dvsscale = DVS_SCALE_ENABLE | (src_w << 16) | src_h;
+	regs->stride = fb->pitches[0];
+	regs->pos = (crtc_y << 16) | crtc_x;
 
-	I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
-	I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
 	if (obj->tiling_mode != I915_TILING_NONE) {
 		y += fb->offsets[0] / fb->pitches[0];
 		x += fb->offsets[0] % fb->pitches[0] / pixel_size;
 
-		I915_WRITE(DVSTILEOFF(pipe), (y << 16) | x);
-	} else {
-		unsigned long offset;
+		regs->tileoff = (y << 16) | x;
+	} else
+		regs->linoff = fb->offsets[0] + y * fb->pitches[0] + x * pixel_size;
+
+	regs->size = (crtc_h << 16) | crtc_w;
+	regs->surf = I915_LO_DISPBASE(regs->surf) | obj->gtt_offset;
+}
+
+static void
+ilk_commit_plane(struct drm_plane *plane, struct drm_framebuffer *fb)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	int pipe = intel_plane->pipe;
+	int pixel_size = fb ? drm_format_plane_cpp(fb->pixel_format, 0) : 0;
+	const struct intel_plane_regs *regs = &intel_plane->regs;
+
+	/* FIXME move somewhere appropriate */
+	intel_update_sprite_watermarks(dev, pipe, regs->size & 0xffff, pixel_size);
+
+	I915_WRITE(DVSKEYVAL(pipe), regs->keyval);
+	I915_WRITE(DVSKEYMAX(pipe), regs->keymaxval);
+	I915_WRITE(DVSKEYMSK(pipe), regs->keymsk);
+	I915_WRITE(DVSSTRIDE(pipe), regs->stride);
+	I915_WRITE(DVSPOS(pipe), regs->pos);
+	I915_WRITE(DVSTILEOFF(pipe), regs->tileoff);
+	I915_WRITE(DVSLINOFF(pipe), regs->linoff);
+	I915_WRITE(DVSSIZE(pipe), regs->size);
+	I915_WRITE(DVSSCALE(pipe), regs->scale);
+	I915_WRITE(DVSCNTR(pipe), regs->cntr);
+	I915_WRITE(DVSSURF(pipe), regs->surf);
+}
+
+static void
+ilk_update_plane(struct drm_plane *plane,
+		 struct drm_framebuffer *fb,
+		 const struct intel_plane_coords *coords)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int pipe = to_intel_plane(plane)->pipe;
+
+	ilk_calc_plane(plane, fb, coords);
+	ilk_commit_plane(plane, fb);
 
-		offset = fb->offsets[0] + y * fb->pitches[0] + x * pixel_size;
-		I915_WRITE(DVSLINOFF(pipe), offset);
-	}
-	I915_WRITE(DVSSIZE(pipe), (crtc_h << 16) | crtc_w);
-	I915_WRITE(DVSSCALE(pipe), dvsscale);
-	I915_WRITE(DVSCNTR(pipe), dvscntr);
-	I915_MODIFY_DISPBASE(DVSSURF(pipe), obj->gtt_offset);
 	POSTING_READ(DVSSURF(pipe));
 }
 
@@ -463,22 +531,50 @@ ilk_disable_plane(struct drm_plane *plane)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	int pipe = intel_plane->pipe;
+	struct intel_plane_regs *regs = &intel_plane->regs;
 
-	I915_WRITE(DVSCNTR(pipe), I915_READ(DVSCNTR(pipe)) & ~DVS_ENABLE);
+	regs->cntr &= ~DVS_ENABLE;
 	/* Disable the scaler */
-	I915_WRITE(DVSSCALE(pipe), 0);
-	/* Flush double buffered register updates */
-	I915_MODIFY_DISPBASE(DVSSURF(pipe), 0);
+	regs->scale = 0;
+
+	ilk_commit_plane(plane, NULL);
 	POSTING_READ(DVSSURF(pipe));
 }
 
+
+void intel_sprite_calc(struct drm_plane *plane, struct drm_framebuffer *fb,
+		       const struct intel_plane_coords *coords)
+{
+	struct drm_device *dev = plane->dev;
+
+	if (INTEL_INFO(dev)->gen == 7)
+		ivb_calc_plane(plane, fb, coords);
+	else
+		ilk_calc_plane(plane, fb, coords);
+}
+
+void intel_sprite_prepare(struct drm_plane *plane)
+{
+}
+
+void intel_sprite_commit(struct drm_plane *plane)
+{
+	struct drm_device *dev = plane->dev;
+
+	if (INTEL_INFO(dev)->gen == 7)
+		ivb_commit_plane(plane, plane->fb);
+	else
+		ilk_commit_plane(plane, plane->fb);
+}
+
+void intel_plane_commit(struct drm_crtc *crtc);
+
 static void
 intel_enable_primary(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int reg = DSPCNTR(intel_crtc->plane);
+	struct intel_plane_regs *regs = &intel_crtc->primary_regs;
 
 	if (!intel_crtc->primary_disabled)
 		return;
@@ -486,21 +582,26 @@ intel_enable_primary(struct drm_crtc *crtc)
 	intel_crtc->primary_disabled = false;
 	intel_update_fbc(dev);
 
-	I915_WRITE(reg, I915_READ(reg) | DISPLAY_PLANE_ENABLE);
+	regs->cntr |= DISPLAY_PLANE_ENABLE;
+
+	// FIXME
+	intel_plane_commit(crtc);
 }
 
 static void
 intel_disable_primary(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int reg = DSPCNTR(intel_crtc->plane);
+	struct intel_plane_regs *regs = &intel_crtc->primary_regs;
 
 	if (intel_crtc->primary_disabled)
 		return;
 
-	I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE);
+	regs->cntr &= ~DISPLAY_PLANE_ENABLE;
+
+	// FIXME
+	intel_plane_commit(crtc);
 
 	intel_crtc->primary_disabled = true;
 	intel_update_fbc(dev);
@@ -512,49 +613,39 @@ ilk_update_colorkey(struct drm_plane *plane,
 {
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_plane *intel_plane;
-	u32 dvscntr;
-	int ret = 0;
-
-	intel_plane = to_intel_plane(plane);
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_plane_regs *regs = &intel_plane->regs;
 
-	I915_WRITE(DVSKEYVAL(intel_plane->pipe), key->min_value);
-	I915_WRITE(DVSKEYMAX(intel_plane->pipe), key->max_value);
-	I915_WRITE(DVSKEYMSK(intel_plane->pipe), key->channel_mask);
+	regs->keyval = key->min_value;
+	regs->keymaxval = key->max_value;
+	regs->keymsk = key->channel_mask;
 
-	dvscntr = I915_READ(DVSCNTR(intel_plane->pipe));
-	dvscntr &= ~(DVS_SOURCE_KEY | DVS_DEST_KEY);
+	regs->cntr &= ~(DVS_SOURCE_KEY | DVS_DEST_KEY);
 	if (key->flags & I915_SET_COLORKEY_DESTINATION)
-		dvscntr |= DVS_DEST_KEY;
+		regs->cntr |= DVS_DEST_KEY;
 	else if (key->flags & I915_SET_COLORKEY_SOURCE)
-		dvscntr |= DVS_SOURCE_KEY;
-	I915_WRITE(DVSCNTR(intel_plane->pipe), dvscntr);
+		regs->cntr |= DVS_SOURCE_KEY;
 
+	ilk_commit_plane(plane, plane->fb);
 	POSTING_READ(DVSKEYMSK(intel_plane->pipe));
 
-	return ret;
+	return 0;
 }
 
 static void
 ilk_get_colorkey(struct drm_plane *plane, struct drm_intel_sprite_colorkey *key)
 {
-	struct drm_device *dev = plane->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_plane *intel_plane;
-	u32 dvscntr;
-
-	intel_plane = to_intel_plane(plane);
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	const struct intel_plane_regs *regs = &intel_plane->regs;
 
-	key->min_value = I915_READ(DVSKEYVAL(intel_plane->pipe));
-	key->max_value = I915_READ(DVSKEYMAX(intel_plane->pipe));
-	key->channel_mask = I915_READ(DVSKEYMSK(intel_plane->pipe));
+	key->min_value = regs->keyval;
+	key->max_value = regs->keymaxval;
+	key->channel_mask = regs->keymsk;
 	key->flags = 0;
 
-	dvscntr = I915_READ(DVSCNTR(intel_plane->pipe));
-
-	if (dvscntr & DVS_DEST_KEY)
+	if (regs->cntr & DVS_DEST_KEY)
 		key->flags = I915_SET_COLORKEY_DESTINATION;
-	else if (dvscntr & DVS_SOURCE_KEY)
+	else if (regs->cntr & DVS_SOURCE_KEY)
 		key->flags = I915_SET_COLORKEY_SOURCE;
 	else
 		key->flags = I915_SET_COLORKEY_NONE;
@@ -803,6 +894,7 @@ static uint32_t snb_plane_formats[] = {
 int
 intel_plane_init(struct drm_device *dev, enum pipe pipe)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane;
 	unsigned long possible_crtcs;
 	const uint32_t *plane_formats;
@@ -825,6 +917,9 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe)
 		intel_plane->update_colorkey = ilk_update_colorkey;
 		intel_plane->get_colorkey = ilk_get_colorkey;
 
+		intel_plane->regs.cntr = I915_READ(DVSCNTR(pipe));
+		intel_plane->regs.surf = I915_READ(DVSSURF(pipe));
+
 		if (IS_GEN6(dev)) {
 			plane_formats = snb_plane_formats;
 			num_plane_formats = ARRAY_SIZE(snb_plane_formats);
@@ -841,6 +936,9 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe)
 		intel_plane->update_colorkey = ivb_update_colorkey;
 		intel_plane->get_colorkey = ivb_get_colorkey;
 
+		intel_plane->regs.cntr = I915_READ(SPRCTL(pipe));
+		intel_plane->regs.surf = I915_READ(SPRSURF(pipe));
+
 		plane_formats = snb_plane_formats;
 		num_plane_formats = ARRAY_SIZE(snb_plane_formats);
 		break;
-- 
1.7.8.6

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH 4/4] drm: i915: Atomic pageflip WIP

[Chris Wilson]

On Wed, 12 Sep 2012 18:47:07 +0300, ville.syrjala@linux.intel.com wrote:
> +static void intel_flip_finish(struct drm_flip *flip)
> +{
> +	struct intel_flip *intel_flip =
> +		container_of(flip, struct intel_flip, base);
> +	struct drm_device *dev = intel_flip->crtc->dev;
> +
> +	if (intel_flip->old_bo) {
> +		mutex_lock(&dev->struct_mutex);
> +
> +		intel_finish_fb(intel_flip->old_bo);

So if I understand correctly, this code is called after the flip is
already complete?

The intel_finish_fb() exists to flush pending batches and flips on the
current fb, prior to changing the scanout registers. (There is a
hardware dependency such that the GPU may be executing a command that
required the current modesetting.) In the case of flip completion, all
of those dependencies have already been retired and so the finish should
be a no-op. And so it should no be required, nor the changes to
intel_finish_fb (which should have included a change in the name to
indicate that is now taking the fb_obj).

> +		intel_unpin_fb_obj(intel_flip->old_bo);
> +
> +		mutex_unlock(&dev->struct_mutex);
> +	}
> +
> +}
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [RFC][PATCH 4/4] drm: i915: Atomic pageflip WIP

[Ville Syrjälä]

On Fri, Sep 14, 2012 at 02:57:26PM +0100, Chris Wilson wrote:
> On Wed, 12 Sep 2012 18:47:07 +0300, ville.syrjala@linux.intel.com wrote:
> > +static void intel_flip_finish(struct drm_flip *flip)
> > +{
> > +	struct intel_flip *intel_flip =
> > +		container_of(flip, struct intel_flip, base);
> > +	struct drm_device *dev = intel_flip->crtc->dev;
> > +
> > +	if (intel_flip->old_bo) {
> > +		mutex_lock(&dev->struct_mutex);
> > +
> > +		intel_finish_fb(intel_flip->old_bo);
> 
> So if I understand correctly, this code is called after the flip is
> already complete?

Yes.

> The intel_finish_fb() exists to flush pending batches and flips on the
> current fb, prior to changing the scanout registers. (There is a
> hardware dependency such that the GPU may be executing a command that
> required the current modesetting.) In the case of flip completion, all
> of those dependencies have already been retired and so the finish should
> be a no-op. And so it should no be required, nor the changes to
> intel_finish_fb (which should have included a change in the name to
> indicate that is now taking the fb_obj).

Actually I'm not quite sure where this intel_finish_fb() call originated.
Based on the name it didn't make sense to me, but I left it there for
now. Hmm. OK it came from one patch from Imre while I was on vacation.
I suppose he got it from intel_pipe_set_base() which does call
intel_finish_fb() on the old fb. Why does it do that?

I've not really dug into the GPU synchronization side and pin/fence stuff,
so it's no surprise my code is a bit fscked up in those areas.

-- 
Ville Syrjälä
Intel OTC

Re: [RFC][PATCH 4/4] drm: i915: Atomic pageflip WIP

[Chris Wilson]

[-- Attachment #1: Type: text/plain, Size: 2134 bytes --]

On Fri, 14 Sep 2012 17:21:30 +0300, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Sep 14, 2012 at 02:57:26PM +0100, Chris Wilson wrote:
> > On Wed, 12 Sep 2012 18:47:07 +0300, ville.syrjala@linux.intel.com wrote:
> > > +static void intel_flip_finish(struct drm_flip *flip)
> > > +{
> > > +	struct intel_flip *intel_flip =
> > > +		container_of(flip, struct intel_flip, base);
> > > +	struct drm_device *dev = intel_flip->crtc->dev;
> > > +
> > > +	if (intel_flip->old_bo) {
> > > +		mutex_lock(&dev->struct_mutex);
> > > +
> > > +		intel_finish_fb(intel_flip->old_bo);
> > 
> > So if I understand correctly, this code is called after the flip is
> > already complete?
> 
> Yes.
> 
> > The intel_finish_fb() exists to flush pending batches and flips on the
> > current fb, prior to changing the scanout registers. (There is a
> > hardware dependency such that the GPU may be executing a command that
> > required the current modesetting.) In the case of flip completion, all
> > of those dependencies have already been retired and so the finish should
> > be a no-op. And so it should no be required, nor the changes to
> > intel_finish_fb (which should have included a change in the name to
> > indicate that is now taking the fb_obj).
> 
> Actually I'm not quite sure where this intel_finish_fb() call originated.
> Based on the name it didn't make sense to me, but I left it there for
> now. Hmm. OK it came from one patch from Imre while I was on vacation.
> I suppose he got it from intel_pipe_set_base() which does call
> intel_finish_fb() on the old fb. Why does it do that?

It all boils down to the modeset being asynchronous to the GPU
processing the command stream. So we may be currently processing a batch
that is waiting on the pipe to go past a particular scanline, and if the
modesetting were to disable that pipe, or to change its size, then we
risk the WAIT_FOR_EVENT never completing - leading to hangcheck
detecting the frozen display and an angry user.

The other aspect is to synchronize the modeset with any outstanding
pageflips.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH 4/4] drm: i915: Atomic pageflip WIP

[Ville Syrjälä]

On Fri, Sep 14, 2012 at 03:27:05PM +0100, Chris Wilson wrote:
> On Fri, 14 Sep 2012 17:21:30 +0300, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Sep 14, 2012 at 02:57:26PM +0100, Chris Wilson wrote:
> > > On Wed, 12 Sep 2012 18:47:07 +0300, ville.syrjala@linux.intel.com wrote:
> > > > +static void intel_flip_finish(struct drm_flip *flip)
> > > > +{
> > > > +	struct intel_flip *intel_flip =
> > > > +		container_of(flip, struct intel_flip, base);
> > > > +	struct drm_device *dev = intel_flip->crtc->dev;
> > > > +
> > > > +	if (intel_flip->old_bo) {
> > > > +		mutex_lock(&dev->struct_mutex);
> > > > +
> > > > +		intel_finish_fb(intel_flip->old_bo);
> > > 
> > > So if I understand correctly, this code is called after the flip is
> > > already complete?
> > 
> > Yes.
> > 
> > > The intel_finish_fb() exists to flush pending batches and flips on the
> > > current fb, prior to changing the scanout registers. (There is a
> > > hardware dependency such that the GPU may be executing a command that
> > > required the current modesetting.) In the case of flip completion, all
> > > of those dependencies have already been retired and so the finish should
> > > be a no-op. And so it should no be required, nor the changes to
> > > intel_finish_fb (which should have included a change in the name to
> > > indicate that is now taking the fb_obj).
> > 
> > Actually I'm not quite sure where this intel_finish_fb() call originated.
> > Based on the name it didn't make sense to me, but I left it there for
> > now. Hmm. OK it came from one patch from Imre while I was on vacation.
> > I suppose he got it from intel_pipe_set_base() which does call
> > intel_finish_fb() on the old fb. Why does it do that?
> 
> It all boils down to the modeset being asynchronous to the GPU
> processing the command stream. So we may be currently processing a batch
> that is waiting on the pipe to go past a particular scanline, and if the
> modesetting were to disable that pipe, or to change its size, then we
> risk the WAIT_FOR_EVENT never completing - leading to hangcheck
> detecting the frozen display and an angry user.

intel_pipe_set_base() won't disable the pipe or change the size,
it'll just flip the primary plane. So that doesn't quite explain
why the call is there, as opposed to being called just from the
full modeset path.

Also wouldn't any batch buffer with WAIT_FOR_EVENT be in risk of
stalling, not just ones related to the old fb?

> The other aspect is to synchronize the modeset with any outstanding
> pageflips.

Right, that does make sense. But doing it from a function called
intel_finish_fb() is a bit confusing, as the condition really
shouldn't depend on any specific fb object. But I suppose this is
just a result of the "only one outstanding flip" policy.


BTW regarding this WAIT_FOR_EVENT thing. I got the impression that
the scanline window wait doesn't work on recent hardware generations
any more. Is that true? I was thinking that perhaps I could use it
along with the load register command to perform the flips through
the command queue.

-- 
Ville Syrjälä
Intel OTC

Re: [RFC][PATCH 4/4] drm: i915: Atomic pageflip WIP

[Daniel Vetter]

On Fri, Sep 14, 2012 at 5:30 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> intel_pipe_set_base() won't disable the pipe or change the size,
> it'll just flip the primary plane. So that doesn't quite explain
> why the call is there, as opposed to being called just from the
> full modeset path.

intel_pipe_set_base is also called in the modeset case, i.e. when we
could potentially change the height of the mode. And if we wait on a
large enough scanline which doesn't exist in the new mode this would
hang.

The other callsite of finish_fb is from intel_crtc_disable.

btw, some slight digging around with git blame gives you in both cases
the commits and comments that explain this all ;-)

Cheers, Daniel
-- 
Daniel Vetter
daniel.vetter@ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch

Re: [RFC][PATCH 4/4] drm: i915: Atomic pageflip WIP

[Ville Syrjälä]

On Fri, Sep 14, 2012 at 05:39:30PM +0200, Daniel Vetter wrote:
> On Fri, Sep 14, 2012 at 5:30 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > intel_pipe_set_base() won't disable the pipe or change the size,
> > it'll just flip the primary plane. So that doesn't quite explain
> > why the call is there, as opposed to being called just from the
> > full modeset path.
> 
> intel_pipe_set_base is also called in the modeset case, i.e. when we
> could potentially change the height of the mode. And if we wait on a
> large enough scanline which doesn't exist in the new mode this would
> hang.

Yes, I know it's called in both cases. But my point is that there
doesn't seem to be any reason to call it in the pure set_base case.

> The other callsite of finish_fb is from intel_crtc_disable.

Yep. There it does make sense to me.

-- 
Ville Syrjälä
Intel OTC

Re: [RFC][PATCH 4/4] drm: i915: Atomic pageflip WIP

[Chris Wilson]

[-- Attachment #1: Type: text/plain, Size: 3982 bytes --]

On Fri, 14 Sep 2012 18:30:21 +0300, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Sep 14, 2012 at 03:27:05PM +0100, Chris Wilson wrote:
> > On Fri, 14 Sep 2012 17:21:30 +0300, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > > On Fri, Sep 14, 2012 at 02:57:26PM +0100, Chris Wilson wrote:
> > > > On Wed, 12 Sep 2012 18:47:07 +0300, ville.syrjala@linux.intel.com wrote:
> > > > > +static void intel_flip_finish(struct drm_flip *flip)
> > > > > +{
> > > > > +	struct intel_flip *intel_flip =
> > > > > +		container_of(flip, struct intel_flip, base);
> > > > > +	struct drm_device *dev = intel_flip->crtc->dev;
> > > > > +
> > > > > +	if (intel_flip->old_bo) {
> > > > > +		mutex_lock(&dev->struct_mutex);
> > > > > +
> > > > > +		intel_finish_fb(intel_flip->old_bo);
> > > > 
> > > > So if I understand correctly, this code is called after the flip is
> > > > already complete?
> > > 
> > > Yes.
> > > 
> > > > The intel_finish_fb() exists to flush pending batches and flips on the
> > > > current fb, prior to changing the scanout registers. (There is a
> > > > hardware dependency such that the GPU may be executing a command that
> > > > required the current modesetting.) In the case of flip completion, all
> > > > of those dependencies have already been retired and so the finish should
> > > > be a no-op. And so it should no be required, nor the changes to
> > > > intel_finish_fb (which should have included a change in the name to
> > > > indicate that is now taking the fb_obj).
> > > 
> > > Actually I'm not quite sure where this intel_finish_fb() call originated.
> > > Based on the name it didn't make sense to me, but I left it there for
> > > now. Hmm. OK it came from one patch from Imre while I was on vacation.
> > > I suppose he got it from intel_pipe_set_base() which does call
> > > intel_finish_fb() on the old fb. Why does it do that?
> > 
> > It all boils down to the modeset being asynchronous to the GPU
> > processing the command stream. So we may be currently processing a batch
> > that is waiting on the pipe to go past a particular scanline, and if the
> > modesetting were to disable that pipe, or to change its size, then we
> > risk the WAIT_FOR_EVENT never completing - leading to hangcheck
> > detecting the frozen display and an angry user.
> 
> intel_pipe_set_base() won't disable the pipe or change the size,
> it'll just flip the primary plane. So that doesn't quite explain
> why the call is there, as opposed to being called just from the
> full modeset path.

Hmm, at the time it was a convenient point. Now, it is clearly called too
late in the modeset sequence. Daniel, fix please. :)

> Also wouldn't any batch buffer with WAIT_FOR_EVENT be in risk of
> stalling, not just ones related to the old fb?
> 
> > The other aspect is to synchronize the modeset with any outstanding
> > pageflips.
> 
> Right, that does make sense. But doing it from a function called
> intel_finish_fb() is a bit confusing, as the condition really
> shouldn't depend on any specific fb object. But I suppose this is
> just a result of the "only one outstanding flip" policy.

Again, a nice convenient point, calling it an intel_crtc_wait_*() would
probably help (after fixing the ordering).

> BTW regarding this WAIT_FOR_EVENT thing. I got the impression that
> the scanline window wait doesn't work on recent hardware generations
> any more. Is that true? I was thinking that perhaps I could use it
> along with the load register command to perform the flips through
> the command queue.

That impression is pretty accurate. There is a suggestion that some form
of scanline wait was restored for IVB, but driving it seems pretty hit
and miss. Atomic flipping should all be possible with MI_DISPLAY_FLIP,
so presumably you are mostly thinking about atomic modeset? Is the
presumption that it will be an infrequent request and so better to keep
as simple as possible?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH 4/4] drm: i915: Atomic pageflip WIP

[Ville Syrjälä]

On Fri, Sep 14, 2012 at 04:56:00PM +0100, Chris Wilson wrote:
> On Fri, 14 Sep 2012 18:30:21 +0300, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Sep 14, 2012 at 03:27:05PM +0100, Chris Wilson wrote:
> > > On Fri, 14 Sep 2012 17:21:30 +0300, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > > > On Fri, Sep 14, 2012 at 02:57:26PM +0100, Chris Wilson wrote:
> > > > > On Wed, 12 Sep 2012 18:47:07 +0300, ville.syrjala@linux.intel.com wrote:
> > > > > > +static void intel_flip_finish(struct drm_flip *flip)
> > > > > > +{
> > > > > > +	struct intel_flip *intel_flip =
> > > > > > +		container_of(flip, struct intel_flip, base);
> > > > > > +	struct drm_device *dev = intel_flip->crtc->dev;
> > > > > > +
> > > > > > +	if (intel_flip->old_bo) {
> > > > > > +		mutex_lock(&dev->struct_mutex);
> > > > > > +
> > > > > > +		intel_finish_fb(intel_flip->old_bo);
> > > > > 
> > > > > So if I understand correctly, this code is called after the flip is
> > > > > already complete?
> > > > 
> > > > Yes.
> > > > 
> > > > > The intel_finish_fb() exists to flush pending batches and flips on the
> > > > > current fb, prior to changing the scanout registers. (There is a
> > > > > hardware dependency such that the GPU may be executing a command that
> > > > > required the current modesetting.) In the case of flip completion, all
> > > > > of those dependencies have already been retired and so the finish should
> > > > > be a no-op. And so it should no be required, nor the changes to
> > > > > intel_finish_fb (which should have included a change in the name to
> > > > > indicate that is now taking the fb_obj).
> > > > 
> > > > Actually I'm not quite sure where this intel_finish_fb() call originated.
> > > > Based on the name it didn't make sense to me, but I left it there for
> > > > now. Hmm. OK it came from one patch from Imre while I was on vacation.
> > > > I suppose he got it from intel_pipe_set_base() which does call
> > > > intel_finish_fb() on the old fb. Why does it do that?
> > > 
> > > It all boils down to the modeset being asynchronous to the GPU
> > > processing the command stream. So we may be currently processing a batch
> > > that is waiting on the pipe to go past a particular scanline, and if the
> > > modesetting were to disable that pipe, or to change its size, then we
> > > risk the WAIT_FOR_EVENT never completing - leading to hangcheck
> > > detecting the frozen display and an angry user.
> > 
> > intel_pipe_set_base() won't disable the pipe or change the size,
> > it'll just flip the primary plane. So that doesn't quite explain
> > why the call is there, as opposed to being called just from the
> > full modeset path.
> 
> Hmm, at the time it was a convenient point. Now, it is clearly called too
> late in the modeset sequence. Daniel, fix please. :)
> 
> > Also wouldn't any batch buffer with WAIT_FOR_EVENT be in risk of
> > stalling, not just ones related to the old fb?
> > 
> > > The other aspect is to synchronize the modeset with any outstanding
> > > pageflips.
> > 
> > Right, that does make sense. But doing it from a function called
> > intel_finish_fb() is a bit confusing, as the condition really
> > shouldn't depend on any specific fb object. But I suppose this is
> > just a result of the "only one outstanding flip" policy.
> 
> Again, a nice convenient point, calling it an intel_crtc_wait_*() would
> probably help (after fixing the ordering).
> 
> > BTW regarding this WAIT_FOR_EVENT thing. I got the impression that
> > the scanline window wait doesn't work on recent hardware generations
> > any more. Is that true? I was thinking that perhaps I could use it
> > along with the load register command to perform the flips through
> > the command queue.
> 
> That impression is pretty accurate. There is a suggestion that some form
> of scanline wait was restored for IVB, but driving it seems pretty hit
> and miss. Atomic flipping should all be possible with MI_DISPLAY_FLIP,
> so presumably you are mostly thinking about atomic modeset? Is the
> presumption that it will be an infrequent request and so better to keep
> as simple as possible?

MI_DISPLAY_FLIP is _very_ limited. You can't really do anything
interesting with. Sure it's enough to flip the buffers of some game
or whatever, but it's practically useless for hardware composition
type stuff.

-- 
Ville Syrjälä
Intel OTC