| Message ID | 20200925115601.23955-1-tzimmermann@suse.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | dma-buf: Flag vmap'ed memory as system or I/O memory | expand |
Hi Thomas, On Fri, Sep 25, 2020 at 01:55:57PM +0200, Thomas Zimmermann wrote: > Dma-buf provides vmap() and vunmap() for retriving 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. > > v3: > * update fastrpc driver (kernel test robot) > * expand documentation (Daniel) > * move documentation into separate patch > v2: > * always clear map parameter in dma_buf_vmap() (Daniel) > * include dma-buf-heaps and i915 selftests (kernel test robot) > * initialize cma_obj before using it in drm_gem_cma_free_object() > (kernel test robot) > > [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 (4): > 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 > dma-buf: Document struct dma_buf_map > > Documentation/driver-api/dma-buf.rst | 9 + > drivers/dma-buf/dma-buf.c | 42 ++-- > drivers/dma-buf/heaps/heap-helpers.c | 10 +- > drivers/gpu/drm/drm_gem_cma_helper.c | 20 +- > drivers/gpu/drm/drm_gem_shmem_helper.c | 17 +- > drivers/gpu/drm/drm_prime.c | 15 +- > 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 +- > .../gpu/drm/i915/gem/selftests/mock_dmabuf.c | 14 +- > 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 +- > drivers/misc/fastrpc.c | 6 +- > include/drm/drm_prime.h | 5 +- > include/linux/dma-buf-map.h | 193 ++++++++++++++++++ > include/linux/dma-buf.h | 11 +- > 18 files changed, 372 insertions(+), 94 deletions(-) > create mode 100644 include/linux/dma-buf-map.h For drivers/media/common/videobuf2 changes: Acked-by: Tomasz Figa <tfiga@chromium.org> Best regards, Tomasz
Hi Thomas. Sorry for chiming in late here, have been offline for a while. On Fri, Sep 25, 2020 at 01:55:57PM +0200, Thomas Zimmermann wrote: > Dma-buf provides vmap() and vunmap() for retriving 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. The whole idea with a struct that can represent both a pointer to system memory and io memory is very nice. dma-buf is one user of this but we may/will see other users. So the naming seems of as this should be a concept independent of dma-buf. And then the struct definition and all the helpers should be moved away from dma-buf. Maybe something like this: struct simap { union { void __iomem *vaddr_iomem; void *vaddr; }; bool is_iomem; }; Where simap is a shorthand for system_iomem_map And it could al be stuffed into a include/linux/simap.h file. Not totally sold on the simap name - but wanted to come up with something. With this approach users do not have to pull in dma-buf to use it and users will not confuse that this is only for dma-buf usage. I am sorry for being late with the feedback. Sam > 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. > > v3: > * update fastrpc driver (kernel test robot) > * expand documentation (Daniel) > * move documentation into separate patch > v2: > * always clear map parameter in dma_buf_vmap() (Daniel) > * include dma-buf-heaps and i915 selftests (kernel test robot) > * initialize cma_obj before using it in drm_gem_cma_free_object() > (kernel test robot) > > [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 (4): > 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 > dma-buf: Document struct dma_buf_map > > Documentation/driver-api/dma-buf.rst | 9 + > drivers/dma-buf/dma-buf.c | 42 ++-- > drivers/dma-buf/heaps/heap-helpers.c | 10 +- > drivers/gpu/drm/drm_gem_cma_helper.c | 20 +- > drivers/gpu/drm/drm_gem_shmem_helper.c | 17 +- > drivers/gpu/drm/drm_prime.c | 15 +- > 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 +- > .../gpu/drm/i915/gem/selftests/mock_dmabuf.c | 14 +- > 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 +- > drivers/misc/fastrpc.c | 6 +- > include/drm/drm_prime.h | 5 +- > include/linux/dma-buf-map.h | 193 ++++++++++++++++++ > include/linux/dma-buf.h | 11 +- > 18 files changed, 372 insertions(+), 94 deletions(-) > create mode 100644 include/linux/dma-buf-map.h > > -- > 2.28.0
Hi Sam Am 26.09.20 um 09:13 schrieb Sam Ravnborg: > Hi Thomas. > > Sorry for chiming in late here, have been offline for a while. > > On Fri, Sep 25, 2020 at 01:55:57PM +0200, Thomas Zimmermann wrote: >> Dma-buf provides vmap() and vunmap() for retriving 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. > > The whole idea with a struct that can represent both a pointer to system > memory and io memory is very nice. > dma-buf is one user of this but we may/will see other users. So the > naming seems of as this should be a concept independent of dma-buf. > > And then the struct definition and all the helpers should be moved away > from dma-buf. > > Maybe something like this: > > struct simap { > union { > void __iomem *vaddr_iomem; > void *vaddr; > }; > bool is_iomem; > }; > > Where simap is a shorthand for system_iomem_map > And it could al be stuffed into a include/linux/simap.h file. > > Not totally sold on the simap name - but wanted to come up with > something. Yes. Others, myself included, have suggested to use a name that does not imply a connection to the dma-buf framework, but no one has come up with a good name. I strongly dislike simap, as it's entirely non-obvious what it does. dma-buf-map is not actually wrong. The structures represents the mapping of a dma-able buffer in most cases. > > With this approach users do not have to pull in dma-buf to use it and > users will not confuse that this is only for dma-buf usage. There's no need to enable dma-buf. It's all in the header file without dependencies on dma-buf. It's really just the name. But there's something else to take into account. The whole issue here is that the buffer is disconnected from its originating driver, so we don't know which kind of memory ops we have to use. Thinking about it, I realized that no one else seemed to have this problem until now. Otherwise there would be a solution already. So maybe the dma-buf framework *is* the native use case for this data structure. Anyway, if a better name than dma-buf-map comes in, I'm willing to rename the thing. Otherwise I intend to merge the patchset by the end of the week. Best regards Thomas > > I am sorry for being late with the feedback. > > Sam > > >> 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. >> >> v3: >> * update fastrpc driver (kernel test robot) >> * expand documentation (Daniel) >> * move documentation into separate patch >> v2: >> * always clear map parameter in dma_buf_vmap() (Daniel) >> * include dma-buf-heaps and i915 selftests (kernel test robot) >> * initialize cma_obj before using it in drm_gem_cma_free_object() >> (kernel test robot) >> >> [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 (4): >> 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 >> dma-buf: Document struct dma_buf_map >> >> Documentation/driver-api/dma-buf.rst | 9 + >> drivers/dma-buf/dma-buf.c | 42 ++-- >> drivers/dma-buf/heaps/heap-helpers.c | 10 +- >> drivers/gpu/drm/drm_gem_cma_helper.c | 20 +- >> drivers/gpu/drm/drm_gem_shmem_helper.c | 17 +- >> drivers/gpu/drm/drm_prime.c | 15 +- >> 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 +- >> .../gpu/drm/i915/gem/selftests/mock_dmabuf.c | 14 +- >> 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 +- >> drivers/misc/fastrpc.c | 6 +- >> include/drm/drm_prime.h | 5 +- >> include/linux/dma-buf-map.h | 193 ++++++++++++++++++ >> include/linux/dma-buf.h | 11 +- >> 18 files changed, 372 insertions(+), 94 deletions(-) >> create mode 100644 include/linux/dma-buf-map.h >> >> -- >> 2.28.0 > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
Hi Thomas. > > > > struct simap { > > union { > > void __iomem *vaddr_iomem; > > void *vaddr; > > }; > > bool is_iomem; > > }; > > > > Where simap is a shorthand for system_iomem_map > > And it could al be stuffed into a include/linux/simap.h file. > > > > Not totally sold on the simap name - but wanted to come up with > > something. > > Yes. Others, myself included, have suggested to use a name that does not > imply a connection to the dma-buf framework, but no one has come up with > a good name. > > I strongly dislike simap, as it's entirely non-obvious what it does. > dma-buf-map is not actually wrong. The structures represents the mapping > of a dma-able buffer in most cases. > > > > > With this approach users do not have to pull in dma-buf to use it and > > users will not confuse that this is only for dma-buf usage. > > There's no need to enable dma-buf. It's all in the header file without > dependencies on dma-buf. It's really just the name. > > But there's something else to take into account. The whole issue here is > that the buffer is disconnected from its originating driver, so we don't > know which kind of memory ops we have to use. Thinking about it, I > realized that no one else seemed to have this problem until now. > Otherwise there would be a solution already. So maybe the dma-buf > framework *is* the native use case for this data structure. We have at least: linux/fb.h: union { char __iomem *screen_base; /* Virtual address */ char *screen_buffer; }; Which solve more or less the same problem. > Anyway, if a better name than dma-buf-map comes in, I'm willing to > rename the thing. Otherwise I intend to merge the patchset by the end of > the week. Well, the main thing is that I think this shoud be moved away from dma-buf. But if indeed dma-buf is the only relevant user in drm then I am totally fine with the current naming. One alternative named that popped up in my head: struct sys_io_map {} But again, if this is kept in dma-buf then I am fine with the current naming. In other words, if you continue to think this is mostly a dma-buf thing all three patches are: Acked-by: Sam Ravnborg <sam@ravnborg.org> Sam
Am 27.09.20 um 21:16 schrieb Sam Ravnborg: > Hi Thomas. > >>> struct simap { >>> union { >>> void __iomem *vaddr_iomem; >>> void *vaddr; >>> }; >>> bool is_iomem; >>> }; >>> >>> Where simap is a shorthand for system_iomem_map >>> And it could al be stuffed into a include/linux/simap.h file. >>> >>> Not totally sold on the simap name - but wanted to come up with >>> something. >> Yes. Others, myself included, have suggested to use a name that does not >> imply a connection to the dma-buf framework, but no one has come up with >> a good name. >> >> I strongly dislike simap, as it's entirely non-obvious what it does. >> dma-buf-map is not actually wrong. The structures represents the mapping >> of a dma-able buffer in most cases. >> >>> With this approach users do not have to pull in dma-buf to use it and >>> users will not confuse that this is only for dma-buf usage. >> There's no need to enable dma-buf. It's all in the header file without >> dependencies on dma-buf. It's really just the name. >> >> But there's something else to take into account. The whole issue here is >> that the buffer is disconnected from its originating driver, so we don't >> know which kind of memory ops we have to use. Thinking about it, I >> realized that no one else seemed to have this problem until now. >> Otherwise there would be a solution already. So maybe the dma-buf >> framework *is* the native use case for this data structure. > We have at least: > linux/fb.h: > union { > char __iomem *screen_base; /* Virtual address */ > char *screen_buffer; > }; > > Which solve more or less the same problem. I also already noted that in TTM we have exactly the same problem and a whole bunch of helpers to allow operations on those pointers. Christian. > > >> Anyway, if a better name than dma-buf-map comes in, I'm willing to >> rename the thing. Otherwise I intend to merge the patchset by the end of >> the week. > Well, the main thing is that I think this shoud be moved away from > dma-buf. But if indeed dma-buf is the only relevant user in drm then > I am totally fine with the current naming. > > One alternative named that popped up in my head: struct sys_io_map {} > But again, if this is kept in dma-buf then I am fine with the current > naming. > > In other words, if you continue to think this is mostly a dma-buf > thing all three patches are: > Acked-by: Sam Ravnborg <sam@ravnborg.org> > > Sam
Hi Am 28.09.20 um 08:50 schrieb Christian König: > Am 27.09.20 um 21:16 schrieb Sam Ravnborg: >> Hi Thomas. >> >>>> struct simap { >>>> union { >>>> void __iomem *vaddr_iomem; >>>> void *vaddr; >>>> }; >>>> bool is_iomem; >>>> }; >>>> >>>> Where simap is a shorthand for system_iomem_map >>>> And it could al be stuffed into a include/linux/simap.h file. >>>> >>>> Not totally sold on the simap name - but wanted to come up with >>>> something. >>> Yes. Others, myself included, have suggested to use a name that does not >>> imply a connection to the dma-buf framework, but no one has come up with >>> a good name. >>> >>> I strongly dislike simap, as it's entirely non-obvious what it does. >>> dma-buf-map is not actually wrong. The structures represents the mapping >>> of a dma-able buffer in most cases. >>> >>>> With this approach users do not have to pull in dma-buf to use it and >>>> users will not confuse that this is only for dma-buf usage. >>> There's no need to enable dma-buf. It's all in the header file without >>> dependencies on dma-buf. It's really just the name. >>> >>> But there's something else to take into account. The whole issue here is >>> that the buffer is disconnected from its originating driver, so we don't >>> know which kind of memory ops we have to use. Thinking about it, I >>> realized that no one else seemed to have this problem until now. >>> Otherwise there would be a solution already. So maybe the dma-buf >>> framework *is* the native use case for this data structure. >> We have at least: >> linux/fb.h: >> union { >> char __iomem *screen_base; /* Virtual address */ >> char *screen_buffer; >> }; >> >> Which solve more or less the same problem. I thought this was for convenience. The important is_iomem bit is missing. > > I also already noted that in TTM we have exactly the same problem and a > whole bunch of helpers to allow operations on those pointers. How do you call this within TTM? The data structure represents a pointer to either system or I/O memory, but not necessatrily device memory. It contains raw data. That would give something like struct databuf_map struct databuf_ptr struct dbuf_map struct dbuf_ptr My favorite would be dbuf_ptr. It's short and the API names would make sense: dbuf_ptr_clear() for clearing, dbuf_ptr_set_vaddr() to set an address, dbuf_ptr_incr() to increment, etc. Also, the _ptr indicates that it's a single address; not an offset with length. Best regards Thomas > > Christian. > >> >> >>> Anyway, if a better name than dma-buf-map comes in, I'm willing to >>> rename the thing. Otherwise I intend to merge the patchset by the end of >>> the week. >> Well, the main thing is that I think this shoud be moved away from >> dma-buf. But if indeed dma-buf is the only relevant user in drm then >> I am totally fine with the current naming. >> >> One alternative named that popped up in my head: struct sys_io_map {} >> But again, if this is kept in dma-buf then I am fine with the current >> naming. >> >> In other words, if you continue to think this is mostly a dma-buf >> thing all three patches are: >> Acked-by: Sam Ravnborg <sam@ravnborg.org> >> >> Sam > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Am 28.09.20 um 09:37 schrieb Thomas Zimmermann: > Hi > > Am 28.09.20 um 08:50 schrieb Christian König: >> Am 27.09.20 um 21:16 schrieb Sam Ravnborg: >>> Hi Thomas. >>> >>>>> struct simap { >>>>> union { >>>>> void __iomem *vaddr_iomem; >>>>> void *vaddr; >>>>> }; >>>>> bool is_iomem; >>>>> }; >>>>> >>>>> Where simap is a shorthand for system_iomem_map >>>>> And it could al be stuffed into a include/linux/simap.h file. >>>>> >>>>> Not totally sold on the simap name - but wanted to come up with >>>>> something. >>>> Yes. Others, myself included, have suggested to use a name that does not >>>> imply a connection to the dma-buf framework, but no one has come up with >>>> a good name. >>>> >>>> I strongly dislike simap, as it's entirely non-obvious what it does. >>>> dma-buf-map is not actually wrong. The structures represents the mapping >>>> of a dma-able buffer in most cases. >>>> >>>>> With this approach users do not have to pull in dma-buf to use it and >>>>> users will not confuse that this is only for dma-buf usage. >>>> There's no need to enable dma-buf. It's all in the header file without >>>> dependencies on dma-buf. It's really just the name. >>>> >>>> But there's something else to take into account. The whole issue here is >>>> that the buffer is disconnected from its originating driver, so we don't >>>> know which kind of memory ops we have to use. Thinking about it, I >>>> realized that no one else seemed to have this problem until now. >>>> Otherwise there would be a solution already. So maybe the dma-buf >>>> framework *is* the native use case for this data structure. >>> We have at least: >>> linux/fb.h: >>> union { >>> char __iomem *screen_base; /* Virtual address */ >>> char *screen_buffer; >>> }; >>> >>> Which solve more or less the same problem. > I thought this was for convenience. The important is_iomem bit is missing. > >> I also already noted that in TTM we have exactly the same problem and a >> whole bunch of helpers to allow operations on those pointers. > How do you call this within TTM? ttm_bus_placement, but I really don't like that name. > > The data structure represents a pointer to either system or I/O memory, > but not necessatrily device memory. It contains raw data. That would > give something like > > struct databuf_map > struct databuf_ptr > struct dbuf_map > struct dbuf_ptr > > My favorite would be dbuf_ptr. It's short and the API names would make > sense: dbuf_ptr_clear() for clearing, dbuf_ptr_set_vaddr() to set an > address, dbuf_ptr_incr() to increment, etc. Also, the _ptr indicates > that it's a single address; not an offset with length. Puh, no idea. All of that doesn't sound like it 100% hits the underlying meaning of the structure. Christian. > > Best regards > Thomas > >> Christian. >> >>> >>>> Anyway, if a better name than dma-buf-map comes in, I'm willing to >>>> rename the thing. Otherwise I intend to merge the patchset by the end of >>>> the week. >>> Well, the main thing is that I think this shoud be moved away from >>> dma-buf. But if indeed dma-buf is the only relevant user in drm then >>> I am totally fine with the current naming. >>> >>> One alternative named that popped up in my head: struct sys_io_map {} >>> But again, if this is kept in dma-buf then I am fine with the current >>> naming. >>> >>> In other words, if you continue to think this is mostly a dma-buf >>> thing all three patches are: >>> Acked-by: Sam Ravnborg <sam@ravnborg.org> >>> >>> Sam >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, Sep 28, 2020 at 01:22:13PM +0200, Christian König wrote: > Am 28.09.20 um 09:37 schrieb Thomas Zimmermann: > > Hi > > > > Am 28.09.20 um 08:50 schrieb Christian König: > > > Am 27.09.20 um 21:16 schrieb Sam Ravnborg: > > > > Hi Thomas. > > > > > > > > > > struct simap { > > > > > > union { > > > > > > void __iomem *vaddr_iomem; > > > > > > void *vaddr; > > > > > > }; > > > > > > bool is_iomem; > > > > > > }; > > > > > > > > > > > > Where simap is a shorthand for system_iomem_map > > > > > > And it could al be stuffed into a include/linux/simap.h file. > > > > > > > > > > > > Not totally sold on the simap name - but wanted to come up with > > > > > > something. > > > > > Yes. Others, myself included, have suggested to use a name that does not > > > > > imply a connection to the dma-buf framework, but no one has come up with > > > > > a good name. > > > > > > > > > > I strongly dislike simap, as it's entirely non-obvious what it does. > > > > > dma-buf-map is not actually wrong. The structures represents the mapping > > > > > of a dma-able buffer in most cases. > > > > > > > > > > > With this approach users do not have to pull in dma-buf to use it and > > > > > > users will not confuse that this is only for dma-buf usage. > > > > > There's no need to enable dma-buf. It's all in the header file without > > > > > dependencies on dma-buf. It's really just the name. > > > > > > > > > > But there's something else to take into account. The whole issue here is > > > > > that the buffer is disconnected from its originating driver, so we don't > > > > > know which kind of memory ops we have to use. Thinking about it, I > > > > > realized that no one else seemed to have this problem until now. > > > > > Otherwise there would be a solution already. So maybe the dma-buf > > > > > framework *is* the native use case for this data structure. > > > > We have at least: > > > > linux/fb.h: > > > > union { > > > > char __iomem *screen_base; /* Virtual address */ > > > > char *screen_buffer; > > > > }; > > > > > > > > Which solve more or less the same problem. > > I thought this was for convenience. The important is_iomem bit is missing. > > > > > I also already noted that in TTM we have exactly the same problem and a > > > whole bunch of helpers to allow operations on those pointers. > > How do you call this within TTM? > > ttm_bus_placement, but I really don't like that name. > > > > > The data structure represents a pointer to either system or I/O memory, > > but not necessatrily device memory. It contains raw data. That would > > give something like > > > > struct databuf_map > > struct databuf_ptr > > struct dbuf_map > > struct dbuf_ptr > > > > My favorite would be dbuf_ptr. It's short and the API names would make > > sense: dbuf_ptr_clear() for clearing, dbuf_ptr_set_vaddr() to set an > > address, dbuf_ptr_incr() to increment, etc. Also, the _ptr indicates > > that it's a single address; not an offset with length. > > Puh, no idea. All of that doesn't sound like it 100% hits the underlying > meaning of the structure. Imo first let's merge this and then second with more users we might come up with a better name. And cocci is fairly good at large-scale rename, to the point where we manged to rename struct fence to struct dma_fence. Also this entire thread here imo shows that we haven't yet figured out the perfect name anyway, and I don't think it's worth it to hold this up because of this bikeshed :-) Cheers, Daniel > > Christian. > > > > > Best regards > > Thomas > > > > > Christian. > > > > > > > > Anyway, if a better name than dma-buf-map comes in, I'm willing to > > > > > rename the thing. Otherwise I intend to merge the patchset by the end of > > > > > the week. > > > > Well, the main thing is that I think this shoud be moved away from > > > > dma-buf. But if indeed dma-buf is the only relevant user in drm then > > > > I am totally fine with the current naming. > > > > > > > > One alternative named that popped up in my head: struct sys_io_map {} > > > > But again, if this is kept in dma-buf then I am fine with the current > > > > naming. > > > > > > > > In other words, if you continue to think this is mostly a dma-buf > > > > thing all three patches are: > > > > Acked-by: Sam Ravnborg <sam@ravnborg.org> > > > > > > > > Sam > > > _______________________________________________ > > > dri-devel mailing list > > > dri-devel@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
Am 29.09.20 um 11:14 schrieb Daniel Vetter: > On Mon, Sep 28, 2020 at 01:22:13PM +0200, Christian König wrote: >> Am 28.09.20 um 09:37 schrieb Thomas Zimmermann: >>> Hi >>> >>> Am 28.09.20 um 08:50 schrieb Christian König: >>>> Am 27.09.20 um 21:16 schrieb Sam Ravnborg: >>>>> Hi Thomas. >>>>> >>>>>>> struct simap { >>>>>>> union { >>>>>>> void __iomem *vaddr_iomem; >>>>>>> void *vaddr; >>>>>>> }; >>>>>>> bool is_iomem; >>>>>>> }; >>>>>>> >>>>>>> Where simap is a shorthand for system_iomem_map >>>>>>> And it could al be stuffed into a include/linux/simap.h file. >>>>>>> >>>>>>> Not totally sold on the simap name - but wanted to come up with >>>>>>> something. >>>>>> Yes. Others, myself included, have suggested to use a name that does not >>>>>> imply a connection to the dma-buf framework, but no one has come up with >>>>>> a good name. >>>>>> >>>>>> I strongly dislike simap, as it's entirely non-obvious what it does. >>>>>> dma-buf-map is not actually wrong. The structures represents the mapping >>>>>> of a dma-able buffer in most cases. >>>>>> >>>>>>> With this approach users do not have to pull in dma-buf to use it and >>>>>>> users will not confuse that this is only for dma-buf usage. >>>>>> There's no need to enable dma-buf. It's all in the header file without >>>>>> dependencies on dma-buf. It's really just the name. >>>>>> >>>>>> But there's something else to take into account. The whole issue here is >>>>>> that the buffer is disconnected from its originating driver, so we don't >>>>>> know which kind of memory ops we have to use. Thinking about it, I >>>>>> realized that no one else seemed to have this problem until now. >>>>>> Otherwise there would be a solution already. So maybe the dma-buf >>>>>> framework *is* the native use case for this data structure. >>>>> We have at least: >>>>> linux/fb.h: >>>>> union { >>>>> char __iomem *screen_base; /* Virtual address */ >>>>> char *screen_buffer; >>>>> }; >>>>> >>>>> Which solve more or less the same problem. >>> I thought this was for convenience. The important is_iomem bit is missing. >>> >>>> I also already noted that in TTM we have exactly the same problem and a >>>> whole bunch of helpers to allow operations on those pointers. >>> How do you call this within TTM? >> ttm_bus_placement, but I really don't like that name. >> >>> The data structure represents a pointer to either system or I/O memory, >>> but not necessatrily device memory. It contains raw data. That would >>> give something like >>> >>> struct databuf_map >>> struct databuf_ptr >>> struct dbuf_map >>> struct dbuf_ptr >>> >>> My favorite would be dbuf_ptr. It's short and the API names would make >>> sense: dbuf_ptr_clear() for clearing, dbuf_ptr_set_vaddr() to set an >>> address, dbuf_ptr_incr() to increment, etc. Also, the _ptr indicates >>> that it's a single address; not an offset with length. >> Puh, no idea. All of that doesn't sound like it 100% hits the underlying >> meaning of the structure. > Imo first let's merge this and then second with more users we might come > up with a better name. And cocci is fairly good at large-scale rename, to > the point where we manged to rename struct fence to struct dma_fence. Agreed, renaming things later on is easy if somebody comes up with something better. But blocking a necessary technical change just because of the naming is usually not a good idea. Christian. > > Also this entire thread here imo shows that we haven't yet figured out the > perfect name anyway, and I don't think it's worth it to hold this up > because of this bikeshed :-) > > Cheers, Daniel > >> Christian. >> >>> Best regards >>> Thomas >>> >>>> Christian. >>>> >>>>>> Anyway, if a better name than dma-buf-map comes in, I'm willing to >>>>>> rename the thing. Otherwise I intend to merge the patchset by the end of >>>>>> the week. >>>>> Well, the main thing is that I think this shoud be moved away from >>>>> dma-buf. But if indeed dma-buf is the only relevant user in drm then >>>>> I am totally fine with the current naming. >>>>> >>>>> One alternative named that popped up in my head: struct sys_io_map {} >>>>> But again, if this is kept in dma-buf then I am fine with the current >>>>> naming. >>>>> >>>>> In other words, if you continue to think this is mostly a dma-buf >>>>> thing all three patches are: >>>>> Acked-by: Sam Ravnborg <sam@ravnborg.org> >>>>> >>>>> Sam >>>> _______________________________________________ >>>> dri-devel mailing list >>>> dri-devel@lists.freedesktop.org >>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=02%7C01%7Cchristian.koenig%40amd.com%7C71c630a7ca1d48476eed08d864581e4f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637369676925032848&sdata=CsekzASvj2lY%2B74FIiLc9B5QG7AHma8B2M5y8Cassj4%3D&reserved=0
Am 29.09.20 um 13:09 schrieb Christian König: > Am 29.09.20 um 11:14 schrieb Daniel Vetter: >> On Mon, Sep 28, 2020 at 01:22:13PM +0200, Christian König wrote: >>> Am 28.09.20 um 09:37 schrieb Thomas Zimmermann: >>>> Hi >>>> >>>> Am 28.09.20 um 08:50 schrieb Christian König: >>>>> Am 27.09.20 um 21:16 schrieb Sam Ravnborg: >>>>>> Hi Thomas. >>>>>> >>>>>>>> struct simap { >>>>>>>> union { >>>>>>>> void __iomem *vaddr_iomem; >>>>>>>> void *vaddr; >>>>>>>> }; >>>>>>>> bool is_iomem; >>>>>>>> }; >>>>>>>> >>>>>>>> Where simap is a shorthand for system_iomem_map >>>>>>>> And it could al be stuffed into a include/linux/simap.h file. >>>>>>>> >>>>>>>> Not totally sold on the simap name - but wanted to come up with >>>>>>>> something. >>>>>>> Yes. Others, myself included, have suggested to use a name that >>>>>>> does not >>>>>>> imply a connection to the dma-buf framework, but no one has come >>>>>>> up with >>>>>>> a good name. >>>>>>> >>>>>>> I strongly dislike simap, as it's entirely non-obvious what it does. >>>>>>> dma-buf-map is not actually wrong. The structures represents the >>>>>>> mapping >>>>>>> of a dma-able buffer in most cases. >>>>>>> >>>>>>>> With this approach users do not have to pull in dma-buf to use >>>>>>>> it and >>>>>>>> users will not confuse that this is only for dma-buf usage. >>>>>>> There's no need to enable dma-buf. It's all in the header file >>>>>>> without >>>>>>> dependencies on dma-buf. It's really just the name. >>>>>>> >>>>>>> But there's something else to take into account. The whole issue >>>>>>> here is >>>>>>> that the buffer is disconnected from its originating driver, so >>>>>>> we don't >>>>>>> know which kind of memory ops we have to use. Thinking about it, I >>>>>>> realized that no one else seemed to have this problem until now. >>>>>>> Otherwise there would be a solution already. So maybe the dma-buf >>>>>>> framework *is* the native use case for this data structure. >>>>>> We have at least: >>>>>> linux/fb.h: >>>>>> union { >>>>>> char __iomem *screen_base; /* Virtual address */ >>>>>> char *screen_buffer; >>>>>> }; >>>>>> >>>>>> Which solve more or less the same problem. >>>> I thought this was for convenience. The important is_iomem bit is >>>> missing. >>>> >>>>> I also already noted that in TTM we have exactly the same problem >>>>> and a >>>>> whole bunch of helpers to allow operations on those pointers. >>>> How do you call this within TTM? >>> ttm_bus_placement, but I really don't like that name. >>> >>>> The data structure represents a pointer to either system or I/O memory, >>>> but not necessatrily device memory. It contains raw data. That would >>>> give something like >>>> >>>> struct databuf_map >>>> struct databuf_ptr >>>> struct dbuf_map >>>> struct dbuf_ptr >>>> >>>> My favorite would be dbuf_ptr. It's short and the API names would make >>>> sense: dbuf_ptr_clear() for clearing, dbuf_ptr_set_vaddr() to set an >>>> address, dbuf_ptr_incr() to increment, etc. Also, the _ptr indicates >>>> that it's a single address; not an offset with length. >>> Puh, no idea. All of that doesn't sound like it 100% hits the underlying >>> meaning of the structure. >> Imo first let's merge this and then second with more users we might come >> up with a better name. And cocci is fairly good at large-scale rename, to >> the point where we manged to rename struct fence to struct dma_fence. > > Agreed, renaming things later on is easy if somebody comes up with > something better. > > But blocking a necessary technical change just because of the naming is > usually not a good idea. OK, merged now. Best regards Thomas > > Christian. > >> >> Also this entire thread here imo shows that we haven't yet figured out >> the >> perfect name anyway, and I don't think it's worth it to hold this up >> because of this bikeshed :-) >> >> Cheers, Daniel >> >>> Christian. >>> >>>> Best regards >>>> Thomas >>>> >>>>> Christian. >>>>> >>>>>>> Anyway, if a better name than dma-buf-map comes in, I'm willing to >>>>>>> rename the thing. Otherwise I intend to merge the patchset by the >>>>>>> end of >>>>>>> the week. >>>>>> Well, the main thing is that I think this shoud be moved away from >>>>>> dma-buf. But if indeed dma-buf is the only relevant user in drm then >>>>>> I am totally fine with the current naming. >>>>>> >>>>>> One alternative named that popped up in my head: struct sys_io_map {} >>>>>> But again, if this is kept in dma-buf then I am fine with the current >>>>>> naming. >>>>>> >>>>>> In other words, if you continue to think this is mostly a dma-buf >>>>>> thing all three patches are: >>>>>> Acked-by: Sam Ravnborg <sam@ravnborg.org> >>>>>> >>>>>> Sam >>>>> _______________________________________________ >>>>> dri-devel mailing list >>>>> dri-devel@lists.freedesktop.org >>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=02%7C01%7Cchristian.koenig%40amd.com%7C71c630a7ca1d48476eed08d864581e4f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637369676925032848&sdata=CsekzASvj2lY%2B74FIiLc9B5QG7AHma8B2M5y8Cassj4%3D&reserved=0 >>>>> > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel