[RFC,3/4] dma-buf: cma_heap: Extend logic to export CMA regions tagged with "linux, cma-heap"

Commit Message

John Stultz May 1, 2020, 7:39 a.m. UTC

This patch reworks the cma_heap initialization so that
we expose both the default CMA region and any CMA regions
tagged with "linux,cma-heap" in the device-tree.

Cc: Rob Herring <[e-mail]>
Cc: Sumit Semwal <[e-mail]>
Cc: "Andrew F. Davis" <[e-mail]>
Cc: Benjamin Gaignard <[e-mail]>
Cc: Liam Mark <[e-mail]>
Cc: Pratik Patel <[e-mail]>
Cc: Laura Abbott <[e-mail]>
Cc: Brian Starkey <[e-mail]>
Cc: Chenbo Feng <[e-mail]>
Cc: Alistair Strachan <[e-mail]>
Cc: Sandeep Patil <[e-mail]>
Cc: Hridya Valsaraju <[e-mail]>
Cc: Christoph Hellwig <[e-mail]>
Cc: Marek Szyprowski <[e-mail]>
Cc: Robin Murphy <[e-mail]>
Cc: Andrew Morton <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Signed-off-by: John Stultz <[e-mail]>
---
 drivers/dma-buf/heaps/cma_heap.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Comments

Brian Starkey May 1, 2020, 10:21 a.m. UTC | #1

Hi John,

On Fri, May 01, 2020 at 07:39:48AM +0000, John Stultz wrote:
> This patch reworks the cma_heap initialization so that
> we expose both the default CMA region and any CMA regions
> tagged with "linux,cma-heap" in the device-tree.
> 
> Cc: Rob Herring <[e-mail]>
> Cc: Sumit Semwal <[e-mail]>
> Cc: "Andrew F. Davis" <[e-mail]>
> Cc: Benjamin Gaignard <[e-mail]>
> Cc: Liam Mark <[e-mail]>
> Cc: Pratik Patel <[e-mail]>
> Cc: Laura Abbott <[e-mail]>
> Cc: Brian Starkey <[e-mail]>
> Cc: Chenbo Feng <[e-mail]>
> Cc: Alistair Strachan <[e-mail]>
> Cc: Sandeep Patil <[e-mail]>
> Cc: Hridya Valsaraju <[e-mail]>
> Cc: Christoph Hellwig <[e-mail]>
> Cc: Marek Szyprowski <[e-mail]>
> Cc: Robin Murphy <[e-mail]>
> Cc: Andrew Morton <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Signed-off-by: John Stultz <[e-mail]>
> ---
>  drivers/dma-buf/heaps/cma_heap.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
> index 626cf7fd033a..dd154e2db101 100644
> --- a/drivers/dma-buf/heaps/cma_heap.c
> +++ b/drivers/dma-buf/heaps/cma_heap.c
> @@ -141,6 +141,11 @@ static int __add_cma_heap(struct cma *cma, void *data)
>  {
>  	struct cma_heap *cma_heap;
>  	struct dma_heap_export_info exp_info;
> +	struct cma *default_cma = dev_get_cma_area(NULL);
> +
> +	/* We only add the default heap and explicitly tagged heaps */
> +	if (cma != default_cma && !cma_dma_heap_enabled(cma))
> +		return 0;

Thinking about the pl111 thread[1], I'm wondering if we should also
let drivers call this directly to expose their CMA pools, even if they
aren't tagged for dma-heaps in DT. But perhaps that's too close to
policy.

Cheers,
-Brian

[1] https://lists.freedesktop.org/archives/dri-devel/2020-April/264358.html

>  
>  	cma_heap = kzalloc(sizeof(*cma_heap), GFP_KERNEL);
>  	if (!cma_heap)
> @@ -162,16 +167,11 @@ static int __add_cma_heap(struct cma *cma, void *data)
>  	return 0;
>  }
>  
> -static int add_default_cma_heap(void)
> +static int cma_heaps_init(void)
>  {
> -	struct cma *default_cma = dev_get_cma_area(NULL);
> -	int ret = 0;
> -
> -	if (default_cma)
> -		ret = __add_cma_heap(default_cma, NULL);
> -
> -	return ret;
> +	cma_for_each_area(__add_cma_heap, NULL);
> +	return 0;
>  }
> -module_init(add_default_cma_heap);
> +module_init(cma_heaps_init);
>  MODULE_DESCRIPTION("DMA-BUF CMA Heap");
>  MODULE_LICENSE("GPL v2");
> -- 
> 2.17.1
>

Robin Murphy May 1, 2020, 11:08 a.m. UTC | #2

