Re: [PATCH v2 3/3] remoteproc: core: add rproc ops for memory allocation

[Bjorn Andersson]

On Fri 16 Sep 00:47 PDT 2016, loic pallardy wrote:

> 
> 
> On 09/15/2016 07:27 PM, Bjorn Andersson wrote:
> >On Tue 06 Sep 00:39 PDT 2016, Loic Pallardy wrote:
> >
> >>Remoteproc core is currently using dma_alloc_coherent for
> >>carveout and vring allocation.
> >>It doesn't allow to support specific use cases like fixed memory
> >>region or internal RAM support.
> >>
> >>Two new rproc ops (alloc and free) is added to provide flexibility
> >>to platform implementation to provide specific memory allocator
> >>taking into account coprocessor characteristics.
> >>rproc_handle_carveout and rproc_alloc_vring functions are modified
> >>to invoke these ops if present, and fallback to regular processing
> >>if platform specific allocation failed and if resquested memory is
> >>not fixed (physical address == FW_RSC_ADDR_ANY)
> >>
> >>Signed-off-by: Loic Pallardy 
> >>---
> >> drivers/remoteproc/remoteproc_core.c | 67 
> >> ++--
> >> include/linux/remoteproc.h   |  4 +++
> >> 2 files changed, 60 insertions(+), 11 deletions(-)
> >>
> >>diff --git a/drivers/remoteproc/remoteproc_core.c 
> >>b/drivers/remoteproc/remoteproc_core.c
> >>index 0d3c191..7493b08 100644
> >>--- a/drivers/remoteproc/remoteproc_core.c
> >>+++ b/drivers/remoteproc/remoteproc_core.c
> >>@@ -207,19 +207,29 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
> >>struct rproc_vring *rvring = >vring[i];
> >>struct fw_rsc_vdev *rsc;
> >>dma_addr_t dma;
> >>-   void *va;
> >>+   void *va = NULL;
> >>int ret, size, notifyid;
> >>
> >>/* actual size of vring (in bytes) */
> >>size = PAGE_ALIGN(vring_size(rvring->len, rvring->align));
> >>
> >>+   rsc = (void *)rproc->table_ptr + rvdev->rsc_offset;
> >>+
> >>/*
> >> * Allocate non-cacheable memory for the vring. In the future
> >> * this call will also configure the IOMMU for us
> >> */
> >>-   va = dma_alloc_coherent(dev->parent, size, , GFP_KERNEL);
> >>+
> >>+   dma = rsc->vring[i].pa;
> >>+
> >>+   if (rproc->ops->alloc)
> >>+   va = rproc->ops->alloc(rproc, size, );
> >
> >I believe this will be awkward for the remoteproc drivers to implement.
> >
> >Imagine a driver that programmatically register some fixed positioned
> >carveouts and ioremapped vring buffers, it would then need internal book
> >keeping to figure out which type of allocation each call is related to.
> 
> Yes true like any allocator does. And it is needed to manage region overlap.

Right, but I'm hoping we don't have to make each remoteproc driver an
allocator - that we rather just have the drivers register a set of
regions with the core and then that's matched with the resource table.

Otherwise there will be a lot of duplicated boilerplate code in the drivers.

> >
> >
> >Rather then deferring the allocation until this point I think we should
> >tie a rproc_mem_entry to each vring and once we reach
> >rproc_alloc_vring() we simply use "va" and "dma" from that.
> >
> >We would get this from rproc_parse_vring() checking to find an existing
> >mem_entry matching the vring requirements (da, then pa) and falling back
> >to allocating a new carveout mem_entry.
> >
> This doesn't answer to use case described by Suman. What if no specific
> address are requested in firmware resource table, but buffers need to be
> allocated in internal RAM for example. Only rproc driver will know on which
> allocator to rely.
> 

If the vrings are listed with FW_RSC_ADDR_ANY as both da and pa, then we
have no way to match it towards an allocation and the same would go for
your proposed API. The alloc() function would not know if the request is
for a carveout or vring - or for which vring it is.

