[PATCH 0/8] FBC locking v3

[PATCH 0/8] FBC locking v3

[Paulo Zanoni]

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

Hi

So this is a new take on the FBC locking improvements. This time I continue with
the "introduce fbc.lock first, then reduce struct_mutex later" strategy in order
to minimize the risks and facilitate review.

I also fixed a "lack of struct_mutex lock" regression I introduced when I first
moved FBC to the frontbuffer tracking infrastructure.

Right now the only places where we should be holding the struct_mutex lock for
FBC is on the 2 intel_fbc_update() calls: one at intel_post_plane_update() and
another at intel_unpin_work_fn(). Notice that we'll probably remove the
intel_fbc_update() call that's inside intel_unpin_work_fn() later, so we'll only
required struct_mutex locking once. We can't get rid of the single struct_mutex
lock requirement since we need to deal with the CFB allocation. Of course we can
always rework that code a little bit more, but let's do it on later patches.

Thanks,
Paulo

Paulo Zanoni (8):
  drm/i915: don't increment the FBC threshold at fbc_enable
  drm/i915: add the FBC mutex
  drm/i915: remove unneded locks on debugs FBC functions
  drm/i915: remove struct_mutex lock from the FBC work function
  drm/i915: simplify FBC start/stop at invalidate/flush
  drm/i915: add struct_mutex WARNs to i915_gem_stolen.c
  drm/i915: reduce struct_mutex coverage at intel_crtc_page_flip()
  drm/i915: remove struct_mutex lock from intel_modeset_cleanup()

 drivers/gpu/drm/i915/i915_debugfs.c    |   8 +--
 drivers/gpu/drm/i915/i915_drv.h        |   1 +
 drivers/gpu/drm/i915/i915_gem_stolen.c |  20 ++++++
 drivers/gpu/drm/i915/intel_display.c   |  16 +----
 drivers/gpu/drm/i915/intel_drv.h       |   1 +
 drivers/gpu/drm/i915/intel_fbc.c       | 110 +++++++++++++++++++++++++++------
 6 files changed, 120 insertions(+), 36 deletions(-)

-- 
2.1.4

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

[PATCH 1/8] 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/8] drm/i915: add the FBC mutex

[Paulo Zanoni]

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

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

With this change, we can start removing the struct_mutex locking we
have around FBC in the next patches. The only struct_mutex requirement
will be around intel_fbc_update(), since it's the place that does the
CFB stolen memory allocations.

v2:
 - Rebase (6 months later)
 - Also lock debugfs and stolen.
v3:
 - Don't lock a single value read (Chris).
 - Replace lockdep assertions with WARNs (Daniel).
 - Improve commit message.
 - Don't forget intel_pre_plane_update() locking.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c    |  4 ++
 drivers/gpu/drm/i915/i915_drv.h        |  1 +
 drivers/gpu/drm/i915/i915_gem_stolen.c |  7 +++
 drivers/gpu/drm/i915/intel_display.c   | 10 +---
 drivers/gpu/drm/i915/intel_drv.h       |  1 +
 drivers/gpu/drm/i915/intel_fbc.c       | 86 +++++++++++++++++++++++++++++-----
 6 files changed, 89 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 31d8768..5248ed9 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1619,6 +1619,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");
@@ -1631,6 +1632,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;
@@ -1661,6 +1663,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;
@@ -1669,6 +1672,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 ea9caf2..1e17722 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -892,6 +892,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..793bcba 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;
 
