[v4,03/10] ethdev: add simple power management API

  Add a simple API allow ethdev get wake up address from PMD.
Also include internal structure update.

Signed-off-by: Liang Ma <liang.j.ma@intel.com>
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_ethdev/rte_ethdev.c           | 19 ++++++++++++++++
 lib/librte_ethdev/rte_ethdev.h           | 24 ++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev_driver.h    | 28 ++++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev_version.map |  1 +
 4 files changed, 72 insertions(+)
  

> +/**
> + * 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.
> + */
> +__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);

It looks to be a very low-level API.
Can't we do something more "ready-to-use" at ethdev level?

Cc'in the relevant maintainers...
Note: sorry this comment come late but ethdev maintainers were not Cc.
Reminder: having no feedback is not a good sign, you should request comments.
  

> > +/**
> > + * 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.
> > + */
> > +__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);
> 
> It looks to be a very low-level API.
> Can't we do something more "ready-to-use" at ethdev level?

I think that series provides both:
There is a low-level API at ethdev/eal to retrieve information to wait for
and actual function to put core to sleep.
Plus there is a high-level API at rte_power lib:
rte_power_pmd_mgmt_queue_enable()/rte_power_pmd_mgmt_queue_disable()
that uses these low-level ones and puts some high-level logic around it.
From my perspective it is a good design choice,
as it keeps all power-related burden inside rte_power library and
provides user a lot of flexibility in terms of API usage.
Konstantin 

> 
> Cc'in the relevant maintainers...
> Note: sorry this comment come late but ethdev maintainers were not Cc.
> Reminder: having no feedback is not a good sign, you should request comments.
>
  

> 
> Add a simple API allow ethdev get wake up address from PMD.
> Also include internal structure update.
> 
> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>  lib/librte_ethdev/rte_ethdev.c           | 19 ++++++++++++++++
>  lib/librte_ethdev/rte_ethdev.h           | 24 ++++++++++++++++++++
>  lib/librte_ethdev/rte_ethdev_driver.h    | 28 ++++++++++++++++++++++++
>  lib/librte_ethdev/rte_ethdev_version.map |  1 +
>  4 files changed, 72 insertions(+)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index d7668114ca..88253d95f9 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -4804,6 +4804,25 @@ 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;
> +	uint16_t ret;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +	dev = &rte_eth_devices[port_id];
> +
> +	ret = (*dev->dev_ops->get_wake_addr)
> +				(dev->data->rx_queues[queue_id],
> +				 wake_addr, expected, mask);


This is an optional dev_ops, so I think you need to check that get_wake_addr()
is defined for that PMD.
Plus you need to check that queue_id is valid.

> +
> +	return ret;
> +}
> +
>  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.
> + */
> +__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.
> + */
> +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
  

On 08-Oct-20 11:26 PM, Ananyev, Konstantin wrote:
>>
>> Add a simple API allow ethdev get wake up address from PMD.
>> Also include internal structure update.
>>
>> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> ---
>>   lib/librte_ethdev/rte_ethdev.c           | 19 ++++++++++++++++
>>   lib/librte_ethdev/rte_ethdev.h           | 24 ++++++++++++++++++++
>>   lib/librte_ethdev/rte_ethdev_driver.h    | 28 ++++++++++++++++++++++++
>>   lib/librte_ethdev/rte_ethdev_version.map |  1 +
>>   4 files changed, 72 insertions(+)
>>
>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>> index d7668114ca..88253d95f9 100644
>> --- a/lib/librte_ethdev/rte_ethdev.c
>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> @@ -4804,6 +4804,25 @@ 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;
>> +uint16_t ret;
>> +
>> +RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +dev = &rte_eth_devices[port_id];
>> +
>> +ret = (*dev->dev_ops->get_wake_addr)
>> +(dev->data->rx_queues[queue_id],
>> + wake_addr, expected, mask);
> 
> 
> This is an optional dev_ops, so I think you need to check that get_wake_addr()
> is defined for that PMD.
> Plus you need to check that queue_id is valid.
> 

Sorry, added dev_ops check but left out the queue id part :( Will fix in v6.