mbox series

[0/3] dma-buf: Flag vmap'ed memory as system or I/O memory

Message ID 20200914112521.1327-1-tzimmermann@suse.de (mailing list archive)
Headers show
Series dma-buf: Flag vmap'ed memory as system or I/O memory | expand

Message

Thomas Zimmermann Sept. 14, 2020, 11:25 a.m. UTC
Dma-buf provides vmap() and vunmap() for retrieving and releasing mappings
of dma-buf memory in kernel address space. The functions operate with plain
addresses and the assumption is that the memory can be accessed with load
and store operations. This is not the case on some architectures (e.g.,
sparc64) where I/O memory can only be accessed with dedicated instructions.

This patchset introduces struct dma_buf_map, which contains the address of
a buffer and a flag that tells whether system- or I/O-memory instructions
are required.

Some background: updating the DRM framebuffer console on sparc64 makes the
kernel panic. This is because the framebuffer memory cannot be accessed with
system-memory instructions. We currently employ a workaround in DRM to
address this specific problem. [1]

To resolve the problem, we'd like to address it at the most common point,
which is the dma-buf framework. The dma-buf mapping ideally knows if I/O
instructions are required and exports this information to it's users. The
new structure struct dma_buf_map stores the buffer address and a flag that
signals I/O memory. Affected users of the buffer (e.g., drivers, frameworks)
can then access the memory accordingly.

This patchset only introduces struct dma_buf_map, and updates struct dma_buf
and it's interfaces. Further patches can update dma-buf users. For example,
there's a prototype patchset for DRM that fixes the framebuffer problem. [2]

Further work: TTM, one of DRM's memory managers, already exports an
is_iomem flag of its own. It could later be switched over to exporting struct
dma_buf_map, thus simplifying some code. Several DRM drivers expect their
fbdev console to operate on I/O memory. These could possibly be switched over
to the generic fbdev emulation, as soon as the generic code uses struct
dma_buf_map.

[1] https://lore.kernel.org/dri-devel/20200725191012.GA434957@ravnborg.org/
[2] https://lore.kernel.org/dri-devel/20200806085239.4606-1-tzimmermann@suse.de/

Thomas Zimmermann (3):
  dma-buf: Add struct dma-buf-map for storing struct dma_buf.vaddr_ptr
  dma-buf: Use struct dma_buf_map in dma_buf_vmap() interfaces
  dma-buf: Use struct dma_buf_map in dma_buf_vunmap() interfaces

 Documentation/driver-api/dma-buf.rst          |   3 +
 drivers/dma-buf/dma-buf.c                     |  40 +++---
 drivers/gpu/drm/drm_gem_cma_helper.c          |  16 ++-
 drivers/gpu/drm/drm_gem_shmem_helper.c        |  17 ++-
 drivers/gpu/drm/drm_prime.c                   |  14 +-
 drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c   |  13 +-
 drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c    |  13 +-
 .../drm/i915/gem/selftests/i915_gem_dmabuf.c  |  18 ++-
 drivers/gpu/drm/tegra/gem.c                   |  23 ++--
 .../common/videobuf2/videobuf2-dma-contig.c   |  17 ++-
 .../media/common/videobuf2/videobuf2-dma-sg.c |  19 ++-
 .../common/videobuf2/videobuf2-vmalloc.c      |  21 ++-
 include/drm/drm_prime.h                       |   5 +-
 include/linux/dma-buf-map.h                   | 126 ++++++++++++++++++
 include/linux/dma-buf.h                       |  11 +-
 15 files changed, 274 insertions(+), 82 deletions(-)
 create mode 100644 include/linux/dma-buf-map.h

--
2.28.0

Comments

Daniel Vetter Sept. 16, 2020, 9:37 a.m. UTC | #1
On Mon, Sep 14, 2020 at 01:25:18PM +0200, Thomas Zimmermann wrote:
> Dma-buf provides vmap() and vunmap() for retrieving and releasing mappings
> of dma-buf memory in kernel address space. The functions operate with plain
> addresses and the assumption is that the memory can be accessed with load
> and store operations. This is not the case on some architectures (e.g.,
> sparc64) where I/O memory can only be accessed with dedicated instructions.
> 
> This patchset introduces struct dma_buf_map, which contains the address of
> a buffer and a flag that tells whether system- or I/O-memory instructions
> are required.
> 
> Some background: updating the DRM framebuffer console on sparc64 makes the
> kernel panic. This is because the framebuffer memory cannot be accessed with
> system-memory instructions. We currently employ a workaround in DRM to
> address this specific problem. [1]
> 
> To resolve the problem, we'd like to address it at the most common point,
> which is the dma-buf framework. The dma-buf mapping ideally knows if I/O
> instructions are required and exports this information to it's users. The
> new structure struct dma_buf_map stores the buffer address and a flag that
> signals I/O memory. Affected users of the buffer (e.g., drivers, frameworks)
> can then access the memory accordingly.
> 
> This patchset only introduces struct dma_buf_map, and updates struct dma_buf
> and it's interfaces. Further patches can update dma-buf users. For example,
> there's a prototype patchset for DRM that fixes the framebuffer problem. [2]
> 
> Further work: TTM, one of DRM's memory managers, already exports an
> is_iomem flag of its own. It could later be switched over to exporting struct
> dma_buf_map, thus simplifying some code. Several DRM drivers expect their
> fbdev console to operate on I/O memory. These could possibly be switched over
> to the generic fbdev emulation, as soon as the generic code uses struct
> dma_buf_map.
> 
> [1] https://lore.kernel.org/dri-devel/20200725191012.GA434957@ravnborg.org/
> [2] https://lore.kernel.org/dri-devel/20200806085239.4606-1-tzimmermann@suse.de/

lgtm, imo ready to convert the follow-up patches over to this. But I think
would be good to get at least some ack from the ttm side for the overall
plan.

Also, I think we should put all the various helpers (writel/readl, memset,
memcpy, whatever else) into the dma-buf-map.h helper, so that most code
using this can just treat it as an abstract pointer type and never look
underneath it.
-Daniel

> 
> Thomas Zimmermann (3):
>   dma-buf: Add struct dma-buf-map for storing struct dma_buf.vaddr_ptr
>   dma-buf: Use struct dma_buf_map in dma_buf_vmap() interfaces
>   dma-buf: Use struct dma_buf_map in dma_buf_vunmap() interfaces
> 
>  Documentation/driver-api/dma-buf.rst          |   3 +
>  drivers/dma-buf/dma-buf.c                     |  40 +++---
>  drivers/gpu/drm/drm_gem_cma_helper.c          |  16 ++-
>  drivers/gpu/drm/drm_gem_shmem_helper.c        |  17 ++-
>  drivers/gpu/drm/drm_prime.c                   |  14 +-
>  drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c   |  13 +-
>  drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c    |  13 +-
>  .../drm/i915/gem/selftests/i915_gem_dmabuf.c  |  18 ++-
>  drivers/gpu/drm/tegra/gem.c                   |  23 ++--
>  .../common/videobuf2/videobuf2-dma-contig.c   |  17 ++-
>  .../media/common/videobuf2/videobuf2-dma-sg.c |  19 ++-
>  .../common/videobuf2/videobuf2-vmalloc.c      |  21 ++-
>  include/drm/drm_prime.h                       |   5 +-
>  include/linux/dma-buf-map.h                   | 126 ++++++++++++++++++
>  include/linux/dma-buf.h                       |  11 +-
>  15 files changed, 274 insertions(+), 82 deletions(-)
>  create mode 100644 include/linux/dma-buf-map.h
> 
> --
> 2.28.0
>
Thomas Zimmermann Sept. 16, 2020, 10:48 a.m. UTC | #2
Hi

