RE: [PATCH v5 4/5] baseband/acc100: modify validation code for ACC101

["Chautru, Nicolas" <nicolas.chautru@intel.com>]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Wednesday, May 25, 2022 7:33 AM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
> gakhil@marvell.com; trix@redhat.com
> Cc: thomas@monjalon.net; Kinsella, Ray <ray.kinsella@intel.com>;
> Richardson, Bruce <bruce.richardson@intel.com>;
> hemant.agrawal@nxp.com; Vargas, Hernan <hernan.vargas@intel.com>;
> david.marchand@redhat.com
> Subject: Re: [PATCH v5 4/5] baseband/acc100: modify validation code for
> ACC101
> 
> 
> 
> On 5/24/22 02:08, Nicolas Chautru wrote:
> > The validation requirement is different for the two devices.
> >
> > Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> > ---
> >   drivers/baseband/acc100/rte_acc100_pmd.c | 47
> ++++++++++++++++++++++++--------
> >   1 file changed, 35 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/baseband/acc100/rte_acc100_pmd.c
> > b/drivers/baseband/acc100/rte_acc100_pmd.c
> > index 41475b2..e3706e0 100644
> > --- a/drivers/baseband/acc100/rte_acc100_pmd.c
> > +++ b/drivers/baseband/acc100/rte_acc100_pmd.c
> > @@ -1289,6 +1289,21 @@
> >   			RTE_BBDEV_TURBO_HALF_ITERATION_EVEN);
> >   }
> >
> > +#ifdef RTE_LIBRTE_BBDEV_DEBUG
> > +
> > +static inline bool
> > +is_acc100(struct acc100_queue *q)
> > +{
> > +	return (q->d->device_variant == ACC100_VARIANT);
> 
> I keep insisting, but please rely on the PCI device ID, there is no need to
> introduce a new field.

Thanks. I have a couple of concerns changing this:
1) the device id is not accessible from the structures currently passed by the functions which would rely on this. Ie. device_id is accessible from rte_bbdev/rte_device but not from acc100_device or acc100_queue. Would be convoluted to have to carry forward this structure when needed instead of using directly acc100_device structure.
2) These call would be done as part of the workload operation where performance matters, best to keep the check as trivial as possible within the PMD. 

Will aim for new patch in next few days based on the other refactory suggestions and unified driver. 

Thanks
Nic

