[PATCH 00/12] Yet another FBC series, v3 part 2

[PATCH 00/12] Yet another FBC series, v3 part 2

[Paulo Zanoni]

Hi

Here are other 12 patches from the last series of 26 which I sent two weeks ago.
The only things missing are the patch that does the flip optimizations - since
it had a bug caught by Ville - and a patch requested by Chris related to getting
framebuffer references or proving we don't need them. I'm leaving these to a
next patch series, since my plans for them includes multiple patches.

Thanks,
Paulo

Paulo Zanoni (12):
  drm/i915: fix the CFB size check
  drm/i915: set dev_priv->fbc.crtc before scheduling the enable work
  drm/i915: pass the crtc as an argument to intel_fbc_update()
  drm/i915: introduce is_active/activate/deactivate to the FBC
    terminology
  drm/i915: introduce intel_fbc_{enable,disable}
  drm/i915: alloc/free the FBC CFB during enable/disable
  drm/i915: check for FBC planes in the same place as the pipes
  drm/i915: use a single intel_fbc_work struct
  drm/i915: wait for a vblank instead of 50ms when enabling FBC
  drm/i915: kill fbc.uncompressed_size
  drm/i915: get rid of FBC {,de}activation messages
  drm/i915: only nuke FBC when a drawing operation triggers a flush

 drivers/gpu/drm/i915/i915_debugfs.c  |   2 +-
 drivers/gpu/drm/i915/i915_drv.h      |  17 +-
 drivers/gpu/drm/i915/intel_display.c |  23 +-
 drivers/gpu/drm/i915/intel_drv.h     |   6 +-
 drivers/gpu/drm/i915/intel_fbc.c     | 602 +++++++++++++++++++----------------
 drivers/gpu/drm/i915/intel_pm.c      |   2 +-
 6 files changed, 351 insertions(+), 301 deletions(-)

-- 
2.6.2

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

[PATCH 01/12] drm/i915: fix the CFB size check

[Paulo Zanoni]

In function find_compression_threshold() we try to over-allocate CFB
space in order to reduce reallocations and fragmentation, and we're
not considering that at the CFB size check. Consider it.

There is also a longer-term plan to kill
dev_priv->fbc.uncompressed_size, but this will come later.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 11fc528..611672f 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -720,7 +720,8 @@ static int intel_fbc_setup_cfb(struct intel_crtc *crtc)
 	size = intel_fbc_calculate_cfb_size(crtc);
 	cpp = drm_format_plane_cpp(fb->pixel_format, 0);
 
-	if (size <= dev_priv->fbc.uncompressed_size)
+	if (dev_priv->fbc.compressed_fb.allocated &&
+	    size <= dev_priv->fbc.compressed_fb.size * dev_priv->fbc.threshold)
 		return 0;
 
 	/* Release any current block */
-- 
2.6.2

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

[PATCH 02/12] drm/i915: set dev_priv->fbc.crtc before scheduling the enable work

[Paulo Zanoni]

This thing where we need to get the crtc either from the work
structure or the fbc structure itself is confusing and unnecessary.
Set fbc.crtc right when scheduling the enable work so we can always
use it.

The problem is not what gets passed and how to retrieve it. The
problem is that when we're in the other parts of the code we always
have to keep in mind that if FBC is already enabled we have to get the
CRTC from place A, if FBC is scheduled we have to get the CRTC from
place B, and if it's disabled there's no CRTC. Having a single place
to retrieve the CRTC from allows us to treat the "is enabled" and "is
scheduled" cases as the same case, reducing the mistake surface. I
guess I should add this to the commit message.

Besides the immediate advantages, this is also going to make one of
the next commits much simpler. And even later, when we introduce
enable/disable + activate/deactivate, this will be even simpler as
we'll set the CRTC at enable time. So all the
activate/deactivate/update code can just look at the single CRTC
variable regardless of the current state.

v2: Improve commit message (Chris).
v3: Rebase after changing the patch order.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  1 -
 drivers/gpu/drm/i915/intel_fbc.c | 22 +++++++++-------------
 2 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 71bd1dc..317414b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -920,7 +920,6 @@ struct i915_fbc {
 
 	struct intel_fbc_work {
 		struct delayed_work work;
-		struct intel_crtc *crtc;
 		struct drm_framebuffer *fb;
 	} *fbc_work;
 
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 611672f..bc9cd33 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -334,14 +334,13 @@ bool intel_fbc_enabled(struct drm_i915_private *dev_priv)
 	return dev_priv->fbc.enabled;
 }
 
-static void intel_fbc_enable(struct intel_crtc *crtc,
-			     const struct drm_framebuffer *fb)
+static void intel_fbc_enable(const struct drm_framebuffer *fb)
 {
-	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct drm_i915_private *dev_priv = fb->dev->dev_private;
+	struct intel_crtc *crtc = dev_priv->fbc.crtc;
 
 	dev_priv->fbc.enable_fbc(crtc);
 
-	dev_priv->fbc.crtc = crtc;
 	dev_priv->fbc.fb_id = fb->base.id;
 	dev_priv->fbc.y = crtc->base.y;
 }
@@ -351,8 +350,8 @@ static void intel_fbc_work_fn(struct work_struct *__work)
 	struct intel_fbc_work *work =
 		container_of(to_delayed_work(__work),
 			     struct intel_fbc_work, work);
-	struct drm_i915_private *dev_priv = work->crtc->base.dev->dev_private;
-	struct drm_framebuffer *crtc_fb = work->crtc->base.primary->fb;
+	struct drm_i915_private *dev_priv = work->fb->dev->dev_private;
+	struct drm_framebuffer *crtc_fb = dev_priv->fbc.crtc->base.primary->fb;
 
 	mutex_lock(&dev_priv->fbc.lock);
 	if (work == dev_priv->fbc.fbc_work) {
@@ -360,7 +359,7 @@ static void intel_fbc_work_fn(struct work_struct *__work)
 		 * the prior work.
 		 */
 		if (crtc_fb == work->fb)
-			intel_fbc_enable(work->crtc, work->fb);
+			intel_fbc_enable(work->fb);
 
 		dev_priv->fbc.fbc_work = NULL;
 	}
@@ -400,15 +399,15 @@ static void intel_fbc_schedule_enable(struct intel_crtc *crtc)
 	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
 
 	intel_fbc_cancel_work(dev_priv);
+	dev_priv->fbc.crtc = crtc;
 
 	work = kzalloc(sizeof(*work), GFP_KERNEL);
 	if (work == NULL) {
 		DRM_ERROR("Failed to allocate FBC work structure\n");
-		intel_fbc_enable(crtc, crtc->base.primary->fb);
+		intel_fbc_enable(crtc->base.primary->fb);
 		return;
 	}
 
-	work->crtc = crtc;
 	work->fb = crtc->base.primary->fb;
 	INIT_DELAYED_WORK(&work->work, intel_fbc_work_fn);
 
@@ -984,11 +983,8 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 
 	mutex_lock(&dev_priv->fbc.lock);
 
-	if (dev_priv->fbc.enabled)
+	if (dev_priv->fbc.enabled || dev_priv->fbc.fbc_work)
 		fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe);
-	else if (dev_priv->fbc.fbc_work)
-		fbc_bits = INTEL_FRONTBUFFER_PRIMARY(
-					dev_priv->fbc.fbc_work->crtc->pipe);
 	else
 		fbc_bits = dev_priv->fbc.possible_framebuffer_bits;
 
-- 
2.6.2

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

[PATCH 03/12] drm/i915: pass the crtc as an argument to intel_fbc_update()

[Paulo Zanoni]

There's no need to reevaluate the status of every single crtc when a
single crtc changes its state.

With this, we're cutting the case where due to a change in pipe B,
intel_fbc_update() is called, then intel_fbc_find_crtc() concludes FBC
should be enabled on pipe A, then it completely rechecks the state of
pipe A only to conclude FBC should remain enabled on pipe A. If any
change on pipe A triggers a need to recompute whether FBC is valid on
pipe A, then at some point someone is going to call
intel_fbc_update(PIPE_A).

The addition of intel_fbc_deactivate() is necessary so we keep track
of the previously selected CRTC when we do invalidate/flush. We're
also going to continue the enable/disable/activate/deactivate concept
in the next patches.

v2: Rebase.
v3: Rebase after changing the patch order.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  3 +-
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 drivers/gpu/drm/i915/intel_fbc.c     | 68 +++++++++++++++---------------------
 3 files changed, 31 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b5f7493..1040705 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4761,7 +4761,6 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
 {
 	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
 	struct drm_device *dev = crtc->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	if (atomic->wait_vblank)
 		intel_wait_for_vblank(dev, crtc->pipe);
@@ -4775,7 +4774,7 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
 		intel_update_watermarks(&crtc->base);
 
 	if (atomic->update_fbc)
-		intel_fbc_update(dev_priv);
+		intel_fbc_update(crtc);
 
 	if (atomic->post_enable_primary)
 		intel_post_enable_primary(&crtc->base);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 523e553..91458fb 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1323,7 +1323,7 @@ static inline void intel_fbdev_restore_mode(struct drm_device *dev)
 
 /* intel_fbc.c */
 bool intel_fbc_enabled(struct drm_i915_private *dev_priv);
-void intel_fbc_update(struct drm_i915_private *dev_priv);
+void intel_fbc_update(struct intel_crtc *crtc);
 void intel_fbc_init(struct drm_i915_private *dev_priv);
 void intel_fbc_disable(struct drm_i915_private *dev_priv);
 void intel_fbc_disable_crtc(struct intel_crtc *crtc);
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index bc9cd33..dae994d 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -429,7 +429,7 @@ static void intel_fbc_schedule_enable(struct intel_crtc *crtc)
 	schedule_delayed_work(&work->work, msecs_to_jiffies(50));
 }
 
-static void __intel_fbc_disable(struct drm_i915_private *dev_priv)
+static void intel_fbc_deactivate(struct drm_i915_private *dev_priv)
 {
 	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
 
@@ -437,6 +437,11 @@ static void __intel_fbc_disable(struct drm_i915_private *dev_priv)
 
 	if (dev_priv->fbc.enabled)
 		dev_priv->fbc.disable_fbc(dev_priv);
+}
+
+static void __intel_fbc_disable(struct drm_i915_private *dev_priv)
+{
+	intel_fbc_deactivate(dev_priv);
 	dev_priv->fbc.crtc = NULL;
 }
 
@@ -501,24 +506,6 @@ static bool crtc_is_valid(struct intel_crtc *crtc)
 	return true;
 }
 
-static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
-{
-	struct drm_crtc *crtc = NULL, *tmp_crtc;
-	enum pipe pipe;
-
-	for_each_pipe(dev_priv, pipe) {
-		tmp_crtc = dev_priv->pipe_to_crtc_mapping[pipe];
-
-		if (crtc_is_valid(to_intel_crtc(tmp_crtc)))
-			crtc = tmp_crtc;
-	}
-
-	if (!crtc)
-		return NULL;
-
-	return crtc;
-}
-
 static bool multiple_pipes_ok(struct drm_i915_private *dev_priv)
 {
 	enum pipe pipe;
@@ -804,21 +791,28 @@ static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc)
 
 /**
  * __intel_fbc_update - enable/disable FBC as needed, unlocked
- * @dev_priv: i915 device instance
+ * @crtc: the CRTC that triggered the update
  *
  * This function completely reevaluates the status of FBC, then enables,
  * disables or maintains it on the same state.
  */
-static void __intel_fbc_update(struct drm_i915_private *dev_priv)
+static void __intel_fbc_update(struct intel_crtc *crtc)
 {
-	struct drm_crtc *drm_crtc = NULL;
-	struct intel_crtc *crtc;
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 	struct drm_framebuffer *fb;
 	struct drm_i915_gem_object *obj;
 	const struct drm_display_mode *adjusted_mode;
 
 	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
 
+	if (!multiple_pipes_ok(dev_priv)) {
+		set_no_fbc_reason(dev_priv, "more than one pipe active");
+		goto out_disable;
+	}
+
+	if (dev_priv->fbc.crtc != NULL && dev_priv->fbc.crtc != crtc)
+		return;
+
 	if (intel_vgpu_active(dev_priv->dev))
 		i915.enable_fbc = 0;
 
@@ -832,18 +826,11 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
 		goto out_disable;
 	}
 
-	drm_crtc = intel_fbc_find_crtc(dev_priv);
-	if (!drm_crtc) {
+	if (!crtc_is_valid(crtc)) {
 		set_no_fbc_reason(dev_priv, "no output");
 		goto out_disable;
 	}
 
-	if (!multiple_pipes_ok(dev_priv)) {
-		set_no_fbc_reason(dev_priv, "more than one pipe active");
-		goto out_disable;
-	}
-
-	crtc = to_intel_crtc(drm_crtc);
 	fb = crtc->base.primary->fb;
 	obj = intel_fb_obj(fb);
 	adjusted_mode = &crtc->config->base.adjusted_mode;
@@ -909,7 +896,8 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
 	 */
 	if (dev_priv->fbc.crtc == crtc &&
 	    dev_priv->fbc.fb_id == fb->base.id &&
-	    dev_priv->fbc.y == crtc->base.y)
+	    dev_priv->fbc.y == crtc->base.y &&
+	    dev_priv->fbc.enabled)
 		return;
 
 	if (intel_fbc_enabled(dev_priv)) {
@@ -955,17 +943,19 @@ out_disable:
 
 /*
  * intel_fbc_update - enable/disable FBC as needed
- * @dev_priv: i915 device instance
+ * @crtc: the CRTC that triggered the update
  *
  * This function reevaluates the overall state and enables or disables FBC.
  */
-void intel_fbc_update(struct drm_i915_private *dev_priv)
+void intel_fbc_update(struct intel_crtc *crtc)
 {
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+
 	if (!fbc_supported(dev_priv))
 		return;
 
 	mutex_lock(&dev_priv->fbc.lock);
-	__intel_fbc_update(dev_priv);
+	__intel_fbc_update(crtc);
 	mutex_unlock(&dev_priv->fbc.lock);
 }
 
@@ -991,7 +981,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 	dev_priv->fbc.busy_bits |= (fbc_bits & frontbuffer_bits);
 
 	if (dev_priv->fbc.busy_bits)
-		__intel_fbc_disable(dev_priv);
+		intel_fbc_deactivate(dev_priv);
 
 	mutex_unlock(&dev_priv->fbc.lock);
 }
@@ -1009,9 +999,9 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
 
 	dev_priv->fbc.busy_bits &= ~frontbuffer_bits;
 
-	if (!dev_priv->fbc.busy_bits) {
-		__intel_fbc_disable(dev_priv);
-		__intel_fbc_update(dev_priv);
+	if (!dev_priv->fbc.busy_bits && dev_priv->fbc.crtc) {
+		intel_fbc_deactivate(dev_priv);
+		__intel_fbc_update(dev_priv->fbc.crtc);
 	}
 
 	mutex_unlock(&dev_priv->fbc.lock);
-- 
2.6.2

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

[PATCH 04/12] drm/i915: introduce is_active/activate/deactivate to the FBC terminology

[Paulo Zanoni]

The long term goal is to have enable/disable as the higher level
functions and activate/deactivate as the lower level functions, just
like we do for PSR and for the CRTC. This way, we'll run enable and
disable once per modeset, while update, activate and deactivate will
be run many times. With this, we can move the checks and code that
need to run only once per modeset to enable(), making the code simpler
and possibly a little faster.

This patch is just the first step on the conversion: it starts by
converting the current low level functions from enable/disable to
activate/deactivate. This patch by itself has no benefits other than
making review and rebase easier. Please see the next patches for more
details on the conversion.

v2:
  - Rebase.
  - Improve commit message (Chris).
v3: Rebase after changing the patch order.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |   2 +-
 drivers/gpu/drm/i915/i915_drv.h      |  10 ++-
 drivers/gpu/drm/i915/intel_display.c |   4 +-
 drivers/gpu/drm/i915/intel_drv.h     |   2 +-
 drivers/gpu/drm/i915/intel_fbc.c     | 114 +++++++++++++++++------------------
 drivers/gpu/drm/i915/intel_pm.c      |   2 +-
 6 files changed, 66 insertions(+), 68 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index d6d69f4..b05e8f3 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1639,7 +1639,7 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
 	intel_runtime_pm_get(dev_priv);
 	mutex_lock(&dev_priv->fbc.lock);
 
-	if (intel_fbc_enabled(dev_priv))
+	if (intel_fbc_is_active(dev_priv))
 		seq_puts(m, "FBC enabled\n");
 	else
 		seq_printf(m, "FBC disabled: %s\n",
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 317414b..6464006 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -914,9 +914,7 @@ struct i915_fbc {
 
 	bool false_color;
 
-	/* Tracks whether the HW is actually enabled, not whether the feature is
-	 * possible. */
-	bool enabled;
+	bool active;
 
 	struct intel_fbc_work {
 		struct delayed_work work;
@@ -925,9 +923,9 @@ struct i915_fbc {
 
 	const char *no_fbc_reason;
 
-	bool (*fbc_enabled)(struct drm_i915_private *dev_priv);
-	void (*enable_fbc)(struct intel_crtc *crtc);
-	void (*disable_fbc)(struct drm_i915_private *dev_priv);
+	bool (*is_active)(struct drm_i915_private *dev_priv);
+	void (*activate)(struct intel_crtc *crtc);
+	void (*deactivate)(struct drm_i915_private *dev_priv);
 };
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1040705..a1e032a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3176,8 +3176,8 @@ intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (dev_priv->fbc.disable_fbc)
-		dev_priv->fbc.disable_fbc(dev_priv);
+	if (dev_priv->fbc.deactivate)
+		dev_priv->fbc.deactivate(dev_priv);
 
 	dev_priv->display.update_primary_plane(crtc, fb, x, y);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 91458fb..285db57 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1322,7 +1322,7 @@ static inline void intel_fbdev_restore_mode(struct drm_device *dev)
 #endif
 
 /* intel_fbc.c */
-bool intel_fbc_enabled(struct drm_i915_private *dev_priv);
+bool intel_fbc_is_active(struct drm_i915_private *dev_priv);
 void intel_fbc_update(struct intel_crtc *crtc);
 void intel_fbc_init(struct drm_i915_private *dev_priv);
 void intel_fbc_disable(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index dae994d..1650060 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -43,7 +43,7 @@
 
 static inline bool fbc_supported(struct drm_i915_private *dev_priv)
 {
-	return dev_priv->fbc.enable_fbc != NULL;
+	return dev_priv->fbc.activate != NULL;
 }
 
 static inline bool fbc_on_pipe_a_only(struct drm_i915_private *dev_priv)
@@ -64,11 +64,11 @@ static unsigned int get_crtc_fence_y_offset(struct intel_crtc *crtc)
 	return crtc->base.y - crtc->adjusted_y;
 }
 
-static void i8xx_fbc_disable(struct drm_i915_private *dev_priv)
+static void i8xx_fbc_deactivate(struct drm_i915_private *dev_priv)
 {
 	u32 fbc_ctl;
 
-	dev_priv->fbc.enabled = false;
+	dev_priv->fbc.active = false;
 
 	/* Disable compression */
 	fbc_ctl = I915_READ(FBC_CONTROL);
@@ -84,10 +84,10 @@ static void i8xx_fbc_disable(struct drm_i915_private *dev_priv)
 		return;
 	}
 
-	DRM_DEBUG_KMS("disabled FBC\n");
+	DRM_DEBUG_KMS("deactivated FBC\n");
 }
 
-static void i8xx_fbc_enable(struct intel_crtc *crtc)
+static void i8xx_fbc_activate(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 	struct drm_framebuffer *fb = crtc->base.primary->fb;
@@ -96,7 +96,7 @@ static void i8xx_fbc_enable(struct intel_crtc *crtc)
 	int i;
 	u32 fbc_ctl;
 
-	dev_priv->fbc.enabled = true;
+	dev_priv->fbc.active = true;
 
 	/* Note: fbc.threshold == 1 for i8xx */
 	cfb_pitch = dev_priv->fbc.uncompressed_size / FBC_LL_SIZE;
@@ -133,23 +133,23 @@ static void i8xx_fbc_enable(struct intel_crtc *crtc)
 	fbc_ctl |= obj->fence_reg;
 	I915_WRITE(FBC_CONTROL, fbc_ctl);
 
-	DRM_DEBUG_KMS("enabled FBC, pitch %d, yoff %d, plane %c\n",
+	DRM_DEBUG_KMS("activated FBC, pitch %d, yoff %d, plane %c\n",
 		      cfb_pitch, crtc->base.y, plane_name(crtc->plane));
 }
 
-static bool i8xx_fbc_enabled(struct drm_i915_private *dev_priv)
+static bool i8xx_fbc_is_active(struct drm_i915_private *dev_priv)
 {
 	return I915_READ(FBC_CONTROL) & FBC_CTL_EN;
 }
 
-static void g4x_fbc_enable(struct intel_crtc *crtc)
+static void g4x_fbc_activate(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 	struct drm_framebuffer *fb = crtc->base.primary->fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	u32 dpfc_ctl;
 
-	dev_priv->fbc.enabled = true;
+	dev_priv->fbc.active = true;
 
 	dpfc_ctl = DPFC_CTL_PLANE(crtc->plane) | DPFC_SR_EN;
 	if (drm_format_plane_cpp(fb->pixel_format, 0) == 2)
@@ -163,14 +163,14 @@ static void g4x_fbc_enable(struct intel_crtc *crtc)
 	/* enable it... */
 	I915_WRITE(DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
 
-	DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(crtc->plane));
+	DRM_DEBUG_KMS("activated fbc on plane %c\n", plane_name(crtc->plane));
 }
 
-static void g4x_fbc_disable(struct drm_i915_private *dev_priv)
+static void g4x_fbc_deactivate(struct drm_i915_private *dev_priv)
 {
 	u32 dpfc_ctl;
 
-	dev_priv->fbc.enabled = false;
+	dev_priv->fbc.active = false;
 
 	/* Disable compression */
 	dpfc_ctl = I915_READ(DPFC_CONTROL);
@@ -178,11 +178,11 @@ static void g4x_fbc_disable(struct drm_i915_private *dev_priv)
 		dpfc_ctl &= ~DPFC_CTL_EN;
 		I915_WRITE(DPFC_CONTROL, dpfc_ctl);
 
-		DRM_DEBUG_KMS("disabled FBC\n");
+		DRM_DEBUG_KMS("deactivated FBC\n");
 	}
 }
 
-static bool g4x_fbc_enabled(struct drm_i915_private *dev_priv)
+static bool g4x_fbc_is_active(struct drm_i915_private *dev_priv)
 {
 	return I915_READ(DPFC_CONTROL) & DPFC_CTL_EN;
 }
@@ -194,7 +194,7 @@ static void intel_fbc_recompress(struct drm_i915_private *dev_priv)
 	POSTING_READ(MSG_FBC_REND_STATE);
 }
 
-static void ilk_fbc_enable(struct intel_crtc *crtc)
+static void ilk_fbc_activate(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 	struct drm_framebuffer *fb = crtc->base.primary->fb;
@@ -203,7 +203,7 @@ static void ilk_fbc_enable(struct intel_crtc *crtc)
 	int threshold = dev_priv->fbc.threshold;
 	unsigned int y_offset;
 
-	dev_priv->fbc.enabled = true;
+	dev_priv->fbc.active = true;
 
 	dpfc_ctl = DPFC_CTL_PLANE(crtc->plane);
 	if (drm_format_plane_cpp(fb->pixel_format, 0) == 2)
@@ -239,14 +239,14 @@ static void ilk_fbc_enable(struct intel_crtc *crtc)
 
 	intel_fbc_recompress(dev_priv);
 
-	DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(crtc->plane));
+	DRM_DEBUG_KMS("activated fbc on plane %c\n", plane_name(crtc->plane));
 }
 
-static void ilk_fbc_disable(struct drm_i915_private *dev_priv)
+static void ilk_fbc_deactivate(struct drm_i915_private *dev_priv)
 {
 	u32 dpfc_ctl;
 
-	dev_priv->fbc.enabled = false;
+	dev_priv->fbc.active = false;
 
 	/* Disable compression */
 	dpfc_ctl = I915_READ(ILK_DPFC_CONTROL);
@@ -254,16 +254,16 @@ static void ilk_fbc_disable(struct drm_i915_private *dev_priv)
 		dpfc_ctl &= ~DPFC_CTL_EN;
 		I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl);
 
-		DRM_DEBUG_KMS("disabled FBC\n");
+		DRM_DEBUG_KMS("deactivated FBC\n");
 	}
 }
 
-static bool ilk_fbc_enabled(struct drm_i915_private *dev_priv)
+static bool ilk_fbc_is_active(struct drm_i915_private *dev_priv)
 {
 	return I915_READ(ILK_DPFC_CONTROL) & DPFC_CTL_EN;
 }
 
-static void gen7_fbc_enable(struct intel_crtc *crtc)
+static void gen7_fbc_activate(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 	struct drm_framebuffer *fb = crtc->base.primary->fb;
@@ -271,7 +271,7 @@ static void gen7_fbc_enable(struct intel_crtc *crtc)
 	u32 dpfc_ctl;
 	int threshold = dev_priv->fbc.threshold;
 
-	dev_priv->fbc.enabled = true;
+	dev_priv->fbc.active = true;
 
 	dpfc_ctl = 0;
 	if (IS_IVYBRIDGE(dev_priv))
@@ -318,28 +318,28 @@ static void gen7_fbc_enable(struct intel_crtc *crtc)
 
 	intel_fbc_recompress(dev_priv);
 
-	DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(crtc->plane));
+	DRM_DEBUG_KMS("activated fbc on plane %c\n", plane_name(crtc->plane));
 }
 
 /**
- * intel_fbc_enabled - Is FBC enabled?
+ * intel_fbc_is_active - Is FBC active?
  * @dev_priv: i915 device instance
  *
  * This function is used to verify the current state of FBC.
  * FIXME: This should be tracked in the plane config eventually
  *        instead of queried at runtime for most callers.
  */
-bool intel_fbc_enabled(struct drm_i915_private *dev_priv)
+bool intel_fbc_is_active(struct drm_i915_private *dev_priv)
 {
-	return dev_priv->fbc.enabled;
+	return dev_priv->fbc.active;
 }
 
-static void intel_fbc_enable(const struct drm_framebuffer *fb)
+static void intel_fbc_activate(const struct drm_framebuffer *fb)
 {
 	struct drm_i915_private *dev_priv = fb->dev->dev_private;
 	struct intel_crtc *crtc = dev_priv->fbc.crtc;
 
-	dev_priv->fbc.enable_fbc(crtc);
+	dev_priv->fbc.activate(crtc);
 
 	dev_priv->fbc.fb_id = fb->base.id;
 	dev_priv->fbc.y = crtc->base.y;
@@ -359,7 +359,7 @@ static void intel_fbc_work_fn(struct work_struct *__work)
 		 * the prior work.
 		 */
 		if (crtc_fb == work->fb)
-			intel_fbc_enable(work->fb);
+			intel_fbc_activate(work->fb);
 
 		dev_priv->fbc.fbc_work = NULL;
 	}
@@ -391,7 +391,7 @@ static void intel_fbc_cancel_work(struct drm_i915_private *dev_priv)
 	dev_priv->fbc.fbc_work = NULL;
 }
 
-static void intel_fbc_schedule_enable(struct intel_crtc *crtc)
+static void intel_fbc_schedule_activation(struct intel_crtc *crtc)
 {
 	struct intel_fbc_work *work;
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
@@ -404,7 +404,7 @@ static void intel_fbc_schedule_enable(struct intel_crtc *crtc)
 	work = kzalloc(sizeof(*work), GFP_KERNEL);
 	if (work == NULL) {
 		DRM_ERROR("Failed to allocate FBC work structure\n");
-		intel_fbc_enable(crtc->base.primary->fb);
+		intel_fbc_activate(crtc->base.primary->fb);
 		return;
 	}
 
@@ -435,8 +435,8 @@ static void intel_fbc_deactivate(struct drm_i915_private *dev_priv)
 
 	intel_fbc_cancel_work(dev_priv);
 
-	if (dev_priv->fbc.enabled)
-		dev_priv->fbc.disable_fbc(dev_priv);
+	if (dev_priv->fbc.active)
+		dev_priv->fbc.deactivate(dev_priv);
 }
 
 static void __intel_fbc_disable(struct drm_i915_private *dev_priv)
@@ -897,10 +897,10 @@ static void __intel_fbc_update(struct intel_crtc *crtc)
 	if (dev_priv->fbc.crtc == crtc &&
 	    dev_priv->fbc.fb_id == fb->base.id &&
 	    dev_priv->fbc.y == crtc->base.y &&
-	    dev_priv->fbc.enabled)
+	    dev_priv->fbc.active)
 		return;
 
-	if (intel_fbc_enabled(dev_priv)) {
+	if (intel_fbc_is_active(dev_priv)) {
 		/* We update FBC along two paths, after changing fb/crtc
 		 * configuration (modeswitching) and after page-flipping
 		 * finishes. For the latter, we know that not only did
@@ -928,13 +928,13 @@ static void __intel_fbc_update(struct intel_crtc *crtc)
 		__intel_fbc_disable(dev_priv);
 	}
 
-	intel_fbc_schedule_enable(crtc);
+	intel_fbc_schedule_activation(crtc);
 	dev_priv->fbc.no_fbc_reason = "FBC enabled (not necessarily active)";
 	return;
 
 out_disable:
 	/* Multiple disables should be harmless */
-	if (intel_fbc_enabled(dev_priv)) {
+	if (intel_fbc_is_active(dev_priv)) {
 		DRM_DEBUG_KMS("unsupported config, disabling FBC\n");
 		__intel_fbc_disable(dev_priv);
 	}
@@ -973,7 +973,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 
 	mutex_lock(&dev_priv->fbc.lock);
 
-	if (dev_priv->fbc.enabled || dev_priv->fbc.fbc_work)
+	if (dev_priv->fbc.active || dev_priv->fbc.fbc_work)
 		fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe);
 	else
 		fbc_bits = dev_priv->fbc.possible_framebuffer_bits;
@@ -1018,7 +1018,7 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
 	enum pipe pipe;
 
 	mutex_init(&dev_priv->fbc.lock);
-	dev_priv->fbc.enabled = false;
+	dev_priv->fbc.active = false;
 
 	if (!HAS_FBC(dev_priv)) {
 		dev_priv->fbc.no_fbc_reason = "unsupported by this chipset";
@@ -1034,29 +1034,29 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
 	}
 
 	if (INTEL_INFO(dev_priv)->gen >= 7) {
-		dev_priv->fbc.fbc_enabled = ilk_fbc_enabled;
-		dev_priv->fbc.enable_fbc = gen7_fbc_enable;
-		dev_priv->fbc.disable_fbc = ilk_fbc_disable;
+		dev_priv->fbc.is_active = ilk_fbc_is_active;
+		dev_priv->fbc.activate = gen7_fbc_activate;
+		dev_priv->fbc.deactivate = ilk_fbc_deactivate;
 	} else if (INTEL_INFO(dev_priv)->gen >= 5) {
-		dev_priv->fbc.fbc_enabled = ilk_fbc_enabled;
-		dev_priv->fbc.enable_fbc = ilk_fbc_enable;
-		dev_priv->fbc.disable_fbc = ilk_fbc_disable;
+		dev_priv->fbc.is_active = ilk_fbc_is_active;
+		dev_priv->fbc.activate = ilk_fbc_activate;
+		dev_priv->fbc.deactivate = ilk_fbc_deactivate;
 	} else if (IS_GM45(dev_priv)) {
-		dev_priv->fbc.fbc_enabled = g4x_fbc_enabled;
-		dev_priv->fbc.enable_fbc = g4x_fbc_enable;
-		dev_priv->fbc.disable_fbc = g4x_fbc_disable;
+		dev_priv->fbc.is_active = g4x_fbc_is_active;
+		dev_priv->fbc.activate = g4x_fbc_activate;
+		dev_priv->fbc.deactivate = g4x_fbc_deactivate;
 	} else {
-		dev_priv->fbc.fbc_enabled = i8xx_fbc_enabled;
-		dev_priv->fbc.enable_fbc = i8xx_fbc_enable;
-		dev_priv->fbc.disable_fbc = i8xx_fbc_disable;
+		dev_priv->fbc.is_active = i8xx_fbc_is_active;
+		dev_priv->fbc.activate = i8xx_fbc_activate;
+		dev_priv->fbc.deactivate = i8xx_fbc_deactivate;
 
 		/* This value was pulled out of someone's hat */
 		I915_WRITE(FBC_CONTROL, 500 << FBC_CTL_INTERVAL_SHIFT);
 	}
 
 	/* We still don't have any sort of hardware state readout for FBC, so
-	 * disable it in case the BIOS enabled it to make sure software matches
-	 * the hardware state. */
-	if (dev_priv->fbc.fbc_enabled(dev_priv))
-		dev_priv->fbc.disable_fbc(dev_priv);
+	 * deactivate it in case the BIOS activated it to make sure software
+	 * matches the hardware state. */
+	if (dev_priv->fbc.is_active(dev_priv))
+		dev_priv->fbc.deactivate(dev_priv);
 }
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 647c0ff..35194c9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2422,7 +2422,7 @@ static void ilk_wm_merge(struct drm_device *dev,
 	 * enabled sometime later.
 	 */
 	if (IS_GEN5(dev) && !merged->fbc_wm_enabled &&
-	    intel_fbc_enabled(dev_priv)) {
+	    intel_fbc_is_active(dev_priv)) {
 		for (level = 2; level <= max_level; level++) {
 			struct intel_wm_level *wm = &merged->wm[level];
 
-- 
2.6.2

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

[PATCH 05/12] drm/i915: introduce intel_fbc_{enable, disable}

[Paulo Zanoni]

The goal is to call FBC enable/disable only once per modeset, while
activate/deactivate/update will be called multiple times.

The enable() function will be responsible for deciding if a CRTC will
have FBC on it and then it will "lock" FBC on this CRTC: it won't be
possible to change FBC's CRTC until disable(). With this, all checks
and resource acquisition that only need to be done once per modeset
can be moved from update() to enable(). And then the update(),
activate() and deactivate() code will also get simpler since they
won't need to worry about the CRTC being changed.

The disable() function will do the reverse operation of enable(). One
of its features is that it should only be called while the pipe is
already off. This guarantees that FBC is stopped and nothing is
using the CFB.

With this, the activate() and deactivate() functions just start and
temporarily stop FBC. They are the ones touching the hardware enable
bit, so HW state reflects dev_priv->crtc.active.

The last function remaining is update(). A lot of times I thought
about renaming update() to activate() or try_to_activate() since it's
called when we want to activate FBC. The thing is that update() may
not only decide to activate FBC, but also deactivate or keep it on the
same state, so I'll leave this name for now.

Moving code to enable() and disable() will also help in case we decide
to move FBC to pipe_config or something else later.

The current patch only puts the very basic code on enable() and
disable(). The next commits will take care of moving more stuff from
update() to the new functions.

v2:
  - Rebase.
  - Improve commit message (Chris).
v3: Rebase after changing the patch order.
v4: Rebase again after upstream changes.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |   1 +
 drivers/gpu/drm/i915/intel_display.c |  16 ++-
 drivers/gpu/drm/i915/intel_drv.h     |   2 +
 drivers/gpu/drm/i915/intel_fbc.c     | 196 +++++++++++++++++++++++++----------
 4 files changed, 157 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6464006..92bb8e1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -914,6 +914,7 @@ struct i915_fbc {
 
 	bool false_color;
 
+	bool enabled;
 	bool active;
 
 	struct intel_fbc_work {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a1e032a..2fe7c25 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4789,7 +4789,7 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
 	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
 
 	if (atomic->disable_fbc)
-		intel_fbc_disable_crtc(crtc);
+		intel_fbc_deactivate(crtc);
 
 	if (crtc->atomic.disable_ips)
 		hsw_disable_ips(crtc);
@@ -4897,6 +4897,8 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	if (intel_crtc->config->has_pch_encoder)
 		intel_wait_for_vblank(dev, pipe);
 	intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
+
+	intel_fbc_enable(intel_crtc);
 }
 
 /* IPS only exists on ULT machines and is tied to pipe A. */
@@ -5004,6 +5006,8 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 		intel_wait_for_vblank(dev, hsw_workaround_pipe);
 		intel_wait_for_vblank(dev, hsw_workaround_pipe);
 	}
+
+	intel_fbc_enable(intel_crtc);
 }
 
 static void ironlake_pfit_disable(struct intel_crtc *crtc, bool force)
@@ -5072,6 +5076,8 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	}
 
 	intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
+
+	intel_fbc_disable_crtc(intel_crtc);
 }
 
 static void haswell_crtc_disable(struct drm_crtc *crtc)
@@ -5123,6 +5129,8 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	if (intel_crtc->config->has_pch_encoder)
 		intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
 						      true);
+
+	intel_fbc_disable_crtc(intel_crtc);
 }
 
 static void i9xx_pfit_enable(struct intel_crtc *crtc)
@@ -6188,6 +6196,8 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->enable(encoder);
+
+	intel_fbc_enable(intel_crtc);
 }
 
 static void i9xx_pfit_disable(struct intel_crtc *crtc)
@@ -6250,6 +6260,8 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 
 	if (!IS_GEN2(dev))
 		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
+
+	intel_fbc_disable_crtc(intel_crtc);
 }
 
 static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
@@ -11523,7 +11535,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 			  to_intel_plane(primary)->frontbuffer_bit);
 	mutex_unlock(&dev->struct_mutex);
 
-	intel_fbc_disable_crtc(intel_crtc);
+	intel_fbc_deactivate(intel_crtc);
 	intel_frontbuffer_flip_prepare(dev,
 				       to_intel_plane(primary)->frontbuffer_bit);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 285db57..2103e8f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1323,8 +1323,10 @@ static inline void intel_fbdev_restore_mode(struct drm_device *dev)
 
 /* intel_fbc.c */
 bool intel_fbc_is_active(struct drm_i915_private *dev_priv);
+void intel_fbc_deactivate(struct intel_crtc *crtc);
 void intel_fbc_update(struct intel_crtc *crtc);
 void intel_fbc_init(struct drm_i915_private *dev_priv);
+void intel_fbc_enable(struct intel_crtc *crtc);
 void intel_fbc_disable(struct drm_i915_private *dev_priv);
 void intel_fbc_disable_crtc(struct intel_crtc *crtc);
 void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 1650060..719fcf6 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -399,7 +399,6 @@ static void intel_fbc_schedule_activation(struct intel_crtc *crtc)
 	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
 
 	intel_fbc_cancel_work(dev_priv);
-	dev_priv->fbc.crtc = crtc;
 
 	work = kzalloc(sizeof(*work), GFP_KERNEL);
 	if (work == NULL) {
@@ -429,7 +428,7 @@ static void intel_fbc_schedule_activation(struct intel_crtc *crtc)
 	schedule_delayed_work(&work->work, msecs_to_jiffies(50));
 }
 
-static void intel_fbc_deactivate(struct drm_i915_private *dev_priv)
+static void __intel_fbc_deactivate(struct drm_i915_private *dev_priv)
 {
 	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
 
@@ -439,35 +438,13 @@ static void intel_fbc_deactivate(struct drm_i915_private *dev_priv)
 		dev_priv->fbc.deactivate(dev_priv);
 }
 
