| Message ID | 20190808134417.10610-1-kraxel@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm: add gem ttm helpers, rework mmap workflow. | expand |
On Thu, Aug 8, 2019 at 7:44 AM Gerd Hoffmann <kraxel@redhat.com> wrote: > > > > Gerd Hoffmann (17): > drm/ttm: turn ttm_bo_device.vma_manager into a pointer > drm/ttm: add gem_ttm_bo_device_init() > drm/vram: switch vram helpers to the new gem_ttm_bo_device_init() > drm/qxl: switch qxl to the new gem_ttm_bo_device_init() > drm: add mmap() to drm_gem_object_funcs > drm/shmem: switch shmem helper to drm_gem_object_funcs->mmap > drm/shmem: drop DEFINE_DRM_GEM_SHMEM_FOPS > drm/ttm: factor out ttm_bo_mmap_vma_setup > drm/ttm: add drm_gem_ttm_mmap() > drm/vram: switch vram helper to drm_gem_object_funcs->mmap codepath > drm/vram: drop verify_access > drm: drop DRM_VRAM_MM_FILE_OPERATIONS > drm/qxl: use drm_gem_object_funcs > drm/qxl: drop qxl_ttm_fault > drm/qxl: switch qxl to drm_gem_object_funcs->mmap codepath > drm/qxl: drop verify_access > drm/qxl: use DEFINE_DRM_GEM_FOPS() Perhaps instead of changing drivers to use DEFINE_DRM_GEM_FOPS, make that the default if .fops is NULL (and perhaps also if the driver sets DRIVER_GEM). That would be in line with other recent rework making various function ptrs optional. Rob
On Thu, Aug 08, 2019 at 08:39:11AM -0600, Rob Herring wrote: > On Thu, Aug 8, 2019 at 7:44 AM Gerd Hoffmann <kraxel@redhat.com> wrote: > > > > > > > > Gerd Hoffmann (17): > > drm/ttm: turn ttm_bo_device.vma_manager into a pointer > > drm/ttm: add gem_ttm_bo_device_init() > > drm/vram: switch vram helpers to the new gem_ttm_bo_device_init() > > drm/qxl: switch qxl to the new gem_ttm_bo_device_init() > > drm: add mmap() to drm_gem_object_funcs > > drm/shmem: switch shmem helper to drm_gem_object_funcs->mmap > > drm/shmem: drop DEFINE_DRM_GEM_SHMEM_FOPS > > drm/ttm: factor out ttm_bo_mmap_vma_setup > > drm/ttm: add drm_gem_ttm_mmap() > > drm/vram: switch vram helper to drm_gem_object_funcs->mmap codepath > > drm/vram: drop verify_access > > drm: drop DRM_VRAM_MM_FILE_OPERATIONS > > drm/qxl: use drm_gem_object_funcs > > drm/qxl: drop qxl_ttm_fault > > drm/qxl: switch qxl to drm_gem_object_funcs->mmap codepath > > drm/qxl: drop verify_access > > drm/qxl: use DEFINE_DRM_GEM_FOPS() > > Perhaps instead of changing drivers to use DEFINE_DRM_GEM_FOPS, make > that the default if .fops is NULL (and perhaps also if the driver sets > DRIVER_GEM). That would be in line with other recent rework making > various function ptrs optional. Not so easy for fops due to .owner = THIS_MODULE cheers, Gerd
Hi, I would have liked to get some context on the purpose of GEM TTM helpers. Is is just share-able code? From my understanding VRAM helpers _are_ GEM TTM helpers. And they where re-named to VRAM helpers, so that the naming is independent from the implementation (and vice versa). Wrt qxl, would it be possible to convert the driver over to VRAM helpers entirely? I noticed a memory region named PRIV. Could we add this to VRAM helpers? Best regards Thomas Am 08.08.19 um 15:44 schrieb Gerd Hoffmann: > > > Gerd Hoffmann (17): > drm/ttm: turn ttm_bo_device.vma_manager into a pointer > drm/ttm: add gem_ttm_bo_device_init() > drm/vram: switch vram helpers to the new gem_ttm_bo_device_init() > drm/qxl: switch qxl to the new gem_ttm_bo_device_init() > drm: add mmap() to drm_gem_object_funcs > drm/shmem: switch shmem helper to drm_gem_object_funcs->mmap > drm/shmem: drop DEFINE_DRM_GEM_SHMEM_FOPS > drm/ttm: factor out ttm_bo_mmap_vma_setup > drm/ttm: add drm_gem_ttm_mmap() > drm/vram: switch vram helper to drm_gem_object_funcs->mmap codepath > drm/vram: drop verify_access > drm: drop DRM_VRAM_MM_FILE_OPERATIONS > drm/qxl: use drm_gem_object_funcs > drm/qxl: drop qxl_ttm_fault > drm/qxl: switch qxl to drm_gem_object_funcs->mmap codepath > drm/qxl: drop verify_access > drm/qxl: use DEFINE_DRM_GEM_FOPS() > > drivers/gpu/drm/qxl/qxl_drv.h | 5 +- > drivers/gpu/drm/qxl/qxl_object.h | 5 -- > include/drm/drm_gem.h | 9 +++ > include/drm/drm_gem_shmem_helper.h | 28 +-------- > include/drm/drm_gem_ttm_helper.h | 32 ++++++++++ > include/drm/drm_gem_vram_helper.h | 9 +-- > include/drm/drm_vram_mm_helper.h | 27 --------- > include/drm/ttm/ttm_bo_api.h | 8 +++ > include/drm/ttm/ttm_bo_driver.h | 11 +++- > drivers/gpu/drm/ast/ast_drv.c | 5 +- > drivers/gpu/drm/bochs/bochs_drv.c | 5 +- > drivers/gpu/drm/cirrus/cirrus.c | 2 +- > drivers/gpu/drm/drm_gem.c | 6 ++ > drivers/gpu/drm/drm_gem_shmem_helper.c | 18 +++--- > drivers/gpu/drm/drm_gem_ttm_helper.c | 47 +++++++++++++++ > drivers/gpu/drm/drm_gem_vram_helper.c | 53 +---------------- > drivers/gpu/drm/drm_vram_mm_helper.c | 44 +------------- > .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 6 +- > drivers/gpu/drm/mgag200/mgag200_drv.c | 5 +- > drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +- > drivers/gpu/drm/panfrost/panfrost_gem.c | 1 + > drivers/gpu/drm/qxl/qxl_drv.c | 19 +----- > drivers/gpu/drm/qxl/qxl_dumb.c | 17 ------ > drivers/gpu/drm/qxl/qxl_ioctl.c | 5 +- > drivers/gpu/drm/qxl/qxl_object.c | 13 +++++ > drivers/gpu/drm/qxl/qxl_ttm.c | 58 ++----------------- > drivers/gpu/drm/ttm/ttm_bo.c | 29 +++++++--- > drivers/gpu/drm/ttm/ttm_bo_vm.c | 53 +++++++++-------- > drivers/gpu/drm/v3d/v3d_bo.c | 1 + > drivers/gpu/drm/v3d/v3d_drv.c | 2 +- > drivers/gpu/drm/vboxvideo/vbox_drv.c | 5 +- > Documentation/gpu/drm-mm.rst | 12 ++++ > drivers/gpu/drm/Kconfig | 8 +++ > drivers/gpu/drm/Makefile | 3 + > drivers/gpu/drm/qxl/Kconfig | 1 + > 35 files changed, 231 insertions(+), 323 deletions(-) > create mode 100644 include/drm/drm_gem_ttm_helper.h > create mode 100644 drivers/gpu/drm/drm_gem_ttm_helper.c >
On Mon, Aug 26, 2019 at 10:47 AM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Hi, > > I would have liked to get some context on the purpose of GEM TTM > helpers. Is is just share-able code? > > From my understanding VRAM helpers _are_ GEM TTM helpers. And they where > re-named to VRAM helpers, so that the naming is independent from the > implementation (and vice versa). The point of the vram helpers was to have something to manage vram for dumb display-only drivers. It's prep work for Thomas Zimmermann's plan to port a pile of fbdev drivers over. So fairly intentionally limit in the use-cases it supports to keep it simple. Kinda similar to how the simple display pipe helper is designed on the kms side of things. > Wrt qxl, would it be possible to convert the driver over to VRAM helpers > entirely? I noticed a memory region named PRIV. Could we add this to > VRAM helpers? For both simple display pipe and vram helpers I'd say if your use-case goes beyond simple dumb display-only driver, it's probably better to have something more flexible. Also this patch series also adjust vram helpers, and I think it has a slightly different goal: Just aligning mmap paths a bit more between ttm and not-ttm based drivers. That's also what motivated my lockdep series, but from a locking rules instead of from a code-sharing point of view. Seems like a good goal, details might need adjustment. -Daniel > > Best regards > Thomas > > > Am 08.08.19 um 15:44 schrieb Gerd Hoffmann: > > > > > > Gerd Hoffmann (17): > > drm/ttm: turn ttm_bo_device.vma_manager into a pointer > > drm/ttm: add gem_ttm_bo_device_init() > > drm/vram: switch vram helpers to the new gem_ttm_bo_device_init() > > drm/qxl: switch qxl to the new gem_ttm_bo_device_init() > > drm: add mmap() to drm_gem_object_funcs > > drm/shmem: switch shmem helper to drm_gem_object_funcs->mmap > > drm/shmem: drop DEFINE_DRM_GEM_SHMEM_FOPS > > drm/ttm: factor out ttm_bo_mmap_vma_setup > > drm/ttm: add drm_gem_ttm_mmap() > > drm/vram: switch vram helper to drm_gem_object_funcs->mmap codepath > > drm/vram: drop verify_access > > drm: drop DRM_VRAM_MM_FILE_OPERATIONS > > drm/qxl: use drm_gem_object_funcs > > drm/qxl: drop qxl_ttm_fault > > drm/qxl: switch qxl to drm_gem_object_funcs->mmap codepath > > drm/qxl: drop verify_access > > drm/qxl: use DEFINE_DRM_GEM_FOPS() > > > > drivers/gpu/drm/qxl/qxl_drv.h | 5 +- > > drivers/gpu/drm/qxl/qxl_object.h | 5 -- > > include/drm/drm_gem.h | 9 +++ > > include/drm/drm_gem_shmem_helper.h | 28 +-------- > > include/drm/drm_gem_ttm_helper.h | 32 ++++++++++ > > include/drm/drm_gem_vram_helper.h | 9 +-- > > include/drm/drm_vram_mm_helper.h | 27 --------- > > include/drm/ttm/ttm_bo_api.h | 8 +++ > > include/drm/ttm/ttm_bo_driver.h | 11 +++- > > drivers/gpu/drm/ast/ast_drv.c | 5 +- > > drivers/gpu/drm/bochs/bochs_drv.c | 5 +- > > drivers/gpu/drm/cirrus/cirrus.c | 2 +- > > drivers/gpu/drm/drm_gem.c | 6 ++ > > drivers/gpu/drm/drm_gem_shmem_helper.c | 18 +++--- > > drivers/gpu/drm/drm_gem_ttm_helper.c | 47 +++++++++++++++ > > drivers/gpu/drm/drm_gem_vram_helper.c | 53 +---------------- > > drivers/gpu/drm/drm_vram_mm_helper.c | 44 +------------- > > .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 6 +- > > drivers/gpu/drm/mgag200/mgag200_drv.c | 5 +- > > drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +- > > drivers/gpu/drm/panfrost/panfrost_gem.c | 1 + > > drivers/gpu/drm/qxl/qxl_drv.c | 19 +----- > > drivers/gpu/drm/qxl/qxl_dumb.c | 17 ------ > > drivers/gpu/drm/qxl/qxl_ioctl.c | 5 +- > > drivers/gpu/drm/qxl/qxl_object.c | 13 +++++ > > drivers/gpu/drm/qxl/qxl_ttm.c | 58 ++----------------- > > drivers/gpu/drm/ttm/ttm_bo.c | 29 +++++++--- > > drivers/gpu/drm/ttm/ttm_bo_vm.c | 53 +++++++++-------- > > drivers/gpu/drm/v3d/v3d_bo.c | 1 + > > drivers/gpu/drm/v3d/v3d_drv.c | 2 +- > > drivers/gpu/drm/vboxvideo/vbox_drv.c | 5 +- > > Documentation/gpu/drm-mm.rst | 12 ++++ > > drivers/gpu/drm/Kconfig | 8 +++ > > drivers/gpu/drm/Makefile | 3 + > > drivers/gpu/drm/qxl/Kconfig | 1 + > > 35 files changed, 231 insertions(+), 323 deletions(-) > > create mode 100644 include/drm/drm_gem_ttm_helper.h > > create mode 100644 drivers/gpu/drm/drm_gem_ttm_helper.c > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah > HRB 21284 (AG Nürnberg) > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, Aug 26, 2019 at 10:47:07AM +0200, Thomas Zimmermann wrote: > Hi, > > I would have liked to get some context on the purpose of GEM TTM > helpers. Is is just share-able code? Yes. Shareable code for drivers which use both ttm and gem (all except vmgfx). > From my understanding VRAM helpers _are_ GEM TTM helpers. And they where > re-named to VRAM helpers, so that the naming is independent from the > implementation (and vice versa). I think vram helpers are for old/simple devices which basically support dumb gem objects stored in vram (device memory typically exposed as pci bar). Using ttm underneath is largely an implementation detail for the users of the vram helpers, they don't need to know. > Wrt qxl, would it be possible to convert the driver over to VRAM helpers > entirely? I noticed a memory region named PRIV. Could we add this to > VRAM helpers? PRIV is vram too, qxl has two pci bars with memory. Historical reasons. There are rules which pci bar can store which kind of objects. I don't think it makes sense to move such device-specific things into vram helpers. Also qxl is complex enough that you can hardly hide ttm behind vram helpers. cheers, Gerd
Hi, > Also this patch series also adjust vram helpers, and I think it has a > slightly different goal: Just aligning mmap paths a bit more between > ttm and not-ttm based drivers. Not just ttm/not-ttm. gem_driver->fops->mmap is the only fops callback where we can't use a generic gem callback today. This series tries to fix that with the new drm_gem_object_funcs->mmap hook, so the mmap() code path isn't the odd one which works different than every other drm_gem_object operation. > Seems like a good goal, details might need adjustment. Which details? cheers, Gerd
On Thu, Aug 08, 2019 at 03:44:00PM +0200, Gerd Hoffmann wrote: > > > Gerd Hoffmann (17): > drm/ttm: turn ttm_bo_device.vma_manager into a pointer > drm/ttm: add gem_ttm_bo_device_init() > drm/vram: switch vram helpers to the new gem_ttm_bo_device_init() > drm/qxl: switch qxl to the new gem_ttm_bo_device_init() > drm: add mmap() to drm_gem_object_funcs > drm/shmem: switch shmem helper to drm_gem_object_funcs->mmap > drm/shmem: drop DEFINE_DRM_GEM_SHMEM_FOPS > drm/ttm: factor out ttm_bo_mmap_vma_setup > drm/ttm: add drm_gem_ttm_mmap() > drm/vram: switch vram helper to drm_gem_object_funcs->mmap codepath > drm/vram: drop verify_access > drm: drop DRM_VRAM_MM_FILE_OPERATIONS > drm/qxl: use drm_gem_object_funcs > drm/qxl: drop qxl_ttm_fault > drm/qxl: switch qxl to drm_gem_object_funcs->mmap codepath > drm/qxl: drop verify_access > drm/qxl: use DEFINE_DRM_GEM_FOPS() I like. Dropped 2 thoughts for a bit more polish/usefulness for all this. Unfortunately I'm kinda burried, so would be better if you find someone else for detailed review and all that. -Daniel > > drivers/gpu/drm/qxl/qxl_drv.h | 5 +- > drivers/gpu/drm/qxl/qxl_object.h | 5 -- > include/drm/drm_gem.h | 9 +++ > include/drm/drm_gem_shmem_helper.h | 28 +-------- > include/drm/drm_gem_ttm_helper.h | 32 ++++++++++ > include/drm/drm_gem_vram_helper.h | 9 +-- > include/drm/drm_vram_mm_helper.h | 27 --------- > include/drm/ttm/ttm_bo_api.h | 8 +++ > include/drm/ttm/ttm_bo_driver.h | 11 +++- > drivers/gpu/drm/ast/ast_drv.c | 5 +- > drivers/gpu/drm/bochs/bochs_drv.c | 5 +- > drivers/gpu/drm/cirrus/cirrus.c | 2 +- > drivers/gpu/drm/drm_gem.c | 6 ++ > drivers/gpu/drm/drm_gem_shmem_helper.c | 18 +++--- > drivers/gpu/drm/drm_gem_ttm_helper.c | 47 +++++++++++++++ > drivers/gpu/drm/drm_gem_vram_helper.c | 53 +---------------- > drivers/gpu/drm/drm_vram_mm_helper.c | 44 +------------- > .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 6 +- > drivers/gpu/drm/mgag200/mgag200_drv.c | 5 +- > drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +- > drivers/gpu/drm/panfrost/panfrost_gem.c | 1 + > drivers/gpu/drm/qxl/qxl_drv.c | 19 +----- > drivers/gpu/drm/qxl/qxl_dumb.c | 17 ------ > drivers/gpu/drm/qxl/qxl_ioctl.c | 5 +- > drivers/gpu/drm/qxl/qxl_object.c | 13 +++++ > drivers/gpu/drm/qxl/qxl_ttm.c | 58 ++----------------- > drivers/gpu/drm/ttm/ttm_bo.c | 29 +++++++--- > drivers/gpu/drm/ttm/ttm_bo_vm.c | 53 +++++++++-------- > drivers/gpu/drm/v3d/v3d_bo.c | 1 + > drivers/gpu/drm/v3d/v3d_drv.c | 2 +- > drivers/gpu/drm/vboxvideo/vbox_drv.c | 5 +- > Documentation/gpu/drm-mm.rst | 12 ++++ > drivers/gpu/drm/Kconfig | 8 +++ > drivers/gpu/drm/Makefile | 3 + > drivers/gpu/drm/qxl/Kconfig | 1 + > 35 files changed, 231 insertions(+), 323 deletions(-) > create mode 100644 include/drm/drm_gem_ttm_helper.h > create mode 100644 drivers/gpu/drm/drm_gem_ttm_helper.c > > -- > 2.18.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Aug 27, 2019 at 07:13:41AM +0200, Gerd Hoffmann wrote: > Hi, > > > Also this patch series also adjust vram helpers, and I think it has a > > slightly different goal: Just aligning mmap paths a bit more between > > ttm and not-ttm based drivers. > > Not just ttm/not-ttm. gem_driver->fops->mmap is the only fops callback > where we can't use a generic gem callback today. This series tries to > fix that with the new drm_gem_object_funcs->mmap hook, so the mmap() > code path isn't the odd one which works different than every other > drm_gem_object operation. > > > Seems like a good goal, details might need adjustment. > > Which details? Just a general comment that the tricky parts are always in the details ... -Daniel