On 2020-05-01 11:21 am, Brian Starkey wrote:
> Hi John,
> 
> On Fri, May 01, 2020 at 07:39:48AM +0000, John Stultz wrote:
>> This patch reworks the cma_heap initialization so that
>> we expose both the default CMA region and any CMA regions
>> tagged with "linux,cma-heap" in the device-tree.
>>
>> Cc: Rob Herring <[e-mail]>
>> Cc: Sumit Semwal <[e-mail]>
>> Cc: "Andrew F. Davis" <[e-mail]>
>> Cc: Benjamin Gaignard <[e-mail]>
>> Cc: Liam Mark <[e-mail]>
>> Cc: Pratik Patel <[e-mail]>
>> Cc: Laura Abbott <[e-mail]>
>> Cc: Brian Starkey <[e-mail]>
>> Cc: Chenbo Feng <[e-mail]>
>> Cc: Alistair Strachan <[e-mail]>
>> Cc: Sandeep Patil <[e-mail]>
>> Cc: Hridya Valsaraju <[e-mail]>
>> Cc: Christoph Hellwig <[e-mail]>
>> Cc: Marek Szyprowski <[e-mail]>
>> Cc: Robin Murphy <[e-mail]>
>> Cc: Andrew Morton <[e-mail]>
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Signed-off-by: John Stultz <[e-mail]>
>> ---
>>   drivers/dma-buf/heaps/cma_heap.c | 18 +++++++++---------
>>   1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
>> index 626cf7fd033a..dd154e2db101 100644
>> --- a/drivers/dma-buf/heaps/cma_heap.c
>> +++ b/drivers/dma-buf/heaps/cma_heap.c
>> @@ -141,6 +141,11 @@ static int __add_cma_heap(struct cma *cma, void *data)
>>   {
>>   	struct cma_heap *cma_heap;
>>   	struct dma_heap_export_info exp_info;
>> +	struct cma *default_cma = dev_get_cma_area(NULL);
>> +
>> +	/* We only add the default heap and explicitly tagged heaps */
>> +	if (cma != default_cma && !cma_dma_heap_enabled(cma))
>> +		return 0;
> 
> Thinking about the pl111 thread[1], I'm wondering if we should also
> let drivers call this directly to expose their CMA pools, even if they
> aren't tagged for dma-heaps in DT. But perhaps that's too close to
> policy.

That sounds much like what my first thoughts were - apologies if I'm 
wildly off-base here, but as far as I understand:

- Device drivers know whether they have their own "memory-region" or not.
- Device drivers already have to do *something* to participate in dma-buf.
- Device drivers know best how they make use of both the above.
- Therefore couldn't it be left to drivers to choose whether to register 
their CMA regions as heaps, without having to mess with DT at all?

Robin.

> 
> Cheers,
> -Brian
> 
> [1] https://lists.freedesktop.org/archives/dri-devel/2020-April/264358.html
> 
>>   
>>   	cma_heap = kzalloc(sizeof(*cma_heap), GFP_KERNEL);
>>   	if (!cma_heap)
>> @@ -162,16 +167,11 @@ static int __add_cma_heap(struct cma *cma, void *data)
>>   	return 0;
>>   }
>>   
>> -static int add_default_cma_heap(void)
>> +static int cma_heaps_init(void)
>>   {
>> -	struct cma *default_cma = dev_get_cma_area(NULL);
>> -	int ret = 0;
>> -
>> -	if (default_cma)
>> -		ret = __add_cma_heap(default_cma, NULL);
>> -
>> -	return ret;
>> +	cma_for_each_area(__add_cma_heap, NULL);
>> +	return 0;
>>   }
>> -module_init(add_default_cma_heap);
>> +module_init(cma_heaps_init);
>>   MODULE_DESCRIPTION("DMA-BUF CMA Heap");
>>   MODULE_LICENSE("GPL v2");
>> -- 
>> 2.17.1
>>

John Stultz May 1, 2020, 7:01 p.m. UTC | #3

