[PATCH 0/8] FBC (+stolen) locking, v5

[PATCH 0/8] FBC (+stolen) locking, v5

[Paulo Zanoni]

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

The big difference in this series is the fact that stolen_lock got
much simpler. I was previously afraid of doing the CFB release and
realloc operations in a non-atomic way, but I redebugged things and
realized that, after we fixed the CFB size checks, we're not trying to
free+allocate the CFB with FBC enabled, so we can make stolen_lock
become much simpler.

Thanks Chris for the suggestions and reviews!

Paulo Zanoni (8):
  drm/i915: add simple wrappers for stolen node insertion/removal
  drm/i915: move FBC code out of i915_gem_stolen.c
  drm/i915: add dev_priv->mm.stolen_lock
  drm/i915: add the FBC mutex
  drm/i915: intel_frontbuffer_flip_prepare() doesn't need struct_mutex
  drm/i915: intel_unregister_dsm_handler() doesn't need struct_mutex
  drm/i915: FBC doesn't need struct_mutex anymore
  drm/i915: protect FBC functions with HAS_FBC checks

 drivers/gpu/drm/i915/i915_debugfs.c    |   8 +-
 drivers/gpu/drm/i915/i915_dma.c        |   1 +
 drivers/gpu/drm/i915/i915_drv.h        |  14 +-
 drivers/gpu/drm/i915/i915_gem_stolen.c | 171 +++++------------------
 drivers/gpu/drm/i915/intel_display.c   |  20 +--
 drivers/gpu/drm/i915/intel_drv.h       |   2 +
 drivers/gpu/drm/i915/intel_fbc.c       | 241 ++++++++++++++++++++++++++++++---
 7 files changed, 280 insertions(+), 177 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: add simple wrappers for stolen node insertion/removal

[Paulo Zanoni]

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

We want to move the FBC code out of i915_gem_stolen.c, but that code
directly adds/removes stolen memory nodes. Let's create this
abstraction, so i915_gme_stolen.c is still in control of all the
stolen memory handling. The abstraction will also allow us to add
locking assertions later.

v2:
  - Add dev_priv as remove_node() argument since we'll need it later
    (Chris).

Requested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h        |  5 ++++
 drivers/gpu/drm/i915/i915_gem_stolen.c | 48 ++++++++++++++++++++++------------
 2 files changed, 37 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1dbd957..1af1b43 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3109,6 +3109,11 @@ static inline void i915_gem_chipset_flush(struct drm_device *dev)
 }
 
 /* i915_gem_stolen.c */
+int i915_gem_stolen_insert_node(struct drm_i915_private *dev_priv,
+				struct drm_mm_node *node, u64 size,
+				unsigned alignment);
+void i915_gem_stolen_remove_node(struct drm_i915_private *dev_priv,
+				 struct drm_mm_node *node);
 int i915_gem_init_stolen(struct drm_device *dev);
 int i915_gem_stolen_setup_compression(struct drm_device *dev, int size, int fb_cpp);
 void i915_gem_stolen_cleanup_compression(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 348ed5a..d811b14 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -42,6 +42,23 @@
  * for is a boon.
  */
 
+int i915_gem_stolen_insert_node(struct drm_i915_private *dev_priv,
+				struct drm_mm_node *node, u64 size,
+				unsigned alignment)
+{
+	if (!drm_mm_initialized(&dev_priv->mm.stolen))
+		return -ENODEV;
+
+	return drm_mm_insert_node(&dev_priv->mm.stolen, node, size, alignment,
+				  DRM_MM_SEARCH_DEFAULT);
+}
+
+void i915_gem_stolen_remove_node(struct drm_i915_private *dev_priv,
+				 struct drm_mm_node *node)
+{
+	drm_mm_remove_node(node);
+}
+
 static unsigned long i915_stolen_to_physical(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -168,8 +185,7 @@ static int find_compression_threshold(struct drm_device *dev,
 	 */
 
 	/* Try to over-allocate to reduce reallocations and fragmentation. */
-	ret = drm_mm_insert_node(&dev_priv->mm.stolen, node,
-				 size <<= 1, 4096, DRM_MM_SEARCH_DEFAULT);
+	ret = i915_gem_stolen_insert_node(dev_priv, node, size <<= 1, 4096);
 	if (ret == 0)
 		return compression_threshold;
 
@@ -179,9 +195,7 @@ again:
 	    (fb_cpp == 2 && compression_threshold == 2))
 		return 0;
 
-	ret = drm_mm_insert_node(&dev_priv->mm.stolen, node,
-				 size >>= 1, 4096,
-				 DRM_MM_SEARCH_DEFAULT);
+	ret = i915_gem_stolen_insert_node(dev_priv, node, size >>= 1, 4096);
 	if (ret && INTEL_INFO(dev)->gen <= 4) {
 		return 0;
 	} else if (ret) {
@@ -218,8 +232,8 @@ static int i915_setup_compression(struct drm_device *dev, int size, int fb_cpp)
 		if (!compressed_llb)
 			goto err_fb;
 
-		ret = drm_mm_insert_node(&dev_priv->mm.stolen, compressed_llb,
-					 4096, 4096, DRM_MM_SEARCH_DEFAULT);
+		ret = i915_gem_stolen_insert_node(dev_priv, compressed_llb,
+						  4096, 4096);
 		if (ret)
 			goto err_fb;
 
@@ -240,7 +254,7 @@ static int i915_setup_compression(struct drm_device *dev, int size, int fb_cpp)
 
 err_fb:
 	kfree(compressed_llb);
-	drm_mm_remove_node(&dev_priv->fbc.compressed_fb);
+	i915_gem_stolen_remove_node(dev_priv, &dev_priv->fbc.compressed_fb);
 err_llb:
 	pr_info_once("drm: not enough stolen space for compressed buffer (need %d more bytes), disabling. Hint: you may be able to increase stolen memory size in the BIOS to avoid this.\n", size);
 	return -ENOSPC;
@@ -269,10 +283,11 @@ void i915_gem_stolen_cleanup_compression(struct drm_device *dev)
 	if (dev_priv->fbc.uncompressed_size == 0)
 		return;
 
-	drm_mm_remove_node(&dev_priv->fbc.compressed_fb);
+	i915_gem_stolen_remove_node(dev_priv, &dev_priv->fbc.compressed_fb);
 
 	if (dev_priv->fbc.compressed_llb) {
-		drm_mm_remove_node(dev_priv->fbc.compressed_llb);
+		i915_gem_stolen_remove_node(dev_priv,
+					    dev_priv->fbc.compressed_llb);
 		kfree(dev_priv->fbc.compressed_llb);
 	}
 
@@ -386,8 +401,10 @@ static void i915_gem_object_put_pages_stolen(struct drm_i915_gem_object *obj)
 static void
 i915_gem_object_release_stolen(struct drm_i915_gem_object *obj)
 {
+	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
+
 	if (obj->stolen) {
-		drm_mm_remove_node(obj->stolen);
+		i915_gem_stolen_remove_node(dev_priv, obj->stolen);
 		kfree(obj->stolen);
 		obj->stolen = NULL;
 	}
@@ -449,8 +466,7 @@ i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
 	if (!stolen)
 		return NULL;
 
-	ret = drm_mm_insert_node(&dev_priv->mm.stolen, stolen, size,
-				 4096, DRM_MM_SEARCH_DEFAULT);
+	ret = i915_gem_stolen_insert_node(dev_priv, stolen, size, 4096);
 	if (ret) {
 		kfree(stolen);
 		return NULL;
@@ -460,7 +476,7 @@ i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
 	if (obj)
 		return obj;
 
-	drm_mm_remove_node(stolen);
+	i915_gem_stolen_remove_node(dev_priv, stolen);
 	kfree(stolen);
 	return NULL;
 }
@@ -505,7 +521,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 	obj = _i915_gem_object_create_stolen(dev, stolen);
 	if (obj == NULL) {
 		DRM_DEBUG_KMS("failed to allocate stolen object\n");
-		drm_mm_remove_node(stolen);
+		i915_gem_stolen_remove_node(dev_priv, stolen);
 		kfree(stolen);
 		return NULL;
 	}
@@ -546,7 +562,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 err_vma:
 	i915_gem_vma_destroy(vma);
 err_out:
-	drm_mm_remove_node(stolen);
+	i915_gem_stolen_remove_node(dev_priv, stolen);
 	kfree(stolen);
 	drm_gem_object_unreference(&obj->base);
 	return NULL;
-- 
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: move FBC code out of i915_gem_stolen.c

[Paulo Zanoni]

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

With the abstractions created by the last patch, we can move this code
and the only thing inside intel_fbc.c that knows about dev_priv->mm is
the code that reads stolen_base.

We also had to move a call to i915_gem_stolen_cleanup_compression()
- now called intel_fbc_cleanup_cfb() - outside i915_gem_stolen.c.

v2:
  - Rebase after the remove_node() changes on the previous patch.

Requested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c        |   1 +
 drivers/gpu/drm/i915/i915_drv.h        |   2 -
 drivers/gpu/drm/i915/i915_gem_stolen.c | 127 --------------------------------
 drivers/gpu/drm/i915/intel_drv.h       |   1 +
 drivers/gpu/drm/i915/intel_fbc.c       | 129 ++++++++++++++++++++++++++++++++-
 5 files changed, 128 insertions(+), 132 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index c5349fa..1ae9e0b 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1120,6 +1120,7 @@ int i915_driver_unload(struct drm_device *dev)
 	i915_gem_cleanup_ringbuffer(dev);
 	i915_gem_context_fini(dev);
 	mutex_unlock(&dev->struct_mutex);
+	intel_fbc_cleanup_cfb(dev);
 	i915_gem_cleanup_stolen(dev);
 
 	intel_csr_ucode_fini(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1af1b43..05b78b2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3115,8 +3115,6 @@ int i915_gem_stolen_insert_node(struct drm_i915_private *dev_priv,
 void i915_gem_stolen_remove_node(struct drm_i915_private *dev_priv,
 				 struct drm_mm_node *node);
 int i915_gem_init_stolen(struct drm_device *dev);
-int i915_gem_stolen_setup_compression(struct drm_device *dev, int size, int fb_cpp);
-void i915_gem_stolen_cleanup_compression(struct drm_device *dev);
 void i915_gem_cleanup_stolen(struct drm_device *dev);
 struct drm_i915_gem_object *
 i915_gem_object_create_stolen(struct drm_device *dev, u32 size);
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index d811b14..d2d556c 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -168,132 +168,6 @@ static unsigned long i915_stolen_to_physical(struct drm_device *dev)
 	return base;
 }
 
-static int find_compression_threshold(struct drm_device *dev,
-				      struct drm_mm_node *node,
-				      int size,
-				      int fb_cpp)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	int compression_threshold = 1;
-	int ret;
-
-	/* HACK: This code depends on what we will do in *_enable_fbc. If that
-	 * code changes, this code needs to change as well.
-	 *
-	 * The enable_fbc code will attempt to use one of our 2 compression
-	 * thresholds, therefore, in that case, we only have 1 resort.
-	 */
-
-	/* Try to over-allocate to reduce reallocations and fragmentation. */
-	ret = i915_gem_stolen_insert_node(dev_priv, node, size <<= 1, 4096);
-	if (ret == 0)
-		return compression_threshold;
-
-again:
-	/* HW's ability to limit the CFB is 1:4 */
-	if (compression_threshold > 4 ||
-	    (fb_cpp == 2 && compression_threshold == 2))
-		return 0;
-
-	ret = i915_gem_stolen_insert_node(dev_priv, node, size >>= 1, 4096);
-	if (ret && INTEL_INFO(dev)->gen <= 4) {
-		return 0;
-	} else if (ret) {
-		compression_threshold <<= 1;
-		goto again;
-	} else {
-		return compression_threshold;
-	}
-}
-
-static int i915_setup_compression(struct drm_device *dev, int size, int fb_cpp)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_mm_node *uninitialized_var(compressed_llb);
-	int ret;
-
-	ret = find_compression_threshold(dev, &dev_priv->fbc.compressed_fb,
-					 size, fb_cpp);
-	if (!ret)
-		goto err_llb;
-	else if (ret > 1) {
-		DRM_INFO("Reducing the compressed framebuffer size. This may lead to less power savings than a non-reduced-size. Try to increase stolen memory size if available in BIOS.\n");
-
-	}
-
-	dev_priv->fbc.threshold = ret;
-
-	if (INTEL_INFO(dev_priv)->gen >= 5)
-		I915_WRITE(ILK_DPFC_CB_BASE, dev_priv->fbc.compressed_fb.start);
-	else if (IS_GM45(dev)) {
-		I915_WRITE(DPFC_CB_BASE, dev_priv->fbc.compressed_fb.start);
-	} else {
-		compressed_llb = kzalloc(sizeof(*compressed_llb), GFP_KERNEL);
-		if (!compressed_llb)
-			goto err_fb;
-
-		ret = i915_gem_stolen_insert_node(dev_priv, compressed_llb,
-						  4096, 4096);
-		if (ret)
-			goto err_fb;
-
-		dev_priv->fbc.compressed_llb = compressed_llb;
-
-		I915_WRITE(FBC_CFB_BASE,
-			   dev_priv->mm.stolen_base + dev_priv->fbc.compressed_fb.start);
-		I915_WRITE(FBC_LL_BASE,
-			   dev_priv->mm.stolen_base + compressed_llb->start);
-	}
-
-	dev_priv->fbc.uncompressed_size = size;
-
-	DRM_DEBUG_KMS("reserved %d bytes of contiguous stolen space for FBC\n",
-		      size);
-
-	return 0;
-
-err_fb:
-	kfree(compressed_llb);
-	i915_gem_stolen_remove_node(dev_priv, &dev_priv->fbc.compressed_fb);
-err_llb:
-	pr_info_once("drm: not enough stolen space for compressed buffer (need %d more bytes), disabling. Hint: you may be able to increase stolen memory size in the BIOS to avoid this.\n", size);
-	return -ENOSPC;
-}
-
-int i915_gem_stolen_setup_compression(struct drm_device *dev, int size, int fb_cpp)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	if (!drm_mm_initialized(&dev_priv->mm.stolen))
-		return -ENODEV;
-
-	if (size <= dev_priv->fbc.uncompressed_size)
-		return 0;
-
-	/* Release any current block */
-	i915_gem_stolen_cleanup_compression(dev);
-
-	return i915_setup_compression(dev, size, fb_cpp);
-}
-
-void i915_gem_stolen_cleanup_compression(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	if (dev_priv->fbc.uncompressed_size == 0)
-		return;
-
-	i915_gem_stolen_remove_node(dev_priv, &dev_priv->fbc.compressed_fb);
-
-	if (dev_priv->fbc.compressed_llb) {
-		i915_gem_stolen_remove_node(dev_priv,
-					    dev_priv->fbc.compressed_llb);
-		kfree(dev_priv->fbc.compressed_llb);
-	}
-
-	dev_priv->fbc.uncompressed_size = 0;
-}
-
 void i915_gem_cleanup_stolen(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -301,7 +175,6 @@ void i915_gem_cleanup_stolen(struct drm_device *dev)
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
 		return;
 
-	i915_gem_stolen_cleanup_compression(dev);
 	drm_mm_takedown(&dev_priv->mm.stolen);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3f0a890..82abbfa 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1258,6 +1258,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 void intel_fbc_flush(struct drm_i915_private *dev_priv,
 		     unsigned int frontbuffer_bits);
 const char *intel_no_fbc_reason_str(enum no_fbc_reason reason);
+void intel_fbc_cleanup_cfb(struct drm_device *dev);
 
 /* intel_hdmi.c */
 void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port);
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 9e55b9b..55711b4 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -515,6 +515,129 @@ static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
 	return crtc;
 }
 
+static int find_compression_threshold(struct drm_device *dev,
+				      struct drm_mm_node *node,
+				      int size,
+				      int fb_cpp)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int compression_threshold = 1;
+	int ret;
+
+	/* HACK: This code depends on what we will do in *_enable_fbc. If that
+	 * code changes, this code needs to change as well.
+	 *
+	 * The enable_fbc code will attempt to use one of our 2 compression
+	 * thresholds, therefore, in that case, we only have 1 resort.
+	 */
+
+	/* Try to over-allocate to reduce reallocations and fragmentation. */
+	ret = i915_gem_stolen_insert_node(dev_priv, node, size <<= 1, 4096);
+	if (ret == 0)
+		return compression_threshold;
+
+again:
+	/* HW's ability to limit the CFB is 1:4 */
+	if (compression_threshold > 4 ||
+	    (fb_cpp == 2 && compression_threshold == 2))
+		return 0;
+
+	ret = i915_gem_stolen_insert_node(dev_priv, node, size >>= 1, 4096);
+	if (ret && INTEL_INFO(dev)->gen <= 4) {
+		return 0;
+	} else if (ret) {
+		compression_threshold <<= 1;
+		goto again;
+	} else {
+		return compression_threshold;
+	}
+}
+
+static int intel_fbc_alloc_cfb(struct drm_device *dev, int size, int fb_cpp)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_mm_node *uninitialized_var(compressed_llb);
+	int ret;
+
+	ret = find_compression_threshold(dev, &dev_priv->fbc.compressed_fb,
+					 size, fb_cpp);
+	if (!ret)
+		goto err_llb;
+	else if (ret > 1) {
+		DRM_INFO("Reducing the compressed framebuffer size. This may lead to less power savings than a non-reduced-size. Try to increase stolen memory size if available in BIOS.\n");
+
+	}
+
+	dev_priv->fbc.threshold = ret;
+
+	if (INTEL_INFO(dev_priv)->gen >= 5)
+		I915_WRITE(ILK_DPFC_CB_BASE, dev_priv->fbc.compressed_fb.start);
+	else if (IS_GM45(dev)) {
+		I915_WRITE(DPFC_CB_BASE, dev_priv->fbc.compressed_fb.start);
+	} else {
+		compressed_llb = kzalloc(sizeof(*compressed_llb), GFP_KERNEL);
+		if (!compressed_llb)
+			goto err_fb;
+
+		ret = i915_gem_stolen_insert_node(dev_priv, compressed_llb,
+						  4096, 4096);
+		if (ret)
+			goto err_fb;
+
+		dev_priv->fbc.compressed_llb = compressed_llb;
+
+		I915_WRITE(FBC_CFB_BASE,
+			   dev_priv->mm.stolen_base + dev_priv->fbc.compressed_fb.start);
+		I915_WRITE(FBC_LL_BASE,
+			   dev_priv->mm.stolen_base + compressed_llb->start);
+	}
+
+	dev_priv->fbc.uncompressed_size = size;
+
+	DRM_DEBUG_KMS("reserved %d bytes of contiguous stolen space for FBC\n",
+		      size);
+
+	return 0;
+
+err_fb:
+	kfree(compressed_llb);
+	i915_gem_stolen_remove_node(dev_priv, &dev_priv->fbc.compressed_fb);
+err_llb:
+	pr_info_once("drm: not enough stolen space for compressed buffer (need %d more bytes), disabling. Hint: you may be able to increase stolen memory size in the BIOS to avoid this.\n", size);
+	return -ENOSPC;
+}
+
+void intel_fbc_cleanup_cfb(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (dev_priv->fbc.uncompressed_size == 0)
+		return;
+
+	i915_gem_stolen_remove_node(dev_priv, &dev_priv->fbc.compressed_fb);
+
+	if (dev_priv->fbc.compressed_llb) {
+		i915_gem_stolen_remove_node(dev_priv,
+					    dev_priv->fbc.compressed_llb);
+		kfree(dev_priv->fbc.compressed_llb);
+	}
+
+	dev_priv->fbc.uncompressed_size = 0;
+}
+
+static int intel_fbc_setup_cfb(struct drm_device *dev, int size, int fb_cpp)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (size <= dev_priv->fbc.uncompressed_size)
+		return 0;
+
+	/* Release any current block */
+	intel_fbc_cleanup_cfb(dev);
+
+	return intel_fbc_alloc_cfb(dev, size, fb_cpp);
+}
+
 /**
  * intel_fbc_update - enable/disable FBC as needed
  * @dev: the drm_device
@@ -624,8 +747,8 @@ void intel_fbc_update(struct drm_device *dev)
 	if (in_dbg_master())
 		goto out_disable;
 
-	if (i915_gem_stolen_setup_compression(dev, obj->base.size,
-					      drm_format_plane_cpp(fb->pixel_format, 0))) {
+	if (intel_fbc_setup_cfb(dev, obj->base.size,
+				drm_format_plane_cpp(fb->pixel_format, 0))) {
 		set_no_fbc_reason(dev_priv, FBC_STOLEN_TOO_SMALL);
 		goto out_disable;
 	}
@@ -678,7 +801,7 @@ out_disable:
 		DRM_DEBUG_KMS("unsupported config, disabling FBC\n");
 		intel_fbc_disable(dev);
 	}
-	i915_gem_stolen_cleanup_compression(dev);
+	intel_fbc_cleanup_cfb(dev);
 }
 
 void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
-- 
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: add dev_priv->mm.stolen_lock

[Paulo Zanoni]

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

Which should protect dev_priv->mm.stolen usage. This will allow us to
simplify the relationship between stolen memory, FBC and struct_mutex.

v2:
  - Rebase after the stolen_remove_node() dev_priv patch move.
  - I realized that after we fixed a few things related to the FBC CFB
    size checks, we're not reallocating the CFB anymore with FBC
    enabled, so we can just move all the locking to i915_gem_stolen.c
    and stop worrying about freezing all the stolen alocations while
    freeing/rellocating the CFB. This allows us to fix the "Too
    coarse" observation from Chris.

Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h        |  4 ++++
 drivers/gpu/drm/i915/i915_gem_stolen.c | 16 ++++++++++++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 05b78b2..0b908b1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1245,6 +1245,10 @@ struct intel_l3_parity {
 struct i915_gem_mm {
 	/** Memory allocator for GTT stolen memory */
 	struct drm_mm stolen;
