[PATCH 00/16] PSR rework and accurate frontbuffer tracking

[PATCH 00/16] PSR rework and accurate frontbuffer tracking

[Daniel Vetter]

Hi all,

Lots of changes since the first time around:
- Piles of bugfixes all over to make the psr stuff actually work.
- Bugfixes in the frontbuffer tracking (I've undersized one bitfield by
  accident, so didn't work on more than one pipe).
- Polished function names and interface semantics for the frontbuffer tracking
  events.
- Some patch splitting and reorg.
- Comments for tricky stuff.
- Improved debugfs data, dumping the new fields.
- Polish all over.

Rodrigo also tested this a bit and we've noticed that the current psr test gets
a few things wrong, so needs improvement. Testing BYT psr is also still pending,
I'd like to have that confirmation before merging.

But otherwise this seems to actually work somewhat.

Another thing I'm thinking about is extracting the frontbuffer handling into a
new file (intel_frontbuffer.c or anyone have a better name) and integrate the
new kerneldoc plus add a short overview section. We could even move some of the
power saving code (psr fits better in intel_dp.c I think, but fbc maybe) in
there, together with the documentation.

Comments & feedback highly welcome.

Rodrigo is signed up to review the entire pile, but for the frontbuffer tracking
I really want Chris&Ville to chime in and ack it, too.

Cheers, Daniel

Daniel Vetter (16):
  drm/i915: Drop unecessary complexity from psr_inactivate
  drm/i915: Ditch intel_edp_psr_update
  drm/i915: Run psr_setup unconditionally
  drm/i915: Drop schedule_back from psr_exit
  drm/i915: Add a FIXME about drrs/psr interactions
  drm/i915: Track the psr dp connector in dev_priv->psr.enabled
  drm/i915: Don't try to disable psr harder from the work item
  drm/i915: Lock down psr sw/hw state tracking
  drm/i915: More checks for psr.enabled
  drm/i915: Add locking to psr code
  drm/i915: Introduce accurate frontbuffer tracking
  drm/i915: Use new frontbuffer bits to increase pll clock
  drm/i915: Properly track domain of the fbcon fb
  drm/i915: Track frontbuffer invalidation/flushing
  drm/i915: Fix up PSR frontbuffer tracking
  drm/i915: Improve PSR debugfs output

 drivers/gpu/drm/i915/i915_debugfs.c        |   8 +-
 drivers/gpu/drm/i915/i915_drv.h            |  48 ++++-
 drivers/gpu/drm/i915/i915_gem.c            |  37 +++-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   6 +-
 drivers/gpu/drm/i915/intel_display.c       | 285 +++++++++++++++++++++++------
 drivers/gpu/drm/i915/intel_dp.c            | 210 +++++++++++----------
 drivers/gpu/drm/i915/intel_drv.h           |  35 +++-
 drivers/gpu/drm/i915/intel_fbdev.c         |  21 ++-
 drivers/gpu/drm/i915/intel_overlay.c       |  14 ++
 drivers/gpu/drm/i915/intel_sprite.c        |  11 +-
 10 files changed, 510 insertions(+), 165 deletions(-)

-- 
2.0.0

[PATCH 01/16] drm/i915: Drop unecessary complexity from psr_inactivate

[Daniel Vetter]

It's not needed and further more will get in the way of a sane
locking scheme - psr_exit _can't_ take modeset locks due to lock
inversion, and at least once dp mst hits the connector list
is no longer static.

But since we track all state in dev_priv->psr there is no need
at all.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_dp.c | 24 +++---------------------
 1 file changed, 3 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 912e9c4de58f..74e194d66bba 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1910,29 +1910,11 @@ static void intel_edp_psr_work(struct work_struct *work)
 static void intel_edp_psr_inactivate(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_connector *connector;
-	struct intel_encoder *encoder;
-	struct intel_crtc *intel_crtc;
-	struct intel_dp *intel_dp = NULL;
-
-	list_for_each_entry(connector, &dev->mode_config.connector_list,
-			    base.head) {
 
-		if (connector->base.dpms != DRM_MODE_DPMS_ON)
-			continue;
-
-		encoder = to_intel_encoder(connector->base.encoder);
-		if (encoder->type == INTEL_OUTPUT_EDP) {
+	dev_priv->psr.active = false;
 
-			intel_dp = enc_to_intel_dp(&encoder->base);
-			intel_crtc = to_intel_crtc(encoder->base.crtc);
-
-			dev_priv->psr.active = false;
-
-			I915_WRITE(EDP_PSR_CTL(dev), I915_READ(EDP_PSR_CTL(dev))
-				   & ~EDP_PSR_ENABLE);
-		}
-	}
+	I915_WRITE(EDP_PSR_CTL(dev), I915_READ(EDP_PSR_CTL(dev))
+		   & ~EDP_PSR_ENABLE);
 }
 
 void intel_edp_psr_exit(struct drm_device *dev, bool schedule_back)
-- 
2.0.0

[PATCH 02/16] drm/i915: Ditch intel_edp_psr_update

[Daniel Vetter]

We have _enable/_disable interfaces now for the modeset sequence and
intel_edp_psr_exit for workarounds.

The callsites in intel_display.c are all redundant with the modeset
sequence enable/disable calls in intel_ddi.c. The one in
intel_sprite.c is real and needs to be switched to psr_exit.

If this breaks anything then we need to augment the enable/disable
functions accordingly.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c |  5 -----
 drivers/gpu/drm/i915/intel_dp.c      | 13 -------------
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 drivers/gpu/drm/i915/intel_sprite.c  |  2 +-
 4 files changed, 1 insertion(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5e8e7113b453..88d9d815cd20 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2763,7 +2763,6 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 
 	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
-	intel_edp_psr_update(dev);
 	mutex_unlock(&dev->struct_mutex);
 
 	return 0;
@@ -3943,7 +3942,6 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
 
 	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
-	intel_edp_psr_update(dev);
 	mutex_unlock(&dev->struct_mutex);
 }
 
@@ -4236,7 +4234,6 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 
 	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
-	intel_edp_psr_update(dev);
 	mutex_unlock(&dev->struct_mutex);
 }
 
@@ -4284,7 +4281,6 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 
 	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
-	intel_edp_psr_update(dev);
 	mutex_unlock(&dev->struct_mutex);
 }
 
@@ -4836,7 +4832,6 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 
 	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
-	intel_edp_psr_update(dev);
 	mutex_unlock(&dev->struct_mutex);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 74e194d66bba..b373b895fe48 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1875,19 +1875,6 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
 	dev_priv->psr.enabled = false;
 }
 
-void intel_edp_psr_update(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	if (!HAS_PSR(dev))
-		return;
-
-	if (!dev_priv->psr.setup_done)
-		return;
-
-	intel_edp_psr_exit(dev, true);
-}
-
 static void intel_edp_psr_work(struct work_struct *work)
 {
 	struct drm_i915_private *dev_priv =
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ab5962b80f48..e92354c9bb44 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -834,7 +834,6 @@ void intel_edp_panel_on(struct intel_dp *intel_dp);
 void intel_edp_panel_off(struct intel_dp *intel_dp);
 void intel_edp_psr_enable(struct intel_dp *intel_dp);
 void intel_edp_psr_disable(struct intel_dp *intel_dp);
-void intel_edp_psr_update(struct drm_device *dev);
 void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate);
 void intel_edp_psr_exit(struct drm_device *dev, bool schedule_back);
 void intel_edp_psr_init(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 404335d53a89..2a211c64ec8d 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1051,7 +1051,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		mutex_unlock(&dev->struct_mutex);
 	}
 
-	intel_edp_psr_update(dev);
+	intel_edp_psr_exit(dev, true);
 
 	return 0;
 }
-- 
2.0.0

[PATCH 03/16] drm/i915: Run psr_setup unconditionally

[Daniel Vetter]

Due to runtime pm and system s/r we need to restore hw state every
time we enable a pipe again. Hence trying to avoid that is just
pointless book-keeping which Rodrigo then tried to work around by
manually adding psr_setup calls to our resume code.

Much simpler to just remove code instead.

v2: Properly bail out of psr exit if psr isn't enabled. Spotted by
Rodrigo.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h | 1 -
 drivers/gpu/drm/i915/intel_dp.c | 7 +------
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0640071e30a9..877c3e0de12e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -636,7 +636,6 @@ struct i915_drrs {
 struct i915_psr {
 	bool sink_support;
 	bool source_ok;
-	bool setup_done;
 	bool enabled;
 	bool active;
 	struct delayed_work work;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b373b895fe48..420939fdb31d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1663,9 +1663,6 @@ static void intel_edp_psr_setup(struct intel_dp *intel_dp)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct edp_vsc_psr psr_vsc;
 
-	if (dev_priv->psr.setup_done)
-		return;
-
 	/* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */
 	memset(&psr_vsc, 0, sizeof(psr_vsc));
 	psr_vsc.sdp_header.HB0 = 0;
@@ -1677,8 +1674,6 @@ static void intel_edp_psr_setup(struct intel_dp *intel_dp)
 	/* Avoid continuous PSR exit by masking memup and hpd */
 	I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP |
 		   EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);
-
-	dev_priv->psr.setup_done = true;
 }
 
 static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
@@ -1911,7 +1906,7 @@ void intel_edp_psr_exit(struct drm_device *dev, bool schedule_back)
 	if (!HAS_PSR(dev))
 		return;
 
-	if (!dev_priv->psr.setup_done)
+	if (!dev_priv->psr.enabled)
 		return;
 
 	cancel_delayed_work_sync(&dev_priv->psr.work);
-- 
2.0.0

[PATCH 04/16] drm/i915: Drop schedule_back from psr_exit

[Daniel Vetter]

It doesn't make sense to never again schedule the work, since by the
time we might want to re-enable psr the world might have changed and
we can do it again.

The only exception is when we shut down the pipe, but that's an
entirely different thing and needs to be handled in psr_disable.

Note that later patch will again split psr_exit into psr_invalidate
and psr_flush. But the split is different and this simplification
helps with the transition.

v2: Improve the commit message a bit.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c      | 6 +++---
 drivers/gpu/drm/i915/intel_display.c | 4 ++--
 drivers/gpu/drm/i915/intel_dp.c      | 7 +++----
 drivers/gpu/drm/i915/intel_drv.h     | 2 +-
 drivers/gpu/drm/i915/intel_sprite.c  | 2 +-
 5 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7f643db26829..7d7d52bc922a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1395,7 +1395,7 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
 		goto unlock;
 	}
 
-	intel_edp_psr_exit(dev, true);
+	intel_edp_psr_exit(dev);
 
 	/* Try to flush the object off the GPU without holding the lock.
 	 * We will repeat the flush holding the lock in the normal manner
@@ -1442,7 +1442,7 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		return ret;
 
-	intel_edp_psr_exit(dev, true);
+	intel_edp_psr_exit(dev);
 
 	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
 	if (&obj->base == NULL) {
@@ -4233,7 +4233,7 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		return ret;
 
-	intel_edp_psr_exit(dev, true);
+	intel_edp_psr_exit(dev);
 
 	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
 	if (&obj->base == NULL) {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 88d9d815cd20..548161d9ac8a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8820,7 +8820,7 @@ void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
 	struct drm_device *dev = obj->base.dev;
 	struct drm_crtc *crtc;
 
-	intel_edp_psr_exit(dev, true);
+	intel_edp_psr_exit(dev);
 
 	if (!i915.powersave)
 		return;
@@ -9430,7 +9430,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 		return -ENOMEM;
 
 	/* Exit PSR early in page flip */
-	intel_edp_psr_exit(dev, true);
+	intel_edp_psr_exit(dev);
 
 	work->event = event;
 	work->crtc = crtc;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 420939fdb31d..0d1efdbb4636 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1899,7 +1899,7 @@ static void intel_edp_psr_inactivate(struct drm_device *dev)
 		   & ~EDP_PSR_ENABLE);
 }
 
-void intel_edp_psr_exit(struct drm_device *dev, bool schedule_back)
+void intel_edp_psr_exit(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -1914,9 +1914,8 @@ void intel_edp_psr_exit(struct drm_device *dev, bool schedule_back)
 	if (dev_priv->psr.active)
 		intel_edp_psr_inactivate(dev);
 
-	if (schedule_back)
-		schedule_delayed_work(&dev_priv->psr.work,
-				      msecs_to_jiffies(100));
+	schedule_delayed_work(&dev_priv->psr.work,
+			      msecs_to_jiffies(100));
 }
 
 void intel_edp_psr_init(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e92354c9bb44..5d20f719309a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -835,7 +835,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp);
 void intel_edp_psr_enable(struct intel_dp *intel_dp);
 void intel_edp_psr_disable(struct intel_dp *intel_dp);
 void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate);
-void intel_edp_psr_exit(struct drm_device *dev, bool schedule_back);
+void intel_edp_psr_exit(struct drm_device *dev);
 void intel_edp_psr_init(struct drm_device *dev);
 
 
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 2a211c64ec8d..9038e2ab73c8 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1051,7 +1051,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		mutex_unlock(&dev->struct_mutex);
 	}
 
-	intel_edp_psr_exit(dev, true);
+	intel_edp_psr_exit(dev);
 
 	return 0;
 }
-- 
2.0.0

[PATCH 05/16] drm/i915: Add a FIXME about drrs/psr interactions

[Daniel Vetter]

Can't review this right now due to lack of DRRS code.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Vandana Kannan <vandana.kannan@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_dp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 0d1efdbb4636..84fe5a75f8b2 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4071,6 +4071,11 @@ void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
 		return;
 	}
 
+	/*
+	 * FIXME: This needs proper synchronization with psr state. But really
+	 * hard to tell without seeing the user of this function of this code.
+	 * Check locking and ordering once that lands.
+	 */
 	if (INTEL_INFO(dev)->gen < 8 && intel_edp_is_psr_enabled(dev)) {
 		DRM_DEBUG_KMS("DRRS is disabled as PSR is enabled\n");
 		return;
-- 
2.0.0

[PATCH 06/16] drm/i915: Track the psr dp connector in dev_priv->psr.enabled

[Daniel Vetter]

Trying to fish that one out through looping is a bit a locking
nightmare. So just set it and use it in the work struct.

v2:
- Don't Oops in psr_work, spotted by Rodrigo.
- Fix compile warning.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c |  2 +-
 drivers/gpu/drm/i915/i915_drv.h     |  3 ++-
 drivers/gpu/drm/i915/intel_dp.c     | 22 +++++++++-------------
 3 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 8057fd4fc86c..4818aff9e6d6 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1973,7 +1973,7 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 
 	seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support));
 	seq_printf(m, "Source_OK: %s\n", yesno(dev_priv->psr.source_ok));
-	seq_printf(m, "Enabled: %s\n", yesno(dev_priv->psr.enabled));
+	seq_printf(m, "Enabled: %s\n", yesno((bool)dev_priv->psr.enabled));
 	seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active));
 
 	enabled = HAS_PSR(dev) &&
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 877c3e0de12e..740a0d95b89a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -633,10 +633,11 @@ struct i915_drrs {
 	struct intel_connector *connector;
 };
 
+struct intel_dp;
 struct i915_psr {
 	bool sink_support;
 	bool source_ok;
-	bool enabled;
+	struct intel_dp *enabled;
 	bool active;
 	struct delayed_work work;
 };
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 84fe5a75f8b2..00ddcaf82ae1 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1826,7 +1826,7 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
 	/* Enable PSR on the host */
 	intel_edp_psr_enable_source(intel_dp);
 
-	dev_priv->psr.enabled = true;
+	dev_priv->psr.enabled = intel_dp;
 	dev_priv->psr.active = true;
 }
 
@@ -1867,26 +1867,22 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
 		       EDP_PSR_STATUS_STATE_MASK) == 0, 2000, 10))
 		DRM_ERROR("Timed out waiting for PSR Idle State\n");
 
-	dev_priv->psr.enabled = false;
+	dev_priv->psr.enabled = NULL;
 }
 
 static void intel_edp_psr_work(struct work_struct *work)
 {
 	struct drm_i915_private *dev_priv =
 		container_of(work, typeof(*dev_priv), psr.work.work);
-	struct drm_device *dev = dev_priv->dev;
-	struct intel_encoder *encoder;
-	struct intel_dp *intel_dp = NULL;
+	struct intel_dp *intel_dp = dev_priv->psr.enabled;
 
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)
-		if (encoder->type == INTEL_OUTPUT_EDP) {
-			intel_dp = enc_to_intel_dp(&encoder->base);
+	if (!intel_dp)
+		return;
 
-			if (!intel_edp_psr_match_conditions(intel_dp))
-				intel_edp_psr_disable(intel_dp);
-			else
-				intel_edp_psr_do_enable(intel_dp);
-		}
+	if (!intel_edp_psr_match_conditions(intel_dp))
+		intel_edp_psr_disable(intel_dp);
+	else
+		intel_edp_psr_do_enable(intel_dp);
 }
 
 static void intel_edp_psr_inactivate(struct drm_device *dev)
-- 
2.0.0

[PATCH 07/16] drm/i915: Don't try to disable psr harder from the work item

[Daniel Vetter]

It's disabled already except when we've raced.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_dp.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 00ddcaf82ae1..a2f39055fbb1 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1879,9 +1879,7 @@ static void intel_edp_psr_work(struct work_struct *work)
 	if (!intel_dp)
 		return;
 
-	if (!intel_edp_psr_match_conditions(intel_dp))
-		intel_edp_psr_disable(intel_dp);
-	else
+	if (intel_edp_psr_match_conditions(intel_dp))
 		intel_edp_psr_do_enable(intel_dp);
 }
 
-- 
2.0.0

[PATCH 08/16] drm/i915: Lock down psr sw/hw state tracking

[Daniel Vetter]

Make sure we track the sw side (psr.active) correctly and WARN
everywhere it might get out of sync with the hw.

v2: Fixup WARN_ON logic inversion, reported by Rodrigo.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_dp.c | 43 ++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a2f39055fbb1..910f73de3a92 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1817,8 +1817,8 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (intel_edp_is_psr_enabled(dev))
-		return;
+	WARN_ON(I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE);
+	WARN_ON(dev_priv->psr.active);
 
 	/* Enable PSR on the panel */
 	intel_edp_psr_enable_sink(intel_dp);
@@ -1859,13 +1859,19 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
 	if (!dev_priv->psr.enabled)
 		return;
 
-	I915_WRITE(EDP_PSR_CTL(dev),
-		   I915_READ(EDP_PSR_CTL(dev)) & ~EDP_PSR_ENABLE);
+	if (dev_priv->psr.active) {
+		I915_WRITE(EDP_PSR_CTL(dev),
+			   I915_READ(EDP_PSR_CTL(dev)) & ~EDP_PSR_ENABLE);
+
+		/* Wait till PSR is idle */
+		if (_wait_for((I915_READ(EDP_PSR_STATUS_CTL(dev)) &
+			       EDP_PSR_STATUS_STATE_MASK) == 0, 2000, 10))
+			DRM_ERROR("Timed out waiting for PSR Idle State\n");
 
-	/* Wait till PSR is idle */
-	if (_wait_for((I915_READ(EDP_PSR_STATUS_CTL(dev)) &
-		       EDP_PSR_STATUS_STATE_MASK) == 0, 2000, 10))
-		DRM_ERROR("Timed out waiting for PSR Idle State\n");
+		dev_priv->psr.active = false;
+	} else {
+		WARN_ON(I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE);
+	}
 
 	dev_priv->psr.enabled = NULL;
 }
@@ -1883,16 +1889,6 @@ static void intel_edp_psr_work(struct work_struct *work)
 		intel_edp_psr_do_enable(intel_dp);
 }
 
-static void intel_edp_psr_inactivate(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	dev_priv->psr.active = false;
-
-	I915_WRITE(EDP_PSR_CTL(dev), I915_READ(EDP_PSR_CTL(dev))
-		   & ~EDP_PSR_ENABLE);
-}
-
 void intel_edp_psr_exit(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1905,8 +1901,15 @@ void intel_edp_psr_exit(struct drm_device *dev)
 
 	cancel_delayed_work_sync(&dev_priv->psr.work);
 
-	if (dev_priv->psr.active)
-		intel_edp_psr_inactivate(dev);
+	if (dev_priv->psr.active) {
+		u32 val = I915_READ(EDP_PSR_CTL(dev));
+
+		WARN_ON(!(val & EDP_PSR_ENABLE));
+
+		I915_WRITE(EDP_PSR_CTL(dev), val & ~EDP_PSR_ENABLE);
+
+		dev_priv->psr.active = false;
+	}
 
 	schedule_delayed_work(&dev_priv->psr.work,
 			      msecs_to_jiffies(100));
-- 
2.0.0

[PATCH 09/16] drm/i915: More checks for psr.enabled

[Daniel Vetter]

We need to make sure that no one else is using this in the
enable function and also that the work item hasn't raced
with the disabled function.

v2: Improve bisectability by moving one hunk to an earlier patch.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_dp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 910f73de3a92..870219ff1187 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1844,6 +1844,11 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
 		return;
 	}
 
+	if (dev_priv->psr.enabled) {
+		DRM_DEBUG_KMS("PSR already in use\n");
+		return;
+	}
+
 	/* Setup PSR once */
 	intel_edp_psr_setup(intel_dp);
 
-- 
2.0.0

Re: [PATCH 09/16] drm/i915: More checks for psr.enabled

[Chris Wilson]

On Wed, Jun 18, 2014 at 01:59:10PM +0200, Daniel Vetter wrote:
> We need to make sure that no one else is using this in the
> enable function and also that the work item hasn't raced
> with the disabled function.
> 
> v2: Improve bisectability by moving one hunk to an earlier patch.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 910f73de3a92..870219ff1187 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1844,6 +1844,11 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
>  		return;
>  	}

Is this the tail of a HAS_PSR() now made obsolete?
  
> +	if (dev_priv->psr.enabled) {
> +		DRM_DEBUG_KMS("PSR already in use\n");
> +		return;
> +	}
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 09/16] drm/i915: More checks for psr.enabled

[Daniel Vetter]

On Wed, Jun 18, 2014 at 01:27:06PM +0100, Chris Wilson wrote:
> On Wed, Jun 18, 2014 at 01:59:10PM +0200, Daniel Vetter wrote:
> > We need to make sure that no one else is using this in the
> > enable function and also that the work item hasn't raced
> > with the disabled function.
> > 
> > v2: Improve bisectability by moving one hunk to an earlier patch.
> > 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 910f73de3a92..870219ff1187 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1844,6 +1844,11 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
> >  		return;
> >  	}
> 
> Is this the tail of a HAS_PSR() now made obsolete?

Yeah, we have a bit of redundancy here now I think. Otoh once we have
locking they make sense again since HAS_PSR can be checked without
grabbing the psr lock, while psr.enabled can't. So I think it makes sense
to keep them.
-Daniel

>   
> > +	if (dev_priv->psr.enabled) {
> > +		DRM_DEBUG_KMS("PSR already in use\n");
> > +		return;
> > +	}
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

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

Re: [PATCH 09/16] drm/i915: More checks for psr.enabled

[Chris Wilson]

On Wed, Jun 18, 2014 at 02:41:34PM +0200, Daniel Vetter wrote:
> On Wed, Jun 18, 2014 at 01:27:06PM +0100, Chris Wilson wrote:
> > On Wed, Jun 18, 2014 at 01:59:10PM +0200, Daniel Vetter wrote:
> > > We need to make sure that no one else is using this in the
> > > enable function and also that the work item hasn't raced
> > > with the disabled function.
> > > 
> > > v2: Improve bisectability by moving one hunk to an earlier patch.
> > > 
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index 910f73de3a92..870219ff1187 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -1844,6 +1844,11 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
> > >  		return;
> > >  	}
> > 
> > Is this the tail of a HAS_PSR() now made obsolete?
> 
> Yeah, we have a bit of redundancy here now I think. Otoh once we have
> locking they make sense again since HAS_PSR can be checked without
> grabbing the psr lock, while psr.enabled can't. So I think it makes sense
> to keep them.

