| Message ID | 20190621115755.8481-1-kraxel@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/ttm: make ttm bo a gem bo subclass | expand |
One little comment on patch #8: > + /* base.vma_node */ Is that really useful? I would just drop it. Apart from that Patches #1, #2, #4, #5, #7 - #12, #14, #15, #18 are Reviewed-by: Christian König <christian.koenig@amd.com>. Patches #3, #6, #13, #16, #17 are Acked-by: Christian König <christian.koenig@amd.com>. You should try to get an rb for the respective maintainer for patches #3 and #6. When that's done I think we can merge it. Any preference for the tree where this goes upstream? If not I suggest to use drm-misc-next. Thanks for the nice cleanup, Christian. Am 21.06.19 um 13:57 schrieb Gerd Hoffmann: > v2: > - build fixes. > - also drop ttm_buffer_object->resv > > Gerd Hoffmann (18): > drm/ttm: add gem base object > drm/vram: use embedded gem object > drm/qxl: use embedded gem object > drm/radeon: use embedded gem object > drm/amdgpu: use embedded gem object > drm/nouveau: use embedded gem object > drm/ttm: use gem reservation object > drm/ttm: use gem vma_node > drm/vram: drop drm_gem_vram_driver_gem_prime_mmap > drm/ttm: set both resv and base.resv pointers > drm/ttm: switch ttm core from bo->resv to bo->base.resv > drm/radeon: switch driver from bo->resv to bo->base.resv > drm/vmwgfx: switch driver from bo->resv to bo->base.resv > drm/amdgpu: switch driver from bo->resv to bo->base.resv > drm/nouveau: switch driver from bo->resv to bo->base.resv > drm/qxl: switch driver from bo->resv to bo->base.resv > drm/virtio: switch driver from bo->resv to bo->base.resv > drm/ttm: drop ttm_buffer_object->resv > > drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 3 +- > drivers/gpu/drm/nouveau/nouveau_bo.h | 5 - > drivers/gpu/drm/nouveau/nouveau_gem.h | 2 +- > drivers/gpu/drm/qxl/qxl_drv.h | 6 +- > drivers/gpu/drm/qxl/qxl_object.h | 6 +- > drivers/gpu/drm/radeon/radeon.h | 3 +- > drivers/gpu/drm/radeon/radeon_object.h | 2 +- > drivers/gpu/drm/virtio/virtgpu_drv.h | 2 +- > include/drm/drm_gem_vram_helper.h | 7 +- > include/drm/ttm/ttm_bo_api.h | 25 +++- > include/drm/ttm/ttm_bo_driver.h | 12 +- > .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 6 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 6 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 6 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 14 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 28 ++-- > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 8 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 4 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 30 ++-- > drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 2 +- > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +- > drivers/gpu/drm/ast/ast_main.c | 2 +- > drivers/gpu/drm/drm_gem_vram_helper.c | 36 ++--- > drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 2 +- > drivers/gpu/drm/mgag200/mgag200_main.c | 2 +- > drivers/gpu/drm/nouveau/dispnv50/wndw.c | 2 +- > drivers/gpu/drm/nouveau/nouveau_abi16.c | 4 +- > drivers/gpu/drm/nouveau/nouveau_bo.c | 8 +- > drivers/gpu/drm/nouveau/nouveau_display.c | 10 +- > drivers/gpu/drm/nouveau/nouveau_fence.c | 2 +- > drivers/gpu/drm/nouveau/nouveau_gem.c | 19 +-- > drivers/gpu/drm/nouveau/nouveau_prime.c | 6 +- > drivers/gpu/drm/qxl/qxl_cmd.c | 4 +- > drivers/gpu/drm/qxl/qxl_debugfs.c | 4 +- > drivers/gpu/drm/qxl/qxl_display.c | 8 +- > drivers/gpu/drm/qxl/qxl_gem.c | 2 +- > drivers/gpu/drm/qxl/qxl_object.c | 20 +-- > drivers/gpu/drm/qxl/qxl_release.c | 8 +- > drivers/gpu/drm/qxl/qxl_ttm.c | 4 +- > drivers/gpu/drm/radeon/radeon_benchmark.c | 4 +- > drivers/gpu/drm/radeon/radeon_cs.c | 4 +- > drivers/gpu/drm/radeon/radeon_display.c | 6 +- > drivers/gpu/drm/radeon/radeon_gem.c | 8 +- > drivers/gpu/drm/radeon/radeon_mn.c | 2 +- > drivers/gpu/drm/radeon/radeon_object.c | 22 +-- > drivers/gpu/drm/radeon/radeon_prime.c | 4 +- > drivers/gpu/drm/radeon/radeon_test.c | 8 +- > drivers/gpu/drm/radeon/radeon_ttm.c | 4 +- > drivers/gpu/drm/radeon/radeon_uvd.c | 2 +- > drivers/gpu/drm/radeon/radeon_vm.c | 6 +- > drivers/gpu/drm/ttm/ttm_bo.c | 136 +++++++++--------- > drivers/gpu/drm/ttm/ttm_bo_util.c | 18 +-- > drivers/gpu/drm/ttm/ttm_bo_vm.c | 15 +- > drivers/gpu/drm/ttm/ttm_execbuf_util.c | 20 +-- > drivers/gpu/drm/ttm/ttm_tt.c | 2 +- > drivers/gpu/drm/vboxvideo/vbox_main.c | 2 +- > drivers/gpu/drm/virtio/virtgpu_ioctl.c | 4 +- > drivers/gpu/drm/virtio/virtgpu_plane.c | 2 +- > drivers/gpu/drm/virtio/virtgpu_prime.c | 3 - > drivers/gpu/drm/vmwgfx/vmwgfx_blit.c | 4 +- > drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 12 +- > drivers/gpu/drm/vmwgfx/vmwgfx_cotable.c | 4 +- > drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 6 +- > drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 4 +- > 68 files changed, 311 insertions(+), 321 deletions(-) >
On 6/21/19 1:57 PM, Gerd Hoffmann wrote: Aargh. Please don't do this. Multiple reasons: 1) I think It's bad to dump all buffer object functionality we can possibly think of in a single struct and force that on all (well at least most) users. It's better to isolate functionality in structs, have utility functions for those and let the drivers derive their buffer objects from whatever functionality they actually need. 2) vmwgfx is not using gem and we don't want to carry that extra payload in the buffer object. 3) TTM historically hasn't been using the various drm layers except for later when common helpers have been used, (like the vma manager and the cache utilities). It's desirable to keep that layer distinction. (which is really what I'm saying in 1.) Now if more and more functionality that originated in TTM is moving into GEM we need to find a better way to do that without duplicating functionality. I suggest adding pointers in the TTM structs and defaulting those pointers to the member in the TTM struct. Optionally to to the member in the GEM struct. If we need to migrate those members out of the TTM struct, vmwgfx would have to provide them in its own buffer class. NAK from the vmwgfx side. Thanks, Thoams
On Fri, Jun 21, 2019 at 05:12:19PM +0200, Thomas Hellström (VMware) wrote: > > > On 6/21/19 1:57 PM, Gerd Hoffmann wrote: > > Aargh. Please don't do this. Multiple reasons: > > 1) I think It's bad to dump all buffer object functionality we can possibly > think of in a single struct and force that on all (well at least most) > users. It's better to isolate functionality in structs, have utility > functions for those and let the drivers derive their buffer objects from > whatever functionality they actually need. > 2) vmwgfx is not using gem and we don't want to carry that extra payload in > the buffer object. > 3) TTM historically hasn't been using the various drm layers except for > later when common helpers have been used, (like the vma manager and the > cache utilities). It's desirable to keep that layer distinction. (which is > really what I'm saying in 1.) > > Now if more and more functionality that originated in TTM is moving into GEM > we need to find a better way to do that without duplicating functionality. I > suggest adding pointers in the TTM structs and defaulting those pointers to > the member in the TTM struct. Optionally to to the member in the GEM struct. > If we need to migrate those members out of the TTM struct, vmwgfx would have > to provide them in its own buffer class. > > NAK from the vmwgfx side. It's 59 DRIVER_GEM vs 1 which is not. I think the verdict is clear what the reasonable thing to do is here, and this will allow us to substantially improve code and concept sharing across drm drivers. 10 years ago it was indeed not clear whether everyone doing the same is a bright idea, but that's no more. If you want I guess you can keep a private copy of ttm in vmwgfx, but not sure that's really worth it long-term. -Daniel
On Fri, Jun 21, 2019 at 03:48:24PM +0200, Christian König wrote: > One little comment on patch #8: > > + /* base.vma_node */ > Is that really useful? I would just drop it. > > Apart from that Patches #1, #2, #4, #5, #7 - #12, #14, #15, #18 are > Reviewed-by: Christian König <christian.koenig@amd.com>. > > Patches #3, #6, #13, #16, #17 are Acked-by: Christian König > <christian.koenig@amd.com>. > > You should try to get an rb for the respective maintainer for patches #3 and > #6. > > When that's done I think we can merge it. Any preference for the tree where > this goes upstream? If not I suggest to use drm-misc-next. I think -misc would be good, since this will conflict with my pile of cleanups in dma-buf and especially simplifying where we find the resv_obj pointer. So need to figure out which one goes in first. -Daniel > > Thanks for the nice cleanup, > Christian. > > Am 21.06.19 um 13:57 schrieb Gerd Hoffmann: > > v2: > > - build fixes. > > - also drop ttm_buffer_object->resv > > > > Gerd Hoffmann (18): > > drm/ttm: add gem base object > > drm/vram: use embedded gem object > > drm/qxl: use embedded gem object > > drm/radeon: use embedded gem object > > drm/amdgpu: use embedded gem object > > drm/nouveau: use embedded gem object > > drm/ttm: use gem reservation object > > drm/ttm: use gem vma_node > > drm/vram: drop drm_gem_vram_driver_gem_prime_mmap > > drm/ttm: set both resv and base.resv pointers > > drm/ttm: switch ttm core from bo->resv to bo->base.resv > > drm/radeon: switch driver from bo->resv to bo->base.resv > > drm/vmwgfx: switch driver from bo->resv to bo->base.resv > > drm/amdgpu: switch driver from bo->resv to bo->base.resv > > drm/nouveau: switch driver from bo->resv to bo->base.resv > > drm/qxl: switch driver from bo->resv to bo->base.resv > > drm/virtio: switch driver from bo->resv to bo->base.resv > > drm/ttm: drop ttm_buffer_object->resv > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h | 2 +- > > drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 3 +- > > drivers/gpu/drm/nouveau/nouveau_bo.h | 5 - > > drivers/gpu/drm/nouveau/nouveau_gem.h | 2 +- > > drivers/gpu/drm/qxl/qxl_drv.h | 6 +- > > drivers/gpu/drm/qxl/qxl_object.h | 6 +- > > drivers/gpu/drm/radeon/radeon.h | 3 +- > > drivers/gpu/drm/radeon/radeon_object.h | 2 +- > > drivers/gpu/drm/virtio/virtgpu_drv.h | 2 +- > > include/drm/drm_gem_vram_helper.h | 7 +- > > include/drm/ttm/ttm_bo_api.h | 25 +++- > > include/drm/ttm/ttm_bo_driver.h | 12 +- > > .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 6 +- > > drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 6 +- > > drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 2 +- > > drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 6 +- > > drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 14 +- > > drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 +- > > drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c | 2 +- > > drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 28 ++-- > > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 8 +- > > drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 4 +- > > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 30 ++-- > > drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 2 +- > > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +- > > drivers/gpu/drm/ast/ast_main.c | 2 +- > > drivers/gpu/drm/drm_gem_vram_helper.c | 36 ++--- > > drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 2 +- > > drivers/gpu/drm/mgag200/mgag200_main.c | 2 +- > > drivers/gpu/drm/nouveau/dispnv50/wndw.c | 2 +- > > drivers/gpu/drm/nouveau/nouveau_abi16.c | 4 +- > > drivers/gpu/drm/nouveau/nouveau_bo.c | 8 +- > > drivers/gpu/drm/nouveau/nouveau_display.c | 10 +- > > drivers/gpu/drm/nouveau/nouveau_fence.c | 2 +- > > drivers/gpu/drm/nouveau/nouveau_gem.c | 19 +-- > > drivers/gpu/drm/nouveau/nouveau_prime.c | 6 +- > > drivers/gpu/drm/qxl/qxl_cmd.c | 4 +- > > drivers/gpu/drm/qxl/qxl_debugfs.c | 4 +- > > drivers/gpu/drm/qxl/qxl_display.c | 8 +- > > drivers/gpu/drm/qxl/qxl_gem.c | 2 +- > > drivers/gpu/drm/qxl/qxl_object.c | 20 +-- > > drivers/gpu/drm/qxl/qxl_release.c | 8 +- > > drivers/gpu/drm/qxl/qxl_ttm.c | 4 +- > > drivers/gpu/drm/radeon/radeon_benchmark.c | 4 +- > > drivers/gpu/drm/radeon/radeon_cs.c | 4 +- > > drivers/gpu/drm/radeon/radeon_display.c | 6 +- > > drivers/gpu/drm/radeon/radeon_gem.c | 8 +- > > drivers/gpu/drm/radeon/radeon_mn.c | 2 +- > > drivers/gpu/drm/radeon/radeon_object.c | 22 +-- > > drivers/gpu/drm/radeon/radeon_prime.c | 4 +- > > drivers/gpu/drm/radeon/radeon_test.c | 8 +- > > drivers/gpu/drm/radeon/radeon_ttm.c | 4 +- > > drivers/gpu/drm/radeon/radeon_uvd.c | 2 +- > > drivers/gpu/drm/radeon/radeon_vm.c | 6 +- > > drivers/gpu/drm/ttm/ttm_bo.c | 136 +++++++++--------- > > drivers/gpu/drm/ttm/ttm_bo_util.c | 18 +-- > > drivers/gpu/drm/ttm/ttm_bo_vm.c | 15 +- > > drivers/gpu/drm/ttm/ttm_execbuf_util.c | 20 +-- > > drivers/gpu/drm/ttm/ttm_tt.c | 2 +- > > drivers/gpu/drm/vboxvideo/vbox_main.c | 2 +- > > drivers/gpu/drm/virtio/virtgpu_ioctl.c | 4 +- > > drivers/gpu/drm/virtio/virtgpu_plane.c | 2 +- > > drivers/gpu/drm/virtio/virtgpu_prime.c | 3 - > > drivers/gpu/drm/vmwgfx/vmwgfx_blit.c | 4 +- > > drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 12 +- > > drivers/gpu/drm/vmwgfx/vmwgfx_cotable.c | 4 +- > > drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 6 +- > > drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 4 +- > > 68 files changed, 311 insertions(+), 321 deletions(-) > > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi, Daniel, On 6/21/19 5:57 PM, Daniel Vetter wrote: > On Fri, Jun 21, 2019 at 05:12:19PM +0200, Thomas Hellström (VMware) wrote: >> >> On 6/21/19 1:57 PM, Gerd Hoffmann wrote: >> >> Aargh. Please don't do this. Multiple reasons: >> >> 1) I think It's bad to dump all buffer object functionality we can possibly >> think of in a single struct and force that on all (well at least most) >> users. It's better to isolate functionality in structs, have utility >> functions for those and let the drivers derive their buffer objects from >> whatever functionality they actually need. >> 2) vmwgfx is not using gem and we don't want to carry that extra payload in >> the buffer object. >> 3) TTM historically hasn't been using the various drm layers except for >> later when common helpers have been used, (like the vma manager and the >> cache utilities). It's desirable to keep that layer distinction. (which is >> really what I'm saying in 1.) >> >> Now if more and more functionality that originated in TTM is moving into GEM >> we need to find a better way to do that without duplicating functionality. I >> suggest adding pointers in the TTM structs and defaulting those pointers to >> the member in the TTM struct. Optionally to to the member in the GEM struct. >> If we need to migrate those members out of the TTM struct, vmwgfx would have >> to provide them in its own buffer class. >> >> NAK from the vmwgfx side. > It's 59 DRIVER_GEM vs 1 which is not. I think the verdict is clear what > the reasonable thing to do is here, and this will allow us to > substantially improve code and concept sharing across drm drivers. > > 10 years ago it was indeed not clear whether everyone doing the same is a > bright idea, but that's no more. If you want I guess you can keep a > private copy of ttm in vmwgfx, but not sure that's really worth it > long-term. > -Daniel It's not a question about whether GEM or TTM, or even the number of drivers using one or the other. (GEM would actually be a good choice for the latter vmwgfx device versions). But this is going against all recent effort to make different parts of drm functionality recently self-contained. Just stop and think for a while what would happen if someone would suggest doing the opposite: making a gem object a derived class of a TTM object, arguing that a lot of GEM drivers are using TTM as a backend. There would probably be a lot of people claiming "we don't want to unnecesarily carry that stuff". That's because that would also be a poor design. What I'm suggesting is, build that improved code and concept sharing around struct gem_ttm_object { struct gem_object; struct ttm_object; }; And lets work toghether to eliminate what's duplicated. The vmwgfx driver is doing what it does mostly because all buffer objects do not need to be user-space visible, and do not need to be mapped by user-space. And there are other types of objects that DO need to be user-space visible, and that do need to be shared by processes. Hence user-space visibility is something that should be abstracted and made available to those objects. Not lumped together with all other potential buffer object functionality. /Thomas
Hi Thomas, On Sat, Jun 22, 2019 at 12:52 AM Thomas Hellstrom <thomas@shipmail.org> wrote: > On 6/21/19 5:57 PM, Daniel Vetter wrote: > > On Fri, Jun 21, 2019 at 05:12:19PM +0200, Thomas Hellström (VMware) wrote: > >> > >> On 6/21/19 1:57 PM, Gerd Hoffmann wrote: > >> > >> Aargh. Please don't do this. Multiple reasons: > >> > >> 1) I think It's bad to dump all buffer object functionality we can possibly > >> think of in a single struct and force that on all (well at least most) > >> users. It's better to isolate functionality in structs, have utility > >> functions for those and let the drivers derive their buffer objects from > >> whatever functionality they actually need. > >> 2) vmwgfx is not using gem and we don't want to carry that extra payload in > >> the buffer object. > >> 3) TTM historically hasn't been using the various drm layers except for > >> later when common helpers have been used, (like the vma manager and the > >> cache utilities). It's desirable to keep that layer distinction. (which is > >> really what I'm saying in 1.) > >> > >> Now if more and more functionality that originated in TTM is moving into GEM > >> we need to find a better way to do that without duplicating functionality. I > >> suggest adding pointers in the TTM structs and defaulting those pointers to > >> the member in the TTM struct. Optionally to to the member in the GEM struct. > >> If we need to migrate those members out of the TTM struct, vmwgfx would have > >> to provide them in its own buffer class. > >> > >> NAK from the vmwgfx side. > > It's 59 DRIVER_GEM vs 1 which is not. I think the verdict is clear what > > the reasonable thing to do is here, and this will allow us to > > substantially improve code and concept sharing across drm drivers. > > > > 10 years ago it was indeed not clear whether everyone doing the same is a > > bright idea, but that's no more. If you want I guess you can keep a > > private copy of ttm in vmwgfx, but not sure that's really worth it > > long-term. > > -Daniel > > It's not a question about whether GEM or TTM, or even the number of > drivers using one or the other. (GEM would actually be a good choice for > the latter vmwgfx device versions). But this is going against all recent > effort to make different parts of drm functionality recently self-contained. > > Just stop and think for a while what would happen if someone would > suggest doing the opposite: making a gem object a derived class of a TTM > object, arguing that a lot of GEM drivers are using TTM as a backend. > There would probably be a lot of people claiming "we don't want to > unnecesarily carry that stuff". That's because that would also be a poor > design. That case is a bit a different case. We have - 5 ttm+gem drivers, recently refactored into vram helpers (but still ttm underneath) - 5 ttm+gem drivers, using ttm directly - 1 ttm driver, no gem - 48 other gem drivers with no vram support - 1 gem driver which will gain vram support shortly, with or without ttm still not clear 11:48 is not even close to 59:1 imo. And I think even if Thomas Zimmermann and others get really busy porting old discrete fbdev drivers to kms, that ratio won't change much since we're also gaining new soc drm drivers at a steady rate. Also I wouldn't mind if we e.g. stuff a struct list_head lru; into drm_gem_buffer_object, that's probably useful for many cases (not the pure display drivers, but they tend to have so few bo it really wont matter even if we add a few kb of cruft). > What I'm suggesting is, build that improved code and concept sharing around > > struct gem_ttm_object { > struct gem_object; > struct ttm_object; > }; I guess technically this would work too. Bit more churn (maybe substantially more, I haven't looked tbh) to roll this out for all the ttm drivers using gem. > And lets work toghether to eliminate what's duplicated. How would you share the bo.resv pointer with the above design? With embedding ttm can use the gem one, and we drop a bunch of code (and for all the ttm+gem drivers, one pointer they don't need twice). With the side-by-side, which is the design all gem+ttm drivers used the past few years, we still need that duplication. Same for the vma node thing, which is also duplicated. And that's just the things Gerd did in this initial patch series. I think Christian König has some ideas around ttm_bo_(un)reserve, and I just sent out a patch series to streamline how we handle the reservation_object pointer for prime import/export. > The vmwgfx driver is doing what it does mostly because all buffer > objects do not need to be user-space visible, and do not need to be > mapped by user-space. And there are other types of objects that DO need > to be user-space visible, and that do need to be shared by processes. > Hence user-space visibility is something that should be abstracted and > made available to those objects. Not lumped together with all other > potential buffer object functionality. I'd still go with pragmatic choice of "this here is typed & reviewed, and a bunch of people are enthusastistic about using this to drive further refactoring in this area". Whatever the reasons, but since a few months the ttm refactor train seems to have gained massive momentum. So if you want to change the direction of this train, I think you need to get typing and show that your solution is at least as effective at faciliting all the things people want to do with code sharing across drm drivers. Cheers, Daniel
Hi, Daniel, On 6/22/19 11:18 AM, Daniel Vetter wrote: > Hi Thomas, > > On Sat, Jun 22, 2019 at 12:52 AM Thomas Hellstrom<thomas@shipmail.org> wrote: >> On 6/21/19 5:57 PM, Daniel Vetter wrote: >>> On Fri, Jun 21, 2019 at 05:12:19PM +0200, Thomas Hellström (VMware) wrote: >>>> On 6/21/19 1:57 PM, Gerd Hoffmann wrote: >>>> >>>> Aargh. Please don't do this. Multiple reasons: >>>> >>>> 1) I think It's bad to dump all buffer object functionality we can possibly >>>> think of in a single struct and force that on all (well at least most) >>>> users. It's better to isolate functionality in structs, have utility >>>> functions for those and let the drivers derive their buffer objects from >>>> whatever functionality they actually need. >>>> 2) vmwgfx is not using gem and we don't want to carry that extra payload in >>>> the buffer object. >>>> 3) TTM historically hasn't been using the various drm layers except for >>>> later when common helpers have been used, (like the vma manager and the >>>> cache utilities). It's desirable to keep that layer distinction. (which is >>>> really what I'm saying in 1.) >>>> >>>> Now if more and more functionality that originated in TTM is moving into GEM >>>> we need to find a better way to do that without duplicating functionality. I >>>> suggest adding pointers in the TTM structs and defaulting those pointers to >>>> the member in the TTM struct. Optionally to to the member in the GEM struct. >>>> If we need to migrate those members out of the TTM struct, vmwgfx would have >>>> to provide them in its own buffer class. >>>> >>>> NAK from the vmwgfx side. >>> It's 59 DRIVER_GEM vs 1 which is not. I think the verdict is clear what >>> the reasonable thing to do is here, and this will allow us to >>> substantially improve code and concept sharing across drm drivers. >>> >>> 10 years ago it was indeed not clear whether everyone doing the same is a >>> bright idea, but that's no more. If you want I guess you can keep a >>> private copy of ttm in vmwgfx, but not sure that's really worth it >>> long-term. >>> -Daniel >> It's not a question about whether GEM or TTM, or even the number of >> drivers using one or the other. (GEM would actually be a good choice for >> the latter vmwgfx device versions). But this is going against all recent >> effort to make different parts of drm functionality recently self-contained. >> >> Just stop and think for a while what would happen if someone would >> suggest doing the opposite: making a gem object a derived class of a TTM >> object, arguing that a lot of GEM drivers are using TTM as a backend. >> There would probably be a lot of people claiming "we don't want to >> unnecesarily carry that stuff". That's because that would also be a poor >> design. > That case is a bit a different case. We have > - 5 ttm+gem drivers, recently refactored into vram helpers (but still > ttm underneath) > - 5 ttm+gem drivers, using ttm directly > - 1 ttm driver, no gem > - 48 other gem drivers with no vram support > - 1 gem driver which will gain vram support shortly, with or without > ttm still not clear > > 11:48 is not even close to 59:1 imo. And I think even if Thomas > Zimmermann and others get really busy porting old discrete fbdev > drivers to kms, that ratio won't change much since we're also gaining > new soc drm drivers at a steady rate. Yeah, my point was not really suggesting that we do this, but rather that people would rightfully get upset because the struct contains unused stuff. Also a trap we might end up with in the future is that with the design suggested in this patch series is that people start assuming that the embedded gem object is actually initialized and working, which could lead to pretty severe problems for vmwgfx... > Also I wouldn't mind if we e.g. stuff a struct list_head lru; into > drm_gem_buffer_object, that's probably useful for many cases (not the > pure display drivers, but they tend to have so few bo it really wont > matter even if we add a few kb of cruft). > >> What I'm suggesting is, build that improved code and concept sharing around >> >> struct gem_ttm_object { >> struct gem_object; >> struct ttm_object; >> }; > I guess technically this would work too. Bit more churn (maybe > substantially more, I haven't looked tbh) to roll this out for all the > ttm drivers using gem. > >> And lets work toghether to eliminate what's duplicated. > How would you share the bo.resv pointer with the above design? With > embedding ttm can use the gem one, and we drop a bunch of code (and > for all the ttm+gem drivers, one pointer they don't need twice). With > the side-by-side, which is the design all gem+ttm drivers used the > past few years, we still need that duplication. Same for the vma node > thing, which is also duplicated. To bemore precise I'd probably define a struct drm_bo_common { struct reservation_object r; struct drm_vma_node v; }; Embed it in a struct drm_gem_object (and in a struct vmwgfx_buffer_object) and then have a pointer to a struct drm_bo_common in the struct ttm_buffer_object. That's a single pointer overhead for everything we want to move. As TTM-specific code disappears, so will the number of members in struct drm_bo_common. Meanwhile, we maintain a nice layering and no extra unused payload for any implementation. > And that's just the things Gerd did > in this initial patch series. I think Christian König has some ideas > around ttm_bo_(un)reserve, and I just sent out a patch series to > streamline how we handle the reservation_object pointer for prime > import/export. > >> The vmwgfx driver is doing what it does mostly because all buffer >> objects do not need to be user-space visible, and do not need to be >> mapped by user-space. And there are other types of objects that DO need >> to be user-space visible, and that do need to be shared by processes. >> Hence user-space visibility is something that should be abstracted and >> made available to those objects. Not lumped together with all other >> potential buffer object functionality. > I'd still go with pragmatic choice of "this here is typed & reviewed, > and a bunch of people are enthusastistic about using this to drive > further refactoring in this area". Whatever the reasons, but since a > few months the ttm refactor train seems to have gained massive > momentum. > > So if you want to change the direction of this train, I think you need > to get typing and show that your solution is at least as effective at > faciliting all the things people want to do with code sharing across > drm drivers. Well I don't think it needs to change direction. I just feel it got temporarily derailed. But if that's what it take I'll put something like the above together to see how it ends up. But I can't do it until August, though when I'm back, so I guess I have to check where the code is at then. /Thomas > > Cheers, Daniel
Hi, > Yeah, my point was not really suggesting that we do this, but rather that > people would rightfully get upset because the struct contains unused stuff. Well, struct drm_gem_object isn't that big, lets have a look: 320 bytes in total, of which are: 184 bytes the embedded vma_mode 64 bytes the embedded _resv struct 8 bytes the resv pointer. 64 bytes everything else. So, after applying this series it's 64 bytes more per bo for vmwgfx, due to some unused fields being added. And it's 256 bytes less per bo for all ttm+gem drivers, because they don't have vma_mode + resv struct + resv pointer twice any more. And that is just the low-hanging fruit, there is room for more ttm_buffer_object elements being removed by using the drm_gem_object struct elements instead. num_pages, kref, maybe more. > Also a trap we might end up with in the future is that with the design > suggested in this patch series is that people start assuming that the > embedded gem object is actually initialized and working, which could lead to > pretty severe problems for vmwgfx... I guess I should reword patch #1 then, making clear that the ttm_bo_uses_embedded_gem_object() helper function is going to stay. What is the reason for vmwgfx to not use gem btw? > > for all the ttm+gem drivers, one pointer they don't need twice). With > > the side-by-side, which is the design all gem+ttm drivers used the > > past few years, we still need that duplication. Same for the vma node > > thing, which is also duplicated. > > To bemore precise I'd probably define a > > struct drm_bo_common { > struct reservation_object r; > struct drm_vma_node v; > }; > > Embed it in a struct drm_gem_object (and in a struct vmwgfx_buffer_object) > and then have a pointer to a struct drm_bo_common in the struct > ttm_buffer_object. A pointer doesn't cut it. Beside removing the duplication the other thing I want is to have a standard way of finding the ttm_buffer_object for a given drm_gem_object for all the ttm+gem drivers. With struct drm_gem_object being embedded the usual containter_of() will work. That'll allow to create drm_gem_ttm_helper.c with helper functions for struct drm_gem_object_funcs. For starters some of the current vram helpers can become generic ttm helpers because they loose the dependency on struct drm_gem_vram_object for finding ttm_buffer_object. > > > The vmwgfx driver is doing what it does mostly because all buffer > > > objects do not need to be user-space visible, and do not need to be > > > mapped by user-space. And there are other types of objects that DO need > > > to be user-space visible, and that do need to be shared by processes. > > > Hence user-space visibility is something that should be abstracted and > > > made available to those objects. Not lumped together with all other > > > potential buffer object functionality. Well, ttm_buffer_object has a vma_node embedded, so it already is "lumped together". This patch series only moves it around ... cheers, Gerd
Hi Thomas, On Sat, Jun 22, 2019 at 09:14:24PM +0200, Thomas Hellstrom wrote: > On 6/22/19 11:18 AM, Daniel Vetter wrote: > > Hi Thomas, > > > > On Sat, Jun 22, 2019 at 12:52 AM Thomas Hellstrom<thomas@shipmail.org> wrote: > > > On 6/21/19 5:57 PM, Daniel Vetter wrote: > > > > On Fri, Jun 21, 2019 at 05:12:19PM +0200, Thomas Hellström (VMware) wrote: > > > > > On 6/21/19 1:57 PM, Gerd Hoffmann wrote: > > > > > > > > > > Aargh. Please don't do this. Multiple reasons: > > > > > > > > > > 1) I think It's bad to dump all buffer object functionality we can possibly > > > > > think of in a single struct and force that on all (well at least most) > > > > > users. It's better to isolate functionality in structs, have utility > > > > > functions for those and let the drivers derive their buffer objects from > > > > > whatever functionality they actually need. > > > > > 2) vmwgfx is not using gem and we don't want to carry that extra payload in > > > > > the buffer object. > > > > > 3) TTM historically hasn't been using the various drm layers except for > > > > > later when common helpers have been used, (like the vma manager and the > > > > > cache utilities). It's desirable to keep that layer distinction. (which is > > > > > really what I'm saying in 1.) > > > > > > > > > > Now if more and more functionality that originated in TTM is moving into GEM > > > > > we need to find a better way to do that without duplicating functionality. I > > > > > suggest adding pointers in the TTM structs and defaulting those pointers to > > > > > the member in the TTM struct. Optionally to to the member in the GEM struct. > > > > > If we need to migrate those members out of the TTM struct, vmwgfx would have > > > > > to provide them in its own buffer class. > > > > > > > > > > NAK from the vmwgfx side. > > > > It's 59 DRIVER_GEM vs 1 which is not. I think the verdict is clear what > > > > the reasonable thing to do is here, and this will allow us to > > > > substantially improve code and concept sharing across drm drivers. > > > > > > > > 10 years ago it was indeed not clear whether everyone doing the same is a > > > > bright idea, but that's no more. If you want I guess you can keep a > > > > private copy of ttm in vmwgfx, but not sure that's really worth it > > > > long-term. > > > > -Daniel > > > It's not a question about whether GEM or TTM, or even the number of > > > drivers using one or the other. (GEM would actually be a good choice for > > > the latter vmwgfx device versions). But this is going against all recent > > > effort to make different parts of drm functionality recently self-contained. > > > > > > Just stop and think for a while what would happen if someone would > > > suggest doing the opposite: making a gem object a derived class of a TTM > > > object, arguing that a lot of GEM drivers are using TTM as a backend. > > > There would probably be a lot of people claiming "we don't want to > > > unnecesarily carry that stuff". That's because that would also be a poor > > > design. > > That case is a bit a different case. We have > > - 5 ttm+gem drivers, recently refactored into vram helpers (but still > > ttm underneath) > > - 5 ttm+gem drivers, using ttm directly > > - 1 ttm driver, no gem > > - 48 other gem drivers with no vram support > > - 1 gem driver which will gain vram support shortly, with or without > > ttm still not clear > > > > 11:48 is not even close to 59:1 imo. And I think even if Thomas > > Zimmermann and others get really busy porting old discrete fbdev > > drivers to kms, that ratio won't change much since we're also gaining > > new soc drm drivers at a steady rate. > > Yeah, my point was not really suggesting that we do this, but rather that > people would rightfully get upset because the struct contains unused stuff. > > Also a trap we might end up with in the future is that with the design > suggested in this patch series is that people start assuming that the > embedded gem object is actually initialized and working, which could lead to > pretty severe problems for vmwgfx... Yeah that's the biggest risk here. But that same risk exists no matter how we share code, people can also add new fields to the common_bo_struct you propose and then forget to intitialize it in one of the three use-cases: - gem only - ttm+gem - ttm only And vmwgfx is the most threatened here because it's the least common config. That's just part of the prize, and if it's really bad I think we could do some self-tests, to make sure basic ttm-only stuff keeps working as it should. Plus get the selftests integrated into default builds. > > Also I wouldn't mind if we e.g. stuff a struct list_head lru; into > > drm_gem_buffer_object, that's probably useful for many cases (not the > > pure display drivers, but they tend to have so few bo it really wont > > matter even if we add a few kb of cruft). > > > > > What I'm suggesting is, build that improved code and concept sharing around > > > > > > struct gem_ttm_object { > > > struct gem_object; > > > struct ttm_object; > > > }; > > I guess technically this would work too. Bit more churn (maybe > > substantially more, I haven't looked tbh) to roll this out for all the > > ttm drivers using gem. > > > > > And lets work toghether to eliminate what's duplicated. > > How would you share the bo.resv pointer with the above design? With > > embedding ttm can use the gem one, and we drop a bunch of code (and > > for all the ttm+gem drivers, one pointer they don't need twice). With > > the side-by-side, which is the design all gem+ttm drivers used the > > past few years, we still need that duplication. Same for the vma node > > thing, which is also duplicated. > > To bemore precise I'd probably define a > > struct drm_bo_common { > struct reservation_object r; > struct drm_vma_node v; > }; > > Embed it in a struct drm_gem_object (and in a struct vmwgfx_buffer_object) > and then have a pointer to a struct drm_bo_common in the struct > ttm_buffer_object. That's a single pointer overhead for everything we want > to move. > > As TTM-specific code disappears, so will the number of members in struct > drm_bo_common. Meanwhile, we maintain a nice layering and no extra unused > payload for any implementation. > > > And that's just the things Gerd did > > in this initial patch series. I think Christian König has some ideas > > around ttm_bo_(un)reserve, and I just sent out a patch series to > > streamline how we handle the reservation_object pointer for prime > > import/export. > > > > > The vmwgfx driver is doing what it does mostly because all buffer > > > objects do not need to be user-space visible, and do not need to be > > > mapped by user-space. And there are other types of objects that DO need > > > to be user-space visible, and that do need to be shared by processes. > > > Hence user-space visibility is something that should be abstracted and > > > made available to those objects. Not lumped together with all other > > > potential buffer object functionality. > > I'd still go with pragmatic choice of "this here is typed & reviewed, > > and a bunch of people are enthusastistic about using this to drive > > further refactoring in this area". Whatever the reasons, but since a > > few months the ttm refactor train seems to have gained massive > > momentum. > > > > So if you want to change the direction of this train, I think you need > > to get typing and show that your solution is at least as effective at > > faciliting all the things people want to do with code sharing across > > drm drivers. > > Well I don't think it needs to change direction. I just feel it got > temporarily derailed. But if that's what it take I'll put something like the > above together to see how it ends up. But I can't do it until August, though > when I'm back, so I guess I have to check where the code is at then. Yeah I think a wait&see is best here, we can course-correct later on still. -Daniel
Am 24.06.19 um 08:32 schrieb Gerd Hoffmann: > Hi, > >> Yeah, my point was not really suggesting that we do this, but rather that >> people would rightfully get upset because the struct contains unused stuff. > Well, struct drm_gem_object isn't that big, lets have a look: > > 320 bytes in total, of which are: > 184 bytes the embedded vma_mode > 64 bytes the embedded _resv struct > 8 bytes the resv pointer. > 64 bytes everything else. > > So, after applying this series it's 64 bytes more per bo for vmwgfx, > due to some unused fields being added. > > And it's 256 bytes less per bo for all ttm+gem drivers, because they > don't have vma_mode + resv struct + resv pointer twice any more. > > And that is just the low-hanging fruit, there is room for more > ttm_buffer_object elements being removed by using the drm_gem_object > struct elements instead. num_pages, kref, maybe more. Yeah, and that is exactly the reason why I'm strongly in favor of this approach. GEM is our de-facto standard for buffer UAPI in DRM. AFAIK vmgfx is one of the few drivers not using it and as you wrote as well it might actually be a good idea to change that. The only thing I can of hand see which is misplaced in the drm_gem_object structure is "struct file *filp;", cause that is specific to a backend implementation. Regards, Christian. > >> Also a trap we might end up with in the future is that with the design >> suggested in this patch series is that people start assuming that the >> embedded gem object is actually initialized and working, which could lead to >> pretty severe problems for vmwgfx... > I guess I should reword patch #1 then, making clear that the > ttm_bo_uses_embedded_gem_object() helper function is going to stay. > > What is the reason for vmwgfx to not use gem btw? > >>> for all the ttm+gem drivers, one pointer they don't need twice). With >>> the side-by-side, which is the design all gem+ttm drivers used the >>> past few years, we still need that duplication. Same for the vma node >>> thing, which is also duplicated. >> To bemore precise I'd probably define a >> >> struct drm_bo_common { >> struct reservation_object r; >> struct drm_vma_node v; >> }; >> >> Embed it in a struct drm_gem_object (and in a struct vmwgfx_buffer_object) >> and then have a pointer to a struct drm_bo_common in the struct >> ttm_buffer_object. > A pointer doesn't cut it. > > Beside removing the duplication the other thing I want is to have a > standard way of finding the ttm_buffer_object for a given drm_gem_object > for all the ttm+gem drivers. With struct drm_gem_object being embedded > the usual containter_of() will work. > > That'll allow to create drm_gem_ttm_helper.c with helper functions for > struct drm_gem_object_funcs. For starters some of the current vram > helpers can become generic ttm helpers because they loose the dependency > on struct drm_gem_vram_object for finding ttm_buffer_object. > >>>> The vmwgfx driver is doing what it does mostly because all buffer >>>> objects do not need to be user-space visible, and do not need to be >>>> mapped by user-space. And there are other types of objects that DO need >>>> to be user-space visible, and that do need to be shared by processes. >>>> Hence user-space visibility is something that should be abstracted and >>>> made available to those objects. Not lumped together with all other >>>> potential buffer object functionality. > Well, ttm_buffer_object has a vma_node embedded, so it already is > "lumped together". This patch series only moves it around ... > > cheers, > Gerd >
On Fri, Jun 21, 2019 at 01:57:37PM +0200, Gerd Hoffmann wrote: > v2: > - build fixes. > - also drop ttm_buffer_object->resv > > Gerd Hoffmann (18): > drm/ttm: add gem base object > drm/vram: use embedded gem object > drm/qxl: use embedded gem object > drm/radeon: use embedded gem object > drm/amdgpu: use embedded gem object > drm/nouveau: use embedded gem object > drm/ttm: use gem reservation object > drm/ttm: use gem vma_node > drm/vram: drop drm_gem_vram_driver_gem_prime_mmap > drm/ttm: set both resv and base.resv pointers > drm/ttm: switch ttm core from bo->resv to bo->base.resv > drm/radeon: switch driver from bo->resv to bo->base.resv > drm/vmwgfx: switch driver from bo->resv to bo->base.resv > drm/amdgpu: switch driver from bo->resv to bo->base.resv > drm/nouveau: switch driver from bo->resv to bo->base.resv > drm/qxl: switch driver from bo->resv to bo->base.resv > drm/virtio: switch driver from bo->resv to bo->base.resv > drm/ttm: drop ttm_buffer_object->resv Imo get maybe another ttm+gem stakeholder to review this (Thomas for vram helpers or Ben for nouveau) and then this can land. I think Thomas Hellstrom tuned down his categorical "nak" to "we'll see where this goes, I might need to jump in and help course-correct". btw I also resubmitted my prime resv_obj cleanup series: https://patchwork.freedesktop.org/series/62735/ I think the only conflicts with your series here is that the setting of gem_bo.resv that I add in radeon/amdgpu/nouveau can be removed again when your stuff lands. -Daniel > > drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 3 +- > drivers/gpu/drm/nouveau/nouveau_bo.h | 5 - > drivers/gpu/drm/nouveau/nouveau_gem.h | 2 +- > drivers/gpu/drm/qxl/qxl_drv.h | 6 +- > drivers/gpu/drm/qxl/qxl_object.h | 6 +- > drivers/gpu/drm/radeon/radeon.h | 3 +- > drivers/gpu/drm/radeon/radeon_object.h | 2 +- > drivers/gpu/drm/virtio/virtgpu_drv.h | 2 +- > include/drm/drm_gem_vram_helper.h | 7 +- > include/drm/ttm/ttm_bo_api.h | 25 +++- > include/drm/ttm/ttm_bo_driver.h | 12 +- > .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 6 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 6 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 6 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 14 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 28 ++-- > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 8 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 4 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 30 ++-- > drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 2 +- > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +- > drivers/gpu/drm/ast/ast_main.c | 2 +- > drivers/gpu/drm/drm_gem_vram_helper.c | 36 ++--- > drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 2 +- > drivers/gpu/drm/mgag200/mgag200_main.c | 2 +- > drivers/gpu/drm/nouveau/dispnv50/wndw.c | 2 +- > drivers/gpu/drm/nouveau/nouveau_abi16.c | 4 +- > drivers/gpu/drm/nouveau/nouveau_bo.c | 8 +- > drivers/gpu/drm/nouveau/nouveau_display.c | 10 +- > drivers/gpu/drm/nouveau/nouveau_fence.c | 2 +- > drivers/gpu/drm/nouveau/nouveau_gem.c | 19 +-- > drivers/gpu/drm/nouveau/nouveau_prime.c | 6 +- > drivers/gpu/drm/qxl/qxl_cmd.c | 4 +- > drivers/gpu/drm/qxl/qxl_debugfs.c | 4 +- > drivers/gpu/drm/qxl/qxl_display.c | 8 +- > drivers/gpu/drm/qxl/qxl_gem.c | 2 +- > drivers/gpu/drm/qxl/qxl_object.c | 20 +-- > drivers/gpu/drm/qxl/qxl_release.c | 8 +- > drivers/gpu/drm/qxl/qxl_ttm.c | 4 +- > drivers/gpu/drm/radeon/radeon_benchmark.c | 4 +- > drivers/gpu/drm/radeon/radeon_cs.c | 4 +- > drivers/gpu/drm/radeon/radeon_display.c | 6 +- > drivers/gpu/drm/radeon/radeon_gem.c | 8 +- > drivers/gpu/drm/radeon/radeon_mn.c | 2 +- > drivers/gpu/drm/radeon/radeon_object.c | 22 +-- > drivers/gpu/drm/radeon/radeon_prime.c | 4 +- > drivers/gpu/drm/radeon/radeon_test.c | 8 +- > drivers/gpu/drm/radeon/radeon_ttm.c | 4 +- > drivers/gpu/drm/radeon/radeon_uvd.c | 2 +- > drivers/gpu/drm/radeon/radeon_vm.c | 6 +- > drivers/gpu/drm/ttm/ttm_bo.c | 136 +++++++++--------- > drivers/gpu/drm/ttm/ttm_bo_util.c | 18 +-- > drivers/gpu/drm/ttm/ttm_bo_vm.c | 15 +- > drivers/gpu/drm/ttm/ttm_execbuf_util.c | 20 +-- > drivers/gpu/drm/ttm/ttm_tt.c | 2 +- > drivers/gpu/drm/vboxvideo/vbox_main.c | 2 +- > drivers/gpu/drm/virtio/virtgpu_ioctl.c | 4 +- > drivers/gpu/drm/virtio/virtgpu_plane.c | 2 +- > drivers/gpu/drm/virtio/virtgpu_prime.c | 3 - > drivers/gpu/drm/vmwgfx/vmwgfx_blit.c | 4 +- > drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 12 +- > drivers/gpu/drm/vmwgfx/vmwgfx_cotable.c | 4 +- > drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 6 +- > drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 4 +- > 68 files changed, 311 insertions(+), 321 deletions(-) > > -- > 2.18.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi, > Imo get maybe another ttm+gem stakeholder to review this (Thomas for vram > helpers or Ben for nouveau) and then this can land. I think Thomas > Hellstrom tuned down his categorical "nak" to "we'll see where this goes, > I might need to jump in and help course-correct". Yep, seems he also is on summer vacation now ... > btw I also resubmitted my prime resv_obj cleanup series: > > https://patchwork.freedesktop.org/series/62735/ What is the merge plan for this? Looks like this is pretty much ready to go. > I think the only conflicts with your series here is that the setting of > gem_bo.resv that I add in radeon/amdgpu/nouveau can be removed again when > your stuff lands. Yep, doesn't look too bad. I can rebase on top of your series, I plan to send v3 anyway, I have some smaller (comment) updates pending. cheers, Gerd
On Thu, Jun 27, 2019 at 02:22:18PM +0200, Gerd Hoffmann wrote: > Hi, > > > Imo get maybe another ttm+gem stakeholder to review this (Thomas for vram > > helpers or Ben for nouveau) and then this can land. I think Thomas > > Hellstrom tuned down his categorical "nak" to "we'll see where this goes, > > I might need to jump in and help course-correct". > > Yep, seems he also is on summer vacation now ... > > > btw I also resubmitted my prime resv_obj cleanup series: > > > > https://patchwork.freedesktop.org/series/62735/ > > What is the merge plan for this? Looks like this is pretty much ready > to go. Needs an r-b from amd and maybe Ben, then I'll merge that I think. > > I think the only conflicts with your series here is that the setting of > > gem_bo.resv that I add in radeon/amdgpu/nouveau can be removed again when > > your stuff lands. > > Yep, doesn't look too bad. I can rebase on top of your series, I plan > to send v3 anyway, I have some smaller (comment) updates pending. It's just 3 lines we need to remove in the end (the ones that set gem_bo.resv), and those 3 lines are harmless I think. I think we can just merge whenever and then do the follow up once both series have landed. -Daniel