Re: [dpdk-dev] [PATCH v23 1/6] dmadev: introduce DMA device library

[Thomas Monjalon <thomas@monjalon.net>]

08/10/2021 09:13, fengchengwen:
> On 2021/10/6 18:26, Thomas Monjalon wrote:
> > 24/09/2021 12:53, Chengwen Feng:
> >> +++ 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.
> 
> Yes, we could remove the stage descriptor because it's a well-known knowledge, but I
> recommend keeping the rte_dma_pmd_allocate and rte_dma_pmd_release functions, how about:
> 
>  * The dmadev are dynamically allocated by rte_dma_pmd_allocate(). And could
>  * be released by rte_dma_pmd_release().

These functions are for PMD.
This file is for applications, so it is not appropriate.

> > [...]
> >> + * 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?
> 
> The default macro is RTE_DMADEV_DEFAULT_MAX_DEVS, and I place it at rte_config.h.

No we avoid adding thinds in rte_config.h.
There should a static default which can be changed at runtime only.

> I think it's better to focus on one place (rte_config.h) than to modify config in multiple places (e.g. rte_dmadev.h/rte_xxx.h).

Config is modified only in one place: the function.

> >> + *
> >> + * @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.
> 
> Yes, we defined dma_data_prepare() which do above thing, it's in 4th patch.
> 
> Because we could not invoke some like allocate inter-process shared memory before
> rte_eal_init, so I think it's better keep rte_dma_dev_max as it is.

Good point.

> >> +++ 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?
> 
> Yes, it's used by dataplane inline functions.

OK, please give this reason in the description.