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 29C76A0C49; Tue, 20 Jul 2021 11:43:55 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1664E40689; Tue, 20 Jul 2021 11:43:55 +0200 (CEST) Received: from mail-io1-f53.google.com (mail-io1-f53.google.com [209.85.166.53]) by mails.dpdk.org (Postfix) with ESMTP id 7522240140 for ; Tue, 20 Jul 2021 11:43:54 +0200 (CEST) Received: by mail-io1-f53.google.com with SMTP id v26so23268181iom.11 for ; Tue, 20 Jul 2021 02:43:54 -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=j853bD3SyDY3Y4jySatFrPiO+vxzA9qlyl3b906F0NU=; b=N35J4KsTWzzKcfpyozUn02vuJQly1XJhNI17Ybpg/R7Nj+b6K7dsiPJ1nkdna6g4EP +mYk2vDyR8xS4w0iBE7xqpKliTUpFw5d4smWkPXkfPsXytx8HwAZVaJNq5Sv5Z2lt4TG empIQdvYMiBA5zwLXEIMUDilliqTtNnTcgbQ/A9Ff2OK1+IZFlrIbHCtl6pAsu80SzBU sVDSQ31Qv/I8Oem59r4IpeIWEwL5CVJOaTJY820skjIEpZzJDOu1ZjatQ2gAK36G9KvI PiuvxXmjgaRJKvfSfUFwnejZrRnnzOiHuV6Jo75ONe+cLfmNxdb/BDfWvZZj9BApWgMT TXqQ== 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=j853bD3SyDY3Y4jySatFrPiO+vxzA9qlyl3b906F0NU=; b=HaIde+MXNMw3Tjriwk/z6tg+5Xwnal6w2tnT6/hEh86neyLuw/bTHzxARmIF9NE6PA jhs7Yt7N3OxMOmYWZekIVt75p69x0LxAhPoUqzwfhum256ganDCp01yn4lpaBWSRU/G4 bwdfpuZMZ4bjL2PFHM4rFaFbTn5sVN5cD/kfY5jrNJknkZorHUQErZ3cokGPHRe+RG++ /S4sB7HK2BqY0MKkCwcsBQHj5EMZhLO1K5aVfCMqJPCXCHxSRxhTe3ky9LvXuX77Dpv2 xz/udAZZ/FNMoj20JlamW1pWhG4iNJHNFG1NN5QrkcZ1pG/dRkEpGeWaF7Tw5B8PEeVj l4wg== X-Gm-Message-State: AOAM531G+ECvBBAlTZNVkgOiBhbJou+tF9/jkQPi7KpI4A2ZWQfPdoUS 4JWWcHiPUDcz1i/0fvMoUw5jyodGUHLani+pOw4= X-Google-Smtp-Source: ABdhPJyxeqUUz35ubXnGGdOiDazKdnSObG3KGD22rwlz50Fbqs1nsDDXn+jl9LAjchNfc6rm01g6TA41QcA1Q46Dh+g= X-Received: by 2002:a05:6638:c9:: with SMTP id w9mr24462827jao.133.1626774233858; Tue, 20 Jul 2021 02:43:53 -0700 (PDT) MIME-Version: 1.0 References: <1625231891-2963-1-git-send-email-fengchengwen@huawei.com> <1626743685-43734-1-git-send-email-fengchengwen@huawei.com> In-Reply-To: From: Jerin Jacob Date: Tue, 20 Jul 2021 15:13:26 +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 v8] 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 Tue, Jul 20, 2021 at 12:23 PM fengchengwen wrote: > > Thanks Jerin, comment inline > > On 2021/7/20 13:03, Jerin Jacob wrote: > > On Tue, Jul 20, 2021 at 6:48 AM 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 > > [snip] > > >> +int > >> +rte_dmadev_info_get(uint16_t dev_id, struct rte_dmadev_info *dev_info) > >> +{ > >> + const struct rte_dmadev *dev = &rte_dmadevices[dev_id]; > >> + int ret; > >> + > >> + RTE_DMADEV_VALID_DEV_ID_OR_ERR_RET(dev_id, -EINVAL); > >> + if (dev_info == NULL) > >> + return -EINVAL; > >> + > >> + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_info_get, -ENOTSUP); > >> + memset(dev_info, 0, sizeof(struct rte_dmadev_info)); > >> + ret = (*dev->dev_ops->dev_info_get)(dev, dev_info, > >> + sizeof(struct rte_dmadev_info)); > >> + if (ret != 0) > >> + return ret; > >> + > >> + dev_info->device = dev->device; > >> + dev_info->nb_vchans = dev->data->dev_conf.max_vchans; > > > > This will be updated after configure stage. > > Yes, the dev_info->nb_vchans hold the number of virtual DMA channel configured. > Do you mean add one comment here ? If are taking care of the case where rte_dmadev_info_get() called first and then configure() then fine. > > > > > > >> + > >> + return 0; > >> +} > >> + > >> +int > >> +rte_dmadev_configure(uint16_t dev_id, const struct rte_dmadev_conf *dev_conf) > >> +{ > >> + struct rte_dmadev *dev = &rte_dmadevices[dev_id]; > >> + struct rte_dmadev_info info; > >> + int ret; > >> + > >> + RTE_DMADEV_VALID_DEV_ID_OR_ERR_RET(dev_id, -EINVAL); > >> + if (dev_conf == NULL) > >> + return -EINVAL; > >> + > >> + ret = rte_dmadev_info_get(dev_id, &info); > >> + if (ret != 0) { > >> + RTE_DMADEV_LOG(ERR, "Device %u get device info fail\n", dev_id); > >> + return -EINVAL; > >> + } > >> + if (dev_conf->max_vchans == 0) { > >> + RTE_DMADEV_LOG(ERR, > >> + "Device %u configure zero vchans\n", dev_id); > >> + return -EINVAL; > >> + } > >> + if (dev_conf->max_vchans > info.max_vchans) { > >> + RTE_DMADEV_LOG(ERR, > >> + "Device %u configure too many vchans\n", dev_id); > >> + return -EINVAL; > >> + } > >> + if (dev_conf->enable_silent && > >> + !(info.dev_capa & RTE_DMADEV_CAPA_SILENT)) { > >> + RTE_DMADEV_LOG(ERR, "Device %u don't support silent\n", dev_id); > >> + return -EINVAL; > >> + } > >> + > >> + if (dev->data->dev_started != 0) { > >> + RTE_DMADEV_LOG(ERR, > >> + "Device %u must be stopped to allow configuration\n", > >> + dev_id); > >> + return -EBUSY; > >> + } > > > > ethdev and other device class common code handles the reconfigure case. i.e > > the application configures N vchan first and reconfigures to N - M > > then free the resources > > attached to M - N. Please do the same here. > > DMA is a simple device, I think it's OK to reconfigure at driver-level. OK. If everyone thinks that way it is OK. No strong opinion. > > PS: If we need support reconfigure at dmadev-level, dmadev should hold the vchan-configuration, > and invoke driver's vchan_release to release resources. This may introduce more complexity. > > > >> +int > >> +rte_dmadev_dump(uint16_t dev_id, FILE *f) > >> +{ > >> + const struct rte_dmadev *dev = &rte_dmadevices[dev_id]; > >> + struct rte_dmadev_info info; > >> + int ret; > >> + > >> + RTE_DMADEV_VALID_DEV_ID_OR_ERR_RET(dev_id, -EINVAL); > >> + if (f == NULL) > >> + return -EINVAL; > >> + > >> + ret = rte_dmadev_info_get(dev_id, &info); > >> + if (ret != 0) { > >> + RTE_DMADEV_LOG(ERR, "Device %u get device info fail\n", dev_id); > >> + return -EINVAL; > >> + } > >> + > >> + fprintf(f, "DMA Dev %u, '%s' [%s]\n", > >> + dev->data->dev_id, > >> + dev->data->dev_name, > >> + dev->data->dev_started ? "started" : "stopped"); > >> + fprintf(f, " dev_capa: 0x%" PRIx64 "\n", info.dev_capa); > >> + fprintf(f, " max_vchans_supported: %u\n", info.max_vchans); > >> + fprintf(f, " max_vchans_configured: %u\n", info.nb_vchans); > >> + fprintf(f, " silent_mode: %s\n", > >> + dev->data->dev_conf.enable_silent ? "on" : "off"); > > > > Probably iterate over each vchan and dumping the at least direction > > will be usefull. > > dmadev hasn't hold vchan-configuration, Need more discussion. Prefer to have that. Leaving to others. >