[PATCH v11 03/18] net/idpf: add Tx queue setup

[Xing, Beilei]

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko at oktetlabs.ru>
> Sent: Tuesday, October 25, 2022 5:40 PM
> To: Guo, Junfeng <junfeng.guo at intel.com>; Zhang, Qi Z
> <qi.z.zhang at intel.com>; Wu, Jingjing <jingjing.wu at intel.com>; Xing, Beilei
> <beilei.xing at intel.com>
> Cc: dev at dpdk.org; Li, Xiaoyun <xiaoyun.li at intel.com>
> Subject: Re: [PATCH v11 03/18] net/idpf: add Tx queue setup
> 
> On 10/24/22 16:12, Junfeng Guo wrote:
> > Add support for tx_queue_setup ops.
> >
> > In the single queue model, the same descriptor queue is used by SW to
> > post buffer descriptors to HW and by HW to post completed descriptors
> > to SW.
> >
> > In the split queue model, "RX buffer queues" are used to pass
> > descriptor buffers from SW to HW while Rx queues are used only to pass
> > the descriptor completions, that is, descriptors that point to
> > completed buffers, from HW to SW. This is contrary to the single queue
> > model in which Rx queues are used for both purposes.
> >
> > Signed-off-by: Beilei Xing <beilei.xing at intel.com>
> > Signed-off-by: Xiaoyun Li <xiaoyun.li at intel.com>
> > Signed-off-by: Junfeng Guo <junfeng.guo at intel.com>
> 
> > +
> > +	size = sizeof(struct idpf_flex_tx_sched_desc) * txq->nb_tx_desc;
> > +	for (i = 0; i < size; i++)
> > +		((volatile char *)txq->desc_ring)[i] = 0;
> 
> Please, add a comment which explains why volatile is required here.

Volatile is used here to make sure the memory write through when touch desc.
It follows Intel PMD's coding style.

> 
> > +
> > +	txq->tx_ring_phys_addr = mz->iova;
> > +	txq->tx_ring = (struct idpf_flex_tx_desc *)mz->addr;
> > +
> > +	txq->mz = mz;
> > +	reset_single_tx_queue(txq);
> > +	txq->q_set = true;
> > +	dev->data->tx_queues[queue_idx] = txq;
> > +	txq->qtx_tail = hw->hw_addr + (vport->chunks_info.tx_qtail_start +
> > +			queue_idx * vport->chunks_info.tx_qtail_spacing);
> 
> I'm sorry, but it looks like too much code is duplicated.
> I guess it could be a shared function to avoid it.

It makes sense to optimize the queue setup, but can we improve it in next release
due to the RC2 deadline? 
And this part should be common module shared by idpf PMD and a new PMD,
It should be in driver/common/idpf folder in DPDK 23.03.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +int
> > +idpf_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
> > +		    uint16_t nb_desc, unsigned int socket_id,
> > +		    const struct rte_eth_txconf *tx_conf) {
> > +	struct idpf_vport *vport = dev->data->dev_private;
> > +
> > +	if (vport->txq_model == VIRTCHNL2_QUEUE_MODEL_SINGLE)
> > +		return idpf_tx_single_queue_setup(dev, queue_idx, nb_desc,
> > +						  socket_id, tx_conf);
> > +	else
> > +		return idpf_tx_split_queue_setup(dev, queue_idx, nb_desc,
> > +						 socket_id, tx_conf);
> > +}
> 
> [snip]