Re: [PATCH 1/2] remoteproc: fall back to using parent memory pool if no dedicated available

[Suman Anna]

Hi Arnaud,

On 3/18/20 4:37 AM, Tero Kristo wrote:
> On 13/03/2020 18:52, Arnaud POULIQUEN wrote:
>> Hi Suman,
>>
>>> -----Original Message-----
>>> From: Suman Anna <s-anna@xxxxxx>
>>> Sent: jeudi 5 mars 2020 23:41
>>> To: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>; Loic PALLARDY
>>> <loic.pallardy@xxxxxx>
>>> Cc: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>; Arnaud POULIQUEN
>>> <arnaud.pouliquen@xxxxxx>; Tero Kristo <t-kristo@xxxxxx>; linux-
>>> remoteproc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Suman Anna
>>> <s-anna@xxxxxx>
>>> Subject: [PATCH 1/2] remoteproc: fall back to using parent memory
>>> pool if no
>>> dedicated available
>>>
>>> From: Tero Kristo <t-kristo@xxxxxx>
>>>
>>> In some cases, like with OMAP remoteproc, we are not creating dedicated
>>> memory pool for the virtio device. Instead, we use the same memory pool
>>> for all shared memories. The current virtio memory pool handling
>>> forces a
>>> split between these two, as a separate device is created for it, causing
>>> memory to be allocated from bad location if the dedicated pool is not
>>> available. Fix this by falling back to using the parent device memory
>>> pool if
>>> dedicated is not available.
>>>
>>> Fixes: 086d08725d34 ("remoteproc: create vdev subdevice with specific
>>> dma
>>> memory pool")
>>> Signed-off-by: Tero Kristo <t-kristo@xxxxxx>
>>> Signed-off-by: Suman Anna <s-anna@xxxxxx>
>>> ---
>>> Â drivers/remoteproc/remoteproc_virtio.c | 10 ++++++++++
>>> Â 1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/remoteproc/remoteproc_virtio.c
>>> b/drivers/remoteproc/remoteproc_virtio.c
>>> index 8c07cb2ca8ba..4723ebe574b8 100644
>>> --- a/drivers/remoteproc/remoteproc_virtio.c
>>> +++ b/drivers/remoteproc/remoteproc_virtio.c
>>> @@ -368,6 +368,16 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev,
>>> int id)
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ goto out;
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ }
>>> ÂÂÂÂÂÂÂÂÂ }
>>> +ÂÂÂ } else {
>>> +ÂÂÂÂÂÂÂ struct device_node *np = rproc->dev.parent->of_node;
>>> +
>>> +ÂÂÂÂÂÂÂ /*
>>> +ÂÂÂÂÂÂÂÂ * If we don't have dedicated buffer, just attempt to
>>> +ÂÂÂÂÂÂÂÂ * re-assign the reserved memory from our parent.
>>> +ÂÂÂÂÂÂÂÂ * Failure is non-critical so don't check return value
>>> +ÂÂÂÂÂÂÂÂ * either.
>>> +ÂÂÂÂÂÂÂÂ */
>>> +ÂÂÂÂÂÂÂ of_reserved_mem_device_init_by_idx(dev, np, 0);
>>> ÂÂÂÂÂ }
>> I aven't tested your patchset yet, but reviewing you code, I wonder
>> if you cannot declare your memory pool
>> in your platform driver using rproc_of_resm_mem_entry_init. 

The patch actually provides a fallback option and even now this path is
entered only when there are no dedicated pools. This restores the code
to how the allocations were made prior to the fixed memory carveout
changes. If the remoteproc drivers themselves do not use any DMA/CMA
pools, then nothing changes and allocations continue to be made from the
global pools.

Something
>> like:
>> ÂÂÂÂstruct device_node *mem_node;
>> ÂÂÂÂstruct reserved_mem *rmem;
>>
>> ÂÂÂÂmem_node = of_parse_phandle(dev->of_node, "memory-region", 0);
>> ÂÂÂÂrmem = of_reserved_mem_lookup(mem_node);
>> ÂÂÂÂmem = rproc_of_resm_mem_entry_init(dev, 0,
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ rmem->size,
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ rmem->base,
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ " vdev0buffer");
>>
>> A main advantage of this implementation would be that the index of the
>> memory region would not be hard coded to 0.

The 0 is the default (equivalent to of_reserved_mem_device_init(), but
we can't use that function here since dev and np are different).

While your suggestion does work for us, this does bring in the knowledge
of how many vdevs a remoteproc driver is supporting. It is fine for
remoteproc drivers that are designed exactly for a known number of vdevs
and/or fixed pools to use the above function, but every other remoteproc
driver would have to repeat similar code. Given that the number of vdevs
are currently defined through the resource table and can change from
firmware to firmware, I think this fallback option patch is the better
scalable solution.

Let's see if others have any opinion on this.

regards
Suman

> 
> It seems like that would work for us also, and thus this patch can be
> dropped. See the following patch. Suman, any comments on this? If this
> seems acceptable, I can send this as a proper patch to the list.
> 
> ------
> 
> From: Tero Kristo <t-kristo@xxxxxx>
> Date: Wed, 18 Mar 2020 11:22:58 +0200
> Subject: [PATCH] remoteproc/omap: Allocate vdev0buffer memory from
> Âreserved memory pool
> 
> Since 086d08725d34 ("remoteproc: create vdev subdevice with specific dma
> memory pool"), remoteprocs must allocate separate vdev memory buffer. As
> OMAP remoteproc does not do this yet, the memory gets allocated from
> default DMA pool, and this memory is not suitable for the use. To fix
> the issue, map the vdev0buffer to use the same device reserved memory
> pool as the rest of the remoteproc.
> 
> Signed-off-by: Tero Kristo <t-kristo@xxxxxx>
> ---
> Âdrivers/remoteproc/omap_remoteproc.c | 16 ++++++++++++++++
> Â1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/remoteproc/omap_remoteproc.c
> b/drivers/remoteproc/omap_remoteproc.c
> index 29d19a608af8..024330e31a9e 100644
> --- a/drivers/remoteproc/omap_remoteproc.c
> +++ b/drivers/remoteproc/omap_remoteproc.c
> @@ -1273,6 +1273,9 @@ static int omap_rproc_probe(struct platform_device
> *pdev)
> ÂÂÂÂ const char *firmware;
> ÂÂÂÂ int ret;
> ÂÂÂÂ struct reset_control *reset;
> +ÂÂÂ struct device_node *mem_node;
> +ÂÂÂ struct reserved_mem *rmem;
> +ÂÂÂ struct rproc_mem_entry *mem;
> 
> ÂÂÂÂ if (!np) {
> ÂÂÂÂÂÂÂÂ dev_err(&pdev->dev, "only DT-based devices are supported\n");
> @@ -1335,6 +1338,19 @@ static int omap_rproc_probe(struct
> platform_device *pdev)
> ÂÂÂÂÂÂÂÂ dev_warn(&pdev->dev, "device does not have specific CMA pool.\n");
> ÂÂÂÂÂÂÂÂ dev_warn(&pdev->dev, "Typically this should be provided,\n");
> ÂÂÂÂÂÂÂÂ dev_warn(&pdev->dev, "only omit if you know what you are
> doing.\n");
> +ÂÂÂ } else {
> +ÂÂÂÂÂÂÂ mem_node = of_parse_phandle(pdev->dev.of_node, "memory-region",
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0);
> +ÂÂÂÂÂÂÂ rmem = of_reserved_mem_lookup(mem_node);
> +ÂÂÂÂÂÂÂ mem = rproc_of_resm_mem_entry_init(&pdev->dev, 0, rmem->size,
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ rmem->base, "vdev0buffer");
> +
> +ÂÂÂÂÂÂÂ if (!mem) {
> +ÂÂÂÂÂÂÂÂÂÂÂ ret = -ENOMEM;
> +ÂÂÂÂÂÂÂÂÂÂÂ goto release_mem;
> +ÂÂÂÂÂÂÂ }
> +
> +ÂÂÂÂÂÂÂ rproc_add_carveout(rproc, mem);
> ÂÂÂÂ }
> 
> ÂÂÂÂ platform_set_drvdata(pdev, rproc);