[PATCH 4/5] drm/vmwgfx: Introduce a new placement for MOB page tables
Thomas Hellström
thomas.hellstrom at linux.intel.com
Tue Oct 12 18:57:38 UTC 2021
On 10/8/21 19:31, Zack Rusin wrote:
> For larger (bigger than a page) and noncontiguous mobs we have
> to create page tables that allow the host to find the memory.
> Those page tables just used regular system memory. Unfortunately
> in TTM those BO's are not allowed to be busy thus can't be
> fenced and we have to fence those bo's because we don't want
> to destroy the page tables while the host is still executing
> the command buffers which might be accessing them.
>
> To solve it we introduce a new placement VMW_PL_SYSTEM which
> is very similar to TTM_PL_SYSTEM except that it allows
> fencing. This fixes kernel oops'es during unloading of the driver
> (and pci hot remove/add) which were caused by busy BO's in
> TTM_PL_SYSTEM being present in the delayed deletion list in
> TTM (TTM_PL_SYSTEM manager is destroyed before the delayed
> deletions are executed)
>
> Signed-off-by: Zack Rusin <zackr at vmware.com>
> Reviewed-by: Martin Krastev <krastevm at vmware.com>
> Cc: Christian König <christian.koenig at amd.com>
> Cc: Thomas Hellström <thomas.hellstrom at linux.intel.com>
In general looks good to me. Some suggestions below:
> ---
> drivers/gpu/drm/vmwgfx/Makefile | 2 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 14 ++-
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 12 ++-
> .../gpu/drm/vmwgfx/vmwgfx_system_manager.c | 90 +++++++++++++++++++
> drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 58 ++++++------
> 5 files changed, 138 insertions(+), 38 deletions(-)
> create mode 100644 drivers/gpu/drm/vmwgfx/vmwgfx_system_manager.c
>
> diff --git a/drivers/gpu/drm/vmwgfx/Makefile b/drivers/gpu/drm/vmwgfx/Makefile
> index bc323f7d4032..0188a312c38c 100644
> --- a/drivers/gpu/drm/vmwgfx/Makefile
> +++ b/drivers/gpu/drm/vmwgfx/Makefile
> @@ -9,7 +9,7 @@ vmwgfx-y := vmwgfx_execbuf.o vmwgfx_gmr.o vmwgfx_kms.o vmwgfx_drv.o \
> vmwgfx_cotable.o vmwgfx_so.o vmwgfx_binding.o vmwgfx_msg.o \
> vmwgfx_simple_resource.o vmwgfx_va.o vmwgfx_blit.o \
> vmwgfx_validation.o vmwgfx_page_dirty.o vmwgfx_streamoutput.o \
> - vmwgfx_devcaps.o ttm_object.o ttm_memory.o
> + vmwgfx_devcaps.o ttm_object.o ttm_memory.o vmwgfx_system_manager.o
>
> vmwgfx-$(CONFIG_DRM_FBDEV_EMULATION) += vmwgfx_fb.o
> vmwgfx-$(CONFIG_TRANSPARENT_HUGEPAGE) += vmwgfx_thp.o
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index 8d0b083ba267..daf65615308a 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -1071,6 +1071,12 @@ static int vmw_driver_load(struct vmw_private *dev_priv, u32 pci_id)
> "3D will be disabled.\n");
> dev_priv->has_mob = false;
> }
> + if (vmw_sys_man_init(dev_priv) != 0) {
> + drm_info(&dev_priv->drm,
> + "No MOB page table memory available. "
> + "3D will be disabled.\n");
> + dev_priv->has_mob = false;
> + }
> }
>
> if (dev_priv->has_mob && (dev_priv->capabilities & SVGA_CAP_DX)) {
> @@ -1121,8 +1127,10 @@ static int vmw_driver_load(struct vmw_private *dev_priv, u32 pci_id)
> vmw_overlay_close(dev_priv);
> vmw_kms_close(dev_priv);
> out_no_kms:
> - if (dev_priv->has_mob)
> + if (dev_priv->has_mob) {
> vmw_gmrid_man_fini(dev_priv, VMW_PL_MOB);
> + vmw_sys_man_fini(dev_priv);
> + }
> if (dev_priv->has_gmr)
> vmw_gmrid_man_fini(dev_priv, VMW_PL_GMR);
> vmw_devcaps_destroy(dev_priv);
> @@ -1172,8 +1180,10 @@ static void vmw_driver_unload(struct drm_device *dev)
> vmw_gmrid_man_fini(dev_priv, VMW_PL_GMR);
>
> vmw_release_device_early(dev_priv);
> - if (dev_priv->has_mob)
> + if (dev_priv->has_mob) {
> vmw_gmrid_man_fini(dev_priv, VMW_PL_MOB);
> + vmw_sys_man_fini(dev_priv);
> + }
> vmw_devcaps_destroy(dev_priv);
> vmw_vram_manager_fini(dev_priv);
> ttm_device_fini(&dev_priv->bdev);
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> index a833751099b5..df19dfb3ce18 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> @@ -82,8 +82,9 @@
> VMWGFX_NUM_GB_SURFACE +\
> VMWGFX_NUM_GB_SCREEN_TARGET)
>
> -#define VMW_PL_GMR (TTM_PL_PRIV + 0)
> -#define VMW_PL_MOB (TTM_PL_PRIV + 1)
> +#define VMW_PL_GMR (TTM_PL_PRIV + 0)
> +#define VMW_PL_MOB (TTM_PL_PRIV + 1)
> +#define VMW_PL_SYSTEM (TTM_PL_PRIV + 2)
>
> #define VMW_RES_CONTEXT ttm_driver_type0
> #define VMW_RES_SURFACE ttm_driver_type1
> @@ -1039,7 +1040,6 @@ extern struct ttm_placement vmw_vram_placement;
> extern struct ttm_placement vmw_vram_sys_placement;
> extern struct ttm_placement vmw_vram_gmr_placement;
> extern struct ttm_placement vmw_sys_placement;
> -extern struct ttm_placement vmw_evictable_placement;
> extern struct ttm_placement vmw_srf_placement;
> extern struct ttm_placement vmw_mob_placement;
> extern struct ttm_placement vmw_nonfixed_placement;
> @@ -1251,6 +1251,12 @@ int vmw_overlay_num_free_overlays(struct vmw_private *dev_priv);
> int vmw_gmrid_man_init(struct vmw_private *dev_priv, int type);
> void vmw_gmrid_man_fini(struct vmw_private *dev_priv, int type);
>
> +/**
> + * System memory manager
> + */
> +int vmw_sys_man_init(struct vmw_private *dev_priv);
> +void vmw_sys_man_fini(struct vmw_private *dev_priv);
> +
> /**
> * Prime - vmwgfx_prime.c
> */
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_system_manager.c b/drivers/gpu/drm/vmwgfx/vmwgfx_system_manager.c
> new file mode 100644
> index 000000000000..2b86e2d8aefe
> --- /dev/null
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_system_manager.c
> @@ -0,0 +1,90 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> +/*
> + * Copyright 2021 VMware, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person
> + * obtaining a copy of this software and associated documentation
> + * files (the "Software"), to deal in the Software without
> + * restriction, including without limitation the rights to use, copy,
> + * modify, merge, publish, distribute, sublicense, and/or sell copies
> + * of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be
> + * included in all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + *
> + */
> +
> +#include "vmwgfx_drv.h"
> +
> +#include <drm/ttm/ttm_bo_driver.h>
> +#include <drm/ttm/ttm_device.h>
> +#include <drm/ttm/ttm_placement.h>
> +#include <drm/ttm/ttm_resource.h>
> +#include <linux/slab.h>
> +
> +
> +static int vmw_sys_man_alloc(struct ttm_resource_manager *man,
> + struct ttm_buffer_object *bo,
> + const struct ttm_place *place,
> + struct ttm_resource **res)
> +{
> + *res = kzalloc(sizeof(**res), GFP_KERNEL);
> + if (!*res)
> + return -ENOMEM;
> +
> + ttm_resource_init(bo, place, *res);
> + return 0;
> +}
> +
> +static void vmw_sys_man_free(struct ttm_resource_manager *man,
> + struct ttm_resource *res)
> +{
> + kfree(res);
> +}
> +
> +static const struct ttm_resource_manager_func vmw_sys_manager_func = {
> + .alloc = vmw_sys_man_alloc,
> + .free = vmw_sys_man_free,
> +};
> +
> +int vmw_sys_man_init(struct vmw_private *dev_priv)
> +{
> + struct ttm_device *bdev = &dev_priv->bdev;
> + struct ttm_resource_manager *man =
> + kzalloc(sizeof(*man), GFP_KERNEL);
> +
> + if (unlikely(!man))
> + return -ENOMEM;
Nit: Branch prediction hints are typically not used in driver code. And
here the potential benefit indeed appears small :)
> +
> + man->use_tt = true;
> + man->func = &vmw_sys_manager_func;
> +
> + ttm_resource_manager_init(man, 0);
> + ttm_set_driver_manager(bdev, VMW_PL_SYSTEM, man);
> + ttm_resource_manager_set_used(man, true);
> + return 0;
> +}
> +
> +void vmw_sys_man_fini(struct vmw_private *dev_priv)
> +{
> + struct ttm_resource_manager *man = ttm_manager_type(&dev_priv->bdev,
> + VMW_PL_SYSTEM);
> +
> + ttm_resource_manager_evict_all(&dev_priv->bdev, man);
> +
> + ttm_resource_manager_set_used(man, false);
> + ttm_resource_manager_cleanup(man);
> +
> + ttm_set_driver_manager(&dev_priv->bdev, VMW_PL_SYSTEM, NULL);
> + kfree(man);
> +}
I seem to recognize the general pattern here from the ttm_sys_manager,
Any chance we could add what's needed to the ttm_sys_manager and make
the code reusable? That's the _fini function and the memory type choice
I guess. I figure i915 will need exactly the same.
/Thomas
More information about the dri-devel
mailing list