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

2014-03-26 Thread 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@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.z...@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@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.de...@gmail.com]
  Sent: Wednesday, March 19, 2014 12:52 PM
  To: Zhao, Halley
  Cc: libva@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.z...@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

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

2014-03-19 Thread Zhao, Halley 
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.de...@gmail.com] 
Sent: Wednesday, March 19, 2014 12:52 PM
To: Zhao, Halley
Cc: libva@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.z...@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.
___
Libva mailing list
Libva@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libva

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

2014-03-19 Thread Zhao Yakui 
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.z...@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@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.de...@gmail.com]
  Sent: Wednesday, March 19, 2014 12:52 PM
  To: Zhao, Halley
  Cc: libva@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.z...@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.


___
Libva mailing list
Libva@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libva

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

2014-03-18 Thread Gwenole Beauchesne 
Hi,

2014-03-14 10:19 GMT+01:00 Zhao, Halley halley.z...@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.
___
Libva mailing list
Libva@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libva