-static void __intel_fbc_disable(struct drm_i915_private *dev_priv)
-{
-	intel_fbc_deactivate(dev_priv);
-	dev_priv->fbc.crtc = NULL;
-}
-
-/**
- * intel_fbc_disable - disable FBC
- * @dev_priv: i915 device instance
- *
- * This function disables FBC.
- */
-void intel_fbc_disable(struct drm_i915_private *dev_priv)
-{
-	if (!fbc_supported(dev_priv))
-		return;
-
-	mutex_lock(&dev_priv->fbc.lock);
-	__intel_fbc_disable(dev_priv);
-	mutex_unlock(&dev_priv->fbc.lock);
-}
-
 /*
- * intel_fbc_disable_crtc - disable FBC if it's associated with crtc
+ * intel_fbc_deactivate - deactivate FBC if it's associated with crtc
  * @crtc: the CRTC
  *
- * This function disables FBC if it's associated with the provided CRTC.
+ * This function deactivates FBC if it's associated with the provided CRTC.
  */
-void intel_fbc_disable_crtc(struct intel_crtc *crtc)
+void intel_fbc_deactivate(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 
@@ -476,7 +453,7 @@ void intel_fbc_disable_crtc(struct intel_crtc *crtc)
 
 	mutex_lock(&dev_priv->fbc.lock);
 	if (dev_priv->fbc.crtc == crtc)
-		__intel_fbc_disable(dev_priv);
+		__intel_fbc_deactivate(dev_priv);
 	mutex_unlock(&dev_priv->fbc.lock);
 }
 
@@ -490,13 +467,18 @@ static void set_no_fbc_reason(struct drm_i915_private *dev_priv,
 	DRM_DEBUG_KMS("Disabling FBC: %s\n", reason);
 }
 
-static bool crtc_is_valid(struct intel_crtc *crtc)
+static bool crtc_can_fbc(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 
 	if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A)
 		return false;
 
+	return true;
+}
+
+static bool crtc_is_valid(struct intel_crtc *crtc)
+{
 	if (!intel_crtc_active(&crtc->base))
 		return false;
 
@@ -790,11 +772,11 @@ static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc)
 }
 
 /**
- * __intel_fbc_update - enable/disable FBC as needed, unlocked
+ * __intel_fbc_update - activate/deactivate FBC as needed, unlocked
  * @crtc: the CRTC that triggered the update
  *
- * This function completely reevaluates the status of FBC, then enables,
- * disables or maintains it on the same state.
+ * This function completely reevaluates the status of FBC, then activates,
+ * deactivates or maintains it on the same state.
  */
 static void __intel_fbc_update(struct intel_crtc *crtc)
 {
@@ -810,22 +792,9 @@ static void __intel_fbc_update(struct intel_crtc *crtc)
 		goto out_disable;
 	}
 
-	if (dev_priv->fbc.crtc != NULL && dev_priv->fbc.crtc != crtc)
+	if (!dev_priv->fbc.enabled || dev_priv->fbc.crtc != crtc)
 		return;
 
-	if (intel_vgpu_active(dev_priv->dev))
-		i915.enable_fbc = 0;
-
-	if (i915.enable_fbc < 0) {
-		set_no_fbc_reason(dev_priv, "disabled per chip default");
-		goto out_disable;
-	}
-
-	if (!i915.enable_fbc) {
-		set_no_fbc_reason(dev_priv, "disabled per module param");
-		goto out_disable;
-	}
-
 	if (!crtc_is_valid(crtc)) {
 		set_no_fbc_reason(dev_priv, "no output");
 		goto out_disable;
@@ -924,8 +893,8 @@ static void __intel_fbc_update(struct intel_crtc *crtc)
 		 * disabling paths we do need to wait for a vblank at
 		 * some point. And we wait before enabling FBC anyway.
 		 */
-		DRM_DEBUG_KMS("disabling active FBC for update\n");
-		__intel_fbc_disable(dev_priv);
+		DRM_DEBUG_KMS("deactivating FBC for update\n");
+		__intel_fbc_deactivate(dev_priv);
 	}
 
 	intel_fbc_schedule_activation(crtc);
@@ -935,17 +904,17 @@ static void __intel_fbc_update(struct intel_crtc *crtc)
 out_disable:
 	/* Multiple disables should be harmless */
 	if (intel_fbc_is_active(dev_priv)) {
-		DRM_DEBUG_KMS("unsupported config, disabling FBC\n");
-		__intel_fbc_disable(dev_priv);
+		DRM_DEBUG_KMS("unsupported config, deactivating FBC\n");
+		__intel_fbc_deactivate(dev_priv);
 	}
 	__intel_fbc_cleanup_cfb(dev_priv);
 }
 
 /*
- * intel_fbc_update - enable/disable FBC as needed
+ * intel_fbc_update - activate/deactivate FBC as needed
  * @crtc: the CRTC that triggered the update
  *
- * This function reevaluates the overall state and enables or disables FBC.
+ * This function reevaluates the overall state and activates or deactivates FBC.
  */
 void intel_fbc_update(struct intel_crtc *crtc)
 {
@@ -973,7 +942,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 
 	mutex_lock(&dev_priv->fbc.lock);
 
-	if (dev_priv->fbc.active || dev_priv->fbc.fbc_work)
+	if (dev_priv->fbc.enabled)
 		fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe);
 	else
 		fbc_bits = dev_priv->fbc.possible_framebuffer_bits;
@@ -981,7 +950,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 	dev_priv->fbc.busy_bits |= (fbc_bits & frontbuffer_bits);
 
 	if (dev_priv->fbc.busy_bits)
-		intel_fbc_deactivate(dev_priv);
+		__intel_fbc_deactivate(dev_priv);
 
 	mutex_unlock(&dev_priv->fbc.lock);
 }
@@ -999,8 +968,8 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
 
 	dev_priv->fbc.busy_bits &= ~frontbuffer_bits;
 
-	if (!dev_priv->fbc.busy_bits && dev_priv->fbc.crtc) {
-		intel_fbc_deactivate(dev_priv);
+	if (!dev_priv->fbc.busy_bits && dev_priv->fbc.enabled) {
+		__intel_fbc_deactivate(dev_priv);
 		__intel_fbc_update(dev_priv->fbc.crtc);
 	}
 
@@ -1008,6 +977,120 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
 }
 
 /**
+ * intel_fbc_enable: tries to enable FBC on the CRTC
+ * @crtc: the CRTC
+ *
+ * This function checks if it's possible to enable FBC on the following CRTC,
+ * then enables it. Notice that it doesn't activate FBC.
+ */
+void intel_fbc_enable(struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+
+	if (!fbc_supported(dev_priv))
+		return;
+
+	mutex_lock(&dev_priv->fbc.lock);
+
+	if (dev_priv->fbc.enabled) {
+		WARN_ON(dev_priv->fbc.crtc == crtc);
+		goto out;
+	}
+
+	WARN_ON(dev_priv->fbc.active);
+	WARN_ON(dev_priv->fbc.crtc != NULL);
+
+	if (intel_vgpu_active(dev_priv->dev)) {
+		set_no_fbc_reason(dev_priv, "VGPU is active");
+		goto out;
+	}
+
+	if (i915.enable_fbc < 0) {
+		set_no_fbc_reason(dev_priv, "disabled per chip default");
+		goto out;
+	}
+
+	if (!i915.enable_fbc) {
+		set_no_fbc_reason(dev_priv, "disabled per module param");
+		goto out;
+	}
+
+	if (!crtc_can_fbc(crtc)) {
+		set_no_fbc_reason(dev_priv, "no enabled pipes can have FBC");
+		goto out;
+	}
+
+	DRM_DEBUG_KMS("Enabling FBC on pipe %c\n", pipe_name(crtc->pipe));
+	dev_priv->fbc.no_fbc_reason = "FBC enabled but not active yet\n";
+
+	dev_priv->fbc.enabled = true;
+	dev_priv->fbc.crtc = crtc;
+out:
+	mutex_unlock(&dev_priv->fbc.lock);
+}
+
+/**
+ * __intel_fbc_disable - disable FBC
+ * @dev_priv: i915 device instance
+ *
+ * This is the low level function that actually disables FBC. Callers should
+ * grab the FBC lock.
+ */
+static void __intel_fbc_disable(struct drm_i915_private *dev_priv)
+{
+	struct intel_crtc *crtc = dev_priv->fbc.crtc;
+
+	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
+	WARN_ON(!dev_priv->fbc.enabled);
+	WARN_ON(dev_priv->fbc.active);
+	assert_pipe_disabled(dev_priv, crtc->pipe);
+
+	DRM_DEBUG_KMS("Disabling FBC on pipe %c\n", pipe_name(crtc->pipe));
+
+	dev_priv->fbc.enabled = false;
+	dev_priv->fbc.crtc = NULL;
+}
+
+/**
+ * intel_fbc_disable_crtc - disable FBC if it's associated with crtc
+ * @crtc: the CRTC
+ *
+ * This function disables FBC if it's associated with the provided CRTC.
+ */
+void intel_fbc_disable_crtc(struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+
+	if (!fbc_supported(dev_priv))
+		return;
+
+	mutex_lock(&dev_priv->fbc.lock);
+	if (dev_priv->fbc.crtc == crtc) {
+		WARN_ON(!dev_priv->fbc.enabled);
+		WARN_ON(dev_priv->fbc.active);
+		__intel_fbc_disable(dev_priv);
+	}
+	mutex_unlock(&dev_priv->fbc.lock);
+}
+
+/**
+ * intel_fbc_disable - globally disable FBC
+ * @dev_priv: i915 device instance
+ *
+ * This function disables FBC regardless of which CRTC is associated with it.
+ */
+void intel_fbc_disable(struct drm_i915_private *dev_priv)
+{
+	if (!fbc_supported(dev_priv))
+		return;
+
+	mutex_lock(&dev_priv->fbc.lock);
+	if (dev_priv->fbc.enabled)
+		__intel_fbc_disable(dev_priv);
+	mutex_unlock(&dev_priv->fbc.lock);
+}
+
+/**
  * intel_fbc_init - Initialize FBC
  * @dev_priv: the i915 device
  *
@@ -1018,6 +1101,7 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
 	enum pipe pipe;
 
 	mutex_init(&dev_priv->fbc.lock);
+	dev_priv->fbc.enabled = false;
 	dev_priv->fbc.active = false;
 
 	if (!HAS_FBC(dev_priv)) {
-- 
2.6.2

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

[PATCH 06/12] drm/i915: alloc/free the FBC CFB during enable/disable

[Paulo Zanoni]

One of the problems with the current code is that it frees the CFB and
releases its drm_mm node as soon as we flip FBC's enable bit. This is
bad because after we disable FBC the hardware may still use the CFB
for the rest of the frame, so in theory we should only release the
drm_mm node one frame after we disable FBC. Otherwise, a stolen memory
allocation done right after an FBC disable may result in either
corrupted memory for the new owner of that memory region or corrupted
screen/underruns in case the new owner changes it while the hardware
is still reading it. This case is not exactly easy to reproduce since
we currently don't do a lot of stolen memory allocations, but I see
patches on the mailing list trying to expose stolen memory to user
space, so races will be possible.

I thought about three different approaches to solve this, and they all
have downsides.

The first approach would be to simply use multiple drm_mm nodes and
freeing the unused ones only after a frame has passed. The problem
with this approach is that since stolen memory is rather small,
there's a risk we just won't be able to allocate a new CFB from stolen
if the previous one was not freed yet. This could happen in case we
quickly disable FBC from pipe A and decide to enable it on pipe B, or
just if we change pipe A's fb stride while FBC is enabled.

The second approach would be similar to the first one, but maintaining
a single drm_mm node and keeping track of when it can be reused. This
would remove the disadvantage of not having enough space for two
nodes, but would create the new problem where we may not be able to
enable FBC at the point intel_fbc_update() is called, so we would have
to add more code to retry updating FBC after the time has passed. And
that can quickly get too complex since we can get invalidate, flush,
disable and other calls in the middle of the wait.

Both solutions above - and also the current code - have the problem
that we unnecessarily free+realloc FBC during invalidate+flush
operations even if the CFB size doesn't change.

The third option would be to move the allocation/deallocation to
enable/disable. This makes sure that the pipe is always disabled when
we allocate/deallocate the CFB, so there's no risk that the FBC
hardware may read or write to the memory right after it is freed from
drm_mm. The downside is that it is possible for user space to change
the buffer stride without triggering a disable/enable - only
deactivate/activate -, so we'll have to handle this case somehow, even
though it is uncommon - see igt's kms_frontbuffer_tracking test,
fbc-stridechange subtest. It could be possible to implement a way to
free+alloc the CFB during said stride change, but it would involve a
lot of book-keeping - exactly as mentioned above - just for a rare
case, so for now I'll keep it simple and just deactivate FBC. Besides,
we may not even need to disable FBC since we do CFB over-allocation.

v2: Rebase after changing the patch order.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 134 ++++++++++++++++++++-------------------
 1 file changed, 69 insertions(+), 65 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 719fcf6..d496a7a 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -64,6 +64,46 @@ static unsigned int get_crtc_fence_y_offset(struct intel_crtc *crtc)
 	return crtc->base.y - crtc->adjusted_y;
 }
 
+/*
+ * For SKL+, the plane source size used by the hardware is based on the value we
+ * write to the PLANE_SIZE register. For BDW-, the hardware looks at the value
+ * we wrote to PIPESRC.
+ */
+static void intel_fbc_get_plane_source_size(struct intel_crtc *crtc,
+					    int *width, int *height)
+{
+	struct intel_plane_state *plane_state =
+			to_intel_plane_state(crtc->base.primary->state);
+	int w, h;
+
+	if (intel_rotation_90_or_270(plane_state->base.rotation)) {
+		w = drm_rect_height(&plane_state->src) >> 16;
+		h = drm_rect_width(&plane_state->src) >> 16;
+	} else {
+		w = drm_rect_width(&plane_state->src) >> 16;
+		h = drm_rect_height(&plane_state->src) >> 16;
+	}
+
+	if (width)
+		*width = w;
+	if (height)
+		*height = h;
+}
+
+static int intel_fbc_calculate_cfb_size(struct intel_crtc *crtc,
+					struct drm_framebuffer *fb)
+{
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	int lines;
+
+	intel_fbc_get_plane_source_size(crtc, NULL, &lines);
+	if (INTEL_INFO(dev_priv)->gen >= 7)
+		lines = min(lines, 2048);
+
+	/* Hardware needs the full buffer stride, not just the active area. */
+	return lines * fb->pitches[0];
+}
+
 static void i8xx_fbc_deactivate(struct drm_i915_private *dev_priv)
 {
 	u32 fbc_ctl;
@@ -558,11 +598,17 @@ again:
 	}
 }
 
-static int intel_fbc_alloc_cfb(struct drm_i915_private *dev_priv, int size,
-			       int fb_cpp)
+static int intel_fbc_alloc_cfb(struct intel_crtc *crtc)
 {
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct drm_framebuffer *fb = crtc->base.primary->state->fb;
 	struct drm_mm_node *uninitialized_var(compressed_llb);
-	int ret;
+	int size, fb_cpp, ret;
+
+	WARN_ON(dev_priv->fbc.compressed_fb.allocated);
+
+	size = intel_fbc_calculate_cfb_size(crtc, fb);
+	fb_cpp = drm_format_plane_cpp(fb->pixel_format, 0);
 
 	ret = find_compression_threshold(dev_priv, &dev_priv->fbc.compressed_fb,
 					 size, fb_cpp);
@@ -639,65 +685,6 @@ void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv)
 	mutex_unlock(&dev_priv->fbc.lock);
 }
 
-/*
- * For SKL+, the plane source size used by the hardware is based on the value we
- * write to the PLANE_SIZE register. For BDW-, the hardware looks at the value
- * we wrote to PIPESRC.
- */
-static void intel_fbc_get_plane_source_size(struct intel_crtc *crtc,
-					    int *width, int *height)
-{
-	struct intel_plane_state *plane_state =
-			to_intel_plane_state(crtc->base.primary->state);
-	int w, h;
-
-	if (intel_rotation_90_or_270(plane_state->base.rotation)) {
-		w = drm_rect_height(&plane_state->src) >> 16;
-		h = drm_rect_width(&plane_state->src) >> 16;
-	} else {
-		w = drm_rect_width(&plane_state->src) >> 16;
-		h = drm_rect_height(&plane_state->src) >> 16;
-	}
-
-	if (width)
-		*width = w;
-	if (height)
-		*height = h;
-}
-
-static int intel_fbc_calculate_cfb_size(struct intel_crtc *crtc)
-{
-	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
-	struct drm_framebuffer *fb = crtc->base.primary->fb;
-	int lines;
-
-	intel_fbc_get_plane_source_size(crtc, NULL, &lines);
-	if (INTEL_INFO(dev_priv)->gen >= 7)
-		lines = min(lines, 2048);
-
-	/* Hardware needs the full buffer stride, not just the active area. */
-	return lines * fb->pitches[0];
-}
-
-static int intel_fbc_setup_cfb(struct intel_crtc *crtc)
-{
-	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
-	struct drm_framebuffer *fb = crtc->base.primary->fb;
-	int size, cpp;
-
-	size = intel_fbc_calculate_cfb_size(crtc);
-	cpp = drm_format_plane_cpp(fb->pixel_format, 0);
-
-	if (dev_priv->fbc.compressed_fb.allocated &&
-	    size <= dev_priv->fbc.compressed_fb.size * dev_priv->fbc.threshold)
-		return 0;
-
-	/* Release any current block */
-	__intel_fbc_cleanup_cfb(dev_priv);
-
-	return intel_fbc_alloc_cfb(dev_priv, size, cpp);
-}
-
 static bool stride_is_valid(struct drm_i915_private *dev_priv,
 			    unsigned int stride)
 {
@@ -853,8 +840,19 @@ static void __intel_fbc_update(struct intel_crtc *crtc)
 		goto out_disable;
 	}
 
-	if (intel_fbc_setup_cfb(crtc)) {
-		set_no_fbc_reason(dev_priv, "not enough stolen memory");
+	/* It is possible for the required CFB size change without a
+	 * crtc->disable + crtc->enable since it is possible to change the
+	 * stride without triggering a full modeset. Since we try to
+	 * over-allocate the CFB, there's a chance we may keep FBC enabled even
+	 * if this happens, but if we exceed the current CFB size we'll have to
+	 * disable FBC. Notice that it would be possible to disable FBC, wait
+	 * for a frame, free the stolen node, then try to reenable FBC in case
+	 * we didn't get any invalidate/deactivate calls, but this would require
+	 * a lot of tracking just for a case we expect to be uncommon, so we
+	 * just don't have this code for now. */
+	if (intel_fbc_calculate_cfb_size(crtc, fb) >
+	    dev_priv->fbc.compressed_fb.size * dev_priv->fbc.threshold) {
+		set_no_fbc_reason(dev_priv, "CFB requirements changed");
 		goto out_disable;
 	}
 
@@ -907,7 +905,6 @@ out_disable:
 		DRM_DEBUG_KMS("unsupported config, deactivating FBC\n");
 		__intel_fbc_deactivate(dev_priv);
 	}
-	__intel_fbc_cleanup_cfb(dev_priv);
 }
 
 /*
@@ -1020,6 +1017,11 @@ void intel_fbc_enable(struct intel_crtc *crtc)
 		goto out;
 	}
 
+	if (intel_fbc_alloc_cfb(crtc)) {
+		set_no_fbc_reason(dev_priv, "not enough stolen memory");
+		goto out;
+	}
+
 	DRM_DEBUG_KMS("Enabling FBC on pipe %c\n", pipe_name(crtc->pipe));
 	dev_priv->fbc.no_fbc_reason = "FBC enabled but not active yet\n";
 
@@ -1047,6 +1049,8 @@ static void __intel_fbc_disable(struct drm_i915_private *dev_priv)
 
 	DRM_DEBUG_KMS("Disabling FBC on pipe %c\n", pipe_name(crtc->pipe));
 
+	__intel_fbc_cleanup_cfb(dev_priv);
+
 	dev_priv->fbc.enabled = false;
 	dev_priv->fbc.crtc = NULL;
 }
-- 
2.6.2

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

[PATCH 07/12] drm/i915: check for FBC planes in the same place as the pipes

[Paulo Zanoni]

This moves the pre-gen4 check from update() to enable(). The HAS_DDI
in the original code is not needed since only gen 2/3 have the plane
swapping code.

v2: Rebase.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index d496a7a..a0784d0 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -514,6 +514,9 @@ static bool crtc_can_fbc(struct intel_crtc *crtc)
 	if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A)
 		return false;
 
+	if (INTEL_INFO(dev_priv)->gen < 4 && crtc->plane != PLANE_A)
+		return false;
+
 	return true;
 }
 
@@ -802,12 +805,6 @@ static void __intel_fbc_update(struct intel_crtc *crtc)
 		goto out_disable;
 	}
 
-	if ((INTEL_INFO(dev_priv)->gen < 4 || HAS_DDI(dev_priv)) &&
-	    crtc->plane != PLANE_A) {
-		set_no_fbc_reason(dev_priv, "FBC unsupported on plane");
-		goto out_disable;
-	}
-
 	/* The use of a CPU fence is mandatory in order to detect writes
 	 * by the CPU to the scanout and trigger updates to the FBC.
 	 */
-- 
2.6.2

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

[PATCH 08/12] drm/i915: use a single intel_fbc_work struct

[Paulo Zanoni]

This was already on my TODO list, and was requested both by Chris and
Ville, for different reasons. The advantages are avoiding a frequent
malloc/free pair, and the locality of having the work structure
embedded in dev_priv. The maximum used memory is also smaller since
previously we could have multiple allocated intel_fbc_work structs at
the same time, and now we'll always have a single one - the one
embedded on dev_priv. Of course, we're now using a little more memory
on the cases where there's nothing scheduled.

The biggest challenge here is to keep everything synchronized the way
it was before.

Currently, when we try to activate FBC, we allocate a new
intel_fbc_work structure. Then later when we conclude we must delay
the FBC activation a little more, we allocate a new intel_fbc_work
struct, and then adjust dev_priv->fbc.fbc_work to point to the new
struct. So when the old work runs - at intel_fbc_work_fn() - it will
check that dev_priv->fbc.fbc_work points to something else, so it does
nothing. Everything is also protected by fbc.lock.

Just cancelling the old delayed work doesn't work because we might
just cancel it after the work function already started to run, but
while it is still waiting to grab fbc.lock. That's why we use the
"dev_priv->fbc.fbc_work == work" check described in the paragraph
above.

So now that we have a single work struct we have to introduce a new
way to synchronize everything. So we're making the work function a
normal work instead of a delayed work, and it will be responsible for
sleeping the appropriate amount of time itself. This way, after it
wakes up it can grab the lock, ask "were we delayed or cancelled?" and
then go back to sleep, enable FBC or give up.

v2:
  - Spelling fixes.
  - Rebase after changing the patch order.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v1)
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |   6 ++-
 drivers/gpu/drm/i915/intel_fbc.c | 106 +++++++++++++++++----------------------
 2 files changed, 51 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 92bb8e1..9418bd5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -918,9 +918,11 @@ struct i915_fbc {
 	bool active;
 
 	struct intel_fbc_work {
-		struct delayed_work work;
+		bool scheduled;
+		struct work_struct work;
 		struct drm_framebuffer *fb;
-	} *fbc_work;
+		unsigned long enable_jiffies;
+	} work;
 
 	const char *no_fbc_reason;
 
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index a0784d0..aa82075 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -387,85 +387,71 @@ static void intel_fbc_activate(const struct drm_framebuffer *fb)
 
 static void intel_fbc_work_fn(struct work_struct *__work)
 {
-	struct intel_fbc_work *work =
-		container_of(to_delayed_work(__work),
-			     struct intel_fbc_work, work);
-	struct drm_i915_private *dev_priv = work->fb->dev->dev_private;
-	struct drm_framebuffer *crtc_fb = dev_priv->fbc.crtc->base.primary->fb;
+	struct drm_i915_private *dev_priv =
+		container_of(__work, struct drm_i915_private, fbc.work.work);
+	struct intel_fbc_work *work = &dev_priv->fbc.work;
+	struct intel_crtc *crtc = dev_priv->fbc.crtc;
+	unsigned long delay_jiffies = msecs_to_jiffies(50);
+
+retry:
+	/* Delay the actual enabling to let pageflipping cease and the
+	 * display to settle before starting the compression. Note that
+	 * this delay also serves a second purpose: it allows for a
+	 * vblank to pass after disabling the FBC before we attempt
+	 * to modify the control registers.
+	 *
+	 * A more complicated solution would involve tracking vblanks
+	 * following the termination of the page-flipping sequence
+	 * and indeed performing the enable as a co-routine and not
+	 * waiting synchronously upon the vblank.
+	 *
+	 * WaFbcWaitForVBlankBeforeEnable:ilk,snb
+	 */
+	wait_remaining_ms_from_jiffies(work->enable_jiffies, delay_jiffies);
 
 	mutex_lock(&dev_priv->fbc.lock);
-	if (work == dev_priv->fbc.fbc_work) {
-		/* Double check that we haven't switched fb without cancelling
-		 * the prior work.
-		 */
-		if (crtc_fb == work->fb)
-			intel_fbc_activate(work->fb);
 
-		dev_priv->fbc.fbc_work = NULL;
+	/* Were we cancelled? */
+	if (!work->scheduled)
+		goto out;
+
+	/* Were we delayed again while this function was sleeping? */
+	if (time_after(work->enable_jiffies + delay_jiffies, jiffies)) {
+		mutex_unlock(&dev_priv->fbc.lock);
+		goto retry;
 	}
-	mutex_unlock(&dev_priv->fbc.lock);
 
-	kfree(work);
+	if (crtc->base.primary->fb == work->fb)
+		intel_fbc_activate(work->fb);
+
+	work->scheduled = false;
+
+out:
+	mutex_unlock(&dev_priv->fbc.lock);
 }
 
 static void intel_fbc_cancel_work(struct drm_i915_private *dev_priv)
 {
 	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
-
-	if (dev_priv->fbc.fbc_work == NULL)
-		return;
-
-	/* Synchronisation is provided by struct_mutex and checking of
-	 * dev_priv->fbc.fbc_work, so we can perform the cancellation
-	 * entirely asynchronously.
-	 */
-	if (cancel_delayed_work(&dev_priv->fbc.fbc_work->work))
-		/* tasklet was killed before being run, clean up */
-		kfree(dev_priv->fbc.fbc_work);
-
-	/* Mark the work as no longer wanted so that if it does
-	 * wake-up (because the work was already running and waiting
-	 * for our mutex), it will discover that is no longer
-	 * necessary to run.
-	 */
-	dev_priv->fbc.fbc_work = NULL;
+	dev_priv->fbc.work.scheduled = false;
 }
 
 static void intel_fbc_schedule_activation(struct intel_crtc *crtc)
 {
-	struct intel_fbc_work *work;
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct intel_fbc_work *work = &dev_priv->fbc.work;
 
 	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
 
-	intel_fbc_cancel_work(dev_priv);
-
-	work = kzalloc(sizeof(*work), GFP_KERNEL);
-	if (work == NULL) {
-		DRM_ERROR("Failed to allocate FBC work structure\n");
-		intel_fbc_activate(crtc->base.primary->fb);
-		return;
-	}
-
+	/* It is useless to call intel_fbc_cancel_work() in this function since
+	 * we're not releasing fbc.lock, so it won't have an opportunity to grab
+	 * it to discover that it was cancelled. So we just update the expected
+	 * jiffy count. */
 	work->fb = crtc->base.primary->fb;
-	INIT_DELAYED_WORK(&work->work, intel_fbc_work_fn);
+	work->scheduled = true;
+	work->enable_jiffies = jiffies;
 
-	dev_priv->fbc.fbc_work = work;
-
-	/* Delay the actual enabling to let pageflipping cease and the
-	 * display to settle before starting the compression. Note that
-	 * this delay also serves a second purpose: it allows for a
-	 * vblank to pass after disabling the FBC before we attempt
-	 * to modify the control registers.
-	 *
-	 * A more complicated solution would involve tracking vblanks
-	 * following the termination of the page-flipping sequence
-	 * and indeed performing the enable as a co-routine and not
-	 * waiting synchronously upon the vblank.
-	 *
-	 * WaFbcWaitForVBlankBeforeEnable:ilk,snb
-	 */
-	schedule_delayed_work(&work->work, msecs_to_jiffies(50));
+	schedule_work(&work->work);
 }
 
 static void __intel_fbc_deactivate(struct drm_i915_private *dev_priv)
@@ -1101,9 +1087,11 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
 {
 	enum pipe pipe;
 
+	INIT_WORK(&dev_priv->fbc.work.work, intel_fbc_work_fn);
 	mutex_init(&dev_priv->fbc.lock);
 	dev_priv->fbc.enabled = false;
 	dev_priv->fbc.active = false;
+	dev_priv->fbc.work.scheduled = false;
 
 	if (!HAS_FBC(dev_priv)) {
 		dev_priv->fbc.no_fbc_reason = "unsupported by this chipset";
-- 
2.6.2

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

[PATCH 09/12] drm/i915: wait for a vblank instead of 50ms when enabling FBC

[Paulo Zanoni]

Instead of waiting for 50ms, just wait until the next vblank, since
it's the minimum requirement.

This moves PC7 residency on my specific BDW machine running Cinnamon
from 60-70% to 84-89%. Without FBC, I get 20-25%. I'm using a
3200x1800 eDP panel. Notice: this was the case when the patch was
originally proposed, the order of the FBC patches changed since then,
so the actual numbers might be slightly different now.

v2:
  - Rebase after changing the patch order.
  - Update the commit message.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  2 +-
 drivers/gpu/drm/i915/intel_fbc.c | 12 +++---------
 2 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9418bd5..ea08714 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -919,9 +919,9 @@ struct i915_fbc {
 
 	struct intel_fbc_work {
 		bool scheduled;
+		u32 scheduled_vblank;
 		struct work_struct work;
 		struct drm_framebuffer *fb;
-		unsigned long enable_jiffies;
 	} work;
 
 	const char *no_fbc_reason;
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index aa82075..72de8a1 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -391,7 +391,6 @@ static void intel_fbc_work_fn(struct work_struct *__work)
 		container_of(__work, struct drm_i915_private, fbc.work.work);
 	struct intel_fbc_work *work = &dev_priv->fbc.work;
 	struct intel_crtc *crtc = dev_priv->fbc.crtc;
-	unsigned long delay_jiffies = msecs_to_jiffies(50);
 
 retry:
 	/* Delay the actual enabling to let pageflipping cease and the
@@ -400,14 +399,9 @@ retry:
 	 * vblank to pass after disabling the FBC before we attempt
 	 * to modify the control registers.
 	 *
-	 * A more complicated solution would involve tracking vblanks
-	 * following the termination of the page-flipping sequence
-	 * and indeed performing the enable as a co-routine and not
-	 * waiting synchronously upon the vblank.
-	 *
 	 * WaFbcWaitForVBlankBeforeEnable:ilk,snb
 	 */
-	wait_remaining_ms_from_jiffies(work->enable_jiffies, delay_jiffies);
+	intel_wait_for_vblank(dev_priv->dev, crtc->pipe);
 
 	mutex_lock(&dev_priv->fbc.lock);
 
@@ -416,7 +410,7 @@ retry:
 		goto out;
 
 	/* Were we delayed again while this function was sleeping? */
-	if (time_after(work->enable_jiffies + delay_jiffies, jiffies)) {
+	if (drm_crtc_vblank_get(&crtc->base) == work->scheduled_vblank) {
 		mutex_unlock(&dev_priv->fbc.lock);
 		goto retry;
 	}
@@ -449,7 +443,7 @@ static void intel_fbc_schedule_activation(struct intel_crtc *crtc)
 	 * jiffy count. */
 	work->fb = crtc->base.primary->fb;
 	work->scheduled = true;
-	work->enable_jiffies = jiffies;
+	work->scheduled_vblank = drm_crtc_vblank_count(&crtc->base);
 
 	schedule_work(&work->work);
 }
-- 
2.6.2

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

[PATCH 10/12] drm/i915: kill fbc.uncompressed_size

[Paulo Zanoni]

Directly call intel_fbc_calculate_cfb_size() in the only place that
actually needs it, and use the proper check before removing the stolen
node. IMHO, this change makes our code easier to understand.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  1 -
 drivers/gpu/drm/i915/intel_fbc.c | 13 ++++---------
 2 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ea08714..43649c5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -901,7 +901,6 @@ struct i915_fbc {
 	/* This is always the inner lock when overlapping with struct_mutex and
 	 * it's the outer lock when overlapping with stolen_lock. */
 	struct mutex lock;
-	unsigned long uncompressed_size;
 	unsigned threshold;
 	unsigned int fb_id;
 	unsigned int possible_framebuffer_bits;
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 72de8a1..d18eb80 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -139,7 +139,7 @@ static void i8xx_fbc_activate(struct intel_crtc *crtc)
 	dev_priv->fbc.active = true;
 
 	/* Note: fbc.threshold == 1 for i8xx */
-	cfb_pitch = dev_priv->fbc.uncompressed_size / FBC_LL_SIZE;
+	cfb_pitch = intel_fbc_calculate_cfb_size(crtc, fb) / FBC_LL_SIZE;
 	if (fb->pitches[0] < cfb_pitch)
 		cfb_pitch = fb->pitches[0];
 
@@ -626,8 +626,6 @@ static int intel_fbc_alloc_cfb(struct intel_crtc *crtc)
 			   dev_priv->mm.stolen_base + compressed_llb->start);
 	}
 
-	dev_priv->fbc.uncompressed_size = size;
-
 	DRM_DEBUG_KMS("reserved %llu bytes of contiguous stolen space for FBC, threshold: %d\n",
 		      dev_priv->fbc.compressed_fb.size,
 		      dev_priv->fbc.threshold);
@@ -644,18 +642,15 @@ err_llb:
 
 static void __intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv)
 {
-	if (dev_priv->fbc.uncompressed_size == 0)
-		return;
-
-	i915_gem_stolen_remove_node(dev_priv, &dev_priv->fbc.compressed_fb);
+	if (dev_priv->fbc.compressed_fb.allocated)
+		i915_gem_stolen_remove_node(dev_priv,
+					    &dev_priv->fbc.compressed_fb);
 
 	if (dev_priv->fbc.compressed_llb) {
 		i915_gem_stolen_remove_node(dev_priv,
 					    dev_priv->fbc.compressed_llb);
 		kfree(dev_priv->fbc.compressed_llb);
 	}
-
-	dev_priv->fbc.uncompressed_size = 0;
 }
 
 void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv)
-- 
2.6.2

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

[PATCH 11/12] drm/i915: get rid of FBC {, de}activation messages

[Paulo Zanoni]

When running Cinnamon I see way too many pairs of these messages: many
per second. Get rid of them as they're just telling us FBC is working
as expected. We already have the messages for enable/disable, so we
don't really need messages for activation/deactivation.

v2: Rebase after changing the patch order.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index d18eb80..b80f232 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -123,8 +123,6 @@ static void i8xx_fbc_deactivate(struct drm_i915_private *dev_priv)
 		DRM_DEBUG_KMS("FBC idle timed out\n");
 		return;
 	}
-
-	DRM_DEBUG_KMS("deactivated FBC\n");
 }
 
 static void i8xx_fbc_activate(struct intel_crtc *crtc)
@@ -172,9 +170,6 @@ static void i8xx_fbc_activate(struct intel_crtc *crtc)
 	fbc_ctl |= (cfb_pitch & 0xff) << FBC_CTL_STRIDE_SHIFT;
 	fbc_ctl |= obj->fence_reg;
 	I915_WRITE(FBC_CONTROL, fbc_ctl);
-
-	DRM_DEBUG_KMS("activated FBC, pitch %d, yoff %d, plane %c\n",
-		      cfb_pitch, crtc->base.y, plane_name(crtc->plane));
 }
 
 static bool i8xx_fbc_is_active(struct drm_i915_private *dev_priv)
@@ -202,8 +197,6 @@ static void g4x_fbc_activate(struct intel_crtc *crtc)
 
 	/* enable it... */
 	I915_WRITE(DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
-
-	DRM_DEBUG_KMS("activated fbc on plane %c\n", plane_name(crtc->plane));
 }
 
 static void g4x_fbc_deactivate(struct drm_i915_private *dev_priv)
@@ -217,8 +210,6 @@ static void g4x_fbc_deactivate(struct drm_i915_private *dev_priv)
 	if (dpfc_ctl & DPFC_CTL_EN) {
 		dpfc_ctl &= ~DPFC_CTL_EN;
 		I915_WRITE(DPFC_CONTROL, dpfc_ctl);
-
-		DRM_DEBUG_KMS("deactivated FBC\n");
 	}
 }
 
@@ -278,8 +269,6 @@ static void ilk_fbc_activate(struct intel_crtc *crtc)
 	}
 
 	intel_fbc_recompress(dev_priv);
-
-	DRM_DEBUG_KMS("activated fbc on plane %c\n", plane_name(crtc->plane));
 }
 
 static void ilk_fbc_deactivate(struct drm_i915_private *dev_priv)
@@ -293,8 +282,6 @@ static void ilk_fbc_deactivate(struct drm_i915_private *dev_priv)
 	if (dpfc_ctl & DPFC_CTL_EN) {
 		dpfc_ctl &= ~DPFC_CTL_EN;
 		I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl);
-
-		DRM_DEBUG_KMS("deactivated FBC\n");
 	}
 }
 
@@ -357,8 +344,6 @@ static void gen7_fbc_activate(struct intel_crtc *crtc)
 	I915_WRITE(DPFC_CPU_FENCE_OFFSET, get_crtc_fence_y_offset(crtc));
 
 	intel_fbc_recompress(dev_priv);
