RE: [EXT] [PATCH v3 00/30] baseband/acc100: changes for 22.11

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

Hi Akhil, 

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Thursday, October 13, 2022 6:02 AM
> To: Vargas, Hernan <hernan.vargas@intel.com>; dev@dpdk.org;
> trix@redhat.com; maxime.coquelin@redhat.com
> Cc: Chautru, Nicolas <nicolas.chautru@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>
> Subject: RE: [EXT] [PATCH v3 00/30] baseband/acc100: changes for 22.11
> 
> > Hernan Vargas (30):
> >   baseband/acc100: fix ring availability calculation
> >   baseband/acc100: add function to check AQ availability
> >   baseband/acc100: memory leak fix
> >   baseband/acc100: add LDPC encoder padding function
> >   baseband/acc100: check turbo dec/enc input
> >   baseband/acc100: check for unlikely operation vals
> >   baseband/acc100: enforce additional check on FCW
> >   baseband/acc100: allocate ring/queue mem when NULL
> >   baseband/acc100: reduce input length for CRC24B
> >   baseband/acc100: fix clearing PF IR outside handler
> >   baseband/acc100: set device min alignment to 1
> >   baseband/acc100: add protection for NULL HARQ input
> >   baseband/acc100: reset pointer after rte_free
> >   baseband/acc100: fix debug print for LDPC FCW
> >   baseband/acc100: add enqueue status
> >   baseband/acc100: add scatter-gather support
> >   baseband/acc100: add HARQ index helper function
> >   baseband/acc100: enable input validation by default
> >   baseband/acc100: added LDPC transport block support
> >   baseband/acc100: update validate LDPC enc/dec
> >   baseband/acc100: implement configurable queue depth
> >   baseband/acc100: add queue stop operation
> >   baseband/acc100: update uplink CB input length
> >   baseband/acc100: rename ldpc encode function arg
> >   baseband/acc100: update log messages
> >   baseband/acc100: store FCW from first CB descriptor
> >   baseband/acc100: update device info
> >   baseband/acc100: add ring companion address
> >   baseband/acc100: add workaround for deRM corner cases
> >   baseband/acc100: configure PMON control registers
> >
> >  drivers/baseband/acc/acc100_pmd.h     |    5 +
> >  drivers/baseband/acc/acc_common.h     |   10 +
> >  drivers/baseband/acc/meson.build      |   21 +
> >  drivers/baseband/acc/rte_acc100_pmd.c | 1197
> > ++++++++++++++++++++-----
> >  4 files changed, 1010 insertions(+), 223 deletions(-)
> >
> Hi Hernan/Nicolas,
> 
> I see some ifdefs being used in the code and there is no documentation for
> them On when and how to enable/disable them.
> It would be much like a dead code which is not compiled at all, if any of the
> build target does not enable them.
> 
> Is it possible to replace them with runtime devargs instead of compile time
> ifdefs?

In term of documentation gap, that is a fair point: 
- I believe the build time variable ACC100_EXT_MEM can be valuable for very specific troubleshoot (not for production) to shortcut the DDR on the card, but good point to document in the acc100.rst. 
- The RTE_BBDEV_OFFLOAD_COST is used across PMDs for a while. That could also be added to each PMD .rst. 
- The #ifndef RTE_LIBRTE_BBDEV_SKIP_VALIDATE is to skip validation of user API which protect from negative scenario and hence save a few cycles by not recommended by default. Can be added to acc100.rst. 

In term of runtime devargs usage, can we decorrelate this from the series? If we introduce such dev args and expose them more explicitly we would need more updated validation in place, are you okay if we defer such change to next year?
If really this is a big concern for you for 22.11 (this is indeed not ideal), we could limit/strip the upstream of the series to the default build configuration only (hence no new #ifdef) but still these options can be genuinely valuable to users (until moved to devargs) so I would prefer to keep as is but with updated documentation in acc100.rst. 

What do you think?

Thanks
Nic