+	WARN_ON(!mutex_is_locked(&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;
 
+	WARN_ON(!mutex_is_locked(&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_display.c b/drivers/gpu/drm/i915/intel_display.c
index eb665d7..eab5aa1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4741,7 +4741,6 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
 static void intel_pre_plane_update(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
 	struct drm_plane *p;
 
@@ -4758,13 +4757,8 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
 	if (atomic->wait_for_flips)
 		intel_crtc_wait_for_pending_flips(&crtc->base);
 
-	if (atomic->disable_fbc &&
-	    dev_priv->fbc.crtc == crtc) {
-		mutex_lock(&dev->struct_mutex);
-		if (dev_priv->fbc.crtc == crtc)
-			intel_fbc_disable(dev);
-		mutex_unlock(&dev->struct_mutex);
-	}
+	if (atomic->disable_fbc)
+		intel_fbc_disable_crtc(crtc);
 
 	if (crtc->atomic.disable_ips)
 		hsw_disable_ips(crtc);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 33cff9d..00eea90 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1236,6 +1236,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 9e55b9b..543b7a2 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)
 {
+	WARN_ON(!mutex_is_locked(&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;
 
+	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
+
 	if (!dev_priv->display.enable_fbc)
 		return;
 
@@ -418,6 +424,21 @@ static void intel_fbc_enable(struct drm_crtc *crtc)
 	schedule_delayed_work(&work->work, msecs_to_jiffies(50));
 }
 
+static void __intel_fbc_disable(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
+
+	intel_fbc_cancel_work(dev_priv);
+
+	if (!dev_priv->display.disable_fbc)
+		return;
+
+	dev_priv->display.disable_fbc(dev);
+	dev_priv->fbc.crtc = NULL;
+}
+
 /**
  * intel_fbc_disable - disable FBC
  * @dev: the drm_device
@@ -428,13 +449,26 @@ void intel_fbc_disable(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	intel_fbc_cancel_work(dev_priv);
+	mutex_lock(&dev_priv->fbc.lock);
+	__intel_fbc_disable(dev);
+	mutex_unlock(&dev_priv->fbc.lock);
+}
 
-	if (!dev_priv->display.disable_fbc)
-		return;
+/*
+ * intel_fbc_disable_crtc - disable FBC if it's associated with crtc
+ * @crtc: the CRTC
+ *
+ * This function disables FBC if it's associated with the provided CRTC.
+ */
+void intel_fbc_disable_crtc(struct intel_crtc *crtc)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	dev_priv->display.disable_fbc(dev);
-	dev_priv->fbc.crtc = NULL;
+	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)
@@ -516,7 +550,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 +568,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 +578,8 @@ void intel_fbc_update(struct drm_device *dev)
 	const struct drm_display_mode *adjusted_mode;
 	unsigned int max_width, max_height;
 
+	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
+
 	if (!HAS_FBC(dev))
 		return;
 
@@ -665,7 +701,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 +712,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 +742,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 +755,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 +765,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 +789,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/8] drm/i915: remove unneded locks on debugs FBC functions

[Paulo Zanoni]

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

The first one is helpless against a single variable read. The second
one is not needed anymore now that we have fbc.lock.

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 5248ed9..4de083b 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1646,9 +1646,7 @@ 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);
 	*val = dev_priv->fbc.false_color;
-	drm_modeset_unlock_all(dev);
 
 	return 0;
 }
@@ -1662,7 +1660,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);
@@ -1673,7 +1670,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;
 }
 
-- 
2.1.4

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

[PATCH 4/8] drm/i915: remove struct_mutex lock from the FBC work function

[Paulo Zanoni]

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

Now that we have the FBC mutex to protect the actual FBC data, we can
remove the struct_mutex from here since we're not dealing with stolen
memory at this point.

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

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 543b7a2..316feb1 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);
 }
-- 
2.1.4

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

[PATCH 5/8] drm/i915: simplify FBC start/stop at invalidate/flush

[Paulo Zanoni]

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

The problem with calling intel_fbc_update() at flush is that it fully
rechecks and recomputes the FBC state, and that includes reallocating
the CFB, which requires a struct_mutex lock that we don't always have.

The lack of struct_mutex lock can be considered a regression from:

commit dbef0f15b5c83231dacb214dbf9a6dba063ca21c
Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
Date:   Fri Feb 13 17:23:46 2015 -0200
    drm/i915: add frontbuffer tracking to FBC

So introduce intel_fbc_stop() that doesn't unset fbc.crtc, then call
stop/enable at invalidate/flush.

Notice that invalidate/flush is only called by the frontbuffer
tracking infrastrucutre, so it's safe to not do a full
intel_fbc_update() here.

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

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 316feb1..0a66814 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -422,7 +422,7 @@ static void intel_fbc_enable(struct drm_crtc *crtc)
 	schedule_delayed_work(&work->work, msecs_to_jiffies(50));
 }
 
-static void __intel_fbc_disable(struct drm_device *dev)
+static void intel_fbc_stop(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -434,6 +434,13 @@ static void __intel_fbc_disable(struct drm_device *dev)
 		return;
 
 	dev_priv->display.disable_fbc(dev);
+}
+
+static void __intel_fbc_disable(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	intel_fbc_stop(dev);
 	dev_priv->fbc.crtc = NULL;
 }
 
@@ -753,7 +760,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 	dev_priv->fbc.busy_bits |= (fbc_bits & frontbuffer_bits);
 
 	if (dev_priv->fbc.busy_bits)
-		__intel_fbc_disable(dev);
+		intel_fbc_stop(dev);
 
 	mutex_unlock(&dev_priv->fbc.lock);
 }
@@ -770,8 +777,11 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
 
 	dev_priv->fbc.busy_bits &= ~frontbuffer_bits;
 
-	if (!dev_priv->fbc.busy_bits)
-		__intel_fbc_update(dev);
+	if (!dev_priv->fbc.busy_bits && dev_priv->fbc.crtc) {
+		if (dev_priv->fbc.enabled)
+			intel_fbc_stop(dev);
+		intel_fbc_enable(&dev_priv->fbc.crtc->base);
+	}
 
 out:
 	mutex_unlock(&dev_priv->fbc.lock);
-- 
2.1.4

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

[PATCH 6/8] drm/i915: add struct_mutex WARNs to i915_gem_stolen.c

[Paulo Zanoni]

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

Let's make sure the future Paulos don't forget that we need
struct_mutex when touching dev_priv->mm.stolen.

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

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 793bcba..cac1bce 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -160,6 +160,8 @@ static int find_compression_threshold(struct drm_device *dev,
 	int compression_threshold = 1;
 	int ret;
 
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
 	/* HACK: This code depends on what we will do in *_enable_fbc. If that
 	 * code changes, this code needs to change as well.
 	 *
@@ -198,6 +200,8 @@ static int i915_setup_compression(struct drm_device *dev, int size, int fb_cpp)
 	struct drm_mm_node *uninitialized_var(compressed_llb);
 	int ret;
 
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
 	ret = find_compression_threshold(dev, &dev_priv->fbc.compressed_fb,
 					 size, fb_cpp);
 	if (!ret)
@@ -250,6 +254,7 @@ int i915_gem_stolen_setup_compression(struct drm_device *dev, int size, int fb_c
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
 	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
 
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
@@ -287,6 +292,8 @@ void i915_gem_cleanup_stolen(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
 		return;
 
@@ -349,6 +356,8 @@ i915_pages_create_for_stolen(struct drm_device *dev,
 	struct sg_table *st;
 	struct scatterlist *sg;
 
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
 	DRM_DEBUG_DRIVER("offset=0x%x, size=%d\n", offset, size);
 	BUG_ON(offset > dev_priv->gtt.stolen_size - size);
 
@@ -445,6 +454,8 @@ i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
 	struct drm_mm_node *stolen;
 	int ret;
 
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
 		return NULL;
 
@@ -485,6 +496,8 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 	struct i915_vma *vma;
 	int ret;
 
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
 		return NULL;
 
-- 
2.1.4

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

[PATCH 7/8] drm/i915: reduce struct_mutex coverage at intel_crtc_page_flip()

[Paulo Zanoni]

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

Neither intel_fbc_disable() nor intel_frontbuffer_flip_prepare() need
that lock.

That intel_fbc_disable() call over there can also certainly be
improved, but let's do it on its own commit.

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 eab5aa1..0d98332 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11445,11 +11445,11 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 
 	i915_gem_track_fb(intel_fb_obj(work->old_fb), obj,
 			  to_intel_plane(primary)->frontbuffer_bit);
+	mutex_unlock(&dev->struct_mutex);
 
 	intel_fbc_disable(dev);
 	intel_frontbuffer_flip_prepare(dev,
 				       to_intel_plane(primary)->frontbuffer_bit);
-	mutex_unlock(&dev->struct_mutex);
 
 	trace_i915_flip_request(intel_crtc->plane, obj);
 
-- 
2.1.4

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

[PATCH 8/8] drm/i915: remove struct_mutex lock from intel_modeset_cleanup()

[Paulo Zanoni]

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

Neither intel_unregister_dsm_handler() nor intel_fbc_disable() need
the lock.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0d98332..2d2df8b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15550,14 +15550,10 @@ void intel_modeset_cleanup(struct drm_device *dev)
 	 */
 	drm_kms_helper_poll_fini(dev);
 
-	mutex_lock(&dev->struct_mutex);
-
 	intel_unregister_dsm_handler();
 
 	intel_fbc_disable(dev);
 
-	mutex_unlock(&dev->struct_mutex);
-
 	/* flush any delayed tasks or pending work */
 	flush_scheduled_work();
 
-- 
2.1.4

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

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

[Chris Wilson]

On Tue, Jun 30, 2015 at 10:53:06AM -0300, Paulo Zanoni wrote:
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index eb665d7..eab5aa1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4741,7 +4741,6 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
>  static void intel_pre_plane_update(struct intel_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
>  	struct drm_plane *p;
>  
> @@ -4758,13 +4757,8 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
>  	if (atomic->wait_for_flips)
>  		intel_crtc_wait_for_pending_flips(&crtc->base);
>  
> -	if (atomic->disable_fbc &&
> -	    dev_priv->fbc.crtc == crtc) {
> -		mutex_lock(&dev->struct_mutex);
> -		if (dev_priv->fbc.crtc == crtc)
> -			intel_fbc_disable(dev);
> -		mutex_unlock(&dev->struct_mutex);
> -	}
> +	if (atomic->disable_fbc)
> +		intel_fbc_disable_crtc(crtc);

Wrong patch? (This patch is called add fbc_mutex without touching
struct_mutex)
-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/8] drm/i915: add the FBC mutex

[Chris Wilson]

On Tue, Jun 30, 2015 at 10:53:06AM -0300, Paulo Zanoni wrote:
>  struct i915_fbc {
	/* This is always the inner lock when overlapping with struct_mutex */
> +	struct mutex lock;
>  	unsigned long uncompressed_size;
>  	unsigned threshold;
>  	unsigned int fb_id;

Just a comment to ease usage and review for ABBA.
-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 6/8] drm/i915: add struct_mutex WARNs to i915_gem_stolen.c

[Chris Wilson]

On Tue, Jun 30, 2015 at 10:53:10AM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Let's make sure the future Paulos don't forget that we need
> struct_mutex when touching dev_priv->mm.stolen.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 793bcba..cac1bce 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -160,6 +160,8 @@ static int find_compression_threshold(struct drm_device *dev,
>  	int compression_threshold = 1;
>  	int ret;
>  
> +	WARN_ON(!mutex_is_locked(&dev->struct_mutex));

I'm not a huge fan of vague mutex warnings that don't even check the owner.
I'm espcially not a fan of adding a WARN and not handling the error.
-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 1/8] drm/i915: don't increment the FBC threshold at fbc_enable

[Chris Wilson]

On Tue, Jun 30, 2015 at 10:53:05AM -0300, Paulo Zanoni wrote:
> 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>

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

You can move all the fbc out of i915_gem_stolen.c now that we have
intel_fbc.c. I don't even think we need to export a function to provide
drm_mm_insert_node(&dev_priv->mm.stolen), but we probably should for
ease of future maintenance.
-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/8] drm/i915: add the FBC mutex

