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@amd.com 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@suse.de 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@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?
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@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@lists.freedesktop.org >>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.free... >>>>>>> _______________________________________________ >>>>>>> amd-gfx mailing list >>>>>>> amd-gfx@lists.freedesktop.org >>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.free... >>>>>> _______________________________________________ >>>>>> dri-devel mailing list >>>>>> dri-devel@lists.freedesktop.org >>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.free... >>>>>> >>>>> _______________________________________________ >>>>> amd-gfx mailing list >>>>> amd-gfx@lists.freedesktop.org >>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.free... > > -- > 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