Hi Ferruh,

在 2023/11/23 19:56, lihuisong (C) 写道:

在 2023/11/2 7:39, Ferruh Yigit 写道:
timesync_read_rx_timestamp
On 9/21/2023 12:59 PM, lihuisong (C) wrote:
add ice & igc maintainers

在 2023/9/21 19:06, Ferruh Yigit 写道:
On 9/21/2023 11:02 AM, lihuisong (C) wrote:
Hi Ferruh,

Sorry for my delay reply because of taking a look at all PMDs
implementation.


在 2023/9/16 1:46, Ferruh Yigit 写道:
On 8/17/2023 9:42 AM, Huisong Li wrote:
   From the first version of ptpclient, it seems that this example
assume that
the PMDs support the PTP feature and enable PTP by default. Please see
commit ab129e9065a5 ("examples/ptpclient: add minimal PTP client")
which are introduced in 2015.

And two years later, Rx HW timestamp offload was introduced to
enable or
disable PTP feature in HW via rte_eth_rxmode. Please see
commit 42ffc45aa340 ("ethdev: add Rx HW timestamp capability").

Hi Huisong,

As far as I know this offload is not for PTP.
PTP and TIMESTAMP are different.
If TIMESTAMP offload cannot stand for PTP, we may need to add one new
offlaod for PTP.

Can you please detail what is "PTP offload"?

It indicates whether the device supports PTP or enable  PTP feature.

We have 'rte_eth_timesync_enable()' and 'rte_eth_timesync_disable()'
APIs to control PTP support.
No, this is just to control it.
we still need to like a device capablity to report application if the port support to call this API, right?

But when mention from "offload", it is something device itself does.

PTP is a protocol (IEEE 1588), and used to synchronize clocks.
What I get is protocol can be parsed by networking stack and it can be
used by application to synchronize clock.

When you are refer to "PTP offload", does it mean device (NIC)
understands the protocol and parse it to synchronize device clock with
other devices?
Good point. PTP offload is unreasonable.
But the capablity is required indeed.
What do you think of introducing a RTE_ETH_DEV_PTP in dev->data->dev_flags for PTP feature?

Can you take a look at this discussion line again?

Let's introduce a  RTE_ETH_DEV_PTP in dev->data->dev_flags to reveal if the device support PTP feature.



We have 'rte_eth_timesync_*()' APIs, my understanding is application
parses the PTP protocol, and it may use this information to configure
NIC to synchronize its clock, but it may also use PTP provided
information to sync any other clock. Is this understanding correct?


If TIMESTAMP offload is not for PTP, I don't know what the point of this
offload independent existence is.

TIMESTAMP offload request device to add timestamp to mbuf in ingress,
and use mbuf timestamp to schedule packet for egress.
Agree.

Technically this time-stamping can be done by driver, but if offload
set, HW timestamp is used for it.

Rx timestamp can be used for various reasons, like debugging and
performance/latency analyses, etc..


PTP is a protocol for time sync.
Rx TIMESTAMP offload is to ask HW to add timestamp to mbuf.
Yes.
But a lot of PMDs actually depand on HW to report Rx timestamp releated
information
because of reading Rx timestamp of PTP SYNC packet in read_rx_timestamp
API.

HW support may be required for PTP but this doesn't mean timestamp
offload is used.
understand.
And then about four years later, ptpclient enable Rx timestamp offload
because some PMDs require this offload to enable. Please see
commit 7a04a4f67dca ("examples/ptpclient: enable Rx timestamp
offload").

dpaa2 seems using TIMESTAMP offload and PTP together, hence they
updated
ptpclient sample to set TIMESTAMP offload.
There are many PMDs doing like this, such as ice, igc, cnxk, dpaa2, hns3
and so on.

Can you please point the ice & igc code, cc'ing their maintainers, we
can look together?
*-->igc code:*

Having following codes in igc_recv_scattered_pkts():

         if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
             uint32_t *ts = rte_pktmbuf_mtod_offset(first_seg,
                     uint32_t *, -IGC_TS_HDR_LEN);
             rxq->rx_timestamp = (uint64_t)ts[3] * NSEC_PER_SEC +
                     ts[2];
             rxm->timesync = rxq->queue_id;
         }
