On Thu, Sep 09, 2021 at 09:54:27PM +0800, fengchengwen wrote: > On 2021/9/9 20:45, Bruce Richardson wrote: > > On Thu, Sep 09, 2021 at 01:29:33PM +0200, Thomas Monjalon wrote: > >> 09/09/2021 13:18, Bruce Richardson: > >>> On Thu, Sep 09, 2021 at 12:33:00PM +0200, Thomas Monjalon wrote: > >>>> 07/09/2021 14:56, Chengwen Feng: > >>>>> + * The first three APIs are used to submit the operation request to > >>>>> the virtual > >>>>> + * DMA channel, if the submission is successful, an uint16_t ring_idx > >>>>> is > >>>>> + * returned, otherwise a negative number is returned. > >>>> > >>>> unsigned or negative? looks weird. > >>> > >>> May be, but it works well. We could perhaps rephase to make it less weird > >>> though: > >>> "if the submission is successful, a positive ring_idx <= UINT16_MAX is > >>> returned, otherwise a negative number is returned." > >> > >> I am advocating for int16_t, > >> it makes a lot of things simpler. > >> > > > > No, it doesn't work as you can't have wrap-around of the IDs once you use > > signed values - and that impacts both the end app and the internals of the > > drivers. Let's keep it as-is otherwise it will have massive impacts - > > including potential perf impacts. > > > >>>>> + * > >>>>> + * The last API was used to issue doorbell to hardware, and also there > >>>>> are flags > >>>>> + * (@see RTE_DMA_OP_FLAG_SUBMIT) parameter of the first three APIs > >>>>> could do the > >>>>> + * same work. > >>>> > >>>> I don't understand this sentence. > >>>> You mean rte_dmadev_submit function? > >>>> Why past tense "was"? > >>>> Why having a redundant function? > >>>> > >>> > >>> Just because there are two ways to do something does not mean that one of > >>> them is redundant, as both may be more suitable for different situations. > >> > >> I agree. > >> > >>> When enqueuing a set of jobs to the device, having a separate submit > >>> outside a loop makes for clearer code than having a check for the last > >>> iteration inside the loop to set a special submit flag. However, for > >>> cases > >>> where one item alone is to be submitted or there is a small set of jobs to > >>> be submitted sequentially, having a submit flag provides a lower-overhead > >>> way of doing the submission while still keeping the code clean. > >> > >> This kind of explanation may be missing in doxygen? > >> > > > > It can be added, sure. > > > >>>>> +bool > >>>>> +rte_dmadev_is_valid_dev(uint16_t dev_id); > >>>> > >>>> I would suggest dropping the final "_dev" in the function name. > >>>> > >>> > >>> The alternative, which I would support, is replacing "rte_dmadev" with > >>> "rte_dma" across the API. This would then become "rte_dma_is_valid_dev" > >>> which is clearer, since the dev is not part of the standard prefix. It > >>> also > >>> would fit in with a possible future function of "rte_dma_is_valid_vchan" > >>> for instance. > >> > >> Yes > >> The question is whether it would make sense to reserver rte_dma_ prefix > >> for some DMA functions which would be outside of dmadev lib? > >> If you think that all DMA functions will be in dmadev, > >> then yes we can shorten the prefix to rte_dma_. > >> > > > > Well, any DPDK dma functions which are not in dma library should have the > > prefix of the library they are in e.g. rte_eal_dma_*, rte_pci_dma_* > > Therefore, I don't think name conflicts should be an issue, and I like > > having less typing to do in function names (and I believe Morten was > > strongly proposing this previously too) > > The dmadev is rather short, if change I prefer all public API with rte_dma_ > prefix, > and don't have rte_dma_dev_ prefix for such start/stop/close. (ps: the > rte_eth_ also > have rte_eth_dev_close which is painful for OCD).
I agree that having rte_dma_dev_* is unpleasant naming for those functions, so if we use rte_dma_ as prefix, any dev should be at the end instead: i.e. rte_dma_stop_dev, rte_dma_start_dev, rte_dma_close_dev, etc. > > Also should the filename(e.g. rte_dmadev.h) and directory-name(lib/dmadev) > also change ? > I would keep those names intact. /Bruce
