[Libva] [PATCH] debug: add g_intel_debug_option_flags for simple driver debug

[Xiang, Haihao]

On Wed, 2014-05-28 at 08:39 +0800, Zhao, Yakui wrote: 
> On Tue, 2014-05-27 at 02:59 -0600, Zhao, Halley wrote:
> > From: "Zhao, Halley" <halley.zhao at intel.com>
> > 
> > VA_INTEL_DEBUG_ASSERT decides assert() is enabled or not
> > VA_INTEL_DEBUG_BENCH  decides skip putsurface or not
> 
> As a whole, this patch looks good to me except one small concern.
> 
> > ---
> >  src/i965_output_dri.c |  5 ++---
> >  src/intel_driver.c    | 10 ++++++++++
> >  src/intel_driver.h    |  9 +++++++--
> >  3 files changed, 19 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/i965_output_dri.c b/src/i965_output_dri.c
> > index 717ee9a..9b00d1b 100644
> > --- a/src/i965_output_dri.c
> > +++ b/src/i965_output_dri.c
> > @@ -137,8 +137,7 @@ i965_put_surface_dri(
> >       * will get here
> >       */
> >      obj_surface = SURFACE(surface);
> > -    if (!obj_surface || !obj_surface->bo)
> > -        return VA_STATUS_SUCCESS;
> > +    ASSERT_RET(obj_surface && obj_surface->bo, VA_STATUS_SUCCESS);
> >  
> >      _i965LockMutex(&i965->render_mutex);
> >  
> > @@ -204,7 +203,7 @@ i965_put_surface_dri(
> >          }
> >      }
> >  
> > -    if (!getenv("INTEL_DEBUG_BENCH"))
> > +    if (!(g_intel_debug_option_flags & VA_INTEL_DEBUG_OPTION_BENCH))
> >          dri_vtable->swap_buffer(ctx, dri_drawable);
> >      obj_surface->flags |= SURFACE_DISPLAYED;
> >  
> > diff --git a/src/intel_driver.c b/src/intel_driver.c
> > index e3e082d..437fd10 100644
> > --- a/src/intel_driver.c
> > +++ b/src/intel_driver.c
> > @@ -34,6 +34,7 @@
> >  #include "intel_batchbuffer.h"
> >  #include "intel_memman.h"
> >  #include "intel_driver.h"
> > +uint32_t g_intel_debug_option_flags = 0;
> >  
> >  static Bool
> >  intel_driver_get_param(struct intel_driver_data *intel, int param, int *value)
> > @@ -75,7 +76,16 @@ intel_driver_init(VADriverContextP ctx)
> >      struct intel_driver_data *intel = intel_driver_data(ctx);
> >      struct drm_state * const drm_state = (struct drm_state *)ctx->drm_state;
> >      int has_exec2 = 0, has_bsd = 0, has_blt = 0, has_vebox = 0;
> > +    char *env_str = NULL;
> >  
> > +    g_intel_debug_option_flags = 0;
> > +    if ((env_str = getenv("VA_INTEL_DEBUG_ASSERT")) && (*env_str == '1'))
> > +        g_intel_debug_option_flags |= VA_INTEL_DEBUG_OPTION_ASSERT;
> > +
> > +    if ((env_str = getenv("VA_INTEL_DEBUG_BENCH")) && (*env_str == '1'))
> > +        g_intel_debug_option_flags |= VA_INTEL_DEBUG_OPTION_BENCH;
> > +
> 
> Can we still use the definition of "INTEL_DEBUG_BENCH" instead of
> "VA_INTEL_DEBUG_BENCH" so that the current usage is not affected?

I prefer the same prefix for environment variables used in the driver
and VA_INTEL_DEBUG_ looks good me. (BTW INTEL_DEBUG has been used in a
mesa driver, and I don't want they are mixed-up together). Actually the
impact of the change is small to the current usage.

Another choice is that debug flags can be bit-ORed, we can use a
variable for all debug options.

> 
> > +    fprintf(stderr, "g_intel_debug_option_flags:%x\n", g_intel_debug_option_flags);
> 
> It is not necessary to print the above option. Otherwise the user will
> be confused by the message printed through err console.
> Of course it can also be printed when the debug option is enabled.

Agree.

> 
> Thanks.
>     Yakui
> 
> >      assert(drm_state);
> >      assert(VA_CHECK_DRM_AUTH_TYPE(ctx, VA_DRM_AUTH_DRI1) ||
> >             VA_CHECK_DRM_AUTH_TYPE(ctx, VA_DRM_AUTH_DRI2) ||
> > diff --git a/src/intel_driver.h b/src/intel_driver.h
> > index 6653f43..487bf03 100644
> > --- a/src/intel_driver.h
> > +++ b/src/intel_driver.h
> > @@ -76,9 +76,14 @@ struct intel_batchbuffer;
> >  #define True 1
> >  #define False 0
> >  
> > +extern uint32_t g_intel_debug_option_flags;
> > +#define VA_INTEL_DEBUG_OPTION_ASSERT    (1 << 0)
> > +#define VA_INTEL_DEBUG_OPTION_BENCH     (1 << 1)
> > +
> >  #define ASSERT_RET(value, fail_ret) do {    \
> > -        if (!(value)) {                 \
> > -            assert(0);                      \
> > +        if (!(value)) {                     \
> > +            if (g_intel_debug_option_flags & VA_INTEL_DEBUG_OPTION_ASSERT)       \
> > +                assert(value);              \
> >              return fail_ret;                \
> >          }                                   \
> >      } while (0)
> 
> 
> _______________________________________________
> Libva mailing list
> Libva at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/libva