Am 16.09.20 um 11:37 schrieb Daniel Vetter:
> On Mon, Sep 14, 2020 at 01:25:18PM +0200, Thomas Zimmermann wrote:
>> Dma-buf provides vmap() and vunmap() for retrieving and releasing mappings
>> of dma-buf memory in kernel address space. The functions operate with plain
>> addresses and the assumption is that the memory can be accessed with load
>> and store operations. This is not the case on some architectures (e.g.,
>> sparc64) where I/O memory can only be accessed with dedicated instructions.
>>
>> This patchset introduces struct dma_buf_map, which contains the address of
>> a buffer and a flag that tells whether system- or I/O-memory instructions
>> are required.
>>
>> Some background: updating the DRM framebuffer console on sparc64 makes the
>> kernel panic. This is because the framebuffer memory cannot be accessed with
>> system-memory instructions. We currently employ a workaround in DRM to
>> address this specific problem. [1]
>>
>> To resolve the problem, we'd like to address it at the most common point,
>> which is the dma-buf framework. The dma-buf mapping ideally knows if I/O
>> instructions are required and exports this information to it's users. The
>> new structure struct dma_buf_map stores the buffer address and a flag that
>> signals I/O memory. Affected users of the buffer (e.g., drivers, frameworks)
>> can then access the memory accordingly.
>>
>> This patchset only introduces struct dma_buf_map, and updates struct dma_buf
>> and it's interfaces. Further patches can update dma-buf users. For example,
>> there's a prototype patchset for DRM that fixes the framebuffer problem. [2]
>>
>> Further work: TTM, one of DRM's memory managers, already exports an
>> is_iomem flag of its own. It could later be switched over to exporting struct
>> dma_buf_map, thus simplifying some code. Several DRM drivers expect their
>> fbdev console to operate on I/O memory. These could possibly be switched over
>> to the generic fbdev emulation, as soon as the generic code uses struct
>> dma_buf_map.
>>
>> [1] https://lore.kernel.org/dri-devel/20200725191012.GA434957@ravnborg.org/
>> [2] https://lore.kernel.org/dri-devel/20200806085239.4606-1-tzimmermann@suse.de/
> 
> lgtm, imo ready to convert the follow-up patches over to this. But I think
> would be good to get at least some ack from the ttm side for the overall
> plan.

Yup, it would be nice if TTM could had out these types automatically.
Then all TTM-based drivers would automatically support it.

> 
> Also, I think we should put all the various helpers (writel/readl, memset,
> memcpy, whatever else) into the dma-buf-map.h helper, so that most code
> using this can just treat it as an abstract pointer type and never look
> underneath it.

We have some framebuffer helpers that rely on pointer arithmetic, so
we'd need that too. No big deal wrt code, but I was worried about the
overhead. If a loop goes over framebuffer memory, there's an if/else
branch for each access to the memory buffer.

Best regards
Thomas

> -Daniel
> 
>>
>> Thomas Zimmermann (3):
>>   dma-buf: Add struct dma-buf-map for storing struct dma_buf.vaddr_ptr
>>   dma-buf: Use struct dma_buf_map in dma_buf_vmap() interfaces
>>   dma-buf: Use struct dma_buf_map in dma_buf_vunmap() interfaces
>>
>>  Documentation/driver-api/dma-buf.rst          |   3 +
>>  drivers/dma-buf/dma-buf.c                     |  40 +++---
>>  drivers/gpu/drm/drm_gem_cma_helper.c          |  16 ++-
>>  drivers/gpu/drm/drm_gem_shmem_helper.c        |  17 ++-
>>  drivers/gpu/drm/drm_prime.c                   |  14 +-
>>  drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c   |  13 +-
>>  drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c    |  13 +-
>>  .../drm/i915/gem/selftests/i915_gem_dmabuf.c  |  18 ++-
>>  drivers/gpu/drm/tegra/gem.c                   |  23 ++--
>>  .../common/videobuf2/videobuf2-dma-contig.c   |  17 ++-
>>  .../media/common/videobuf2/videobuf2-dma-sg.c |  19 ++-
>>  .../common/videobuf2/videobuf2-vmalloc.c      |  21 ++-
>>  include/drm/drm_prime.h                       |   5 +-
>>  include/linux/dma-buf-map.h                   | 126 ++++++++++++++++++
>>  include/linux/dma-buf.h                       |  11 +-
>>  15 files changed, 274 insertions(+), 82 deletions(-)
>>  create mode 100644 include/linux/dma-buf-map.h
>>
>> --
>> 2.28.0
>>
>
Daniel Vetter Sept. 16, 2020, 12:24 p.m. UTC | #3
On Wed, Sep 16, 2020 at 12:48:20PM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 16.09.20 um 11:37 schrieb Daniel Vetter:
> > On Mon, Sep 14, 2020 at 01:25:18PM +0200, Thomas Zimmermann wrote:
> >> Dma-buf provides vmap() and vunmap() for retrieving and releasing mappings
> >> of dma-buf memory in kernel address space. The functions operate with plain
> >> addresses and the assumption is that the memory can be accessed with load
> >> and store operations. This is not the case on some architectures (e.g.,
> >> sparc64) where I/O memory can only be accessed with dedicated instructions.
> >>
> >> This patchset introduces struct dma_buf_map, which contains the address of
> >> a buffer and a flag that tells whether system- or I/O-memory instructions
> >> are required.
> >>
> >> Some background: updating the DRM framebuffer console on sparc64 makes the
> >> kernel panic. This is because the framebuffer memory cannot be accessed with
> >> system-memory instructions. We currently employ a workaround in DRM to
> >> address this specific problem. [1]
> >>
> >> To resolve the problem, we'd like to address it at the most common point,
> >> which is the dma-buf framework. The dma-buf mapping ideally knows if I/O
> >> instructions are required and exports this information to it's users. The
> >> new structure struct dma_buf_map stores the buffer address and a flag that
> >> signals I/O memory. Affected users of the buffer (e.g., drivers, frameworks)
> >> can then access the memory accordingly.
> >>
> >> This patchset only introduces struct dma_buf_map, and updates struct dma_buf
> >> and it's interfaces. Further patches can update dma-buf users. For example,
> >> there's a prototype patchset for DRM that fixes the framebuffer problem. [2]
> >>
> >> Further work: TTM, one of DRM's memory managers, already exports an
> >> is_iomem flag of its own. It could later be switched over to exporting struct
> >> dma_buf_map, thus simplifying some code. Several DRM drivers expect their
> >> fbdev console to operate on I/O memory. These could possibly be switched over
> >> to the generic fbdev emulation, as soon as the generic code uses struct
> >> dma_buf_map.
> >>
> >> [1] https://lore.kernel.org/dri-devel/20200725191012.GA434957@ravnborg.org/
> >> [2] https://lore.kernel.org/dri-devel/20200806085239.4606-1-tzimmermann@suse.de/
> > 
> > lgtm, imo ready to convert the follow-up patches over to this. But I think
> > would be good to get at least some ack from the ttm side for the overall
> > plan.
> 
> Yup, it would be nice if TTM could had out these types automatically.
> Then all TTM-based drivers would automatically support it.
> 
> > 
> > Also, I think we should put all the various helpers (writel/readl, memset,
> > memcpy, whatever else) into the dma-buf-map.h helper, so that most code
> > using this can just treat it as an abstract pointer type and never look
> > underneath it.
> 
> We have some framebuffer helpers that rely on pointer arithmetic, so
> we'd need that too. No big deal wrt code, but I was worried about the
> overhead. If a loop goes over framebuffer memory, there's an if/else
> branch for each access to the memory buffer.