We do try to encourage the idiom of checking compatibility once at init,
then checking state at runtime. If the lock becomes burdensome we can
mitigate it.  Hmm, such as

frontbuffer_bits &= ~fb_tracking.busy_bits; /* only notify us for changes in state */

Further promoting the idea of refactoring cpu/gpu/flip tracking. :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 09/16] drm/i915: More checks for psr.enabled

[Daniel Vetter]

On Wed, Jun 18, 2014 at 01:46:16PM +0100, Chris Wilson wrote:
> On Wed, Jun 18, 2014 at 02:41:34PM +0200, Daniel Vetter wrote:
> > On Wed, Jun 18, 2014 at 01:27:06PM +0100, Chris Wilson wrote:
> > > On Wed, Jun 18, 2014 at 01:59:10PM +0200, Daniel Vetter wrote:
> > > > We need to make sure that no one else is using this in the
> > > > enable function and also that the work item hasn't raced
> > > > with the disabled function.
> > > > 
> > > > v2: Improve bisectability by moving one hunk to an earlier patch.
> > > > 
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 910f73de3a92..870219ff1187 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -1844,6 +1844,11 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
> > > >  		return;
> > > >  	}
> > > 
> > > Is this the tail of a HAS_PSR() now made obsolete?
> > 
> > Yeah, we have a bit of redundancy here now I think. Otoh once we have
> > locking they make sense again since HAS_PSR can be checked without
> > grabbing the psr lock, while psr.enabled can't. So I think it makes sense
> > to keep them.
> 
> We do try to encourage the idiom of checking compatibility once at init,
> then checking state at runtime. If the lock becomes burdensome we can
> mitigate it.  Hmm, such as

Ok, convinced, I'll throw a cleanup patch on top.

> frontbuffer_bits &= ~fb_tracking.busy_bits; /* only notify us for changes in state */
> 
> Further promoting the idea of refactoring cpu/gpu/flip tracking. :)

The idea is that consumers like psr track the state and have the edge
detection they care about. Once we have drrs and fbc converted we can have
a look at this again and whether extracting common logic makes sense. But
I suspect that there's not much room given that we always accumulate funky
platform hacks like the hsw sprite case. So passing the unfilter events to
the consumers seemed like the right approach.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 10/16] drm/i915: Add locking to psr code

[Daniel Vetter]

It's not really optional to have locking ...

The ugly part is how much locking the psr work needs since it has to
recheck everything. Which is way too much. But we need to ditch the
psr work in it's current form anyway and implement proper frontbuffer
tracking.

The other nasty bit that had to go was the delayed work cancle in
psr_exit. Which means a bunch of races just became a bit more likely,
but mea culpa.

v2: Fixup HAS_PSR checks, resulting in uninitialized mutex issues.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/intel_dp.c | 39 ++++++++++++++++++++++++++++++++-------
 2 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 740a0d95b89a..b6195ea334e6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -635,6 +635,7 @@ struct i915_drrs {
 
 struct intel_dp;
 struct i915_psr {
+	struct mutex lock;
 	bool sink_support;
 	bool source_ok;
 	struct intel_dp *enabled;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 870219ff1187..e17a85468f78 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1749,6 +1749,11 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
 	struct drm_i915_gem_object *obj = to_intel_framebuffer(crtc->primary->fb)->obj;
 	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
 
+	lockdep_assert_held(&dev_priv->psr.lock);
+	lockdep_assert_held(&dev->struct_mutex);
+	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
+	WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
+
 	dev_priv->psr.source_ok = false;
 
 	if (!HAS_PSR(dev)) {
@@ -1819,6 +1824,7 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
 
 	WARN_ON(I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE);
 	WARN_ON(dev_priv->psr.active);
+	lockdep_assert_held(&dev_priv->psr.lock);
 
 	/* Enable PSR on the panel */
 	intel_edp_psr_enable_sink(intel_dp);
@@ -1833,6 +1839,7 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
 void intel_edp_psr_enable(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	if (!HAS_PSR(dev)) {
 		DRM_DEBUG_KMS("PSR not supported on this platform\n");
@@ -1844,8 +1851,10 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
 		return;
 	}
 
+	mutex_lock(&dev_priv->psr.lock);
 	if (dev_priv->psr.enabled) {
 		DRM_DEBUG_KMS("PSR already in use\n");
+		mutex_unlock(&dev_priv->psr.lock);
 		return;
 	}
 
@@ -1854,6 +1863,7 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
 
 	if (intel_edp_psr_match_conditions(intel_dp))
 		intel_edp_psr_do_enable(intel_dp);
+	mutex_unlock(&dev_priv->psr.lock);
 }
 
 void intel_edp_psr_disable(struct intel_dp *intel_dp)
@@ -1861,8 +1871,14 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (!dev_priv->psr.enabled)
+	if (!HAS_PSR(dev))
+		return;
+
+	mutex_lock(&dev_priv->psr.lock);
+	if (!dev_priv->psr.enabled) {
+		mutex_unlock(&dev_priv->psr.lock);
 		return;
+	}
 
 	if (dev_priv->psr.active) {
 		I915_WRITE(EDP_PSR_CTL(dev),
@@ -1879,19 +1895,30 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
 	}
 
 	dev_priv->psr.enabled = NULL;
+	mutex_unlock(&dev_priv->psr.lock);
 }
 
 static void intel_edp_psr_work(struct work_struct *work)
 {
 	struct drm_i915_private *dev_priv =
 		container_of(work, typeof(*dev_priv), psr.work.work);
+	struct drm_device *dev = dev_priv->dev;
 	struct intel_dp *intel_dp = dev_priv->psr.enabled;
 
+	drm_modeset_lock_all(dev);
+	mutex_lock(&dev->struct_mutex);
+	mutex_lock(&dev_priv->psr.lock);
+	intel_dp = dev_priv->psr.enabled;
+
 	if (!intel_dp)
-		return;
+		goto unlock;
 
 	if (intel_edp_psr_match_conditions(intel_dp))
 		intel_edp_psr_do_enable(intel_dp);
+unlock:
+	mutex_unlock(&dev_priv->psr.lock);
+	mutex_unlock(&dev->struct_mutex);
+	drm_modeset_unlock_all(dev);
 }
 
 void intel_edp_psr_exit(struct drm_device *dev)
@@ -1904,8 +1931,7 @@ void intel_edp_psr_exit(struct drm_device *dev)
 	if (!dev_priv->psr.enabled)
 		return;
 
-	cancel_delayed_work_sync(&dev_priv->psr.work);
-
+	mutex_lock(&dev_priv->psr.lock);
 	if (dev_priv->psr.active) {
 		u32 val = I915_READ(EDP_PSR_CTL(dev));
 
@@ -1918,16 +1944,15 @@ void intel_edp_psr_exit(struct drm_device *dev)
 
 	schedule_delayed_work(&dev_priv->psr.work,
 			      msecs_to_jiffies(100));
+	mutex_unlock(&dev_priv->psr.lock);
 }
 
 void intel_edp_psr_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (!HAS_PSR(dev))
-		return;
-
 	INIT_DELAYED_WORK(&dev_priv->psr.work, intel_edp_psr_work);
+	mutex_init(&dev_priv->psr.lock);
 }
 
 static void intel_disable_dp(struct intel_encoder *encoder)
-- 
2.0.0

[PATCH 11/16] drm/i915: Introduce accurate frontbuffer tracking

[Daniel Vetter]

So from just a quick look we seem to have enough information to
accurately figure out whether a given gem bo is used as a frontbuffer
and where exactly: We have obj->pin_count as a first check with no
false negatives and only negligible false positives. And then we can
just walk the modeset objects and figure out where exactly a buffer is
used as scanout.

Except that we can't due to locking order: If we already hold
dev->struct_mutex we can't acquire any modeset locks, so could
potential chase freed pointers and other evil stuff.

So we need something else. For that introduce a new set of bits
obj->frontbuffer_bits to track where a buffer object is used. That we
can then chase without grabbing any modeset locks.

Of course the consumers of this (DRRS, PSR, FBC, ...) still need to be
able to do their magic both when called from modeset and from gem
code. But that can be easily achieved by adding locks for these
specific subsystems which always nest within either kms or gem
locking.

This patch just adds the relevant update code to all places.

Note that if we ever support multi-planar scanout targets then we need
one frontbuffer tracking bit per attachment point that we expose to
userspace.

v2:
- Fix more oopsen. Oops.
- WARN if we leak obj->frontbuffer_bits when freeing a gem buffer. Fix
  the bugs this brought to light.
- s/update_frontbuffer_bits/update_fb_bits/. More consistent with the
  fb tracking functions (fb for gem object, frontbuffer for raw bits).
  And the function name was way too long.

v3: Size obj->frontbuffer_bits correctly so that all pipes fit in.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h      | 26 +++++++++++++
 drivers/gpu/drm/i915/i915_gem.c      | 19 ++++++++++
 drivers/gpu/drm/i915/intel_display.c | 72 +++++++++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_overlay.c | 11 ++++++
 drivers/gpu/drm/i915/intel_sprite.c  |  7 ++++
 5 files changed, 118 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b6195ea334e6..2ae2b96c371d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1595,6 +1595,26 @@ struct drm_i915_gem_object_ops {
 	void (*release)(struct drm_i915_gem_object *);
 };
 
+/*
+ * Frontbuffer tracking bits. Set in obj->frontbuffer_bits while a gem bo is
+ * considered to be the frontbuffer for the given plane interface-vise. This
+ * doesn't mean that the hw necessarily already scans it out, but that any
+ * rendering (by the cpu or gpu) will land in the frontbuffer eventually.
+ *
+ * We have one bit per pipe and per scanout plane type.
+ */
+#define INTEL_FRONTBUFFER_BITS_PER_PIPE 4
+#define INTEL_FRONTBUFFER_BITS \
+	(INTEL_FRONTBUFFER_BITS_PER_PIPE * I915_MAX_PIPES)
+#define INTEL_FRONTBUFFER_PRIMARY(pipe) \
+	(1 << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
+#define INTEL_FRONTBUFFER_CURSOR(pipe) \
+	(1 << (1 +(INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe))))
+#define INTEL_FRONTBUFFER_SPRITE(pipe) \
+	(1 << (2 +(INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe))))
+#define INTEL_FRONTBUFFER_OVERLAY(pipe) \
+	(1 << (3 +(INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe))))
+
 struct drm_i915_gem_object {
 	struct drm_gem_object base;
 
@@ -1682,6 +1702,8 @@ struct drm_i915_gem_object {
 	unsigned int has_global_gtt_mapping:1;
 	unsigned int has_dma_mapping:1;
 
+	unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS;
+
 	struct sg_table *pages;
 	int pages_pin_count;
 
@@ -1728,6 +1750,10 @@ struct drm_i915_gem_object {
 };
 #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base)
 
+void i915_gem_update_fb_bits(struct drm_i915_gem_object *old,
+			     struct drm_i915_gem_object *new,
+			     unsigned frontbuffer_bits);
+
 /**
  * Request queue structure.
  *
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7d7d52bc922a..c40dd4ba2c27 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4449,6 +4449,8 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
 	if (obj->stolen)
 		i915_gem_object_unpin_pages(obj);
 
+	WARN_ON(obj->frontbuffer_bits);
+
 	if (WARN_ON(obj->pages_pin_count))
 		obj->pages_pin_count = 0;
 	if (discard_backing_storage(obj))
@@ -4993,6 +4995,23 @@ int i915_gem_open(struct drm_device *dev, struct drm_file *file)
 	return ret;
 }
 
+void i915_gem_update_fb_bits(struct drm_i915_gem_object *old,
+			     struct drm_i915_gem_object *new,
+			     unsigned frontbuffer_bits)
+{
+	if (old) {
+		WARN_ON(!mutex_is_locked(&old->base.dev->struct_mutex));
+		WARN_ON(!(old->frontbuffer_bits & frontbuffer_bits));
+		old->frontbuffer_bits &= ~frontbuffer_bits;
+	}
+
+	if (new) {
+		WARN_ON(!mutex_is_locked(&new->base.dev->struct_mutex));
+		WARN_ON(new->frontbuffer_bits & frontbuffer_bits);
+		new->frontbuffer_bits |= frontbuffer_bits;
+	}
+}
+
 static bool mutex_is_locked_by(struct mutex *mutex, struct task_struct *task)
 {
 	if (!mutex_is_locked(mutex))
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 548161d9ac8a..87d3be939488 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2351,6 +2351,7 @@ static bool intel_alloc_plane_obj(struct intel_crtc *crtc,
 		goto out_unref_obj;
 	}
 
+	obj->frontbuffer_bits = INTEL_FRONTBUFFER_PRIMARY(crtc->pipe);
 	mutex_unlock(&dev->struct_mutex);
 
 	DRM_DEBUG_KMS("plane fb obj %p\n", obj);
@@ -2396,6 +2397,7 @@ static void intel_find_plane_obj(struct intel_crtc *intel_crtc,
 		if (i915_gem_obj_ggtt_offset(fb->obj) == plane_config->base) {
 			drm_framebuffer_reference(c->primary->fb);
 			intel_crtc->base.primary->fb = c->primary->fb;
+			fb->obj->frontbuffer_bits |= INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
 			break;
 		}
 	}
@@ -2684,7 +2686,9 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	enum pipe pipe = intel_crtc->pipe;
 	struct drm_framebuffer *old_fb;
+	struct drm_i915_gem_object *obj = to_intel_framebuffer(fb)->obj;
 	int ret;
 
 	if (intel_crtc_has_pending_flip(crtc)) {
@@ -2705,10 +2709,12 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 		return -EINVAL;
 	}
 
+	old_fb = crtc->primary->fb;
+
 	mutex_lock(&dev->struct_mutex);
-	ret = intel_pin_and_fence_fb_obj(dev,
-					 to_intel_framebuffer(fb)->obj,
-					 NULL);
+	ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
+	i915_gem_update_fb_bits(to_intel_framebuffer(old_fb)->obj, obj,
+				INTEL_FRONTBUFFER_PRIMARY(pipe));
 	mutex_unlock(&dev->struct_mutex);
 	if (ret != 0) {
 		DRM_ERROR("pin & fence failed\n");
@@ -2748,7 +2754,6 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 
 	dev_priv->display.update_primary_plane(crtc, fb, x, y);
 
-	old_fb = crtc->primary->fb;
 	crtc->primary->fb = fb;
 	crtc->x = x;
 	crtc->y = y;
@@ -4922,6 +4927,8 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
 	struct drm_device *dev = crtc->dev;
 	struct drm_connector *connector;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_gem_object *old_obj;
+	enum pipe pipe = to_intel_crtc(crtc)->pipe;
 
 	/* crtc should still be enabled when we disable it. */
 	WARN_ON(!crtc->enabled);
@@ -4935,8 +4942,11 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
 	assert_pipe_disabled(dev->dev_private, to_intel_crtc(crtc)->pipe);
 
 	if (crtc->primary->fb) {
+		old_obj = to_intel_framebuffer(crtc->primary->fb)->obj;
 		mutex_lock(&dev->struct_mutex);
-		intel_unpin_fb_obj(to_intel_framebuffer(crtc->primary->fb)->obj);
+		intel_unpin_fb_obj(old_obj);
+		i915_gem_update_fb_bits(old_obj, NULL,
+					INTEL_FRONTBUFFER_PRIMARY(pipe));
 		mutex_unlock(&dev->struct_mutex);
 		crtc->primary->fb = NULL;
 	}
@@ -8103,6 +8113,7 @@ static int intel_crtc_cursor_set_obj(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);
+	enum pipe pipe = intel_crtc->pipe;
 	unsigned old_width;
 	uint32_t addr;
 	int ret;
@@ -8182,6 +8193,8 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
 	}
 
+	i915_gem_update_fb_bits(intel_crtc->cursor_bo, obj,
+				INTEL_FRONTBUFFER_CURSOR(pipe));
 	mutex_unlock(&dev->struct_mutex);
 
 	old_width = intel_crtc->cursor_width;
@@ -9404,6 +9417,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	struct drm_framebuffer *old_fb = crtc->primary->fb;
 	struct drm_i915_gem_object *obj = to_intel_framebuffer(fb)->obj;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	enum pipe pipe = intel_crtc->pipe;
 	struct intel_unpin_work *work;
 	struct intel_engine_cs *ring;
 	unsigned long flags;
@@ -9503,6 +9517,9 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	if (ret)
 		goto cleanup_unpin;
 
+	i915_gem_update_fb_bits(work->old_fb_obj, obj,
+				INTEL_FRONTBUFFER_PRIMARY(pipe));
+
 	intel_disable_fbc(dev);
 	intel_mark_fb_busy(obj, NULL);
 	mutex_unlock(&dev->struct_mutex);
@@ -10569,10 +10586,14 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 	 */
 	for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
 		struct drm_framebuffer *old_fb;
+		struct drm_i915_gem_object *old_obj = NULL;
+		struct drm_i915_gem_object *obj =
+			to_intel_framebuffer(fb)->obj;
+		enum pipe pipe = intel_crtc->pipe;
 
 		mutex_lock(&dev->struct_mutex);
 		ret = intel_pin_and_fence_fb_obj(dev,
-						 to_intel_framebuffer(fb)->obj,
+						 obj,
 						 NULL);
 		if (ret != 0) {
 			DRM_ERROR("pin & fence failed\n");
@@ -10580,8 +10601,12 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 			goto done;
 		}
 		old_fb = crtc->primary->fb;
-		if (old_fb)
-			intel_unpin_fb_obj(to_intel_framebuffer(old_fb)->obj);
+		if (old_fb) {
+			old_obj = to_intel_framebuffer(old_fb)->obj;
+			intel_unpin_fb_obj(old_obj);
+		}
+		i915_gem_update_fb_bits(old_obj, obj,
+					INTEL_FRONTBUFFER_PRIMARY(pipe));
 		mutex_unlock(&dev->struct_mutex);
 
 		crtc->primary->fb = fb;
@@ -11173,6 +11198,7 @@ intel_primary_plane_disable(struct drm_plane *plane)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct intel_crtc *intel_crtc;
+	enum pipe pipe;
 
 	if (!plane->fb)
 		return 0;
@@ -11180,6 +11206,7 @@ intel_primary_plane_disable(struct drm_plane *plane)
 	BUG_ON(!plane->crtc);
 
 	intel_crtc = to_intel_crtc(plane->crtc);
+	pipe = intel_crtc->pipe;
 
 	/*
 	 * Even though we checked plane->fb above, it's still possible that
@@ -11196,8 +11223,9 @@ intel_primary_plane_disable(struct drm_plane *plane)
 	intel_crtc_wait_for_pending_flips(plane->crtc);
 	intel_disable_primary_hw_plane(dev_priv, intel_plane->plane,
 				       intel_plane->pipe);
-
 disable_unpin:
+	i915_gem_update_fb_bits(to_intel_framebuffer(plane->fb)->obj, NULL,
+				INTEL_FRONTBUFFER_PRIMARY(pipe));
 	intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
 	plane->fb = NULL;
 
@@ -11214,7 +11242,9 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	enum pipe pipe = intel_crtc->pipe;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct drm_i915_gem_object *obj, *old_obj = NULL;
 	struct drm_rect dest = {
 		/* integer pixels */
 		.x1 = crtc_x,
@@ -11246,6 +11276,10 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (ret)
 		return ret;
 
+	if (plane->fb)
+		old_obj = to_intel_framebuffer(plane->fb)->obj;
+	obj = to_intel_framebuffer(fb)->obj;
+
 	/*
 	 * If the CRTC isn't enabled, we're just pinning the framebuffer,
 	 * updating the fb pointer, and returning without touching the
@@ -11253,17 +11287,20 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 	 * turn on the display with all planes setup as desired.
 	 */
 	if (!crtc->enabled) {
+		enum pipe pipe = intel_crtc->pipe;
+
 		/*
 		 * If we already called setplane while the crtc was disabled,
 		 * we may have an fb pinned; unpin it.
 		 */
 		if (plane->fb)
-			intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
+			intel_unpin_fb_obj(old_obj);
+
+		i915_gem_update_fb_bits(old_obj, obj,
+					INTEL_FRONTBUFFER_PRIMARY(pipe));
 
 		/* Pin and return without programming hardware */
-		return intel_pin_and_fence_fb_obj(dev,
-						  to_intel_framebuffer(fb)->obj,
-						  NULL);
+		return intel_pin_and_fence_fb_obj(dev, obj, NULL);
 	}
 
 	intel_crtc_wait_for_pending_flips(crtc);
@@ -11280,13 +11317,14 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 		 * fail.
 		 */
 		if (plane->fb != fb) {
-			ret = intel_pin_and_fence_fb_obj(dev,
-							 to_intel_framebuffer(fb)->obj,
-							 NULL);
+			ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
 			if (ret)
 				return ret;
 		}
 
+		i915_gem_update_fb_bits(old_obj, obj,
+					INTEL_FRONTBUFFER_PRIMARY(pipe));
+
 		if (intel_crtc->primary_enabled)
 			intel_disable_primary_hw_plane(dev_priv,
 						       intel_plane->plane,
@@ -11295,7 +11333,7 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 
 		if (plane->fb != fb)
 			if (plane->fb)
-				intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
+				intel_unpin_fb_obj(old_obj);
 
 		return 0;
 	}
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index daa118978eec..6aeda6a489a0 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -390,6 +390,7 @@ static int intel_overlay_release_old_vid(struct intel_overlay *overlay)
 	struct drm_device *dev = overlay->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *ring = &dev_priv->ring[RCS];
+	enum pipe pipe;
 	int ret;
 
 	/* Only wait if there is actually an old frame to release to
@@ -398,6 +399,8 @@ static int intel_overlay_release_old_vid(struct intel_overlay *overlay)
 	if (!overlay->old_vid_bo)
 		return 0;
 
+	pipe = overlay->crtc->pipe;
+
 	if (I915_READ(ISR) & I915_OVERLAY_PLANE_FLIP_PENDING_INTERRUPT) {
 		/* synchronous slowpath */
 		ret = intel_ring_begin(ring, 2);
@@ -415,6 +418,10 @@ static int intel_overlay_release_old_vid(struct intel_overlay *overlay)
 	}
 
 	intel_overlay_release_old_vid_tail(overlay);
+
+
+	i915_gem_update_fb_bits(overlay->old_vid_bo, NULL,
+				INTEL_FRONTBUFFER_OVERLAY(pipe));
 	return 0;
 }
 
@@ -686,6 +693,7 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
 	bool scale_changed = false;
 	struct drm_device *dev = overlay->dev;
 	u32 swidth, swidthsw, sheight, ostride;
+	enum pipe pipe = overlay->crtc->pipe;
 
 	BUG_ON(!mutex_is_locked(&dev->struct_mutex));
 	BUG_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
@@ -776,6 +784,9 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
 	if (ret)
 		goto out_unpin;
 
+	i915_gem_update_fb_bits(overlay->vid_bo, new_bo,
+				INTEL_FRONTBUFFER_OVERLAY(pipe));
+
 	overlay->old_vid_bo = overlay->vid_bo;
 	overlay->vid_bo = new_bo;
 
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 9038e2ab73c8..3648594aeb7e 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -811,6 +811,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = plane->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	enum pipe pipe = intel_crtc->pipe;
 	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
 	struct drm_i915_gem_object *obj = intel_fb->obj;
 	struct drm_i915_gem_object *old_obj = intel_plane->obj;
@@ -998,6 +999,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	 */
 	ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
 
+	i915_gem_update_fb_bits(old_obj, obj,
+				INTEL_FRONTBUFFER_SPRITE(pipe));
 	mutex_unlock(&dev->struct_mutex);
 
 	if (ret)
@@ -1062,6 +1065,7 @@ intel_disable_plane(struct drm_plane *plane)
 	struct drm_device *dev = plane->dev;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct intel_crtc *intel_crtc;
+	enum pipe pipe;
 
 	if (!plane->fb)
 		return 0;