+	/** Protects the usage of the GTT stolen memory allocator. This is
+	 * always the inner lock when overlapping with struct_mutex. */
+	struct mutex stolen_lock;
+
 	/** List of all objects in gtt_space. Used to restore gtt
 	 * mappings on resume */
 	struct list_head bound_list;
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index d2d556c..de76d88 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -46,17 +46,25 @@ int i915_gem_stolen_insert_node(struct drm_i915_private *dev_priv,
 				struct drm_mm_node *node, u64 size,
 				unsigned alignment)
 {
+	int ret;
+
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
 		return -ENODEV;
 
-	return drm_mm_insert_node(&dev_priv->mm.stolen, node, size, alignment,
-				  DRM_MM_SEARCH_DEFAULT);
+	mutex_lock(&dev_priv->mm.stolen_lock);
+	ret = drm_mm_insert_node(&dev_priv->mm.stolen, node, size, alignment,
+				 DRM_MM_SEARCH_DEFAULT);
+	mutex_unlock(&dev_priv->mm.stolen_lock);
+
+	return ret;
 }
 
 void i915_gem_stolen_remove_node(struct drm_i915_private *dev_priv,
 				 struct drm_mm_node *node)
 {
+	mutex_lock(&dev_priv->mm.stolen_lock);
 	drm_mm_remove_node(node);
+	mutex_unlock(&dev_priv->mm.stolen_lock);
 }
 
 static unsigned long i915_stolen_to_physical(struct drm_device *dev)
