Message-ID: <aApirJb9P-LOOB8j@jkangas-thinkpadp1gen3.rmtuswa.csb>
Date: Thu, 24 Apr 2025 09:11:24 -0700
From: Jared Kangas <jkangas@...hat.com>
To: Maxime Ripard <mripard@...nel.org>
Cc: sumit.semwal@...aro.org, benjamin.gaignard@...labora.com,
	Brian.Starkey@....com, jstultz@...gle.com, tjmercier@...gle.com,
	christian.koenig@....com, linux-media@...r.kernel.org,
	dri-devel@...ts.freedesktop.org, linaro-mm-sig@...ts.linaro.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] dma-buf: heaps: Give default CMA heap a fixed name

Hi Maxime,

On Thu, Apr 24, 2025 at 10:33:58AM +0200, Maxime Ripard wrote:
> Hi Jared,
> 
> Thanks for working on this
> 
> On Tue, Apr 22, 2025 at 12:19:39PM -0700, Jared Kangas wrote:
> > The CMA heap's name in devtmpfs can vary depending on how the heap is
> > defined. Its name defaults to "reserved", but if a CMA area is defined
> > in the devicetree, the heap takes on the devicetree node's name, such as
> > "default-pool" or "linux,cma". To simplify naming, just name it
> > "default_cma", and keep a legacy node in place backed by the same
> > underlying structure for backwards compatibility.
> > 
> > Signed-off-by: Jared Kangas <jkangas@...hat.com>
> > ---
> >  Documentation/userspace-api/dma-buf-heaps.rst | 11 +++++++----
> >  drivers/dma-buf/heaps/Kconfig                 | 10 ++++++++++
> >  drivers/dma-buf/heaps/cma_heap.c              | 14 +++++++++++++-
> >  3 files changed, 30 insertions(+), 5 deletions(-)
> > 
> > diff --git a/Documentation/userspace-api/dma-buf-heaps.rst b/Documentation/userspace-api/dma-buf-heaps.rst
> > index 535f49047ce64..577de813ba461 100644
> > --- a/Documentation/userspace-api/dma-buf-heaps.rst
> > +++ b/Documentation/userspace-api/dma-buf-heaps.rst
> > @@ -19,7 +19,10 @@ following heaps:
> >   - The ``cma`` heap allocates physically contiguous, cacheable,
> >     buffers. Only present if a CMA region is present. Such a region is
> >     usually created either through the kernel commandline through the
> > -   `cma` parameter, a memory region Device-Tree node with the
> > -   `linux,cma-default` property set, or through the `CMA_SIZE_MBYTES` or
> > -   `CMA_SIZE_PERCENTAGE` Kconfig options. Depending on the platform, it
> > -   might be called ``reserved``, ``linux,cma``, or ``default-pool``.
> > +   ``cma`` parameter, a memory region Device-Tree node with the
> > +   ``linux,cma-default`` property set, or through the ``CMA_SIZE_MBYTES`` or
> > +   ``CMA_SIZE_PERCENTAGE`` Kconfig options. The heap's name in devtmpfs is
> > +   ``default_cma``. For backwards compatibility, when the
> > +   ``DMABUF_HEAPS_CMA_LEGACY`` Kconfig option is set, a duplicate node is
> > +   created following legacy naming conventions; the legacy name might be
> > +   ``reserved``, ``linux,cma``, or ``default-pool``.
> 
> It looks like, in addition to documenting the new naming, you also
> changed all the backticks to double backticks. Why did you do so? It
> seems mostly unrelated to that patch, so it would be better in a
> separate patch.

Ah, I thought that since it was touching the immediate area and it's a
small fix it would be suitable for this patch. Thanks for the catch,
I'll extract this in v3.

As for the rationale: as I understand it, the single backticks here are
semantically incorrect, at least in general reST -- they're mainly used
for links and roles. In this instance, the syntax is interpreted as the
default role:

    https://docutils.sourceforge.io/docs/ref/rst/roles.html

