Re: [PATCH 2/6] drm/ttm: Store the bo_kmap_type in struct iosys_map

[Christian König]

Hi,

Am 17.06.24 um 14:32 schrieb Thomas Zimmermann:
Hi
Am 14.06.24 um 16:31 schrieb Christian König:
Am 14.06.24 um 15:21 schrieb Thomas Zimmermann:
For each instances of struct iosys_map set up by ttm_bo_vmap(), store
the type of allocation in the instance. Use this information to unmap
the memory in ttm_bo_vunmap(). This change simplifies the unmap code
and puts the complicated logic entirely into the map code.

I'm not sure that's a good idea.

The mapping information should already be available in the resource 
and storing it in the iosys_map structures duplicates that information.

So we might run into the issue that the resource has changed and so 
we need a different approach now, but the iosys_map will say that we 
should unmap things for example.

Patches 1 and 2 are only here to make patch 4 (add the kmap special 
case) work. How can I distinguish between vmap'ed and kmap'ed memory? 
It's not clear to me, whether there is a benefit from patch 4. The xe 
driver makes it sound like that, but the rest of the kernel appears to 
disagree.

Yeah, exactly that's the point.

The last time we talked about that we came to the conclusion that the 
kmap approach of mapping only a single page or range of pages isn't that 
useful in general.

The only use case where you actually need this is the 
ttm_bo_vm_access_kmap() helper and that is static and internal to TTM.

So what exactly is the use case xe tries to handle here?

Regards,
Christian.


Best regards
Thomas


Regards,
Christian.


Signed-off-by: Thomas Zimmermann <tzimmerm...@suse.de>
---
  drivers/gpu/drm/ttm/ttm_bo_util.c | 46 
+++++++++++++++++++++----------
  1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c 
b/drivers/gpu/drm/ttm/ttm_bo_util.c
index 0b3f4267130c4..a9df0deff2deb 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -36,6 +36,7 @@
  #include <drm/ttm/ttm_tt.h>
    #include <drm/drm_cache.h>
+#include <drm/drm_device.h>
    struct ttm_transfer_obj {
      struct ttm_buffer_object base;
@@ -479,24 +480,29 @@ int ttm_bo_vmap(struct ttm_buffer_object *bo, 
struct iosys_map *map)
        if (mem->bus.is_iomem) {
          void __iomem *vaddr_iomem;
+        u16 alloc_flags;
  -        if (mem->bus.addr)
+        if (mem->bus.addr) {
              vaddr_iomem = (void __iomem *)mem->bus.addr;
-        else if (mem->bus.caching == ttm_write_combined)
-            vaddr_iomem = ioremap_wc(mem->bus.offset,
-                         bo->base.size);
+            alloc_flags = ttm_bo_map_premapped;
+        } else if (mem->bus.caching == ttm_write_combined) {
+            vaddr_iomem = ioremap_wc(mem->bus.offset, bo->base.size);
+            alloc_flags = ttm_bo_map_iomap;
  #ifdef CONFIG_X86
-        else if (mem->bus.caching == ttm_cached)
-            vaddr_iomem = ioremap_cache(mem->bus.offset,
-                          bo->base.size);
+        } else if (mem->bus.caching == ttm_cached) {
+            vaddr_iomem = ioremap_cache(mem->bus.offset, 
bo->base.size);
+            alloc_flags = ttm_bo_map_iomap;
  #endif
-        else
+        } else {
              vaddr_iomem = ioremap(mem->bus.offset, bo->base.size);
+            alloc_flags = ttm_bo_map_iomap;
+        }
            if (!vaddr_iomem)
              return -ENOMEM;
            iosys_map_set_vaddr_iomem(map, vaddr_iomem);
+        map->alloc_flags = alloc_flags;
        } else {
          struct ttm_operation_ctx ctx = {
@@ -506,6 +512,7 @@ int ttm_bo_vmap(struct ttm_buffer_object *bo, 
struct iosys_map *map)
          struct ttm_tt *ttm = bo->ttm;
          pgprot_t prot;
          void *vaddr;
+        u16 alloc_flags;
            ret = ttm_tt_populate(bo->bdev, ttm, &ctx);
          if (ret)
@@ -519,8 +526,10 @@ int ttm_bo_vmap(struct ttm_buffer_object *bo, 
struct iosys_map *map)
          vaddr = vmap(ttm->pages, ttm->num_pages, 0, prot);
          if (!vaddr)
              return -ENOMEM;
+        alloc_flags = ttm_bo_map_vmap;
            iosys_map_set_vaddr(map, vaddr);
+        map->alloc_flags = alloc_flags;
      }
        return 0;
@@ -537,20 +546,27 @@ EXPORT_SYMBOL(ttm_bo_vmap);
   */
  void ttm_bo_vunmap(struct ttm_buffer_object *bo, struct iosys_map 
*map)
  {
-    struct ttm_resource *mem = bo->resource;
-
      dma_resv_assert_held(bo->base.resv);
        if (iosys_map_is_null(map))
          return;
  -    if (!map->is_iomem)
-        vunmap(map->vaddr);
-    else if (!mem->bus.addr)
+    switch (map->alloc_flags) {
+    case ttm_bo_map_iomap:
          iounmap(map->vaddr_iomem);
-    iosys_map_clear(map);
-
+        break;
+    case ttm_bo_map_vmap:
+        vunmap(map->vaddr);
+        break;
+    case ttm_bo_map_premapped:
+        break;
+    default:
+        drm_err(bo->base.dev, "Unsupported alloc_flags 0x%x\n", 
map->alloc_flags);
+        return;
+    }
      ttm_mem_io_free(bo->bdev, bo->resource);
+
+    iosys_map_clear(map);
  }
  EXPORT_SYMBOL(ttm_bo_vunmap);