| Message ID | 20241018210046.2222313-3-maaz.mombasawala@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vmwgfx: Add support for userspace managed surfaces. | expand |
On Fri, Oct 18, 2024 at 5:01 PM Maaz Mombasawala <maaz.mombasawala@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@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
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; }
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@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(-)