Re: [dpdk-dev] [PATCH 04/12] net/iavf: flexible Rx descriptor support in normal path

["Wu, Jingjing" <jingjing.wu@intel.com>]

One general comment:
Looks like there are exact same code comparing with legacy rx, such as logic to update tail, multi-segments loop.
It will be good if all the common code can be wrapped and use for multi recv functions

[...]

+/* Get the number of used descriptors of a rx queue for flexible RXD */ 
+uint32_t iavf_dev_rxq_count_flex_rxd(struct rte_eth_dev *dev, uint16_t 
+queue_id) { #define IAVF_RXQ_SCAN_INTERVAL 4
+	volatile union iavf_rx_flex_desc *rxdp;
+	struct iavf_rx_queue *rxq;
+	uint16_t desc = 0;
+
+	rxq = dev->data->rx_queues[queue_id];
+	rxdp = (volatile union iavf_rx_flex_desc *)&rxq->rx_ring[rxq->rx_tail];
+	while ((desc < rxq->nb_rx_desc) &&
+	       rte_le_to_cpu_16(rxdp->wb.status_error0) &
+	       (1 << IAVF_RX_FLEX_DESC_STATUS0_DD_S)) {
+		/* Check the DD bit of a rx descriptor of each 4 in a group,
+		 * to avoid checking too frequently and downgrading performance
+		 * too much.
+		 */
+		desc += IAVF_RXQ_SCAN_INTERVAL;
+		rxdp += IAVF_RXQ_SCAN_INTERVAL;
+		if (rxq->rx_tail + desc >= rxq->nb_rx_desc)
+			rxdp = (volatile union iavf_rx_flex_desc *)
+				&(rxq->rx_ring[rxq->rx_tail +
+					desc - rxq->nb_rx_desc]);
+	}
+
+	return desc;
+}

No much difference between iavf_dev_rxq_count. Why do we need a new one? DD bit is located in the same place, right?
Can we merge those two functions to one?  
+
 /* Get the number of used descriptors of a rx queue */  uint32_t  iavf_dev_rxq_count(struct rte_eth_dev *dev, uint16_t queue_id) @@ -1795,6 +2264,10 @@ iavf_dev_rxq_count(struct rte_eth_dev *dev, uint16_t queue_id)
 
 	rxq = dev->data->rx_queues[queue_id];
 	rxdp = &rxq->rx_ring[rxq->rx_tail];
+
+	if (rxq->rxdid == IAVF_RXDID_COMMS_OVS_1)
+		return iavf_dev_rxq_count_flex_rxd(dev, queue_id);
+
 	while ((desc < rxq->nb_rx_desc) &&
 	       ((rte_le_to_cpu_64(rxdp->wb.qword1.status_error_len) &
 		 IAVF_RXD_QW1_STATUS_MASK) >> IAVF_RXD_QW1_STATUS_SHIFT) & @@ -1813,6 +2286,31 @@ iavf_dev_rxq_count(struct rte_eth_dev *dev, uint16_t queue_id)
 	return desc;
 }
 
+int
+iavf_dev_rx_desc_status_flex_rxd(void *rx_queue, uint16_t offset) {
+	volatile union iavf_rx_flex_desc *rxdp;
+	struct iavf_rx_queue *rxq = rx_queue;
+	uint32_t desc;
+
+	if (unlikely(offset >= rxq->nb_rx_desc))
+		return -EINVAL;
+
+	if (offset >= rxq->nb_rx_desc - rxq->nb_rx_hold)
+		return RTE_ETH_RX_DESC_UNAVAIL;
+
+	desc = rxq->rx_tail + offset;
+	if (desc >= rxq->nb_rx_desc)
+		desc -= rxq->nb_rx_desc;
+
+	rxdp = (volatile union iavf_rx_flex_desc *)&rxq->rx_ring[desc];
+	if (rte_le_to_cpu_16(rxdp->wb.status_error0) &
+		(1 << IAVF_RX_FLEX_DESC_STATUS0_DD_S))
+		return RTE_ETH_RX_DESC_DONE;
+
+	return RTE_ETH_RX_DESC_AVAIL;
+}
+

Similar comments as above.

[......] 

  { @@ -569,6 +596,20 @@ iavf_configure_queues(struct iavf_adapter *adapter)
 			vc_qp->rxq.ring_len = rxq[i]->nb_rx_desc;
 			vc_qp->rxq.dma_ring_addr = rxq[i]->rx_ring_phys_addr;
 			vc_qp->rxq.databuffer_size = rxq[i]->rx_buf_len;
+
+			if (vf->vf_res->vf_cap_flags &
+			    VIRTCHNL_VF_OFFLOAD_RX_FLEX_DESC &&
+			    vf->supported_rxdid & BIT(IAVF_RXDID_COMMS_OVS_1)) {
+				vc_qp->rxq.rxdid = IAVF_RXDID_COMMS_OVS_1;
+				rxq[i]->rxdid = IAVF_RXDID_COMMS_OVS_1;
Because this function is used to construct virtchnl message for configure queues.
The rxdid in rxq[i] should be set before this function is called. How about to move this line assignment to iavf_dev_rx_queue_setup?
+				PMD_DRV_LOG(NOTICE, "request RXDID == %d in "
+					    "Queue[%d]", vc_qp->rxq.rxdid, i);
+			} else {
+				vc_qp->rxq.rxdid = IAVF_RXDID_LEGACY_1;
+				rxq[i]->rxdid = IAVF_RXDID_LEGACY_1;
Same as above.

+				PMD_DRV_LOG(NOTICE, "request RXDID == %d in "
+					    "Queue[%d]", vc_qp->rxq.rxdid, i);
+			}
 		}
 	}
 
--
2.17.1