Re: [PATCH 03/10] drm/i915: Add GEM debugging Kconfig option

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On la, 2016-04-09 at 12:19 +0100, 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>

A few comments below, still:

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/Kconfig.debug | 12 ++++++++++++
>  drivers/gpu/drm/i915/i915_drv.h    |  1 +
>  drivers/gpu/drm/i915/i915_gem.c    | 12 +++++-------
>  drivers/gpu/drm/i915/i915_gem.h    | 34 ++++++++++++++++++++++++++++++++++
>  4 files changed, 52 insertions(+), 7 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/i915_gem.h
> 
> diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug
> index 61485c8ba3a8..8f404103341d 100644
> --- a/drivers/gpu/drm/i915/Kconfig.debug
> +++ b/drivers/gpu/drm/i915/Kconfig.debug
> @@ -27,3 +27,15 @@ config DRM_I915_DEBUG
>  
>            If in doubt, say "N".
>  
> +config DRM_I915_DEBUG_GEM
> +        bool "Insert extra checks into the GEM internals"
> +        default n
> +        depends on DRM_I915_WERROR

Not sure if this needs to be explicit.

> +        help
> +          Enable extra sanity checks (including BUGs) along the GEM driver
> +          paths that may slow the system down and if hit hang the machine.
> +
> +          Recommended for driver developers only.
> +
> +          If in doubt, say "N".
> +
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1753077aebbc..fcecc0a7332f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -57,6 +57,7 @@
>  #include "intel_lrc.h"
>  #include "intel_ringbuffer.h"
>  
> +#include "i915_gem.h"
>  #include "i915_gem_gtt.h"
>  #include "i915_gem_render_state.h"
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 5a65a7663b88..716b7e00dd88 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -38,8 +38,6 @@
>  #include 
>  #include 
>  
> -#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
> @@ -1521,7 +1519,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;
> @@ -2422,8 +2420,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_engine_flag(obj->last_write_req->engine)));
> +	GEM_BUG_ON(obj->last_write_req == NULL);
> +	GEM_BUG_ON(!(obj->active & intel_engine_flag(obj->last_write_req->engine)));
>  
>  	i915_gem_request_assign(&obj->last_write_req, NULL);
>  	intel_fb_obj_flush(obj, true, ORIGIN_CS);
> @@ -2434,8 +2432,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->engine_list[ring]);
>  	i915_gem_request_assign(&obj->last_read_req[ring], NULL);
> diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h
> new file mode 100644
> index 000000000000..8292e797d9b5
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_gem.h
> @@ -0,0 +1,34 @@
> +/*
> + * Copyright © 2016 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + */
> +
> +#ifndef __I915_GEM_H__
> +#define __I915_GEM_H__
> +
> +#ifdef CONFIG_DRM_I915_DEBUG_GEM
> +#define GEM_BUG_ON(expr) BUG_ON(expr)
> +#else
> +#define GEM_BUG_ON(expr)

There seems to be no consistent way of doing this within kernel so
guess this is fin, s/(expr)//g would do, too.

Regards, Joonas

> +#endif
> +
> +#endif /* __I915_GEM_H__ */
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx