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 18570A0C47; Wed, 14 Jul 2021 18:05:22 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7EF5F4069F; Wed, 14 Jul 2021 18:05:21 +0200 (CEST) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by mails.dpdk.org (Postfix) with ESMTP id 058264068A for ; Wed, 14 Jul 2021 18:05:18 +0200 (CEST) X-IronPort-AV: E=McAfee;i="6200,9189,10045"; a="271487599" X-IronPort-AV: E=Sophos;i="5.84,239,1620716400"; d="scan'208";a="271487599" Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Jul 2021 09:05:17 -0700 X-IronPort-AV: E=Sophos;i="5.84,239,1620716400"; d="scan'208";a="413333675" Received: from bricha3-mobl.ger.corp.intel.com ([10.252.5.184]) by orsmga003-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 14 Jul 2021 09:05:13 -0700 Date: Wed, 14 Jul 2021 17:05:10 +0100 From: Bruce Richardson To: Chengwen Feng Cc: thomas@monjalon.net, ferruh.yigit@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 Message-ID: References: <1625231891-2963-1-git-send-email-fengchengwen@huawei.com> <1626179263-14645-1-git-send-email-fengchengwen@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1626179263-14645-1-git-send-email-fengchengwen@huawei.com> Subject: Re: [dpdk-dev] [PATCH v3] 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" On Tue, Jul 13, 2021 at 08:27:43PM +0800, Chengwen Feng wrote: > This patch introduce 'dmadevice' which is a generic type of DMA > device. > > The APIs of dmadev library exposes some generic operations which can > enable configuration and I/O with the DMA devices. > > Signed-off-by: Chengwen Feng More review comments - mostly stylistic - inline below. /Bruce > --- > v3: > * rm reset and fill_sg ops. > * rm MT-safe capabilities. > * add submit flag. > * redefine rte_dma_sg to implement asymmetric copy. > * delete some reserved field for future use. > * rearrangement rte_dmadev/rte_dmadev_data struct. > * refresh rte_dmadev.h copyright. > * update vchan setup parameter. > * modified some inappropriate descriptions. > * arrange version.map alphabetically. > * other minor modifications from review comment. > --- > + > +#include > +#include > +#ifdef RTE_DMADEV_DEBUG > +#include > +#endif I don't see the value in conditionally including this. I'd simplify by just always including it. > +#include > +#include > + > +#ifdef __cplusplus > +extern "C" { > +#endif > + > +#define RTE_DMADEV_NAME_MAX_LEN RTE_DEV_NAME_MAX_LEN > + > +/** > + * @warning > + * @b EXPERIMENTAL: this API may change without prior notice. > + * > + * @param dev_id > + * DMA device index. > + * > + * @return > + * - If the device index is valid (true) or not (false). > + */ > +__rte_experimental > +bool > +rte_dmadev_is_valid_dev(uint16_t dev_id); > + > +/** > + * @warning > + * @b EXPERIMENTAL: this API may change without prior notice. > + * > + * Get the total number of DMA devices that have been successfully > + * initialised. > + * > + * @return > + * The total number of usable DMA devices. > + */ > +__rte_experimental > +uint16_t > +rte_dmadev_count(void); > + > +/** > + * The capabilities of a DMA device > + */ This should be a non-doxygen comment, as it doesn't apply to a code element. > +#define RTE_DMA_DEV_CAPA_MEM_TO_MEM (1ull << 0) > +/**< DMA device support memory-to-memory transfer. > + * > + * @see struct rte_dmadev_info::dev_capa > + */ These comments should come before the items they refer to, not after. > +#define RTE_DMA_DEV_CAPA_MEM_TO_DEV (1ull << 1) > +/**< DMA device support memory-to-device transfer. > + * > + * @see struct rte_dmadev_info::dev_capa > + */ > +#define RTE_DMA_DEV_CAPA_DEV_TO_MEM (1ull << 2) > +/**< DMA device support device-to-memory transfer. > + * > + * @see struct rte_dmadev_info::dev_capa > + */ > +#define RTE_DMA_DEV_CAPA_DEV_TO_DEV (1ull << 3) > +/**< DMA device support device-to-device transfer. > + * > + * @see struct rte_dmadev_info::dev_capa > + */ > +#define RTE_DMA_DEV_CAPA_OPS_COPY (1ull << 4) Do we want to leave gaps in the flags so that they are grouped by op type? Is it possible that we might ahve more RTE_DMA_DEV_X_TO_Y flags in future, because if so, we should move this out to bit 8, for example. > +/**< DMA device support copy ops. > + * > + * @see struct rte_dmadev_info::dev_capa > + */ > +#define RTE_DMA_DEV_CAPA_OPS_FILL (1ull << 5) > +/**< DMA device support fill ops. > + * > + * @see struct rte_dmadev_info::dev_capa > + */ > +#define RTE_DMA_DEV_CAPA_OPS_SG (1ull << 6) > +/**< DMA device support scatter-list ops. > + * If device support ops_copy and ops_sg, it means supporting copy_sg ops. > + * > + * @see struct rte_dmadev_info::dev_capa > + */ Rather than a general SG flag, this should probably be for SG_COPY, since we aren't offering an SG_FILL option. > +#define RTE_DMA_DEV_CAPA_FENCE (1ull << 7) > +/**< DMA device support fence. > + * If device support fence, then application could set a fence flags when > + * enqueue operation by rte_dma_copy/copy_sg/fill/fill_sg. > + * If a operation has a fence flags, it means the operation must be processed > + * only after all previous operations are completed. > + * > + * @see struct rte_dmadev_info::dev_capa > + */ Drop this flag as unnecessary. All devices either always provide ordering guarantee - in which case it's a no-op - or else support the flag. > +#define RTE_DMA_DEV_CAPA_SVA (1ull << 8) Again, if we are ok to leave gaps, I'd suggest moving this one well down, e.g. to bit 32. > +/**< DMA device support SVA which could use VA as DMA address. > + * If device support SVA then application could pass any VA address like memory > + * from rte_malloc(), rte_memzone(), malloc, stack memory. > + * If device don't support SVA, then application should pass IOVA address which > + * from rte_malloc(), rte_memzone(). > + * > + * @see struct rte_dmadev_info::dev_capa > + */ > + > +/** > + * A structure used to retrieve the contextual information of > + * an DMA device > + */ > +struct rte_dmadev_info { > + struct rte_device *device; /**< Generic Device information */ > + uint64_t dev_capa; /**< Device capabilities (RTE_DMA_DEV_CAPA_*) */ > + /** Maximum number of virtual DMA channels supported */ > + uint16_t max_vchans; > + /** Maximum allowed number of virtual DMA channel descriptors */ > + uint16_t max_desc; > + /** Minimum allowed number of virtual DMA channel descriptors */ > + uint16_t min_desc; > + uint16_t nb_vchans; /**< Number of virtual DMA channel configured */ > +}; Minor nit - I suggest standardizing the comment format here and have them all either before, or all afterwards. Since they won't all fit in your 80-column limit, make all comments appear before the item. > + > +/** > + * @warning > + * @b EXPERIMENTAL: this API may change without prior notice. > + * > + * Retrieve the contextual information of a DMA device. Suggest shortening to "Retrieve information about a DMA device". There is no context info provided here. > + * > + * @param dev_id > + * The identifier of the device. > + * @param[out] dev_info > + * A pointer to a structure of type *rte_dmadev_info* to be filled with the > + * contextual information of the device. I'd drop the word "contextual" here too. > + * > + * @return > + * - =0: Success, driver updates the contextual information of the DMA device > + * - <0: Error code returned by the driver info get function. > + * > + */ > +__rte_experimental > +int > +rte_dmadev_info_get(uint16_t dev_id, struct rte_dmadev_info *dev_info); > + > +/** > + * DMA transfer direction defines. > + */ > +#define RTE_DMA_MEM_TO_MEM (1ull << 0) > +/**< DMA transfer direction - from memory to memory. > + * > + * @see struct rte_dmadev_vchan_conf::direction > + */ As with other bit flags, please put the comments on top. > +#define RTE_DMA_MEM_TO_DEV (1ull << 1) > +/**< DMA transfer direction - from memory to device. > + * In a typical scenario, ARM SoCs are installed on x86 servers as iNICs > + * through the PCIE interface. In this case, the ARM SoCs works in EP(endpoint) > + * mode, it could initiate a DMA move request from memory (which is ARM memory) > + * to device (which is x86 host memory). > +/** > + * DMA flags to augment operation preparation. > + * Used as the 'flags' parameter of rte_dmadev_copy/fill. > + */ > +#define RTE_DMA_OP_FLAG_FENCE (1ull << 0) > +/**< DMA fence flag > + * It means the operation with this flag must be processed only after all > + * previous operations are completed. > + * > + * @see rte_dmadev_copy() > + * @see rte_dmadev_copy_sg() > + * @see rte_dmadev_fill() > + */ > +#define RTE_DMA_OP_FLAG_SUBMIT (1ull << 1) > +/**< DMA submit flag > + * It means the operation with this flag must issue doorbell to hardware after > + * enqueued jobs. > + */ Comments before define. > + > +/** > +/** > + * DMA transfer status code defines > + */ > +enum rte_dma_status_code { > + /** The operation completed successfully */ > + RTE_DMA_STATUS_SUCCESSFUL = 0, > + /** The operation failed to complete due active drop > + * This is mainly used when processing dev_stop, allow outstanding > + * requests to be completed as much as possible. > + */ > + RTE_DMA_STATUS_ACTIVE_DROP, Is this saying that the operation is aborted? I'm not familiar with the phrase "active drop". > + /** The operation failed to complete due invalid source address */ > + RTE_DMA_STATUS_INVALID_SRC_ADDR, > + /** The operation failed to complete due invalid destination address */ > + RTE_DMA_STATUS_INVALID_DST_ADDR, > + /** The operation failed to complete due invalid length */ > + RTE_DMA_STATUS_INVALID_LENGTH, > + /** The operation failed to complete due invalid opcode > + * The DMA descriptor could have multiple format, which are > + * distinguished by the opcode field. > + */ > + RTE_DMA_STATUS_INVALID_OPCODE, > + /** The operation failed to complete due bus err */ > + RTE_DMA_STATUS_BUS_ERROR, > + /** The operation failed to complete due data poison */ > + RTE_DMA_STATUS_DATA_POISION, > + /** The operation failed to complete due descriptor read error */ > + RTE_DMA_STATUS_DESCRIPTOR_READ_ERROR, > + /** The operation failed to complete due device link error > + * Used to indicates that the link error in the mem-to-dev/dev-to-mem/ > + * dev-to-dev transfer scenario. > + */ > + RTE_DMA_STATUS_DEV_LINK_ERROR, > + /** The operation failed to complete due unknown reason */ > + RTE_DMA_STATUS_UNKNOWN, > + /** Driver specific status code offset > + * Start status code for the driver to define its own error code. > + */ > + RTE_DMA_STATUS_DRV_SPECIFIC_OFFSET = 0x10000, > +}; I think we need a status error code for "not attempted", where jobs in a particular batch are not attempted because they appeared after a fence where a previous job failed. In our HW implementation it's possible for jobs from later batches would be completed, though, so we need to report the status from the not attempted jobs before reporting those newer completed jobs. > + > +/** > + * @warning > + * @b EXPERIMENTAL: this API may change without prior notice. > + * > + * Returns the number of operations that failed to complete. > + * NOTE: This API was used when rte_dmadev_completed has_error was set. > + * > + * @param dev_id > + * The identifier of the device. > + * @param vchan > + * The identifier of virtual DMA channel. > + * @param nb_status > + * Indicates the size of status array. > + * @param[out] status > + * The error code of operations that failed to complete. > + * Some standard error code are described in 'enum rte_dma_status_code' > + * @see rte_dma_status_code > + * @param[out] last_idx > + * The last failed completed operation's index. > + * > + * @return > + * The number of operations that failed to complete. > + */ > +__rte_experimental > +static inline uint16_t > +rte_dmadev_completed_fails(uint16_t dev_id, uint16_t vchan, > + const uint16_t nb_status, uint32_t *status, > + uint16_t *last_idx) > +{ Switch the final two parameters around, so that the prototype matches that of the previous completed() function, i.e. all start with dev_id, vchan, "count", last_idx, and then only differ in the final parameter. > + struct rte_dmadev *dev = &rte_dmadevices[dev_id]; > +#ifdef RTE_DMADEV_DEBUG > + if (!rte_dmadev_is_valid_dev(dev_id) || > + vchan >= dev->data->dev_conf.max_vchans || > + nb_status == 0 || > + status == NULL || > + last_idx == NULL) > + return -EINVAL; > + RTE_FUNC_PTR_OR_ERR_RET(*dev->completed_fails, -ENOTSUP); > +#endif Unlike "completed" there is no fallback assigning to non-null parameters. If we want to make the final two parameters mandatory, we should document this. > + return (*dev->completed_fails)(dev, vchan, nb_status, status, last_idx); > +} > + > +#ifdef __cplusplus > +} > +#endif > + > +#endif /* _RTE_DMADEV_H_ */ > diff --git a/lib/dmadev/rte_dmadev_core.h b/lib/dmadev/rte_dmadev_core.h > new file mode 100644 > index 0000000..b0b6494 > --- /dev/null > +++ b/lib/dmadev/rte_dmadev_core.h > @@ -0,0 +1,161 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2021 HiSilicon Limited. > + * Copyright(c) 2021 Intel Corporation. > + */ > + > +#ifndef _RTE_DMADEV_CORE_H_ > +#define _RTE_DMADEV_CORE_H_ > + > +/** > + * @file > + * > + * RTE 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. > + * > + */ > + > +struct rte_dmadev; > + > +/** @internal Used to get device information of a device. */ > +typedef int (*dmadev_info_get_t)(const struct rte_dmadev *dev, > + struct rte_dmadev_info *dev_info, > + uint32_t info_sz); > + Since rte_dmadev_core.h is included in rte_dmadev.h, these will be in the public namespace for all apps using dmadev, so they do need the "rte_" prefix. > +/** @internal Used to configure a device. */ > +typedef int (*dmadev_configure_t)(struct rte_dmadev *dev, > + const struct rte_dmadev_conf *dev_conf); > +/** > + * @internal > + * The generic data structure associated with each DMA device. > + * > + * The dataplane APIs are located at the beginning of the structure, along > + * with the pointer to where all the data elements for the particular device > + * are stored in shared memory. This split scheme allows the function pointer > + * and driver data to be per-process, while the actual configuration data for > + * the device is shared. > + */ > +struct rte_dmadev { > + dmadev_copy_t copy; > + dmadev_copy_sg_t copy_sg; > + dmadev_fill_t fill; > + dmadev_submit_t submit; > + dmadev_completed_t completed; > + dmadev_completed_fails_t completed_fails; > + void *reserved_ptr; /**< Reserved for future IO function */ > + struct rte_dmadev_data *data; /**< Pointer to device data. */ > + I think we will get better performance if we move this back down the struct, and instead put a copy of the data->dev_private pointer in its place. Driver implementations tend to use the private data much more than the generic public data struct. > + const struct rte_dmadev_ops *dev_ops; /**< Functions exported by PMD. */ > + /** Device info which supplied during device initialization. */ > + struct rte_device *device; > + enum rte_dmadev_state state; /**< Flag indicating the device state */ > + uint64_t reserved[2]; /**< Reserved for future fields */ > +} __rte_cache_aligned; > + > +extern struct rte_dmadev rte_dmadevices[]; > + > +#endif /* _RTE_DMADEV_CORE_H_ */ > diff --git a/lib/dmadev/rte_dmadev_pmd.h b/lib/dmadev/rte_dmadev_pmd.h