[PATCH 0/7] FBC (+stolen) locking, v4

[PATCH 0/7] FBC (+stolen) locking, v4

[Paulo Zanoni]

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

Hi

So, based on the reviews, here's v4 of the series, now with
dev_priv->mm.stolen_lock. This allows us to completely get rid of struct_mutex
locking around FBC calls. Kudos to Chris for the suggestion.

The patch that added intel_fbc_stop() got removed from this series because the
addition of mm.stolen_lock turned it into just an optimization instead of a
bugfix. Let's leave it to another series, where I'll also try to clarify all the
function names involved.

Since we didn't seem to reach any conclusions on the lockdep_assert_held vs
WARN_ON discussion, I kept using WARN_ON since it's what the maintainer is
asking and since it's what's winning in usage count. If we decide to change it,
we can always do it in a later patch.

Thanks,
Paulo

Paulo Zanoni (7):
  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

 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 | 215 +++++++++--------------------
 drivers/gpu/drm/i915/intel_display.c   |  20 +--
 drivers/gpu/drm/i915/intel_drv.h       |   2 +
 drivers/gpu/drm/i915/intel_fbc.c       | 239 ++++++++++++++++++++++++++++++---
 7 files changed, 309 insertions(+), 190 deletions(-)

-- 
2.1.4

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

[PATCH 1/7] 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. These abstractions will also allow us to add
locking assertions later.

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        |  4 ++++
 drivers/gpu/drm/i915/i915_gem_stolen.c | 44 +++++++++++++++++++++-------------
 2 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1dbd957..b9de374 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3109,6 +3109,10 @@ 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_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..6b43234 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -42,6 +42,22 @@
  * 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_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 +184,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 +194,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 +231,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 +253,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->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 +282,10 @@ 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->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->fbc.compressed_llb);
 		kfree(dev_priv->fbc.compressed_llb);
 	}
 
@@ -387,7 +400,7 @@ static void
 i915_gem_object_release_stolen(struct drm_i915_gem_object *obj)
 {
 	if (obj->stolen) {
-		drm_mm_remove_node(obj->stolen);
+		i915_gem_stolen_remove_node(obj->stolen);
 		kfree(obj->stolen);
 		obj->stolen = NULL;
 	}
@@ -449,8 +462,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 +472,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(stolen);
 	kfree(stolen);
 	return NULL;
 }
@@ -505,7 +517,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(stolen);
 		kfree(stolen);
 		return NULL;
 	}
@@ -546,7 +558,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(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/7] 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.

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 | 126 --------------------------------
 drivers/gpu/drm/i915/intel_drv.h       |   1 +
 drivers/gpu/drm/i915/intel_fbc.c       | 128 ++++++++++++++++++++++++++++++++-
 5 files changed, 127 insertions(+), 131 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 b9de374..c955037 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3114,8 +3114,6 @@ int i915_gem_stolen_insert_node(struct drm_i915_private *dev_priv,
 				unsigned alignment);
 void i915_gem_stolen_remove_node(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 6b43234..0619786 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -167,131 +167,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->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->fbc.compressed_fb);
-
-	if (dev_priv->fbc.compressed_llb) {
-		i915_gem_stolen_remove_node(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;
@@ -299,7 +174,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..a91bf82 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -515,6 +515,128 @@ 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->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->fbc.compressed_fb);
+
+	if (dev_priv->fbc.compressed_llb) {
+		i915_gem_stolen_remove_node(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 +746,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 +800,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/7] 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.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h        |  7 +++-
 drivers/gpu/drm/i915/i915_gem_stolen.c | 69 +++++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_fbc.c       | 29 +++++++++++---
 3 files changed, 77 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c955037..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;
@@ -3112,7 +3116,8 @@ static inline void i915_gem_chipset_flush(struct drm_device *dev)
 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_mm_node *node);
+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);
 void i915_gem_cleanup_stolen(struct drm_device *dev);
 struct drm_i915_gem_object *
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 0619786..b432085 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -46,6 +46,8 @@ int i915_gem_stolen_insert_node(struct drm_i915_private *dev_priv,
 				struct drm_mm_node *node, u64 size,
 				unsigned alignment)
 {
+	WARN_ON(!mutex_is_locked(&dev_priv->mm.stolen_lock));
+
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
 		return -ENODEV;
 
@@ -53,8 +55,11 @@ int i915_gem_stolen_insert_node(struct drm_i915_private *dev_priv,
 				  DRM_MM_SEARCH_DEFAULT);
 }
 
