[Libva] [PATCH 0/2] add user specified tiling/stride support, clean up assert

[Zhao, Halley]

Yes, debug log is good.

Before there is log, something (env for assert on/off) is better than nothing.


-----Original Message-----
From: Zhao, Yakui 
Sent: Thursday, March 20, 2014 9:08 AM
To: Zhao, Halley
Cc: 'Gwenole Beauchesne'; 'libva at lists.freedesktop.org'
Subject: Re: [Libva] [PATCH 0/2] add user specified tiling/stride support, clean up assert

On Wed, 2014-03-19 at 02:46 -0600, Zhao, Halley wrote:
> Are you ok with this patch?
> 

Hi, Halley

    Thanks for the patch. The env is added to control whether the
assert(XXX) is enabled. This is still an improvement.

    It seems that Gwenole's suggestion is to remove the corresponding assert and use the debug log in driver level to collect the corresponding info(for example: incorrect parameter, position and so on). In such case the debug log is helpful to root cause/analyse the problem.

Thanks.
    Yakui   
> 
> From e7e2ab35d2a355879bfe9d6de2cf05996e30f3d4 Mon Sep 17 00:00:00 2001
> From: "Zhao, Halley" <halley.zhao at intel.com>
> Date: Wed, 19 Mar 2014 16:31:54 +0800
> Subject: [PATCH] debug: use VA_INTEL_DRIVER_ENABLE_ASSERT env to 
> enable assert
> 
> ---
>  src/intel_driver.c | 7 +++++++
>  src/intel_driver.h | 9 ++++++---
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/src/intel_driver.c b/src/intel_driver.c index 
> 8650dba..bec45a5 100644
> --- a/src/intel_driver.c
> +++ b/src/intel_driver.c
> @@ -33,7 +33,9 @@
>  
>  #include "intel_batchbuffer.h"
>  #include "intel_memman.h"
> +#define _INTEL_DRIVER_C_
>  #include "intel_driver.h"
> +int g_intel_driver_enable_assert = 0;
>  
>  static Bool
>  intel_driver_get_param(struct intel_driver_data *intel, int param, 
> int *value) @@ -73,6 +75,10 @@ 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;
> +
> +    if ((env_str = getenv("VA_INTEL_DRIVER_ENABLE_ASSERT")))
> +        g_intel_driver_enable_assert = (*env_str == '1');
>  
>      assert(drm_state);
>      assert(VA_CHECK_DRM_AUTH_TYPE(ctx, VA_DRM_AUTH_DRI1) || @@ -112,4 
> +118,5 @@ intel_driver_terminate(VADriverContextP ctx)
>  
>      intel_memman_terminate(intel);
>      pthread_mutex_destroy(&intel->ctxmutex);
> +    g_intel_driver_enable_assert = 0;
>  }
> diff --git a/src/intel_driver.h b/src/intel_driver.h index 
> a1764b2..241a067 100644
> --- a/src/intel_driver.h
> +++ b/src/intel_driver.h
> @@ -75,10 +75,13 @@ struct intel_batchbuffer;  #define Bool int  
> #define True 1  #define False 0
> -
> +#ifndef _INTEL_DRIVER_C_
> +    extern int g_intel_driver_enable_assert; #endif
>  #define ASSERT_RET(value, fail_ret) do {    \
> -        if (!(value)) {                 \
> -            assert(0);                      \
> +        if (!(value)) {                     \
> +            if (g_intel_driver_enable_assert) \
> +                assert(value);              \
>              return fail_ret;                \
>          }                                   \
>      } while (0)
> --
> 1.8.3.2
> 
> 
> > -----Original Message-----
> > From: Zhao, Halley
> > Sent: Wednesday, March 19, 2014 2:17 PM
> > To: Gwenole Beauchesne
> > Cc: libva at lists.freedesktop.org
> > Subject: RE: [Libva] [PATCH 0/2] add user specified tiling/stride 
> > support, clean up assert
> > 
> > Thanks for your comments.
> > 
> > The 2nd patch is so big that it is blocked by mail man, so I sent to 
> > some stake holders for review.
> > Your VA_INTEL_DEBUG=asserts sounds good, we can consider it.
> > 
> > 
> > -----Original Message-----
> > From: Gwenole Beauchesne [mailto:gb.devel at gmail.com]
> > Sent: Wednesday, March 19, 2014 12:52 PM
> > To: Zhao, Halley
> > Cc: libva at lists.freedesktop.org
> > Subject: Re: [Libva] [PATCH 0/2] add user specified tiling/stride 
> > support, clean up assert
> > 
> > Hi,
> > 
> > 2014-03-14 10:19 GMT+01:00 Zhao, Halley <halley.zhao at intel.com>:
> > 
> > > when I implements feature for user specified tiling/stride 
> > > support, haihao mentioned that I'd better 'return fail' instead of assert().
> > > so, I tried to clean up the assert in i965_drv_video.c as well.
> > 
> > The assert_ret() patch did not appear on the list but was committed 
> > as 12c8122.
> > 
> > What Haihao mentioned was right, but what I had precisely indicated 
> > beforehand was to not have the VA driver fail at all in the default 
> > case too. Besides, the proposed patch turned down the indication of 
> > what went wrong. i.e. assert(0) is totally meaningless, unless you 
> > get the original source code the binary was built with, and look for 
> > the specified file/line number information.
> > 
> > The proper approach would have been to log the error in any case, 
> > possibly controlled by an environment variable (e.g.
> > VA_INTEL_DEBUG=asserts), but definitely not an assert(0) again. That 
> > way, you can still get informative enough bug reports. And, most 
> > importantly, you also get a chance to test/exercice the error 
> > handling code of upper layers in the general case too.
> > 
> > Thanks,
> > Gwenole.