[PATCH 00/25] FBC crtc/fb locking + smaller fixes

[PATCH 00/25] FBC crtc/fb locking + smaller fixes

[Paulo Zanoni]

Hi

Here's yet another patch series randomly modifying the FBC code. We
start by refactoring things in order to fix the locking problems, then
fix a few other smaller problems and apply some polishing.

Just to keep the tradition of the past 10 cover letters, I guess I
should say that this series is the last one and that we may consider
enabling FBC on HSW/BDW/SKL after it is merged :)

For SKL specifically, I tested this series on a two-weeks old version
of drm-intel-nightly since today's version is giving me a BUG() even
without my patches applied.

Thanks,
Paulo

Paulo Zanoni (25):
  drm/i915/fbc: wait for a vblank instead of 50ms when enabling
  drm/i915/fbc: extract intel_fbc_can_activate()
  drm/i915/fbc: extract intel_fbc_can_enable()
  drm/i915/fbc: introduce struct intel_fbc_reg_params
  drm/i915/fbc: replace frequent dev_priv->fbc.x with fbc->x
  drm/i915/fbc: don't use the frontbuffer tracking subsystem for flips
  drm/i915/fbc: don't flush for operations on the wrong frontbuffer
  drm/i915/fbc: unconditionally update FBC during atomic commits
  drm/i915/fbc: introduce struct intel_fbc_state_cache
  drm/i915/fbc: split intel_fbc_update into pre and post update
  drm/i915/fbc: fix the FBC state checking code
  drm/i915/fbc: unexport intel_fbc_deactivate
  drm/i915/fbc: rename the FBC disable functions
  drm/i915/fbc: make sure we cancel the work function at fbc_disable
  drm/i915/fbc: rewrite the multiple_pipes_ok() code for locking
  drm/i915: simplify struct drm_device access at intel_atomic_check()
  drm/i915/fbc: choose the new FBC CRTC during atomic check
  drm/i915/fbc: move intel_fbc_{enable,disable} call one level up
  drm/i915/fbc: make FBC work with fastboot
  drm/i915/fbc: don't try to deactivate FBC if it's not enabled
  drm/i915/fbc: don't print no_fbc_reason to dmesg
  drm/i915/fbc: don't store the fb_id on reg_params
  drm/i915/fbc: call intel_fbc_pre_update earlier during page flips
  drm/i915/fbc: don't store/check a pointer to the FB
  drm/i915/fbc: refactor some small functions called only once

 drivers/gpu/drm/i915/i915_drv.h      |  51 +-
 drivers/gpu/drm/i915/i915_suspend.c  |   2 +-
 drivers/gpu/drm/i915/intel_display.c |  59 +--
 drivers/gpu/drm/i915/intel_drv.h     |  18 +-
 drivers/gpu/drm/i915/intel_fbc.c     | 901 ++++++++++++++++++++---------------
 5 files changed, 589 insertions(+), 442 deletions(-)

-- 
2.6.4

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

[PATCH 01/25] drm/i915/fbc: wait for a vblank instead of 50ms when enabling

[Paulo Zanoni]

Instead of waiting for 50ms, just wait until the next vblank, since
it's the minimum requirement. The whole infrastructure of FBC is based
on vblanks, so waiting for X vblanks instead of X milliseconds sounds
like the correct way to go. Besides, 50ms may be less than a vblank on
super slow modes that may or may not exist.

There are some small improvements in PC state residency (due to the
fact that we're now using 16ms for the common modes instead of 50ms),
but the biggest advantage is still the correctness of being
vblank-based instead of time-based.

v2:
  - Rebase after changing the patch order.
  - Update the commit message.
v3:
  - Fix bogus vblank_get() instead of vblank_count() (Ville).
  - Don't forget to call drm_crtc_vblank_{get,put} (Chris, Ville)
  - Adjust the performance details on 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 | 43 ++++++++++++++++++++++++++++------------
 2 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index af30148..33217a4 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -925,9 +925,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 a1988a4..6b43ec3 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -381,7 +381,15 @@ 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;
-	int delay_ms = 50;
+	struct drm_vblank_crtc *vblank = &dev_priv->dev->vblank[crtc->pipe];
+
+	mutex_lock(&dev_priv->fbc.lock);
+	if (drm_crtc_vblank_get(&crtc->base)) {
+		DRM_ERROR("vblank not available for FBC on pipe %c\n",
+			  pipe_name(crtc->pipe));
+		goto out;
+	}
+	mutex_unlock(&dev_priv->fbc.lock);
 
 retry:
 	/* Delay the actual enabling to let pageflipping cease and the
@@ -390,24 +398,25 @@ 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
+	 *
+	 * It is also worth mentioning that since work->scheduled_vblank can be
+	 * updated multiple times by the other threads, hitting the timeout is
+	 * not an error condition. We'll just end up hitting the "goto retry"
+	 * case below.
 	 */
-	wait_remaining_ms_from_jiffies(work->enable_jiffies, delay_ms);
+	wait_event_timeout(vblank->queue,
+		drm_crtc_vblank_count(&crtc->base) != work->scheduled_vblank,
+		msecs_to_jiffies(50));
 
 	mutex_lock(&dev_priv->fbc.lock);
 
 	/* Were we cancelled? */
 	if (!work->scheduled)
-		goto out;
+		goto out_put;
 
 	/* Were we delayed again while this function was sleeping? */
-	if (time_after(work->enable_jiffies + msecs_to_jiffies(delay_ms),
-		       jiffies)) {
+	if (drm_crtc_vblank_count(&crtc->base) == work->scheduled_vblank) {
 		mutex_unlock(&dev_priv->fbc.lock);
 		goto retry;
 	}
@@ -415,9 +424,10 @@ retry:
 	if (crtc->base.primary->fb == work->fb)
 		intel_fbc_activate(work->fb);
 
-	work->scheduled = false;
-
+out_put:
+	drm_crtc_vblank_put(&crtc->base);
 out:
+	work->scheduled = false;
 	mutex_unlock(&dev_priv->fbc.lock);
 }
 
@@ -434,13 +444,20 @@ static void intel_fbc_schedule_activation(struct intel_crtc *crtc)
 
 	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
 
+	if (drm_crtc_vblank_get(&crtc->base)) {
+		DRM_ERROR("vblank not available for FBC on pipe %c\n",
+			  pipe_name(crtc->pipe));
+		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;
 	work->scheduled = true;
-	work->enable_jiffies = jiffies;
+	work->scheduled_vblank = drm_crtc_vblank_count(&crtc->base);
+	drm_crtc_vblank_put(&crtc->base);
 
 	schedule_work(&work->work);
 }
-- 
2.6.4

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

[PATCH 02/25] drm/i915/fbc: extract intel_fbc_can_activate()

[Paulo Zanoni]

Extract all the code that checks if the FBC configuration is valid to
its own function, making __intel_fbc_update() much simpler.

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

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 6b43ec3..dff30e1 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -514,17 +514,6 @@ static bool crtc_can_fbc(struct intel_crtc *crtc)
 	return true;
 }
 
-static bool crtc_is_valid(struct intel_crtc *crtc)
-{
-	if (!intel_crtc_active(&crtc->base))
-		return false;
-
-	if (!to_intel_plane_state(crtc->base.primary->state)->visible)
-		return false;
-
-	return true;
-}
-
 static bool multiple_pipes_ok(struct drm_i915_private *dev_priv)
 {
 	enum pipe pipe;
@@ -750,48 +739,40 @@ static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc)
 	return effective_w <= max_w && effective_h <= max_h;
 }
 
-/**
- * __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 activates,
- * deactivates or maintains it on the same state.
- */
-static void __intel_fbc_update(struct intel_crtc *crtc)
+static bool intel_fbc_can_activate(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct drm_plane *primary;
 	struct drm_framebuffer *fb;
+	struct intel_plane_state *plane_state;
 	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.enabled || dev_priv->fbc.crtc != crtc)
-		return;
-
-	if (!crtc_is_valid(crtc)) {
-		set_no_fbc_reason(dev_priv, "no output");
-		goto out_disable;
+	if (!intel_crtc_active(&crtc->base)) {
+		set_no_fbc_reason(dev_priv, "CRTC not active");
+		return false;
 	}
 
-	fb = crtc->base.primary->fb;
+	primary = crtc->base.primary;
+	fb = primary->fb;
 	obj = intel_fb_obj(fb);
 	adjusted_mode = &crtc->config->base.adjusted_mode;
+	plane_state = to_intel_plane_state(primary->state);
+
+	if (!plane_state->visible) {
+		set_no_fbc_reason(dev_priv, "primary plane not visible");
+		return false;
+	}
 
 	if ((adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) ||
 	    (adjusted_mode->flags & DRM_MODE_FLAG_DBLSCAN)) {
 		set_no_fbc_reason(dev_priv, "incompatible mode");
-		goto out_disable;
+		return false;
 	}
 
 	if (!intel_fbc_hw_tracking_covers_screen(crtc)) {
 		set_no_fbc_reason(dev_priv, "mode too large for compression");
-		goto out_disable;
+		return false;
 	}
 
 	/* The use of a CPU fence is mandatory in order to detect writes
@@ -800,22 +781,22 @@ static void __intel_fbc_update(struct intel_crtc *crtc)
 	if (obj->tiling_mode != I915_TILING_X ||
 	    obj->fence_reg == I915_FENCE_REG_NONE) {
 		set_no_fbc_reason(dev_priv, "framebuffer not tiled or fenced");
-		goto out_disable;
+		return false;
 	}
 	if (INTEL_INFO(dev_priv)->gen <= 4 && !IS_G4X(dev_priv) &&
-	    crtc->base.primary->state->rotation != BIT(DRM_ROTATE_0)) {
+	    plane_state->base.rotation != BIT(DRM_ROTATE_0)) {
 		set_no_fbc_reason(dev_priv, "rotation unsupported");
-		goto out_disable;
+		return false;
 	}
 
 	if (!stride_is_valid(dev_priv, fb->pitches[0])) {
 		set_no_fbc_reason(dev_priv, "framebuffer stride not supported");
-		goto out_disable;
+		return false;
 	}
 
 	if (!pixel_format_is_valid(fb)) {
 		set_no_fbc_reason(dev_priv, "pixel format is invalid");
-		goto out_disable;
+		return false;
 	}
 
 	/* WaFbcExceedCdClockThreshold:hsw,bdw */
@@ -823,7 +804,7 @@ static void __intel_fbc_update(struct intel_crtc *crtc)
 	    ilk_pipe_pixel_rate(crtc->config) >=
 	    dev_priv->cdclk_freq * 95 / 100) {
 		set_no_fbc_reason(dev_priv, "pixel rate is too big");
-		goto out_disable;
+		return false;
 	}
 
 	/* It is possible for the required CFB size change without a
@@ -839,16 +820,43 @@ static void __intel_fbc_update(struct intel_crtc *crtc)
 	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");
+		return false;
+	}
+
+	return true;
+}
+
+/**
+ * __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 activates,
+ * deactivates or maintains it on the same state.
+ */
+static void __intel_fbc_update(struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+
+	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.enabled || dev_priv->fbc.crtc != crtc)
+		return;
+
+	if (!intel_fbc_can_activate(crtc))
+		goto out_disable;
+
 	/* If the scanout has not changed, don't modify the FBC settings.
 	 * Note that we make the fundamental assumption that the fb->obj
 	 * cannot be unpinned (and have its GTT offset and fence revoked)
 	 * without first being decoupled from the scanout and FBC disabled.
 	 */
 	if (dev_priv->fbc.crtc == crtc &&
-	    dev_priv->fbc.fb_id == fb->base.id &&
+	    dev_priv->fbc.fb_id == crtc->base.primary->fb->base.id &&
 	    dev_priv->fbc.y == crtc->base.y &&
 	    dev_priv->fbc.active)
 		return;
-- 
2.6.4

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

[PATCH 03/25] drm/i915/fbc: extract intel_fbc_can_enable()

[Paulo Zanoni]

Make our enable/activate checking model more explicit, especially
since we now have intel_fbc_can_activate().

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

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index dff30e1..f3cc6a3 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -826,6 +826,33 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
 	return true;
 }
 
+static bool intel_fbc_can_enable(struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+
+	if (intel_vgpu_active(dev_priv->dev)) {
+		set_no_fbc_reason(dev_priv, "VGPU is active");
+		return false;
+	}
+
+	if (i915.enable_fbc < 0) {
+		set_no_fbc_reason(dev_priv, "disabled per chip default");
+		return false;
+	}
+
+	if (!i915.enable_fbc) {
+		set_no_fbc_reason(dev_priv, "disabled per module param");
+		return false;
+	}
+
+	if (!crtc_can_fbc(crtc)) {
+		set_no_fbc_reason(dev_priv, "no enabled pipes can have FBC");
+		return false;
+	}
+
+	return true;
+}
+
 /**
  * __intel_fbc_update - activate/deactivate FBC as needed, unlocked
  * @crtc: the CRTC that triggered the update
@@ -995,25 +1022,8 @@ void intel_fbc_enable(struct intel_crtc *crtc)
 	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");
+	if (!intel_fbc_can_enable(crtc))
 		goto out;
-	}
-
-	if (!crtc_can_fbc(crtc)) {
-		set_no_fbc_reason(dev_priv, "no enabled pipes can have FBC");
-		goto out;
-	}
 
 	if (intel_fbc_alloc_cfb(crtc)) {
 		set_no_fbc_reason(dev_priv, "not enough stolen memory");
-- 
2.6.4

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

[PATCH 04/25] drm/i915/fbc: introduce struct intel_fbc_reg_params

[Paulo Zanoni]

The early return inside __intel_fbc_update does not completely check
all the parameters that affect the FBC register values. For example,
we currently lack looking at crtc->adjusted_y (for the fence Y offset)
and all the parameters that affect the CFB size (for i8xx).

Instead of just adding the missing parameters to the check and hoping
that any changes to the fbc_activate functions also come with a
matching change to the __intel_fbc_update check, introduce a new
structure where we store these parameters and use the structure at the
fbc_activate function. Of course, it's still possible to access
everything from dev_priv in those functions, but IMHO the new code
will be harder to break.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 33217a4..aa9c35e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -909,11 +909,9 @@ struct i915_fbc {
 	 * it's the outer lock when overlapping with stolen_lock. */
 	struct mutex lock;
 	unsigned threshold;
-	unsigned int fb_id;
 	unsigned int possible_framebuffer_bits;
 	unsigned int busy_bits;
 	struct intel_crtc *crtc;
-	int y;
 
 	struct drm_mm_node compressed_fb;
 	struct drm_mm_node *compressed_llb;
@@ -923,6 +921,24 @@ struct i915_fbc {
 	bool enabled;
 	bool active;
 
+	struct intel_fbc_reg_params {
+		struct {
+			enum pipe pipe;
+			enum plane plane;
+			unsigned int fence_y_offset;
+		} crtc;
+
+		struct {
+			u64 ggtt_offset;
+			uint32_t id;
+			uint32_t pixel_format;
+			unsigned int stride;
+			int fence_reg;
+		} fb;
+
+		int cfb_size;
+	} params;
+
 	struct intel_fbc_work {
 		bool scheduled;
 		u32 scheduled_vblank;
@@ -933,7 +949,7 @@ struct i915_fbc {
 	const char *no_fbc_reason;
 
 	bool (*is_active)(struct drm_i915_private *dev_priv);
-	void (*activate)(struct intel_crtc *crtc);
+	void (*activate)(struct drm_i915_private *dev_priv);
 	void (*deactivate)(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 f3cc6a3..f4ed2b3 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -130,11 +130,9 @@ static void i8xx_fbc_deactivate(struct drm_i915_private *dev_priv)
 	}
 }
 
-static void i8xx_fbc_activate(struct intel_crtc *crtc)
+static void i8xx_fbc_activate(struct drm_i915_private *dev_priv)
 {
-	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);
+	struct intel_fbc_reg_params *params = &dev_priv->fbc.params;
 	int cfb_pitch;
 	int i;
 	u32 fbc_ctl;
@@ -142,9 +140,9 @@ static void i8xx_fbc_activate(struct intel_crtc *crtc)
 	dev_priv->fbc.active = true;
 
 	/* Note: fbc.threshold == 1 for i8xx */
-	cfb_pitch = intel_fbc_calculate_cfb_size(crtc, fb) / FBC_LL_SIZE;
-	if (fb->pitches[0] < cfb_pitch)
-		cfb_pitch = fb->pitches[0];
+	cfb_pitch = params->cfb_size / FBC_LL_SIZE;
+	if (params->fb.stride < cfb_pitch)
+		cfb_pitch = params->fb.stride;
 
 	/* FBC_CTL wants 32B or 64B units */
 	if (IS_GEN2(dev_priv))
@@ -161,9 +159,9 @@ static void i8xx_fbc_activate(struct intel_crtc *crtc)
 
 		/* Set it up... */
 		fbc_ctl2 = FBC_CTL_FENCE_DBL | FBC_CTL_IDLE_IMM | FBC_CTL_CPU_FENCE;
-		fbc_ctl2 |= FBC_CTL_PLANE(crtc->plane);
+		fbc_ctl2 |= FBC_CTL_PLANE(params->crtc.plane);
 		I915_WRITE(FBC_CONTROL2, fbc_ctl2);
-		I915_WRITE(FBC_FENCE_OFF, get_crtc_fence_y_offset(crtc));
+		I915_WRITE(FBC_FENCE_OFF, params->crtc.fence_y_offset);
 	}
 
 	/* enable it... */
@@ -173,7 +171,7 @@ static void i8xx_fbc_activate(struct intel_crtc *crtc)
 	if (IS_I945GM(dev_priv))
 		fbc_ctl |= FBC_CTL_C3_IDLE; /* 945 needs special SR handling */
 	fbc_ctl |= (cfb_pitch & 0xff) << FBC_CTL_STRIDE_SHIFT;
-	fbc_ctl |= obj->fence_reg;
+	fbc_ctl |= params->fb.fence_reg;
 	I915_WRITE(FBC_CONTROL, fbc_ctl);
 }
 
@@ -182,23 +180,21 @@ static bool i8xx_fbc_is_active(struct drm_i915_private *dev_priv)
 	return I915_READ(FBC_CONTROL) & FBC_CTL_EN;
 }
 
-static void g4x_fbc_activate(struct intel_crtc *crtc)
+static void g4x_fbc_activate(struct drm_i915_private *dev_priv)
 {
-	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);
+	struct intel_fbc_reg_params *params = &dev_priv->fbc.params;
 	u32 dpfc_ctl;
 
 	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)
+	dpfc_ctl = DPFC_CTL_PLANE(params->crtc.plane) | DPFC_SR_EN;
+	if (drm_format_plane_cpp(params->fb.pixel_format, 0) == 2)
 		dpfc_ctl |= DPFC_CTL_LIMIT_2X;
 	else
 		dpfc_ctl |= DPFC_CTL_LIMIT_1X;
-	dpfc_ctl |= DPFC_CTL_FENCE_EN | obj->fence_reg;
+	dpfc_ctl |= DPFC_CTL_FENCE_EN | params->fb.fence_reg;
 
-	I915_WRITE(DPFC_FENCE_YOFF, get_crtc_fence_y_offset(crtc));
+	I915_WRITE(DPFC_FENCE_YOFF, params->crtc.fence_y_offset);
 
 	/* enable it... */
 	I915_WRITE(DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
@@ -230,19 +226,16 @@ static void intel_fbc_recompress(struct drm_i915_private *dev_priv)
 	POSTING_READ(MSG_FBC_REND_STATE);
 }
 
-static void ilk_fbc_activate(struct intel_crtc *crtc)
+static void ilk_fbc_activate(struct drm_i915_private *dev_priv)
 {
-	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);
+	struct intel_fbc_reg_params *params = &dev_priv->fbc.params;
 	u32 dpfc_ctl;
 	int threshold = dev_priv->fbc.threshold;
-	unsigned int y_offset;
 
 	dev_priv->fbc.active = true;
 
-	dpfc_ctl = DPFC_CTL_PLANE(crtc->plane);
-	if (drm_format_plane_cpp(fb->pixel_format, 0) == 2)
+	dpfc_ctl = DPFC_CTL_PLANE(params->crtc.plane);
+	if (drm_format_plane_cpp(params->fb.pixel_format, 0) == 2)
 		threshold++;
 
 	switch (threshold) {
@@ -259,18 +252,17 @@ static void ilk_fbc_activate(struct intel_crtc *crtc)
 	}
 	dpfc_ctl |= DPFC_CTL_FENCE_EN;
 	if (IS_GEN5(dev_priv))
-		dpfc_ctl |= obj->fence_reg;
+		dpfc_ctl |= params->fb.fence_reg;
 
-	y_offset = get_crtc_fence_y_offset(crtc);
-	I915_WRITE(ILK_DPFC_FENCE_YOFF, y_offset);
-	I915_WRITE(ILK_FBC_RT_BASE, i915_gem_obj_ggtt_offset(obj) | ILK_FBC_RT_VALID);
+	I915_WRITE(ILK_DPFC_FENCE_YOFF, params->crtc.fence_y_offset);
+	I915_WRITE(ILK_FBC_RT_BASE, params->fb.ggtt_offset | ILK_FBC_RT_VALID);
 	/* enable it... */
 	I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
 
 	if (IS_GEN6(dev_priv)) {
 		I915_WRITE(SNB_DPFC_CTL_SA,
-			   SNB_CPU_FENCE_ENABLE | obj->fence_reg);
-		I915_WRITE(DPFC_CPU_FENCE_OFFSET, y_offset);
+			   SNB_CPU_FENCE_ENABLE | params->fb.fence_reg);
+		I915_WRITE(DPFC_CPU_FENCE_OFFSET, params->crtc.fence_y_offset);
 	}
 
 	intel_fbc_recompress(dev_priv);
@@ -295,11 +287,9 @@ 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_activate(struct intel_crtc *crtc)
+static void gen7_fbc_activate(struct drm_i915_private *dev_priv)
 {
-	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);
+	struct intel_fbc_reg_params *params = &dev_priv->fbc.params;
 	u32 dpfc_ctl;
 	int threshold = dev_priv->fbc.threshold;
 
@@ -307,9 +297,9 @@ static void gen7_fbc_activate(struct intel_crtc *crtc)
 
 	dpfc_ctl = 0;
 	if (IS_IVYBRIDGE(dev_priv))
-		dpfc_ctl |= IVB_DPFC_CTL_PLANE(crtc->plane);
+		dpfc_ctl |= IVB_DPFC_CTL_PLANE(params->crtc.plane);
 
-	if (drm_format_plane_cpp(fb->pixel_format, 0) == 2)
+	if (drm_format_plane_cpp(params->fb.pixel_format, 0) == 2)
 		threshold++;
 
 	switch (threshold) {
@@ -337,16 +327,16 @@ static void gen7_fbc_activate(struct intel_crtc *crtc)
 			   ILK_FBCQ_DIS);
 	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
 		/* WaFbcAsynchFlipDisableFbcQueue:hsw,bdw */
-		I915_WRITE(CHICKEN_PIPESL_1(crtc->pipe),
-			   I915_READ(CHICKEN_PIPESL_1(crtc->pipe)) |
+		I915_WRITE(CHICKEN_PIPESL_1(params->crtc.pipe),
+			   I915_READ(CHICKEN_PIPESL_1(params->crtc.pipe)) |
 			   HSW_FBCQ_DIS);
 	}
 
 	I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
 
 	I915_WRITE(SNB_DPFC_CTL_SA,
-		   SNB_CPU_FENCE_ENABLE | obj->fence_reg);
-	I915_WRITE(DPFC_CPU_FENCE_OFFSET, get_crtc_fence_y_offset(crtc));
+		   SNB_CPU_FENCE_ENABLE | params->fb.fence_reg);
+	I915_WRITE(DPFC_CPU_FENCE_OFFSET, params->crtc.fence_y_offset);
 
 	intel_fbc_recompress(dev_priv);
 }
