Re: [dpdk-dev] [PATCH] net/ice: enable Rx timestamp on Flex Descriptor

["Su, Simei" <simei.su@intel.com>]

Hi, Qi

> -----Original Message-----
> From: Zhang, Qi Z <qi.z.zhang@intel.com>
> Sent: Friday, September 24, 2021 1:25 PM
> To: Su, Simei <simei.su@intel.com>
> Cc: dev@dpdk.org; Wang, Haiyue <haiyue.wang@intel.com>
> Subject: RE: [PATCH] net/ice: enable Rx timestamp on Flex Descriptor
> 
> 
> 
> > -----Original Message-----
> > From: Su, Simei <simei.su@intel.com>
> > Sent: Saturday, September 18, 2021 2:57 PM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > Cc: dev@dpdk.org; Wang, Haiyue <haiyue.wang@intel.com>; Su, Simei
> > <simei.su@intel.com>
> > Subject: [PATCH] net/ice: enable Rx timestamp on Flex Descriptor
> >
> > Use the dynamic mbuf to register timestamp field and flag.
> > The ice has the feature to dump Rx timestamp value into dynamic mbuf
> > field by flex descriptor. This feature is turned on by dev config
> > "enable-rx-timestamp". Currently, it's only supported under scalar path.
> >
> > Signed-off-by: Simei Su <simei.su@intel.com>
> > ---
> >  doc/guides/rel_notes/release_21_11.rst |   1 +
> >  drivers/net/ice/ice_ethdev.c           |   6 +-
> >  drivers/net/ice/ice_rxtx.c             | 107
> > +++++++++++++++++++++++++++++++++
> >  drivers/net/ice/ice_rxtx.h             |   6 ++
> >  drivers/net/ice/ice_rxtx_vec_common.h  |   3 +
> >  5 files changed, 122 insertions(+), 1 deletion(-)
> >
> > diff --git a/doc/guides/rel_notes/release_21_11.rst
> > b/doc/guides/rel_notes/release_21_11.rst
> > index dc44739..1b9dac6 100644
> > --- a/doc/guides/rel_notes/release_21_11.rst
> > +++ b/doc/guides/rel_notes/release_21_11.rst
> > @@ -70,6 +70,7 @@ New Features
> >  * **Updated Intel ice driver.**
> >
> >    Added 1PPS out support by a devargs.
> > +  * Added Rx timstamp support by dynamic mbuf on Flex Descriptor.
> 
> How about just "added DEV_RX_OFFLOAD_TIMESTAMP support"
> 

  OK. I will change it.

