[libcamera-devel] [PATCH 3/3] ipa: ipu3: af: Remove redundant memcpy
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Mar 23 12:06:17 CET 2022
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 at 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 at 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
More information about the libcamera-devel
mailing list