@@ -1070,6 +1074,7 @@ intel_disable_plane(struct drm_plane *plane)
 		return -EINVAL;
 
 	intel_crtc = to_intel_crtc(plane->crtc);
+	pipe = intel_crtc->pipe;
 
 	if (intel_crtc->active) {
 		bool primary_was_enabled = intel_crtc->primary_enabled;
@@ -1088,6 +1093,8 @@ intel_disable_plane(struct drm_plane *plane)
 
 		mutex_lock(&dev->struct_mutex);
 		intel_unpin_fb_obj(intel_plane->obj);
+		i915_gem_update_fb_bits(intel_plane->obj, NULL,
+					INTEL_FRONTBUFFER_SPRITE(pipe));
 		mutex_unlock(&dev->struct_mutex);
 
 		intel_plane->obj = NULL;
-- 
2.0.0

Re: [PATCH 11/16] drm/i915: Introduce accurate frontbuffer tracking

[Chris Wilson]

On Wed, Jun 18, 2014 at 01:59:12PM +0200, Daniel Vetter wrote:
>  	mutex_lock(&dev->struct_mutex);
> -	ret = intel_pin_and_fence_fb_obj(dev,
> -					 to_intel_framebuffer(fb)->obj,
> -					 NULL);
> +	ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
> +	i915_gem_update_fb_bits(to_intel_framebuffer(old_fb)->obj, obj,
> +				INTEL_FRONTBUFFER_PRIMARY(pipe));

Hey, no changing state on a failed operation.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: Introduce accurate frontbuffer tracking

[Daniel Vetter]

So from just a quick look we seem to have enough information to
accurately figure out whether a given gem bo is used as a frontbuffer
and where exactly: We have obj->pin_count as a first check with no
false negatives and only negligible false positives. And then we can
just walk the modeset objects and figure out where exactly a buffer is
used as scanout.

Except that we can't due to locking order: If we already hold
dev->struct_mutex we can't acquire any modeset locks, so could
potential chase freed pointers and other evil stuff.

So we need something else. For that introduce a new set of bits
obj->frontbuffer_bits to track where a buffer object is used. That we
can then chase without grabbing any modeset locks.

Of course the consumers of this (DRRS, PSR, FBC, ...) still need to be
able to do their magic both when called from modeset and from gem
code. But that can be easily achieved by adding locks for these
specific subsystems which always nest within either kms or gem
locking.

This patch just adds the relevant update code to all places.

Note that if we ever support multi-planar scanout targets then we need
one frontbuffer tracking bit per attachment point that we expose to
userspace.

v2:
- Fix more oopsen. Oops.
- WARN if we leak obj->frontbuffer_bits when freeing a gem buffer. Fix
  the bugs this brought to light.
- s/update_frontbuffer_bits/update_fb_bits/. More consistent with the
  fb tracking functions (fb for gem object, frontbuffer for raw bits).
  And the function name was way too long.

v3: Size obj->frontbuffer_bits correctly so that all pipes fit in.

v4: Don't update fb bits in set_base on failure. Noticed by Chris.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h      | 26 +++++++++++++
 drivers/gpu/drm/i915/i915_gem.c      | 19 ++++++++++
 drivers/gpu/drm/i915/intel_display.c | 73 +++++++++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_overlay.c | 11 ++++++
 drivers/gpu/drm/i915/intel_sprite.c  |  7 ++++
 5 files changed, 119 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b6195ea334e6..2ae2b96c371d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1595,6 +1595,26 @@ struct drm_i915_gem_object_ops {
 	void (*release)(struct drm_i915_gem_object *);
 };
 
+/*
+ * Frontbuffer tracking bits. Set in obj->frontbuffer_bits while a gem bo is
+ * considered to be the frontbuffer for the given plane interface-vise. This
+ * doesn't mean that the hw necessarily already scans it out, but that any
+ * rendering (by the cpu or gpu) will land in the frontbuffer eventually.
+ *
+ * We have one bit per pipe and per scanout plane type.
+ */
+#define INTEL_FRONTBUFFER_BITS_PER_PIPE 4
+#define INTEL_FRONTBUFFER_BITS \
+	(INTEL_FRONTBUFFER_BITS_PER_PIPE * I915_MAX_PIPES)
+#define INTEL_FRONTBUFFER_PRIMARY(pipe) \
+	(1 << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
+#define INTEL_FRONTBUFFER_CURSOR(pipe) \
+	(1 << (1 +(INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe))))
+#define INTEL_FRONTBUFFER_SPRITE(pipe) \
+	(1 << (2 +(INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe))))
+#define INTEL_FRONTBUFFER_OVERLAY(pipe) \
+	(1 << (3 +(INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe))))
+
 struct drm_i915_gem_object {
 	struct drm_gem_object base;
 
@@ -1682,6 +1702,8 @@ struct drm_i915_gem_object {
 	unsigned int has_global_gtt_mapping:1;
 	unsigned int has_dma_mapping:1;
 
+	unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS;
+
 	struct sg_table *pages;
 	int pages_pin_count;
 
@@ -1728,6 +1750,10 @@ struct drm_i915_gem_object {
 };
 #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base)
 
+void i915_gem_update_fb_bits(struct drm_i915_gem_object *old,
+			     struct drm_i915_gem_object *new,
+			     unsigned frontbuffer_bits);
+
 /**
  * Request queue structure.
  *
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7d7d52bc922a..c40dd4ba2c27 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4449,6 +4449,8 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
 	if (obj->stolen)
 		i915_gem_object_unpin_pages(obj);
 
+	WARN_ON(obj->frontbuffer_bits);
+
 	if (WARN_ON(obj->pages_pin_count))
 		obj->pages_pin_count = 0;
 	if (discard_backing_storage(obj))
@@ -4993,6 +4995,23 @@ int i915_gem_open(struct drm_device *dev, struct drm_file *file)
 	return ret;
 }
 
+void i915_gem_update_fb_bits(struct drm_i915_gem_object *old,
+			     struct drm_i915_gem_object *new,
+			     unsigned frontbuffer_bits)
+{
+	if (old) {
+		WARN_ON(!mutex_is_locked(&old->base.dev->struct_mutex));
+		WARN_ON(!(old->frontbuffer_bits & frontbuffer_bits));
+		old->frontbuffer_bits &= ~frontbuffer_bits;
+	}
+
+	if (new) {
+		WARN_ON(!mutex_is_locked(&new->base.dev->struct_mutex));
+		WARN_ON(new->frontbuffer_bits & frontbuffer_bits);
+		new->frontbuffer_bits |= frontbuffer_bits;
+	}
+}
+
 static bool mutex_is_locked_by(struct mutex *mutex, struct task_struct *task)
 {
 	if (!mutex_is_locked(mutex))
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 548161d9ac8a..386e78df338b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2351,6 +2351,7 @@ static bool intel_alloc_plane_obj(struct intel_crtc *crtc,
 		goto out_unref_obj;
 	}
 
+	obj->frontbuffer_bits = INTEL_FRONTBUFFER_PRIMARY(crtc->pipe);
 	mutex_unlock(&dev->struct_mutex);
 
 	DRM_DEBUG_KMS("plane fb obj %p\n", obj);
@@ -2396,6 +2397,7 @@ static void intel_find_plane_obj(struct intel_crtc *intel_crtc,
 		if (i915_gem_obj_ggtt_offset(fb->obj) == plane_config->base) {
 			drm_framebuffer_reference(c->primary->fb);
 			intel_crtc->base.primary->fb = c->primary->fb;
+			fb->obj->frontbuffer_bits |= INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
 			break;
 		}
 	}
@@ -2684,7 +2686,9 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	enum pipe pipe = intel_crtc->pipe;
 	struct drm_framebuffer *old_fb;
+	struct drm_i915_gem_object *obj = to_intel_framebuffer(fb)->obj;
 	int ret;
 
 	if (intel_crtc_has_pending_flip(crtc)) {
@@ -2705,10 +2709,13 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 		return -EINVAL;
 	}
 
+	old_fb = crtc->primary->fb;
+
 	mutex_lock(&dev->struct_mutex);
-	ret = intel_pin_and_fence_fb_obj(dev,
-					 to_intel_framebuffer(fb)->obj,
-					 NULL);
+	ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
+	if (ret == 0)
+		i915_gem_update_fb_bits(to_intel_framebuffer(old_fb)->obj, obj,
+					INTEL_FRONTBUFFER_PRIMARY(pipe));
 	mutex_unlock(&dev->struct_mutex);
 	if (ret != 0) {
 		DRM_ERROR("pin & fence failed\n");
@@ -2748,7 +2755,6 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 
 	dev_priv->display.update_primary_plane(crtc, fb, x, y);
 
-	old_fb = crtc->primary->fb;
 	crtc->primary->fb = fb;
 	crtc->x = x;
 	crtc->y = y;
@@ -4922,6 +4928,8 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
 	struct drm_device *dev = crtc->dev;
 	struct drm_connector *connector;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_gem_object *old_obj;
+	enum pipe pipe = to_intel_crtc(crtc)->pipe;
 
 	/* crtc should still be enabled when we disable it. */
 	WARN_ON(!crtc->enabled);
@@ -4935,8 +4943,11 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
 	assert_pipe_disabled(dev->dev_private, to_intel_crtc(crtc)->pipe);
 
 	if (crtc->primary->fb) {
+		old_obj = to_intel_framebuffer(crtc->primary->fb)->obj;
 		mutex_lock(&dev->struct_mutex);
-		intel_unpin_fb_obj(to_intel_framebuffer(crtc->primary->fb)->obj);
+		intel_unpin_fb_obj(old_obj);
+		i915_gem_update_fb_bits(old_obj, NULL,
+					INTEL_FRONTBUFFER_PRIMARY(pipe));
 		mutex_unlock(&dev->struct_mutex);
 		crtc->primary->fb = NULL;
 	}
@@ -8103,6 +8114,7 @@ static int intel_crtc_cursor_set_obj(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);
+	enum pipe pipe = intel_crtc->pipe;
 	unsigned old_width;
 	uint32_t addr;
 	int ret;
@@ -8182,6 +8194,8 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
 	}
 
+	i915_gem_update_fb_bits(intel_crtc->cursor_bo, obj,
+				INTEL_FRONTBUFFER_CURSOR(pipe));
 	mutex_unlock(&dev->struct_mutex);
 
 	old_width = intel_crtc->cursor_width;
@@ -9404,6 +9418,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	struct drm_framebuffer *old_fb = crtc->primary->fb;
 	struct drm_i915_gem_object *obj = to_intel_framebuffer(fb)->obj;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	enum pipe pipe = intel_crtc->pipe;
 	struct intel_unpin_work *work;
 	struct intel_engine_cs *ring;
 	unsigned long flags;
@@ -9503,6 +9518,9 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	if (ret)
 		goto cleanup_unpin;
 
+	i915_gem_update_fb_bits(work->old_fb_obj, obj,
+				INTEL_FRONTBUFFER_PRIMARY(pipe));
+
 	intel_disable_fbc(dev);
 	intel_mark_fb_busy(obj, NULL);
 	mutex_unlock(&dev->struct_mutex);
@@ -10569,10 +10587,14 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 	 */
 	for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
 		struct drm_framebuffer *old_fb;
+		struct drm_i915_gem_object *old_obj = NULL;
+		struct drm_i915_gem_object *obj =
+			to_intel_framebuffer(fb)->obj;
+		enum pipe pipe = intel_crtc->pipe;
 
 		mutex_lock(&dev->struct_mutex);
 		ret = intel_pin_and_fence_fb_obj(dev,
-						 to_intel_framebuffer(fb)->obj,
+						 obj,
 						 NULL);
 		if (ret != 0) {
 			DRM_ERROR("pin & fence failed\n");
@@ -10580,8 +10602,12 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 			goto done;
 		}
 		old_fb = crtc->primary->fb;
-		if (old_fb)
-			intel_unpin_fb_obj(to_intel_framebuffer(old_fb)->obj);
+		if (old_fb) {
+			old_obj = to_intel_framebuffer(old_fb)->obj;
+			intel_unpin_fb_obj(old_obj);
+		}
+		i915_gem_update_fb_bits(old_obj, obj,
+					INTEL_FRONTBUFFER_PRIMARY(pipe));
 		mutex_unlock(&dev->struct_mutex);
 
 		crtc->primary->fb = fb;
@@ -11173,6 +11199,7 @@ intel_primary_plane_disable(struct drm_plane *plane)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct intel_crtc *intel_crtc;
+	enum pipe pipe;
 
 	if (!plane->fb)
 		return 0;
@@ -11180,6 +11207,7 @@ intel_primary_plane_disable(struct drm_plane *plane)
 	BUG_ON(!plane->crtc);
 
 	intel_crtc = to_intel_crtc(plane->crtc);
+	pipe = intel_crtc->pipe;
 
 	/*
 	 * Even though we checked plane->fb above, it's still possible that
@@ -11196,8 +11224,9 @@ intel_primary_plane_disable(struct drm_plane *plane)
 	intel_crtc_wait_for_pending_flips(plane->crtc);
 	intel_disable_primary_hw_plane(dev_priv, intel_plane->plane,
 				       intel_plane->pipe);
-
 disable_unpin:
+	i915_gem_update_fb_bits(to_intel_framebuffer(plane->fb)->obj, NULL,
+				INTEL_FRONTBUFFER_PRIMARY(pipe));
 	intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
 	plane->fb = NULL;
 
@@ -11214,7 +11243,9 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	enum pipe pipe = intel_crtc->pipe;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct drm_i915_gem_object *obj, *old_obj = NULL;
 	struct drm_rect dest = {
 		/* integer pixels */
 		.x1 = crtc_x,
@@ -11246,6 +11277,10 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (ret)
 		return ret;
 
+	if (plane->fb)
+		old_obj = to_intel_framebuffer(plane->fb)->obj;
+	obj = to_intel_framebuffer(fb)->obj;
+
 	/*
 	 * If the CRTC isn't enabled, we're just pinning the framebuffer,
 	 * updating the fb pointer, and returning without touching the
@@ -11253,17 +11288,20 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 	 * turn on the display with all planes setup as desired.
 	 */
 	if (!crtc->enabled) {
+		enum pipe pipe = intel_crtc->pipe;
+
 		/*
 		 * If we already called setplane while the crtc was disabled,
 		 * we may have an fb pinned; unpin it.
 		 */
 		if (plane->fb)
-			intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
+			intel_unpin_fb_obj(old_obj);
+
+		i915_gem_update_fb_bits(old_obj, obj,
+					INTEL_FRONTBUFFER_PRIMARY(pipe));
 
 		/* Pin and return without programming hardware */
-		return intel_pin_and_fence_fb_obj(dev,
-						  to_intel_framebuffer(fb)->obj,
-						  NULL);
+		return intel_pin_and_fence_fb_obj(dev, obj, NULL);
 	}
 
 	intel_crtc_wait_for_pending_flips(crtc);
@@ -11280,13 +11318,14 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 		 * fail.
 		 */
 		if (plane->fb != fb) {
-			ret = intel_pin_and_fence_fb_obj(dev,
-							 to_intel_framebuffer(fb)->obj,
-							 NULL);
+			ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
 			if (ret)
 				return ret;
 		}
 
+		i915_gem_update_fb_bits(old_obj, obj,
+					INTEL_FRONTBUFFER_PRIMARY(pipe));
+
 		if (intel_crtc->primary_enabled)
 			intel_disable_primary_hw_plane(dev_priv,
 						       intel_plane->plane,
@@ -11295,7 +11334,7 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 
 		if (plane->fb != fb)
 			if (plane->fb)
-				intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
+				intel_unpin_fb_obj(old_obj);
 
 		return 0;
 	}
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index daa118978eec..6aeda6a489a0 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -390,6 +390,7 @@ static int intel_overlay_release_old_vid(struct intel_overlay *overlay)
 	struct drm_device *dev = overlay->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *ring = &dev_priv->ring[RCS];
+	enum pipe pipe;
 	int ret;
 
 	/* Only wait if there is actually an old frame to release to
@@ -398,6 +399,8 @@ static int intel_overlay_release_old_vid(struct intel_overlay *overlay)
 	if (!overlay->old_vid_bo)
 		return 0;
 
+	pipe = overlay->crtc->pipe;
+
 	if (I915_READ(ISR) & I915_OVERLAY_PLANE_FLIP_PENDING_INTERRUPT) {
 		/* synchronous slowpath */
 		ret = intel_ring_begin(ring, 2);
@@ -415,6 +418,10 @@ static int intel_overlay_release_old_vid(struct intel_overlay *overlay)
 	}
 
 	intel_overlay_release_old_vid_tail(overlay);
+
+
+	i915_gem_update_fb_bits(overlay->old_vid_bo, NULL,
+				INTEL_FRONTBUFFER_OVERLAY(pipe));
 	return 0;
 }
 
@@ -686,6 +693,7 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
 	bool scale_changed = false;
 	struct drm_device *dev = overlay->dev;
 	u32 swidth, swidthsw, sheight, ostride;
+	enum pipe pipe = overlay->crtc->pipe;
 
 	BUG_ON(!mutex_is_locked(&dev->struct_mutex));
 	BUG_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
@@ -776,6 +784,9 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
 	if (ret)
 		goto out_unpin;
 
+	i915_gem_update_fb_bits(overlay->vid_bo, new_bo,
+				INTEL_FRONTBUFFER_OVERLAY(pipe));
+
 	overlay->old_vid_bo = overlay->vid_bo;
 	overlay->vid_bo = new_bo;
 
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 9038e2ab73c8..3648594aeb7e 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -811,6 +811,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = plane->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	enum pipe pipe = intel_crtc->pipe;
 	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
 	struct drm_i915_gem_object *obj = intel_fb->obj;
 	struct drm_i915_gem_object *old_obj = intel_plane->obj;
@@ -998,6 +999,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	 */
 	ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
 
+	i915_gem_update_fb_bits(old_obj, obj,
+				INTEL_FRONTBUFFER_SPRITE(pipe));
 	mutex_unlock(&dev->struct_mutex);
 
 	if (ret)
@@ -1062,6 +1065,7 @@ intel_disable_plane(struct drm_plane *plane)
 	struct drm_device *dev = plane->dev;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct intel_crtc *intel_crtc;
+	enum pipe pipe;
 
 	if (!plane->fb)
 		return 0;
@@ -1070,6 +1074,7 @@ intel_disable_plane(struct drm_plane *plane)
 		return -EINVAL;
 
 	intel_crtc = to_intel_crtc(plane->crtc);
