[006/190] drm/i915: Add GEM debugging Kconfig option
diff mbox

Message ID 1452503961-14837-6-git-send-email-chris@chris-wilson.co.uk
State New
Headers show

Commit Message

Chris Wilson Jan. 11, 2016, 9:16 a.m. UTC
Currently there is a #define to enable extra BUG_ON for debugging
requests and associated activities. I want to expand its use to cover
all of GEM internals (so that we can saturate the code with asserts).
We can add a Kconfig option to make it easier to enable - with the usual
caveats of not enabling unless explicitly requested.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/Kconfig.debug |  8 ++++++++
 drivers/gpu/drm/i915/i915_drv.h    |  6 ++++++
 drivers/gpu/drm/i915/i915_gem.c    | 12 +++++-------
 3 files changed, 19 insertions(+), 7 deletions(-)

Comments

Dave Gordon Jan. 12, 2016, 5:44 p.m. UTC | #1
On 11/01/16 09:16, Chris Wilson wrote:
> Currently there is a #define to enable extra BUG_ON for debugging
> requests and associated activities. I want to expand its use to cover
> all of GEM internals (so that we can saturate the code with asserts).
> We can add a Kconfig option to make it easier to enable - with the usual
> caveats of not enabling unless explicitly requested.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/Kconfig.debug |  8 ++++++++
>   drivers/gpu/drm/i915/i915_drv.h    |  6 ++++++
>   drivers/gpu/drm/i915/i915_gem.c    | 12 +++++-------
>   3 files changed, 19 insertions(+), 7 deletions(-)
> diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug
> index 1f10ee228eda..7fa6b97635e5 100644
> --- a/drivers/gpu/drm/i915/Kconfig.debug
> +++ b/drivers/gpu/drm/i915/Kconfig.debug
> @@ -10,3 +10,11 @@ config DRM_I915_WERROR
>   	---help---
>   	  Add -Werror to the build flags for (and only for) i915.ko.
>   	  Do not enable this unless you are writing code for the i915.ko module.
> +
> +config DRM_I915_DEBUG_GEM
> +	bool "Insert extra checks into the GEM internals"
> +	default n
> +	depends on DRM_I915_WERROR

This comes up as an option only if DRM_I915_WERROR is already selected? 
Surely it should be orthogonal to compile-time checks, with each as 
independent options but both restricted to EXPERT mode. So the line 
above should be "depends on EXPERT" not "depends on DRM_I915_WERROR"?

> +	---help---
> +	  Enable extra sanity checks (including BUGs) that may slow the
> +          system down and if hit hang the machine.

"hang the machine if hit". Unless you want commas round "if hit"?

Otherwise looks OK.

.Dave.

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug
index 1f10ee228eda..7fa6b97635e5 100644
--- a/drivers/gpu/drm/i915/Kconfig.debug
+++ b/drivers/gpu/drm/i915/Kconfig.debug
@@ -10,3 +10,11 @@  config DRM_I915_WERROR
 	---help---
 	  Add -Werror to the build flags for (and only for) i915.ko.
 	  Do not enable this unless you are writing code for the i915.ko module.
+
+config DRM_I915_DEBUG_GEM
+	bool "Insert extra checks into the GEM internals"
+	default n
+	depends on DRM_I915_WERROR
+	---help---
+	  Enable extra sanity checks (including BUGs) that may slow the
+          system down and if hit hang the machine.
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ec20814adb0c..1a6168affadd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2271,6 +2271,12 @@  struct drm_i915_gem_request {
 
 };
 
+#ifdef CONFIG_DRM_I915_DEBUG_GEM
+#define GEM_BUG_ON(expr) BUG_ON(expr)
+#else
+#define GEM_BUG_ON(expr)
+#endif
+
 int i915_gem_request_alloc(struct intel_engine_cs *ring,
 			   struct intel_context *ctx,
 			   struct drm_i915_gem_request **req_out);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index fd24877eb0a0..99fd6aa4dd62 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -38,8 +38,6 @@ 
 #include <linux/pci.h>
 #include <linux/dma-buf.h>
 
-#define RQ_BUG_ON(expr)
-
 static void i915_gem_object_flush_gtt_write_domain(struct drm_i915_gem_object *obj);
 static void i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *obj);
 static void
@@ -1520,7 +1518,7 @@  i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj,
 
 			i915_gem_object_retire__read(obj, i);
 		}
-		RQ_BUG_ON(obj->active);
+		GEM_BUG_ON(obj->active);
 	}
 
 	return 0;
@@ -2430,8 +2428,8 @@  void i915_vma_move_to_active(struct i915_vma *vma,
 static void
 i915_gem_object_retire__write(struct drm_i915_gem_object *obj)
 {
-	RQ_BUG_ON(obj->last_write_req == NULL);
-	RQ_BUG_ON(!(obj->active & intel_ring_flag(obj->last_write_req->ring)));
+	GEM_BUG_ON(obj->last_write_req == NULL);
+	GEM_BUG_ON(!(obj->active & intel_ring_flag(obj->last_write_req->ring)));
 
 	i915_gem_request_assign(&obj->last_write_req, NULL);
 	intel_fb_obj_flush(obj, true, ORIGIN_CS);
@@ -2442,8 +2440,8 @@  i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring)
 {
 	struct i915_vma *vma;
 
-	RQ_BUG_ON(obj->last_read_req[ring] == NULL);
-	RQ_BUG_ON(!(obj->active & (1 << ring)));
+	GEM_BUG_ON(obj->last_read_req[ring] == NULL);
+	GEM_BUG_ON(!(obj->active & (1 << ring)));
 
 	list_del_init(&obj->ring_list[ring]);
 	i915_gem_request_assign(&obj->last_read_req[ring], NULL);