@@ -184,6 +192,8 @@ int i915_gem_init_stolen(struct drm_device *dev)
 	u32 tmp;
 	int bios_reserved = 0;
 
+	mutex_init(&dev_priv->mm.stolen_lock);
+
 #ifdef CONFIG_INTEL_IOMMU
 	if (intel_iommu_gfx_mapped && INTEL_INFO(dev)->gen < 8) {
 		DRM_INFO("DMAR active, disabling use of stolen memory\n");
@@ -384,7 +394,9 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 
 	stolen->start = stolen_offset;
 	stolen->size = size;
+	mutex_lock(&dev_priv->mm.stolen_lock);
 	ret = drm_mm_reserve_node(&dev_priv->mm.stolen, stolen);
+	mutex_unlock(&dev_priv->mm.stolen_lock);
 	if (ret) {
 		DRM_DEBUG_KMS("failed to allocate stolen space\n");
 		kfree(stolen);
-- 
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: 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 and the stolen_lock from the previous patch, we can
start removing the struct_mutex locking we have around FBC in the next
patches.

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.
v4:
 - Don't remove struct_mutex at intel_pre_plane_update() (Chris).
 - Add comment regarding locking dependencies (Chris).
 - Rebase after the stolen code rework.
 - Rebase again after drm-intel-nightly changes.
v5:
 - Rebase after the new stolen_lock patch.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v4)
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      |   3 ++
 drivers/gpu/drm/i915/intel_display.c |   6 +--
 drivers/gpu/drm/i915/intel_drv.h     |   1 +
 drivers/gpu/drm/i915/intel_fbc.c     | 101 +++++++++++++++++++++++++++++------
 5 files changed, 96 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 71ba519..b2f3919 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1633,6 +1633,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");
@@ -1645,6 +1646,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;
@@ -1675,6 +1677,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;
@@ -1683,6 +1686,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 0b908b1..4d3d4103 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -899,6 +899,9 @@ enum fb_op_origin {
 };
 
 struct i915_fbc {
+	/* This is always the inner lock when overlapping with struct_mutex and
+	 * it's the outer lock when overlapping with stolen_lock. */
+	struct mutex lock;
 	unsigned long uncompressed_size;
 	unsigned threshold;
 	unsigned int fb_id;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5de1ded..e12ed4f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4783,11 +4783,9 @@ 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) {
+	if (atomic->disable_fbc) {
 		mutex_lock(&dev->struct_mutex);
-		if (dev_priv->fbc.crtc == crtc)
-			intel_fbc_disable(dev);
+		intel_fbc_disable_crtc(crtc);
 		mutex_unlock(&dev->struct_mutex);
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 82abbfa..63d7d32 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1252,6 +1252,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 55711b4..a076c7a 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)
@@ -607,7 +641,7 @@ err_llb:
 	return -ENOSPC;
 }
 
-void intel_fbc_cleanup_cfb(struct drm_device *dev)
+static void __intel_fbc_cleanup_cfb(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -625,6 +659,15 @@ void intel_fbc_cleanup_cfb(struct drm_device *dev)
 	dev_priv->fbc.uncompressed_size = 0;
 }
 
+void intel_fbc_cleanup_cfb(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	mutex_lock(&dev_priv->fbc.lock);
+	__intel_fbc_cleanup_cfb(dev);
+	mutex_unlock(&dev_priv->fbc.lock);
+}
+
 static int intel_fbc_setup_cfb(struct drm_device *dev, int size, int fb_cpp)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -633,13 +676,13 @@ static int intel_fbc_setup_cfb(struct drm_device *dev, int size, int fb_cpp)
 		return 0;
 
 	/* Release any current block */
-	intel_fbc_cleanup_cfb(dev);
+	__intel_fbc_cleanup_cfb(dev);
 
 	return intel_fbc_alloc_cfb(dev, size, fb_cpp);
 }
 
 /**
- * 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
@@ -657,7 +700,7 @@ static int intel_fbc_setup_cfb(struct drm_device *dev, int size, int fb_cpp)
  *
  * 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;
@@ -670,6 +713,8 @@ void intel_fbc_update(struct drm_device *dev)
 	if (!HAS_FBC(dev))
 		return;
 
+	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
+
 	/* disable framebuffer compression in vGPU */
 	if (intel_vgpu_active(dev))
 		i915.enable_fbc = 0;
