| Message ID | 20181018162752.2241-1-tzimmermann@suse.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | Provide struct ttm_global for TTM global state | expand |
Hi Thomas, I honestly still find that way to complicated compared to what it actually does. Both ttm_mem_global and ttm_bo_global can just be some static variables. E.g. the whole handling with drm_global_reference is just superfluous. Can I just write a patch to clean up the mess we created? Regards, Christian. Am 18.10.18 um 18:27 schrieb Thomas Zimmermann: > TTM provides global memory and a global BO that is shared by all > TTM-based drivers. The data structures are provided by struct drm_global > and its helpers. All TTM-based DRM drivers copy the initialization and > clean-up code for the global TTM state from each other; leading to code > duplication. > > The new structure struct ttm_global and its helpers provide a unified > implementation. Drivers only have to initialize it and forward the > contained BO global object to ttm_bo_device_init(). > > The amdgpu and radeon drivers are converted to struct ttm_global as > part of this patch set. All other TTM-based drivers share exactly the > same code patterns and can be converted in the same way. > > Future directions: I already have patches for converting the remaining > TTM drivers to struct ttm_global. These patches can be merged after > the structure has become available in upstream. struct ttm_global is > implemented on top of struct drm_global. The latter actually maintains > TTM state instead of DRM state. Once all drivers have been converted, > the code for struct drm_global can be merged into struct ttm_global. > > (Resending this patch set, as I forgot to CC the mailing lists as first.) > > Thomas Zimmermann (3): > drm/ttm: Provide struct ttm_global for referencing TTM global state > drm/amdgpu: Replace TTM initialization/release with ttm_global > drm/radeon: Replace TTM initialization/release with ttm_global > > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 63 ++-------------- > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h | 4 +- > drivers/gpu/drm/radeon/radeon.h | 4 +- > drivers/gpu/drm/radeon/radeon_ttm.c | 40 ++-------- > drivers/gpu/drm/ttm/Makefile | 2 +- > drivers/gpu/drm/ttm/ttm_global.c | 98 +++++++++++++++++++++++++ > include/drm/drm_global.h | 8 ++ > include/drm/ttm/ttm_global.h | 79 ++++++++++++++++++++ > 8 files changed, 200 insertions(+), 98 deletions(-) > create mode 100644 drivers/gpu/drm/ttm/ttm_global.c > create mode 100644 include/drm/ttm/ttm_global.h > > -- > 2.19.1 >
Hi Am 19.10.18 um 09:24 schrieb Koenig, Christian: > Hi Thomas, > > I honestly still find that way to complicated compared to what it > actually does. > > Both ttm_mem_global and ttm_bo_global can just be some static variables. > E.g. the whole handling with drm_global_reference is just superfluous. > > Can I just write a patch to clean up the mess we created? 'Can I?' I don't know :) Sure, it's a mess. For example, if a driver inits drm_global_reference() and is later unloaded, there are dangling function pointers to the driver's code. I do a slow clean-up of the code because TTM-based drivers depend on the current way of doing things. Maybe let me post my full patch set for RFC and discuss this instead. Best regards Thomas > Regards, > Christian. > > Am 18.10.18 um 18:27 schrieb Thomas Zimmermann: >> TTM provides global memory and a global BO that is shared by all >> TTM-based drivers. The data structures are provided by struct drm_global >> and its helpers. All TTM-based DRM drivers copy the initialization and >> clean-up code for the global TTM state from each other; leading to code >> duplication. >> >> The new structure struct ttm_global and its helpers provide a unified >> implementation. Drivers only have to initialize it and forward the >> contained BO global object to ttm_bo_device_init(). >> >> The amdgpu and radeon drivers are converted to struct ttm_global as >> part of this patch set. All other TTM-based drivers share exactly the >> same code patterns and can be converted in the same way. >> >> Future directions: I already have patches for converting the remaining >> TTM drivers to struct ttm_global. These patches can be merged after >> the structure has become available in upstream. struct ttm_global is >> implemented on top of struct drm_global. The latter actually maintains >> TTM state instead of DRM state. Once all drivers have been converted, >> the code for struct drm_global can be merged into struct ttm_global. >> >> (Resending this patch set, as I forgot to CC the mailing lists as first.) >> >> Thomas Zimmermann (3): >> drm/ttm: Provide struct ttm_global for referencing TTM global state >> drm/amdgpu: Replace TTM initialization/release with ttm_global >> drm/radeon: Replace TTM initialization/release with ttm_global >> >> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 63 ++-------------- >> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h | 4 +- >> drivers/gpu/drm/radeon/radeon.h | 4 +- >> drivers/gpu/drm/radeon/radeon_ttm.c | 40 ++-------- >> drivers/gpu/drm/ttm/Makefile | 2 +- >> drivers/gpu/drm/ttm/ttm_global.c | 98 +++++++++++++++++++++++++ >> include/drm/drm_global.h | 8 ++ >> include/drm/ttm/ttm_global.h | 79 ++++++++++++++++++++ >> 8 files changed, 200 insertions(+), 98 deletions(-) >> create mode 100644 drivers/gpu/drm/ttm/ttm_global.c >> create mode 100644 include/drm/ttm/ttm_global.h >> >> -- >> 2.19.1 >> >
Am 19.10.18 um 09:44 schrieb Thomas Zimmermann: > Hi > > Am 19.10.18 um 09:24 schrieb Koenig, Christian: >> Hi Thomas, >> >> I honestly still find that way to complicated compared to what it >> actually does. >> >> Both ttm_mem_global and ttm_bo_global can just be some static variables. >> E.g. the whole handling with drm_global_reference is just superfluous. >> >> Can I just write a patch to clean up the mess we created? > 'Can I?' I don't know :) > > Sure, it's a mess. For example, if a driver inits drm_global_reference() > and is later unloaded, there are dangling function pointers to the > driver's code. > > I do a slow clean-up of the code because TTM-based drivers depend on the > current way of doing things. > > Maybe let me post my full patch set for RFC and discuss this instead. Yeah, the key point is that this is a lot of stuff to review while the end result is we just trow away most of it. I would rather just go ahead and do a few simple patches instead: 1. Use only a single instance of ttm_mem_global in ttm_memory.c and remove all "global" parameters from the functions. 2. Use only a single instance of ttm_bo_global in ttm_bo.c. 3. Remove bdev->global. The *_global_init()/*_global_release() should just increase/decrease a static reference count protected by a static lock. Regards, Christian. > > Best regards > Thomas > > >> Regards, >> Christian. >> >> Am 18.10.18 um 18:27 schrieb Thomas Zimmermann: >>> TTM provides global memory and a global BO that is shared by all >>> TTM-based drivers. The data structures are provided by struct drm_global >>> and its helpers. All TTM-based DRM drivers copy the initialization and >>> clean-up code for the global TTM state from each other; leading to code >>> duplication. >>> >>> The new structure struct ttm_global and its helpers provide a unified >>> implementation. Drivers only have to initialize it and forward the >>> contained BO global object to ttm_bo_device_init(). >>> >>> The amdgpu and radeon drivers are converted to struct ttm_global as >>> part of this patch set. All other TTM-based drivers share exactly the >>> same code patterns and can be converted in the same way. >>> >>> Future directions: I already have patches for converting the remaining >>> TTM drivers to struct ttm_global. These patches can be merged after >>> the structure has become available in upstream. struct ttm_global is >>> implemented on top of struct drm_global. The latter actually maintains >>> TTM state instead of DRM state. Once all drivers have been converted, >>> the code for struct drm_global can be merged into struct ttm_global. >>> >>> (Resending this patch set, as I forgot to CC the mailing lists as first.) >>> >>> Thomas Zimmermann (3): >>> drm/ttm: Provide struct ttm_global for referencing TTM global state >>> drm/amdgpu: Replace TTM initialization/release with ttm_global >>> drm/radeon: Replace TTM initialization/release with ttm_global >>> >>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 63 ++-------------- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h | 4 +- >>> drivers/gpu/drm/radeon/radeon.h | 4 +- >>> drivers/gpu/drm/radeon/radeon_ttm.c | 40 ++-------- >>> drivers/gpu/drm/ttm/Makefile | 2 +- >>> drivers/gpu/drm/ttm/ttm_global.c | 98 +++++++++++++++++++++++++ >>> include/drm/drm_global.h | 8 ++ >>> include/drm/ttm/ttm_global.h | 79 ++++++++++++++++++++ >>> 8 files changed, 200 insertions(+), 98 deletions(-) >>> create mode 100644 drivers/gpu/drm/ttm/ttm_global.c >>> create mode 100644 include/drm/ttm/ttm_global.h >>> >>> -- >>> 2.19.1 >>>
TTM provides global memory and a global BO that is shared by all TTM-based drivers. The data structures are provided by struct drm_global and its helpers. All TTM-based DRM drivers copy the initialization and clean-up code for the global TTM state from each other; leading to code duplication. The new structure struct ttm_global and its helpers provide a unified implementation. Drivers only have to initialize it and forward the contained BO global object to ttm_bo_device_init(). The amdgpu and radeon drivers are converted to struct ttm_global as part of this patch set. All other TTM-based drivers share exactly the same code patterns and can be converted in the same way. Future directions: I already have patches for converting the remaining TTM drivers to struct ttm_global. These patches can be merged after the structure has become available in upstream. struct ttm_global is implemented on top of struct drm_global. The latter actually maintains TTM state instead of DRM state. Once all drivers have been converted, the code for struct drm_global can be merged into struct ttm_global. (Resending this patch set, as I forgot to CC the mailing lists as first.) Thomas Zimmermann (3): drm/ttm: Provide struct ttm_global for referencing TTM global state drm/amdgpu: Replace TTM initialization/release with ttm_global drm/radeon: Replace TTM initialization/release with ttm_global drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 63 ++-------------- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h | 4 +- drivers/gpu/drm/radeon/radeon.h | 4 +- drivers/gpu/drm/radeon/radeon_ttm.c | 40 ++-------- drivers/gpu/drm/ttm/Makefile | 2 +- drivers/gpu/drm/ttm/ttm_global.c | 98 +++++++++++++++++++++++++ include/drm/drm_global.h | 8 ++ include/drm/ttm/ttm_global.h | 79 ++++++++++++++++++++ 8 files changed, 200 insertions(+), 98 deletions(-) create mode 100644 drivers/gpu/drm/ttm/ttm_global.c create mode 100644 include/drm/ttm/ttm_global.h -- 2.19.1