From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 688F1A034F; Fri, 8 Oct 2021 09:55:20 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E8A3340140; Fri, 8 Oct 2021 09:55:19 +0200 (CEST) Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by mails.dpdk.org (Postfix) with ESMTP id 1D60B40040 for ; Fri, 8 Oct 2021 09:55:18 +0200 (CEST) Received: from dggemv704-chm.china.huawei.com (unknown [172.30.72.54]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4HQgNW6K3wzbcw6; Fri, 8 Oct 2021 15:50:51 +0800 (CST) Received: from dggpeml500024.china.huawei.com (7.185.36.10) by dggemv704-chm.china.huawei.com (10.3.19.47) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2308.8; Fri, 8 Oct 2021 15:55:16 +0800 Received: from [127.0.0.1] (10.67.100.224) by dggpeml500024.china.huawei.com (7.185.36.10) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2308.8; Fri, 8 Oct 2021 15:55:15 +0800 To: Thomas Monjalon CC: , , , , , , , , , , , , , , , , References: <1625231891-2963-1-git-send-email-fengchengwen@huawei.com> <20210924105357.15386-1-fengchengwen@huawei.com> <20210924105357.15386-3-fengchengwen@huawei.com> <11425429.3DvE4F450j@thomas> From: fengchengwen Message-ID: Date: Fri, 8 Oct 2021 15:55:15 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 MIME-Version: 1.0 In-Reply-To: <11425429.3DvE4F450j@thomas> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [10.67.100.224] X-ClientProxiedBy: dggems702-chm.china.huawei.com (10.3.19.179) To dggpeml500024.china.huawei.com (7.185.36.10) X-CFilter-Loop: Reflected Subject: Re: [dpdk-dev] [PATCH v23 2/6] dmadev: add control plane function support X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 2021/10/6 18:46, Thomas Monjalon wrote: > 24/09/2021 12:53, Chengwen Feng: >> --- a/doc/guides/prog_guide/dmadev.rst >> +++ b/doc/guides/prog_guide/dmadev.rst >> @@ -62,3 +62,44 @@ identifiers: >> >> - A device name used to designate the DMA device in console messages, for >> administration or debugging purposes. [snip] > >> --- a/lib/dmadev/rte_dmadev.c >> +++ b/lib/dmadev/rte_dmadev.c >> @@ -218,6 +218,9 @@ rte_dma_pmd_release(const char *name) >> if (dev == NULL) >> return -EINVAL; >> >> + if (dev->state == RTE_DMA_DEV_READY) >> + return rte_dma_close(dev->dev_id); > > What is the logic here? > The only exposed function should be rte_dma_close() > and it should call the freeing function. > The API should use the dev_id. As you said somewhere else, > the name is only for debugging. > Please remove the function rte_dma_pmd_release(const char *name). The rte_dma_pmd_release corresponding to pmd_allocate, so both use the 'const char *name' parameter. The rte_dma_pmd_release is also used for error handling when PMD init. Therefore, a status variable is used here. If the device is not ready, only resources need to be released. Otherwise, the close interface of the driver is invoked. For PMD, the rte_dma_pmd_release is only wrap for dev_close when remove device, it does not need to implement two callbacks. If we replace rte_dma_pmd_release with rte_dma_close, then we should invoke rte_dma_close in error handling when PMD init, this can lead to conceptual inconsistencies because the initialization has not been successful. So I think it's better keep rte_dma_pmd_release. > > [...] >> --- a/lib/dmadev/rte_dmadev.h >> +++ b/lib/dmadev/rte_dmadev.h >> + * The functions exported by the dmadev API to setup a device designated by its >> + * device identifier must be invoked in the following order: >> + * - rte_dma_configure() >> + * - rte_dma_vchan_setup() >> + * - rte_dma_start() >> + * >> + * If the application wants to change the configuration (i.e. invoke >> + * rte_dma_configure() or rte_dma_vchan_setup()), it must invoke >> + * rte_dma_stop() first to stop the device and then do the reconfiguration >> + * before invoking rte_dma_start() again. The dataplane functions should not >> + * be invoked when the device is stopped. >> + * >> + * Finally, an application can close a dmadev by invoking the rte_dma_close() >> + * function. > > Yes rte_dma_close, not rte_dma_pmd_release. > >> + * >> + * About MT-safe, all the functions of the dmadev API exported by a PMD are > > API is not exported by a PMD, but implemented. > >> + * lock-free functions which assume to not be invoked in parallel on different >> + * logical cores to work on the same target dmadev object. >> + * @note Different virtual DMA channels on the same dmadev *DO NOT* support >> + * parallel invocation because these virtual DMA channels share the same >> + * HW-DMA-channel. >> + * >> */ > > No need of final blank line in a comment. > >> +/** DMA device support memory-to-memory transfer. >> + * >> + * @see struct rte_dma_info::dev_capa >> + */ >> +#define RTE_DMA_CAPA_MEM_TO_MEM RTE_BIT64(0) >> +/** DMA device support memory-to-device transfer. >> + * >> + * @see struct rte_dma_info::dev_capa >> + */ >> +#define RTE_DMA_CAPA_MEM_TO_DEV RTE_BIT64(1) > > Same comment as in earlier version: please group the flags > in a doxygen group. Example of doxygen group: > https://patches.dpdk.org/project/dpdk/patch/20210830104232.598703-1-thomas@monjalon.net/ Tried, but found it didn't coexist well with multi-line comments. > > [...] > You are using uint64_t bitfields and anonymous union in below struct, > it may not compile if not using __extension__ from RTE_STD_C11. > >> +struct rte_dma_port_param { >> + /** The device access port type. >> + * >> + * @see enum rte_dma_port_type >> + */ >> + enum rte_dma_port_type port_type; >> + union { > [...] >> + struct { >> + uint64_t coreid : 4; /**< PCIe core id used. */ >> + uint64_t pfid : 8; /**< PF id used. */ >> + uint64_t vfen : 1; /**< VF enable bit. */ >> + uint64_t vfid : 16; /**< VF id used. */ >> + /** The pasid filed in TLP packet. */ >> + uint64_t pasid : 20; >> + /** The attributes filed in TLP packet. */ >> + uint64_t attr : 3; >> + /** The processing hint filed in TLP packet. */ >> + uint64_t ph : 2; >> + /** The steering tag filed in TLP packet. */ >> + uint64_t st : 16; >> + } pcie; >> + }; >> + uint64_t reserved[2]; /**< Reserved for future fields. */ >> +}; > >> --- a/lib/dmadev/rte_dmadev_core.h >> +++ b/lib/dmadev/rte_dmadev_core.h >> +/** @internal Used to get device information of a device. */ >> +typedef int (*rte_dma_info_get_t)(const struct rte_dma_dev *dev, >> + struct rte_dma_info *dev_info, >> + uint32_t info_sz); > > Please move all driver interfaces in a file dedicated to drivers. There are three head file: rte_dmadev.h, rte_dmadev_core.h, rte_dmadev_pmd.h And we build the following dependency: rte_dmadev.h ---> rte_dmadev_core.h // mainly because dataplane inline API. ^ | --------------------- | | Application rte_dmadev_pmd.h ^ | DMA PMD If move all driver interfaces to rte_dmadev_pmd.h from rte_dmadev_core.h, bidirectional dependency may exist, e.g. rte_dmadev.h ---> rte_dmadev_core.h ---> rte_dmadev_pmd.h ^ | --------------------- | | Application rte_dmadev_pmd.h ^ | DMA PMD So I think it's better keep it that way. > > [...] >> @@ -40,9 +96,13 @@ struct rte_dma_dev { >> int16_t dev_id; /**< Device [external] identifier. */ >> int16_t numa_node; /**< Local NUMA memory ID. -1 if unknown. */ >> void *dev_private; /**< PMD-specific private data. */ >> + /** Functions exported by PMD. */ > > s/exported/implemented/ > >> + const struct rte_dma_dev_ops *dev_ops; >> + struct rte_dma_conf dev_conf; /**< DMA device configuration. */ >> /** Device info which supplied during device initialization. */ >> struct rte_device *device; >> enum rte_dma_dev_state state; /**< Flag indicating the device state. */ >> + uint8_t dev_started : 1; /**< Device state: STARTED(1)/STOPPED(0). */ >> uint64_t reserved[2]; /**< Reserved for future fields. */ >> } __rte_cache_aligned; > > > > > . > Thanks