| Message ID | 20180813102443.12662-1-tzimmermann@suse.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | Provide init/release functions for struct ttm_bo_global | expand |
Yes, please! I had it on my TODO list to clean that up for an eternity. Actually I never understood why that should be driver work to setup TTM? I mean can't we just have a module_init/module_exit for TTM? Thanks, Christian. Am 13.08.2018 um 12:24 schrieb Thomas Zimmermann: > TTM uses global memory and BO for backing graphics buffers. These are > represented by struct ttm_mem_global and struct ttm_bo_global. > > Currently, struct ttm_bo_global can only be initialized and released through > struct ttm_bo_global_ref. This is a workaround for passing an instance of > ttm_mem_global to the BO global initialization code. > > The use of struct ttm_bo_global_ref makes driver code unnecessary hard to > understand. At the same time drivers can use any combination of memory and > BO for initializing the global instances. This can result in subtle bugs > when the order of initializing and releasing drivers changes. > > As a first step for resolving these problems, the provided patch set > separates initialization and release of struct ttm_bo_global from > struct ttm_bo_global_ref. > > The first patch only renames ttm_bo_global_{init/release}. Hopefully this > change can be applied at once for all drivers. > > Future directions: All TTM-based drivers follow the same pattern for setting > up the TTM. In a follow-up patch, this code can be moved into a single place > and shared among drivers. > > Thomas Zimmermann (2): > drm/ttm: Rename ttm_bo_global_{init,release}() to > ttm_bo_global_ref_*() > drm/ttm: Provide ttm_bo_global_{init/release}() for struct > ttm_bo_global > > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 4 +- > drivers/gpu/drm/ast/ast_ttm.c | 4 +- > drivers/gpu/drm/bochs/bochs_mm.c | 4 +- > drivers/gpu/drm/cirrus/cirrus_ttm.c | 4 +- > drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 4 +- > drivers/gpu/drm/mgag200/mgag200_ttm.c | 4 +- > drivers/gpu/drm/nouveau/nouveau_ttm.c | 4 +- > drivers/gpu/drm/qxl/qxl_ttm.c | 4 +- > drivers/gpu/drm/radeon/radeon_ttm.c | 4 +- > drivers/gpu/drm/ttm/ttm_bo.c | 12 ++--- > drivers/gpu/drm/virtio/virtgpu_ttm.c | 4 +- > drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 4 +- > drivers/staging/vboxvideo/vbox_ttm.c | 4 +- > include/drm/ttm/ttm_bo_driver.h | 53 ++++++++++++++++----- > 14 files changed, 70 insertions(+), 43 deletions(-) > > -- > 2.18.0 >
Hi Am 13.08.2018 um 12:33 schrieb Christian König: > Yes, please! I had it on my TODO list to clean that up for an eternity. On top of these patches, I have a patch set that provides a single init/release interface for TTM global data. I'll post it when the current patches got some feed back. I'd really like to move the code from drm_global.c back into TTM. It's TTM-specific and not useful elsewhere. However, the first commit message from 2010 [1] says that some unnamed, external driver uses this code for something. Do you know if this still applies? Best regards Thomas [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ba4420c224c2808f2661cf8428f43ceef7a73a4a > > Actually I never understood why that should be driver work to setup TTM? > > I mean can't we just have a module_init/module_exit for TTM? > > Thanks, > Christian. > > Am 13.08.2018 um 12:24 schrieb Thomas Zimmermann: >> TTM uses global memory and BO for backing graphics buffers. These are >> represented by struct ttm_mem_global and struct ttm_bo_global. >> >> Currently, struct ttm_bo_global can only be initialized and released >> through >> struct ttm_bo_global_ref. This is a workaround for passing an instance of >> ttm_mem_global to the BO global initialization code. >> >> The use of struct ttm_bo_global_ref makes driver code unnecessary hard to >> understand. At the same time drivers can use any combination of memory >> and >> BO for initializing the global instances. This can result in subtle bugs >> when the order of initializing and releasing drivers changes. >> >> As a first step for resolving these problems, the provided patch set >> separates initialization and release of struct ttm_bo_global from >> struct ttm_bo_global_ref. >> >> The first patch only renames ttm_bo_global_{init/release}. Hopefully this >> change can be applied at once for all drivers. >> >> Future directions: All TTM-based drivers follow the same pattern for >> setting >> up the TTM. In a follow-up patch, this code can be moved into a single >> place >> and shared among drivers. >> >> Thomas Zimmermann (2): >> drm/ttm: Rename ttm_bo_global_{init,release}() to >> ttm_bo_global_ref_*() >> drm/ttm: Provide ttm_bo_global_{init/release}() for struct >> ttm_bo_global >> >> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 4 +- >> drivers/gpu/drm/ast/ast_ttm.c | 4 +- >> drivers/gpu/drm/bochs/bochs_mm.c | 4 +- >> drivers/gpu/drm/cirrus/cirrus_ttm.c | 4 +- >> drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 4 +- >> drivers/gpu/drm/mgag200/mgag200_ttm.c | 4 +- >> drivers/gpu/drm/nouveau/nouveau_ttm.c | 4 +- >> drivers/gpu/drm/qxl/qxl_ttm.c | 4 +- >> drivers/gpu/drm/radeon/radeon_ttm.c | 4 +- >> drivers/gpu/drm/ttm/ttm_bo.c | 12 ++--- >> drivers/gpu/drm/virtio/virtgpu_ttm.c | 4 +- >> drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 4 +- >> drivers/staging/vboxvideo/vbox_ttm.c | 4 +- >> include/drm/ttm/ttm_bo_driver.h | 53 ++++++++++++++++----- >> 14 files changed, 70 insertions(+), 43 deletions(-) >> >> -- >> 2.18.0 >> >
On 08/13/2018 02:28 PM, Thomas Zimmermann wrote: > Hi > > Am 13.08.2018 um 12:33 schrieb Christian König: >> Yes, please! I had it on my TODO list to clean that up for an eternity. > On top of these patches, I have a patch set that provides a single > init/release interface for TTM global data. I'll post it when the > current patches got some feed back. > > I'd really like to move the code from drm_global.c back into TTM. It's > TTM-specific and not useful elsewhere. However, the first commit message > from 2010 [1] says that some unnamed, external driver uses this code for > something. Do you know if this still applies? I'm not sure actually, and not even sure that external driver ever intended to be compatible with upcoming upstream versions of drm. In that case I guess whoever maintains it should speak up now... The drm global stuff was added to facilitate for subsystems that wanted to register stuff that was really system-global and not per-device, and also not used by all drivers. As an example, even if the memory accounting is currently restricted to TTM, IMO we probably want something drm global to avoid malicious user-space app consuming all kernel memory by, for example, repeatedly open gem handles until kernel memory is exhausted. TTM drivers should stop that, but it's pretty meaningless if they share the system with a non-TTM driver that considers this "stupid and difficult". So having said that, I'm not really against ditching the drm_global stuff since it hasn't found any out-of-ttm usage. /Thomas > > Best regards > Thomas > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ba4420c224c2808f2661cf8428f43ceef7a73a4a > >> Actually I never understood why that should be driver work to setup TTM? >> >> I mean can't we just have a module_init/module_exit for TTM? >> >> Thanks, >> Christian. >> >> Am 13.08.2018 um 12:24 schrieb Thomas Zimmermann: >>> TTM uses global memory and BO for backing graphics buffers. These are >>> represented by struct ttm_mem_global and struct ttm_bo_global. >>> >>> Currently, struct ttm_bo_global can only be initialized and released >>> through >>> struct ttm_bo_global_ref. This is a workaround for passing an instance of >>> ttm_mem_global to the BO global initialization code. >>> >>> The use of struct ttm_bo_global_ref makes driver code unnecessary hard to >>> understand. At the same time drivers can use any combination of memory >>> and >>> BO for initializing the global instances. This can result in subtle bugs >>> when the order of initializing and releasing drivers changes. >>> >>> As a first step for resolving these problems, the provided patch set >>> separates initialization and release of struct ttm_bo_global from >>> struct ttm_bo_global_ref. >>> >>> The first patch only renames ttm_bo_global_{init/release}. Hopefully this >>> change can be applied at once for all drivers. >>> >>> Future directions: All TTM-based drivers follow the same pattern for >>> setting >>> up the TTM. In a follow-up patch, this code can be moved into a single >>> place >>> and shared among drivers. >>> >>> Thomas Zimmermann (2): >>> drm/ttm: Rename ttm_bo_global_{init,release}() to >>> ttm_bo_global_ref_*() >>> drm/ttm: Provide ttm_bo_global_{init/release}() for struct >>> ttm_bo_global >>> >>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 4 +- >>> drivers/gpu/drm/ast/ast_ttm.c | 4 +- >>> drivers/gpu/drm/bochs/bochs_mm.c | 4 +- >>> drivers/gpu/drm/cirrus/cirrus_ttm.c | 4 +- >>> drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 4 +- >>> drivers/gpu/drm/mgag200/mgag200_ttm.c | 4 +- >>> drivers/gpu/drm/nouveau/nouveau_ttm.c | 4 +- >>> drivers/gpu/drm/qxl/qxl_ttm.c | 4 +- >>> drivers/gpu/drm/radeon/radeon_ttm.c | 4 +- >>> drivers/gpu/drm/ttm/ttm_bo.c | 12 ++--- >>> drivers/gpu/drm/virtio/virtgpu_ttm.c | 4 +- >>> drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 4 +- >>> drivers/staging/vboxvideo/vbox_ttm.c | 4 +- >>> include/drm/ttm/ttm_bo_driver.h | 53 ++++++++++++++++----- >>> 14 files changed, 70 insertions(+), 43 deletions(-) >>> >>> -- >>> 2.18.0 >>>
Hi Christian, I just wanted to ask if there's something else required to get this patch set reviewed and landed. On top of these two patches, I have a patch set that replaces the driver-specific global-bo-and-mem combos with a single struct ttm_global structure. It further merges struct drm_global into struct ttm_global and implements it such that drivers can either share the global memory or create their private instance. It's also a step towards drm device hotplug, which someone just asked. Best regards Thomas Am 13.08.2018 um 12:33 schrieb Christian König: > Yes, please! I had it on my TODO list to clean that up for an eternity. > > Actually I never understood why that should be driver work to setup TTM? > > I mean can't we just have a module_init/module_exit for TTM? > > Thanks, > Christian. > > Am 13.08.2018 um 12:24 schrieb Thomas Zimmermann: >> TTM uses global memory and BO for backing graphics buffers. These are >> represented by struct ttm_mem_global and struct ttm_bo_global. >> >> Currently, struct ttm_bo_global can only be initialized and released >> through >> struct ttm_bo_global_ref. This is a workaround for passing an instance of >> ttm_mem_global to the BO global initialization code. >> >> The use of struct ttm_bo_global_ref makes driver code unnecessary hard to >> understand. At the same time drivers can use any combination of memory >> and >> BO for initializing the global instances. This can result in subtle bugs >> when the order of initializing and releasing drivers changes. >> >> As a first step for resolving these problems, the provided patch set >> separates initialization and release of struct ttm_bo_global from >> struct ttm_bo_global_ref. >> >> The first patch only renames ttm_bo_global_{init/release}. Hopefully this >> change can be applied at once for all drivers. >> >> Future directions: All TTM-based drivers follow the same pattern for >> setting >> up the TTM. In a follow-up patch, this code can be moved into a single >> place >> and shared among drivers. >> >> Thomas Zimmermann (2): >> drm/ttm: Rename ttm_bo_global_{init,release}() to >> ttm_bo_global_ref_*() >> drm/ttm: Provide ttm_bo_global_{init/release}() for struct >> ttm_bo_global >> >> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 4 +- >> drivers/gpu/drm/ast/ast_ttm.c | 4 +- >> drivers/gpu/drm/bochs/bochs_mm.c | 4 +- >> drivers/gpu/drm/cirrus/cirrus_ttm.c | 4 +- >> drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 4 +- >> drivers/gpu/drm/mgag200/mgag200_ttm.c | 4 +- >> drivers/gpu/drm/nouveau/nouveau_ttm.c | 4 +- >> drivers/gpu/drm/qxl/qxl_ttm.c | 4 +- >> drivers/gpu/drm/radeon/radeon_ttm.c | 4 +- >> drivers/gpu/drm/ttm/ttm_bo.c | 12 ++--- >> drivers/gpu/drm/virtio/virtgpu_ttm.c | 4 +- >> drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 4 +- >> drivers/staging/vboxvideo/vbox_ttm.c | 4 +- >> include/drm/ttm/ttm_bo_driver.h | 53 ++++++++++++++++----- >> 14 files changed, 70 insertions(+), 43 deletions(-) >> >> -- >> 2.18.0 >> >
Hi Thomas, Am 30.08.2018 um 08:34 schrieb Thomas Zimmermann: > Hi Christian, > > I just wanted to ask if there's something else required to get this > patch set reviewed and landed. I just need to find some time to review them. > On top of these two patches, I have a patch set that replaces the > driver-specific global-bo-and-mem combos with a single struct ttm_global > structure. It further merges struct drm_global into struct ttm_global > and implements it such that drivers can either share the global memory That sounds good. > or create their private instance. That doesn't sounds good. Drivers should not be allowed to create their own private instance of that. Thanks for doing this, Christian. > It's also a step towards drm device > hotplug, which someone just asked. > > Best regards > Thomas > > > Am 13.08.2018 um 12:33 schrieb Christian König: >> Yes, please! I had it on my TODO list to clean that up for an eternity. >> >> Actually I never understood why that should be driver work to setup TTM? >> >> I mean can't we just have a module_init/module_exit for TTM? >> >> Thanks, >> Christian. >> >> Am 13.08.2018 um 12:24 schrieb Thomas Zimmermann: >>> TTM uses global memory and BO for backing graphics buffers. These are >>> represented by struct ttm_mem_global and struct ttm_bo_global. >>> >>> Currently, struct ttm_bo_global can only be initialized and released >>> through >>> struct ttm_bo_global_ref. This is a workaround for passing an instance of >>> ttm_mem_global to the BO global initialization code. >>> >>> The use of struct ttm_bo_global_ref makes driver code unnecessary hard to >>> understand. At the same time drivers can use any combination of memory >>> and >>> BO for initializing the global instances. This can result in subtle bugs >>> when the order of initializing and releasing drivers changes. >>> >>> As a first step for resolving these problems, the provided patch set >>> separates initialization and release of struct ttm_bo_global from >>> struct ttm_bo_global_ref. >>> >>> The first patch only renames ttm_bo_global_{init/release}. Hopefully this >>> change can be applied at once for all drivers. >>> >>> Future directions: All TTM-based drivers follow the same pattern for >>> setting >>> up the TTM. In a follow-up patch, this code can be moved into a single >>> place >>> and shared among drivers. >>> >>> Thomas Zimmermann (2): >>> drm/ttm: Rename ttm_bo_global_{init,release}() to >>> ttm_bo_global_ref_*() >>> drm/ttm: Provide ttm_bo_global_{init/release}() for struct >>> ttm_bo_global >>> >>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 4 +- >>> drivers/gpu/drm/ast/ast_ttm.c | 4 +- >>> drivers/gpu/drm/bochs/bochs_mm.c | 4 +- >>> drivers/gpu/drm/cirrus/cirrus_ttm.c | 4 +- >>> drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 4 +- >>> drivers/gpu/drm/mgag200/mgag200_ttm.c | 4 +- >>> drivers/gpu/drm/nouveau/nouveau_ttm.c | 4 +- >>> drivers/gpu/drm/qxl/qxl_ttm.c | 4 +- >>> drivers/gpu/drm/radeon/radeon_ttm.c | 4 +- >>> drivers/gpu/drm/ttm/ttm_bo.c | 12 ++--- >>> drivers/gpu/drm/virtio/virtgpu_ttm.c | 4 +- >>> drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 4 +- >>> drivers/staging/vboxvideo/vbox_ttm.c | 4 +- >>> include/drm/ttm/ttm_bo_driver.h | 53 ++++++++++++++++----- >>> 14 files changed, 70 insertions(+), 43 deletions(-) >>> >>> -- >>> 2.18.0 >>>
Am 30.08.2018 um 08:45 schrieb Christian König: [...] >> or create their private instance. > > That doesn't sounds good. Drivers should not be allowed to create their > own private instance of that. OK, will be changed in the patchset. Best regards Thomas > > Thanks for doing this, > Christian. > >> It's also a step towards drm device >> hotplug, which someone just asked. >> >> Best regards >> Thomas >> >> >> Am 13.08.2018 um 12:33 schrieb Christian König: >>> Yes, please! I had it on my TODO list to clean that up for an eternity. >>> >>> Actually I never understood why that should be driver work to setup TTM? >>> >>> I mean can't we just have a module_init/module_exit for TTM? >>> >>> Thanks, >>> Christian. >>> >>> Am 13.08.2018 um 12:24 schrieb Thomas Zimmermann: >>>> TTM uses global memory and BO for backing graphics buffers. These are >>>> represented by struct ttm_mem_global and struct ttm_bo_global. >>>> >>>> Currently, struct ttm_bo_global can only be initialized and released >>>> through >>>> struct ttm_bo_global_ref. This is a workaround for passing an >>>> instance of >>>> ttm_mem_global to the BO global initialization code. >>>> >>>> The use of struct ttm_bo_global_ref makes driver code unnecessary >>>> hard to >>>> understand. At the same time drivers can use any combination of memory >>>> and >>>> BO for initializing the global instances. This can result in subtle >>>> bugs >>>> when the order of initializing and releasing drivers changes. >>>> >>>> As a first step for resolving these problems, the provided patch set >>>> separates initialization and release of struct ttm_bo_global from >>>> struct ttm_bo_global_ref. >>>> >>>> The first patch only renames ttm_bo_global_{init/release}. Hopefully >>>> this >>>> change can be applied at once for all drivers. >>>> >>>> Future directions: All TTM-based drivers follow the same pattern for >>>> setting >>>> up the TTM. In a follow-up patch, this code can be moved into a single >>>> place >>>> and shared among drivers. >>>> >>>> Thomas Zimmermann (2): >>>> drm/ttm: Rename ttm_bo_global_{init,release}() to >>>> ttm_bo_global_ref_*() >>>> drm/ttm: Provide ttm_bo_global_{init/release}() for struct >>>> ttm_bo_global >>>> >>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 4 +- >>>> drivers/gpu/drm/ast/ast_ttm.c | 4 +- >>>> drivers/gpu/drm/bochs/bochs_mm.c | 4 +- >>>> drivers/gpu/drm/cirrus/cirrus_ttm.c | 4 +- >>>> drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 4 +- >>>> drivers/gpu/drm/mgag200/mgag200_ttm.c | 4 +- >>>> drivers/gpu/drm/nouveau/nouveau_ttm.c | 4 +- >>>> drivers/gpu/drm/qxl/qxl_ttm.c | 4 +- >>>> drivers/gpu/drm/radeon/radeon_ttm.c | 4 +- >>>> drivers/gpu/drm/ttm/ttm_bo.c | 12 ++--- >>>> drivers/gpu/drm/virtio/virtgpu_ttm.c | 4 +- >>>> drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 4 +- >>>> drivers/staging/vboxvideo/vbox_ttm.c | 4 +- >>>> include/drm/ttm/ttm_bo_driver.h | 53 >>>> ++++++++++++++++----- >>>> 14 files changed, 70 insertions(+), 43 deletions(-) >>>> >>>> -- >>>> 2.18.0 >>>> > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
TTM uses global memory and BO for backing graphics buffers. These are represented by struct ttm_mem_global and struct ttm_bo_global. Currently, struct ttm_bo_global can only be initialized and released through struct ttm_bo_global_ref. This is a workaround for passing an instance of ttm_mem_global to the BO global initialization code. The use of struct ttm_bo_global_ref makes driver code unnecessary hard to understand. At the same time drivers can use any combination of memory and BO for initializing the global instances. This can result in subtle bugs when the order of initializing and releasing drivers changes. As a first step for resolving these problems, the provided patch set separates initialization and release of struct ttm_bo_global from struct ttm_bo_global_ref. The first patch only renames ttm_bo_global_{init/release}. Hopefully this change can be applied at once for all drivers. Future directions: All TTM-based drivers follow the same pattern for setting up the TTM. In a follow-up patch, this code can be moved into a single place and shared among drivers. Thomas Zimmermann (2): drm/ttm: Rename ttm_bo_global_{init,release}() to ttm_bo_global_ref_*() drm/ttm: Provide ttm_bo_global_{init/release}() for struct ttm_bo_global drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 4 +- drivers/gpu/drm/ast/ast_ttm.c | 4 +- drivers/gpu/drm/bochs/bochs_mm.c | 4 +- drivers/gpu/drm/cirrus/cirrus_ttm.c | 4 +- drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 4 +- drivers/gpu/drm/mgag200/mgag200_ttm.c | 4 +- drivers/gpu/drm/nouveau/nouveau_ttm.c | 4 +- drivers/gpu/drm/qxl/qxl_ttm.c | 4 +- drivers/gpu/drm/radeon/radeon_ttm.c | 4 +- drivers/gpu/drm/ttm/ttm_bo.c | 12 ++--- drivers/gpu/drm/virtio/virtgpu_ttm.c | 4 +- drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 4 +- drivers/staging/vboxvideo/vbox_ttm.c | 4 +- include/drm/ttm/ttm_bo_driver.h | 53 ++++++++++++++++----- 14 files changed, 70 insertions(+), 43 deletions(-) -- 2.18.0