@@ -364,17 +354,6 @@ bool intel_fbc_is_active(struct drm_i915_private *dev_priv)
 	return dev_priv->fbc.active;
 }
 
-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.activate(crtc);
-
-	dev_priv->fbc.fb_id = fb->base.id;
-	dev_priv->fbc.y = crtc->base.y;
-}
-
 static void intel_fbc_work_fn(struct work_struct *__work)
 {
 	struct drm_i915_private *dev_priv =
@@ -422,7 +401,7 @@ retry:
 	}
 
 	if (crtc->base.primary->fb == work->fb)
-		intel_fbc_activate(work->fb);
+		dev_priv->fbc.activate(dev_priv);
 
 out_put:
 	drm_crtc_vblank_put(&crtc->base);
@@ -853,6 +832,42 @@ static bool intel_fbc_can_enable(struct intel_crtc *crtc)
 	return true;
 }
 
+static void intel_fbc_get_reg_params(struct intel_crtc *crtc,
+				     struct intel_fbc_reg_params *params)
+{
+	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);
+
+	/* Since all our fields are integer types, use memset here so the
+	 * comparison function can rely on memcmp because the padding will be
+	 * zero. */
+	memset(params, 0, sizeof(*params));
+
+	params->crtc.pipe = crtc->pipe;
+	params->crtc.plane = crtc->plane;
+	params->crtc.fence_y_offset = get_crtc_fence_y_offset(crtc);
+
+	params->fb.id = fb->base.id;
+	params->fb.pixel_format = fb->pixel_format;
+	params->fb.stride = fb->pitches[0];
+	params->fb.fence_reg = obj->fence_reg;
+
+	params->cfb_size = intel_fbc_calculate_cfb_size(crtc, fb);
+
+	/* FIXME: We lack the proper locking here, so only run this on the
+	 * platforms that need. */
+	if (dev_priv->fbc.activate == ilk_fbc_activate)
+		params->fb.ggtt_offset = i915_gem_obj_ggtt_offset(obj);
+}
+
+static bool intel_fbc_reg_params_equal(struct intel_fbc_reg_params *params1,
+				       struct intel_fbc_reg_params *params2)
+{
+	/* We can use this since intel_fbc_get_reg_params() does a memset. */
+	return memcmp(params1, params2, sizeof(*params1)) == 0;
+}
+
 /**
  * __intel_fbc_update - activate/deactivate FBC as needed, unlocked
  * @crtc: the CRTC that triggered the update
@@ -863,6 +878,7 @@ static bool intel_fbc_can_enable(struct intel_crtc *crtc)
 static void __intel_fbc_update(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct intel_fbc_reg_params old_params;
 
 	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
 
@@ -877,15 +893,16 @@ static void __intel_fbc_update(struct intel_crtc *crtc)
 	if (!intel_fbc_can_activate(crtc))
 		goto out_disable;
 
+	old_params = dev_priv->fbc.params;
+	intel_fbc_get_reg_params(crtc, &dev_priv->fbc.params);
+
 	/* If the scanout has not changed, don't modify the FBC settings.
 	 * Note that we make the fundamental assumption that the fb->obj
 	 * cannot be unpinned (and have its GTT offset and fence revoked)
 	 * without first being decoupled from the scanout and FBC disabled.
 	 */
-	if (dev_priv->fbc.crtc == crtc &&
-	    dev_priv->fbc.fb_id == crtc->base.primary->fb->base.id &&
-	    dev_priv->fbc.y == crtc->base.y &&
-	    dev_priv->fbc.active)
+	if (dev_priv->fbc.active &&
+	    intel_fbc_reg_params_equal(&old_params, &dev_priv->fbc.params))
 		return;
 
 	if (intel_fbc_is_active(dev_priv)) {
-- 
2.6.4

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

[PATCH 05/25] drm/i915/fbc: replace frequent dev_priv->fbc.x with fbc->x

[Paulo Zanoni]

We say "dev_priv->fbc.something" way too many times in our code while
we could be saying just "fbc->something" with a previous declaration
of fbc. This has been bothering me for a while but I didn't want to
patch it since I wanted to fix the real problems first. But as I add
more code I keep thinking about it, especially since it makes the code
easier to read and it can make us fit 80 columns easier, so let's just
do the change now.

While at it, also rename from i915_fbc to intel_fbc because the whole
FBC code uses intel_fbc.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index aa9c35e..6d11575 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -904,7 +904,7 @@ enum fb_op_origin {
 	ORIGIN_DIRTYFB,
 };
 
-struct i915_fbc {
+struct intel_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;
@@ -1778,7 +1778,7 @@ struct drm_i915_private {
 	u32 pipestat_irq_mask[I915_MAX_PIPES];
 
 	struct i915_hotplug hotplug;
-	struct i915_fbc fbc;
+	struct intel_fbc fbc;
 	struct i915_drrs drrs;
 	struct intel_opregion opregion;
 	struct intel_vbt_data vbt;
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index f4ed2b3..b4a4191 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -358,17 +358,18 @@ static void intel_fbc_work_fn(struct work_struct *__work)
 {
 	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;
+	struct intel_fbc *fbc = &dev_priv->fbc;
+	struct intel_fbc_work *work = &fbc->work;
+	struct intel_crtc *crtc = fbc->crtc;
 	struct drm_vblank_crtc *vblank = &dev_priv->dev->vblank[crtc->pipe];
 
-	mutex_lock(&dev_priv->fbc.lock);
+	mutex_lock(&fbc->lock);
 	if (drm_crtc_vblank_get(&crtc->base)) {
 		DRM_ERROR("vblank not available for FBC on pipe %c\n",
 			  pipe_name(crtc->pipe));
 		goto out;
 	}
-	mutex_unlock(&dev_priv->fbc.lock);
+	mutex_unlock(&fbc->lock);
 
 retry:
 	/* Delay the actual enabling to let pageflipping cease and the
@@ -388,7 +389,7 @@ retry:
 		drm_crtc_vblank_count(&crtc->base) != work->scheduled_vblank,
 		msecs_to_jiffies(50));
 
-	mutex_lock(&dev_priv->fbc.lock);
+	mutex_lock(&fbc->lock);
 
 	/* Were we cancelled? */
 	if (!work->scheduled)
@@ -396,32 +397,35 @@ retry:
 
 	/* Were we delayed again while this function was sleeping? */
 	if (drm_crtc_vblank_count(&crtc->base) == work->scheduled_vblank) {
-		mutex_unlock(&dev_priv->fbc.lock);
+		mutex_unlock(&fbc->lock);
 		goto retry;
 	}
 
 	if (crtc->base.primary->fb == work->fb)
-		dev_priv->fbc.activate(dev_priv);
+		fbc->activate(dev_priv);
 
 out_put:
 	drm_crtc_vblank_put(&crtc->base);
 out:
 	work->scheduled = false;
-	mutex_unlock(&dev_priv->fbc.lock);
+	mutex_unlock(&fbc->lock);
 }
 
 static void intel_fbc_cancel_work(struct drm_i915_private *dev_priv)
 {
-	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
-	dev_priv->fbc.work.scheduled = false;
+	struct intel_fbc *fbc = &dev_priv->fbc;
+
+	WARN_ON(!mutex_is_locked(&fbc->lock));
+	fbc->work.scheduled = false;
 }
 
 static void intel_fbc_schedule_activation(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
-	struct intel_fbc_work *work = &dev_priv->fbc.work;
+	struct intel_fbc *fbc = &dev_priv->fbc;
+	struct intel_fbc_work *work = &fbc->work;
 
-	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
+	WARN_ON(!mutex_is_locked(&fbc->lock));
 
 	if (drm_crtc_vblank_get(&crtc->base)) {
 		DRM_ERROR("vblank not available for FBC on pipe %c\n",
@@ -443,12 +447,14 @@ static void intel_fbc_schedule_activation(struct intel_crtc *crtc)
 
 static void __intel_fbc_deactivate(struct drm_i915_private *dev_priv)
 {
-	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
+	struct intel_fbc *fbc = &dev_priv->fbc;
+
+	WARN_ON(!mutex_is_locked(&fbc->lock));
 
 	intel_fbc_cancel_work(dev_priv);
 
-	if (dev_priv->fbc.active)
-		dev_priv->fbc.deactivate(dev_priv);
+	if (fbc->active)
+		fbc->deactivate(dev_priv);
 }
 
 /*
@@ -460,23 +466,26 @@ static void __intel_fbc_deactivate(struct drm_i915_private *dev_priv)
 void intel_fbc_deactivate(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct intel_fbc *fbc = &dev_priv->fbc;
 
 	if (!fbc_supported(dev_priv))
 		return;
 
-	mutex_lock(&dev_priv->fbc.lock);
-	if (dev_priv->fbc.crtc == crtc)
+	mutex_lock(&fbc->lock);
+	if (fbc->crtc == crtc)
 		__intel_fbc_deactivate(dev_priv);
-	mutex_unlock(&dev_priv->fbc.lock);
+	mutex_unlock(&fbc->lock);
 }
 
 static void set_no_fbc_reason(struct drm_i915_private *dev_priv,
 			      const char *reason)
 {
-	if (dev_priv->fbc.no_fbc_reason == reason)
+	struct intel_fbc *fbc = &dev_priv->fbc;
+
+	if (fbc->no_fbc_reason == reason)
 		return;
 
-	dev_priv->fbc.no_fbc_reason = reason;
+	fbc->no_fbc_reason = reason;
 	DRM_DEBUG_KMS("Disabling FBC: %s\n", reason);
 }
 
@@ -566,16 +575,17 @@ again:
 static int intel_fbc_alloc_cfb(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct intel_fbc *fbc = &dev_priv->fbc;
 	struct drm_framebuffer *fb = crtc->base.primary->state->fb;
 	struct drm_mm_node *uninitialized_var(compressed_llb);
 	int size, fb_cpp, ret;
 
-	WARN_ON(drm_mm_node_allocated(&dev_priv->fbc.compressed_fb));
+	WARN_ON(drm_mm_node_allocated(&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,
+	ret = find_compression_threshold(dev_priv, &fbc->compressed_fb,
 					 size, fb_cpp);
 	if (!ret)
 		goto err_llb;
@@ -584,12 +594,12 @@ static int intel_fbc_alloc_cfb(struct intel_crtc *crtc)
 
 	}
 
-	dev_priv->fbc.threshold = ret;
+	fbc->threshold = ret;
 
 	if (INTEL_INFO(dev_priv)->gen >= 5)
-		I915_WRITE(ILK_DPFC_CB_BASE, dev_priv->fbc.compressed_fb.start);
+		I915_WRITE(ILK_DPFC_CB_BASE, fbc->compressed_fb.start);
 	else if (IS_GM45(dev_priv)) {
-		I915_WRITE(DPFC_CB_BASE, dev_priv->fbc.compressed_fb.start);
+		I915_WRITE(DPFC_CB_BASE, fbc->compressed_fb.start);
 	} else {
 		compressed_llb = kzalloc(sizeof(*compressed_llb), GFP_KERNEL);
 		if (!compressed_llb)
@@ -600,23 +610,22 @@ static int intel_fbc_alloc_cfb(struct intel_crtc *crtc)
 		if (ret)
 			goto err_fb;
 
-		dev_priv->fbc.compressed_llb = compressed_llb;
+		fbc->compressed_llb = compressed_llb;
 
 		I915_WRITE(FBC_CFB_BASE,
-			   dev_priv->mm.stolen_base + dev_priv->fbc.compressed_fb.start);
+			   dev_priv->mm.stolen_base + fbc->compressed_fb.start);
 		I915_WRITE(FBC_LL_BASE,
 			   dev_priv->mm.stolen_base + compressed_llb->start);
 	}
 
 	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);
+		      fbc->compressed_fb.size, fbc->threshold);
 
 	return 0;
 
 err_fb:
 	kfree(compressed_llb);
-	i915_gem_stolen_remove_node(dev_priv, &dev_priv->fbc.compressed_fb);
+	i915_gem_stolen_remove_node(dev_priv, &fbc->compressed_fb);
 err_llb:
 	pr_info_once("drm: not enough stolen space for compressed buffer (need %d more bytes), disabling. Hint: you may be able to increase stolen memory size in the BIOS to avoid this.\n", size);
 	return -ENOSPC;
@@ -624,25 +633,27 @@ err_llb:
 
 static void __intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv)
 {
-	if (drm_mm_node_allocated(&dev_priv->fbc.compressed_fb))
-		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);
+	struct intel_fbc *fbc = &dev_priv->fbc;
+
+	if (drm_mm_node_allocated(&fbc->compressed_fb))
+		i915_gem_stolen_remove_node(dev_priv, &fbc->compressed_fb);
+
+	if (fbc->compressed_llb) {
+		i915_gem_stolen_remove_node(dev_priv, fbc->compressed_llb);
+		kfree(fbc->compressed_llb);
 	}
 }
 
 void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv)
 {
+	struct intel_fbc *fbc = &dev_priv->fbc;
+
 	if (!fbc_supported(dev_priv))
 		return;
 
-	mutex_lock(&dev_priv->fbc.lock);
+	mutex_lock(&fbc->lock);
 	__intel_fbc_cleanup_cfb(dev_priv);
-	mutex_unlock(&dev_priv->fbc.lock);
+	mutex_unlock(&fbc->lock);
 }
 
 static bool stride_is_valid(struct drm_i915_private *dev_priv,
@@ -721,6 +732,7 @@ static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc)
 static bool intel_fbc_can_activate(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct intel_fbc *fbc = &dev_priv->fbc;
 	struct drm_plane *primary;
 	struct drm_framebuffer *fb;
 	struct intel_plane_state *plane_state;
@@ -797,7 +809,7 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
 	 * 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) {
+	    fbc->compressed_fb.size * fbc->threshold) {
 		set_no_fbc_reason(dev_priv, "CFB requirements changed");
 		return false;
 	}
@@ -878,31 +890,32 @@ static bool intel_fbc_reg_params_equal(struct intel_fbc_reg_params *params1,
 static void __intel_fbc_update(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct intel_fbc *fbc = &dev_priv->fbc;
 	struct intel_fbc_reg_params old_params;
 
-	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
+	WARN_ON(!mutex_is_locked(&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.enabled || dev_priv->fbc.crtc != crtc)
+	if (!fbc->enabled || fbc->crtc != crtc)
 		return;
 
 	if (!intel_fbc_can_activate(crtc))
 		goto out_disable;
 
-	old_params = dev_priv->fbc.params;
-	intel_fbc_get_reg_params(crtc, &dev_priv->fbc.params);
+	old_params = fbc->params;
+	intel_fbc_get_reg_params(crtc, &fbc->params);
 
 	/* If the scanout has not changed, don't modify the FBC settings.
 	 * Note that we make the fundamental assumption that the fb->obj
 	 * cannot be unpinned (and have its GTT offset and fence revoked)
 	 * without first being decoupled from the scanout and FBC disabled.
 	 */
-	if (dev_priv->fbc.active &&
-	    intel_fbc_reg_params_equal(&old_params, &dev_priv->fbc.params))
+	if (fbc->active &&
+	    intel_fbc_reg_params_equal(&old_params, &fbc->params))
 		return;
 
 	if (intel_fbc_is_active(dev_priv)) {
@@ -934,7 +947,7 @@ static void __intel_fbc_update(struct intel_crtc *crtc)
 	}
 
 	intel_fbc_schedule_activation(crtc);
-	dev_priv->fbc.no_fbc_reason = "FBC enabled (not necessarily active)";
+	fbc->no_fbc_reason = "FBC enabled (not necessarily active)";
 	return;
 
 out_disable:
@@ -954,19 +967,21 @@ out_disable:
 void intel_fbc_update(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct intel_fbc *fbc = &dev_priv->fbc;
 
 	if (!fbc_supported(dev_priv))
 		return;
 
-	mutex_lock(&dev_priv->fbc.lock);
+	mutex_lock(&fbc->lock);
 	__intel_fbc_update(crtc);
-	mutex_unlock(&dev_priv->fbc.lock);
+	mutex_unlock(&fbc->lock);
 }
 
 void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 			  unsigned int frontbuffer_bits,
 			  enum fb_op_origin origin)
 {
+	struct intel_fbc *fbc = &dev_priv->fbc;
 	unsigned int fbc_bits;
 
 	if (!fbc_supported(dev_priv))
@@ -975,44 +990,46 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 	if (origin == ORIGIN_GTT)
 		return;
 
-	mutex_lock(&dev_priv->fbc.lock);
+	mutex_lock(&fbc->lock);
 
-	if (dev_priv->fbc.enabled)
-		fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe);
+	if (fbc->enabled)
+		fbc_bits = INTEL_FRONTBUFFER_PRIMARY(fbc->crtc->pipe);
 	else
-		fbc_bits = dev_priv->fbc.possible_framebuffer_bits;
+		fbc_bits = fbc->possible_framebuffer_bits;
 
-	dev_priv->fbc.busy_bits |= (fbc_bits & frontbuffer_bits);
+	fbc->busy_bits |= (fbc_bits & frontbuffer_bits);
 
-	if (dev_priv->fbc.busy_bits)
+	if (fbc->busy_bits)
 		__intel_fbc_deactivate(dev_priv);
 
-	mutex_unlock(&dev_priv->fbc.lock);
+	mutex_unlock(&fbc->lock);
 }
 
 void intel_fbc_flush(struct drm_i915_private *dev_priv,
 		     unsigned int frontbuffer_bits, enum fb_op_origin origin)
 {
+	struct intel_fbc *fbc = &dev_priv->fbc;
+
 	if (!fbc_supported(dev_priv))
 		return;
 
 	if (origin == ORIGIN_GTT)
 		return;
 
-	mutex_lock(&dev_priv->fbc.lock);
+	mutex_lock(&fbc->lock);
 
-	dev_priv->fbc.busy_bits &= ~frontbuffer_bits;
+	fbc->busy_bits &= ~frontbuffer_bits;
 
-	if (!dev_priv->fbc.busy_bits && dev_priv->fbc.enabled) {
-		if (origin != ORIGIN_FLIP && dev_priv->fbc.active) {
+	if (!fbc->busy_bits && fbc->enabled) {
+		if (origin != ORIGIN_FLIP && fbc->active) {
 			intel_fbc_recompress(dev_priv);
 		} else {
 			__intel_fbc_deactivate(dev_priv);
-			__intel_fbc_update(dev_priv->fbc.crtc);
+			__intel_fbc_update(fbc->crtc);
 		}
 	}
 
-	mutex_unlock(&dev_priv->fbc.lock);
+	mutex_unlock(&fbc->lock);
 }
 
 /**
@@ -1025,19 +1042,20 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
 void intel_fbc_enable(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct intel_fbc *fbc = &dev_priv->fbc;
 
 	if (!fbc_supported(dev_priv))
 		return;
 
-	mutex_lock(&dev_priv->fbc.lock);
+	mutex_lock(&fbc->lock);
 
-	if (dev_priv->fbc.enabled) {
-		WARN_ON(dev_priv->fbc.crtc == crtc);
+	if (fbc->enabled) {
+		WARN_ON(fbc->crtc == crtc);
 		goto out;
 	}
 
-	WARN_ON(dev_priv->fbc.active);
-	WARN_ON(dev_priv->fbc.crtc != NULL);
+	WARN_ON(fbc->active);
+	WARN_ON(fbc->crtc != NULL);
 
 	if (!intel_fbc_can_enable(crtc))
 		goto out;
@@ -1048,12 +1066,12 @@ void intel_fbc_enable(struct intel_crtc *crtc)
 	}
 
 	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";
+	fbc->no_fbc_reason = "FBC enabled but not active yet\n";
 
-	dev_priv->fbc.enabled = true;
-	dev_priv->fbc.crtc = crtc;
+	fbc->enabled = true;
+	fbc->crtc = crtc;
 out:
-	mutex_unlock(&dev_priv->fbc.lock);
+	mutex_unlock(&fbc->lock);
 }
 
 /**
@@ -1065,19 +1083,20 @@ out:
  */
 static void __intel_fbc_disable(struct drm_i915_private *dev_priv)
 {
-	struct intel_crtc *crtc = dev_priv->fbc.crtc;
+	struct intel_fbc *fbc = &dev_priv->fbc;
+	struct intel_crtc *crtc = fbc->crtc;
 
-	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
-	WARN_ON(!dev_priv->fbc.enabled);
-	WARN_ON(dev_priv->fbc.active);
+	WARN_ON(!mutex_is_locked(&fbc->lock));
+	WARN_ON(!fbc->enabled);
+	WARN_ON(fbc->active);
 	assert_pipe_disabled(dev_priv, crtc->pipe);
 
 	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;
+	fbc->enabled = false;
+	fbc->crtc = NULL;
 }
 
 /**
@@ -1089,17 +1108,18 @@ static void __intel_fbc_disable(struct drm_i915_private *dev_priv)
 void intel_fbc_disable_crtc(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct intel_fbc *fbc = &dev_priv->fbc;
 
 	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);
+	mutex_lock(&fbc->lock);
+	if (fbc->crtc == crtc) {
+		WARN_ON(!fbc->enabled);
+		WARN_ON(fbc->active);
 		__intel_fbc_disable(dev_priv);
 	}
-	mutex_unlock(&dev_priv->fbc.lock);
+	mutex_unlock(&fbc->lock);
 }
 
 /**
@@ -1110,13 +1130,15 @@ void intel_fbc_disable_crtc(struct intel_crtc *crtc)
  */
 void intel_fbc_disable(struct drm_i915_private *dev_priv)
 {
+	struct intel_fbc *fbc = &dev_priv->fbc;
+
 	if (!fbc_supported(dev_priv))
 		return;
 
-	mutex_lock(&dev_priv->fbc.lock);
-	if (dev_priv->fbc.enabled)
+	mutex_lock(&fbc->lock);
+	if (fbc->enabled)
 		__intel_fbc_disable(dev_priv);
-	mutex_unlock(&dev_priv->fbc.lock);
+	mutex_unlock(&fbc->lock);
 }
 
 /**
@@ -1127,21 +1149,22 @@ void intel_fbc_disable(struct drm_i915_private *dev_priv)
  */
 void intel_fbc_init(struct drm_i915_private *dev_priv)
 {
+	struct intel_fbc *fbc = &dev_priv->fbc;
 	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;
+	INIT_WORK(&fbc->work.work, intel_fbc_work_fn);
+	mutex_init(&fbc->lock);
+	fbc->enabled = false;
+	fbc->active = false;
+	fbc->work.scheduled = false;
 
 	if (!HAS_FBC(dev_priv)) {
-		dev_priv->fbc.no_fbc_reason = "unsupported by this chipset";
+		fbc->no_fbc_reason = "unsupported by this chipset";
 		return;
 	}
 
 	for_each_pipe(dev_priv, pipe) {
-		dev_priv->fbc.possible_framebuffer_bits |=
+		fbc->possible_framebuffer_bits |=
 				INTEL_FRONTBUFFER_PRIMARY(pipe);
 
 		if (fbc_on_pipe_a_only(dev_priv))
@@ -1149,21 +1172,21 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
 	}
 
 	if (INTEL_INFO(dev_priv)->gen >= 7) {
-		dev_priv->fbc.is_active = ilk_fbc_is_active;
-		dev_priv->fbc.activate = gen7_fbc_activate;
-		dev_priv->fbc.deactivate = ilk_fbc_deactivate;
+		fbc->is_active = ilk_fbc_is_active;
+		fbc->activate = gen7_fbc_activate;
+		fbc->deactivate = ilk_fbc_deactivate;
 	} else if (INTEL_INFO(dev_priv)->gen >= 5) {
-		dev_priv->fbc.is_active = ilk_fbc_is_active;
-		dev_priv->fbc.activate = ilk_fbc_activate;
-		dev_priv->fbc.deactivate = ilk_fbc_deactivate;
+		fbc->is_active = ilk_fbc_is_active;
+		fbc->activate = ilk_fbc_activate;
+		fbc->deactivate = ilk_fbc_deactivate;
 	} else if (IS_GM45(dev_priv)) {
-		dev_priv->fbc.is_active = g4x_fbc_is_active;
-		dev_priv->fbc.activate = g4x_fbc_activate;
-		dev_priv->fbc.deactivate = g4x_fbc_deactivate;
+		fbc->is_active = g4x_fbc_is_active;
+		fbc->activate = g4x_fbc_activate;
+		fbc->deactivate = g4x_fbc_deactivate;
 	} else {
-		dev_priv->fbc.is_active = i8xx_fbc_is_active;
-		dev_priv->fbc.activate = i8xx_fbc_activate;
-		dev_priv->fbc.deactivate = i8xx_fbc_deactivate;
+		fbc->is_active = i8xx_fbc_is_active;
+		fbc->activate = i8xx_fbc_activate;
+		fbc->deactivate = i8xx_fbc_deactivate;
 
 		/* This value was pulled out of someone's hat */
 		I915_WRITE(FBC_CONTROL, 500 << FBC_CTL_INTERVAL_SHIFT);
@@ -1172,6 +1195,6 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
 	/* We still don't have any sort of hardware state readout for FBC, so
 	 * 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);
+	if (fbc->is_active(dev_priv))
+		fbc->deactivate(dev_priv);
 }
-- 
2.6.4

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

[PATCH 06/25] drm/i915/fbc: don't use the frontbuffer tracking subsystem for flips

[Paulo Zanoni]

Before this patch, page flips would call intel_frontbuffer_flip() and
intel_frontbuffer_flip_complete(), which would call intel_fbc_flush(),
which would call intel_fbc_update(). The problem is that drawing
operations also trigger intel_fbc_flush() calls, so it's not
guaranteed that we have the CRTC and FB locks grabbed when
intel_fbc_flush() happens, since the call trace may come from the
rendering path.

We're trying to make the FBC code grab the appropriate CRTC/FB locks,
so split the drawing and the flipping logic in order to achieve that
in later patches. So now the frontbuffer tracking code is just going
to be used for frontbuffer drawing, and intel_fbc_update() is going to
be used directly for actual page flips.

As a note, we don't need to call intel_fbc_flip() during the two
places where we call intel_frontbuffer_flip() since in one of them we
already have an intel_fbc_update() call, and in the other we have the
planes disabled.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a851cb7..f026ade 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10935,6 +10935,7 @@ static void intel_unpin_work_fn(struct work_struct *__work)
 	mutex_unlock(&dev->struct_mutex);
 
 	intel_frontbuffer_flip_complete(dev, to_intel_plane(primary)->frontbuffer_bit);
+	intel_fbc_update(crtc);
 	drm_framebuffer_unreference(work->old_fb);
 
 	BUG_ON(atomic_read(&crtc->unpin_work_count) == 0);
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index b4a4191..dad4c6e 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -987,7 +987,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 	if (!fbc_supported(dev_priv))
 		return;
 
-	if (origin == ORIGIN_GTT)
+	if (origin == ORIGIN_GTT || origin == ORIGIN_FLIP)
 		return;
 
 	mutex_lock(&fbc->lock);
@@ -1013,7 +1013,7 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
 	if (!fbc_supported(dev_priv))
 		return;
 
-	if (origin == ORIGIN_GTT)
+	if (origin == ORIGIN_GTT || origin == ORIGIN_FLIP)
 		return;
 
 	mutex_lock(&fbc->lock);
@@ -1021,12 +1021,10 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
 	fbc->busy_bits &= ~frontbuffer_bits;
 
 	if (!fbc->busy_bits && fbc->enabled) {
-		if (origin != ORIGIN_FLIP && fbc->active) {
+		if (fbc->active)
 			intel_fbc_recompress(dev_priv);
-		} else {
-			__intel_fbc_deactivate(dev_priv);
+		else
 			__intel_fbc_update(fbc->crtc);
-		}
 	}
 
 	mutex_unlock(&fbc->lock);
-- 
2.6.4

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

[PATCH 07/25] drm/i915/fbc: don't flush for operations on the wrong frontbuffer

[Paulo Zanoni]

If frontbuffer_bits doesn't match the current frontbuffer, there's no
reason to recompress or update FBC.

There was a plan to make the FBC test suite catch this type of
problem, but it never got implemented due to being low priority.

While at it, also implement Ville's suggestion and use
plane->frontbuffer_bit instead of INTEL_FRONTBUFFER_PRIMARY.

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

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index dad4c6e..2f25a45 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -977,12 +977,19 @@ void intel_fbc_update(struct intel_crtc *crtc)
 	mutex_unlock(&fbc->lock);
 }
 
+static unsigned int intel_fbc_get_frontbuffer_bit(struct intel_fbc *fbc)
+{
+	if (fbc->enabled)
+		return to_intel_plane(fbc->crtc->base.primary)->frontbuffer_bit;
+	else
+		return fbc->possible_framebuffer_bits;
+}
+
 void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 			  unsigned int frontbuffer_bits,
 			  enum fb_op_origin origin)
 {
 	struct intel_fbc *fbc = &dev_priv->fbc;
-	unsigned int fbc_bits;
 
 	if (!fbc_supported(dev_priv))
 		return;
@@ -992,12 +999,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 
 	mutex_lock(&fbc->lock);
 
-	if (fbc->enabled)
-		fbc_bits = INTEL_FRONTBUFFER_PRIMARY(fbc->crtc->pipe);
-	else
-		fbc_bits = fbc->possible_framebuffer_bits;
-
-	fbc->busy_bits |= (fbc_bits & frontbuffer_bits);
+	fbc->busy_bits |= intel_fbc_get_frontbuffer_bit(fbc) & frontbuffer_bits;
 
 	if (fbc->busy_bits)
 		__intel_fbc_deactivate(dev_priv);
@@ -1020,7 +1022,8 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
 
 	fbc->busy_bits &= ~frontbuffer_bits;
 
-	if (!fbc->busy_bits && fbc->enabled) {
+	if (!fbc->busy_bits && fbc->enabled &&
+	    (frontbuffer_bits & intel_fbc_get_frontbuffer_bit(fbc))) {
 		if (fbc->active)
 			intel_fbc_recompress(dev_priv);
 		else
-- 
2.6.4

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

[PATCH 08/25] drm/i915/fbc: unconditionally update FBC during atomic commits

[Paulo Zanoni]

We unconditionally disable/update FBC even during the page flip
IOCTLs, and an unconditional disable/update at every atomic commit
touching the primary plane shouldn't impact PC state residency
noticeably. Besides, the code that checks for rotation is a good hint
that we may be forgetting something else, so let's leave all the
decisions to intel_fbc.c, making the code much safer.

Once we have the code to properly make FBC enable/update decisions
based on atomic states, with proper locking, then we'll be able to
evaluate whether it will be worth trying to optimize the cases where a
disable isn't needed.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f026ade..baab41046 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11928,6 +11928,8 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	case DRM_PLANE_TYPE_PRIMARY:
 		intel_crtc->atomic.pre_disable_primary = turn_off;
 		intel_crtc->atomic.post_enable_primary = turn_on;
+		intel_crtc->atomic.disable_fbc = true;
+		intel_crtc->atomic.update_fbc = true;
 
 		if (turn_off) {
 			/*
@@ -11939,28 +11941,9 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 			 * disable.
 			 */
 			intel_crtc->atomic.disable_ips = true;
-
-			intel_crtc->atomic.disable_fbc = true;
 		}
 
 		/*
-		 * FBC does not work on some platforms for rotated
-		 * planes, so disable it when rotation is not 0 and
-		 * update it when rotation is set back to 0.
-		 *
-		 * FIXME: This is redundant with the fbc update done in
-		 * the primary plane enable function except that that
-		 * one is done too late. We eventually need to unify
-		 * this.
-		 */
-
-		if (visible &&
-		    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
-		    dev_priv->fbc.crtc == intel_crtc &&
-		    plane_state->rotation != BIT(DRM_ROTATE_0))
-			intel_crtc->atomic.disable_fbc = true;
-
-		/*
 		 * BDW signals flip done immediately if the plane
 		 * is disabled, even if the plane enable is already
 		 * armed to occur at the next vblank :(
@@ -11968,7 +11951,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 		if (turn_on && IS_BROADWELL(dev))
 			intel_crtc->atomic.wait_vblank = true;
 
-		intel_crtc->atomic.update_fbc |= visible || mode_changed;
 		break;
 	case DRM_PLANE_TYPE_CURSOR:
 		break;
-- 
2.6.4

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

[PATCH 09/25] drm/i915/fbc: introduce struct intel_fbc_state_cache

[Paulo Zanoni]

Per the new atomic locking rules, we need to cache the CRTC, plane and
FB state structures we use so we can access them later without needing
more locks. So do this.

Notice that there are some pieces of the FBC code that look at things
that are only computed during the modeset, so we can't just can't
precompute whether FBC can be activated during the update_state_cache
stage. We may be able to do this later.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6d11575..02e6869 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -921,6 +921,29 @@ struct intel_fbc {
 	bool enabled;
 	bool active;
 
+	struct intel_fbc_state_cache {
+		struct {
+			unsigned int mode_flags;
+			uint32_t hsw_bdw_pixel_rate;
+		} crtc;
+
+		struct {
+			unsigned int rotation;
+			int src_w;
+			int src_h;
+			bool visible;
+		} plane;
+
+		struct {
+			u64 ilk_ggtt_offset;
+			uint32_t id;
+			uint32_t pixel_format;
+			unsigned int stride;
+			int fence_reg;
+			unsigned int tiling_mode;
+		} fb;
+	} state_cache;
+
 	struct intel_fbc_reg_params {
 		struct {
 			enum pipe pipe;
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 2f25a45..7396287 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -74,19 +74,17 @@ static unsigned int get_crtc_fence_y_offset(struct intel_crtc *crtc)
  * 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,
+static void intel_fbc_get_plane_source_size(struct intel_fbc_state_cache *cache,
 					    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;
+	if (intel_rotation_90_or_270(cache->plane.rotation)) {
+		w = cache->plane.src_h;
+		h = cache->plane.src_w;
 	} else {
-		w = drm_rect_width(&plane_state->src) >> 16;
-		h = drm_rect_height(&plane_state->src) >> 16;
+		w = cache->plane.src_w;
+		h = cache->plane.src_h;
 	}
 
 	if (width)
@@ -95,18 +93,17 @@ static void intel_fbc_get_plane_source_size(struct intel_crtc *crtc,
 		*height = h;
 }
 
-static int intel_fbc_calculate_cfb_size(struct intel_crtc *crtc,
-					struct drm_framebuffer *fb)
+static int intel_fbc_calculate_cfb_size(struct drm_i915_private *dev_priv,
+					struct intel_fbc_state_cache *cache)
 {
-	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 	int lines;
 
-	intel_fbc_get_plane_source_size(crtc, NULL, &lines);
+	intel_fbc_get_plane_source_size(cache, 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];
+	return lines * cache->fb.stride;
 }
 
 static void i8xx_fbc_deactivate(struct drm_i915_private *dev_priv)
@@ -576,14 +573,13 @@ static int intel_fbc_alloc_cfb(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 	struct intel_fbc *fbc = &dev_priv->fbc;
-	struct drm_framebuffer *fb = crtc->base.primary->state->fb;
 	struct drm_mm_node *uninitialized_var(compressed_llb);
 	int size, fb_cpp, ret;
 
 	WARN_ON(drm_mm_node_allocated(&fbc->compressed_fb));
 
-	size = intel_fbc_calculate_cfb_size(crtc, fb);
-	fb_cpp = drm_format_plane_cpp(fb->pixel_format, 0);
+	size = intel_fbc_calculate_cfb_size(dev_priv, &fbc->state_cache);
+	fb_cpp = drm_format_plane_cpp(fbc->state_cache.fb.pixel_format, 0);
 
 	ret = find_compression_threshold(dev_priv, &fbc->compressed_fb,
 					 size, fb_cpp);
@@ -677,19 +673,17 @@ static bool stride_is_valid(struct drm_i915_private *dev_priv,
 	return true;
 }
 
-static bool pixel_format_is_valid(struct drm_framebuffer *fb)
+static bool pixel_format_is_valid(struct drm_i915_private *dev_priv,
+				  uint32_t pixel_format)
 {
-	struct drm_device *dev = fb->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	switch (fb->pixel_format) {
+	switch (pixel_format) {
 	case DRM_FORMAT_XRGB8888:
 	case DRM_FORMAT_XBGR8888:
 		return true;
 	case DRM_FORMAT_XRGB1555:
 	case DRM_FORMAT_RGB565:
 		/* 16bpp not supported on gen2 */
-		if (IS_GEN2(dev))
+		if (IS_GEN2(dev_priv))
 			return false;
 		/* WaFbcOnly1to1Ratio:ctg */
 		if (IS_G4X(dev_priv))
@@ -709,6 +703,7 @@ static bool pixel_format_is_valid(struct drm_framebuffer *fb)
 static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct intel_fbc *fbc = &dev_priv->fbc;
 	unsigned int effective_w, effective_h, max_w, max_h;
 
 	if (INTEL_INFO(dev_priv)->gen >= 8 || IS_HASWELL(dev_priv)) {
@@ -722,41 +717,64 @@ static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc)
 		max_h = 1536;
 	}
 
-	intel_fbc_get_plane_source_size(crtc, &effective_w, &effective_h);
+	intel_fbc_get_plane_source_size(&fbc->state_cache, &effective_w,
+					&effective_h);
 	effective_w += crtc->adjusted_x;
 	effective_h += crtc->adjusted_y;
 
 	return effective_w <= max_w && effective_h <= max_h;
 }
 
-static bool intel_fbc_can_activate(struct intel_crtc *crtc)
+static void intel_fbc_update_state_cache(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 	struct intel_fbc *fbc = &dev_priv->fbc;
-	struct drm_plane *primary;
-	struct drm_framebuffer *fb;
-	struct intel_plane_state *plane_state;
+	struct intel_fbc_state_cache *cache = &fbc->state_cache;
+	struct intel_crtc_state *crtc_state = crtc->config;
+	struct intel_plane_state *plane_state =
+		to_intel_plane_state(crtc->base.primary->state);
+	struct drm_framebuffer *fb = plane_state->base.fb;
 	struct drm_i915_gem_object *obj;
-	const struct drm_display_mode *adjusted_mode;
 
-	if (!intel_crtc_active(&crtc->base)) {
-		set_no_fbc_reason(dev_priv, "CRTC not active");
-		return false;
-	}
+	cache->crtc.mode_flags = crtc_state->base.adjusted_mode.flags;
+	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
+		cache->crtc.hsw_bdw_pixel_rate =
+			ilk_pipe_pixel_rate(crtc_state);
+
+	cache->plane.rotation = plane_state->base.rotation;
+	cache->plane.src_w = drm_rect_width(&plane_state->src) >> 16;
+	cache->plane.src_h = drm_rect_height(&plane_state->src) >> 16;
+	cache->plane.visible = plane_state->visible;
+
+	if (!cache->plane.visible)
+		return;
 
-	primary = crtc->base.primary;
-	fb = primary->fb;
 	obj = intel_fb_obj(fb);
-	adjusted_mode = &crtc->config->base.adjusted_mode;
-	plane_state = to_intel_plane_state(primary->state);
 
-	if (!plane_state->visible) {
+	/* FIXME: We lack the proper locking here, so only run this on the
+	 * platforms that need. */
+	if (dev_priv->fbc.activate == ilk_fbc_activate)
+		cache->fb.ilk_ggtt_offset = i915_gem_obj_ggtt_offset(obj);
+	cache->fb.id = fb->base.id;
+	cache->fb.pixel_format = fb->pixel_format;
+	cache->fb.stride = fb->pitches[0];
+	cache->fb.fence_reg = obj->fence_reg;
+	cache->fb.tiling_mode = obj->tiling_mode;
+}
+
+static bool intel_fbc_can_activate(struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct intel_fbc *fbc = &dev_priv->fbc;
+	struct intel_fbc_state_cache *cache = &fbc->state_cache;
+
+	if (!cache->plane.visible) {
 		set_no_fbc_reason(dev_priv, "primary plane not visible");
 		return false;
 	}
 
-	if ((adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) ||
-	    (adjusted_mode->flags & DRM_MODE_FLAG_DBLSCAN)) {
+	if ((cache->crtc.mode_flags & DRM_MODE_FLAG_INTERLACE) ||
+	    (cache->crtc.mode_flags & DRM_MODE_FLAG_DBLSCAN)) {
 		set_no_fbc_reason(dev_priv, "incompatible mode");
 		return false;
 	}
@@ -769,31 +787,30 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
 	/* 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.
 	 */
-	if (obj->tiling_mode != I915_TILING_X ||
-	    obj->fence_reg == I915_FENCE_REG_NONE) {
+	if (cache->fb.tiling_mode != I915_TILING_X ||
+	    cache->fb.fence_reg == I915_FENCE_REG_NONE) {
 		set_no_fbc_reason(dev_priv, "framebuffer not tiled or fenced");
 		return false;
 	}
 	if (INTEL_INFO(dev_priv)->gen <= 4 && !IS_G4X(dev_priv) &&
-	    plane_state->base.rotation != BIT(DRM_ROTATE_0)) {
+	    cache->plane.rotation != BIT(DRM_ROTATE_0)) {
 		set_no_fbc_reason(dev_priv, "rotation unsupported");
 		return false;
 	}
 
-	if (!stride_is_valid(dev_priv, fb->pitches[0])) {
+	if (!stride_is_valid(dev_priv, cache->fb.stride)) {
 		set_no_fbc_reason(dev_priv, "framebuffer stride not supported");
 		return false;
 	}
 
-	if (!pixel_format_is_valid(fb)) {
+	if (!pixel_format_is_valid(dev_priv, cache->fb.pixel_format)) {
 		set_no_fbc_reason(dev_priv, "pixel format is invalid");
 		return false;
 	}
 
 	/* WaFbcExceedCdClockThreshold:hsw,bdw */
 	if ((IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) &&
-	    ilk_pipe_pixel_rate(crtc->config) >=
-	    dev_priv->cdclk_freq * 95 / 100) {
+	    cache->crtc.hsw_bdw_pixel_rate >= dev_priv->cdclk_freq * 95 / 100) {
 		set_no_fbc_reason(dev_priv, "pixel rate is too big");
 		return false;
 	}
@@ -808,7 +825,7 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
 	 * 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) >
+	if (intel_fbc_calculate_cfb_size(dev_priv, &fbc->state_cache) >
 	    fbc->compressed_fb.size * fbc->threshold) {
 		set_no_fbc_reason(dev_priv, "CFB requirements changed");
 		return false;
@@ -848,8 +865,8 @@ static void intel_fbc_get_reg_params(struct intel_crtc *crtc,
 				     struct intel_fbc_reg_params *params)
 {
 	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);
+	struct intel_fbc *fbc = &dev_priv->fbc;
+	struct intel_fbc_state_cache *cache = &fbc->state_cache;
 
 	/* Since all our fields are integer types, use memset here so the
 	 * comparison function can rely on memcmp because the padding will be
@@ -860,17 +877,14 @@ static void intel_fbc_get_reg_params(struct intel_crtc *crtc,
 	params->crtc.plane = crtc->plane;
 	params->crtc.fence_y_offset = get_crtc_fence_y_offset(crtc);
 
-	params->fb.id = fb->base.id;
-	params->fb.pixel_format = fb->pixel_format;
-	params->fb.stride = fb->pitches[0];
-	params->fb.fence_reg = obj->fence_reg;
+	params->fb.id = cache->fb.id;
+	params->fb.pixel_format = cache->fb.pixel_format;
+	params->fb.stride = cache->fb.stride;
+	params->fb.fence_reg = cache->fb.fence_reg;
 
-	params->cfb_size = intel_fbc_calculate_cfb_size(crtc, fb);
+	params->cfb_size = intel_fbc_calculate_cfb_size(dev_priv, cache);
 
-	/* FIXME: We lack the proper locking here, so only run this on the
-	 * platforms that need. */
-	if (dev_priv->fbc.activate == ilk_fbc_activate)
-		params->fb.ggtt_offset = i915_gem_obj_ggtt_offset(obj);
+	params->fb.ggtt_offset = cache->fb.ilk_ggtt_offset;
 }
 
 static bool intel_fbc_reg_params_equal(struct intel_fbc_reg_params *params1,
@@ -903,6 +917,8 @@ static void __intel_fbc_update(struct intel_crtc *crtc)
 	if (!fbc->enabled || fbc->crtc != crtc)
 		return;
 
+	intel_fbc_update_state_cache(crtc);
+
 	if (!intel_fbc_can_activate(crtc))
 		goto out_disable;
 
@@ -1061,6 +1077,7 @@ void intel_fbc_enable(struct intel_crtc *crtc)
 	if (!intel_fbc_can_enable(crtc))
 		goto out;
 
+	intel_fbc_update_state_cache(crtc);
 	if (intel_fbc_alloc_cfb(crtc)) {
 		set_no_fbc_reason(dev_priv, "not enough stolen memory");
 		goto out;
-- 
2.6.4

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

[PATCH 10/25] drm/i915/fbc: split intel_fbc_update into pre and post update

[Paulo Zanoni]

So now pre_update will be responsible for unconditionally deactivating
FBC and updating the state cache, while post_update will be
responsible for checking if it can be enabled, then enabling it.

This is one more step into proper locking.

Notice that intel_fbc_flush now calls post_update directly. The FBC
flush can only happen for drawing operations - since we explicitly
ignore the flips -, so the FBC state is not expected to have changed
at this point. With this we can just run post_update, which will make
sure we won't deactivate+reactivate FBC as would be the case now if we
called pre_update + post_update.

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

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 7396287..2983bcd 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -894,24 +894,16 @@ static bool intel_fbc_reg_params_equal(struct intel_fbc_reg_params *params1,
 	return memcmp(params1, params2, sizeof(*params1)) == 0;
 }
 
-/**
- * __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 activates,
- * deactivates or maintains it on the same state.
- */
-static void __intel_fbc_update(struct intel_crtc *crtc)
+static void intel_fbc_pre_update(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 	struct intel_fbc *fbc = &dev_priv->fbc;
-	struct intel_fbc_reg_params old_params;
 
 	WARN_ON(!mutex_is_locked(&fbc->lock));
 
 	if (!multiple_pipes_ok(dev_priv)) {
 		set_no_fbc_reason(dev_priv, "more than one pipe active");
-		goto out_disable;
+		goto deactivate;
 	}
 
 	if (!fbc->enabled || fbc->crtc != crtc)
@@ -919,8 +911,25 @@ static void __intel_fbc_update(struct intel_crtc *crtc)
 
 	intel_fbc_update_state_cache(crtc);
 
-	if (!intel_fbc_can_activate(crtc))
-		goto out_disable;
+deactivate:
+	__intel_fbc_deactivate(dev_priv);
+}
+
+static void intel_fbc_post_update(struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct intel_fbc *fbc = &dev_priv->fbc;
+	struct intel_fbc_reg_params old_params;
+
+	WARN_ON(!mutex_is_locked(&fbc->lock));
+
+	if (!fbc->enabled || fbc->crtc != crtc)
+		return;
+
+	if (!intel_fbc_can_activate(crtc)) {
+		WARN_ON(fbc->active);
+		return;
+	}
 
 	old_params = fbc->params;
 	intel_fbc_get_reg_params(crtc, &fbc->params);
@@ -934,44 +943,9 @@ static void __intel_fbc_update(struct intel_crtc *crtc)
 	    intel_fbc_reg_params_equal(&old_params, &fbc->params))
 		return;
 
-	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
-		 * we disable the FBC at the start of the page-flip
-		 * sequence, but also more than one vblank has passed.
-		 *
-		 * For the former case of modeswitching, it is possible
-		 * to switch between two FBC valid configurations
-		 * instantaneously so we do need to disable the FBC
-		 * before we can modify its control registers. We also
-		 * have to wait for the next vblank for that to take
-		 * effect. However, since we delay enabling FBC we can
-		 * assume that a vblank has passed since disabling and
-		 * that we can safely alter the registers in the deferred
-		 * callback.
-		 *
-		 * In the scenario that we go from a valid to invalid
-		 * and then back to valid FBC configuration we have
-		 * no strict enforcement that a vblank occurred since
-		 * disabling the FBC. However, along all current pipe
-		 * disabling paths we do need to wait for a vblank at
-		 * some point. And we wait before enabling FBC anyway.
-		 */
-		DRM_DEBUG_KMS("deactivating FBC for update\n");
-		__intel_fbc_deactivate(dev_priv);
-	}
-
+	__intel_fbc_deactivate(dev_priv);
 	intel_fbc_schedule_activation(crtc);
-	fbc->no_fbc_reason = "FBC enabled (not necessarily active)";
-	return;
-
-out_disable:
-	/* Multiple disables should be harmless */
-	if (intel_fbc_is_active(dev_priv)) {
-		DRM_DEBUG_KMS("unsupported config, deactivating FBC\n");
-		__intel_fbc_deactivate(dev_priv);
-	}
+	fbc->no_fbc_reason = "FBC enabled (active or scheduled)";
 }
 
 /*
@@ -989,7 +963,8 @@ void intel_fbc_update(struct intel_crtc *crtc)
 		return;
 
 	mutex_lock(&fbc->lock);
-	__intel_fbc_update(crtc);
+	intel_fbc_pre_update(crtc);
+	intel_fbc_post_update(crtc);
 	mutex_unlock(&fbc->lock);
 }
 
@@ -1043,7 +1018,7 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
 		if (fbc->active)
 			intel_fbc_recompress(dev_priv);
 		else
-			__intel_fbc_update(fbc->crtc);
+			intel_fbc_post_update(fbc->crtc);
 	}
 
 	mutex_unlock(&fbc->lock);
-- 
2.6.4

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

[PATCH 11/25] drm/i915/fbc: fix the FBC state checking code

[Paulo Zanoni]

We'll now call intel_fbc_pre_update instead of intel_fbc_deactivate
during atomic commits. This will continue to guarantee that we
deactivate FBC and it will also update the state checking structures
at the correct time. Then, later, at the point where we were calling
intel_fbc_update, we'll only need to call intel_fbc_post_update.

Also add the proper warnings in case we don't have the appropriate
locks. Daniel mentioned the warnings will have to be removed for async
commits, but let's keep them here while we can.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index baab41046..baa4cc9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4803,7 +4803,7 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
 		intel_update_watermarks(&crtc->base);
 
 	if (atomic->update_fbc)
-		intel_fbc_update(crtc);
+		intel_fbc_post_update(crtc);
 
 	if (atomic->post_enable_primary)
 		intel_post_enable_primary(&crtc->base);
@@ -4819,8 +4819,8 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
 	struct intel_crtc_state *pipe_config =
 		to_intel_crtc_state(crtc->base.state);
 
-	if (atomic->disable_fbc)
-		intel_fbc_deactivate(crtc);
+	if (atomic->update_fbc)
+		intel_fbc_pre_update(crtc);
 
 	if (crtc->atomic.disable_ips)
 		hsw_disable_ips(crtc);
@@ -10935,7 +10935,7 @@ static void intel_unpin_work_fn(struct work_struct *__work)
 	mutex_unlock(&dev->struct_mutex);
 
 	intel_frontbuffer_flip_complete(dev, to_intel_plane(primary)->frontbuffer_bit);
-	intel_fbc_update(crtc);
+	intel_fbc_post_update(crtc);
 	drm_framebuffer_unreference(work->old_fb);
 
 	BUG_ON(atomic_read(&crtc->unpin_work_count) == 0);
@@ -11733,7 +11733,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 			  to_intel_plane(primary)->frontbuffer_bit);
 	mutex_unlock(&dev->struct_mutex);
 
-	intel_fbc_deactivate(intel_crtc);
+	intel_fbc_pre_update(intel_crtc);
 	intel_frontbuffer_flip_prepare(dev,
 				       to_intel_plane(primary)->frontbuffer_bit);
 
@@ -11928,7 +11928,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	case DRM_PLANE_TYPE_PRIMARY:
 		intel_crtc->atomic.pre_disable_primary = turn_off;
 		intel_crtc->atomic.post_enable_primary = turn_on;
-		intel_crtc->atomic.disable_fbc = true;
 		intel_crtc->atomic.update_fbc = true;
 
 		if (turn_off) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 059b46e..986644d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -564,16 +564,17 @@ struct intel_mmio_flip {
  */
 struct intel_crtc_atomic_commit {
 	/* Sleepable operations to perform before commit */
-	bool disable_fbc;
 	bool disable_ips;
 	bool pre_disable_primary;
 
 	/* Sleepable operations to perform after commit */
 	unsigned fb_bits;
 	bool wait_vblank;
-	bool update_fbc;
 	bool post_enable_primary;
 	unsigned update_sprite_watermarks;
+
+	/* Sleepable operations to perform before and after commit */
+	bool update_fbc;
 };
 
 struct intel_crtc {
@@ -1353,7 +1354,8 @@ 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_pre_update(struct intel_crtc *crtc);
+void intel_fbc_post_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);
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 2983bcd..4f2133e 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -508,6 +508,7 @@ static bool multiple_pipes_ok(struct drm_i915_private *dev_priv)
 	if (INTEL_INFO(dev_priv)->gen > 4)
 		return true;
 
+	/* FIXME: we don't have the appropriate state locks to do this here. */
 	for_each_pipe(dev_priv, pipe) {
 		crtc = dev_priv->pipe_to_crtc_mapping[pipe];
 
@@ -730,12 +731,16 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc)
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 	struct intel_fbc *fbc = &dev_priv->fbc;
 	struct intel_fbc_state_cache *cache = &fbc->state_cache;
-	struct intel_crtc_state *crtc_state = crtc->config;
+	struct intel_crtc_state *crtc_state =
+		to_intel_crtc_state(crtc->base.state);
 	struct intel_plane_state *plane_state =
 		to_intel_plane_state(crtc->base.primary->state);
 	struct drm_framebuffer *fb = plane_state->base.fb;
 	struct drm_i915_gem_object *obj;
 
+	WARN_ON(!drm_modeset_is_locked(&crtc->base.mutex));
+	WARN_ON(!drm_modeset_is_locked(&crtc->base.primary->mutex));
+
 	cache->crtc.mode_flags = crtc_state->base.adjusted_mode.flags;
 	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
 		cache->crtc.hsw_bdw_pixel_rate =
@@ -894,12 +899,15 @@ static bool intel_fbc_reg_params_equal(struct intel_fbc_reg_params *params1,
 	return memcmp(params1, params2, sizeof(*params1)) == 0;
 }
 
-static void intel_fbc_pre_update(struct intel_crtc *crtc)
+void intel_fbc_pre_update(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 	struct intel_fbc *fbc = &dev_priv->fbc;
 
-	WARN_ON(!mutex_is_locked(&fbc->lock));
+	if (!fbc_supported(dev_priv))
+		return;
+
+	mutex_lock(&fbc->lock);
 
 	if (!multiple_pipes_ok(dev_priv)) {
 		set_no_fbc_reason(dev_priv, "more than one pipe active");
@@ -907,15 +915,17 @@ static void intel_fbc_pre_update(struct intel_crtc *crtc)
 	}
 
 	if (!fbc->enabled || fbc->crtc != crtc)
-		return;
+		goto unlock;
 
 	intel_fbc_update_state_cache(crtc);
 
 deactivate:
 	__intel_fbc_deactivate(dev_priv);
+unlock:
+	mutex_unlock(&fbc->lock);
 }
 
-static void intel_fbc_post_update(struct intel_crtc *crtc)
+static void __intel_fbc_post_update(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 	struct intel_fbc *fbc = &dev_priv->fbc;
@@ -948,13 +958,7 @@ static void intel_fbc_post_update(struct intel_crtc *crtc)
 	fbc->no_fbc_reason = "FBC enabled (active or scheduled)";
 }
 
-/*
- * intel_fbc_update - activate/deactivate FBC as needed
- * @crtc: the CRTC that triggered the update
- *
- * This function reevaluates the overall state and activates or deactivates FBC.
- */
-void intel_fbc_update(struct intel_crtc *crtc)
+void intel_fbc_post_update(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 	struct intel_fbc *fbc = &dev_priv->fbc;
@@ -963,8 +967,7 @@ void intel_fbc_update(struct intel_crtc *crtc)
 		return;
 
 	mutex_lock(&fbc->lock);
-	intel_fbc_pre_update(crtc);
-	intel_fbc_post_update(crtc);
+	__intel_fbc_post_update(crtc);
 	mutex_unlock(&fbc->lock);
 }
 
@@ -1018,7 +1021,7 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
 		if (fbc->active)
 			intel_fbc_recompress(dev_priv);
 		else
-			intel_fbc_post_update(fbc->crtc);
+			__intel_fbc_post_update(fbc->crtc);
 	}
 
 	mutex_unlock(&fbc->lock);
-- 
2.6.4

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

[PATCH 12/25] drm/i915/fbc: unexport intel_fbc_deactivate

[Paulo Zanoni]

With the addition and usage of intel_fbc_pre_update,
intel_fbc_deactivate is not used anymore outside intel_fbc.c, so kill
the exported function and rename __intel_fbc_deactivate.

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

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 986644d..a53784e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1353,7 +1353,6 @@ 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_pre_update(struct intel_crtc *crtc);
 void intel_fbc_post_update(struct intel_crtc *crtc);
 void intel_fbc_init(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 4f2133e..3b002d2 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -442,7 +442,7 @@ static void intel_fbc_schedule_activation(struct intel_crtc *crtc)
 	schedule_work(&work->work);
 }
 
-static void __intel_fbc_deactivate(struct drm_i915_private *dev_priv)
+static void intel_fbc_deactivate(struct drm_i915_private *dev_priv)
 {
 	struct intel_fbc *fbc = &dev_priv->fbc;
 
@@ -454,26 +454,6 @@ static void __intel_fbc_deactivate(struct drm_i915_private *dev_priv)
 		fbc->deactivate(dev_priv);
 }
 
-/*
- * intel_fbc_deactivate - deactivate FBC if it's associated with crtc
- * @crtc: the CRTC
- *
- * This function deactivates FBC if it's associated with the provided CRTC.
- */
-void intel_fbc_deactivate(struct intel_crtc *crtc)
-{
-	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
-	struct intel_fbc *fbc = &dev_priv->fbc;
-
-	if (!fbc_supported(dev_priv))
-		return;
-
-	mutex_lock(&fbc->lock);
-	if (fbc->crtc == crtc)
-		__intel_fbc_deactivate(dev_priv);
-	mutex_unlock(&fbc->lock);
-}
-
 static void set_no_fbc_reason(struct drm_i915_private *dev_priv,
 			      const char *reason)
 {
@@ -920,7 +900,7 @@ void intel_fbc_pre_update(struct intel_crtc *crtc)
 	intel_fbc_update_state_cache(crtc);
 
 deactivate:
-	__intel_fbc_deactivate(dev_priv);
+	intel_fbc_deactivate(dev_priv);
 unlock:
 	mutex_unlock(&fbc->lock);
 }
@@ -953,7 +933,7 @@ static void __intel_fbc_post_update(struct intel_crtc *crtc)
 	    intel_fbc_reg_params_equal(&old_params, &fbc->params))
 		return;
 
-	__intel_fbc_deactivate(dev_priv);
+	intel_fbc_deactivate(dev_priv);
 	intel_fbc_schedule_activation(crtc);
 	fbc->no_fbc_reason = "FBC enabled (active or scheduled)";
 }
@@ -996,7 +976,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 	fbc->busy_bits |= intel_fbc_get_frontbuffer_bit(fbc) & frontbuffer_bits;
 
 	if (fbc->busy_bits)
-		__intel_fbc_deactivate(dev_priv);
+		intel_fbc_deactivate(dev_priv);
 
 	mutex_unlock(&fbc->lock);
 }
-- 
2.6.4

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

[PATCH 13/25] drm/i915/fbc: rename the FBC disable functions

[Paulo Zanoni]

Instead of:
 - intel_fbc_disable_crtc(crtc)
 - intel_fbc_disable(dev_priv)
we now have:
 - intel_fbc_disable(crtc)
 - intel_fbc_global_disable(dev_priv)

This is because all the other functions that take a CRTC are called
 - intel_fbc_something(crtc)
Instead of:
 - intel_fbc_something_crtc(crtc)

And I also hope that the word "global" is going to help make it more
explicit that "global" is the unusual case, not the opposite.

Reported-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_suspend.c  | 2 +-
 drivers/gpu/drm/i915/intel_display.c | 8 ++++----
 drivers/gpu/drm/i915/intel_drv.h     | 4 ++--
 drivers/gpu/drm/i915/intel_fbc.c     | 8 ++++----
 4 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index a2aa09c..6c6bedf 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -92,7 +92,7 @@ static void i915_restore_display(struct drm_device *dev)
 	}
 
 	/* only restore FBC info on the platform that supports FBC*/
-	intel_fbc_disable(dev_priv);
+	intel_fbc_global_disable(dev_priv);
 
 	/* restore FBC interval */
 	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev))
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index baa4cc9..b7c0707 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5163,7 +5163,7 @@ 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);
+	intel_fbc_disable(intel_crtc);
 }
 
 static void haswell_crtc_disable(struct drm_crtc *crtc)
@@ -5215,7 +5215,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 						      true);
 	}
 
