From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 31CDA23C for ; Sun, 6 May 2018 02:28:42 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 May 2018 17:28:41 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,367,1520924400"; d="scan'208";a="47038982" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by FMSMGA003.fm.intel.com with ESMTP; 05 May 2018 17:28:40 -0700 Received: from fmsmsx154.amr.corp.intel.com (10.18.116.70) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sat, 5 May 2018 17:28:40 -0700 Received: from cdsmsx151.ccr.corp.intel.com (172.17.4.38) by FMSMSX154.amr.corp.intel.com (10.18.116.70) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sat, 5 May 2018 17:28:40 -0700 Received: from cdsmsx104.ccr.corp.intel.com ([169.254.4.183]) by CDSMSX151.ccr.corp.intel.com ([169.254.3.38]) with mapi id 14.03.0319.002; Sun, 6 May 2018 08:28:37 +0800 From: "Zhang, Tianfei" To: Shreyansh Jain , "Xu, Rosen" , "dev@dpdk.org" CC: "Doherty, Declan" , "Richardson, Bruce" , "Yigit, Ferruh" , "Ananyev, Konstantin" , "Liu, Song" , "Wu, Hao" , "gaetan.rivet@6wind.com" , "Wu, Yanglong" Thread-Topic: [PATCH v7 3/5] iFPGA: Add Intel FPGA BUS Rawdev Driver Thread-Index: AQHT47GZ8yCs6dWYTUuyByDOF3NdzKQg9DmAgADmiXA= Date: Sun, 6 May 2018 00:28:37 +0000 Message-ID: References: <1521553556-62982-1-git-send-email-rosen.xu@intel.com> <1525443062-43231-1-git-send-email-rosen.xu@intel.com> <1525443062-43231-4-git-send-email-rosen.xu@intel.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNTg5NGU1NjAtOGUwZC00MjQzLTk1ZGItYTM3ZWUwMWU1ZWNhIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjIuNS4xOCIsIlRydXN0ZWRMYWJlbEhhc2giOiJVNkU1YUhHMTh2UEcrOUhqOEpmSmFFeXArcUg2T3JsXC93azFKUlZ2RWVKM0R3emkrTGhUR0JSMEo1MElkOHJMbyJ9 x-ctpclassification: CTP_NT x-originating-ip: [172.17.6.105] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v7 3/5] iFPGA: Add Intel FPGA BUS Rawdev Driver X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 06 May 2018 00:28:42 -0000 > -----Original Message----- > From: Shreyansh Jain [mailto:shreyansh.jain@nxp.com] > Sent: Sunday, May 6, 2018 2:43 AM > To: Xu, Rosen ; dev@dpdk.org > Cc: Doherty, Declan ; Richardson, Bruce > ; Yigit, Ferruh ; > Ananyev, Konstantin ; Zhang, Tianfei > ; Liu, Song ; Wu, Hao > ; gaetan.rivet@6wind.com; Wu, Yanglong > > Subject: RE: [PATCH v7 3/5] iFPGA: Add Intel FPGA BUS Rawdev Driver >=20 > Hello Rosen, >=20 > > -----Original Message----- > > From: Xu, Rosen [mailto:rosen.xu@intel.com] > > Sent: Friday, May 4, 2018 7:41 PM > > To: dev@dpdk.org > > Cc: rosen.xu@intel.com; declan.doherty@intel.com; > > bruce.richardson@intel.com; Shreyansh Jain ; > > ferruh.yigit@intel.com; konstantin.ananyev@intel.com; > > tianfei.zhang@intel.com; song.liu@intel.com; hao.wu@intel.com; > > gaetan.rivet@6wind.com; Yanglong Wu > > Subject: [PATCH v7 3/5] iFPGA: Add Intel FPGA BUS Rawdev Driver > > > > From: Rosen Xu > > > > Add Intel FPGA BUS Rawdev Driver which is based on librte_rawdev > > library. > > > > Signed-off-by: Rosen Xu > > Signed-off-by: Yanglong Wu > > Signed-off-by: Figo zhang > > --- > > config/common_base | 1 + > > drivers/raw/Makefile | 1 + > > drivers/raw/ifpga_rawdev/Makefile | 36 ++ > > drivers/raw/ifpga_rawdev/ifpga_rawdev.c | 599 > > +++++++++++++++++++++ > > drivers/raw/ifpga_rawdev/ifpga_rawdev.h | 37 ++ > > .../raw/ifpga_rawdev/rte_ifpga_rawdev_version.map | 4 + > > mk/rte.app.mk | 1 + > > 7 files changed, 679 insertions(+) > > create mode 100644 drivers/raw/ifpga_rawdev/Makefile create mode > > 100644 drivers/raw/ifpga_rawdev/ifpga_rawdev.c > > create mode 100644 drivers/raw/ifpga_rawdev/ifpga_rawdev.h > > create mode 100644 > > drivers/raw/ifpga_rawdev/rte_ifpga_rawdev_version.map > > >=20 > [...] >=20 > > --- /dev/null > > +++ b/drivers/raw/ifpga_rawdev/ifpga_rawdev.c > > @@ -0,0 +1,599 @@ > > +/* SPDX-License-Identifier: BSD-3-Clause > > + * Copyright(c) 2010-2018 Intel Corporation */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include "base/opae_hw_api.h" > > +#include "rte_rawdev.h" > > +#include "rte_rawdev_pmd.h" > > +#include "rte_bus_ifpga.h" > > +#include "ifpga_common.h" > > +#include "ifpga_logs.h" > > +#include "ifpga_rawdev.h" > > + > > +int ifpga_rawdev_logtype; > > + > > +#define PCI_VENDOR_ID_INTEL 0x8086 > > +/* PCI Device ID */ > > +#define PCIE_DEVICE_ID_PF_INT_5_X 0xBCBD > > +#define PCIE_DEVICE_ID_PF_INT_6_X 0xBCC0 > > +#define PCIE_DEVICE_ID_PF_DSC_1_X 0x09C4 > > +/* VF Device */ > > +#define PCIE_DEVICE_ID_VF_INT_5_X 0xBCBF > > +#define PCIE_DEVICE_ID_VF_INT_6_X 0xBCC1 > > +#define PCIE_DEVICE_ID_VF_DSC_1_X 0x09C5 > > +#define RTE_MAX_RAW_DEVICE 10 > > + > > +static const struct rte_pci_id pci_ifpga_map[] =3D { > > + { RTE_PCI_DEVICE(PCI_VENDOR_ID_INTEL, > PCIE_DEVICE_ID_PF_INT_5_X) > > }, > > + { RTE_PCI_DEVICE(PCI_VENDOR_ID_INTEL, > PCIE_DEVICE_ID_VF_INT_5_X) > > }, > > + { RTE_PCI_DEVICE(PCI_VENDOR_ID_INTEL, > PCIE_DEVICE_ID_PF_INT_6_X) > > }, > > + { RTE_PCI_DEVICE(PCI_VENDOR_ID_INTEL, > PCIE_DEVICE_ID_VF_INT_6_X) > > }, > > + { RTE_PCI_DEVICE(PCI_VENDOR_ID_INTEL, > PCIE_DEVICE_ID_PF_DSC_1_X) > > }, > > + { RTE_PCI_DEVICE(PCI_VENDOR_ID_INTEL, > PCIE_DEVICE_ID_VF_DSC_1_X) > > }, > > + { .vendor_id =3D 0, /* sentinel */ }, > > +}; > > + > > +static int ifpga_fill_afu_dev(struct opae_accelerator *acc, > > + struct rte_afu_device *afu_dev) >=20 > Sorry, for nitpicking, but the function definitions should follow [1] mod= el > where return types are different lines than name. >=20 > Something like: >=20 > static int > ifpga_fill_afu_dev(...) >=20 >=20 > [1] > http://dpdk.org/doc/guides/contributing/coding_style.html#c-function-defi > nition-declaration-and-use >=20 > This is valid for multiple function definitions. Can you please fix when = you > send v8 (which you might have to send for rebasing over master). >=20 > > +{ > > + struct rte_mem_resource *res =3D afu_dev->mem_resource; > > + struct opae_acc_region_info region_info; > > + struct opae_acc_info info; > > + unsigned long i; > > + int ret; > > + > > + ret =3D opae_acc_get_info(acc, &info); > > + if (ret) > > + return ret; > > + > > + if (info.num_regions > PCI_MAX_RESOURCE) > > + return -EFAULT; > > + > > + afu_dev->num_region =3D info.num_regions; > > + > > + for (i =3D 0; i < info.num_regions; i++) { > > + region_info.index =3D i; > > + ret =3D opae_acc_get_region_info(acc, ®ion_info); > > + if (ret) > > + return ret; > > + > > + if ((region_info.flags & ACC_REGION_MMIO) && > > + (region_info.flags & ACC_REGION_READ) && > > + (region_info.flags & ACC_REGION_WRITE)) { > > + res[i].phys_addr =3D region_info.phys_addr; > > + res[i].len =3D region_info.len; > > + res[i].addr =3D region_info.addr; > > + } else > > + return -EFAULT; > > + } > > + > > + return 0; > > +} > > + > > +static void ifpga_rawdev_info_get(struct rte_rawdev *dev, > > + rte_rawdev_obj_t dev_info) > > +{ > > + struct opae_adapter *adapter; > > + struct opae_accelerator *acc; > > + struct rte_afu_device *afu_dev; > > + > > + IFPGA_RAWDEV_PMD_FUNC_TRACE(); > > + >=20 > [...] >=20 > > + > > +static int > > +ifpga_rawdev_pci_probe(struct rte_pci_driver *pci_drv __rte_unused, > > + struct rte_pci_device *pci_dev) > > +{ > > + > > + IFPGA_RAWDEV_PMD_INFO("## %s\n", __func__); >=20 > Actually, this is not INFO - this is debug. And, this seems like place fo= r > FUNC_TRACE call you have already defined in your log file. >=20 > > + return ifpga_rawdev_create(pci_dev, rte_socket_id()); } > > + > > +static int ifpga_rawdev_pci_remove(struct rte_pci_device *pci_dev) { > > + return ifpga_rawdev_destroy(pci_dev); } > > + > > +static struct rte_pci_driver rte_ifpga_rawdev_pmd =3D { > > + .id_table =3D pci_ifpga_map, > > + .drv_flags =3D RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC, > > + .probe =3D ifpga_rawdev_pci_probe, > > + .remove =3D ifpga_rawdev_pci_remove, > > +}; > > + >=20 > [...] >=20 > > diff --git a/drivers/raw/ifpga_rawdev/ifpga_rawdev.h > > b/drivers/raw/ifpga_rawdev/ifpga_rawdev.h > > new file mode 100644 > > index 0000000..9a09561 > > --- /dev/null > > +++ b/drivers/raw/ifpga_rawdev/ifpga_rawdev.h > > @@ -0,0 +1,37 @@ > > +/* SPDX-License-Identifier: BSD-3-Clause > > + * Copyright(c) 2010-2018 Intel Corporation */ > > + > > +#ifndef _IFPGA_RAWDEV_H_ > > +#define _IFPGA_RAWDEV_H_ > > + > > +extern int ifpga_rawdev_logtype; > > + > > +#define IFPGA_RAWDEV_PMD_LOG(level, fmt, args...) \ > > + rte_log(RTE_LOG_ ## level, ifpga_rawdev_logtype, "ifgpa: " fmt > > "\n", \ > > + ##args) >=20 > Another one. > In many of your logs, you are adding '\n' in the end. Your PMD_LOG > definition also has a '\n' - that is double new lines being printed for t= hose > logs. >=20 > > + > > +#define IFPGA_RAWDEV_PMD_FUNC_TRACE() > IFPGA_RAWDEV_PMD_LOG(DEBUG, > > ">>") > > + > > +#define IFPGA_RAWDEV_PMD_DEBUG(fmt, args...) \ > > + IFPGA_RAWDEV_PMD_LOG(DEBUG, fmt, ## args) #define > > +IFPGA_RAWDEV_PMD_INFO(fmt, args...) \ > > + IFPGA_RAWDEV_PMD_LOG(INFO, fmt, ## args) #define > > +IFPGA_RAWDEV_PMD_ERR(fmt, args...) \ > > + IFPGA_RAWDEV_PMD_LOG(ERR, fmt, ## args) #define > > +IFPGA_RAWDEV_PMD_WARN(fmt, args...) \ > > + IFPGA_RAWDEV_PMD_LOG(WARNING, fmt, ## args) > > + > > +enum ifpga_rawdev_device_state { > > + IFPGA_IDLE, > > + IFPGA_READY, > > + IFPGA_ERROR > > +}; > > + > > +static inline struct opae_adapter * > > +ifpga_rawdev_get_priv(const struct rte_rawdev *rawdev) { > > + return rawdev->dev_private; > > +} > > + > > +#endif /* _IFPGA_RAWDEV_H_ */ >=20 > [...] >=20 > There are some nitpicks highlighted in mail above - but nothing major. I > would have no more comments (as well as review) for this. > In principle, I am OK with this. For v8, please feel free to use for this= patch: >=20 > Acked-by: Shreyansh Jain >=20 > Thanks for the work and patience. Thanks Shreyansh great support, we will send the v8 patches today.