From 9da2957f9f81ed29d0046021f131be352cb2199e Mon Sep 17 00:00:00 2001 From: Zack Rusin Date: Mon, 30 Jan 2023 22:35:35 -0500 Subject: drm/vmwgfx: Use the common gem mmap instead of the custom code Before vmwgfx supported gem it needed to implement the entire mmap logic explicitly. With GEM support that's not needed and the generic code can be used by simply setting the vm_ops to vmwgfx specific ones on the gem object itself. Removes a lot of code from vmwgfx without any functional difference. Signed-off-by: Zack Rusin Reviewed-by: Thomas Zimmermann Reviewed-by: Martin Krastev Reviewed-by: Maaz Mombasawala Link: https://patchwork.freedesktop.org/patch/msgid/20230131033542.953249-2-zack@kde.org --- drivers/gpu/drm/vmwgfx/vmwgfx_gem.c | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'drivers/gpu/drm/vmwgfx/vmwgfx_gem.c') diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c index ce609e7d758f..ba4ddd9f7a7e 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c @@ -103,6 +103,13 @@ static struct sg_table *vmw_gem_object_get_sg_table(struct drm_gem_object *obj) return drm_prime_pages_to_sg(obj->dev, vmw_tt->dma_ttm.pages, vmw_tt->dma_ttm.num_pages); } +static const struct vm_operations_struct vmw_vm_ops = { + .pfn_mkwrite = vmw_bo_vm_mkwrite, + .page_mkwrite = vmw_bo_vm_mkwrite, + .fault = vmw_bo_vm_fault, + .open = ttm_bo_vm_open, + .close = ttm_bo_vm_close, +}; static const struct drm_gem_object_funcs vmw_gem_object_funcs = { .free = vmw_gem_object_free, @@ -115,6 +122,7 @@ static const struct drm_gem_object_funcs vmw_gem_object_funcs = { .vmap = drm_gem_ttm_vmap, .vunmap = drm_gem_ttm_vunmap, .mmap = drm_gem_ttm_mmap, + .vm_ops = &vmw_vm_ops, }; /** -- cgit From 6b2e8aa45126161135fb4a88870c9526fd8319f8 Mon Sep 17 00:00:00 2001 From: Zack Rusin Date: Mon, 30 Jan 2023 22:35:36 -0500 Subject: drm/vmwgfx: Remove the duplicate bo_free function Remove the explicit bo_free parameter which was switching between vmw_bo_bo_free and vmw_gem_destroy which had exactly the same implementation. It makes no sense to keep parameter which is always the same, remove it and all code referencing it. Instead use the vmw_bo_bo_free directly. Signed-off-by: Zack Rusin Reviewed-by: Martin Krastev Reviewed-by: Maaz Mombasawala Acked-by: Thomas Zimmermann Link: https://patchwork.freedesktop.org/patch/msgid/20230131033542.953249-3-zack@kde.org --- drivers/gpu/drm/vmwgfx/vmwgfx_gem.c | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) (limited to 'drivers/gpu/drm/vmwgfx/vmwgfx_gem.c') diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c index ba4ddd9f7a7e..ae39029fec4a 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c @@ -125,22 +125,6 @@ static const struct drm_gem_object_funcs vmw_gem_object_funcs = { .vm_ops = &vmw_vm_ops, }; -/** - * vmw_gem_destroy - vmw buffer object destructor - * - * @bo: Pointer to the embedded struct ttm_buffer_object - */ -void vmw_gem_destroy(struct ttm_buffer_object *bo) -{ - struct vmw_buffer_object *vbo = vmw_buffer_object(bo); - - WARN_ON(vbo->dirty); - WARN_ON(!RB_EMPTY_ROOT(&vbo->res_tree)); - vmw_bo_unmap(vbo); - drm_gem_object_release(&vbo->base.base); - kfree(vbo); -} - int vmw_gem_object_create_with_handle(struct vmw_private *dev_priv, struct drm_file *filp, uint32_t size, @@ -153,7 +137,7 @@ int vmw_gem_object_create_with_handle(struct vmw_private *dev_priv, (dev_priv->has_mob) ? &vmw_sys_placement : &vmw_vram_sys_placement, - true, false, &vmw_gem_destroy, p_vbo); + true, false, p_vbo); (*p_vbo)->base.base.funcs = &vmw_gem_object_funcs; if (ret != 0) -- cgit From 09881d2940bbd641f27f9ae7907e8a1893bc54b2 Mon Sep 17 00:00:00 2001 From: Zack Rusin Date: Mon, 30 Jan 2023 22:35:37 -0500 Subject: drm/vmwgfx: Rename vmw_buffer_object to vmw_bo The rest of the drivers which are using ttm have mostly standardized on driver_prefix_bo as the name for subclasses of the TTM buffer object. Make vmwgfx match the rest of the drivers and follow the same naming semantics. This is especially clear given that the name of the file in which the object was defined is vmw_bo.c. Signed-off-by: Zack Rusin Reviewed-by: Martin Krastev Reviewed-by: Maaz Mombasawala Acked-by: Thomas Zimmermann Link: https://patchwork.freedesktop.org/patch/msgid/20230131033542.953249-4-zack@kde.org --- drivers/gpu/drm/vmwgfx/vmwgfx_gem.c | 27 +++++++-------------------- 1 file changed, 7 insertions(+), 20 deletions(-) (limited to 'drivers/gpu/drm/vmwgfx/vmwgfx_gem.c') diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c index ae39029fec4a..c7ebcd4f3afa 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c @@ -1,6 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0 OR MIT */ /* - * Copyright 2021 VMware, Inc. + * Copyright 2021-2023 VMware, Inc. * * Permission is hereby granted, free of charge, to any person * obtaining a copy of this software and associated documentation @@ -24,25 +24,12 @@ * */ +#include "vmwgfx_bo.h" #include "vmwgfx_drv.h" #include "drm/drm_prime.h" #include "drm/drm_gem_ttm_helper.h" -/** - * vmw_buffer_object - Convert a struct ttm_buffer_object to a struct - * vmw_buffer_object. - * - * @bo: Pointer to the TTM buffer object. - * Return: Pointer to the struct vmw_buffer_object embedding the - * TTM buffer object. - */ -static struct vmw_buffer_object * -vmw_buffer_object(struct ttm_buffer_object *bo) -{ - return container_of(bo, struct vmw_buffer_object, base); -} - static void vmw_gem_object_free(struct drm_gem_object *gobj) { struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gobj); @@ -65,7 +52,7 @@ static void vmw_gem_object_close(struct drm_gem_object *obj, static int vmw_gem_pin_private(struct drm_gem_object *obj, bool do_pin) { struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(obj); - struct vmw_buffer_object *vbo = vmw_buffer_object(bo); + struct vmw_bo *vbo = to_vmw_bo(obj); int ret; ret = ttm_bo_reserve(bo, false, false, NULL); @@ -129,7 +116,7 @@ int vmw_gem_object_create_with_handle(struct vmw_private *dev_priv, struct drm_file *filp, uint32_t size, uint32_t *handle, - struct vmw_buffer_object **p_vbo) + struct vmw_bo **p_vbo) { int ret; @@ -159,7 +146,7 @@ int vmw_gem_object_create_ioctl(struct drm_device *dev, void *data, (union drm_vmw_alloc_dmabuf_arg *)data; struct drm_vmw_alloc_dmabuf_req *req = &arg->req; struct drm_vmw_dmabuf_rep *rep = &arg->rep; - struct vmw_buffer_object *vbo; + struct vmw_bo *vbo; uint32_t handle; int ret; @@ -178,7 +165,7 @@ out_no_bo: #if defined(CONFIG_DEBUG_FS) -static void vmw_bo_print_info(int id, struct vmw_buffer_object *bo, struct seq_file *m) +static void vmw_bo_print_info(int id, struct vmw_bo *bo, struct seq_file *m) { const char *placement; const char *type; @@ -259,7 +246,7 @@ static int vmw_debugfs_gem_info_show(struct seq_file *m, void *unused) spin_lock(&file->table_lock); idr_for_each_entry(&file->object_idr, gobj, id) { - struct vmw_buffer_object *bo = gem_to_vmw_bo(gobj); + struct vmw_bo *bo = to_vmw_bo(gobj); vmw_bo_print_info(id, bo, m); } -- cgit From 39985eea5a6dd1e844f216028252870e980b9e7f Mon Sep 17 00:00:00 2001 From: Zack Rusin Date: Mon, 30 Jan 2023 22:35:41 -0500 Subject: drm/vmwgfx: Abstract placement selection Problem with explicit placement selection in vmwgfx is that by the time the buffer object needs to be validated the information about which placement was supposed to be used is lost. To workaround this the driver had a bunch of state in various places e.g. as_mob or cpu_blit to somehow convey the information on which placement was intended. Fix it properly by allowing the buffer objects to hold their preferred placement so it can be reused whenever needed. This makes the entire validation pipeline a lot easier both to understand and maintain. Signed-off-by: Zack Rusin Reviewed-by: Martin Krastev Reviewed-by: Maaz Mombasawala Acked-by: Thomas Zimmermann Link: https://patchwork.freedesktop.org/patch/msgid/20230131033542.953249-8-zack@kde.org --- drivers/gpu/drm/vmwgfx/vmwgfx_gem.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'drivers/gpu/drm/vmwgfx/vmwgfx_gem.c') diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c index c7ebcd4f3afa..5f383578a320 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c @@ -121,9 +121,8 @@ int vmw_gem_object_create_with_handle(struct vmw_private *dev_priv, int ret; ret = vmw_bo_create(dev_priv, size, - (dev_priv->has_mob) ? - &vmw_sys_placement : - &vmw_vram_sys_placement, + (dev_priv->has_mob) ? VMW_BO_DOMAIN_SYS : VMW_BO_DOMAIN_VRAM, + VMW_BO_DOMAIN_SYS, true, false, p_vbo); (*p_vbo)->base.base.funcs = &vmw_gem_object_funcs; -- cgit From 668b206601c5f5063e03b76784a0d3024fa2b249 Mon Sep 17 00:00:00 2001 From: Zack Rusin Date: Mon, 30 Jan 2023 22:35:42 -0500 Subject: drm/vmwgfx: Stop using raw ttm_buffer_object's Various bits of the driver used raw ttm_buffer_object instead of the driver specific vmw_bo object. All those places used to duplicate the mapped bo caching policy of vmw_bo. Instead of duplicating all of that code and special casing various functions to work both with vmw_bo and raw ttm_buffer_object's unify the buffer object handling code. As part of that work fix the naming of bo's, e.g. insted of generic backup use 'guest_memory' because that's what it really is. All of it makes the driver easier to maintain and the code easier to read. Saves 100+ loc as well. Signed-off-by: Zack Rusin Reviewed-by: Martin Krastev Reviewed-by: Maaz Mombasawala Acked-by: Thomas Zimmermann Link: https://patchwork.freedesktop.org/patch/msgid/20230131033542.953249-9-zack@kde.org --- drivers/gpu/drm/vmwgfx/vmwgfx_gem.c | 37 ++++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 17 deletions(-) (limited to 'drivers/gpu/drm/vmwgfx/vmwgfx_gem.c') diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c index 5f383578a320..f042e22b8b59 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c @@ -33,9 +33,8 @@ static void vmw_gem_object_free(struct drm_gem_object *gobj) { struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gobj); - if (bo) { + if (bo) ttm_bo_put(bo); - } } static int vmw_gem_object_open(struct drm_gem_object *obj, @@ -119,19 +118,23 @@ int vmw_gem_object_create_with_handle(struct vmw_private *dev_priv, struct vmw_bo **p_vbo) { int ret; + struct vmw_bo_params params = { + .domain = (dev_priv->has_mob) ? VMW_BO_DOMAIN_SYS : VMW_BO_DOMAIN_VRAM, + .busy_domain = VMW_BO_DOMAIN_SYS, + .bo_type = ttm_bo_type_device, + .size = size, + .pin = false + }; - ret = vmw_bo_create(dev_priv, size, - (dev_priv->has_mob) ? VMW_BO_DOMAIN_SYS : VMW_BO_DOMAIN_VRAM, - VMW_BO_DOMAIN_SYS, - true, false, p_vbo); + ret = vmw_bo_create(dev_priv, ¶ms, p_vbo); - (*p_vbo)->base.base.funcs = &vmw_gem_object_funcs; + (*p_vbo)->tbo.base.funcs = &vmw_gem_object_funcs; if (ret != 0) goto out_no_bo; - ret = drm_gem_handle_create(filp, &(*p_vbo)->base.base, handle); + ret = drm_gem_handle_create(filp, &(*p_vbo)->tbo.base, handle); /* drop reference from allocate - handle holds it now */ - drm_gem_object_put(&(*p_vbo)->base.base); + drm_gem_object_put(&(*p_vbo)->tbo.base); out_no_bo: return ret; } @@ -155,7 +158,7 @@ int vmw_gem_object_create_ioctl(struct drm_device *dev, void *data, goto out_no_bo; rep->handle = handle; - rep->map_handle = drm_vma_node_offset_addr(&vbo->base.base.vma_node); + rep->map_handle = drm_vma_node_offset_addr(&vbo->tbo.base.vma_node); rep->cur_gmr_id = handle; rep->cur_gmr_offset = 0; out_no_bo: @@ -169,7 +172,7 @@ static void vmw_bo_print_info(int id, struct vmw_bo *bo, struct seq_file *m) const char *placement; const char *type; - switch (bo->base.resource->mem_type) { + switch (bo->tbo.resource->mem_type) { case TTM_PL_SYSTEM: placement = " CPU"; break; @@ -190,7 +193,7 @@ static void vmw_bo_print_info(int id, struct vmw_bo *bo, struct seq_file *m) break; } - switch (bo->base.type) { + switch (bo->tbo.type) { case ttm_bo_type_device: type = "device"; break; @@ -206,12 +209,12 @@ static void vmw_bo_print_info(int id, struct vmw_bo *bo, struct seq_file *m) } seq_printf(m, "\t\t0x%08x: %12zu bytes %s, type = %s", - id, bo->base.base.size, placement, type); + id, bo->tbo.base.size, placement, type); seq_printf(m, ", priority = %u, pin_count = %u, GEM refs = %d, TTM refs = %d", - bo->base.priority, - bo->base.pin_count, - kref_read(&bo->base.base.refcount), - kref_read(&bo->base.kref)); + bo->tbo.priority, + bo->tbo.pin_count, + kref_read(&bo->tbo.base.refcount), + kref_read(&bo->tbo.kref)); seq_puts(m, "\n"); } -- cgit From 36d421e632e9a0e8375eaed0143551a34d81a7e3 Mon Sep 17 00:00:00 2001 From: Zack Rusin Date: Wed, 8 Feb 2023 13:00:50 -0500 Subject: drm/vmwgfx: Stop accessing buffer objects which failed init ttm_bo_init_reserved on failure puts the buffer object back which causes it to be deleted, but kfree was still being called on the same buffer in vmw_bo_create leading to a double free. After the double free the vmw_gem_object_create_with_handle was setting the gem function objects before checking the return status of vmw_bo_create leading to null pointer access. Fix the entire path by relaying on ttm_bo_init_reserved to delete the buffer objects on failure and making sure the return status is checked before setting the gem function objects on the buffer object. Signed-off-by: Zack Rusin Fixes: 8afa13a0583f ("drm/vmwgfx: Implement DRIVER_GEM") Reviewed-by: Maaz Mombasawala Reviewed-by: Martin Krastev Link: https://patchwork.freedesktop.org/patch/msgid/20230208180050.2093426-1-zack@kde.org --- drivers/gpu/drm/vmwgfx/vmwgfx_gem.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'drivers/gpu/drm/vmwgfx/vmwgfx_gem.c') diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c index f042e22b8b59..51bd1f8c5cc4 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c @@ -127,11 +127,11 @@ int vmw_gem_object_create_with_handle(struct vmw_private *dev_priv, }; ret = vmw_bo_create(dev_priv, ¶ms, p_vbo); - - (*p_vbo)->tbo.base.funcs = &vmw_gem_object_funcs; if (ret != 0) goto out_no_bo; + (*p_vbo)->tbo.base.funcs = &vmw_gem_object_funcs; + ret = drm_gem_handle_create(filp, &(*p_vbo)->tbo.base, handle); /* drop reference from allocate - handle holds it now */ drm_gem_object_put(&(*p_vbo)->tbo.base); -- cgit From 9ef8d83e8e25d5f1811b3a38eb1484f85f64296c Mon Sep 17 00:00:00 2001 From: Zack Rusin Date: Sat, 11 Feb 2023 00:05:14 -0500 Subject: drm/vmwgfx: Do not drop the reference to the handle too soon v3: Fix vmw_user_bo_lookup which was also dropping the gem reference before the kernel was done with buffer depending on userspace doing the right thing. Same bug, different spot. It is possible for userspace to predict the next buffer handle and to destroy the buffer while it's still used by the kernel. Delay dropping the internal reference on the buffers until kernel is done with them. Instead of immediately dropping the gem reference in vmw_user_bo_lookup and vmw_gem_object_create_with_handle let the callers decide when they're ready give the control back to userspace. Also fixes the second usage of vmw_gem_object_create_with_handle in vmwgfx_surface.c which wasn't grabbing an explicit reference to the gem object which could have been destroyed by the userspace on the owning surface at any point. Signed-off-by: Zack Rusin Fixes: 8afa13a0583f ("drm/vmwgfx: Implement DRIVER_GEM") Reviewed-by: Martin Krastev Reviewed-by: Maaz Mombasawala Link: https://patchwork.freedesktop.org/patch/msgid/20230211050514.2431155-1-zack@kde.org --- drivers/gpu/drm/vmwgfx/vmwgfx_gem.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'drivers/gpu/drm/vmwgfx/vmwgfx_gem.c') diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c index 51bd1f8c5cc4..d6baf73a6458 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c @@ -133,8 +133,6 @@ int vmw_gem_object_create_with_handle(struct vmw_private *dev_priv, (*p_vbo)->tbo.base.funcs = &vmw_gem_object_funcs; ret = drm_gem_handle_create(filp, &(*p_vbo)->tbo.base, handle); - /* drop reference from allocate - handle holds it now */ - drm_gem_object_put(&(*p_vbo)->tbo.base); out_no_bo: return ret; } @@ -161,6 +159,8 @@ int vmw_gem_object_create_ioctl(struct drm_device *dev, void *data, rep->map_handle = drm_vma_node_offset_addr(&vbo->tbo.base.vma_node); rep->cur_gmr_id = handle; rep->cur_gmr_offset = 0; + /* drop reference from allocate - handle holds it now */ + drm_gem_object_put(&vbo->tbo.base); out_no_bo: return ret; } -- cgit From 4230cea89cafb11b2c2e4dcac8b505e7a766b386 Mon Sep 17 00:00:00 2001 From: Tvrtko Ursulin Date: Tue, 14 Mar 2023 14:18:55 +0000 Subject: drm: Track clients by tgid and not tid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Thread group id (aka pid from userspace point of view) is a more interesting thing to show as an owner of a DRM fd, so track and show that instead of the thread id. In the next patch we will make the owner updated post file descriptor handover, which will also be tgid based to avoid ping-pong when multiple threads access the fd. Signed-off-by: Tvrtko Ursulin Reviewed-by: Zack Rusin Reviewed-by: Christian König Signed-off-by: Christian König Link: https://patchwork.freedesktop.org/patch/msgid/20230314141904.1210824-2-tvrtko.ursulin@linux.intel.com --- drivers/gpu/drm/vmwgfx/vmwgfx_gem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/gpu/drm/vmwgfx/vmwgfx_gem.c') diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c index d6baf73a6458..c0da89e16e6f 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c @@ -241,7 +241,7 @@ static int vmw_debugfs_gem_info_show(struct seq_file *m, void *unused) * Therefore, we need to protect this ->comm access using RCU. */ rcu_read_lock(); - task = pid_task(file->pid, PIDTYPE_PID); + task = pid_task(file->pid, PIDTYPE_TGID); seq_printf(m, "pid %8d command %s:\n", pid_nr(file->pid), task ? task->comm : ""); rcu_read_unlock(); -- cgit