I believe double backticks are what the doc is looking for here, used
for code and rendered as monospaced text. Although there are a number of
places around existing documentation that use single backticks (which
happens to render as italics because the default role is
:title-reference: if not configured in conf.py), a look through doc
history points to double backticks being treated as correct for code,
such as f6314b76d826 ("docs: kbuild/kconfig: reformat/cleanup") or
2f0e2a39bbab ("docs/kbuild/makefiles: unify quoting").

> 
> > diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
> > index a5eef06c42264..83f3770fa820a 100644
> > --- a/drivers/dma-buf/heaps/Kconfig
> > +++ b/drivers/dma-buf/heaps/Kconfig
> > @@ -12,3 +12,13 @@ config DMABUF_HEAPS_CMA
> >  	  Choose this option to enable dma-buf CMA heap. This heap is backed
> >  	  by the Contiguous Memory Allocator (CMA). If your system has these
> >  	  regions, you should say Y here.
> > +
> > +config DMABUF_HEAPS_CMA_LEGACY
> > +	bool "DMA-BUF CMA Heap"
> > +	default y
> > +	depends on DMABUF_HEAPS_CMA
> > +	help
> > +	  Add a duplicate CMA-backed dma-buf heap with legacy naming derived
> > +	  from the CMA area's devicetree node, or "reserved" if the area is not
> > +	  defined in the devicetree. This uses the same underlying allocator as
> > +	  CONFIG_DMABUF_HEAPS_CMA.
> > diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
> > index e998d8ccd1dc6..cd742c961190d 100644
> > --- a/drivers/dma-buf/heaps/cma_heap.c
> > +++ b/drivers/dma-buf/heaps/cma_heap.c
> > @@ -22,6 +22,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/vmalloc.h>
> >  
> > +#define DEFAULT_CMA_NAME "default_cma"
> 
> I appreciate this is kind of bikeshed-color territory, but I think "cma"
> would be a better option here. There's nothing "default" about it.

I'm a little uncertain about plain "cma"; John mentioned the possibility
of other CMA regions, suggesting that if we used "cma", we'd need to
rename it again in the future to disambiguate. It doesn't sound
guaranteed that other regions will be added, so I can see starting with
"cma" and adjusting later if needed, but "default_cma" seemed
inoffensive enough because it's consistent with current wording (e.g.,
"linux,cma-default", "default-pool"), and has a lower chance of changing
if other CMA regions/heaps were added. Let me know what you think.

> 
> >  struct cma_heap {
> >  	struct dma_heap *heap;
> > @@ -394,15 +395,26 @@ static int __init __add_cma_heap(struct cma *cma, const char *name)
> >  static int __init add_default_cma_heap(void)
> >  {
> >  	struct cma *default_cma = dev_get_cma_area(NULL);
> > +	const char *legacy_cma_name;
> >  	int ret;
> >  
> >  	if (!default_cma)
> >  		return 0;
> >  
> > -	ret = __add_cma_heap(default_cma, cma_get_name(default_cma));
> > +	ret = __add_cma_heap(default_cma, DEFAULT_CMA_NAME);
> >  	if (ret)
> >  		return ret;
> >  
> > +	legacy_cma_name = cma_get_name(default_cma);
> > +
> > +	if (IS_ENABLED(CONFIG_DMABUF_HEAPS_CMA_LEGACY) &&
> > +	    strcmp(legacy_cma_name, DEFAULT_CMA_NAME)) {
> > +		ret = __add_cma_heap(default_cma, legacy_cma_name);
> > +		if (ret)
> > +			pr_warn("cma_heap: failed to add legacy heap: %pe\n",
> > +				ERR_PTR(-ret));
> > +	}
> > +
> 
> It would also simplify this part, since you would always create the legacy heap.

By "always", do you mean removing the strcmp? I added this to guard
against cases where the devicetree node's name clashed with the default
name, given that the DT name isn't necessarily restricted to one of the
current names in use ("linux,cma" or "default-pool"). It seems like the
strcmp would be relevant regardless of the naming choice, but if this is
overly cautious, I can remove it in v3.

Jared

> 
> Maxime

Powered by blists - more mailing lists