Re: [PATCH v2] dma-contiguous: define proper name for global cma region

[John Stultz]

On Wed, Aug 9, 2023 at 8:04 AM Pintu Agarwal <pintu.ping@xxxxxxxxx> wrote:
>
> Hi,
>
> On Thu, 3 Aug 2023 at 23:04, Pintu Agarwal <pintu.ping@xxxxxxxxx> wrote:
> >
> > Hi,
> >
> > On Wed, 2 Aug 2023 at 15:17, Christoph Hellwig <hch@xxxxxx> wrote:
> > >
> > > On Tue, Aug 01, 2023 at 10:39:04PM -0700, John Stultz wrote:
> > > > So, forgive me, I've not had a chance to look into this, but my
> > > > recollection was "reserved" is the name we see on x86, but other names
> > > > are possibly provided via the dts node?
> > >
> > No, I think "reserved" is the name hard-coded (for all arch) in Kernel
> > for global-cma.
> > So, I don't think this is x86 specific. I am checking on arm32 itself.
> > When we can dma_alloc_coherent we see these in the logs (if dts region
> > is not present).
> > cma: cma_alloc(cma (ptrval), name: reserved, count 64, align 6)
> > Now, with this change we will see this:
> > cma: cma_alloc(cma (ptrval), name: global-cma-region, count 64, align 6)
> >
> > > Indeed, dma_contiguous_default_area can also be set through
> > > rmem_cma_setup, which then takes the name from DT.
> > >
> > I think this is a different case. If DT entry is present we get this:
> > Reserved memory: created CMA memory pool at 0x98000000, name: name:
> > linux,cma, size 128 MiB
> > cma: cma_alloc(cma (ptrval), name: linux,cma, count 64, align 6)
> >
> > Here we are talking about the default hard-coded name in Kernel code
> > if DT is not defined.
> > So, in one of the boards, this DT entry was not present and it shows
> > as "reserved".
> >
> > > > I believe on the hikey board its "linux,cma" is the name, so forcing
> > > > it to reserved would break that.
> > > >
> > Yes, everywhere in the DT it's defined as "linux,cma".
> > You mean this also should be changed to "linux,cma-global-region"
> > everywhere with this change ?
> >
> > > > Maybe instead add a compat config option to force the cma name (so x86
> > > > can set it to "default" if needed)?
> > >
> > Yes, having it in config is also a good option instead of hard-coding in Kernel.
> > >
> > > I think we'll just need to leave it as-is.  I with dma-heaps had never
> > > exposed the name to userspace, but we'll have to lіve with it now.
> >
> > Can you point me to the userspace utility we are talking about here ?
> > I think we should not worry much about userspace name exposure.
> > I guess it should fetch whatever is declared in Kernel or DTS, right ?
>
> Just to follow-up on this.
> For now, can we change the Kernel hard-coded value from "reserved" to
> "global-cma-region" ?
> Later, for the DTS defined name let it be "linux,cma" or change that
> also to "linux,global-cma-region" ?
>
> Will this make sense ?

Apologies, sorry for not responding to your earlier message, it slipped by.

So, the concern is there may be allocators (like gralloc in Android)
that allocate from the CMA region via the dma-buf heaps interface.

So by changing the name (either hardcoded or DTS names), you'll change
the user-visible heap name, potentially breaking those userland
allocators.

Now, the dmabuf heaps are designed to be like other dynamic devices
(like disks or partitions), which may be different from device to
device. However, changing the name would still be an inconvenience for
folks who have hard-coded that name in their userland allocator which
was designed for a single device.  This would be similar to the old
issue of an existing fstab breaking from the ide (hda) to sata (sda)
driver transition.  Or similar to what folks went through a while back
with network device names changing from eth0 -> ens0 or whatever.

That said, most android devices historically haven't upreved to new
kernel versions wihtout major userspace changes, so the impact might
be minimal, but that is likely to change in the future so we should be
careful here.

What I'd propose instead is to either leave it alone as Christoph
suggested, or have a build option/boot argument so folks can preserve
the legacy name if they need.

thanks
-john