Re: [PATCH 3/6] drm/ttm: Support partial buffer mappings for ttm_bo_vmap()

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

Hi

Am 14.06.24 um 16:33 schrieb Christian König:
> Am 14.06.24 um 15:21 schrieb Thomas Zimmermann:
> > Add offset and size parameters to ttm_bo_vmap() to allow for partial
> > mappings of a buffer object. This brings the functionality on par with
> > ttm_bo_kmap().
>
> Well the long term plan was to remove this functionality from 
> ttm_bo_kmap() and nuke that function sooner or later.
>
> What exactly is the use case for partial mappings?

I don't have a use case; except for streamlining the DRM internals.

For the drivers, xe uses partial mappings in 
i915_gem_object_read_from_page(). But struct iosys_map to map the whole 
buffer and do memcpys within the range. See patch 6 for that. I think it 
could do without partial mappings. Vmwgfx uses a local mappings at [1], 
but as the nearby comment says, this could be a TTM helper. Or maybe TTM 
could provide a dedicated ttm_bo_kmap_local_page() for temporary 
mappings like this one. There's also [2], which looks like a candidate 
for a possible ttm_bo_kmap_local_page(). And in vmwgfx, there's the 
memcpy at [3], which could be replaced with ttm_bo_vmap() plus iosys_map 
helpers. I think that's it. Best regards Thomas [1] 
https://elixir.bootlin.com/linux/v6.9/source/drivers/gpu/drm/vmwgfx/vmwgfx_cotable.c#L457 
[2] 
https://elixir.bootlin.com/linux/v6.9/source/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c#L418 
[3] 
https://elixir.bootlin.com/linux/v6.9/source/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c#L430 

