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 96CF6A0C41; Wed, 6 Oct 2021 12:26:52 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2DB8540696; Wed, 6 Oct 2021 12:26:52 +0200 (CEST) Received: from new1-smtp.messagingengine.com (new1-smtp.messagingengine.com [66.111.4.221]) by mails.dpdk.org (Postfix) with ESMTP id 38FD440688 for ; Wed, 6 Oct 2021 12:26:51 +0200 (CEST) Received: from compute6.internal (compute6.nyi.internal [10.202.2.46]) by mailnew.nyi.internal (Postfix) with ESMTP id 9279F58113D; Wed, 6 Oct 2021 06:26:50 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute6.internal (MEProxy); Wed, 06 Oct 2021 06:26:50 -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= nhEyOlnbT6NqrCBZQav+k1eR4jpYICR52bA61QxU4D0=; b=I1M00G1sa8WIst83 YRNVNOE6HH3qZ4gOK/XtmlXpqXKziQPTXIyT0Qt81m+7nxewQkTvEQgsuzZ2GcJj B0haYKO2OhvUlWYqlm8MbgaDBoZK9oq6lZak5nwRxF9oZZ8GYAMfkCp2scgz5Ehl 3lgHhDUFmccddaEsJnvolfof4qiPQYBz4ZUiLQS2wITLG/DlbKdGd+X3T7NqFNbQ cETRgN1FklWadj/p7cB6ftN5WkeEvUe0wR4ESJUUy9Wb9rsP5xRUUd3K6n0XrAg3 xDbIaxSEuXg0N+I6HlwMEcgNPY17r90B8NvW1yGfNi2qrwlRlPVBqgug+X3+iBKt 0P8ndA== 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=nhEyOlnbT6NqrCBZQav+k1eR4jpYICR52bA61QxU4 D0=; b=Chgb/4OM73Naowf2NBC6aE5D4y3p6WrbqLJIuiyx0sz7wu/MoOQVOkoYc mZnF4hmG1xM+9rMQRVLFXgFzVibgvfoQa8Qz6z9rg8+FMTTwIEIpC5A+3y+zpLNX qQWw2r/TYs76FecrSYMREeokDRCKXiA0fZdZRDbdg+AZIzyG0Upv1c2SfbhYfZ4T T5wpYG3DFifdFoT91lks1/MIqu1M4sec8MqNZlzD0mhCgl88HcnpMOA6AaqYNIAA vekLLxOwkX4As5CqToF+LkvXKzFbhxGMb/q6WRxdz1UxC29D2N95wJuQBB23UBlR I/VV5IQoonZkLdZ+p69CTa+lsnqTg== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrudeliedgvdeiucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvufffkfgjfhgggfgtsehtufertddttddvnecuhfhrohhmpefvhhhomhgr shcuofhonhhjrghlohhnuceothhhohhmrghssehmohhnjhgrlhhonhdrnhgvtheqnecugg ftrfgrthhtvghrnhepudeggfdvfeduffdtfeeglefghfeukefgfffhueejtdetuedtjeeu ieeivdffgeehnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrh homhepthhhohhmrghssehmohhnjhgrlhhonhdrnhgvth X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Wed, 6 Oct 2021 06:26:45 -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:26:42 +0200 Message-ID: <11670203.VNtdCpnXh1@thomas> In-Reply-To: <20210924105357.15386-2-fengchengwen@huawei.com> References: <1625231891-2963-1-git-send-email-fengchengwen@huawei.com> <20210924105357.15386-1-fengchengwen@huawei.com> <20210924105357.15386-2-fengchengwen@huawei.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH v23 1/6] dmadev: introduce DMA device library 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: > The 'dmadevice' is a generic type of DMA device. Do you mean 'dmadev' ? > This patch introduce the 'dmadevice' device allocation APIs. > > The infrastructure is prepared to welcome drivers in drivers/dma/ Good [...] > +The DMA library provides a DMA device framework for management and provisioning > +of hardware and software DMA poll mode drivers, defining generic APIs which We could consider the whole as *one* API. > +support a number of different DMA operations. > + > + > +Design Principles > +----------------- > + > +The DMA library follows the same basic principles as those used in DPDK's > +Ethernet Device framework and the RegEx framework. Not sure what this sentence means. Better to remove. > The DMA framework provides > +a generic DMA device framework which supports both physical (hardware) > +and virtual (software) DMA devices as well as a generic DMA API which allows > +DMA devices to be managed and configured and supports DMA operations to be > +provisioned on DMA poll mode driver. You could split this long sentence. [...] > +Physical DMA controllers are discovered during the PCI probe/enumeration of the > +EAL function which is executed at DPDK initialization, this is based on their > +PCI BDF (bus/bridge, device, function). Specific physical DMA controllers, like > +other physical devices in DPDK can be listed using the EAL command line options. > + > +The dmadevs are dynamically allocated by using the API not API, but function > +``rte_dma_pmd_allocate`` based on the number of hardware DMA channels. After the > +dmadev initialized successfully, the driver needs to switch the dmadev state to > +``RTE_DMA_DEV_READY``. Are we sure we need these details? > +Device Identification > +~~~~~~~~~~~~~~~~~~~~~ > + > +Each DMA device, whether physical or virtual is uniquely designated by two > +identifiers: > + > +- A unique device index used to designate the DMA device in all functions > + exported by the DMA API. > + > +- A device name used to designate the DMA device in console messages, for > + administration or debugging purposes. Good [...] > --- a/doc/guides/rel_notes/release_21_11.rst > +++ b/doc/guides/rel_notes/release_21_11.rst > @@ -106,6 +106,10 @@ New Features > Added command-line options to specify total number of processes and > current process ID. Each process owns subset of Rx and Tx queues. > > +* **Introduced dmadev library with:** > + > + * Device allocation APIs. There is no API for that, it is internal. >From an user perspective, you need only to list outstanding features in the release notes. [...] > +++ b/lib/dmadev/rte_dmadev.c > +RTE_LOG_REGISTER_DEFAULT(rte_dma_logtype, INFO); > +#define RTE_DMA_LOG(level, fmt, args...) \ > + rte_log(RTE_LOG_ ## level, rte_dma_logtype, "%s(): " fmt "\n", \ > + __func__, ##args) I don't like having function name in all logs. I recommend this form of macro: #define RTE_DMA_LOG(level, ...) \ rte_log(RTE_LOG_ ## level, rte_dma_logtype, RTE_FMT("dma: " \ RTE_FMT_HEAD(__VA_ARGS__,) "\n", RTE_FMT_TAIL(__VA_ARGS__,))) > +/* Macros to check for valid device id */ > +#define RTE_DMA_VALID_DEV_ID_OR_ERR_RET(dev_id, retval) do { \ > + if (!rte_dma_is_valid(dev_id)) { \ > + RTE_DMA_LOG(ERR, "Invalid dev_id=%d", dev_id); \ > + return retval; \ > + } \ > +} while (0) I dislike this macro doing a return. It is hiding stuff. I know we have it in other classes but I think it is a mistake, we should avoid macro blocks. > +static int16_t > +dma_find_free_dev(void) Actually it is looking for an ID, so it should be dma_find_free_id. > +{ > + int16_t i; > + > + if (rte_dma_devices == NULL) > + return -1; > + > + for (i = 0; i < dma_devices_max; i++) { > + if (rte_dma_devices[i].dev_name[0] == '\0') Instead of checking its name, it looks more logical to check the state. > + return i; > + } > + > + return -1; > +} > + > +static struct rte_dma_dev* > +dma_find(const char *name) dma_find_by_name? [...] > +++ b/lib/dmadev/rte_dmadev.h > + * The dmadev are dynamically allocated by rte_dma_pmd_allocate() during the > + * PCI/SoC device probing phase performed at EAL initialization time. And could > + * be released by rte_dma_pmd_release() during the PCI/SoC device removing > + * phase. I don't think this text has value, and we could imagine allocating a device ata later stage. [...] > + * Configure the maximum number of dmadevs. > + * @note This function can be invoked before the primary process rte_eal_init() > + * to change the maximum number of dmadevs. You should mention what is the default. Is the default exported to the app in this file? > + * > + * @param dev_max > + * maximum number of dmadevs. > + * > + * @return > + * 0 on success. Otherwise negative value is returned. > + */ > +__rte_experimental > +int rte_dma_dev_max(size_t dev_max); What about a function able to do more with the name rte_dma_init? It should allocate the inter-process shared memory, and do the lookup in case of secondary process. > +/** > + * @warning > + * @b EXPERIMENTAL: this API may change without prior notice. > + * > + * Get the device identifier for the named DMA device. > + * > + * @param name > + * DMA device name. > + * > + * @return > + * Returns DMA device identifier on success. > + * - <0: Failure to find named DMA device. > + */ > +__rte_experimental > +int rte_dma_get_dev_id(const char *name); Should we add _by_name? We could have a function to retrieve the ID by devargs as well. > +++ b/lib/dmadev/rte_dmadev_core.h > +/** > + * @file > + * > + * DMA Device internal header. > + * > + * This header contains internal data types, that are used by the DMA devices > + * in order to expose their ops to the class. > + * > + * Applications should not use these API directly. If it is not part of the API, it should not be exposed at all. Why not having all these stuff in a file dmadev_driver.h? Is it used by some inline functions? [...] > +++ b/lib/dmadev/rte_dmadev_pmd.h > +/** > + * @file > + * > + * DMA Device PMD APIs > + * > + * Driver facing APIs for a DMA device. These are not to be called directly by You cannot say API for drivers, because API means application interface. What you mean is "driver interface". > + * any application. > + */ [...] > + * Allocates a new dmadev slot for an DMA device and returns the pointer > + * to that slot for the driver to use. Please in all comments, use the infinitive form. Example: Allocates -> Allocate > + * > + * @param name > + * DMA device name. > + * @param numa_node > + * Driver's private data's numa node. s/numa/NUMA/ > + * @param private_data_size > + * Driver's private data size. > + * > + * @return > + * A pointer to the DMA device slot case of success, > + * NULL otherwise. > + */ > +__rte_internal > +struct rte_dma_dev *rte_dma_pmd_allocate(const char *name, int numa_node, > + size_t private_data_size); OK, sorry there are a lot of comments. Overrall, that's a good work. I know you are in holidays, I hope we can finish during next week.