> -----Original Message-----
> From: Qiu, Michael
> Sent: Thursday, January 29, 2015 11:40 AM
> To: Zhou, Danny; dev at 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 at 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
> > +
> > + /* 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);
> >
