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.