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.z...@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.
>
> > + 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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/libva
_______________________________________________
Libva mailing list
Libva@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libva