-void i915_gem_stolen_remove_node(struct drm_mm_node *node)
+void i915_gem_stolen_remove_node(struct drm_i915_private *dev_priv,
+				 struct drm_mm_node *node)
 {
+	WARN_ON(!mutex_is_locked(&dev_priv->mm.stolen_lock));
+
 	drm_mm_remove_node(node);
 }
 
@@ -171,10 +176,15 @@ void i915_gem_cleanup_stolen(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	mutex_lock(&dev_priv->mm.stolen_lock);
+
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
-		return;
+		goto out;
 
 	drm_mm_takedown(&dev_priv->mm.stolen);
+
+out:
+	mutex_unlock(&dev_priv->mm.stolen_lock);
 }
 
 int i915_gem_init_stolen(struct drm_device *dev)
@@ -183,6 +193,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");
@@ -273,8 +285,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) {
-		i915_gem_stolen_remove_node(obj->stolen);
+		i915_gem_stolen_remove_node(dev_priv, obj->stolen);
 		kfree(obj->stolen);
 		obj->stolen = NULL;
 	}
@@ -325,29 +339,36 @@ i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
 	struct drm_mm_node *stolen;
 	int ret;
 
+	mutex_lock(&dev_priv->mm.stolen_lock);
+
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
-		return NULL;
+		goto out_unlock;
 
 	DRM_DEBUG_KMS("creating stolen object: size=%x\n", size);
 	if (size == 0)
-		return NULL;
+		goto out_unlock;
 
 	stolen = kzalloc(sizeof(*stolen), GFP_KERNEL);
 	if (!stolen)
-		return NULL;
+		goto out_unlock;
 
 	ret = i915_gem_stolen_insert_node(dev_priv, stolen, size, 4096);
-	if (ret) {
-		kfree(stolen);
-		return NULL;
-	}
+	if (ret)
+		goto out_free;
 
 	obj = _i915_gem_object_create_stolen(dev, stolen);
-	if (obj)
-		return obj;
+	if (!obj)
+		goto out_node;
 
-	i915_gem_stolen_remove_node(stolen);
+	mutex_unlock(&dev_priv->mm.stolen_lock);
+	return obj;
+
+out_node:
+	i915_gem_stolen_remove_node(dev_priv, stolen);
+out_free:
 	kfree(stolen);
+out_unlock:
+	mutex_unlock(&dev_priv->mm.stolen_lock);
 	return NULL;
 }
 
@@ -364,8 +385,10 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 	struct i915_vma *vma;
 	int ret;
 
+	mutex_lock(&dev_priv->mm.stolen_lock);
+
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
-		return NULL;
+		goto err_unlock;
 
 	DRM_DEBUG_KMS("creating preallocated stolen object: stolen_offset=%x, gtt_offset=%x, size=%x\n",
 			stolen_offset, gtt_offset, size);
@@ -373,11 +396,11 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 	/* KISS and expect everything to be page-aligned */
 	if (WARN_ON(size == 0) || WARN_ON(size & 4095) ||
 	    WARN_ON(stolen_offset & 4095))
-		return NULL;
+		goto err_unlock;
 
 	stolen = kzalloc(sizeof(*stolen), GFP_KERNEL);
 	if (!stolen)
-		return NULL;
+		goto err_unlock;
 
 	stolen->start = stolen_offset;
 	stolen->size = size;
@@ -385,20 +408,20 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 	if (ret) {
 		DRM_DEBUG_KMS("failed to allocate stolen space\n");
 		kfree(stolen);
-		return NULL;
+		goto err_unlock;
 	}
 
 	obj = _i915_gem_object_create_stolen(dev, stolen);
 	if (obj == NULL) {
 		DRM_DEBUG_KMS("failed to allocate stolen object\n");
-		i915_gem_stolen_remove_node(stolen);
+		i915_gem_stolen_remove_node(dev_priv, stolen);
 		kfree(stolen);
-		return NULL;
+		goto err_unlock;
 	}
 
 	/* Some objects just need physical mem from stolen space */
 	if (gtt_offset == I915_GTT_OFFSET_NONE)
-		return obj;
+		goto success;
 
 	vma = i915_gem_obj_lookup_or_create_vma(obj, ggtt);
 	if (IS_ERR(vma)) {
@@ -427,13 +450,17 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 	list_add_tail(&vma->mm_list, &ggtt->inactive_list);
 	i915_gem_object_pin_pages(obj);
 
+success:
+	mutex_unlock(&dev_priv->mm.stolen_lock);
 	return obj;
 
 err_vma:
 	i915_gem_vma_destroy(vma);
 err_out:
-	i915_gem_stolen_remove_node(stolen);
+	i915_gem_stolen_remove_node(dev_priv, stolen);
 	kfree(stolen);
 	drm_gem_object_unreference(&obj->base);
+err_unlock:
+	mutex_unlock(&dev_priv->mm.stolen_lock);
 	return NULL;
 }
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index a91bf82..dcd83ab 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -601,40 +601,57 @@ static int intel_fbc_alloc_cfb(struct drm_device *dev, int size, int fb_cpp)
 
 err_fb:
 	kfree(compressed_llb);
-	i915_gem_stolen_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;
 }
 
-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;
 
 	if (dev_priv->fbc.uncompressed_size == 0)
 		return;
 
-	i915_gem_stolen_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) {
-		i915_gem_stolen_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);
 	}
 
 	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->mm.stolen_lock);
+	__intel_fbc_cleanup_cfb(dev);
+	mutex_unlock(&dev_priv->mm.stolen_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;
+	int ret;
 
 	if (size <= dev_priv->fbc.uncompressed_size)
 		return 0;
 
+	mutex_lock(&dev_priv->mm.stolen_lock);
+
 	/* Release any current block */
-	intel_fbc_cleanup_cfb(dev);
+	__intel_fbc_cleanup_cfb(dev);
+
+	ret = intel_fbc_alloc_cfb(dev, size, fb_cpp);
+
+	mutex_unlock(&dev_priv->mm.stolen_lock);
 
-	return intel_fbc_alloc_cfb(dev, size, fb_cpp);
+	return ret;
 }
 
 /**
-- 
2.1.4

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

[PATCH 4/7] 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.

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     | 94 +++++++++++++++++++++++++++++++-----
 5 files changed, 91 insertions(+), 17 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 dcd83ab..3a98bc1 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)
@@ -629,9 +663,13 @@ void intel_fbc_cleanup_cfb(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	mutex_lock(&dev_priv->fbc.lock);
 	mutex_lock(&dev_priv->mm.stolen_lock);
+
 	__intel_fbc_cleanup_cfb(dev);
+
 	mutex_unlock(&dev_priv->mm.stolen_lock);
+	mutex_unlock(&dev_priv->fbc.lock);
 }
 
 static int intel_fbc_setup_cfb(struct drm_device *dev, int size, int fb_cpp)
@@ -655,7 +693,7 @@ static int intel_fbc_setup_cfb(struct drm_device *dev, int size, int 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
@@ -673,7 +711,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;
@@ -683,6 +721,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;
 
@@ -804,7 +844,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);
@@ -815,9 +855,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);
 	}
-	intel_fbc_cleanup_cfb(dev);
+	mutex_lock(&dev_priv->mm.stolen_lock);
+	__intel_fbc_cleanup_cfb(dev);
+	mutex_unlock(&dev_priv->mm.stolen_lock);
+}
+
+/*
+ * 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,
@@ -830,6 +887,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)
@@ -841,7 +900,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,
@@ -849,13 +910,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);
 }
 
 /**
@@ -868,6 +934,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/7] 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.

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/7] 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.

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/7] 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.

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 3a98bc1..1c9f092 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

Re: [PATCH 3/7] drm/i915: add dev_priv->mm.stolen_lock

[Chris Wilson]

On Wed, Jul 01, 2015 at 05:15:22PM -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.

Too coarse. The locking need only be around the stolen drm_mm, i.e. just
insert/remove node. (And you don't need the lock around drm_mm_initialized,
similarly teardown since like the init, they are serialised through
other means.)
-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/7] drm/i915: add simple wrappers for stolen node insertion/removal

[Chris Wilson]

On Wed, Jul 01, 2015 at 05:15:20PM -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. These abstractions will also allow us to add
> locking assertions later.
> 
> 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        |  4 ++++
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 44 +++++++++++++++++++++-------------
>  2 files changed, 32 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1dbd957..b9de374 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3109,6 +3109,10 @@ 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_mm_node *node);

Might as well pass in dev_priv now to save changing the interface later.


>  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..6b43234 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -42,6 +42,22 @@
>   * 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;

Might as well take advantage of this test here to remove the same check
from i915_gem_object_create_stolen_for_preallocated and
i915_gem_object_create_stolen

Other than those minor, looks good.
-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/7] drm/i915: move FBC code out of i915_gem_stolen.c

[Chris Wilson]

On Wed, Jul 01, 2015 at 05:15:21PM -0300, Paulo Zanoni wrote:

Looks much cleaner with the split.

> +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->fbc.compressed_fb);
> +
> +	if (dev_priv->fbc.compressed_llb) {
> +		i915_gem_stolen_remove_node(dev_priv->fbc.compressed_llb);
> +		kfree(dev_priv->fbc.compressed_llb);
> +	}

Any reason why one node is embedded and the other allocated? Just feels
a little inconsistent, so lacks an explanation. Just that one is always
used, and the other on rare gen would probably suffice.
-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 4/7] drm/i915: add the FBC mutex

[Chris Wilson]

On Wed, Jul 01, 2015 at 05:15:23PM -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 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.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

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

See below.

> @@ -683,6 +721,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;

This is now __intel_fbc_update() inside the fbc.lock. One would think
that the internal functions would only be called when FBC is desired.

That looks to be true, except for the new intel_fbc_update(). You can
upgrade this to if (WARN_ON(!HAS_FBC(dev_priv))) return; after adding
the proper guard to intel_fbc_update().
-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 7/7] drm/i915: FBC doesn't need struct_mutex anymore

[Chris Wilson]

On Wed, Jul 01, 2015 at 05:15:26PM -0300, Paulo Zanoni wrote:
> 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.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
5-7 Reviewed-by: Chris wilson <chris@chris-wilson.co.uk>

They all look to be safely guarded by a specific mutex now.
-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/7] drm/i915: add simple wrappers for stolen node insertion/removal

[Paulo Zanoni]

2015-07-01 17:38 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Wed, Jul 01, 2015 at 05:15:20PM -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. These abstractions will also allow us to add
>> locking assertions later.
>>
>> 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        |  4 ++++
>>  drivers/gpu/drm/i915/i915_gem_stolen.c | 44 +++++++++++++++++++++-------------
>>  2 files changed, 32 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 1dbd957..b9de374 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3109,6 +3109,10 @@ 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_mm_node *node);
>
> Might as well pass in dev_priv now to save changing the interface later.

I thought about doing it, but dev_priv is just for the mutex WARN, and
I thought it could be rejected, so I didn't add dev_priv because of
the risk of having the WARN rejected, so dev_priv would be useless.
But I can change this, no problem.

>
>
>>  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..6b43234 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
>> @@ -42,6 +42,22 @@
>>   * 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;
>
> Might as well take advantage of this test here to remove the same check
> from i915_gem_object_create_stolen_for_preallocated and
> i915_gem_object_create_stolen
>
> Other than those minor, looks good.
> -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/7] drm/i915: add simple wrappers for stolen node insertion/removal

[Chris Wilson]

On Thu, Jul 02, 2015 at 10:33:27AM -0300, Paulo Zanoni wrote:
> 2015-07-01 17:38 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Wed, Jul 01, 2015 at 05:15:20PM -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. These abstractions will also allow us to add
> >> locking assertions later.
> >>
> >> 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        |  4 ++++
> >>  drivers/gpu/drm/i915/i915_gem_stolen.c | 44 +++++++++++++++++++++-------------
> >>  2 files changed, 32 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> index 1dbd957..b9de374 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -3109,6 +3109,10 @@ 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_mm_node *node);
> >
> > Might as well pass in dev_priv now to save changing the interface later.
> 
> I thought about doing it, but dev_priv is just for the mutex WARN, and
> I thought it could be rejected, so I didn't add dev_priv because of
> the risk of having the WARN rejected, so dev_priv would be useless.
> But I can change this, no problem.

If you change it over to taking the stolen_mutex inside these functions
(which is the only place we need to), then it is required.
-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/7] drm/i915: move FBC code out of i915_gem_stolen.c

[Paulo Zanoni]

2015-07-01 17:44 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Wed, Jul 01, 2015 at 05:15:21PM -0300, Paulo Zanoni wrote:
>
> Looks much cleaner with the split.
>
>> +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->fbc.compressed_fb);
>> +
>> +     if (dev_priv->fbc.compressed_llb) {
>> +             i915_gem_stolen_remove_node(dev_priv->fbc.compressed_llb);
>> +             kfree(dev_priv->fbc.compressed_llb);
>> +     }
>
> Any reason why one node is embedded and the other allocated? Just feels
> a little inconsistent, so lacks an explanation. Just that one is always
> used, and the other on rare gen would probably suffice.

I really didn't stop to pay attention to the ancient FBC pieces. IMHO
reasoning/explanation/change about this should be on a separate patch,
since this one is just moving the code around.

I only think about the gen2-4 FBC code when I remember it has the
"disable FBC when more than one pipe is visible" restriction which I
can't even find on the documentation I have. I wish we could either
remove it or just remove the whole gen2-4 FBC support (will we ever
have the courage to enable it by default?).

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



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

Re: [PATCH 2/7] drm/i915: move FBC code out of i915_gem_stolen.c

[Chris Wilson]

On Thu, Jul 02, 2015 at 10:39:05AM -0300, Paulo Zanoni wrote:
> 2015-07-01 17:44 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Wed, Jul 01, 2015 at 05:15:21PM -0300, Paulo Zanoni wrote:
> >
> > Looks much cleaner with the split.
> >
> >> +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->fbc.compressed_fb);
> >> +
> >> +     if (dev_priv->fbc.compressed_llb) {
> >> +             i915_gem_stolen_remove_node(dev_priv->fbc.compressed_llb);
> >> +             kfree(dev_priv->fbc.compressed_llb);
> >> +     }
> >
> > Any reason why one node is embedded and the other allocated? Just feels
> > a little inconsistent, so lacks an explanation. Just that one is always
> > used, and the other on rare gen would probably suffice.
> 
> I really didn't stop to pay attention to the ancient FBC pieces. IMHO
> reasoning/explanation/change about this should be on a separate patch,
> since this one is just moving the code around.
> 
> I only think about the gen2-4 FBC code when I remember it has the
> "disable FBC when more than one pipe is visible" restriction which I
> can't even find on the documentation I have. I wish we could either
> remove it or just remove the whole gen2-4 FBC support (will we ever
> have the courage to enable it by default?).

Oh, no worries, that is how the code was is no problem. It just stood
out as inconsistent and hence made me stop and think when reviewing.

Might as well make them the same whilst we are here so I don't ask
stupid questions.
-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/7] drm/i915: add simple wrappers for stolen node insertion/removal

[Paulo Zanoni]

2015-07-01 17:38 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Wed, Jul 01, 2015 at 05:15:20PM -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. These abstractions will also allow us to add
>> locking assertions later.
>>
>> 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        |  4 ++++
>>  drivers/gpu/drm/i915/i915_gem_stolen.c | 44 +++++++++++++++++++++-------------
>>  2 files changed, 32 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 1dbd957..b9de374 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3109,6 +3109,10 @@ 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_mm_node *node);
>
> Might as well pass in dev_priv now to save changing the interface later.
>
>
>>  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..6b43234 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
>> @@ -42,6 +42,22 @@
>>   * 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;
>
> Might as well take advantage of this test here to remove the same check
> from i915_gem_object_create_stolen_for_preallocated and
> i915_gem_object_create_stolen

If we do this, we'll start printing "creating stolen something" and
"failed to alloc stolen node" messages even when stolen is not
present, and we'll also do some useless alloc+free calls.

For the specific case of
i915_gem_object_create_stolen_for_preallocated(), we'll then run
drm_mm_reserve_node() without doing the check first, and a brief look
at the implementation suggests it would probably fail with a
non-initialized mm.stolen (it seems drm_mm_for_each_hole() assumes the
hole_stack list is initialized). So we'd need to reorganize the
function first, and the result would be a code that is a little more
fragile.

It looks like i915_gem_object_create_stolen() would survive without
the drm_mm_initialized(), but it would still have useless alloc/free
calls and debug messages.

Based on that, and on the fact that drm_mm_initialized() is a simple
inline pointer check, I'd vote to not remove the drm_mm_initialized()
calls.

>
> Other than those minor, looks good.
> -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/7] drm/i915: add simple wrappers for stolen node insertion/removal

[Chris Wilson]

On Thu, Jul 02, 2015 at 04:07:08PM -0300, Paulo Zanoni wrote:
> 2015-07-01 17:38 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Wed, Jul 01, 2015 at 05:15:20PM -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. These abstractions will also allow us to add
> >> locking assertions later.
> >>
> >> 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        |  4 ++++
> >>  drivers/gpu/drm/i915/i915_gem_stolen.c | 44 +++++++++++++++++++++-------------
> >>  2 files changed, 32 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> index 1dbd957..b9de374 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -3109,6 +3109,10 @@ 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_mm_node *node);
> >
> > Might as well pass in dev_priv now to save changing the interface later.
> >
> >
> >>  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..6b43234 100644
> >> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> >> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> >> @@ -42,6 +42,22 @@
> >>   * 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;
> >
> > Might as well take advantage of this test here to remove the same check
> > from i915_gem_object_create_stolen_for_preallocated and
> > i915_gem_object_create_stolen
> 
> If we do this, we'll start printing "creating stolen something" and
> "failed to alloc stolen node" messages even when stolen is not
> present, and we'll also do some useless alloc+free calls.

The messages we can delete if you think they are a nuisance, certainly
error ones here tend to be (since we often use stolen then failsafe).
 
> For the specific case of
> i915_gem_object_create_stolen_for_preallocated(), we'll then run
> drm_mm_reserve_node() without doing the check first, and a brief look
> at the implementation suggests it would probably fail with a
> non-initialized mm.stolen (it seems drm_mm_for_each_hole() assumes the
> hole_stack list is initialized). So we'd need to reorganize the
> function first, and the result would be a code that is a little more
> fragile.

Ok, that's a nuisance. I was just trying to think of a way to trim a few
lines of code.

> It looks like i915_gem_object_create_stolen() would survive without
> the drm_mm_initialized(), but it would still have useless alloc/free
> calls and debug messages.
> 
> Based on that, and on the fact that drm_mm_initialized() is a simple
> inline pointer check, I'd vote to not remove the drm_mm_initialized()
> calls.

Ok.
-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 4/7] drm/i915: add the FBC mutex

[Paulo Zanoni]

2015-07-01 17:49 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Wed, Jul 01, 2015 at 05:15:23PM -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 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.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> See below.
>
>> @@ -683,6 +721,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;
>
> This is now __intel_fbc_update() inside the fbc.lock. One would think
> that the internal functions would only be called when FBC is desired.
>
> That looks to be true, except for the new intel_fbc_update(). You can
> upgrade this to if (WARN_ON(!HAS_FBC(dev_priv))) return; after adding
> the proper guard to intel_fbc_update().

__intel_fbc_update() is also called by intel_fbc_flush(). See "[PATCH
8/8] drm/i915: protect FBC functions with HAS_FBC checks" on the new
series. I still have another series just with minor changes like this
which I intend to send after the locking 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/7] drm/i915: add simple wrappers for stolen node insertion/removal

[Daniel Vetter]

On Wed, Jul 01, 2015 at 05:15:20PM -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. These abstractions will also allow us to add
> locking assertions later.
> 
> Requested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

I guess you'll follow up with a nice kerneldoc patch for i915_gem_stolen?
You're the expert on this code now ;-)

Cheers, Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h        |  4 ++++
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 44 +++++++++++++++++++++-------------
>  2 files changed, 32 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1dbd957..b9de374 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3109,6 +3109,10 @@ 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_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..6b43234 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -42,6 +42,22 @@
>   * 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_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 +184,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 +194,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 +231,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 +253,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->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 +282,10 @@ 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->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->fbc.compressed_llb);
>  		kfree(dev_priv->fbc.compressed_llb);
>  	}
>  
> @@ -387,7 +400,7 @@ static void
>  i915_gem_object_release_stolen(struct drm_i915_gem_object *obj)
>  {
>  	if (obj->stolen) {
> -		drm_mm_remove_node(obj->stolen);
> +		i915_gem_stolen_remove_node(obj->stolen);
>  		kfree(obj->stolen);
>  		obj->stolen = NULL;
>  	}
> @@ -449,8 +462,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 +472,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(stolen);
>  	kfree(stolen);
>  	return NULL;
>  }
> @@ -505,7 +517,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(stolen);
>  		kfree(stolen);
>  		return NULL;
>  	}
> @@ -546,7 +558,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(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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915: move FBC code out of i915_gem_stolen.c

[Daniel Vetter]

On Thu, Jul 02, 2015 at 10:39:05AM -0300, Paulo Zanoni wrote:
> 2015-07-01 17:44 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Wed, Jul 01, 2015 at 05:15:21PM -0300, Paulo Zanoni wrote:
> >
> > Looks much cleaner with the split.
> >
> >> +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->fbc.compressed_fb);
> >> +
> >> +     if (dev_priv->fbc.compressed_llb) {
> >> +             i915_gem_stolen_remove_node(dev_priv->fbc.compressed_llb);
> >> +             kfree(dev_priv->fbc.compressed_llb);
> >> +     }
> >
> > Any reason why one node is embedded and the other allocated? Just feels
> > a little inconsistent, so lacks an explanation. Just that one is always
> > used, and the other on rare gen would probably suffice.
> 
> I really didn't stop to pay attention to the ancient FBC pieces. IMHO
> reasoning/explanation/change about this should be on a separate patch,
> since this one is just moving the code around.
> 
> I only think about the gen2-4 FBC code when I remember it has the
> "disable FBC when more than one pipe is visible" restriction which I
> can't even find on the documentation I have. I wish we could either
> remove it or just remove the whole gen2-4 FBC support (will we ever
> have the courage to enable it by default?).

Yeah I think killing gen2-4 fbc would be ok. Same for g4x fbc (hw too
broken) and ilk fbc (same really according to Art). Then we'd only need to
deal with fbc on gen6+, which is reasonably sane and consistent.

But we can rip the code out whenever you want, no hurry.
-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