From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
by inbox.dpdk.org (Postfix) with ESMTP id 694D6A0C49;
Tue, 20 Jul 2021 12:13:19 +0200 (CEST)
Received: from [217.70.189.124] (localhost [127.0.0.1])
by mails.dpdk.org (Postfix) with ESMTP id 0047C40689;
Tue, 20 Jul 2021 12:13:18 +0200 (CEST)
Received: from mga02.intel.com (mga02.intel.com [134.134.136.20])
by mails.dpdk.org (Postfix) with ESMTP id 97E1540140
for <dev@dpdk.org>; Tue, 20 Jul 2021 12:13:17 +0200 (CEST)
X-IronPort-AV: E=McAfee;i="6200,9189,10050"; a="198412077"
X-IronPort-AV: E=Sophos;i="5.84,254,1620716400"; d="scan'208";a="198412077"
Received: from orsmga008.jf.intel.com ([10.7.209.65])
by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;
20 Jul 2021 03:13:15 -0700
X-IronPort-AV: E=Sophos;i="5.84,254,1620716400"; d="scan'208";a="461972665"
Received: from bricha3-mobl.ger.corp.intel.com ([10.252.28.140])
by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA;
20 Jul 2021 03:13:11 -0700
Date: Tue, 20 Jul 2021 11:13:07 +0100
From: Bruce Richardson <bruce.richardson@intel.com>
To: fengchengwen <fengchengwen@huawei.com>
Cc: Jerin Jacob <jerinjacobk@gmail.com>, Thomas Monjalon <thomas@monjalon.net>,
Ferruh Yigit <ferruh.yigit@intel.com>, Jerin Jacob <jerinj@marvell.com>,
Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>, dpdk-dev <dev@dpdk.org>,
Morten =?iso-8859-1?Q?Br=F8rup?= <mb@smartsharesystems.com>,
Nipun Gupta <nipun.gupta@nxp.com>, Hemant Agrawal <hemant.agrawal@nxp.com>,
Maxime Coquelin <maxime.coquelin@redhat.com>,
Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>,
David Marchand <david.marchand@redhat.com>,
Satananda Burla <sburla@marvell.com>, Prasun Kapoor <pkapoor@marvell.com>,
"Ananyev, Konstantin" <konstantin.ananyev@intel.com>
Message-ID: <YPahs43M0YhK2zo1@bricha3-MOBL.ger.corp.intel.com>
References: <1625231891-2963-1-git-send-email-fengchengwen@huawei.com>
<1626743685-43734-1-git-send-email-fengchengwen@huawei.com>
<CALBAE1Pgo151aO-2wOgyyyPE6DD0ocarDdoEC6mfFwMRJ3dEzQ@mail.gmail.com>
<dc9fcd96-1923-012d-469d-12cb5ac7ec1c@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <dc9fcd96-1923-012d-469d-12cb5ac7ec1c@huawei.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
On Tue, Jul 20, 2021 at 02:53:08PM +0800, 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 <fengchengwen@huawei.com> 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 <fengchengwen@huawei.com>
>
> [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 ?
>
> >
> >
> >> +
> >> + 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.
>
> 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.
>
I would tend to agree to keep this as it is.
>
> >> +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.
>
I think this is fine as-is. The use of vchans doesn't apply to all devices
- unlike ethdev queues, for example - so I think having the driver manage
them is good.
/Bruce