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.