-
-	DRM_DEBUG_KMS("activated fbc on plane %c\n", plane_name(crtc->plane));
 }
 
 /**
-- 
2.6.2

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

[PATCH 12/12] drm/i915: only nuke FBC when a drawing operation triggers a flush

[Paulo Zanoni]

There's no need to stop and restart FBC: a nuke should be fine.

v2: Make it simpler (Chris).
v3: Rewrite the patch again due to patch order changes.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index b80f232..207cf23 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -923,8 +923,12 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
 	dev_priv->fbc.busy_bits &= ~frontbuffer_bits;
 
 	if (!dev_priv->fbc.busy_bits && dev_priv->fbc.enabled) {
-		__intel_fbc_deactivate(dev_priv);
-		__intel_fbc_update(dev_priv->fbc.crtc);
+		if (origin != ORIGIN_FLIP && dev_priv->fbc.active) {
+			intel_fbc_recompress(dev_priv);
+		} else {
+			__intel_fbc_deactivate(dev_priv);
+			__intel_fbc_update(dev_priv->fbc.crtc);
+		}
 	}
 
 	mutex_unlock(&dev_priv->fbc.lock);
-- 
2.6.2

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

Re: [PATCH 02/12] drm/i915: set dev_priv->fbc.crtc before scheduling the enable work

[Chris Wilson]

On Fri, Nov 13, 2015 at 05:53:34PM -0200, Paulo Zanoni wrote:
> This thing where we need to get the crtc either from the work
> structure or the fbc structure itself is confusing and unnecessary.
> Set fbc.crtc right when scheduling the enable work so we can always
> use it.
> 
> The problem is not what gets passed and how to retrieve it. The
> problem is that when we're in the other parts of the code we always
> have to keep in mind that if FBC is already enabled we have to get the
> CRTC from place A, if FBC is scheduled we have to get the CRTC from
> place B, and if it's disabled there's no CRTC. Having a single place
> to retrieve the CRTC from allows us to treat the "is enabled" and "is
> scheduled" cases as the same case, reducing the mistake surface. I
> guess I should add this to the commit message.
> 
> Besides the immediate advantages, this is also going to make one of
> the next commits much simpler. And even later, when we introduce
> enable/disable + activate/deactivate, this will be even simpler as
> we'll set the CRTC at enable time. So all the
> activate/deactivate/update code can just look at the single CRTC
> variable regardless of the current state.
> 
> v2: Improve commit message (Chris).
> v3: Rebase after changing the patch order.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  1 -
>  drivers/gpu/drm/i915/intel_fbc.c | 22 +++++++++-------------
>  2 files changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 71bd1dc..317414b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -920,7 +920,6 @@ struct i915_fbc {
>  
>  	struct intel_fbc_work {
>  		struct delayed_work work;
> -		struct intel_crtc *crtc;
>  		struct drm_framebuffer *fb;
>  	} *fbc_work;
>  
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 611672f..bc9cd33 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -334,14 +334,13 @@ bool intel_fbc_enabled(struct drm_i915_private *dev_priv)
>  	return dev_priv->fbc.enabled;
>  }
>  
> -static void intel_fbc_enable(struct intel_crtc *crtc,
> -			     const struct drm_framebuffer *fb)
> +static void intel_fbc_enable(const struct drm_framebuffer *fb)
>  {
> -	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> +	struct drm_i915_private *dev_priv = fb->dev->dev_private;
> +	struct intel_crtc *crtc = dev_priv->fbc.crtc;

This is confusing me. I think of FBC in terms of the CRTC/pipe, and the
fb just describes the plane currently bound to the primary. You've
pushed everywhere else to also work on the CRTC, I think it would be
consistent here to pass crtc and then use fbc.fb_id =
crtc->primary->fb->id.

Have I missed something in the later patches that explains the choice
here?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/12] drm/i915: wait for a vblank instead of 50ms when enabling FBC

[Chris Wilson]

On Fri, Nov 13, 2015 at 05:53:41PM -0200, Paulo Zanoni wrote:
> Instead of waiting for 50ms, just wait until the next vblank, since
> it's the minimum requirement.
> 
> This moves PC7 residency on my specific BDW machine running Cinnamon
> from 60-70% to 84-89%. Without FBC, I get 20-25%. I'm using a
> 3200x1800 eDP panel. Notice: this was the case when the patch was
> originally proposed, the order of the FBC patches changed since then,
> so the actual numbers might be slightly different now.
> 
> v2:
>   - Rebase after changing the patch order.
>   - Update the commit message.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  2 +-
>  drivers/gpu/drm/i915/intel_fbc.c | 12 +++---------
>  2 files changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 9418bd5..ea08714 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -919,9 +919,9 @@ struct i915_fbc {
>  
>  	struct intel_fbc_work {
>  		bool scheduled;
> +		u32 scheduled_vblank;
>  		struct work_struct work;
>  		struct drm_framebuffer *fb;
> -		unsigned long enable_jiffies;
>  	} work;
>  
>  	const char *no_fbc_reason;
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index aa82075..72de8a1 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -391,7 +391,6 @@ static void intel_fbc_work_fn(struct work_struct *__work)
>  		container_of(__work, struct drm_i915_private, fbc.work.work);
>  	struct intel_fbc_work *work = &dev_priv->fbc.work;
>  	struct intel_crtc *crtc = dev_priv->fbc.crtc;
> -	unsigned long delay_jiffies = msecs_to_jiffies(50);
>  
>  retry:
>  	/* Delay the actual enabling to let pageflipping cease and the
> @@ -400,14 +399,9 @@ retry:
>  	 * vblank to pass after disabling the FBC before we attempt
>  	 * to modify the control registers.
>  	 *
> -	 * A more complicated solution would involve tracking vblanks
> -	 * following the termination of the page-flipping sequence
> -	 * and indeed performing the enable as a co-routine and not
> -	 * waiting synchronously upon the vblank.
> -	 *
>  	 * WaFbcWaitForVBlankBeforeEnable:ilk,snb
>  	 */
> -	wait_remaining_ms_from_jiffies(work->enable_jiffies, delay_jiffies);
> +	intel_wait_for_vblank(dev_priv->dev, crtc->pipe);
>  
>  	mutex_lock(&dev_priv->fbc.lock);
>  
> @@ -416,7 +410,7 @@ retry:
>  		goto out;
>  
>  	/* Were we delayed again while this function was sleeping? */
> -	if (time_after(work->enable_jiffies + delay_jiffies, jiffies)) {
> +	if (drm_crtc_vblank_get(&crtc->base) == work->scheduled_vblank) {
>  		mutex_unlock(&dev_priv->fbc.lock);
>  		goto retry;
>  	}
> @@ -449,7 +443,7 @@ static void intel_fbc_schedule_activation(struct intel_crtc *crtc)
>  	 * jiffy count. */
>  	work->fb = crtc->base.primary->fb;
>  	work->scheduled = true;
> -	work->enable_jiffies = jiffies;
> +	work->scheduled_vblank = drm_crtc_vblank_count(&crtc->base);

Isn't the frame counter only incrementing whilst the vblank IRQ is
enabled? Ville?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/12] drm/i915: get rid of FBC {, de}activation messages

[Chris Wilson]

On Fri, Nov 13, 2015 at 05:53:43PM -0200, Paulo Zanoni wrote:
> When running Cinnamon I see way too many pairs of these messages: many
> per second. Get rid of them as they're just telling us FBC is working
> as expected. We already have the messages for enable/disable, so we
> don't really need messages for activation/deactivation.
> 
> v2: Rebase after changing the patch order.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Fair enough, at this granularity logging isn't the best method of
evaluating behaviour and the outer enable/disable should give enough
hints for debugging.
Reviwed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/12] drm/i915: introduce intel_fbc_{enable, disable}

[Chris Wilson]

On Fri, Nov 13, 2015 at 05:53:37PM -0200, Paulo Zanoni wrote:
> The goal is to call FBC enable/disable only once per modeset, while
> activate/deactivate/update will be called multiple times.
> 
> The enable() function will be responsible for deciding if a CRTC will
> have FBC on it and then it will "lock" FBC on this CRTC: it won't be
> possible to change FBC's CRTC until disable(). With this, all checks
> and resource acquisition that only need to be done once per modeset
> can be moved from update() to enable(). And then the update(),
> activate() and deactivate() code will also get simpler since they
> won't need to worry about the CRTC being changed.
> 
> The disable() function will do the reverse operation of enable(). One
> of its features is that it should only be called while the pipe is
> already off. This guarantees that FBC is stopped and nothing is
> using the CFB.
> 
> With this, the activate() and deactivate() functions just start and
> temporarily stop FBC. They are the ones touching the hardware enable
> bit, so HW state reflects dev_priv->crtc.active.
> 
> The last function remaining is update(). A lot of times I thought
> about renaming update() to activate() or try_to_activate() since it's
> called when we want to activate FBC. The thing is that update() may
> not only decide to activate FBC, but also deactivate or keep it on the
> same state, so I'll leave this name for now.
> 
> Moving code to enable() and disable() will also help in case we decide
> to move FBC to pipe_config or something else later.
> 
> The current patch only puts the very basic code on enable() and
> disable(). The next commits will take care of moving more stuff from
> update() to the new functions.
> 
> v2:
>   - Rebase.
>   - Improve commit message (Chris).
> v3: Rebase after changing the patch order.
> v4: Rebase again after upstream changes.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/12] drm/i915: pass the crtc as an argument to intel_fbc_update()

[Chris Wilson]

On Fri, Nov 13, 2015 at 05:53:35PM -0200, Paulo Zanoni wrote:
> There's no need to reevaluate the status of every single crtc when a
> single crtc changes its state.
> 
> With this, we're cutting the case where due to a change in pipe B,
> intel_fbc_update() is called, then intel_fbc_find_crtc() concludes FBC
> should be enabled on pipe A, then it completely rechecks the state of
> pipe A only to conclude FBC should remain enabled on pipe A. If any
> change on pipe A triggers a need to recompute whether FBC is valid on
> pipe A, then at some point someone is going to call
> intel_fbc_update(PIPE_A).
> 
> The addition of intel_fbc_deactivate() is necessary so we keep track
> of the previously selected CRTC when we do invalidate/flush. We're
> also going to continue the enable/disable/activate/deactivate concept
> in the next patches.
> 
> v2: Rebase.
> v3: Rebase after changing the patch order.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/12] drm/i915: set dev_priv->fbc.crtc before scheduling the enable work

[Paulo Zanoni]

2015-11-13 18:56 GMT-02:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Fri, Nov 13, 2015 at 05:53:34PM -0200, Paulo Zanoni wrote:
>> This thing where we need to get the crtc either from the work
>> structure or the fbc structure itself is confusing and unnecessary.
>> Set fbc.crtc right when scheduling the enable work so we can always
>> use it.
>>
>> The problem is not what gets passed and how to retrieve it. The
>> problem is that when we're in the other parts of the code we always
>> have to keep in mind that if FBC is already enabled we have to get the
>> CRTC from place A, if FBC is scheduled we have to get the CRTC from
>> place B, and if it's disabled there's no CRTC. Having a single place
>> to retrieve the CRTC from allows us to treat the "is enabled" and "is
>> scheduled" cases as the same case, reducing the mistake surface. I
>> guess I should add this to the commit message.
>>
>> Besides the immediate advantages, this is also going to make one of
>> the next commits much simpler. And even later, when we introduce
>> enable/disable + activate/deactivate, this will be even simpler as
>> we'll set the CRTC at enable time. So all the
>> activate/deactivate/update code can just look at the single CRTC
>> variable regardless of the current state.
>>
>> v2: Improve commit message (Chris).
>> v3: Rebase after changing the patch order.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h  |  1 -
>>  drivers/gpu/drm/i915/intel_fbc.c | 22 +++++++++-------------
>>  2 files changed, 9 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 71bd1dc..317414b 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -920,7 +920,6 @@ struct i915_fbc {
>>
>>       struct intel_fbc_work {
>>               struct delayed_work work;
>> -             struct intel_crtc *crtc;
>>               struct drm_framebuffer *fb;
>>       } *fbc_work;
>>
>> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
>> index 611672f..bc9cd33 100644
>> --- a/drivers/gpu/drm/i915/intel_fbc.c
>> +++ b/drivers/gpu/drm/i915/intel_fbc.c
>> @@ -334,14 +334,13 @@ bool intel_fbc_enabled(struct drm_i915_private *dev_priv)
>>       return dev_priv->fbc.enabled;
>>  }
>>
>> -static void intel_fbc_enable(struct intel_crtc *crtc,
>> -                          const struct drm_framebuffer *fb)
>> +static void intel_fbc_enable(const struct drm_framebuffer *fb)
>>  {
>> -     struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>> +     struct drm_i915_private *dev_priv = fb->dev->dev_private;
>> +     struct intel_crtc *crtc = dev_priv->fbc.crtc;
>
> This is confusing me. I think of FBC in terms of the CRTC/pipe, and the
> fb just describes the plane currently bound to the primary. You've
> pushed everywhere else to also work on the CRTC, I think it would be
> consistent here to pass crtc and then use fbc.fb_id =
> crtc->primary->fb->id.
>
> Have I missed something in the later patches that explains the choice
> here?

You are right that this is confusing. This is basically an artifact
from the previous FBC design that I overlooked. I guess during the
conversion, my thinking was "well, the arguments are the stuff we
store in the work struct, and since we don't store the CRTC anymore,
let's remove it and keep passing the FB since it's still part of the
work struct". Now that you bring this under a different perspective, I
see the problem. We don't even need to pass these things as arguments
since we do check that work->fb is equal to crtc->fb.

I just hope you let me address this with follow-up patches. Function
intel_fbc_activate() was created because there were 2 callers: one for
the case there the work_fn happens, another for the case where we
failed to allocate the work_fn. Later in the series there's only one
caller because there's no allocation anymore, so the best way to fix
is probably to just move those 3 lines to the place of the only
caller.

You also requested me to grab references of fbc.fb_id, and I have some
evil plans to completely kill it, which would even reduce those 3
lines further.

Thanks,
Paulo

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 09/12] drm/i915: wait for a vblank instead of 50ms when enabling FBC

[Zanoni, Paulo R]

