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 26A41A0C51; Tue, 13 Jul 2021 15:06:50 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 939E0410DF; Tue, 13 Jul 2021 15:06:49 +0200 (CEST) Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by mails.dpdk.org (Postfix) with ESMTP id 31BB2406FF for ; Tue, 13 Jul 2021 15:06:47 +0200 (CEST) Received: from dggemv703-chm.china.huawei.com (unknown [172.30.72.55]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4GPLNb2MHhzXqjw; Tue, 13 Jul 2021 21:01:03 +0800 (CST) Received: from dggpeml500024.china.huawei.com (7.185.36.10) by dggemv703-chm.china.huawei.com (10.3.19.46) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2176.2; Tue, 13 Jul 2021 21:06:40 +0800 Received: from [10.40.190.165] (10.40.190.165) 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.2176.2; Tue, 13 Jul 2021 21:06:39 +0800 From: fengchengwen To: , , , , , CC: , , , , , , , , , References: <1625231891-2963-1-git-send-email-fengchengwen@huawei.com> <1626179263-14645-1-git-send-email-fengchengwen@huawei.com> Message-ID: <02340ac8-9d09-15e8-3969-1850f08e5831@huawei.com> Date: Tue, 13 Jul 2021 21:06:39 +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: <1626179263-14645-1-git-send-email-fengchengwen@huawei.com> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit X-Originating-IP: [10.40.190.165] X-ClientProxiedBy: dggems701-chm.china.huawei.com (10.3.19.178) To dggpeml500024.china.huawei.com (7.185.36.10) X-CFilter-Loop: Reflected Subject: Re: [dpdk-dev] [PATCH v3] dmadev: introduce DMA device library 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" Thank you for your valuable comments, and I think we've taken a big step forward. @andrew Could you provide the copyright line so that I can add it to relevant file. @burce, jerin Some unmodified review comments are returned here: 1. COMMENT: We allow up to 100 characters per line for DPDK code, so these don't need to be wrapped so aggressively. REPLY: Our CI still has 80 characters limit, and I review most framework still comply. 2. COMMENT: > +#define RTE_DMA_MEM_TO_MEM (1ull << 0) RTE_DMA_DIRECTION_... REPLY: add the 'DIRECTION' may the macro too long, I prefer keep it simple. 3. COMMENT: > +rte_dmadev_vchan_release(uint16_t dev_id, uint16_t vchan); We are not making release as pubic API in other device class. See ethdev spec. bbdev/eventdev/rawdev REPLY: because ethdev's queue is hard-queue, and here is the software defined channels, I think release is OK, BTW: bbdev/eventdev also have release ops. 4. COMMENT:> + uint64_t reserved[4]; /**< Reserved for future fields */ > +}; Please add the capability for each counter in info structure as one device may support all the counters. REPLY: This is a statistics function. If this function is not supported, then do not need to implement the stats ops function. Also could to set the unimplemented ones to zero. 5. COMMENT: > +#endif > + return (*dev->fill)(dev, vchan, pattern, dst, length, flags); Instead of every driver set the NOP function, In the common code, If the CAPA is not set, common code can set NOP function for this with <0 return value. REPLY: I don't think it's a good idea to judge in IO path, it's application duty to ensure don't call API which driver not supported (which could get from capabilities). 6. COMMENT: > +rte_dmadev_completed_fails(uint16_t dev_id, uint16_t vchan, > + const uint16_t nb_status, uint32_t *status, uint32_t -> enum rte_dma_status_code REPLY：I'm still evaluating this. It takes a long time for the driver to perform error code conversion in this API. Do we need to provide an error code conversion function alone ? 7. COMMENT: > +typedef int (*dmadev_info_get_t)(struct rte_dmadev *dev, > + struct rte_dmadev_info *dev_info); Please change to rte_dmadev_info_get_t to avoid conflict due to namespace issue as this header is exported. REPLY: I prefer not add 'rte_' prefix, it make the define too long. 8. COMMENT: > + * - rte_dmadev_completed_fails() > + * - return the number of operation requests failed to complete. Please rename this to "completed_status" to allow the return of information other than just errors. As I suggested before, I think this should also be usable as a slower version of "completed" even in the case where there are no errors, in that it returns status information for each and every job rather than just returning as soon as it hits a failure. REPLY: well, I think it maybe confuse (current OK/FAIL API is easy to understand.), and we can build the slow path function on the two API. 9. COMMENT: > +#define RTE_DMA_DEV_CAPA_MEM_TO_MEM (1ull << 0) > +/**< DMA device support mem-to-mem transfer. Do we need this? Can we assume that any device appearing as a dmadev can do mem-to-mem copies, and drop the capability for mem-to-mem and the capability for copying? also for RTE_DMA_DEV_CAPA_OPS_COPY REPLY: yes, I insist on adding this for the sake of conceptual integrity. For ioat driver just make a statement. 10. COMMENT: > + uint16_t nb_vchans; /**< Number of virtual DMA channel configured */ > +}; Let's add rte_dmadev_conf struct into this to return the configuration settings. REPLY: If we add rte_dmadev_conf in, it may break ABI when rte_dmadev_conf add fields. [snip] On 2021/7/13 20:27, Chengwen Feng wrote: > This patch introduce 'dmadevice' which is a generic type of DMA > device. > > The APIs of dmadev library exposes some generic operations which can > enable configuration and I/O with the DMA devices. > > Signed-off-by: Chengwen Feng > --- > v3: > * rm reset and fill_sg ops. > * rm MT-safe capabilities. > * add submit flag. > * redefine rte_dma_sg to implement asymmetric copy. > * delete some reserved field for future use. > * rearrangement rte_dmadev/rte_dmadev_data struct. > * refresh rte_dmadev.h copyright. > * update vchan setup parameter. > * modified some inappropriate descriptions. > * arrange version.map alphabetically. > * other minor modifications from review comment. > --- > MAINTAINERS | 4 + > config/rte_config.h | 3 + > lib/dmadev/meson.build | 7 + > lib/dmadev/rte_dmadev.c | 561 +++++++++++++++++++++++++ > lib/dmadev/rte_dmadev.h | 968 +++++++++++++++++++++++++++++++++++++++++++ > lib/dmadev/rte_dmadev_core.h | 161 +++++++ > lib/dmadev/rte_dmadev_pmd.h | 72 ++++ > lib/dmadev/version.map | 37 ++ > lib/meson.build | 1 +