+	pipe = intel_crtc->pipe;
 
 	if (intel_crtc->active) {
 		bool primary_was_enabled = intel_crtc->primary_enabled;
@@ -1088,6 +1093,8 @@ intel_disable_plane(struct drm_plane *plane)
 
 		mutex_lock(&dev->struct_mutex);
 		intel_unpin_fb_obj(intel_plane->obj);
+		i915_gem_update_fb_bits(intel_plane->obj, NULL,
+					INTEL_FRONTBUFFER_SPRITE(pipe));
 		mutex_unlock(&dev->struct_mutex);
 
 		intel_plane->obj = NULL;
-- 
2.0.0

Re: [PATCH] drm/i915: Introduce accurate frontbuffer tracking

[Chris Wilson]

On Wed, Jun 18, 2014 at 03:01:25PM +0200, Daniel Vetter wrote:
> +void i915_gem_update_fb_bits(struct drm_i915_gem_object *old,
> +			     struct drm_i915_gem_object *new,
> +			     unsigned frontbuffer_bits);
> +

Time to be a nuisance:

i915_gem_object_track_fb()

The key part is that is operates on the object. The other is just to try
and shorten the name as compensation.


> @@ -1062,6 +1065,7 @@ intel_disable_plane(struct drm_plane *plane)
>  	struct drm_device *dev = plane->dev;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	struct intel_crtc *intel_crtc;
> +	enum pipe pipe;
>  
>  	if (!plane->fb)
>  		return 0;
> @@ -1070,6 +1074,7 @@ intel_disable_plane(struct drm_plane *plane)
>  		return -EINVAL;
>  
>  	intel_crtc = to_intel_crtc(plane->crtc);
> +	pipe = intel_crtc->pipe;
>  
>  	if (intel_crtc->active) {
>  		bool primary_was_enabled = intel_crtc->primary_enabled;
> @@ -1088,6 +1093,8 @@ intel_disable_plane(struct drm_plane *plane)
>  
>  		mutex_lock(&dev->struct_mutex);
>  		intel_unpin_fb_obj(intel_plane->obj);
> +		i915_gem_update_fb_bits(intel_plane->obj, NULL,
> +					INTEL_FRONTBUFFER_SPRITE(pipe));

Introducing a local here for a one off, or was checkpatch in an angry
mood with INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe) ?

That's all I spotted, the logic looks fine. But just remind me,
i915_gem_object_track_fb() did do a lockdep_assert_held(&dev->struct_mutex)?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Introduce accurate frontbuffer tracking

[Daniel Vetter]

On Wed, Jun 18, 2014 at 03:55:44PM +0100, Chris Wilson wrote:
> On Wed, Jun 18, 2014 at 03:01:25PM +0200, Daniel Vetter wrote:
> > +void i915_gem_update_fb_bits(struct drm_i915_gem_object *old,
> > +			     struct drm_i915_gem_object *new,
> > +			     unsigned frontbuffer_bits);
> > +
> 
> Time to be a nuisance:
> 
> i915_gem_object_track_fb()
> 
> The key part is that is operates on the object. The other is just to try
> and shorten the name as compensation.

Hm, I've thought the i915_gem part is a giveaway - I'm not too fond of the
i915_gem_obj prefix since it's so long ...

> > @@ -1062,6 +1065,7 @@ intel_disable_plane(struct drm_plane *plane)
> >  	struct drm_device *dev = plane->dev;
> >  	struct intel_plane *intel_plane = to_intel_plane(plane);
> >  	struct intel_crtc *intel_crtc;
> > +	enum pipe pipe;
> >  
> >  	if (!plane->fb)
> >  		return 0;
> > @@ -1070,6 +1074,7 @@ intel_disable_plane(struct drm_plane *plane)
> >  		return -EINVAL;
> >  
> >  	intel_crtc = to_intel_crtc(plane->crtc);
> > +	pipe = intel_crtc->pipe;
> >  
> >  	if (intel_crtc->active) {
> >  		bool primary_was_enabled = intel_crtc->primary_enabled;
> > @@ -1088,6 +1093,8 @@ intel_disable_plane(struct drm_plane *plane)
> >  
> >  		mutex_lock(&dev->struct_mutex);
> >  		intel_unpin_fb_obj(intel_plane->obj);
> > +		i915_gem_update_fb_bits(intel_plane->obj, NULL,
> > +					INTEL_FRONTBUFFER_SPRITE(pipe));
> 
> Introducing a local here for a one off, or was checkpatch in an angry
> mood with INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe) ?

Just a case of shortening the line a bit, it looks terribly ugly imo. I
hope we can prettify this once we have a bit more unified nuclear flip
logic for all planes.

> That's all I spotted, the logic looks fine. But just remind me,
> i915_gem_object_track_fb() did do a lockdep_assert_held(&dev->struct_mutex)?

Yeah, both when new or old are non-NULL, as a prep for per-object locking.
If we ever get there ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Introduce accurate frontbuffer tracking

[Chris Wilson]

On Wed, Jun 18, 2014 at 05:55:31PM +0200, Daniel Vetter wrote:
> On Wed, Jun 18, 2014 at 03:55:44PM +0100, Chris Wilson wrote:
> > On Wed, Jun 18, 2014 at 03:01:25PM +0200, Daniel Vetter wrote:
> > > +void i915_gem_update_fb_bits(struct drm_i915_gem_object *old,
> > > +			     struct drm_i915_gem_object *new,
> > > +			     unsigned frontbuffer_bits);
> > > +
> > 
> > Time to be a nuisance:
> > 
> > i915_gem_object_track_fb()
> > 
> > The key part is that is operates on the object. The other is just to try
> > and shorten the name as compensation.
> 
> Hm, I've thought the i915_gem part is a giveaway - I'm not too fond of the
> i915_gem_obj prefix since it's so long ...

Until we make the wholehearted change, stick to convention.

i915_bo_* coccinelle script?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Introduce accurate frontbuffer tracking

[Daniel Vetter]

On Wed, Jun 18, 2014 at 04:58:54PM +0100, Chris Wilson wrote:
> On Wed, Jun 18, 2014 at 05:55:31PM +0200, Daniel Vetter wrote:
> > On Wed, Jun 18, 2014 at 03:55:44PM +0100, Chris Wilson wrote:
> > > On Wed, Jun 18, 2014 at 03:01:25PM +0200, Daniel Vetter wrote:
> > > > +void i915_gem_update_fb_bits(struct drm_i915_gem_object *old,
> > > > +			     struct drm_i915_gem_object *new,
> > > > +			     unsigned frontbuffer_bits);
> > > > +
> > > 
> > > Time to be a nuisance:
> > > 
> > > i915_gem_object_track_fb()
> > > 
> > > The key part is that is operates on the object. The other is just to try
> > > and shorten the name as compensation.
> > 
> > Hm, I've thought the i915_gem part is a giveaway - I'm not too fond of the
> > i915_gem_obj prefix since it's so long ...
> 
> Until we make the wholehearted change, stick to convention.

Well object or objects? It operates on both after all since often we need
both so separate set/clear seemed too noisy.

> i915_bo_* coccinelle script?

cocci can't really do that, sed seems better for that.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Introduce accurate frontbuffer tracking

[Chris Wilson]

On Wed, Jun 18, 2014 at 06:05:02PM +0200, Daniel Vetter wrote:
> On Wed, Jun 18, 2014 at 04:58:54PM +0100, Chris Wilson wrote:
> > On Wed, Jun 18, 2014 at 05:55:31PM +0200, Daniel Vetter wrote:
> > > On Wed, Jun 18, 2014 at 03:55:44PM +0100, Chris Wilson wrote:
> > > > On Wed, Jun 18, 2014 at 03:01:25PM +0200, Daniel Vetter wrote:
> > > > > +void i915_gem_update_fb_bits(struct drm_i915_gem_object *old,
> > > > > +			     struct drm_i915_gem_object *new,
> > > > > +			     unsigned frontbuffer_bits);
> > > > > +
> > > > 
> > > > Time to be a nuisance:
> > > > 
> > > > i915_gem_object_track_fb()
> > > > 
> > > > The key part is that is operates on the object. The other is just to try
> > > > and shorten the name as compensation.
> > > 
> > > Hm, I've thought the i915_gem part is a giveaway - I'm not too fond of the
> > > i915_gem_obj prefix since it's so long ...
> > 
> > Until we make the wholehearted change, stick to convention.
> 
> Well object or objects? It operates on both after all since often we need
> both so separate set/clear seemed too noisy.

Oops. Yes it does, just ignore this subthread, it is indeed
i915_gem_track_fb(). (Not keen on bits, it's too generic in English.)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: Introduce accurate frontbuffer tracking

[Daniel Vetter]

So from just a quick look we seem to have enough information to
accurately figure out whether a given gem bo is used as a frontbuffer
and where exactly: We have obj->pin_count as a first check with no
false negatives and only negligible false positives. And then we can
just walk the modeset objects and figure out where exactly a buffer is
used as scanout.

Except that we can't due to locking order: If we already hold
dev->struct_mutex we can't acquire any modeset locks, so could
potential chase freed pointers and other evil stuff.

So we need something else. For that introduce a new set of bits
obj->frontbuffer_bits to track where a buffer object is used. That we
can then chase without grabbing any modeset locks.

Of course the consumers of this (DRRS, PSR, FBC, ...) still need to be
able to do their magic both when called from modeset and from gem
code. But that can be easily achieved by adding locks for these
specific subsystems which always nest within either kms or gem
locking.

This patch just adds the relevant update code to all places.

Note that if we ever support multi-planar scanout targets then we need
one frontbuffer tracking bit per attachment point that we expose to
userspace.

v2:
- Fix more oopsen. Oops.
- WARN if we leak obj->frontbuffer_bits when freeing a gem buffer. Fix
  the bugs this brought to light.
- s/update_frontbuffer_bits/update_fb_bits/. More consistent with the
  fb tracking functions (fb for gem object, frontbuffer for raw bits).
  And the function name was way too long.

v3: Size obj->frontbuffer_bits correctly so that all pipes fit in.

v4: Don't update fb bits in set_base on failure. Noticed by Chris.

v5: s/i915_gem_update_fb_bits/i915_gem_track_fb/ Also remove a few
local enum pipe variables which are now no longer needed to make the
function arguments no drop over the 80 char limit.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h      | 26 +++++++++++++
 drivers/gpu/drm/i915/i915_gem.c      | 19 +++++++++
 drivers/gpu/drm/i915/intel_display.c | 75 ++++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_overlay.c | 10 ++++-
 drivers/gpu/drm/i915/intel_sprite.c  |  7 ++++
 5 files changed, 115 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b6195ea334e6..cc9bfaee1764 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1595,6 +1595,26 @@ struct drm_i915_gem_object_ops {
 	void (*release)(struct drm_i915_gem_object *);
 };
 
+/*
+ * Frontbuffer tracking bits. Set in obj->frontbuffer_bits while a gem bo is
+ * considered to be the frontbuffer for the given plane interface-vise. This
+ * doesn't mean that the hw necessarily already scans it out, but that any
+ * rendering (by the cpu or gpu) will land in the frontbuffer eventually.
+ *
+ * We have one bit per pipe and per scanout plane type.
+ */
+#define INTEL_FRONTBUFFER_BITS_PER_PIPE 4
+#define INTEL_FRONTBUFFER_BITS \
+	(INTEL_FRONTBUFFER_BITS_PER_PIPE * I915_MAX_PIPES)
+#define INTEL_FRONTBUFFER_PRIMARY(pipe) \
+	(1 << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
+#define INTEL_FRONTBUFFER_CURSOR(pipe) \
+	(1 << (1 +(INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe))))
+#define INTEL_FRONTBUFFER_SPRITE(pipe) \
+	(1 << (2 +(INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe))))
+#define INTEL_FRONTBUFFER_OVERLAY(pipe) \
+	(1 << (3 +(INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe))))
+
 struct drm_i915_gem_object {
 	struct drm_gem_object base;
 
@@ -1682,6 +1702,8 @@ struct drm_i915_gem_object {
 	unsigned int has_global_gtt_mapping:1;
 	unsigned int has_dma_mapping:1;
 
+	unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS;
+
 	struct sg_table *pages;
 	int pages_pin_count;
 
@@ -1728,6 +1750,10 @@ struct drm_i915_gem_object {
 };
 #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base)
 
+void i915_gem_track_fb(struct drm_i915_gem_object *old,
+		       struct drm_i915_gem_object *new,
+		       unsigned frontbuffer_bits);
+
 /**
  * Request queue structure.
  *
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7d7d52bc922a..08a524c19c01 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4449,6 +4449,8 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
 	if (obj->stolen)
 		i915_gem_object_unpin_pages(obj);
 
+	WARN_ON(obj->frontbuffer_bits);
+
 	if (WARN_ON(obj->pages_pin_count))
 		obj->pages_pin_count = 0;
 	if (discard_backing_storage(obj))
@@ -4993,6 +4995,23 @@ int i915_gem_open(struct drm_device *dev, struct drm_file *file)
 	return ret;
 }
 
+void i915_gem_track_fb(struct drm_i915_gem_object *old,
+		       struct drm_i915_gem_object *new,
+		       unsigned frontbuffer_bits)
+{
+	if (old) {
+		WARN_ON(!mutex_is_locked(&old->base.dev->struct_mutex));
+		WARN_ON(!(old->frontbuffer_bits & frontbuffer_bits));
+		old->frontbuffer_bits &= ~frontbuffer_bits;
+	}
+
+	if (new) {
+		WARN_ON(!mutex_is_locked(&new->base.dev->struct_mutex));
+		WARN_ON(new->frontbuffer_bits & frontbuffer_bits);
+		new->frontbuffer_bits |= frontbuffer_bits;
+	}
+}
+
 static bool mutex_is_locked_by(struct mutex *mutex, struct task_struct *task)
 {
 	if (!mutex_is_locked(mutex))
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 548161d9ac8a..ae24829f799f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2351,6 +2351,7 @@ static bool intel_alloc_plane_obj(struct intel_crtc *crtc,
 		goto out_unref_obj;
 	}
 
+	obj->frontbuffer_bits = INTEL_FRONTBUFFER_PRIMARY(crtc->pipe);
 	mutex_unlock(&dev->struct_mutex);
 
 	DRM_DEBUG_KMS("plane fb obj %p\n", obj);
@@ -2396,6 +2397,7 @@ static void intel_find_plane_obj(struct intel_crtc *intel_crtc,
 		if (i915_gem_obj_ggtt_offset(fb->obj) == plane_config->base) {
 			drm_framebuffer_reference(c->primary->fb);
 			intel_crtc->base.primary->fb = c->primary->fb;
+			fb->obj->frontbuffer_bits |= INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
 			break;
 		}
 	}
@@ -2684,7 +2686,9 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	enum pipe pipe = intel_crtc->pipe;
 	struct drm_framebuffer *old_fb;
+	struct drm_i915_gem_object *obj = to_intel_framebuffer(fb)->obj;
 	int ret;
 
 	if (intel_crtc_has_pending_flip(crtc)) {
@@ -2705,10 +2709,13 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 		return -EINVAL;
 	}
 
+	old_fb = crtc->primary->fb;
+
 	mutex_lock(&dev->struct_mutex);
-	ret = intel_pin_and_fence_fb_obj(dev,
-					 to_intel_framebuffer(fb)->obj,
-					 NULL);
+	ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
+	if (ret == 0)
+		i915_gem_track_fb(to_intel_framebuffer(old_fb)->obj, obj,
+				  INTEL_FRONTBUFFER_PRIMARY(pipe));
 	mutex_unlock(&dev->struct_mutex);
 	if (ret != 0) {
 		DRM_ERROR("pin & fence failed\n");
@@ -2748,7 +2755,6 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 
 	dev_priv->display.update_primary_plane(crtc, fb, x, y);
 
-	old_fb = crtc->primary->fb;
 	crtc->primary->fb = fb;
 	crtc->x = x;
 	crtc->y = y;
@@ -4922,6 +4928,8 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
 	struct drm_device *dev = crtc->dev;
 	struct drm_connector *connector;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_gem_object *old_obj;
+	enum pipe pipe = to_intel_crtc(crtc)->pipe;
 
 	/* crtc should still be enabled when we disable it. */
 	WARN_ON(!crtc->enabled);
@@ -4931,12 +4939,15 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
 	dev_priv->display.off(crtc);
 
 	assert_plane_disabled(dev->dev_private, to_intel_crtc(crtc)->plane);
-	assert_cursor_disabled(dev_priv, to_intel_crtc(crtc)->pipe);
-	assert_pipe_disabled(dev->dev_private, to_intel_crtc(crtc)->pipe);
+	assert_cursor_disabled(dev_priv, pipe);
+	assert_pipe_disabled(dev->dev_private, pipe);
 
 	if (crtc->primary->fb) {
+		old_obj = to_intel_framebuffer(crtc->primary->fb)->obj;
 		mutex_lock(&dev->struct_mutex);
-		intel_unpin_fb_obj(to_intel_framebuffer(crtc->primary->fb)->obj);
+		intel_unpin_fb_obj(old_obj);
+		i915_gem_track_fb(old_obj, NULL,
+				  INTEL_FRONTBUFFER_PRIMARY(pipe));
 		mutex_unlock(&dev->struct_mutex);
 		crtc->primary->fb = NULL;
 	}
@@ -8103,6 +8114,7 @@ static int intel_crtc_cursor_set_obj(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);
+	enum pipe pipe = intel_crtc->pipe;
 	unsigned old_width;
 	uint32_t addr;
 	int ret;
@@ -8182,6 +8194,8 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
 	}
 
+	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
+			  INTEL_FRONTBUFFER_CURSOR(pipe));
 	mutex_unlock(&dev->struct_mutex);
 
 	old_width = intel_crtc->cursor_width;
@@ -9404,6 +9418,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	struct drm_framebuffer *old_fb = crtc->primary->fb;
 	struct drm_i915_gem_object *obj = to_intel_framebuffer(fb)->obj;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	enum pipe pipe = intel_crtc->pipe;
 	struct intel_unpin_work *work;
 	struct intel_engine_cs *ring;
 	unsigned long flags;
@@ -9475,7 +9490,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	intel_crtc->reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
 
 	if (INTEL_INFO(dev)->gen >= 5 || IS_G4X(dev))
-		work->flip_count = I915_READ(PIPE_FLIPCOUNT_GM45(intel_crtc->pipe)) + 1;
+		work->flip_count = I915_READ(PIPE_FLIPCOUNT_GM45(pipe)) + 1;
 
 	if (IS_VALLEYVIEW(dev)) {
 		ring = &dev_priv->ring[BCS];
@@ -9503,6 +9518,9 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	if (ret)
 		goto cleanup_unpin;
 
+	i915_gem_track_fb(work->old_fb_obj, obj,
+			  INTEL_FRONTBUFFER_PRIMARY(pipe));
+
 	intel_disable_fbc(dev);
 	intel_mark_fb_busy(obj, NULL);
 	mutex_unlock(&dev->struct_mutex);
@@ -9534,7 +9552,7 @@ out_hang:
 		intel_crtc_wait_for_pending_flips(crtc);
 		ret = intel_pipe_set_base(crtc, crtc->x, crtc->y, fb);
 		if (ret == 0 && event)
-			drm_send_vblank_event(dev, intel_crtc->pipe, event);
+			drm_send_vblank_event(dev, pipe, event);
 	}
 	return ret;
 }
@@ -10569,10 +10587,13 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 	 */
 	for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
 		struct drm_framebuffer *old_fb;
+		struct drm_i915_gem_object *old_obj = NULL;
+		struct drm_i915_gem_object *obj =
+			to_intel_framebuffer(fb)->obj;
 
 		mutex_lock(&dev->struct_mutex);
 		ret = intel_pin_and_fence_fb_obj(dev,
-						 to_intel_framebuffer(fb)->obj,
+						 obj,
 						 NULL);
 		if (ret != 0) {
 			DRM_ERROR("pin & fence failed\n");
@@ -10580,8 +10601,12 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 			goto done;
 		}
 		old_fb = crtc->primary->fb;
-		if (old_fb)
-			intel_unpin_fb_obj(to_intel_framebuffer(old_fb)->obj);
+		if (old_fb) {
+			old_obj = to_intel_framebuffer(old_fb)->obj;
+			intel_unpin_fb_obj(old_obj);
+		}
+		i915_gem_track_fb(old_obj, obj,
+				  INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));
 		mutex_unlock(&dev->struct_mutex);
 
 		crtc->primary->fb = fb;
@@ -11196,8 +11221,9 @@ intel_primary_plane_disable(struct drm_plane *plane)
 	intel_crtc_wait_for_pending_flips(plane->crtc);
 	intel_disable_primary_hw_plane(dev_priv, intel_plane->plane,
 				       intel_plane->pipe);
-
 disable_unpin:
+	i915_gem_track_fb(to_intel_framebuffer(plane->fb)->obj, NULL,
+			  INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));
 	intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
 	plane->fb = NULL;
 
@@ -11215,6 +11241,7 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct drm_i915_gem_object *obj, *old_obj = NULL;
 	struct drm_rect dest = {
 		/* integer pixels */
 		.x1 = crtc_x,
@@ -11246,6 +11273,10 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (ret)
 		return ret;
 
+	if (plane->fb)
+		old_obj = to_intel_framebuffer(plane->fb)->obj;
+	obj = to_intel_framebuffer(fb)->obj;
+
 	/*
 	 * If the CRTC isn't enabled, we're just pinning the framebuffer,
 	 * updating the fb pointer, and returning without touching the
@@ -11258,12 +11289,13 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 		 * we may have an fb pinned; unpin it.
 		 */
 		if (plane->fb)
-			intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
+			intel_unpin_fb_obj(old_obj);
+
+		i915_gem_track_fb(old_obj, obj,
+				  INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));
 
 		/* Pin and return without programming hardware */
-		return intel_pin_and_fence_fb_obj(dev,
-						  to_intel_framebuffer(fb)->obj,
-						  NULL);
+		return intel_pin_and_fence_fb_obj(dev, obj, NULL);
 	}
 
 	intel_crtc_wait_for_pending_flips(crtc);
@@ -11280,13 +11312,14 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 		 * fail.
 		 */
 		if (plane->fb != fb) {
-			ret = intel_pin_and_fence_fb_obj(dev,
-							 to_intel_framebuffer(fb)->obj,
-							 NULL);
+			ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
 			if (ret)
 				return ret;
 		}
 
+		i915_gem_track_fb(old_obj, obj,
+				  INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));
+
 		if (intel_crtc->primary_enabled)
 			intel_disable_primary_hw_plane(dev_priv,
 						       intel_plane->plane,
@@ -11295,7 +11328,7 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 
 		if (plane->fb != fb)
 			if (plane->fb)
-				intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
+				intel_unpin_fb_obj(old_obj);
 
 		return 0;
 	}
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index daa118978eec..99b6c142a095 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -415,6 +415,10 @@ static int intel_overlay_release_old_vid(struct intel_overlay *overlay)
 	}
 
 	intel_overlay_release_old_vid_tail(overlay);
+
+
+	i915_gem_track_fb(overlay->old_vid_bo, NULL,
+			  INTEL_FRONTBUFFER_OVERLAY(overlay->crtc->pipe));
 	return 0;
 }
 
@@ -686,6 +690,7 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
 	bool scale_changed = false;
 	struct drm_device *dev = overlay->dev;
 	u32 swidth, swidthsw, sheight, ostride;
+	enum pipe pipe = overlay->crtc->pipe;
 
 	BUG_ON(!mutex_is_locked(&dev->struct_mutex));
 	BUG_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
@@ -713,7 +718,7 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
 		oconfig = OCONF_CC_OUT_8BIT;
 		if (IS_GEN4(overlay->dev))
 			oconfig |= OCONF_CSC_MODE_BT709;
-		oconfig |= overlay->crtc->pipe == 0 ?
+		oconfig |= pipe == 0 ?
 			OCONF_PIPE_A : OCONF_PIPE_B;
 		iowrite32(oconfig, &regs->OCONFIG);
 		intel_overlay_unmap_regs(overlay, regs);
@@ -776,6 +781,9 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
 	if (ret)
 		goto out_unpin;
 
+	i915_gem_track_fb(overlay->vid_bo, new_bo,
+			  INTEL_FRONTBUFFER_OVERLAY(pipe));
+
 	overlay->old_vid_bo = overlay->vid_bo;
 	overlay->vid_bo = new_bo;
 
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 9038e2ab73c8..140bd8359f0e 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -811,6 +811,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = plane->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	enum pipe pipe = intel_crtc->pipe;
 	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
 	struct drm_i915_gem_object *obj = intel_fb->obj;
 	struct drm_i915_gem_object *old_obj = intel_plane->obj;
@@ -998,6 +999,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	 */
 	ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
 
+	i915_gem_track_fb(old_obj, obj,
+			  INTEL_FRONTBUFFER_SPRITE(pipe));
 	mutex_unlock(&dev->struct_mutex);
 
 	if (ret)
@@ -1062,6 +1065,7 @@ intel_disable_plane(struct drm_plane *plane)
 	struct drm_device *dev = plane->dev;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct intel_crtc *intel_crtc;
+	enum pipe pipe;
 
 	if (!plane->fb)
 		return 0;
@@ -1070,6 +1074,7 @@ intel_disable_plane(struct drm_plane *plane)
 		return -EINVAL;
 
 	intel_crtc = to_intel_crtc(plane->crtc);
+	pipe = intel_crtc->pipe;
 
 	if (intel_crtc->active) {
 		bool primary_was_enabled = intel_crtc->primary_enabled;
@@ -1088,6 +1093,8 @@ intel_disable_plane(struct drm_plane *plane)
 
 		mutex_lock(&dev->struct_mutex);
 		intel_unpin_fb_obj(intel_plane->obj);
+		i915_gem_track_fb(intel_plane->obj, NULL,
+				  INTEL_FRONTBUFFER_SPRITE(pipe));
 		mutex_unlock(&dev->struct_mutex);
 
 		intel_plane->obj = NULL;
-- 
2.0.0

Re: [PATCH] drm/i915: Introduce accurate frontbuffer tracking

[Chris Wilson]

On Wed, Jun 18, 2014 at 11:28:09PM +0200, Daniel Vetter wrote:
> So from just a quick look we seem to have enough information to
> accurately figure out whether a given gem bo is used as a frontbuffer
> and where exactly: We have obj->pin_count as a first check with no
> false negatives and only negligible false positives. And then we can
> just walk the modeset objects and figure out where exactly a buffer is
> used as scanout.
> 
> Except that we can't due to locking order: If we already hold
> dev->struct_mutex we can't acquire any modeset locks, so could
> potential chase freed pointers and other evil stuff.
> 
> So we need something else. For that introduce a new set of bits
> obj->frontbuffer_bits to track where a buffer object is used. That we
> can then chase without grabbing any modeset locks.
> 
> Of course the consumers of this (DRRS, PSR, FBC, ...) still need to be
> able to do their magic both when called from modeset and from gem
> code. But that can be easily achieved by adding locks for these
> specific subsystems which always nest within either kms or gem
> locking.
> 
> This patch just adds the relevant update code to all places.
> 
> Note that if we ever support multi-planar scanout targets then we need
> one frontbuffer tracking bit per attachment point that we expose to
> userspace.
> 
> v2:
> - Fix more oopsen. Oops.
> - WARN if we leak obj->frontbuffer_bits when freeing a gem buffer. Fix
>   the bugs this brought to light.
> - s/update_frontbuffer_bits/update_fb_bits/. More consistent with the
>   fb tracking functions (fb for gem object, frontbuffer for raw bits).
>   And the function name was way too long.
> 
> v3: Size obj->frontbuffer_bits correctly so that all pipes fit in.
> 
> v4: Don't update fb bits in set_base on failure. Noticed by Chris.
> 
> v5: s/i915_gem_update_fb_bits/i915_gem_track_fb/ Also remove a few
> local enum pipe variables which are now no longer needed to make the
> function arguments no drop over the 80 char limit.

Now that you mention it, there are other places in this patch that could
do with a whitespace cleanup. ;)

> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: Properly track domain of the fbcon fb

[Daniel Vetter]

