Daniel Vetter
2020-Sep-30 09:47 UTC
[Nouveau] [PATCH v3 2/7] drm/ttm: Add ttm_kmap_obj_to_dma_buf_map() for type conversion
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. -Daniel> > Thanks, > Christian. > > > > > Best regards > > Thomas > > > > > Regards, > > > Christian. > > > > > > > Best regards > > > > Thomas > > > > > > > > > Regards, > > > > > Christian. > > > > > > > > > > > Signed-off-by: Thomas Zimmermann <tzimmermann at 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-devel at lists.freedesktop.org > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > _______________________________________________ > > > > amd-gfx mailing list > > > > amd-gfx at lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx > > > > > > _______________________________________________ > > > dri-devel mailing list > > > dri-devel at lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > > > > _______________________________________________ > > amd-gfx mailing list > > amd-gfx at lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx >-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Daniel Vetter
2020-Sep-30 12:51 UTC
[Nouveau] [PATCH v3 2/7] drm/ttm: Add ttm_kmap_obj_to_dma_buf_map() for type conversion
On Wed, Sep 30, 2020 at 2:34 PM Christian K?nig <christian.koenig at 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. -Daniel> > Christian. > > > -Daniel > > > >> Thanks, > >> Christian. > >> > >>> Best regards > >>> Thomas > >>> > >>>> Regards, > >>>> Christian. > >>>> > >>>>> Best regards > >>>>> Thomas > >>>>> > >>>>>> Regards, > >>>>>> Christian. > >>>>>> > >>>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann at 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-devel at 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-gfx at 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-devel at 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-gfx at 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
Possibly Parallel Threads
- [PATCH v3 2/7] drm/ttm: Add ttm_kmap_obj_to_dma_buf_map() for type conversion
- [PATCH v3 2/7] drm/ttm: Add ttm_kmap_obj_to_dma_buf_map() for type conversion
- [PATCH v3 2/7] drm/ttm: Add ttm_kmap_obj_to_dma_buf_map() for type conversion
- [PATCH v3 2/7] drm/ttm: Add ttm_kmap_obj_to_dma_buf_map() for type conversion
- [PATCH v3 2/7] drm/ttm: Add ttm_kmap_obj_to_dma_buf_map() for type conversion