RE: [PATCH v3 09/13] remoteproc: modify rproc_handle_carveout to support pre-registered region

[Loic PALLARDY]

> -----Original Message-----
> From: Bjorn Andersson [mailto:bjorn.andersson@xxxxxxxxxx]
> Sent: Thursday, May 10, 2018 2:43 AM
> To: Loic PALLARDY <loic.pallardy@xxxxxx>
> Cc: ohad@xxxxxxxxxx; linux-remoteproc@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Arnaud POULIQUEN <arnaud.pouliquen@xxxxxx>;
> benjamin.gaignard@xxxxxxxxxx
> Subject: Re: [PATCH v3 09/13] remoteproc: modify rproc_handle_carveout to
> support pre-registered region
> 
> On Thu 01 Mar 08:23 PST 2018, Loic Pallardy wrote:
> 
> > In current version rproc_handle_carveout function support only dynamic
> > region allocation.
> > This patch extends rproc_handle_carveout function to support pre-
> registered
> > region. Match is done on region name, then requested device address and
> > length are checked.
> > If no name match found, original allocation is used.
> >
> > Signed-off-by: Loic Pallardy <loic.pallardy@xxxxxx>
> > ---
> >  drivers/remoteproc/remoteproc_core.c | 49
> +++++++++++++++++++++++++++++++++++-
> >  1 file changed, 48 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> > index 0ebbc4f..49b28a0 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -679,7 +679,7 @@ static int rproc_handle_carveout(struct rproc
> *rproc,
> >  				 struct fw_rsc_carveout *rsc,
> >  				 int offset, int avail)
> >  {
> > -	struct rproc_mem_entry *carveout, *mapping = NULL;
> > +	struct rproc_mem_entry *carveout, *mapping = NULL, *mem;
> >  	struct device *dev = &rproc->dev;
> >  	dma_addr_t dma;
> >  	void *va;
> > @@ -699,6 +699,51 @@ static int rproc_handle_carveout(struct rproc
> *rproc,
> >  	dev_dbg(dev, "carveout rsc: name: %s, da 0x%x, pa 0x%x, len 0x%x,
> flags 0x%x\n",
> >  		rsc->name, rsc->da, rsc->pa, rsc->len, rsc->flags);
> >
> > +	/* Check carveout rsc already part of a registered carveout */
> > +	/* Search by name */
> > +	mem = rproc_find_carveout_by_name(rproc, rsc->name);
> > +	if (mem) {
> 
> I don't fancy the concept of "check if there is another registered
> carveout and if so update this carveouts data based on that one and then
> skip the bottom half of this function but keep them both on the
> carveouts list".
> 
> It's unfortunately not very easy to follow and it doesn't allow us to
> reuse the carveout-handler for allocations in remoteprocs without a
> resource table.
> 
> How about splitting the handling of the resource table in two parts; one
> that creates or updates a carveout on the carvouts list and a second
> part that runs through all carveouts and "allocate" (similar to your
> specific release function) them.
> 
> 
> The first part of this function would then attempt to find a carveout
> entry matching the one we're trying to "handle";
> 
> * if one is found we check if it's compatible (as you do here), update a
>   rsc_offset (as we do with vrings) and return.
> 
> * if no match is found we create a new rproc_mem_entry, fill it out
>   based on the fw_rsc_carveout information and stash it at the end of
>   the carveouts list.
> 
> We do the same in the other resource handlers (just allocate entries
> onto the lists).
> 
> 
> As that is done the second step is to loop over all carveouts, devmem,
> trace and vdev resources and actually "allocate" the resources, by
> calling a "alloc" function pointer next to your proposed release one.
> 
> For memremap() memory this could be as simple as filling out the
> resource table at the associated rsc_offset or simply doing the
> memremap().
> 
> The default alloc (filled out in step 1, if not already specified) would
> be what's today found in rproc_handle_carveout().
> 
> 
> This allows carveout resources not specified in the resource table to be
> allocated as well. Which comes in handy for the handling of vdev
> resources:
> 
> In rproc_parse_vdev() we do a similar operation to the parser of a
> fw_rsc_carveout and try to find an existing carveout by name and if not
> create a new one on the list.
> 
> As the actual allocation of carveouts is done before the "allocation" of
> vrings there will be an allocated carveout ready when we hit
> rproc_alloc_vring() - and we don't care if it came from
> dma_alloc_coherent() or a driver defined region.
> 
> 
> Does this sound reasonable?
Yes, better to separate resource table parsing and memory carveout allocation.
I'll update series in that way

Regards,
Loic


> 
> Regards,
> Bjorn