X could end up putting the fbcon fb into other domains, e.g.
for smooth take-overs. Also we want this for accurate frontbuffer
tracking: The set_config is an implicit flush and will re-enable
psr and similar features, so we need to bring the bo back into
the gtt domain.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 27975c3e21c5..53c50240d1c2 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -43,10 +43,29 @@
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 
+static int intel_fbdev_set_par(struct fb_info *info)
+{
+	struct drm_fb_helper *fb_helper = info->par;
+	struct intel_fbdev *ifbdev =
+		container_of(fb_helper, struct intel_fbdev, helper);
+	int ret;
+
+	ret = drm_fb_helper_set_par(info);
+
+	if (ret == 0) {
+		mutex_lock(&fb_helper->dev->struct_mutex);
+		ret = i915_gem_object_set_to_gtt_domain(ifbdev->fb->obj,
+							true);
+		mutex_unlock(&fb_helper->dev->struct_mutex);
+	}
+
+	return ret;
+}
+
 static struct fb_ops intelfb_ops = {
 	.owner = THIS_MODULE,
 	.fb_check_var = drm_fb_helper_check_var,
-	.fb_set_par = drm_fb_helper_set_par,
+	.fb_set_par = intel_fbdev_set_par,
 	.fb_fillrect = cfb_fillrect,
 	.fb_copyarea = cfb_copyarea,
 	.fb_imageblit = cfb_imageblit,
-- 
2.0.0

Re: [PATCH] drm/i915: Properly track domain of the fbcon fb

[Chris Wilson]

On Wed, Jun 18, 2014 at 03:05:19PM +0200, Daniel Vetter wrote:
> X could end up putting the fbcon fb into other domains, e.g.
> for smooth take-overs. Also we want this for accurate frontbuffer
> tracking: The set_config is an implicit flush and will re-enable
> psr and similar features, so we need to bring the bo back into
> the gtt domain.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 27975c3e21c5..53c50240d1c2 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -43,10 +43,29 @@
>  #include <drm/i915_drm.h>
>  #include "i915_drv.h"
>  
> +static int intel_fbdev_set_par(struct fb_info *info)
> +{
> +	struct drm_fb_helper *fb_helper = info->par;
> +	struct intel_fbdev *ifbdev =
> +		container_of(fb_helper, struct intel_fbdev, helper);
> +	int ret;
> +
> +	ret = drm_fb_helper_set_par(info);
> +
> +	if (ret == 0) {

Add a comment here saying you are happy with the potential deadlock,
double-fault and what not, and I'll accept it with provisons that I'll
still worry that this will explode in our faces.

> +		mutex_lock(&fb_helper->dev->struct_mutex);
> +		ret = i915_gem_object_set_to_gtt_domain(ifbdev->fb->obj,
> +							true);
> +		mutex_unlock(&fb_helper->dev->struct_mutex);
> +	}
> +
> +	return ret;
> +}
> +
>  static struct fb_ops intelfb_ops = {
>  	.owner = THIS_MODULE,
>  	.fb_check_var = drm_fb_helper_check_var,
> -	.fb_set_par = drm_fb_helper_set_par,
> +	.fb_set_par = intel_fbdev_set_par,
>  	.fb_fillrect = cfb_fillrect,
>  	.fb_copyarea = cfb_copyarea,
>  	.fb_imageblit = cfb_imageblit,
> -- 
> 2.0.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Properly track domain of the fbcon fb

[Daniel Vetter]

On Wed, Jun 18, 2014 at 03:57:49PM +0100, Chris Wilson wrote:
> On Wed, Jun 18, 2014 at 03:05:19PM +0200, Daniel Vetter wrote:
> > X could end up putting the fbcon fb into other domains, e.g.
> > for smooth take-overs. Also we want this for accurate frontbuffer
> > tracking: The set_config is an implicit flush and will re-enable
> > psr and similar features, so we need to bring the bo back into
> > the gtt domain.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_fbdev.c | 21 ++++++++++++++++++++-
> >  1 file changed, 20 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > index 27975c3e21c5..53c50240d1c2 100644
> > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > @@ -43,10 +43,29 @@
> >  #include <drm/i915_drm.h>
> >  #include "i915_drv.h"
> >  
> > +static int intel_fbdev_set_par(struct fb_info *info)
> > +{
> > +	struct drm_fb_helper *fb_helper = info->par;
> > +	struct intel_fbdev *ifbdev =
> > +		container_of(fb_helper, struct intel_fbdev, helper);
> > +	int ret;
> > +
> > +	ret = drm_fb_helper_set_par(info);
> > +
> > +	if (ret == 0) {
> 
> Add a comment here saying you are happy with the potential deadlock,
> double-fault and what not, and I'll accept it with provisons that I'll
> still worry that this will explode in our faces.

/*
 * FIXME: fbdev presumes that all callbacks also work from atomic contexts
 * and relies on that for emergency oops printing. KMS totally doesn't do
 * that and the locking here is by far not the only place this goes wrong.
 * Ignore this for now until we solve this for real.
 */

Like this?
-Daniel
> 
> > +		mutex_lock(&fb_helper->dev->struct_mutex);
> > +		ret = i915_gem_object_set_to_gtt_domain(ifbdev->fb->obj,
> > +							true);
> > +		mutex_unlock(&fb_helper->dev->struct_mutex);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  static struct fb_ops intelfb_ops = {
> >  	.owner = THIS_MODULE,
> >  	.fb_check_var = drm_fb_helper_check_var,
> > -	.fb_set_par = drm_fb_helper_set_par,
> > +	.fb_set_par = intel_fbdev_set_par,
> >  	.fb_fillrect = cfb_fillrect,
> >  	.fb_copyarea = cfb_copyarea,
> >  	.fb_imageblit = cfb_imageblit,
> > -- 
> > 2.0.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH] drm/i915: Properly track domain of the fbcon fb

[Chris Wilson]

On Wed, Jun 18, 2014 at 05:57:39PM +0200, Daniel Vetter wrote:
> On Wed, Jun 18, 2014 at 03:57:49PM +0100, Chris Wilson wrote:
> > On Wed, Jun 18, 2014 at 03:05:19PM +0200, Daniel Vetter wrote:
> > > X could end up putting the fbcon fb into other domains, e.g.
> > > for smooth take-overs. Also we want this for accurate frontbuffer
> > > tracking: The set_config is an implicit flush and will re-enable
> > > psr and similar features, so we need to bring the bo back into
> > > the gtt domain.
> > > 
> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> > >  drivers/gpu/drm/i915/intel_fbdev.c | 21 ++++++++++++++++++++-
> > >  1 file changed, 20 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > > index 27975c3e21c5..53c50240d1c2 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > > @@ -43,10 +43,29 @@
> > >  #include <drm/i915_drm.h>
> > >  #include "i915_drv.h"
> > >  
> > > +static int intel_fbdev_set_par(struct fb_info *info)
> > > +{
> > > +	struct drm_fb_helper *fb_helper = info->par;
> > > +	struct intel_fbdev *ifbdev =
> > > +		container_of(fb_helper, struct intel_fbdev, helper);
> > > +	int ret;
> > > +
> > > +	ret = drm_fb_helper_set_par(info);
> > > +
> > > +	if (ret == 0) {
> > 
> > Add a comment here saying you are happy with the potential deadlock,
> > double-fault and what not, and I'll accept it with provisons that I'll
> > still worry that this will explode in our faces.
> 
> /*
>  * FIXME: fbdev presumes that all callbacks also work from atomic contexts
>  * and relies on that for emergency oops printing. KMS totally doesn't do
>  * that and the locking here is by far not the only place this goes wrong.
>  * Ignore this for now until we solve this for real.
>  */
> 
> Like this?

That'll do. "Please report all hung systems to <daniel.vetter@ffwll.ch>" would be nice
as well. ;-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 12/16] drm/i915: Use new frontbuffer bits to increase pll clock

[Daniel Vetter]

The downclocking checks a few more things, so not that simple to
convert. Also, this should get unified with the drrs handling and also
use the locking of that. Otoh the drrs locking is about as hapzardous
as no locking, at least on first sight.

For easier conversion ditch the upclocking on unload - we'll turn off
everything anyway.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h      |  2 ++
 drivers/gpu/drm/i915/intel_display.c | 32 ++++++++++----------------------
 2 files changed, 12 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2ae2b96c371d..dcd8fd956ec7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1614,6 +1614,8 @@ struct drm_i915_gem_object_ops {
 	(1 << (2 +(INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe))))
 #define INTEL_FRONTBUFFER_OVERLAY(pipe) \
 	(1 << (3 +(INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe))))
+#define INTEL_FRONTBUFFER_ALL_MASK(pipe) \
+	(0xf << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
 
 struct drm_i915_gem_object {
 	struct drm_gem_object base;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 87d3be939488..e6a387169af2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -76,7 +76,8 @@ static const uint32_t intel_cursor_formats[] = {
 #define DIV_ROUND_CLOSEST_ULL(ll, d)	\
 ({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; })
 
-static void intel_increase_pllclock(struct drm_crtc *crtc);
+static void intel_increase_pllclock(struct drm_device *dev,
+				    enum pipe pipe);
 static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
 
 static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
@@ -2585,7 +2586,7 @@ intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 
 	if (dev_priv->display.disable_fbc)
 		dev_priv->display.disable_fbc(dev);
-	intel_increase_pllclock(crtc);
+	intel_increase_pllclock(dev, to_intel_crtc(crtc)->pipe);
 
 	dev_priv->display.update_primary_plane(crtc, fb, x, y);
 
@@ -8720,12 +8721,10 @@ struct drm_display_mode *intel_crtc_mode_get(struct drm_device *dev,
 	return mode;
 }
 
-static void intel_increase_pllclock(struct drm_crtc *crtc)
+static void intel_increase_pllclock(struct drm_device *dev,
+				    enum pipe pipe)
 {
-	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;
 	int dpll_reg = DPLL(pipe);
 	int dpll;
 
@@ -8831,21 +8830,19 @@ void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
 			struct intel_engine_cs *ring)
 {
 	struct drm_device *dev = obj->base.dev;
-	struct drm_crtc *crtc;
+	enum pipe pipe;
 
 	intel_edp_psr_exit(dev);
 
 	if (!i915.powersave)
 		return;
 
-	for_each_crtc(dev, crtc) {
-		if (!crtc->primary->fb)
-			continue;
-
-		if (to_intel_framebuffer(crtc->primary->fb)->obj != obj)
+	for_each_pipe(pipe) {
+		if (!(obj->frontbuffer_bits &
+		      INTEL_FRONTBUFFER_ALL_MASK(pipe)))
 			continue;
 
-		intel_increase_pllclock(crtc);
+		intel_increase_pllclock(dev, pipe);
 		if (ring && intel_fbc_enabled(dev))
 			ring->fbc_dirty = true;
 	}
@@ -12801,7 +12798,6 @@ void intel_connector_unregister(struct intel_connector *intel_connector)
 void intel_modeset_cleanup(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_crtc *crtc;
 	struct drm_connector *connector;
 
 	/*
@@ -12821,14 +12817,6 @@ void intel_modeset_cleanup(struct drm_device *dev)
 
 	intel_unregister_dsm_handler();
 
-	for_each_crtc(dev, crtc) {
-		/* Skip inactive CRTCs */
-		if (!crtc->primary->fb)
-			continue;
-
-		intel_increase_pllclock(crtc);
-	}
-
 	intel_disable_fbc(dev);
 
 	intel_disable_gt_powersave(dev);
-- 
2.0.0

Re: [PATCH 12/16] drm/i915: Use new frontbuffer bits to increase pll clock

[Chris Wilson]

On Wed, Jun 18, 2014 at 01:59:13PM +0200, Daniel Vetter wrote:
> The downclocking checks a few more things, so not that simple to
> convert. Also, this should get unified with the drrs handling and also
> use the locking of that. Otoh the drrs locking is about as hapzardous
> as no locking, at least on first sight.
> 
> For easier conversion ditch the upclocking on unload - we'll turn off
> everything anyway.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 13/16] drm/i915: Properly track domain of the fbcon fb

[Daniel Vetter]

X could end up putting the fbcon fb into other domains, e.g.
for smooth take-overs. Also we want this for accurate frontbuffer
tracking: The set_config is an implicit flush and will re-enable
psr and similar features, so we need to bring the bo back into
the gtt domain.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 27975c3e21c5..53c50240d1c2 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -43,10 +43,29 @@
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 
+static int intel_fbdev_set_par(struct fb_info *info)
+{
+	struct drm_fb_helper *fb_helper = info->par;
+	struct intel_fbdev *ifbdev =
+		container_of(fb_helper, struct intel_fbdev, helper);
+	int ret;
+
+	ret = drm_fb_helper_set_par(info);
+
+	if (ret == 0) {
+		mutex_lock(&fb_helper->dev->struct_mutex);
+		ret = i915_gem_object_set_to_gtt_domain(ifbdev->fb->obj,
+							true);
+		mutex_unlock(&fb_helper->dev->struct_mutex);
+	}
+
+	return ret;
+}
+
 static struct fb_ops intelfb_ops = {
 	.owner = THIS_MODULE,
 	.fb_check_var = drm_fb_helper_check_var,
-	.fb_set_par = drm_fb_helper_set_par,
+	.fb_set_par = intel_fbdev_set_par,
 	.fb_fillrect = cfb_fillrect,
 	.fb_copyarea = cfb_copyarea,
 	.fb_imageblit = cfb_imageblit,
-- 
2.0.0

Re: [PATCH 13/16] drm/i915: Properly track domain of the fbcon fb

[Chris Wilson]

On Wed, Jun 18, 2014 at 01:59:14PM +0200, Daniel Vetter wrote:
> X could end up putting the fbcon fb into other domains, e.g.
> for smooth take-overs. Also we want this for accurate frontbuffer
> tracking: The set_config is an implicit flush and will re-enable
> psr and similar features, so we need to bring the bo back into
> the gtt domain.

Is this possibly an atomic path? It would be nice to have a note on
fb_ops which were. But I remember having lots of in_atomic() handling
for fbdev acceleration (copied from nouveau).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 13/16] drm/i915: Properly track domain of the fbcon fb

[Daniel Vetter]

On Wed, Jun 18, 2014 at 01:10:32PM +0100, Chris Wilson wrote:
> On Wed, Jun 18, 2014 at 01:59:14PM +0200, Daniel Vetter wrote:
> > X could end up putting the fbcon fb into other domains, e.g.
> > for smooth take-overs. Also we want this for accurate frontbuffer
> > tracking: The set_config is an implicit flush and will re-enable
> > psr and similar features, so we need to bring the bo back into
> > the gtt domain.
> 
> Is this possibly an atomic path? It would be nice to have a note on
> fb_ops which were. But I remember having lots of in_atomic() handling
> for fbdev acceleration (copied from nouveau).

They are all callable from atomic, at least in Oopses. fbdev accel is
completely bonghits in that regard (imnsho) and I think the only option we
have is to block _any_ fbdev operation in atomic contexts from the start
and use David Herrmann's special last effort emergency logging support to
print the Oops. Even trying to make all this code work from atomic
contexts is imo a losing battle and a complete validation nightmare.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/i915: Properly track domain of the fbcon fb

[Daniel Vetter]

X could end up putting the fbcon fb into other domains, e.g.
for smooth take-overs. Also we want this for accurate frontbuffer
tracking: The set_config is an implicit flush and will re-enable
psr and similar features, so we need to bring the bo back into
the gtt domain.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 27975c3e21c5..53c50240d1c2 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -43,10 +43,29 @@
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 
+static int intel_fbdev_set_par(struct fb_info *info)
+{
+	struct drm_fb_helper *fb_helper = info->par;
+	struct intel_fbdev *ifbdev =
+		container_of(fb_helper, struct intel_fbdev, helper);
+	int ret;
+
+	ret = drm_fb_helper_set_par(info);
+
+	if (ret == 0) {
+		mutex_lock(&fb_helper->dev->struct_mutex);
+		ret = i915_gem_object_set_to_gtt_domain(ifbdev->fb->obj,
+							true);
+		mutex_unlock(&fb_helper->dev->struct_mutex);
+	}
+
+	return ret;
+}
+
 static struct fb_ops intelfb_ops = {
 	.owner = THIS_MODULE,
 	.fb_check_var = drm_fb_helper_check_var,
-	.fb_set_par = drm_fb_helper_set_par,
+	.fb_set_par = intel_fbdev_set_par,
 	.fb_fillrect = cfb_fillrect,
 	.fb_copyarea = cfb_copyarea,
 	.fb_imageblit = cfb_imageblit,
-- 
2.0.0

[PATCH 14/16] drm/i915: Track frontbuffer invalidation/flushing

[Daniel Vetter]

So these are the guts of the new beast. This tracks when a frontbuffer
gets invalidated (due to frontbuffer rendering) and hence should be
constantly scaned out, and when it's flushed again and can be
compressed/one-shot-upload.

Rules for flushing are simple: The frontbuffer needs one more full
upload starting from the next vblank. Which means that the flushing
can _only_ be called once the frontbuffer update has been latched.

But this poses a problem for pageflips: We can't just delay the
flushing until the pageflip is latched, since that would pose the risk
that we override frontbuffer rendering that has been scheduled
in-between the pageflip ioctl and the actual latching.

To handle this track asynchronous invalidations (and also pageflip)
state per-ring and delay any in-between flushing until the rendering
has completed. And also cancel any delayed flushing if we get a new
invalidation request (whether delayed or not).

Also call intel_mark_fb_busy in both cases in all cases to make sure
that we keep the screen at the highest refresh rate both on flips,
synchronous plane updates and for frontbuffer rendering.

v2: Lots of improvements

Suggestions from Chris:
- Move invalidate/flush in flush_*_domain and set_to_*_domain.
- Drop the flush in busy_ioctl since it's redundant. Was a leftover
  from an earlier concept to track flips/delayed flushes.
- Don't forget about the initial modeset enable/final disable.
  Suggested by Chris.

Track flips accurately, too. Since flips complete independently of
rendering we need to track pending flips in a separate mask. Again if
an invalidate happens we need to cancel the evenutal flush to avoid
races.

v3:
Provide correct header declarations for flip functions. Currently not
needed outside of intel_display.c, but part of the proper interface.

v4: Add proper domain management to fbcon so that the fbcon buffer is
also tracked correctly.

v5: Fixup locking around the fbcon set_to_gtt_domain call.

v6: More comments from Chris:
- Split out fbcon changes.
- Drop superflous checks for potential scanout before calling intel_fb
  functions - we can micro-optimize this later.
- s/intel_fb_/intel_fb_obj_/ to make it clear that this deals in gem
  object. We already have precedence for fb_obj in the pin_and_fence
  functions.

v7: Clarify the semantics of the flip flush handling by renaming
things a bit:
- Don't go through a gem object but take the relevant frontbuffer bits
  directly. These functions center on the plane, the actual object is
  irrelevant - even a flip to the same object as already active should
  cause a flush.
- Add a new intel_frontbuffer_flip for synchronous plane updates. It
  currently just calls intel_frontbuffer_flush since the implemenation
  differs.

This way we achieve a clear split between one-shot update events on
one side and frontbuffer rendering with potentially a very long delay
between the invalidate and flush.

Chris and I also had some discussions about mark_busy and whether it
is appropriate to call from flush. But mark busy is a state which
should be derived from the 3 events (invalidate, flush, flip) we now
have by the users, like psr does by tracking relevant information in
psr.busy_frontbuffer_bits. DRRS (the only real use of mark_busy for
frontbuffer) needs to have similar logic. With that the overall
mark_busy in the core could be removed.

v8: Only when retiring gpu buffers only flush frontbuffer bits we
actually invalidated in a batch. Just for safety since before any
additional usage/invalidate we should always retire current rendering.
Suggested by Chris Wilson.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h            |  14 +++
 drivers/gpu/drm/i915/i915_gem.c            |  18 ++-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   6 +-
 drivers/gpu/drm/i915/intel_display.c       | 180 +++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_drv.h           |  29 ++++-
 drivers/gpu/drm/i915/intel_overlay.c       |   3 +
 drivers/gpu/drm/i915/intel_sprite.c        |   4 +-
 7 files changed, 229 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index dcd8fd956ec7..0a309fe53911 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1332,6 +1332,17 @@ struct intel_pipe_crc {
 	wait_queue_head_t wq;
 };
 
+struct i915_frontbuffer_tracking {
+	struct mutex lock;
+
+	/*
+	 * Tracking bits for delayed frontbuffer flushing du to gpu activity or
+	 * scheduled flips.
+	 */
+	unsigned busy_bits;
+	unsigned flip_bits;
+};
+
 struct drm_i915_private {
 	struct drm_device *dev;
 	struct kmem_cache *slab;
@@ -1478,6 +1489,9 @@ struct drm_i915_private {
 	bool lvds_downclock_avail;
 	/* indicates the reduced downclock for LVDS*/
 	int lvds_downclock;
+
+	struct i915_frontbuffer_tracking fb_tracking;
+
 	u16 orig_clock;
 
 	bool mchbar_need_disable;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c40dd4ba2c27..23c4b0da3c09 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1395,8 +1395,6 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
 		goto unlock;
 	}
 
-	intel_edp_psr_exit(dev);
-
 	/* Try to flush the object off the GPU without holding the lock.
 	 * We will repeat the flush holding the lock in the normal manner
 	 * to catch cases where we are gazumped.
@@ -1442,8 +1440,6 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		return ret;
 
-	intel_edp_psr_exit(dev);
-
 	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
 	if (&obj->base == NULL) {
 		ret = -ENOENT;
@@ -2220,6 +2216,8 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
 			list_move_tail(&vma->mm_list, &vm->inactive_list);
 	}
 
+	intel_fb_obj_flush(obj, true);
+
 	list_del_init(&obj->ring_list);
 	obj->ring = NULL;
 
@@ -3549,6 +3547,8 @@ i915_gem_object_flush_gtt_write_domain(struct drm_i915_gem_object *obj)
 	old_write_domain = obj->base.write_domain;
 	obj->base.write_domain = 0;
 
+	intel_fb_obj_flush(obj, false);
+
 	trace_i915_gem_object_change_domain(obj,
 					    obj->base.read_domains,
 					    old_write_domain);
@@ -3570,6 +3570,8 @@ i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *obj,
 	old_write_domain = obj->base.write_domain;
 	obj->base.write_domain = 0;
 
+	intel_fb_obj_flush(obj, false);
+
 	trace_i915_gem_object_change_domain(obj,
 					    obj->base.read_domains,
 					    old_write_domain);
@@ -3623,6 +3625,8 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write)
 		obj->dirty = 1;
 	}
 
+	intel_fb_obj_invalidate(obj, NULL);
+
 	trace_i915_gem_object_change_domain(obj,
 					    old_read_domains,
 					    old_write_domain);
@@ -3959,6 +3963,8 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write)
 		obj->base.write_domain = I915_GEM_DOMAIN_CPU;
 	}
 
+	intel_fb_obj_invalidate(obj, NULL);
+
 	trace_i915_gem_object_change_domain(obj,
 					    old_read_domains,
 					    old_write_domain);
@@ -4233,8 +4239,6 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		return ret;
 
-	intel_edp_psr_exit(dev);
-
 	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
 	if (&obj->base == NULL) {
 		ret = -ENOENT;
@@ -4934,6 +4938,8 @@ i915_gem_load(struct drm_device *dev)
 
 	dev_priv->mm.oom_notifier.notifier_call = i915_gem_shrinker_oom;
 	register_oom_notifier(&dev_priv->mm.oom_notifier);
+
+	mutex_init(&dev_priv->fb_tracking.lock);
 }
 
 void i915_gem_release(struct drm_device *dev, struct drm_file *file)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 93d7f7246588..d815ef51a5ea 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -975,10 +975,8 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas,
 		if (obj->base.write_domain) {
 			obj->dirty = 1;
 			obj->last_write_seqno = intel_ring_get_seqno(ring);
-			/* check for potential scanout */
-			if (i915_gem_obj_ggtt_bound(obj) &&
-			    i915_gem_obj_to_ggtt(obj)->pin_count)
-				intel_mark_fb_busy(obj, ring);
+
+			intel_fb_obj_invalidate(obj, ring);
 
 			/* update for the implicit flush after a batch */
 			obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e6a387169af2..2361d03ea1f3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2755,6 +2755,8 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 
 	dev_priv->display.update_primary_plane(crtc, fb, x, y);
 
+	intel_frontbuffer_flush(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
+
 	crtc->primary->fb = fb;
 	crtc->x = x;
 	crtc->y = y;
@@ -3949,6 +3951,8 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
 	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
 	mutex_unlock(&dev->struct_mutex);
+
+	intel_frontbuffer_flush(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe));
 }
 
 static void intel_crtc_disable_planes(struct drm_crtc *crtc)
@@ -3971,6 +3975,8 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc)
 	intel_disable_planes(crtc);
 	intel_disable_primary_hw_plane(dev_priv, plane, pipe);
 
+	intel_frontbuffer_flush(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe));
+
 	drm_vblank_off(dev, pipe);
 }
 
@@ -8211,6 +8217,8 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
 	}
 
+	intel_frontbuffer_flush(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
+
 	return 0;
 fail_unpin:
 	i915_gem_object_unpin_from_display_plane(obj);
@@ -8826,20 +8834,26 @@ out:
 }
 
 
-void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
-			struct intel_engine_cs *ring)
+/**
+ * intel_mark_fb_busy - mark given planes as busy
+ * @dev: DRM device
+ * @frontbuffer_bits: bits for the affected planes
+ * @ring: optional ring for asynchronous commands
+ *
+ * This function gets called every time the screen contents change. It can be
+ * used to keep e.g. the update rate at the nominal refresh rate with DRRS.
+ */
+static void intel_mark_fb_busy(struct drm_device *dev,
+			       unsigned frontbuffer_bits,
+			       struct intel_engine_cs *ring)
 {
-	struct drm_device *dev = obj->base.dev;
 	enum pipe pipe;
 
-	intel_edp_psr_exit(dev);
-
 	if (!i915.powersave)
 		return;
 
 	for_each_pipe(pipe) {
-		if (!(obj->frontbuffer_bits &
-		      INTEL_FRONTBUFFER_ALL_MASK(pipe)))
+		if (!(frontbuffer_bits & INTEL_FRONTBUFFER_ALL_MASK(pipe)))
 			continue;
 
 		intel_increase_pllclock(dev, pipe);
@@ -8848,6 +8862,150 @@ void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
 	}
 }
 
+/**
+ * intel_fb_obj_invalidate - invalidate frontbuffer object
+ * @obj: GEM object to invalidate
+ * @ring: set for asynchronous rendering
+ *
+ * This function gets called every time rendering on the given object starts and
+ * frontbuffer caching (fbc, low refresh rate for DRRS, panel self refresh) must
+ * be invalidated. If @ring is non-NULL any subsequent invalidation will be delayed
+ * until the rendering completes or a flip on this frontbuffer plane is
+ * scheduled.
+ */
+void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
+			     struct intel_engine_cs *ring)
+{
+	struct drm_device *dev = obj->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
+	if (!obj->frontbuffer_bits)
+		return;
+
+	if (ring) {
+		mutex_lock(&dev_priv->fb_tracking.lock);
+		dev_priv->fb_tracking.busy_bits
+			|= obj->frontbuffer_bits;
+		dev_priv->fb_tracking.flip_bits
+			&= ~obj->frontbuffer_bits;
+		mutex_unlock(&dev_priv->fb_tracking.lock);
+	}
+
+	intel_mark_fb_busy(dev, obj->frontbuffer_bits, ring);
+
+	intel_edp_psr_exit(dev);
+}
+
+/**
+ * intel_frontbuffer_flush - flush frontbuffer
+ * @dev: DRM device
+ * @frontbuffer_bits: frontbuffer plane tracking bits
+ *
+ * This function gets called every time rendering on the given planes has
+ * completed and frontbuffer caching can be started again. Flushes will get
+ * delayed if they're blocked by some oustanding asynchronous rendering.
+ *
+ * Can be called without any locks held.
+ */
+void intel_frontbuffer_flush(struct drm_device *dev,
+			     unsigned frontbuffer_bits)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	/* Delay flushing when rings are still busy.*/
+	mutex_lock(&dev_priv->fb_tracking.lock);
+	frontbuffer_bits &= ~dev_priv->fb_tracking.busy_bits;
+	mutex_unlock(&dev_priv->fb_tracking.lock);
+
+	intel_mark_fb_busy(dev, frontbuffer_bits, NULL);
+
+	intel_edp_psr_exit(dev);
+}
+
+/**
+ * intel_fb_obj_flush - flush frontbuffer object
+ * @obj: GEM object to flush
+ * @retire: set when retiring asynchronous rendering
+ *
+ * This function gets called every time rendering on the given object has
+ * completed and frontbuffer caching can be started again. If @retire is true
+ * then any delayed flushes will be unblocked.
+ */
+void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
+			bool retire)
+{
+	struct drm_device *dev = obj->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	unsigned frontbuffer_bits;
+
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
+	if (!obj->frontbuffer_bits)
+		return;
+
+	frontbuffer_bits = obj->frontbuffer_bits;
+
+	if (retire) {
+		mutex_lock(&dev_priv->fb_tracking.lock);
+		/* Filter out new bits since rendering started. */
+		frontbuffer_bits &= dev_priv->fb_tracking.busy_bits;
+
+		dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
+		mutex_unlock(&dev_priv->fb_tracking.lock);
+	}
+
+	intel_frontbuffer_flush(dev, frontbuffer_bits);
+}
+
+/**
+ * intel_frontbuffer_flip_prepare - prepare asnychronous frontbuffer flip
+ * @dev: DRM device
+ * @frontbuffer_bits: frontbuffer plane tracking bits
+ *
+ * This function gets called after scheduling a flip on @obj. The actual
+ * frontbuffer flushing will be delayed until completion is signalled with
+ * intel_frontbuffer_flip_complete. If an invalidate happens in between this
+ * flush will be cancelled.
+ *
+ * Can be called without any locks held.
+ */
+void intel_frontbuffer_flip_prepare(struct drm_device *dev,
+				    unsigned frontbuffer_bits)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	mutex_lock(&dev_priv->fb_tracking.lock);
+	dev_priv->fb_tracking.flip_bits
+		|= frontbuffer_bits;
+	mutex_unlock(&dev_priv->fb_tracking.lock);
+}
+
+/**
+ * intel_frontbuffer_flip_complete - complete asynchronous frontbuffer flush
+ * @dev: DRM device
+ * @frontbuffer_bits: frontbuffer plane tracking bits
+ *
+ * This function gets called after the flip has been latched and will complete
+ * on the next vblank. It will execute the fush if it hasn't been cancalled yet.
+ *
+ * Can be called without any locks held.
+ */
+void intel_frontbuffer_flip_complete(struct drm_device *dev,
+				     unsigned frontbuffer_bits)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	mutex_lock(&dev_priv->fb_tracking.lock);
+	/* Mask any cancelled flips. */
+	frontbuffer_bits &= dev_priv->fb_tracking.flip_bits;
+	dev_priv->fb_tracking.flip_bits &= ~frontbuffer_bits;
+	mutex_unlock(&dev_priv->fb_tracking.lock);
+
+	intel_frontbuffer_flush(dev, frontbuffer_bits);
+}
+
 static void intel_crtc_destroy(struct drm_crtc *crtc)
 {
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -8875,6 +9033,7 @@ static void intel_unpin_work_fn(struct work_struct *__work)
 	struct intel_unpin_work *work =
 		container_of(__work, struct intel_unpin_work, work);
 	struct drm_device *dev = work->crtc->dev;
+	enum pipe pipe = to_intel_crtc(work->crtc)->pipe;
 
 	mutex_lock(&dev->struct_mutex);
 	intel_unpin_fb_obj(work->old_fb_obj);
@@ -8884,6 +9043,8 @@ static void intel_unpin_work_fn(struct work_struct *__work)
 	intel_update_fbc(dev);
 	mutex_unlock(&dev->struct_mutex);
 
+	intel_frontbuffer_flip_complete(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
+
 	BUG_ON(atomic_read(&to_intel_crtc(work->crtc)->unpin_work_count) == 0);
 	atomic_dec(&to_intel_crtc(work->crtc)->unpin_work_count);
 
@@ -9440,9 +9601,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	if (work == NULL)
 		return -ENOMEM;
 
-	/* Exit PSR early in page flip */
-	intel_edp_psr_exit(dev);
-
 	work->event = event;
 	work->crtc = crtc;
 	work->old_fb_obj = to_intel_framebuffer(old_fb)->obj;
@@ -9518,7 +9676,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 				INTEL_FRONTBUFFER_PRIMARY(pipe));
 
 	intel_disable_fbc(dev);
-	intel_mark_fb_busy(obj, NULL);
+	intel_frontbuffer_flip_prepare(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
 	mutex_unlock(&dev->struct_mutex);
 
 	trace_i915_flip_request(intel_crtc->plane, obj);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5d20f719309a..bd0d10eeaf44 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -724,8 +724,33 @@ bool intel_has_pending_fb_unpin(struct drm_device *dev);
 int intel_pch_rawclk(struct drm_device *dev);
 int valleyview_cur_cdclk(struct drm_i915_private *dev_priv);
 void intel_mark_busy(struct drm_device *dev);
-void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
-			struct intel_engine_cs *ring);
+void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
+			     struct intel_engine_cs *ring);
+void intel_frontbuffer_flip_prepare(struct drm_device *dev,
+				    unsigned frontbuffer_bits);
+void intel_frontbuffer_flip_complete(struct drm_device *dev,
+				     unsigned frontbuffer_bits);
+void intel_frontbuffer_flush(struct drm_device *dev,
+			     unsigned frontbuffer_bits);
+/**
+ * intel_frontbuffer_flip - prepare frontbuffer flip
+ * @dev: DRM device
+ * @frontbuffer_bits: frontbuffer plane tracking bits
+ *
+ * This function gets called after scheduling a flip on @obj. This is for
+ * synchronous plane updates which will happen on the next vblank and which will
+ * not get delayed by pending gpu rendering.
+ *
+ * Can be called without any locks held.
+ */
+static inline
+void intel_frontbuffer_flip(struct drm_device *dev,
+			    unsigned frontbuffer_bits)
+{
+	intel_frontbuffer_flush(dev, frontbuffer_bits);
+}
+
+void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire);
 void intel_mark_idle(struct drm_device *dev);
 void intel_crtc_restore_mode(struct drm_crtc *crtc);
 void intel_crtc_update_dpms(struct drm_crtc *crtc);
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 6aeda6a489a0..16e42071d56e 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -790,6 +790,9 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
 	overlay->old_vid_bo = overlay->vid_bo;
 	overlay->vid_bo = new_bo;
 
