<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On 19 October 2015 at 16:55, Emil Velikov <<a href="mailto:emil.l.velikov@gmail.com" target="_blank">emil.l.velikov@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 17 October 2015 at 00:14, Julien Isorce <<a href="mailto:julien.isorce@gmail.com">julien.isorce@gmail.com</a>> wrote: > Inspired from <a href="http://cgit.freedesktop.org/vaapi/intel-driver/tree/src/i965_drv_video.c" rel="noreferrer" target="_blank">http://cgit.freedesktop.org/vaapi/intel-driver/tree/src/i965_drv_video.c</a> > Please mention the actual usecase here and/or how you've tested this. </blockquote><div> </div><div>Thx for the review. </div><div> </div><div>All right I'll amend commit message that this is to support gstreamer-vaapi. </div><div>Also the first advantage to use VaCreateSurfaces2 over existing VaCreateSurfaces, is that you can select which pixel format you want for the surface. Pixel format that you can query with new function: vlVaQuerySurfaceAttributes</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div><div class="h5"> > Signed-off-by: Julien Isorce <<a href="mailto:j.isorce@samsung.com">j.isorce@samsung.com</a>> > --- > src/gallium/state_trackers/va/context.c | 5 +- > src/gallium/state_trackers/va/surface.c | 288 ++++++++++++++++++++++++----- > src/gallium/state_trackers/va/va_private.h | 7 + > 3 files changed, 249 insertions(+), 51 deletions(-) > > diff --git a/src/gallium/state_trackers/va/context.c b/src/gallium/state_trackers/va/context.c > index 8b003ae..8949d42 100644 > --- a/src/gallium/state_trackers/va/context.c > +++ b/src/gallium/state_trackers/va/context.c > @@ -81,7 +81,10 @@ static struct VADriverVTable vtable = > &vlVaSetDisplayAttributes, > &vlVaBufferInfo, > &vlVaLockSurface, > - &vlVaUnlockSurface > + &vlVaUnlockSurface, > + &vlVaGetSurfaceAttributes, > + &vlVaCreateSurfaces2, > + &vlVaQuerySurfaceAttributes > }; > > PUBLIC VAStatus > diff --git a/src/gallium/state_trackers/va/surface.c b/src/gallium/state_trackers/va/surface.c > index 8d4487b..be435cb 100644 > --- a/src/gallium/state_trackers/va/surface.c > +++ b/src/gallium/state_trackers/va/surface.c > @@ -29,6 +29,8 @@ > #include "pipe/p_screen.h" > #include "pipe/p_video_codec.h" > > +#include "state_tracker/drm_driver.h" > + > #include "util/u_memory.h" > #include "util/u_handle_table.h" > #include "util/u_rect.h" > @@ -36,6 +38,7 @@ > #include "util/u_surface.h" > > #include "vl/vl_compositor.h" > +#include "vl/vl_video_buffer.h" > #include "vl/vl_winsys.h" > > #include "va_private.h" > @@ -44,56 +47,8 @@ VAStatus > vlVaCreateSurfaces(VADriverContextP ctx, int width, int height, int format, > int num_surfaces, VASurfaceID *surfaces) > { > - struct pipe_video_buffer templat = {}; > - struct pipe_screen *pscreen; > - vlVaDriver *drv; > - int i; > - > - if (!ctx) > - return VA_STATUS_ERROR_INVALID_CONTEXT; > - > - if (!(width && height)) > - return VA_STATUS_ERROR_INVALID_IMAGE_FORMAT; > - > - drv = VL_VA_DRIVER(ctx); > - pscreen = VL_VA_PSCREEN(ctx); > - > - templat.buffer_format = pscreen->get_video_param > - ( > - pscreen, > - PIPE_VIDEO_PROFILE_UNKNOWN, > - PIPE_VIDEO_ENTRYPOINT_BITSTREAM, > - PIPE_VIDEO_CAP_PREFERED_FORMAT > - ); > - templat.chroma_format = ChromaToPipe(format); > - templat.width = width; > - templat.height = height; > - templat.interlaced = pscreen->get_video_param > - ( > - pscreen, > - PIPE_VIDEO_PROFILE_UNKNOWN, > - PIPE_VIDEO_ENTRYPOINT_BITSTREAM, > - PIPE_VIDEO_CAP_PREFERS_INTERLACED > - ); > - > - for (i = 0; i < num_surfaces; ++i) { > - vlVaSurface *surf = CALLOC(1, sizeof(vlVaSurface)); > - if (!surf) > - goto no_res; > - > - surf->templat = templat; > - surf->buffer = drv->pipe->create_video_buffer(drv->pipe, &templat); > - util_dynarray_init(&surf->subpics); > - surfaces[i] = handle_table_add(drv->htab, surf); > - } > - > - return VA_STATUS_SUCCESS; > - > -no_res: > - if (i) > - vlVaDestroySurfaces(ctx, surfaces, i); > - > - return VA_STATUS_ERROR_ALLOCATION_FAILED; > + return vlVaCreateSurfaces2(ctx, format, width, height, surfaces, num_surfaces, > + NULL, 0); > } > > VAStatus > @@ -349,3 +304,236 @@ vlVaUnlockSurface(VADriverContextP ctx, VASurfaceID surface) > > return VA_STATUS_ERROR_UNIMPLEMENTED; > } > + > +VAStatus > +vlVaGetSurfaceAttributes(VADriverContextP ctx, VAConfigID config, > + VASurfaceAttrib *attrib_list, unsigned int num_attribs) > +{ > + return VA_STATUS_ERROR_UNIMPLEMENTED; /* DEPRECATED */ > +} </div></div>Hmm implemented only to return ENOIMPL. Why ? </blockquote><div> </div><div>2 reasons, first that I do not know how to set it in "VADriverVTable vtable" otherwise, mhh actually maybe I can just set NULL in place of "&vlVaGetSurfaceAttributes" :) libva indeed checks for its presence. And also to make it clear in mesa st/va code that this function is deprecated, see libva/va_backend.h. It is replaced by VaQuerySurfaceAttributes.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > + > +VAStatus > +vlVaQuerySurfaceAttributes(VADriverContextP ctx, VAConfigID config, > + VASurfaceAttrib *attrib_list, unsigned int *num_attribs) > +{ > + VAStatus vaStatus = VA_STATUS_SUCCESS; > + vlVaDriver *drv = NULL; > + VASurfaceAttrib *attribs = NULL; > + struct pipe_screen *pscreen = NULL; > + int i = 0; > + You don't need most of these variable initializers. st/va doesn't seem to follow this approach either. </blockquote><div>ok </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > + if (config == VA_INVALID_ID) > + return VA_STATUS_ERROR_INVALID_CONFIG; > + > + if (!attrib_list && !num_attribs) > + return VA_STATUS_ERROR_INVALID_PARAMETER; > + > + if (attrib_list == NULL) { > + *num_attribs = VASurfaceAttribCount; > + return VA_STATUS_SUCCESS; > + } > + > + attribs = CALLOC(VASurfaceAttribCount, sizeof(VASurfaceAttrib)); > + > + if (attribs == NULL) > + return VA_STATUS_ERROR_ALLOCATION_FAILED; > + Push the calloc/check further down, where it's needed ? Otherwise you leak on the memory on the next three return statements. </blockquote><div> </div><div>I see, thx. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div><div class="h5"> > + if (!ctx) > + return VA_STATUS_ERROR_INVALID_CONTEXT; > + > + drv = VL_VA_DRIVER(ctx); > + > + if (!drv) > + return VA_STATUS_ERROR_INVALID_CONTEXT; > + > + pscreen = VL_VA_PSCREEN(ctx); > + > + if (!pscreen) > + return VA_STATUS_ERROR_INVALID_CONTEXT; > + > + if (config == PIPE_VIDEO_PROFILE_UNKNOWN) { > + /* Assume VAEntrypointVideoProc for now. */ > + attribs[i].type = VASurfaceAttribPixelFormat; > + attribs[i].value.type = VAGenericValueTypeInteger; > + attribs[i].flags = VA_SURFACE_ATTRIB_GETTABLE | VA_SURFACE_ATTRIB_SETTABLE; > + attribs[i].value.value.i = VA_FOURCC_BGRA; > + i++; > + > + attribs[i].type = VASurfaceAttribPixelFormat; > + attribs[i].value.type = VAGenericValueTypeInteger; > + attribs[i].flags = VA_SURFACE_ATTRIB_GETTABLE | VA_SURFACE_ATTRIB_SETTABLE; > + attribs[i].value.value.i = VA_FOURCC_RGBA; > + i++; > + } else { > + /* Assume VAEntrypointVLD for now. */ > + attribs[i].type = VASurfaceAttribPixelFormat; > + attribs[i].value.type = VAGenericValueTypeInteger; > + attribs[i].flags = VA_SURFACE_ATTRIB_GETTABLE | VA_SURFACE_ATTRIB_SETTABLE; > + attribs[i].value.value.i = VA_FOURCC_NV12; > + i++; > + } > + > + attribs[i].type = VASurfaceAttribMemoryType; > + attribs[i].value.type = VAGenericValueTypeInteger; > + attribs[i].flags = VA_SURFACE_ATTRIB_GETTABLE | VA_SURFACE_ATTRIB_SETTABLE; > + attribs[i].value.value.i = VA_SURFACE_ATTRIB_MEM_TYPE_VA; > + i++; > + > + attribs[i].type = VASurfaceAttribExternalBufferDescriptor; > + attribs[i].value.type = VAGenericValueTypePointer; > + attribs[i].flags = VA_SURFACE_ATTRIB_SETTABLE; > + attribs[i].value.value.p = NULL; /* ignore */ > + i++; > + > + attribs[i].type = VASurfaceAttribMaxWidth; > + attribs[i].value.type = VAGenericValueTypeInteger; > + attribs[i].flags = VA_SURFACE_ATTRIB_GETTABLE; > + attribs[i].value.value.i = vl_video_buffer_max_size(pscreen); > + i++; > + > + attribs[i].type = VASurfaceAttribMaxHeight; > + attribs[i].value.type = VAGenericValueTypeInteger; > + attribs[i].flags = VA_SURFACE_ATTRIB_GETTABLE; > + attribs[i].value.value.i = vl_video_buffer_max_size(pscreen); > + i++; > + > + if (i > *num_attribs) { > + *num_attribs = i; > + FREE(attribs); > + return VA_STATUS_ERROR_MAX_NUM_EXCEEDED; > + } > + > + *num_attribs = i; > + memcpy(attrib_list, attribs, i * sizeof(VASurfaceAttrib)); > + FREE(attribs); > + > + return vaStatus; > +} > + > +VAStatus > +vlVaCreateSurfaces2(VADriverContextP ctx, unsigned int format, > + unsigned int width, unsigned int height, > + VASurfaceID *surfaces, unsigned int num_surfaces, > + VASurfaceAttrib *attrib_list, unsigned int num_attribs) > +{ > + vlVaDriver *drv = NULL; > + VASurfaceAttribExternalBuffers *memory_attibute = NULL; > + VAStatus vaStatus = VA_STATUS_SUCCESS; > + struct pipe_video_buffer templat = {}; > + struct pipe_screen *pscreen = NULL; > + int memory_type = VA_SURFACE_ATTRIB_MEM_TYPE_VA; > + int expected_fourcc = 0; > + int i = 0; > + </div></div>Please drop the initializers, unless needed. </blockquote><div> </div><div>ok </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div><div class="h5"> > + if (!ctx) > + return VA_STATUS_ERROR_INVALID_CONTEXT; > + > + if (!(width && height)) > + return VA_STATUS_ERROR_INVALID_IMAGE_FORMAT; > + > + drv = VL_VA_DRIVER(ctx); > + pscreen = VL_VA_PSCREEN(ctx); > + > + for (i = 0; i < num_attribs && attrib_list; i++) { > + if ((attrib_list[i].type == VASurfaceAttribPixelFormat) && > + (attrib_list[i].flags & VA_SURFACE_ATTRIB_SETTABLE)) { > + if (attrib_list[i].value.type != VAGenericValueTypeInteger) > + return VA_STATUS_ERROR_INVALID_PARAMETER; > + expected_fourcc = attrib_list[i].value.value.i; > + } > + > + if ((attrib_list[i].type == VASurfaceAttribMemoryType) && > + (attrib_list[i].flags & VA_SURFACE_ATTRIB_SETTABLE)) { > + > + if (attrib_list[i].value.type != VAGenericValueTypeInteger) > + return VA_STATUS_ERROR_INVALID_PARAMETER; > + > + switch (attrib_list[i].value.value.i) { > + case VA_SURFACE_ATTRIB_MEM_TYPE_VA: > + default:{ > + return VA_STATUS_ERROR_UNSUPPORTED_MEMORY_TYPE;} > + } > + } > + > + if ((attrib_list[i].type == VASurfaceAttribExternalBufferDescriptor) && > + (attrib_list[i].flags == VA_SURFACE_ATTRIB_SETTABLE)) { > + if (attrib_list[i].value.type != VAGenericValueTypePointer) > + return VA_STATUS_ERROR_INVALID_PARAMETER; > + memory_attibute = (VASurfaceAttribExternalBuffers *)attrib_list[i].value.value.p; > + } > + } > + > + /* support 420 & 422 & RGB32 format, 422 and RGB32 are only used > + * for post-processing (including color conversion) */ > + if (VA_RT_FORMAT_YUV420 != format && > + VA_RT_FORMAT_YUV422 != format && > + VA_RT_FORMAT_YUV444 != format && > + VA_RT_FORMAT_YUV411 != format && > + VA_RT_FORMAT_YUV400 != format && </div></div>We don't have any support for 400 or 411. Why are they in the list ? </blockquote><div> Just a copy past, oups :) </div><div>Thx </div><div>Julien </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Cheers, Emil </blockquote></div> </div></div>