[Mesa-dev] [PATCH] st/va: Default to YUV420 RT format when creating a config

[Julien Isorce]

Hi Mark,

Yes I actually saw that too in the intel driver though I think it does not
add VA_RT_FORMAT_RGB32 ? Or I missed something ?
Maybe this is a bug. In any case yes as said before " the intel va driver
always return the full list for vpp." from vaQuerySurfaceAttributes
no matter the format selected when creating the config.

So combining all it should probably be something like this:

diff --git a/src/gallium/state_trackers/va/config.c
b/src/gallium/state_trackers/va/config.c
index 2f96eb6..11afc81 100644
--- a/src/gallium/state_trackers/va/config.c
+++ b/src/gallium/state_trackers/va/config.c
@@ -185,6 +185,8 @@ vlVaCreateConfig(VADriverContextP ctx, VAProfile
profile, VAEntrypoint entrypoin
    if (profile == VAProfileNone && entrypoint == VAEntrypointVideoProc) {
       config->entrypoint = VAEntrypointVideoProc;
       config->profile = PIPE_VIDEO_PROFILE_UNKNOWN;
+      config->rt_format = VA_RT_FORMAT_YUV420 | VA_RT_FORMAT_RGB32;
+
       for (int i = 0; i < num_attribs; i++) {
          if (attrib_list[i].type == VAConfigAttribRTFormat) {
             if (attrib_list[i].value & (VA_RT_FORMAT_YUV420 |
VA_RT_FORMAT_RGB32)) {
diff --git a/src/gallium/state_trackers/va/surface.c
b/src/gallium/state_trackers/va/surface.c
index 5e92980..f8513d9 100644
--- a/src/gallium/state_trackers/va/surface.c
+++ b/src/gallium/state_trackers/va/surface.c
@@ -419,7 +419,7 @@ vlVaQuerySurfaceAttributes(VADriverContextP ctx,
VAConfigID config_id,
    /* vlVaCreateConfig returns PIPE_VIDEO_PROFILE_UNKNOWN
     * only for VAEntrypointVideoProc. */
    if (config->profile == PIPE_VIDEO_PROFILE_UNKNOWN) {
-      if (config->rt_format == VA_RT_FORMAT_RGB32) {
+      if (config->rt_format & VA_RT_FORMAT_RGB32) {
          for (j = 0; j < ARRAY_SIZE(vpp_surface_formats); ++j) {
             attribs[i].type = VASurfaceAttribPixelFormat;
             attribs[i].value.type = VAGenericValueTypeInteger;
@@ -427,7 +427,8 @@ vlVaQuerySurfaceAttributes(VADriverContextP ctx,
VAConfigID config_id,
             attribs[i].value.value.i =
PipeFormatToVaFourcc(vpp_surface_formats[j]);
             i++;
          }
-      } else if (config->rt_format == VA_RT_FORMAT_YUV420) {
+      }
+      if (config->rt_format & VA_RT_FORMAT_YUV420) {
          attribs[i].type = VASurfaceAttribPixelFormat;
          attribs[i].value.type = VAGenericValueTypeInteger;
          attribs[i].flags = VA_SURFACE_ATTRIB_GETTABLE |
VA_SURFACE_ATTRIB_SETTABLE;

Will it be ok for your case ?

Cheers
Julien






On 17 October 2016 at 15:54, Mark Thompson <sw at jkqxz.net> wrote:

> On 17/10/16 15:42, Mark Thompson wrote:
> > The default will only be used if the VAConfigRTFormat attribute is not
> > provided by the user.
> > ---
> > On 17/10/16 15:21, Julien Isorce wrote:
> >> Hi Mark,
> >>
> >> Thx for the patch. I can see it has already landed.
> >>
> >> I just tried it with gstreamer-vaapi and it causes problem since they
> create the
> >> config like this for VPP:
> >>
> >> va_status = vaCreateConfig (filter->va_display, VAProfileNone,
> >>       VAEntrypointVideoProc, NULL, 0, &filter->va_config);
> >>
> >> As you can see num attrivs is 0 so it makes vaQuerySurfaceAttributes to
> return
> >> no supported format
> >> because config->rt_format is 0.
> >>
> >> So I plan to make a patch that looks like this:
> >>
> >>
> >> --- a/src/gallium/state_trackers/va/surface.c
> >> +++ b/src/gallium/state_trackers/va/surface.c
> >> @@ -419,7 +419,7 @@ vlVaQuerySurfaceAttributes(VADriverContextP ctx,
> VAConfigID
> >> config_id,
> >>     /* vlVaCreateConfig returns PIPE_VIDEO_PROFILE_UNKNOWN
> >>      * only for VAEntrypointVideoProc. */
> >>     if (config->profile == PIPE_VIDEO_PROFILE_UNKNOWN) {
> >> -      if (config->rt_format == VA_RT_FORMAT_RGB32) {
> >> +      if (!config->rt_format || config->rt_format ==
> VA_RT_FORMAT_RGB32) {
> >>           for (j = 0; j < ARRAY_SIZE(vpp_surface_formats); ++j) {
> >>              attribs[i].type = VASurfaceAttribPixelFormat;
> >>              attribs[i].value.type = VAGenericValueTypeInteger;
> >> @@ -427,7 +427,8 @@ vlVaQuerySurfaceAttributes(VADriverContextP ctx,
> VAConfigID
> >> config_id,
> >>              attribs[i].value.value.i =
> >> PipeFormatToVaFourcc(vpp_surface_formats[j]);
> >>              i++;
> >>           }
> >> -      } else if (config->rt_format == VA_RT_FORMAT_YUV420) {
> >> +      }
> >> +      if (!config->rt_format || config->rt_format ==
> VA_RT_FORMAT_YUV420) {
> >>           attribs[i].type = VASurfaceAttribPixelFormat;
> >>           attribs[i].value.type = VAGenericValueTypeInteger;
> >>           attribs[i].flags = VA_SURFACE_ATTRIB_GETTABLE |
> >> VA_SURFACE_ATTRIB_SETTABLE;
> >>
> >>
> >> Will it be ok for whatever test application you are using ?
> >>
> >> Not that the intel va driver always return the full list for vpp.
> >>
> >> Cheers
> >> Julien
> >
> > Hi Julien,
> >
> > On vaCreateConfig(), va.h says:
> > /**
> >  * Create a configuration for the decode pipeline
> >  * it passes in the attribute list that specifies the attributes it cares
> >  * about, with the rest taking default values.
> >  */
> >
> > I think that means that it should be fixed there instead?  That is, if
> we don't
> > pass the render target format attribute, just assume a default value
> which I
> > suppose should be VA_RT_FORMAT_YUV420.
> >
> > So, something like the enclosing patch?  (Not tested.)
>
> Hmm.  The i965 driver uses all of the possible values (depending on the
> profile/entrypoint) ored together here - thats probably a more compatible
> choice
> than YUV420 only.
>
> Thanks,
>
> - Mark
>
>
> That is, add:
>
> >  src/gallium/state_trackers/va/config.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/src/gallium/state_trackers/va/config.c
> > b/src/gallium/state_trackers/va/config.c
> > index 2f96eb6..f2a89b7 100644
> > --- a/src/gallium/state_trackers/va/config.c
> > +++ b/src/gallium/state_trackers/va/config.c
> > @@ -182,6 +182,9 @@ vlVaCreateConfig(VADriverContextP ctx, VAProfile
> profile,
> > VAEntrypoint entrypoin
> >     if (!config)
> >        return VA_STATUS_ERROR_ALLOCATION_FAILED;
> >
> > +   // Default value, overwritten by the VAConfigRTformat attribute if
> present.
> > +   config->rt_format = VA_RT_FORMAT_YUV420;
> > +
> >     if (profile == VAProfileNone && entrypoint == VAEntrypointVideoProc)
> {
> >        config->entrypoint = VAEntrypointVideoProc;
> >        config->profile = PIPE_VIDEO_PROFILE_UNKNOWN;
>
> config->rt_format |= VA_RT_FORMAT_RGB32;
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20161017/e6f4c26d/attachment.html>