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 1B9B3A0C50; Fri, 16 Jul 2021 14:54:38 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B84EF4067B; Fri, 16 Jul 2021 14:54:37 +0200 (CEST) Received: from mail-io1-f43.google.com (mail-io1-f43.google.com [209.85.166.43]) by mails.dpdk.org (Postfix) with ESMTP id D6DB040151 for ; Fri, 16 Jul 2021 14:54:35 +0200 (CEST) Received: by mail-io1-f43.google.com with SMTP id d9so10427338ioo.2 for ; Fri, 16 Jul 2021 05:54:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=AECTIjvTKAQVh3ksOMLIAVY6DNpzElKCoNlAVkHGw8g=; b=tm+j/6h+r1Q08MNFsfPy0rAFQny/wbLRMFNWHNIku5HKuPKA4ILri7vyf70LtUqLXD 65IyxVOWKuUBoCygPUd/T+Mj77fmK9Vk0qGmx/sbboSwKWXYe9jrT8SWp4zjOubpLl6R iDyVLQ5ql4HqH9tTHc8RD/ZIqF7AzzGhfo9t88jFIcsQ3uQbHIYvSsu3YEj/8UIkA2YU ouvE2R/a/AekDC66ZYjJwQ8UmsdO28scG8bj5cNh9fgmKv86lDg9vPqTf+Z5ilxqb8sl T2YSTlJFIZBGSpaOKZSGr3mOURAD2vIMEMGejntczmHL4hbyl5sherkjtUp3d32ddUU/ 25SQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=AECTIjvTKAQVh3ksOMLIAVY6DNpzElKCoNlAVkHGw8g=; b=LiYAFy3Er+4Aq/LpYS5uPOPgnDq8o94vbZOxqLFfqQWEb5b0hY/SPDfgim2PvNH60P 0wzUkcXa7jzyXr83Tx11F4uI/z6hLqgGD6lN8Im6kLG/lLkAwrmmooUshdCx81WA/jpf LkufZwXs0fL0uBs/V34xETDgo5U8Sk0aja5JfoBrwt4Kiqo07u9r3fidPL4MilSgh0/6 qZZ+ktf7l9Z3ce2a5XPPThELwdRL+Su+9u5u5GVqAcXJ2Esz1QrGRTwWb4YXQZw1pINy qYdb2CpGIF0Pyf43wo5TGiiKx17opyBP/N8Fo2YV+pH09tXrtl83EQnlNBTnN2coJfxt eI9A== X-Gm-Message-State: AOAM530ddsQH4k0GPBODjJm9TYpEtRytemA9AhGynxzp0y78IfvdZgJ4 EjMoCK+E8L5GBY7pOGNfU04WqHyI48UDgxO8DPc= X-Google-Smtp-Source: ABdhPJxo840+sHVkq4uQ1oN1A7iCrIBFtDOBBtUTQiusHiOgfROwGb2aGphgdho0/eQRRWkxQTXNq7yI6iVnjAOjqSo= X-Received: by 2002:a05:6638:c9:: with SMTP id w9mr7942779jao.133.1626440075133; Fri, 16 Jul 2021 05:54:35 -0700 (PDT) MIME-Version: 1.0 References: <1625231891-2963-1-git-send-email-fengchengwen@huawei.com> <1626363661-51103-1-git-send-email-fengchengwen@huawei.com> In-Reply-To: From: Jerin Jacob Date: Fri, 16 Jul 2021 18:24:08 +0530 Message-ID: To: fengchengwen Cc: Thomas Monjalon , Ferruh Yigit , "Richardson, Bruce" , Jerin Jacob , Andrew Rybchenko , dpdk-dev , =?UTF-8?Q?Morten_Br=C3=B8rup?= , Nipun Gupta , Hemant Agrawal , Maxime Coquelin , Honnappa Nagarahalli , David Marchand , Satananda Burla , Prasun Kapoor , "Ananyev, Konstantin" Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH v4] 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" On Thu, Jul 15, 2021 at 9:34 PM fengchengwen wrote: > > @burce, jerin Some unmodified review comments are returned here: > > 1. > COMMENT: > + memset(dmadev_shared_data->data, 0, > > + sizeof(dmadev_shared_data->data)); > I believe all memzones are zero on allocation anyway, so this memset is > unecessary and can be dropped. > > REPLY: I didn't find a comment to clear with this function. > > 2. > COMMENT: > + * @see struct rte_dmadev_info::dev_capa > > + */ > Drop this flag as unnecessary. All devices either always provide ordering > guarantee - in which case it's a no-op - or else support the flag. > > REPLY: I prefer define it, it could let user know whether support fence. > > 3. > COMMENT: I would suggest v4 to split the patch like (so that we can review and > ack each patch) > 1) Only public header file with Doxygen inclusion, (There is a lot of > Doxygen syntax issue in the patch) > 2) 1 or more patches for implementation. > > REPLY: the V4 still one patch and with doxyen files, It's better now for just > one patch of header file and implementation. > Later I will push doxyen file patch and skelton file patch. Makes sense. Whichever version you would like to have Ack, please split in that version. > > 4. > COMMENT: > + * @see RTE_DMA_DEV_TO_MEM > > + * @see RTE_DMA_DEV_TO_DEV > Since we can set of only one direction per vchan . Should be we make > it as enum to > make it clear. > > REPLY: May some devices support it. I think it's OK for future use. Sent comment in another thread. > > 5. > COMMENT: > +__rte_experimental > > +int > > +rte_dmadev_vchan_release(uint16_t dev_id, uint16_t vchan); > I would like to remove this to align with other device class in DPDK and use > configure and start again if there change in vchannel setup/ > > REPLY: I think we could have dynamic reconfig vchan without stop device ability. I think, In general, drivers are doing a lot of stuff in _start(), this will give enough flexibility to driver(like getting a complete view of HW resources at start()). IMO, there is no need to deviate from other classes of DPDK devices here. Also, the release is a slow path operation, so adding more calls for reconfiguring is OK like other DPDK device classes. > > 6. > COMMENT: > + > > +#define RTE_DMADEV_ALL_VCHAN 0xFFFFu > RTE_DMADEV_VCHAN_ALL ?? > > REPLY: I don't like reserved a fix length queue stats in 'struct rte_eth_stats', > which may counter the RTE_ETHDEV_QUEUE_STAT_CNTRS too short problem. > So here I define the API could get one or ALL vchans stats. I meant subsystem name can come as the third item like RTE_DMADEV_VCHAN_ALL vs RTE_DMADEV_ALL_VCHAN where ALL is not a subsystem i.e RTE_DMADEV__.._ACTION/VERB/etc > > 7. > COMMENT: > +rte_dmadev_copy_sg(uint16_t dev_id, uint16_t vchan, const struct rte_dma_sg *sg, > > + uint64_t flags) > In order to avoid population of rte_dma_sg in stack (as it is > fastpath), I would like > to change the API as > rte_dmadev_copy_sg(uint16_t dev_id, uint16_t vchan, struct rte_dma_sge > *src, struct rte_dma_sge *dst, uint16_t nb_src, uint16_t nb_dst, > uint64_t flags) > > REPLY: Too many (7) parameters if it separates. I prefer define a struct wrap it. I agree if it is a slow path function. However, I strongly prefer to not have one more stack indirection in fastpath and also we will not be adding new arguments to this function in the future. So that way, it is better as rte_dmadev_copy_sg(uint16_t dev_id, uint16_t vchan, struct rte_dma_sge src, struct rte_dma_sge *dst, uint16_t nb_src, uint16_t nb_dst, uint64_t flags) > > 8. > COMMENT: change RTE_LOG_REGISTER(rte_dmadev_logtype, lib.dmadev, INFO); to > RTE_LOG_REGISTER_DEFAULT, and change rte_dmadev_logtype to logtype. > > REPLY: Our CI version still don't support RTE_LOG_REGISTER_DEFAULT (have not sync newest version), > I think it could fix in next version or patch. > and because RTE_LOG_REGISTER define the rte_dmadev_logtype as a un-static variable, if we > change to logtype, there maybe namespace conflict, so I think it's ok to retain the original > variables. > > thanks. > > [snip] > > On 2021/7/15 23:41, 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 > > --- > > v4: > > * replace xxx_complete_fails with xxx_completed_status. > > * add SILENT capability, also a silent_mode in rte_dmadev_conf. > > * add op_flag_llc for performance. > > * rename dmadev_xxx_t to rte_dmadev_xxx_t to avoid namespace conflict. > > * delete filed 'enqueued_count' from rte_dmadev_stats. > > * make rte_dmadev hold 'dev_private' filed. > > * add RTE_DMA_STATUS_NOT_ATTEMPED status code. > > * rename RTE_DMA_STATUS_ACTIVE_DROP to RTE_DMA_STATUS_USER_ABORT. > > * rename rte_dma_sg(e) to rte_dmadev_sg(e) to make sure all struct > > prefix with rte_dmadev. > > * put the comment afterwards. > > * fix some doxgen problem. > > * delete macro RTE_DMADEV_VALID_DEV_ID_OR_RET and > > RTE_DMADEV_PTR_OR_ERR_RET. > > * replace strlcpy with rte_strscpy. > > * other minor modifications from review comment. > > v3: > > [snip]