> >
> >
> >  Removed Items
> > diff --git a/drivers/net/ice/ice_ethdev.c
> > b/drivers/net/ice/ice_ethdev.c index
> > 76dcabf..06adf43 100644
> > --- a/drivers/net/ice/ice_ethdev.c
> > +++ b/drivers/net/ice/ice_ethdev.c
> > @@ -31,6 +31,9 @@
> >  #define ICE_HW_DEBUG_MASK_ARG     "hw_debug_mask"
> >  #define ICE_ONE_PPS_OUT_ARG       "pps_out"
> >
> > +uint64_t ice_timestamp_dynflag;
> > +int ice_timestamp_dynfield_offset = -1;
> > +
> >  static const char * const ice_valid_args[] = {
> > ICE_SAFE_MODE_SUPPORT_ARG,  ICE_PIPELINE_MODE_SUPPORT_ARG,
> @@ -3672,7
> > +3675,8 @@ ice_dev_info_get(struct rte_eth_dev *dev, struct
> > rte_eth_dev_info *dev_info)  DEV_RX_OFFLOAD_QINQ_STRIP |
> > DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM |
> DEV_RX_OFFLOAD_VLAN_EXTEND |
> > -DEV_RX_OFFLOAD_RSS_HASH;
> > +DEV_RX_OFFLOAD_RSS_HASH |
> > +DEV_RX_OFFLOAD_TIMESTAMP;
> >  dev_info->tx_offload_capa |=
> >  DEV_TX_OFFLOAD_QINQ_INSERT |
> >  DEV_TX_OFFLOAD_IPV4_CKSUM |
> > diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c
> > index
> > 5d7ab4f..717d3f0 100644
> > --- a/drivers/net/ice/ice_rxtx.c
> > +++ b/drivers/net/ice/ice_rxtx.c
> > @@ -302,6 +302,18 @@ ice_program_hw_rx_queue(struct ice_rx_queue
> > *rxq)
> >  }
> >  }
> >
> > +if (rxq->offloads & DEV_RX_OFFLOAD_TIMESTAMP) {
> > +/* Register mbuf field and flag for Rx timestamp */ err =
> > +rte_mbuf_dyn_rx_timestamp_register(
> > +&ice_timestamp_dynfield_offset,
> > +&ice_timestamp_dynflag);
> > +if (err != 0) {
> > +PMD_INIT_LOG(ERR,
> > +"Cannot register mbuf field/flag for timestamp"); return -EINVAL; } }
> > +
> >  memset(&rx_ctx, 0, sizeof(rx_ctx));
> >
> >  rx_ctx.base = rxq->rx_ring_dma / ICE_QUEUE_BASE_ADDR_UNIT; @@
> > -354,6 +366,9 @@ ice_program_hw_rx_queue(struct ice_rx_queue *rxq)
> > regval |= (0x03 << QRXFLXP_CNTXT_RXDID_PRIO_S) &
> > QRXFLXP_CNTXT_RXDID_PRIO_M;
> >
> > +if (rxq->offloads & DEV_RX_OFFLOAD_TIMESTAMP) regval |=
> > +QRXFLXP_CNTXT_TS_M;
> > +
> >  ICE_WRITE_REG(hw, QRXFLXP_CNTXT(rxq->reg_idx), regval);
> >
> >  err = ice_clear_rxq_ctx(hw, rxq->reg_idx); @@ -1530,6 +1545,45 @@
> > ice_rxd_to_vlan_tci(struct rte_mbuf *mb, volatile union
> > ice_rx_flex_desc
> > *rxdp)
> >     mb->vlan_tci, mb->vlan_tci_outer);  }
> >
> > +uint64_t
> > +ice_read_time(struct ice_hw *hw)
> > +{
> > +uint32_t hi, lo, lo2;
> > +uint64_t time;
> > +
> > +lo = ICE_READ_REG(hw, GLTSYN_TIME_L(0)); hi = ICE_READ_REG(hw,
> > +GLTSYN_TIME_H(0));
> > +lo2 = ICE_READ_REG(hw, GLTSYN_TIME_L(0));
> > +
> > +if (lo2 < lo) {
> > +lo = ICE_READ_REG(hw, GLTSYN_TIME_L(0)); hi = ICE_READ_REG(hw,
> > +GLTSYN_TIME_H(0)); }
> > +
> > +time = ((uint64_t)hi << 32) | lo;
> > +
> > +return time;
> > +}
> > +
> > +uint64_t
> > +ice_tstamp_convert_32b_64b(uint64_t time, uint64_t timestamp) { const
> > +uint64_t mask = 0xFFFFFFFF; uint32_t delta; uint64_t ns;
> > +
> > +delta = (timestamp - (uint32_t)(time & mask));
> > +
> > +if (delta > (mask / 2)) {
> > +delta = ((uint32_t)(time & mask) - timestamp); ns = time - delta; }
> > +else { ns = time + delta; }
> > +
> > +return ns;
> > +}
> 
> Above two helper functions can be merged into one or be wrapped by a new
> function.
> and all functions can be defined as a static inline function in ice_rxtx.h.
> 

  Yes, above two functions can be merged into one, but I don't define it as a static function
because subsequent timesync support patch also need to use this function in ice_ethdev.c.

