[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 Eoff, Ullysses A
> Sent: Wednesday, October 26, 2016 6:53 PM
> To: Charles, Daniel <daniel.charles at intel.com>; libva at lists.freedesktop.org
> Subject: Re: [Libva] [PATCH 2/2 v2][libva-intel-driver] i965_test_config: return properly unsupported profile
> 
> > -----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();

Oops, I meant the following condition here...

if (!HAS_H264_DECODING(i965) && !HAS _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
> _______________________________________________
> Libva mailing list
> Libva at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/libva