For the case of the vrings residing in some device memory that we
ioremap it makes sense to specify the "pa" for this in the resource
table and we can match this towards a rproc_mem_entry.

For the case of the vrings being allocated from sram using a dynamic
allocator, we're out of luck with the current resource table - we have
nothing to match this on.
By associating a rproc_mem_entry to the vring a driver could
programmatically register a vdev with a set of vrings with oddly
allocated memory. But there is no standard way of communicating these
addresses to the remote.

> By memremaping a complete memory area and offering va to dma (pa)
> conversion, you don't verify possible overlap between requested regions.
> This is done today by allocator.
> 

I share this view, I don't think we should rely on da_to_va() here, but
rather only match whole rproc_mem_entries.

> The idea from ST pov, was to rely on memory region, to declare subdev
> associated to rproc driver and to rely on dma_alloc_coherent.
> I think TI wants to rely on its internal RAM memory allocator.
> 

There are additional constraints, beyond using a fixed "pa" that makes
the ST suggestion of creating a dma-dev worth while - e.g. hardware that
can only address parts of RAM.
But as we've 

Re: [PATCH v2 3/3] remoteproc: core: add rproc ops for memory allocation

[Bjorn Andersson]

On Fri 16 Sep 00:47 PDT 2016, loic pallardy wrote:

> 
> 
> On 09/15/2016 07:27 PM, Bjorn Andersson wrote:
> >On Tue 06 Sep 00:39 PDT 2016, Loic Pallardy wrote:
> >
> >>Remoteproc core is currently using dma_alloc_coherent for
> >>carveout and vring allocation.
> >>It doesn't allow to support specific use cases like fixed memory
> >>region or internal RAM support.
> >>
> >>Two new rproc ops (alloc and free) is added to provide flexibility
> >>to platform implementation to provide specific memory allocator
> >>taking into account coprocessor characteristics.
> >>rproc_handle_carveout and rproc_alloc_vring functions are modified
> >>to invoke these ops if present, and fallback to regular processing
> >>if platform specific allocation failed and if resquested memory is
> >>not fixed (physical address == FW_RSC_ADDR_ANY)
> >>
> >>Signed-off-by: Loic Pallardy 
> >>---
> >> drivers/remoteproc/remoteproc_core.c | 67 
> >> ++--
> >> include/linux/remoteproc.h   |  4 +++
> >> 2 files changed, 60 insertions(+), 11 deletions(-)
> >>
> >>diff --git a/drivers/remoteproc/remoteproc_core.c 
> >>b/drivers/remoteproc/remoteproc_core.c
> >>index 0d3c191..7493b08 100644
> >>--- a/drivers/remoteproc/remoteproc_core.c
> >>+++ b/drivers/remoteproc/remoteproc_core.c
> >>@@ -207,19 +207,29 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
> >>struct rproc_vring *rvring = >vring[i];
> >>struct fw_rsc_vdev *rsc;
> >>dma_addr_t dma;
> >>-   void *va;
> >>+   void *va = NULL;
> >>int ret, size, notifyid;
> >>
> >>/* actual size of vring (in bytes) */
> >>size = PAGE_ALIGN(vring_size(rvring->len, rvring->align));
> >>
> >>+   rsc = (void *)rproc->table_ptr + rvdev->rsc_offset;
> >>+
> >>/*
> >> * Allocate non-cacheable memory for the vring. In the future
> >> * this call will also configure the IOMMU for us
> >> */
> >>-   va = dma_alloc_coherent(dev->parent, size, , GFP_KERNEL);
> >>+
> >>+   dma = rsc->vring[i].pa;
> >>+
> >>+   if (rproc->ops->alloc)
> >>+   va = rproc->ops->alloc(rproc, size, );
> >
> >I believe this will be awkward for the remoteproc drivers to implement.
> >
> >Imagine a driver that programmatically register some fixed positioned
> >carveouts and ioremapped vring buffers, it would then need internal book
> >keeping to figure out which type of allocation each call is related to.
> 
> Yes true like any allocator does. And it is needed to manage region overlap.

Right, but I'm hoping we don't have to make each remoteproc driver an
allocator - that we rather just have the drivers register a set of
regions with the core and then that's matched with the resource table.

Otherwise there will be a lot of duplicated boilerplate code in the drivers.

> >
> >
> >Rather then deferring the allocation until this point I think we should
> >tie a rproc_mem_entry to each vring and once we reach
> >rproc_alloc_vring() we simply use "va" and "dma" from that.
> >
> >We would get this from rproc_parse_vring() checking to find an existing
> >mem_entry matching the vring requirements (da, then pa) and falling back
> >to allocating a new carveout mem_entry.
> >
> This doesn't answer to use case described by Suman. What if no specific
> address are requested in firmware resource table, but buffers need to be
> allocated in internal RAM for example. Only rproc driver will know on which
> allocator to rely.
> 

If the vrings are listed with FW_RSC_ADDR_ANY as both da and pa, then we
have no way to match it towards an allocation and the same would go for
your proposed API. The alloc() function would not know if the request is
for a carveout or vring - or for which vring it is.

For the case of the vrings residing in some device memory that we
ioremap it makes sense to specify the "pa" for this in the resource
table and we can match this towards a rproc_mem_entry.

For the case of the vrings being allocated from sram using a dynamic
allocator, we're out of luck with the current resource table - we have
nothing to match this on.
By associating a rproc_mem_entry to the vring a driver could
programmatically register a vdev with a set of vrings with oddly
allocated memory. But there is no standard way of communicating these
addresses to the remote.

> By memremaping a complete memory area and offering va to dma (pa)
> conversion, you don't verify possible overlap between requested regions.
> This is done today by allocator.
> 

I share this view, I don't think we should rely on da_to_va() here, but
rather only match whole rproc_mem_entries.

> The idea from ST pov, was to rely on memory region, to declare subdev
> associated to rproc driver and to rely on dma_alloc_coherent.
> I think TI wants to rely on its internal RAM memory allocator.
> 

There are additional constraints, beyond using a fixed "pa" that makes
the ST suggestion of creating a dma-dev worth while - e.g. hardware that
can only address parts of RAM.
But as we've concluded we have an 

Re: [PATCH v2 3/3] remoteproc: core: add rproc ops for memory allocation

[loic pallardy]

On 09/15/2016 07:27 PM, Bjorn Andersson wrote:

On Tue 06 Sep 00:39 PDT 2016, Loic Pallardy wrote:


Remoteproc core is currently using dma_alloc_coherent for
carveout and vring allocation.
It doesn't allow to support specific use cases like fixed memory
region or internal RAM support.

Two new rproc ops (alloc and free) is added to provide flexibility
to platform implementation to provide specific memory allocator
taking into account coprocessor characteristics.
rproc_handle_carveout and rproc_alloc_vring functions are modified
to invoke these ops if present, and fallback to regular processing
if platform specific allocation failed and if resquested memory is
not fixed (physical address == FW_RSC_ADDR_ANY)

Signed-off-by: Loic Pallardy 
---
 drivers/remoteproc/remoteproc_core.c | 67 ++--
 include/linux/remoteproc.h   |  4 +++
 2 files changed, 60 insertions(+), 11 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c 
b/drivers/remoteproc/remoteproc_core.c
index 0d3c191..7493b08 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -207,19 +207,29 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
struct rproc_vring *rvring = >vring[i];
struct fw_rsc_vdev *rsc;
dma_addr_t dma;
-   void *va;
+   void *va = NULL;
int ret, size, notifyid;

/* actual size of vring (in bytes) */
size = PAGE_ALIGN(vring_size(rvring->len, rvring->align));

+   rsc = (void *)rproc->table_ptr + rvdev->rsc_offset;
+
/*
 * Allocate non-cacheable memory for the vring. In the future
 * this call will also configure the IOMMU for us
 */
-   va = dma_alloc_coherent(dev->parent, size, , GFP_KERNEL);
+
+   dma = rsc->vring[i].pa;
+
+   if (rproc->ops->alloc)
+   va = rproc->ops->alloc(rproc, size, );


I believe this will be awkward for the remoteproc drivers to implement.

