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

[Zhao, Yakui]

On Tue, 2014-05-27 at 19:49 -0600, Xiang, Haihao wrote:
> 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.

OK. This make sense. So continue to use the "VA_INTEL_DEBUG_BENCH" as
the impact is small.

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