-	intel_fbc_disable_crtc(intel_crtc);
+	intel_fbc_disable(intel_crtc);
 }
 
 static void i9xx_pfit_enable(struct intel_crtc *crtc)
@@ -6391,7 +6391,7 @@ 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);
+	intel_fbc_disable(intel_crtc);
 }
 
 static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
@@ -16110,7 +16110,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
 
 	intel_unregister_dsm_handler();
 
-	intel_fbc_disable(dev_priv);
+	intel_fbc_global_disable(dev_priv);
 
 	/* flush any delayed tasks or pending work */
 	flush_scheduled_work();
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a53784e..f227bf1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1357,8 +1357,8 @@ void intel_fbc_pre_update(struct intel_crtc *crtc);
 void intel_fbc_post_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_disable(struct intel_crtc *crtc);
+void intel_fbc_global_disable(struct drm_i915_private *dev_priv);
 void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 			  unsigned int frontbuffer_bits,
 			  enum fb_op_origin origin);
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 3b002d2..9995cfc 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -1076,12 +1076,12 @@ static void __intel_fbc_disable(struct drm_i915_private *dev_priv)
 }
 
 /**
- * intel_fbc_disable_crtc - disable FBC if it's associated with crtc
+ * intel_fbc_disable - 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)
+void intel_fbc_disable(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 	struct intel_fbc *fbc = &dev_priv->fbc;
@@ -1099,12 +1099,12 @@ void intel_fbc_disable_crtc(struct intel_crtc *crtc)
 }
 
 /**
- * intel_fbc_disable - globally disable FBC
+ * intel_fbc_global_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)
+void intel_fbc_global_disable(struct drm_i915_private *dev_priv)
 {
 	struct intel_fbc *fbc = &dev_priv->fbc;
 
-- 
2.6.4

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

[PATCH 14/25] drm/i915/fbc: make sure we cancel the work function at fbc_disable

[Paulo Zanoni]

Just to be sure nothing will survive a module unload. We need to do
this after the unlock in order to make sure the function won't get
stuck trying to grab the lock we already own while we wait for it to
finish.

Reported-by: Reported-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 9995cfc..1977176 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -1096,6 +1096,8 @@ void intel_fbc_disable(struct intel_crtc *crtc)
 		__intel_fbc_disable(dev_priv);
 	}
 	mutex_unlock(&fbc->lock);
+
+	cancel_work_sync(&fbc->work.work);
 }
 
 /**
@@ -1115,6 +1117,8 @@ void intel_fbc_global_disable(struct drm_i915_private *dev_priv)
 	if (fbc->enabled)
 		__intel_fbc_disable(dev_priv);
 	mutex_unlock(&fbc->lock);
+
+	cancel_work_sync(&fbc->work.work);
 }
 
 /**
-- 
2.6.4

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

[PATCH 15/25] drm/i915/fbc: rewrite the multiple_pipes_ok() code for locking

[Paulo Zanoni]

Older FBC platforms have this restriction where FBC can't be enabled
if multiple pipes are enabled. In the current code, we disable FBC
before the second pipe becomes visible.

One of the problems with this code is that the current
multiple_pipes_ok() implementation just iterates through all CRTCs
looking at their states, but it doesn't make sure that the state
locks are grabbed. It also can't just grab the locks for every CRTC
since this would kill one of the biggest advantages of atomic
modesetting.

After the recent FBC changes, we now have the appropriate locks for
the given CRTC, so we can just try to maintain the state of each CRTC
and update it once intel_fbc_pre_update is called.

As a last note, I don't have gen 2/3 machines to test this code. My
current plan is to enable FBC on just the newer platforms, so this
patch is just an attempt to get the gen 2/3 code at least looking
sane, so if one day someone decide to fix FBC on these platforms, they
may have less work to do.

Not-tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com> (only on HSW+)
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 |  2 ++
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_fbc.c     | 55 +++++++++++++++++++++++++++---------
 4 files changed, 45 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 02e6869..65e5771 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -911,6 +911,7 @@ struct intel_fbc {
 	unsigned threshold;
 	unsigned int possible_framebuffer_bits;
 	unsigned int busy_bits;
+	unsigned int visible_pipes_mask;
 	struct intel_crtc *crtc;
 
 	struct drm_mm_node compressed_fb;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b7c0707..bfd5336 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15981,6 +15981,8 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
 			modeset_put_power_domains(dev_priv, put_domains);
 	}
 	intel_display_set_init_power(dev_priv, false);
+
+	intel_fbc_init_pipe_state(dev_priv);
 }
 
 void intel_display_resume(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f227bf1..62a25c3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1356,6 +1356,7 @@ bool intel_fbc_is_active(struct drm_i915_private *dev_priv);
 void intel_fbc_pre_update(struct intel_crtc *crtc);
 void intel_fbc_post_update(struct intel_crtc *crtc);
 void intel_fbc_init(struct drm_i915_private *dev_priv);
+void intel_fbc_init_pipe_state(struct drm_i915_private *dev_priv);
 void intel_fbc_enable(struct intel_crtc *crtc);
 void intel_fbc_disable(struct intel_crtc *crtc);
 void intel_fbc_global_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 1977176..fe0754c 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -56,6 +56,11 @@ static inline bool fbc_on_plane_a_only(struct drm_i915_private *dev_priv)
 	return INTEL_INFO(dev_priv)->gen < 4;
 }
 
+static inline bool no_fbc_on_multiple_pipes(struct drm_i915_private *dev_priv)
+{
+	return INTEL_INFO(dev_priv)->gen <= 3;
+}
+
 /*
  * In some platforms where the CRTC's x:0/y:0 coordinates doesn't match the
  * frontbuffer's x:0/y:0 coordinates we lie to the hardware about the plane's
@@ -479,25 +484,25 @@ static bool crtc_can_fbc(struct intel_crtc *crtc)
 	return true;
 }
 
-static bool multiple_pipes_ok(struct drm_i915_private *dev_priv)
+static bool multiple_pipes_ok(struct intel_crtc *crtc)
 {
-	enum pipe pipe;
-	int n_pipes = 0;
-	struct drm_crtc *crtc;
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct drm_plane *primary = crtc->base.primary;
+	struct intel_fbc *fbc = &dev_priv->fbc;
+	enum pipe pipe = crtc->pipe;
 
-	if (INTEL_INFO(dev_priv)->gen > 4)
+	/* Don't even bother tracking anything we don't need. */
+	if (!no_fbc_on_multiple_pipes(dev_priv))
 		return true;
 
-	/* FIXME: we don't have the appropriate state locks to do this here. */
-	for_each_pipe(dev_priv, pipe) {
-		crtc = dev_priv->pipe_to_crtc_mapping[pipe];
+	WARN_ON(!drm_modeset_is_locked(&primary->mutex));
 
-		if (intel_crtc_active(crtc) &&
-		    to_intel_plane_state(crtc->primary->state)->visible)
-			n_pipes++;
-	}
+	if (to_intel_plane_state(primary->state)->visible)
+		fbc->visible_pipes_mask |= (1 << pipe);
+	else
+		fbc->visible_pipes_mask &= ~(1 << pipe);
 
-	return (n_pipes < 2);
+	return (fbc->visible_pipes_mask & ~(1 << pipe)) != 0;
 }
 
 static int find_compression_threshold(struct drm_i915_private *dev_priv,
@@ -889,7 +894,7 @@ void intel_fbc_pre_update(struct intel_crtc *crtc)
 
 	mutex_lock(&fbc->lock);
 
-	if (!multiple_pipes_ok(dev_priv)) {
+	if (!multiple_pipes_ok(crtc)) {
 		set_no_fbc_reason(dev_priv, "more than one pipe active");
 		goto deactivate;
 	}
@@ -1122,6 +1127,28 @@ void intel_fbc_global_disable(struct drm_i915_private *dev_priv)
 }
 
 /**
+ * intel_fbc_init_pipe_state - initialize FBC's CRTC visibility tracking
+ * @dev_priv: i915 device instance
+ *
+ * The FBC code needs to track CRTC visibility since the older platforms can't
+ * have FBC enabled while multiple pipes are used. This function does the
+ * initial setup at driver load to make sure FBC is matching the real hardware.
+ */
+void intel_fbc_init_pipe_state(struct drm_i915_private *dev_priv)
+{
+	struct intel_crtc *crtc;
+
+	/* Don't even bother tracking anything if we don't need. */
+	if (!no_fbc_on_multiple_pipes(dev_priv))
+		return;
+
+	for_each_intel_crtc(dev_priv->dev, crtc)
+		if (intel_crtc_active(&crtc->base) &&
+		    to_intel_plane_state(crtc->base.primary->state)->visible)
+			dev_priv->fbc.visible_pipes_mask |= (1 << crtc->pipe);
+}
+
+/**
  * intel_fbc_init - Initialize FBC
  * @dev_priv: the i915 device
  *
-- 
2.6.4

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

[PATCH 16/25] drm/i915: simplify struct drm_device access at intel_atomic_check()

[Paulo Zanoni]

We already have a dev variable, there's no need to access state->dev.
Also, I plan to add another dev_priv user here, so declare one for the
current user.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bfd5336..c7b0580 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13375,6 +13375,7 @@ static void calc_watermark_data(struct drm_atomic_state *state)
 static int intel_atomic_check(struct drm_device *dev,
 			      struct drm_atomic_state *state)
 {
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
@@ -13417,7 +13418,7 @@ static int intel_atomic_check(struct drm_device *dev,
 			return ret;
 
 		if (i915.fastboot &&
-		    intel_pipe_config_compare(state->dev,
+		    intel_pipe_config_compare(dev,
 					to_intel_crtc_state(crtc->state),
 					pipe_config, true)) {
 			crtc_state->mode_changed = false;
@@ -13443,9 +13444,9 @@ static int intel_atomic_check(struct drm_device *dev,
 		if (ret)
 			return ret;
 	} else
-		intel_state->cdclk = to_i915(state->dev)->cdclk_freq;
+		intel_state->cdclk = dev_priv->cdclk_freq;
 
-	ret = drm_atomic_helper_check_planes(state->dev, state);
+	ret = drm_atomic_helper_check_planes(dev, state);
 	if (ret)
 		return ret;
 
-- 
2.6.4

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

[PATCH 17/25] drm/i915/fbc: choose the new FBC CRTC during atomic check

[Paulo Zanoni]

This opens the possibility of implementing nicer schemes to choose the
CRTC, such as checking the amount of stolen memory available, or
choosing the best pipe on platforms that don't die FBC to pipe or
plane A.

This code was written for another refactor that I ended up discarding,
so I don't actually need it, but I figured this patch would be an
improvement on its own so I kept it on the series.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c7b0580..3ee3f98 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13450,6 +13450,7 @@ static int intel_atomic_check(struct drm_device *dev,
 	if (ret)
 		return ret;
 
+	intel_fbc_choose_crtc(dev_priv, state);
 	calc_watermark_data(state);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 62a25c3..89be9d0 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -498,6 +498,8 @@ struct intel_crtc_state {
 
 	bool ips_enabled;
 
+	bool enable_fbc;
+
 	bool double_wide;
 
 	bool dp_encoder_is_mst;
@@ -1352,6 +1354,8 @@ static inline void intel_fbdev_restore_mode(struct drm_device *dev)
 #endif
 
 /* intel_fbc.c */
+void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
+			   struct drm_atomic_state *state);
 bool intel_fbc_is_active(struct drm_i915_private *dev_priv);
 void intel_fbc_pre_update(struct intel_crtc *crtc);
 void intel_fbc_post_update(struct intel_crtc *crtc);
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index fe0754c..e3d8ace 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -824,7 +824,7 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
 	return true;
 }
 
