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

[fengchengwen]

On 2021/10/6 18:26, Thomas Monjalon wrote:
> 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

[snip]

> 
> [...]
>> +++ 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().

> 
> [...]
>> + * 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.

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).

> 
>> + *
>> + * @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.

> 
>> +/**
>> + * @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?

Yes, it's used by dataplane inline functions.

[snip]

> .
> 

Thanks