[dpdk-dev] [PATCH 2/5] fm10k: enable Rx queue interrupts for PF and VF

[He, Shaopeng]

Hi, Mark

Thanks for the comments, please find my reply inline.

Thanks,
--Shaopeng
> -----Original Message-----
> From: Chen, Jing D
> Sent: Thursday, October 22, 2015 2:52 PM
> To: He, Shaopeng; dev at dpdk.org
> Subject: RE: [dpdk-dev] [PATCH 2/5] fm10k: enable Rx queue interrupts for
> PF and VF
> 
> Hi,
> 
> Best Regards,
> Mark
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Shaopeng He
> > Sent: Friday, September 25, 2015 1:37 PM
> > To: dev at dpdk.org
> > Subject: [dpdk-dev] [PATCH 2/5] fm10k: enable Rx queue interrupts for
> > PF and VF
> >
> > The patch does below things for fm10k PF and VF:
> > - Setup NIC to generate MSI-X interrupts
> > - Set the RXINT register to map interrupt causes to vectors
> > - Implement interrupt enable/disable functions
> 
> The description is too simple, can you help to extend?
> Besides that, there are complicated changes in this patch.
> Can you help you split it to several smaller ones for better understanding?
Sure, I will send v2 to split into smaller ones.

> 
> >
> > Signed-off-by: Shaopeng He <shaopeng.he at intel.com>
> > ---
> >  drivers/net/fm10k/fm10k_ethdev.c | 147
> > +++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 140 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/fm10k/fm10k_ethdev.c
> > b/drivers/net/fm10k/fm10k_ethdev.c
> > index a82cd59..6648934 100644
> > --- a/drivers/net/fm10k/fm10k_ethdev.c
> > +++ b/drivers/net/fm10k/fm10k_ethdev.c
> >
> >  static int
> > +fm10k_dev_rx_queue_intr_enable(struct rte_eth_dev *dev, uint16_t
> > queue_id)
> > +{
> > +	struct fm10k_hw *hw = FM10K_DEV_PRIVATE_TO_HW(dev->data-
> > >dev_private);
> > +
> > +	/* Enable ITR */
> > +	if (hw->mac.type == fm10k_mac_pf)
> > +		FM10K_WRITE_REG(hw, FM10K_ITR(Q2V(dev, queue_id)),
> > +			FM10K_ITR_AUTOMASK |
> > FM10K_ITR_MASK_CLEAR);
> > +	else
> > +		FM10K_WRITE_REG(hw, FM10K_VFITR(Q2V(dev, queue_id)),
> > +			FM10K_ITR_AUTOMASK |
> > FM10K_ITR_MASK_CLEAR);
> > +	rte_intr_enable(&dev->pci_dev->intr_handle);
> > +	return 0;
> > +}
> > +
> > +static int
> > +fm10k_dev_rx_queue_intr_disable(struct rte_eth_dev *dev, uint16_t
> > queue_id)
> > +{
> > +	struct fm10k_hw *hw = FM10K_DEV_PRIVATE_TO_HW(dev->data-
> > >dev_private);
> > +
> > +	/* Disable ITR */
> > +	if (hw->mac.type == fm10k_mac_pf)
> > +		FM10K_WRITE_REG(hw, FM10K_ITR(Q2V(dev, queue_id)),
> > +			FM10K_ITR_MASK_SET);
> > +	else
> > +		FM10K_WRITE_REG(hw, FM10K_VFITR(Q2V(dev, queue_id)),
> > +			FM10K_ITR_MASK_SET);
> 
> In previous enable function, you'll use rte_intr_enable() to enable interrupt,
> but You needn't disable it in this function?
rte_intr_enable() is needed in fm10k_dev_rx_queue_intr_enable to re-enable
the interrupt in port level. To disable individual queue interrupt, currently only 
need to disable the hardware side by register setting.

> 
> > +	return 0;
> > +}
> > +
> > +static int
> > +fm10k_dev_rxq_interrupt_setup(struct rte_eth_dev *dev) {
> > +	struct fm10k_hw *hw = FM10K_DEV_PRIVATE_TO_HW(dev->data-
> > >dev_private);
> > +	struct rte_intr_handle *intr_handle = &dev->pci_dev->intr_handle;
> > +	uint32_t intr_vector, vec;
> > +	uint16_t queue_id;
> > +	int result = 0;
> > +
> > +	/* fm10k needs interrupt for mailbox
> > +	 * so igb_uio is not supported for rx interrupt
> > +	 */
> 
> I guess you'll support both igb_uio and VFIO, RX interrupt mode only enabled
> in case VFIO is used.
> I suggest you add more comments here for better understanding.
You are right, current igb_uio only has one interrupt vector, and mailbox
needs that, so igb_uio cannot support RX interrupt mode. I will add more
comments here in next version.

> 
> > +	if (!rte_intr_cap_multiple(intr_handle) ||
> > +			dev->data->dev_conf.intr_conf.rxq == 0)
> > +		return result;
> > +
> > +	intr_vector = dev->data->nb_rx_queues;
> > +
> > +	/* disable interrupt first */
> > +	rte_intr_disable(&dev->pci_dev->intr_handle);
> > +	if (hw->mac.type == fm10k_mac_pf)
> > +		fm10k_dev_disable_intr_pf(dev);
> > +	else
> > +		fm10k_dev_disable_intr_vf(dev);
> > +
> > +	if (rte_intr_efd_enable(intr_handle, intr_vector)) {
> > +		PMD_INIT_LOG(ERR, "Failed to init event fd");
> > +		result = -EIO;
> > +	}
> > +
> > +	if (rte_intr_dp_is_en(intr_handle) && !result) {
> > +		intr_handle->intr_vec =	rte_zmalloc("intr_vec",
> > +			dev->data->nb_rx_queues * sizeof(int), 0);
> > +		if (intr_handle->intr_vec) {
> > +			for (queue_id = 0, vec = RX_VEC_START;
> > +					queue_id < dev->data-
> > >nb_rx_queues;
> > +					queue_id++) {
> > +				intr_handle->intr_vec[queue_id] = vec;
> > +				if (vec < intr_handle->nb_efd - 1 +
> > RX_VEC_START)
> > +					vec++;
> 
> No "else" to handle exceptional case?
For the "else" case, when all available efd (event fd) are used, currently we
just reuse the last efd, keep the same value for "vec", so no "else" is needed.

> 
> > +			}
> > +		} else {
> > +			PMD_INIT_LOG(ERR, "Failed to allocate %d
> > rx_queues"
> > +				" intr_vec", dev->data->nb_rx_queues);
> > +			result = -ENOMEM;
> > +		}
> > +	}
> > +
> > +	if (hw->mac.type == fm10k_mac_pf)
> > +		fm10k_dev_enable_intr_pf(dev);
> > +	else
> > +		fm10k_dev_enable_intr_vf(dev);
> > +	rte_intr_enable(&dev->pci_dev->intr_handle);
> > +	hw->mac.ops.update_int_moderator(hw);
> > +	return result;
> > +}
> > +
> > +static int
> >  fm10k_dev_handle_fault(struct fm10k_hw *hw, uint32_t eicr)  {
> >  	struct fm10k_fault fault;
> > @@ -2050,6 +2181,8 @@ static const struct eth_dev_ops
> > fm10k_eth_dev_ops = {
> >  	.tx_queue_setup		= fm10k_tx_queue_setup,
> >  	.tx_queue_release	= fm10k_tx_queue_release,
> >  	.rx_descriptor_done	= fm10k_dev_rx_descriptor_done,
> > +	.rx_queue_intr_enable	= fm10k_dev_rx_queue_intr_enable,
> > +	.rx_queue_intr_disable	= fm10k_dev_rx_queue_intr_disable,
> >  	.reta_update		= fm10k_reta_update,
> >  	.reta_query		= fm10k_reta_query,
> >  	.rss_hash_update	= fm10k_rss_hash_update,
> > @@ -2060,7 +2193,7 @@ static int
> >  eth_fm10k_dev_init(struct rte_eth_dev *dev)  {
> >  	struct fm10k_hw *hw = FM10K_DEV_PRIVATE_TO_HW(dev->data-
> > >dev_private);
> > -	int diag;
> > +	int diag, i;
> >  	struct fm10k_macvlan_filter_info *macvlan;
> >
> >  	PMD_INIT_FUNC_TRACE();
> > @@ -2177,7 +2310,7 @@ eth_fm10k_dev_init(struct rte_eth_dev *dev)
> >  		fm10k_dev_enable_intr_vf(dev);
> >  	}
> >
> > -	/* Enable uio intr after callback registered */
> > +	/* Enable intr after callback registered */
> >  	rte_intr_enable(&(dev->pci_dev->intr_handle));
> >
> >  	hw->mac.ops.update_int_moderator(hw);
> > @@ -2185,7 +2318,6 @@ eth_fm10k_dev_init(struct rte_eth_dev *dev)
> >  	/* Make sure Switch Manager is ready before going forward. */
> >  	if (hw->mac.type == fm10k_mac_pf) {
> >  		int switch_ready = 0;
> > -		int i;
> >
> >  		for (i = 0; i < MAX_QUERY_SWITCH_STATE_TIMES; i++) {
> >  			fm10k_mbx_lock(hw);
> > @@ -2291,7 +2423,8 @@ static struct eth_driver rte_pmd_fm10k = {
> >  	.pci_drv = {
> >  		.name = "rte_pmd_fm10k",
> >  		.id_table = pci_id_fm10k_map,
> > -		.drv_flags = RTE_PCI_DRV_NEED_MAPPING |
> > RTE_PCI_DRV_DETACHABLE,
> > +		.drv_flags = RTE_PCI_DRV_NEED_MAPPING |
> > RTE_PCI_DRV_INTR_LSC |
> > +			RTE_PCI_DRV_DETACHABLE,
> >  	},
> >  	.eth_dev_init = eth_fm10k_dev_init,
> >  	.eth_dev_uninit = eth_fm10k_dev_uninit,
> > --
> > 1.9.3