Re: [dpdk-dev] [RFC] Wireless Base Band Device (bbdev)

[Thomas Monjalon <thomas@monjalon.net>]

25/08/2017 15:46, Amr Mokhtar:
> +/**
> + * Configure a device.
> + * This function must be called on a device before setting up the queues and
> + * starting the device. It can also be called when a device is in the stopped
> + * state. If any device queues have been configured their configuration will be
> + * cleared by a call to this function.
> + *
> + * @param dev_id
> + *   The identifier of the device.
> + * @param num_queues
> + *   Number of queues to configure on device.
> + * @param conf
> + *   The device configuration. If NULL, a default configuration will be used.
> + *
> + * @return
> + *   - 0 on success
> + *   - EINVAL if num_queues is invalid, 0 or greater than maximum
> + *   - EBUSY if the identified device has already started
> + *   - ENOMEM if unable to allocate memory
> + */
> +int
> +rte_bbdev_configure(uint8_t dev_id, uint16_t num_queues,
> +		const struct rte_bbdev_conf *conf);

I am not convinced by the "configure all" function in ethdev.
We break the ABI each time we add a new feature to configure.
And it does not really help to have all configurations in one struct.
Would you mind to split the struct rte_bbdev_conf and split
the function accordingly?

[...]
> +struct rte_bbdev_info {
> +	int socket_id;  /**< NUMA socket that device is on */
> +	const char *dev_name;  /**< Unique device name */
> +	const struct rte_pci_device *pci_dev;  /**< PCI information */
> +	unsigned int num_queues;  /**< Number of queues currently configured */
> +	struct rte_bbdev_conf conf;  /**< Current device configuration */
> +	bool started;  /**< Set if device is currently started */
> +	struct rte_bbdev_driver_info drv;  /**< Info from device driver */
> +};

As Stephen said, PCI must not appear in this API.
Please use the bus abstraction.

[...]
> +struct __rte_cache_aligned rte_bbdev {
> +	rte_bbdev_enqueue_ops_t enqueue_ops; /**< Enqueue function */
> +	rte_bbdev_dequeue_ops_t dequeue_ops;  /**< Dequeue function */
> +	const struct rte_bbdev_ops *dev_ops;  /**< Functions exported by PMD */
> +	struct rte_bbdev_data *data;  /**< Pointer to device data */
> +	bool attached;  /**< If device is currently attached or not */

What "attached" means?
I'm afraid you are trying to manage hotplug in the wrong layer.

> +	struct rte_device *device; /**< Backing device (HW only) */

SW port should have also a rte_device (vdev).

[...]
> +/** Data input and output buffer for Turbo operations */
> +struct rte_bbdev_op_data {

Why there is no "turbo" word in the name of this struct?

> +	struct rte_mbuf *data;
> +	/**< First mbuf segment with input/output data. */
> +	uint32_t offset;
> +	/**< The starting point for the Turbo input/output, in bytes, from the
> +	 * start of the data in the data buffer. It must be smaller than
> +	 * data_len of the mbuf's first segment!
> +	 */
> +	uint32_t length;
> +	/**< For input operations: the length, in bytes, of the source buffer
> +	 * on which the Turbo encode/decode will be computed.
> +	 * For output operations: the length, in bytes, of the output buffer
> +	 * of the Turbo operation.
> +	 */
> +};

[...]
> +/** Structure specifying a single operation */
> +struct rte_bbdev_op {
> +	enum rte_bbdev_op_type type;  /**< Type of this operation */
> +	int status;  /**< Status of operation that was performed */
> +	struct rte_mempool *mempool;  /**< Mempool which op instance is in */
> +	void *opaque_data;  /**< Opaque pointer for user data */
> +	/**
> +	 * Anonymous union of operation-type specific parameters. When allocated
> +	 * using rte_bbdev_op_pool_create(), space is allocated for the
> +	 * parameters at the end of each rte_bbdev_op structure, and the
> +	 * pointers here point to it.
> +	 */
> +	RTE_STD_C11
> +	union {
> +		void *generic;
> +		struct rte_bbdev_op_turbo_dec *turbo_dec;
> +		struct rte_bbdev_op_turbo_enc *turbo_enc;
> +	};
> +};

I am not sure it is a good idea to fit every operations in the
same struct and the same functions.

[...]
> +/**
> + * Helper macro for logging
> + *
> + * @param level
> + *   Log level: EMERG, ALERT, CRIT, ERR, WARNING, NOTICE, INFO, or DEBUG
> + * @param fmt
> + *   The format string, as in printf(3).
> + * @param ...
> + *   The variable arguments required by the format string.
> + *
> + * @return
> + *   - 0 on success
> + *   - Negative on error
> + */
> +#define rte_bbdev_log(level, fmt, ...) \
> +		RTE_LOG(level, BBDEV, fmt "\n", ##__VA_ARGS__)

This is the legacy log system.
Please use dynamic log type.

[...]
> +#ifdef RTE_LIBRTE_BBDEV_DEBUG
> +#define rte_bbdev_log_verbose(fmt, ...)  rte_bbdev_log_debug(fmt, ##__VA_ARGS__)
> +#else
> +#define rte_bbdev_log_verbose(fmt, ...)
> +#endif

With the new log functions, you do not need to disable debug log
at compilation time.

> +/**
> + *  Initialisation params structure that can be used by software based drivers
> + */
> +struct rte_bbdev_init_params {
> +	int socket_id;  /**< Base band null device socket */
> +	uint16_t queues_num;  /**< Base band null device queues number */
> +};
> +
> +/**
> + * Parse generic parameters that could be used for software based devices.
> + *
> + * @param params
> + *   Pointer to structure that will hold the parsed parameters.
> + * @param input_args
> + *   Pointer to arguments to be parsed.
> + *
> + * @return
> + *   - 0 on success
> + *   - EINVAL if invalid parameter pointer is provided
> + *   - EFAULT if unable to parse provided arguments
> + */
> +int
> +rte_bbdev_parse_params(struct rte_bbdev_init_params *params,
> +		const char *input_args);

I do not understand the intent of these parameters.
Are they common to every PMDs?
Or could they be moved in software PMDs?

End of this first review pass :)