+	intel_frontbuffer_flush(dev,
+				INTEL_FRONTBUFFER_OVERLAY(pipe));
+
 	return 0;
 
 out_unpin:
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 3648594aeb7e..4d9c47368094 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1034,6 +1034,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		else
 			intel_plane->disable_plane(plane, crtc);
 
+		intel_frontbuffer_flush(dev, INTEL_FRONTBUFFER_SPRITE(pipe));
+
 		if (!primary_was_enabled && primary_enabled)
 			intel_post_enable_primary(crtc);
 	}
@@ -1054,8 +1056,6 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		mutex_unlock(&dev->struct_mutex);
 	}
 
-	intel_edp_psr_exit(dev);
-
 	return 0;
 }
 
-- 
2.0.0

Re: [PATCH 14/16] drm/i915: Track frontbuffer invalidation/flushing

[Chris Wilson]

On Wed, Jun 18, 2014 at 01:59:15PM +0200, Daniel Vetter wrote:
> Chris and I also had some discussions about mark_busy and whether it
> is appropriate to call from flush. But mark busy is a state which
> should be derived from the 3 events (invalidate, flush, flip) we now
> have by the users, like psr does by tracking relevant information in
> psr.busy_frontbuffer_bits. DRRS (the only real use of mark_busy for
> frontbuffer) needs to have similar logic. With that the overall
> mark_busy in the core could be removed.

Just as a reminder, Daniel has an addendum to put more of what we
discussed into practice.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: Track frontbuffer invalidation/flushing

[Daniel Vetter]

So these are the guts of the new beast. This tracks when a frontbuffer
gets invalidated (due to frontbuffer rendering) and hence should be
constantly scaned out, and when it's flushed again and can be
compressed/one-shot-upload.

Rules for flushing are simple: The frontbuffer needs one more full
upload starting from the next vblank. Which means that the flushing
can _only_ be called once the frontbuffer update has been latched.

But this poses a problem for pageflips: We can't just delay the
flushing until the pageflip is latched, since that would pose the risk
that we override frontbuffer rendering that has been scheduled
in-between the pageflip ioctl and the actual latching.

To handle this track asynchronous invalidations (and also pageflip)
state per-ring and delay any in-between flushing until the rendering
has completed. And also cancel any delayed flushing if we get a new
invalidation request (whether delayed or not).

Also call intel_mark_fb_busy in both cases in all cases to make sure
that we keep the screen at the highest refresh rate both on flips,
synchronous plane updates and for frontbuffer rendering.

v2: Lots of improvements

Suggestions from Chris:
- Move invalidate/flush in flush_*_domain and set_to_*_domain.
- Drop the flush in busy_ioctl since it's redundant. Was a leftover
  from an earlier concept to track flips/delayed flushes.
- Don't forget about the initial modeset enable/final disable.
  Suggested by Chris.

Track flips accurately, too. Since flips complete independently of
rendering we need to track pending flips in a separate mask. Again if
an invalidate happens we need to cancel the evenutal flush to avoid
races.

v3:
Provide correct header declarations for flip functions. Currently not
needed outside of intel_display.c, but part of the proper interface.

v4: Add proper domain management to fbcon so that the fbcon buffer is
also tracked correctly.

v5: Fixup locking around the fbcon set_to_gtt_domain call.

v6: More comments from Chris:
- Split out fbcon changes.
- Drop superflous checks for potential scanout before calling intel_fb
  functions - we can micro-optimize this later.
- s/intel_fb_/intel_fb_obj_/ to make it clear that this deals in gem
  object. We already have precedence for fb_obj in the pin_and_fence
  functions.

v7: Clarify the semantics of the flip flush handling by renaming
things a bit:
- Don't go through a gem object but take the relevant frontbuffer bits
  directly. These functions center on the plane, the actual object is
  irrelevant - even a flip to the same object as already active should
  cause a flush.
- Add a new intel_frontbuffer_flip for synchronous plane updates. It
  currently just calls intel_frontbuffer_flush since the implemenation
  differs.

This way we achieve a clear split between one-shot update events on
one side and frontbuffer rendering with potentially a very long delay
between the invalidate and flush.

Chris and I also had some discussions about mark_busy and whether it
is appropriate to call from flush. But mark busy is a state which
should be derived from the 3 events (invalidate, flush, flip) we now
have by the users, like psr does by tracking relevant information in
psr.busy_frontbuffer_bits. DRRS (the only real use of mark_busy for
frontbuffer) needs to have similar logic. With that the overall
mark_busy in the core could be removed.

v8: Only when retiring gpu buffers only flush frontbuffer bits we
actually invalidated in a batch. Just for safety since before any
additional usage/invalidate we should always retire current rendering.
Suggested by Chris Wilson.

v9: Actually use intel_frontbuffer_flip in all appropriate places.
Spotted by Chris.

v10: Address more comments from Chris:
- Don't call _flip in set_base when the crtc is inactive, avoids redunancy
  in the modeset case with the initial enabling of all planes.
- Add comments explaining that the initial/final plane enable/disable
  still has work left to do before it's fully generic.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h            |  14 +++
 drivers/gpu/drm/i915/i915_gem.c            |  18 ++-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   6 +-
 drivers/gpu/drm/i915/intel_display.c       | 191 +++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_drv.h           |  29 ++++-
 drivers/gpu/drm/i915/intel_overlay.c       |   3 +
 drivers/gpu/drm/i915/intel_sprite.c        |   4 +-
 7 files changed, 240 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4978b9deaeb4..b4e30bfd4d57 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1331,6 +1331,17 @@ struct intel_pipe_crc {
 	wait_queue_head_t wq;
 };
 
+struct i915_frontbuffer_tracking {
+	struct mutex lock;
+
+	/*
+	 * Tracking bits for delayed frontbuffer flushing du to gpu activity or
+	 * scheduled flips.
+	 */
+	unsigned busy_bits;
+	unsigned flip_bits;
+};
+
 struct drm_i915_private {
 	struct drm_device *dev;
 	struct kmem_cache *slab;
@@ -1477,6 +1488,9 @@ struct drm_i915_private {
 	bool lvds_downclock_avail;
 	/* indicates the reduced downclock for LVDS*/
 	int lvds_downclock;
+
+	struct i915_frontbuffer_tracking fb_tracking;
+
 	u16 orig_clock;
 
 	bool mchbar_need_disable;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index caed6621d71a..0a9c44bd9fd7 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1395,8 +1395,6 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
 		goto unlock;
 	}
 
-	intel_edp_psr_exit(dev);
-
 	/* Try to flush the object off the GPU without holding the lock.
 	 * We will repeat the flush holding the lock in the normal manner
 	 * to catch cases where we are gazumped.
@@ -1442,8 +1440,6 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		return ret;
 
-	intel_edp_psr_exit(dev);
-
 	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
 	if (&obj->base == NULL) {
 		ret = -ENOENT;
@@ -2223,6 +2219,8 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
 			list_move_tail(&vma->mm_list, &vm->inactive_list);
 	}
 
+	intel_fb_obj_flush(obj, true);
+
 	list_del_init(&obj->ring_list);
 	obj->ring = NULL;
 
@@ -3552,6 +3550,8 @@ i915_gem_object_flush_gtt_write_domain(struct drm_i915_gem_object *obj)
 	old_write_domain = obj->base.write_domain;
 	obj->base.write_domain = 0;
 
+	intel_fb_obj_flush(obj, false);
+
 	trace_i915_gem_object_change_domain(obj,
 					    obj->base.read_domains,
 					    old_write_domain);
@@ -3573,6 +3573,8 @@ i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *obj,
 	old_write_domain = obj->base.write_domain;
 	obj->base.write_domain = 0;
 
+	intel_fb_obj_flush(obj, false);
+
 	trace_i915_gem_object_change_domain(obj,
 					    obj->base.read_domains,
 					    old_write_domain);
@@ -3626,6 +3628,8 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write)
 		obj->dirty = 1;
 	}
 
+	intel_fb_obj_invalidate(obj, NULL);
+
 	trace_i915_gem_object_change_domain(obj,
 					    old_read_domains,
 					    old_write_domain);
@@ -3962,6 +3966,8 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write)
 		obj->base.write_domain = I915_GEM_DOMAIN_CPU;
 	}
 
+	intel_fb_obj_invalidate(obj, NULL);
+
 	trace_i915_gem_object_change_domain(obj,
 					    old_read_domains,
 					    old_write_domain);
@@ -4236,8 +4242,6 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		return ret;
 
-	intel_edp_psr_exit(dev);
-
 	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
 	if (&obj->base == NULL) {
 		ret = -ENOENT;
@@ -4937,6 +4941,8 @@ i915_gem_load(struct drm_device *dev)
 
 	dev_priv->mm.oom_notifier.notifier_call = i915_gem_shrinker_oom;
 	register_oom_notifier(&dev_priv->mm.oom_notifier);
+
+	mutex_init(&dev_priv->fb_tracking.lock);
 }
 
 void i915_gem_release(struct drm_device *dev, struct drm_file *file)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 93d7f7246588..d815ef51a5ea 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -975,10 +975,8 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas,
 		if (obj->base.write_domain) {
 			obj->dirty = 1;
 			obj->last_write_seqno = intel_ring_get_seqno(ring);
-			/* check for potential scanout */
-			if (i915_gem_obj_ggtt_bound(obj) &&
-			    i915_gem_obj_to_ggtt(obj)->pin_count)
-				intel_mark_fb_busy(obj, ring);
+
+			intel_fb_obj_invalidate(obj, ring);
 
 			/* update for the implicit flush after a batch */
 			obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d9e0c51388df..a7af8b58a73d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2756,6 +2756,9 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 
 	dev_priv->display.update_primary_plane(crtc, fb, x, y);
 
+	if (intel_crtc->active)
+		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
+
 	crtc->primary->fb = fb;
 	crtc->x = x;
 	crtc->y = y;
@@ -3950,6 +3953,13 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
 	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
 	mutex_unlock(&dev->struct_mutex);
+
+	/*
+	 * FIXME: Once we grow proper nuclear flip support out of this we need
+	 * to compute the mask of flip planes precisely. For the time being
+	 * consider this a flip from a NULL plane.
+	 */
+	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe));
 }
 
 static void intel_crtc_disable_planes(struct drm_crtc *crtc)
@@ -3972,6 +3982,13 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc)
 	intel_disable_planes(crtc);
 	intel_disable_primary_hw_plane(dev_priv, plane, pipe);
 
+	/*
+	 * FIXME: Once we grow proper nuclear flip support out of this we need
+	 * to compute the mask of flip planes precisely. For the time being
+	 * consider this a flip to a NULL plane.
+	 */
+	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe));
+
 	drm_vblank_off(dev, pipe);
 }
 
@@ -8212,6 +8229,8 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
 	}
 
+	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
+
 	return 0;
 fail_unpin:
 	i915_gem_object_unpin_from_display_plane(obj);
@@ -8827,20 +8846,26 @@ out:
 }
 
 
-void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
-			struct intel_engine_cs *ring)
+/**
+ * intel_mark_fb_busy - mark given planes as busy
+ * @dev: DRM device
+ * @frontbuffer_bits: bits for the affected planes
+ * @ring: optional ring for asynchronous commands
+ *
+ * This function gets called every time the screen contents change. It can be
+ * used to keep e.g. the update rate at the nominal refresh rate with DRRS.
+ */
+static void intel_mark_fb_busy(struct drm_device *dev,
+			       unsigned frontbuffer_bits,
+			       struct intel_engine_cs *ring)
 {
-	struct drm_device *dev = obj->base.dev;
 	enum pipe pipe;
 
-	intel_edp_psr_exit(dev);
-
 	if (!i915.powersave)
 		return;
 
 	for_each_pipe(pipe) {
-		if (!(obj->frontbuffer_bits &
-		      INTEL_FRONTBUFFER_ALL_MASK(pipe)))
+		if (!(frontbuffer_bits & INTEL_FRONTBUFFER_ALL_MASK(pipe)))
 			continue;
 
 		intel_increase_pllclock(dev, pipe);
@@ -8849,6 +8874,150 @@ void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
 	}
 }
 
