| Message ID | 20230228083406.1720795-10-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/9] drm: execution context for GEM buffers v3 | expand |
On Tue, 2023-02-28 at 09:34 +0100, Christian König wrote: > VMWGFX is the only remaining user of this and should probably moved over > to drm_exec when it starts using GEM as well. Is this because vmwgfx piggybacks buffer-id relocations on top of ttm validations or did you just find it too hard to port it over? I'd prefer to avoid ttm moves to vmwgfx and at least have a clear idea of what we need to do to port. z
Am 08.03.23 um 06:14 schrieb Zack Rusin: > On Tue, 2023-02-28 at 09:34 +0100, Christian König wrote: >> VMWGFX is the only remaining user of this and should probably moved over >> to drm_exec when it starts using GEM as well. > Is this because vmwgfx piggybacks buffer-id relocations on top of ttm validations or > did you just find it too hard to port it over? I'd prefer to avoid ttm moves to > vmwgfx and at least have a clear idea of what we need to do to port. I've just found it to hard to port it over because vmwgfx does some strange things with the validation code here. If you want we can take a deeper look at this together, but I need to find some time. Alternatively just tell me how to do it and I will add that to the patch set :) Regards, Christian. > > z
On Wed, 2023-03-08 at 10:10 +0100, Christian König wrote: > > Am 08.03.23 um 06:14 schrieb Zack Rusin: > > On Tue, 2023-02-28 at 09:34 +0100, Christian König wrote: > > > VMWGFX is the only remaining user of this and should probably moved over > > > to drm_exec when it starts using GEM as well. > > Is this because vmwgfx piggybacks buffer-id relocations on top of ttm > > validations or > > did you just find it too hard to port it over? I'd prefer to avoid ttm moves to > > vmwgfx and at least have a clear idea of what we need to do to port. > > I've just found it to hard to port it over because vmwgfx does some > strange things with the validation code here. > > If you want we can take a deeper look at this together, but I need to > find some time. > > Alternatively just tell me how to do it and I will add that to the patch > set :) I don't want to hold up the set (it looks good btw), because I had to look at something else today and tomorrow. We overload the validation lists to do quite a bit more than just reservations though. There are, I think, four separate things that need to be refactored there (Christian, feel free to skip this section, this is mainly for VMware folks on the team): 1) Relocations - userspace uses the id's of the bo's in the command stream, but on the kernel side those id's are different (or in vmwgfx terminology gem id != mob id), so the buffer id's in the command stream need to be replaced, 2) Resource validation. vmwgfx splits the userspace objects into buffers and resources (shaders, surfaces, contexts). The resources are not buffers but are backed by them. A single buffer can back multiple different resources and sometimes the kernel has to actually allocate a buffer to back a resource and attach it to it (i.e. in common terminology buffer is the memory and resources are placed in it) . Now this shouldn't be in the kernel at all, the resources shouldn't have been kernel objects and instead we should have left them completely to userspace. 3) Coherency tracking. We use validation lists as a central place for tracking which bo's/resources are used in a command buffer and we use it to keep track of which buffers/resources will endup dirty to implement coherency. 4) Central place to allocate memory for relocation/validation nodes. Where we want to endup is with 2 completely gone from the kernel side and 1, 3 and 4 refactored and cleaned up. I think there's at least 4 separate patches to this port, so it's not a trivial thing. We will take a look at this on Friday in more detail to see what we can do. z
On 3/8/23 10:10, Christian König wrote: > Am 08.03.23 um 06:14 schrieb Zack Rusin: >> On Tue, 2023-02-28 at 09:34 +0100, Christian König wrote: >>> VMWGFX is the only remaining user of this and should probably moved >>> over >>> to drm_exec when it starts using GEM as well. >> Is this because vmwgfx piggybacks buffer-id relocations on top of ttm >> validations or >> did you just find it too hard to port it over? I'd prefer to avoid >> ttm moves to >> vmwgfx and at least have a clear idea of what we need to do to port. > > I've just found it to hard to port it over because vmwgfx does some > strange things with the validation code here. > > If you want we can take a deeper look at this together, but I need to > find some time. > > Alternatively just tell me how to do it and I will add that to the > patch set :) > > Regards, > Christian. Hmm. It turns out Xe was using these from the very start as well. But I will take a look at what it take to deprecate that usage, so don't let that stop this removal. We need a more flexible WW transaction handling anyway. /Thomas > >> >> z
Hi, On 3/9/23 06:14, Zack Rusin wrote: > On Wed, 2023-03-08 at 10:10 +0100, Christian König wrote: >> Am 08.03.23 um 06:14 schrieb Zack Rusin: >>> On Tue, 2023-02-28 at 09:34 +0100, Christian König wrote: >>>> VMWGFX is the only remaining user of this and should probably moved over >>>> to drm_exec when it starts using GEM as well. >>> Is this because vmwgfx piggybacks buffer-id relocations on top of ttm >>> validations or >>> did you just find it too hard to port it over? I'd prefer to avoid ttm moves to >>> vmwgfx and at least have a clear idea of what we need to do to port. >> I've just found it to hard to port it over because vmwgfx does some >> strange things with the validation code here. >> >> If you want we can take a deeper look at this together, but I need to >> find some time. >> >> Alternatively just tell me how to do it and I will add that to the patch >> set :) > I don't want to hold up the set (it looks good btw), because I had to look at > something else today and tomorrow. > > We overload the validation lists to do quite a bit more than just reservations > though. > > There are, I think, four separate things that need to be refactored there > (Christian, feel free to skip this section, this is mainly for VMware folks on the > team): > 1) Relocations - userspace uses the id's of the bo's in the command stream, but on > the kernel side those id's are different (or in vmwgfx terminology gem id != mob > id), so the buffer id's in the command stream need to be replaced, > 2) Resource validation. vmwgfx splits the userspace objects into buffers and > resources (shaders, surfaces, contexts). The resources are not buffers but are > backed by them. A single buffer can back multiple different resources and sometimes > the kernel has to actually allocate a buffer to back a resource and attach it to it > (i.e. in common terminology buffer is the memory and resources are placed in it) . > Now this shouldn't be in the kernel at all, the resources shouldn't have been kernel > objects and instead we should have left them completely to userspace. The reason behind this is partly historical, since initially the resources (IIRC surfaces and shaders at that time), were per-device and not per context and not backed by buffer objects. The main reason for not doing the transition to user-space for resources was the SVGA device "between-draw-call-validation". If you for example unbound a mob that was backing a resource that was bound to a context, you'd need to start a whole chain of resource invalidations to avoid the device complaining about invalid setups at awkward moments, for example when it felt like suspending. Not sure whether that is gone now though, or whether there are better ways to handle that. /Thomas > 3) Coherency tracking. We use validation lists as a central place for tracking which > bo's/resources are used in a command buffer and we use it to keep track of which > buffers/resources will endup dirty to implement coherency. > 4) Central place to allocate memory for relocation/validation nodes. > > Where we want to endup is with 2 completely gone from the kernel side and 1, 3 and 4 > refactored and cleaned up. I think there's at least 4 separate patches to this port, > so it's not a trivial thing. We will take a look at this on Friday in more detail to > see what we can do. > > z
diff --git a/drivers/gpu/drm/ttm/Makefile b/drivers/gpu/drm/ttm/Makefile index f906b22959cf..b05a8477d0d0 100644 --- a/drivers/gpu/drm/ttm/Makefile +++ b/drivers/gpu/drm/ttm/Makefile @@ -3,8 +3,8 @@ # Makefile for the drm device driver. This driver provides support for the ttm-y := ttm_tt.o ttm_bo.o ttm_bo_util.o ttm_bo_vm.o ttm_module.o \ - ttm_execbuf_util.o ttm_range_manager.o ttm_resource.o ttm_pool.o \ - ttm_device.o ttm_sys_manager.o + ttm_range_manager.o ttm_resource.o ttm_pool.o ttm_device.o \ + ttm_sys_manager.o ttm-$(CONFIG_AGP) += ttm_agp_backend.o obj-$(CONFIG_DRM_TTM) += ttm.o diff --git a/drivers/gpu/drm/vmwgfx/Makefile b/drivers/gpu/drm/vmwgfx/Makefile index e94479d9cd5b..e30e10e25c53 100644 --- a/drivers/gpu/drm/vmwgfx/Makefile +++ b/drivers/gpu/drm/vmwgfx/Makefile @@ -1,6 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 vmwgfx-y := vmwgfx_execbuf.o vmwgfx_gmr.o vmwgfx_kms.o vmwgfx_drv.o \ - vmwgfx_ioctl.o vmwgfx_resource.o vmwgfx_ttm_buffer.o \ + vmwgfx_ioctl.o vmwgfx_resource.o vmwgfx_ttm_buffer.o ttm_execbuf_util.o \ vmwgfx_cmd.o vmwgfx_irq.o vmwgfx_ldu.o \ vmwgfx_overlay.o vmwgfx_gmrid_manager.o vmwgfx_fence.o \ vmwgfx_bo.o vmwgfx_scrn.o vmwgfx_context.o \ diff --git a/drivers/gpu/drm/ttm/ttm_execbuf_util.c b/drivers/gpu/drm/vmwgfx/ttm_execbuf_util.c similarity index 97% rename from drivers/gpu/drm/ttm/ttm_execbuf_util.c rename to drivers/gpu/drm/vmwgfx/ttm_execbuf_util.c index f1c60fa80c2d..5e4e28899acd 100644 --- a/drivers/gpu/drm/ttm/ttm_execbuf_util.c +++ b/drivers/gpu/drm/vmwgfx/ttm_execbuf_util.c @@ -26,8 +26,13 @@ * **************************************************************************/ -#include <drm/ttm/ttm_execbuf_util.h> #include <drm/ttm/ttm_bo.h> +#include <drm/ttm/ttm_placement.h> +#include <linux/wait.h> +#include <linux/sched.h> +#include <linux/module.h> + +#include "ttm_execbuf_util.h" static void ttm_eu_backoff_reservation_reverse(struct list_head *list, struct ttm_validate_buffer *entry) diff --git a/include/drm/ttm/ttm_execbuf_util.h b/drivers/gpu/drm/vmwgfx/ttm_execbuf_util.h similarity index 100% rename from include/drm/ttm/ttm_execbuf_util.h rename to drivers/gpu/drm/vmwgfx/ttm_execbuf_util.h diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h index fb8f0c0642c0..49e3dd8c04ec 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h @@ -37,11 +37,11 @@ #include <drm/drm_file.h> #include <drm/drm_rect.h> -#include <drm/ttm/ttm_execbuf_util.h> #include <drm/ttm/ttm_tt.h> #include <drm/ttm/ttm_placement.h> #include <drm/ttm/ttm_bo.h> +#include "ttm_execbuf_util.h" #include "ttm_object.h" #include "vmwgfx_fence.h" diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_validation.h b/drivers/gpu/drm/vmwgfx/vmwgfx_validation.h index 240ee0c4ebfd..927fc8afdbfe 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_validation.h +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_validation.h @@ -32,7 +32,7 @@ #include <linux/hashtable.h> #include <linux/ww_mutex.h> -#include <drm/ttm/ttm_execbuf_util.h> +#include "ttm_execbuf_util.h" #define VMW_RES_DIRTY_NONE 0 #define VMW_RES_DIRTY_SET BIT(0)
VMWGFX is the only remaining user of this and should probably moved over to drm_exec when it starts using GEM as well. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/ttm/Makefile | 4 ++-- drivers/gpu/drm/vmwgfx/Makefile | 2 +- drivers/gpu/drm/{ttm => vmwgfx}/ttm_execbuf_util.c | 7 ++++++- .../drm/ttm => drivers/gpu/drm/vmwgfx}/ttm_execbuf_util.h | 0 drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 2 +- drivers/gpu/drm/vmwgfx/vmwgfx_validation.h | 2 +- 6 files changed, 11 insertions(+), 6 deletions(-) rename drivers/gpu/drm/{ttm => vmwgfx}/ttm_execbuf_util.c (97%) rename {include/drm/ttm => drivers/gpu/drm/vmwgfx}/ttm_execbuf_util.h (100%)