From: Loic PALLARDY <loic.pallardy@st.com>
To: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: "ohad@wizery.com" <ohad@wizery.com>,
"linux-remoteproc@vger.kernel.org"
<linux-remoteproc@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Arnaud POULIQUEN <arnaud.pouliquen@st.com>,
"benjamin.gaignard@linaro.org" <benjamin.gaignard@linaro.org>
Subject: RE: [PATCH v3 09/13] remoteproc: modify rproc_handle_carveout to support pre-registered region
Date: Mon, 14 May 2018 14:52:32 +0000 [thread overview]
Message-ID: <35c35efde19e464285402786cba732ff@SFHDAG7NODE2.st.com> (raw)
In-Reply-To: <20180510004244.GE29093@builder>
> -----Original Message-----
> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]
> Sent: Thursday, May 10, 2018 2:43 AM
> To: Loic PALLARDY <loic.pallardy@st.com>
> Cc: ohad@wizery.com; linux-remoteproc@vger.kernel.org; linux-
> kernel@vger.kernel.org; Arnaud POULIQUEN <arnaud.pouliquen@st.com>;
> benjamin.gaignard@linaro.org
> 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@st.com>
> > ---
> > 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
next prev parent reply other threads:[~2018-05-14 14:52 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-01 16:23 [PATCH v3 00/13] remoteproc: add fixed memory region support Loic Pallardy
2018-03-01 16:23 ` [PATCH v3 01/13] remoteproc: configure IOMMU only if device address requested Loic Pallardy
2018-03-01 16:23 ` [PATCH v3 02/13] remoteproc: add rproc_va_to_pa function Loic Pallardy
2018-05-10 0:11 ` Bjorn Andersson
2018-03-01 16:23 ` [PATCH v3 03/13] remoteproc: add release ops in rproc_mem_entry struct Loic Pallardy
2018-05-10 0:53 ` Bjorn Andersson
2018-03-01 16:23 ` [PATCH v3 04/13] remoteproc: add name " Loic Pallardy
2018-05-10 0:12 ` Bjorn Andersson
2018-03-01 16:23 ` [PATCH v3 05/13] remoteproc: add helper function to allocate and init " Loic Pallardy
2018-03-01 16:23 ` [PATCH v3 06/13] remoteproc: introduce rproc_add_carveout function Loic Pallardy
2018-05-10 0:56 ` Bjorn Andersson
2018-03-01 16:23 ` [PATCH v3 07/13] remoteproc: introduce rproc_find_carveout_by_name function Loic Pallardy
2018-05-10 0:19 ` Bjorn Andersson
2018-05-14 14:40 ` Loic PALLARDY
2018-03-01 16:23 ` [PATCH v3 08/13] remoteproc: add prepare and unprepare ops Loic Pallardy
2018-05-10 0:52 ` Bjorn Andersson
2018-05-14 15:03 ` Loic PALLARDY
2018-10-24 3:12 ` Suman Anna
2018-03-01 16:23 ` [PATCH v3 09/13] remoteproc: modify rproc_handle_carveout to support pre-registered region Loic Pallardy
2018-05-10 0:42 ` Bjorn Andersson
2018-05-14 14:52 ` Loic PALLARDY [this message]
2018-03-01 16:23 ` [PATCH v3 10/13] remoteproc: modify vring allocation " Loic Pallardy
2018-05-10 0:59 ` Bjorn Andersson
2018-05-14 15:43 ` Loic PALLARDY
2018-03-01 16:23 ` [PATCH v3 11/13] remoteproc: create vdev subdevice with specific dma memory pool Loic Pallardy
2018-05-10 1:06 ` Bjorn Andersson
2018-05-14 15:57 ` Loic PALLARDY
2018-03-01 16:23 ` [PATCH v3 12/13] rpmsg: virtio: allocate buffer from parent Loic Pallardy
2018-03-01 16:24 ` [PATCH v3 13/13] remoteproc: st: add reserved memory support Loic Pallardy
2018-04-03 12:04 ` [PATCH v3 00/13] remoteproc: add fixed memory region support Loic PALLARDY
2018-06-25 3:23 ` Anup Patel
2018-06-26 8:17 ` Loic PALLARDY
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=35c35efde19e464285402786cba732ff@SFHDAG7NODE2.st.com \
--to=loic.pallardy@st.com \
--cc=arnaud.pouliquen@st.com \
--cc=benjamin.gaignard@linaro.org \
--cc=bjorn.andersson@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-remoteproc@vger.kernel.org \
--cc=ohad@wizery.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).