+/**
+ * intel_fb_obj_invalidate - invalidate frontbuffer object
+ * @obj: GEM object to invalidate
+ * @ring: set for asynchronous rendering
+ *
+ * This function gets called every time rendering on the given object starts and
+ * frontbuffer caching (fbc, low refresh rate for DRRS, panel self refresh) must
+ * be invalidated. If @ring is non-NULL any subsequent invalidation will be delayed
+ * until the rendering completes or a flip on this frontbuffer plane is
+ * scheduled.
+ */
+void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
+			     struct intel_engine_cs *ring)
+{
+	struct drm_device *dev = obj->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
+	if (!obj->frontbuffer_bits)
+		return;
+
+	if (ring) {
+		mutex_lock(&dev_priv->fb_tracking.lock);
+		dev_priv->fb_tracking.busy_bits
+			|= obj->frontbuffer_bits;
+		dev_priv->fb_tracking.flip_bits
+			&= ~obj->frontbuffer_bits;
+		mutex_unlock(&dev_priv->fb_tracking.lock);
+	}
+
+	intel_mark_fb_busy(dev, obj->frontbuffer_bits, ring);
+
+	intel_edp_psr_exit(dev);
+}
+
+/**
+ * intel_frontbuffer_flush - flush frontbuffer
+ * @dev: DRM device
+ * @frontbuffer_bits: frontbuffer plane tracking bits
+ *
+ * This function gets called every time rendering on the given planes has
+ * completed and frontbuffer caching can be started again. Flushes will get
+ * delayed if they're blocked by some oustanding asynchronous rendering.
+ *
+ * Can be called without any locks held.
+ */
+void intel_frontbuffer_flush(struct drm_device *dev,
+			     unsigned frontbuffer_bits)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	/* Delay flushing when rings are still busy.*/
+	mutex_lock(&dev_priv->fb_tracking.lock);
+	frontbuffer_bits &= ~dev_priv->fb_tracking.busy_bits;
+	mutex_unlock(&dev_priv->fb_tracking.lock);
+
+	intel_mark_fb_busy(dev, frontbuffer_bits, NULL);
+
+	intel_edp_psr_exit(dev);
+}
+
+/**
+ * intel_fb_obj_flush - flush frontbuffer object
+ * @obj: GEM object to flush
+ * @retire: set when retiring asynchronous rendering
+ *
+ * This function gets called every time rendering on the given object has
+ * completed and frontbuffer caching can be started again. If @retire is true
+ * then any delayed flushes will be unblocked.
+ */
+void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
+			bool retire)
+{
+	struct drm_device *dev = obj->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	unsigned frontbuffer_bits;
+
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
+	if (!obj->frontbuffer_bits)
+		return;
+
+	frontbuffer_bits = obj->frontbuffer_bits;
+
+	if (retire) {
+		mutex_lock(&dev_priv->fb_tracking.lock);
+		/* Filter out new bits since rendering started. */
+		frontbuffer_bits &= dev_priv->fb_tracking.busy_bits;
+
+		dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
+		mutex_unlock(&dev_priv->fb_tracking.lock);
+	}
+
+	intel_frontbuffer_flush(dev, frontbuffer_bits);
+}
+
+/**
+ * intel_frontbuffer_flip_prepare - prepare asnychronous frontbuffer flip
+ * @dev: DRM device
+ * @frontbuffer_bits: frontbuffer plane tracking bits
+ *
+ * This function gets called after scheduling a flip on @obj. The actual
+ * frontbuffer flushing will be delayed until completion is signalled with
+ * intel_frontbuffer_flip_complete. If an invalidate happens in between this
+ * flush will be cancelled.
+ *
+ * Can be called without any locks held.
+ */
+void intel_frontbuffer_flip_prepare(struct drm_device *dev,
+				    unsigned frontbuffer_bits)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	mutex_lock(&dev_priv->fb_tracking.lock);
+	dev_priv->fb_tracking.flip_bits
+		|= frontbuffer_bits;
+	mutex_unlock(&dev_priv->fb_tracking.lock);
+}
+
+/**
+ * intel_frontbuffer_flip_complete - complete asynchronous frontbuffer flush
+ * @dev: DRM device
+ * @frontbuffer_bits: frontbuffer plane tracking bits
+ *
+ * This function gets called after the flip has been latched and will complete
+ * on the next vblank. It will execute the fush if it hasn't been cancalled yet.
+ *
+ * Can be called without any locks held.
+ */
+void intel_frontbuffer_flip_complete(struct drm_device *dev,
+				     unsigned frontbuffer_bits)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	mutex_lock(&dev_priv->fb_tracking.lock);
+	/* Mask any cancelled flips. */
+	frontbuffer_bits &= dev_priv->fb_tracking.flip_bits;
+	dev_priv->fb_tracking.flip_bits &= ~frontbuffer_bits;
+	mutex_unlock(&dev_priv->fb_tracking.lock);
+
+	intel_frontbuffer_flush(dev, frontbuffer_bits);
+}
+
 static void intel_crtc_destroy(struct drm_crtc *crtc)
 {
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -8876,6 +9045,7 @@ static void intel_unpin_work_fn(struct work_struct *__work)
 	struct intel_unpin_work *work =
 		container_of(__work, struct intel_unpin_work, work);
 	struct drm_device *dev = work->crtc->dev;
+	enum pipe pipe = to_intel_crtc(work->crtc)->pipe;
 
 	mutex_lock(&dev->struct_mutex);
 	intel_unpin_fb_obj(work->old_fb_obj);
@@ -8885,6 +9055,8 @@ static void intel_unpin_work_fn(struct work_struct *__work)
 	intel_update_fbc(dev);
 	mutex_unlock(&dev->struct_mutex);
 
+	intel_frontbuffer_flip_complete(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
+
 	BUG_ON(atomic_read(&to_intel_crtc(work->crtc)->unpin_work_count) == 0);
 	atomic_dec(&to_intel_crtc(work->crtc)->unpin_work_count);
 
@@ -9441,9 +9613,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	if (work == NULL)
 		return -ENOMEM;
 
-	/* Exit PSR early in page flip */
-	intel_edp_psr_exit(dev);
-
 	work->event = event;
 	work->crtc = crtc;
 	work->old_fb_obj = to_intel_framebuffer(old_fb)->obj;
@@ -9519,7 +9688,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 			  INTEL_FRONTBUFFER_PRIMARY(pipe));
 
 	intel_disable_fbc(dev);
-	intel_mark_fb_busy(obj, NULL);
+	intel_frontbuffer_flip_prepare(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
 	mutex_unlock(&dev->struct_mutex);
 
 	trace_i915_flip_request(intel_crtc->plane, obj);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5d20f719309a..bd0d10eeaf44 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -724,8 +724,33 @@ bool intel_has_pending_fb_unpin(struct drm_device *dev);
 int intel_pch_rawclk(struct drm_device *dev);
 int valleyview_cur_cdclk(struct drm_i915_private *dev_priv);
 void intel_mark_busy(struct drm_device *dev);
-void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
-			struct intel_engine_cs *ring);
+void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
+			     struct intel_engine_cs *ring);
+void intel_frontbuffer_flip_prepare(struct drm_device *dev,
+				    unsigned frontbuffer_bits);
+void intel_frontbuffer_flip_complete(struct drm_device *dev,
+				     unsigned frontbuffer_bits);
+void intel_frontbuffer_flush(struct drm_device *dev,
+			     unsigned frontbuffer_bits);
+/**
+ * intel_frontbuffer_flip - prepare frontbuffer flip
+ * @dev: DRM device
+ * @frontbuffer_bits: frontbuffer plane tracking bits
+ *
+ * This function gets called after scheduling a flip on @obj. This is for
+ * synchronous plane updates which will happen on the next vblank and which will
+ * not get delayed by pending gpu rendering.
+ *
+ * Can be called without any locks held.
+ */
+static inline
+void intel_frontbuffer_flip(struct drm_device *dev,
+			    unsigned frontbuffer_bits)
+{
+	intel_frontbuffer_flush(dev, frontbuffer_bits);
+}
+
+void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire);
 void intel_mark_idle(struct drm_device *dev);
 void intel_crtc_restore_mode(struct drm_crtc *crtc);
 void intel_crtc_update_dpms(struct drm_crtc *crtc);
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 99b6c142a095..1c002196ed47 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -787,6 +787,9 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
 	overlay->old_vid_bo = overlay->vid_bo;
 	overlay->vid_bo = new_bo;
 
+	intel_frontbuffer_flush(dev,
+				INTEL_FRONTBUFFER_OVERLAY(pipe));
+
 	return 0;
 
 out_unpin:
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 140bd8359f0e..0e3fd5c59e28 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1034,6 +1034,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		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);
 	}
@@ -1054,8 +1056,6 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		mutex_unlock(&dev->struct_mutex);
 	}
 
-	intel_edp_psr_exit(dev);
-
 	return 0;
 }
 
-- 
2.0.0

Re: [PATCH] drm/i915: Track frontbuffer invalidation/flushing

[Chris Wilson]

On Thu, Jun 19, 2014 at 02:41:27PM +0200, Daniel Vetter wrote:
> @@ -3626,6 +3628,8 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write)
>  		obj->dirty = 1;
>  	}
>  
> +	intel_fb_obj_invalidate(obj, NULL);
We should only invalidate when we start writing.

> +
>  	trace_i915_gem_object_change_domain(obj,
>  					    old_read_domains,
>  					    old_write_domain);
> @@ -3962,6 +3966,8 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write)
>  		obj->base.write_domain = I915_GEM_DOMAIN_CPU;
>  	}
>  
> +	intel_fb_obj_invalidate(obj, NULL);
Again only for writes. Now I think that this is skippable as the CPU
domain is explicitly incoherent with display, but for symmetry of
invalidate/flush it is required.

> +
>  	trace_i915_gem_object_change_domain(obj,
>  					    old_read_domains,
>  					    old_write_domain);

> +void intel_frontbuffer_flush(struct drm_device *dev,
> +			     unsigned frontbuffer_bits)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	/* Delay flushing when rings are still busy.*/
> +	mutex_lock(&dev_priv->fb_tracking.lock);
> +	frontbuffer_bits &= ~dev_priv->fb_tracking.busy_bits;
> +	mutex_unlock(&dev_priv->fb_tracking.lock);
> +
> +	intel_mark_fb_busy(dev, frontbuffer_bits, NULL);
> +
> +	intel_edp_psr_exit(dev);
Please tell me to plan to change this functions name. As we certainly
don't want to disable PSR here, but hopefully reenable it.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Track frontbuffer invalidation/flushing

[Daniel Vetter]

On Thu, Jun 19, 2014 at 02:02:26PM +0100, Chris Wilson wrote:
> On Thu, Jun 19, 2014 at 02:41:27PM +0200, Daniel Vetter wrote:
> > @@ -3626,6 +3628,8 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write)
> >  		obj->dirty = 1;
> >  	}
> >  
> > +	intel_fb_obj_invalidate(obj, NULL);
> We should only invalidate when we start writing.
> 
> > +
> >  	trace_i915_gem_object_change_domain(obj,
> >  					    old_read_domains,
> >  					    old_write_domain);
> > @@ -3962,6 +3966,8 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write)
> >  		obj->base.write_domain = I915_GEM_DOMAIN_CPU;
> >  	}
> >  
> > +	intel_fb_obj_invalidate(obj, NULL);
> Again only for writes. Now I think that this is skippable as the CPU
> domain is explicitly incoherent with display, but for symmetry of
> invalidate/flush it is required.

Good call, will fix up both places.
> 
> > +
> >  	trace_i915_gem_object_change_domain(obj,
> >  					    old_read_domains,
> >  					    old_write_domain);
> 
> > +void intel_frontbuffer_flush(struct drm_device *dev,
> > +			     unsigned frontbuffer_bits)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +	/* Delay flushing when rings are still busy.*/
> > +	mutex_lock(&dev_priv->fb_tracking.lock);
> > +	frontbuffer_bits &= ~dev_priv->fb_tracking.busy_bits;
> > +	mutex_unlock(&dev_priv->fb_tracking.lock);
> > +
> > +	intel_mark_fb_busy(dev, frontbuffer_bits, NULL);
> > +
> > +	intel_edp_psr_exit(dev);
> Please tell me to plan to change this functions name. As we certainly
> don't want to disable PSR here, but hopefully reenable it.

Yeah, the psr patch on top of this splits psr_exit into psr_invalidate and
psr_flush and generally dtrt.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/i915: Track frontbuffer invalidation/flushing

[Daniel Vetter]

So these are the guts of the new beast. This tracks when a frontbuffer
gets invalidated (due to frontbuffer rendering) and hence should be
constantly scaned out, and when it's flushed again and can be
compressed/one-shot-upload.

Rules for flushing are simple: The frontbuffer needs one more full
upload starting from the next vblank. Which means that the flushing
can _only_ be called once the frontbuffer update has been latched.

But this poses a problem for pageflips: We can't just delay the
flushing until the pageflip is latched, since that would pose the risk
that we override frontbuffer rendering that has been scheduled
in-between the pageflip ioctl and the actual latching.

To handle this track asynchronous invalidations (and also pageflip)
state per-ring and delay any in-between flushing until the rendering
has completed. And also cancel any delayed flushing if we get a new
invalidation request (whether delayed or not).

Also call intel_mark_fb_busy in both cases in all cases to make sure
that we keep the screen at the highest refresh rate both on flips,
synchronous plane updates and for frontbuffer rendering.

v2: Lots of improvements

Suggestions from Chris:
- Move invalidate/flush in flush_*_domain and set_to_*_domain.
- Drop the flush in busy_ioctl since it's redundant. Was a leftover
  from an earlier concept to track flips/delayed flushes.
- Don't forget about the initial modeset enable/final disable.
  Suggested by Chris.

Track flips accurately, too. Since flips complete independently of
rendering we need to track pending flips in a separate mask. Again if
an invalidate happens we need to cancel the evenutal flush to avoid
races.

v3:
Provide correct header declarations for flip functions. Currently not
needed outside of intel_display.c, but part of the proper interface.

v4: Add proper domain management to fbcon so that the fbcon buffer is
also tracked correctly.

v5: Fixup locking around the fbcon set_to_gtt_domain call.

v6: More comments from Chris:
- Split out fbcon changes.
- Drop superflous checks for potential scanout before calling intel_fb
  functions - we can micro-optimize this later.
- s/intel_fb_/intel_fb_obj_/ to make it clear that this deals in gem
  object. We already have precedence for fb_obj in the pin_and_fence
  functions.

v7: Clarify the semantics of the flip flush handling by renaming
things a bit:
- Don't go through a gem object but take the relevant frontbuffer bits
  directly. These functions center on the plane, the actual object is
  irrelevant - even a flip to the same object as already active should
  cause a flush.
- Add a new intel_frontbuffer_flip for synchronous plane updates. It
  currently just calls intel_frontbuffer_flush since the implemenation
  differs.

This way we achieve a clear split between one-shot update events on
one side and frontbuffer rendering with potentially a very long delay
between the invalidate and flush.

Chris and I also had some discussions about mark_busy and whether it
is appropriate to call from flush. But mark busy is a state which
should be derived from the 3 events (invalidate, flush, flip) we now
have by the users, like psr does by tracking relevant information in
psr.busy_frontbuffer_bits. DRRS (the only real use of mark_busy for
frontbuffer) needs to have similar logic. With that the overall
mark_busy in the core could be removed.

v8: Only when retiring gpu buffers only flush frontbuffer bits we
actually invalidated in a batch. Just for safety since before any
additional usage/invalidate we should always retire current rendering.
Suggested by Chris Wilson.

v9: Actually use intel_frontbuffer_flip in all appropriate places.
Spotted by Chris.

v10: Address more comments from Chris:
- Don't call _flip in set_base when the crtc is inactive, avoids redunancy
  in the modeset case with the initial enabling of all planes.
- Add comments explaining that the initial/final plane enable/disable
  still has work left to do before it's fully generic.

v11: Only invalidate for gtt/cpu access when writing. Spotted by Chris.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h            |  14 +++
 drivers/gpu/drm/i915/i915_gem.c            |  20 ++-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   6 +-
 drivers/gpu/drm/i915/intel_display.c       | 191 +++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_drv.h           |  29 ++++-
 drivers/gpu/drm/i915/intel_overlay.c       |   3 +
 drivers/gpu/drm/i915/intel_sprite.c        |   4 +-
 7 files changed, 242 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4978b9deaeb4..b4e30bfd4d57 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1331,6 +1331,17 @@ struct intel_pipe_crc {
 	wait_queue_head_t wq;
 };
 
+struct i915_frontbuffer_tracking {
+	struct mutex lock;
+
+	/*
+	 * Tracking bits for delayed frontbuffer flushing du to gpu activity or
+	 * scheduled flips.
+	 */
+	unsigned busy_bits;
+	unsigned flip_bits;
+};
+
 struct drm_i915_private {
 	struct drm_device *dev;
 	struct kmem_cache *slab;
@@ -1477,6 +1488,9 @@ struct drm_i915_private {
 	bool lvds_downclock_avail;
 	/* indicates the reduced downclock for LVDS*/
 	int lvds_downclock;
+
+	struct i915_frontbuffer_tracking fb_tracking;
+
 	u16 orig_clock;
 
 	bool mchbar_need_disable;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index caed6621d71a..f6d123828926 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1395,8 +1395,6 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
 		goto unlock;
 	}
 
-	intel_edp_psr_exit(dev);
-
 	/* Try to flush the object off the GPU without holding the lock.
 	 * We will repeat the flush holding the lock in the normal manner
 	 * to catch cases where we are gazumped.
@@ -1442,8 +1440,6 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		return ret;
 
-	intel_edp_psr_exit(dev);
-
 	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
 	if (&obj->base == NULL) {
 		ret = -ENOENT;
@@ -2223,6 +2219,8 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
 			list_move_tail(&vma->mm_list, &vm->inactive_list);
 	}
 
+	intel_fb_obj_flush(obj, true);
+
 	list_del_init(&obj->ring_list);
 	obj->ring = NULL;
 
@@ -3552,6 +3550,8 @@ i915_gem_object_flush_gtt_write_domain(struct drm_i915_gem_object *obj)
 	old_write_domain = obj->base.write_domain;
 	obj->base.write_domain = 0;
 
+	intel_fb_obj_flush(obj, false);
+
 	trace_i915_gem_object_change_domain(obj,
 					    obj->base.read_domains,
 					    old_write_domain);
@@ -3573,6 +3573,8 @@ i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *obj,
 	old_write_domain = obj->base.write_domain;
 	obj->base.write_domain = 0;
 
+	intel_fb_obj_flush(obj, false);
+
 	trace_i915_gem_object_change_domain(obj,
 					    obj->base.read_domains,
 					    old_write_domain);
@@ -3626,6 +3628,9 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write)
 		obj->dirty = 1;
 	}
 
+	if (write)
+		intel_fb_obj_invalidate(obj, NULL);
+
 	trace_i915_gem_object_change_domain(obj,
 					    old_read_domains,
 					    old_write_domain);
@@ -3962,6 +3967,9 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write)
 		obj->base.write_domain = I915_GEM_DOMAIN_CPU;
 	}
 
+	if (write)
+		intel_fb_obj_invalidate(obj, NULL);
+
 	trace_i915_gem_object_change_domain(obj,
 					    old_read_domains,
 					    old_write_domain);
@@ -4236,8 +4244,6 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		return ret;
 
-	intel_edp_psr_exit(dev);
-
 	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
 	if (&obj->base == NULL) {
 		ret = -ENOENT;
@@ -4937,6 +4943,8 @@ i915_gem_load(struct drm_device *dev)
 
 	dev_priv->mm.oom_notifier.notifier_call = i915_gem_shrinker_oom;
 	register_oom_notifier(&dev_priv->mm.oom_notifier);
+
+	mutex_init(&dev_priv->fb_tracking.lock);
 }
 
 void i915_gem_release(struct drm_device *dev, struct drm_file *file)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 93d7f7246588..d815ef51a5ea 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -975,10 +975,8 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas,
 		if (obj->base.write_domain) {
 			obj->dirty = 1;
 			obj->last_write_seqno = intel_ring_get_seqno(ring);
-			/* check for potential scanout */
-			if (i915_gem_obj_ggtt_bound(obj) &&
-			    i915_gem_obj_to_ggtt(obj)->pin_count)
-				intel_mark_fb_busy(obj, ring);
+
+			intel_fb_obj_invalidate(obj, ring);
 
 			/* update for the implicit flush after a batch */
 			obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d9e0c51388df..a7af8b58a73d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2756,6 +2756,9 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 
 	dev_priv->display.update_primary_plane(crtc, fb, x, y);
 
+	if (intel_crtc->active)
+		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
+
 	crtc->primary->fb = fb;
 	crtc->x = x;
 	crtc->y = y;
@@ -3950,6 +3953,13 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
 	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
 	mutex_unlock(&dev->struct_mutex);
+
+	/*
+	 * FIXME: Once we grow proper nuclear flip support out of this we need
+	 * to compute the mask of flip planes precisely. For the time being
+	 * consider this a flip from a NULL plane.
+	 */
+	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe));
 }
 
 static void intel_crtc_disable_planes(struct drm_crtc *crtc)
@@ -3972,6 +3982,13 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc)
 	intel_disable_planes(crtc);
 	intel_disable_primary_hw_plane(dev_priv, plane, pipe);
 
+	/*
+	 * FIXME: Once we grow proper nuclear flip support out of this we need
+	 * to compute the mask of flip planes precisely. For the time being
+	 * consider this a flip to a NULL plane.
+	 */
+	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe));
+
 	drm_vblank_off(dev, pipe);
 }
 
@@ -8212,6 +8229,8 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
 	}
 
+	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
+
 	return 0;
 fail_unpin:
 	i915_gem_object_unpin_from_display_plane(obj);
@@ -8827,20 +8846,26 @@ out:
 }
 
 
-void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
-			struct intel_engine_cs *ring)
+/**
+ * intel_mark_fb_busy - mark given planes as busy
+ * @dev: DRM device
+ * @frontbuffer_bits: bits for the affected planes
+ * @ring: optional ring for asynchronous commands
+ *
+ * This function gets called every time the screen contents change. It can be
+ * used to keep e.g. the update rate at the nominal refresh rate with DRRS.
+ */
+static void intel_mark_fb_busy(struct drm_device *dev,
+			       unsigned frontbuffer_bits,
+			       struct intel_engine_cs *ring)
 {
-	struct drm_device *dev = obj->base.dev;
 	enum pipe pipe;
 
-	intel_edp_psr_exit(dev);
-
 	if (!i915.powersave)
 		return;
 
 	for_each_pipe(pipe) {
-		if (!(obj->frontbuffer_bits &
-		      INTEL_FRONTBUFFER_ALL_MASK(pipe)))
+		if (!(frontbuffer_bits & INTEL_FRONTBUFFER_ALL_MASK(pipe)))
 			continue;
 
 		intel_increase_pllclock(dev, pipe);
@@ -8849,6 +8874,150 @@ void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
 	}
 }
 