[Chris Wilson]

On Tue, Jun 30, 2015 at 10:53:06AM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Make sure we're not going to have weird races in really weird cases
> where a lot of different CRTCs are doing rendering and modesets at the
> same time.
> 
> With this change, we can start removing the struct_mutex locking we
> have around FBC in the next patches. The only struct_mutex requirement
> will be around intel_fbc_update(), since it's the place that does the
> CFB stolen memory allocations.
> 
> v2:
>  - Rebase (6 months later)
>  - Also lock debugfs and stolen.
> v3:
>  - Don't lock a single value read (Chris).
>  - Replace lockdep assertions with WARNs (Daniel).
>  - Improve commit message.
>  - Don't forget intel_pre_plane_update() locking.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

I haven't spotted anything scary, but I only did a cursory scan of the
patch for errors, and not down a delve into each function looking for
trouble.

Can I pretty please ask that you split fbc out of i915_gem_stolen.c
before this patch (rather than teach stolen more about fbc)?
-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 6/8] drm/i915: add struct_mutex WARNs to i915_gem_stolen.c

[Paulo Zanoni]

2015-06-30 11:15 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Tue, Jun 30, 2015 at 10:53:10AM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Let's make sure the future Paulos don't forget that we need
>> struct_mutex when touching dev_priv->mm.stolen.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_gem_stolen.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
>> index 793bcba..cac1bce 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
>> @@ -160,6 +160,8 @@ static int find_compression_threshold(struct drm_device *dev,
>>       int compression_threshold = 1;
>>       int ret;
>>
>> +     WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>
> I'm not a huge fan of vague mutex warnings that don't even check the owner.
> I'm espcially not a fan of adding a WARN and not handling the error.

But then, what exactly is your proposal? What would you like to see here?

We can discard this patch if you want. But I hope you're not
advocating for lockdep_assert_held(), because if I switch to lockdep,
then Daniel is going to deny it again. Also, this type of WARN_ON is a
common pattern on our codebase...


> -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 5/8] drm/i915: simplify FBC start/stop at invalidate/flush

[Chris Wilson]

On Tue, Jun 30, 2015 at 10:53:09AM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> The problem with calling intel_fbc_update() at flush is that it fully
> rechecks and recomputes the FBC state, and that includes reallocating
> the CFB, which requires a struct_mutex lock that we don't always have.

Actually, that is something we should fix with a stolen_mutex (will be
needed elsewhere).
 
> The lack of struct_mutex lock can be considered a regression from:
> 
> commit dbef0f15b5c83231dacb214dbf9a6dba063ca21c
> Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Date:   Fri Feb 13 17:23:46 2015 -0200
>     drm/i915: add frontbuffer tracking to FBC
> 
> So introduce intel_fbc_stop() that doesn't unset fbc.crtc, then call
> stop/enable at invalidate/flush.
> 
> Notice that invalidate/flush is only called by the frontbuffer
> tracking infrastrucutre, so it's safe to not do a full
> intel_fbc_update() here.

I think intel_fbc_update() is confusing. I can understand start/stop,
but update to me is more akin to a flush. If it was called flush, then I
would not expect it to enable or disable fbc or do any reallocations at
all.

static void intel_fbc_flush(struct drm_device *dev)
{
	if (!dev_priv->fbc.busy_bits && dev_priv->fbc.crtc) {
		if (dev_priv->fbc.enabled)
			intel_fbc_stop(dev);
		intel_fbc_enable(&dev_priv->fbc.crtc->base);
	}
}

And now I look at enable/disable, start/stop and am confused as to what
all the stages actually are.

I presume that start/stop are the highest, and control the sw state. And
that enable/disable are just hw interaction. And who sets fbc.enabled?
start()? enable()? disable()? stop()?

In confusion,
-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/8] drm/i915: add the FBC mutex

[Paulo Zanoni]

2015-06-30 11:25 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Tue, Jun 30, 2015 at 10:53:06AM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Make sure we're not going to have weird races in really weird cases
>> where a lot of different CRTCs are doing rendering and modesets at the
>> same time.
>>
>> With this change, we can start removing the struct_mutex locking we
>> have around FBC in the next patches. The only struct_mutex requirement
>> will be around intel_fbc_update(), since it's the place that does the
>> CFB stolen memory allocations.
>>
>> v2:
>>  - Rebase (6 months later)
>>  - Also lock debugfs and stolen.
>> v3:
>>  - Don't lock a single value read (Chris).
>>  - Replace lockdep assertions with WARNs (Daniel).
>>  - Improve commit message.
>>  - Don't forget intel_pre_plane_update() locking.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> I haven't spotted anything scary, but I only did a cursory scan of the
> patch for errors, and not down a delve into each function looking for
> trouble.

Thanks for the reviews! I'll add the ABBA comment and introduce a new
patch for the struct_mutex removal you spotted.

>
> Can I pretty please ask that you split fbc out of i915_gem_stolen.c
> before this patch (rather than teach stolen more about fbc)?

Ok, I'll do it.

> -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 6/8] drm/i915: add struct_mutex WARNs to i915_gem_stolen.c

[Chris Wilson]

On Tue, Jun 30, 2015 at 10:53:10AM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Let's make sure the future Paulos don't forget that we need
> struct_mutex when touching dev_priv->mm.stolen.

As I elluded to in patch 5, I think the stolen warns are a misstep.
-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 6/8] drm/i915: add struct_mutex WARNs to i915_gem_stolen.c

[Chris Wilson]

On Tue, Jun 30, 2015 at 11:26:11AM -0300, Paulo Zanoni wrote:
> 2015-06-30 11:15 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Tue, Jun 30, 2015 at 10:53:10AM -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> Let's make sure the future Paulos don't forget that we need
> >> struct_mutex when touching dev_priv->mm.stolen.
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_gem_stolen.c | 13 +++++++++++++
> >>  1 file changed, 13 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> >> index 793bcba..cac1bce 100644
> >> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> >> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> >> @@ -160,6 +160,8 @@ static int find_compression_threshold(struct drm_device *dev,
> >>       int compression_threshold = 1;
> >>       int ret;
> >>
> >> +     WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> >
> > I'm not a huge fan of vague mutex warnings that don't even check the owner.
> > I'm espcially not a fan of adding a WARN and not handling the error.
> 
> But then, what exactly is your proposal? What would you like to see here?
> 
> We can discard this patch if you want. But I hope you're not
> advocating for lockdep_assert_held(), because if I switch to lockdep,
> then Daniel is going to deny it again. Also, this type of WARN_ON is a
> common pattern on our codebase...

I'm just trying to convince Daniel that blindly using this pattern is
the wrong approach and encouraging a proliferation of unhandled WARN_ON
doesn't improve driver robustness.
-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 6/8] drm/i915: add struct_mutex WARNs to i915_gem_stolen.c

[Jesse Barnes]

On 06/30/2015 07:36 AM, Chris Wilson wrote:
> On Tue, Jun 30, 2015 at 11:26:11AM -0300, Paulo Zanoni wrote:
>> 2015-06-30 11:15 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
>>> On Tue, Jun 30, 2015 at 10:53:10AM -0300, Paulo Zanoni wrote:
>>>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>>>
>>>> Let's make sure the future Paulos don't forget that we need
>>>> struct_mutex when touching dev_priv->mm.stolen.
>>>>
>>>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/i915_gem_stolen.c | 13 +++++++++++++
>>>>  1 file changed, 13 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
>>>> index 793bcba..cac1bce 100644
>>>> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
>>>> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
>>>> @@ -160,6 +160,8 @@ static int find_compression_threshold(struct drm_device *dev,
>>>>       int compression_threshold = 1;
>>>>       int ret;
>>>>
>>>> +     WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>>>
>>> I'm not a huge fan of vague mutex warnings that don't even check the owner.
>>> I'm espcially not a fan of adding a WARN and not handling the error.
>>
>> But then, what exactly is your proposal? What would you like to see here?
>>
>> We can discard this patch if you want. But I hope you're not
>> advocating for lockdep_assert_held(), because if I switch to lockdep,
>> then Daniel is going to deny it again. Also, this type of WARN_ON is a
>> common pattern on our codebase...
> 
> I'm just trying to convince Daniel that blindly using this pattern is
> the wrong approach and encouraging a proliferation of unhandled WARN_ON
> doesn't improve driver robustness.

I think they serve as useful documentation at the very least, whether in
lockdep form, WARN form, or BUG form.  It's not really something we can
recover from either (maybe returning early before touching data?), so...

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

Re: [PATCH 6/8] drm/i915: add struct_mutex WARNs to i915_gem_stolen.c

[Chris Wilson]

On Tue, Jun 30, 2015 at 01:30:27PM -0700, Jesse Barnes wrote:
> I think they serve as useful documentation at the very least, whether in
> lockdep form, WARN form, or BUG form.  It's not really something we can
> recover from either (maybe returning early before touching data?), so...

As a debug aide they are less useful than lockdep and no more useful
than lockdep for documentation purposes.

But it feels like more and more of the WARN_ONs are instead of error
handling. See the olr fiasco.
-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 5/8] drm/i915: simplify FBC start/stop at invalidate/flush

[Paulo Zanoni]

2015-06-30 11:34 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Tue, Jun 30, 2015 at 10:53:09AM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> The problem with calling intel_fbc_update() at flush is that it fully
>> rechecks and recomputes the FBC state, and that includes reallocating
>> the CFB, which requires a struct_mutex lock that we don't always have.
>
> Actually, that is something we should fix with a stolen_mutex (will be
> needed elsewhere).
>
>> The lack of struct_mutex lock can be considered a regression from:
>>
>> commit dbef0f15b5c83231dacb214dbf9a6dba063ca21c
>> Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> Date:   Fri Feb 13 17:23:46 2015 -0200
>>     drm/i915: add frontbuffer tracking to FBC
>>
>> So introduce intel_fbc_stop() that doesn't unset fbc.crtc, then call
>> stop/enable at invalidate/flush.
>>
>> Notice that invalidate/flush is only called by the frontbuffer
>> tracking infrastrucutre, so it's safe to not do a full
>> intel_fbc_update() here.
>
> I think intel_fbc_update() is confusing. I can understand start/stop,
> but update to me is more akin to a flush. If it was called flush, then I
> would not expect it to enable or disable fbc or do any reallocations at
> all.
>
> static void intel_fbc_flush(struct drm_device *dev)
> {
>         if (!dev_priv->fbc.busy_bits && dev_priv->fbc.crtc) {
>                 if (dev_priv->fbc.enabled)
>                         intel_fbc_stop(dev);
>                 intel_fbc_enable(&dev_priv->fbc.crtc->base);
>         }
> }
>
> And now I look at enable/disable, start/stop and am confused as to what
> all the stages actually are.
>
> I presume that start/stop are the highest, and control the sw state. And
> that enable/disable are just hw interaction. And who sets fbc.enabled?
> start()? enable()? disable()? stop()?
>
> In confusion,

I understand your concerns and I agree with you that this is
confusing. I also agree that the addition of stop() makes things even
worse. One of the problems is that intel_fbc_update() does
"everything": it picks the CRTC, it can enable FBC, it can disable
FBC, it can change the CRTC, etc. So we have: update(), enable(),
disable(), flush() and invalidate(), and the patch added stop().

I had some patches that would move us to enable/disable (high level)
activate/deactivate (low level), flush/invalidate (wrappers for
activate/deactivate) and update (highest level). This would make the
naming scheme similar to PSR. I wanted to merge the locking fixes
first, but I can put everything on the same series if you want. Or
leave this patch out of the "locking" series and add it to the next
series...

> -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 1/8] drm/i915: don't increment the FBC threshold at fbc_enable

[Daniel Vetter]

On Tue, Jun 30, 2015 at 03:22:51PM +0100, Chris Wilson wrote:
> On Tue, Jun 30, 2015 at 10:53:05AM -0300, Paulo Zanoni wrote:
> > 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>
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Queued for -next, thanks for the patch.
-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 6/8] drm/i915: add struct_mutex WARNs to i915_gem_stolen.c

[Daniel Vetter]

On Tue, Jun 30, 2015 at 01:30:27PM -0700, Jesse Barnes wrote:
> On 06/30/2015 07:36 AM, Chris Wilson wrote:
> > On Tue, Jun 30, 2015 at 11:26:11AM -0300, Paulo Zanoni wrote:
> >> 2015-06-30 11:15 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> >>> On Tue, Jun 30, 2015 at 10:53:10AM -0300, Paulo Zanoni wrote:
> >>>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>>>
> >>>> Let's make sure the future Paulos don't forget that we need
> >>>> struct_mutex when touching dev_priv->mm.stolen.
> >>>>
> >>>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>>> ---
> >>>>  drivers/gpu/drm/i915/i915_gem_stolen.c | 13 +++++++++++++
> >>>>  1 file changed, 13 insertions(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> >>>> index 793bcba..cac1bce 100644
> >>>> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> >>>> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> >>>> @@ -160,6 +160,8 @@ static int find_compression_threshold(struct drm_device *dev,
> >>>>       int compression_threshold = 1;
> >>>>       int ret;
> >>>>
> >>>> +     WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> >>>
> >>> I'm not a huge fan of vague mutex warnings that don't even check the owner.
> >>> I'm espcially not a fan of adding a WARN and not handling the error.
> >>
> >> But then, what exactly is your proposal? What would you like to see here?
> >>
> >> We can discard this patch if you want. But I hope you're not
> >> advocating for lockdep_assert_held(), because if I switch to lockdep,
> >> then Daniel is going to deny it again. Also, this type of WARN_ON is a
> >> common pattern on our codebase...
> > 
> > I'm just trying to convince Daniel that blindly using this pattern is
> > the wrong approach and encouraging a proliferation of unhandled WARN_ON
> > doesn't improve driver robustness.
> 
> I think they serve as useful documentation at the very least, whether in
> lockdep form, WARN form, or BUG form.  It's not really something we can
> recover from either (maybe returning early before touching data?), so...

Not grabbing a lock is generally a harmless error since real races out
there are rare with X being single-threaded and all that. Especially in
stuff called from modeset code. Hence I think just WARN_ON plus continuing
on with blissful ignorance is the best approach.

I don't the lockdep versions personally since they don't work when lockdep
is disabled, which is pretty much always the case. Might be useful to do
an assert_mutex_held which always does the most paranoid check (i.e.
WARN_ON without lockdep, lockdep_assert_held with lockdep).
-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 6/8] drm/i915: add struct_mutex WARNs to i915_gem_stolen.c

[Daniel Vetter]

On Tue, Jun 30, 2015 at 03:34:55PM +0100, Chris Wilson wrote:
> On Tue, Jun 30, 2015 at 10:53:10AM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > Let's make sure the future Paulos don't forget that we need
> > struct_mutex when touching dev_priv->mm.stolen.
> 
> As I elluded to in patch 5, I think the stolen warns are a misstep.

Imo switching to a separate stolen_mutex should be a separate patch, this
just documents the current rules. Which seems fine to me.
-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 6/8] drm/i915: add struct_mutex WARNs to i915_gem_stolen.c

[Chris Wilson]

On Wed, Jul 01, 2015 at 04:00:23PM +0200, Daniel Vetter wrote:
> On Tue, Jun 30, 2015 at 03:34:55PM +0100, Chris Wilson wrote:
> > On Tue, Jun 30, 2015 at 10:53:10AM -0300, Paulo Zanoni wrote:
> > > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > 
> > > Let's make sure the future Paulos don't forget that we need
> > > struct_mutex when touching dev_priv->mm.stolen.
> > 
> > As I elluded to in patch 5, I think the stolen warns are a misstep.
> 
> Imo switching to a separate stolen_mutex should be a separate patch, this
> just documents the current rules. Which seems fine to me.

Introducing a stolen mutex won't be a very much larger patch, and the
current locking rules are an impediment for use elsewhere.
-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 6/8] drm/i915: add struct_mutex WARNs to i915_gem_stolen.c

[Paulo Zanoni]

2015-07-01 11:02 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Wed, Jul 01, 2015 at 04:00:23PM +0200, Daniel Vetter wrote:
>> On Tue, Jun 30, 2015 at 03:34:55PM +0100, Chris Wilson wrote:
>> > On Tue, Jun 30, 2015 at 10:53:10AM -0300, Paulo Zanoni wrote:
>> > > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > >
>> > > Let's make sure the future Paulos don't forget that we need
>> > > struct_mutex when touching dev_priv->mm.stolen.
>> >
>> > As I elluded to in patch 5, I think the stolen warns are a misstep.
>>
>> Imo switching to a separate stolen_mutex should be a separate patch, this
>> just documents the current rules. Which seems fine to me.
>
> Introducing a stolen mutex won't be a very much larger patch, and the
> current locking rules are an impediment for use elsewhere.

I wrote the stolen_mutex patches yesterday, I'll send them soon.

> -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 5/8] drm/i915: simplify FBC start/stop at invalidate/flush

[Chris Wilson]

On Tue, Jun 30, 2015 at 06:12:59PM -0300, Paulo Zanoni wrote:
> 2015-06-30 11:34 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > I presume that start/stop are the highest, and control the sw state. And
> > that enable/disable are just hw interaction. And who sets fbc.enabled?
> > start()? enable()? disable()? stop()?
> >
> > In confusion,
> 
> I understand your concerns and I agree with you that this is
> confusing. I also agree that the addition of stop() makes things even
> worse. One of the problems is that intel_fbc_update() does
> "everything": it picks the CRTC, it can enable FBC, it can disable
> FBC, it can change the CRTC, etc. So we have: update(), enable(),
> disable(), flush() and invalidate(), and the patch added stop().
> 
> I had some patches that would move us to enable/disable (high level)
> activate/deactivate (low level), flush/invalidate (wrappers for
> activate/deactivate) and update (highest level). This would make the
> naming scheme similar to PSR. I wanted to merge the locking fixes
> first, but I can put everything on the same series if you want. Or
> leave this patch out of the "locking" series and add it to the next
> series...

To keep it minimal, a quick outline comment telling me the layers and
ordering would be of use right now to review the patches, and longer
term to review the code.
-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 6/8] drm/i915: add struct_mutex WARNs to i915_gem_stolen.c

[Jesse Barnes]