Imagine a driver that programmatically register some fixed positioned
carveouts and ioremapped vring buffers, it would then need internal book
keeping to figure out which type of allocation each call is related to.


Yes true like any allocator does. And it is needed to manage region overlap.



Rather then deferring the allocation until this point I think we should
tie a rproc_mem_entry to each vring and once we reach
rproc_alloc_vring() we simply use "va" and "dma" from that.

We would get this from rproc_parse_vring() checking to find an existing
mem_entry matching the vring requirements (da, then pa) and falling back
to allocating a new carveout mem_entry.

This doesn't answer to use case described by Suman. What if no specific 
address are requested in firmware resource table, but buffers need to be 
allocated in internal RAM for example. Only rproc driver will know on 
which allocator to rely.


By memremaping a complete memory area and offering va to dma (pa) 
conversion, you don't verify possible overlap between requested regions. 
This is done today by allocator.


The idea from ST pov, was to rely on memory region, to declare subdev 
associated to rproc driver and to rely on dma_alloc_coherent.

I think TI wants to rely on its internal RAM memory allocator.

Regards,
Loic



By then making the current "carveouts" list heterogeneous we would allow
for arbitrary memory types to be used for backing vrings, as well as
trace devices, code and data segments.

Regards,
Bjorn

Re: [PATCH v2 3/3] remoteproc: core: add rproc ops for memory allocation

[loic pallardy]

On 09/15/2016 07:27 PM, Bjorn Andersson wrote:

On Tue 06 Sep 00:39 PDT 2016, Loic Pallardy wrote:


Remoteproc core is currently using dma_alloc_coherent for
carveout and vring allocation.
It doesn't allow to support specific use cases like fixed memory
region or internal RAM support.

Two new rproc ops (alloc and free) is added to provide flexibility
to platform implementation to provide specific memory allocator
taking into account coprocessor characteristics.
rproc_handle_carveout and rproc_alloc_vring functions are modified
to invoke these ops if present, and fallback to regular processing
if platform specific allocation failed and if resquested memory is
not fixed (physical address == FW_RSC_ADDR_ANY)

Signed-off-by: Loic Pallardy 
---
 drivers/remoteproc/remoteproc_core.c | 67 ++--
 include/linux/remoteproc.h   |  4 +++
 2 files changed, 60 insertions(+), 11 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c 
b/drivers/remoteproc/remoteproc_core.c
index 0d3c191..7493b08 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -207,19 +207,29 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
struct rproc_vring *rvring = >vring[i];
struct fw_rsc_vdev *rsc;
dma_addr_t dma;
-   void *va;
+   void *va = NULL;
int ret, size, notifyid;

/* actual size of vring (in bytes) */
size = PAGE_ALIGN(vring_size(rvring->len, rvring->align));

+   rsc = (void *)rproc->table_ptr + rvdev->rsc_offset;
+
/*
 * Allocate non-cacheable memory for the vring. In the future
 * this call will also configure the IOMMU for us
 */
-   va = dma_alloc_coherent(dev->parent, size, , GFP_KERNEL);
+
+   dma = rsc->vring[i].pa;
+
+   if (rproc->ops->alloc)
+   va = rproc->ops->alloc(rproc, size, );


I believe this will be awkward for the remoteproc drivers to implement.

Imagine a driver that programmatically register some fixed positioned
carveouts and ioremapped vring buffers, it would then need internal book
keeping to figure out which type of allocation each call is related to.


Yes true like any allocator does. And it is needed to manage region overlap.



Rather then deferring the allocation until this point I think we should
tie a rproc_mem_entry to each vring and once we reach
rproc_alloc_vring() we simply use "va" and "dma" from that.

We would get this from rproc_parse_vring() checking to find an existing
mem_entry matching the vring requirements (da, then pa) and falling back
to allocating a new carveout mem_entry.

This doesn't answer to use case described by Suman. What if no specific 
address are requested in firmware resource table, but buffers need to be 
allocated in internal RAM for example. Only rproc driver will know on 
which allocator to rely.


By memremaping a complete memory area and offering va to dma (pa) 
conversion, you don't verify possible overlap between requested regions. 
This is done today by allocator.


The idea from ST pov, was to rely on memory region, to declare subdev 
associated to rproc driver and to rely on dma_alloc_coherent.

I think TI wants to rely on its internal RAM memory allocator.

Regards,
Loic



By then making the current "carveouts" list heterogeneous we would allow
for arbitrary memory types to be used for backing vrings, as well as
trace devices, code and data segments.

Regards,
Bjorn

Re: [PATCH v2 3/3] remoteproc: core: add rproc ops for memory allocation

[Bjorn Andersson]

On Tue 06 Sep 00:39 PDT 2016, Loic Pallardy wrote:

> Remoteproc core is currently using dma_alloc_coherent for
> carveout and vring allocation.
> It doesn't allow to support specific use cases like fixed memory
> region or internal RAM support.
> 
> Two new rproc ops (alloc and free) is added to provide flexibility
> to platform implementation to provide specific memory allocator
> taking into account coprocessor characteristics.
> rproc_handle_carveout and rproc_alloc_vring functions are modified
> to invoke these ops if present, and fallback to regular processing
> if platform specific allocation failed and if resquested memory is
> not fixed (physical address == FW_RSC_ADDR_ANY)
> 
> Signed-off-by: Loic Pallardy 
> ---
>  drivers/remoteproc/remoteproc_core.c | 67 
> ++--
>  include/linux/remoteproc.h   |  4 +++
>  2 files changed, 60 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c 
> b/drivers/remoteproc/remoteproc_core.c
> index 0d3c191..7493b08 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -207,19 +207,29 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
>   struct rproc_vring *rvring = >vring[i];
>   struct fw_rsc_vdev *rsc;
>   dma_addr_t dma;
> - void *va;
> + void *va = NULL;
>   int ret, size, notifyid;
>  
>   /* actual size of vring (in bytes) */
>   size = PAGE_ALIGN(vring_size(rvring->len, rvring->align));
>  
> + rsc = (void *)rproc->table_ptr + rvdev->rsc_offset;
> +
>   /*
>* Allocate non-cacheable memory for the vring. In the future
>* this call will also configure the IOMMU for us
>*/
> - va = dma_alloc_coherent(dev->parent, size, , GFP_KERNEL);
> +
> + dma = rsc->vring[i].pa;
> +
> + if (rproc->ops->alloc)
> + va = rproc->ops->alloc(rproc, size, );

I believe this will be awkward for the remoteproc drivers to implement.

Imagine a driver that programmatically register some fixed positioned
carveouts and ioremapped vring buffers, it would then need internal book
keeping to figure out which type of allocation each call is related to.


Rather then deferring the allocation until this point I think we should
tie a rproc_mem_entry to each vring and once we reach
rproc_alloc_vring() we simply use "va" and "dma" from that.

We would get this from rproc_parse_vring() checking to find an existing
mem_entry matching the vring requirements (da, then pa) and falling back
to allocating a new carveout mem_entry.


By then making the current "carveouts" list heterogeneous we would allow
for arbitrary memory types to be used for backing vrings, as well as
trace devices, code and data segments.

Regards,
Bjorn

Re: [PATCH v2 3/3] remoteproc: core: add rproc ops for memory allocation

[Bjorn Andersson]

On Tue 06 Sep 00:39 PDT 2016, Loic Pallardy wrote:

