[libcamera-devel] [RFC PATCH] ipa: ipu3: Clear incoming parameter use flags

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Fri, Sep 10, 2021 at 05:24:12PM +0100, Kieran Bingham wrote:
> On 10/09/2021 16:49, Kieran Bingham wrote:
> > The incoming params buffer may contain uninitialised data, or the
> > parameters of previously queued frames. Clearing the entire buffer
> > may be an expensive operation, and the kernel will only read from
> > structures which have their associated use-flag set.
> > 
> > It is the responsibility of the algorithms to set the use flags
> > accordingly for any data structure they update during prepare().
> 
> To extend this commit message from the introduction:
> 
> "Clear the use flags of the parameter buffer before passing the buffer
> to the algorithms during their prepare() operations."
>
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

But... this has led me to check ipu3_uapi_flags. It's defined as
follows:

struct ipu3_uapi_flags {
	__u32 gdc:1;
	__u32 obgrid:1;
	__u32 reserved1:30;

	__u32 acc_bnr:1;
	__u32 acc_green_disparity:1;
	__u32 acc_dm:1;
	__u32 acc_ccm:1;
	__u32 acc_gamma:1;
	__u32 acc_csc:1;
	__u32 acc_cds:1;
	__u32 acc_shd:1;
	__u32 reserved2:2;
	__u32 acc_iefd:1;
	__u32 acc_yds_c0:1;
	__u32 acc_chnr_c0:1;
	__u32 acc_y_ee_nr:1;
	__u32 acc_yds:1;
	__u32 acc_chnr:1;
	__u32 acc_ytm:1;
	__u32 acc_yds2:1;
	__u32 acc_tcc:1;
	__u32 acc_dpc:1;
	__u32 acc_bds:1;
	__u32 acc_anr:1;
	__u32 acc_awb_fr:1;
	__u32 acc_ae:1;
	__u32 acc_af:1;
	__u32 acc_awb:1;
	__u32 reserved3:4;

	__u32 lin_vmem_params:1;
	__u32 tnr3_vmem_params:1;
	__u32 xnr3_vmem_params:1;
	__u32 tnr3_dmem_params:1;
	__u32 xnr3_dmem_params:1;
	__u32 reserved4:1;
	__u32 obgrid_param:1;
	__u32 reserved5:25;
} __attribute__((packed));

The first group spans 32 bits (1+1+32). So does the last group
(1+1+1+1+1+1+1+25). Guess how many bits the second group spans? Hint:
it's not 32. I wonder if this operates as expected.

> > ---
> > 
> > Yes, the commit message is the same as the comment before the line.
> > I felt the text was worthy of documenting the clearing of the flags, and
> > ensuring that it's documented in the code that the algorithms are
> > responsible for setting their use flag of any structure they modify.
> > 
> > Note that this is sent compile tested only, as it's something I noticed,
> > while writing documentation and wanted to check.
> > 
> >  src/ipa/ipu3/ipu3.cpp | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > index c925cf642611..30d2a53903ec 100644
> > --- a/src/ipa/ipu3/ipu3.cpp
> > +++ b/src/ipa/ipu3/ipu3.cpp
> > @@ -457,6 +457,17 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,
> >  
> >  void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
> >  {
> > +	/*
> > +	 * The incoming params buffer may contain uninitialised data, or the
> > +	 * parameters of previously queued frames. Clearing the entire buffer
> > +	 * may be an expensive operation, and the kernel will only read from
> > +	 * structures which have their associated use-flag set.
> > +	 *
> > +	 * It is the responsibility of the algorithms to set the use flags
> > +	 * accordingly for any data structure they update during prepare().
> > +	 */
> > +	params->use = {};
> > +
> >  	for (auto const &algo : algorithms_)
> >  		algo->prepare(context_, params);
> >  

-- 
Regards,

Laurent Pinchart