From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 43C2DA0A0C; Fri, 2 Jul 2021 09:39:14 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 14F9C40141; Fri, 2 Jul 2021 09:39:14 +0200 (CEST) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id B2EEB4003E for ; Fri, 2 Jul 2021 09:39:12 +0200 (CEST) Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Date: Fri, 2 Jul 2021 09:39:10 +0200 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35C618D1@smartserver.smartshare.dk> In-Reply-To: X-MimeOLE: Produced By Microsoft Exchange V6.5 X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [dpdk-dev] dmadev discussion summary Thread-Index: AdduluM0E8ZMlQFxRKOacYbU2fcZOAAdoS8w References: <25d29598-c26d-8497-2867-9b650c79df49@huawei.com> <3db2eda0-4490-2b8f-c65d-636bcf794494@huawei.com> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Bruce Richardson" , "Jerin Jacob" Cc: "fengchengwen" , "Jerin Jacob" , "Nipun Gupta" , "Thomas Monjalon" , "Ferruh Yigit" , "dpdk-dev" , "Hemant Agrawal" , "Maxime Coquelin" , "Honnappa Nagarahalli" , "David Marchand" , "Satananda Burla" , "Prasun Kapoor" Subject: Re: [dpdk-dev] dmadev discussion summary X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > From: Bruce Richardson [mailto:bruce.richardson@intel.com] > Sent: Thursday, 1 July 2021 18.34 >=20 > On Thu, Jul 01, 2021 at 08:31:00PM +0530, Jerin Jacob wrote: > > On Sat, Jun 26, 2021 at 9:29 AM fengchengwen > wrote: > > > > > > Hi, all > > > I analyzed the current DPAM DMA driver and drew this summary in > conjunction > > > with the previous discussion, and this will as a basis for the V2 > implementation. > > > Feedback is welcome, thanks > > > > Thanks for the write-up. > > > > > > > > > [queue_setup]: create one virt-queue, has following main > parameters: > > > HW-queue-index: the HW-queue index used > > > nb_desc: the number of HW descriptors > > > opaque: driver's specific info > > > Note1: this API return virt-queue index which will used = in > later API. > > > If user want create multiple virt-queue one the > same HW-queue, > > > they could achieved by call queue_setup with the > same > > > HW-queue-index. > > > Note2: I think it's hard to define queue_setup config > paramter, and > > > also this is control API, so I think it's OK to = use > opaque > > > pointer to implement it. > > > [dma_copy/memset/sg]: all has vq_id input parameter. > > > Note: I notice dpaa can't support single and sg in one > virt-queue, and > > > I think it's maybe software implement policy other > than HW > > > restriction because virt-queue could share the same > HW-queue. > > > Here we use vq_id to tackle different scenario, like local- > to-local/ > > > local-to-host and etc. > > > > IMO, The index representation has an additional overhead as one = needs > > to translate it > > to memory pointer. I prefer to avoid by having object handle and use > > _lookup() API get to make it work > > in multi-process cases to avoid the additional indirection. Like > mempool object. >=20 > While it doesn't matter to me that much which is chosen, an index = seems > cleaner to me and more consistent with other device types in DPDK. The > objects pointed to by the memory pointers you refer too can't just be > stored in an internal array in your driver and accessed directly by > index, > saving that layer of redirection? The rte_eth_rx/tx_burst() functions use the parameter "uint16_t port_id" = to identify the device. rte_eth_rx/tx_burst() needs to look up the rte_eth_dev pointer in the = rte_eth_devices array, which could be avoided by using "struct = rte_eth_dev *dev" instead of "uint16_t port_id". It would be faster. However, in the case of rte_eth_rx/tx_burst(), the port_id is rapidly = changing at runtime, and often comes from the mbuf or similar, where it = is preferable storing a 16 bit value rather than a 64 bit pointer. I think that the choice of DMA device (and virt-queue) for DMA fast path = operations will be much more constant than the choice of port_id (and = queue_id) in Ethdev fast path operations. If you agree with this, we = should use "struct rte_dma_dev *dev" rather than "uint16_t dev_id" as = parameter to the DMA fast path functions. Going even further, why do we need to pass both dev_id and virt_queue_id = to the DMA fast path functions, instead of just passing a pointer to the = virt-queue? That pointer could be returned by the rte_dma_queue_setup() = function. And it could be opaque or a well defined structure. Or even = more exotic: It could be a structure where the first part is common and = well defined, and the rest of the structure is driver specific. >=20 > > > 5) And the dmadev public data-plane API (just prototype): > > > dma_cookie_t rte_dmadev_memset(dev, vq_id, pattern, dst, len, > flags) > > > -- flags: used as an extended parameter, it could be > uint32_t > > > dma_cookie_t rte_dmadev_memcpy(dev, vq_id, src, dst, len, > flags) > > > dma_cookie_t rte_dmadev_memcpy_sg(dev, vq_id, sg, sg_len, > flags) > > > -- sg: struct dma_scatterlist array > > > uint16_t rte_dmadev_completed(dev, vq_id, dma_cookie_t > *cookie, > > > uint16_t nb_cpls, bool > *has_error) > > > -- nb_cpls: indicate max process operations number > > > -- has_error: indicate if there is an error > > > -- return value: the number of successful completed > operations. > > > -- example: > > > 1) If there are already 32 completed ops, and 4th is > error, and > > > nb_cpls is 32, then the ret will be 3(because 1/2/3th > is OK), and > > > has_error will be true. > > > 2) If there are already 32 completed ops, and all > successful > > > completed, then the ret will be min(32, nb_cpls), and > has_error > > > will be false. > > > 3) If there are already 32 completed ops, and all failed > completed, > > > then the ret will be 0, and has_error will be true. > > > > +1. IMO, it is better to call ring_idx instead of a cookie. To > enforce > > that it the ring index. > > > +1, I like that name better too. If it is "ring index" then it probably should be an uintXX_t type, and = thus the functions cannot return <0 to indicate error. By "ring index", do you actually mean "descriptor index" (for the DMA = engine's descriptors)? Does the application need to process this value = as anything but an opaque handle? Wouldn't an opaque pointer type = provide performance? It would also allow using NULL to indicate error. Do you expect applications to store the cookie in structures that are = instantiated many times (e.g. like the port_id is stored in the mbuf = structure), so there is an advantage to using an uint16_t instead of a = pointer? >=20 > > > uint16_t rte_dmadev_completed_status(dev_id, vq_id, > dma_cookie_t *cookie, > > > uint16_t nb_status, > uint32_t *status) > > > -- return value: the number of failed completed operations. > > > > See above. Here we are assuming it is an index otherwise we need to > > pass an array > > cookies. > > > > > And here I agree with Morten: we should design API which > adapts to DPDK > > > service scenarios. So we don't support some sound-cards DMA, > and 2D memory > > > copy which mainly used in video scenarios. > > > 6) The dma_cookie_t is signed int type, when <0 it mean error, > it's > > > monotonically increasing base on HW-queue (other than virt- > queue). The > > > driver needs to make sure this because the damdev framework > don't manage > > > the dma_cookie's creation. > > > > +1 and see above. > > > > > 7) Because data-plane APIs are not thread-safe, and user could > determine > > > virt-queue to HW-queue's map (at the queue-setup stage), so = it > is user's > > > duty to ensure thread-safe. > > > > +1. But I am not sure how easy for the fast-path application to have > this logic, > > Instead, I think, it is better to tell the capa for queue by driver > > and in channel configuration, > > the application can request for requirement (Is multiple producers > enq > > to the same HW queue or not). > > Based on the request, the implementation can pick the correct > function > > pointer for enq.(lock vs lockless version if HW does not support > > lockless) > > >=20 > Non-thread safety is the norm in DPDK for all other queue resources, > however, haivng multi-thread safety as a capability sounds reasonable. > > > > > 8) One example: vq_id =3D rte_dmadev_queue_setup(dev, > > > config.{HW-queue-index=3Dx, opaque}); if (vq_id < 0) { // create > > > virt-queue failed return; } // submit memcpy task cookit =3D > > > rte_dmadev_memcpy(dev, vq_id, src, dst, len, flags); if (cookie = < > 0) > > > { // submit failed return; } // get complete task ret =3D > > > rte_dmadev_completed(dev, vq_id, &cookie, 1, has_error); if > > > (!has_error && ret =3D=3D 1) { // the memcpy successful complete = } > > > > +1 > > > > > 9) As octeontx2_dma support sg-list which has many valid buffers > in > > > dpi_dma_buf_ptr_s, it could call the rte_dmadev_memcpy_sg API. > > > > +1 > > > > > 10) As ioat, it could delcare support one HW-queue at > dev_configure > > > stage, and only support create one virt-queue. 11) As > dpaa2_qdma, I > > > think it could migrate to new framework, but still wait for > > > dpaa2_qdma guys feedback. 12) About the prototype src/dst > parameters > > > of rte_dmadev_memcpy API, we have two candidates which are iova > and > > > void *, how about introduce dma_addr_t type which could be va or > iova > > > ? > > > > I think, conversion looks ugly, better to have void * and share the > > constraints of void * as limitation/capability using flag. So that > driver > > can update it. > > > I'm ok with either rte_iova_t or void * as parameter type. Let's not > define a new type though, +1 > and +1 to just using capabilities to define what kinds > of addressing are supported by the device instances. +1 >=20 > /Bruce And regarding naming: Consider rte_dma_xx() instead of rte_dmadev_xx(). -Morten