Re: [PATCH v4 1/1] dmadev: support priority configuration

[fengchengwen <fengchengwen@huawei.com>]

Hi,

Sorry to reply now, because I just got back from our National Day vacation.

From the commit it seem confused about the dmadev and HW channel, Current one dmadev is corresponding a HW channel if its a hardware device.
So I suggest try not to mention HW channel, but the dmadev or DMA device.

Suggest the tile of the commit: dmadev: support strict priority configuration

On 2024/10/3 19:53, Vamsi Krishna wrote:
> From: Vamsi Attunuru <vattunuru@marvell.com>
> 
> Some DMA controllers offer the ability to configure priority level
> for the hardware command queues, allowing for the prioritization of

Let align the queues to channels
for the hardware DMA channels,

> DMA command execution based on queue importance.

based on channel importance.

> 
> This patch introduces the necessary fields in the dmadev structures to
> retrieve information about the hardware-supported priority levels and to
> enable priority configuration from the application.

This patch supports such strict priority configuration. If the dmadev supports, it should
declared the capability flag RTE_DMA_CAPA_PRI_POLICY_SP, then application could enable strict priority configuration.

> 
> Signed-off-by: Vamsi Attunuru <vattunuru@marvell.com>
> Signed-off-by: Amit Prakash Shukla <amitprakashs@marvell.com>
> ---
> V4 changes:
> * Rebased onto the latest
> 
> V3 changes:
> * Corrected patch title
> 
> V2 changes:
> * Reverted removed text from release_24_11.rst
> 
> V1 changes:
> * Added trace support
> * Added new capability flag
> 
> Deprecation notice:
> https://patches.dpdk.org/project/dpdk/patch/20240730144612.2132848-1-amitprakashs@marvell.com/
> 
> * Assuming we do not anticipate any advanced scheduling schemes for dmadev queues,
>   this patch is intended to support a strict priority scheme.
> 
>  doc/guides/rel_notes/release_24_11.rst |  8 ++++++++
>  lib/dmadev/rte_dmadev.c                | 15 +++++++++++++++
>  lib/dmadev/rte_dmadev.h                | 21 +++++++++++++++++++++
>  lib/dmadev/rte_dmadev_trace.h          |  2 ++
>  4 files changed, 46 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/release_24_11.rst b/doc/guides/rel_notes/release_24_11.rst
> index e0a9aa55a1..9672d8c679 100644
> --- a/doc/guides/rel_notes/release_24_11.rst
> +++ b/doc/guides/rel_notes/release_24_11.rst
> @@ -67,6 +67,11 @@ New Features
>  
>    The new statistics are useful for debugging and profiling.
>  
> +* **Added strict priority capability flag in dmadev.**
> +
> +  Added new capability flag ``RTE_DMA_CAPA_PRI_POLICY_SP`` to check if the
> +  DMA device supports assigning fixed priority to its channels, allowing
> +  for better control over resource allocation and scheduling.

Please refine:

Add strict priority capability for dmadev.

Added new capability flag ``RTE_DMA_CAPA_PRI_POLICY_SP`` to check if the DMA device supports assigned fixed priority, allowing
for better control over resource allocation and scheduling.

>  
>  Removed Items
>  -------------
> @@ -112,6 +117,9 @@ ABI Changes
>     Also, make sure to start the actual text at the margin.
>     =======================================================
>  
> +* dmadev: Added ``nb_priorities`` field to ``rte_dma_info`` structure and
> +  ``priority`` field to ``rte_dma_conf`` structure to get device supported
> +  priority levels and configure required priority from the application.
>  
>  Known Issues
>  ------------
> diff --git a/lib/dmadev/rte_dmadev.c b/lib/dmadev/rte_dmadev.c
> index 845727210f..3d9063dee3 100644
> --- a/lib/dmadev/rte_dmadev.c
> +++ b/lib/dmadev/rte_dmadev.c
> @@ -497,6 +497,21 @@ rte_dma_configure(int16_t dev_id, const struct rte_dma_conf *dev_conf)
>  		return -EINVAL;
>  	}
>  
> +	if (dev_conf->priority && !(dev_info.dev_capa & RTE_DMA_CAPA_PRI_POLICY_SP)) {
> +		RTE_DMA_LOG(ERR, "Device %d don't support prioritization", dev_id);
> +		return -EINVAL;
> +	}
> +
> +	if (dev_info.nb_priorities == 1) {
> +		RTE_DMA_LOG(ERR, "Device %d must support more than 1 priority, or else 0", dev_id);
> +		return -EINVAL;
> +	}

Please consider other driver which don't set nb_priorities, then it will failed in the above branch.

