[PATCH 0/3] FBC locking

[PATCH 0/3] FBC locking

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Release early, release often!

So patch 1 is not really FBC locking but it's another trivial patch from
December which I wanted to stop carrying around.

Patches 2 and 3 are the real locking thing. I know they could have been just a
single patch, but I decided to split them for bisectability reasons. We probably
shouldn't bisect anything to the "add the FBC mutex" patch, and if we bisect
something to the other patch, it will be easier to fix the problem - or revert -
if we already have fbc.lock. I can squash them if needed, no problem.

Notice that on patch 3 I remove the locking around a lot of calls from
intel_display.c. On a future patch I will completely remove all those calls and
rely only on the frontbuffer tracking infrastructure to update fbc through the
invalidate/flush functions. But before we can do this, we need an additional fix
which I'm already discussing with Daniel about.

The patches pass kms_frontbuffer_testing and kms_fbc_crc without any lockdep
assertions.

Thanks,
Paulo

Paulo Zanoni (3):
  drm/i915: don't increment the FBC threshold at fbc_enable
  drm/i915: add the FBC mutex
  drm/i915: stop using struct_mutex for FBC

 drivers/gpu/drm/i915/i915_debugfs.c    |  10 ++--
 drivers/gpu/drm/i915/i915_drv.h        |   1 +
 drivers/gpu/drm/i915/i915_gem_stolen.c |   7 +++
 drivers/gpu/drm/i915/intel_display.c   |  18 +-----
 drivers/gpu/drm/i915/intel_drv.h       |   1 +
 drivers/gpu/drm/i915/intel_fbc.c       | 101 ++++++++++++++++++++++++++-------
 6 files changed, 99 insertions(+), 39 deletions(-)

-- 
2.1.4

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

[PATCH 1/3] drm/i915: don't increment the FBC threshold at fbc_enable

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

We first set the threshold value when we're allocating the CFB, and
then later at {ilk,gen7}_fbc_enable() we increment it in case we're
using 16bpp. While that is correct, it is dangerous: if we rework the
code a little bit in a way that allows us to call intel_fbc_enable()
without necessarily calling i915_gem_stolen_setup_compression() first,
we might end up incrementing threshold more than once. To prevent
that, increment a temporary variable instead.

v2: Rebase.

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

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 50ed333..9e55b9b 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -188,14 +188,15 @@ static void ilk_fbc_enable(struct drm_crtc *crtc)
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	u32 dpfc_ctl;
+	int threshold = dev_priv->fbc.threshold;
 
 	dev_priv->fbc.enabled = true;
 
 	dpfc_ctl = DPFC_CTL_PLANE(intel_crtc->plane);
 	if (drm_format_plane_cpp(fb->pixel_format, 0) == 2)
-		dev_priv->fbc.threshold++;
+		threshold++;
 
