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

Commit Message

Thomas Zimmermann Sept. 29, 2020, 3:14 p.m. UTC

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.

Signed-off-by: Thomas Zimmermann <[e-mail]>
---
 include/drm/ttm/ttm_bo_api.h | 24 ++++++++++++++++++++++++
 include/linux/dma-buf-map.h  | 20 ++++++++++++++++++++
 2 files changed, 44 insertions(+)

Comments

Christian König Sept. 29, 2020, 3:35 p.m. UTC | #1

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?

Regards,
Christian.

>
> Signed-off-by: Thomas Zimmermann <[e-mail]>
> ---
>   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

Daniel Vetter Sept. 29, 2020, 3:44 p.m. UTC | #2

On Tue, Sep 29, 2020 at 5:35 PM Christian König
<[e-mail]> wrote:
>
> 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?

Patch 3 in this series.

I also think this makes sense for gradual conversion:
1. add this helper
2. convert over all users of vmap, this should get rid of is_iomem
flags (and will probably result in a pile of small additions to
dma-buf-map.h)
3. push the struct dma_buf_map down into ttm drivers

Cheers, Daniel

> Regards,
> Christian.
>
> >
> > Signed-off-by: Thomas Zimmermann <[e-mail]>
> > ---
> >   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
>

Thomas Zimmermann Sept. 29, 2020, 5:49 p.m. UTC | #3

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.

Best regards
Thomas

> 
> Regards,
> Christian.
> 
>>
>> Signed-off-by: Thomas Zimmermann <[e-mail]>
>> ---
>>   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
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Christian König Sept. 30, 2020, 8:05 a.m. UTC | #4

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.

Regards,
Christian.

>
> Best regards
> Thomas
>
>> Regards,
>> Christian.
>>
>>> Signed-off-by: Thomas Zimmermann <[e-mail]>
>>> ---
>>>    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
>> [e-mail]
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> _______________________________________________
> amd-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Thomas Zimmermann Sept. 30, 2020, 8:19 a.m. UTC | #5

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.

Best regards
Thomas

> 
> Regards,
> Christian.
> 
>> Best regards
>> Thomas
>>
>>> Regards,
>>> Christian.
>>>
>>>> Signed-off-by: Thomas Zimmermann <[e-mail]>
>>>> ---
>>>>   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
>>> [e-mail]
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>> _______________________________________________
>> amd-gfx mailing list
>> [e-mail]
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 
> 
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Christian König Sept. 30, 2020, 8:34 a.m. UTC | #6

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.

Thanks,
Christian.

>
> Best regards
> Thomas
>
>> Regards,
>> Christian.
>>
>>> Best regards
>>> Thomas
>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> Signed-off-by: Thomas Zimmermann <[e-mail]>
>>>>> ---
>>>>>    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
>>>> [e-mail]
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>> _______________________________________________
>>> amd-gfx mailing list
>>> [e-mail]
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
>> _______________________________________________
>> dri-devel mailing list
>> [e-mail]
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>
> _______________________________________________
> amd-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Daniel Vetter Sept. 30, 2020, 9:47 a.m. UTC | #7

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 <[e-mail]>
> > > > > > ---
> > > > > >    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
> > > > > [e-mail]
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > _______________________________________________
> > > > amd-gfx mailing list
> > > > [e-mail]
> > > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > [e-mail]
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > 
> > 
> > _______________________________________________
> > amd-gfx mailing list
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>

Christian König Sept. 30, 2020, 12:34 p.m. UTC | #8

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.

Christian.

> -Daniel
>
>> Thanks,
>> Christian.
>>
>>> Best regards
>>> Thomas
>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> Best regards
>>>>> Thomas
>>>>>
>>>>>> Regards,
>>>>>> Christian.
>>>>>>
>>>>>>> Signed-off-by: Thomas Zimmermann <[e-mail]>
>>>>>>> ---
>>>>>>>     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
>>>>>> [e-mail]
>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=HdHOA%2F1VcIX%2F7YtfYTiAqYEvw7Ag%2FS%2BxS5VwJKOv5y0%3D&amp;reserved=0
>>>>> _______________________________________________
>>>>> amd-gfx mailing list
>>>>> [e-mail]
>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=H%2B5HKCsTrksRV2EyEiFGSTyS79jsWCmJimSMoJYusx8%3D&amp;reserved=0
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> [e-mail]
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=HdHOA%2F1VcIX%2F7YtfYTiAqYEvw7Ag%2FS%2BxS5VwJKOv5y0%3D&amp;reserved=0
>>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> [e-mail]
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=H%2B5HKCsTrksRV2EyEiFGSTyS79jsWCmJimSMoJYusx8%3D&amp;reserved=0

