[dpdk-dev] [PATCH v5 1/3] net/i40e: queue region set and flush

[Wu, Jingjing]

> +static int
> +i40e_vsi_update_queue_region_mapping(struct i40e_hw *hw,
> +			      struct i40e_pf *pf)
> +{
> +	uint16_t i;
> +	struct i40e_vsi *vsi = pf->main_vsi;
> +	uint16_t queue_offset, bsf, tc_index;
> +	struct i40e_vsi_context ctxt;
> +	struct i40e_aqc_vsi_properties_data *vsi_info;
> +	struct i40e_queue_region_info *region_info =
> +				&pf->queue_region;
> +	int32_t ret = -EINVAL;
> +
> +	if (!region_info->queue_region_number) {
> +		PMD_INIT_LOG(ERR, "there is no that region id been set before");
> +		return ret;
> +	}
> +
> +	memset(&ctxt, 0, sizeof(struct i40e_vsi_context));
> +
> +	/* Update Queue Pairs Mapping for currently enabled UPs */
> +	ctxt.seid = vsi->seid;
> +	ctxt.pf_num = hw->pf_id;
> +	ctxt.vf_num = 0;
> +	ctxt.uplink_seid = vsi->uplink_seid;
> +	ctxt.info = vsi->info;
> +	vsi_info = &ctxt.info;
> +
> +	memset(vsi_info->tc_mapping, 0, sizeof(uint16_t) * 8);
> +	memset(vsi_info->queue_mapping, 0, sizeof(uint16_t) * 16);
> +
> +	/**
> +	 * Configure queue region and queue mapping parameters,
> +	 * for enabled queue region, allocate queues to this region.
> +	 */
> +
> +	for (i = 0; i < region_info->queue_region_number; i++) {
> +		tc_index = region_info->region[i].region_id;
> +		bsf = rte_bsf32(region_info->region[i].queue_num);
> +		queue_offset = region_info->region[i].queue_start_index;
> +		vsi_info->tc_mapping[tc_index] = rte_cpu_to_le_16(
> +			(queue_offset << I40E_AQ_VSI_TC_QUE_OFFSET_SHIFT) |
> +				(bsf << I40E_AQ_VSI_TC_QUE_NUMBER_SHIFT));
> +	}
> +
> +	/* Associate queue number with VSI, Keep vsi->nb_qps unchanged */
> +	if (vsi->type == I40E_VSI_SRIOV) {
You already assign the vsi to main_vsi in the beginning of the function, so
It is impossible here the type is SRIOV. I think I already commented it in your v3 patch set?
 
[......]
> +static int
> +i40e_queue_region_set_region(struct i40e_pf *pf,
> +				struct rte_i40e_rss_region_conf *conf_ptr)
> +{
> +	uint16_t i;
> +	struct i40e_vsi *main_vsi = pf->main_vsi;
> +	struct i40e_queue_region_info *info = &pf->queue_region;
> +	int32_t ret = -EINVAL;
> +
> +	if (!((rte_is_power_of_2(conf_ptr->queue_num)) &&
> +				conf_ptr->queue_num <= 64)) {
> +		PMD_DRV_LOG(ERR, "The region sizes should be any of the following values: 1,
> 2, 4, 8, 16, 32, 64 as long as the "
> +			"total number of queues do not exceed the VSI allocation");
> +		return ret;
> +	}
> +
> +	if (conf_ptr->region_id > I40E_REGION_MAX_INDEX) {
> +		PMD_DRV_LOG(ERR, "the queue region max index is 7");
> +		return ret;
> +	}
> +
> +	if ((conf_ptr->queue_start_index + conf_ptr->queue_num)
> +					> main_vsi->nb_used_qps) {
> +		PMD_DRV_LOG(ERR, "the queue index exceeds the VSI range");
> +		return ret;
> +	}
> +
You are using nb_used_qps for the comparison, is that to say the function
Is supposed to be called after dev_start?

> +	for (i = 0; i < info->queue_region_number; i++)
> +		if (conf_ptr->region_id == info->region[i].region_id)
> +			break;
> +
> +	if (i == info->queue_region_number &&
> +				i <= I40E_REGION_MAX_INDEX) {
> +		info->region[i].region_id = conf_ptr->region_id;
> +		info->region[i].queue_num = conf_ptr->queue_num;
> +		info->region[i].queue_start_index =
> +			conf_ptr->queue_start_index;
> +		info->queue_region_number++;
> +	} else {
> +		PMD_DRV_LOG(ERR, "queue region number exceeds maxnum 8 or the queue
> region id has been set before");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +i40e_queue_region_set_flowtype(struct i40e_pf *pf,
> +			struct rte_i40e_rss_region_conf *rss_region_conf)
> +{
> +	int32_t ret = -EINVAL;
> +	struct i40e_queue_region_info *info = &pf->queue_region;
> +	uint16_t i, j, flowtype_set = 0;
> +	uint16_t region_index, flowtype_index;
> +
> +	/**
> +	 * For the pctype or hardware flowtype of packet,
> +	 * the specific index for each type has been defined
> +	 * in file i40e_type.h as enum i40e_filter_pctype.
> +	 */
> +
Are you meeing the type of hw_flowtype in rss_region_conf
Is defined in i40e_type.h?

> +	if (rss_region_conf->region_id > I40E_PFQF_HREGION_MAX_INDEX) {
> +		PMD_DRV_LOG(ERR, "the queue region max index is 7");
> +		return ret;
> +	}
> +
> +	if (rss_region_conf->hw_flowtype >= I40E_FILTER_PCTYPE_MAX) {
> +		PMD_DRV_LOG(ERR, "the hw_flowtype or PCTYPE max index is 63");
> +		return ret;
> +	}
> +
> +
> +	for (i = 0; i < info->queue_region_number; i++)
> +		if (rss_region_conf->region_id == info->region[i].region_id)
> +			break;
> +
> +	if (i == info->queue_region_number) {
> +		PMD_DRV_LOG(ERR, "that region id has not been set before");
> +		ret = -ENODATA;
> +		return ret;
> +	}
> +	region_index = i;
> +
> +	for (i = 0; i < info->queue_region_number; i++) {
> +		for (j = 0; j < info->region[i].flowtype_num; j++) {
> +			if (rss_region_conf->hw_flowtype ==
> +				info->region[i].hw_flowtype[j]) {
> +				flowtype_set = 1;
> +				break;
You can just return here?

> +			}
> +		}
> +	}
> +

 [......]
> +static int
> +i40e_queue_region_set_user_priority(struct i40e_pf *pf,
> +		struct rte_i40e_rss_region_conf *rss_region_conf)
> +{
> +	struct i40e_queue_region_info *info = &pf->queue_region;
> +	int32_t ret = -EINVAL;
> +	uint16_t i, j, region_index, user_priority_set = 0;
> +
> +	if (rss_region_conf->user_priority >= I40E_MAX_USER_PRIORITY) {
> +		PMD_DRV_LOG(ERR, "the queue region max index is 7");
> +		return ret;
> +	}
> +
> +	if (rss_region_conf->region_id >= I40E_REGION_MAX_INDEX) {
> +		PMD_DRV_LOG(ERR, "the region_id max index is 7");
> +		return ret;
> +	}
> +
> +	for (i = 0; i < info->queue_region_number; i++)
> +		if (rss_region_conf->region_id == info->region[i].region_id)
> +			break;
> +
> +	if (i == info->queue_region_number) {
> +		PMD_DRV_LOG(ERR, "that region id has not been set before");
> +		ret = -ENODATA;
> +		return ret;
> +	}
> +	region_index = i;
> +
> +	for (i = 0; i < info->queue_region_number; i++) {
> +		for (j = 0; j < info->region[i].user_priority_num; j++) {
> +			if (info->region[i].user_priority[j] ==
> +				rss_region_conf->user_priority) {
> +				user_priority_set = 1;
The same, you can return here.
> +				break;

[......] 
> +
> +static int
> +i40e_queue_region_dcb_configure(struct i40e_hw *hw,
> +				struct i40e_pf *pf)
> +{
> +	struct i40e_dcbx_config dcb_cfg_local;
> +	struct i40e_dcbx_config *dcb_cfg;
> +	struct i40e_queue_region_info *info = &pf->queue_region;
> +	struct i40e_dcbx_config *old_cfg = &hw->local_dcbx_config;
> +	int32_t ret = -EINVAL;
> +	uint16_t i, j, prio_index, region_index;
> +	uint8_t tc_map, tc_bw, bw_lf;
> +
> +	if (!info->queue_region_number) {
> +		PMD_DRV_LOG(ERR, "there is no that region id been set before");
> +		return ret;
> +	}
> +
> +	dcb_cfg = &dcb_cfg_local;
> +	memset(dcb_cfg, 0, sizeof(struct i40e_dcbx_config));
> +
> +	/* assume each tc has the same bw */
> +	tc_bw = I40E_MAX_PERCENT / info->queue_region_number;
> +	for (i = 0; i < info->queue_region_number; i++)
> +		dcb_cfg->etscfg.tcbwtable[i] = tc_bw;
> +	/* to ensure the sum of tcbw is equal to 100 */
> +	bw_lf = I40E_MAX_PERCENT %  info->queue_region_number;
> +	for (i = 0; i < bw_lf; i++)
> +		dcb_cfg->etscfg.tcbwtable[i]++;
> +
> +	/* assume each tc has the same Transmission Selection Algorithm */
> +	for (i = 0; i < info->queue_region_number; i++)
> +		dcb_cfg->etscfg.tsatable[i] = I40E_IEEE_TSA_ETS;
> +
> +	for (i = 0; i < info->queue_region_number; i++) {
> +		for (j = 0; j < info->region[i].user_priority_num; j++) {
> +			prio_index = info->region[i].user_priority[j];
> +			region_index = info->region[i].region_id;
> +			dcb_cfg->etscfg.prioritytable[prio_index] =
> +						region_index;
> +		}
> +	}
> +
> +	/* FW needs one App to configure HW */
> +	dcb_cfg->numapps = I40E_DEFAULT_DCB_APP_NUM;
> +	dcb_cfg->app[0].selector = I40E_APP_SEL_ETHTYPE;
> +	dcb_cfg->app[0].priority = I40E_DEFAULT_DCB_APP_PRIO;
> +	dcb_cfg->app[0].protocolid = I40E_APP_PROTOID_FCOE;
> +
> +	tc_map = RTE_LEN2MASK(info->queue_region_number, uint8_t);
> +
> +	dcb_cfg->pfc.willing = 0;
> +	dcb_cfg->pfc.pfccap = I40E_MAX_TRAFFIC_CLASS;
> +	dcb_cfg->pfc.pfcenable = tc_map;
> +
> +	/* Copy the new config to the current config */
> +	*old_cfg = *dcb_cfg;
> +	old_cfg->etsrec = old_cfg->etscfg;
> +	ret = i40e_set_dcb_config(hw);
> +
> +	if (ret) {
> +		PMD_DRV_LOG(ERR, "Set queue region DCB Config failed, err %s aq_err %s",
> +			 i40e_stat_str(hw, ret),
> +			 i40e_aq_str(hw, hw->aq.asq_last_status));
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +int
> +i40e_flush_queue_region_all_conf(struct rte_eth_dev *dev,
> +	struct i40e_hw *hw, struct i40e_pf *pf, uint16_t on)
> +{
> +	int32_t ret = -EINVAL;
> +	struct i40e_queue_region_info *info = &pf->queue_region;
> +
> +	if (on) {
> +		i40e_queue_region_pf_flowtype_conf(hw, pf);
> +
> +		ret = i40e_vsi_update_queue_region_mapping(hw, pf);
> +		if (ret != I40E_SUCCESS) {
> +			PMD_DRV_LOG(INFO, "Failed to flush queue region mapping.");
> +			return ret;
> +		}
> +
> +		ret = i40e_queue_region_dcb_configure(hw, pf);
> +		if (ret != I40E_SUCCESS) {
> +			PMD_DRV_LOG(INFO, "Failed to flush dcb.");
> +			return ret;
> +		}
> +
> +		return 0;
> +	}
> +
> +	info->queue_region_number = 1;
> +	info->region[0].queue_num = 64;
> +	info->region[0].queue_start_index = 0;
> +
> +	ret = i40e_vsi_update_queue_region_mapping(hw, pf);
> +	if (ret != I40E_SUCCESS)
> +		PMD_DRV_LOG(INFO, "Failed to flush queue region mapping.");
> +
> +	ret = i40e_dcb_init_configure(dev, TRUE);
> +	if (ret != I40E_SUCCESS) {
> +		PMD_DRV_LOG(INFO, "Failed to flush dcb.");
> +		pf->flags &= ~I40E_FLAG_DCB;
> +	}
> +
> +	i40e_init_queue_region_conf(dev);
> +
> +	return 0;
> +}
> +
> +static int
> +i40e_queue_region_pf_check_rss(struct i40e_pf *pf)
> +{
> +	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
> +	uint64_t hena;
> +
> +	hena = (uint64_t)i40e_read_rx_ctl(hw, I40E_PFQF_HENA(0));
> +	hena |= ((uint64_t)i40e_read_rx_ctl(hw, I40E_PFQF_HENA(1))) << 32;
> +	if (hw->mac.type == I40E_MAC_X722)
> +		hena &= I40E_RSS_HENA_ALL_X722;
> +	else
> +		hena &= I40E_RSS_HENA_ALL;
> +
> +	if (!hena)
> +		return -ENOTSUP;
> +
Why made such change? Will it be impacted by kiril's pctype patches and beilei's new pctype patches?

> +	return 0;
> +}
> +
> +static void
> +i40e_queue_region_get_all_info(struct i40e_pf *pf, uint16_t port_id)
> +{
In this function, it looks like you print all the information about queue region?
If so, You can use "display" instead of "get".
In my opinion, generally, we are getting info but not display info in API level.

[......]
> +
> +/**
> + *  Option types of queue region.
> + */
> +enum rte_pmd_i40e_queue_region_op {
> +	RTE_PMD_I40E_REGION_UNDEFINED,
> +	RTE_PMD_I40E_QUEUE_REGION_SET,      /**< add queue region set */
> +	RTE_PMD_I40E_REGION_FLOWTYPE_SET,   /**< add pf region pctype set */
> +	/**< add queue region user priority set */
If the comments is added before the definition, you need to use /** xxx */ instead of /**< XX */
for doxygen-likely format you can check http://www.dpdk.org/doc/guides/contributing/documentation.html#doxygen-guidelines

Thanks
Jingjing