Em Sex, 2015-11-13 às 21:03 +0000, Chris Wilson escreveu:
> On Fri, Nov 13, 2015 at 05:53:41PM -0200, Paulo Zanoni wrote:
> > Instead of waiting for 50ms, just wait until the next vblank, since
> > it's the minimum requirement.
> > 
> > This moves PC7 residency on my specific BDW machine running
> > Cinnamon
> > from 60-70% to 84-89%. Without FBC, I get 20-25%. I'm using a
> > 3200x1800 eDP panel. Notice: this was the case when the patch was
> > originally proposed, the order of the FBC patches changed since
> > then,
> > so the actual numbers might be slightly different now.
> > 
> > v2:
> >   - Rebase after changing the patch order.
> >   - Update the commit message.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  |  2 +-
> >  drivers/gpu/drm/i915/intel_fbc.c | 12 +++---------
> >  2 files changed, 4 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 9418bd5..ea08714 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -919,9 +919,9 @@ struct i915_fbc {
> >  
> >  	struct intel_fbc_work {
> >  		bool scheduled;
> > +		u32 scheduled_vblank;
> >  		struct work_struct work;
> >  		struct drm_framebuffer *fb;
> > -		unsigned long enable_jiffies;
> >  	} work;
> >  
> >  	const char *no_fbc_reason;
> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > b/drivers/gpu/drm/i915/intel_fbc.c
> > index aa82075..72de8a1 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -391,7 +391,6 @@ static void intel_fbc_work_fn(struct
> > work_struct *__work)
> >  		container_of(__work, struct drm_i915_private,
> > fbc.work.work);
> >  	struct intel_fbc_work *work = &dev_priv->fbc.work;
> >  	struct intel_crtc *crtc = dev_priv->fbc.crtc;
> > -	unsigned long delay_jiffies = msecs_to_jiffies(50);
> >  
> >  retry:
> >  	/* Delay the actual enabling to let pageflipping cease and
> > the
> > @@ -400,14 +399,9 @@ retry:
> >  	 * vblank to pass after disabling the FBC before we
> > attempt
> >  	 * to modify the control registers.
> >  	 *
> > -	 * A more complicated solution would involve tracking
> > vblanks
> > -	 * following the termination of the page-flipping sequence
> > -	 * and indeed performing the enable as a co-routine and
> > not
> > -	 * waiting synchronously upon the vblank.
> > -	 *
> >  	 * WaFbcWaitForVBlankBeforeEnable:ilk,snb
> >  	 */
> > -	wait_remaining_ms_from_jiffies(work->enable_jiffies,
> > delay_jiffies);
> > +	intel_wait_for_vblank(dev_priv->dev, crtc->pipe);
> >  
> >  	mutex_lock(&dev_priv->fbc.lock);
> >  
> > @@ -416,7 +410,7 @@ retry:
> >  		goto out;
> >  
> >  	/* Were we delayed again while this function was sleeping?
> > */
> > -	if (time_after(work->enable_jiffies + delay_jiffies,
> > jiffies)) {
> > +	if (drm_crtc_vblank_get(&crtc->base) == work-
> > >scheduled_vblank) {
> >  		mutex_unlock(&dev_priv->fbc.lock);
> >  		goto retry;
> >  	}
> > @@ -449,7 +443,7 @@ static void
> > intel_fbc_schedule_activation(struct intel_crtc *crtc)
> >  	 * jiffy count. */
> >  	work->fb = crtc->base.primary->fb;
> >  	work->scheduled = true;
> > -	work->enable_jiffies = jiffies;
> > +	work->scheduled_vblank = drm_crtc_vblank_count(&crtc-
> > >base);
> 
> Isn't the frame counter only incrementing whilst the vblank IRQ is
> enabled? Ville?

At the work function we call intel_wait_for_vblank(), which calls
drm_wait_one_vblank(), which calls drm_vblank_get().

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

Re: [PATCH 06/12] drm/i915: alloc/free the FBC CFB during enable/disable

[Chris Wilson]

On Fri, Nov 13, 2015 at 05:53:38PM -0200, Paulo Zanoni wrote:
> One of the problems with the current code is that it frees the CFB and
> releases its drm_mm node as soon as we flip FBC's enable bit. This is
> bad because after we disable FBC the hardware may still use the CFB
> for the rest of the frame, so in theory we should only release the
> drm_mm node one frame after we disable FBC. Otherwise, a stolen memory
> allocation done right after an FBC disable may result in either
> corrupted memory for the new owner of that memory region or corrupted
> screen/underruns in case the new owner changes it while the hardware
> is still reading it. This case is not exactly easy to reproduce since
> we currently don't do a lot of stolen memory allocations, but I see
> patches on the mailing list trying to expose stolen memory to user
> space, so races will be possible.
> 
> I thought about three different approaches to solve this, and they all
> have downsides.
> 
> The first approach would be to simply use multiple drm_mm nodes and
> freeing the unused ones only after a frame has passed. The problem
> with this approach is that since stolen memory is rather small,
> there's a risk we just won't be able to allocate a new CFB from stolen
> if the previous one was not freed yet. This could happen in case we
> quickly disable FBC from pipe A and decide to enable it on pipe B, or
> just if we change pipe A's fb stride while FBC is enabled.
> 
> The second approach would be similar to the first one, but maintaining
> a single drm_mm node and keeping track of when it can be reused. This
> would remove the disadvantage of not having enough space for two
> nodes, but would create the new problem where we may not be able to
> enable FBC at the point intel_fbc_update() is called, so we would have
> to add more code to retry updating FBC after the time has passed. And
> that can quickly get too complex since we can get invalidate, flush,
> disable and other calls in the middle of the wait.
> 
> Both solutions above - and also the current code - have the problem
> that we unnecessarily free+realloc FBC during invalidate+flush
> operations even if the CFB size doesn't change.
> 
> The third option would be to move the allocation/deallocation to
> enable/disable. This makes sure that the pipe is always disabled when
> we allocate/deallocate the CFB, so there's no risk that the FBC
> hardware may read or write to the memory right after it is freed from
> drm_mm. The downside is that it is possible for user space to change
> the buffer stride without triggering a disable/enable - only
> deactivate/activate -, so we'll have to handle this case somehow, even
> though it is uncommon - see igt's kms_frontbuffer_tracking test,
> fbc-stridechange subtest. It could be possible to implement a way to
> free+alloc the CFB during said stride change, but it would involve a
> lot of book-keeping - exactly as mentioned above - just for a rare
> case, so for now I'll keep it simple and just deactivate FBC. Besides,
> we may not even need to disable FBC since we do CFB over-allocation.

It's not really that rare. Starting a fullscreen client that covers a
single monitor in a multi-monitor setup will trigger a change in stride
on one of the CRTC (the monitors will be flipped independently).

Not that actually affects your argument, just presenting a use-case that
exists in the wild today.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/12] drm/i915: wait for a vblank instead of 50ms when enabling FBC

[Ville Syrjälä]

On Fri, Nov 13, 2015 at 09:03:43PM +0000, Chris Wilson wrote:
> On Fri, Nov 13, 2015 at 05:53:41PM -0200, Paulo Zanoni wrote:
> > Instead of waiting for 50ms, just wait until the next vblank, since
> > it's the minimum requirement.
> > 
> > This moves PC7 residency on my specific BDW machine running Cinnamon
> > from 60-70% to 84-89%. Without FBC, I get 20-25%. I'm using a
> > 3200x1800 eDP panel. Notice: this was the case when the patch was
> > originally proposed, the order of the FBC patches changed since then,
> > so the actual numbers might be slightly different now.
> > 
> > v2:
> >   - Rebase after changing the patch order.
> >   - Update the commit message.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  |  2 +-
> >  drivers/gpu/drm/i915/intel_fbc.c | 12 +++---------
> >  2 files changed, 4 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 9418bd5..ea08714 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -919,9 +919,9 @@ struct i915_fbc {
> >  
> >  	struct intel_fbc_work {
> >  		bool scheduled;
> > +		u32 scheduled_vblank;
> >  		struct work_struct work;
> >  		struct drm_framebuffer *fb;
> > -		unsigned long enable_jiffies;
> >  	} work;
> >  
> >  	const char *no_fbc_reason;
> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> > index aa82075..72de8a1 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -391,7 +391,6 @@ static void intel_fbc_work_fn(struct work_struct *__work)
> >  		container_of(__work, struct drm_i915_private, fbc.work.work);
> >  	struct intel_fbc_work *work = &dev_priv->fbc.work;
> >  	struct intel_crtc *crtc = dev_priv->fbc.crtc;
> > -	unsigned long delay_jiffies = msecs_to_jiffies(50);
> >  
> >  retry:
> >  	/* Delay the actual enabling to let pageflipping cease and the
> > @@ -400,14 +399,9 @@ retry:
> >  	 * vblank to pass after disabling the FBC before we attempt
> >  	 * to modify the control registers.
> >  	 *
> > -	 * A more complicated solution would involve tracking vblanks
> > -	 * following the termination of the page-flipping sequence
> > -	 * and indeed performing the enable as a co-routine and not
> > -	 * waiting synchronously upon the vblank.
> > -	 *
> >  	 * WaFbcWaitForVBlankBeforeEnable:ilk,snb
> >  	 */
> > -	wait_remaining_ms_from_jiffies(work->enable_jiffies, delay_jiffies);
> > +	intel_wait_for_vblank(dev_priv->dev, crtc->pipe);
> >  
> >  	mutex_lock(&dev_priv->fbc.lock);
> >  
> > @@ -416,7 +410,7 @@ retry:
> >  		goto out;
> >  
> >  	/* Were we delayed again while this function was sleeping? */
> > -	if (time_after(work->enable_jiffies + delay_jiffies, jiffies)) {
> > +	if (drm_crtc_vblank_get(&crtc->base) == work->scheduled_vblank) {
> >  		mutex_unlock(&dev_priv->fbc.lock);
> >  		goto retry;
> >  	}
> > @@ -449,7 +443,7 @@ static void intel_fbc_schedule_activation(struct intel_crtc *crtc)
> >  	 * jiffy count. */
> >  	work->fb = crtc->base.primary->fb;
> >  	work->scheduled = true;
> > -	work->enable_jiffies = jiffies;
> > +	work->scheduled_vblank = drm_crtc_vblank_count(&crtc->base);
> 
> Isn't the frame counter only incrementing whilst the vblank IRQ is
> enabled? Ville?

I see a "+ if (drm_crtc_vblank_get(" earlier.

That said, drm_crtc_vblank_count() is racy. The reader may race with
the vblank irq handler after the start of vblank has already been
passed, thus sampling a stale value, and then actually not waiting
until the next vblank.

It's more of a problem on gen2/3 which didn't have the "start of
vblank" interrupt and instead rely on the "frame start" interrupt.
There's at least one (well, almost) scanline between the two events.

I've been meaning to add another function to the vblank code that
could be called between the drm_vblank_get() and drm_crtc_vblank_count()
to make sure the sampled count is really up to date. I'd use that on
gen2 at least since it lacks the hw counter. For the other platforms,
it ought to be easier to just use the hw counter everywhere since
that increments atomically with the start of vblank and doesn't suffer
from this race.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/12] drm/i915: wait for a vblank instead of 50ms when enabling FBC

[chris]

On Fri, Nov 13, 2015 at 09:17:04PM +0000, Zanoni, Paulo R wrote:
> Em Sex, 2015-11-13 às 21:03 +0000, Chris Wilson escreveu:
> > On Fri, Nov 13, 2015 at 05:53:41PM -0200, Paulo Zanoni wrote:
> > > Instead of waiting for 50ms, just wait until the next vblank, since
> > > it's the minimum requirement.
> > > 
> > > This moves PC7 residency on my specific BDW machine running
> > > Cinnamon
> > > from 60-70% to 84-89%. Without FBC, I get 20-25%. I'm using a
> > > 3200x1800 eDP panel. Notice: this was the case when the patch was
> > > originally proposed, the order of the FBC patches changed since
> > > then,
> > > so the actual numbers might be slightly different now.
> > > 
> > > v2:
> > >   - Rebase after changing the patch order.
> > >   - Update the commit message.
> > > 
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h  |  2 +-
> > >  drivers/gpu/drm/i915/intel_fbc.c | 12 +++---------
> > >  2 files changed, 4 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 9418bd5..ea08714 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -919,9 +919,9 @@ struct i915_fbc {
> > >  
> > >  	struct intel_fbc_work {
> > >  		bool scheduled;
> > > +		u32 scheduled_vblank;
> > >  		struct work_struct work;
> > >  		struct drm_framebuffer *fb;
> > > -		unsigned long enable_jiffies;
> > >  	} work;
> > >  
> > >  	const char *no_fbc_reason;
> > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > index aa82075..72de8a1 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > @@ -391,7 +391,6 @@ static void intel_fbc_work_fn(struct
> > > work_struct *__work)
> > >  		container_of(__work, struct drm_i915_private,
> > > fbc.work.work);
> > >  	struct intel_fbc_work *work = &dev_priv->fbc.work;
> > >  	struct intel_crtc *crtc = dev_priv->fbc.crtc;
> > > -	unsigned long delay_jiffies = msecs_to_jiffies(50);
> > >  
> > >  retry:
> > >  	/* Delay the actual enabling to let pageflipping cease and
> > > the
> > > @@ -400,14 +399,9 @@ retry:
> > >  	 * vblank to pass after disabling the FBC before we
> > > attempt
> > >  	 * to modify the control registers.
> > >  	 *
> > > -	 * A more complicated solution would involve tracking
> > > vblanks
> > > -	 * following the termination of the page-flipping sequence
> > > -	 * and indeed performing the enable as a co-routine and
> > > not
> > > -	 * waiting synchronously upon the vblank.
> > > -	 *
> > >  	 * WaFbcWaitForVBlankBeforeEnable:ilk,snb
> > >  	 */
> > > -	wait_remaining_ms_from_jiffies(work->enable_jiffies,
> > > delay_jiffies);
> > > +	intel_wait_for_vblank(dev_priv->dev, crtc->pipe);
> > >  
> > >  	mutex_lock(&dev_priv->fbc.lock);
> > >  
> > > @@ -416,7 +410,7 @@ retry:
> > >  		goto out;
> > >  
> > >  	/* Were we delayed again while this function was sleeping?
> > > */
> > > -	if (time_after(work->enable_jiffies + delay_jiffies,
> > > jiffies)) {
> > > +	if (drm_crtc_vblank_get(&crtc->base) == work-
> > > >scheduled_vblank) {
> > >  		mutex_unlock(&dev_priv->fbc.lock);
> > >  		goto retry;
> > >  	}
> > > @@ -449,7 +443,7 @@ static void
> > > intel_fbc_schedule_activation(struct intel_crtc *crtc)
> > >  	 * jiffy count. */
> > >  	work->fb = crtc->base.primary->fb;
> > >  	work->scheduled = true;
> > > -	work->enable_jiffies = jiffies;
> > > +	work->scheduled_vblank = drm_crtc_vblank_count(&crtc-
> > > >base);
> > 
> > Isn't the frame counter only incrementing whilst the vblank IRQ is
> > enabled? Ville?
> 
> At the work function we call intel_wait_for_vblank(), which calls
> drm_wait_one_vblank(), which calls drm_vblank_get().

And drm_vblank_put...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/12] drm/i915: wait for a vblank instead of 50ms when enabling FBC

[Ville Syrjälä]

On Fri, Nov 13, 2015 at 11:20:19PM +0200, Ville Syrjälä wrote:
> On Fri, Nov 13, 2015 at 09:03:43PM +0000, Chris Wilson wrote:
> > On Fri, Nov 13, 2015 at 05:53:41PM -0200, Paulo Zanoni wrote:
> > > Instead of waiting for 50ms, just wait until the next vblank, since
> > > it's the minimum requirement.
> > > 
> > > This moves PC7 residency on my specific BDW machine running Cinnamon
> > > from 60-70% to 84-89%. Without FBC, I get 20-25%. I'm using a
> > > 3200x1800 eDP panel. Notice: this was the case when the patch was
> > > originally proposed, the order of the FBC patches changed since then,
> > > so the actual numbers might be slightly different now.
> > > 
> > > v2:
> > >   - Rebase after changing the patch order.
> > >   - Update the commit message.
> > > 
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h  |  2 +-
> > >  drivers/gpu/drm/i915/intel_fbc.c | 12 +++---------
> > >  2 files changed, 4 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 9418bd5..ea08714 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -919,9 +919,9 @@ struct i915_fbc {
> > >  
> > >  	struct intel_fbc_work {
> > >  		bool scheduled;
> > > +		u32 scheduled_vblank;
> > >  		struct work_struct work;
> > >  		struct drm_framebuffer *fb;
> > > -		unsigned long enable_jiffies;
> > >  	} work;
> > >  
> > >  	const char *no_fbc_reason;
> > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> > > index aa82075..72de8a1 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > @@ -391,7 +391,6 @@ static void intel_fbc_work_fn(struct work_struct *__work)
> > >  		container_of(__work, struct drm_i915_private, fbc.work.work);
> > >  	struct intel_fbc_work *work = &dev_priv->fbc.work;
> > >  	struct intel_crtc *crtc = dev_priv->fbc.crtc;
> > > -	unsigned long delay_jiffies = msecs_to_jiffies(50);
> > >  
> > >  retry:
> > >  	/* Delay the actual enabling to let pageflipping cease and the
> > > @@ -400,14 +399,9 @@ retry:
> > >  	 * vblank to pass after disabling the FBC before we attempt
> > >  	 * to modify the control registers.
> > >  	 *
> > > -	 * A more complicated solution would involve tracking vblanks
> > > -	 * following the termination of the page-flipping sequence
> > > -	 * and indeed performing the enable as a co-routine and not
> > > -	 * waiting synchronously upon the vblank.
> > > -	 *
> > >  	 * WaFbcWaitForVBlankBeforeEnable:ilk,snb
> > >  	 */
> > > -	wait_remaining_ms_from_jiffies(work->enable_jiffies, delay_jiffies);
> > > +	intel_wait_for_vblank(dev_priv->dev, crtc->pipe);
> > >  
> > >  	mutex_lock(&dev_priv->fbc.lock);
> > >  
> > > @@ -416,7 +410,7 @@ retry:
> > >  		goto out;
> > >  
> > >  	/* Were we delayed again while this function was sleeping? */
> > > -	if (time_after(work->enable_jiffies + delay_jiffies, jiffies)) {
> > > +	if (drm_crtc_vblank_get(&crtc->base) == work->scheduled_vblank) {
> > >  		mutex_unlock(&dev_priv->fbc.lock);
> > >  		goto retry;
> > >  	}
> > > @@ -449,7 +443,7 @@ static void intel_fbc_schedule_activation(struct intel_crtc *crtc)
> > >  	 * jiffy count. */
> > >  	work->fb = crtc->base.primary->fb;
> > >  	work->scheduled = true;
> > > -	work->enable_jiffies = jiffies;
> > > +	work->scheduled_vblank = drm_crtc_vblank_count(&crtc->base);
> > 
> > Isn't the frame counter only incrementing whilst the vblank IRQ is
> > enabled? Ville?
> 
> I see a "+ if (drm_crtc_vblank_get(" earlier.

Hmm. Actually it's doing
"drm_crtc_vblank_get(&crtc->base) == work->scheduled_vblank)"
which looks rather like nonsense.

Not sure what the intention here was...

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/12] drm/i915: introduce is_active/activate/deactivate to the FBC terminology

[Chris Wilson]

On Fri, Nov 13, 2015 at 05:53:36PM -0200, Paulo Zanoni wrote:
> The long term goal is to have enable/disable as the higher level
> functions and activate/deactivate as the lower level functions, just
> like we do for PSR and for the CRTC. This way, we'll run enable and
> disable once per modeset, while update, activate and deactivate will
> be run many times. With this, we can move the checks and code that
> need to run only once per modeset to enable(), making the code simpler
> and possibly a little faster.
> 
> This patch is just the first step on the conversion: it starts by
> converting the current low level functions from enable/disable to
> activate/deactivate. This patch by itself has no benefits other than
> making review and rebase easier. Please see the next patches for more
> details on the conversion.
> 
> v2:
>   - Rebase.
>   - Improve commit message (Chris).
> v3: Rebase after changing the patch order.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/12] drm/i915: wait for a vblank instead of 50ms when enabling FBC

[Zanoni, Paulo R]

Em Sex, 2015-11-13 às 23:26 +0200, Ville Syrjälä escreveu:
> On Fri, Nov 13, 2015 at 11:20:19PM +0200, Ville Syrjälä wrote:
> > On Fri, Nov 13, 2015 at 09:03:43PM +0000, Chris Wilson wrote:
> > > On Fri, Nov 13, 2015 at 05:53:41PM -0200, Paulo Zanoni wrote:
> > > > Instead of waiting for 50ms, just wait until the next vblank,
> > > > since
> > > > it's the minimum requirement.
> > > > 
> > > > This moves PC7 residency on my specific BDW machine running
> > > > Cinnamon
> > > > from 60-70% to 84-89%. Without FBC, I get 20-25%. I'm using a
> > > > 3200x1800 eDP panel. Notice: this was the case when the patch
> > > > was
> > > > originally proposed, the order of the FBC patches changed since
> > > > then,
> > > > so the actual numbers might be slightly different now.
> > > > 
> > > > v2:
> > > >   - Rebase after changing the patch order.
> > > >   - Update the commit message.
> > > > 
> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.h  |  2 +-
> > > >  drivers/gpu/drm/i915/intel_fbc.c | 12 +++---------
> > > >  2 files changed, 4 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 9418bd5..ea08714 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -919,9 +919,9 @@ struct i915_fbc {
> > > >  
> > > >  	struct intel_fbc_work {
> > > >  		bool scheduled;
> > > > +		u32 scheduled_vblank;
> > > >  		struct work_struct work;
> > > >  		struct drm_framebuffer *fb;
> > > > -		unsigned long enable_jiffies;
> > > >  	} work;
> > > >  
> > > >  	const char *no_fbc_reason;
> > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > > index aa82075..72de8a1 100644
> > > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > > @@ -391,7 +391,6 @@ static void intel_fbc_work_fn(struct
> > > > work_struct *__work)
> > > >  		container_of(__work, struct drm_i915_private,
> > > > fbc.work.work);
> > > >  	struct intel_fbc_work *work = &dev_priv->fbc.work;
> > > >  	struct intel_crtc *crtc = dev_priv->fbc.crtc;
> > > > -	unsigned long delay_jiffies = msecs_to_jiffies(50);
> > > >  
> > > >  retry:
> > > >  	/* Delay the actual enabling to let pageflipping cease
> > > > and the
> > > > @@ -400,14 +399,9 @@ retry:
> > > >  	 * vblank to pass after disabling the FBC before we
> > > > attempt
> > > >  	 * to modify the control registers.
> > > >  	 *
> > > > -	 * A more complicated solution would involve tracking
> > > > vblanks
> > > > -	 * following the termination of the page-flipping
> > > > sequence
> > > > -	 * and indeed performing the enable as a co-routine
> > > > and not
> > > > -	 * waiting synchronously upon the vblank.
> > > > -	 *
> > > >  	 * WaFbcWaitForVBlankBeforeEnable:ilk,snb
> > > >  	 */
> > > > -	wait_remaining_ms_from_jiffies(work->enable_jiffies,
> > > > delay_jiffies);
> > > > +	intel_wait_for_vblank(dev_priv->dev, crtc->pipe);
> > > >  
> > > >  	mutex_lock(&dev_priv->fbc.lock);
> > > >  
> > > > @@ -416,7 +410,7 @@ retry:
> > > >  		goto out;
> > > >  
> > > >  	/* Were we delayed again while this function was
> > > > sleeping? */
> > > > -	if (time_after(work->enable_jiffies + delay_jiffies,
> > > > jiffies)) {
> > > > +	if (drm_crtc_vblank_get(&crtc->base) == work-
> > > > >scheduled_vblank) {
> > > >  		mutex_unlock(&dev_priv->fbc.lock);
> > > >  		goto retry;
> > > >  	}
> > > > @@ -449,7 +443,7 @@ static void
> > > > intel_fbc_schedule_activation(struct intel_crtc *crtc)
> > > >  	 * jiffy count. */
> > > >  	work->fb = crtc->base.primary->fb;
> > > >  	work->scheduled = true;
> > > > -	work->enable_jiffies = jiffies;
> > > > +	work->scheduled_vblank = drm_crtc_vblank_count(&crtc-
> > > > >base);
> > > 
> > > Isn't the frame counter only incrementing whilst the vblank IRQ
> > > is
> > > enabled? Ville?
> > 
> > I see a "+ if (drm_crtc_vblank_get(" earlier.
> 
> Hmm. Actually it's doing
> "drm_crtc_vblank_get(&crtc->base) == work->scheduled_vblank)"
> which looks rather like nonsense.
> 
> Not sure what the intention here was...

Ouch. The intent was for that to be another call for
drm_crtc_vblank_count().

The code in discussion is completely based on the drm_wait_one_vblank()
code: call drm_vblank_count(), then call it again until it returns
something different. The difference is that we actually call
drm_wait_one_vblank() in the middle of the process, and that
scheduled_vblank may also be updated in the meantime, so so may have to
call drm_wait_one_vblank() again.

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

Re: [PATCH 02/12] drm/i915: set dev_priv->fbc.crtc before scheduling the enable work

[Chris Wilson]

On Fri, Nov 13, 2015 at 07:13:40PM -0200, Paulo Zanoni wrote:
> 2015-11-13 18:56 GMT-02:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Fri, Nov 13, 2015 at 05:53:34PM -0200, Paulo Zanoni wrote:
> >> This thing where we need to get the crtc either from the work
> >> structure or the fbc structure itself is confusing and unnecessary.
> >> Set fbc.crtc right when scheduling the enable work so we can always
> >> use it.
> >>
> >> The problem is not what gets passed and how to retrieve it. The
> >> problem is that when we're in the other parts of the code we always
> >> have to keep in mind that if FBC is already enabled we have to get the
> >> CRTC from place A, if FBC is scheduled we have to get the CRTC from
> >> place B, and if it's disabled there's no CRTC. Having a single place
> >> to retrieve the CRTC from allows us to treat the "is enabled" and "is
> >> scheduled" cases as the same case, reducing the mistake surface. I
> >> guess I should add this to the commit message.
> >>
> >> Besides the immediate advantages, this is also going to make one of
> >> the next commits much simpler. And even later, when we introduce
> >> enable/disable + activate/deactivate, this will be even simpler as
> >> we'll set the CRTC at enable time. So all the
> >> activate/deactivate/update code can just look at the single CRTC
> >> variable regardless of the current state.
> >>
> >> v2: Improve commit message (Chris).
> >> v3: Rebase after changing the patch order.
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_drv.h  |  1 -
> >>  drivers/gpu/drm/i915/intel_fbc.c | 22 +++++++++-------------
> >>  2 files changed, 9 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> index 71bd1dc..317414b 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -920,7 +920,6 @@ struct i915_fbc {
> >>
> >>       struct intel_fbc_work {
> >>               struct delayed_work work;
> >> -             struct intel_crtc *crtc;
> >>               struct drm_framebuffer *fb;
> >>       } *fbc_work;
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> >> index 611672f..bc9cd33 100644
> >> --- a/drivers/gpu/drm/i915/intel_fbc.c
> >> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> >> @@ -334,14 +334,13 @@ bool intel_fbc_enabled(struct drm_i915_private *dev_priv)
> >>       return dev_priv->fbc.enabled;
> >>  }
> >>
> >> -static void intel_fbc_enable(struct intel_crtc *crtc,
> >> -                          const struct drm_framebuffer *fb)
> >> +static void intel_fbc_enable(const struct drm_framebuffer *fb)
> >>  {
> >> -     struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> >> +     struct drm_i915_private *dev_priv = fb->dev->dev_private;
> >> +     struct intel_crtc *crtc = dev_priv->fbc.crtc;
> >
> > This is confusing me. I think of FBC in terms of the CRTC/pipe, and the
> > fb just describes the plane currently bound to the primary. You've
> > pushed everywhere else to also work on the CRTC, I think it would be
> > consistent here to pass crtc and then use fbc.fb_id =
> > crtc->primary->fb->id.
> >
> > Have I missed something in the later patches that explains the choice
> > here?
> 
> You are right that this is confusing. This is basically an artifact
> from the previous FBC design that I overlooked. I guess during the
> conversion, my thinking was "well, the arguments are the stuff we
> store in the work struct, and since we don't store the CRTC anymore,
> let's remove it and keep passing the FB since it's still part of the
> work struct". Now that you bring this under a different perspective, I
> see the problem. We don't even need to pass these things as arguments
> since we do check that work->fb is equal to crtc->fb.
> 
> I just hope you let me address this with follow-up patches. Function
> intel_fbc_activate() was created because there were 2 callers: one for
> the case there the work_fn happens, another for the case where we
> failed to allocate the work_fn. Later in the series there's only one
> caller because there's no allocation anymore, so the best way to fix
> is probably to just move those 3 lines to the place of the only
> caller.
> 
> You also requested me to grab references of fbc.fb_id, and I have some
> evil plans to completely kill it, which would even reduce those 3
> lines further.

If you have it in hand, the rest seems fine, so
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/12] drm/i915: kill fbc.uncompressed_size

[Chris Wilson]

On Fri, Nov 13, 2015 at 05:53:42PM -0200, Paulo Zanoni wrote:
> Directly call intel_fbc_calculate_cfb_size() in the only place that
> actually needs it, and use the proper check before removing the stolen
> node. IMHO, this change makes our code easier to understand.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  1 -
>  drivers/gpu/drm/i915/intel_fbc.c | 13 ++++---------
>  2 files changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ea08714..43649c5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -901,7 +901,6 @@ struct i915_fbc {
>  	/* This is always the inner lock when overlapping with struct_mutex and
>  	 * it's the outer lock when overlapping with stolen_lock. */
>  	struct mutex lock;
> -	unsigned long uncompressed_size;
>  	unsigned threshold;
>  	unsigned int fb_id;
>  	unsigned int possible_framebuffer_bits;
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 72de8a1..d18eb80 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -139,7 +139,7 @@ static void i8xx_fbc_activate(struct intel_crtc *crtc)
>  	dev_priv->fbc.active = true;
>  
>  	/* Note: fbc.threshold == 1 for i8xx */
> -	cfb_pitch = dev_priv->fbc.uncompressed_size / FBC_LL_SIZE;
> +	cfb_pitch = intel_fbc_calculate_cfb_size(crtc, fb) / FBC_LL_SIZE;
>  	if (fb->pitches[0] < cfb_pitch)
>  		cfb_pitch = fb->pitches[0];
>  
> @@ -626,8 +626,6 @@ static int intel_fbc_alloc_cfb(struct intel_crtc *crtc)
>  			   dev_priv->mm.stolen_base + compressed_llb->start);
>  	}
>  
> -	dev_priv->fbc.uncompressed_size = size;
> -
>  	DRM_DEBUG_KMS("reserved %llu bytes of contiguous stolen space for FBC, threshold: %d\n",
>  		      dev_priv->fbc.compressed_fb.size,
>  		      dev_priv->fbc.threshold);
> @@ -644,18 +642,15 @@ err_llb:
>  
>  static void __intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv)
>  {
> -	if (dev_priv->fbc.uncompressed_size == 0)
> -		return;
> -
> -	i915_gem_stolen_remove_node(dev_priv, &dev_priv->fbc.compressed_fb);
> +	if (dev_priv->fbc.compressed_fb.allocated)

if (drm_mm_node_allocated(&dev_priv->fb.compressed_fb)) right?

We've been pretty consistent in using drm_mm_node_allocated() so we may
as well stick with the rigour.

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/12] drm/i915: fix the CFB size check

[Chris Wilson]

On Fri, Nov 13, 2015 at 05:53:33PM -0200, Paulo Zanoni wrote:
> In function find_compression_threshold() we try to over-allocate CFB
> space in order to reduce reallocations and fragmentation, and we're
> not considering that at the CFB size check. Consider it.
> 
> There is also a longer-term plan to kill
> dev_priv->fbc.uncompressed_size, but this will come later.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 11fc528..611672f 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -720,7 +720,8 @@ static int intel_fbc_setup_cfb(struct intel_crtc *crtc)
>  	size = intel_fbc_calculate_cfb_size(crtc);
>  	cpp = drm_format_plane_cpp(fb->pixel_format, 0);
>  
> -	if (size <= dev_priv->fbc.uncompressed_size)
> +	if (dev_priv->fbc.compressed_fb.allocated &&
> +	    size <= dev_priv->fbc.compressed_fb.size * dev_priv->fbc.threshold)
>  		return 0;

Hmm, not sure if it would be worth just clearing compressed_fb.size after
remove.

But at any rate, you want to use
drm_mm_node_allocated(&fbc.compressed_fb).

With that minor change,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/12] drm/i915: check for FBC planes in the same place as the pipes

[Chris Wilson]

On Fri, Nov 13, 2015 at 05:53:39PM -0200, Paulo Zanoni wrote:
> This moves the pre-gen4 check from update() to enable(). The HAS_DDI
> in the original code is not needed since only gen 2/3 have the plane
> swapping code.
> 
> v2: Rebase.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

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

> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index d496a7a..a0784d0 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -514,6 +514,9 @@ static bool crtc_can_fbc(struct intel_crtc *crtc)
>  	if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A)
>  		return false;
>  
> +	if (INTEL_INFO(dev_priv)->gen < 4 && crtc->plane != PLANE_A)
> +		return false;

Nit: you know what would really put the icing on the cake here,
if (fbc_on_plane_a_only(dev_priv) && crtc->plane != PLANE_A)
	return false;
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/12] drm/i915: only nuke FBC when a drawing operation triggers a flush