Daniel Vetter Sept. 30, 2020, 12:51 p.m. UTC | #9

On Wed, Sep 30, 2020 at 2:34 PM Christian König
<[e-mail]> 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 <[e-mail]>
> >>>>>>> ---
> >>>>>>>     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
> >>>>>> [e-mail]
> >>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=HdHOA%2F1VcIX%2F7YtfYTiAqYEvw7Ag%2FS%2BxS5VwJKOv5y0%3D&amp;reserved=0
> >>>>> _______________________________________________
> >>>>> amd-gfx mailing list
> >>>>> [e-mail]
> >>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=H%2B5HKCsTrksRV2EyEiFGSTyS79jsWCmJimSMoJYusx8%3D&amp;reserved=0
> >>>> _______________________________________________
> >>>> dri-devel mailing list
> >>>> [e-mail]
> >>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=HdHOA%2F1VcIX%2F7YtfYTiAqYEvw7Ag%2FS%2BxS5VwJKOv5y0%3D&amp;reserved=0
> >>>>
> >>> _______________________________________________
> >>> amd-gfx mailing list
> >>> [e-mail]
> >>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=H%2B5HKCsTrksRV2EyEiFGSTyS79jsWCmJimSMoJYusx8%3D&amp;reserved=0
>

Daniel Vetter Oct. 2, 2020, 9:58 a.m. UTC | #10

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
> <[e-mail]> 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 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 <[e-mail]>
> > >>>>>>> ---
> > >>>>>>>     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
> > >>>>>> [e-mail]
> > >>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=HdHOA%2F1VcIX%2F7YtfYTiAqYEvw7Ag%2FS%2BxS5VwJKOv5y0%3D&amp;reserved=0
> > >>>>> _______________________________________________
> > >>>>> amd-gfx mailing list
> > >>>>> [e-mail]
> > >>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=H%2B5HKCsTrksRV2EyEiFGSTyS79jsWCmJimSMoJYusx8%3D&amp;reserved=0
> > >>>> _______________________________________________
> > >>>> dri-devel mailing list
> > >>>> [e-mail]
> > >>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=HdHOA%2F1VcIX%2F7YtfYTiAqYEvw7Ag%2FS%2BxS5VwJKOv5y0%3D&amp;reserved=0
> > >>>>
> > >>> _______________________________________________
> > >>> amd-gfx mailing list
> > >>> [e-mail]
> > >>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=H%2B5HKCsTrksRV2EyEiFGSTyS79jsWCmJimSMoJYusx8%3D&amp;reserved=0
> >
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Christian König Oct. 2, 2020, 11:30 a.m. UTC | #11

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
>> <[e-mail]> 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.

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 <[e-mail]>
>>>>>>>>>> ---
>>>>>>>>>>      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
>>>>>>>>> [e-mail]
>>>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=HdHOA%2F1VcIX%2F7YtfYTiAqYEvw7Ag%2FS%2BxS5VwJKOv5y0%3D&amp;reserved=0
>>>>>>>> _______________________________________________
>>>>>>>> amd-gfx mailing list
>>>>>>>> [e-mail]
>>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=H%2B5HKCsTrksRV2EyEiFGSTyS79jsWCmJimSMoJYusx8%3D&amp;reserved=0
>>>>>>> _______________________________________________
>>>>>>> dri-devel mailing list
>>>>>>> [e-mail]
>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=HdHOA%2F1VcIX%2F7YtfYTiAqYEvw7Ag%2FS%2BxS5VwJKOv5y0%3D&amp;reserved=0
>>>>>>>
>>>>>> _______________________________________________
>>>>>> amd-gfx mailing list
>>>>>> [e-mail]
>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=H%2B5HKCsTrksRV2EyEiFGSTyS79jsWCmJimSMoJYusx8%3D&amp;reserved=0
>>
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch

Daniel Vetter Oct. 2, 2020, 12:21 p.m. UTC | #12

On Fri, Oct 2, 2020 at 1:30 PM Christian König
<[e-mail]> 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
> >> <[e-mail]> 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 <[e-mail]>
> >>>>>>>>>> ---
> >>>>>>>>>>      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
> >>>>>>>>> [e-mail]
> >>>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=HdHOA%2F1VcIX%2F7YtfYTiAqYEvw7Ag%2FS%2BxS5VwJKOv5y0%3D&amp;reserved=0
> >>>>>>>> _______________________________________________
> >>>>>>>> amd-gfx mailing list
> >>>>>>>> [e-mail]
> >>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=H%2B5HKCsTrksRV2EyEiFGSTyS79jsWCmJimSMoJYusx8%3D&amp;reserved=0
> >>>>>>> _______________________________________________
> >>>>>>> dri-devel mailing list
> >>>>>>> [e-mail]
> >>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=HdHOA%2F1VcIX%2F7YtfYTiAqYEvw7Ag%2FS%2BxS5VwJKOv5y0%3D&amp;reserved=0
> >>>>>>>
> >>>>>> _______________________________________________
> >>>>>> amd-gfx mailing list
> >>>>>> [e-mail]
> >>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=H%2B5HKCsTrksRV2EyEiFGSTyS79jsWCmJimSMoJYusx8%3D&amp;reserved=0
> >>
> >> --
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> http://blog.ffwll.ch
>

