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

[Zhao, Yakui]

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?

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

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)