On 07/01/2015 06:56 AM, Daniel Vetter wrote:
> On Tue, Jun 30, 2015 at 01:30:27PM -0700, Jesse Barnes wrote:
>> On 06/30/2015 07:36 AM, Chris Wilson wrote:
>>> On Tue, Jun 30, 2015 at 11:26:11AM -0300, Paulo Zanoni wrote:
>>>> 2015-06-30 11:15 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
>>>>> On Tue, Jun 30, 2015 at 10:53:10AM -0300, Paulo Zanoni wrote:
>>>>>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>>>>>
>>>>>> Let's make sure the future Paulos don't forget that we need
>>>>>> struct_mutex when touching dev_priv->mm.stolen.
>>>>>>
>>>>>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>>>>> ---
>>>>>>  drivers/gpu/drm/i915/i915_gem_stolen.c | 13 +++++++++++++
>>>>>>  1 file changed, 13 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
>>>>>> index 793bcba..cac1bce 100644
>>>>>> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
>>>>>> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
>>>>>> @@ -160,6 +160,8 @@ static int find_compression_threshold(struct drm_device *dev,
>>>>>>       int compression_threshold = 1;
>>>>>>       int ret;
>>>>>>
>>>>>> +     WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>>>>>
>>>>> I'm not a huge fan of vague mutex warnings that don't even check the owner.
>>>>> I'm espcially not a fan of adding a WARN and not handling the error.
>>>>
>>>> But then, what exactly is your proposal? What would you like to see here?
>>>>
>>>> We can discard this patch if you want. But I hope you're not
>>>> advocating for lockdep_assert_held(), because if I switch to lockdep,
>>>> then Daniel is going to deny it again. Also, this type of WARN_ON is a
>>>> common pattern on our codebase...
>>>
>>> I'm just trying to convince Daniel that blindly using this pattern is
>>> the wrong approach and encouraging a proliferation of unhandled WARN_ON
>>> doesn't improve driver robustness.
>>
>> I think they serve as useful documentation at the very least, whether in
>> lockdep form, WARN form, or BUG form.  It's not really something we can
>> recover from either (maybe returning early before touching data?), so...
> 
> Not grabbing a lock is generally a harmless error since real races out
> there are rare with X being single-threaded and all that. Especially in
> stuff called from modeset code. Hence I think just WARN_ON plus continuing
> on with blissful ignorance is the best approach.
> 
> I don't the lockdep versions personally since they don't work when lockdep
> is disabled, which is pretty much always the case. Might be useful to do
> an assert_mutex_held which always does the most paranoid check (i.e.
> WARN_ON without lockdep, lockdep_assert_held with lockdep).

Maybe we should add WARN_ONs to the lockdep_assert macros in the
!CONFIG_LOCKDEP case.  That would give us documentation, checking in
both cases, and everyone would be happy, right?

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

Re: [PATCH 6/8] drm/i915: add struct_mutex WARNs to i915_gem_stolen.c

[Daniel Vetter]

On Wed, Jul 01, 2015 at 08:17:37AM -0700, Jesse Barnes wrote:
> On 07/01/2015 06:56 AM, Daniel Vetter wrote:
> > On Tue, Jun 30, 2015 at 01:30:27PM -0700, Jesse Barnes wrote:
> >> On 06/30/2015 07:36 AM, Chris Wilson wrote:
> >>> On Tue, Jun 30, 2015 at 11:26:11AM -0300, Paulo Zanoni wrote:
> >>>> 2015-06-30 11:15 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> >>>>> On Tue, Jun 30, 2015 at 10:53:10AM -0300, Paulo Zanoni wrote:
> >>>>>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>>>>>
> >>>>>> Let's make sure the future Paulos don't forget that we need
> >>>>>> struct_mutex when touching dev_priv->mm.stolen.
> >>>>>>
> >>>>>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>>>>> ---
> >>>>>>  drivers/gpu/drm/i915/i915_gem_stolen.c | 13 +++++++++++++
> >>>>>>  1 file changed, 13 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> >>>>>> index 793bcba..cac1bce 100644
> >>>>>> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> >>>>>> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> >>>>>> @@ -160,6 +160,8 @@ static int find_compression_threshold(struct drm_device *dev,
> >>>>>>       int compression_threshold = 1;
> >>>>>>       int ret;
> >>>>>>
> >>>>>> +     WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> >>>>>
> >>>>> I'm not a huge fan of vague mutex warnings that don't even check the owner.
> >>>>> I'm espcially not a fan of adding a WARN and not handling the error.
> >>>>
> >>>> But then, what exactly is your proposal? What would you like to see here?
> >>>>
> >>>> We can discard this patch if you want. But I hope you're not
> >>>> advocating for lockdep_assert_held(), because if I switch to lockdep,
> >>>> then Daniel is going to deny it again. Also, this type of WARN_ON is a
> >>>> common pattern on our codebase...
> >>>
> >>> I'm just trying to convince Daniel that blindly using this pattern is
> >>> the wrong approach and encouraging a proliferation of unhandled WARN_ON
> >>> doesn't improve driver robustness.
> >>
> >> I think they serve as useful documentation at the very least, whether in
> >> lockdep form, WARN form, or BUG form.  It's not really something we can
> >> recover from either (maybe returning early before touching data?), so...
> > 
> > Not grabbing a lock is generally a harmless error since real races out
> > there are rare with X being single-threaded and all that. Especially in
> > stuff called from modeset code. Hence I think just WARN_ON plus continuing
> > on with blissful ignorance is the best approach.
> > 
> > I don't the lockdep versions personally since they don't work when lockdep
> > is disabled, which is pretty much always the case. Might be useful to do
> > an assert_mutex_held which always does the most paranoid check (i.e.
> > WARN_ON without lockdep, lockdep_assert_held with lockdep).
> 
> Maybe we should add WARN_ONs to the lockdep_assert macros in the
> !CONFIG_LOCKDEP case.  That would give us documentation, checking in
> both cases, and everyone would be happy, right?

tbh never tried that ;-)
-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