Thomas Zimmermann Oct. 7, 2020, 12:57 p.m. UTC | #13

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
>> <[e-mail]> 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.

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 <[e-mail]>
>>>>>>>>>> ---
>>>>>>>>>>     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
>>>>>>>>> [e-mail]
>>>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=HdHOA%2F1VcIX%2F7YtfYTiAqYEvw7Ag%2FS%2BxS5VwJKOv5y0%3D&amp;reserved=0
>>>>>>>> _______________________________________________
>>>>>>>> amd-gfx mailing list
>>>>>>>> [e-mail]
>>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=H%2B5HKCsTrksRV2EyEiFGSTyS79jsWCmJimSMoJYusx8%3D&amp;reserved=0
>>>>>>> _______________________________________________
>>>>>>> dri-devel mailing list
>>>>>>> [e-mail]
>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=HdHOA%2F1VcIX%2F7YtfYTiAqYEvw7Ag%2FS%2BxS5VwJKOv5y0%3D&amp;reserved=0
>>>>>>>
>>>>>> _______________________________________________
>>>>>> amd-gfx mailing list
>>>>>> [e-mail]
>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=H%2B5HKCsTrksRV2EyEiFGSTyS79jsWCmJimSMoJYusx8%3D&amp;reserved=0
>>>
>>
>>
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
>

Daniel Vetter Oct. 7, 2020, 1:10 p.m. UTC | #14

On Wed, Oct 7, 2020 at 2:57 PM Thomas Zimmermann <[e-mail]> 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
> >> <[e-mail]> 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?
-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 <[e-mail]>
> >>>>>>>>>> ---
> >>>>>>>>>>     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
> >>>>>>>>> [e-mail]
> >>>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=HdHOA%2F1VcIX%2F7YtfYTiAqYEvw7Ag%2FS%2BxS5VwJKOv5y0%3D&amp;reserved=0
> >>>>>>>> _______________________________________________
> >>>>>>>> amd-gfx mailing list
> >>>>>>>> [e-mail]
> >>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=H%2B5HKCsTrksRV2EyEiFGSTyS79jsWCmJimSMoJYusx8%3D&amp;reserved=0
> >>>>>>> _______________________________________________
> >>>>>>> dri-devel mailing list
> >>>>>>> [e-mail]
> >>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=HdHOA%2F1VcIX%2F7YtfYTiAqYEvw7Ag%2FS%2BxS5VwJKOv5y0%3D&amp;reserved=0
> >>>>>>>
> >>>>>> _______________________________________________
> >>>>>> amd-gfx mailing list
> >>>>>> [e-mail]
> >>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=H%2B5HKCsTrksRV2EyEiFGSTyS79jsWCmJimSMoJYusx8%3D&amp;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 Oct. 7, 2020, 1:20 p.m. UTC | #15

Hi

Am 07.10.20 um 15:10 schrieb Daniel Vetter:
> On Wed, Oct 7, 2020 at 2:57 PM Thomas Zimmermann <[e-mail]> 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
>>>> <[e-mail]> 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.

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 <[e-mail]>
>>>>>>>>>>>> ---
>>>>>>>>>>>>     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
>>>>>>>>>>> [e-mail]
>>>>>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=HdHOA%2F1VcIX%2F7YtfYTiAqYEvw7Ag%2FS%2BxS5VwJKOv5y0%3D&amp;reserved=0
>>>>>>>>>> _______________________________________________
>>>>>>>>>> amd-gfx mailing list
>>>>>>>>>> [e-mail]
>>>>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=H%2B5HKCsTrksRV2EyEiFGSTyS79jsWCmJimSMoJYusx8%3D&amp;reserved=0
>>>>>>>>> _______________________________________________
>>>>>>>>> dri-devel mailing list
>>>>>>>>> [e-mail]
>>>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=HdHOA%2F1VcIX%2F7YtfYTiAqYEvw7Ag%2FS%2BxS5VwJKOv5y0%3D&amp;reserved=0
>>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> amd-gfx mailing list
>>>>>>>> [e-mail]
>>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=H%2B5HKCsTrksRV2EyEiFGSTyS79jsWCmJimSMoJYusx8%3D&amp;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
>>
> 
>

