[libcamera-devel,3/3] ipa: ipu3: af: Remove redundant memcpy
diff mbox series

Message ID 20220315175345.479951-4-kieran.bingham@ideasonboard.com
State Superseded
Delegated to: Kieran Bingham
Headers show
Series
  • ipa: ipu3: af: Small improvements
Related show

Commit Message

Kieran Bingham March 15, 2022, 5:53 p.m. UTC 
The af statistics can be accessed directly from the mapped buffer.
Remove the redundant memcpy, and simplify the call to
afEstimateVariance().

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/af.cpp | 12 ++++--------
 src/ipa/ipu3/algorithms/af.h   |  2 +-
 2 files changed, 5 insertions(+), 9 deletions(-)

Comments

Laurent Pinchart March 23, 2022, 1:24 a.m. UTC | #1 
Hi Kieran,

Thank you for the patch.

On Tue, Mar 15, 2022 at 05:53:45PM +0000, Kieran Bingham via libcamera-devel wrote:
> The af statistics can be accessed directly from the mapped buffer.
> Remove the redundant memcpy, and simplify the call to
> afEstimateVariance().
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/af.cpp | 12 ++++--------
>  src/ipa/ipu3/algorithms/af.h   |  2 +-
>  2 files changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp
> index 469762a29937..e10cff968895 100644
> --- a/src/ipa/ipu3/algorithms/af.cpp
> +++ b/src/ipa/ipu3/algorithms/af.cpp
> @@ -336,7 +336,7 @@ void Af::afIgnoreFrameReset()
>   *
>   * \return The variance of the values in the data set \a y_item selected by \a isY1
>   */
> -double Af::afEstimateVariance(y_table_item_t *y_item, uint32_t len,
> +double Af::afEstimateVariance(const y_table_item_t *y_item, uint32_t len,
>  			      bool isY1)

Not a candidate for this patch, but pointer + len immediately makes me
think the Span class should be used instead.

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

>  {
>  	uint32_t z = 0;
> @@ -406,25 +406,21 @@ bool Af::afIsOutOfFocus(IPAContext context)
>   */
>  void Af::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
>  {
> -	y_table_item_t y_item[IPU3_UAPI_AF_Y_TABLE_MAX_SIZE / sizeof(y_table_item_t)];
> +	const y_table_item_t *y_item = reinterpret_cast<const y_table_item_t *>(&stats->af_raw_buffer.y_table);
>  	uint32_t afRawBufferLen;
>  
>  	/* Evaluate the AF buffer length */
>  	afRawBufferLen = context.configuration.af.afGrid.width *
>  			 context.configuration.af.afGrid.height;
>  
> -	memcpy(y_item, stats->af_raw_buffer.y_table,
> -	       afRawBufferLen * sizeof(y_table_item_t));
> +	ASSERT(afRawBufferLen / sizeof(y_table_item_t) < IPU3_UAPI_AF_Y_TABLE_MAX_SIZE);
>  
>  	/*
>  	 * Calculate the mean and the variance of AF statistics for a given grid.
>  	 * For coarse: y1 are used.
>  	 * For fine: y2 results are used.
>  	 */
> -	if (coarseCompleted_)
> -		currentVariance_ = afEstimateVariance(y_item, afRawBufferLen, false);
> -	else
> -		currentVariance_ = afEstimateVariance(y_item, afRawBufferLen, true);
> +	currentVariance_ = afEstimateVariance(y_item, afRawBufferLen, !coarseCompleted_);
>  
>  	if (!context.frameContext.af.stable) {
>  		afCoarseScan(context);
> diff --git a/src/ipa/ipu3/algorithms/af.h b/src/ipa/ipu3/algorithms/af.h
> index 906f2843dd49..3b5758e868ea 100644
> --- a/src/ipa/ipu3/algorithms/af.h
> +++ b/src/ipa/ipu3/algorithms/af.h
> @@ -41,7 +41,7 @@ private:
>  	void afReset(IPAContext &context);
>  	bool afNeedIgnoreFrame();
>  	void afIgnoreFrameReset();
> -	double afEstimateVariance(y_table_item_t *y_item, uint32_t len,
> +	double afEstimateVariance(const y_table_item_t *y_item, uint32_t len,
>  				  bool isY1);
>  	bool afIsOutOfFocus(IPAContext context);
>
Kieran Bingham March 23, 2022, 11:06 a.m. UTC | #2 
Quoting Laurent Pinchart (2022-03-23 01:24:45)
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Tue, Mar 15, 2022 at 05:53:45PM +0000, Kieran Bingham via libcamera-devel wrote:
> > The af statistics can be accessed directly from the mapped buffer.
> > Remove the redundant memcpy, and simplify the call to
> > afEstimateVariance().
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  src/ipa/ipu3/algorithms/af.cpp | 12 ++++--------
> >  src/ipa/ipu3/algorithms/af.h   |  2 +-
> >  2 files changed, 5 insertions(+), 9 deletions(-)
> > 
> > diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp
> > index 469762a29937..e10cff968895 100644
> > --- a/src/ipa/ipu3/algorithms/af.cpp
> > +++ b/src/ipa/ipu3/algorithms/af.cpp
> > @@ -336,7 +336,7 @@ void Af::afIgnoreFrameReset()
> >   *
> >   * \return The variance of the values in the data set \a y_item selected by \a isY1
> >   */
> > -double Af::afEstimateVariance(y_table_item_t *y_item, uint32_t len,
> > +double Af::afEstimateVariance(const y_table_item_t *y_item, uint32_t len,
> >                             bool isY1)
> 
> Not a candidate for this patch, but pointer + len immediately makes me
> think the Span class should be used instead.

I agree, I think the Span would help improve the iterators in here too.
Will do on top.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> >  {
> >       uint32_t z = 0;
> > @@ -406,25 +406,21 @@ bool Af::afIsOutOfFocus(IPAContext context)
> >   */
> >  void Af::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
> >  {
> > -     y_table_item_t y_item[IPU3_UAPI_AF_Y_TABLE_MAX_SIZE / sizeof(y_table_item_t)];
> > +     const y_table_item_t *y_item = reinterpret_cast<const y_table_item_t *>(&stats->af_raw_buffer.y_table);
> >       uint32_t afRawBufferLen;
> >  
> >       /* Evaluate the AF buffer length */
> >       afRawBufferLen = context.configuration.af.afGrid.width *
> >                        context.configuration.af.afGrid.height;
> >  
> > -     memcpy(y_item, stats->af_raw_buffer.y_table,
> > -            afRawBufferLen * sizeof(y_table_item_t));
> > +     ASSERT(afRawBufferLen / sizeof(y_table_item_t) < IPU3_UAPI_AF_Y_TABLE_MAX_SIZE);

Looking at 
include/linux/intel-ipu3.h:     __u8 y_table[IPU3_UAPI_AF_Y_TABLE_MAX_SIZE] __attribute__((aligned(32)));

IPU3_UAPI_AF_Y_TABLE_MAX_SIZE is the maximum number of entries of y_table_item_t

So I think this ASSERT is wrong, and should be:

	ASSERT(afRawBufferLen < IPU3_UAPI_AF_Y_TABLE_MAX_SIZE);

> >  
> >       /*
> >        * Calculate the mean and the variance of AF statistics for a given grid.
> >        * For coarse: y1 are used.
> >        * For fine: y2 results are used.
> >        */
> > -     if (coarseCompleted_)
> > -             currentVariance_ = afEstimateVariance(y_item, afRawBufferLen, false);
> > -     else
> > -             currentVariance_ = afEstimateVariance(y_item, afRawBufferLen, true);
> > +     currentVariance_ = afEstimateVariance(y_item, afRawBufferLen, !coarseCompleted_);
> >  
> >       if (!context.frameContext.af.stable) {
> >               afCoarseScan(context);
> > diff --git a/src/ipa/ipu3/algorithms/af.h b/src/ipa/ipu3/algorithms/af.h
> > index 906f2843dd49..3b5758e868ea 100644
> > --- a/src/ipa/ipu3/algorithms/af.h
> > +++ b/src/ipa/ipu3/algorithms/af.h
> > @@ -41,7 +41,7 @@ private:
> >       void afReset(IPAContext &context);
> >       bool afNeedIgnoreFrame();
> >       void afIgnoreFrameReset();
> > -     double afEstimateVariance(y_table_item_t *y_item, uint32_t len,
> > +     double afEstimateVariance(const y_table_item_t *y_item, uint32_t len,
> >                                 bool isY1);
> >       bool afIsOutOfFocus(IPAContext context);
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp
index 469762a29937..e10cff968895 100644
--- a/src/ipa/ipu3/algorithms/af.cpp
+++ b/src/ipa/ipu3/algorithms/af.cpp
@@ -336,7 +336,7 @@  void Af::afIgnoreFrameReset()
  *
  * \return The variance of the values in the data set \a y_item selected by \a isY1
  */
-double Af::afEstimateVariance(y_table_item_t *y_item, uint32_t len,
+double Af::afEstimateVariance(const y_table_item_t *y_item, uint32_t len,
 			      bool isY1)
 {
 	uint32_t z = 0;
@@ -406,25 +406,21 @@  bool Af::afIsOutOfFocus(IPAContext context)
  */
 void Af::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
 {
-	y_table_item_t y_item[IPU3_UAPI_AF_Y_TABLE_MAX_SIZE / sizeof(y_table_item_t)];
+	const y_table_item_t *y_item = reinterpret_cast<const y_table_item_t *>(&stats->af_raw_buffer.y_table);
 	uint32_t afRawBufferLen;
 
 	/* Evaluate the AF buffer length */
 	afRawBufferLen = context.configuration.af.afGrid.width *
 			 context.configuration.af.afGrid.height;
 
-	memcpy(y_item, stats->af_raw_buffer.y_table,
-	       afRawBufferLen * sizeof(y_table_item_t));
+	ASSERT(afRawBufferLen / sizeof(y_table_item_t) < IPU3_UAPI_AF_Y_TABLE_MAX_SIZE);
 
 	/*
 	 * Calculate the mean and the variance of AF statistics for a given grid.
 	 * For coarse: y1 are used.
 	 * For fine: y2 results are used.
 	 */
-	if (coarseCompleted_)
-		currentVariance_ = afEstimateVariance(y_item, afRawBufferLen, false);
-	else
-		currentVariance_ = afEstimateVariance(y_item, afRawBufferLen, true);
+	currentVariance_ = afEstimateVariance(y_item, afRawBufferLen, !coarseCompleted_);
 
 	if (!context.frameContext.af.stable) {
 		afCoarseScan(context);
diff --git a/src/ipa/ipu3/algorithms/af.h b/src/ipa/ipu3/algorithms/af.h
index 906f2843dd49..3b5758e868ea 100644
--- a/src/ipa/ipu3/algorithms/af.h
+++ b/src/ipa/ipu3/algorithms/af.h
@@ -41,7 +41,7 @@  private:
 	void afReset(IPAContext &context);
 	bool afNeedIgnoreFrame();
 	void afIgnoreFrameReset();
-	double afEstimateVariance(y_table_item_t *y_item, uint32_t len,
+	double afEstimateVariance(const y_table_item_t *y_item, uint32_t len,
 				  bool isY1);
 	bool afIsOutOfFocus(IPAContext context);