> 
> > +}
> > +
> > +static inline bool
> > +validate_op_required(struct acc100_queue *q) {
> > +	return is_acc100(q);
> > +}
> > +#endif
> > +
> >   /* Fill in a frame control word for LDPC decoding. */
> >   static inline void
> >   acc100_fcw_ld_fill(struct rte_bbdev_dec_op *op, struct acc100_fcw_ld
> > *fcw, @@ -2176,8 +2191,10 @@ static inline uint32_t hq_index(uint32_t
> offset)
> >   #ifdef RTE_LIBRTE_BBDEV_DEBUG
> >   /* Validates turbo encoder parameters */
> >   static inline int
> > -validate_enc_op(struct rte_bbdev_enc_op *op)
> > +validate_enc_op(struct rte_bbdev_enc_op *op, struct acc100_queue *q)
> >   {
> > +	if (!validate_op_required(q))
> > +		return 0;
> >   	struct rte_bbdev_op_turbo_enc *turbo_enc = &op->turbo_enc;
> >   	struct rte_bbdev_op_enc_turbo_cb_params *cb = NULL;
> >   	struct rte_bbdev_op_enc_turbo_tb_params *tb = NULL; @@ -2314,8
> > +2331,10 @@ static inline uint32_t hq_index(uint32_t offset)
> >   }
> >   /* Validates LDPC encoder parameters */
> >   static inline int
> > -validate_ldpc_enc_op(struct rte_bbdev_enc_op *op)
> > +validate_ldpc_enc_op(struct rte_bbdev_enc_op *op, struct acc100_queue
> > +*q)
> >   {
> > +	if (!validate_op_required(q))
> > +		return 0;
> >   	struct rte_bbdev_op_ldpc_enc *ldpc_enc = &op->ldpc_enc;
> >
> >   	if (op->mempool == NULL) {
> > @@ -2367,8 +2386,10 @@ static inline uint32_t hq_index(uint32_t
> > offset)
> >
> >   /* Validates LDPC decoder parameters */
> >   static inline int
> > -validate_ldpc_dec_op(struct rte_bbdev_dec_op *op)
> > +validate_ldpc_dec_op(struct rte_bbdev_dec_op *op, struct acc100_queue
> > +*q)
> >   {
> > +	if (!validate_op_required(q))
> > +		return 0;
> >   	struct rte_bbdev_op_ldpc_dec *ldpc_dec = &op->ldpc_dec;
> >
> >   	if (op->mempool == NULL) {
> > @@ -2423,7 +2444,7 @@ static inline uint32_t hq_index(uint32_t offset)
> >
> >   #ifdef RTE_LIBRTE_BBDEV_DEBUG
> >   	/* Validate op structure */
> > -	if (validate_enc_op(op) == -1) {
> > +	if (validate_enc_op(op, q) == -1) {
> >   		rte_bbdev_log(ERR, "Turbo encoder validation failed");
> >   		return -EINVAL;
> >   	}
> > @@ -2477,7 +2498,7 @@ static inline uint32_t hq_index(uint32_t offset)
> >
> >   #ifdef RTE_LIBRTE_BBDEV_DEBUG
> >   	/* Validate op structure */
> > -	if (validate_ldpc_enc_op(ops[0]) == -1) {
> > +	if (validate_ldpc_enc_op(ops[0], q) == -1) {
> >   		rte_bbdev_log(ERR, "LDPC encoder validation failed");
> >   		return -EINVAL;
> >   	}
> > @@ -2539,7 +2560,7 @@ static inline uint32_t hq_index(uint32_t offset)
> >
> >   #ifdef RTE_LIBRTE_BBDEV_DEBUG
> >   	/* Validate op structure */
> > -	if (validate_ldpc_enc_op(op) == -1) {
> > +	if (validate_ldpc_enc_op(op, q) == -1) {
> >   		rte_bbdev_log(ERR, "LDPC encoder validation failed");
> >   		return -EINVAL;
> >   	}
> > @@ -2596,7 +2617,7 @@ static inline uint32_t hq_index(uint32_t offset)
> >
> >   #ifdef RTE_LIBRTE_BBDEV_DEBUG
> >   	/* Validate op structure */
> > -	if (validate_enc_op(op) == -1) {
> > +	if (validate_enc_op(op, q) == -1) {
> >   		rte_bbdev_log(ERR, "Turbo encoder validation failed");
> >   		return -EINVAL;
> >   	}
> > @@ -2669,8 +2690,10 @@ static inline uint32_t hq_index(uint32_t offset)
> >   #ifdef RTE_LIBRTE_BBDEV_DEBUG
> >   /* Validates turbo decoder parameters */
> >   static inline int
> > -validate_dec_op(struct rte_bbdev_dec_op *op)
> > +validate_dec_op(struct rte_bbdev_dec_op *op, struct acc100_queue *q)
> >   {
> > +	if (!validate_op_required(q))
> > +		return 0;
> >   	struct rte_bbdev_op_turbo_dec *turbo_dec = &op->turbo_dec;
> >   	struct rte_bbdev_op_dec_turbo_cb_params *cb = NULL;
> >   	struct rte_bbdev_op_dec_turbo_tb_params *tb = NULL; @@ -2816,7
> > +2839,7 @@ static inline uint32_t hq_index(uint32_t offset)
> >
> >   #ifdef RTE_LIBRTE_BBDEV_DEBUG
> >   	/* Validate op structure */
> > -	if (validate_dec_op(op) == -1) {
> > +	if (validate_dec_op(op, q) == -1) {
> >   		rte_bbdev_log(ERR, "Turbo decoder validation failed");
> >   		return -EINVAL;
> >   	}
> > @@ -3041,7 +3064,7 @@ static inline uint32_t hq_index(uint32_t offset)
> >
> >   #ifdef RTE_LIBRTE_BBDEV_DEBUG
> >   	/* Validate op structure */
> > -	if (validate_ldpc_dec_op(op) == -1) {
> > +	if (validate_ldpc_dec_op(op, q) == -1) {
> >   		rte_bbdev_log(ERR, "LDPC decoder validation failed");
> >   		return -EINVAL;
> >   	}
> > @@ -3145,7 +3168,7 @@ static inline uint32_t hq_index(uint32_t offset)
> >
> >   #ifdef RTE_LIBRTE_BBDEV_DEBUG
> >   	/* Validate op structure */
> > -	if (validate_ldpc_dec_op(op) == -1) {
> > +	if (validate_ldpc_dec_op(op, q) == -1) {
> >   		rte_bbdev_log(ERR, "LDPC decoder validation failed");
> >   		return -EINVAL;
> >   	}
> > @@ -3235,7 +3258,7 @@ static inline uint32_t hq_index(uint32_t offset)
> >
> >   #ifdef RTE_LIBRTE_BBDEV_DEBUG
> >   	/* Validate op structure */
> > -	if (validate_dec_op(op) == -1) {
> > +	if (validate_dec_op(op, q) == -1) {
> >   		rte_bbdev_log(ERR, "Turbo decoder validation failed");
> >   		return -EINVAL;
> >   	}