Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: UUnfZaWDKeVNgKdugKnUXn9tks9fEa+G2r/iaggfdsnrFHBULZyE9kea5fQPMI8RXjfC1dZXx5
 8Yq3vqcnj14g==
IronPort-SDR: I2D2p0AMo5G1kk6j6apY0XIGrIu5i1o3GbzifrQO+oGCv5UBkrW2HSYZmgXWtU2tAULrgZcRb9
 8coOKUuC3Yqw==
From:   "Gross, Mark" <mark.gross@intel.com>
To:     Greg KH <gregkh@linuxfoundation.org>,
        Daniele Alessandrelli <daniele.alessandrelli@linux.intel.com>
CC:     "Alessandrelli, Daniele" <daniele.alessandrelli@intel.com>,
        "mgross@linux.intel.com" <mgross@linux.intel.com>,
        "markgross@kernel.org" <markgross@kernel.org>,
        "arnd@arndb.de" <arnd@arndb.de>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "sumit.semwal@linaro.org" <sumit.semwal@linaro.org>,
        "christian.koenig@amd.com" <christian.koenig@amd.com>
Subject: RE: [PATCH 03/22] keembay-ipc: Add Keem Bay IPC module
Thread-Topic: [PATCH 03/22] keembay-ipc: Add Keem Bay IPC module
Thread-Index: AQHWyDJR6ibfO+n2Z0iYweamtROKuKnjVd+AgApCJ4CAAA2ngIADETYAgADmXICAAJXDQA==
Date:   Fri, 11 Dec 2020 17:26:09 +0000
Message-ID: <MWHPR11MB1679089EDC581294CE9EB0E38ECA0@MWHPR11MB1679.namprd11.prod.outlook.com>
References: <20201201223511.65542-1-mgross@linux.intel.com>
 <20201201223511.65542-4-mgross@linux.intel.com> <X8cyA1qKCwrayEOp@kroah.com>
 <bcf8bad08a5f586093a151126aba2127eee44c02.camel@linux.intel.com>
 <X8/YceIsM/Akt/E/@kroah.com>
 <a5a13b6b399745f1f8b369334b0e626ee16d532a.camel@linux.intel.com>
 <X9MsXRDcdoftq8ZC@kroah.com>
In-Reply-To: <X9MsXRDcdoftq8ZC@kroah.com>
Accept-Language: en-US
Content-Language: en-US
dlp-version: 11.5.1.3
dlp-product: dlpe-windows
dlp-reaction: no-action
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 085736f1-2268-40ad-30d6-08d89df9d993
X-MS-Exchange-CrossTenant-originalarrivaltime: 11 Dec 2020 17:26:09.4105
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: XobFMIi2tnJCJH0rXL3VlwILzGK91lce9MNjc6Yj+PfURulfal6OBGy39BPts1ScVJhMajIxbW2q1neBNx7rjQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR11MB1742
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Friday, December 11, 2020 12:23 AM
> To: Daniele Alessandrelli <daniele.alessandrelli@linux.intel.com>
> Cc: Alessandrelli, Daniele <daniele.alessandrelli@intel.com>;
> mgross@linux.intel.com; markgross@kernel.org; arnd@arndb.de;
> robh+dt@kernel.org; linux-kernel@vger.kernel.org; sumit.semwal@linaro.org=
;
> christian.koenig@amd.com
> Subject: Re: [PATCH 03/22] keembay-ipc: Add Keem Bay IPC module
>=20
> On Thu, Dec 10, 2020 at 06:38:24PM +0000, Daniele Alessandrelli wrote:
> > On Tue, 2020-12-08 at 20:48 +0100, Greg KH wrote:
> > > On Tue, Dec 08, 2020 at 06:59:09PM +0000, Daniele Alessandrelli wrote=
:
> > > > Hi Greg,
> > > >
> > > > Thanks for your feedback.
> > > >
> > > > On Wed, 2020-12-02 at 07:19 +0100, Greg KH wrote:
> > > > > On Tue, Dec 01, 2020 at 02:34:52PM -0800, mgross@linux.intel.com
> wrote:
> > > > > > From: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
> > > > > >
> > > > > > On the Intel Movidius SoC code named Keem Bay, communication
> > > > > > between the Computing Sub-System (CSS), i.e., the CPU, and the
> > > > > > Multimedia Sub-System (MSS), i.e., the VPU is enabled by the
> > > > > > Keem Bay Inter-Processor Communication (IPC) mechanism.
> > > > > >
> > > > > > Add the driver for using Keem Bay IPC from within the Linux Ker=
nel.
> > > > > >
> > > > > > Keem Bay IPC uses the following terminology:
> > > > > >
> > > > > > - Node:    A processing entity that can use the IPC to communic=
ate;
> > > > > > 	   currently, we just have two nodes, CPU (CSS) and VPU (MSS).
> > > > > >
> > > > > > - Link:    Two nodes that can communicate over IPC form an IPC =
link
> > > > > > 	   (currently, we just have one link, the one between the CPU
> > > > > > 	   and VPU).
> > > > > >
> > > > > > - Channel: An IPC link can provide multiple IPC channels. IPC c=
hannels
> > > > > > 	   allow communication multiplexing, i.e., the same IPC link c=
an
> > > > > > 	   be used by different applications for different
> > > > > > 	   communications. Each channel is identified by a channel ID,
> > > > > > 	   which must be unique within a single IPC link. Channels are
> > > > > > 	   divided in two categories, High-Speed (HS) channels and
> > > > > > 	   General-Purpose (GP) channels. HS channels have higher
> > > > > > 	   priority over GP channels.
> > > > > >
> > > > > > Keem Bay IPC mechanism is based on shared memory and hardware
> FIFOs.
> > > > > > Both the CPU and the VPU have their own hardware FIFO. When
> > > > > > the CPU wants to send an IPC message to the VPU, it writes to
> > > > > > the VPU FIFO (MSS FIFO); similarly, when MSS wants to send an
> > > > > > IPC message to the CPU, it writes to the CPU FIFO (CSS FIFO).
> > > > > >
> > > > > > A FIFO entry is simply a pointer to an IPC buffer (aka IPC
> > > > > > header) stored in a portion of memory shared between the CPU an=
d
> the VPU.
> > > > > > Specifically, the FIFO entry contains the (VPU) physical
> > > > > > address of the IPC buffer being transferred.
> > > > > >
> > > > > > In turn, the IPC buffer contains the (VPU) physical address of
> > > > > > the payload (which must be located in shared memory too) as
> > > > > > well as other information (payload size, IPC channel ID, etc.).
> > > > > >
> > > > > > Each IPC node instantiates a pool of IPC buffers from its own
> > > > > > IPC buffer memory region. When instantiated, IPC buffers are
> > > > > > marked as free. When the node needs to send an IPC message, it
> > > > > > gets the first free buffer it finds (from its own pool), marks
> > > > > > it as allocated (used), and puts its physical address into the
> > > > > > IPC FIFO of the destination node. The destination node (which
> > > > > > is notified by an interrupt when there are entries pending in
> > > > > > its FIFO) extract the physical address from the FIFO and
> > > > > > process the IPC buffer, marking it as free once done (so that t=
he sender
> can reuse the buffer).
> > > > >
> > > > > Any reason you can't use the dmabuf interface for these memory
> > > > > buffers you are creating and having to manage "by hand"?  I
> > > > > thought that was what the kernel was wanting to unify on such
> > > > > that individual drivers/subsystems didn't have to do this on thei=
r own.
> > > >
> > > > My understanding is that the dmabuf interface is used to share DMA
> > > > buffers across different drivers, while these buffers are used
> > > > only internally to the IPC driver (and exchanged only with the VPU
> > > > firmware). They basically are packet headers that are sent to the V=
PU.
> > >
> > > There's no reason you couldn't use these to share your buffers
> > > "internally" as well, because you have the same lifetime rules and
> > > accounting and all other sorts of things you have to handle, right?
> > > Why rewrite something like this when you should take advantage of
> > > common code instead?
> >
> > I looked at dma-buf again, but I'm still failing to see how we can use
> > it in this driver :/
> >
> > The problem I'm not able to solve is exactly how to match the lifetime
> > of this IPC packets (IPC buffers is probably a misleading name, my bad
> > for using it in the code) with the dma-buf lifetime rules.
> >
> > Basically, these IPC packets are cache-aligned (64 bytes) and have the
> > following fixed structure:
> >
> >    struct kmb_ipc_buf {
> >           u32 data_addr;
> >           u32 data_size;
> >           u16 channel;
> >           u8  src_node;
> >           u8  dst_node;
> >           u8  status;
> >    } __packed __aligned(KMB_IPC_ALIGNMENT);
>=20
> If that is shared out of the kernel, please use the proper data types for=
 it.