-	switch (dev_priv->fbc.threshold) {
+	switch (threshold) {
 	case 4:
 	case 3:
 		dpfc_ctl |= DPFC_CTL_LIMIT_4X;
@@ -259,6 +260,7 @@ static void gen7_fbc_enable(struct drm_crtc *crtc)
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	u32 dpfc_ctl;
+	int threshold = dev_priv->fbc.threshold;
 
 	dev_priv->fbc.enabled = true;
 
@@ -267,9 +269,9 @@ static void gen7_fbc_enable(struct drm_crtc *crtc)
 		dpfc_ctl |= IVB_DPFC_CTL_PLANE(intel_crtc->plane);
 
 	if (drm_format_plane_cpp(fb->pixel_format, 0) == 2)
-		dev_priv->fbc.threshold++;
+		threshold++;
 
-	switch (dev_priv->fbc.threshold) {
+	switch (threshold) {
 	case 4:
 	case 3:
 		dpfc_ctl |= DPFC_CTL_LIMIT_4X;
-- 
2.1.4

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

[PATCH 2/3] drm/i915: add the FBC mutex

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Make sure we're not gonna have weird races in really weird cases where
a lot of different CRTCs are doing rendering and modesets at the same
time.

v2:
 - Rebase (6 months later)
 - Also lock debugfs and stolen.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c    |  8 ++++
 drivers/gpu/drm/i915/i915_drv.h        |  1 +
 drivers/gpu/drm/i915/i915_gem_stolen.c |  7 ++++
 drivers/gpu/drm/i915/intel_fbc.c       | 73 +++++++++++++++++++++++++++-------
 4 files changed, 75 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index c49fe2a..c99be0e 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1593,6 +1593,7 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
 	}
 
 	intel_runtime_pm_get(dev_priv);
+	mutex_lock(&dev_priv->fbc.lock);
 
 	if (intel_fbc_enabled(dev))
 		seq_puts(m, "FBC enabled\n");
@@ -1605,6 +1606,7 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
 			   yesno(I915_READ(FBC_STATUS2) &
 				 FBC_COMPRESSION_MASK));
 
+	mutex_unlock(&dev_priv->fbc.lock);
 	intel_runtime_pm_put(dev_priv);
 
 	return 0;
@@ -1619,7 +1621,11 @@ static int i915_fbc_fc_get(void *data, u64 *val)
 		return -ENODEV;
 
 	drm_modeset_lock_all(dev);
+	mutex_lock(&dev_priv->fbc.lock);
+
 	*val = dev_priv->fbc.false_color;
+
+	mutex_unlock(&dev_priv->fbc.lock);
 	drm_modeset_unlock_all(dev);
 
 	return 0;
@@ -1635,6 +1641,7 @@ static int i915_fbc_fc_set(void *data, u64 val)
 		return -ENODEV;
 
 	drm_modeset_lock_all(dev);
+	mutex_lock(&dev_priv->fbc.lock);
 
 	reg = I915_READ(ILK_DPFC_CONTROL);
 	dev_priv->fbc.false_color = val;
@@ -1643,6 +1650,7 @@ static int i915_fbc_fc_set(void *data, u64 val)
 		   (reg | FBC_CTL_FALSE_COLOR) :
 		   (reg & ~FBC_CTL_FALSE_COLOR));
 
+	mutex_unlock(&dev_priv->fbc.lock);
 	drm_modeset_unlock_all(dev);
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 491ef0c..9ab04f7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -891,6 +891,7 @@ enum fb_op_origin {
 };
 
 struct i915_fbc {
+	struct mutex lock;
 	unsigned long uncompressed_size;
 	unsigned threshold;
 	unsigned int fb_id;
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 348ed5a..947ddb4 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -250,6 +250,8 @@ int i915_gem_stolen_setup_compression(struct drm_device *dev, int size, int fb_c
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	lockdep_assert_held(&dev_priv->fbc.lock);
+
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
 		return -ENODEV;
 
@@ -266,6 +268,8 @@ void i915_gem_stolen_cleanup_compression(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	lockdep_assert_held(&dev_priv->fbc.lock);
+
 	if (dev_priv->fbc.uncompressed_size == 0)
 		return;
 
@@ -286,7 +290,10 @@ void i915_gem_cleanup_stolen(struct drm_device *dev)
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
 		return;
 
+	mutex_lock(&dev_priv->fbc.lock);
 	i915_gem_stolen_cleanup_compression(dev);
+	mutex_unlock(&dev_priv->fbc.lock);
+
 	drm_mm_takedown(&dev_priv->mm.stolen);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 9e55b9b..31ff88b 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -336,6 +336,7 @@ static void intel_fbc_work_fn(struct work_struct *__work)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	mutex_lock(&dev->struct_mutex);
+	mutex_lock(&dev_priv->fbc.lock);
 	if (work == dev_priv->fbc.fbc_work) {
 		/* Double check that we haven't switched fb without cancelling
 		 * the prior work.
@@ -350,6 +351,7 @@ static void intel_fbc_work_fn(struct work_struct *__work)
 
 		dev_priv->fbc.fbc_work = NULL;
 	}
+	mutex_unlock(&dev_priv->fbc.lock);
 	mutex_unlock(&dev->struct_mutex);
 
 	kfree(work);
@@ -357,6 +359,8 @@ static void intel_fbc_work_fn(struct work_struct *__work)
 
 static void intel_fbc_cancel_work(struct drm_i915_private *dev_priv)
 {
+	lockdep_assert_held(&dev_priv->fbc.lock);
+
 	if (dev_priv->fbc.fbc_work == NULL)
 		return;
 
@@ -384,6 +388,8 @@ static void intel_fbc_enable(struct drm_crtc *crtc)
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	lockdep_assert_held(&dev_priv->fbc.lock);
+
 	if (!dev_priv->display.enable_fbc)
 		return;
 
@@ -418,16 +424,12 @@ static void intel_fbc_enable(struct drm_crtc *crtc)
 	schedule_delayed_work(&work->work, msecs_to_jiffies(50));
 }
 
-/**
- * intel_fbc_disable - disable FBC
- * @dev: the drm_device
- *
- * This function disables FBC.
- */
-void intel_fbc_disable(struct drm_device *dev)
+static void __intel_fbc_disable(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	lockdep_assert_held(&dev_priv->fbc.lock);
+
 	intel_fbc_cancel_work(dev_priv);
 
 	if (!dev_priv->display.disable_fbc)
@@ -437,6 +439,21 @@ void intel_fbc_disable(struct drm_device *dev)
 	dev_priv->fbc.crtc = NULL;
 }
 
+/**
+ * intel_fbc_disable - disable FBC
+ * @dev: the drm_device
+ *
+ * This function disables FBC.
+ */
+void intel_fbc_disable(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	mutex_lock(&dev_priv->fbc.lock);
+	__intel_fbc_disable(dev);
+	mutex_unlock(&dev_priv->fbc.lock);
+}
+
 const char *intel_no_fbc_reason_str(enum no_fbc_reason reason)
 {
 	switch (reason) {
@@ -516,7 +533,7 @@ static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
 }
 
 /**
- * intel_fbc_update - enable/disable FBC as needed
+ * __intel_fbc_update - enable/disable FBC as needed, unlocked
  * @dev: the drm_device
  *
  * Set up the framebuffer compression hardware at mode set time.  We
@@ -534,7 +551,7 @@ static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
  *
  * We need to enable/disable FBC on a global basis.
  */
-void intel_fbc_update(struct drm_device *dev)
+static void __intel_fbc_update(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc = NULL;
@@ -544,6 +561,8 @@ void intel_fbc_update(struct drm_device *dev)
 	const struct drm_display_mode *adjusted_mode;
 	unsigned int max_width, max_height;
 
+	lockdep_assert_held(&dev_priv->fbc.lock);
+
 	if (!HAS_FBC(dev))
 		return;
 
@@ -665,7 +684,7 @@ void intel_fbc_update(struct drm_device *dev)
 		 * some point. And we wait before enabling FBC anyway.
 		 */
 		DRM_DEBUG_KMS("disabling active FBC for update\n");
-		intel_fbc_disable(dev);
+		__intel_fbc_disable(dev);
 	}
 
 	intel_fbc_enable(crtc);
@@ -676,11 +695,26 @@ out_disable:
 	/* Multiple disables should be harmless */
 	if (intel_fbc_enabled(dev)) {
 		DRM_DEBUG_KMS("unsupported config, disabling FBC\n");
-		intel_fbc_disable(dev);
+		__intel_fbc_disable(dev);
 	}
 	i915_gem_stolen_cleanup_compression(dev);
 }
 
+/*
+ * intel_fbc_update - enable/disable FBC as needed
+ * @dev: the drm_device
+ *
+ * This function reevaluates the overall state and enables or disables FBC.
+ */
+void intel_fbc_update(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	mutex_lock(&dev_priv->fbc.lock);
+	__intel_fbc_update(dev);
+	mutex_unlock(&dev_priv->fbc.lock);
+}
+
 void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 			  unsigned int frontbuffer_bits,
 			  enum fb_op_origin origin)
@@ -691,6 +725,8 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 	if (origin == ORIGIN_GTT)
 		return;
 
+	mutex_lock(&dev_priv->fbc.lock);
+
 	if (dev_priv->fbc.enabled)
 		fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe);
 	else if (dev_priv->fbc.fbc_work)
@@ -702,7 +738,9 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 	dev_priv->fbc.busy_bits |= (fbc_bits & frontbuffer_bits);
 
 	if (dev_priv->fbc.busy_bits)
-		intel_fbc_disable(dev);
+		__intel_fbc_disable(dev);
+
+	mutex_unlock(&dev_priv->fbc.lock);
 }
 
 void intel_fbc_flush(struct drm_i915_private *dev_priv,
@@ -710,13 +748,18 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
 {
 	struct drm_device *dev = dev_priv->dev;
 
+	mutex_lock(&dev_priv->fbc.lock);
+
 	if (!dev_priv->fbc.busy_bits)
-		return;
+		goto out;
 
 	dev_priv->fbc.busy_bits &= ~frontbuffer_bits;
 
 	if (!dev_priv->fbc.busy_bits)
-		intel_fbc_update(dev);
+		__intel_fbc_update(dev);
+
+out:
+	mutex_unlock(&dev_priv->fbc.lock);
 }
 
 /**
@@ -729,6 +772,8 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
 {
 	enum pipe pipe;
 
+	mutex_init(&dev_priv->fbc.lock);
+
 	if (!HAS_FBC(dev_priv)) {
 		dev_priv->fbc.enabled = false;
 		dev_priv->fbc.no_fbc_reason = FBC_UNSUPPORTED;
-- 
2.1.4

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

[PATCH 3/3] drm/i915: stop using struct_mutex for FBC

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

We now have dev_priv->fbc.lock and it should be enough to protect
everything related to FBC.

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index c99be0e..d36aa639 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1620,13 +1620,9 @@ static int i915_fbc_fc_get(void *data, u64 *val)
 	if (INTEL_INFO(dev)->gen < 7 || !HAS_FBC(dev))
 		return -ENODEV;
 
-	drm_modeset_lock_all(dev);
 	mutex_lock(&dev_priv->fbc.lock);
-
 	*val = dev_priv->fbc.false_color;
-
 	mutex_unlock(&dev_priv->fbc.lock);
-	drm_modeset_unlock_all(dev);
 
 	return 0;
 }
@@ -1640,7 +1636,6 @@ static int i915_fbc_fc_set(void *data, u64 val)
 	if (INTEL_INFO(dev)->gen < 7 || !HAS_FBC(dev))
 		return -ENODEV;
 
-	drm_modeset_lock_all(dev);
 	mutex_lock(&dev_priv->fbc.lock);
 
 	reg = I915_READ(ILK_DPFC_CONTROL);
@@ -1651,7 +1646,6 @@ static int i915_fbc_fc_set(void *data, u64 val)
 		   (reg & ~FBC_CTL_FALSE_COLOR));
 
 	mutex_unlock(&dev_priv->fbc.lock);
-	drm_modeset_unlock_all(dev);
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3f48917..cf3a86d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4682,9 +4682,7 @@ intel_post_enable_primary(struct drm_crtc *crtc)
 	 */
 	hsw_enable_ips(intel_crtc);
 
-	mutex_lock(&dev->struct_mutex);
 	intel_fbc_update(dev);
-	mutex_unlock(&dev->struct_mutex);
 
 	/*
 	 * Gen2 reports pipe underruns whenever all planes are disabled.
@@ -4740,10 +4738,7 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
 	if (HAS_GMCH_DISPLAY(dev))
 		intel_set_memory_cxsr(dev_priv, false);
 
-	mutex_lock(&dev->struct_mutex);
-	if (dev_priv->fbc.crtc == intel_crtc)
-		intel_fbc_disable(dev);
-	mutex_unlock(&dev->struct_mutex);
+	intel_fbc_disable_crtc(intel_crtc);
 
 	/*
 	 * FIXME IPS should be fine as long as one plane is
@@ -5045,9 +5040,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	intel_crtc->active = false;
 	intel_update_watermarks(crtc);
 
-	mutex_lock(&dev->struct_mutex);
 	intel_fbc_update(dev);
-	mutex_unlock(&dev->struct_mutex);
 }
 
 static void haswell_crtc_disable(struct drm_crtc *crtc)
@@ -5100,9 +5093,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	intel_crtc->active = false;
 	intel_update_watermarks(crtc);
 
-	mutex_lock(&dev->struct_mutex);
 	intel_fbc_update(dev);
-	mutex_unlock(&dev->struct_mutex);
 
 	if (intel_crtc_to_shared_dpll(intel_crtc))
 		intel_disable_shared_dpll(intel_crtc);
@@ -6199,9 +6190,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	intel_crtc->active = false;
 	intel_update_watermarks(crtc);
 
-	mutex_lock(&dev->struct_mutex);
 	intel_fbc_update(dev);
-	mutex_unlock(&dev->struct_mutex);
 }
 
 static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
@@ -13786,11 +13775,8 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 
 	intel_frontbuffer_flip(dev, intel_crtc->atomic.fb_bits);
 
-	if (intel_crtc->atomic.update_fbc) {
-		mutex_lock(&dev->struct_mutex);
+	if (intel_crtc->atomic.update_fbc)
 		intel_fbc_update(dev);
-		mutex_unlock(&dev->struct_mutex);
-	}
 
 	if (intel_crtc->atomic.post_enable_primary)
 		intel_post_enable_primary(crtc);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index bcafefc..ac34041 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1251,6 +1251,7 @@ bool intel_fbc_enabled(struct drm_device *dev);
 void intel_fbc_update(struct drm_device *dev);
 void intel_fbc_init(struct drm_i915_private *dev_priv);
 void intel_fbc_disable(struct drm_device *dev);
+void intel_fbc_disable_crtc(struct intel_crtc *crtc);
 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 31ff88b..ef52847 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -335,7 +335,6 @@ static void intel_fbc_work_fn(struct work_struct *__work)
 	struct drm_device *dev = work->crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	mutex_lock(&dev->struct_mutex);
 	mutex_lock(&dev_priv->fbc.lock);
 	if (work == dev_priv->fbc.fbc_work) {
 		/* Double check that we haven't switched fb without cancelling
@@ -352,7 +351,6 @@ static void intel_fbc_work_fn(struct work_struct *__work)
 		dev_priv->fbc.fbc_work = NULL;
 	}
 	mutex_unlock(&dev_priv->fbc.lock);
-	mutex_unlock(&dev->struct_mutex);
 
 	kfree(work);
 }
@@ -366,7 +364,7 @@ static void intel_fbc_cancel_work(struct drm_i915_private *dev_priv)
 
 	DRM_DEBUG_KMS("cancelling pending FBC enable\n");
 
-	/* Synchronisation is provided by struct_mutex and checking of
+	/* Synchronisation is provided by fbc.lock and checking of
 	 * dev_priv->fbc.fbc_work, so we can perform the cancellation
 	 * entirely asynchronously.
 	 */
@@ -454,6 +452,24 @@ void intel_fbc_disable(struct drm_device *dev)
 	mutex_unlock(&dev_priv->fbc.lock);
 }
 
+/**
+ * intel_fbc_disable_crtc - disable FBC for CRTC
+ * @crtc: the CRTC
+ *
+ * This function is called when the CRTC is being disabled. If it is the FBC
+ * CRTC, then FBC will also be disabled.
+ */
+void intel_fbc_disable_crtc(struct intel_crtc *crtc)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	mutex_lock(&dev_priv->fbc.lock);
+	if (dev_priv->fbc.crtc == crtc)
+		__intel_fbc_disable(dev);
+	mutex_unlock(&dev_priv->fbc.lock);
+}
+
 const char *intel_no_fbc_reason_str(enum no_fbc_reason reason)
 {
 	switch (reason) {
-- 
2.1.4

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

Re: [PATCH 0/3] FBC locking

[Paulo Zanoni]

2001-01-02 4:58 GMT-02:00 Paulo Zanoni <przanoni@gmail.com>:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Release early, release often!
>
> So patch 1 is not really FBC locking but it's another trivial patch from
> December which I wanted to stop carrying around.
>
> Patches 2 and 3 are the real locking thing. I know they could have been just a
> single patch, but I decided to split them for bisectability reasons. We probably
> shouldn't bisect anything to the "add the FBC mutex" patch, and if we bisect
> something to the other patch, it will be easier to fix the problem - or revert -
> if we already have fbc.lock. I can squash them if needed, no problem.
>
> Notice that on patch 3 I remove the locking around a lot of calls from
> intel_display.c. On a future patch I will completely remove all those calls and
> rely only on the frontbuffer tracking infrastructure to update fbc through the
> invalidate/flush functions. But before we can do this, we need an additional fix
> which I'm already discussing with Daniel about.
>
> The patches pass kms_frontbuffer_testing and kms_fbc_crc without any lockdep
> assertions.

For some reason the email came with a date of 2001. So let's bump the
thread to the top of people's inboxes using this reply. Sorry.

>
> Thanks,
> Paulo
>
> Paulo Zanoni (3):
>   drm/i915: don't increment the FBC threshold at fbc_enable
>   drm/i915: add the FBC mutex
>   drm/i915: stop using struct_mutex for FBC
>
>  drivers/gpu/drm/i915/i915_debugfs.c    |  10 ++--
>  drivers/gpu/drm/i915/i915_drv.h        |   1 +
>  drivers/gpu/drm/i915/i915_gem_stolen.c |   7 +++
>  drivers/gpu/drm/i915/intel_display.c   |  18 +-----
>  drivers/gpu/drm/i915/intel_drv.h       |   1 +
>  drivers/gpu/drm/i915/intel_fbc.c       | 101 ++++++++++++++++++++++++++-------
>  6 files changed, 99 insertions(+), 39 deletions(-)
>
> --
> 2.1.4
>



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

Re: [PATCH 2/3] drm/i915: add the FBC mutex

[Chris Wilson]

On Tue, Jan 02, 2001 at 04:58:58AM -0200, Paulo Zanoni wrote:
> @@ -1619,7 +1621,11 @@ static int i915_fbc_fc_get(void *data, u64 *val)
>  		return -ENODEV;
>  
>  	drm_modeset_lock_all(dev);
> +	mutex_lock(&dev_priv->fbc.lock);
> +
>  	*val = dev_priv->fbc.false_color;
> +
> +	mutex_unlock(&dev_priv->fbc.lock);
>  	drm_modeset_unlock_all(dev);

A read of of a single value doesn't need any of these locks.
>  
>  	return 0;

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

Re: [PATCH 3/3] drm/i915: stop using struct_mutex for FBC

[Daniel Vetter]

On Tue, Jan 02, 2001 at 04:58:59AM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We now have dev_priv->fbc.lock and it should be enough to protect
> everything related to FBC.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Managing stolen memory still needs dev->struct_mutex which means you can
only push down the mutex lock/unlock calls down into intel_fbc_update
right around the compressed memory reallocation. Well maybe do even more
and push it down into the fbc functions in i915_gem_stolen.c.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  |  6 ------
>  drivers/gpu/drm/i915/intel_display.c | 18 ++----------------
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  drivers/gpu/drm/i915/intel_fbc.c     | 22 +++++++++++++++++++---
>  4 files changed, 22 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index c99be0e..d36aa639 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1620,13 +1620,9 @@ static int i915_fbc_fc_get(void *data, u64 *val)
>  	if (INTEL_INFO(dev)->gen < 7 || !HAS_FBC(dev))
>  		return -ENODEV;
>  
> -	drm_modeset_lock_all(dev);
>  	mutex_lock(&dev_priv->fbc.lock);
> -
>  	*val = dev_priv->fbc.false_color;
> -
>  	mutex_unlock(&dev_priv->fbc.lock);
> -	drm_modeset_unlock_all(dev);
>  
>  	return 0;
>  }
> @@ -1640,7 +1636,6 @@ static int i915_fbc_fc_set(void *data, u64 val)
>  	if (INTEL_INFO(dev)->gen < 7 || !HAS_FBC(dev))
>  		return -ENODEV;
>  
> -	drm_modeset_lock_all(dev);
>  	mutex_lock(&dev_priv->fbc.lock);
>  
>  	reg = I915_READ(ILK_DPFC_CONTROL);
> @@ -1651,7 +1646,6 @@ static int i915_fbc_fc_set(void *data, u64 val)
>  		   (reg & ~FBC_CTL_FALSE_COLOR));
>  
>  	mutex_unlock(&dev_priv->fbc.lock);
> -	drm_modeset_unlock_all(dev);
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3f48917..cf3a86d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4682,9 +4682,7 @@ intel_post_enable_primary(struct drm_crtc *crtc)
>  	 */
>  	hsw_enable_ips(intel_crtc);
>  
> -	mutex_lock(&dev->struct_mutex);
>  	intel_fbc_update(dev);
> -	mutex_unlock(&dev->struct_mutex);
>  
>  	/*
>  	 * Gen2 reports pipe underruns whenever all planes are disabled.
> @@ -4740,10 +4738,7 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
>  	if (HAS_GMCH_DISPLAY(dev))
>  		intel_set_memory_cxsr(dev_priv, false);
>  
> -	mutex_lock(&dev->struct_mutex);
> -	if (dev_priv->fbc.crtc == intel_crtc)
> -		intel_fbc_disable(dev);
> -	mutex_unlock(&dev->struct_mutex);
> +	intel_fbc_disable_crtc(intel_crtc);
>  
>  	/*
>  	 * FIXME IPS should be fine as long as one plane is
> @@ -5045,9 +5040,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>  	intel_crtc->active = false;
>  	intel_update_watermarks(crtc);
>  
> -	mutex_lock(&dev->struct_mutex);
>  	intel_fbc_update(dev);
> -	mutex_unlock(&dev->struct_mutex);
>  }
>  
>  static void haswell_crtc_disable(struct drm_crtc *crtc)
> @@ -5100,9 +5093,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  	intel_crtc->active = false;
>  	intel_update_watermarks(crtc);
>  
> -	mutex_lock(&dev->struct_mutex);
>  	intel_fbc_update(dev);
> -	mutex_unlock(&dev->struct_mutex);
>  
>  	if (intel_crtc_to_shared_dpll(intel_crtc))
>  		intel_disable_shared_dpll(intel_crtc);
> @@ -6199,9 +6190,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>  	intel_crtc->active = false;
>  	intel_update_watermarks(crtc);
>  
> -	mutex_lock(&dev->struct_mutex);
>  	intel_fbc_update(dev);
> -	mutex_unlock(&dev->struct_mutex);
>  }
>  
>  static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
> @@ -13786,11 +13775,8 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
>  
>  	intel_frontbuffer_flip(dev, intel_crtc->atomic.fb_bits);
>  
> -	if (intel_crtc->atomic.update_fbc) {
> -		mutex_lock(&dev->struct_mutex);
> +	if (intel_crtc->atomic.update_fbc)
>  		intel_fbc_update(dev);
> -		mutex_unlock(&dev->struct_mutex);
> -	}
>  
>  	if (intel_crtc->atomic.post_enable_primary)
>  		intel_post_enable_primary(crtc);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index bcafefc..ac34041 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1251,6 +1251,7 @@ bool intel_fbc_enabled(struct drm_device *dev);
>  void intel_fbc_update(struct drm_device *dev);
>  void intel_fbc_init(struct drm_i915_private *dev_priv);
>  void intel_fbc_disable(struct drm_device *dev);
> +void intel_fbc_disable_crtc(struct intel_crtc *crtc);
>  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 31ff88b..ef52847 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -335,7 +335,6 @@ static void intel_fbc_work_fn(struct work_struct *__work)
>  	struct drm_device *dev = work->crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	mutex_lock(&dev->struct_mutex);
>  	mutex_lock(&dev_priv->fbc.lock);
>  	if (work == dev_priv->fbc.fbc_work) {
>  		/* Double check that we haven't switched fb without cancelling
> @@ -352,7 +351,6 @@ static void intel_fbc_work_fn(struct work_struct *__work)
>  		dev_priv->fbc.fbc_work = NULL;
>  	}
>  	mutex_unlock(&dev_priv->fbc.lock);
> -	mutex_unlock(&dev->struct_mutex);
>  
>  	kfree(work);
>  }
> @@ -366,7 +364,7 @@ static void intel_fbc_cancel_work(struct drm_i915_private *dev_priv)
>  
>  	DRM_DEBUG_KMS("cancelling pending FBC enable\n");
>  
> -	/* Synchronisation is provided by struct_mutex and checking of
> +	/* Synchronisation is provided by fbc.lock and checking of
>  	 * dev_priv->fbc.fbc_work, so we can perform the cancellation
>  	 * entirely asynchronously.
>  	 */
> @@ -454,6 +452,24 @@ void intel_fbc_disable(struct drm_device *dev)
>  	mutex_unlock(&dev_priv->fbc.lock);
>  }
>  
> +/**
> + * intel_fbc_disable_crtc - disable FBC for CRTC
> + * @crtc: the CRTC
> + *
> + * This function is called when the CRTC is being disabled. If it is the FBC
> + * CRTC, then FBC will also be disabled.
> + */
> +void intel_fbc_disable_crtc(struct intel_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	mutex_lock(&dev_priv->fbc.lock);
> +	if (dev_priv->fbc.crtc == crtc)
> +		__intel_fbc_disable(dev);
> +	mutex_unlock(&dev_priv->fbc.lock);
> +}
> +
>  const char *intel_no_fbc_reason_str(enum no_fbc_reason reason)
>  {
>  	switch (reason) {
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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 2/3] drm/i915: add the FBC mutex

[Chris Wilson]

On Tue, Jan 02, 2001 at 04:58:58AM -0200, Paulo Zanoni wrote:
>  void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
>  			  unsigned int frontbuffer_bits,
>  			  enum fb_op_origin origin)
> @@ -691,6 +725,8 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
>  	if (origin == ORIGIN_GTT)
>  		return;
>  
> +	mutex_lock(&dev_priv->fbc.lock);
> +
>  	if (dev_priv->fbc.enabled)
>  		fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe);
>  	else if (dev_priv->fbc.fbc_work)
> @@ -702,7 +738,9 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
>  	dev_priv->fbc.busy_bits |= (fbc_bits & frontbuffer_bits);
>  
>  	if (dev_priv->fbc.busy_bits)
> -		intel_fbc_disable(dev);
> +		__intel_fbc_disable(dev);
> +
> +	mutex_unlock(&dev_priv->fbc.lock);
>  }
>  
>  void intel_fbc_flush(struct drm_i915_private *dev_priv,
> @@ -710,13 +748,18 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
>  {
>  	struct drm_device *dev = dev_priv->dev;
>  
> +	mutex_lock(&dev_priv->fbc.lock);
> +
>  	if (!dev_priv->fbc.busy_bits)
> -		return;
> +		goto out;
>  
>  	dev_priv->fbc.busy_bits &= ~frontbuffer_bits;
>  
>  	if (!dev_priv->fbc.busy_bits)
> -		intel_fbc_update(dev);
> +		__intel_fbc_update(dev);
> +
> +out:
> +	mutex_unlock(&dev_priv->fbc.lock);
>  }

These busy bits are locked higher up. In fact I want to migrate that
lock to a spinlock, which has implications here. I didn't see anything
that mandated using a mutex for fbc, right?
-Chris

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

Re: [PATCH 2/3] drm/i915: add the FBC mutex

[Daniel Vetter]

On Tue, Jan 02, 2001 at 04:58:58AM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Make sure we're not gonna have weird races in really weird cases where
> a lot of different CRTCs are doing rendering and modesets at the same
> time.
> 
> v2:
>  - Rebase (6 months later)
>  - Also lock debugfs and stolen.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c    |  8 ++++
>  drivers/gpu/drm/i915/i915_drv.h        |  1 +
>  drivers/gpu/drm/i915/i915_gem_stolen.c |  7 ++++
>  drivers/gpu/drm/i915/intel_fbc.c       | 73 +++++++++++++++++++++++++++-------
>  4 files changed, 75 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index c49fe2a..c99be0e 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1593,6 +1593,7 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
>  	}
>  
>  	intel_runtime_pm_get(dev_priv);
> +	mutex_lock(&dev_priv->fbc.lock);
>  
>  	if (intel_fbc_enabled(dev))
>  		seq_puts(m, "FBC enabled\n");
> @@ -1605,6 +1606,7 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
>  			   yesno(I915_READ(FBC_STATUS2) &
>  				 FBC_COMPRESSION_MASK));
>  
> +	mutex_unlock(&dev_priv->fbc.lock);
>  	intel_runtime_pm_put(dev_priv);
>  
>  	return 0;
> @@ -1619,7 +1621,11 @@ static int i915_fbc_fc_get(void *data, u64 *val)
>  		return -ENODEV;
>  
>  	drm_modeset_lock_all(dev);