Suggest add this verify in rte_dma_info_get(), make sure that this field should be > 1 if it supported PRI_POLICY_SP.

> +
> +	if (dev_info.nb_priorities && (dev_conf->priority >= dev_info.nb_priorities)) {
> +		RTE_DMA_LOG(ERR, "Device %d configure invalid priority", dev_id);
> +		return -EINVAL;
> +	}
> +
>  	if (*dev->dev_ops->dev_configure == NULL)
>  		return -ENOTSUP;
>  	ret = (*dev->dev_ops->dev_configure)(dev, dev_conf,
> diff --git a/lib/dmadev/rte_dmadev.h b/lib/dmadev/rte_dmadev.h
> index 5474a5281d..e5f730c327 100644
> --- a/lib/dmadev/rte_dmadev.h
> +++ b/lib/dmadev/rte_dmadev.h
> @@ -268,6 +268,16 @@ int16_t rte_dma_next_dev(int16_t start_dev_id);
>  #define RTE_DMA_CAPA_OPS_COPY_SG	RTE_BIT64(33)
>  /** Support fill operation. */
>  #define RTE_DMA_CAPA_OPS_FILL		RTE_BIT64(34)
> +/** Support strict prioritization at DMA HW channel level
> + *
> + * If device supports HW channel prioritization then application could
> + * assign fixed priority to the DMA HW channel using 'priority' field in
> + * struct rte_dma_conf. Number of supported priority levels will be known
> + * from 'nb_priorities' field in struct rte_dma_info.
> + *
> + * DMA devices which support prioritization can advertise this capability.

How about:
Support strict priority scheduling.

Application could assign fixed priority to the DMA device using 'priority' field in
struct rte_dma_conf. Number of supported priority levels will be known from 'nb_priorities'
field in struct rte_dma_info.

> + */
> +#define RTE_DMA_CAPA_PRI_POLICY_SP	RTE_BIT64(35)

This capa is a control plane flag, so please add after RTE_DMA_CAPA_M2D_AUTO_FREE.

>  /**@}*/
>  
>  /**
> @@ -297,6 +307,10 @@ struct rte_dma_info {
>  	int16_t numa_node;
>  	/** Number of virtual DMA channel configured. */
>  	uint16_t nb_vchans;
> +	/** Number of priority levels (must be > 1), if supported by DMA HW channel.
> +	 * 0 otherwise.

How about "Number of priority levels (must be > 1) if supported priority scheduling."

A DMA HW channel was just one dmadev, suggest don't introduce it.

> +	 */
> +	uint16_t nb_priorities;
>  };
>  
>  /**
> @@ -332,6 +346,13 @@ struct rte_dma_conf {
>  	 * @see RTE_DMA_CAPA_SILENT
>  	 */
>  	bool enable_silent;
> +	/* The priority of the DMA HW channel.
> +	 * This value cannot be greater than or equal to the field 'nb_priorities'
> +	 * of struct rte_dma_info which get from rte_dma_info_get().
> +	 * Among the values between '0' and 'nb_priorities - 1', lowest value
> +	 * indicates higher priority and vice-versa.

How about (you could retouch it. I'm not a native English speaker.):
The priority of the DMA device.
If the the DMA device don't support priority scheduling, this value should be zero.
Otherwise, the value should lower than the field 'nb_priorities' of struct rte_dma_info
which get from rte_dma_info_get(). And also lowest value indicates higher priority and vice-versa.

> +	 */
> +	uint16_t priority;
>  };
>  
>  /**
> diff --git a/lib/dmadev/rte_dmadev_trace.h b/lib/dmadev/rte_dmadev_trace.h
> index e55c4c6091..be089c065c 100644
> --- a/lib/dmadev/rte_dmadev_trace.h
> +++ b/lib/dmadev/rte_dmadev_trace.h
> @@ -35,6 +35,7 @@ RTE_TRACE_POINT(
>  	rte_trace_point_emit_u16(dev_info->max_sges);
>  	rte_trace_point_emit_i16(dev_info->numa_node);
>  	rte_trace_point_emit_u16(dev_info->nb_vchans);
> +	rte_trace_point_emit_u16(dev_info->nb_priorities);
>  )
>  
>  RTE_TRACE_POINT(
> @@ -48,6 +49,7 @@ RTE_TRACE_POINT(
>  	int enable_silent = (int)dev_conf->enable_silent;
>  	rte_trace_point_emit_i16(dev_id);
>  	rte_trace_point_emit_u16(dev_conf->nb_vchans);
> +	rte_trace_point_emit_u16(dev_conf->priority);
>  	rte_trace_point_emit_int(enable_silent);
>  	rte_trace_point_emit_int(ret);
>  )

Please also modify dma_capability_name and dmadev_handle_dev_info,