On Wed, Oct 7, 2020 at 3:25 PM Christian König <christian.koenig@xxxxxxx> wrote:
>
> Am 07.10.20 um 15:20 schrieb Thomas Zimmermann:
> > Hi
> >
> > Am 07.10.20 um 15:10 schrieb Daniel Vetter:
> >> On Wed, Oct 7, 2020 at 2:57 PM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
> >>> Hi
> >>>
> >>> Am 02.10.20 um 11:58 schrieb Daniel Vetter:
> >>>> On Wed, Sep 30, 2020 at 02:51:46PM +0200, Daniel Vetter wrote:
> >>>>> On Wed, Sep 30, 2020 at 2:34 PM Christian König
> >>>>> <christian.koenig@xxxxxxx> wrote:
> >>>>>> Am 30.09.20 um 11:47 schrieb Daniel Vetter:
> >>>>>>> On Wed, Sep 30, 2020 at 10:34:31AM +0200, Christian König wrote:
> >>>>>>>> Am 30.09.20 um 10:19 schrieb Thomas Zimmermann:
> >>>>>>>>> Hi
> >>>>>>>>>
> >>>>>>>>> Am 30.09.20 um 10:05 schrieb Christian König:
> >>>>>>>>>> Am 29.09.20 um 19:49 schrieb Thomas Zimmermann:
> >>>>>>>>>>> Hi Christian
> >>>>>>>>>>>
> >>>>>>>>>>> Am 29.09.20 um 17:35 schrieb Christian König:
> >>>>>>>>>>>> Am 29.09.20 um 17:14 schrieb Thomas Zimmermann:
> >>>>>>>>>>>>> The new helper ttm_kmap_obj_to_dma_buf() extracts address and location
> >>>>>>>>>>>>> from and instance of TTM's kmap_obj and initializes struct dma_buf_map
> >>>>>>>>>>>>> with these values. Helpful for TTM-based drivers.
> >>>>>>>>>>>> We could completely drop that if we use the same structure inside TTM as
> >>>>>>>>>>>> well.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Additional to that which driver is going to use this?
> >>>>>>>>>>> As Daniel mentioned, it's in patch 3. The TTM-based drivers will
> >>>>>>>>>>> retrieve the pointer via this function.
> >>>>>>>>>>>
> >>>>>>>>>>> I do want to see all that being more tightly integrated into TTM, but
> >>>>>>>>>>> not in this series. This one is about fixing the bochs-on-sparc64
> >>>>>>>>>>> problem for good. Patch 7 adds an update to TTM to the DRM TODO list.
> >>>>>>>>>> I should have asked which driver you try to fix here :)
> >>>>>>>>>>
> >>>>>>>>>> In this case just keep the function inside bochs and only fix it there.
> >>>>>>>>>>
> >>>>>>>>>> All other drivers can be fixed when we generally pump this through TTM.
> >>>>>>>>> Did you take a look at patch 3? This function will be used by VRAM
> >>>>>>>>> helpers, nouveau, radeon, amdgpu and qxl. If we don't put it here, we
> >>>>>>>>> have to duplicate the functionality in each if these drivers. Bochs
> >>>>>>>>> itself uses VRAM helpers and doesn't touch the function directly.
> >>>>>>>> Ah, ok can we have that then only in the VRAM helpers?
> >>>>>>>>
> >>>>>>>> Alternative you could go ahead and use dma_buf_map in ttm_bo_kmap_obj
> >>>>>>>> directly and drop the hack with the TTM_BO_MAP_IOMEM_MASK.
> >>>>>>>>
> >>>>>>>> What I want to avoid is to have another conversion function in TTM because
> >>>>>>>> what happens here is that we already convert from ttm_bus_placement to
> >>>>>>>> ttm_bo_kmap_obj and then to dma_buf_map.
> >>>>>>> Hm I'm not really seeing how that helps with a gradual conversion of
> >>>>>>> everything over to dma_buf_map and assorted helpers for access? There's
> >>>>>>> too many places in ttm drivers where is_iomem and related stuff is used to
> >>>>>>> be able to convert it all in one go. An intermediate state with a bunch of
> >>>>>>> conversions seems fairly unavoidable to me.
> >>>>>> Fair enough. I would just have started bottom up and not top down.
> >>>>>>
> >>>>>> Anyway feel free to go ahead with this approach as long as we can remove
> >>>>>> the new function again when we clean that stuff up for good.
> >>>>> Yeah I guess bottom up would make more sense as a refactoring. But the
> >>>>> main motivation to land this here is to fix the __mmio vs normal
> >>>>> memory confusion in the fbdev emulation helpers for sparc (and
> >>>>> anything else that needs this). Hence the top down approach for
> >>>>> rolling this out.
> >>>> Ok I started reviewing this a bit more in-depth, and I think this is a bit
> >>>> too much of a de-tour.
> >>>>
> >>>> Looking through all the callers of ttm_bo_kmap almost everyone maps the
> >>>> entire object. Only vmwgfx uses to map less than that. Also, everyone just
> >>>> immediately follows up with converting that full object map into a
> >>>> pointer.
> >>>>
> >>>> So I think what we really want here is:
> >>>> - new function
> >>>>
> >>>> int ttm_bo_vmap(struct ttm_buffer_object *bo, struct dma_buf_map *map);
> >>>>
> >>>> _vmap name since that's consistent with both dma_buf functions and
> >>>> what's usually used to implement this. Outside of the ttm world kmap
> >>>> usually just means single-page mappings using kmap() or it's iomem
> >>>> sibling io_mapping_map* so rather confusing name for a function which
> >>>> usually is just used to set up a vmap of the entire buffer.
> >>>>
> >>>> - a helper which can be used for the drm_gem_object_funcs vmap/vunmap
> >>>> functions for all ttm drivers. We should be able to make this fully
> >>>> generic because a) we now have dma_buf_map and b) drm_gem_object is
> >>>> embedded in the ttm_bo, so we can upcast for everyone who's both a ttm
> >>>> and gem driver.
> >>>>
> >>>> This is maybe a good follow-up, since it should allow us to ditch quite
> >>>> a bit of the vram helper code for this more generic stuff. I also might
> >>>> have missed some special-cases here, but from a quick look everything
> >>>> just pins the buffer to the current location and that's it.
> >>>>
> >>>> Also this obviously requires Christian's generic ttm_bo_pin rework
> >>>> first.
> >>>>
> >>>> - roll the above out to drivers.
> >>>>
> >>>> Christian/Thomas, thoughts on this?
> >>> I agree on the goals, but what is the immediate objective here?
> >>>
> >>> Adding ttm_bo_vmap() does not work out easily, as struct ttm_bo_kmap_obj
> >>> is a central part of the internals of TTM. struct ttm_bo_kmap_obj has
> >>> more internal state that struct dma_buf_map, so they are not easily
> >>> convertible either. What you propose seems to require a reimplementation
> >>> of the existing ttm_bo_kmap() code. That is it's own patch series.
> >>>
> >>> I'd rather go with some variant of the existing patch and add
> >>> ttm_bo_vmap() in a follow-up.
> >> ttm_bo_vmap would simply wrap what you currently open-code as
> >> ttm_bo_kmap + ttm_kmap_obj_to_dma_buf_map. Removing ttm_kmap_obj would
> >> be a much later step. Why do you think adding ttm_bo_vmap is not
> >> possible?
> > The calls to ttm_bo_kmap/_kunmap() require an instance of struct
> > ttm_bo_kmap_obj that is stored in each driver's private bo structure
> > (e.g., struct drm_gem_vram_object, struct radeon_bo, etc). When I made
> > patch 3, I flirted with the idea of unifying the driver's _vmap code in
> > a shared helper, but I couldn't find a simple way of doing it. That's
> > why it's open-coded in the first place.
Yeah we'd need a ttm_bo_vunmap I guess to make this work. Which
shouldn't be more than a few lines, but maybe too much to do in this
series.
> Well that makes kind of sense. Keep in mind that ttm_bo_kmap is
> currently way to complicated.
Yeah, simplifying this into a ttm_bo_vmap on one side, and a simple
1-page kmap helper on the other should help a lot.
-Daniel
>
> Christian.
>
> >
> > Best regards
> > Thomas
> >
> >> -Daniel
> >>
> >>
> >>> Best regards
> >>> Thomas
> >>>
> >>>> I think for the immediate need of rolling this out for vram helpers and
> >>>> fbdev code we should be able to do this, but just postpone the driver wide
> >>>> roll-out for now.
> >>>>
> >>>> Cheers, Daniel
> >>>>
> >>>>> -Daniel
> >>>>>
> >>>>>> Christian.
> >>>>>>
> >>>>>>> -Daniel
> >>>>>>>
> >>>>>>>> Thanks,
> >>>>>>>> Christian.
> >>>>>>>>
> >>>>>>>>> Best regards
> >>>>>>>>> Thomas
> >>>>>>>>>
> >>>>>>>>>> Regards,
> >>>>>>>>>> Christian.
> >>>>>>>>>>
> >>>>>>>>>>> Best regards
> >>>>>>>>>>> Thomas
> >>>>>>>>>>>
> >>>>>>>>>>>> Regards,
> >>>>>>>>>>>> Christian.
> >>>>>>>>>>>>
> >>>>>>>>>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> >>>>>>>>>>>>> ---
> >>>>>>>>>>>>> include/drm/ttm/ttm_bo_api.h | 24 ++++++++++++++++++++++++
> >>>>>>>>>>>>> include/linux/dma-buf-map.h | 20 ++++++++++++++++++++
> >>>>>>>>>>>>> 2 files changed, 44 insertions(+)
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
> >>>>>>>>>>>>> index c96a25d571c8..62d89f05a801 100644
> >>>>>>>>>>>>> --- a/include/drm/ttm/ttm_bo_api.h
> >>>>>>>>>>>>> +++ b/include/drm/ttm/ttm_bo_api.h
> >>>>>>>>>>>>> @@ -34,6 +34,7 @@
> >>>>>>>>>>>>> #include <drm/drm_gem.h>
> >>>>>>>>>>>>> #include <drm/drm_hashtab.h>
> >>>>>>>>>>>>> #include <drm/drm_vma_manager.h>
> >>>>>>>>>>>>> +#include <linux/dma-buf-map.h>
> >>>>>>>>>>>>> #include <linux/kref.h>
> >>>>>>>>>>>>> #include <linux/list.h>
> >>>>>>>>>>>>> #include <linux/wait.h>
> >>>>>>>>>>>>> @@ -486,6 +487,29 @@ static inline void *ttm_kmap_obj_virtual(struct
> >>>>>>>>>>>>> ttm_bo_kmap_obj *map,
> >>>>>>>>>>>>> return map->virtual;
> >>>>>>>>>>>>> }
> >>>>>>>>>>>>> +/**
> >>>>>>>>>>>>> + * ttm_kmap_obj_to_dma_buf_map
> >>>>>>>>>>>>> + *
> >>>>>>>>>>>>> + * @kmap: A struct ttm_bo_kmap_obj returned from ttm_bo_kmap.
> >>>>>>>>>>>>> + * @map: Returns the mapping as struct dma_buf_map
> >>>>>>>>>>>>> + *
> >>>>>>>>>>>>> + * Converts struct ttm_bo_kmap_obj to struct dma_buf_map. If the memory
> >>>>>>>>>>>>> + * is not mapped, the returned mapping is initialized to NULL.
> >>>>>>>>>>>>> + */
> >>>>>>>>>>>>> +static inline void ttm_kmap_obj_to_dma_buf_map(struct ttm_bo_kmap_obj
> >>>>>>>>>>>>> *kmap,
> >>>>>>>>>>>>> + struct dma_buf_map *map)
> >>>>>>>>>>>>> +{
> >>>>>>>>>>>>> + bool is_iomem;
> >>>>>>>>>>>>> + void *vaddr = ttm_kmap_obj_virtual(kmap, &is_iomem);
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>> + if (!vaddr)
> >>>>>>>>>>>>> + dma_buf_map_clear(map);
> >>>>>>>>>>>>> + else if (is_iomem)
> >>>>>>>>>>>>> + dma_buf_map_set_vaddr_iomem(map, (void __force __iomem *)vaddr);
> >>>>>>>>>>>>> + else
> >>>>>>>>>>>>> + dma_buf_map_set_vaddr(map, vaddr);
> >>>>>>>>>>>>> +}
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>> /**
> >>>>>>>>>>>>> * ttm_bo_kmap
> >>>>>>>>>>>>> *
> >>>>>>>>>>>>> diff --git a/include/linux/dma-buf-map.h b/include/linux/dma-buf-map.h
> >>>>>>>>>>>>> index fd1aba545fdf..2e8bbecb5091 100644
> >>>>>>>>>>>>> --- a/include/linux/dma-buf-map.h
> >>>>>>>>>>>>> +++ b/include/linux/dma-buf-map.h
> >>>>>>>>>>>>> @@ -45,6 +45,12 @@
> >>>>>>>>>>>>> *
> >>>>>>>>>>>>> * dma_buf_map_set_vaddr(&map. 0xdeadbeaf);
> >>>>>>>>>>>>> *
> >>>>>>>>>>>>> + * To set an address in I/O memory, use dma_buf_map_set_vaddr_iomem().
> >>>>>>>>>>>>> + *
> >>>>>>>>>>>>> + * .. code-block:: c
> >>>>>>>>>>>>> + *
> >>>>>>>>>>>>> + * dma_buf_map_set_vaddr_iomem(&map. 0xdeadbeaf);
> >>>>>>>>>>>>> + *
> >>>>>>>>>>>>> * Test if a mapping is valid with either dma_buf_map_is_set() or
> >>>>>>>>>>>>> * dma_buf_map_is_null().
> >>>>>>>>>>>>> *
> >>>>>>>>>>>>> @@ -118,6 +124,20 @@ static inline void dma_buf_map_set_vaddr(struct
> >>>>>>>>>>>>> dma_buf_map *map, void *vaddr)
> >>>>>>>>>>>>> map->is_iomem = false;
> >>>>>>>>>>>>> }
> >>>>>>>>>>>>> +/**
> >>>>>>>>>>>>> + * dma_buf_map_set_vaddr_iomem - Sets a dma-buf mapping structure to
> >>>>>>>>>>>>> an address in I/O memory
> >>>>>>>>>>>>> + * @map: The dma-buf mapping structure
> >>>>>>>>>>>>> + * @vaddr_iomem: An I/O-memory address
> >>>>>>>>>>>>> + *
> >>>>>>>>>>>>> + * Sets the address and the I/O-memory flag.
> >>>>>>>>>>>>> + */
> >>>>>>>>>>>>> +static inline void dma_buf_map_set_vaddr_iomem(struct dma_buf_map *map,
> >>>>>>>>>>>>> + void __iomem *vaddr_iomem)
> >>>>>>>>>>>>> +{
> >>>>>>>>>>>>> + map->vaddr_iomem = vaddr_iomem;
> >>>>>>>>>>>>> + map->is_iomem = true;
> >>>>>>>>>>>>> +}
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>> /**
> >>>>>>>>>>>>> * dma_buf_map_is_equal - Compares two dma-buf mapping structures
> >>>>>>>>>>>>> for equality
> >>>>>>>>>>>>> * @lhs: The dma-buf mapping structure
> >>>>>>>>>>>> _______________________________________________
> >>>>>>>>>>>> dri-devel mailing list
> >>>>>>>>>>>> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> >>>>>>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&sdata=HdHOA%2F1VcIX%2F7YtfYTiAqYEvw7Ag%2FS%2BxS5VwJKOv5y0%3D&reserved=0
> >>>>>>>>>>> _______________________________________________
> >>>>>>>>>>> amd-gfx mailing list
> >>>>>>>>>>> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> >>>>>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&sdata=H%2B5HKCsTrksRV2EyEiFGSTyS79jsWCmJimSMoJYusx8%3D&reserved=0
> >>>>>>>>>> _______________________________________________
> >>>>>>>>>> dri-devel mailing list
> >>>>>>>>>> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> >>>>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&sdata=HdHOA%2F1VcIX%2F7YtfYTiAqYEvw7Ag%2FS%2BxS5VwJKOv5y0%3D&reserved=0
> >>>>>>>>>>
> >>>>>>>>> _______________________________________________
> >>>>>>>>> amd-gfx mailing list
> >>>>>>>>> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> >>>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&sdata=H%2B5HKCsTrksRV2EyEiFGSTyS79jsWCmJimSMoJYusx8%3D&reserved=0
> >>>>>
> >>>>> --
> >>>>> Daniel Vetter
> >>>>> Software Engineer, Intel Corporation
> >>>>> http://blog.ffwll.ch
> >>> --
> >>> Thomas Zimmermann
> >>> Graphics Driver Developer
> >>> SUSE Software Solutions Germany GmbH
> >>> Maxfeldstr. 5, 90409 Nürnberg, Germany
> >>> (HRB 36809, AG Nürnberg)
> >>> Geschäftsführer: Felix Imendörffer
> >>>
> >>
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