Modeset locks aren't required any more here. That was just to prevent
concurrent modesets, but if we have a new unified fbc lock for all of this
it's not needed any more.

> +	mutex_lock(&dev_priv->fbc.lock);
> +
>  	*val = dev_priv->fbc.false_color;
> +
> +	mutex_unlock(&dev_priv->fbc.lock);
>  	drm_modeset_unlock_all(dev);
>  
>  	return 0;
> @@ -1635,6 +1641,7 @@ static int i915_fbc_fc_set(void *data, u64 val)
>  		return -ENODEV;
>  
>  	drm_modeset_lock_all(dev);

Same.

> +	mutex_lock(&dev_priv->fbc.lock);
>  
>  	reg = I915_READ(ILK_DPFC_CONTROL);
>  	dev_priv->fbc.false_color = val;
> @@ -1643,6 +1650,7 @@ static int i915_fbc_fc_set(void *data, u64 val)
>  		   (reg | FBC_CTL_FALSE_COLOR) :
>  		   (reg & ~FBC_CTL_FALSE_COLOR));
>  
> +	mutex_unlock(&dev_priv->fbc.lock);
>  	drm_modeset_unlock_all(dev);
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 491ef0c..9ab04f7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -891,6 +891,7 @@ enum fb_op_origin {
>  };
>  
>  struct i915_fbc {
> +	struct mutex lock;
>  	unsigned long uncompressed_size;
>  	unsigned threshold;
>  	unsigned int fb_id;
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 348ed5a..947ddb4 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -250,6 +250,8 @@ int i915_gem_stolen_setup_compression(struct drm_device *dev, int size, int fb_c
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> +	lockdep_assert_held(&dev_priv->fbc.lock);

Please use WARN_ON(!mutex_locked). lockdep_assert_held is a no-op when
lockdep is compiled out. And the benefit of also checking that the current
task is the lock owner is imo negligible (it only matters when you have an
actual race, which almost never happens).

Also see my comment on the next patch, stolen memory management is protect
by dev->struct_mutex actually.

Locking looks good otherwise, but I guess we need to re-audit all this
once it's all settled down into the new design.

> +
>  	if (!drm_mm_initialized(&dev_priv->mm.stolen))
>  		return -ENODEV;
>  
> @@ -266,6 +268,8 @@ void i915_gem_stolen_cleanup_compression(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> +	lockdep_assert_held(&dev_priv->fbc.lock);
> +
>  	if (dev_priv->fbc.uncompressed_size == 0)
>  		return;
>  
> @@ -286,7 +290,10 @@ void i915_gem_cleanup_stolen(struct drm_device *dev)
>  	if (!drm_mm_initialized(&dev_priv->mm.stolen))
>  		return;
>  
> +	mutex_lock(&dev_priv->fbc.lock);
>  	i915_gem_stolen_cleanup_compression(dev);
> +	mutex_unlock(&dev_priv->fbc.lock);
> +
>  	drm_mm_takedown(&dev_priv->mm.stolen);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 9e55b9b..31ff88b 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -336,6 +336,7 @@ static void intel_fbc_work_fn(struct work_struct *__work)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	mutex_lock(&dev->struct_mutex);
> +	mutex_lock(&dev_priv->fbc.lock);
>  	if (work == dev_priv->fbc.fbc_work) {
>  		/* Double check that we haven't switched fb without cancelling
>  		 * the prior work.
> @@ -350,6 +351,7 @@ static void intel_fbc_work_fn(struct work_struct *__work)
>  
>  		dev_priv->fbc.fbc_work = NULL;
>  	}
> +	mutex_unlock(&dev_priv->fbc.lock);
>  	mutex_unlock(&dev->struct_mutex);
>  
>  	kfree(work);
> @@ -357,6 +359,8 @@ static void intel_fbc_work_fn(struct work_struct *__work)
>  
>  static void intel_fbc_cancel_work(struct drm_i915_private *dev_priv)
>  {
> +	lockdep_assert_held(&dev_priv->fbc.lock);

Small note for the future: The async worker cancelling scheme used by fbc
doesn't match what psr/drrs use - they all cancel the work synchronously,
but outside of any locked critical section. We couldn't do that with fbc
because dev->struct_mutex is too big. But with the new locking it might
make sense to switch to the same design for consistency.

But we can only do that once fbc looks more like psr/drrs with dedicated
enable/disable, and that's still a way off.

Cheers, Daniel

> +
>  	if (dev_priv->fbc.fbc_work == NULL)
>  		return;
>  
> @@ -384,6 +388,8 @@ static void intel_fbc_enable(struct drm_crtc *crtc)
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> +	lockdep_assert_held(&dev_priv->fbc.lock);
> +
>  	if (!dev_priv->display.enable_fbc)
>  		return;
>  
> @@ -418,16 +424,12 @@ static void intel_fbc_enable(struct drm_crtc *crtc)
>  	schedule_delayed_work(&work->work, msecs_to_jiffies(50));
>  }
>  
> -/**
> - * intel_fbc_disable - disable FBC
> - * @dev: the drm_device
> - *
> - * This function disables FBC.
> - */
> -void intel_fbc_disable(struct drm_device *dev)
> +static void __intel_fbc_disable(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> +	lockdep_assert_held(&dev_priv->fbc.lock);
> +
>  	intel_fbc_cancel_work(dev_priv);
>  
>  	if (!dev_priv->display.disable_fbc)
> @@ -437,6 +439,21 @@ void intel_fbc_disable(struct drm_device *dev)
>  	dev_priv->fbc.crtc = NULL;
>  }
>  
> +/**
> + * intel_fbc_disable - disable FBC
> + * @dev: the drm_device
> + *
> + * This function disables FBC.
> + */
> +void intel_fbc_disable(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	mutex_lock(&dev_priv->fbc.lock);
> +	__intel_fbc_disable(dev);
> +	mutex_unlock(&dev_priv->fbc.lock);
> +}
> +
>  const char *intel_no_fbc_reason_str(enum no_fbc_reason reason)
>  {
>  	switch (reason) {
> @@ -516,7 +533,7 @@ static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
>  }
>  
>  /**
> - * intel_fbc_update - enable/disable FBC as needed
> + * __intel_fbc_update - enable/disable FBC as needed, unlocked
>   * @dev: the drm_device
>   *
>   * Set up the framebuffer compression hardware at mode set time.  We
> @@ -534,7 +551,7 @@ static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
>   *
>   * We need to enable/disable FBC on a global basis.
>   */
> -void intel_fbc_update(struct drm_device *dev)
> +static void __intel_fbc_update(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *crtc = NULL;
> @@ -544,6 +561,8 @@ void intel_fbc_update(struct drm_device *dev)
>  	const struct drm_display_mode *adjusted_mode;
>  	unsigned int max_width, max_height;
>  
> +	lockdep_assert_held(&dev_priv->fbc.lock);
> +
>  	if (!HAS_FBC(dev))
>  		return;
>  
> @@ -665,7 +684,7 @@ void intel_fbc_update(struct drm_device *dev)
>  		 * some point. And we wait before enabling FBC anyway.
>  		 */
>  		DRM_DEBUG_KMS("disabling active FBC for update\n");
> -		intel_fbc_disable(dev);
> +		__intel_fbc_disable(dev);
>  	}
>  
>  	intel_fbc_enable(crtc);
> @@ -676,11 +695,26 @@ out_disable:
>  	/* Multiple disables should be harmless */
>  	if (intel_fbc_enabled(dev)) {
>  		DRM_DEBUG_KMS("unsupported config, disabling FBC\n");
> -		intel_fbc_disable(dev);
> +		__intel_fbc_disable(dev);
>  	}
>  	i915_gem_stolen_cleanup_compression(dev);
>  }
>  
> +/*
> + * intel_fbc_update - enable/disable FBC as needed
> + * @dev: the drm_device
> + *
> + * This function reevaluates the overall state and enables or disables FBC.
> + */
> +void intel_fbc_update(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	mutex_lock(&dev_priv->fbc.lock);
> +	__intel_fbc_update(dev);
> +	mutex_unlock(&dev_priv->fbc.lock);
> +}
> +
>  void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
>  			  unsigned int frontbuffer_bits,
>  			  enum fb_op_origin origin)
> @@ -691,6 +725,8 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
>  	if (origin == ORIGIN_GTT)
>  		return;
>  
> +	mutex_lock(&dev_priv->fbc.lock);
> +
>  	if (dev_priv->fbc.enabled)
>  		fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe);
>  	else if (dev_priv->fbc.fbc_work)
> @@ -702,7 +738,9 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
>  	dev_priv->fbc.busy_bits |= (fbc_bits & frontbuffer_bits);
>  
>  	if (dev_priv->fbc.busy_bits)
> -		intel_fbc_disable(dev);
> +		__intel_fbc_disable(dev);
> +
> +	mutex_unlock(&dev_priv->fbc.lock);
>  }
>  
>  void intel_fbc_flush(struct drm_i915_private *dev_priv,
> @@ -710,13 +748,18 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
>  {
>  	struct drm_device *dev = dev_priv->dev;
>  
> +	mutex_lock(&dev_priv->fbc.lock);
> +
>  	if (!dev_priv->fbc.busy_bits)
> -		return;
> +		goto out;
>  
>  	dev_priv->fbc.busy_bits &= ~frontbuffer_bits;
>  
>  	if (!dev_priv->fbc.busy_bits)
> -		intel_fbc_update(dev);
> +		__intel_fbc_update(dev);
> +
> +out:
> +	mutex_unlock(&dev_priv->fbc.lock);
>  }
>  
>  /**
> @@ -729,6 +772,8 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
>  {
>  	enum pipe pipe;
>  
> +	mutex_init(&dev_priv->fbc.lock);
> +
>  	if (!HAS_FBC(dev_priv)) {
>  		dev_priv->fbc.enabled = false;
>  		dev_priv->fbc.no_fbc_reason = FBC_UNSUPPORTED;
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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 2/3] drm/i915: add the FBC mutex

[Paulo Zanoni]

2015-06-17 4:52 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Tue, Jan 02, 2001 at 04:58:58AM -0200, Paulo Zanoni wrote:
>>  void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
>>                         unsigned int frontbuffer_bits,
>>                         enum fb_op_origin origin)
>> @@ -691,6 +725,8 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
>>       if (origin == ORIGIN_GTT)
>>               return;
>>
>> +     mutex_lock(&dev_priv->fbc.lock);
>> +
>>       if (dev_priv->fbc.enabled)
>>               fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe);
>>       else if (dev_priv->fbc.fbc_work)
>> @@ -702,7 +738,9 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
>>       dev_priv->fbc.busy_bits |= (fbc_bits & frontbuffer_bits);
>>
>>       if (dev_priv->fbc.busy_bits)
>> -             intel_fbc_disable(dev);
>> +             __intel_fbc_disable(dev);
>> +
>> +     mutex_unlock(&dev_priv->fbc.lock);
>>  }
>>
>>  void intel_fbc_flush(struct drm_i915_private *dev_priv,
>> @@ -710,13 +748,18 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
>>  {
>>       struct drm_device *dev = dev_priv->dev;
>>
>> +     mutex_lock(&dev_priv->fbc.lock);
>> +
>>       if (!dev_priv->fbc.busy_bits)
>> -             return;
>> +             goto out;
>>
>>       dev_priv->fbc.busy_bits &= ~frontbuffer_bits;
>>
>>       if (!dev_priv->fbc.busy_bits)
>> -             intel_fbc_update(dev);
>> +             __intel_fbc_update(dev);
>> +
>> +out:
>> +     mutex_unlock(&dev_priv->fbc.lock);
>>  }
>
> These busy bits are locked higher up. In fact I want to migrate that
> lock to a spinlock, which has implications here. I didn't see anything
> that mandated using a mutex for fbc, right?

I didn't understand your idea. You want to replace the whole FBC mutex
for a spinlock? Why?

Please notice that we have dev_priv->fbc.busy_bits and also
dev_priv->fb_tracking.busy_bits. The FBC busy bits are only handled in
the intel_fbc.c functions. So maybe you want the spilock around the
fb_tracking ones? That wouldn't require changing the FBC mutex to a
spinlock, and it could be done today.

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



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

Re: [PATCH 2/3] drm/i915: add the FBC mutex

[Chris Wilson]

On Wed, Jun 17, 2015 at 04:39:32PM -0300, Paulo Zanoni wrote:
> 2015-06-17 4:52 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > These busy bits are locked higher up. In fact I want to migrate that
> > lock to a spinlock, which has implications here. I didn't see anything
> > that mandated using a mutex for fbc, right?
> 
> I didn't understand your idea. You want to replace the whole FBC mutex
> for a spinlock? Why?

I want to replace the frontbuffer mutex with a spinlock. You are
inserting a mutex under my intended spinlock, which blows my idea of
trying to speed up the normal operations.
 
> Please notice that we have dev_priv->fbc.busy_bits and also
> dev_priv->fb_tracking.busy_bits. The FBC busy bits are only handled in
> the intel_fbc.c functions. So maybe you want the spilock around the
> fb_tracking ones? That wouldn't require changing the FBC mutex to a
> spinlock, and it could be done today.

Somehow I need to avoid the mutex here, so kicking off the fbc
enable/disable needs to be lockless (or spinlocked at most). Of course,
if that is not practical, I will just have to live with not converting
the higher mutex into the spinlock.
-Chris

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

Re: [PATCH 2/3] drm/i915: add the FBC mutex

[Paulo Zanoni]

2015-06-17 17:25 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Wed, Jun 17, 2015 at 04:39:32PM -0300, Paulo Zanoni wrote:
>> 2015-06-17 4:52 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
>> > These busy bits are locked higher up. In fact I want to migrate that
>> > lock to a spinlock, which has implications here. I didn't see anything
>> > that mandated using a mutex for fbc, right?
>>
>> I didn't understand your idea. You want to replace the whole FBC mutex
>> for a spinlock? Why?
>
> I want to replace the frontbuffer mutex with a spinlock. You are
> inserting a mutex under my intended spinlock, which blows my idea of
> trying to speed up the normal operations.

As far as I can see, fb_tracking.lock is not held when we call
intel_fbc_flush and intel_fbc_invalidate. So your change could be
possible now.

>
>> Please notice that we have dev_priv->fbc.busy_bits and also
>> dev_priv->fb_tracking.busy_bits. The FBC busy bits are only handled in
>> the intel_fbc.c functions. So maybe you want the spilock around the
>> fb_tracking ones? That wouldn't require changing the FBC mutex to a
>> spinlock, and it could be done today.
>
> Somehow I need to avoid the mutex here, so kicking off the fbc
> enable/disable needs to be lockless (or spinlocked at most). Of course,
> if that is not practical, I will just have to live with not converting
> the higher mutex into the spinlock.

I think we will reach a point where this doable. After we migrate some
of the FBC stuff to pipe_config, and make FBC fully rely on the
frontbuffer tracking infrastructure (i.e., no intel_fbc_something
calls from intel_display.c), the FBC update/flush/invalidate functions
will get much simpler, not needing to handle stolen memory, etc.
Another current problem I can see is the delayed work stuff we have,
but Daniel is already requesting to simplify/remove it, so that might
die too. I'll try to keep the spinlock in mind when thinking about
this. We can always try :). I hope you're fine with the idea of just
converting the mutex to a spinlock later.

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



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

Re: [PATCH 2/3] drm/i915: add the FBC mutex

[Chris Wilson]

On Wed, Jun 17, 2015 at 05:47:10PM -0300, Paulo Zanoni wrote:
> 2015-06-17 17:25 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Wed, Jun 17, 2015 at 04:39:32PM -0300, Paulo Zanoni wrote:
> >> 2015-06-17 4:52 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> >> > These busy bits are locked higher up. In fact I want to migrate that
> >> > lock to a spinlock, which has implications here. I didn't see anything
> >> > that mandated using a mutex for fbc, right?
> >>
> >> I didn't understand your idea. You want to replace the whole FBC mutex
> >> for a spinlock? Why?
> >
> > I want to replace the frontbuffer mutex with a spinlock. You are
> > inserting a mutex under my intended spinlock, which blows my idea of
> > trying to speed up the normal operations.
> 
> As far as I can see, fb_tracking.lock is not held when we call
> intel_fbc_flush and intel_fbc_invalidate. So your change could be
> possible now.

Hmm, right. My bad, I should have checked the extents of the lock first!

Thanks,
-Chris

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