> -----Original Message-----
> From: Burakov, Anatoly <anatoly.bura...@intel.com>
> Sent: Wednesday, October 14, 2020 5:07 PM
> To: Guo, Jia <jia....@intel.com>; dev@dpdk.org
> Cc: Ma, Liang J <liang.j...@intel.com>; Thomas Monjalon
> <tho...@monjalon.net>; Yigit, Ferruh <ferruh.yi...@intel.com>; Andrew
> Rybchenko <andrew.rybche...@oktetlabs.ru>; Ray Kinsella
> <m...@ashroe.eu>; Neil Horman <nhor...@tuxdriver.com>; Hunt, David
> <david.h...@intel.com>; Ananyev, Konstantin
> <konstantin.anan...@intel.com>; jerinjac...@gmail.com; Richardson,
> Bruce <bruce.richard...@intel.com>; McDaniel, Timothy
> <timothy.mcdan...@intel.com>; Eads, Gage <gage.e...@intel.com>;
> Macnamara, Chris <chris.macnam...@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v5 04/10] ethdev: add simple power
> management API
>
> On 14-Oct-20 4:10 AM, Guo, Jia wrote:
> >
> >> -----Original Message-----
> >> From: dev <dev-boun...@dpdk.org> On Behalf Of Anatoly Burakov
> >> Sent: Saturday, October 10, 2020 12:02 AM
> >> To: dev@dpdk.org
> >> Cc: Ma, Liang J <liang.j...@intel.com>; Thomas Monjalon
> >> <tho...@monjalon.net>; Yigit, Ferruh <ferruh.yi...@intel.com>; Andrew
> >> Rybchenko <andrew.rybche...@oktetlabs.ru>; Ray Kinsella
> >> <m...@ashroe.eu>; Neil Horman <nhor...@tuxdriver.com>; Hunt, David
> >> <david.h...@intel.com>; Ananyev, Konstantin
> >> <konstantin.anan...@intel.com>; jerinjac...@gmail.com; Richardson,
> >> Bruce <bruce.richard...@intel.com>; McDaniel, Timothy
> >> <timothy.mcdan...@intel.com>; Eads, Gage <gage.e...@intel.com>;
> >> Macnamara, Chris <chris.macnam...@intel.com>
> >> Subject: [dpdk-dev] [PATCH v5 04/10] ethdev: add simple power
> >> management API
> >>
> >> From: Liang Ma <liang.j...@intel.com>
> >>
> >> Add a simple API to allow getting address of next RX descriptor from
> >> the PMD, as well as release notes information.
> >>
> >> Signed-off-by: Liang Ma <liang.j...@intel.com>
> >> Signed-off-by: Anatoly Burakov <anatoly.bura...@intel.com>
> >> ---
>
> Hi Jia,
>
> Thanks for your review. Responses below.
>
> >>
> >> Notes:
> >> v5:
> >> - Bring function format in line with other functions in the file
> >> - Ensure the API is supported by the driver before calling it
> >> (Konstantin)
> >>
> >> doc/guides/rel_notes/release_20_11.rst | 16 ++++++++++++++
> >> lib/librte_ethdev/rte_ethdev.c | 17 ++++++++++++++
> >> lib/librte_ethdev/rte_ethdev.h | 24 ++++++++++++++++++++
> >> lib/librte_ethdev/rte_ethdev_driver.h | 28
> ++++++++++++++++++++++++
> >> lib/librte_ethdev/rte_ethdev_version.map | 1 +
> >> 5 files changed, 86 insertions(+)
> >>
> >> diff --git a/doc/guides/rel_notes/release_20_11.rst
> >> b/doc/guides/rel_notes/release_20_11.rst
> >> index 808bdc4e54..e85af5d3e9 100644
> >> --- a/doc/guides/rel_notes/release_20_11.rst
> >> +++ b/doc/guides/rel_notes/release_20_11.rst
> >> @@ -55,6 +55,11 @@ New Features
> >> Also, make sure to start the actual text at the margin.
> >>
> =======================================================
> >>
> >> +* **ethdev: add 1 new EXPERIMENTAL API for PMD power
> >> management.**
> >> +
> >> + * ``rte_eth_get_wake_addr()``
> >> + * add new eth_dev_ops ``get_wake_addr``
> >> +
> >> * **Updated Broadcom bnxt driver.**
> >>
> >> Updated the Broadcom bnxt driver with new features and
> >> improvements,
> >> including:
> >> @@ -136,6 +141,17 @@ New Features
> >> * Extern objects and functions can be plugged into the pipeline.
> >> * Transaction-oriented table updates.
> >>
> >> +* **Add PMD power management mechanism**
> >> +
> >> + 3 new Ethernet PMD power management mechanism is added through
> >
> > " mechanisms are " please.
> >
> >> + existing RX callback infrastructure.
> >> +
> >> + * Add power saving scheme based on UMWAIT instruction (x86 only)
> >> + * Add power saving scheme based on ``rte_pause()``
> >> + * Add power saving scheme based on frequency scaling through the
> >> + power library
> >> + * Add new EXPERIMENTAL API
> >> ``rte_power_pmd_mgmt_queue_enable()``
> >> + * Add new EXPERIMENTAL API
> >> ``rte_power_pmd_mgmt_queue_disable()``
> >> +
> >
> > Could this doc be separate to other specific patch if it is not related
> > with this
> patch?
>
> It is related - it's the doc changes that add mention of this API. I was under
> the impression current policy was having doc updates in the same patch as
> the changes made?
>
Do you think this part would be better separate into [PATCH v5 05/10]?
> >
> >>
> >> Removed Items
> >> -------------
> >> diff --git a/lib/librte_ethdev/rte_ethdev.c
> >> b/lib/librte_ethdev/rte_ethdev.c index 48d1333b17..352108f43c 100644
> >> --- a/lib/librte_ethdev/rte_ethdev.c
> >> +++ b/lib/librte_ethdev/rte_ethdev.c
> >> @@ -4804,6 +4804,23 @@ rte_eth_tx_burst_mode_get(uint16_t port_id,
> >> uint16_t queue_id,
> >> dev->dev_ops->tx_burst_mode_get(dev, queue_id,
> mode)); }
> >>
> >> +int
> >> +rte_eth_get_wake_addr(uint16_t port_id, uint16_t queue_id,
> >> + volatile void **wake_addr, uint64_t *expected, uint64_t
> >> *mask) {
> >> + struct rte_eth_dev *dev;
> >> +
> >> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> >> +
> >> + dev = &rte_eth_devices[port_id];
> >> +
> >> + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->get_wake_addr, -
> >> ENOTSUP);
> >> +
> >> + return eth_err(port_id,
> >> + dev->dev_ops->get_wake_addr(dev->data-
> >>> rx_queues[queue_id],
> >> + wake_addr, expected, mask));
> >> +}
> >> +
> >> int
> >> rte_eth_dev_set_mc_addr_list(uint16_t port_id,
> >> struct rte_ether_addr *mc_addr_set, diff --git
> >> a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> >> index
> >> d2bf74f128..a6cfe3cd57 100644
> >> --- a/lib/librte_ethdev/rte_ethdev.h
> >> +++ b/lib/librte_ethdev/rte_ethdev.h
> >> @@ -4014,6 +4014,30 @@ __rte_experimental int
> >> rte_eth_tx_burst_mode_get(uint16_t port_id, uint16_t queue_id,
> >> struct rte_eth_burst_mode *mode);
> >>
> >> +/**
> >> + * Retrieve the wake up address from specific queue
> >> + *
> >> + * @param port_id
> >> + * The port identifier of the Ethernet device.
> >> + * @param queue_id
> >> + * The Tx queue on the Ethernet device for which information
> >> + * will be retrieved.
> >> + * @param wake_addr
> >> + * The pointer point to the address which is used for monitoring.
> >> + * @param expected
> >> + * The pointer point to value to be expected when descriptor is set.
> >> + * @param mask
> >> + * The pointer point to comparison bitmask for the expected value.
> >> + *
> >> + * @return
> >> + * - 0: Success.
> >> + * -EINVAL: Failed to get wake address.
> >> + */
> >
> > Is that "-EINVAL " is the only error value which will be return?
>
> Also -ENOTSUP, i'll add this, thanks.
>
> >
> >> +__rte_experimental
> >> +int rte_eth_get_wake_addr(uint16_t port_id, uint16_t queue_id,
> >> + volatile void **wake_addr,
> >> + uint64_t *expected, uint64_t *mask);
> >> +
> >> /**
> >> * Retrieve device registers and register attributes (number of registers
> and
> >> * register size)
> >> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h
> >> b/lib/librte_ethdev/rte_ethdev_driver.h
> >> index c3062c246c..935d46f25c 100644
> >> --- a/lib/librte_ethdev/rte_ethdev_driver.h
> >> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
> >> @@ -574,6 +574,31 @@ typedef int (*eth_tx_hairpin_queue_setup_t)
> >> uint16_t nb_tx_desc,
> >> const struct rte_eth_hairpin_conf *hairpin_conf);
> >>
> >> +/**
> >> + * @internal
> >> + * Get the Wake up address.
> >> + *
> >> + * @param rxq
> >> + * Ethdev queue pointer.
> >> + * @param tail_desc_addr
> >> + * The pointer point to descriptor address var.
> >> + * @param expected
> >> + * The pointer point to value to be expected when descriptor is set.
> >> + * @param mask
> >> + * The pointer point to comparison bitmask for the expected value.
> >> + * @return
> >> + * Negative errno value on error, 0 on success.
> >> + *
> >> + * @retval 0
> >> + * Success.
> >> + * @retval -EINVAL
> >> + * Failed to get descriptor address.
> >> + */
> >
> > The question is the same as above.
>
> This is a driver function pointer, so return value will depend on driver
> implementation. So far we only see 0 or -EINVAL values from the driver itself,
> while -ENOTSUP will be returned by ethdev in case there is no driver
> implementation of this function. So, in this case this is correct.
>
Ok.
> >
> >> +typedef int (*eth_get_wake_addr_t)
> >> + (void *rxq, volatile void **tail_desc_addr,
> >> + uint64_t *expected, uint64_t *mask);
> >> +
> >> +
> >> /**
> >> * @internal A structure containing the functions exported by an
> Ethernet
> >> driver.
> >> */
> >> @@ -713,6 +738,9 @@ struct eth_dev_ops {
> >> /**< Set up device RX hairpin queue. */
> >> eth_tx_hairpin_queue_setup_t tx_hairpin_queue_setup;
> >> /**< Set up device TX hairpin queue. */
> >> + eth_get_wake_addr_t get_wake_addr;
> >> + /**< Get wake up address. */
> >> +
> >> };
> >>
> >> /**
> >> diff --git a/lib/librte_ethdev/rte_ethdev_version.map
> >> b/lib/librte_ethdev/rte_ethdev_version.map
> >> index c95ef5157a..3cb2093980 100644
> >> --- a/lib/librte_ethdev/rte_ethdev_version.map
> >> +++ b/lib/librte_ethdev/rte_ethdev_version.map
> >> @@ -229,6 +229,7 @@ EXPERIMENTAL {
> >> # added in 20.11
> >> rte_eth_link_speed_to_str;
> >> rte_eth_link_to_str;
> >> + rte_eth_get_wake_addr;
> >> };
> >>
> >> INTERNAL {
> >> --
> >> 2.17.1
>
>
> --
> Thanks,
> Anatoly
