Re: [dpdk-dev] [PATCH v1 2/5] ixgbe: enable rx queue interrupts for both PF and VF

["Qiu, Michael" <michael.qiu@intel.com>]

On 1/29/2015 1:39 PM, Zhou, Danny wrote:
>
>> -----Original Message-----
>> From: Qiu, Michael
>> Sent: Thursday, January 29, 2015 11:40 AM
>> To: Zhou, Danny; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v1 2/5] ixgbe: enable rx queue interrupts for both PF and VF
>>
>> On 1/28/2015 5:52 PM, Danny Zhou wrote:
>>> Signed-off-by: Danny Zhou <danny.zhou@intel.com>
>>> ---
>>>  lib/librte_pmd_ixgbe/ixgbe_ethdev.c | 371 ++++++++++++++++++++++++++++++++++++
>>>  lib/librte_pmd_ixgbe/ixgbe_ethdev.h |   9 +
>>>  2 files changed, 380 insertions(+)
>>>
>>> diff --git a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
>>> index b341dd0..39f883a 100644
>>> --- a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
>>> +++ b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
>>> @@ -60,6 +60,7 @@
>>>  #include <rte_atomic.h>
>>>  #include <rte_malloc.h>
>>>  #include <rte_random.h>
>>> +#include <rte_spinlock.h>
>>>  #include <rte_dev.h>
>>>
>>>  #include "ixgbe_logs.h"
>>> @@ -173,6 +174,7 @@ static int ixgbe_dev_rss_reta_query(struct rte_eth_dev *dev,
>>>  			uint16_t reta_size);
>>>  static void ixgbe_dev_link_status_print(struct rte_eth_dev *dev);
>>>  static int ixgbe_dev_lsc_interrupt_setup(struct rte_eth_dev *dev);
>>> +static int ixgbe_dev_rxq_interrupt_setup(struct rte_eth_dev *dev);
>>>  static int ixgbe_dev_interrupt_get_status(struct rte_eth_dev *dev);
>>>  static int ixgbe_dev_interrupt_action(struct rte_eth_dev *dev);
>>>  static void ixgbe_dev_interrupt_handler(struct rte_intr_handle *handle,
>>> @@ -186,11 +188,14 @@ static void ixgbe_dcb_init(struct ixgbe_hw *hw,struct ixgbe_dcb_config *dcb_conf
>>>  /* For Virtual Function support */
>>>  static int eth_ixgbevf_dev_init(struct eth_driver *eth_drv,
>>>  		struct rte_eth_dev *eth_dev);
>>> +static int ixgbevf_dev_interrupt_get_status(struct rte_eth_dev *dev);
>>> +static int ixgbevf_dev_interrupt_action(struct rte_eth_dev *dev);
>>>  static int  ixgbevf_dev_configure(struct rte_eth_dev *dev);
>>>  static int  ixgbevf_dev_start(struct rte_eth_dev *dev);
>>>  static void ixgbevf_dev_stop(struct rte_eth_dev *dev);
>>>  static void ixgbevf_dev_close(struct rte_eth_dev *dev);
>>>  static void ixgbevf_intr_disable(struct ixgbe_hw *hw);
>>> +static void ixgbevf_intr_enable(struct ixgbe_hw *hw);
>>>  static void ixgbevf_dev_stats_get(struct rte_eth_dev *dev,
>>>  		struct rte_eth_stats *stats);
>>>  static void ixgbevf_dev_stats_reset(struct rte_eth_dev *dev);
>>> @@ -198,8 +203,15 @@ static int ixgbevf_vlan_filter_set(struct rte_eth_dev *dev,
>>>  		uint16_t vlan_id, int on);
>>>  static void ixgbevf_vlan_strip_queue_set(struct rte_eth_dev *dev,
>>>  		uint16_t queue, int on);
>>> +static void ixgbevf_set_ivar(struct ixgbe_hw *hw, s8 direction, u8 queue, u8 msix_vector);
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>  static void ixgbevf_vlan_offload_set(struct rte_eth_dev *dev, int mask);
>>>  static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on);
>>> +static void ixgbevf_dev_interrupt_handler(struct rte_intr_handle *handle,
>>> +		void *param);
>>> +static int ixgbevf_dev_rx_queue_intr_enable(struct rte_eth_dev *dev, uint16_t queue_id);
>>> +static int ixgbevf_dev_rx_queue_intr_disable(struct rte_eth_dev *dev, uint16_t queue_id);
>>> +static void ixgbevf_set_ivar(struct ixgbe_hw *hw, s8 direction, u8 queue, u8 msix_vector);
>> Yes re-claim static void ixgbevf_set_ivar()  for twice? Or are they
>> different?
>>
> Good catch.
>
>>> +static void ixgbevf_configure_msix(struct  ixgbe_hw *hw);
>>>
>>>  /* For Eth VMDQ APIs support */
>>>  static int ixgbe_uc_hash_table_set(struct rte_eth_dev *dev, struct
>>> @@ -217,6 +229,11 @@ static int ixgbe_mirror_rule_set(struct rte_eth_dev *dev,
>>>  static int ixgbe_mirror_rule_reset(struct rte_eth_dev *dev,
>>>  		uint8_t	rule_id);
>> [...]
>>> +static void
>>> +ixgbe_configure_msix(struct ixgbe_hw *hw)
>>> +{
>>> +	int queue_id;
>>> +	u32 mask;
>>> +	u32 gpie;
>>> +
>>> +	/* set GPIE for in MSI-x mode */
>>> +	gpie = IXGBE_READ_REG(hw, IXGBE_GPIE);
>>> +	gpie = IXGBE_GPIE_MSIX_MODE | IXGBE_GPIE_PBA_SUPPORT |
>>> +		   IXGBE_GPIE_OCD;
>>> +	gpie |= IXGBE_GPIE_EIAME;
>> As you will override gpie with other flags why need to read the reg and
>> save to gpie first?
>>
>> Maybe read the reg to reset?
>>
>> I guess should be:
>>
>> +	gpie = IXGBE_READ_REG(hw, IXGBE_GPIE);
>> +	gpie |= IXGBE_GPIE_MSIX_MODE | IXGBE_GPIE_PBA_SUPPORT |
>> +		IXGBE_GPIE_OCD | IXGBE_GPIE_EIAME;
>>
>> Maybe not correct as I not familiar with IXGBE.
>>
>>
> Accepted. 
>
>>> +	/*
>>> +	 * use EIAM to auto-mask when MSI-X interrupt is asserted
>>> +	 * this saves a register write for every interrupt
>>> +	 */
>>> +	switch (hw->mac.type) {
>>> +	case ixgbe_mac_82598EB:
>>> +		IXGBE_WRITE_REG(hw, IXGBE_EIAM, IXGBE_EICS_RTX_QUEUE);
>>> +		break;
>>> +	case ixgbe_mac_82599EB:
>>> +	case ixgbe_mac_X540:
>>> +	default:
>>> +		IXGBE_WRITE_REG(hw, IXGBE_EIAM_EX(0), 0xFFFFFFFF);
>>> +		IXGBE_WRITE_REG(hw, IXGBE_EIAM_EX(1), 0xFFFFFFFF);
>>> +		break;
>>> +	}
>>> +	IXGBE_WRITE_REG(hw, IXGBE_GPIE, gpie);
>>> +
>>> +	/*
>>> +	* Populate the IVAR table and set the ITR values to the
>>> +	* corresponding register.
>>> +	*/
>>> +	for (queue_id = 0; queue_id < VFIO_MAX_QUEUE_ID; queue_id++)
>>> +		ixgbe_set_ivar(hw, 0, queue_id, queue_id);
>>> +
>>> +	switch (hw->mac.type) {
>>> +	case ixgbe_mac_82598EB:
>>> +		ixgbe_set_ivar(hw, -1, IXGBE_IVAR_OTHER_CAUSES_INDEX,
>>> +			       VFIO_MAX_QUEUE_ID);
>>> +		break;
>>> +	case ixgbe_mac_82599EB:
>>> +	case ixgbe_mac_X540:
>>> +		ixgbe_set_ivar(hw, -1, 1, 32);
>> May be better to make those values for a macro just as above.
>>
> To be fixed in V2.
>
>>> +		break;
>>> +	default:
>>> +		break;
>>> +	}
>>> +	IXGBE_WRITE_REG(hw, IXGBE_EITR(queue_id), 1950);
>> Also here, what's "1950" stands for?
>>
> It is an experienced interrupt throttle value which is used for interrupt support

Then, I think it is better to define it in a macro and do some brief
introduction on this value.

Thanks,
Michael
>>> +
>>> +	/* set up to autoclear timer, and the vectors */
>>> +	mask = IXGBE_EIMS_ENABLE_MASK;
>>> +	mask &= ~(IXGBE_EIMS_OTHER |
>>> +		  IXGBE_EIMS_MAILBOX |
>>> +		  IXGBE_EIMS_LSC);
>>> +
>>> +	IXGBE_WRITE_REG(hw, IXGBE_EIAC, mask);
>>> +}
>>> +
>>>  static int ixgbe_set_queue_rate_limit(struct rte_eth_dev *dev,
>>>  	uint16_t queue_idx, uint16_t tx_rate)
>>>  {
>>> diff --git a/lib/librte_pmd_ixgbe/ixgbe_ethdev.h b/lib/librte_pmd_ixgbe/ixgbe_ethdev.h
>>> index 1383194..328c387 100644
>>> --- a/lib/librte_pmd_ixgbe/ixgbe_ethdev.h
>>> +++ b/lib/librte_pmd_ixgbe/ixgbe_ethdev.h
>>> @@ -38,6 +38,8 @@
>>>  #include "ixgbe/ixgbe_dcb_82598.h"
>>>  #include "ixgbe_bypass.h"
>>>
>>> +#include <rte_spinlock.h>
>>> +
>>>  /* need update link, bit flag */
>>>  #define IXGBE_FLAG_NEED_LINK_UPDATE (uint32_t)(1 << 0)
>>>  #define IXGBE_FLAG_MAILBOX          (uint32_t)(1 << 1)
>>> @@ -98,6 +100,11 @@
>>>  #define IXGBE_5TUPLE_MAX_PRI            7
>>>  #define IXGBE_5TUPLE_MIN_PRI            1
>>>
>>> +#define IXGBE_VF_IRQ_ENABLE_MASK        3          /* vf interrupt enable mask */
>>> +#define IXGBE_VF_MAXMSIVECTOR			1
>>> +/* maximum other interrupts besides rx&tx*/
>>> +#define IXGBE_MAX_OTHER_INTR		1
>>> +#define IXGBEVF_MAX_OTHER_INTR		1
>>>  /*
>>>   * Information about the fdir mode.
>>>   */
>>> @@ -116,6 +123,7 @@ struct ixgbe_hw_fdir_info {
>>>  struct ixgbe_interrupt {
>>>  	uint32_t flags;
>>>  	uint32_t mask;
>>> +	rte_spinlock_t lock;
>>>  };
>>>
>>>  struct ixgbe_stat_mapping_registers {
>>> @@ -260,6 +268,7 @@ uint32_t ixgbe_dev_rx_queue_count(struct rte_eth_dev *dev,
>>>  		uint16_t rx_queue_id);
>>>
>>>  int ixgbe_dev_rx_descriptor_done(void *rx_queue, uint16_t offset);
>>> +int ixgbevf_dev_rx_descriptor_done(void *rx_queue, uint16_t offset);
>>>
>>>  int ixgbe_dev_rx_init(struct rte_eth_dev *dev);
>>>
>