[Mesa-dev] [PATCH v3] virgl: native fence fd support

[Emil Velikov]

[This time with mesa-dev@ in the list, and less typos]

Hi Rob,

On Mon, 12 Nov 2018 at 15:14, Robert Foss <robert.foss at collabora.com> wrote:

> +++ b/src/gallium/drivers/virgl/virgl_screen.c
> @@ -340,7 +340,7 @@ virgl_get_param(struct pipe_screen *screen, enum pipe_cap param)
>     case PIPE_CAP_VIDEO_MEMORY:
>        return 0;
>     case PIPE_CAP_NATIVE_FENCE_FD:
> -      return 0;
> +      return vscreen->vws->driver_version(vscreen->vws) >= 1;

It seems like the driver_version() vfunc is missing for the vtest winsys.

One could go with an empty stub or drop the function in faviour of a winsys
variable (or bitmask). Personally, I'm leaning towards the latter, although
either will do.


> +static void virgl_fence_server_sync(struct virgl_winsys *vws,
> +                                   struct virgl_cmd_buf *cbuf,
> +                                    struct pipe_fence_handle *fence)
> +{
> +   struct virgl_hw_res *hw_res = virgl_hw_res(fence);
> +
> +   assert(hw_res->fence_fd != -1);
> +
Skimming at other drivers - they're not using an assert, so I'd change this to
an if statement.

> +   if (cbuf->in_fence_fd == -1) {
> +       cbuf->in_fence_fd = dup(hw_res->fence_fd);
> +   } else {
> +        int new_fd = sync_merge("virgl", cbuf->in_fence_fd, hw_res->fence_fd);
> +        close(cbuf->in_fence_fd);
> +        cbuf->in_fence_fd = new_fd;

The above if/else seems like an open-coded version of sync_accumulate().

Despite the above comments, the kernel interface seems reasonable IMHO.
Would be great if one more person else double-checks it though.

With the three bits handled the patch is:
Reviewed-by: Emil Velikov <emil.velikov at collabora.com>

HTH
Emil