+/**
+ * intel_fb_obj_invalidate - invalidate frontbuffer object
+ * @obj: GEM object to invalidate
+ * @ring: set for asynchronous rendering
+ *
+ * This function gets called every time rendering on the given object starts and
+ * frontbuffer caching (fbc, low refresh rate for DRRS, panel self refresh) must
+ * be invalidated. If @ring is non-NULL any subsequent invalidation will be delayed
+ * until the rendering completes or a flip on this frontbuffer plane is
+ * scheduled.
+ */
+void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
+			     struct intel_engine_cs *ring)
+{
+	struct drm_device *dev = obj->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
+	if (!obj->frontbuffer_bits)
+		return;
+
+	if (ring) {
+		mutex_lock(&dev_priv->fb_tracking.lock);
+		dev_priv->fb_tracking.busy_bits
+			|= obj->frontbuffer_bits;
+		dev_priv->fb_tracking.flip_bits
+			&= ~obj->frontbuffer_bits;
+		mutex_unlock(&dev_priv->fb_tracking.lock);
+	}
+
+	intel_mark_fb_busy(dev, obj->frontbuffer_bits, ring);
+
+	intel_edp_psr_exit(dev);
+}
+
+/**
+ * intel_frontbuffer_flush - flush frontbuffer
+ * @dev: DRM device
+ * @frontbuffer_bits: frontbuffer plane tracking bits
+ *
+ * This function gets called every time rendering on the given planes has
+ * completed and frontbuffer caching can be started again. Flushes will get
+ * delayed if they're blocked by some oustanding asynchronous rendering.
+ *
+ * Can be called without any locks held.
+ */
+void intel_frontbuffer_flush(struct drm_device *dev,
+			     unsigned frontbuffer_bits)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	/* Delay flushing when rings are still busy.*/
+	mutex_lock(&dev_priv->fb_tracking.lock);
+	frontbuffer_bits &= ~dev_priv->fb_tracking.busy_bits;
+	mutex_unlock(&dev_priv->fb_tracking.lock);
+
+	intel_mark_fb_busy(dev, frontbuffer_bits, NULL);
+
+	intel_edp_psr_exit(dev);
+}
+
+/**
+ * intel_fb_obj_flush - flush frontbuffer object
+ * @obj: GEM object to flush
+ * @retire: set when retiring asynchronous rendering
+ *
+ * This function gets called every time rendering on the given object has
+ * completed and frontbuffer caching can be started again. If @retire is true
+ * then any delayed flushes will be unblocked.
+ */
+void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
+			bool retire)
+{
+	struct drm_device *dev = obj->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	unsigned frontbuffer_bits;
+
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
+	if (!obj->frontbuffer_bits)
+		return;
+
+	frontbuffer_bits = obj->frontbuffer_bits;
+
+	if (retire) {
+		mutex_lock(&dev_priv->fb_tracking.lock);
+		/* Filter out new bits since rendering started. */
+		frontbuffer_bits &= dev_priv->fb_tracking.busy_bits;
+
+		dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
+		mutex_unlock(&dev_priv->fb_tracking.lock);
+	}
+
+	intel_frontbuffer_flush(dev, frontbuffer_bits);
+}
+
+/**
+ * intel_frontbuffer_flip_prepare - prepare asnychronous frontbuffer flip
+ * @dev: DRM device
+ * @frontbuffer_bits: frontbuffer plane tracking bits
+ *
+ * This function gets called after scheduling a flip on @obj. The actual
+ * frontbuffer flushing will be delayed until completion is signalled with
+ * intel_frontbuffer_flip_complete. If an invalidate happens in between this
+ * flush will be cancelled.
+ *
+ * Can be called without any locks held.
+ */
+void intel_frontbuffer_flip_prepare(struct drm_device *dev,
+				    unsigned frontbuffer_bits)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	mutex_lock(&dev_priv->fb_tracking.lock);
+	dev_priv->fb_tracking.flip_bits
+		|= frontbuffer_bits;
+	mutex_unlock(&dev_priv->fb_tracking.lock);
+}
+
+/**
+ * intel_frontbuffer_flip_complete - complete asynchronous frontbuffer flush
+ * @dev: DRM device
+ * @frontbuffer_bits: frontbuffer plane tracking bits
+ *
+ * This function gets called after the flip has been latched and will complete
+ * on the next vblank. It will execute the fush if it hasn't been cancalled yet.
+ *
+ * Can be called without any locks held.
+ */
+void intel_frontbuffer_flip_complete(struct drm_device *dev,
+				     unsigned frontbuffer_bits)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	mutex_lock(&dev_priv->fb_tracking.lock);
+	/* Mask any cancelled flips. */
+	frontbuffer_bits &= dev_priv->fb_tracking.flip_bits;
+	dev_priv->fb_tracking.flip_bits &= ~frontbuffer_bits;
+	mutex_unlock(&dev_priv->fb_tracking.lock);
+
+	intel_frontbuffer_flush(dev, frontbuffer_bits);
+}
+
 static void intel_crtc_destroy(struct drm_crtc *crtc)
 {
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -8876,6 +9045,7 @@ static void intel_unpin_work_fn(struct work_struct *__work)
 	struct intel_unpin_work *work =
 		container_of(__work, struct intel_unpin_work, work);
 	struct drm_device *dev = work->crtc->dev;
+	enum pipe pipe = to_intel_crtc(work->crtc)->pipe;
 
 	mutex_lock(&dev->struct_mutex);
 	intel_unpin_fb_obj(work->old_fb_obj);
@@ -8885,6 +9055,8 @@ static void intel_unpin_work_fn(struct work_struct *__work)
 	intel_update_fbc(dev);
 	mutex_unlock(&dev->struct_mutex);
 
+	intel_frontbuffer_flip_complete(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
+
 	BUG_ON(atomic_read(&to_intel_crtc(work->crtc)->unpin_work_count) == 0);
 	atomic_dec(&to_intel_crtc(work->crtc)->unpin_work_count);
 
@@ -9441,9 +9613,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	if (work == NULL)
 		return -ENOMEM;
 
-	/* Exit PSR early in page flip */
-	intel_edp_psr_exit(dev);
-
 	work->event = event;
 	work->crtc = crtc;
 	work->old_fb_obj = to_intel_framebuffer(old_fb)->obj;
@@ -9519,7 +9688,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 			  INTEL_FRONTBUFFER_PRIMARY(pipe));
 
 	intel_disable_fbc(dev);
-	intel_mark_fb_busy(obj, NULL);
+	intel_frontbuffer_flip_prepare(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
 	mutex_unlock(&dev->struct_mutex);
 
 	trace_i915_flip_request(intel_crtc->plane, obj);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5d20f719309a..bd0d10eeaf44 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -724,8 +724,33 @@ bool intel_has_pending_fb_unpin(struct drm_device *dev);
 int intel_pch_rawclk(struct drm_device *dev);
 int valleyview_cur_cdclk(struct drm_i915_private *dev_priv);
 void intel_mark_busy(struct drm_device *dev);
-void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
-			struct intel_engine_cs *ring);
+void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
+			     struct intel_engine_cs *ring);
+void intel_frontbuffer_flip_prepare(struct drm_device *dev,
+				    unsigned frontbuffer_bits);
+void intel_frontbuffer_flip_complete(struct drm_device *dev,
+				     unsigned frontbuffer_bits);
+void intel_frontbuffer_flush(struct drm_device *dev,
+			     unsigned frontbuffer_bits);
+/**
+ * intel_frontbuffer_flip - prepare frontbuffer flip
+ * @dev: DRM device
+ * @frontbuffer_bits: frontbuffer plane tracking bits
+ *
+ * This function gets called after scheduling a flip on @obj. This is for
+ * synchronous plane updates which will happen on the next vblank and which will
+ * not get delayed by pending gpu rendering.
+ *
+ * Can be called without any locks held.
+ */
+static inline
+void intel_frontbuffer_flip(struct drm_device *dev,
+			    unsigned frontbuffer_bits)
+{
+	intel_frontbuffer_flush(dev, frontbuffer_bits);
+}
+
+void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire);
 void intel_mark_idle(struct drm_device *dev);
 void intel_crtc_restore_mode(struct drm_crtc *crtc);
 void intel_crtc_update_dpms(struct drm_crtc *crtc);
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 99b6c142a095..1c002196ed47 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -787,6 +787,9 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
 	overlay->old_vid_bo = overlay->vid_bo;
 	overlay->vid_bo = new_bo;
 
+	intel_frontbuffer_flush(dev,
+				INTEL_FRONTBUFFER_OVERLAY(pipe));
+
 	return 0;
 
 out_unpin:
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 140bd8359f0e..0e3fd5c59e28 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1034,6 +1034,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		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);
 	}
@@ -1054,8 +1056,6 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		mutex_unlock(&dev->struct_mutex);
 	}
 
-	intel_edp_psr_exit(dev);
-
 	return 0;
 }
 
-- 
2.0.0

Re: [PATCH] drm/i915: Track frontbuffer invalidation/flushing

[Chris Wilson]

On Thu, Jun 19, 2014 at 04:01:59PM +0200, Daniel Vetter wrote:
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index 99b6c142a095..1c002196ed47 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -787,6 +787,9 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
>  	overlay->old_vid_bo = overlay->vid_bo;
>  	overlay->vid_bo = new_bo;
>  
> +	intel_frontbuffer_flush(dev,
> +				INTEL_FRONTBUFFER_OVERLAY(pipe));
> +

If we want to be strict, we could do the flip_prepare, flip_complete
here as well. Basically, I'm arguing that this is actually a
semi-disguised flip.

With this changed to _flip,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Track frontbuffer invalidation/flushing

[Daniel Vetter]

On Thu, Jun 19, 2014 at 04:12:08PM +0100, Chris Wilson wrote:
> On Thu, Jun 19, 2014 at 04:01:59PM +0200, Daniel Vetter wrote:
> > diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> > index 99b6c142a095..1c002196ed47 100644
> > --- a/drivers/gpu/drm/i915/intel_overlay.c
> > +++ b/drivers/gpu/drm/i915/intel_overlay.c
> > @@ -787,6 +787,9 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
> >  	overlay->old_vid_bo = overlay->vid_bo;
> >  	overlay->vid_bo = new_bo;
> >  
> > +	intel_frontbuffer_flush(dev,
> > +				INTEL_FRONTBUFFER_OVERLAY(pipe));
> > +
> 
> If we want to be strict, we could do the flip_prepare, flip_complete
> here as well. Basically, I'm arguing that this is actually a
> semi-disguised flip.

Fixed.

> With this changed to _flip,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks, I've merged the core patches + the psr patches that Rodrigo
already reviewed to dinq. The remaining parts are in my psr-stuff branch
so that Rodrigo finish the byt rebasing on top of it while I'm
vacationing.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 15/16] drm/i915: Fix up PSR frontbuffer tracking

[Daniel Vetter]

I've tried to split this up, but all the changes are so tightly
related that I didn't find a good way to do this without breaking
bisecting. Essentially this completely changes how psr is glued into
the overall driver, and there's not much you can do to soften such a
paradigm change.

- Use frontbuffer tracking bits stuff to separate disable and
  re-enable.

- Don't re-check everything in the psr work. We have now accurate
  tracking for everything, so no need to check for sprites or tiling
  really. Allows us to ditch tons of locks.

- That in turn allows us to properly cancel the work in the disable
  function - no more deadlocks.

- Add a check for HSW sprites and force a flush. Apparently the
  hardware doesn't forward the flushing when updating the sprite base
  address. We can do the same trick everywhere else we have such
  issues, e.g. on baytrail with ... everything.

- Don't re-enable psr with a delay in psr_exit. It really must be
  turned off forever if we detect a gtt write. At least with the
  current frontbuffer render tracking. Userspace can do a busy ioctl
  call or no-op pageflip to re-enable psr.

- Drop redundant checks for crtc and crtc->active - now that they're
  only called from enable this is guaranteed.

- Fix up the hsw port check. eDP can also happen on port D, but the
  issue is exactly that it doesn't work there. So an || check is
  wrong.

- We still schedule the psr work with a delay. The frontbuffer
  flushing interface mandates that we upload the next full frame, so
  need to wait a bit. Once we have single-shot frame uploads we can do
  better here.

v2: Don't enable psr initially, rely upon the fb flush of the initial
plane setup for that. Gives us more unified code flow and makes the
crtc enable sequence less a special case.

v3: s/psr_exit/psr_invalidate/ for consistency

v4: Fixup whitespace.

v5: Correctly bail out of psr_invalidate/flush when
dev_priv->psr.enabled is NULL. Spotted by Rodrigo.

v6:
- Only schedule work when there's work to do. Fixes WARNINGs reported
  by Rodrigo.
- Comments Chris requested to clarify the code.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h      |   1 +
 drivers/gpu/drm/i915/intel_display.c |   4 +-
 drivers/gpu/drm/i915/intel_dp.c      | 125 ++++++++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_drv.h     |   5 +-
 4 files changed, 85 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0a309fe53911..1e52da99ca79 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -641,6 +641,7 @@ struct i915_psr {
 	struct intel_dp *enabled;
 	bool active;
 	struct delayed_work work;
+	unsigned busy_frontbuffer_bits;
 };
 
 enum intel_pch {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2361d03ea1f3..50bca2c7c3f6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8895,7 +8895,7 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
 
 	intel_mark_fb_busy(dev, obj->frontbuffer_bits, ring);
 
-	intel_edp_psr_exit(dev);
+	intel_edp_psr_invalidate(dev, obj->frontbuffer_bits);
 }
 
 /**
@@ -8921,7 +8921,7 @@ void intel_frontbuffer_flush(struct drm_device *dev,
 
 	intel_mark_fb_busy(dev, frontbuffer_bits, NULL);
 
-	intel_edp_psr_exit(dev);
+	intel_edp_psr_flush(dev, frontbuffer_bits);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e17a85468f78..1a8b3d90e422 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1746,8 +1746,6 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc = dig_port->base.base.crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct drm_i915_gem_object *obj = to_intel_framebuffer(crtc->primary->fb)->obj;
-	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
 
 	lockdep_assert_held(&dev_priv->psr.lock);
 	lockdep_assert_held(&dev->struct_mutex);
@@ -1761,8 +1759,7 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
 		return false;
 	}
 
-	if (IS_HASWELL(dev) && (intel_encoder->type != INTEL_OUTPUT_EDP ||
-				dig_port->port != PORT_A)) {
+	if (IS_HASWELL(dev) && dig_port->port != PORT_A) {
 		DRM_DEBUG_KMS("HSW ties PSR to DDI A (eDP)\n");
 		return false;
 	}
@@ -1772,34 +1769,10 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
 		return false;
 	}
 
-	crtc = dig_port->base.base.crtc;
-	if (crtc == NULL) {
-		DRM_DEBUG_KMS("crtc not active for PSR\n");
-		return false;
-	}
-
-	intel_crtc = to_intel_crtc(crtc);
-	if (!intel_crtc_active(crtc)) {
-		DRM_DEBUG_KMS("crtc not active for PSR\n");
-		return false;
-	}
-
-	obj = to_intel_framebuffer(crtc->primary->fb)->obj;
-	if (obj->tiling_mode != I915_TILING_X ||
-	    obj->fence_reg == I915_FENCE_REG_NONE) {
-		DRM_DEBUG_KMS("PSR condition failed: fb not tiled or fenced\n");
-		return false;
-	}
-
 	/* Below limitations aren't valid for Broadwell */
 	if (IS_BROADWELL(dev))
 		goto out;
 
-	if (I915_READ(SPRCTL(intel_crtc->pipe)) & SPRITE_ENABLE) {
-		DRM_DEBUG_KMS("PSR condition failed: Sprite is Enabled\n");
-		return false;
-	}
-
 	if (I915_READ(HSW_STEREO_3D_CTL(intel_crtc->config.cpu_transcoder)) &
 	    S3D_ENABLE) {
 		DRM_DEBUG_KMS("PSR condition failed: Stereo 3D is Enabled\n");
@@ -1832,7 +1805,6 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
 	/* Enable PSR on the host */
 	intel_edp_psr_enable_source(intel_dp);
 
-	dev_priv->psr.enabled = intel_dp;
 	dev_priv->psr.active = true;
 }
 
@@ -1858,11 +1830,13 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
 		return;
 	}
 
+	dev_priv->psr.busy_frontbuffer_bits = 0;
+
 	/* Setup PSR once */
 	intel_edp_psr_setup(intel_dp);
 
 	if (intel_edp_psr_match_conditions(intel_dp))
-		intel_edp_psr_do_enable(intel_dp);
+		dev_priv->psr.enabled = intel_dp;
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
@@ -1896,42 +1870,39 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
 
 	dev_priv->psr.enabled = NULL;
 	mutex_unlock(&dev_priv->psr.lock);
+
+	cancel_delayed_work_sync(&dev_priv->psr.work);
 }
 
 static void intel_edp_psr_work(struct work_struct *work)
 {
 	struct drm_i915_private *dev_priv =
 		container_of(work, typeof(*dev_priv), psr.work.work);
-	struct drm_device *dev = dev_priv->dev;
 	struct intel_dp *intel_dp = dev_priv->psr.enabled;
 
-	drm_modeset_lock_all(dev);
-	mutex_lock(&dev->struct_mutex);
 	mutex_lock(&dev_priv->psr.lock);
 	intel_dp = dev_priv->psr.enabled;
 
 	if (!intel_dp)
 		goto unlock;
 
-	if (intel_edp_psr_match_conditions(intel_dp))
-		intel_edp_psr_do_enable(intel_dp);
+	/*
+	 * The delayed work can race with an invalidate hence we need to
+	 * recheck. Since psr_flush first clears this and then reschedules we
+	 * won't ever miss a flush when bailing out here.
+	 */
+	if (dev_priv->psr.busy_frontbuffer_bits)
+		goto unlock;
+
+	intel_edp_psr_do_enable(intel_dp);
 unlock:
 	mutex_unlock(&dev_priv->psr.lock);
-	mutex_unlock(&dev->struct_mutex);
-	drm_modeset_unlock_all(dev);
 }
 
-void intel_edp_psr_exit(struct drm_device *dev)
+static void intel_edp_psr_do_exit(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (!HAS_PSR(dev))
-		return;
-
-	if (!dev_priv->psr.enabled)
-		return;
-
-	mutex_lock(&dev_priv->psr.lock);
 	if (dev_priv->psr.active) {
 		u32 val = I915_READ(EDP_PSR_CTL(dev));
 
@@ -1942,8 +1913,68 @@ void intel_edp_psr_exit(struct drm_device *dev)
 		dev_priv->psr.active = false;
 	}
 
-	schedule_delayed_work(&dev_priv->psr.work,
-			      msecs_to_jiffies(100));
+}
+
+void intel_edp_psr_invalidate(struct drm_device *dev,
+			      unsigned frontbuffer_bits)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc;
+	enum pipe pipe;
+
+	if (!HAS_PSR(dev))
+		return;
+
+	mutex_lock(&dev_priv->psr.lock);
+	if (!dev_priv->psr.enabled) {
+		mutex_unlock(&dev_priv->psr.lock);
+		return;
+	}
+
+	crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc;
+	pipe = to_intel_crtc(crtc)->pipe;
+
+	intel_edp_psr_do_exit(dev);
+
+	frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
+
+	dev_priv->psr.busy_frontbuffer_bits |= frontbuffer_bits;
+	mutex_unlock(&dev_priv->psr.lock);
+}
+
+void intel_edp_psr_flush(struct drm_device *dev,
+			 unsigned frontbuffer_bits)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc;
+	enum pipe pipe;
+
+	if (!HAS_PSR(dev))
+		return;
+
+	mutex_lock(&dev_priv->psr.lock);
+	if (!dev_priv->psr.enabled) {
+		mutex_unlock(&dev_priv->psr.lock);
+		return;
+	}
+
+	crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc;
+	pipe = to_intel_crtc(crtc)->pipe;
+	dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
+
+	/*
+	 * On Haswell sprite plane updates don't result in a psr invalidating
+	 * signal in the hardware. Which means we need to manually fake this in
+	 * software for all flushes, not just when we've seen a preceding
+	 * invalidation through frontbuffer rendering.
+	 */
+	if (IS_HASWELL(dev) &&
+	    (frontbuffer_bits & INTEL_FRONTBUFFER_SPRITE(pipe)))
+		intel_edp_psr_do_exit(dev);
+
+	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
+		schedule_delayed_work(&dev_priv->psr.work,
+				      msecs_to_jiffies(100));
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index bd0d10eeaf44..f44f315f1c6a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -860,7 +860,10 @@ void intel_edp_panel_off(struct intel_dp *intel_dp);
 void intel_edp_psr_enable(struct intel_dp *intel_dp);
 void intel_edp_psr_disable(struct intel_dp *intel_dp);
 void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate);
-void intel_edp_psr_exit(struct drm_device *dev);
+void intel_edp_psr_invalidate(struct drm_device *dev,
+			      unsigned frontbuffer_bits);
+void intel_edp_psr_flush(struct drm_device *dev,
+			 unsigned frontbuffer_bits);
 void intel_edp_psr_init(struct drm_device *dev);
 
 
-- 
2.0.0

[PATCH 16/16] drm/i915: Improve PSR debugfs output

[Daniel Vetter]

Add busy_frontbuffer_bits and locking.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 4818aff9e6d6..6c0413fd3d37 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1971,10 +1971,15 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 
 	intel_runtime_pm_get(dev_priv);
 
+	mutex_lock(&dev_priv->psr.lock);
 	seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support));
 	seq_printf(m, "Source_OK: %s\n", yesno(dev_priv->psr.source_ok));
 	seq_printf(m, "Enabled: %s\n", yesno((bool)dev_priv->psr.enabled));
 	seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active));
+	seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
+		   dev_priv->psr.busy_frontbuffer_bits);
+	seq_printf(m, "Re-enable work scheduled: %s\n",
+		   yesno(work_busy(&dev_priv->psr.work.work)));
 
 	enabled = HAS_PSR(dev) &&
 		I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE;
@@ -1984,6 +1989,7 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 		psrperf = I915_READ(EDP_PSR_PERF_CNT(dev)) &
 			EDP_PSR_PERF_CNT_MASK;
 	seq_printf(m, "Performance_Counter: %u\n", psrperf);
+	mutex_unlock(&dev_priv->psr.lock);
 
 	intel_runtime_pm_put(dev_priv);
 	return 0;
-- 
2.0.0

[PATCH] drm/i915: Print obj->frontbuffer_bits in debugfs output

[Daniel Vetter]

Can be useful to figure out imbalances and bugs in the frontbuffer
tracking.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 6c0413fd3d37..73510481bdba 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -170,6 +170,8 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
 	}
 	if (obj->ring != NULL)
 		seq_printf(m, " (%s)", obj->ring->name);
+	if (obj->frontbuffer_bits)
+		seq_printf(m, " (frontbuffer: 0x%03x)", obj->frontbuffer_bits);
 }
 
 static void describe_ctx(struct seq_file *m, struct intel_context *ctx)
-- 
2.0.0

Re: [PATCH] drm/i915: Print obj->frontbuffer_bits in debugfs output

[Chris Wilson]

On Wed, Jun 18, 2014 at 02:46:49PM +0200, Daniel Vetter wrote:
> Can be useful to figure out imbalances and bugs in the frontbuffer
> tracking.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

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

I don't think this will be useful for error state. All the failure modes
here I think relate to display coherency, unless you can think of
something else?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Print obj->frontbuffer_bits in debugfs output

[Daniel Vetter]

On Wed, Jun 18, 2014 at 03:50:20PM +0100, Chris Wilson wrote:
> On Wed, Jun 18, 2014 at 02:46:49PM +0200, Daniel Vetter wrote:
> > Can be useful to figure out imbalances and bugs in the frontbuffer
> > tracking.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> I don't think this will be useful for error state. All the failure modes
> here I think relate to display coherency, unless you can think of
> something else?

No, should really only be relevant for display coherency bugs, not unhappy
gpus. I think the biggest risk there is that flushing at inopportune times
might upset the hardware. But since these operations happen at few times
per frame we can't dump a tracing stream of debug noise by default.

So nothing to add here until we have dynamic debugging or some other means
for more fine-grained debug logging.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 16/16] drm/i915: Improve PSR debugfs output

[Chris Wilson]

On Wed, Jun 18, 2014 at 01:59:17PM +0200, Daniel Vetter wrote:
> Add busy_frontbuffer_bits and locking.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 4818aff9e6d6..6c0413fd3d37 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1971,10 +1971,15 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>  
>  	intel_runtime_pm_get(dev_priv);
>  
> +	mutex_lock(&dev_priv->psr.lock);

It's debugfs. Play nice and let the user interrupt us, just in case.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 16/16] drm/i915: Improve PSR debugfs output

[Daniel Vetter]

On Wed, Jun 18, 2014 at 03:51:01PM +0100, Chris Wilson wrote:
> On Wed, Jun 18, 2014 at 01:59:17PM +0200, Daniel Vetter wrote:
> > Add busy_frontbuffer_bits and locking.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 4818aff9e6d6..6c0413fd3d37 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -1971,10 +1971,15 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
> >  
> >  	intel_runtime_pm_get(dev_priv);
> >  
> > +	mutex_lock(&dev_priv->psr.lock);
> 
> It's debugfs. Play nice and let the user interrupt us, just in case.

I agree with dev->struct_mutex since the gpu hangs often, but for general
hw features I think the backtrace is good enough and the simpler locking
better. Worst case you need to spawn a new terminal over ssh because the
first one is stuck ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/i915: Remove redundant HAS_PSR checks

[Daniel Vetter]

We only need to check for this in psr_enable, everything else is
already protect by the dev_priv->psr.enabled checks. Those need the
psr locking, but these functions are called infrequent enough that the
locking overhead is negligible.

Suggested by Chris Wilson.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_dp.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1a8b3d90e422..0c1c77ae16fb 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1754,11 +1754,6 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
 
 	dev_priv->psr.source_ok = false;
 
-	if (!HAS_PSR(dev)) {
-		DRM_DEBUG_KMS("PSR not supported on this platform\n");
-		return false;
-	}
-
 	if (IS_HASWELL(dev) && dig_port->port != PORT_A) {
 		DRM_DEBUG_KMS("HSW ties PSR to DDI A (eDP)\n");
 		return false;
@@ -1845,9 +1840,6 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (!HAS_PSR(dev))
-		return;
-
 	mutex_lock(&dev_priv->psr.lock);
 	if (!dev_priv->psr.enabled) {
 		mutex_unlock(&dev_priv->psr.lock);
@@ -1922,9 +1914,6 @@ void intel_edp_psr_invalidate(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	enum pipe pipe;
 
-	if (!HAS_PSR(dev))
-		return;
-
 	mutex_lock(&dev_priv->psr.lock);
 	if (!dev_priv->psr.enabled) {
 		mutex_unlock(&dev_priv->psr.lock);
@@ -1949,9 +1938,6 @@ void intel_edp_psr_flush(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	enum pipe pipe;
 
-	if (!HAS_PSR(dev))
-		return;
-
 	mutex_lock(&dev_priv->psr.lock);
 	if (!dev_priv->psr.enabled) {
 		mutex_unlock(&dev_priv->psr.lock);
-- 
2.0.0