On Thu, Jan 23, 2025 at 3:44 PM Ian Forbes <ian.for...@broadcom.com> wrote:
>
> Dumb buffers were not being freed because the GEM reference that was
> acquired in gb_surface_define was not dropped like it is in the 2D case.
> Dropping this ref uncovered a few additional issues with freeing the
> resources associated with dirty tracking in vmw_bo_free/release.
>
> Additionally the TTM object associated with the surface were also leaking
> which meant that when the ttm_object_file was closed at process exit the
> destructor unreferenced an already destroyed surface.
>
> The solution is to remove the destructor from the vmw_user_surface
> associated with the dumb_buffer and immediately unreferencing the TTM
> object which his removes it from the ttm_object_file.
>
> This also allows the early return in vmw_user_surface_base_release for the
> dumb buffer case to be removed as it should no longer occur.
>
> The chain of references now has the GEM handle(s) owning the dumb buffer.
> The GEM handles have a singular GEM reference to the vmw_bo which is
> dropped when all handles are closed. When the GEM reference count hits
> zero the vmw_bo is freed which then unreferences the surface via
> vmw_resource_release in vmw_bo_release.
>
> Fixes: d6667f0ddf46 ("drm/vmwgfx: Fix handling of dumb buffers")
> Signed-off-by: Ian Forbes <ian.for...@broadcom.com>
> ---
>
> v2:
> - Update commit description
> - Clean up leaked dirty tracking resources
> - Fix handling of leaked TTM objects
>
> drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 6 ++++--
> drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 2 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 18 ++++++++++++------
> 3 files changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> index 9b5b8c1f063b..87d8b2ba0e8c 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> @@ -51,11 +51,13 @@ static void vmw_bo_release(struct vmw_bo *vbo)
> mutex_lock(&res->dev_priv->cmdbuf_mutex);
> (void)vmw_resource_reserve(res, false, true);
> vmw_resource_mob_detach(res);
> + if (res->dirty)
> + res->func->dirty_free(res);
> if (res->coherent)
> vmw_bo_dirty_release(res->guest_memory_bo);
> res->guest_memory_bo = NULL;
> res->guest_memory_offset = 0;
> - vmw_resource_unreserve(res, false, false, false, NULL,
> + vmw_resource_unreserve(res, true, false, false, NULL,
> 0);
> mutex_unlock(&res->dev_priv->cmdbuf_mutex);
> }
> @@ -73,9 +75,9 @@ static void vmw_bo_free(struct ttm_buffer_object *bo)
> {
> struct vmw_bo *vbo = to_vmw_bo(&bo->base);
>
> - WARN_ON(vbo->dirty);
> WARN_ON(!RB_EMPTY_ROOT(&vbo->res_tree));
> vmw_bo_release(vbo);
> + WARN_ON(vbo->dirty);
> kfree(vbo);
> }
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> index a73af8a355fb..c4d5fe5f330f 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> @@ -273,7 +273,7 @@ int vmw_user_resource_lookup_handle(struct vmw_private
> *dev_priv,
> goto out_bad_resource;
>
> res = converter->base_obj_to_res(base);
> - kref_get(&res->kref);
> + vmw_resource_reference(res);
>
> *p_res = res;
> ret = 0;
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> index 5721c74da3e0..5e022ed466ae 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> @@ -658,7 +658,7 @@ static void vmw_user_surface_free(struct vmw_resource
> *res)
> struct vmw_user_surface *user_srf =
> container_of(srf, struct vmw_user_surface, srf);
>
> - WARN_ON_ONCE(res->dirty);
> + WARN_ON(res->dirty);
> if (user_srf->master)
> drm_master_put(&user_srf->master);
> kfree(srf->offsets);
> @@ -689,8 +689,7 @@ static void vmw_user_surface_base_release(struct
> ttm_base_object **p_base)
> * Dumb buffers own the resource and they'll unref the
> * resource themselves
> */
> - if (res && res->guest_memory_bo && res->guest_memory_bo->is_dumb)
> - return;
> + WARN_ON(res && res->guest_memory_bo && res->guest_memory_bo->is_dumb);
>
> vmw_resource_unreference(&res);
> }
> @@ -2358,12 +2357,19 @@ int vmw_dumb_create(struct drm_file *file_priv,
> vbo = res->guest_memory_bo;
> vbo->is_dumb = true;
> vbo->dumb_surface = vmw_res_to_srf(res);
> -
> + drm_gem_object_put(&vbo->tbo.base);
> + /*
> + * Unset the user surface dtor since this in not actually exposed
> + * to userspace. The suface is owned via the dumb_buffer's GEM handle
> + */
> + struct vmw_user_surface *usurf = container_of(vbo->dumb_surface,
> + struct vmw_user_surface, srf);
> + usurf->prime.base.refcount_release = NULL;
> err:
> if (res)
> vmw_resource_unreference(&res);
> - if (ret)
> - ttm_ref_object_base_unref(tfile, arg.rep.handle);
> +
> + ttm_ref_object_base_unref(tfile, arg.rep.handle);
>
> return ret;
> }
> --
> 2.43.0
>This version looks good. Thanks for tackling this! Reviewed-by: Zack Rusin <zack.ru...@broadcom.com> z
smime.p7s
Description: S/MIME Cryptographic Signature
