Hi Vipin,
> -----Original Message----- > From: Varghese, Vipin > Sent: Thursday, December 13, 2018 2:02 PM > To: Lu, Wenzhuo <wenzhuo...@intel.com>; dev@dpdk.org > Cc: Lu, Wenzhuo <wenzhuo...@intel.com> > Subject: RE: [dpdk-dev] [PATCH v3 00/34] A new net PMD - ice > > Hi, > > > -----Original Message----- > > From: dev <dev-boun...@dpdk.org> On Behalf Of Wenzhuo Lu > > Sent: Wednesday, December 12, 2018 12:30 PM > > To: dev@dpdk.org > > Cc: Lu, Wenzhuo <wenzhuo...@intel.com> > > Subject: [dpdk-dev] [PATCH v3 00/34] A new net PMD - ice > > > > This patch set adds the support of a new net PMD, Intel® Ethernet > > Network Adapters E810, also called ice. > > > > Besides enabling this new NIC, also some other features supported on this > NIC. > > Can you mention the other features Sorry for misleading you. The other features is below features. I'll reword it. > > > Like below, > > > > Basic features: > > 1, Basic device operations: probe, initialization, start/stop, configure, > > info > get. > > 2, RX/TX queue operations: setup/release, start/stop, info get. > > 3, RX/TX. > > > > HW Offload features: > > 1, CRC Stripping/insertion. > > 2, L2/L3 checksum strip/insertion. > > 3, PVID set. > > 4, TPID change. > > 5, TSO (LRO/RSC not supported). > > > > Stats: > > 1, statics & xstatics. > > > > Switch functions: > > 1, MAC Filter Add/Delete. > > 2, VLAN Filter Add/Delete. > > > > Power saving: > > 1, RX interrupt mode. > > > > Misc: > > 1, Interrupt For Link Status. > > 2, firmware info query. > > 3, Jumbo Frame Support. > > 4, ptype check. > > 5, EEPROM check and set. > > > > Can you add section to highlight the changes with "---". This is part of > 'http://doc.dpdk.org/guides/contributing/patches.html' for 'This can be > added to the cover letter or the annotations' Will add it. > > > v2: > > - Fix shared lib compile issue. > > - Add meson build support. > > - Update documents. > > - Fix more checkpatch issues. > > > > v3: > > - Removed the support of secondary process. > > - Splitted the base code to more patches. > > - Pass NULL to rte_zmalloc. > > - Changed some magic numbers to macros. > > - Fixed the wrong implementation of a specific bitmap. > > Not all comments are addressed or closed from V2. So I have to assume you > will be doing the same for v4. > > > Some of the items > > [PATCH v2 03/20] net/ice: support device and queue ops > > > > > + > > > > > +static int > > > > > +ice_dev_start(struct rte_eth_dev *dev) { > > > > > + struct rte_eth_dev_data *data = dev->data; > > > > > + struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data- > > > > > >dev_private); > > > > > + struct ice_pf *pf = ICE_DEV_PRIVATE_TO_PF(dev->data- > > > > >dev_private); > > > > > + uint16_t nb_rxq = 0; > > > > > + uint16_t nb_txq, i; > > > > > + int ret; > > > > > + > > > > > + ICE_PROC_SECONDARY_CHECK; > > > > > > > > Device start is not supported, but how is this differentiated from > > > > primary configured device vs secondary configured device. > > > > > > > > Ie: primary uses black list '-b BB:DD:F' while secondary uses '-w > > > > BB:DD:F'. In this case since we are checking process type this > > > > will return without > > > start? > > Two updates with respect to your comment, 1. tool and application like > > dpdk-procinfo will no longer be able pull data since you are asking to black > list. > > 2. If there are functions which needs to shared, like primary using rx-0 and > tx-0 then secondary rx-1 and tx-1 how to make this work? > > > [PATCH v2 01/20] net/ice: add base code > > Note: In version 1 I enquired about unit or DTS validation for PMD. Is > > this still holding good? > Yes, it's planned and on going. I confirmed validation is going on this device. But not impact this patch set unless we found bug and need to fix it. > > [PATCH v2 02/20] net/ice: support device initialization > > +# Compile burst-oriented ICE PMD driver # > CONFIG_RTE_LIBRTE_ICE_PMD=y > > Based on ' https://patches.dpdk.org/patch/48488/' it is suggested to > configure. But here is it already set to 'y'. Is this correct? If yes can you > update ' https://patches.dpdk.org/patch/48488/' We've discussed the setting. I don’t know anything left. If there's, please let me know. > > [PATCH v2 20/20] net/ice: support meson build > > > > > Should not meson build option be add start. That is in patch > > > > > 1/20 so compile options does not fail? > > > > It will not fail. Enabling the compile earlier only means the code > > > > can be > > > compiled. > > > > But, to use this device we do need the whole patch set. From this > > > > point of view, compiling it at the end maybe better. > > > Thanks for update, so will 'meson-build' success if apply 3 patches? > > Sure, meson build will not be broken by any one of these patches. Only > > until this patch, what built by meson can support ice. > Thanks for confirmation that you have tried './devtools/test-meson- > builds.sh' and the intermediate build for ICE_DSI PMD does not fail. I said meson build is working. But don't know what's ICE_DSI PMD. > > [PATCH v2 16/20] net/ice: support basic RX/TX > > [PATCH v2 14/20] net/ice: support statistics > > > > + ns->eth.tx_bytes -= (ns->eth.tx_unicast + ns->eth.tx_multicast + > > > > + ns->eth.tx_broadcast) * ETHER_CRC_LEN; > > > > > > In earlier patch for 'mtu set check' we added VSI SWITCH VLAN. > > > Should we add VSI VLAN here? > > Don't need. They're different functions. We add crc length here > > because of HW counts the packet length before crc is added. > So you are not fetching stats from HW registers from switch is this correct? > How will you get stats for actually transmitted in xstats? As I understand > xstats is for switch HW stats right? No. The stats is got from HW. We just correct it as HW doesn’t have the chance to add CRC length. But I don't understand why switch is mentioned here. > > [PATCH v2 04/20] net/ice: support getting device information > > > Does this mean per queue offload capability is not supported? If > > > yes, can you mention this in release notes under 'support or limitation' > > No, it's not supported. We have a document, ice.ini, to list all the > > features supported. All the others are not supported. > > BTW, I don't think anything not supported is limitation. > If I understand correctly, ICE_DSI_PMD is advertising it has not offload for > RX and TX. But you are stating in ice.ini you are listing offload supports. So > let me rephrase the question 'if you support port level offload capability, it > will reflect for all queues rx and tx. But if you reflect queue level offload > as 0 > for rx and tx, then APIs rte_eth_rx_queue_setup and > rte_eth_tx_queue_setup if queue offload enabled should fail. Is this correct > understanding?' Sorry, I don’t know what's ICE_DSI_PMD. > > > > If device switch is not configured (default value from NVM) should > > > we highlight the switch can support speed 10, 100, 1000, 1000 and son > on? > > No, this's the capability getting from HW. > If HW is supported or configured for 10, 100, 25G then those should be > returned correctly this I agree. But when the device is queried for capability > it should highlight all supported speeds of switch. Am I right? No. Here shows the result not all the speeds supported. Like the speed after auto negotiation. > > > > If speed is not true as stated above, can you please add this to > > > release notes and documentation. > > Here listed all the case we can get from HW. > Please add to ice_dsi documentation also. Sorry, no idea about ice_dsi. > > snipped
