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 9BEB3A04B7; Wed, 14 Oct 2020 11:30:29 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 22ED21DDA1; Wed, 14 Oct 2020 11:30:22 +0200 (CEST) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id BD6C81DAAC for ; Wed, 14 Oct 2020 11:30:19 +0200 (CEST) IronPort-SDR: xls+sOUJgSXj9+UiAk8CQPwyjLFP8UJ4AvoPhBvbpAk53qzBF5vfHyMT659grqabzb2eRgrjxt 8mrGxgXUoAkg== X-IronPort-AV: E=McAfee;i="6000,8403,9773"; a="145383784" X-IronPort-AV: E=Sophos;i="5.77,374,1596524400"; d="scan'208";a="145383784" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Oct 2020 02:30:18 -0700 IronPort-SDR: +KbdpCQYCbmXKZf1L12XpHFWhhj8xDrgCQ1x6GqpNlCCThY727vUqLDO1vE/3pAMMDnxNRvVuN 1mPOZW5T8LPQ== X-IronPort-AV: E=Sophos;i="5.77,374,1596524400"; d="scan'208";a="520311663" 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:30:16 -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> <05a92739-b0f5-5808-cedf-4fd1c4d44f97@intel.com> From: "Burakov, Anatoly" Message-ID: <8da2c513-2cc0-3ac9-4bf7-ecc763207144@intel.com> Date: Wed, 14 Oct 2020 10:30:14 +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 10:15 AM, Guo, Jia wrote: > >> -----Original Message----- >> From: Burakov, Anatoly >> Sent: Wednesday, October 14, 2020 5:07 PM >> 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 >> 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 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? >> > > Do you think this part would be better separate into [PATCH v5 05/10]? Oh, sorry, you're right, these aren't related, as this functionality isn't in this patch yet. Will fix. > >>> >>>> >>>> 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 -- Thanks, Anatoly