On Fri, Oct 18, 2024 at 5:01 PM Maaz Mombasawala
<maaz.mombasawala@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> ---
> 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
