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

[Zhao, Halley]

Are you ok with this patch?


>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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-debug-use-VA_INTEL_DRIVER_ENABLE_ASSERT-env-to-enabl.patch
Type: application/octet-stream
Size: 2164 bytes
Desc: 0001-debug-use-VA_INTEL_DRIVER_ENABLE_ASSERT-env-to-enabl.patch
URL: <http://lists.freedesktop.org/archives/libva/attachments/20140319/ffd24ab6/attachment.obj>