> Regards,
> Christian.
>
> > Callers pass the byte offset and size within the buffer object and
> > receive a page-aligned mapping of the buffer object's memory for the
> > specified area.
> >
> > Also update all callers of ttm_bo_vmap() for the new parameters. As
> > before, existing callers map the buffer object's complete memory.
> >
> > Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> > ---
> >   drivers/gpu/drm/drm_gem_ttm_helper.c  |  2 +-
> >   drivers/gpu/drm/drm_gem_vram_helper.c |  2 +-
> >   drivers/gpu/drm/loongson/lsdc_gem.c   |  2 +-
> >   drivers/gpu/drm/qxl/qxl_object.c      |  2 +-
> >   drivers/gpu/drm/ttm/ttm_bo_util.c     | 21 +++++++++++++++------
> >   drivers/gpu/drm/xe/xe_lrc.c           |  2 +-
> >   drivers/gpu/drm/xe/xe_vm.c            |  2 +-
> >   include/drm/ttm/ttm_bo.h              |  4 +++-
> >   8 files changed, 24 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_gem_ttm_helper.c 
> > b/drivers/gpu/drm/drm_gem_ttm_helper.c
> > index 3734aa2d1c5b5..f26b7c9077a68 100644
> > --- a/drivers/gpu/drm/drm_gem_ttm_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_ttm_helper.c
> > @@ -67,7 +67,7 @@ int drm_gem_ttm_vmap(struct drm_gem_object *gem,
> >   {
> >       struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
> >   -    return ttm_bo_vmap(bo, map);
> > +    return ttm_bo_vmap(bo, 0, gem->size, map);
> >   }
> >   EXPORT_SYMBOL(drm_gem_ttm_vmap);
> >   diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c 
> > b/drivers/gpu/drm/drm_gem_vram_helper.c
> > index 6027584406af6..1670f9a459a9d 100644
> > --- a/drivers/gpu/drm/drm_gem_vram_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
> > @@ -398,7 +398,7 @@ int drm_gem_vram_vmap(struct drm_gem_vram_object 
> > *gbo, struct iosys_map *map)
> >        * no mapping present.
> >        */
> >       if (iosys_map_is_null(&gbo->map)) {
> > -        ret = ttm_bo_vmap(&gbo->bo, &gbo->map);
> > +        ret = ttm_bo_vmap(&gbo->bo, 0, gbo->bo.base.size, &gbo->map);
> >           if (ret)
> >               return ret;
> >       }
> > diff --git a/drivers/gpu/drm/loongson/lsdc_gem.c 
> > b/drivers/gpu/drm/loongson/lsdc_gem.c
> > index a720d8f532093..f709960c781b9 100644
> > --- a/drivers/gpu/drm/loongson/lsdc_gem.c
> > +++ b/drivers/gpu/drm/loongson/lsdc_gem.c
> > @@ -77,7 +77,7 @@ static int lsdc_gem_object_vmap(struct 
> > drm_gem_object *obj, struct iosys_map *ma
> >           return ret;
> >       }
> >   -    ret = ttm_bo_vmap(tbo, &lbo->map);
> > +    ret = ttm_bo_vmap(tbo, 0, tbo->base.size, &lbo->map);
> >       if (ret) {
> >           drm_err(obj->dev, "ttm bo vmap failed\n");
> >           lsdc_bo_unpin(lbo);
> > diff --git a/drivers/gpu/drm/qxl/qxl_object.c 
> > b/drivers/gpu/drm/qxl/qxl_object.c
> > index 5893e27a7ae50..9f06d5e26a32c 100644
> > --- a/drivers/gpu/drm/qxl/qxl_object.c
> > +++ b/drivers/gpu/drm/qxl/qxl_object.c
> > @@ -164,7 +164,7 @@ int qxl_bo_vmap_locked(struct qxl_bo *bo, struct 
> > iosys_map *map)
> >           goto out;
> >       }
> >   -    r = ttm_bo_vmap(&bo->tbo, &bo->map);
> > +    r = ttm_bo_vmap(&bo->tbo, 0, bo->tbo.base.size, &bo->map);
> >       if (r) {
> >           qxl_bo_unpin_locked(bo);
> >           return r;
> > diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c 
> > b/drivers/gpu/drm/ttm/ttm_bo_util.c
> > index a9df0deff2deb..31f9772f05dac 100644
> > --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
> > +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
> > @@ -457,17 +457,23 @@ EXPORT_SYMBOL(ttm_bo_kunmap);
> >    * ttm_bo_vmap
> >    *
> >    * @bo: The buffer object.
> > + * @offset: Byte offset into the buffer.
> > + * @size: Number of bytes to map.
> >    * @map: pointer to a struct iosys_map representing the map.
> >    *
> >    * Sets up a kernel virtual mapping, using ioremap or vmap to the
> >    * data in the buffer object. The parameter @map returns the virtual
> >    * address as struct iosys_map. Unmap the buffer with ttm_bo_vunmap().
> > + * The address stored in @map will be aligned to the next lower page
> > + * boundaries.
> >    *
> >    * Returns
> >    * -ENOMEM: Out of memory.
> >    * -EINVAL: Invalid range.
> >    */
> > -int ttm_bo_vmap(struct ttm_buffer_object *bo, struct iosys_map *map)
> > +int ttm_bo_vmap(struct ttm_buffer_object *bo,
> > +        unsigned long offset, unsigned long size,
> > +        struct iosys_map *map)
> >   {
> >       struct ttm_resource *mem = bo->resource;
> >       int ret;
> > @@ -483,18 +489,18 @@ int ttm_bo_vmap(struct ttm_buffer_object *bo, 
> > struct iosys_map *map)
> >           u16 alloc_flags;
> >             if (mem->bus.addr) {
> > -            vaddr_iomem = (void __iomem *)mem->bus.addr;
> > +            vaddr_iomem = (u8 __iomem *)mem->bus.addr + offset;
> >               alloc_flags = ttm_bo_map_premapped;
> >           } else if (mem->bus.caching == ttm_write_combined) {
> > -            vaddr_iomem = ioremap_wc(mem->bus.offset, bo->base.size);
> > +            vaddr_iomem = ioremap_wc(mem->bus.offset + offset, 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);
> > +            vaddr_iomem = ioremap_cache(mem->bus.offset + offset, 
> > size);
> >               alloc_flags = ttm_bo_map_iomap;
> >   #endif
> >           } else {
> > -            vaddr_iomem = ioremap(mem->bus.offset, bo->base.size);
> > +            vaddr_iomem = ioremap(mem->bus.offset + offset, size);
> >               alloc_flags = ttm_bo_map_iomap;
> >           }
> >   @@ -510,6 +516,9 @@ int ttm_bo_vmap(struct ttm_buffer_object *bo, 
> > struct iosys_map *map)
> >               .no_wait_gpu = false
> >           };
> >           struct ttm_tt *ttm = bo->ttm;
> > +        unsigned long start_page = offset >> PAGE_SHIFT;
> > +        unsigned long aligned_size = size + (offset - (start_page << 
> > PAGE_SHIFT));
> > +        unsigned long num_pages = DIV_ROUND_UP(aligned_size, 
> > PAGE_SIZE);
> >           pgprot_t prot;
> >           void *vaddr;
> >           u16 alloc_flags;
> > @@ -523,7 +532,7 @@ int ttm_bo_vmap(struct ttm_buffer_object *bo, 
> > struct iosys_map *map)
> >            * or to make the buffer object look contiguous.
> >            */
> >           prot = ttm_io_prot(bo, mem, PAGE_KERNEL);
> > -        vaddr = vmap(ttm->pages, ttm->num_pages, 0, prot);
> > +        vaddr = vmap(ttm->pages + start_page, num_pages, 0, prot);
> >           if (!vaddr)
> >               return -ENOMEM;
> >           alloc_flags = ttm_bo_map_vmap;
> > diff --git a/drivers/gpu/drm/xe/xe_lrc.c b/drivers/gpu/drm/xe/xe_lrc.c
> > index c1bb85d2e243f..3a68fe6d592ed 100644
> > --- a/drivers/gpu/drm/xe/xe_lrc.c
> > +++ b/drivers/gpu/drm/xe/xe_lrc.c
> > @@ -1595,7 +1595,7 @@ void xe_lrc_snapshot_capture_delayed(struct 
> > xe_lrc_snapshot *snapshot)
> >           goto put_bo;
> >         xe_bo_lock(bo, false);
> > -    if (!ttm_bo_vmap(&bo->ttm, &src)) {
> > +    if (!ttm_bo_vmap(&bo->ttm, 0, bo->ttm.base.size, &src)) {
> >           xe_map_memcpy_from(xe_bo_device(bo),
> >                      snapshot->lrc_snapshot, &src, snapshot->lrc_offset,
> >                      snapshot->lrc_size);
> > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > index 99bf7412475c0..81306c32f5d09 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.c
> > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > @@ -3485,7 +3485,7 @@ void xe_vm_snapshot_capture_delayed(struct 
> > xe_vm_snapshot *snap)
> >             if (bo) {
> >               xe_bo_lock(bo, false);
> > -            err = ttm_bo_vmap(&bo->ttm, &src);
> > +            err = ttm_bo_vmap(&bo->ttm, 0, bo->ttm.base.size, &src);
> >               if (!err) {
> >                   xe_map_memcpy_from(xe_bo_device(bo),
> >                              snap->snap[i].data,
> > diff --git a/include/drm/ttm/ttm_bo.h b/include/drm/ttm/ttm_bo.h
> > index 6ccf96c91f3ae..c421ffe3563b1 100644
> > --- a/include/drm/ttm/ttm_bo.h
> > +++ b/include/drm/ttm/ttm_bo.h
> > @@ -375,7 +375,9 @@ int ttm_bo_init_validate(struct ttm_device *bdev, 
> > struct ttm_buffer_object *bo,
> >   int ttm_bo_kmap(struct ttm_buffer_object *bo, unsigned long 
> > start_page,
> >           unsigned long num_pages, struct ttm_bo_kmap_obj *map);
> >   void ttm_bo_kunmap(struct ttm_bo_kmap_obj *map);
> > -int ttm_bo_vmap(struct ttm_buffer_object *bo, struct iosys_map *map);
> > +int ttm_bo_vmap(struct ttm_buffer_object *bo,
> > +        unsigned long offset, unsigned long size,
> > +        struct iosys_map *map);
> >   void ttm_bo_vunmap(struct ttm_buffer_object *bo, struct iosys_map 
> > *map);
> >   int ttm_bo_mmap_obj(struct vm_area_struct *vma, struct 
> > ttm_buffer_object *bo);
> >   int ttm_bo_swapout(struct ttm_buffer_object *bo, struct 
> > ttm_operation_ctx *ctx,

--
--
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)