On Fri, May 1, 2020 at 4:08 AM Robin Murphy <[e-mail]> wrote:
>
> On 2020-05-01 11:21 am, Brian Starkey wrote:
> > Hi John,
> >
> > On Fri, May 01, 2020 at 07:39:48AM +0000, John Stultz wrote:
> >> This patch reworks the cma_heap initialization so that
> >> we expose both the default CMA region and any CMA regions
> >> tagged with "linux,cma-heap" in the device-tree.
> >>
> >> Cc: Rob Herring <[e-mail]>
> >> Cc: Sumit Semwal <[e-mail]>
> >> Cc: "Andrew F. Davis" <[e-mail]>
> >> Cc: Benjamin Gaignard <[e-mail]>
> >> Cc: Liam Mark <[e-mail]>
> >> Cc: Pratik Patel <[e-mail]>
> >> Cc: Laura Abbott <[e-mail]>
> >> Cc: Brian Starkey <[e-mail]>
> >> Cc: Chenbo Feng <[e-mail]>
> >> Cc: Alistair Strachan <[e-mail]>
> >> Cc: Sandeep Patil <[e-mail]>
> >> Cc: Hridya Valsaraju <[e-mail]>
> >> Cc: Christoph Hellwig <[e-mail]>
> >> Cc: Marek Szyprowski <[e-mail]>
> >> Cc: Robin Murphy <[e-mail]>
> >> Cc: Andrew Morton <[e-mail]>
> >> Cc: [e-mail]
> >> Cc: [e-mail]
> >> Cc: [e-mail]
> >> Signed-off-by: John Stultz <[e-mail]>
> >> ---
> >>   drivers/dma-buf/heaps/cma_heap.c | 18 +++++++++---------
> >>   1 file changed, 9 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
> >> index 626cf7fd033a..dd154e2db101 100644
> >> --- a/drivers/dma-buf/heaps/cma_heap.c
> >> +++ b/drivers/dma-buf/heaps/cma_heap.c
> >> @@ -141,6 +141,11 @@ static int __add_cma_heap(struct cma *cma, void *data)
> >>   {
> >>      struct cma_heap *cma_heap;
> >>      struct dma_heap_export_info exp_info;
> >> +    struct cma *default_cma = dev_get_cma_area(NULL);
> >> +
> >> +    /* We only add the default heap and explicitly tagged heaps */
> >> +    if (cma != default_cma && !cma_dma_heap_enabled(cma))
> >> +            return 0;
> >
> > Thinking about the pl111 thread[1], I'm wondering if we should also
> > let drivers call this directly to expose their CMA pools, even if they
> > aren't tagged for dma-heaps in DT. But perhaps that's too close to
> > policy.
>
> That sounds much like what my first thoughts were - apologies if I'm
> wildly off-base here, but as far as I understand:
>
> - Device drivers know whether they have their own "memory-region" or not.
> - Device drivers already have to do *something* to participate in dma-buf.
> - Device drivers know best how they make use of both the above.
> - Therefore couldn't it be left to drivers to choose whether to register
> their CMA regions as heaps, without having to mess with DT at all?

I guess I'm not opposed to this. But I guess I'd like to see some more
details? You're thinking the pl111 driver would add the
"memory-region" node itself?

Assuming that's the case, my only worry is what if that memory-region
node isn't a CMA area, but instead something like a carveout? Does the
driver need to parse enough of the dt to figure out where to register
the region as a heap?

thanks
-john

Brian Starkey May 4, 2020, 9:06 a.m. UTC | #4

On Fri, May 01, 2020 at 12:01:40PM -0700, John Stultz wrote:
> On Fri, May 1, 2020 at 4:08 AM Robin Murphy <[e-mail]> wrote:
> >
> > On 2020-05-01 11:21 am, Brian Starkey wrote:
> > > Hi John,
> > >
> > > On Fri, May 01, 2020 at 07:39:48AM +0000, John Stultz wrote:
> > >> This patch reworks the cma_heap initialization so that
> > >> we expose both the default CMA region and any CMA regions
> > >> tagged with "linux,cma-heap" in the device-tree.
> > >>
> > >> Cc: Rob Herring <[e-mail]>
> > >> Cc: Sumit Semwal <[e-mail]>
> > >> Cc: "Andrew F. Davis" <[e-mail]>
> > >> Cc: Benjamin Gaignard <[e-mail]>
> > >> Cc: Liam Mark <[e-mail]>
> > >> Cc: Pratik Patel <[e-mail]>
> > >> Cc: Laura Abbott <[e-mail]>
> > >> Cc: Brian Starkey <[e-mail]>
> > >> Cc: Chenbo Feng <[e-mail]>
> > >> Cc: Alistair Strachan <[e-mail]>
> > >> Cc: Sandeep Patil <[e-mail]>
> > >> Cc: Hridya Valsaraju <[e-mail]>
> > >> Cc: Christoph Hellwig <[e-mail]>
> > >> Cc: Marek Szyprowski <[e-mail]>
> > >> Cc: Robin Murphy <[e-mail]>
> > >> Cc: Andrew Morton <[e-mail]>
> > >> Cc: [e-mail]
> > >> Cc: [e-mail]
> > >> Cc: [e-mail]
> > >> Signed-off-by: John Stultz <[e-mail]>
> > >> ---
> > >>   drivers/dma-buf/heaps/cma_heap.c | 18 +++++++++---------
> > >>   1 file changed, 9 insertions(+), 9 deletions(-)
> > >>
> > >> diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
> > >> index 626cf7fd033a..dd154e2db101 100644
> > >> --- a/drivers/dma-buf/heaps/cma_heap.c
> > >> +++ b/drivers/dma-buf/heaps/cma_heap.c
> > >> @@ -141,6 +141,11 @@ static int __add_cma_heap(struct cma *cma, void *data)
> > >>   {
> > >>      struct cma_heap *cma_heap;
> > >>      struct dma_heap_export_info exp_info;
> > >> +    struct cma *default_cma = dev_get_cma_area(NULL);
> > >> +
> > >> +    /* We only add the default heap and explicitly tagged heaps */
> > >> +    if (cma != default_cma && !cma_dma_heap_enabled(cma))
> > >> +            return 0;
> > >
> > > Thinking about the pl111 thread[1], I'm wondering if we should also
> > > let drivers call this directly to expose their CMA pools, even if they
> > > aren't tagged for dma-heaps in DT. But perhaps that's too close to
> > > policy.
> >
> > That sounds much like what my first thoughts were - apologies if I'm
> > wildly off-base here, but as far as I understand:
> >
> > - Device drivers know whether they have their own "memory-region" or not.
> > - Device drivers already have to do *something* to participate in dma-buf.
> > - Device drivers know best how they make use of both the above.
> > - Therefore couldn't it be left to drivers to choose whether to register
> > their CMA regions as heaps, without having to mess with DT at all?
> 
> I guess I'm not opposed to this. But I guess I'd like to see some more
> details? You're thinking the pl111 driver would add the
> "memory-region" node itself?
> 
> Assuming that's the case, my only worry is what if that memory-region
> node isn't a CMA area, but instead something like a carveout? Does the
> driver need to parse enough of the dt to figure out where to register
> the region as a heap?

My thinking was more like there would already be a reserved-memory
node in DT for the chunk of memory, appropriately tagged so that it
gets added as a CMA region. 

The device's node would have "memory-region=<&blah>;" and would use
of_reserved_mem_device_init() to link up dev->cma_area to the
corresponding cma region.

So far, that's all in-place already. The bit that's missing is
exposing that dev->cma_area to userspace as a dma_heap - so we could
just have "int cma_heap_add(struct cma *cma)" or "int
cma_heap_dev_add(struct device *dev)" or something exported for
drivers to expose their device-assigned cma region if they wanted to.

I don't think this runs into the lifetime problems of generalised
heaps-as-modules either, because the CMA region will never go away
even if the driver does.

Alongside that, I do think the completely DT-driven approach can be
useful too - because there may be regions which aren't associated with
any specific device driver, that we want exported as heaps.

Hope that makes sense,
-Brian

> 
> thanks
> -john

Rob Herring (Arm) May 12, 2020, 4:37 p.m. UTC | #5

On Mon, May 04, 2020 at 10:06:28AM +0100, Brian Starkey wrote:
> On Fri, May 01, 2020 at 12:01:40PM -0700, John Stultz wrote:
> > On Fri, May 1, 2020 at 4:08 AM Robin Murphy <[e-mail]> wrote:
> > >
> > > On 2020-05-01 11:21 am, Brian Starkey wrote:
> > > > Hi John,
> > > >
> > > > On Fri, May 01, 2020 at 07:39:48AM +0000, John Stultz wrote:
> > > >> This patch reworks the cma_heap initialization so that
> > > >> we expose both the default CMA region and any CMA regions
> > > >> tagged with "linux,cma-heap" in the device-tree.
> > > >>
> > > >> Cc: Rob Herring <[e-mail]>
> > > >> Cc: Sumit Semwal <[e-mail]>
> > > >> Cc: "Andrew F. Davis" <[e-mail]>
> > > >> Cc: Benjamin Gaignard <[e-mail]>
> > > >> Cc: Liam Mark <[e-mail]>
> > > >> Cc: Pratik Patel <[e-mail]>
> > > >> Cc: Laura Abbott <[e-mail]>
> > > >> Cc: Brian Starkey <[e-mail]>
> > > >> Cc: Chenbo Feng <[e-mail]>
> > > >> Cc: Alistair Strachan <[e-mail]>
> > > >> Cc: Sandeep Patil <[e-mail]>
> > > >> Cc: Hridya Valsaraju <[e-mail]>
> > > >> Cc: Christoph Hellwig <[e-mail]>
> > > >> Cc: Marek Szyprowski <[e-mail]>
> > > >> Cc: Robin Murphy <[e-mail]>
> > > >> Cc: Andrew Morton <[e-mail]>
> > > >> Cc: [e-mail]
> > > >> Cc: [e-mail]
> > > >> Cc: [e-mail]
> > > >> Signed-off-by: John Stultz <[e-mail]>
> > > >> ---
> > > >>   drivers/dma-buf/heaps/cma_heap.c | 18 +++++++++---------
> > > >>   1 file changed, 9 insertions(+), 9 deletions(-)
> > > >>
> > > >> diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
> > > >> index 626cf7fd033a..dd154e2db101 100644
> > > >> --- a/drivers/dma-buf/heaps/cma_heap.c
> > > >> +++ b/drivers/dma-buf/heaps/cma_heap.c
> > > >> @@ -141,6 +141,11 @@ static int __add_cma_heap(struct cma *cma, void *data)
> > > >>   {
> > > >>      struct cma_heap *cma_heap;
> > > >>      struct dma_heap_export_info exp_info;
> > > >> +    struct cma *default_cma = dev_get_cma_area(NULL);
> > > >> +
> > > >> +    /* We only add the default heap and explicitly tagged heaps */
> > > >> +    if (cma != default_cma && !cma_dma_heap_enabled(cma))
> > > >> +            return 0;
> > > >
> > > > Thinking about the pl111 thread[1], I'm wondering if we should also
> > > > let drivers call this directly to expose their CMA pools, even if they
> > > > aren't tagged for dma-heaps in DT. But perhaps that's too close to
> > > > policy.
> > >
> > > That sounds much like what my first thoughts were - apologies if I'm
> > > wildly off-base here, but as far as I understand:
> > >
> > > - Device drivers know whether they have their own "memory-region" or not.
> > > - Device drivers already have to do *something* to participate in dma-buf.
> > > - Device drivers know best how they make use of both the above.
> > > - Therefore couldn't it be left to drivers to choose whether to register
> > > their CMA regions as heaps, without having to mess with DT at all?

+1, but I'm biased toward any solution not using DT. :)

> > I guess I'm not opposed to this. But I guess I'd like to see some more
> > details? You're thinking the pl111 driver would add the
> > "memory-region" node itself?
> > 
> > Assuming that's the case, my only worry is what if that memory-region
> > node isn't a CMA area, but instead something like a carveout? Does the
> > driver need to parse enough of the dt to figure out where to register
> > the region as a heap?
> 
> My thinking was more like there would already be a reserved-memory
> node in DT for the chunk of memory, appropriately tagged so that it
> gets added as a CMA region. 
> 
> The device's node would have "memory-region=<&blah>;" and would use
> of_reserved_mem_device_init() to link up dev->cma_area to the
> corresponding cma region.
> 
> So far, that's all in-place already. The bit that's missing is
> exposing that dev->cma_area to userspace as a dma_heap - so we could
> just have "int cma_heap_add(struct cma *cma)" or "int
> cma_heap_dev_add(struct device *dev)" or something exported for
> drivers to expose their device-assigned cma region if they wanted to.
> 
> I don't think this runs into the lifetime problems of generalised
> heaps-as-modules either, because the CMA region will never go away
> even if the driver does.
> 
> Alongside that, I do think the completely DT-driven approach can be
> useful too - because there may be regions which aren't associated with
> any specific device driver, that we want exported as heaps.

And they are associated with the hardware description rather than the 
userspace environment? 

Rob

Brian Starkey May 13, 2020, 10:44 a.m. UTC | #6

Hi Rob,

On Tue, May 12, 2020 at 11:37:14AM -0500, Rob Herring wrote:
> On Mon, May 04, 2020 at 10:06:28AM +0100, Brian Starkey wrote:
> > On Fri, May 01, 2020 at 12:01:40PM -0700, John Stultz wrote:
> > > On Fri, May 1, 2020 at 4:08 AM Robin Murphy <[e-mail]> wrote:
> > > >
> > > > On 2020-05-01 11:21 am, Brian Starkey wrote:
> > > > > Hi John,
> > > > >
> > > > > On Fri, May 01, 2020 at 07:39:48AM +0000, John Stultz wrote:
> > > > >> This patch reworks the cma_heap initialization so that
> > > > >> we expose both the default CMA region and any CMA regions
> > > > >> tagged with "linux,cma-heap" in the device-tree.
> > > > >>
> > > > >> Cc: Rob Herring <[e-mail]>
> > > > >> Cc: Sumit Semwal <[e-mail]>
> > > > >> Cc: "Andrew F. Davis" <[e-mail]>
> > > > >> Cc: Benjamin Gaignard <[e-mail]>
> > > > >> Cc: Liam Mark <[e-mail]>
> > > > >> Cc: Pratik Patel <[e-mail]>
> > > > >> Cc: Laura Abbott <[e-mail]>
> > > > >> Cc: Brian Starkey <[e-mail]>
> > > > >> Cc: Chenbo Feng <[e-mail]>
> > > > >> Cc: Alistair Strachan <[e-mail]>
> > > > >> Cc: Sandeep Patil <[e-mail]>
> > > > >> Cc: Hridya Valsaraju <[e-mail]>
> > > > >> Cc: Christoph Hellwig <[e-mail]>
> > > > >> Cc: Marek Szyprowski <[e-mail]>
> > > > >> Cc: Robin Murphy <[e-mail]>
> > > > >> Cc: Andrew Morton <[e-mail]>
> > > > >> Cc: [e-mail]
> > > > >> Cc: [e-mail]
> > > > >> Cc: [e-mail]
> > > > >> Signed-off-by: John Stultz <[e-mail]>
> > > > >> ---
> > > > >>   drivers/dma-buf/heaps/cma_heap.c | 18 +++++++++---------
> > > > >>   1 file changed, 9 insertions(+), 9 deletions(-)
> > > > >>
> > > > >> diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
> > > > >> index 626cf7fd033a..dd154e2db101 100644
> > > > >> --- a/drivers/dma-buf/heaps/cma_heap.c
> > > > >> +++ b/drivers/dma-buf/heaps/cma_heap.c
> > > > >> @@ -141,6 +141,11 @@ static int __add_cma_heap(struct cma *cma, void *data)
> > > > >>   {
> > > > >>      struct cma_heap *cma_heap;
> > > > >>      struct dma_heap_export_info exp_info;
> > > > >> +    struct cma *default_cma = dev_get_cma_area(NULL);
> > > > >> +
> > > > >> +    /* We only add the default heap and explicitly tagged heaps */
> > > > >> +    if (cma != default_cma && !cma_dma_heap_enabled(cma))
> > > > >> +            return 0;
> > > > >
> > > > > Thinking about the pl111 thread[1], I'm wondering if we should also
> > > > > let drivers call this directly to expose their CMA pools, even if they
> > > > > aren't tagged for dma-heaps in DT. But perhaps that's too close to
> > > > > policy.
> > > >
> > > > That sounds much like what my first thoughts were - apologies if I'm
> > > > wildly off-base here, but as far as I understand:
> > > >
> > > > - Device drivers know whether they have their own "memory-region" or not.
> > > > - Device drivers already have to do *something* to participate in dma-buf.
> > > > - Device drivers know best how they make use of both the above.
> > > > - Therefore couldn't it be left to drivers to choose whether to register
> > > > their CMA regions as heaps, without having to mess with DT at all?
> 
> +1, but I'm biased toward any solution not using DT. :)
> 
> > > I guess I'm not opposed to this. But I guess I'd like to see some more
> > > details? You're thinking the pl111 driver would add the
> > > "memory-region" node itself?
> > > 
> > > Assuming that's the case, my only worry is what if that memory-region
> > > node isn't a CMA area, but instead something like a carveout? Does the
> > > driver need to parse enough of the dt to figure out where to register
> > > the region as a heap?
> > 
> > My thinking was more like there would already be a reserved-memory
> > node in DT for the chunk of memory, appropriately tagged so that it
> > gets added as a CMA region. 
> > 
> > The device's node would have "memory-region=<&blah>;" and would use
> > of_reserved_mem_device_init() to link up dev->cma_area to the
> > corresponding cma region.
> > 
> > So far, that's all in-place already. The bit that's missing is
> > exposing that dev->cma_area to userspace as a dma_heap - so we could
> > just have "int cma_heap_add(struct cma *cma)" or "int
> > cma_heap_dev_add(struct device *dev)" or something exported for
> > drivers to expose their device-assigned cma region if they wanted to.
> > 
> > I don't think this runs into the lifetime problems of generalised
> > heaps-as-modules either, because the CMA region will never go away
> > even if the driver does.
> > 
> > Alongside that, I do think the completely DT-driven approach can be
> > useful too - because there may be regions which aren't associated with
> > any specific device driver, that we want exported as heaps.
> 
> And they are associated with the hardware description rather than the 
> userspace environment? 

I'm not sure how to answer that. We already have CMA regions being
created from device-tree, so we're only talking about explicitly
exposing those to userspace.

Are you thinking that userspace should be deciding whether they get
exposed or not? I don't know how userspace would discover them in
order to make that decision.

Thanks,
-Brian

> 
> Rob

Rob Herring (Arm) May 14, 2020, 2:52 p.m. UTC | #7

On Wed, May 13, 2020 at 5:44 AM Brian Starkey <[e-mail]> wrote:
>
> Hi Rob,
>
> On Tue, May 12, 2020 at 11:37:14AM -0500, Rob Herring wrote:
> > On Mon, May 04, 2020 at 10:06:28AM +0100, Brian Starkey wrote:
> > > On Fri, May 01, 2020 at 12:01:40PM -0700, John Stultz wrote:
> > > > On Fri, May 1, 2020 at 4:08 AM Robin Murphy <[e-mail]> wrote:
> > > > >
> > > > > On 2020-05-01 11:21 am, Brian Starkey wrote:
> > > > > > Hi John,
> > > > > >
> > > > > > On Fri, May 01, 2020 at 07:39:48AM +0000, John Stultz wrote:
> > > > > >> This patch reworks the cma_heap initialization so that
> > > > > >> we expose both the default CMA region and any CMA regions
> > > > > >> tagged with "linux,cma-heap" in the device-tree.
> > > > > >>
> > > > > >> Cc: Rob Herring <[e-mail]>
> > > > > >> Cc: Sumit Semwal <[e-mail]>
> > > > > >> Cc: "Andrew F. Davis" <[e-mail]>
> > > > > >> Cc: Benjamin Gaignard <[e-mail]>
> > > > > >> Cc: Liam Mark <[e-mail]>
> > > > > >> Cc: Pratik Patel <[e-mail]>
> > > > > >> Cc: Laura Abbott <[e-mail]>
> > > > > >> Cc: Brian Starkey <[e-mail]>
> > > > > >> Cc: Chenbo Feng <[e-mail]>
> > > > > >> Cc: Alistair Strachan <[e-mail]>
> > > > > >> Cc: Sandeep Patil <[e-mail]>
> > > > > >> Cc: Hridya Valsaraju <[e-mail]>
> > > > > >> Cc: Christoph Hellwig <[e-mail]>
> > > > > >> Cc: Marek Szyprowski <[e-mail]>
> > > > > >> Cc: Robin Murphy <[e-mail]>
> > > > > >> Cc: Andrew Morton <[e-mail]>
> > > > > >> Cc: [e-mail]
> > > > > >> Cc: [e-mail]
> > > > > >> Cc: [e-mail]
> > > > > >> Signed-off-by: John Stultz <[e-mail]>
> > > > > >> ---
> > > > > >>   drivers/dma-buf/heaps/cma_heap.c | 18 +++++++++---------
> > > > > >>   1 file changed, 9 insertions(+), 9 deletions(-)
> > > > > >>
> > > > > >> diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
> > > > > >> index 626cf7fd033a..dd154e2db101 100644
> > > > > >> --- a/drivers/dma-buf/heaps/cma_heap.c
> > > > > >> +++ b/drivers/dma-buf/heaps/cma_heap.c
> > > > > >> @@ -141,6 +141,11 @@ static int __add_cma_heap(struct cma *cma, void *data)
> > > > > >>   {
> > > > > >>      struct cma_heap *cma_heap;
> > > > > >>      struct dma_heap_export_info exp_info;
> > > > > >> +    struct cma *default_cma = dev_get_cma_area(NULL);
> > > > > >> +
> > > > > >> +    /* We only add the default heap and explicitly tagged heaps */
> > > > > >> +    if (cma != default_cma && !cma_dma_heap_enabled(cma))
> > > > > >> +            return 0;
> > > > > >
> > > > > > Thinking about the pl111 thread[1], I'm wondering if we should also
> > > > > > let drivers call this directly to expose their CMA pools, even if they
> > > > > > aren't tagged for dma-heaps in DT. But perhaps that's too close to
> > > > > > policy.
> > > > >
> > > > > That sounds much like what my first thoughts were - apologies if I'm
> > > > > wildly off-base here, but as far as I understand:
> > > > >
> > > > > - Device drivers know whether they have their own "memory-region" or not.
> > > > > - Device drivers already have to do *something* to participate in dma-buf.
> > > > > - Device drivers know best how they make use of both the above.
> > > > > - Therefore couldn't it be left to drivers to choose whether to register
> > > > > their CMA regions as heaps, without having to mess with DT at all?
> >
> > +1, but I'm biased toward any solution not using DT. :)
> >
> > > > I guess I'm not opposed to this. But I guess I'd like to see some more
> > > > details? You're thinking the pl111 driver would add the
> > > > "memory-region" node itself?
> > > >
> > > > Assuming that's the case, my only worry is what if that memory-region
> > > > node isn't a CMA area, but instead something like a carveout? Does the
> > > > driver need to parse enough of the dt to figure out where to register
> > > > the region as a heap?
> > >
> > > My thinking was more like there would already be a reserved-memory
> > > node in DT for the chunk of memory, appropriately tagged so that it
> > > gets added as a CMA region.
> > >
> > > The device's node would have "memory-region=<&blah>;" and would use
> > > of_reserved_mem_device_init() to link up dev->cma_area to the
> > > corresponding cma region.
> > >
> > > So far, that's all in-place already. The bit that's missing is
> > > exposing that dev->cma_area to userspace as a dma_heap - so we could
> > > just have "int cma_heap_add(struct cma *cma)" or "int
> > > cma_heap_dev_add(struct device *dev)" or something exported for
> > > drivers to expose their device-assigned cma region if they wanted to.
> > >
> > > I don't think this runs into the lifetime problems of generalised
> > > heaps-as-modules either, because the CMA region will never go away
> > > even if the driver does.
> > >
> > > Alongside that, I do think the completely DT-driven approach can be
> > > useful too - because there may be regions which aren't associated with
> > > any specific device driver, that we want exported as heaps.
> >
> > And they are associated with the hardware description rather than the
> > userspace environment?
>
> I'm not sure how to answer that. We already have CMA regions being
> created from device-tree, so we're only talking about explicitly
> exposing those to userspace.

It's easier to argue that how much CMA memory a system/device needs is
h/w description as that's more a function of devices and frame sizes.
But exposing to userspace or not is an OS architecture decision. It's
not really any different than a kernel vs. userspace driver question.
What's exposed by UIO or spi-dev is purely a kernel thing.

> Are you thinking that userspace should be deciding whether they get
> exposed or not? I don't know how userspace would discover them in
> order to make that decision.

Or perhaps the kernel should be deciding. Expose to userspace what the
kernel doesn't need or drivers decide?

It's hard to argue against 1 new property. It's death by a 1000 cuts though.

Rob

Brian Starkey May 15, 2020, 9:32 a.m. UTC | #8

On Thu, May 14, 2020 at 09:52:35AM -0500, Rob Herring wrote:
> On Wed, May 13, 2020 at 5:44 AM Brian Starkey <[e-mail]> wrote:
> >
> > Hi Rob,
> >
> > On Tue, May 12, 2020 at 11:37:14AM -0500, Rob Herring wrote:
> > > On Mon, May 04, 2020 at 10:06:28AM +0100, Brian Starkey wrote:
> > > > On Fri, May 01, 2020 at 12:01:40PM -0700, John Stultz wrote:
> > > > > On Fri, May 1, 2020 at 4:08 AM Robin Murphy <[e-mail]> wrote:
> > > > > >
> > > > > > On 2020-05-01 11:21 am, Brian Starkey wrote:
> > > > > > > Hi John,
> > > > > > >
> > > > > > > On Fri, May 01, 2020 at 07:39:48AM +0000, John Stultz wrote:
> > > > > > >> This patch reworks the cma_heap initialization so that
> > > > > > >> we expose both the default CMA region and any CMA regions
> > > > > > >> tagged with "linux,cma-heap" in the device-tree.
> > > > > > >>
> > > > > > >> Cc: Rob Herring <[e-mail]>
> > > > > > >> Cc: Sumit Semwal <[e-mail]>
> > > > > > >> Cc: "Andrew F. Davis" <[e-mail]>
> > > > > > >> Cc: Benjamin Gaignard <[e-mail]>
> > > > > > >> Cc: Liam Mark <[e-mail]>
> > > > > > >> Cc: Pratik Patel <[e-mail]>
> > > > > > >> Cc: Laura Abbott <[e-mail]>
> > > > > > >> Cc: Brian Starkey <[e-mail]>
> > > > > > >> Cc: Chenbo Feng <[e-mail]>
> > > > > > >> Cc: Alistair Strachan <[e-mail]>
> > > > > > >> Cc: Sandeep Patil <[e-mail]>
> > > > > > >> Cc: Hridya Valsaraju <[e-mail]>
> > > > > > >> Cc: Christoph Hellwig <[e-mail]>
> > > > > > >> Cc: Marek Szyprowski <[e-mail]>
> > > > > > >> Cc: Robin Murphy <[e-mail]>
> > > > > > >> Cc: Andrew Morton <[e-mail]>
> > > > > > >> Cc: [e-mail]
> > > > > > >> Cc: [e-mail]
> > > > > > >> Cc: [e-mail]
> > > > > > >> Signed-off-by: John Stultz <[e-mail]>
> > > > > > >> ---
> > > > > > >>   drivers/dma-buf/heaps/cma_heap.c | 18 +++++++++---------
> > > > > > >>   1 file changed, 9 insertions(+), 9 deletions(-)
> > > > > > >>
> > > > > > >> diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
> > > > > > >> index 626cf7fd033a..dd154e2db101 100644
> > > > > > >> --- a/drivers/dma-buf/heaps/cma_heap.c
> > > > > > >> +++ b/drivers/dma-buf/heaps/cma_heap.c
> > > > > > >> @@ -141,6 +141,11 @@ static int __add_cma_heap(struct cma *cma, void *data)
> > > > > > >>   {
> > > > > > >>      struct cma_heap *cma_heap;
> > > > > > >>      struct dma_heap_export_info exp_info;
> > > > > > >> +    struct cma *default_cma = dev_get_cma_area(NULL);
> > > > > > >> +
> > > > > > >> +    /* We only add the default heap and explicitly tagged heaps */
> > > > > > >> +    if (cma != default_cma && !cma_dma_heap_enabled(cma))
> > > > > > >> +            return 0;
> > > > > > >
> > > > > > > Thinking about the pl111 thread[1], I'm wondering if we should also
> > > > > > > let drivers call this directly to expose their CMA pools, even if they
> > > > > > > aren't tagged for dma-heaps in DT. But perhaps that's too close to
> > > > > > > policy.
> > > > > >
> > > > > > That sounds much like what my first thoughts were - apologies if I'm
> > > > > > wildly off-base here, but as far as I understand:
> > > > > >
> > > > > > - Device drivers know whether they have their own "memory-region" or not.
> > > > > > - Device drivers already have to do *something* to participate in dma-buf.
> > > > > > - Device drivers know best how they make use of both the above.
> > > > > > - Therefore couldn't it be left to drivers to choose whether to register
> > > > > > their CMA regions as heaps, without having to mess with DT at all?
> > >
> > > +1, but I'm biased toward any solution not using DT. :)
> > >
> > > > > I guess I'm not opposed to this. But I guess I'd like to see some more
> > > > > details? You're thinking the pl111 driver would add the
> > > > > "memory-region" node itself?
> > > > >
> > > > > Assuming that's the case, my only worry is what if that memory-region
> > > > > node isn't a CMA area, but instead something like a carveout? Does the
> > > > > driver need to parse enough of the dt to figure out where to register
> > > > > the region as a heap?
> > > >
> > > > My thinking was more like there would already be a reserved-memory
> > > > node in DT for the chunk of memory, appropriately tagged so that it
> > > > gets added as a CMA region.
> > > >
> > > > The device's node would have "memory-region=<&blah>;" and would use
> > > > of_reserved_mem_device_init() to link up dev->cma_area to the
> > > > corresponding cma region.
> > > >
> > > > So far, that's all in-place already. The bit that's missing is
> > > > exposing that dev->cma_area to userspace as a dma_heap - so we could
> > > > just have "int cma_heap_add(struct cma *cma)" or "int
> > > > cma_heap_dev_add(struct device *dev)" or something exported for
> > > > drivers to expose their device-assigned cma region if they wanted to.
> > > >
> > > > I don't think this runs into the lifetime problems of generalised
> > > > heaps-as-modules either, because the CMA region will never go away
> > > > even if the driver does.
> > > >
> > > > Alongside that, I do think the completely DT-driven approach can be
> > > > useful too - because there may be regions which aren't associated with
> > > > any specific device driver, that we want exported as heaps.
> > >
> > > And they are associated with the hardware description rather than the
> > > userspace environment?
> >
> > I'm not sure how to answer that. We already have CMA regions being
> > created from device-tree, so we're only talking about explicitly
> > exposing those to userspace.
> 
> It's easier to argue that how much CMA memory a system/device needs is
> h/w description as that's more a function of devices and frame sizes.
> But exposing to userspace or not is an OS architecture decision. It's
> not really any different than a kernel vs. userspace driver question.
> What's exposed by UIO or spi-dev is purely a kernel thing.
> 
> > Are you thinking that userspace should be deciding whether they get
> > exposed or not? I don't know how userspace would discover them in
> > order to make that decision.
> 
> Or perhaps the kernel should be deciding. Expose to userspace what the
> kernel doesn't need or drivers decide?

If not via device-tree how would the kernel make that decision? For
regions which get "claimed" by a device/driver, obviously that driver
can make the decision, and I totally think we should provide a way for
them to expose it.

But for something like a shared pool amongst the media subsystem,
there may not be any specific single device/driver - and creating a
dummy driver with the sole purpose of exposing the heap doesn't seem
like an improvement over a simple "linux,cma-heap".

Cheers,
-Brian

> 
> It's hard to argue against 1 new property. It's death by a 1000 cuts though.
> 
> Rob

Patch

diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
index 626cf7fd033a..dd154e2db101 100644
--- a/drivers/dma-buf/heaps/cma_heap.c
+++ b/drivers/dma-buf/heaps/cma_heap.c
@@ -141,6 +141,11 @@  static int __add_cma_heap(struct cma *cma, void *data)
 {
 	struct cma_heap *cma_heap;
 	struct dma_heap_export_info exp_info;
+	struct cma *default_cma = dev_get_cma_area(NULL);
+
+	/* We only add the default heap and explicitly tagged heaps */
+	if (cma != default_cma && !cma_dma_heap_enabled(cma))
+		return 0;
 
 	cma_heap = kzalloc(sizeof(*cma_heap), GFP_KERNEL);
 	if (!cma_heap)
@@ -162,16 +167,11 @@  static int __add_cma_heap(struct cma *cma, void *data)
 	return 0;
 }
 
-static int add_default_cma_heap(void)
+static int cma_heaps_init(void)
 {
-	struct cma *default_cma = dev_get_cma_area(NULL);
-	int ret = 0;
-
-	if (default_cma)
-		ret = __add_cma_heap(default_cma, NULL);
-
-	return ret;
+	cma_for_each_area(__add_cma_heap, NULL);
+	return 0;
 }
-module_init(add_default_cma_heap);
+module_init(cma_heaps_init);
 MODULE_DESCRIPTION("DMA-BUF CMA Heap");
 MODULE_LICENSE("GPL v2");