Hi, Qi
> -----Original Message-----
> From: Zhang, Qi Z <qi.z.zh...@intel.com>
> Sent: Friday, September 24, 2021 1:25 PM
> To: Su, Simei <simei...@intel.com>
> Cc: dev@dpdk.org; Wang, Haiyue <haiyue.w...@intel.com>
> Subject: RE: [PATCH] net/ice: enable Rx timestamp on Flex Descriptor
>
>
>
> > -----Original Message-----
> > From: Su, Simei <simei...@intel.com>
> > Sent: Saturday, September 18, 2021 2:57 PM
> > To: Zhang, Qi Z <qi.z.zh...@intel.com>
> > Cc: dev@dpdk.org; Wang, Haiyue <haiyue.w...@intel.com>; Su, Simei
> > <simei...@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...@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.
