Xen project Mailing List

Hi Am 07.10.20 um 16:30 schrieb Daniel Vetter: > 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. I'm not too happy about the plan, but I'll send out something like this in the next iteration. Best regards Thomas > -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 >>>>> >>>> >> > > -- 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

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.

Re: [PATCH v3 2/7] drm/ttm: Add ttm_kmap_obj_to_dma_buf_map() for type conversion