| Message ID | 20200501073949.120396-1-john.stultz@linaro.org |
|---|---|
| Headers | show |
| Series | Support non-default CMA regions to the dmabuf heaps interface | expand |
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 <robh+dt@kernel.org> > Cc: Sumit Semwal <sumit.semwal@linaro.org> > Cc: "Andrew F. Davis" <afd@ti.com> > Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org> > Cc: Liam Mark <lmark@codeaurora.org> > Cc: Pratik Patel <pratikp@codeaurora.org> > Cc: Laura Abbott <labbott@redhat.com> > Cc: Brian Starkey <Brian.Starkey@arm.com> > Cc: Chenbo Feng <fengc@google.com> > Cc: Alistair Strachan <astrachan@google.com> > Cc: Sandeep Patil <sspatil@google.com> > Cc: Hridya Valsaraju <hridya@google.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Marek Szyprowski <m.szyprowski@samsung.com> > Cc: Robin Murphy <robin.murphy@arm.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: devicetree@vger.kernel.org > Cc: dri-devel@lists.freedesktop.org > Cc: linux-mm@kvack.org > Signed-off-by: John Stultz <john.stultz@linaro.org> > --- > 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 >
On Fri, May 01, 2020 at 07:39:47AM +0000, John Stultz wrote: > This patch adds a dma_heap flag on the cma structure, > along with accessors to set and read the flag. > > This is then used to process and store the "linux,cma-heap" > property documented in the previous patch. > > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Sumit Semwal <sumit.semwal@linaro.org> > Cc: "Andrew F. Davis" <afd@ti.com> > Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org> > Cc: Liam Mark <lmark@codeaurora.org> > Cc: Pratik Patel <pratikp@codeaurora.org> > Cc: Laura Abbott <labbott@redhat.com> > Cc: Brian Starkey <Brian.Starkey@arm.com> > Cc: Chenbo Feng <fengc@google.com> > Cc: Alistair Strachan <astrachan@google.com> > Cc: Sandeep Patil <sspatil@google.com> > Cc: Hridya Valsaraju <hridya@google.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Marek Szyprowski <m.szyprowski@samsung.com> > Cc: Robin Murphy <robin.murphy@arm.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: devicetree@vger.kernel.org > Cc: dri-devel@lists.freedesktop.org > Cc: linux-mm@kvack.org > Signed-off-by: John Stultz <john.stultz@linaro.org> > --- > include/linux/cma.h | 3 +++ > kernel/dma/contiguous.c | 3 +++ > mm/cma.c | 11 +++++++++++ > mm/cma.h | 1 + > 4 files changed, 18 insertions(+) > > diff --git a/include/linux/cma.h b/include/linux/cma.h > index 6ff79fefd01f..d8b8e6ce221c 100644 > --- a/include/linux/cma.h > +++ b/include/linux/cma.h > @@ -25,6 +25,9 @@ extern phys_addr_t cma_get_base(const struct cma *cma); > extern unsigned long cma_get_size(const struct cma *cma); > extern const char *cma_get_name(const struct cma *cma); > > +extern void __init cma_enable_dma_heap(struct cma *cma, bool enabled); > +extern bool cma_dma_heap_enabled(struct cma *cma); > + > extern int __init cma_declare_contiguous_nid(phys_addr_t base, > phys_addr_t size, phys_addr_t limit, > phys_addr_t alignment, unsigned int order_per_bit, > diff --git a/kernel/dma/contiguous.c b/kernel/dma/contiguous.c > index 8bc6f2d670f9..f667fd51daa2 100644 > --- a/kernel/dma/contiguous.c > +++ b/kernel/dma/contiguous.c > @@ -303,6 +303,7 @@ static int __init rmem_cma_setup(struct reserved_mem *rmem) > phys_addr_t mask = align - 1; > unsigned long node = rmem->fdt_node; > bool default_cma = of_get_flat_dt_prop(node, "linux,cma-default", NULL); > + bool heap_exported = of_get_flat_dt_prop(node, "linux,cma-heap", NULL); > struct cma *cma; > int err; > > @@ -332,6 +333,8 @@ static int __init rmem_cma_setup(struct reserved_mem *rmem) > if (default_cma) > dma_contiguous_set_default(cma); > > + cma_enable_dma_heap(cma, heap_exported); > + > rmem->ops = &rmem_cma_ops; > rmem->priv = cma; > > diff --git a/mm/cma.c b/mm/cma.c > index 0463ad2ce06b..ec671bd8f66e 100644 > --- a/mm/cma.c > +++ b/mm/cma.c > @@ -55,6 +55,16 @@ const char *cma_get_name(const struct cma *cma) > return cma->name ? cma->name : "(undefined)"; > } > > +void __init cma_enable_dma_heap(struct cma *cma, bool enabled) > +{ > + cma->dma_heap = enabled; > +} > + > +bool cma_dma_heap_enabled(struct cma *cma) > +{ > + return !!cma->dma_heap; Stylistic thing, but I don't think the !! is really necessary. It's already a bool anyway. > +} > + > static unsigned long cma_bitmap_aligned_mask(const struct cma *cma, > unsigned int align_order) > { > @@ -157,6 +167,7 @@ static int __init cma_init_reserved_areas(void) > } > core_initcall(cma_init_reserved_areas); > > + nit: spurious newline Cheers, -Brian > /** > * cma_init_reserved_mem() - create custom contiguous area from reserved memory > * @base: Base address of the reserved area > diff --git a/mm/cma.h b/mm/cma.h > index 33c0b517733c..6fe2242c724f 100644 > --- a/mm/cma.h > +++ b/mm/cma.h > @@ -13,6 +13,7 @@ struct cma { > spinlock_t mem_head_lock; > #endif > const char *name; > + bool dma_heap; > }; > > extern struct cma cma_areas[MAX_CMA_AREAS]; > -- > 2.17.1 >
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 <robh+dt@kernel.org> >> Cc: Sumit Semwal <sumit.semwal@linaro.org> >> Cc: "Andrew F. Davis" <afd@ti.com> >> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org> >> Cc: Liam Mark <lmark@codeaurora.org> >> Cc: Pratik Patel <pratikp@codeaurora.org> >> Cc: Laura Abbott <labbott@redhat.com> >> Cc: Brian Starkey <Brian.Starkey@arm.com> >> Cc: Chenbo Feng <fengc@google.com> >> Cc: Alistair Strachan <astrachan@google.com> >> Cc: Sandeep Patil <sspatil@google.com> >> Cc: Hridya Valsaraju <hridya@google.com> >> Cc: Christoph Hellwig <hch@lst.de> >> Cc: Marek Szyprowski <m.szyprowski@samsung.com> >> Cc: Robin Murphy <robin.murphy@arm.com> >> Cc: Andrew Morton <akpm@linux-foundation.org> >> Cc: devicetree@vger.kernel.org >> Cc: dri-devel@lists.freedesktop.org >> Cc: linux-mm@kvack.org >> Signed-off-by: John Stultz <john.stultz@linaro.org> >> --- >> 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 >>
On Fri, May 1, 2020 at 3:48 AM Brian Starkey <brian.starkey@arm.com> wrote: > On Fri, May 01, 2020 at 07:39:47AM +0000, John Stultz wrote: > > +bool cma_dma_heap_enabled(struct cma *cma) > > +{ > > + return !!cma->dma_heap; > > Stylistic thing, but I don't think the !! is really necessary. It's > already a bool anyway. Yea, I was using a bit field earlier and then moved to a bool for simplicity and left this. I saw it as soon as I sent the patch, so it's already fixed up. > > @@ -157,6 +167,7 @@ static int __init cma_init_reserved_areas(void) > > } > > core_initcall(cma_init_reserved_areas); > > > > + > > nit: spurious newline Yep. Same. The things you only see once the mail is sent. :) Thanks so much for the review though! -john
On Fri, May 1, 2020 at 4:08 AM Robin Murphy <robin.murphy@arm.com> 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 <robh+dt@kernel.org> > >> Cc: Sumit Semwal <sumit.semwal@linaro.org> > >> Cc: "Andrew F. Davis" <afd@ti.com> > >> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org> > >> Cc: Liam Mark <lmark@codeaurora.org> > >> Cc: Pratik Patel <pratikp@codeaurora.org> > >> Cc: Laura Abbott <labbott@redhat.com> > >> Cc: Brian Starkey <Brian.Starkey@arm.com> > >> Cc: Chenbo Feng <fengc@google.com> > >> Cc: Alistair Strachan <astrachan@google.com> > >> Cc: Sandeep Patil <sspatil@google.com> > >> Cc: Hridya Valsaraju <hridya@google.com> > >> Cc: Christoph Hellwig <hch@lst.de> > >> Cc: Marek Szyprowski <m.szyprowski@samsung.com> > >> Cc: Robin Murphy <robin.murphy@arm.com> > >> Cc: Andrew Morton <akpm@linux-foundation.org> > >> Cc: devicetree@vger.kernel.org > >> Cc: dri-devel@lists.freedesktop.org > >> Cc: linux-mm@kvack.org > >> Signed-off-by: John Stultz <john.stultz@linaro.org> > >> --- > >> 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
On Fri, May 01, 2020 at 12:01:40PM -0700, John Stultz wrote: > On Fri, May 1, 2020 at 4:08 AM Robin Murphy <robin.murphy@arm.com> 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 <robh+dt@kernel.org> > > >> Cc: Sumit Semwal <sumit.semwal@linaro.org> > > >> Cc: "Andrew F. Davis" <afd@ti.com> > > >> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org> > > >> Cc: Liam Mark <lmark@codeaurora.org> > > >> Cc: Pratik Patel <pratikp@codeaurora.org> > > >> Cc: Laura Abbott <labbott@redhat.com> > > >> Cc: Brian Starkey <Brian.Starkey@arm.com> > > >> Cc: Chenbo Feng <fengc@google.com> > > >> Cc: Alistair Strachan <astrachan@google.com> > > >> Cc: Sandeep Patil <sspatil@google.com> > > >> Cc: Hridya Valsaraju <hridya@google.com> > > >> Cc: Christoph Hellwig <hch@lst.de> > > >> Cc: Marek Szyprowski <m.szyprowski@samsung.com> > > >> Cc: Robin Murphy <robin.murphy@arm.com> > > >> Cc: Andrew Morton <akpm@linux-foundation.org> > > >> Cc: devicetree@vger.kernel.org > > >> Cc: dri-devel@lists.freedesktop.org > > >> Cc: linux-mm@kvack.org > > >> Signed-off-by: John Stultz <john.stultz@linaro.org> > > >> --- > > >> 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
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 <robin.murphy@arm.com> 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 <robh+dt@kernel.org> > > > >> Cc: Sumit Semwal <sumit.semwal@linaro.org> > > > >> Cc: "Andrew F. Davis" <afd@ti.com> > > > >> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org> > > > >> Cc: Liam Mark <lmark@codeaurora.org> > > > >> Cc: Pratik Patel <pratikp@codeaurora.org> > > > >> Cc: Laura Abbott <labbott@redhat.com> > > > >> Cc: Brian Starkey <Brian.Starkey@arm.com> > > > >> Cc: Chenbo Feng <fengc@google.com> > > > >> Cc: Alistair Strachan <astrachan@google.com> > > > >> Cc: Sandeep Patil <sspatil@google.com> > > > >> Cc: Hridya Valsaraju <hridya@google.com> > > > >> Cc: Christoph Hellwig <hch@lst.de> > > > >> Cc: Marek Szyprowski <m.szyprowski@samsung.com> > > > >> Cc: Robin Murphy <robin.murphy@arm.com> > > > >> Cc: Andrew Morton <akpm@linux-foundation.org> > > > >> Cc: devicetree@vger.kernel.org > > > >> Cc: dri-devel@lists.freedesktop.org > > > >> Cc: linux-mm@kvack.org > > > >> Signed-off-by: John Stultz <john.stultz@linaro.org> > > > >> --- > > > >> 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
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 <robin.murphy@arm.com> 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 <robh+dt@kernel.org> > > > > >> Cc: Sumit Semwal <sumit.semwal@linaro.org> > > > > >> Cc: "Andrew F. Davis" <afd@ti.com> > > > > >> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org> > > > > >> Cc: Liam Mark <lmark@codeaurora.org> > > > > >> Cc: Pratik Patel <pratikp@codeaurora.org> > > > > >> Cc: Laura Abbott <labbott@redhat.com> > > > > >> Cc: Brian Starkey <Brian.Starkey@arm.com> > > > > >> Cc: Chenbo Feng <fengc@google.com> > > > > >> Cc: Alistair Strachan <astrachan@google.com> > > > > >> Cc: Sandeep Patil <sspatil@google.com> > > > > >> Cc: Hridya Valsaraju <hridya@google.com> > > > > >> Cc: Christoph Hellwig <hch@lst.de> > > > > >> Cc: Marek Szyprowski <m.szyprowski@samsung.com> > > > > >> Cc: Robin Murphy <robin.murphy@arm.com> > > > > >> Cc: Andrew Morton <akpm@linux-foundation.org> > > > > >> Cc: devicetree@vger.kernel.org > > > > >> Cc: dri-devel@lists.freedesktop.org > > > > >> Cc: linux-mm@kvack.org > > > > >> Signed-off-by: John Stultz <john.stultz@linaro.org> > > > > >> --- > > > > >> 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
On Wed, May 13, 2020 at 5:44 AM Brian Starkey <brian.starkey@arm.com> 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 <robin.murphy@arm.com> 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 <robh+dt@kernel.org> > > > > > >> Cc: Sumit Semwal <sumit.semwal@linaro.org> > > > > > >> Cc: "Andrew F. Davis" <afd@ti.com> > > > > > >> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org> > > > > > >> Cc: Liam Mark <lmark@codeaurora.org> > > > > > >> Cc: Pratik Patel <pratikp@codeaurora.org> > > > > > >> Cc: Laura Abbott <labbott@redhat.com> > > > > > >> Cc: Brian Starkey <Brian.Starkey@arm.com> > > > > > >> Cc: Chenbo Feng <fengc@google.com> > > > > > >> Cc: Alistair Strachan <astrachan@google.com> > > > > > >> Cc: Sandeep Patil <sspatil@google.com> > > > > > >> Cc: Hridya Valsaraju <hridya@google.com> > > > > > >> Cc: Christoph Hellwig <hch@lst.de> > > > > > >> Cc: Marek Szyprowski <m.szyprowski@samsung.com> > > > > > >> Cc: Robin Murphy <robin.murphy@arm.com> > > > > > >> Cc: Andrew Morton <akpm@linux-foundation.org> > > > > > >> Cc: devicetree@vger.kernel.org > > > > > >> Cc: dri-devel@lists.freedesktop.org > > > > > >> Cc: linux-mm@kvack.org > > > > > >> Signed-off-by: John Stultz <john.stultz@linaro.org> > > > > > >> --- > > > > > >> 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
On Thu, May 14, 2020 at 09:52:35AM -0500, Rob Herring wrote: > On Wed, May 13, 2020 at 5:44 AM Brian Starkey <brian.starkey@arm.com> 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 <robin.murphy@arm.com> 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 <robh+dt@kernel.org> > > > > > > >> Cc: Sumit Semwal <sumit.semwal@linaro.org> > > > > > > >> Cc: "Andrew F. Davis" <afd@ti.com> > > > > > > >> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org> > > > > > > >> Cc: Liam Mark <lmark@codeaurora.org> > > > > > > >> Cc: Pratik Patel <pratikp@codeaurora.org> > > > > > > >> Cc: Laura Abbott <labbott@redhat.com> > > > > > > >> Cc: Brian Starkey <Brian.Starkey@arm.com> > > > > > > >> Cc: Chenbo Feng <fengc@google.com> > > > > > > >> Cc: Alistair Strachan <astrachan@google.com> > > > > > > >> Cc: Sandeep Patil <sspatil@google.com> > > > > > > >> Cc: Hridya Valsaraju <hridya@google.com> > > > > > > >> Cc: Christoph Hellwig <hch@lst.de> > > > > > > >> Cc: Marek Szyprowski <m.szyprowski@samsung.com> > > > > > > >> Cc: Robin Murphy <robin.murphy@arm.com> > > > > > > >> Cc: Andrew Morton <akpm@linux-foundation.org> > > > > > > >> Cc: devicetree@vger.kernel.org > > > > > > >> Cc: dri-devel@lists.freedesktop.org > > > > > > >> Cc: linux-mm@kvack.org > > > > > > >> Signed-off-by: John Stultz <john.stultz@linaro.org> > > > > > > >> --- > > > > > > >> 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
This is a much belated second stab at allowing non-default CMA regions to be exposed via the dmabuf heaps interface. Previous attempt was here: https://lore.kernel.org/lkml/20191025225009.50305-2-john.stultz@linaro.org/T/ This pass tried to take Rob's earlier suggestion to use a flag property. Feedback would be greatly welcome! thanks -john Cc: Rob Herring <robh+dt@kernel.org> Cc: Sumit Semwal <sumit.semwal@linaro.org> Cc: "Andrew F. Davis" <afd@ti.com> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org> Cc: Liam Mark <lmark@codeaurora.org> Cc: Pratik Patel <pratikp@codeaurora.org> Cc: Laura Abbott <labbott@redhat.com> Cc: Brian Starkey <Brian.Starkey@arm.com> Cc: Chenbo Feng <fengc@google.com> Cc: Alistair Strachan <astrachan@google.com> Cc: Sandeep Patil <sspatil@google.com> Cc: Hridya Valsaraju <hridya@google.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Marek Szyprowski <m.szyprowski@samsung.com> Cc: Robin Murphy <robin.murphy@arm.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: devicetree@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Cc: linux-mm@kvack.org John Stultz (4): devicetree: bindings: Add linux,cma-heap tag for reserved memory mm: cma: Add dma_heap flag to cma structure dma-buf: cma_heap: Extend logic to export CMA regions tagged with "linux,cma-heap" example: dts: hi3660-hikey960: Add dts entries to test cma heap binding .../reserved-memory/reserved-memory.txt | 3 +++ .../boot/dts/hisilicon/hi3660-hikey960.dts | 7 +++++++ drivers/dma-buf/heaps/cma_heap.c | 18 +++++++++--------- include/linux/cma.h | 3 +++ kernel/dma/contiguous.c | 3 +++ mm/cma.c | 11 +++++++++++ mm/cma.h | 1 + 7 files changed, 37 insertions(+), 9 deletions(-)