> Remoteproc core is currently using dma_alloc_coherent for
> carveout and vring allocation.
> It doesn't allow to support specific use cases like fixed memory
> region or internal RAM support.
> 
> Two new rproc ops (alloc and free) is added to provide flexibility
> to platform implementation to provide specific memory allocator
> taking into account coprocessor characteristics.
> rproc_handle_carveout and rproc_alloc_vring functions are modified
> to invoke these ops if present, and fallback to regular processing
> if platform specific allocation failed and if resquested memory is
> not fixed (physical address == FW_RSC_ADDR_ANY)
> 
> Signed-off-by: Loic Pallardy 
> ---
>  drivers/remoteproc/remoteproc_core.c | 67 
> ++--
>  include/linux/remoteproc.h   |  4 +++
>  2 files changed, 60 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c 
> b/drivers/remoteproc/remoteproc_core.c
> index 0d3c191..7493b08 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -207,19 +207,29 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
>   struct rproc_vring *rvring = >vring[i];
>   struct fw_rsc_vdev *rsc;
>   dma_addr_t dma;
> - void *va;
> + void *va = NULL;
>   int ret, size, notifyid;
>  
>   /* actual size of vring (in bytes) */
>   size = PAGE_ALIGN(vring_size(rvring->len, rvring->align));
>  
> + rsc = (void *)rproc->table_ptr + rvdev->rsc_offset;
> +
>   /*
>* Allocate non-cacheable memory for the vring. In the future
>* this call will also configure the IOMMU for us
>*/
> - va = dma_alloc_coherent(dev->parent, size, , GFP_KERNEL);
> +
> + dma = rsc->vring[i].pa;
> +
> + if (rproc->ops->alloc)
> + va = rproc->ops->alloc(rproc, size, );

I believe this will be awkward for the remoteproc drivers to implement.

Imagine a driver that programmatically register some fixed positioned
carveouts and ioremapped vring buffers, it would then need internal book
keeping to figure out which type of allocation each call is related to.


Rather then deferring the allocation until this point I think we should
tie a rproc_mem_entry to each vring and once we reach
rproc_alloc_vring() we simply use "va" and "dma" from that.

We would get this from rproc_parse_vring() checking to find an existing
mem_entry matching the vring requirements (da, then pa) and falling back
to allocating a new carveout mem_entry.


By then making the current "carveouts" list heterogeneous we would allow
for arbitrary memory types to be used for backing vrings, as well as
trace devices, code and data segments.

Regards,
Bjorn

Re: [PATCH v2 3/3] remoteproc: core: add rproc ops for memory allocation

[Lee Jones]

On Tue, 06 Sep 2016, Loic Pallardy wrote:

