[Mesa-dev] [PATCH 1/2] st/va: enable dual instances encode by sync surface

[Mark Thompson]

Hi,

This patch (applied as <https://cgit.freedesktop.org/mesa/mesa/commit/?id=c59628d11b134fc016388a170880f7646e100d6f>) changes the meaning of vaSyncSurface() in a way I don't think is quite right.

The way it is implemented appears to make vaSyncSurface() mean "given a surface with an outstanding rendering operation we haven't yet synced, wait for that operation to complete or consume the notification that it already has".

I think this should really be "given any surface, if the surface has an outstanding rendering operation, wait for that operation to complete".

The immediate case where this is visible is if you call vaSyncSurface() on a surface which has never been rendered to, you get an error.  (For why you might want to do this, consider code which is uploading/downloading surfaces - you want to sync before copying, but that should be as late as possible and your code there doesn't want to keep track of what surfaces are being used for; for the code where I am hitting this have a look at <https://git.libav.org/?p=libav.git;a=blob;f=libavutil/hwcontext_vaapi.c#l706 >.)

So, I want to suggest something like:

diff --git a/src/gallium/state_trackers/va/surface.c b/src/gallium/state_trackers/va/surface.c
index 3ee1cdd..aad296e 100644
--- a/src/gallium/state_trackers/va/surface.c
+++ b/src/gallium/state_trackers/va/surface.c
@@ -112,12 +112,7 @@ vlVaSyncSurface(VADriverContextP ctx, VASurfaceID render_target)
    }

    context = handle_table_get(drv->htab, surf->ctx);
-   if (!context) {
-      pipe_mutex_unlock(drv->mutex);
-      return VA_STATUS_ERROR_INVALID_CONTEXT;
-   }
-
-   if (context->decoder->entrypoint == PIPE_VIDEO_ENTRYPOINT_ENCODE) {
+   if (context && context->decoder->entrypoint == PIPE_VIDEO_ENTRYPOINT_ENCODE) {
       int frame_diff;
       if (context->desc.h264enc.frame_num_cnt > surf->frame_num_cnt)
          frame_diff = context->desc.h264enc.frame_num_cnt - surf->frame_num_cnt;

However, this doesn't quite work.  Now you can upload once to a surface which hasn't been touched before in order to pass it to an encoder, but any following upload to that same surface (when you reuse it) is running into the fact that it has already been synced to by the encoder (to wait for the bitstream output).  That's now undefined behaviour by the definition above (there isn't an outstanding rendering operation).

(It segfaults in rcve_get_feedback() because surf->feedback is destroyed the first time and therefore null the second time.  You can also get this case by calling vaSyncSurface() twice on any surface which has been rendered to.)

Some simplistic attempts to fix this by checking surf->feedback in vlVaSyncSurface() before calling get_feedback() didn't yield anything useful (seems to spin in the second sync operation), and I don't really know enough about this code to do much more.

So, would you mind having another look at this patch, and clarifying what you think vaSyncSurface() should mean?

Thanks,

- Mark