[Chris Wilson]

On Fri, Nov 13, 2015 at 05:53:44PM -0200, Paulo Zanoni wrote:

drm/i915: Only trigger a FBC recompress after flushing a drawing operation

> There's no need to stop and restart FBC: a nuke should be fine.

There's no need to stop and restart FBC, which is quite expensive as we
have to revalidate the CRTC state and reallocate resources, after flushing
a drawing operation (as the CRTC state hasn't changed). A nuke (recompress)
should be fine.

> v2: Make it simpler (Chris).
> v3: Rewrite the patch again due to patch order changes.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

If you can make the changelog understandable,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/12] drm/i915: wait for a vblank instead of 50ms when enabling FBC

[Ville Syrjälä]

On Fri, Nov 13, 2015 at 09:38:50PM +0000, Zanoni, Paulo R wrote:
> Em Sex, 2015-11-13 às 23:26 +0200, Ville Syrjälä escreveu:
> > On Fri, Nov 13, 2015 at 11:20:19PM +0200, Ville Syrjälä wrote:
> > > On Fri, Nov 13, 2015 at 09:03:43PM +0000, Chris Wilson wrote:
> > > > On Fri, Nov 13, 2015 at 05:53:41PM -0200, Paulo Zanoni wrote:
> > > > > Instead of waiting for 50ms, just wait until the next vblank,
> > > > > since
> > > > > it's the minimum requirement.
> > > > > 
> > > > > This moves PC7 residency on my specific BDW machine running
> > > > > Cinnamon
> > > > > from 60-70% to 84-89%. Without FBC, I get 20-25%. I'm using a
> > > > > 3200x1800 eDP panel. Notice: this was the case when the patch
> > > > > was
> > > > > originally proposed, the order of the FBC patches changed since
> > > > > then,
> > > > > so the actual numbers might be slightly different now.
> > > > > 
> > > > > v2:
> > > > >   - Rebase after changing the patch order.
> > > > >   - Update the commit message.
> > > > > 
> > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_drv.h  |  2 +-
> > > > >  drivers/gpu/drm/i915/intel_fbc.c | 12 +++---------
> > > > >  2 files changed, 4 insertions(+), 10 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > > index 9418bd5..ea08714 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > @@ -919,9 +919,9 @@ struct i915_fbc {
> > > > >  
> > > > >  	struct intel_fbc_work {
> > > > >  		bool scheduled;
> > > > > +		u32 scheduled_vblank;
> > > > >  		struct work_struct work;
> > > > >  		struct drm_framebuffer *fb;
> > > > > -		unsigned long enable_jiffies;
> > > > >  	} work;
> > > > >  
> > > > >  	const char *no_fbc_reason;
> > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > > > index aa82075..72de8a1 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > > > @@ -391,7 +391,6 @@ static void intel_fbc_work_fn(struct
> > > > > work_struct *__work)
> > > > >  		container_of(__work, struct drm_i915_private,
> > > > > fbc.work.work);
> > > > >  	struct intel_fbc_work *work = &dev_priv->fbc.work;
> > > > >  	struct intel_crtc *crtc = dev_priv->fbc.crtc;
> > > > > -	unsigned long delay_jiffies = msecs_to_jiffies(50);
> > > > >  
> > > > >  retry:
> > > > >  	/* Delay the actual enabling to let pageflipping cease
> > > > > and the
> > > > > @@ -400,14 +399,9 @@ retry:
> > > > >  	 * vblank to pass after disabling the FBC before we
> > > > > attempt
> > > > >  	 * to modify the control registers.
> > > > >  	 *
> > > > > -	 * A more complicated solution would involve tracking
> > > > > vblanks
> > > > > -	 * following the termination of the page-flipping
> > > > > sequence
> > > > > -	 * and indeed performing the enable as a co-routine
> > > > > and not
> > > > > -	 * waiting synchronously upon the vblank.
> > > > > -	 *
> > > > >  	 * WaFbcWaitForVBlankBeforeEnable:ilk,snb
> > > > >  	 */
> > > > > -	wait_remaining_ms_from_jiffies(work->enable_jiffies,
> > > > > delay_jiffies);
> > > > > +	intel_wait_for_vblank(dev_priv->dev, crtc->pipe);
> > > > >  
> > > > >  	mutex_lock(&dev_priv->fbc.lock);
> > > > >  
> > > > > @@ -416,7 +410,7 @@ retry:
> > > > >  		goto out;
> > > > >  
> > > > >  	/* Were we delayed again while this function was
> > > > > sleeping? */
> > > > > -	if (time_after(work->enable_jiffies + delay_jiffies,
> > > > > jiffies)) {
> > > > > +	if (drm_crtc_vblank_get(&crtc->base) == work-
> > > > > >scheduled_vblank) {
> > > > >  		mutex_unlock(&dev_priv->fbc.lock);
> > > > >  		goto retry;
> > > > >  	}
> > > > > @@ -449,7 +443,7 @@ static void
> > > > > intel_fbc_schedule_activation(struct intel_crtc *crtc)
> > > > >  	 * jiffy count. */
> > > > >  	work->fb = crtc->base.primary->fb;
> > > > >  	work->scheduled = true;
> > > > > -	work->enable_jiffies = jiffies;
> > > > > +	work->scheduled_vblank = drm_crtc_vblank_count(&crtc-
> > > > > >base);
> > > > 
> > > > Isn't the frame counter only incrementing whilst the vblank IRQ
> > > > is
> > > > enabled? Ville?
> > > 
> > > I see a "+ if (drm_crtc_vblank_get(" earlier.
> > 
> > Hmm. Actually it's doing
> > "drm_crtc_vblank_get(&crtc->base) == work->scheduled_vblank)"
> > which looks rather like nonsense.
> > 
> > Not sure what the intention here was...
> 
> Ouch. The intent was for that to be another call for
> drm_crtc_vblank_count().
> 
> The code in discussion is completely based on the drm_wait_one_vblank()
> code: call drm_vblank_count(), then call it again until it returns
> something different. The difference is that we actually call
> drm_wait_one_vblank() in the middle of the process, and that
> scheduled_vblank may also be updated in the meantime, so so may have to
> call drm_wait_one_vblank() again.

You have no guarantees that drm_crtc_vblank_count() won't give you
something totally stale unless you have a vblank reference when calling
it. If you have a reference it's guaranteed to give you something fairly
recent, but the race I outlined in the earlier mail still exists. And yes,
drm_wait_one_vblank() is no good due to that same race.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/12] drm/i915: wait for a vblank instead of 50ms when enabling FBC

[Zanoni, Paulo R]

Em Sáb, 2015-11-14 às 01:17 +0200, Ville Syrjälä escreveu:
> On Fri, Nov 13, 2015 at 09:38:50PM +0000, Zanoni, Paulo R wrote:
> > Em Sex, 2015-11-13 às 23:26 +0200, Ville Syrjälä escreveu:
> > > On Fri, Nov 13, 2015 at 11:20:19PM +0200, Ville Syrjälä wrote:
> > > > On Fri, Nov 13, 2015 at 09:03:43PM +0000, Chris Wilson wrote:
> > > > > On Fri, Nov 13, 2015 at 05:53:41PM -0200, Paulo Zanoni wrote:
> > > > > > Instead of waiting for 50ms, just wait until the next
> > > > > > vblank,
> > > > > > since
> > > > > > it's the minimum requirement.
> > > > > > 
> > > > > > This moves PC7 residency on my specific BDW machine running
> > > > > > Cinnamon
> > > > > > from 60-70% to 84-89%. Without FBC, I get 20-25%. I'm using
> > > > > > a
> > > > > > 3200x1800 eDP panel. Notice: this was the case when the
> > > > > > patch
> > > > > > was
> > > > > > originally proposed, the order of the FBC patches changed
> > > > > > since
> > > > > > then,
> > > > > > so the actual numbers might be slightly different now.
> > > > > > 
> > > > > > v2:
> > > > > >   - Rebase after changing the patch order.
> > > > > >   - Update the commit message.
> > > > > > 
> > > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/i915_drv.h  |  2 +-
> > > > > >  drivers/gpu/drm/i915/intel_fbc.c | 12 +++---------
> > > > > >  2 files changed, 4 insertions(+), 10 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > > > index 9418bd5..ea08714 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > > @@ -919,9 +919,9 @@ struct i915_fbc {
> > > > > >  
> > > > > >  	struct intel_fbc_work {
> > > > > >  		bool scheduled;
> > > > > > +		u32 scheduled_vblank;
> > > > > >  		struct work_struct work;
> > > > > >  		struct drm_framebuffer *fb;
> > > > > > -		unsigned long enable_jiffies;
> > > > > >  	} work;
> > > > > >  
> > > > > >  	const char *no_fbc_reason;
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > > > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > > > > index aa82075..72de8a1 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > > > > @@ -391,7 +391,6 @@ static void intel_fbc_work_fn(struct
> > > > > > work_struct *__work)
> > > > > >  		container_of(__work, struct
> > > > > > drm_i915_private,
> > > > > > fbc.work.work);
> > > > > >  	struct intel_fbc_work *work = &dev_priv->fbc.work;
> > > > > >  	struct intel_crtc *crtc = dev_priv->fbc.crtc;
> > > > > > -	unsigned long delay_jiffies =
> > > > > > msecs_to_jiffies(50);
> > > > > >  
> > > > > >  retry:
> > > > > >  	/* Delay the actual enabling to let pageflipping
> > > > > > cease
> > > > > > and the
> > > > > > @@ -400,14 +399,9 @@ retry:
> > > > > >  	 * vblank to pass after disabling the FBC before
> > > > > > we
> > > > > > attempt
> > > > > >  	 * to modify the control registers.
> > > > > >  	 *
> > > > > > -	 * A more complicated solution would involve
> > > > > > tracking
> > > > > > vblanks
> > > > > > -	 * following the termination of the page-flipping
> > > > > > sequence
> > > > > > -	 * and indeed performing the enable as a co-
> > > > > > routine
> > > > > > and not
> > > > > > -	 * waiting synchronously upon the vblank.
> > > > > > -	 *
> > > > > >  	 * WaFbcWaitForVBlankBeforeEnable:ilk,snb
> > > > > >  	 */
> > > > > > -	wait_remaining_ms_from_jiffies(work-
> > > > > > >enable_jiffies,
> > > > > > delay_jiffies);
> > > > > > +	intel_wait_for_vblank(dev_priv->dev, crtc->pipe);
> > > > > >  
> > > > > >  	mutex_lock(&dev_priv->fbc.lock);
> > > > > >  
> > > > > > @@ -416,7 +410,7 @@ retry:
> > > > > >  		goto out;
> > > > > >  
> > > > > >  	/* Were we delayed again while this function was
> > > > > > sleeping? */
> > > > > > -	if (time_after(work->enable_jiffies +
> > > > > > delay_jiffies,
> > > > > > jiffies)) {
> > > > > > +	if (drm_crtc_vblank_get(&crtc->base) == work-
> > > > > > > scheduled_vblank) {
> > > > > >  		mutex_unlock(&dev_priv->fbc.lock);
> > > > > >  		goto retry;
> > > > > >  	}
> > > > > > @@ -449,7 +443,7 @@ static void
> > > > > > intel_fbc_schedule_activation(struct intel_crtc *crtc)
> > > > > >  	 * jiffy count. */
> > > > > >  	work->fb = crtc->base.primary->fb;
> > > > > >  	work->scheduled = true;
> > > > > > -	work->enable_jiffies = jiffies;
> > > > > > +	work->scheduled_vblank =
> > > > > > drm_crtc_vblank_count(&crtc-
> > > > > > > base);
> > > > > 
> > > > > Isn't the frame counter only incrementing whilst the vblank
> > > > > IRQ
> > > > > is
> > > > > enabled? Ville?
> > > > 
> > > > I see a "+ if (drm_crtc_vblank_get(" earlier.
> > > 
> > > Hmm. Actually it's doing
> > > "drm_crtc_vblank_get(&crtc->base) == work->scheduled_vblank)"
> > > which looks rather like nonsense.
> > > 
> > > Not sure what the intention here was...
> > 
> > Ouch. The intent was for that to be another call for
> > drm_crtc_vblank_count().
> > 
> > The code in discussion is completely based on the
> > drm_wait_one_vblank()
> > code: call drm_vblank_count(), then call it again until it returns
> > something different. The difference is that we actually call
> > drm_wait_one_vblank() in the middle of the process, and that
> > scheduled_vblank may also be updated in the meantime, so so may
> > have to
> > call drm_wait_one_vblank() again.
> 
> You have no guarantees that drm_crtc_vblank_count() won't give you
> something totally stale unless you have a vblank reference when
> calling
> it. If you have a reference it's guaranteed to give you something
> fairly
> recent,

All the points mentioned above are easily fixable.

> but the race I outlined in the earlier mail still exists.

I've been trying to understand this, and it seems
i915_get_vblank_counter() tries to work around that problem with the
logic behind the "pixel" and "vbl_start" variables. Isn't that enough?
If no, why?


>  And yes,
> drm_wait_one_vblank() is no good due to that same race.

So shouldn't we stop using intel_wait_for_vblank() on the affected
platforms? Or at least add a big comment explaining the problem, or add
a WARN(gen == 3 || gen == 4 && !is_g4x), or something else. I got a
little surprised when you said the function is unreliable (was the
previous implementation based on I915_READ also equally unreliable?).

Also, since we are already running this risk in other parts of the
code, I'm not sure what are the conditions for my patch to be
acceptable. Just fixing the original implementation, while keeping it
based on drm_wait_one_vblank() and maybe adding some TODO comment would
do? Or do you see an actual simple solution for the race you mentioned?

Thanks,
Paulo

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

Re: [PATCH 09/12] drm/i915: wait for a vblank instead of 50ms when enabling FBC

[Ville Syrjälä]

On Tue, Nov 17, 2015 at 07:03:13PM +0000, Zanoni, Paulo R wrote:
> Em Sáb, 2015-11-14 às 01:17 +0200, Ville Syrjälä escreveu:
> > On Fri, Nov 13, 2015 at 09:38:50PM +0000, Zanoni, Paulo R wrote:
> > > Em Sex, 2015-11-13 às 23:26 +0200, Ville Syrjälä escreveu:
> > > > On Fri, Nov 13, 2015 at 11:20:19PM +0200, Ville Syrjälä wrote:
> > > > > On Fri, Nov 13, 2015 at 09:03:43PM +0000, Chris Wilson wrote:
> > > > > > On Fri, Nov 13, 2015 at 05:53:41PM -0200, Paulo Zanoni wrote:
> > > > > > > Instead of waiting for 50ms, just wait until the next
> > > > > > > vblank,
> > > > > > > since
> > > > > > > it's the minimum requirement.
> > > > > > > 
> > > > > > > This moves PC7 residency on my specific BDW machine running
> > > > > > > Cinnamon
> > > > > > > from 60-70% to 84-89%. Without FBC, I get 20-25%. I'm using
> > > > > > > a
> > > > > > > 3200x1800 eDP panel. Notice: this was the case when the
> > > > > > > patch
> > > > > > > was
> > > > > > > originally proposed, the order of the FBC patches changed
> > > > > > > since
> > > > > > > then,
> > > > > > > so the actual numbers might be slightly different now.
> > > > > > > 
> > > > > > > v2:
> > > > > > >   - Rebase after changing the patch order.
> > > > > > >   - Update the commit message.
> > > > > > > 
> > > > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/i915/i915_drv.h  |  2 +-
> > > > > > >  drivers/gpu/drm/i915/intel_fbc.c | 12 +++---------
> > > > > > >  2 files changed, 4 insertions(+), 10 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > > > > index 9418bd5..ea08714 100644
> > > > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > > > @@ -919,9 +919,9 @@ struct i915_fbc {
> > > > > > >  
> > > > > > >  	struct intel_fbc_work {
> > > > > > >  		bool scheduled;
> > > > > > > +		u32 scheduled_vblank;
> > > > > > >  		struct work_struct work;
> > > > > > >  		struct drm_framebuffer *fb;
> > > > > > > -		unsigned long enable_jiffies;
> > > > > > >  	} work;
> > > > > > >  
> > > > > > >  	const char *no_fbc_reason;
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > > > > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > > > > > index aa82075..72de8a1 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > > > > > @@ -391,7 +391,6 @@ static void intel_fbc_work_fn(struct
> > > > > > > work_struct *__work)
> > > > > > >  		container_of(__work, struct
> > > > > > > drm_i915_private,
> > > > > > > fbc.work.work);
> > > > > > >  	struct intel_fbc_work *work = &dev_priv->fbc.work;
> > > > > > >  	struct intel_crtc *crtc = dev_priv->fbc.crtc;
> > > > > > > -	unsigned long delay_jiffies =
> > > > > > > msecs_to_jiffies(50);
> > > > > > >  
> > > > > > >  retry:
> > > > > > >  	/* Delay the actual enabling to let pageflipping
> > > > > > > cease
> > > > > > > and the
> > > > > > > @@ -400,14 +399,9 @@ retry:
> > > > > > >  	 * vblank to pass after disabling the FBC before
> > > > > > > we
> > > > > > > attempt
> > > > > > >  	 * to modify the control registers.
> > > > > > >  	 *
> > > > > > > -	 * A more complicated solution would involve
> > > > > > > tracking
> > > > > > > vblanks
> > > > > > > -	 * following the termination of the page-flipping
> > > > > > > sequence
> > > > > > > -	 * and indeed performing the enable as a co-
> > > > > > > routine
> > > > > > > and not
> > > > > > > -	 * waiting synchronously upon the vblank.
> > > > > > > -	 *
> > > > > > >  	 * WaFbcWaitForVBlankBeforeEnable:ilk,snb
> > > > > > >  	 */
> > > > > > > -	wait_remaining_ms_from_jiffies(work-
> > > > > > > >enable_jiffies,
> > > > > > > delay_jiffies);
> > > > > > > +	intel_wait_for_vblank(dev_priv->dev, crtc->pipe);
> > > > > > >  
> > > > > > >  	mutex_lock(&dev_priv->fbc.lock);
> > > > > > >  
> > > > > > > @@ -416,7 +410,7 @@ retry:
> > > > > > >  		goto out;
> > > > > > >  
> > > > > > >  	/* Were we delayed again while this function was
> > > > > > > sleeping? */
> > > > > > > -	if (time_after(work->enable_jiffies +
> > > > > > > delay_jiffies,
> > > > > > > jiffies)) {
> > > > > > > +	if (drm_crtc_vblank_get(&crtc->base) == work-
> > > > > > > > scheduled_vblank) {
> > > > > > >  		mutex_unlock(&dev_priv->fbc.lock);
> > > > > > >  		goto retry;
> > > > > > >  	}
> > > > > > > @@ -449,7 +443,7 @@ static void
> > > > > > > intel_fbc_schedule_activation(struct intel_crtc *crtc)
> > > > > > >  	 * jiffy count. */
> > > > > > >  	work->fb = crtc->base.primary->fb;
> > > > > > >  	work->scheduled = true;
> > > > > > > -	work->enable_jiffies = jiffies;
> > > > > > > +	work->scheduled_vblank =
> > > > > > > drm_crtc_vblank_count(&crtc-
> > > > > > > > base);
> > > > > > 
> > > > > > Isn't the frame counter only incrementing whilst the vblank
> > > > > > IRQ
> > > > > > is
> > > > > > enabled? Ville?
> > > > > 
> > > > > I see a "+ if (drm_crtc_vblank_get(" earlier.
> > > > 
> > > > Hmm. Actually it's doing
> > > > "drm_crtc_vblank_get(&crtc->base) == work->scheduled_vblank)"
> > > > which looks rather like nonsense.
> > > > 
> > > > Not sure what the intention here was...
> > > 
> > > Ouch. The intent was for that to be another call for
> > > drm_crtc_vblank_count().
> > > 
> > > The code in discussion is completely based on the
> > > drm_wait_one_vblank()
> > > code: call drm_vblank_count(), then call it again until it returns
> > > something different. The difference is that we actually call
> > > drm_wait_one_vblank() in the middle of the process, and that
> > > scheduled_vblank may also be updated in the meantime, so so may
> > > have to
> > > call drm_wait_one_vblank() again.
> > 
> > You have no guarantees that drm_crtc_vblank_count() won't give you
> > something totally stale unless you have a vblank reference when
> > calling
> > it. If you have a reference it's guaranteed to give you something
> > fairly
> > recent,
> 
> All the points mentioned above are easily fixable.
> 
> > but the race I outlined in the earlier mail still exists.
> 
> I've been trying to understand this, and it seems
> i915_get_vblank_counter() tries to work around that problem with the
> logic behind the "pixel" and "vbl_start" variables. Isn't that enough?
> If no, why?

No i915_get_vblank_counter() is there for another reason; On gen3/4
the hw counter increments at the wrong time, so we have to convert
it to look like it incremented at the right time. Fortunately we
can read out the hw counter and the pixel counter atomically, so
there's no race or anything due to the conversion.

> >  And yes,
> > drm_wait_one_vblank() is no good due to that same race.
> 
> So shouldn't we stop using intel_wait_for_vblank() on the affected
> platforms?

All platforms are affected, just some are more susceptible than others.
We should fix intel_wait_for_vblank() to do the right thing.

> Or at least add a big comment explaining the problem, or add
> a WARN(gen == 3 || gen == 4 && !is_g4x), or something else. I got a
> little surprised when you said the function is unreliable (was the
> previous implementation based on I915_READ also equally unreliable?).

On g4x+ the polling implementation didn't suffer from this issue since
it was polling the hw frame counter. But the fact that it was polling
was sub-optimal.

On older platforms a similar race was there since it was polling the
"frame start" bit and not the "start of vblank" bit. In addition
it could also effectively eat the interrupt that someone was waiting
for (on gen2/3 since they use the "frame start" as the interrupt as
well).

> Also, since we are already running this risk in other parts of the
> code, I'm not sure what are the conditions for my patch to be
> acceptable.

The main problem in your patch is using drm_vblank_count() w/o a
drm_vblank_get/put(). So that should be fixed somehow.

I guess we can ignore the race with the vblank interrupt handler
for now since the same problem exists everywhere else already.

> Just fixing the original implementation, while keeping it
> based on drm_wait_one_vblank() and maybe adding some TODO comment would
> do? Or do you see an actual simple solution for the race you mentioned?
> 
> Thanks,
> Paulo
> 
> > 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 06/12] drm/i915: alloc/free the FBC CFB during enable/disable

[Paulo Zanoni]

One of the problems with the current code is that it frees the CFB and
releases its drm_mm node as soon as we flip FBC's enable bit. This is
bad because after we disable FBC the hardware may still use the CFB
for the rest of the frame, so in theory we should only release the
drm_mm node one frame after we disable FBC. Otherwise, a stolen memory
allocation done right after an FBC disable may result in either
corrupted memory for the new owner of that memory region or corrupted
screen/underruns in case the new owner changes it while the hardware
is still reading it. This case is not exactly easy to reproduce since
we currently don't do a lot of stolen memory allocations, but I see
patches on the mailing list trying to expose stolen memory to user
space, so races will be possible.

I thought about three different approaches to solve this, and they all
have downsides.

The first approach would be to simply use multiple drm_mm nodes and
freeing the unused ones only after a frame has passed. The problem
with this approach is that since stolen memory is rather small,
there's a risk we just won't be able to allocate a new CFB from stolen
if the previous one was not freed yet. This could happen in case we
quickly disable FBC from pipe A and decide to enable it on pipe B, or
just if we change pipe A's fb stride while FBC is enabled.

The second approach would be similar to the first one, but maintaining
a single drm_mm node and keeping track of when it can be reused. This
would remove the disadvantage of not having enough space for two
nodes, but would create the new problem where we may not be able to
enable FBC at the point intel_fbc_update() is called, so we would have
to add more code to retry updating FBC after the time has passed. And
that can quickly get too complex since we can get invalidate, flush,
disable and other calls in the middle of the wait.

Both solutions above - and also the current code - have the problem
that we unnecessarily free+realloc FBC during invalidate+flush
operations even if the CFB size doesn't change.

The third option would be to move the allocation/deallocation to
enable/disable. This makes sure that the pipe is always disabled when
we allocate/deallocate the CFB, so there's no risk that the FBC
hardware may read or write to the memory right after it is freed from
drm_mm. The downside is that it is possible for user space to change
the buffer stride without triggering a disable/enable - only
deactivate/activate -, so we'll have to handle this case somehow - see
igt's kms_frontbuffer_tracking test, fbc-stridechange subtest. It
could be possible to implement a way to free+alloc the CFB during said
stride change, but it would involve a lot of book-keeping - exactly as
mentioned above - just for on case, so for now I'll keep it simple and
just deactivate FBC. Besides, we may not even need to disable FBC
since we do CFB over-allocation.

Note from Chris: "Starting a fullscreen client that covers a single
monitor in a multi-monitor setup will trigger a change in stride on
one of the CRTCs (the monitors will be flipped independently).". It
shouldn't be a huge problem if we lose FBC on multi-monitor setups
since these setups already have problems reaching deep PC states
anyway.

v2: Rebase after changing the patch order.
v3:
  - Remove references to the stride change case being "uncommon" and
    paste Chris' example.
  - Rebase after a change in a previous patch.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 134 ++++++++++++++++++++-------------------
 1 file changed, 69 insertions(+), 65 deletions(-)


I didn't want to resend yet the patches that had "do this trivial
change then consider it Reviewed-by", so this one won't apply nicely
on top of the others currently on the list. This, along with patch 09
are the only ones missing a RB tag, and I'll skip 09 for now due to
the vblank_disable_immediate discussion.


diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 6125c7b..958f973 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -64,6 +64,46 @@ static unsigned int get_crtc_fence_y_offset(struct intel_crtc *crtc)
 	return crtc->base.y - crtc->adjusted_y;
 }
 
+/*
+ * For SKL+, the plane source size used by the hardware is based on the value we
+ * write to the PLANE_SIZE register. For BDW-, the hardware looks at the value
+ * we wrote to PIPESRC.
+ */
+static void intel_fbc_get_plane_source_size(struct intel_crtc *crtc,
+					    int *width, int *height)
+{
+	struct intel_plane_state *plane_state =
+			to_intel_plane_state(crtc->base.primary->state);
+	int w, h;
+
+	if (intel_rotation_90_or_270(plane_state->base.rotation)) {
+		w = drm_rect_height(&plane_state->src) >> 16;
+		h = drm_rect_width(&plane_state->src) >> 16;
+	} else {
+		w = drm_rect_width(&plane_state->src) >> 16;
+		h = drm_rect_height(&plane_state->src) >> 16;
+	}
+
+	if (width)
+		*width = w;
+	if (height)
+		*height = h;
+}
+
+static int intel_fbc_calculate_cfb_size(struct intel_crtc *crtc,
+					struct drm_framebuffer *fb)
+{
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	int lines;
+
+	intel_fbc_get_plane_source_size(crtc, NULL, &lines);
+	if (INTEL_INFO(dev_priv)->gen >= 7)
+		lines = min(lines, 2048);
+
+	/* Hardware needs the full buffer stride, not just the active area. */
+	return lines * fb->pitches[0];
+}
+
 static void i8xx_fbc_deactivate(struct drm_i915_private *dev_priv)
 {
 	u32 fbc_ctl;
@@ -558,11 +598,17 @@ again:
 	}
 }
 
-static int intel_fbc_alloc_cfb(struct drm_i915_private *dev_priv, int size,
-			       int fb_cpp)
+static int intel_fbc_alloc_cfb(struct intel_crtc *crtc)
 {
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct drm_framebuffer *fb = crtc->base.primary->state->fb;
 	struct drm_mm_node *uninitialized_var(compressed_llb);
-	int ret;
+	int size, fb_cpp, ret;
+
+	WARN_ON(drm_mm_node_allocated(&dev_priv->fbc.compressed_fb));
+
+	size = intel_fbc_calculate_cfb_size(crtc, fb);
+	fb_cpp = drm_format_plane_cpp(fb->pixel_format, 0);
 
 	ret = find_compression_threshold(dev_priv, &dev_priv->fbc.compressed_fb,
 					 size, fb_cpp);
@@ -639,65 +685,6 @@ void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv)
 	mutex_unlock(&dev_priv->fbc.lock);
 }
 
-/*
- * For SKL+, the plane source size used by the hardware is based on the value we
- * write to the PLANE_SIZE register. For BDW-, the hardware looks at the value
- * we wrote to PIPESRC.
- */
-static void intel_fbc_get_plane_source_size(struct intel_crtc *crtc,
-					    int *width, int *height)
-{
-	struct intel_plane_state *plane_state =
-			to_intel_plane_state(crtc->base.primary->state);
-	int w, h;
-
-	if (intel_rotation_90_or_270(plane_state->base.rotation)) {
-		w = drm_rect_height(&plane_state->src) >> 16;
-		h = drm_rect_width(&plane_state->src) >> 16;
-	} else {
-		w = drm_rect_width(&plane_state->src) >> 16;
-		h = drm_rect_height(&plane_state->src) >> 16;
-	}
-
-	if (width)
-		*width = w;
-	if (height)
-		*height = h;
-}
-
-static int intel_fbc_calculate_cfb_size(struct intel_crtc *crtc)
-{
-	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
-	struct drm_framebuffer *fb = crtc->base.primary->fb;
-	int lines;
-
-	intel_fbc_get_plane_source_size(crtc, NULL, &lines);
-	if (INTEL_INFO(dev_priv)->gen >= 7)
-		lines = min(lines, 2048);
-
-	/* Hardware needs the full buffer stride, not just the active area. */
-	return lines * fb->pitches[0];
-}
-
-static int intel_fbc_setup_cfb(struct intel_crtc *crtc)
-{
-	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
-	struct drm_framebuffer *fb = crtc->base.primary->fb;
-	int size, cpp;
-
-	size = intel_fbc_calculate_cfb_size(crtc);
-	cpp = drm_format_plane_cpp(fb->pixel_format, 0);
-
-	if (drm_mm_node_allocated(&dev_priv->fbc.compressed_fb) &&
-	    size <= dev_priv->fbc.compressed_fb.size * dev_priv->fbc.threshold)
-		return 0;
-
-	/* Release any current block */
-	__intel_fbc_cleanup_cfb(dev_priv);
-
-	return intel_fbc_alloc_cfb(dev_priv, size, cpp);
-}
-
 static bool stride_is_valid(struct drm_i915_private *dev_priv,
 			    unsigned int stride)
 {
@@ -853,8 +840,19 @@ static void __intel_fbc_update(struct intel_crtc *crtc)
 		goto out_disable;
 	}
 
-	if (intel_fbc_setup_cfb(crtc)) {
-		set_no_fbc_reason(dev_priv, "not enough stolen memory");
+	/* It is possible for the required CFB size change without a
+	 * crtc->disable + crtc->enable since it is possible to change the
+	 * stride without triggering a full modeset. Since we try to
+	 * over-allocate the CFB, there's a chance we may keep FBC enabled even
+	 * if this happens, but if we exceed the current CFB size we'll have to
+	 * disable FBC. Notice that it would be possible to disable FBC, wait
+	 * for a frame, free the stolen node, then try to reenable FBC in case
+	 * we didn't get any invalidate/deactivate calls, but this would require
+	 * a lot of tracking just for a specific case. If we conclude it's an
+	 * important case, we can implement it later. */
+	if (intel_fbc_calculate_cfb_size(crtc, fb) >
+	    dev_priv->fbc.compressed_fb.size * dev_priv->fbc.threshold) {
+		set_no_fbc_reason(dev_priv, "CFB requirements changed");
 		goto out_disable;
 	}
 
@@ -907,7 +905,6 @@ out_disable:
 		DRM_DEBUG_KMS("unsupported config, deactivating FBC\n");
 		__intel_fbc_deactivate(dev_priv);
 	}
-	__intel_fbc_cleanup_cfb(dev_priv);
 }
 
 /*
@@ -1020,6 +1017,11 @@ void intel_fbc_enable(struct intel_crtc *crtc)
 		goto out;
 	}
 
+	if (intel_fbc_alloc_cfb(crtc)) {
+		set_no_fbc_reason(dev_priv, "not enough stolen memory");
+		goto out;
+	}
+
 	DRM_DEBUG_KMS("Enabling FBC on pipe %c\n", pipe_name(crtc->pipe));
 	dev_priv->fbc.no_fbc_reason = "FBC enabled but not active yet\n";
 
@@ -1047,6 +1049,8 @@ static void __intel_fbc_disable(struct drm_i915_private *dev_priv)
 
 	DRM_DEBUG_KMS("Disabling FBC on pipe %c\n", pipe_name(crtc->pipe));
 
+	__intel_fbc_cleanup_cfb(dev_priv);
+
 	dev_priv->fbc.enabled = false;
 	dev_priv->fbc.crtc = NULL;
 }
-- 
2.6.2

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

Re: [PATCH 02/12] drm/i915: set dev_priv->fbc.crtc before scheduling the enable work

[Daniel Stone]

Hi,

On 13 November 2015 at 21:13, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2015-11-13 18:56 GMT-02:00 Chris Wilson <chris@chris-wilson.co.uk>:
>> This is confusing me. I think of FBC in terms of the CRTC/pipe, and the
>> fb just describes the plane currently bound to the primary. You've
>> pushed everywhere else to also work on the CRTC, I think it would be
>> consistent here to pass crtc and then use fbc.fb_id =
>> crtc->primary->fb->id.
>>
>> Have I missed something in the later patches that explains the choice
>> here?
>
> You are right that this is confusing. This is basically an artifact
> from the previous FBC design that I overlooked. I guess during the
> conversion, my thinking was "well, the arguments are the stuff we
> store in the work struct, and since we don't store the CRTC anymore,
> let's remove it and keep passing the FB since it's still part of the
> work struct". Now that you bring this under a different perspective, I
> see the problem. We don't even need to pass these things as arguments
> since we do check that work->fb is equal to crtc->fb.

With async modesetting on its way, this won't actually hold true for
long. Tracking the fb separately, rather than pulling it back out of
the CRTC, is the right thing to do here.

Basically, if you ever want to check crtc->primary->fb, and you're not
in atomic_check, you're doing it wrong.

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