@@ -788,7 +833,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);
@@ -799,9 +844,24 @@ 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);
 	}
-	intel_fbc_cleanup_cfb(dev);
+	__intel_fbc_cleanup_cfb(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,
@@ -814,6 +874,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)
@@ -825,7 +887,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,
@@ -833,13 +897,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);
 }
 
 /**
@@ -852,6 +921,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 5/8] drm/i915: intel_frontbuffer_flip_prepare() doesn't need struct_mutex

[Paulo Zanoni]

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

So release the lock earlier.

Reviewed-by: Chris wilson <chris@chris-wilson.co.uk>
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 e12ed4f..219e4c5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11475,9 +11475,9 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 			  to_intel_plane(primary)->frontbuffer_bit);
 
 	intel_fbc_disable(dev);
+	mutex_unlock(&dev->struct_mutex);
 	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 6/8] drm/i915: intel_unregister_dsm_handler() doesn't need struct_mutex

[Paulo Zanoni]

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

So don't grab the lock before calling the function.

Reviewed-by: Chris wilson <chris@chris-wilson.co.uk>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 219e4c5..01d7cff 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15603,12 +15603,10 @@ void intel_modeset_cleanup(struct drm_device *dev)
 	 */
 	drm_kms_helper_poll_fini(dev);
 
-	mutex_lock(&dev->struct_mutex);
-
 	intel_unregister_dsm_handler();
 
+	mutex_lock(&dev->struct_mutex);
 	intel_fbc_disable(dev);
-
 	mutex_unlock(&dev->struct_mutex);
 
 	/* flush any delayed tasks or pending work */
-- 
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: FBC doesn't need struct_mutex anymore

[Paulo Zanoni]

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

Everything is covered either by fbc.lock or mm.stolen_lock, and
intel_fbc.c is already responsible for grabbing the appropriate locks
when it needs them.

Reviewed-by: Chris wilson <chris@chris-wilson.co.uk>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |  4 ----
 drivers/gpu/drm/i915/intel_display.c | 14 +++-----------
 drivers/gpu/drm/i915/intel_fbc.c     |  2 --
 3 files changed, 3 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index b2f3919..98fd3c9 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1660,9 +1660,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;
 }
@@ -1676,7 +1674,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);
@@ -1687,7 +1684,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 01d7cff..83d971c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4747,11 +4747,8 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
 	if (crtc->atomic.update_wm_post)
 		intel_update_watermarks(&crtc->base);
 
-	if (atomic->update_fbc) {
-		mutex_lock(&dev->struct_mutex);
+	if (atomic->update_fbc)
 		intel_fbc_update(dev);
-		mutex_unlock(&dev->struct_mutex);
-	}
 
 	if (atomic->post_enable_primary)
 		intel_post_enable_primary(&crtc->base);
@@ -4783,11 +4780,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) {
-		mutex_lock(&dev->struct_mutex);
+	if (atomic->disable_fbc)
 		intel_fbc_disable_crtc(crtc);
-		mutex_unlock(&dev->struct_mutex);
-	}
 
 	if (crtc->atomic.disable_ips)
 		hsw_disable_ips(crtc);
@@ -11473,9 +11467,9 @@ 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);
-	mutex_unlock(&dev->struct_mutex);
 	intel_frontbuffer_flip_prepare(dev,
 				       to_intel_plane(primary)->frontbuffer_bit);
 
