[PATCH 1/5] drm/ttm: use a static ttm_mem_global instance
Thomas Zimmermann
tzimmermann at suse.de
Tue Oct 23 06:22:52 UTC 2018
Hi Christian
Am 19.10.18 um 18:41 schrieb Christian König:
> As the name says we only need one global instance of ttm_mem_global.
>
> Drop all the driver initialization and just use a single exported
> instance which is initialized during BO global initialization.
>
> Signed-off-by: Christian König <christian.koenig at amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 44 -------------------------
> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h | 1 -
> drivers/gpu/drm/ast/ast_drv.h | 1 -
> drivers/gpu/drm/ast/ast_ttm.c | 32 ++----------------
> drivers/gpu/drm/bochs/bochs.h | 1 -
> drivers/gpu/drm/bochs/bochs_mm.c | 30 ++---------------
> drivers/gpu/drm/cirrus/cirrus_drv.h | 1 -
> drivers/gpu/drm/cirrus/cirrus_ttm.c | 32 ++----------------
> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 1 -
> drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 31 +++--------------
> drivers/gpu/drm/mgag200/mgag200_drv.h | 1 -
> drivers/gpu/drm/mgag200/mgag200_ttm.c | 32 ++----------------
> drivers/gpu/drm/nouveau/nouveau_drv.h | 1 -
> drivers/gpu/drm/nouveau/nouveau_ttm.c | 34 ++-----------------
> drivers/gpu/drm/qxl/qxl_drv.h | 1 -
> drivers/gpu/drm/qxl/qxl_ttm.c | 28 ----------------
> drivers/gpu/drm/radeon/radeon.h | 1 -
> drivers/gpu/drm/radeon/radeon_ttm.c | 26 ---------------
> drivers/gpu/drm/ttm/ttm_bo.c | 10 ++++--
> drivers/gpu/drm/ttm/ttm_memory.c | 5 +--
> drivers/gpu/drm/virtio/virtgpu_drv.h | 1 -
> drivers/gpu/drm/virtio/virtgpu_ttm.c | 27 ---------------
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 4 +--
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 3 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 27 ---------------
> drivers/staging/vboxvideo/vbox_drv.h | 1 -
> drivers/staging/vboxvideo/vbox_ttm.c | 24 --------------
> include/drm/ttm/ttm_bo_driver.h | 8 ++---
> include/drm/ttm/ttm_memory.h | 4 +--
> 29 files changed, 32 insertions(+), 380 deletions(-)
Great that you removed all the global TTM state from all the drivers.
This removes a lot of duplication and simplifies driver development a bit.
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 9edece6510d3..3006050b1720 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -1526,18 +1526,22 @@ void ttm_bo_global_release(struct ttm_bo_global *glob)
> {
> kobject_del(&glob->kobj);
> kobject_put(&glob->kobj);
> + ttm_mem_global_release(&ttm_mem_glob);
> }
> EXPORT_SYMBOL(ttm_bo_global_release);
>
> -int ttm_bo_global_init(struct ttm_bo_global *glob,
> - struct ttm_mem_global *mem_glob)
> +int ttm_bo_global_init(struct ttm_bo_global *glob)
> {
> int ret;
> unsigned i;
>
> + ret = ttm_mem_global_init(&ttm_mem_glob);
> + if (ret)
> + return ret;
> +
What I really dislike about this patch set is that it mixes state and
implementation into that same functions. The original code had a fairly
good separation of both. Now the mechanisms and policies are located in
the same places.[^1]
This looks like a simplification, but from my experience, such code is a
setup for long-term maintenance problems. For example, I can imagine
that someone at some point wants multiple global buffers (e.g., on a
NUMA-like architecture).
I understand that I'm new here, have no say, and probably don't get the
big picture, but from my point of view, this is not a forward-thinking
change.
Best regards
Thomas
[^1] More philosophically speaking, program state can be global, but
data structures can only be share-able. ttm_mem_global and ttm_bo_global
should be renamed to something like ttm_shared_mem, rsp. ttm_shared_bo.
> mutex_init(&glob->device_list_mutex);
> spin_lock_init(&glob->lru_lock);
> - glob->mem_glob = mem_glob;
> + glob->mem_glob = &ttm_mem_glob;
> glob->mem_glob->bo_glob = glob;
> glob->dummy_read_page = alloc_page(__GFP_ZERO | GFP_DMA32);
>
> diff --git a/drivers/gpu/drm/ttm/ttm_memory.c b/drivers/gpu/drm/ttm/ttm_memory.c
> index 450387c92b63..7704e17c402f 100644
> --- a/drivers/gpu/drm/ttm/ttm_memory.c
> +++ b/drivers/gpu/drm/ttm/ttm_memory.c
> @@ -41,6 +41,9 @@
>
> #define TTM_MEMORY_ALLOC_RETRIES 4
>
> +struct ttm_mem_global ttm_mem_glob;
> +EXPORT_SYMBOL(ttm_mem_glob);
> +
> struct ttm_mem_zone {
> struct kobject kobj;
> struct ttm_mem_global *glob;
> @@ -464,7 +467,6 @@ int ttm_mem_global_init(struct ttm_mem_global *glob)
> ttm_mem_global_release(glob);
> return ret;
> }
> -EXPORT_SYMBOL(ttm_mem_global_init);
>
> void ttm_mem_global_release(struct ttm_mem_global *glob)
> {
> @@ -486,7 +488,6 @@ void ttm_mem_global_release(struct ttm_mem_global *glob)
> kobject_del(&glob->kobj);
> kobject_put(&glob->kobj);
> }
> -EXPORT_SYMBOL(ttm_mem_global_release);
>
> static void ttm_check_swapping(struct ttm_mem_global *glob)
> {
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index 65605e207bbe..dec42d421e00 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -133,7 +133,6 @@ struct virtio_gpu_framebuffer {
>
> struct virtio_gpu_mman {
> struct ttm_bo_global_ref bo_global_ref;
> - struct drm_global_reference mem_global_ref;
> bool mem_global_referenced;
> struct ttm_bo_device bdev;
> };
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ttm.c b/drivers/gpu/drm/virtio/virtgpu_ttm.c
> index 0ec46b47b423..b99ecc6d97d3 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ttm.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ttm.c
> @@ -50,37 +50,12 @@ virtio_gpu_device *virtio_gpu_get_vgdev(struct ttm_bo_device *bdev)
> return vgdev;
> }
>
> -static int virtio_gpu_ttm_mem_global_init(struct drm_global_reference *ref)
> -{
> - return ttm_mem_global_init(ref->object);
> -}
> -
> -static void virtio_gpu_ttm_mem_global_release(struct drm_global_reference *ref)
> -{
> - ttm_mem_global_release(ref->object);
> -}
> -
> static int virtio_gpu_ttm_global_init(struct virtio_gpu_device *vgdev)
> {
> struct drm_global_reference *global_ref;
> int r;
>
> vgdev->mman.mem_global_referenced = false;
> - global_ref = &vgdev->mman.mem_global_ref;
> - global_ref->global_type = DRM_GLOBAL_TTM_MEM;
> - global_ref->size = sizeof(struct ttm_mem_global);
> - global_ref->init = &virtio_gpu_ttm_mem_global_init;
> - global_ref->release = &virtio_gpu_ttm_mem_global_release;
> -
> - r = drm_global_item_ref(global_ref);
> - if (r != 0) {
> - DRM_ERROR("Failed setting up TTM memory accounting "
> - "subsystem.\n");
> - return r;
> - }
> -
> - vgdev->mman.bo_global_ref.mem_glob =
> - vgdev->mman.mem_global_ref.object;
> global_ref = &vgdev->mman.bo_global_ref.ref;
> global_ref->global_type = DRM_GLOBAL_TTM_BO;
> global_ref->size = sizeof(struct ttm_bo_global);
> @@ -89,7 +64,6 @@ static int virtio_gpu_ttm_global_init(struct virtio_gpu_device *vgdev)
> r = drm_global_item_ref(global_ref);
> if (r != 0) {
> DRM_ERROR("Failed setting up TTM BO subsystem.\n");
> - drm_global_item_unref(&vgdev->mman.mem_global_ref);
> return r;
> }
>
> @@ -101,7 +75,6 @@ static void virtio_gpu_ttm_global_fini(struct virtio_gpu_device *vgdev)
> {
> if (vgdev->mman.mem_global_referenced) {
> drm_global_item_unref(&vgdev->mman.bo_global_ref.ref);
> - drm_global_item_unref(&vgdev->mman.mem_global_ref);
> vgdev->mman.mem_global_referenced = false;
> }
> }
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index bb6dbbe18835..57df776d987c 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -828,8 +828,8 @@ static int vmw_driver_load(struct drm_device *dev, unsigned long chipset)
> goto out_err4;
> }
>
> - dev_priv->tdev = ttm_object_device_init
> - (dev_priv->mem_global_ref.object, 12, &vmw_prime_dmabuf_ops);
> + dev_priv->tdev = ttm_object_device_init(&ttm_mem_glob, 12,
> + &vmw_prime_dmabuf_ops);
>
> if (unlikely(dev_priv->tdev == NULL)) {
> DRM_ERROR("Unable to initialize TTM object management.\n");
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> index 1abe21758b0d..df15a745efc3 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> @@ -366,7 +366,6 @@ enum {
> struct vmw_private {
> struct ttm_bo_device bdev;
> struct ttm_bo_global_ref bo_global_ref;
> - struct drm_global_reference mem_global_ref;
>
> struct vmw_fifo_state fifo;
>
> @@ -1288,7 +1287,7 @@ vmw_bo_reference(struct vmw_buffer_object *buf)
>
> static inline struct ttm_mem_global *vmw_mem_glob(struct vmw_private *dev_priv)
> {
> - return (struct ttm_mem_global *) dev_priv->mem_global_ref.object;
> + return &ttm_mem_glob;
> }
>
> static inline void vmw_fifo_resource_inc(struct vmw_private *dev_priv)
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
> index f3ce43c41978..0ac473cd5136 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
> @@ -43,36 +43,11 @@ int vmw_mmap(struct file *filp, struct vm_area_struct *vma)
> return ttm_bo_mmap(filp, vma, &dev_priv->bdev);
> }
>
> -static int vmw_ttm_mem_global_init(struct drm_global_reference *ref)
> -{
> - DRM_INFO("global init.\n");
> - return ttm_mem_global_init(ref->object);
> -}
> -
> -static void vmw_ttm_mem_global_release(struct drm_global_reference *ref)
> -{
> - ttm_mem_global_release(ref->object);
> -}
> -
> int vmw_ttm_global_init(struct vmw_private *dev_priv)
> {
> struct drm_global_reference *global_ref;
> int ret;
>
> - global_ref = &dev_priv->mem_global_ref;
> - global_ref->global_type = DRM_GLOBAL_TTM_MEM;
> - global_ref->size = sizeof(struct ttm_mem_global);
> - global_ref->init = &vmw_ttm_mem_global_init;
> - global_ref->release = &vmw_ttm_mem_global_release;
> -
> - ret = drm_global_item_ref(global_ref);
> - if (unlikely(ret != 0)) {
> - DRM_ERROR("Failed setting up TTM memory accounting.\n");
> - return ret;
> - }
> -
> - dev_priv->bo_global_ref.mem_glob =
> - dev_priv->mem_global_ref.object;
> global_ref = &dev_priv->bo_global_ref.ref;
> global_ref->global_type = DRM_GLOBAL_TTM_BO;
> global_ref->size = sizeof(struct ttm_bo_global);
> @@ -87,12 +62,10 @@ int vmw_ttm_global_init(struct vmw_private *dev_priv)
>
> return 0;
> out_no_bo:
> - drm_global_item_unref(&dev_priv->mem_global_ref);
> return ret;
> }
>
> void vmw_ttm_global_release(struct vmw_private *dev_priv)
> {
> drm_global_item_unref(&dev_priv->bo_global_ref.ref);
> - drm_global_item_unref(&dev_priv->mem_global_ref);
> }
> diff --git a/drivers/staging/vboxvideo/vbox_drv.h b/drivers/staging/vboxvideo/vbox_drv.h
> index 594f84272957..41f760b77704 100644
> --- a/drivers/staging/vboxvideo/vbox_drv.h
> +++ b/drivers/staging/vboxvideo/vbox_drv.h
> @@ -95,7 +95,6 @@ struct vbox_private {
> int fb_mtrr;
>
> struct {
> - struct drm_global_reference mem_global_ref;
> struct ttm_bo_global_ref bo_global_ref;
> struct ttm_bo_device bdev;
> } ttm;
> diff --git a/drivers/staging/vboxvideo/vbox_ttm.c b/drivers/staging/vboxvideo/vbox_ttm.c
> index 2329a55d4636..88cdacf2b0f0 100644
> --- a/drivers/staging/vboxvideo/vbox_ttm.c
> +++ b/drivers/staging/vboxvideo/vbox_ttm.c
> @@ -35,16 +35,6 @@ static inline struct vbox_private *vbox_bdev(struct ttm_bo_device *bd)
> return container_of(bd, struct vbox_private, ttm.bdev);
> }
>
> -static int vbox_ttm_mem_global_init(struct drm_global_reference *ref)
> -{
> - return ttm_mem_global_init(ref->object);
> -}
> -
> -static void vbox_ttm_mem_global_release(struct drm_global_reference *ref)
> -{
> - ttm_mem_global_release(ref->object);
> -}
> -
> /**
> * Adds the vbox memory manager object/structures to the global memory manager.
> */
> @@ -53,18 +43,6 @@ static int vbox_ttm_global_init(struct vbox_private *vbox)
> struct drm_global_reference *global_ref;
> int ret;
>
> - global_ref = &vbox->ttm.mem_global_ref;
> - global_ref->global_type = DRM_GLOBAL_TTM_MEM;
> - global_ref->size = sizeof(struct ttm_mem_global);
> - global_ref->init = &vbox_ttm_mem_global_init;
> - global_ref->release = &vbox_ttm_mem_global_release;
> - ret = drm_global_item_ref(global_ref);
> - if (ret) {
> - DRM_ERROR("Failed setting up TTM memory subsystem.\n");
> - return ret;
> - }
> -
> - vbox->ttm.bo_global_ref.mem_glob = vbox->ttm.mem_global_ref.object;
> global_ref = &vbox->ttm.bo_global_ref.ref;
> global_ref->global_type = DRM_GLOBAL_TTM_BO;
> global_ref->size = sizeof(struct ttm_bo_global);
> @@ -74,7 +52,6 @@ static int vbox_ttm_global_init(struct vbox_private *vbox)
> ret = drm_global_item_ref(global_ref);
> if (ret) {
> DRM_ERROR("Failed setting up TTM BO subsystem.\n");
> - drm_global_item_unref(&vbox->ttm.mem_global_ref);
> return ret;
> }
>
> @@ -87,7 +64,6 @@ static int vbox_ttm_global_init(struct vbox_private *vbox)
> static void vbox_ttm_global_release(struct vbox_private *vbox)
> {
> drm_global_item_unref(&vbox->ttm.bo_global_ref.ref);
> - drm_global_item_unref(&vbox->ttm.mem_global_ref);
> }
>
> static void vbox_bo_ttm_destroy(struct ttm_buffer_object *tbo)
> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
> index c6ee07d10281..4ae6fc33f761 100644
> --- a/include/drm/ttm/ttm_bo_driver.h
> +++ b/include/drm/ttm/ttm_bo_driver.h
> @@ -570,8 +570,7 @@ void ttm_bo_mem_put_locked(struct ttm_buffer_object *bo,
> struct ttm_mem_reg *mem);
>
> void ttm_bo_global_release(struct ttm_bo_global *glob);
> -int ttm_bo_global_init(struct ttm_bo_global *glob,
> - struct ttm_mem_global *mem_glob);
> +int ttm_bo_global_init(struct ttm_bo_global *glob);
>
> int ttm_bo_device_release(struct ttm_bo_device *bdev);
>
> @@ -895,7 +894,6 @@ extern const struct ttm_mem_type_manager_func ttm_bo_manager_func;
>
> struct ttm_bo_global_ref {
> struct drm_global_reference ref;
> - struct ttm_mem_global *mem_glob;
> };
>
> /**
> @@ -909,9 +907,7 @@ struct ttm_bo_global_ref {
> */
> static inline int ttm_bo_global_ref_init(struct drm_global_reference *ref)
> {
> - struct ttm_bo_global_ref *bo_ref =
> - container_of(ref, struct ttm_bo_global_ref, ref);
> - return ttm_bo_global_init(ref->object, bo_ref->mem_glob);
> + return ttm_bo_global_init(ref->object);
> }
>
> /**
> diff --git a/include/drm/ttm/ttm_memory.h b/include/drm/ttm/ttm_memory.h
> index 737b5fed8003..3ff48a0a2d7b 100644
> --- a/include/drm/ttm/ttm_memory.h
> +++ b/include/drm/ttm/ttm_memory.h
> @@ -63,7 +63,7 @@
>
> #define TTM_MEM_MAX_ZONES 2
> struct ttm_mem_zone;
> -struct ttm_mem_global {
> +extern struct ttm_mem_global {
> struct kobject kobj;
> struct ttm_bo_global *bo_glob;
> struct workqueue_struct *swap_queue;
> @@ -78,7 +78,7 @@ struct ttm_mem_global {
> #else
> struct ttm_mem_zone *zone_dma32;
> #endif
> -};
> +} ttm_mem_glob;
>
> extern int ttm_mem_global_init(struct ttm_mem_global *glob);
> extern void ttm_mem_global_release(struct ttm_mem_global *glob);
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstr. 5, D-90409 Nürnberg
Tel: +49-911-74053-0; Fax: +49-911-7417755; https://www.suse.com/
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard,
Graham Norton, HRB 21284 (AG Nürnberg)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20181023/d39be971/attachment-0001.sig>
More information about the amd-gfx
mailing list