[libcamera-devel] [PATCH v2 01/17] libcamera: ipu3: Use the optimal sensor size
Jacopo Mondi
jacopo at jmondi.org
Mon Oct 11 17:30:07 CEST 2021
On Mon, Oct 11, 2021 at 06:20:41PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Oct 11, 2021 at 05:00:40PM +0200, Jacopo Mondi wrote:
> > On Mon, Oct 11, 2021 at 05:08:26PM +0300, Laurent Pinchart wrote:
> > > On Mon, Oct 11, 2021 at 10:18:17AM +0200, Jacopo Mondi wrote:
> > > > On Wed, Sep 22, 2021 at 04:51:26AM +0300, Laurent Pinchart wrote:
> > > > > Hi Jacopo,
> > > > >
> > > > > Thank you for the patch.
> > > > >
> > > > > (Resending with Umang's correct e-mail address now)
> > > > >
> > > > > On Tue, Sep 07, 2021 at 09:40:51PM +0200, Jacopo Mondi wrote:
> > > > > > As reported by commit 7208e70211a6 ("libcamera: ipu3: Always use sensor
> > > > > > full frame size") the current implementation of the IPU3 pipeline
> > > > > > handler always uses the sensor resolution as the ImgU input frame size in
> > > > > > order to work around an issue with the ImgU configuration procedure.
> > > > > >
> > > > > > Now that the frame selection policy has been modified in the CIO2Device
> > > > > > class implementation to comply with the requirements of the ImgU
> > > > > > configuration script we can remove the workaround and select the most
> > > > > > opportune sensor size to feed the ImgU with.
> > > > >
> > > > > On a side note, I missed reviewing the changes in cio2.cpp, doesn't that
> > > > > belong to ipu3.cpp instead ?
> > > >
> > > > $ git show 5fc426fbfe58a82e30021d7a9ca12a4daeaec0f3 --stat
> > > > src/libcamera/pipeline/ipu3/cio2.cpp | 8 ++++++++
> > >
> > > I meant doesn't CIO2Device::getSensorFormat() belong to ipu3.cpp ?
> >
> > Are you suggesting to move them here ?
>
> Yes, but not as part of this series. I wanted your opinion on whether
> this would be a good idea or not. CIO2Device::getSensorFormat()
> implements constraints coming from the ImgU, which seems to be a bit of
> a layering violation to me.
>
mmmm, dunno... it is conceptually a requirement of the ImgU, but the
whole sensor handling is done by the CIO2Device class and hidden from
the rest (something I was never really found of). So in both cases we
violates a layer. Not sure to be honest
> > > > > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > > > Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> > > > > > Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> > > > > > ---
> > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 41 ++++++++++++++++------------
> > > > > > 1 file changed, 23 insertions(+), 18 deletions(-)
> > > > > >
> > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > > > index c287bf86e79a..291338288685 100644
> > > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > > > @@ -229,7 +229,16 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> > > > > > status = Adjusted;
> > > > > > }
> > > > > >
> > > > > > - /* Validate the requested stream configuration */
> > > > > > + /*
> > > > > > + * Validate the requested stream configuration and select the sensor
> > > > > > + * format by collecting the maximum RAW stream width and height and
> > > > > > + * picking the closest larger match.
> > > > > > + *
> > > > > > + * If no RAW stream is requested use the one of the largest YUV stream,
> > > > > > + * plus margin pixels for the IF and BDS rectangle to downscale.
> > > > > > + *
> > > > > > + * \todo Clarify the IF and BDS margins requirements.
> > > > > > + */
> > > > > > unsigned int rawCount = 0;
> > > > > > unsigned int yuvCount = 0;
> > > > > > Size maxYuvSize;
> > > > > > @@ -240,7 +249,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> > > > > >
> > > > > > if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {
> > > > > > rawCount++;
> > > > > > - rawSize = cfg.size;
> > > > > > + rawSize.expandTo(cfg.size);
> > > > >
> > > > > We support a single raw stream only, is this change needed ?
> > > >
> > > > It makes the two branches look the same
> > > >
> > > > > > } else {
> > > > > > yuvCount++;
> > > > > > maxYuvSize.expandTo(cfg.size);
> > > > > > @@ -269,24 +278,20 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> > > > > > /*
> > > > > > * Generate raw configuration from CIO2.
> > > > > > *
> > > > > > - * \todo The image sensor frame size should be selected to optimize
> > > > > > - * operations based on the sizes of the requested streams. However such
> > > > > > - * a selection makes the pipeline configuration procedure fail for small
> > > > > > - * resolutions (for example: 640x480 with OV5670) and causes the capture
> > > > > > - * operations to stall for some stream size combinations (see the
> > > > > > - * commit message of the patch that introduced this comment for more
> > > > > > - * failure examples).
> > > > > > + * The output YUV streams will be limited in size to the
> > > > > > + * maximum frame size requested for the RAW stream, if present.
> > > > >
> > > > > This could be reflowed.
> > > > >
> > > > > > *
> > > > > > - * Until the sensor frame size calculation criteria are clarified, when
> > > > > > - * capturing from ImgU always use the largest possible size which
> > > > > > - * guarantees better results at the expense of the frame rate and CSI-2
> > > > > > - * bus bandwidth. When only a raw stream is requested use the requested
> > > > > > - * size instead, as the ImgU is not involved.
> > > > > > + * If no raw stream is requested generate a size as large as the maximum
> > > > > > + * requested YUV size aligned to the ImgU constraints and bound by the
> > > > > > + * sensor's maximum resolution. See
> > > > > > + * https://bugs.libcamera.org/show_bug.cgi?id=32
> > > > > > */
> > > > > > - if (!yuvCount)
> > > > > > - cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);
> > > > > > - else
> > > > > > - cio2Configuration_ = data_->cio2_.generateConfiguration({});
> > > > > > + if (rawSize.isNull())
> > > > > > + rawSize = maxYuvSize.alignedUpTo(40, 540)
> > > > >
> > > > > Is alignedUpTo() the right function ? It will round up the height to a
> > > > > multiple of 540, that doesn't sound right.
> > > > >
> > > >
> > > > No it should probably be expandedTo()
> > > >
> > > > > > + .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,
> > > > > > + IMGU_OUTPUT_HEIGHT_MARGIN)
> > > > >
> > > > > Same here.
> > > >
> > > > While I guess this one is correct as we need to align up to the
> > > > margins
> > >
> > > Are margins alignments, or a number of pixels to be added because they
> > > will be cropped by the processing blocks ?
> >
> > I wish I knew.
> >
> > We never had any real understanding of how to get from a raw size to a
> > suitable output size and what are the requirements of the ImgU in
> > terms of processing margins.
> >
> > This one works, I would rather not touch it
>
> But this is new code, right ? Do you mean you'd rather not alter the
> behaviour of the existing implementation in the libcamera master branch,
> or the behaviour of this patch ?
>
Correct, and now that I look at how margins are used I could take a
bet and use instead the defines we have for alignment ?
static constexpr unsigned int IMGU_OUTPUT_WIDTH_ALIGN = 64;
static constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4;
static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64;
static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32;
To find out if moving the alignement from 32 to 4 casues any issue.
> > (I don't like operating in
> > such conservative way because of a lack of understanding, but I have
> > no other options here).
>
> I still have this on my todo list, not to prove you wrong as such, but
> because it bothers me too :-)
>
I understand :) We've been dragging this debt for too long, I concur.
Thanks
j
> > > > > > + .boundedTo(data_->cio2_.sensor()->resolution());
> > > > > > + cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);
> > > > > > if (!cio2Configuration_.pixelFormat.isValid())
> > > > > > return Invalid;
> > > > > >
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list