Re: [dpdk-dev] [PATCH 08/22] event/dlb2: add infos get and configure

["Eads, Gage" <gage.eads@intel.com>]

> -----Original Message-----
> From: McDaniel, Timothy <timothy.mcdaniel@intel.com>
> Sent: Friday, September 11, 2020 3:26 PM
> Cc: dev@dpdk.org; Carrillo, Erik G <erik.g.carrillo@intel.com>; Eads, Gage
> <gage.eads@intel.com>; Van Haaren, Harry <harry.van.haaren@intel.com>;
> jerinj@marvell.com
> Subject: [PATCH 08/22] event/dlb2: add infos get and configure
> 
> Add support for configuring the DLB hardware.

This is a large patch, even if most of the code is in the low-level/hardware-specific
base directory, and could use more explanation in the commit message.

> 
> Signed-off-by: Timothy McDaniel <timothy.mcdaniel@intel.com>
> ---
>  drivers/event/dlb2/dlb2.c                  |  334 +++
>  drivers/event/dlb2/dlb2_iface.c            |    7 +-
>  drivers/event/dlb2/dlb2_iface.h            |    5 +
>  drivers/event/dlb2/pf/base/dlb2_resource.c | 3234
> ++++++++++++++++++++++++++++
>  drivers/event/dlb2/pf/dlb2_main.c          |   14 +
>  drivers/event/dlb2/pf/dlb2_pf.c            |   44 +
>  6 files changed, 3637 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/event/dlb2/dlb2.c b/drivers/event/dlb2/dlb2.c
> index 0d6fea4..58e953b 100644
> --- a/drivers/event/dlb2/dlb2.c
> +++ b/drivers/event/dlb2/dlb2.c
> @@ -92,6 +92,28 @@ dlb2_get_queue_depth(struct dlb2_eventdev *dlb2,
>  	return 0;
>  }
> 
> +static void
> +dlb2_free_qe_mem(struct dlb2_port *qm_port)
> +{
> +	if (qm_port == NULL)
> +		return;
> +
> +	if (qm_port->qe4) {
> +		rte_free(qm_port->qe4);
> +		qm_port->qe4 = NULL;
> +	}
> +
> +	if (qm_port->int_arm_qe) {
> +		rte_free(qm_port->int_arm_qe);
> +		qm_port->int_arm_qe = NULL;
> +	}
> +
> +	if (qm_port->consume_qe) {
> +		rte_free(qm_port->consume_qe);
> +		qm_port->consume_qe = NULL;
> +	}

You can pass a NULL pointer to rte_free() -- it's guaranteed to do nothing in that case.

> +}
> +
>  /* override defaults with value(s) provided on command line */
>  static void
>  dlb2_init_queue_depth_thresholds(struct dlb2_eventdev *dlb2,
> @@ -366,10 +388,322 @@ set_qid_depth_thresh(const char *key __rte_unused,
>  }
> 
>  static void
> +dlb2_eventdev_info_get(struct rte_eventdev *dev,
> +		       struct rte_event_dev_info *dev_info)
> +{
> +	struct dlb2_eventdev *dlb2 = dlb2_pmd_priv(dev);
> +	int ret;
> +
> +	ret = dlb2_hw_query_resources(dlb2);
> +	if (ret) {
> +		const struct rte_eventdev_data *data = dev->data;
> +
> +		DLB2_LOG_ERR("get resources err=%d, devid=%d\n",
> +			     ret, data->dev_id);
> +		/* fn is void, so fall through and return values set up in
> +		 * probe
> +		 */
> +	}
> +
> +	/* Add num resources currently owned by this domain.
> +	 * These would become available if the scheduling domain were reset
> due
> +	 * to the application recalling eventdev_configure to *reconfigure* the
> +	 * domain.
> +	 */
> +	evdev_dlb2_default_info.max_event_ports += dlb2->num_ldb_ports;
> +	evdev_dlb2_default_info.max_event_queues += dlb2-
> >num_ldb_queues;
> +	evdev_dlb2_default_info.max_num_events += dlb2->max_ldb_credits;
> +
> +	evdev_dlb2_default_info.max_event_queues =
> +		RTE_MIN(evdev_dlb2_default_info.max_event_queues,
> +			RTE_EVENT_MAX_QUEUES_PER_DEV);
> +
> +	evdev_dlb2_default_info.max_num_events =
> +		RTE_MIN(evdev_dlb2_default_info.max_num_events,
> +			dlb2->max_num_events_override);
> +
> +	*dev_info = evdev_dlb2_default_info;
> +}
> +
> +static int
> +dlb2_hw_create_sched_domain(struct dlb2_hw_dev *handle,
> +			    const struct dlb2_hw_rsrcs *resources_asked)
> +{
> +	int ret = 0;
> +	struct dlb2_create_sched_domain_args *config_params;
> +
> +	if (resources_asked == NULL) {
> +		DLB2_LOG_ERR("dlb2: dlb2_create NULL parameter\n");
> +		ret = EINVAL;
> +		goto error_exit;
> +	}
> +
> +	/* Map generic qm resources to dlb2 resources */
> +	config_params = &handle->cfg.resources;
> +
> +	/* DIR ports and queues */
> +
> +	config_params->num_dir_ports =
> +		resources_asked->num_dir_ports;
> +
> +	config_params->num_dir_credits =
> +		resources_asked->num_dir_credits;
> +
> +	/* LDB queues */
> +
> +	config_params->num_ldb_queues =
> +		resources_asked->num_ldb_queues;
> +
> +	/* LDB ports */
> +
> +	config_params->cos_strict = 0; /* Best effort */
> +	config_params->num_cos_ldb_ports[0] = 0;
> +	config_params->num_cos_ldb_ports[1] = 0;
> +	config_params->num_cos_ldb_ports[2] = 0;
> +	config_params->num_cos_ldb_ports[3] = 0;
> +
> +	switch (handle->cos_id) {
> +	case DLB2_COS_0:
> +		config_params->num_ldb_ports = 0; /* no don't care ports */
> +		config_params->num_cos_ldb_ports[0] =
> +			resources_asked->num_ldb_ports;
> +		break;
> +	case DLB2_COS_1:
> +		config_params->num_ldb_ports = 0; /* no don't care ports */
> +		config_params->num_cos_ldb_ports[1] =
> +			resources_asked->num_ldb_ports;
> +		break;
> +	case DLB2_COS_2:
> +		config_params->num_ldb_ports = 0; /* no don't care ports */
> +		config_params->num_cos_ldb_ports[2] =
> +			resources_asked->num_ldb_ports;
> +		break;
> +	case DLB2_COS_3:
> +		config_params->num_ldb_ports = 0; /* no don't care ports */
> +		config_params->num_cos_ldb_ports[3] =
> +			resources_asked->num_ldb_ports;
> +		break;
> +	case DLB2_COS_DEFAULT:
> +		/* all ldb ports are don't care ports from a cos perspective */
> +		config_params->num_ldb_ports =
> +			resources_asked->num_ldb_ports;
> +		break;
> +	}
> +
> +	config_params->num_ldb_credits =
> +		resources_asked->num_ldb_credits;
> +
> +	config_params->num_atomic_inflights =
> +		DLB2_NUM_ATOMIC_INFLIGHTS_PER_QUEUE *
> +		config_params->num_ldb_queues;
> +
> +	config_params->num_hist_list_entries = resources_asked-
> >num_ldb_ports *
> +		DLB2_NUM_HIST_LIST_ENTRIES_PER_LDB_PORT;
> +
> +	DLB2_LOG_DBG("sched domain create - ldb_qs=%d, ldb_ports=%d,
> dir_ports=%d, atomic_inflights=%d, hist_list_entries=%d, ldb_credits=%d,
> dir_credits=%d\n",
> +		     config_params->num_ldb_queues,
> +		     resources_asked->num_ldb_ports,
> +		     config_params->num_dir_ports,
> +		     config_params->num_atomic_inflights,
> +		     config_params->num_hist_list_entries,
> +		     config_params->num_ldb_credits,
> +		     config_params->num_dir_credits);
> +
> +	/* Configure the QM */
> +
> +	ret = dlb2_iface_sched_domain_create(handle, config_params);
> +	if (ret < 0) {
> +		DLB2_LOG_ERR("dlb2: domain create failed, device_id = %d,
> (driver ret = %d, extra status: %s)\n",
> +			     handle->device_id,
> +			     ret,
> +			     dlb2_error_strings
> +				[config_params->response.status]);

Perhaps rename/shorten config_params so this array+index can fit on one line, or
assign config_params->response.status to a local variable? 

> +
> +		goto error_exit;
> +	}
> +
> +	handle->domain_id = config_params->response.id;
> +	handle->domain_id_valid = 1;
> +	handle->cfg.configured = true;
> +
> +error_exit:
> +
> +	return ret;
> +}
> +
> +static void
> +dlb2_hw_reset_sched_domain(const struct rte_eventdev *dev, bool reconfig)
> +{
> +	struct dlb2_eventdev *dlb2 = dlb2_pmd_priv(dev);
> +	enum dlb2_configuration_state config_state;
> +	int i, j;
> +
> +	dlb2_iface_domain_reset(dlb2);
> +
> +	/* Free all dynamically allocated port memory */
> +	for (i = 0; i < dlb2->num_ports; i++)
> +		dlb2_free_qe_mem(&dlb2->ev_ports[i].qm_port);
> +
> +	/* If reconfiguring, mark the device's queues and ports as "previously
> +	 * configured." If the user doesn't reconfigure them, the PMD will
> +	 * reapply their previous configuration when the device is started.
> +	 */
> +	config_state = (reconfig) ? DLB2_PREV_CONFIGURED :
> +		DLB2_NOT_CONFIGURED;
> +
> +	for (i = 0; i < dlb2->num_ports; i++) {
> +		dlb2->ev_ports[i].qm_port.config_state = config_state;
> +		/* Reset setup_done so ports can be reconfigured */
> +		dlb2->ev_ports[i].setup_done = false;
> +		for (j = 0; j < DLB2_MAX_NUM_QIDS_PER_LDB_CQ; j++)
> +			dlb2->ev_ports[i].link[j].mapped = false;
> +	}
> +
> +	for (i = 0; i < dlb2->num_queues; i++)
> +		dlb2->ev_queues[i].qm_queue.config_state = config_state;
> +
> +	for (i = 0; i < DLB2_MAX_NUM_QUEUES; i++)
> +		dlb2->ev_queues[i].setup_done = false;
> +
> +	dlb2->num_ports = 0;
> +	dlb2->num_ldb_ports = 0;
> +	dlb2->num_dir_ports = 0;
> +	dlb2->num_queues = 0;
> +	dlb2->num_ldb_queues = 0;
> +	dlb2->num_dir_queues = 0;
> +	dlb2->configured = false;
> +}
> +
> +/* Note: 1 QM instance per QM device, QM instance/device == event device */
> +static int
> +dlb2_eventdev_configure(const struct rte_eventdev *dev)
> +{
> +	struct dlb2_eventdev *dlb2 = dlb2_pmd_priv(dev);
> +	struct dlb2_hw_dev *handle = &dlb2->qm_instance;
> +	struct dlb2_hw_rsrcs *rsrcs = &handle->info.hw_rsrc_max;
> +	const struct rte_eventdev_data *data = dev->data;
> +	const struct rte_event_dev_config *config = &data->dev_conf;
> +	int ret;
> +
> +	/* If this eventdev is already configured, we must release the current
> +	 * scheduling domain before attempting to configure a new one.
> +	 */
> +	if (dlb2->configured) {
> +		dlb2_hw_reset_sched_domain(dev, true);
> +
> +		ret = dlb2_hw_query_resources(dlb2);
> +		if (ret) {
> +			DLB2_LOG_ERR("get resources err=%d, devid=%d\n",
> +				     ret, data->dev_id);
> +			return ret;
> +		}
> +	}
> +
> +	if (config->nb_event_queues > rsrcs->num_queues) {
> +		DLB2_LOG_ERR("nb_event_queues parameter (%d) exceeds the
> QM device's capabilities (%d).\n",
> +			     config->nb_event_queues,
> +			     rsrcs->num_queues);
> +		return -EINVAL;
> +	}
> +	if (config->nb_event_ports > (rsrcs->num_ldb_ports
> +			+ rsrcs->num_dir_ports)) {
> +		DLB2_LOG_ERR("nb_event_ports parameter (%d) exceeds the
> QM device's capabilities (%d).\n",
> +			     config->nb_event_ports,
> +			     (rsrcs->num_ldb_ports + rsrcs->num_dir_ports));
> +		return -EINVAL;
> +	}
> +	if (config->nb_events_limit > rsrcs->nb_events_limit) {
> +		DLB2_LOG_ERR("nb_events_limit parameter (%d) exceeds the
> QM device's capabilities (%d).\n",
> +			     config->nb_events_limit,
> +			     rsrcs->nb_events_limit);
> +		return -EINVAL;
> +	}
> +
> +	if (config->event_dev_cfg &
> RTE_EVENT_DEV_CFG_PER_DEQUEUE_TIMEOUT) {
> +		dlb2->global_dequeue_wait = false;

Nit -- coding style discourages braces on single-statement conditionals, even if
It's followed by a multi-statement else block.

https://doc.dpdk.org/guides/contributing/coding_style.html#control-statements-and-loops

> +	} else {
> +		uint32_t timeout32;
> +
> +		dlb2->global_dequeue_wait = true;
> +
> +		/* Craziness here is due to size mismatch in eventdev lib.
> +		 * TODO: Submit patch so dequeue API and config use same bit
> +		 * width timeout value and same units or time, instead of one
> +		 * being 32b ns and the other being 64b ticks.

Leftover TODO

> +		 */
> +
> +		timeout32 = config->dequeue_timeout_ns;
> +
> +		dlb2->global_dequeue_wait_ticks =
> +			timeout32 * (rte_get_timer_hz() / 1E9);
> +	}
> +
> +	/* Does this platform support umonitor/umwait? */
> +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_UMWAIT)) {
> +		if (RTE_LIBRTE_PMD_DLB2_UMWAIT_CTL_STATE != 0 &&
> +		    RTE_LIBRTE_PMD_DLB2_UMWAIT_CTL_STATE != 1) {
> +			DLB2_LOG_ERR("invalid value (%d) for
> RTE_LIBRTE_PMD_DLB2_UMWAIT_CTL_STATE, must be 0 or 1.\n",
> +
> RTE_LIBRTE_PMD_DLB2_UMWAIT_CTL_STATE);
> +			return -EINVAL;
> +		}
> +		dlb2->umwait_allowed = true;
> +	}
> +
> +	/* FIXME: DLB should revert to load-balanced ports if dir are not
> +	 * available
> +	 */

