[Intel-gfx] [PATCH 03/10] drm/i915: Add GEM debugging Kconfig option

[Chris Wilson]

On Mon, Apr 11, 2016 at 11:12:48AM +0300, Joonas Lahtinen wrote:
> 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 at chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> A few comments below, still:
> 
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen at 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.

I was using it to hide the option unless in "developer" mode. When we
hit these BUG_ON we can often limp on without any noticeable effect
(yes, they are precursors of infinite loops, memory corruption and
whatnot but not always ;)

> > +#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.

The choice is whether we want side-effects from expr to be evaluated. I
voted for no, as that allows us to write complicated expr such that only
the BUG_ON() need to jump through 29 pointers and function calls to sanity
check calling conditions. And partly why I want Werror enabled so that
I have to fix any bugs in the BUG_ON() for the code to compile!
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre