Re: [PATCH v2 18/19] remoteproc: core: Add function to create remoteproc local resource table

[loic pallardy]

On 09/15/2016 08:58 PM, Bjorn Andersson wrote:
> On Wed 31 Aug 13:50 PDT 2016, Loic Pallardy wrote:
>
> > Rproc driver has now the capability to add resources dynamically
> > thanks to rproc_request_resource API.
> > Depending on associated action, resource request could impact
> > firmware resource table or define new local resource.
> >
> > In order to preserve current remoteproc resource handling
> > mechanism, all local resources are gathered in a local resource
> > table which won't be shared with firmware and proceed by
> > remoteproc core as firmware one.
> >
> > It is rproc driver responsibility to provide the right resource
> > information using rproc_request_resource API.
> >
> > Signed-off-by: Loic Pallardy <loic.pallardy@xxxxxx>
> > ---
> >  drivers/remoteproc/remoteproc_core.c | 80 +++++++++++++++++++++++++++++++++++-
> >  include/linux/remoteproc.h           |  1 +
> >  2 files changed, 80 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index cbfbdf8..73b460a 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -1270,6 +1270,65 @@ static int rproc_apply_resource_overrides(struct rproc *rproc,
> >  	return ret;
> >  }
> >
> > +static struct resource_table*
> > +rproc_local_resource_create(struct rproc *rproc, int *tablesz)
>
> The resource table is a persistent data structure and as such it's
> awkward and inconvenient to operate on. This does, unfortunately, show
> throughout this series.
>
> But to here traverse a list of resources, generate a new table of the
> left-over ones just so that we can allocate them and put them back on
> another list is a clear sign to me that we're doing it wrong way.

Ok goal was to keep current resource allocation mechanism without 
changing current rproc_handle_xxx functions.
The idea was to avoid fake resource table creation at rproc driver level...
> Further more, we will have to support 64 bit addresses in the firmware
> so I do not want to lock in the core implementation in version 1 of the
> resource table format.
>
>
>
> Rather than using the resource tables directly I think we should parse
> the resource table into lists of objects that are convenient to operate
> on. While doing this we can squash "duplicates" and validate that they
> are compatible.

Yes possible to keep only internal list of request.
> This allows the driver to (optionally) register static resources
> and the resource table parser to (optionally!) register firmware
> resources. We would then traverse these lists and if resources are
> referenced update the resource table entries - i.e. like vrings are
> handled already.
>
>
> We can do this two ways:
> 1) As we register entries in these lists we check for previous
> allocations and fall back to allocate the resources directly. Allowing
> the drivers to fill in the lists and override resources from the table
> parser. A drawback with this approach is that driver-registered
> resources must stay allocated throughout the driver's life cycle.
>
> 2) We defer allocations to after we've built up the resource lists,
> allowing the static/driver resources to be allocated as we boot the
> rproc. Grouping the allocations would, likely, make it easier to reason
> about error paths etc.
As we are discussing specific rproc allocation in another thread and 
that local resource management is linked to, I propose to drop local 
resource concept from this series and come back later with a solution 
alignment with dedicated rproc memory chunk management.

Regards,
Loic

> Regards,
> Bjorn