If we make all the helpers static inline, then the compiler should be able
to see that dma_buf_map.is_iomem is always the same, and produced really
optimized code for it by pulling that check out from all the loops.

So should only result in somewhat verbose code of having to call
dma_buf_map pointer arthimetic helpers, but not in bad generated code.
Still worth double-checking I think, since e.g. on x86 the generated code
should be the same for both cases (but maybe the compiler doesn't see
through the inline asm to realize that, so we might end up with 2 copies).
-Daniel


> 
> Best regards
> Thomas
> 
> > -Daniel
> > 
> >>
> >> Thomas Zimmermann (3):
> >>   dma-buf: Add struct dma-buf-map for storing struct dma_buf.vaddr_ptr
> >>   dma-buf: Use struct dma_buf_map in dma_buf_vmap() interfaces
> >>   dma-buf: Use struct dma_buf_map in dma_buf_vunmap() interfaces
> >>
> >>  Documentation/driver-api/dma-buf.rst          |   3 +
> >>  drivers/dma-buf/dma-buf.c                     |  40 +++---
> >>  drivers/gpu/drm/drm_gem_cma_helper.c          |  16 ++-
> >>  drivers/gpu/drm/drm_gem_shmem_helper.c        |  17 ++-
> >>  drivers/gpu/drm/drm_prime.c                   |  14 +-
> >>  drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c   |  13 +-
> >>  drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c    |  13 +-
> >>  .../drm/i915/gem/selftests/i915_gem_dmabuf.c  |  18 ++-
> >>  drivers/gpu/drm/tegra/gem.c                   |  23 ++--
> >>  .../common/videobuf2/videobuf2-dma-contig.c   |  17 ++-
> >>  .../media/common/videobuf2/videobuf2-dma-sg.c |  19 ++-
> >>  .../common/videobuf2/videobuf2-vmalloc.c      |  21 ++-
> >>  include/drm/drm_prime.h                       |   5 +-
> >>  include/linux/dma-buf-map.h                   | 126 ++++++++++++++++++
> >>  include/linux/dma-buf.h                       |  11 +-
> >>  15 files changed, 274 insertions(+), 82 deletions(-)
> >>  create mode 100644 include/linux/dma-buf-map.h
> >>
> >> --
> >> 2.28.0
> >>
> > 
> 
> -- 
> 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 Sept. 16, 2020, 12:59 p.m. UTC | #4
Am 16.09.20 um 14:24 schrieb Daniel Vetter:
> On Wed, Sep 16, 2020 at 12:48:20PM +0200, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 16.09.20 um 11:37 schrieb Daniel Vetter:
>>> On Mon, Sep 14, 2020 at 01:25:18PM +0200, Thomas Zimmermann wrote:
>>>> Dma-buf provides vmap() and vunmap() for retrieving and releasing mappings
>>>> of dma-buf memory in kernel address space. The functions operate with plain
>>>> addresses and the assumption is that the memory can be accessed with load
>>>> and store operations. This is not the case on some architectures (e.g.,
>>>> sparc64) where I/O memory can only be accessed with dedicated instructions.
>>>>
>>>> This patchset introduces struct dma_buf_map, which contains the address of
>>>> a buffer and a flag that tells whether system- or I/O-memory instructions
>>>> are required.
>>>>
>>>> Some background: updating the DRM framebuffer console on sparc64 makes the
>>>> kernel panic. This is because the framebuffer memory cannot be accessed with
>>>> system-memory instructions. We currently employ a workaround in DRM to
>>>> address this specific problem. [1]
>>>>
>>>> To resolve the problem, we'd like to address it at the most common point,
>>>> which is the dma-buf framework. The dma-buf mapping ideally knows if I/O
>>>> instructions are required and exports this information to it's users. The
>>>> new structure struct dma_buf_map stores the buffer address and a flag that
>>>> signals I/O memory. Affected users of the buffer (e.g., drivers, frameworks)
>>>> can then access the memory accordingly.
>>>>
>>>> This patchset only introduces struct dma_buf_map, and updates struct dma_buf
>>>> and it's interfaces. Further patches can update dma-buf users. For example,
>>>> there's a prototype patchset for DRM that fixes the framebuffer problem. [2]
>>>>
>>>> Further work: TTM, one of DRM's memory managers, already exports an
>>>> is_iomem flag of its own. It could later be switched over to exporting struct
>>>> dma_buf_map, thus simplifying some code. Several DRM drivers expect their
>>>> fbdev console to operate on I/O memory. These could possibly be switched over
>>>> to the generic fbdev emulation, as soon as the generic code uses struct
>>>> dma_buf_map.
>>>>
>>>> [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fdri-devel%2F20200725191012.GA434957%40ravnborg.org%2F&data=02%7C01%7Cchristian.koenig%40amd.com%7C04e3cc3e03ae40f1fa0f08d85a3b6a68%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637358558524732385&sdata=wTmFuB95GhKUU%2F2Q91V0%2BtzAu4%2BEe3VBUcriBy3jx2g%3D&reserved=0
>>>> [2] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fdri-devel%2F20200806085239.4606-1-tzimmermann%40suse.de%2F&data=02%7C01%7Cchristian.koenig%40amd.com%7C04e3cc3e03ae40f1fa0f08d85a3b6a68%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637358558524732385&sdata=L4rBHmegO63b%2FiTQdTyH158KNxAZwSuJCQOaFszo5L0%3D&reserved=0
>>> lgtm, imo ready to convert the follow-up patches over to this. But I think
>>> would be good to get at least some ack from the ttm side for the overall
>>> plan.
>> Yup, it would be nice if TTM could had out these types automatically.
>> Then all TTM-based drivers would automatically support it.
>>
>>> Also, I think we should put all the various helpers (writel/readl, memset,
>>> memcpy, whatever else) into the dma-buf-map.h helper, so that most code
>>> using this can just treat it as an abstract pointer type and never look
>>> underneath it.
>> We have some framebuffer helpers that rely on pointer arithmetic, so
>> we'd need that too. No big deal wrt code, but I was worried about the
>> overhead. If a loop goes over framebuffer memory, there's an if/else
>> branch for each access to the memory buffer.
> If we make all the helpers static inline, then the compiler should be able
> to see that dma_buf_map.is_iomem is always the same, and produced really
> optimized code for it by pulling that check out from all the loops.
>
> So should only result in somewhat verbose code of having to call
> dma_buf_map pointer arthimetic helpers, but not in bad generated code.
> Still worth double-checking I think, since e.g. on x86 the generated code
> should be the same for both cases (but maybe the compiler doesn't see
> through the inline asm to realize that, so we might end up with 2 copies).

Can we have that even independent of DMA-buf? We have essentially the 
same problem in TTM and the code around that is a complete mess if you 
ask me.

Christian.

> -Daniel
>
>
>> Best regards
>> Thomas
>>
>>> -Daniel
>>>
>>>> Thomas Zimmermann (3):
>>>>    dma-buf: Add struct dma-buf-map for storing struct dma_buf.vaddr_ptr
>>>>    dma-buf: Use struct dma_buf_map in dma_buf_vmap() interfaces
>>>>    dma-buf: Use struct dma_buf_map in dma_buf_vunmap() interfaces
>>>>
>>>>   Documentation/driver-api/dma-buf.rst          |   3 +
>>>>   drivers/dma-buf/dma-buf.c                     |  40 +++---
>>>>   drivers/gpu/drm/drm_gem_cma_helper.c          |  16 ++-
>>>>   drivers/gpu/drm/drm_gem_shmem_helper.c        |  17 ++-
>>>>   drivers/gpu/drm/drm_prime.c                   |  14 +-
>>>>   drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c   |  13 +-
>>>>   drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c    |  13 +-
>>>>   .../drm/i915/gem/selftests/i915_gem_dmabuf.c  |  18 ++-
>>>>   drivers/gpu/drm/tegra/gem.c                   |  23 ++--
>>>>   .../common/videobuf2/videobuf2-dma-contig.c   |  17 ++-
>>>>   .../media/common/videobuf2/videobuf2-dma-sg.c |  19 ++-
>>>>   .../common/videobuf2/videobuf2-vmalloc.c      |  21 ++-
>>>>   include/drm/drm_prime.h                       |   5 +-
>>>>   include/linux/dma-buf-map.h                   | 126 ++++++++++++++++++
>>>>   include/linux/dma-buf.h                       |  11 +-
>>>>   15 files changed, 274 insertions(+), 82 deletions(-)
>>>>   create mode 100644 include/linux/dma-buf-map.h
>>>>
>>>> --
>>>> 2.28.0
>>>>
>> -- 
>> 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 Sept. 16, 2020, 1:12 p.m. UTC | #5
Hi

Am 16.09.20 um 14:59 schrieb Christian König:
> Am 16.09.20 um 14:24 schrieb Daniel Vetter:
>> On Wed, Sep 16, 2020 at 12:48:20PM +0200, Thomas Zimmermann wrote:
>>> Hi
>>>
>>> Am 16.09.20 um 11:37 schrieb Daniel Vetter:
>>>> On Mon, Sep 14, 2020 at 01:25:18PM +0200, Thomas Zimmermann wrote:
>>>>> Dma-buf provides vmap() and vunmap() for retrieving and releasing
>>>>> mappings
>>>>> of dma-buf memory in kernel address space. The functions operate
>>>>> with plain
>>>>> addresses and the assumption is that the memory can be accessed
>>>>> with load
>>>>> and store operations. This is not the case on some architectures
>>>>> (e.g.,
>>>>> sparc64) where I/O memory can only be accessed with dedicated
>>>>> instructions.
>>>>>
>>>>> This patchset introduces struct dma_buf_map, which contains the
>>>>> address of
>>>>> a buffer and a flag that tells whether system- or I/O-memory
>>>>> instructions
>>>>> are required.
>>>>>
>>>>> Some background: updating the DRM framebuffer console on sparc64
>>>>> makes the
>>>>> kernel panic. This is because the framebuffer memory cannot be
>>>>> accessed with
>>>>> system-memory instructions. We currently employ a workaround in DRM to
>>>>> address this specific problem. [1]
>>>>>
>>>>> To resolve the problem, we'd like to address it at the most common
>>>>> point,
>>>>> which is the dma-buf framework. The dma-buf mapping ideally knows
>>>>> if I/O
>>>>> instructions are required and exports this information to it's
>>>>> users. The
>>>>> new structure struct dma_buf_map stores the buffer address and a
>>>>> flag that
>>>>> signals I/O memory. Affected users of the buffer (e.g., drivers,
>>>>> frameworks)
>>>>> can then access the memory accordingly.
>>>>>
>>>>> This patchset only introduces struct dma_buf_map, and updates
>>>>> struct dma_buf
>>>>> and it's interfaces. Further patches can update dma-buf users. For
>>>>> example,
>>>>> there's a prototype patchset for DRM that fixes the framebuffer
>>>>> problem. [2]
>>>>>
>>>>> Further work: TTM, one of DRM's memory managers, already exports an
>>>>> is_iomem flag of its own. It could later be switched over to
>>>>> exporting struct
>>>>> dma_buf_map, thus simplifying some code. Several DRM drivers expect
>>>>> their
>>>>> fbdev console to operate on I/O memory. These could possibly be
>>>>> switched over
>>>>> to the generic fbdev emulation, as soon as the generic code uses
>>>>> struct
>>>>> dma_buf_map.
>>>>>
>>>>> [1]
>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fdri-devel%2F20200725191012.GA434957%40ravnborg.org%2F&data=02%7C01%7Cchristian.koenig%40amd.com%7C04e3cc3e03ae40f1fa0f08d85a3b6a68%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637358558524732385&sdata=wTmFuB95GhKUU%2F2Q91V0%2BtzAu4%2BEe3VBUcriBy3jx2g%3D&reserved=0
>>>>>
>>>>> [2]
>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fdri-devel%2F20200806085239.4606-1-tzimmermann%40suse.de%2F&data=02%7C01%7Cchristian.koenig%40amd.com%7C04e3cc3e03ae40f1fa0f08d85a3b6a68%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637358558524732385&sdata=L4rBHmegO63b%2FiTQdTyH158KNxAZwSuJCQOaFszo5L0%3D&reserved=0
>>>>>
>>>> lgtm, imo ready to convert the follow-up patches over to this. But I
>>>> think
>>>> would be good to get at least some ack from the ttm side for the
>>>> overall
>>>> plan.
>>> Yup, it would be nice if TTM could had out these types automatically.
>>> Then all TTM-based drivers would automatically support it.
>>>
>>>> Also, I think we should put all the various helpers (writel/readl,
>>>> memset,
>>>> memcpy, whatever else) into the dma-buf-map.h helper, so that most code
>>>> using this can just treat it as an abstract pointer type and never look
>>>> underneath it.
>>> We have some framebuffer helpers that rely on pointer arithmetic, so
>>> we'd need that too. No big deal wrt code, but I was worried about the
>>> overhead. If a loop goes over framebuffer memory, there's an if/else
>>> branch for each access to the memory buffer.
>> If we make all the helpers static inline, then the compiler should be
>> able
>> to see that dma_buf_map.is_iomem is always the same, and produced really
>> optimized code for it by pulling that check out from all the loops.
>>
>> So should only result in somewhat verbose code of having to call
>> dma_buf_map pointer arthimetic helpers, but not in bad generated code.
>> Still worth double-checking I think, since e.g. on x86 the generated code
>> should be the same for both cases (but maybe the compiler doesn't see
>> through the inline asm to realize that, so we might end up with 2
>> copies).
> 
> Can we have that even independent of DMA-buf? We have essentially the
> same problem in TTM and the code around that is a complete mess if you
> ask me.

I already put this into dma-buf because it's at the intersection of all
the affected modules. For non-dma-buf pointers (say in framebuffer
damage handling), the idea is to initialize struct dma_buf_map by hand
and use this.

Where would you want to put it?

Best regards
Thomas

> 
> Christian.
> 
>> -Daniel
>>
>>
>>> Best regards
>>> Thomas
>>>
>>>> -Daniel
>>>>
>>>>> Thomas Zimmermann (3):
>>>>>    dma-buf: Add struct dma-buf-map for storing struct
>>>>> dma_buf.vaddr_ptr
>>>>>    dma-buf: Use struct dma_buf_map in dma_buf_vmap() interfaces
>>>>>    dma-buf: Use struct dma_buf_map in dma_buf_vunmap() interfaces
>>>>>
>>>>>   Documentation/driver-api/dma-buf.rst          |   3 +
>>>>>   drivers/dma-buf/dma-buf.c                     |  40 +++---
>>>>>   drivers/gpu/drm/drm_gem_cma_helper.c          |  16 ++-
>>>>>   drivers/gpu/drm/drm_gem_shmem_helper.c        |  17 ++-
>>>>>   drivers/gpu/drm/drm_prime.c                   |  14 +-
>>>>>   drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c   |  13 +-
>>>>>   drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c    |  13 +-
>>>>>   .../drm/i915/gem/selftests/i915_gem_dmabuf.c  |  18 ++-
>>>>>   drivers/gpu/drm/tegra/gem.c                   |  23 ++--
>>>>>   .../common/videobuf2/videobuf2-dma-contig.c   |  17 ++-
>>>>>   .../media/common/videobuf2/videobuf2-dma-sg.c |  19 ++-
>>>>>   .../common/videobuf2/videobuf2-vmalloc.c      |  21 ++-
>>>>>   include/drm/drm_prime.h                       |   5 +-
>>>>>   include/linux/dma-buf-map.h                   | 126
>>>>> ++++++++++++++++++
>>>>>   include/linux/dma-buf.h                       |  11 +-
>>>>>   15 files changed, 274 insertions(+), 82 deletions(-)
>>>>>   create mode 100644 include/linux/dma-buf-map.h
>>>>>
>>>>> -- 
>>>>> 2.28.0
>>>>>
>>> -- 
>>> 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
>>>
>>
>>
>>
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Thomas Hellström (Intel) Sept. 16, 2020, 1:37 p.m. UTC | #6
On 9/16/20 2:59 PM, Christian König wrote:
> Am 16.09.20 um 14:24 schrieb Daniel Vetter:
>> On Wed, Sep 16, 2020 at 12:48:20PM +0200, Thomas Zimmermann wrote:
>>> Hi
>>>
>>> Am 16.09.20 um 11:37 schrieb Daniel Vetter:
>>>> On Mon, Sep 14, 2020 at 01:25:18PM +0200, Thomas Zimmermann wrote:
>>>>> Dma-buf provides vmap() and vunmap() for retrieving and releasing 
>>>>> mappings
>>>>> of dma-buf memory in kernel address space. The functions operate 
>>>>> with plain
>>>>> addresses and the assumption is that the memory can be accessed 
>>>>> with load
>>>>> and store operations. This is not the case on some architectures 
>>>>> (e.g.,
>>>>> sparc64) where I/O memory can only be accessed with dedicated 
>>>>> instructions.
>>>>>
>>>>> This patchset introduces struct dma_buf_map, which contains the 
>>>>> address of
>>>>> a buffer and a flag that tells whether system- or I/O-memory 
>>>>> instructions
>>>>> are required.
>>>>>
>>>>> Some background: updating the DRM framebuffer console on sparc64 
>>>>> makes the
>>>>> kernel panic. This is because the framebuffer memory cannot be 
>>>>> accessed with
>>>>> system-memory instructions. We currently employ a workaround in 
>>>>> DRM to
>>>>> address this specific problem. [1]
>>>>>
>>>>> To resolve the problem, we'd like to address it at the most common 
>>>>> point,
>>>>> which is the dma-buf framework. The dma-buf mapping ideally knows 
>>>>> if I/O
>>>>> instructions are required and exports this information to it's 
>>>>> users. The
>>>>> new structure struct dma_buf_map stores the buffer address and a 
>>>>> flag that
>>>>> signals I/O memory. Affected users of the buffer (e.g., drivers, 
>>>>> frameworks)
>>>>> can then access the memory accordingly.
>>>>>
>>>>> This patchset only introduces struct dma_buf_map, and updates 
>>>>> struct dma_buf
>>>>> and it's interfaces. Further patches can update dma-buf users. For 
>>>>> example,
>>>>> there's a prototype patchset for DRM that fixes the framebuffer 
>>>>> problem. [2]
>>>>>
>>>>> Further work: TTM, one of DRM's memory managers, already exports an
>>>>> is_iomem flag of its own. It could later be switched over to 
>>>>> exporting struct
>>>>> dma_buf_map, thus simplifying some code. Several DRM drivers 
>>>>> expect their
>>>>> fbdev console to operate on I/O memory. These could possibly be 
>>>>> switched over
>>>>> to the generic fbdev emulation, as soon as the generic code uses 
>>>>> struct
>>>>> dma_buf_map.
>>>>>
>>>>> [1] 
>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fdri-devel%2F20200725191012.GA434957%40ravnborg.org%2F&data=02%7C01%7Cchristian.koenig%40amd.com%7C04e3cc3e03ae40f1fa0f08d85a3b6a68%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637358558524732385&sdata=wTmFuB95GhKUU%2F2Q91V0%2BtzAu4%2BEe3VBUcriBy3jx2g%3D&reserved=0
>>>>> [2] 
>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fdri-devel%2F20200806085239.4606-1-tzimmermann%40suse.de%2F&data=02%7C01%7Cchristian.koenig%40amd.com%7C04e3cc3e03ae40f1fa0f08d85a3b6a68%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637358558524732385&sdata=L4rBHmegO63b%2FiTQdTyH158KNxAZwSuJCQOaFszo5L0%3D&reserved=0
>>>> lgtm, imo ready to convert the follow-up patches over to this. But 
>>>> I think
>>>> would be good to get at least some ack from the ttm side for the 
>>>> overall
>>>> plan.
>>> Yup, it would be nice if TTM could had out these types automatically.
>>> Then all TTM-based drivers would automatically support it.
>>>
>>>> Also, I think we should put all the various helpers (writel/readl, 
>>>> memset,
>>>> memcpy, whatever else) into the dma-buf-map.h helper, so that most 
>>>> code
>>>> using this can just treat it as an abstract pointer type and never 
>>>> look
>>>> underneath it.
>>> We have some framebuffer helpers that rely on pointer arithmetic, so
>>> we'd need that too. No big deal wrt code, but I was worried about the
>>> overhead. If a loop goes over framebuffer memory, there's an if/else
>>> branch for each access to the memory buffer.
>> If we make all the helpers static inline, then the compiler should be 
>> able
>> to see that dma_buf_map.is_iomem is always the same, and produced really
>> optimized code for it by pulling that check out from all the loops.
>>
>> So should only result in somewhat verbose code of having to call
>> dma_buf_map pointer arthimetic helpers, but not in bad generated code.
>> Still worth double-checking I think, since e.g. on x86 the generated 
>> code
>> should be the same for both cases (but maybe the compiler doesn't see
>> through the inline asm to realize that, so we might end up with 2 
>> copies).
>
> Can we have that even independent of DMA-buf? We have essentially the 
> same problem in TTM and the code around that is a complete mess if you 
> ask me.
>
> Christian.
>
I think this patchset looks good. Changing ttm_bo_kmap() over to 
returning a struct dma-buf-map would probably work just fine. If we then 
can have a set of helpers to operate on it, that's great.

/Thomas
Thomas Zimmermann Sept. 17, 2020, 7:16 a.m. UTC | #7
Hi Christian and Thomas

Am 16.09.20 um 15:37 schrieb Thomas Hellström (Intel):
> 
> On 9/16/20 2:59 PM, Christian König wrote:
>> Am 16.09.20 um 14:24 schrieb Daniel Vetter:
>>> On Wed, Sep 16, 2020 at 12:48:20PM +0200, Thomas Zimmermann wrote:
>>>> Hi
>>>>
>>>> Am 16.09.20 um 11:37 schrieb Daniel Vetter:
>>>>> On Mon, Sep 14, 2020 at 01:25:18PM +0200, Thomas Zimmermann wrote:
>>>>>> Dma-buf provides vmap() and vunmap() for retrieving and releasing
>>>>>> mappings
>>>>>> of dma-buf memory in kernel address space. The functions operate
>>>>>> with plain
>>>>>> addresses and the assumption is that the memory can be accessed
>>>>>> with load
>>>>>> and store operations. This is not the case on some architectures
>>>>>> (e.g.,
>>>>>> sparc64) where I/O memory can only be accessed with dedicated
>>>>>> instructions.
>>>>>>
>>>>>> This patchset introduces struct dma_buf_map, which contains the
>>>>>> address of
>>>>>> a buffer and a flag that tells whether system- or I/O-memory
>>>>>> instructions
>>>>>> are required.
>>>>>>
>>>>>> Some background: updating the DRM framebuffer console on sparc64
>>>>>> makes the
>>>>>> kernel panic. This is because the framebuffer memory cannot be
>>>>>> accessed with
>>>>>> system-memory instructions. We currently employ a workaround in
>>>>>> DRM to
>>>>>> address this specific problem. [1]
>>>>>>
>>>>>> To resolve the problem, we'd like to address it at the most common
>>>>>> point,
>>>>>> which is the dma-buf framework. The dma-buf mapping ideally knows
>>>>>> if I/O
>>>>>> instructions are required and exports this information to it's
>>>>>> users. The
>>>>>> new structure struct dma_buf_map stores the buffer address and a
>>>>>> flag that
>>>>>> signals I/O memory. Affected users of the buffer (e.g., drivers,
>>>>>> frameworks)
>>>>>> can then access the memory accordingly.
>>>>>>
>>>>>> This patchset only introduces struct dma_buf_map, and updates
>>>>>> struct dma_buf
>>>>>> and it's interfaces. Further patches can update dma-buf users. For
>>>>>> example,
>>>>>> there's a prototype patchset for DRM that fixes the framebuffer
>>>>>> problem. [2]
>>>>>>
>>>>>> Further work: TTM, one of DRM's memory managers, already exports an
>>>>>> is_iomem flag of its own. It could later be switched over to
>>>>>> exporting struct
>>>>>> dma_buf_map, thus simplifying some code. Several DRM drivers
>>>>>> expect their
>>>>>> fbdev console to operate on I/O memory. These could possibly be
>>>>>> switched over
>>>>>> to the generic fbdev emulation, as soon as the generic code uses
>>>>>> struct
>>>>>> dma_buf_map.
>>>>>>
>>>>>> [1]
>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fdri-devel%2F20200725191012.GA434957%40ravnborg.org%2F&data=02%7C01%7Cchristian.koenig%40amd.com%7C04e3cc3e03ae40f1fa0f08d85a3b6a68%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637358558524732385&sdata=wTmFuB95GhKUU%2F2Q91V0%2BtzAu4%2BEe3VBUcriBy3jx2g%3D&reserved=0
>>>>>>
>>>>>> [2]
>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fdri-devel%2F20200806085239.4606-1-tzimmermann%40suse.de%2F&data=02%7C01%7Cchristian.koenig%40amd.com%7C04e3cc3e03ae40f1fa0f08d85a3b6a68%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637358558524732385&sdata=L4rBHmegO63b%2FiTQdTyH158KNxAZwSuJCQOaFszo5L0%3D&reserved=0
>>>>>>
>>>>> lgtm, imo ready to convert the follow-up patches over to this. But
>>>>> I think
>>>>> would be good to get at least some ack from the ttm side for the
>>>>> overall
>>>>> plan.
>>>> Yup, it would be nice if TTM could had out these types automatically.
>>>> Then all TTM-based drivers would automatically support it.
>>>>
>>>>> Also, I think we should put all the various helpers (writel/readl,
>>>>> memset,
>>>>> memcpy, whatever else) into the dma-buf-map.h helper, so that most
>>>>> code
>>>>> using this can just treat it as an abstract pointer type and never
>>>>> look
>>>>> underneath it.
>>>> We have some framebuffer helpers that rely on pointer arithmetic, so
>>>> we'd need that too. No big deal wrt code, but I was worried about the
>>>> overhead. If a loop goes over framebuffer memory, there's an if/else
>>>> branch for each access to the memory buffer.
>>> If we make all the helpers static inline, then the compiler should be
>>> able
>>> to see that dma_buf_map.is_iomem is always the same, and produced really
>>> optimized code for it by pulling that check out from all the loops.
>>>
>>> So should only result in somewhat verbose code of having to call
>>> dma_buf_map pointer arthimetic helpers, but not in bad generated code.
>>> Still worth double-checking I think, since e.g. on x86 the generated
>>> code
>>> should be the same for both cases (but maybe the compiler doesn't see
>>> through the inline asm to realize that, so we might end up with 2
>>> copies).
>>
>> Can we have that even independent of DMA-buf? We have essentially the
>> same problem in TTM and the code around that is a complete mess if you
>> ask me.
>>
>> Christian.
>>
> I think this patchset looks good. Changing ttm_bo_kmap() over to
> returning a struct dma-buf-map would probably work just fine. If we then
> can have a set of helpers to operate on it, that's great.
> 
> /Thomas

Can I count this as an A-b by one of you?

Best regards
Thomas

> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Christian König Sept. 17, 2020, 8:04 a.m. UTC | #8
Am 17.09.20 um 09:16 schrieb Thomas Zimmermann:
> Hi Christian and Thomas
>
> Am 16.09.20 um 15:37 schrieb Thomas Hellström (Intel):
>> On 9/16/20 2:59 PM, Christian König wrote:
>>> Am 16.09.20 um 14:24 schrieb Daniel Vetter:
>>>> On Wed, Sep 16, 2020 at 12:48:20PM +0200, Thomas Zimmermann wrote:
>>>>> Hi
>>>>>
>>>>> Am 16.09.20 um 11:37 schrieb Daniel Vetter:
>>>>>> On Mon, Sep 14, 2020 at 01:25:18PM +0200, Thomas Zimmermann wrote:
>>>>>>> Dma-buf provides vmap() and vunmap() for retrieving and releasing
>>>>>>> mappings
>>>>>>> of dma-buf memory in kernel address space. The functions operate
>>>>>>> with plain
>>>>>>> addresses and the assumption is that the memory can be accessed
>>>>>>> with load
>>>>>>> and store operations. This is not the case on some architectures
>>>>>>> (e.g.,
>>>>>>> sparc64) where I/O memory can only be accessed with dedicated
>>>>>>> instructions.
>>>>>>>
>>>>>>> This patchset introduces struct dma_buf_map, which contains the
>>>>>>> address of
>>>>>>> a buffer and a flag that tells whether system- or I/O-memory
>>>>>>> instructions
>>>>>>> are required.
>>>>>>>
>>>>>>> Some background: updating the DRM framebuffer console on sparc64
>>>>>>> makes the
>>>>>>> kernel panic. This is because the framebuffer memory cannot be
>>>>>>> accessed with
>>>>>>> system-memory instructions. We currently employ a workaround in
>>>>>>> DRM to
>>>>>>> address this specific problem. [1]
>>>>>>>
>>>>>>> To resolve the problem, we'd like to address it at the most common
>>>>>>> point,
>>>>>>> which is the dma-buf framework. The dma-buf mapping ideally knows
>>>>>>> if I/O
>>>>>>> instructions are required and exports this information to it's
>>>>>>> users. The
>>>>>>> new structure struct dma_buf_map stores the buffer address and a
>>>>>>> flag that
>>>>>>> signals I/O memory. Affected users of the buffer (e.g., drivers,
>>>>>>> frameworks)
>>>>>>> can then access the memory accordingly.
>>>>>>>
>>>>>>> This patchset only introduces struct dma_buf_map, and updates
>>>>>>> struct dma_buf
>>>>>>> and it's interfaces. Further patches can update dma-buf users. For
>>>>>>> example,
>>>>>>> there's a prototype patchset for DRM that fixes the framebuffer
>>>>>>> problem. [2]
>>>>>>>
>>>>>>> Further work: TTM, one of DRM's memory managers, already exports an
>>>>>>> is_iomem flag of its own. It could later be switched over to
>>>>>>> exporting struct
>>>>>>> dma_buf_map, thus simplifying some code. Several DRM drivers
>>>>>>> expect their
>>>>>>> fbdev console to operate on I/O memory. These could possibly be
>>>>>>> switched over
>>>>>>> to the generic fbdev emulation, as soon as the generic code uses
>>>>>>> struct
>>>>>>> dma_buf_map.
>>>>>>>
>>>>>>> [1]
>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fdri-devel%2F20200725191012.GA434957%40ravnborg.org%2F&data=02%7C01%7Cchristian.koenig%40amd.com%7C04e3cc3e03ae40f1fa0f08d85a3b6a68%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637358558524732385&sdata=wTmFuB95GhKUU%2F2Q91V0%2BtzAu4%2BEe3VBUcriBy3jx2g%3D&reserved=0
>>>>>>>
>>>>>>> [2]
>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fdri-devel%2F20200806085239.4606-1-tzimmermann%40suse.de%2F&data=02%7C01%7Cchristian.koenig%40amd.com%7C04e3cc3e03ae40f1fa0f08d85a3b6a68%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637358558524732385&sdata=L4rBHmegO63b%2FiTQdTyH158KNxAZwSuJCQOaFszo5L0%3D&reserved=0
>>>>>>>
>>>>>> lgtm, imo ready to convert the follow-up patches over to this. But
>>>>>> I think
>>>>>> would be good to get at least some ack from the ttm side for the
>>>>>> overall
>>>>>> plan.
>>>>> Yup, it would be nice if TTM could had out these types automatically.
>>>>> Then all TTM-based drivers would automatically support it.
>>>>>
>>>>>> Also, I think we should put all the various helpers (writel/readl,
>>>>>> memset,
>>>>>> memcpy, whatever else) into the dma-buf-map.h helper, so that most
>>>>>> code
>>>>>> using this can just treat it as an abstract pointer type and never
>>>>>> look
>>>>>> underneath it.
>>>>> We have some framebuffer helpers that rely on pointer arithmetic, so
>>>>> we'd need that too. No big deal wrt code, but I was worried about the
>>>>> overhead. If a loop goes over framebuffer memory, there's an if/else
>>>>> branch for each access to the memory buffer.
>>>> If we make all the helpers static inline, then the compiler should be
>>>> able
>>>> to see that dma_buf_map.is_iomem is always the same, and produced really
>>>> optimized code for it by pulling that check out from all the loops.
>>>>
>>>> So should only result in somewhat verbose code of having to call
>>>> dma_buf_map pointer arthimetic helpers, but not in bad generated code.
>>>> Still worth double-checking I think, since e.g. on x86 the generated
>>>> code
>>>> should be the same for both cases (but maybe the compiler doesn't see
>>>> through the inline asm to realize that, so we might end up with 2
>>>> copies).
>>> Can we have that even independent of DMA-buf? We have essentially the
>>> same problem in TTM and the code around that is a complete mess if you
>>> ask me.
>>>
>>> Christian.
>>>
>> I think this patchset looks good. Changing ttm_bo_kmap() over to
>> returning a struct dma-buf-map would probably work just fine. If we then
>> can have a set of helpers to operate on it, that's great.
>>
>> /Thomas
> Can I count this as an A-b by one of you?

For the the general approach, certainly yes.

Regards,
Christian.

>
> Best regards
> Thomas
>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Sumit Semwal Sept. 18, 2020, 6:06 a.m. UTC | #9
Hello Thomas,

On Mon, 14 Sep 2020 at 16:55, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Dma-buf provides vmap() and vunmap() for retrieving and releasing mappings
> of dma-buf memory in kernel address space. The functions operate with plain
> addresses and the assumption is that the memory can be accessed with load
> and store operations. This is not the case on some architectures (e.g.,
> sparc64) where I/O memory can only be accessed with dedicated instructions.
>
> This patchset introduces struct dma_buf_map, which contains the address of
> a buffer and a flag that tells whether system- or I/O-memory instructions
> are required.

Thank you for the patchset - it is a really nice, clean bit to add!
>
> Some background: updating the DRM framebuffer console on sparc64 makes the
> kernel panic. This is because the framebuffer memory cannot be accessed with
> system-memory instructions. We currently employ a workaround in DRM to
> address this specific problem. [1]
>
> To resolve the problem, we'd like to address it at the most common point,
> which is the dma-buf framework. The dma-buf mapping ideally knows if I/O
> instructions are required and exports this information to it's users. The
> new structure struct dma_buf_map stores the buffer address and a flag that
> signals I/O memory. Affected users of the buffer (e.g., drivers, frameworks)
> can then access the memory accordingly.
>
> This patchset only introduces struct dma_buf_map, and updates struct dma_buf
> and it's interfaces. Further patches can update dma-buf users. For example,
> there's a prototype patchset for DRM that fixes the framebuffer problem. [2]
>
> Further work: TTM, one of DRM's memory managers, already exports an
> is_iomem flag of its own. It could later be switched over to exporting struct
> dma_buf_map, thus simplifying some code. Several DRM drivers expect their
> fbdev console to operate on I/O memory. These could possibly be switched over
> to the generic fbdev emulation, as soon as the generic code uses struct
> dma_buf_map.
>
> [1] https://lore.kernel.org/dri-devel/20200725191012.GA434957@ravnborg.org/
> [2] https://lore.kernel.org/dri-devel/20200806085239.4606-1-tzimmermann@suse.de/
>
> Thomas Zimmermann (3):
>   dma-buf: Add struct dma-buf-map for storing struct dma_buf.vaddr_ptr
>   dma-buf: Use struct dma_buf_map in dma_buf_vmap() interfaces
>   dma-buf: Use struct dma_buf_map in dma_buf_vunmap() interfaces

FWIW, for the series, please feel free to add my
Acked-by: Sumit Semwal <sumit.semwal@linaro.org>

>
>  Documentation/driver-api/dma-buf.rst          |   3 +
>  drivers/dma-buf/dma-buf.c                     |  40 +++---
>  drivers/gpu/drm/drm_gem_cma_helper.c          |  16 ++-
>  drivers/gpu/drm/drm_gem_shmem_helper.c        |  17 ++-
>  drivers/gpu/drm/drm_prime.c                   |  14 +-
>  drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c   |  13 +-
>  drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c    |  13 +-
>  .../drm/i915/gem/selftests/i915_gem_dmabuf.c  |  18 ++-
>  drivers/gpu/drm/tegra/gem.c                   |  23 ++--
>  .../common/videobuf2/videobuf2-dma-contig.c   |  17 ++-
>  .../media/common/videobuf2/videobuf2-dma-sg.c |  19 ++-
>  .../common/videobuf2/videobuf2-vmalloc.c      |  21 ++-
>  include/drm/drm_prime.h                       |   5 +-
>  include/linux/dma-buf-map.h                   | 126 ++++++++++++++++++
>  include/linux/dma-buf.h                       |  11 +-
>  15 files changed, 274 insertions(+), 82 deletions(-)
>  create mode 100644 include/linux/dma-buf-map.h
>
> --
> 2.28.0
>

Best,
Sumit.
Sumit Semwal Sept. 18, 2020, 8:32 a.m. UTC | #10
Hi Thomas,

On Fri, 18 Sep 2020 at 11:36, Sumit Semwal <sumit.semwal@linaro.org> wrote:
>
> Hello Thomas,
>
> On Mon, 14 Sep 2020 at 16:55, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >
> > Dma-buf provides vmap() and vunmap() for retrieving and releasing mappings
> > of dma-buf memory in kernel address space. The functions operate with plain
> > addresses and the assumption is that the memory can be accessed with load
> > and store operations. This is not the case on some architectures (e.g.,
> > sparc64) where I/O memory can only be accessed with dedicated instructions.
> >
> > This patchset introduces struct dma_buf_map, which contains the address of
> > a buffer and a flag that tells whether system- or I/O-memory instructions
> > are required.
>
> Thank you for the patchset - it is a really nice, clean bit to add!
> >
> > Some background: updating the DRM framebuffer console on sparc64 makes the
> > kernel panic. This is because the framebuffer memory cannot be accessed with
> > system-memory instructions. We currently employ a workaround in DRM to
> > address this specific problem. [1]
> >
> > To resolve the problem, we'd like to address it at the most common point,
> > which is the dma-buf framework. The dma-buf mapping ideally knows if I/O
> > instructions are required and exports this information to it's users. The
> > new structure struct dma_buf_map stores the buffer address and a flag that
> > signals I/O memory. Affected users of the buffer (e.g., drivers, frameworks)
> > can then access the memory accordingly.
> >
> > This patchset only introduces struct dma_buf_map, and updates struct dma_buf
> > and it's interfaces. Further patches can update dma-buf users. For example,
> > there's a prototype patchset for DRM that fixes the framebuffer problem. [2]
> >
> > Further work: TTM, one of DRM's memory managers, already exports an
> > is_iomem flag of its own. It could later be switched over to exporting struct
> > dma_buf_map, thus simplifying some code. Several DRM drivers expect their
> > fbdev console to operate on I/O memory. These could possibly be switched over
> > to the generic fbdev emulation, as soon as the generic code uses struct
> > dma_buf_map.
> >
> > [1] https://lore.kernel.org/dri-devel/20200725191012.GA434957@ravnborg.org/
> > [2] https://lore.kernel.org/dri-devel/20200806085239.4606-1-tzimmermann@suse.de/
> >
> > Thomas Zimmermann (3):
> >   dma-buf: Add struct dma-buf-map for storing struct dma_buf.vaddr_ptr
> >   dma-buf: Use struct dma_buf_map in dma_buf_vmap() interfaces
> >   dma-buf: Use struct dma_buf_map in dma_buf_vunmap() interfaces
>
> FWIW, for the series, please feel free to add my
> Acked-by: Sumit Semwal <sumit.semwal@linaro.org>
Of course, once the errors found by kernel test robot are fixed :).
>
> >
> >  Documentation/driver-api/dma-buf.rst          |   3 +
> >  drivers/dma-buf/dma-buf.c                     |  40 +++---
> >  drivers/gpu/drm/drm_gem_cma_helper.c          |  16 ++-
> >  drivers/gpu/drm/drm_gem_shmem_helper.c        |  17 ++-
> >  drivers/gpu/drm/drm_prime.c                   |  14 +-
> >  drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c   |  13 +-
> >  drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c    |  13 +-
> >  .../drm/i915/gem/selftests/i915_gem_dmabuf.c  |  18 ++-
> >  drivers/gpu/drm/tegra/gem.c                   |  23 ++--
> >  .../common/videobuf2/videobuf2-dma-contig.c   |  17 ++-
> >  .../media/common/videobuf2/videobuf2-dma-sg.c |  19 ++-
> >  .../common/videobuf2/videobuf2-vmalloc.c      |  21 ++-
> >  include/drm/drm_prime.h                       |   5 +-
> >  include/linux/dma-buf-map.h                   | 126 ++++++++++++++++++
> >  include/linux/dma-buf.h                       |  11 +-
> >  15 files changed, 274 insertions(+), 82 deletions(-)
> >  create mode 100644 include/linux/dma-buf-map.h
> >
> > --
> > 2.28.0
> >
>
> Best,
> Sumit.

Best,
Sumit.