-static bool intel_fbc_can_enable(struct intel_crtc *crtc)
+static bool intel_fbc_can_choose(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 
@@ -1013,11 +1013,76 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
 }
 
 /**
+ * intel_fbc_choose_crtc - select a CRTC to enable FBC on
+ * @dev_priv: i915 device instance
+ * @state: the atomic state structure
+ *
+ * This function looks at the proposed state for CRTCs and planes, then chooses
+ * which pipe is going to have FBC by setting intel_crtc_state->enable_fbc to
+ * true.
+ *
+ * Later, intel_fbc_enable is going to look for state->enable_fbc and then maybe
+ * enable FBC for the chosen CRTC. If it does, it will set dev_priv->fbc.crtc.
+ */
+void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
+			   struct drm_atomic_state *state)
+{
+	struct intel_fbc *fbc = &dev_priv->fbc;
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+	struct drm_plane *plane;
+	struct drm_plane_state *plane_state;
+	bool fbc_crtc_present = false;
+	int i, j;
+
+	mutex_lock(&fbc->lock);
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		if (fbc->crtc == to_intel_crtc(crtc)) {
+			fbc_crtc_present = true;
+			break;
+		}
+	}
+	/* This atomic commit doesn't involve the CRTC currently tied to FBC. */
+	if (!fbc_crtc_present && fbc->crtc != NULL)
+		goto out;
+
+	/* Simply choose the first CRTC that is compatible and has a visible
+	 * plane. We could go for fancier schemes such as checking the plane
+	 * size, but this would just affect the few platforms that don't tie FBC
+	 * to pipe or plane A. */
+	for_each_plane_in_state(state, plane, plane_state, i) {
+		struct intel_plane_state *intel_plane_state =
+			to_intel_plane_state(plane_state);
+
+		if (!intel_plane_state->visible)
+			continue;
+
+		for_each_crtc_in_state(state, crtc, crtc_state, j) {
+			struct intel_crtc_state *intel_crtc_state =
+				to_intel_crtc_state(crtc_state);
+
+			if (plane_state->crtc != crtc)
+				continue;
+
+			if (!intel_fbc_can_choose(to_intel_crtc(crtc)))
+				break;
+
+			intel_crtc_state->enable_fbc = true;
+			goto out;
+		}
+	}
+
+out:
+	mutex_unlock(&fbc->lock);
+}
+
+/**
  * 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.
+ * This function checks if the given CRTC was chosen for FBC, then enables it if
+ * possible. Notice that it doesn't activate FBC.
  */
 void intel_fbc_enable(struct intel_crtc *crtc)
 {
@@ -1034,12 +1099,12 @@ void intel_fbc_enable(struct intel_crtc *crtc)
 		goto out;
 	}
 
+	if (!crtc->config->enable_fbc)
+		goto out;
+
 	WARN_ON(fbc->active);
 	WARN_ON(fbc->crtc != NULL);
 
-	if (!intel_fbc_can_enable(crtc))
-		goto out;
-
 	intel_fbc_update_state_cache(crtc);
 	if (intel_fbc_alloc_cfb(crtc)) {
 		set_no_fbc_reason(dev_priv, "not enough stolen memory");
-- 
2.6.4

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

[PATCH 18/25] drm/i915/fbc: move intel_fbc_{enable, disable} call one level up

[Paulo Zanoni]

Instead of duplicating the calls for every platform, let's just put
them in the correct places inside intel_atomic_commit. This will also
make it easier for us to move the enable call in order to support
fasbtoot.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3ee3f98..c74dfd3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4966,8 +4966,6 @@ 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. */
@@ -5080,8 +5078,6 @@ 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)
@@ -5162,8 +5158,6 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	}
 
 	intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
-
-	intel_fbc_disable(intel_crtc);
 }
 
 static void haswell_crtc_disable(struct drm_crtc *crtc)
@@ -5214,8 +5208,6 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 		intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
 						      true);
 	}
-
-	intel_fbc_disable(intel_crtc);
 }
 
 static void i9xx_pfit_enable(struct intel_crtc *crtc)
@@ -6326,8 +6318,6 @@ 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)
@@ -6390,8 +6380,6 @@ 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(intel_crtc);
 }
 
 static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
@@ -6415,6 +6403,7 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
 
 	dev_priv->display.crtc_disable(crtc);
 	intel_crtc->active = false;
+	intel_fbc_disable(intel_crtc);
 	intel_update_watermarks(crtc);
 	intel_disable_shared_dpll(intel_crtc);
 
@@ -13578,6 +13567,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 			intel_crtc_disable_planes(crtc, crtc_state->plane_mask);
 			dev_priv->display.crtc_disable(crtc);
 			intel_crtc->active = false;
+			intel_fbc_disable(intel_crtc);
 			intel_disable_shared_dpll(intel_crtc);
 
 			/*
@@ -13617,6 +13607,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 		if (modeset && crtc->state->active) {
 			update_scanline_offset(to_intel_crtc(crtc));
 			dev_priv->display.crtc_enable(crtc);
+			intel_fbc_enable(intel_crtc);
 		}
 
 		if (update_pipe) {
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index e3d8ace..22195cb 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -1135,7 +1135,7 @@ static void __intel_fbc_disable(struct drm_i915_private *dev_priv)
 	WARN_ON(!mutex_is_locked(&fbc->lock));
 	WARN_ON(!fbc->enabled);
 	WARN_ON(fbc->active);
-	assert_pipe_disabled(dev_priv, crtc->pipe);
+	WARN_ON(crtc->active);
 
 	DRM_DEBUG_KMS("Disabling FBC on pipe %c\n", pipe_name(crtc->pipe));
 
-- 
2.6.4

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

[PATCH 19/25] drm/i915/fbc: make FBC work with fastboot

[Paulo Zanoni]

Move intel_fbc_enable to a place where it is called regardless of the
"modeset" variable, and make sure intel_fbc_enable can be called
multiple times without intel_fbc_disable being called.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c74dfd3..5263233 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13607,7 +13607,6 @@ static int intel_atomic_commit(struct drm_device *dev,
 		if (modeset && crtc->state->active) {
 			update_scanline_offset(to_intel_crtc(crtc));
 			dev_priv->display.crtc_enable(crtc);
-			intel_fbc_enable(intel_crtc);
 		}
 
 		if (update_pipe) {
@@ -13620,6 +13619,9 @@ static int intel_atomic_commit(struct drm_device *dev,
 		if (!modeset)
 			intel_pre_plane_update(intel_crtc);
 
+		if (crtc->state->active && intel_crtc->atomic.update_fbc)
+			intel_fbc_enable(intel_crtc);
+
 		if (crtc->state->active &&
 		    (crtc->state->planes_changed || update_pipe))
 			drm_atomic_helper_commit_planes_on_crtc(crtc_state);
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 22195cb..a1c1dd5 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -1082,7 +1082,9 @@ out:
  * @crtc: the CRTC
  *
  * This function checks if the given CRTC was chosen for FBC, then enables it if
- * possible. Notice that it doesn't activate FBC.
+ * possible. Notice that it doesn't activate FBC. It is valid to call
+ * intel_fbc_enable multiple times for the same pipe without an
+ * intel_fbc_disable in the middle, as long as it is deactivated.
  */
 void intel_fbc_enable(struct intel_crtc *crtc)
 {
@@ -1095,7 +1097,11 @@ void intel_fbc_enable(struct intel_crtc *crtc)
 	mutex_lock(&fbc->lock);
 
 	if (fbc->enabled) {
-		WARN_ON(fbc->crtc == crtc);
+		WARN_ON(fbc->crtc == NULL);
+		if (fbc->crtc == crtc) {
+			WARN_ON(!crtc->config->enable_fbc);
+			WARN_ON(fbc->active);
+		}
 		goto out;
 	}
 
-- 
2.6.4

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

[PATCH 20/25] drm/i915/fbc: don't try to deactivate FBC if it's not enabled

[Paulo Zanoni]

During FBC invalidation, don't call intel_fbc_deactivate if it's not
enabled. This doesn't fix any bug, but helps making the interface
saner.

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

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index a1c1dd5..7564f1e 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -980,7 +980,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 
 	fbc->busy_bits |= intel_fbc_get_frontbuffer_bit(fbc) & frontbuffer_bits;
 
-	if (fbc->busy_bits)
+	if (fbc->enabled && fbc->busy_bits)
 		intel_fbc_deactivate(dev_priv);
 
 	mutex_unlock(&fbc->lock);
-- 
2.6.4

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

[PATCH 21/25] drm/i915/fbc: don't print no_fbc_reason to dmesg

[Paulo Zanoni]

Our dmesg messages started being misleading after we converted to the
enable+activate model: we always print "Disabling FBC", even when
we're just deactivating it. So, for example, when I boot my machine
and do "dmesg | grep -i fbc", I see:
  [drm:intel_fbc_enable] Enabling FBC on pipe A
  [drm:set_no_fbc_reason] Disabling FBC: framebuffer not tiled or fenced
but then, if I read the debugfs file, I will see:
  $ sudo cat i915_fbc_status
  FBC enabled
  Compressing: yes
so we can conclude that dmesg is misleading, since FBC is actually
enabled. What happened is that we deactivated FBC due to fbcon not
being tiled, but when we silently reactivated it when the display
manager started. We don't print activation messages since there may be
way too many of these operations per second during normal desktop
usage.

One possible solution would be to change set_no_fbc_reason to
correctly differentiate between disable and deactivation, but we
removed support from printing activation/deactivation messages in the
past because they were too frequent. So instead of doing this, let's
just not print anything on dmesg, and leave the debugfs file if the
user needs to investigate something. We already print when we enable
and disable FBC anyway on a given pipe, so this should already help
triaging bugs.

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

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 7564f1e..67dfffc 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -459,18 +459,6 @@ static void intel_fbc_deactivate(struct drm_i915_private *dev_priv)
 		fbc->deactivate(dev_priv);
 }
 
-static void set_no_fbc_reason(struct drm_i915_private *dev_priv,
-			      const char *reason)
-{
-	struct intel_fbc *fbc = &dev_priv->fbc;
-
-	if (fbc->no_fbc_reason == reason)
-		return;
-
-	fbc->no_fbc_reason = reason;
-	DRM_DEBUG_KMS("Disabling FBC: %s\n", reason);
-}
-
 static bool crtc_can_fbc(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
@@ -759,18 +747,18 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
 	struct intel_fbc_state_cache *cache = &fbc->state_cache;
 
 	if (!cache->plane.visible) {
-		set_no_fbc_reason(dev_priv, "primary plane not visible");
+		fbc->no_fbc_reason = "primary plane not visible";
 		return false;
 	}
 
 	if ((cache->crtc.mode_flags & DRM_MODE_FLAG_INTERLACE) ||
 	    (cache->crtc.mode_flags & DRM_MODE_FLAG_DBLSCAN)) {
-		set_no_fbc_reason(dev_priv, "incompatible mode");
+		fbc->no_fbc_reason = "incompatible mode";
 		return false;
 	}
 
 	if (!intel_fbc_hw_tracking_covers_screen(crtc)) {
-		set_no_fbc_reason(dev_priv, "mode too large for compression");
+		fbc->no_fbc_reason = "mode too large for compression";
 		return false;
 	}
 
@@ -779,29 +767,29 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
 	 */
 	if (cache->fb.tiling_mode != I915_TILING_X ||
 	    cache->fb.fence_reg == I915_FENCE_REG_NONE) {
-		set_no_fbc_reason(dev_priv, "framebuffer not tiled or fenced");
+		fbc->no_fbc_reason = "framebuffer not tiled or fenced";
 		return false;
 	}
 	if (INTEL_INFO(dev_priv)->gen <= 4 && !IS_G4X(dev_priv) &&
 	    cache->plane.rotation != BIT(DRM_ROTATE_0)) {
-		set_no_fbc_reason(dev_priv, "rotation unsupported");
+		fbc->no_fbc_reason = "rotation unsupported";
 		return false;
 	}
 
 	if (!stride_is_valid(dev_priv, cache->fb.stride)) {
-		set_no_fbc_reason(dev_priv, "framebuffer stride not supported");
+		fbc->no_fbc_reason = "framebuffer stride not supported";
 		return false;
 	}
 
 	if (!pixel_format_is_valid(dev_priv, cache->fb.pixel_format)) {
-		set_no_fbc_reason(dev_priv, "pixel format is invalid");
+		fbc->no_fbc_reason = "pixel format is invalid";
 		return false;
 	}
 
 	/* WaFbcExceedCdClockThreshold:hsw,bdw */
 	if ((IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) &&
 	    cache->crtc.hsw_bdw_pixel_rate >= dev_priv->cdclk_freq * 95 / 100) {
-		set_no_fbc_reason(dev_priv, "pixel rate is too big");
+		fbc->no_fbc_reason = "pixel rate is too big";
 		return false;
 	}
 
@@ -817,7 +805,7 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
 	 * important case, we can implement it later. */
 	if (intel_fbc_calculate_cfb_size(dev_priv, &fbc->state_cache) >
 	    fbc->compressed_fb.size * fbc->threshold) {
-		set_no_fbc_reason(dev_priv, "CFB requirements changed");
+		fbc->no_fbc_reason = "CFB requirements changed";
 		return false;
 	}
 
@@ -827,24 +815,25 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
 static bool intel_fbc_can_choose(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct intel_fbc *fbc = &dev_priv->fbc;
 
 	if (intel_vgpu_active(dev_priv->dev)) {
-		set_no_fbc_reason(dev_priv, "VGPU is active");
+		fbc->no_fbc_reason = "VGPU is active";
 		return false;
 	}
 
 	if (i915.enable_fbc < 0) {
-		set_no_fbc_reason(dev_priv, "disabled per chip default");
+		fbc->no_fbc_reason = "disabled per chip default";
 		return false;
 	}
 
 	if (!i915.enable_fbc) {
-		set_no_fbc_reason(dev_priv, "disabled per module param");
+		fbc->no_fbc_reason = "disabled per module param";
 		return false;
 	}
 
 	if (!crtc_can_fbc(crtc)) {
-		set_no_fbc_reason(dev_priv, "no enabled pipes can have FBC");
+		fbc->no_fbc_reason = "no enabled pipes can have FBC";
 		return false;
 	}
 
@@ -895,7 +884,7 @@ void intel_fbc_pre_update(struct intel_crtc *crtc)
 	mutex_lock(&fbc->lock);
 
 	if (!multiple_pipes_ok(crtc)) {
-		set_no_fbc_reason(dev_priv, "more than one pipe active");
+		fbc->no_fbc_reason = "more than one pipe active";
 		goto deactivate;
 	}
 
@@ -1113,7 +1102,7 @@ void intel_fbc_enable(struct intel_crtc *crtc)
 
 	intel_fbc_update_state_cache(crtc);
 	if (intel_fbc_alloc_cfb(crtc)) {
-		set_no_fbc_reason(dev_priv, "not enough stolen memory");
+		fbc->no_fbc_reason = "not enough stolen memory";
 		goto out;
 	}
 
-- 
2.6.4

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

[PATCH 22/25] drm/i915/fbc: don't store the fb_id on reg_params

[Paulo Zanoni]

We don't actually use fb_id anywhere. We already compare all
parameters that matter to the hardware: pixel format, stride,
fence_reg and ggtt_offset. The ID shouldn't make a difference.

Besides, we already update the FBC data at every modeset/flip, so this
can't change behind our backs.

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 | 2 --
 2 files changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 65e5771..4763627 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -937,7 +937,6 @@ struct intel_fbc {
 
 		struct {
 			u64 ilk_ggtt_offset;
-			uint32_t id;
 			uint32_t pixel_format;
 			unsigned int stride;
 			int fence_reg;
@@ -954,7 +953,6 @@ struct intel_fbc {
 
 		struct {
 			u64 ggtt_offset;
-			uint32_t id;
 			uint32_t pixel_format;
 			unsigned int stride;
 			int fence_reg;
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 67dfffc..1485356 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -733,7 +733,6 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc)
 	 * platforms that need. */
 	if (dev_priv->fbc.activate == ilk_fbc_activate)
 		cache->fb.ilk_ggtt_offset = i915_gem_obj_ggtt_offset(obj);
-	cache->fb.id = fb->base.id;
 	cache->fb.pixel_format = fb->pixel_format;
 	cache->fb.stride = fb->pitches[0];
 	cache->fb.fence_reg = obj->fence_reg;
@@ -856,7 +855,6 @@ static void intel_fbc_get_reg_params(struct intel_crtc *crtc,
 	params->crtc.plane = crtc->plane;
 	params->crtc.fence_y_offset = get_crtc_fence_y_offset(crtc);
 
-	params->fb.id = cache->fb.id;
 	params->fb.pixel_format = cache->fb.pixel_format;
 	params->fb.stride = cache->fb.stride;
 	params->fb.fence_reg = cache->fb.fence_reg;
-- 
2.6.4

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

[PATCH 23/25] drm/i915/fbc: call intel_fbc_pre_update earlier during page flips

[Paulo Zanoni]

Make sure we do the pre_update - which also deactivates FBC - before
we actually schedule the page flip, just to make sure we don't
flip to the new FB with FBC still activated for the previous FB.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5263233..37f43aa 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11640,6 +11640,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 
 	crtc->primary->fb = fb;
 	update_state_fb(crtc->primary);
+	intel_fbc_pre_update(intel_crtc);
 
 	work->pending_flip_obj = obj;
 
@@ -11722,7 +11723,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 			  to_intel_plane(primary)->frontbuffer_bit);
 	mutex_unlock(&dev->struct_mutex);
 
-	intel_fbc_pre_update(intel_crtc);
 	intel_frontbuffer_flip_prepare(dev,
 				       to_intel_plane(primary)->frontbuffer_bit);
 
-- 
2.6.4

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

[PATCH 24/25] drm/i915/fbc: don't store/check a pointer to the FB

[Paulo Zanoni]

We already make sure we run intel_fbc_update_update during modesets
and page flips, and this function takes care of deactivating FBC, so
it shouldn't be possible for us to reach the condition we check at
intel_fbc_work_fn. So instead of grabbing framebuffer references and
adding a lot of code to track when we need to free them, just don't
track anything at all since we shouldn't need to.

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 | 4 +---
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4763627..624faeb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -965,7 +965,6 @@ struct intel_fbc {
 		bool scheduled;
 		u32 scheduled_vblank;
 		struct work_struct work;
-		struct drm_framebuffer *fb;
 	} 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 1485356..6a4fe0e 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -403,8 +403,7 @@ retry:
 		goto retry;
 	}
 
-	if (crtc->base.primary->fb == work->fb)
-		fbc->activate(dev_priv);
+	fbc->activate(dev_priv);
 
 out_put:
 	drm_crtc_vblank_put(&crtc->base);
@@ -439,7 +438,6 @@ static void intel_fbc_schedule_activation(struct intel_crtc *crtc)
 	 * 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;
 	work->scheduled = true;
 	work->scheduled_vblank = drm_crtc_vblank_count(&crtc->base);
 	drm_crtc_vblank_put(&crtc->base);
-- 
2.6.4

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

[PATCH 25/25] drm/i915/fbc: refactor some small functions called only once

[Paulo Zanoni]

The FBC fixes we've been doing in the last months required a lot of
refactor, so functions that were once big and called from different
spots are now small and called only once. IMHO now it's better to just
move the contents of these functions to their only callers since this
reduces the number of indirections while reading the code.

While at it, also improve the related comments a little bit.

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

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 6a4fe0e..3abbeec 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -412,14 +412,6 @@ out:
 	mutex_unlock(&fbc->lock);
 }
 
-static void intel_fbc_cancel_work(struct drm_i915_private *dev_priv)
-{
-	struct intel_fbc *fbc = &dev_priv->fbc;
-
-	WARN_ON(!mutex_is_locked(&fbc->lock));
-	fbc->work.scheduled = false;
-}
-
 static void intel_fbc_schedule_activation(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
@@ -434,10 +426,10 @@ static void intel_fbc_schedule_activation(struct intel_crtc *crtc)
 		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. */
+	/* It is useless to call intel_fbc_cancel_work() or 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->scheduled = true;
 	work->scheduled_vblank = drm_crtc_vblank_count(&crtc->base);
 	drm_crtc_vblank_put(&crtc->base);
@@ -451,25 +443,15 @@ static void intel_fbc_deactivate(struct drm_i915_private *dev_priv)
 
 	WARN_ON(!mutex_is_locked(&fbc->lock));
 
-	intel_fbc_cancel_work(dev_priv);
+	/* Calling cancel_work() here won't help due to the fact that the work
+	 * function grabs fbc->lock. Just set scheduled to false so the work
+	 * function can know it was cancelled. */
+	fbc->work.scheduled = false;
 
 	if (fbc->active)
 		fbc->deactivate(dev_priv);
 }
 
-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;
-
-	if (fbc_on_plane_a_only(dev_priv) && crtc->plane != PLANE_A)
-		return false;
-
-	return true;
-}
-
 static bool multiple_pipes_ok(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
@@ -829,11 +811,16 @@ static bool intel_fbc_can_choose(struct intel_crtc *crtc)
 		return false;
 	}
 
-	if (!crtc_can_fbc(crtc)) {
+	if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A) {
 		fbc->no_fbc_reason = "no enabled pipes can have FBC";
 		return false;
 	}
 
+	if (fbc_on_plane_a_only(dev_priv) && crtc->plane != PLANE_A) {
+		fbc->no_fbc_reason = "no enabled planes can have FBC";
+		return false;
+	}
+
 	return true;
 }
 
-- 
2.6.4

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

Re: [PATCH 08/25] drm/i915/fbc: unconditionally update FBC during atomic commits

[kbuild test robot]

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

Hi Paulo,

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on next-20160119]
[cannot apply to v4.4]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Paulo-Zanoni/FBC-crtc-fb-locking-smaller-fixes/20160119-214108
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-x011-01180513 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/i915/intel_display.c: In function 'intel_plane_atomic_calc_changes':
>> drivers/gpu/drm/i915/intel_display.c:11811:27: warning: unused variable 'dev_priv' [-Wunused-variable]
     struct drm_i915_private *dev_priv = dev->dev_private;
                              ^