@@ -15605,9 +15599,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
 
 	intel_unregister_dsm_handler();
 
-	mutex_lock(&dev->struct_mutex);
 	intel_fbc_disable(dev);
-	mutex_unlock(&dev->struct_mutex);
 
 	/* flush any delayed tasks or pending work */
 	flush_scheduled_work();
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index a076c7a..cc9b7ef 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 8/8] drm/i915: protect FBC functions with HAS_FBC checks

[Paulo Zanoni]

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

Now all the functions called by other files have the HAS_FBC
protection. This allows us to drop the checks for the low level
function pointers.

Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index cc9b7ef..bc3cdb3 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -388,9 +388,6 @@ static void intel_fbc_enable(struct drm_crtc *crtc)
 
 	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
 
-	if (!dev_priv->display.enable_fbc)
-		return;
-
 	intel_fbc_cancel_work(dev_priv);
 
 	work = kzalloc(sizeof(*work), GFP_KERNEL);
@@ -430,9 +427,6 @@ static void __intel_fbc_disable(struct drm_device *dev)
 
 	intel_fbc_cancel_work(dev_priv);
 
-	if (!dev_priv->display.disable_fbc)
-		return;
-
 	dev_priv->display.disable_fbc(dev);
 	dev_priv->fbc.crtc = NULL;
 }
@@ -447,6 +441,9 @@ void intel_fbc_disable(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	if (!HAS_FBC(dev_priv))
+		return;
+
 	mutex_lock(&dev_priv->fbc.lock);
 	__intel_fbc_disable(dev);
 	mutex_unlock(&dev_priv->fbc.lock);
@@ -463,6 +460,9 @@ 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;
 
+	if (!HAS_FBC(dev_priv))
+		return;
+
 	mutex_lock(&dev_priv->fbc.lock);
 	if (dev_priv->fbc.crtc == crtc)
 		__intel_fbc_disable(dev);
@@ -661,6 +661,9 @@ void intel_fbc_cleanup_cfb(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	if (!HAS_FBC(dev_priv))
+		return;
+
 	mutex_lock(&dev_priv->fbc.lock);
 	__intel_fbc_cleanup_cfb(dev);
 	mutex_unlock(&dev_priv->fbc.lock);
@@ -708,9 +711,6 @@ static void __intel_fbc_update(struct drm_device *dev)
 	const struct drm_display_mode *adjusted_mode;
 	unsigned int max_width, max_height;
 
-	if (!HAS_FBC(dev))
-		return;
-
 	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
 
 	/* disable framebuffer compression in vGPU */
@@ -857,6 +857,9 @@ void intel_fbc_update(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	if (!HAS_FBC(dev_priv))
+		return;
+
 	mutex_lock(&dev_priv->fbc.lock);
 	__intel_fbc_update(dev);
 	mutex_unlock(&dev_priv->fbc.lock);
@@ -869,6 +872,9 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 	struct drm_device *dev = dev_priv->dev;
 	unsigned int fbc_bits;
 
+	if (!HAS_FBC(dev_priv))
+		return;
+
 	if (origin == ORIGIN_GTT)
 		return;
 
@@ -895,6 +901,9 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
 {
 	struct drm_device *dev = dev_priv->dev;
 
+	if (!HAS_FBC(dev_priv))
+		return;
+
 	mutex_lock(&dev_priv->fbc.lock);
 
 	if (!dev_priv->fbc.busy_bits)
-- 
2.1.4

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

Re: [PATCH 8/8] drm/i915: protect FBC functions with HAS_FBC checks

[Chris Wilson]

On Thu, Jul 02, 2015 at 07:25:14PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Now all the functions called by other files have the HAS_FBC
> protection. This allows us to drop the checks for the low level
> function pointers.
> 
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index cc9b7ef..bc3cdb3 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -388,9 +388,6 @@ static void intel_fbc_enable(struct drm_crtc *crtc)
>  
>  	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
>  
> -	if (!dev_priv->display.enable_fbc)
> -		return;
> -
>  	intel_fbc_cancel_work(dev_priv);
>  
>  	work = kzalloc(sizeof(*work), GFP_KERNEL);

> @@ -661,6 +661,9 @@ void intel_fbc_cleanup_cfb(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> +	if (!HAS_FBC(dev_priv))
> +		return;
> +
>  	mutex_lock(&dev_priv->fbc.lock);
>  	__intel_fbc_cleanup_cfb(dev);
>  	mutex_unlock(&dev_priv->fbc.lock);

dev_priv->display.enable_fbc is one less pointer dereference than
HAS_FBC() and is a better indication of whether we have initialised the
display device for FBC (i.e. it also carries information about whether
the setup succeeded, maybe some platforms have HAS_FBC but we fail to
negotiate etc).
-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 3/8] drm/i915: add dev_priv->mm.stolen_lock

[Chris Wilson]

On Thu, Jul 02, 2015 at 07:25:09PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Which should protect dev_priv->mm.stolen usage. This will allow us to
> simplify the relationship between stolen memory, FBC and struct_mutex.
> 
> v2:
>   - Rebase after the stolen_remove_node() dev_priv patch move.
>   - I realized that after we fixed a few things related to the FBC CFB
>     size checks, we're not reallocating the CFB anymore with FBC
>     enabled, so we can just move all the locking to i915_gem_stolen.c
>     and stop worrying about freezing all the stolen alocations while
>     freeing/rellocating the CFB. This allows us to fix the "Too
>     coarse" observation from Chris.
> 
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

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

Re: [PATCH 1/8] drm/i915: add simple wrappers for stolen node insertion/removal

[Chris Wilson]

On Thu, Jul 02, 2015 at 07:25:07PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We want to move the FBC code out of i915_gem_stolen.c, but that code
> directly adds/removes stolen memory nodes. Let's create this
> abstraction, so i915_gme_stolen.c is still in control of all the
> stolen memory handling. The abstraction will also allow us to add
> locking assertions later.
> 
> v2:
>   - Add dev_priv as remove_node() argument since we'll need it later
>     (Chris).
> 
> Requested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

-- 
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: move FBC code out of i915_gem_stolen.c

[Chris Wilson]

On Thu, Jul 02, 2015 at 07:25:08PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> With the abstractions created by the last patch, we can move this code
> and the only thing inside intel_fbc.c that knows about dev_priv->mm is
> the code that reads stolen_base.
> 
> We also had to move a call to i915_gem_stolen_cleanup_compression()
> - now called intel_fbc_cleanup_cfb() - outside i915_gem_stolen.c.
> 
> v2:
>   - Rebase after the remove_node() changes on the previous patch.
> 
> Requested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

No sneaky changes detected,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

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

Re: [PATCH 8/8] drm/i915: protect FBC functions with HAS_FBC checks

[Chris Wilson]

On Fri, Jul 03, 2015 at 04:56:31PM +0100, Chris Wilson wrote:
> On Thu, Jul 02, 2015 at 07:25:14PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > Now all the functions called by other files have the HAS_FBC
> > protection. This allows us to drop the checks for the low level
> > function pointers.
> > 
> > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_fbc.c | 27 ++++++++++++++++++---------
> >  1 file changed, 18 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> > index cc9b7ef..bc3cdb3 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -388,9 +388,6 @@ static void intel_fbc_enable(struct drm_crtc *crtc)
> >  
> >  	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
> >  
> > -	if (!dev_priv->display.enable_fbc)
> > -		return;
> > -
> >  	intel_fbc_cancel_work(dev_priv);
> >  
> >  	work = kzalloc(sizeof(*work), GFP_KERNEL);
> 
> > @@ -661,6 +661,9 @@ void intel_fbc_cleanup_cfb(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> > +	if (!HAS_FBC(dev_priv))
> > +		return;
> > +
> >  	mutex_lock(&dev_priv->fbc.lock);
> >  	__intel_fbc_cleanup_cfb(dev);
> >  	mutex_unlock(&dev_priv->fbc.lock);
> 
> dev_priv->display.enable_fbc is one less pointer dereference than
> HAS_FBC() and is a better indication of whether we have initialised the
> display device for FBC (i.e. it also carries information about whether
> the setup succeeded, maybe some platforms have HAS_FBC but we fail to
> negotiate etc).

With or without that change (would prefer with!),
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

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

Re: [PATCH 8/8] drm/i915: protect FBC functions with HAS_FBC checks

[Paulo Zanoni]

2015-07-03 12:56 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Thu, Jul 02, 2015 at 07:25:14PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Now all the functions called by other files have the HAS_FBC
>> protection. This allows us to drop the checks for the low level
>> function pointers.
>>
>> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_fbc.c | 27 ++++++++++++++++++---------
>>  1 file changed, 18 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
>> index cc9b7ef..bc3cdb3 100644
>> --- a/drivers/gpu/drm/i915/intel_fbc.c
>> +++ b/drivers/gpu/drm/i915/intel_fbc.c
>> @@ -388,9 +388,6 @@ static void intel_fbc_enable(struct drm_crtc *crtc)
>>
>>       WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
>>
>> -     if (!dev_priv->display.enable_fbc)
>> -             return;
>> -
>>       intel_fbc_cancel_work(dev_priv);
>>
>>       work = kzalloc(sizeof(*work), GFP_KERNEL);
>
>> @@ -661,6 +661,9 @@ void intel_fbc_cleanup_cfb(struct drm_device *dev)
>>  {
>>       struct drm_i915_private *dev_priv = dev->dev_private;
>>
>> +     if (!HAS_FBC(dev_priv))
>> +             return;
>> +
>>       mutex_lock(&dev_priv->fbc.lock);
>>       __intel_fbc_cleanup_cfb(dev);
>>       mutex_unlock(&dev_priv->fbc.lock);
>
> dev_priv->display.enable_fbc is one less pointer dereference than HAS_FBC()

HAS_FBC(dev_priv) is supposed to be dev_priv->info.has_fbc, which has
the same number of pointer dereferences than
dev_priv->display.enable_fbc. The problem would be if I had used
HAS_FBC(dev).

I also took a look at the __I915__ macro and, although I didn't check
the assembly, it seems the "if" statement is removed by the compiler
since we never compile BUILD_BUG(). If we wanted to be even more sure
we could replace the if statement with nested __builtin_choose_expr()
expressions (I even tried doing that, but it didn't reduce the size of
stripped i915.ko).


> and is a better indication of whether we have initialised the
> display device for FBC (i.e. it also carries information about whether
> the setup succeeded, maybe some platforms have HAS_FBC but we fail to
> negotiate etc).

This argument argument is valid - although that case doesn't happen -,
but OTOH a check for HAS_FBC makes it very clear to the code reader
what is being excluded, while a check for a function pointer makes the
reader wonder about those cases which you mentioned, and they don't
really exist, so I'm not sure if this is a good thing. So I guess it's
a matter of style preference here.

> -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 8/8] drm/i915: protect FBC functions with HAS_FBC checks

[Chris Wilson]

On Fri, Jul 03, 2015 at 02:59:28PM -0300, Paulo Zanoni wrote:
> 2015-07-03 12:56 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Thu, Jul 02, 2015 at 07:25:14PM -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> Now all the functions called by other files have the HAS_FBC
> >> protection. This allows us to drop the checks for the low level
> >> function pointers.
> >>
> >> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_fbc.c | 27 ++++++++++++++++++---------
> >>  1 file changed, 18 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> >> index cc9b7ef..bc3cdb3 100644
> >> --- a/drivers/gpu/drm/i915/intel_fbc.c
> >> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> >> @@ -388,9 +388,6 @@ static void intel_fbc_enable(struct drm_crtc *crtc)
> >>
> >>       WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
> >>
> >> -     if (!dev_priv->display.enable_fbc)
> >> -             return;
> >> -
> >>       intel_fbc_cancel_work(dev_priv);
> >>
> >>       work = kzalloc(sizeof(*work), GFP_KERNEL);
> >
> >> @@ -661,6 +661,9 @@ void intel_fbc_cleanup_cfb(struct drm_device *dev)
> >>  {
> >>       struct drm_i915_private *dev_priv = dev->dev_private;
> >>
> >> +     if (!HAS_FBC(dev_priv))
> >> +             return;
> >> +
> >>       mutex_lock(&dev_priv->fbc.lock);
> >>       __intel_fbc_cleanup_cfb(dev);
> >>       mutex_unlock(&dev_priv->fbc.lock);
> >
> > dev_priv->display.enable_fbc is one less pointer dereference than HAS_FBC()
> 
> HAS_FBC(dev_priv) is supposed to be dev_priv->info.has_fbc, which has
> the same number of pointer dereferences than
> dev_priv->display.enable_fbc. The problem would be if I had used
> HAS_FBC(dev).

I had forgotten we did the copy of intel_device_info into
drm_i915_private and still lived in the pre-PCH_NOP days.

> I also took a look at the __I915__ macro and, although I didn't check
> the assembly, it seems the "if" statement is removed by the compiler
> since we never compile BUILD_BUG(). If we wanted to be even more sure
> we could replace the if statement with nested __builtin_choose_expr()
> expressions (I even tried doing that, but it didn't reduce the size of
> stripped i915.ko).

if (compile_time_constant) is dead-code eliminated into the single
branch (without the jmp).

Hmm, the big advantage (not useful here) is __builtin_choose_expr() can
be used as an lvalue. Otherwise it doesn't look like it will generate
more readable code than the current macro.

> > and is a better indication of whether we have initialised the
> > display device for FBC (i.e. it also carries information about whether
> > the setup succeeded, maybe some platforms have HAS_FBC but we fail to
> > negotiate etc).
> 
> This argument argument is valid - although that case doesn't happen -,
> but OTOH a check for HAS_FBC makes it very clear to the code reader
> what is being excluded, while a check for a function pointer makes the
> reader wonder about those cases which you mentioned, and they don't
> really exist, so I'm not sure if this is a good thing. So I guess it's
> a matter of style preference here.

It's the style we have been using elsewhere, certainly in new code
(hopefully!). Do the general test during init, then later we ask whether
the init suceeded before using the feature.
-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

[PATCH 8/8] drm/i915: protect FBC functions with FBC checks

[Paulo Zanoni]

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

Now all the functions called by other files check whether FBC has been
initialized. This allows us to drop the checks on the static
functions.

v2:
 - s/HAS_FBC/dev_priv->display.enable_fbc/ everywhere but the init
   function (Chris).

Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)


I'm not 100% sure this is what Chris was asking for v2, so please clarify if
this is not what we want.


diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index cc9b7ef..65f08e3 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -388,9 +388,6 @@ static void intel_fbc_enable(struct drm_crtc *crtc)
 
 	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
 
-	if (!dev_priv->display.enable_fbc)
-		return;
-
 	intel_fbc_cancel_work(dev_priv);
 
 	work = kzalloc(sizeof(*work), GFP_KERNEL);
@@ -430,9 +427,6 @@ static void __intel_fbc_disable(struct drm_device *dev)
 
 	intel_fbc_cancel_work(dev_priv);
 
-	if (!dev_priv->display.disable_fbc)
-		return;
-
 	dev_priv->display.disable_fbc(dev);
 	dev_priv->fbc.crtc = NULL;
 }
@@ -447,6 +441,9 @@ void intel_fbc_disable(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	if (!dev_priv->display.enable_fbc)
+		return;
+
 	mutex_lock(&dev_priv->fbc.lock);
 	__intel_fbc_disable(dev);
 	mutex_unlock(&dev_priv->fbc.lock);
@@ -463,6 +460,9 @@ 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;
 
+	if (!dev_priv->display.enable_fbc)
+		return;
+
 	mutex_lock(&dev_priv->fbc.lock);
 	if (dev_priv->fbc.crtc == crtc)
 		__intel_fbc_disable(dev);
@@ -661,6 +661,9 @@ void intel_fbc_cleanup_cfb(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	if (!dev_priv->display.enable_fbc)
+		return;
+
 	mutex_lock(&dev_priv->fbc.lock);
 	__intel_fbc_cleanup_cfb(dev);
 	mutex_unlock(&dev_priv->fbc.lock);
@@ -708,9 +711,6 @@ static void __intel_fbc_update(struct drm_device *dev)
 	const struct drm_display_mode *adjusted_mode;
 	unsigned int max_width, max_height;
 
-	if (!HAS_FBC(dev))
-		return;
-
 	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
 
 	/* disable framebuffer compression in vGPU */
@@ -857,6 +857,9 @@ void intel_fbc_update(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	if (!dev_priv->display.enable_fbc)
+		return;
+
 	mutex_lock(&dev_priv->fbc.lock);
 	__intel_fbc_update(dev);
 	mutex_unlock(&dev_priv->fbc.lock);
@@ -869,6 +872,9 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 	struct drm_device *dev = dev_priv->dev;
 	unsigned int fbc_bits;
 
+	if (!dev_priv->display.enable_fbc)
+		return;
+
 	if (origin == ORIGIN_GTT)
 		return;
 
@@ -895,6 +901,9 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
 {
 	struct drm_device *dev = dev_priv->dev;
 
+	if (!dev_priv->display.enable_fbc)
+		return;
+
 	mutex_lock(&dev_priv->fbc.lock);
 
 	if (!dev_priv->fbc.busy_bits)
-- 
2.1.4

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

Re: [PATCH 8/8] drm/i915: protect FBC functions with FBC checks

[Chris Wilson]

On Fri, Jul 03, 2015 at 03:40:54PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Now all the functions called by other files check whether FBC has been
> initialized. This allows us to drop the checks on the static
> functions.
> 
> v2:
>  - s/HAS_FBC/dev_priv->display.enable_fbc/ everywhere but the init
>    function (Chris).
> 
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> 
> I'm not 100% sure this is what Chris was asking for v2, so please clarify if
> this is not what we want.

Does the trick for me,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

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

Re: [PATCH 8/8] drm/i915: protect FBC functions with FBC checks

[Daniel Vetter]

On Fri, Jul 03, 2015 at 07:50:31PM +0100, Chris Wilson wrote:
> On Fri, Jul 03, 2015 at 03:40:54PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > Now all the functions called by other files check whether FBC has been
> > initialized. This allows us to drop the checks on the static
> > functions.
> > 
> > v2:
> >  - s/HAS_FBC/dev_priv->display.enable_fbc/ everywhere but the init
> >    function (Chris).
> > 
> > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_fbc.c | 27 ++++++++++++++++++---------
> >  1 file changed, 18 insertions(+), 9 deletions(-)
> > 
> > 
> > I'm not 100% sure this is what Chris was asking for v2, so please clarify if
> > this is not what we want.
> 
> Does the trick for me,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Entire series merged to dinq, thanks.
-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