> Remoteproc core is currently using dma_alloc_coherent for
> carveout and vring allocation.
> It doesn't allow to support specific use cases like fixed memory
> region or internal RAM support.
> 
> Two new rproc ops (alloc and free) is added to provide flexibility
> to platform implementation to provide specific memory allocator
> taking into account coprocessor characteristics.
> rproc_handle_carveout and rproc_alloc_vring functions are modified
> to invoke these ops if present, and fallback to regular processing
> if platform specific allocation failed and if resquested memory is
> not fixed (physical address == FW_RSC_ADDR_ANY)
> 
> Signed-off-by: Loic Pallardy 
> ---
>  drivers/remoteproc/remoteproc_core.c | 67 
> ++--
>  include/linux/remoteproc.h   |  4 +++
>  2 files changed, 60 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c 
> b/drivers/remoteproc/remoteproc_core.c
> index 0d3c191..7493b08 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -207,19 +207,29 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
>   struct rproc_vring *rvring = >vring[i];
>   struct fw_rsc_vdev *rsc;
>   dma_addr_t dma;
> - void *va;
> + void *va = NULL;
>   int ret, size, notifyid;
>  
>   /* actual size of vring (in bytes) */
>   size = PAGE_ALIGN(vring_size(rvring->len, rvring->align));
>  
> + rsc = (void *)rproc->table_ptr + rvdev->rsc_offset;
> +
>   /*
>* Allocate non-cacheable memory for the vring. In the future
>* this call will also configure the IOMMU for us
>*/
> - va = dma_alloc_coherent(dev->parent, size, , GFP_KERNEL);
> +
> + dma = rsc->vring[i].pa;
> +
> + if (rproc->ops->alloc)
> + va = rproc->ops->alloc(rproc, size, );
> +
> + if (!va && rsc->vring[i].pa == FW_RSC_ADDR_ANY)
> + va = dma_alloc_coherent(dev->parent, size, , GFP_KERNEL);
> +
>   if (!va) {
> - dev_err(dev->parent, "dma_alloc_coherent failed\n");
> + dev_err(dev->parent, "Failed to get valid ving[%d] va\n", i);

Error messages isn't the place for abbreviations IMO.

"Failed to allocate memory for ... XXX"

>   return -EINVAL;
>   }
>  
> @@ -231,7 +241,10 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
>   ret = idr_alloc(>notifyids, rvring, 0, 0, GFP_KERNEL);
>   if (ret < 0) {
>   dev_err(dev, "idr_alloc failed: %d\n", ret);
> - dma_free_coherent(dev->parent, size, va, dma);
> + if (rproc->ops->free)
> + ret = rproc->ops->free(rproc, size, va, dma);
> + if (!ret)
> + dma_free_coherent(dev->parent, size, va, dma);

Are you sure this is what you want to do?

Won't this free the memory twice?

Looking at this *very* briefly, shouldn't this be something like:

else if (va)
 dma_free_coherent(dev->parent, size, va, dma);

>   return ret;
>   }
>   notifyid = ret;
> @@ -249,8 +262,8 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
>* set up the iommu. In this case the device address (da) will
>* hold the physical address and not the device address.
>*/
> - rsc = (void *)rproc->table_ptr + rvdev->rsc_offset;
>   rsc->vring[i].da = dma;
> + rsc->vring[i].pa = dma;
>   rsc->vring[i].notifyid = notifyid;
>   return 0;
>  }
> @@ -273,6 +286,15 @@ rproc_parse_vring(struct rproc_vdev *rvdev, struct 
> fw_rsc_vdev *rsc, int i)
>   return -EINVAL;
>   }
>  
> + /*
> +  * pa field was previously reserved and fixed to 0
> +  * used FW_RSC_ADDR_ANY as default value if 0 detected
> +  * to keep backward compatibility and have vring allocated
> +  * by dma_alloc_coherent
> +  */
> + if (vring->pa == 0)
> + vring->pa = FW_RSC_ADDR_ANY;
> +
>   rvring->len = vring->num;
>   rvring->align = vring->align;
>   rvring->rvdev = rvdev;
> @@ -286,8 +308,15 @@ void rproc_free_vring(struct rproc_vring *rvring)
>   struct rproc *rproc = rvring->rvdev->rproc;
>   int idx = rvring->rvdev->vring - rvring;
>   struct fw_rsc_vdev *rsc;
> + int ret = 0;
> +
> + if (rproc->ops->free)
> + ret = rproc->ops->free(rproc, size, rvring->va, rvring->dma);
> +
> + if (!ret)
> + dma_free_coherent(rproc->dev.parent, size, rvring->va,
> +   rvring->dma);

Same here.

> - dma_free_coherent(rproc->dev.parent, size, rvring->va, rvring->dma);
>   idr_remove(>notifyids, rvring->notifyid);
>  
>   /* reset resource entry info */
> @@ -558,7 +587,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
>   struct rproc_mem_entry *carveout, *mapping;
>   struct device *dev = >dev;
>   dma_addr_t dma;
> - 

Re: [PATCH v2 3/3] remoteproc: core: add rproc ops for memory allocation

[Lee Jones]

On Tue, 06 Sep 2016, Loic Pallardy wrote:

> Remoteproc core is currently using dma_alloc_coherent for
> carveout and vring allocation.
> It doesn't allow to support specific use cases like fixed memory
> region or internal RAM support.
> 
> Two new rproc ops (alloc and free) is added to provide flexibility
> to platform implementation to provide specific memory allocator
> taking into account coprocessor characteristics.
> rproc_handle_carveout and rproc_alloc_vring functions are modified
> to invoke these ops if present, and fallback to regular processing
> if platform specific allocation failed and if resquested memory is
> not fixed (physical address == FW_RSC_ADDR_ANY)
> 
> Signed-off-by: Loic Pallardy 
> ---
>  drivers/remoteproc/remoteproc_core.c | 67 
> ++--
>  include/linux/remoteproc.h   |  4 +++
>  2 files changed, 60 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c 
> b/drivers/remoteproc/remoteproc_core.c
> index 0d3c191..7493b08 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -207,19 +207,29 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
>   struct rproc_vring *rvring = >vring[i];
>   struct fw_rsc_vdev *rsc;
>   dma_addr_t dma;
> - void *va;
> + void *va = NULL;
>   int ret, size, notifyid;
>  
>   /* actual size of vring (in bytes) */
>   size = PAGE_ALIGN(vring_size(rvring->len, rvring->align));
>  
> + rsc = (void *)rproc->table_ptr + rvdev->rsc_offset;
> +
>   /*
>* Allocate non-cacheable memory for the vring. In the future
>* this call will also configure the IOMMU for us
>*/
> - va = dma_alloc_coherent(dev->parent, size, , GFP_KERNEL);
> +
> + dma = rsc->vring[i].pa;
> +
> + if (rproc->ops->alloc)
> + va = rproc->ops->alloc(rproc, size, );
> +
> + if (!va && rsc->vring[i].pa == FW_RSC_ADDR_ANY)
> + va = dma_alloc_coherent(dev->parent, size, , GFP_KERNEL);
> +
>   if (!va) {
> - dev_err(dev->parent, "dma_alloc_coherent failed\n");
> + dev_err(dev->parent, "Failed to get valid ving[%d] va\n", i);

Error messages isn't the place for abbreviations IMO.

"Failed to allocate memory for ... XXX"

>   return -EINVAL;
>   }
>  
> @@ -231,7 +241,10 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
>   ret = idr_alloc(>notifyids, rvring, 0, 0, GFP_KERNEL);
>   if (ret < 0) {
>   dev_err(dev, "idr_alloc failed: %d\n", ret);
> - dma_free_coherent(dev->parent, size, va, dma);
> + if (rproc->ops->free)
> + ret = rproc->ops->free(rproc, size, va, dma);
> + if (!ret)
> + dma_free_coherent(dev->parent, size, va, dma);

Are you sure this is what you want to do?

Won't this free the memory twice?

Looking at this *very* briefly, shouldn't this be something like:

else if (va)
 dma_free_coherent(dev->parent, size, va, dma);

>   return ret;
>   }
>   notifyid = ret;
> @@ -249,8 +262,8 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
>* set up the iommu. In this case the device address (da) will
>* hold the physical address and not the device address.
>*/
> - rsc = (void *)rproc->table_ptr + rvdev->rsc_offset;
>   rsc->vring[i].da = dma;
> + rsc->vring[i].pa = dma;
>   rsc->vring[i].notifyid = notifyid;
>   return 0;
>  }
> @@ -273,6 +286,15 @@ rproc_parse_vring(struct rproc_vdev *rvdev, struct 
> fw_rsc_vdev *rsc, int i)
>   return -EINVAL;
>   }
>  
> + /*
> +  * pa field was previously reserved and fixed to 0
> +  * used FW_RSC_ADDR_ANY as default value if 0 detected
> +  * to keep backward compatibility and have vring allocated
> +  * by dma_alloc_coherent
> +  */
> + if (vring->pa == 0)
> + vring->pa = FW_RSC_ADDR_ANY;
> +
>   rvring->len = vring->num;
>   rvring->align = vring->align;
>   rvring->rvdev = rvdev;
> @@ -286,8 +308,15 @@ void rproc_free_vring(struct rproc_vring *rvring)
>   struct rproc *rproc = rvring->rvdev->rproc;
>   int idx = rvring->rvdev->vring - rvring;
>   struct fw_rsc_vdev *rsc;
> + int ret = 0;
> +
> + if (rproc->ops->free)
> + ret = rproc->ops->free(rproc, size, rvring->va, rvring->dma);
> +
> + if (!ret)
> + dma_free_coherent(rproc->dev.parent, size, rvring->va,
> +   rvring->dma);

Same here.

> - dma_free_coherent(rproc->dev.parent, size, rvring->va, rvring->dma);
>   idr_remove(>notifyids, rvring->notifyid);
>  
>   /* reset resource entry info */
> @@ -558,7 +587,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
>   struct rproc_mem_entry *carveout, *mapping;
>   struct device *dev = >dev;
>   dma_addr_t dma;
> - void *va;
> +