Leftover FIXME

> +
> +	rsrcs->num_dir_ports = config->nb_single_link_event_port_queues;
> +	rsrcs->num_ldb_ports  = config->nb_event_ports - rsrcs-
> >num_dir_ports;
> +	/* 1 dir queue per dir port */
> +	rsrcs->num_ldb_queues = config->nb_event_queues - rsrcs-
> >num_dir_ports;
> +
> +	/* Scale down nb_events_limit by 4 for directed credits, since there
> +	 * are 4x as many load-balanced credits.
> +	 */
> +	rsrcs->num_ldb_credits = 0;
> +	rsrcs->num_dir_credits = 0;
> +
> +	if (rsrcs->num_ldb_queues)
> +		rsrcs->num_ldb_credits = config->nb_events_limit;
> +	if (rsrcs->num_dir_ports)
> +		rsrcs->num_dir_credits = config->nb_events_limit / 4;
> +	if (dlb2->num_dir_credits_override != -1)
> +		rsrcs->num_dir_credits = dlb2->num_dir_credits_override;
> +
> +	if (dlb2_hw_create_sched_domain(handle, rsrcs) < 0) {
> +		DLB2_LOG_ERR("dlb2_hw_create_sched_domain failed\n");
> +		return -ENODEV;
> +	}
> +
> +	dlb2->new_event_limit = config->nb_events_limit;
> +	__atomic_store_n(&dlb2->inflights, 0, __ATOMIC_SEQ_CST);
> +
> +

Nit: extra whitespace

> +	/* Save number of ports/queues for this event dev */
> +	dlb2->num_ports = config->nb_event_ports;
> +	dlb2->num_queues = config->nb_event_queues;
> +	dlb2->num_dir_ports = rsrcs->num_dir_ports;
> +	dlb2->num_ldb_ports = dlb2->num_ports - dlb2->num_dir_ports;
> +	dlb2->num_ldb_queues = dlb2->num_queues - dlb2->num_dir_ports;
> +	dlb2->num_dir_queues = dlb2->num_dir_ports;
> +	dlb2->ldb_credit_pool = rsrcs->num_ldb_credits;
> +	dlb2->max_ldb_credits = rsrcs->num_ldb_credits;
> +	dlb2->dir_credit_pool = rsrcs->num_dir_credits;
> +	dlb2->max_dir_credits = rsrcs->num_dir_credits;
> +
> +	dlb2->configured = true;
> +
> +	return 0;
> +}

Thanks,
Gage