vim +/dev_priv +11811 drivers/gpu/drm/i915/intel_display.c

d21fbe87 Matt Roper        2015-09-24  11795  	int src_w = drm_rect_width(&state->src) >> 16;
d21fbe87 Matt Roper        2015-09-24  11796  	int src_h = drm_rect_height(&state->src) >> 16;
d21fbe87 Matt Roper        2015-09-24  11797  	int dst_w = drm_rect_width(&state->dst);
d21fbe87 Matt Roper        2015-09-24  11798  	int dst_h = drm_rect_height(&state->dst);
d21fbe87 Matt Roper        2015-09-24  11799  
d21fbe87 Matt Roper        2015-09-24  11800  	return (src_w != dst_w || src_h != dst_h);
d21fbe87 Matt Roper        2015-09-24  11801  }
d21fbe87 Matt Roper        2015-09-24  11802  
da20eabd Maarten Lankhorst 2015-06-15  11803  int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
da20eabd Maarten Lankhorst 2015-06-15  11804  				    struct drm_plane_state *plane_state)
da20eabd Maarten Lankhorst 2015-06-15  11805  {
ab1d3a0e Maarten Lankhorst 2015-11-19  11806  	struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc_state);
da20eabd Maarten Lankhorst 2015-06-15  11807  	struct drm_crtc *crtc = crtc_state->crtc;
da20eabd Maarten Lankhorst 2015-06-15  11808  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
da20eabd Maarten Lankhorst 2015-06-15  11809  	struct drm_plane *plane = plane_state->plane;
da20eabd Maarten Lankhorst 2015-06-15  11810  	struct drm_device *dev = crtc->dev;
da20eabd Maarten Lankhorst 2015-06-15 @11811  	struct drm_i915_private *dev_priv = dev->dev_private;
da20eabd Maarten Lankhorst 2015-06-15  11812  	struct intel_plane_state *old_plane_state =
da20eabd Maarten Lankhorst 2015-06-15  11813  		to_intel_plane_state(plane->state);
da20eabd Maarten Lankhorst 2015-06-15  11814  	int idx = intel_crtc->base.base.id, ret;
da20eabd Maarten Lankhorst 2015-06-15  11815  	int i = drm_plane_index(plane);
da20eabd Maarten Lankhorst 2015-06-15  11816  	bool mode_changed = needs_modeset(crtc_state);
da20eabd Maarten Lankhorst 2015-06-15  11817  	bool was_crtc_enabled = crtc->state->active;
da20eabd Maarten Lankhorst 2015-06-15  11818  	bool is_crtc_enabled = crtc_state->active;
da20eabd Maarten Lankhorst 2015-06-15  11819  	bool turn_off, turn_on, visible, was_visible;

:::::: The code at line 11811 was first introduced by commit
:::::: da20eabd2c69761f9dfd849985eb299e3335531f drm/i915: Split plane updates of crtc->atomic into a helper, v2.

:::::: TO: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
:::::: CC: Daniel Vetter <daniel.vetter@ffwll.ch>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 23478 bytes --]

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

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

✗ Fi.CI.BAT: warning for FBC crtc/fb locking + smaller fixes

[Patchwork]

== Summary ==

Built on 20c388faff9d8c41ab27e825c685526561b892a2 drm-intel-nightly: 2016y-01m-19d-13h-31m-46s UTC integration manifest

Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                pass       -> DMESG-WARN (skl-i5k-2)

bdw-nuci7        total:140  pass:131  dwarn:0   dfail:0   fail:0   skip:9  
bsw-nuc-2        total:143  pass:117  dwarn:2   dfail:0   fail:0   skip:24 
byt-nuc          total:143  pass:125  dwarn:3   dfail:0   fail:0   skip:15 
hsw-brixbox      total:143  pass:136  dwarn:0   dfail:0   fail:0   skip:7  
hsw-gt2          total:143  pass:139  dwarn:0   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:143  pass:102  dwarn:3   dfail:0   fail:0   skip:38 
ivb-t430s        total:137  pass:124  dwarn:3   dfail:4   fail:0   skip:6  
skl-i5k-2        total:143  pass:132  dwarn:3   dfail:0   fail:0   skip:8  
snb-dellxps      total:143  pass:124  dwarn:5   dfail:0   fail:0   skip:14 
snb-x220t        total:143  pass:124  dwarn:5   dfail:0   fail:1   skip:13 

Results at /archive/results/CI_IGT_test/Patchwork_1221/

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

Re: [PATCH 02/25] drm/i915/fbc: extract intel_fbc_can_activate()

[Maarten Lankhorst]