> > +
> >  #define ICE_LOOK_AHEAD 8
> >  #if (ICE_LOOK_AHEAD != 8)
> >  #error "PMD ICE: ICE_LOOK_AHEAD must be 8\n"
> > @@ -1546,6 +1600,9 @@ ice_rx_scan_hw_ring(struct ice_rx_queue *rxq)
> > int32_t i, j, nb_rx = 0;  uint64_t pkt_flags = 0;  uint32_t *ptype_tbl
> > = rxq->vsi->adapter->ptype_tbl;
> > +struct ice_vsi *vsi = rxq->vsi;
> > +struct ice_hw *hw = ICE_VSI_TO_HW(vsi); uint64_t time, ts_ns;
> >
> >  rxdp = &rxq->rx_ring[rxq->rx_tail];
> >  rxep = &rxq->sw_ring[rxq->rx_tail];
> > @@ -1589,6 +1646,20 @@ ice_rx_scan_hw_ring(struct ice_rx_queue *rxq)
> > ice_rxd_to_vlan_tci(mb, &rxdp[j]);  rxq->rxd_to_pkt_fields(rxq, mb,
> > &rxdp[j]);
> >
> > +if (rxq->offloads & DEV_RX_OFFLOAD_TIMESTAMP) {
> > +rxq->time_high =
> > +   rte_le_to_cpu_32(rxdp[j].wb.flex_ts.ts_high);
> > +time = ice_read_time(hw);
> > +ts_ns = ice_tstamp_convert_32b_64b(time,
> > +rxq->time_high);
> > +if (ice_timestamp_dynflag > 0) {
> > +*RTE_MBUF_DYNFIELD(mb,
> > +ice_timestamp_dynfield_offset,
> > +rte_mbuf_timestamp_t *) = ts_ns;
> > +mb->ol_flags |= ice_timestamp_dynflag;
> > +}
> > +}
> > +
> >  mb->ol_flags |= pkt_flags;
> >  }
> >
> > @@ -1772,6 +1843,9 @@ ice_recv_scattered_pkts(void *rx_queue,
> > uint64_t dma_addr;  uint64_t pkt_flags;  uint32_t *ptype_tbl =
> > rxq->vsi->adapter->ptype_tbl;
> > +struct ice_vsi *vsi = rxq->vsi;
> > +struct ice_hw *hw = ICE_VSI_TO_HW(vsi); uint64_t time, ts_ns;
> >
> >  while (nb_rx < nb_pkts) {
> >  rxdp = &rx_ring[rx_id];
> > @@ -1882,6 +1956,21 @@ ice_recv_scattered_pkts(void *rx_queue,
> > ice_rxd_to_vlan_tci(first_seg, &rxd);  rxq->rxd_to_pkt_fields(rxq,
> > first_seg, &rxd);  pkt_flags =
> > ice_rxd_error_to_pkt_flags(rx_stat_err0);
> > +
> > +if (rxq->offloads & DEV_RX_OFFLOAD_TIMESTAMP) {
> > +rxq->time_high =
> > +   rte_le_to_cpu_32(rxd.wb.flex_ts.ts_high);
> > +time = ice_read_time(hw);
> > +ts_ns = ice_tstamp_convert_32b_64b(time,
> > +rxq->time_high);
> > +if (ice_timestamp_dynflag > 0) {
> > +*RTE_MBUF_DYNFIELD(first_seg,
> > +ice_timestamp_dynfield_offset,
> > +rte_mbuf_timestamp_t *) = ts_ns;
> > +first_seg->ol_flags |= ice_timestamp_dynflag; } }
> > +
> >  first_seg->ol_flags |= pkt_flags;
> >  /* Prefetch data of first segment, if configured to do so. */
> > rte_prefetch0(RTE_PTR_ADD(first_seg->buf_addr,
> > @@ -2237,6 +2326,9 @@ ice_recv_pkts(void *rx_queue,  uint64_t
> > dma_addr;  uint64_t pkt_flags;  uint32_t *ptype_tbl =
> > rxq->vsi->adapter->ptype_tbl;
> > +struct ice_vsi *vsi = rxq->vsi;
> > +struct ice_hw *hw = ICE_VSI_TO_HW(vsi); uint64_t time, ts_ns;
> >
> >  while (nb_rx < nb_pkts) {
> >  rxdp = &rx_ring[rx_id];
> > @@ -2288,6 +2380,21 @@ ice_recv_pkts(void *rx_queue,
> > ice_rxd_to_vlan_tci(rxm, &rxd);  rxq->rxd_to_pkt_fields(rxq, rxm,
> > &rxd);  pkt_flags = ice_rxd_error_to_pkt_flags(rx_stat_err0);
> > +
> > +if (rxq->offloads & DEV_RX_OFFLOAD_TIMESTAMP) {
> > +rxq->time_high =
> > +   rte_le_to_cpu_32(rxd.wb.flex_ts.ts_high);
> > +time = ice_read_time(hw);
> > +ts_ns = ice_tstamp_convert_32b_64b(time,
> > +rxq->time_high);
> > +if (ice_timestamp_dynflag > 0) {
> > +*RTE_MBUF_DYNFIELD(rxm,
> > +ice_timestamp_dynfield_offset,
> > +rte_mbuf_timestamp_t *) = ts_ns;
> > +rxm->ol_flags |= ice_timestamp_dynflag;
> > +}
> > +}
> > +
> >  rxm->ol_flags |= pkt_flags;
> >  /* copy old mbuf to rx_pkts */
> >  rx_pkts[nb_rx++] = rxm;
> > diff --git a/drivers/net/ice/ice_rxtx.h b/drivers/net/ice/ice_rxtx.h
> > index b10db08..b3dc80b 100644
> > --- a/drivers/net/ice/ice_rxtx.h
> > +++ b/drivers/net/ice/ice_rxtx.h
> > @@ -40,6 +40,9 @@
> >
> >  #define ICE_RXDID_COMMS_OVS22
> >
> > +extern uint64_t ice_timestamp_dynflag; extern int
> > +ice_timestamp_dynfield_offset;
> > +
> >  typedef void (*ice_rx_release_mbufs_t)(struct ice_rx_queue *rxq);
> > typedef void (*ice_tx_release_mbufs_t)(struct ice_tx_queue *txq);
> > typedef void (*ice_rxd_to_pkt_fields_t)(struct ice_rx_queue *rxq, @@
> > -89,6 +92,7 @@ struct ice_rx_queue {  ice_rxd_to_pkt_fields_t
> > rxd_to_pkt_fields; /* handle FlexiMD by RXDID */
> > ice_rx_release_mbufs_t rx_rel_mbufs;  uint64_t offloads;
> > +uint32_t time_high;
> 
> Why we need this field?
> I saw the only place it is referenced is to set value with
> rte_le_to_cpu_32(rxdp[j].wb.flex_ts.ts_high and parse it to
> ice_tstamp_convert_32b_64b
> 
> >
> 

Yes, timesync patch can also reuse this field, so I add this field, but you are right, I will remove this field
can add it in the following timesync patch.