On Fri, Oct 2, 2020 at 1:30 PM Christian König <ckoenig.leichtzumer...@gmail.com> wrote: > > 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.koe...@amd.com> 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? > > Calling this vmap instead of kmap certainly makes sense. > > Not 100% sure about the generic helpers, but it sounds like this should > indeed look rather clean in the end.
Yeah generic helper is probably better left for a later step, after we've rolled out ttm_bo_vmap out everywhere. -Daniel > > Christian. > > > > > 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 <tzimmerm...@suse.de> > >>>>>>>>>> --- > >>>>>>>>>> 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-de...@lists.freedesktop.org > >>>>>>>>> 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-...@lists.freedesktop.org > >>>>>>>> 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-de...@lists.freedesktop.org > >>>>>>> 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-...@lists.freedesktop.org > >>>>>> 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 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization