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 3D723A0C41; Wed, 6 Oct 2021 12:46:31 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1219B40688; Wed, 6 Oct 2021 12:46:31 +0200 (CEST) Received: from new1-smtp.messagingengine.com (new1-smtp.messagingengine.com [66.111.4.221]) by mails.dpdk.org (Postfix) with ESMTP id 403A840140 for ; Wed, 6 Oct 2021 12:46:29 +0200 (CEST) Received: from compute4.internal (compute4.nyi.internal [10.202.2.44]) by mailnew.nyi.internal (Postfix) with ESMTP id DDCCE5810D8; Wed, 6 Oct 2021 06:46:28 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute4.internal (MEProxy); Wed, 06 Oct 2021 06:46:28 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=fm2; bh= 4GvGLBkzw5UQkMQd0auE9Rx/DlJ1Jst8aPorRLGjrIs=; b=ZCnjNcCNEdEmFkOO gCo5az23t/++CZYi6kMLNwKMcVakSsWzBVG0u3gD7trOAL/iQTzRUhTtWF0pXoRd gfI54IxhyPWyF9r7/PTqzwE4IxHmQO94f87jYBrnTlLAttyrcWn47Ig+pkpmu0cV ohCjRGLEQRqKR2KWhFOHxq4uCZLl1VDuafyWzybigZpf7UQ7HRtXaxtXqRuOCY1j d35FPe2nZIBXaxDLj2AxbF7MUjWTJt6r2A59LYwm8zVAQHYxZvV4/Wfezf5Rcuv5 x/Ce+l8at/Mgb1pReczAGdC5jlLYsTV43rDzaWj1PLCHlKbtatT5G1oVzrM6T/C9 KwUBcg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm1; bh=4GvGLBkzw5UQkMQd0auE9Rx/DlJ1Jst8aPorRLGjr Is=; b=KkdpGNR5tF+Nrmzfha5k/8mH5hIvVP+oWQbvwCNNWBvkVU/3i9gKjrKSP PQDsE1n/5/2RQON8pukEWrG5X/CQeVyzB2sxkK0ga6JKf2UMJLyvUgV24lMACo0o Z4Yy4VITaMl3BYMhsGP9R1bEQEDU4fZsx522fMpYrLtMzTWcjtWTIo84k5OV8aFg y6rrEZXLo5KLCT1xgibx90sZ4JE1/Qq2AxBpcR7dlL89CrgP2Xa1Zb8HS3WubuKK yHwWFTyPt/bSDSgA/AqFfVNk2z01kaHIi9wJoNM+t2b0FBkRqwS9o8Po3xYZZjQx qUCvbJEqYktyiuyW8hI/nYveRvrsw== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrudeliedgfedtucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvufffkfgjfhgggfgtsehtufertddttddvnecuhfhrohhmpefvhhhomhgr shcuofhonhhjrghlohhnuceothhhohhmrghssehmohhnjhgrlhhonhdrnhgvtheqnecugg ftrfgrthhtvghrnhepffdvffejueetleefieeludduuefgteejleevfeekjeefieegheet ffdvkeefgedunecuffhomhgrihhnpeguphgukhdrohhrghenucevlhhushhtvghrufhiii gvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehthhhomhgrshesmhhonhhjrghlohhn rdhnvght X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Wed, 6 Oct 2021 06:46:24 -0400 (EDT) From: Thomas Monjalon To: Chengwen Feng Cc: ferruh.yigit@intel.com, bruce.richardson@intel.com, jerinj@marvell.com, jerinjacobk@gmail.com, andrew.rybchenko@oktetlabs.ru, dev@dpdk.org, mb@smartsharesystems.com, nipun.gupta@nxp.com, hemant.agrawal@nxp.com, maxime.coquelin@redhat.com, honnappa.nagarahalli@arm.com, david.marchand@redhat.com, sburla@marvell.com, pkapoor@marvell.com, konstantin.ananyev@intel.com, conor.walsh@intel.com, kevin.laatz@intel.com Date: Wed, 06 Oct 2021 12:46:22 +0200 Message-ID: <11425429.3DvE4F450j@thomas> In-Reply-To: <20210924105357.15386-3-fengchengwen@huawei.com> References: <1625231891-2963-1-git-send-email-fengchengwen@huawei.com> <20210924105357.15386-1-fengchengwen@huawei.com> <20210924105357.15386-3-fengchengwen@huawei.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH v23 2/6] dmadev: add control plane function support 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" 24/09/2021 12:53, Chengwen Feng: > --- a/doc/guides/prog_guide/dmadev.rst > +++ b/doc/guides/prog_guide/dmadev.rst > @@ -62,3 +62,44 @@ identifiers: > > - A device name used to designate the DMA device in console messages, for > administration or debugging purposes. > + > + > +Device Configuration > +~~~~~~~~~~~~~~~~~~~~ > + > +The rte_dma_configure API is used to configure a DMA device. > + > +.. code-block:: c > + > + int rte_dma_configure(int16_t dev_id, > + const struct rte_dma_conf *dev_conf); > + > +The ``rte_dma_conf`` structure is used to pass the configuration parameters > +for the DMA device for example the number of virtual DMA channels to set up, > +indication of whether to enable silent mode. > + > + > +Configuration of Virtual DMA Channels > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + > +The rte_dma_vchan_setup API is used to configure a virtual DMA channel. > + > +.. code-block:: c > + > + int rte_dma_vchan_setup(int16_t dev_id, uint16_t vchan, > + const struct rte_dma_vchan_conf *conf); > + > +The ``rte_dma_vchan_conf`` structure is used to pass the configuration > +parameters for the virtual DMA channel for example transfer direction, number of > +descriptor for the virtual DMA channel, source device access port parameter, > +destination device access port parameter. You should avoid being redundant with the Doxygen documentation. In the guide, it should be only explaining the concepts, not the details. For the details of each function, we refer to Doxygen. > --- a/lib/dmadev/rte_dmadev.c > +++ b/lib/dmadev/rte_dmadev.c > @@ -218,6 +218,9 @@ rte_dma_pmd_release(const char *name) > if (dev == NULL) > return -EINVAL; > > + if (dev->state == RTE_DMA_DEV_READY) > + return rte_dma_close(dev->dev_id); What is the logic here? The only exposed function should be rte_dma_close() and it should call the freeing function. The API should use the dev_id. As you said somewhere else, the name is only for debugging. Please remove the function rte_dma_pmd_release(const char *name). [...] > --- a/lib/dmadev/rte_dmadev.h > +++ b/lib/dmadev/rte_dmadev.h > + * The functions exported by the dmadev API to setup a device designated by its > + * device identifier must be invoked in the following order: > + * - rte_dma_configure() > + * - rte_dma_vchan_setup() > + * - rte_dma_start() > + * > + * If the application wants to change the configuration (i.e. invoke > + * rte_dma_configure() or rte_dma_vchan_setup()), it must invoke > + * rte_dma_stop() first to stop the device and then do the reconfiguration > + * before invoking rte_dma_start() again. The dataplane functions should not > + * be invoked when the device is stopped. > + * > + * Finally, an application can close a dmadev by invoking the rte_dma_close() > + * function. Yes rte_dma_close, not rte_dma_pmd_release. > + * > + * About MT-safe, all the functions of the dmadev API exported by a PMD are API is not exported by a PMD, but implemented. > + * lock-free functions which assume to not be invoked in parallel on different > + * logical cores to work on the same target dmadev object. > + * @note Different virtual DMA channels on the same dmadev *DO NOT* support > + * parallel invocation because these virtual DMA channels share the same > + * HW-DMA-channel. > + * > */ No need of final blank line in a comment. > +/** DMA device support memory-to-memory transfer. > + * > + * @see struct rte_dma_info::dev_capa > + */ > +#define RTE_DMA_CAPA_MEM_TO_MEM RTE_BIT64(0) > +/** DMA device support memory-to-device transfer. > + * > + * @see struct rte_dma_info::dev_capa > + */ > +#define RTE_DMA_CAPA_MEM_TO_DEV RTE_BIT64(1) Same comment as in earlier version: please group the flags in a doxygen group. Example of doxygen group: https://patches.dpdk.org/project/dpdk/patch/20210830104232.598703-1-thomas@monjalon.net/ [...] You are using uint64_t bitfields and anonymous union in below struct, it may not compile if not using __extension__ from RTE_STD_C11. > +struct rte_dma_port_param { > + /** The device access port type. > + * > + * @see enum rte_dma_port_type > + */ > + enum rte_dma_port_type port_type; > + union { [...] > + struct { > + uint64_t coreid : 4; /**< PCIe core id used. */ > + uint64_t pfid : 8; /**< PF id used. */ > + uint64_t vfen : 1; /**< VF enable bit. */ > + uint64_t vfid : 16; /**< VF id used. */ > + /** The pasid filed in TLP packet. */ > + uint64_t pasid : 20; > + /** The attributes filed in TLP packet. */ > + uint64_t attr : 3; > + /** The processing hint filed in TLP packet. */ > + uint64_t ph : 2; > + /** The steering tag filed in TLP packet. */ > + uint64_t st : 16; > + } pcie; > + }; > + uint64_t reserved[2]; /**< Reserved for future fields. */ > +}; > --- a/lib/dmadev/rte_dmadev_core.h > +++ b/lib/dmadev/rte_dmadev_core.h > +/** @internal Used to get device information of a device. */ > +typedef int (*rte_dma_info_get_t)(const struct rte_dma_dev *dev, > + struct rte_dma_info *dev_info, > + uint32_t info_sz); Please move all driver interfaces in a file dedicated to drivers. [...] > @@ -40,9 +96,13 @@ struct rte_dma_dev { > int16_t dev_id; /**< Device [external] identifier. */ > int16_t numa_node; /**< Local NUMA memory ID. -1 if unknown. */ > void *dev_private; /**< PMD-specific private data. */ > + /** Functions exported by PMD. */ s/exported/implemented/ > + const struct rte_dma_dev_ops *dev_ops; > + struct rte_dma_conf dev_conf; /**< DMA device configuration. */ > /** Device info which supplied during device initialization. */ > struct rte_device *device; > enum rte_dma_dev_state state; /**< Flag indicating the device state. */ > + uint8_t dev_started : 1; /**< Device state: STARTED(1)/STOPPED(0). */ > uint64_t reserved[2]; /**< Reserved for future fields. */ > } __rte_cache_aligned;