Op 19-01-16 om 14:35 schreef Paulo Zanoni:
> Extract all the code that checks if the FBC configuration is valid to
> its own function, making __intel_fbc_update() much simpler.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 92 ++++++++++++++++++++++------------------
>  1 file changed, 50 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 6b43ec3..dff30e1 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -514,17 +514,6 @@ static bool crtc_can_fbc(struct intel_crtc *crtc)
>  	return true;
>  }
>  
> -static bool crtc_is_valid(struct intel_crtc *crtc)
> -{
> -	if (!intel_crtc_active(&crtc->base))
> -		return false;
> -	if (!to_intel_plane_state(crtc->base.primary->state)->visible)
> -		return false;
> -
> -	return true;
> -}
> -
>  static bool multiple_pipes_ok(struct drm_i915_private *dev_priv)
>  {
>  	enum pipe pipe;
> @@ -750,48 +739,40 @@ static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc)
>  	return effective_w <= max_w && effective_h <= max_h;
>  }
>  
> -/**
> - * __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 activates,
> - * deactivates or maintains it on the same state.
> - */
> -static void __intel_fbc_update(struct intel_crtc *crtc)
> +static bool intel_fbc_can_activate(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> +	struct drm_plane *primary;
>  	struct drm_framebuffer *fb;
> +	struct intel_plane_state *plane_state;
>  	struct drm_i915_gem_object *obj;
>  	const struct drm_display_mode *adjusted_mode;
It would be nice if this function had a plane_state and crtc_state passed into it for validation.
I suppose it can wait though, maybe something for the future.
> -	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.enabled || dev_priv->fbc.crtc != crtc)
> -		return;
> -
> -	if (!crtc_is_valid(crtc)) {
> -		set_no_fbc_reason(dev_priv, "no output");
> -		goto out_disable;
> +	if (!intel_crtc_active(&crtc->base)) {
> +		set_no_fbc_reason(dev_priv, "CRTC not active");
> +		return false;
>  	}
This one has become redundant with plane_state->visible. You can't have a visible plane with crtc off any more.

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

Re: [PATCH 02/25] drm/i915/fbc: extract intel_fbc_can_activate()

[Zanoni, Paulo R]

Em Qui, 2016-01-21 às 12:35 +0100, Maarten Lankhorst escreveu:
> Op 19-01-16 om 14:35 schreef Paulo Zanoni:
> > Extract all the code that checks if the FBC configuration is valid
> > to
> > its own function, making __intel_fbc_update() much simpler.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_fbc.c | 92 ++++++++++++++++++++++----
> > --------------
> >  1 file changed, 50 insertions(+), 42 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > b/drivers/gpu/drm/i915/intel_fbc.c
> > index 6b43ec3..dff30e1 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -514,17 +514,6 @@ static bool crtc_can_fbc(struct intel_crtc
> > *crtc)
> >  	return true;
> >  }
> >  
> > -static bool crtc_is_valid(struct intel_crtc *crtc)
> > -{
> > -	if (!intel_crtc_active(&crtc->base))
> > -		return false;
> > -	if (!to_intel_plane_state(crtc->base.primary->state)-
> > >visible)
> > -		return false;
> > -
> > -	return true;
> > -}
> > -
> >  static bool multiple_pipes_ok(struct drm_i915_private *dev_priv)
> >  {
> >  	enum pipe pipe;
> > @@ -750,48 +739,40 @@ static bool
> > intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc)
> >  	return effective_w <= max_w && effective_h <= max_h;
> >  }
> >  
> > -/**
> > - * __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
> > activates,
> > - * deactivates or maintains it on the same state.
> > - */
> > -static void __intel_fbc_update(struct intel_crtc *crtc)
> > +static bool intel_fbc_can_activate(struct intel_crtc *crtc)
> >  {
> >  	struct drm_i915_private *dev_priv = crtc->base.dev-
> > >dev_private;
> > +	struct drm_plane *primary;
> >  	struct drm_framebuffer *fb;
> > +	struct intel_plane_state *plane_state;
> >  	struct drm_i915_gem_object *obj;
> >  	const struct drm_display_mode *adjusted_mode;
> It would be nice if this function had a plane_state and crtc_state
> passed into it for validation.
> I suppose it can wait though, maybe something for the future.

The way we handle this will change a few times during the series. In
the end this function will use the state cache, and
intel_fbc_update_state_cache() will be responsible for finding the
state structures.

> > -	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.enabled || dev_priv->fbc.crtc != crtc)
> > -		return;
> > -
> > -	if (!crtc_is_valid(crtc)) {
> > -		set_no_fbc_reason(dev_priv, "no output");
> > -		goto out_disable;
> > +	if (!intel_crtc_active(&crtc->base)) {
> > +		set_no_fbc_reason(dev_priv, "CRTC not active");
> > +		return false;
> >  	}
> This one has become redundant with plane_state->visible. You can't 
> have a visible plane with crtc off any more.

We fix this later in the series. If we just remove this right now, we
may have a NULL primary->fb causing a segfault, so we get rid of it
when the segfault won't be possible anymore.

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

Re: [PATCH 04/25] drm/i915/fbc: introduce struct intel_fbc_reg_params

[Maarten Lankhorst]

Op 19-01-16 om 14:35 schreef Paulo Zanoni:
> The early return inside __intel_fbc_update does not completely check
> all the parameters that affect the FBC register values. For example,
> we currently lack looking at crtc->adjusted_y (for the fence Y offset)
> and all the parameters that affect the CFB size (for i8xx).
>
> Instead of just adding the missing parameters to the check and hoping
> that any changes to the fbc_activate functions also come with a
> matching change to the __intel_fbc_update check, introduce a new
> structure where we store these parameters and use the structure at the
> fbc_activate function. Of course, it's still possible to access
> everything from dev_priv in those functions, but IMHO the new code
> will be harder to break.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  22 ++++++-
>  drivers/gpu/drm/i915/intel_fbc.c | 131 ++++++++++++++++++++++-----------------
>  2 files changed, 93 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 33217a4..aa9c35e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -909,11 +909,9 @@ struct i915_fbc {
>  	 * it's the outer lock when overlapping with stolen_lock. */
>  	struct mutex lock;
>  	unsigned threshold;
> -	unsigned int fb_id;
>  	unsigned int possible_framebuffer_bits;
>  	unsigned int busy_bits;
>  	struct intel_crtc *crtc;
> -	int y;
>  
>  	struct drm_mm_node compressed_fb;
>  	struct drm_mm_node *compressed_llb;
> @@ -923,6 +921,24 @@ struct i915_fbc {
>  	bool enabled;
>  	bool active;
>  
> +	struct intel_fbc_reg_params {
> +		struct {
> +			enum pipe pipe;
> +			enum plane plane;
> +			unsigned int fence_y_offset;
> +		} crtc;
> +
> +		struct {
> +			u64 ggtt_offset;
> +			uint32_t id;
On a casual glance it looks like this fb.id is write-only now, but the whole struct is being compared.

It might be worth making a note about that.

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

Re: [PATCH 04/25] drm/i915/fbc: introduce struct intel_fbc_reg_params

[Zanoni, Paulo R]

Em Qui, 2016-01-21 às 13:48 +0100, Maarten Lankhorst escreveu:
> Op 19-01-16 om 14:35 schreef Paulo Zanoni:
> > The early return inside __intel_fbc_update does not completely
> > check
> > all the parameters that affect the FBC register values. For
> > example,
> > we currently lack looking at crtc->adjusted_y (for the fence Y
> > offset)
> > and all the parameters that affect the CFB size (for i8xx).
> > 
> > Instead of just adding the missing parameters to the check and
> > hoping
> > that any changes to the fbc_activate functions also come with a
> > matching change to the __intel_fbc_update check, introduce a new
> > structure where we store these parameters and use the structure at
> > the
> > fbc_activate function. Of course, it's still possible to access
> > everything from dev_priv in those functions, but IMHO the new code
> > will be harder to break.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  |  22 ++++++-
> >  drivers/gpu/drm/i915/intel_fbc.c | 131 ++++++++++++++++++++++-----
> > ------------
> >  2 files changed, 93 insertions(+), 60 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 33217a4..aa9c35e 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -909,11 +909,9 @@ struct i915_fbc {
> >  	 * it's the outer lock when overlapping with stolen_lock.
> > */
> >  	struct mutex lock;
> >  	unsigned threshold;
> > -	unsigned int fb_id;
> >  	unsigned int possible_framebuffer_bits;
> >  	unsigned int busy_bits;
> >  	struct intel_crtc *crtc;
> > -	int y;
> >  
> >  	struct drm_mm_node compressed_fb;
> >  	struct drm_mm_node *compressed_llb;
> > @@ -923,6 +921,24 @@ struct i915_fbc {
> >  	bool enabled;
> >  	bool active;
> >  
> > +	struct intel_fbc_reg_params {
> > +		struct {
> > +			enum pipe pipe;
> > +			enum plane plane;
> > +			unsigned int fence_y_offset;
> > +		} crtc;
> > +
> > +		struct {
> > +			u64 ggtt_offset;
> > +			uint32_t id;
> On a casual glance it looks like this fb.id is write-only now, but
> the whole struct is being compared.

It is read during intel_fbc_reg_params_equal(), so we're still
comparing it. I tried to be conservative and just remove the fb.id
comparison later, just to make bisecting easier in case something
happens. But we remove fb.id in patch 22/25.

> 
> It might be worth making a note about that.
> ~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/25] drm/i915/fbc: unconditionally update FBC during atomic commits

[Maarten Lankhorst]

Op 19-01-16 om 14:35 schreef Paulo Zanoni:
> We unconditionally disable/update FBC even during the page flip
> IOCTLs, and an unconditional disable/update at every atomic commit
> touching the primary plane shouldn't impact PC state residency
> noticeably. Besides, the code that checks for rotation is a good hint
> that we may be forgetting something else, so let's leave all the
> decisions to intel_fbc.c, making the code much safer.
>
> Once we have the code to properly make FBC enable/update decisions
> based on atomic states, with proper locking, then we'll be able to
> evaluate whether it will be worth trying to optimize the cases where a
> disable isn't needed.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
I would rather have this patch remove those 2 members entirely, but I can work with this for now.

Could nuke at least disable_fbc though, being redundant with update_fbc. :)

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

Re: [PATCH 08/25] drm/i915/fbc: unconditionally update FBC during atomic commits

[Zanoni, Paulo R]

Em Qui, 2016-01-21 às 14:04 +0100, Maarten Lankhorst escreveu:
> Op 19-01-16 om 14:35 schreef Paulo Zanoni:
> > We unconditionally disable/update FBC even during the page flip
> > IOCTLs, and an unconditional disable/update at every atomic commit
> > touching the primary plane shouldn't impact PC state residency
> > noticeably. Besides, the code that checks for rotation is a good
> > hint
> > that we may be forgetting something else, so let's leave all the
> > decisions to intel_fbc.c, making the code much safer.
> > 
> > Once we have the code to properly make FBC enable/update decisions
> > based on atomic states, with proper locking, then we'll be able to
> > evaluate whether it will be worth trying to optimize the cases
> > where a
> > disable isn't needed.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> I would rather have this patch remove those 2 members entirely, but I
> can work with this for now.

And what would be the new way to know whether a given atomic commit
touches the primary plane of a given crtc?

> 
> Could nuke at least disable_fbc though, being redundant with
> update_fbc. :)

Check patch 11 :)

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

Re: [PATCH 15/25] drm/i915/fbc: rewrite the multiple_pipes_ok() code for locking

[Maarten Lankhorst]

Op 19-01-16 om 14:35 schreef Paulo Zanoni:
> Older FBC platforms have this restriction where FBC can't be enabled
> if multiple pipes are enabled. In the current code, we disable FBC
> before the second pipe becomes visible.
>
> One of the problems with this code is that the current
> multiple_pipes_ok() implementation just iterates through all CRTCs
> looking at their states, but it doesn't make sure that the state
> locks are grabbed. It also can't just grab the locks for every CRTC
> since this would kill one of the biggest advantages of atomic
> modesetting.
>
> After the recent FBC changes, we now have the appropriate locks for
> the given CRTC, so we can just try to maintain the state of each CRTC
> and update it once intel_fbc_pre_update is called.
>
> As a last note, I don't have gen 2/3 machines to test this code. My
> current plan is to enable FBC on just the newer platforms, so this
> patch is just an attempt to get the gen 2/3 code at least looking
> sane, so if one day someone decide to fix FBC on these platforms, they
> may have less work to do.
>
It would still be nice if pre/post update took a crtc_state and plane_state so we wouldn't have to worry about lifetime issues.
Right now it's pulled from various places.

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

Re: [PATCH 08/25] drm/i915/fbc: unconditionally update FBC during atomic commits

[Maarten Lankhorst]

Op 21-01-16 om 14:27 schreef Zanoni, Paulo R:
> Em Qui, 2016-01-21 às 14:04 +0100, Maarten Lankhorst escreveu:
>> Op 19-01-16 om 14:35 schreef Paulo Zanoni:
>>> We unconditionally disable/update FBC even during the page flip
>>> IOCTLs, and an unconditional disable/update at every atomic commit
>>> touching the primary plane shouldn't impact PC state residency
>>> noticeably. Besides, the code that checks for rotation is a good
>>> hint
>>> that we may be forgetting something else, so let's leave all the
>>> decisions to intel_fbc.c, making the code much safer.
>>>
>>> Once we have the code to properly make FBC enable/update decisions
>>> based on atomic states, with proper locking, then we'll be able to
>>> evaluate whether it will be worth trying to optimize the cases
>>> where a
>>> disable isn't needed.
>>>
>>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> I would rather have this patch remove those 2 members entirely, but I
>> can work with this for now.
> And what would be the new way to know whether a given atomic commit
> touches the primary plane of a given crtc?
if (drm_atomic_get_existing_plane_state(old_crtc_state->state, crtc->primary))
>> Could nuke at least disable_fbc though, being redundant with
>> update_fbc. :)
> Check patch 11 :)
>
>> ~Maarten

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

Re: [PATCH 01/25] drm/i915/fbc: wait for a vblank instead of 50ms when enabling

[Maarten Lankhorst]

Op 19-01-16 om 14:35 schreef Paulo Zanoni:
> Instead of waiting for 50ms, just wait until the next vblank, since
> it's the minimum requirement. The whole infrastructure of FBC is based
> on vblanks, so waiting for X vblanks instead of X milliseconds sounds
> like the correct way to go. Besides, 50ms may be less than a vblank on
> super slow modes that may or may not exist.
>
> There are some small improvements in PC state residency (due to the
> fact that we're now using 16ms for the common modes instead of 50ms),
> but the biggest advantage is still the correctness of being
> vblank-based instead of time-based.
>
> v2:
>   - Rebase after changing the patch order.
>   - Update the commit message.
> v3:
>   - Fix bogus vblank_get() instead of vblank_count() (Ville).
>   - Don't forget to call drm_crtc_vblank_{get,put} (Chris, Ville)
>   - Adjust the performance details on 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 | 43 ++++++++++++++++++++++++++++------------
>  2 files changed, 31 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index af30148..33217a4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -925,9 +925,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 a1988a4..6b43ec3 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -381,7 +381,15 @@ 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;
> -	int delay_ms = 50;
> +	struct drm_vblank_crtc *vblank = &dev_priv->dev->vblank[crtc->pipe];
> +
> +	mutex_lock(&dev_priv->fbc.lock);
> +	if (drm_crtc_vblank_get(&crtc->base)) {
> +		DRM_ERROR("vblank not available for FBC on pipe %c\n",
> +			  pipe_name(crtc->pipe));
> +		goto out;
> +	}
> +	mutex_unlock(&dev_priv->fbc.lock);
What does the lock protect here?

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

Re: [PATCH 00/25] FBC crtc/fb locking + smaller fixes

[Maarten Lankhorst]

Op 19-01-16 om 14:35 schreef Paulo Zanoni:
> Hi
>
> Here's yet another patch series randomly modifying the FBC code. We
> start by refactoring things in order to fix the locking problems, then
> fix a few other smaller problems and apply some polishing.
>
> Just to keep the tradition of the past 10 cover letters, I guess I
> should say that this series is the last one and that we may consider
> enabling FBC on HSW/BDW/SKL after it is merged :)
>
> For SKL specifically, I tested this series on a two-weeks old version
> of drm-intel-nightly since today's version is giving me a BUG() even
> without my patches applied.
>
> Thanks,
> Paulo
>
> Paulo Zanoni (25):
>   drm/i915/fbc: wait for a vblank instead of 50ms when enabling
>   drm/i915/fbc: extract intel_fbc_can_activate()
>   drm/i915/fbc: extract intel_fbc_can_enable()
>   drm/i915/fbc: introduce struct intel_fbc_reg_params
>   drm/i915/fbc: replace frequent dev_priv->fbc.x with fbc->x
>   drm/i915/fbc: don't use the frontbuffer tracking subsystem for flips
>   drm/i915/fbc: don't flush for operations on the wrong frontbuffer
>   drm/i915/fbc: unconditionally update FBC during atomic commits
>   drm/i915/fbc: introduce struct intel_fbc_state_cache
>   drm/i915/fbc: split intel_fbc_update into pre and post update
>   drm/i915/fbc: fix the FBC state checking code
>   drm/i915/fbc: unexport intel_fbc_deactivate
>   drm/i915/fbc: rename the FBC disable functions
>   drm/i915/fbc: make sure we cancel the work function at fbc_disable
>   drm/i915/fbc: rewrite the multiple_pipes_ok() code for locking
>   drm/i915: simplify struct drm_device access at intel_atomic_check()
>   drm/i915/fbc: choose the new FBC CRTC during atomic check
>   drm/i915/fbc: move intel_fbc_{enable,disable} call one level up
>   drm/i915/fbc: make FBC work with fastboot
>   drm/i915/fbc: don't try to deactivate FBC if it's not enabled
>   drm/i915/fbc: don't print no_fbc_reason to dmesg
>   drm/i915/fbc: don't store the fb_id on reg_params
>   drm/i915/fbc: call intel_fbc_pre_update earlier during page flips
>   drm/i915/fbc: don't store/check a pointer to the FB
>   drm/i915/fbc: refactor some small functions called only once
>
For the whole series except patch 1

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/25] drm/i915/fbc: wait for a vblank instead of 50ms when enabling

[Zanoni, Paulo R]

Em Qui, 2016-01-21 às 15:17 +0100, Maarten Lankhorst escreveu:
> Op 19-01-16 om 14:35 schreef Paulo Zanoni:
> > Instead of waiting for 50ms, just wait until the next vblank, since
> > it's the minimum requirement. The whole infrastructure of FBC is
> > based
> > on vblanks, so waiting for X vblanks instead of X milliseconds
> > sounds
> > like the correct way to go. Besides, 50ms may be less than a vblank
> > on
> > super slow modes that may or may not exist.
> > 
> > There are some small improvements in PC state residency (due to the
> > fact that we're now using 16ms for the common modes instead of
> > 50ms),
> > but the biggest advantage is still the correctness of being
> > vblank-based instead of time-based.
> > 
> > v2:
> >   - Rebase after changing the patch order.
> >   - Update the commit message.
> > v3:
> >   - Fix bogus vblank_get() instead of vblank_count() (Ville).
> >   - Don't forget to call drm_crtc_vblank_{get,put} (Chris, Ville)
> >   - Adjust the performance details on 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 | 43
> > ++++++++++++++++++++++++++++------------
> >  2 files changed, 31 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index af30148..33217a4 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -925,9 +925,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 a1988a4..6b43ec3 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -381,7 +381,15 @@ 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;
> > -	int delay_ms = 50;
> > +	struct drm_vblank_crtc *vblank = &dev_priv->dev-
> > >vblank[crtc->pipe];
> > +
> > +	mutex_lock(&dev_priv->fbc.lock);
> > +	if (drm_crtc_vblank_get(&crtc->base)) {
> > +		DRM_ERROR("vblank not available for FBC on pipe
> > %c\n",
> > +			  pipe_name(crtc->pipe));
> > +		goto out;
> > +	}
> > +	mutex_unlock(&dev_priv->fbc.lock);
> What does the lock protect here?

Hmmm, yeah, it protects nothing... Well observed.

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

[PATCH 01/25] drm/i915/fbc: wait for a vblank instead of 50ms when enabling

[Paulo Zanoni]

Instead of waiting for 50ms, just wait until the next vblank, since
it's the minimum requirement. The whole infrastructure of FBC is based
on vblanks, so waiting for X vblanks instead of X milliseconds sounds
like the correct way to go. Besides, 50ms may be less than a vblank on
super slow modes that may or may not exist.

There are some small improvements in PC state residency (due to the
fact that we're now using 16ms for the common modes instead of 50ms),
but the biggest advantage is still the correctness of being
vblank-based instead of time-based.

v2:
  - Rebase after changing the patch order.
  - Update the commit message.
v3:
  - Fix bogus vblank_get() instead of vblank_count() (Ville).
  - Don't forget to call drm_crtc_vblank_{get,put} (Chris, Ville)
  - Adjust the performance details on the commit message.
v4:
  - Don't grab the FBC mutex just to grab the vblank (Maarten)

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 | 39 +++++++++++++++++++++++++++++----------
 2 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 204661f..d8f21f0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -921,9 +921,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 a1988a4..3993b43 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -381,7 +381,17 @@ 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;
-	int delay_ms = 50;
+	struct drm_vblank_crtc *vblank = &dev_priv->dev->vblank[crtc->pipe];
+
+	if (drm_crtc_vblank_get(&crtc->base)) {
+		DRM_ERROR("vblank not available for FBC on pipe %c\n",
+			  pipe_name(crtc->pipe));
+
+		mutex_lock(&dev_priv->fbc.lock);
+		work->scheduled = false;
+		mutex_unlock(&dev_priv->fbc.lock);
+		return;
+	}
 
 retry:
 	/* Delay the actual enabling to let pageflipping cease and the
@@ -390,14 +400,16 @@ 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
+	 *
+	 * It is also worth mentioning that since work->scheduled_vblank can be
+	 * updated multiple times by the other threads, hitting the timeout is
+	 * not an error condition. We'll just end up hitting the "goto retry"
+	 * case below.
 	 */
-	wait_remaining_ms_from_jiffies(work->enable_jiffies, delay_ms);
+	wait_event_timeout(vblank->queue,
+		drm_crtc_vblank_count(&crtc->base) != work->scheduled_vblank,
+		msecs_to_jiffies(50));
 
 	mutex_lock(&dev_priv->fbc.lock);
 
@@ -406,8 +418,7 @@ retry:
 		goto out;
 
 	/* Were we delayed again while this function was sleeping? */
-	if (time_after(work->enable_jiffies + msecs_to_jiffies(delay_ms),
-		       jiffies)) {
+	if (drm_crtc_vblank_count(&crtc->base) == work->scheduled_vblank) {
 		mutex_unlock(&dev_priv->fbc.lock);
 		goto retry;
 	}
@@ -419,6 +430,7 @@ retry:
 
 out:
 	mutex_unlock(&dev_priv->fbc.lock);
+	drm_crtc_vblank_put(&crtc->base);
 }
 
 static void intel_fbc_cancel_work(struct drm_i915_private *dev_priv)
@@ -434,13 +446,20 @@ static void intel_fbc_schedule_activation(struct intel_crtc *crtc)
 
 	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
 
+	if (drm_crtc_vblank_get(&crtc->base)) {
+		DRM_ERROR("vblank not available for FBC on pipe %c\n",
+			  pipe_name(crtc->pipe));
+		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;
 	work->scheduled = true;
-	work->enable_jiffies = jiffies;
+	work->scheduled_vblank = drm_crtc_vblank_count(&crtc->base);
+	drm_crtc_vblank_put(&crtc->base);
 
 	schedule_work(&work->work);
 }
-- 
2.6.4

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

[PATCH 08/25] drm/i915/fbc: unconditionally update FBC during atomic commits

[Paulo Zanoni]

We unconditionally disable/update FBC even during the page flip
IOCTLs, and an unconditional disable/update at every atomic commit
touching the primary plane shouldn't impact PC state residency
noticeably. Besides, the code that checks for rotation is a good hint
that we may be forgetting something else, so let's leave all the
decisions to intel_fbc.c, making the code much safer.

Once we have the code to properly make FBC enable/update decisions
based on atomic states, with proper locking, then we'll be able to
evaluate whether it will be worth trying to optimize the cases where a
disable isn't needed.

v2: Upstream moved and now our patch needs to remove dev_priv.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4329ba1..d876887 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11838,7 +11838,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_plane *plane = plane_state->plane;
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane_state *old_plane_state =
 		to_intel_plane_state(plane->state);
 	int idx = intel_crtc->base.base.id, ret;
@@ -11906,6 +11905,8 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	case DRM_PLANE_TYPE_PRIMARY:
 		intel_crtc->atomic.pre_disable_primary = turn_off;
 		intel_crtc->atomic.post_enable_primary = turn_on;
+		intel_crtc->atomic.disable_fbc = true;
+		intel_crtc->atomic.update_fbc = true;
 
 		if (turn_off) {
 			/*
@@ -11917,28 +11918,9 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 			 * disable.
 			 */
 			intel_crtc->atomic.disable_ips = true;
-
-			intel_crtc->atomic.disable_fbc = true;
 		}
 
 		/*
-		 * FBC does not work on some platforms for rotated
-		 * planes, so disable it when rotation is not 0 and
-		 * update it when rotation is set back to 0.
-		 *
-		 * FIXME: This is redundant with the fbc update done in
-		 * the primary plane enable function except that that
-		 * one is done too late. We eventually need to unify
-		 * this.
-		 */
-
-		if (visible &&
-		    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
-		    dev_priv->fbc.crtc == intel_crtc &&
-		    plane_state->rotation != BIT(DRM_ROTATE_0))
-			intel_crtc->atomic.disable_fbc = true;
-
-		/*
 		 * BDW signals flip done immediately if the plane
 		 * is disabled, even if the plane enable is already
 		 * armed to occur at the next vblank :(
@@ -11946,7 +11928,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 		if (turn_on && IS_BROADWELL(dev))
 			intel_crtc->atomic.wait_vblank = true;
 
-		intel_crtc->atomic.update_fbc |= visible || mode_changed;
 		break;
 	case DRM_PLANE_TYPE_CURSOR:
 		break;
-- 
2.6.4

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

Re: ✗ Fi.CI.BAT: warning for FBC crtc/fb locking + smaller fixes

[Zanoni, Paulo R]

Em Ter, 2016-01-19 às 14:50 +0000, Patchwork escreveu:
> == Summary ==
> 
> Built on 20c388faff9d8c41ab27e825c685526561b892a2 drm-intel-nightly:
> 2016y-01m-19d-13h-31m-46s UTC integration manifest
> 
> Test kms_flip:
>         Subgroup basic-flip-vs-modeset:
>                 pass       -> DMESG-WARN (skl-i5k-2)

The dmesg warn is the following:

[  273.515394] [drm:gen8_irq_handler [i915]] *ERROR* The master control
interrupt lied (DE PIPE)!

I coulnd't find anything showing that this error was present before the
patch series, but I also can't reproduce it on my SKL machine and I
find it hard to believe that this error was introduced by any of the
patches in question.

So, with the new rules, how do I proceed here?

If the series was shorter I'd just resubmit and see if the RNG allows
me to pass, but sending 25 patches again won't be cool. It would be
good to have a way to ask the CI to run the failed subtests again on
the same patch series in order to confirm things.

> 
> bdw-
> nuci7        total:140  pass:131  dwarn:0   dfail:0   fail:0   skip:9
>   
> bsw-nuc-
> 2        total:143  pass:117  dwarn:2   dfail:0   fail:0   skip:24 
> byt-
> nuc          total:143  pass:125  dwarn:3   dfail:0   fail:0   skip:1
> 5 
> hsw-
> brixbox      total:143  pass:136  dwarn:0   dfail:0   fail:0   skip:7
>   
> hsw-
> gt2          total:143  pass:139  dwarn:0   dfail:0   fail:0   skip:4
>   
> ilk-
> hp8440p      total:143  pass:102  dwarn:3   dfail:0   fail:0   skip:3
> 8 
> ivb-
> t430s        total:137  pass:124  dwarn:3   dfail:4   fail:0   skip:6
>   
> skl-i5k-
> 2        total:143  pass:132  dwarn:3   dfail:0   fail:0   skip:8  
> snb-
> dellxps      total:143  pass:124  dwarn:5   dfail:0   fail:0   skip:1
> 4 
> snb-
> x220t        total:143  pass:124  dwarn:5   dfail:0   fail:1   skip:1
> 3 
> 
> Results at /archive/results/CI_IGT_test/Patchwork_1221/
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for FBC crtc/fb locking + smaller fixes (rev3)

[Patchwork]

== Summary ==

HEAD is now at 8fe9e78 drm-intel-nightly: 2016y-01m-21d-11h-02m-42s UTC integration manifest
Applying: drm/i915/fbc: wait for a vblank instead of 50ms when enabling
Applying: drm/i915/fbc: extract intel_fbc_can_activate()
Applying: drm/i915/fbc: extract intel_fbc_can_enable()
Applying: drm/i915/fbc: introduce struct intel_fbc_reg_params
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/i915_drv.h
M	drivers/gpu/drm/i915/intel_fbc.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/intel_fbc.c
Auto-merging drivers/gpu/drm/i915/i915_drv.h
Applying: drm/i915/fbc: replace frequent dev_priv->fbc.x with fbc->x
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/i915_drv.h
M	drivers/gpu/drm/i915/intel_fbc.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/intel_fbc.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/intel_fbc.c
Auto-merging drivers/gpu/drm/i915/i915_drv.h
Patch failed at 0005 drm/i915/fbc: replace frequent dev_priv->fbc.x with fbc->x

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

Re: ✗ Fi.CI.BAT: warning for FBC crtc/fb locking + smaller fixes

[Damien Lespiau]

On Thu, Jan 21, 2016 at 08:14:34PM +0000, Zanoni, Paulo R wrote:
> Em Ter, 2016-01-19 às 14:50 +0000, Patchwork escreveu:
> > == Summary ==
> > 
> > Built on 20c388faff9d8c41ab27e825c685526561b892a2 drm-intel-nightly:
> > 2016y-01m-19d-13h-31m-46s UTC integration manifest
> > 
> > Test kms_flip:
> >         Subgroup basic-flip-vs-modeset:
> >                 pass       -> DMESG-WARN (skl-i5k-2)
> 
> The dmesg warn is the following:
> 
> [  273.515394] [drm:gen8_irq_handler [i915]] *ERROR* The master control
> interrupt lied (DE PIPE)!
> 
> I coulnd't find anything showing that this error was present before the
> patch series, but I also can't reproduce it on my SKL machine and I
> find it hard to believe that this error was introduced by any of the
> patches in question.
> 
> So, with the new rules, how do I proceed here?

I'm sure we can be pragmatic here. We've seen that one before, at least
on BDW.

> If the series was shorter I'd just resubmit and see if the RNG allows
> me to pass, but sending 25 patches again won't be cool. It would be
> good to have a way to ask the CI to run the failed subtests again on
> the same patch series in order to confirm things.

Right, that's indeed a feature that is planned, while I'd like to finish
other things first, I guess it's simple enough that I can bump the
priority a bit.

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

Re: [PATCH 01/25] drm/i915/fbc: wait for a vblank instead of 50ms when enabling

[Zanoni, Paulo R]

Em Qui, 2016-01-21 às 18:03 -0200, Paulo Zanoni escreveu:
> Instead of waiting for 50ms, just wait until the next vblank, since
> it's the minimum requirement. The whole infrastructure of FBC is
> based
> on vblanks, so waiting for X vblanks instead of X milliseconds sounds
> like the correct way to go. Besides, 50ms may be less than a vblank
> on
> super slow modes that may or may not exist.
> 
> There are some small improvements in PC state residency (due to the
> fact that we're now using 16ms for the common modes instead of 50ms),
> but the biggest advantage is still the correctness of being
> vblank-based instead of time-based.
> 
> v2:
>   - Rebase after changing the patch order.
>   - Update the commit message.
> v3:
>   - Fix bogus vblank_get() instead of vblank_count() (Ville).
>   - Don't forget to call drm_crtc_vblank_{get,put} (Chris, Ville)
>   - Adjust the performance details on the commit message.
> v4:
>   - Don't grab the FBC mutex just to grab the vblank (Maarten)

CCing Chris and Ville per Maarten's request on IRC: "would be nice if
someone who reviewed the earlier version would ack it too".

> 
> 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 | 39 +++++++++++++++++++++++++++++-
> ---------
>  2 files changed, 30 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 204661f..d8f21f0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -921,9 +921,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 a1988a4..3993b43 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -381,7 +381,17 @@ 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;
> -	int delay_ms = 50;
> +	struct drm_vblank_crtc *vblank = &dev_priv->dev-
> >vblank[crtc->pipe];
> +
> +	if (drm_crtc_vblank_get(&crtc->base)) {
> +		DRM_ERROR("vblank not available for FBC on pipe
> %c\n",
> +			  pipe_name(crtc->pipe));
> +
> +		mutex_lock(&dev_priv->fbc.lock);
> +		work->scheduled = false;
> +		mutex_unlock(&dev_priv->fbc.lock);
> +		return;
> +	}
>  
>  retry:
>  	/* Delay the actual enabling to let pageflipping cease and
> the
> @@ -390,14 +400,16 @@ 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
> +	 *
> +	 * It is also worth mentioning that since work-
> >scheduled_vblank can be
> +	 * updated multiple times by the other threads, hitting the
> timeout is
> +	 * not an error condition. We'll just end up hitting the
> "goto retry"
> +	 * case below.
>  	 */
> -	wait_remaining_ms_from_jiffies(work->enable_jiffies,
> delay_ms);
> +	wait_event_timeout(vblank->queue,
> +		drm_crtc_vblank_count(&crtc->base) != work-
> >scheduled_vblank,
> +		msecs_to_jiffies(50));
>  
>  	mutex_lock(&dev_priv->fbc.lock);
>  
> @@ -406,8 +418,7 @@ retry:
>  		goto out;
>  
>  	/* Were we delayed again while this function was sleeping?
> */
> -	if (time_after(work->enable_jiffies +
> msecs_to_jiffies(delay_ms),
> -		       jiffies)) {
> +	if (drm_crtc_vblank_count(&crtc->base) == work-
> >scheduled_vblank) {
>  		mutex_unlock(&dev_priv->fbc.lock);
>  		goto retry;
>  	}
> @@ -419,6 +430,7 @@ retry:
>  
>  out:
>  	mutex_unlock(&dev_priv->fbc.lock);
> +	drm_crtc_vblank_put(&crtc->base);
>  }
>  
>  static void intel_fbc_cancel_work(struct drm_i915_private *dev_priv)
> @@ -434,13 +446,20 @@ static void
> intel_fbc_schedule_activation(struct intel_crtc *crtc)
>  
>  	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
>  
> +	if (drm_crtc_vblank_get(&crtc->base)) {
> +		DRM_ERROR("vblank not available for FBC on pipe
> %c\n",
> +			  pipe_name(crtc->pipe));
> +		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;
>  	work->scheduled = true;
> -	work->enable_jiffies = jiffies;
> +	work->scheduled_vblank = drm_crtc_vblank_count(&crtc->base);
> +	drm_crtc_vblank_put(&crtc->base);
>  
>  	schedule_work(&work->work);
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: warning for FBC crtc/fb locking + smaller fixes

[Daniel Vetter]

On Fri, Jan 22, 2016 at 11:28:07AM +0000, Damien Lespiau wrote:
> On Thu, Jan 21, 2016 at 08:14:34PM +0000, Zanoni, Paulo R wrote:
> > Em Ter, 2016-01-19 às 14:50 +0000, Patchwork escreveu:
> > > == Summary ==
> > > 
> > > Built on 20c388faff9d8c41ab27e825c685526561b892a2 drm-intel-nightly:
> > > 2016y-01m-19d-13h-31m-46s UTC integration manifest
> > > 
> > > Test kms_flip:
> > >         Subgroup basic-flip-vs-modeset:
> > >                 pass       -> DMESG-WARN (skl-i5k-2)
> > 
> > The dmesg warn is the following:
> > 
> > [  273.515394] [drm:gen8_irq_handler [i915]] *ERROR* The master control
> > interrupt lied (DE PIPE)!
> > 
> > I coulnd't find anything showing that this error was present before the
> > patch series, but I also can't reproduce it on my SKL machine and I
> > find it hard to believe that this error was introduced by any of the
> > patches in question.
> > 
> > So, with the new rules, how do I proceed here?
> 
> I'm sure we can be pragmatic here. We've seen that one before, at least
> on BDW.

Yeah, there should be a bug report for this one already, just upgrade it
to P1 and add a link here. And if there's indeed none then file a new one
- we've seen this one before.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/25] drm/i915/fbc: wait for a vblank instead of 50ms when enabling

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 5865 bytes --]

On Thu, Jan 21, 2016 at 12:03 PM Paulo Zanoni <paulo.r.zanoni@intel.com>
wrote:

> Instead of waiting for 50ms, just wait until the next vblank, since
> it's the minimum requirement. The whole infrastructure of FBC is based
> on vblanks, so waiting for X vblanks instead of X milliseconds sounds
> like the correct way to go. Besides, 50ms may be less than a vblank on
> super slow modes that may or may not exist.
>
> There are some small improvements in PC state residency (due to the
> fact that we're now using 16ms for the common modes instead of 50ms),
> but the biggest advantage is still the correctness of being
> vblank-based instead of time-based.
>
> v2:
>   - Rebase after changing the patch order.
>   - Update the commit message.
> v3:
>   - Fix bogus vblank_get() instead of vblank_count() (Ville).
>   - Don't forget to call drm_crtc_vblank_{get,put} (Chris, Ville)
>   - Adjust the performance details on the commit message.
> v4:
>   - Don't grab the FBC mutex just to grab the vblank (Maarten)
>
> 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 | 39
> +++++++++++++++++++++++++++++----------
>  2 files changed, 30 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 204661f..d8f21f0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -921,9 +921,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 a1988a4..3993b43 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -381,7 +381,17 @@ 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;
> -       int delay_ms = 50;
> +       struct drm_vblank_crtc *vblank =
> &dev_priv->dev->vblank[crtc->pipe];
> +
> +       if (drm_crtc_vblank_get(&crtc->base)) {
> +               DRM_ERROR("vblank not available for FBC on pipe %c\n",
> +                         pipe_name(crtc->pipe));
> +
> +               mutex_lock(&dev_priv->fbc.lock);
> +               work->scheduled = false;
>

I couldn't understand this here... doesn't look related to s/time/vblank...
could you please explain?

+               mutex_unlock(&dev_priv->fbc.lock);
> +               return;
> +       }
>
>  retry:
>         /* Delay the actual enabling to let pageflipping cease and the
> @@ -390,14 +400,16 @@ 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
>

hm... is it still valid for newer platforms or we should put a if gen <=6
on these checks?


> +        *
> +        * It is also worth mentioning that since work->scheduled_vblank
> can be
> +        * updated multiple times by the other threads, hitting the
> timeout is
> +        * not an error condition. We'll just end up hitting the "goto
> retry"
> +        * case below.
>          */
> -       wait_remaining_ms_from_jiffies(work->enable_jiffies, delay_ms);
> +       wait_event_timeout(vblank->queue,
> +               drm_crtc_vblank_count(&crtc->base) !=
> work->scheduled_vblank,
> +               msecs_to_jiffies(50));
>
>         mutex_lock(&dev_priv->fbc.lock);
>
> @@ -406,8 +418,7 @@ retry:
>                 goto out;
>
>         /* Were we delayed again while this function was sleeping? */
> -       if (time_after(work->enable_jiffies + msecs_to_jiffies(delay_ms),
> -                      jiffies)) {
> +       if (drm_crtc_vblank_count(&crtc->base) == work->scheduled_vblank) {
>                 mutex_unlock(&dev_priv->fbc.lock);
>                 goto retry;
>         }
> @@ -419,6 +430,7 @@ retry:
>
>  out:
>         mutex_unlock(&dev_priv->fbc.lock);
> +       drm_crtc_vblank_put(&crtc->base);
>  }
>
>  static void intel_fbc_cancel_work(struct drm_i915_private *dev_priv)
> @@ -434,13 +446,20 @@ static void intel_fbc_schedule_activation(struct
> intel_crtc *crtc)
>
>         WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
>
> +       if (drm_crtc_vblank_get(&crtc->base)) {
> +               DRM_ERROR("vblank not available for FBC on pipe %c\n",
> +                         pipe_name(crtc->pipe));
> +               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;
>         work->scheduled = true;
> -       work->enable_jiffies = jiffies;
> +       work->scheduled_vblank = drm_crtc_vblank_count(&crtc->base);
> +       drm_crtc_vblank_put(&crtc->base);
>
>         schedule_work(&work->work);
>  }
> --
> 2.6.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

[-- Attachment #1.2: Type: text/html, Size: 7687 bytes --]

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

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

Re: [PATCH 01/25] drm/i915/fbc: wait for a vblank instead of 50ms when enabling

[Zanoni, Paulo R]

Em Ter, 2016-01-26 às 17:44 +0000, Rodrigo Vivi escreveu:
> 
> 
> On Thu, Jan 21, 2016 at 12:03 PM Paulo Zanoni <paulo.r.zanoni@intel.c
> om> wrote:
> > Instead of waiting for 50ms, just wait until the next vblank, since
> > it's the minimum requirement. The whole infrastructure of FBC is
> > based
> > on vblanks, so waiting for X vblanks instead of X milliseconds
> > sounds
> > like the correct way to go. Besides, 50ms may be less than a vblank
> > on
> > super slow modes that may or may not exist.
> > 
> > There are some small improvements in PC state residency (due to the
> > fact that we're now using 16ms for the common modes instead of
> > 50ms),
> > but the biggest advantage is still the correctness of being
> > vblank-based instead of time-based.
> > 
> > v2:
> >   - Rebase after changing the patch order.
> >   - Update the commit message.
> > v3:
> >   - Fix bogus vblank_get() instead of vblank_count() (Ville).
> >   - Don't forget to call drm_crtc_vblank_{get,put} (Chris, Ville)
> >   - Adjust the performance details on the commit message.
> > v4:
> >   - Don't grab the FBC mutex just to grab the vblank (Maarten)
> > 
> > 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 | 39
> > +++++++++++++++++++++++++++++----------
> >  2 files changed, 30 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 204661f..d8f21f0 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -921,9 +921,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 a1988a4..3993b43 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -381,7 +381,17 @@ 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;
> > -       int delay_ms = 50;
> > +       struct drm_vblank_crtc *vblank = &dev_priv->dev-
> > >vblank[crtc->pipe];
> > +
> > +       if (drm_crtc_vblank_get(&crtc->base)) {
> > +               DRM_ERROR("vblank not available for FBC on pipe
> > %c\n",
> > +                         pipe_name(crtc->pipe));
> > +
> > +               mutex_lock(&dev_priv->fbc.lock);
> > +               work->scheduled = false;
> I couldn't understand this here... doesn't look related to
> s/time/vblank...
> could you please explain?

Previously we were just dealing with "wait a certain amount of
time/jiffies", and for that we can just call the delay/sleep/wait/etc
calls without needing any get/put calls.

Now we'll wait for a certain number of vblanks, and we need to have the
vblank interrupts enabled before we can wait for them. That's why we
have the vblank get/put calls. And since get() can fail, we need an
error path.

Under normal FBC operation, every vblank get/put call should succeed
because the pipe's supposed to be running.  But in case we actually
fail to get vblanks, we just exit from the work function. The way we
signal "the work function is not running" is by setting work->scheduled 
to false.

> 
> >  +               mutex_unlock(&dev_priv->fbc.lock);
> > +               return;
> > +       }
> > 
> >  retry:
> >         /* Delay the actual enabling to let pageflipping cease and
> > the
> > @@ -390,14 +400,16 @@ 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
> hm... is it still valid for newer platforms or we should put a if gen
> <=6 on these checks?

I tested this on BDW some time ago, and it seems we don't actually need
the vblank wait anymore (although I didn't check the docs if we still
need the wait). But I wanted to convert the code to use vblanks before
optimizing it more. And the residency impact won't be big.

>  
> >  +        *
> > +        * It is also worth mentioning that since work-
> > >scheduled_vblank can be
> > +        * updated multiple times by the other threads, hitting the
> > timeout is
> > +        * not an error condition. We'll just end up hitting the
> > "goto retry"
> > +        * case below.
> >          */
> > -       wait_remaining_ms_from_jiffies(work->enable_jiffies,
> > delay_ms);
> > +       wait_event_timeout(vblank->queue,
> > +               drm_crtc_vblank_count(&crtc->base) != work-
> > >scheduled_vblank,
> > +               msecs_to_jiffies(50));
> > 
> >         mutex_lock(&dev_priv->fbc.lock);
> > 
> > @@ -406,8 +418,7 @@ retry:
> >                 goto out;
> > 
> >         /* Were we delayed again while this function was sleeping?
> > */
> > -       if (time_after(work->enable_jiffies +
> > msecs_to_jiffies(delay_ms),
> > -                      jiffies)) {
> > +       if (drm_crtc_vblank_count(&crtc->base) == work-
> > >scheduled_vblank) {
> >                 mutex_unlock(&dev_priv->fbc.lock);
> >                 goto retry;
> >         }
> > @@ -419,6 +430,7 @@ retry:
> > 
> >  out:
> >         mutex_unlock(&dev_priv->fbc.lock);
> > +       drm_crtc_vblank_put(&crtc->base);
> >  }
> > 
> >  static void intel_fbc_cancel_work(struct drm_i915_private
> > *dev_priv)
> > @@ -434,13 +446,20 @@ static void
> > intel_fbc_schedule_activation(struct intel_crtc *crtc)
> > 
> >         WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
> > 
> > +       if (drm_crtc_vblank_get(&crtc->base)) {
> > +               DRM_ERROR("vblank not available for FBC on pipe
> > %c\n",
> > +                         pipe_name(crtc->pipe));
> > +               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;
> >         work->scheduled = true;
> > -       work->enable_jiffies = jiffies;
> > +       work->scheduled_vblank = drm_crtc_vblank_count(&crtc-
> > >base);
> > +       drm_crtc_vblank_put(&crtc->base);
> > 
> >         schedule_work(&work->work);
> >  }
> > --
> > 2.6.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/25] drm/i915/fbc: wait for a vblank instead of 50ms when enabling

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 8082 bytes --]

Thanks for all the explanation.

Makes sense now and everything looks fine for me.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


On Tue, Jan 26, 2016 at 10:08 AM Zanoni, Paulo R <paulo.r.zanoni@intel.com>
wrote:

> Em Ter, 2016-01-26 às 17:44 +0000, Rodrigo Vivi escreveu:
> >
> >
> > On Thu, Jan 21, 2016 at 12:03 PM Paulo Zanoni <paulo.r.zanoni@intel.c
> > om> wrote:
> > > Instead of waiting for 50ms, just wait until the next vblank, since
> > > it's the minimum requirement. The whole infrastructure of FBC is
> > > based
> > > on vblanks, so waiting for X vblanks instead of X milliseconds
> > > sounds
> > > like the correct way to go. Besides, 50ms may be less than a vblank
> > > on
> > > super slow modes that may or may not exist.
> > >
> > > There are some small improvements in PC state residency (due to the
> > > fact that we're now using 16ms for the common modes instead of
> > > 50ms),
> > > but the biggest advantage is still the correctness of being
> > > vblank-based instead of time-based.
> > >
> > > v2:
> > >   - Rebase after changing the patch order.
> > >   - Update the commit message.
> > > v3:
> > >   - Fix bogus vblank_get() instead of vblank_count() (Ville).
> > >   - Don't forget to call drm_crtc_vblank_{get,put} (Chris, Ville)
> > >   - Adjust the performance details on the commit message.
> > > v4:
> > >   - Don't grab the FBC mutex just to grab the vblank (Maarten)
> > >
> > > 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 | 39
> > > +++++++++++++++++++++++++++++----------
> > >  2 files changed, 30 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 204661f..d8f21f0 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -921,9 +921,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 a1988a4..3993b43 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > @@ -381,7 +381,17 @@ 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;
> > > -       int delay_ms = 50;
> > > +       struct drm_vblank_crtc *vblank = &dev_priv->dev-
> > > >vblank[crtc->pipe];
> > > +
> > > +       if (drm_crtc_vblank_get(&crtc->base)) {
> > > +               DRM_ERROR("vblank not available for FBC on pipe
> > > %c\n",
> > > +                         pipe_name(crtc->pipe));
> > > +
> > > +               mutex_lock(&dev_priv->fbc.lock);
> > > +               work->scheduled = false;
> > I couldn't understand this here... doesn't look related to
> > s/time/vblank...
> > could you please explain?
>
> Previously we were just dealing with "wait a certain amount of
> time/jiffies", and for that we can just call the delay/sleep/wait/etc
> calls without needing any get/put calls.
>
> Now we'll wait for a certain number of vblanks, and we need to have the
> vblank interrupts enabled before we can wait for them. That's why we
> have the vblank get/put calls. And since get() can fail, we need an
> error path.
>
> Under normal FBC operation, every vblank get/put call should succeed
> because the pipe's supposed to be running.  But in case we actually
> fail to get vblanks, we just exit from the work function. The way we
> signal "the work function is not running" is by setting work->scheduled
> to false.
>
> >
> > >  +               mutex_unlock(&dev_priv->fbc.lock);
> > > +               return;
> > > +       }
> > >
> > >  retry:
> > >         /* Delay the actual enabling to let pageflipping cease and
> > > the
> > > @@ -390,14 +400,16 @@ 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
> > hm... is it still valid for newer platforms or we should put a if gen
> > <=6 on these checks?
>
> I tested this on BDW some time ago, and it seems we don't actually need
> the vblank wait anymore (although I didn't check the docs if we still
> need the wait). But I wanted to convert the code to use vblanks before
> optimizing it more. And the residency impact won't be big.
>
> >
> > >  +        *
> > > +        * It is also worth mentioning that since work-
> > > >scheduled_vblank can be
> > > +        * updated multiple times by the other threads, hitting the
> > > timeout is
> > > +        * not an error condition. We'll just end up hitting the
> > > "goto retry"
> > > +        * case below.
> > >          */
> > > -       wait_remaining_ms_from_jiffies(work->enable_jiffies,
> > > delay_ms);
> > > +       wait_event_timeout(vblank->queue,
> > > +               drm_crtc_vblank_count(&crtc->base) != work-
> > > >scheduled_vblank,
> > > +               msecs_to_jiffies(50));
> > >
> > >         mutex_lock(&dev_priv->fbc.lock);
> > >
> > > @@ -406,8 +418,7 @@ retry:
> > >                 goto out;
> > >
> > >         /* Were we delayed again while this function was sleeping?
> > > */
> > > -       if (time_after(work->enable_jiffies +
> > > msecs_to_jiffies(delay_ms),
> > > -                      jiffies)) {
> > > +       if (drm_crtc_vblank_count(&crtc->base) == work-
> > > >scheduled_vblank) {
> > >                 mutex_unlock(&dev_priv->fbc.lock);
> > >                 goto retry;
> > >         }
> > > @@ -419,6 +430,7 @@ retry:
> > >
> > >  out:
> > >         mutex_unlock(&dev_priv->fbc.lock);
> > > +       drm_crtc_vblank_put(&crtc->base);
> > >  }
> > >
> > >  static void intel_fbc_cancel_work(struct drm_i915_private
> > > *dev_priv)
> > > @@ -434,13 +446,20 @@ static void
> > > intel_fbc_schedule_activation(struct intel_crtc *crtc)
> > >
> > >         WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
> > >
> > > +       if (drm_crtc_vblank_get(&crtc->base)) {
> > > +               DRM_ERROR("vblank not available for FBC on pipe
> > > %c\n",
> > > +                         pipe_name(crtc->pipe));
> > > +               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;
> > >         work->scheduled = true;
> > > -       work->enable_jiffies = jiffies;
> > > +       work->scheduled_vblank = drm_crtc_vblank_count(&crtc-
> > > >base);
> > > +       drm_crtc_vblank_put(&crtc->base);
> > >
> > >         schedule_work(&work->work);
> > >  }
> > > --
> > > 2.6.4
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > >

[-- Attachment #1.2: Type: text/html, Size: 10771 bytes --]

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

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

Re: [PATCH 01/25] drm/i915/fbc: wait for a vblank instead of 50ms when enabling

[Zanoni, Paulo R]

Em Sex, 2016-01-29 às 00:07 +0000, Rodrigo Vivi escreveu:
> Thanks for all the explanation.
> 
> Makes sense now and everything looks fine for me.
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Thanks Rodrigo and Maarten for the reviews.

The CI part was discussed offline with Daniel, so I just pushed the
patches.

Thanks,
Paulo

> 
> 
> On Tue, Jan 26, 2016 at 10:08 AM Zanoni, Paulo R <paulo.r.zanoni@inte
> l.com> wrote:
> > Em Ter, 2016-01-26 às 17:44 +0000, Rodrigo Vivi escreveu:
> > >
> > >
> > > On Thu, Jan 21, 2016 at 12:03 PM Paulo Zanoni <paulo.r.zanoni@int
> > el.c
> > > om> wrote:
> > > > Instead of waiting for 50ms, just wait until the next vblank,
> > since
> > > > it's the minimum requirement. The whole infrastructure of FBC
> > is
> > > > based
> > > > on vblanks, so waiting for X vblanks instead of X milliseconds
> > > > sounds
> > > > like the correct way to go. Besides, 50ms may be less than a
> > vblank
> > > > on
> > > > super slow modes that may or may not exist.
> > > >
> > > > There are some small improvements in PC state residency (due to
> > the
> > > > fact that we're now using 16ms for the common modes instead of
> > > > 50ms),
> > > > but the biggest advantage is still the correctness of being
> > > > vblank-based instead of time-based.
> > > >
> > > > v2:
> > > >   - Rebase after changing the patch order.
> > > >   - Update the commit message.
> > > > v3:
> > > >   - Fix bogus vblank_get() instead of vblank_count() (Ville).
> > > >   - Don't forget to call drm_crtc_vblank_{get,put} (Chris,
> > Ville)
> > > >   - Adjust the performance details on the commit message.
> > > > v4:
> > > >   - Don't grab the FBC mutex just to grab the vblank (Maarten)
> > > >
> > > > 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 | 39
> > > > +++++++++++++++++++++++++++++----------
> > > >  2 files changed, 30 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 204661f..d8f21f0 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -921,9 +921,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 a1988a4..3993b43 100644
> > > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > > @@ -381,7 +381,17 @@ 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;
> > > > -       int delay_ms = 50;
> > > > +       struct drm_vblank_crtc *vblank = &dev_priv->dev-
> > > > >vblank[crtc->pipe];
> > > > +
> > > > +       if (drm_crtc_vblank_get(&crtc->base)) {
> > > > +               DRM_ERROR("vblank not available for FBC on pipe
> > > > %c\n",
> > > > +                         pipe_name(crtc->pipe));
> > > > +
> > > > +               mutex_lock(&dev_priv->fbc.lock);
> > > > +               work->scheduled = false;
> > > I couldn't understand this here... doesn't look related to
> > > s/time/vblank...
> > > could you please explain?
> > 
> > Previously we were just dealing with "wait a certain amount of
> > time/jiffies", and for that we can just call the
> > delay/sleep/wait/etc
> > calls without needing any get/put calls.
> > 
> > Now we'll wait for a certain number of vblanks, and we need to have
> > the
> > vblank interrupts enabled before we can wait for them. That's why
> > we
> > have the vblank get/put calls. And since get() can fail, we need an
> > error path.
> > 
> > Under normal FBC operation, every vblank get/put call should
> > succeed
> > because the pipe's supposed to be running.  But in case we actually
> > fail to get vblanks, we just exit from the work function. The way
> > we
> > signal "the work function is not running" is by setting work-
> > >scheduled
> > to false.
> > 
> > >
> > > >  +               mutex_unlock(&dev_priv->fbc.lock);
> > > > +               return;
> > > > +       }
> > > >
> > > >  retry:
> > > >         /* Delay the actual enabling to let pageflipping cease
> > and
> > > > the
> > > > @@ -390,14 +400,16 @@ 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
> > > hm... is it still valid for newer platforms or we should put a if
> > gen
> > > <=6 on these checks?
> > 
> > I tested this on BDW some time ago, and it seems we don't actually
> > need
> > the vblank wait anymore (although I didn't check the docs if we
> > still
> > need the wait). But I wanted to convert the code to use vblanks
> > before
> > optimizing it more. And the residency impact won't be big.
> > 
> > >  
> > > >  +        *
> > > > +        * It is also worth mentioning that since work-
> > > > >scheduled_vblank can be
> > > > +        * updated multiple times by the other threads, hitting
> > the
> > > > timeout is
> > > > +        * not an error condition. We'll just end up hitting
> > the
> > > > "goto retry"
> > > > +        * case below.
> > > >          */
> > > > -       wait_remaining_ms_from_jiffies(work->enable_jiffies,
> > > > delay_ms);
> > > > +       wait_event_timeout(vblank->queue,
> > > > +               drm_crtc_vblank_count(&crtc->base) != work-
> > > > >scheduled_vblank,
> > > > +               msecs_to_jiffies(50));
> > > >
> > > >         mutex_lock(&dev_priv->fbc.lock);
> > > >
> > > > @@ -406,8 +418,7 @@ retry:
> > > >                 goto out;
> > > >
> > > >         /* Were we delayed again while this function was
> > sleeping?
> > > > */
> > > > -       if (time_after(work->enable_jiffies +
> > > > msecs_to_jiffies(delay_ms),
> > > > -                      jiffies)) {
> > > > +       if (drm_crtc_vblank_count(&crtc->base) == work-
> > > > >scheduled_vblank) {
> > > >                 mutex_unlock(&dev_priv->fbc.lock);
> > > >                 goto retry;
> > > >         }
> > > > @@ -419,6 +430,7 @@ retry:
> > > >
> > > >  out:
> > > >         mutex_unlock(&dev_priv->fbc.lock);
> > > > +       drm_crtc_vblank_put(&crtc->base);
> > > >  }
> > > >
> > > >  static void intel_fbc_cancel_work(struct drm_i915_private
> > > > *dev_priv)
> > > > @@ -434,13 +446,20 @@ static void
> > > > intel_fbc_schedule_activation(struct intel_crtc *crtc)
> > > >
> > > >         WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
> > > >
> > > > +       if (drm_crtc_vblank_get(&crtc->base)) {
> > > > +               DRM_ERROR("vblank not available for FBC on pipe
> > > > %c\n",
> > > > +                         pipe_name(crtc->pipe));
> > > > +               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;
> > > >         work->scheduled = true;
> > > > -       work->enable_jiffies = jiffies;
> > > > +       work->scheduled_vblank = drm_crtc_vblank_count(&crtc-
> > > > >base);
> > > > +       drm_crtc_vblank_put(&crtc->base);
> > > >
> > > >         schedule_work(&work->work);
> > > >  }
> > > > --
> > > > 2.6.4
> > > >
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/25] drm/i915/fbc: fix the FBC state checking code

[Chris Wilson]

On Tue, Jan 19, 2016 at 11:35:44AM -0200, Paulo Zanoni wrote:
> We'll now call intel_fbc_pre_update instead of intel_fbc_deactivate
> during atomic commits. This will continue to guarantee that we
> deactivate FBC and it will also update the state checking structures
> at the correct time. Then, later, at the point where we were calling
> intel_fbc_update, we'll only need to call intel_fbc_post_update.
> 
> Also add the proper warnings in case we don't have the appropriate
> locks. Daniel mentioned the warnings will have to be removed for async
> commits, but let's keep them here while we can.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 11 +++++------
>  drivers/gpu/drm/i915/intel_drv.h     |  8 +++++---
>  drivers/gpu/drm/i915/intel_fbc.c     | 33 ++++++++++++++++++---------------
>  3 files changed, 28 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index baab41046..baa4cc9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c

> @@ -11733,7 +11733,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  			  to_intel_plane(primary)->frontbuffer_bit);
>  	mutex_unlock(&dev->struct_mutex);
>  
> -	intel_fbc_deactivate(intel_crtc);
> +	intel_fbc_pre_update(intel_crtc);
>  	intel_frontbuffer_flip_prepare(dev,
>  				       to_intel_plane(primary)->frontbuffer_bit);
>  
> +void intel_fbc_pre_update(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>  	struct intel_fbc *fbc = &dev_priv->fbc;
>  
> -	WARN_ON(!mutex_is_locked(&fbc->lock));
> +	if (!fbc_supported(dev_priv))
> +		return;
> +
> +	mutex_lock(&fbc->lock);
>  
>  	if (!multiple_pipes_ok(dev_priv)) {
>  		set_no_fbc_reason(dev_priv, "more than one pipe active");
> @@ -907,15 +915,17 @@ static void intel_fbc_pre_update(struct intel_crtc *crtc)
>  	}
>  
>  	if (!fbc->enabled || fbc->crtc != crtc)
> -		return;
> +		goto unlock;
>  
>  	intel_fbc_update_state_cache(crtc);
>  
>  deactivate:
>  	__intel_fbc_deactivate(dev_priv);
> +unlock:
> +	mutex_unlock(&fbc->lock);
>  }


So this change is broken as intel_fbc_pre_update() depends upon state
derived from the pinned framebuffer object but is being called by
intel_crtc_page_flip() before that object is pinned with
intel_pin_and_fence_fb - i.e. state such as the gtt_offset or the fence
for the object are incorrect, and even trying to look at them can cause
an oops.
-Chris

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

Re: [PATCH 11/25] drm/i915/fbc: fix the FBC state checking code

[Zanoni, Paulo R]

Em Seg, 2016-08-15 às 21:55 +0100, Chris Wilson escreveu:
> On Tue, Jan 19, 2016 at 11:35:44AM -0200, Paulo Zanoni wrote:
> > 
> > We'll now call intel_fbc_pre_update instead of intel_fbc_deactivate
> > during atomic commits. This will continue to guarantee that we
> > deactivate FBC and it will also update the state checking
> > structures
> > at the correct time. Then, later, at the point where we were
> > calling
> > intel_fbc_update, we'll only need to call intel_fbc_post_update.
> > 
> > Also add the proper warnings in case we don't have the appropriate
> > locks. Daniel mentioned the warnings will have to be removed for
> > async
> > commits, but let's keep them here while we can.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 11 +++++------
> >  drivers/gpu/drm/i915/intel_drv.h     |  8 +++++---
> >  drivers/gpu/drm/i915/intel_fbc.c     | 33 ++++++++++++++++++----
> > -----------
> >  3 files changed, 28 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index baab41046..baa4cc9 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> 
> > 
> > @@ -11733,7 +11733,7 @@ static int intel_crtc_page_flip(struct
> > drm_crtc *crtc,
> >  			  to_intel_plane(primary)-
> > >frontbuffer_bit);
> >  	mutex_unlock(&dev->struct_mutex);
> >  
> > -	intel_fbc_deactivate(intel_crtc);
> > +	intel_fbc_pre_update(intel_crtc);
> >  	intel_frontbuffer_flip_prepare(dev,
> >  				       to_intel_plane(primary)-
> > >frontbuffer_bit);
> >  
> > +void intel_fbc_pre_update(struct intel_crtc *crtc)
> >  {
> >  	struct drm_i915_private *dev_priv = crtc->base.dev-
> > >dev_private;
> >  	struct intel_fbc *fbc = &dev_priv->fbc;
> >  
> > -	WARN_ON(!mutex_is_locked(&fbc->lock));
> > +	if (!fbc_supported(dev_priv))
> > +		return;
> > +
> > +	mutex_lock(&fbc->lock);
> >  
> >  	if (!multiple_pipes_ok(dev_priv)) {
> >  		set_no_fbc_reason(dev_priv, "more than one pipe
> > active");
> > @@ -907,15 +915,17 @@ static void intel_fbc_pre_update(struct
> > intel_crtc *crtc)
> >  	}
> >  
> >  	if (!fbc->enabled || fbc->crtc != crtc)
> > -		return;
> > +		goto unlock;
> >  
> >  	intel_fbc_update_state_cache(crtc);
> >  
> >  deactivate:
> >  	__intel_fbc_deactivate(dev_priv);
> > +unlock:
> > +	mutex_unlock(&fbc->lock);
> >  }
> 
> 
> So this change is broken as intel_fbc_pre_update() depends upon state
> derived from the pinned framebuffer object but is being called by
> intel_crtc_page_flip() before that object is pinned with
> intel_pin_and_fence_fb - i.e. state such as the gtt_offset or the
> fence
> for the object are incorrect, and even trying to look at them can
> cause
> an oops.

Nope:
$ git show
1eb52238a5f5b6a3f497b47e6da39ccfebe6b878:drivers/gpu/drm/i915/intel_dis
play.c

Back when the commit was merged, the pre_update() call was done after
pin_and_fence_fb(). It looks like some later commit introduced the
problem you point.

Still, looks like we have code to fix.

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

Re: [PATCH 11/25] drm/i915/fbc: fix the FBC state checking code

[chris]

On Mon, Aug 15, 2016 at 10:47:18PM +0000, Zanoni, Paulo R wrote:
> Em Seg, 2016-08-15 às 21:55 +0100, Chris Wilson escreveu:
> > So this change is broken as intel_fbc_pre_update() depends upon state
> > derived from the pinned framebuffer object but is being called by
> > intel_crtc_page_flip() before that object is pinned with
> > intel_pin_and_fence_fb - i.e. state such as the gtt_offset or the
> > fence
> > for the object are incorrect, and even trying to look at them can
> > cause
> > an oops.
> 
> Nope:
> $ git show
> 1eb52238a5f5b6a3f497b47e6da39ccfebe6b878:drivers/gpu/drm/i915/intel_dis
> play.c
> 
> Back when the commit was merged, the pre_update() call was done after
> pin_and_fence_fb(). It looks like some later commit introduced the
> problem you point.

Hmm, 5a21b6650a239ebc020912968a44047701104159 ?

> Still, looks like we have code to fix.

Well the good news is that answers the question of whether that code is
intended to be safe under the struct_mutex as it once previously was.

So we can just rearrange the code to do the pre_update after the pin?
-Chris

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