[PATCH v2 2/3] drm/vmwgfx: Support hw_destroy for userspace managed surfaces

[Zack Rusin]

On Fri, Oct 18, 2024 at 5:01 PM Maaz Mombasawala
<maaz.mombasawala at broadcom.com> wrote:
>
> A userspace may create a userspace managed surface but not destroy it,
> add hw_destroy function for userspace surfaces so that vmwgfx records the
> destroy command and submits it when the userspace context is destroyed.
>
> Signed-off-by: Maaz Mombasawala <maaz.mombasawala at broadcom.com>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.h     |  1 +
>  drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c |  7 ++++-
>  drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 41 +++++++++++++++++++++++++
>  3 files changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> index 67f75d366f9d..6cb3f648eaad 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> @@ -267,6 +267,7 @@ struct vmw_surface {
>  struct vmw_cmdbuf_surface {
>         struct vmw_surface surface;
>         struct ttm_base_object base;
> +       bool cmdbuf_destroy;
>  };
>
>  struct vmw_fifo_state {
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> index 0468c9f4f293..70f816062fd2 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> @@ -3374,6 +3374,8 @@ static int vmw_cmd_destroy_gb_surface(struct vmw_private *dev_priv,
>                                       SVGA3dCmdHeader *header)
>  {
>         VMW_DECLARE_CMD_VAR(*cmd, SVGA3dCmdDestroyGBSurface);
> +       struct vmw_resource *res;
> +       struct vmw_cmdbuf_surface *surface;
>         int ret;
>
>         cmd = container_of(header, typeof(*cmd), header);
> @@ -3382,10 +3384,13 @@ static int vmw_cmd_destroy_gb_surface(struct vmw_private *dev_priv,
>                 return -EINVAL;
>
>         ret = vmw_cmd_res_check(dev_priv, sw_context, vmw_res_surface,
> -                               VMW_RES_DIRTY_NONE, NULL, &cmd->body.sid, NULL);
> +                               VMW_RES_DIRTY_NONE, NULL, &cmd->body.sid, &res);
>         if (unlikely(ret))
>                 return ret;
>
> +       surface = vmw_res_to_cmdbuf_srf(res);
> +       surface->cmdbuf_destroy = true;
> +
>         ret = vmw_cmdbuf_surface_destroy(dev_priv, sw_context, cmd->body.sid);
>         if (unlikely(ret))
>                 return ret;
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> index 4dfce3ea104c..26a71af79a39 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> @@ -2403,6 +2403,44 @@ static void vmw_cmdbuf_surface_base_release(struct ttm_base_object **p_base)
>  }
>
>
> +static void vmw_cmdbuf_surface_hw_destroy(struct vmw_resource *res)
> +{
> +       struct vmw_private *dev_priv = res->dev_priv;
> +       struct vmw_cmdbuf_surface *surface;
> +       struct {
> +               SVGA3dCmdHeader header;
> +               SVGA3dCmdDestroyGBSurface body;
> +       } *cmd;
> +
> +       if (res->id == -1)
> +               return;
> +
> +       surface = vmw_res_to_cmdbuf_srf(res);
> +       /**
> +        * If userspace is submitting a destroy command there is no need for
> +        * kernel to do so.
> +        */
> +       if (surface->cmdbuf_destroy)
> +               return;
> +
> +       mutex_lock(&dev_priv->binding_mutex);
> +       vmw_view_surface_list_destroy(dev_priv, &surface->surface.view_list);
> +       vmw_binding_res_list_scrub(&res->binding_head);
> +
> +       cmd = VMW_CMD_RESERVE(dev_priv, sizeof(*cmd));
> +       if (unlikely(!cmd)) {
> +               mutex_unlock(&dev_priv->binding_mutex);
> +               return;
> +       }
> +
> +       cmd->header.id = SVGA_3D_CMD_DESTROY_GB_SURFACE;
> +       cmd->header.size = sizeof(cmd->body);
> +       cmd->body.sid = res->id;
> +       vmw_cmd_commit(dev_priv, sizeof(*cmd));
> +       mutex_unlock(&dev_priv->binding_mutex);
> +}
> +
> +
>  int vmw_cmdbuf_surface_define(struct vmw_private *dev_priv,
>                               struct vmw_sw_context *sw_context,
>                               struct vmw_surface_metadata *metadata,
> @@ -2484,6 +2522,9 @@ int vmw_cmdbuf_surface_define(struct vmw_private *dev_priv,
>                 return ret;
>         }
>
> +       res->hw_destroy = vmw_cmdbuf_surface_hw_destroy;
> +       surface->cmdbuf_destroy = false;
> +
>         return 0;
>  }
>
> --
> 2.43.0
>

Hmm, this looks like a hack. So what seems to be happening is that the
reference count on the resource is still active. When vmw_resource
reference count goes to zero it should call hw_destroy, which should
invoke vmw_gb_surface_destroy which should delete the surface. It
looks like the reference count of userspace resources is off, so with
this patch we'd be destroying the gb surface but the vmw_resource is
still alive. If that's the case, then what we want to do is fix the
reference counting of the vmw_resource rather than working around it.

z