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

[Thomas Zimmermann <tzimmermann@suse.de>]

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.

Best regards
Thomas

>
> Regards,
> Christian.
>
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@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);
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)