Re: [PATCH v3 0/4] dma-buf: Flag vmap'ed memory as system or I/O memory

From: Daniel Vetter <daniel@ffwll.ch>
To: "Christian König" <christian.koenig@amd.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>,
	Sam Ravnborg <sam@ravnborg.org>,
	linux-doc@vger.kernel.org, airlied@linux.ie,
	dri-devel@lists.freedesktop.org, thierry.reding@gmail.com,
	kraxel@redhat.com, afd@ti.com, m.szyprowski@samsung.com,
	arnd@arndb.de, corbet@lwn.net, jonathanh@nvidia.com,
	matthew.auld@intel.com, linux+etnaviv@armlinux.org.uk,
	labbott@redhat.com, linux-media@vger.kernel.org,
	pawel@osciak.com, intel-gfx@lists.freedesktop.org,
	etnaviv@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org,
	thomas.hellstrom@intel.com, rodrigo.vivi@intel.com,
	linux-tegra@vger.kernel.org, mchehab@kernel.org,
	gregkh@linuxfoundation.org, lmark@codeaurora.org,
	tfiga@chromium.org, kyungmin.park@samsung.com,
	robin.murphy@arm.com
Subject: Re: [PATCH v3 0/4] dma-buf: Flag vmap'ed memory as system or I/O memory
Date: Tue, 29 Sep 2020 11:14:45 +0200	[thread overview]
Message-ID: <20200929091445.GI438822@phenom.ffwll.local> (raw)
In-Reply-To: <49c4dcec-cd69-510a-9781-e8fa5fb669f9@amd.com>

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch