From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 9E10CA04B7; Wed, 14 Oct 2020 11:07:22 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 8B1A21D9C3; Wed, 14 Oct 2020 11:07:20 +0200 (CEST) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id 7AA021C192 for ; Wed, 14 Oct 2020 11:07:18 +0200 (CEST) IronPort-SDR: rcOo+9NYMT6uZdA3g32ys21+y+lap6ZR6cB7HnS69CUQXfAF79YghvpyF89a0UM3MynrbesHQt yTSwHr6+EthQ== X-IronPort-AV: E=McAfee;i="6000,8403,9773"; a="145930843" X-IronPort-AV: E=Sophos;i="5.77,374,1596524400"; d="scan'208";a="145930843" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Oct 2020 02:07:16 -0700 IronPort-SDR: Y1LPlE0PEW1ftFEuK1zPPGfRCx9SHTIySutq1zaSuMWkTpirk8+xxkUKYM94Rx5854THuqr16p 0mwFMnsyZK2w== X-IronPort-AV: E=Sophos;i="5.77,374,1596524400"; d="scan'208";a="520306957" Received: from cfitzp2-mobl1.ger.corp.intel.com (HELO [10.251.84.158]) ([10.251.84.158]) by fmsmga006-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Oct 2020 02:07:11 -0700 To: "Guo, Jia" , "dev@dpdk.org" Cc: "Ma, Liang J" , Thomas Monjalon , "Yigit, Ferruh" , Andrew Rybchenko , Ray Kinsella , Neil Horman , "Hunt, David" , "Ananyev, Konstantin" , "jerinjacobk@gmail.com" , "Richardson, Bruce" , "McDaniel, Timothy" , "Eads, Gage" , "Macnamara, Chris" References: <1601647919-25312-1-git-send-email-liang.j.ma@intel.com> <931cbea6d091f16a51ad7eed736b4b6e69df93aa.1602258833.git.anatoly.burakov@intel.com> From: "Burakov, Anatoly" Message-ID: <05a92739-b0f5-5808-cedf-4fd1c4d44f97@intel.com> Date: Wed, 14 Oct 2020 10:07:09 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v5 04/10] ethdev: add simple power management API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 14-Oct-20 4:10 AM, Guo, Jia wrote: > >> -----Original Message----- >> From: dev On Behalf Of Anatoly Burakov >> Sent: Saturday, October 10, 2020 12:02 AM >> To: dev@dpdk.org >> Cc: Ma, Liang J ; Thomas Monjalon >> ; Yigit, Ferruh ; Andrew >> Rybchenko ; Ray Kinsella >> ; Neil Horman ; Hunt, David >> ; Ananyev, Konstantin >> ; jerinjacobk@gmail.com; Richardson, >> Bruce ; McDaniel, Timothy >> ; Eads, Gage ; >> Macnamara, Chris >> Subject: [dpdk-dev] [PATCH v5 04/10] ethdev: add simple power >> management API >> >> From: Liang Ma >> >> 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 >> Signed-off-by: Anatoly Burakov >> --- 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? > >> >> 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. > >> +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