>=20
> And it isn't properly padded out?  That feels like a hardware design mist=
ake
> somewhere, anyway...
>=20
> > Now, let's ignore the first 5 fields and focus on the last one that
> > controls the lifetime of the packet. The status field can be either
> > FREE or ALLOCATED (perhaps should be renamed to IN_USE).
> >
> > Basically, during probe, the driver allocates an array (pool) of these
> > packets from its 'local' reserved memory region. This is done using
> > dma_alloc_coherent(). This array remains allocated until remove() is
> > called.
> >
> > At run-time, every time the driver sends a message to the VPU, the
> > following happens:
> >
> >    1. The IPC driver loops through the array of 'local' packets to find
> >       the first one marked as FREE
> >    2. The IPC driver marks the packet as IN_USE and shares it with the =
VPU
> >       by putting its physical address into the VPU HW FIFO.
> >    3. The VPU Firmware processes the packet and, when done with it, it
> >       marks it as FREE again so that the IPC driver can use it again.
>=20
> How does the firmware "mark it as free"?
>=20
> > Please note that the IPC driver is not notified by the VPU when the
> > packet is marked as free (there is no IRQ or other async mechanism to
> > do that). The driver will simply find the message free the next time
> > it loops over the message pool again (i.e., when it is trying to send
> > a new message).
> >
> > Given that the free operation is silently done by the VPU, I'm failing
> > to see how I can use dmabuf (or gen_pool or dma_pool). Is there
> > anything that I'm missing?
>=20
> So maybe this isn't set up to use the dma buffers, but it still feels rea=
lly odd.  I
> recommend getting a review by the dmabuf authors before declaring that yo=
u
> really should be rolling your own buffer management logic here, as you ca=
n see
> that it feels like you are doing much the same thing, sharing memory outs=
ide of
> the kernel to something else.
Great idea!  I will do this on the V2 posting.

Say, we've gotten some really good feedback so far and I'm thinking about p=
osting an update with most of the issues raised addressed next week.  There=
 is one feedback that looks like will take a bit longer to address (splitti=
ng out and use of the common mailbox framework in the keembay-ipc driver). =
 Would it be acceptable to post a V2 that has some ToDo's in it to allow fo=
r more feedback to get to the rest of the stack or should we hold until we =
get all the issues address before posting the V2?

Thanks again for your advice and review feedback!

--mark


>=20
> thanks,
>=20
> greg k-h