Christian König Oct. 7, 2020, 1:24 p.m. UTC | #16

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 <[e-mail]> 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
>>>>> <[e-mail]> 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.

Well that makes kind of sense. Keep in mind that ttm_bo_kmap is 
currently way to complicated.

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 <[e-mail]>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>      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
>>>>>>>>>>>> [e-mail]
>>>>>>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=HdHOA%2F1VcIX%2F7YtfYTiAqYEvw7Ag%2FS%2BxS5VwJKOv5y0%3D&amp;reserved=0
>>>>>>>>>>> _______________________________________________
>>>>>>>>>>> amd-gfx mailing list
>>>>>>>>>>> [e-mail]
>>>>>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=H%2B5HKCsTrksRV2EyEiFGSTyS79jsWCmJimSMoJYusx8%3D&amp;reserved=0
>>>>>>>>>> _______________________________________________
>>>>>>>>>> dri-devel mailing list
>>>>>>>>>> [e-mail]
>>>>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=HdHOA%2F1VcIX%2F7YtfYTiAqYEvw7Ag%2FS%2BxS5VwJKOv5y0%3D&amp;reserved=0
>>>>>>>>>>
>>>>>>>>> _______________________________________________
>>>>>>>>> amd-gfx mailing list
>>>>>>>>> [e-mail]
>>>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=H%2B5HKCsTrksRV2EyEiFGSTyS79jsWCmJimSMoJYusx8%3D&amp;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 Oct. 7, 2020, 2:30 p.m. UTC | #17

On Wed, Oct 7, 2020 at 3:25 PM Christian König <[e-mail]> 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 <[e-mail]> 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
> >>>>> <[e-mail]> 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 <[e-mail]>
> >>>>>>>>>>>>> ---
> >>>>>>>>>>>>>      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
> >>>>>>>>>>>> [e-mail]
> >>>>>>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=HdHOA%2F1VcIX%2F7YtfYTiAqYEvw7Ag%2FS%2BxS5VwJKOv5y0%3D&amp;reserved=0
> >>>>>>>>>>> _______________________________________________
> >>>>>>>>>>> amd-gfx mailing list
> >>>>>>>>>>> [e-mail]
> >>>>>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=H%2B5HKCsTrksRV2EyEiFGSTyS79jsWCmJimSMoJYusx8%3D&amp;reserved=0
> >>>>>>>>>> _______________________________________________
> >>>>>>>>>> dri-devel mailing list
> >>>>>>>>>> [e-mail]
> >>>>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=HdHOA%2F1VcIX%2F7YtfYTiAqYEvw7Ag%2FS%2BxS5VwJKOv5y0%3D&amp;reserved=0
> >>>>>>>>>>
> >>>>>>>>> _______________________________________________
> >>>>>>>>> amd-gfx mailing list
> >>>>>>>>> [e-mail]
> >>>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=H%2B5HKCsTrksRV2EyEiFGSTyS79jsWCmJimSMoJYusx8%3D&amp;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 Oct. 8, 2020, 9 a.m. UTC | #18

Hi

Am 07.10.20 um 16:30 schrieb Daniel Vetter:
> On Wed, Oct 7, 2020 at 3:25 PM Christian König <[e-mail]> 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 <[e-mail]> 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
>>>>>>> <[e-mail]> 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 <[e-mail]>
>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>      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
>>>>>>>>>>>>>> [e-mail]
>>>>>>>>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=HdHOA%2F1VcIX%2F7YtfYTiAqYEvw7Ag%2FS%2BxS5VwJKOv5y0%3D&amp;reserved=0
>>>>>>>>>>>>> _______________________________________________
>>>>>>>>>>>>> amd-gfx mailing list
>>>>>>>>>>>>> [e-mail]
>>>>>>>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=H%2B5HKCsTrksRV2EyEiFGSTyS79jsWCmJimSMoJYusx8%3D&amp;reserved=0
>>>>>>>>>>>> _______________________________________________
>>>>>>>>>>>> dri-devel mailing list
>>>>>>>>>>>> [e-mail]
>>>>>>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=HdHOA%2F1VcIX%2F7YtfYTiAqYEvw7Ag%2FS%2BxS5VwJKOv5y0%3D&amp;reserved=0
>>>>>>>>>>>>
>>>>>>>>>>> _______________________________________________
>>>>>>>>>>> amd-gfx mailing list
>>>>>>>>>>> [e-mail]
>>>>>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C472c3d655a61411deb6708d86525d1b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637370560438965013&amp;sdata=H%2B5HKCsTrksRV2EyEiFGSTyS79jsWCmJimSMoJYusx8%3D&amp;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
>>>>>
>>>>
>>
> 
>

Patch

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