Note:this rxm->timesync will be used in timesync_read_rx_timestamp()

Above code requires TIMESTAMP offload to set timesync, but this
shouldn't be a requirement. Usage seems mixed.

*-->ice code:*

#ifndef RTE_LIBRTE_ICE_16BYTE_RX_DESC
         if (ice_timestamp_dynflag > 0 &&
             (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP)) {
             rxq->time_high =
                rte_le_to_cpu_32(rxd.wb.flex_ts.ts_high);
             if (unlikely(is_tsinit)) {
                 ts_ns = ice_tstamp_convert_32b_64b(hw, ad, 1,
rxq->time_high);
                 rxq->hw_time_low = (uint32_t)ts_ns;
                 rxq->hw_time_high = (uint32_t)(ts_ns >> 32);
                 is_tsinit = false;
             } else {
                 if (rxq->time_high < rxq->hw_time_low)
                     rxq->hw_time_high += 1;
                 ts_ns = (uint64_t)rxq->hw_time_high << 32 | rxq->time_high;
                 rxq->hw_time_low = rxq->time_high;
             }
             rxq->hw_time_update = rte_get_timer_cycles() /
                          (rte_get_timer_hz() / 1000);
             *RTE_MBUF_DYNFIELD(rxm,
                        (ice_timestamp_dynfield_offset),
                        rte_mbuf_timestamp_t *) = ts_ns;
             pkt_flags |= ice_timestamp_dynflag;
         }

         if (ad->ptp_ena && ((rxm->packet_type & RTE_PTYPE_L2_MASK) ==
             RTE_PTYPE_L2_ETHER_TIMESYNC)) {
             rxq->time_high =
                rte_le_to_cpu_32(rxd.wb.flex_ts.ts_high);
             rxm->timesync = rxq->queue_id;
             pkt_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
         }
#endif

Note: rxm->timesync and rxq->time_high will be used in
timesync_read_rx_timestamp()

This usage looks good, if TIMESTAMP offload enabled mbuf dynamic field
and flag set accordingly.
hns3 also implemented PTP as ice did.

And if PTP enabled, and PTP packet type detected relevant flag set in
mbuf, and timesyc value set to use later for 'timesync_read_rx_timestamp()'.
Yes.


Although above usage looks correct, I can see in 'ice_timesync_enable()'
TIMESTAMP offload is used requirement to enable timesync.
TIMESTAMP offload seems used as way to enable HW timestamp, as Hemant
mentioned what is done is dpaa2.



We need to clarify dpaa2 usage.

By all the records, this is more like a process of perfecting PTP
feature.
Not all network adaptors support PTP feature. So adding the check for
PTP
capability in ethdev layer is necessary.

Nope, as PTP (IEEE1588/802.1AS) implemented as dev_ops, and ops already
checked, so no additional check is needed.
But only having dev_ops about PTP doesn't satisfy the use of this
feature.
For example,
there are serveal network ports belonged to a driver on one OS, and only
one port support PTP function.
So driver needs one *PTP* offload.
We just need to clarify TIMESTAMP offload and PTP usage and find out
what is causing confusion.
Yes it is a little bit confusion.
There are two kinds of implementation:
A: ixgbe and txgbe (it seems that their HW is similar) don't need
TIMESTAMP offload,and only use dev_ops to finish PTP feature.
B:  saving "Rx timestamp related information" from Rx description when
receive PTP SYNC packet and
      report it in read_rx_timestamp API.
For case B, most of driver use TIMESTAMP offload to decide if driver
save "Rx timestamp related information.
What do you think about this, Ferruh?
I would be great if you can help on clarification, and update
documentation or API comments, or what ever required, for this.
ok
---
v3:
    - patch [2/3] for hns3 has been applied and so remove it.
    - ops pointer check is closer to usage.

Huisong Li (2):
     examples/ptpclient: add the check for PTP capability
     ethdev: add the check for the valitity of timestamp offload

    examples/ptpclient/ptpclient.c |  5 +++
    lib/ethdev/rte_ethdev.c        | 57
+++++++++++++++++++++++++++++++++-
    2 files changed, 61 insertions(+), 1 deletion(-)

.
.
.

.

Reply via email to