> -----Original Message----- > From: Varghese, Vipin > Sent: Friday, December 14, 2018 11:26 AM > To: Lu, Wenzhuo <wenzhuo...@intel.com>; dev@dpdk.org > Subject: RE: [dpdk-dev] [PATCH v3 00/34] A new net PMD - ice > > Hi Wenzhuo, > > snipped > > > > > > Thanks for the update, couple of suggestion in my opinion shared > > > below > > > > > > Snipped > > > > > [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. > > > I think I poorly communicated, so let me try again. In document it > > > is stated to configure as 'y'. But in your default config is 'y'. So > > > either make the default as 'n' so user can configure or reword in > > > document as 'Ensure the config is set to y for before building' > > In the patch, I only see "``CONFIG_RTE_LIBRTE_ICE_PMD`` (default > > ``y``) ". No conflict with the configure file. > I think I am failing you to convey even after multiple attempts. Hence I > humbly leave this to other from mailing list to get it sorted. I also feel confused. As the compile config is set to 'y' and the document says the default value is 'y'. I just don’t find the problem.
> > > > > > > > > > > > > > > > > > > > [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. > > > > > > Once again apologies if I poorly communicated ICE_PMD, my question > > > is 'if I take patch 1/20 in v2 can I build for librte_pmd_ice'? is > > > not we are expecting each additional layering for functionality like > > > mtu set, switch set should be compiling and not just the last build? > > O, you want to support meson build from the beginning. I can move it > > forward, although I recommend to use it after all the patches. > Thank you for consideration, I think this approach helps a lot since we are > systematically adding code to ICE_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. > > > Will ice pmd for CVL expose all the switch stats as 'xstats'? If > > > yes, can you share in patch comments what are switch level stats? > > No. xstats means the extend stats which is not covered by the basic > > stats. It can be anything which is not in basic stats. > Thanks for confirming, hence is xstats covering for ICE Switch stats which is > not covered in PMD stats? If yes, can you mention the list of stats covered > under xstats that is fetched specifically from ICE switch? Sorry. I cannot do that. The HW is somehow black box to us. I cannot tell you something I'm not sure about. > > > > > > > > > > > > > > > > > > > > [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. > > > ICE_PMD > > No exactly. We follow the rule of queue and port offload setting. If > > the feature is enabled in port level, we can ignore the queue setting. > > If the feature is not enabled in port level, we still can enable it per > > queue. > Thanks for confirming, so what will be result for non-configured ICE port for > offload for port and queue using rte_eth_dev_get_info? The get_info also show the default value. The offload is set to 0 by default. > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > As per your current statement "If user uses API rte_eth_dev_info_get > > > to get speed_capa, current speed will be returned as auto negotiated > > > value and not ' ETH_LINK_SPEED_10M| ETH_LINK_SPEED_100M| > > > ETH_LINK_SPEED_1G| ETH_LINK_SPEED_10G| ETH_LINK_SPEED_25G'". I > will > > > leave this to others to comment since in my humble opinion this is > > > not expected. > > OK. I'll change it to the bitmap. > Thanks, appreciate the understanding > > > > > > > > > > > > > > > > > > > > > > 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. > > > My mistake ICE_PMD is what I am referring to. > > > > > > > > > > > > > > > > > snipped
