[Libva] [PATCH 2/2 v2][libva-intel-driver] i965_test_config: return properly unsupported profile

[Eoff, Ullysses A]

> -----Original Message-----
> From: Libva [mailto:libva-bounces at lists.freedesktop.org] On Behalf Of Daniel Charles
> Sent: Wednesday, October 26, 2016 3:46 PM
> To: libva at lists.freedesktop.org
> Subject: [Libva] [PATCH 2/2 v2][libva-intel-driver] i965_test_config: return properly unsupported profile
> 
> jpege and avce config tests to check against all supported
> entrypoints for a profile.  UNSUPPORTED_PROFILE is expected
> when no entrypoints are available for a given profile, else
> expect UNSUPPORTED_ENTRYPOINT.
> 
> Signed-off-by: Daniel Charles <daniel.charles at intel.com>
> ---
>  test/i965_avce_config_test.cpp  | 25 ++++++++++++++++++-------
>  test/i965_jpege_config_test.cpp | 17 ++++++++---------
>  2 files changed, 26 insertions(+), 16 deletions(-)
> 
> diff --git a/test/i965_avce_config_test.cpp b/test/i965_avce_config_test.cpp
> index b30abbc..a6db97d 100644
> --- a/test/i965_avce_config_test.cpp
> +++ b/test/i965_avce_config_test.cpp
> @@ -45,8 +45,13 @@ VAStatus HasEncodeSupport()
>      struct i965_driver_data *i965(*env);
>      EXPECT_PTR(i965);
> 
> -    return HAS_H264_ENCODING(i965) ? VA_STATUS_SUCCESS :
> -        EntrypointNotSupported();
> +    if (HAS_H264_ENCODING(i965))
> +        return VA_STATUS_SUCCESS;
> +    else if (!HAS_H264_ENCODING(i965) && !HAS_H264_DECODING(i965)

At this point, we already know !HAS_H264_ENCODING(i965) is true because
the first condition is false if we get here.

> +             && !HAS_LP_H264_ENCODING(i965))
> +        return ProfileNotSupported();
> +    else
> +        return EntrypointNotSupported();
>  }
> 
>  VAStatus HasLPEncodeSupport()
> @@ -59,9 +64,11 @@ VAStatus HasLPEncodeSupport()
> 
>      if (IS_SKL(i965->intel.device_info))
>          return VA_STATUS_SUCCESS;
> -
> -    return HAS_LP_H264_ENCODING(i965) ? VA_STATUS_SUCCESS :
> -        EntrypointNotSupported();
> +    else if (!HAS_H264_ENCODING(i965) && !HAS_H264_DECODING(i965)
> +             && !HAS_LP_H264_ENCODING(i965))
> +        return ProfileNotSupported();
> +    else
> +        return EntrypointNotSupported();

Need to return VA_STATUS_SUCCESS when HAS_LP_H264_ENCODING(i965)
is true.  Maybe it's better not to use "else if ... else" here.  Perhaps...

if (IS_SKL(i965->intel.device_info))
	return VA_STATUS_SUCCESS;
if (HAS_LP_H264_ENCODING(i965))
	return VA_STATUS_SUCCESS;
// we already know HAS_LP_H264_ENCODING is false here
if (!HAS_H264_DECODING(i965) && !HAS_LP_H264_ENCODING(i965))
	return ProfileNotSupported();
return EntrypointNoSupported();

>  }
> 
>  VAStatus HasMVCEncodeSupport()
> @@ -72,8 +79,12 @@ VAStatus HasMVCEncodeSupport()
>      struct i965_driver_data *i965(*env);
>      EXPECT_PTR(i965);
> 
> -    return HAS_H264_MVC_ENCODING(i965) ? VA_STATUS_SUCCESS :
> -        EntrypointNotSupported();
> +    if (HAS_H264_MVC_ENCODING(i965))
> +        return VA_STATUS_SUCCESS;
> +    else if (!HAS_H264_MVC_ENCODING(i965) && !HAS_H264_MVC_DECODING(i965))

Should this be !HAS_H264_MVC_DECODING_PROFILE(i965) instead of
!HAS_H264_MVC_DECODING(i965)?  You might want to confirm.

> +        return ProfileNotSupported();
> +    else
> +        return EntrypointNotSupported();
>  }
> 

>  static const std::vector<ConfigTestInput> inputs = {

Shouldn't there be modifications here for the "EntrypointNotSupported"
input cases?  For example,

{VAProfileH264ConstrainedBaseline, VAEntrypointEncPicture, &EntrypointNotSupported},

...could be either ProfileNotSupported or EntrypointNotSupported now because
of patch 1.

> diff --git a/test/i965_jpege_config_test.cpp b/test/i965_jpege_config_test.cpp
> index 924eccb..fdf98b6 100644
> --- a/test/i965_jpege_config_test.cpp
> +++ b/test/i965_jpege_config_test.cpp
> @@ -27,11 +27,6 @@
>  namespace JPEG {
>  namespace Encode {
> 
> -VAStatus EntrypointNotSupported()
> -{
> -    return VA_STATUS_ERROR_UNSUPPORTED_ENTRYPOINT;
> -}
> -
>  VAStatus HasEncodeSupport()
>  {
>      I965TestEnvironment *env(I965TestEnvironment::instance());
> @@ -40,14 +35,18 @@ VAStatus HasEncodeSupport()
>      struct i965_driver_data *i965(*env);
>      EXPECT_PTR(i965);
> 
> -    return HAS_JPEG_ENCODING(i965) ? VA_STATUS_SUCCESS :
> -        EntrypointNotSupported();
> +    if (HAS_JPEG_ENCODING(i965))
> +        return VA_STATUS_SUCCESS;
> +    else if (!HAS_JPEG_ENCODING(i965) && !HAS_JPEG_DECODING(i965))

At this point, we already know !HAS_JPEG_ENCODING(i965) is true... so no
need to check it.

> +        return VA_STATUS_ERROR_UNSUPPORTED_PROFILE;
> +    else
> +        return VA_STATUS_ERROR_UNSUPPORTED_ENTRYPOINT;
>  }
> 
>  static const std::vector<ConfigTestInput> inputs = {
>      {VAProfileJPEGBaseline, VAEntrypointEncPicture, &HasEncodeSupport},
> -    {VAProfileJPEGBaseline, VAEntrypointEncSlice, &EntrypointNotSupported},
> -    {VAProfileJPEGBaseline, VAEntrypointEncSliceLP, &EntrypointNotSupported},
> +    {VAProfileJPEGBaseline, VAEntrypointEncSlice, &HasEncodeSupport},
> +    {VAProfileJPEGBaseline, VAEntrypointEncSliceLP, &HasEncodeSupport},

EncSlice and EncSlicLP are not supported on any platform for JPEG.  This will result in
VA_STATUS_SUCCESS if HAS_JPEG_ENCODING == true, regardless of entrypoint.
Perhaps create a new function (e.g. NotSupported) to return either unsupported
profile or unsupported entrypoint depending on the conditions you've introduced
from the previous patch.  Keep in mind, the callback functions don't tell the whole
story.  It is here, where we define the inputs, which completes the "actual"
expectation we want for the profile/entrypoint combination.

>  };
> 
>  INSTANTIATE_TEST_CASE_P(
> --
> 2.5.5
> 
> _______________________________________________
> Libva mailing list
> Libva at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/libva