[libcamera-devel] [PATCH] libcamera: ipu3: Always use sensor full frame size
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Sep 18 15:33:02 CEST 2020
Hi Kieran,
On Fri, Sep 18, 2020 at 09:24:51AM +0100, Kieran Bingham wrote:
> On 18/09/2020 03:57, Laurent Pinchart wrote:
> > On Thu, Sep 17, 2020 at 01:28:39PM +0200, Jacopo Mondi wrote:
> >> When calculating the pipeline configuration for the IPU3 platform,
> >> libcamera tries to be smart and select the smallest sensor frame
> >> resolution larger enough to accommodate the stream sizes
> >
> > s/larger/large/
> >
> >> requested by the application.
> >>
> >> While this seems to make a lot of sense, in practice optimizing the
> >
> > s/seems to make/makes/
> >
> >> selected sensor resolution makes the pipeline configuration calculation
> >> process fail in multiple occasions, or results in stalls during capture.
> >>
> >> As a trivial example, capturing with cam with the following command
> >> line results in a stall:
> >> $ cam -swidth=1280,height=720 -swidth=640,height=480 -c1 -C
> >>
> >> Likewise, the Android HAL supported format enumeration fails in
> >> reporting smaller resolutions as supported when used with the OV5670
> >> sensor.
> >>
> >> 320x240:
> >> DEBUG IPU3 ipu3.cpp:192 CIO2 configuration: 648x486-SGRBG10_IPU3
> >> ERROR IPU3 imgu.cpp:408 Failed to calculate pipe configuration
> >> ERROR IPU3 ipu3.cpp:299 Failed to calculate pipe configuration: unsupported resolutions.
> >>
> >> 640x480:
> >> DEBUG IPU3 ipu3.cpp:192 CIO2 configuration: 320x240-SGRBG10_IPU3
> >> ERROR IPU3 imgu.cpp:408 Failed to calculate pipe configuration
> >> ERROR IPU3 ipu3.cpp:299 Failed to calculate pipe configuration: unsupported resolutions.
> >>
> >> Furthermore the reference xml files used for the IPU3 camera
> >> configuration on the ChromeOS platform restricts the number of sensor
> >> resolution to be used for the OV5670 sensor to 2 from the 6 supported by
> >> the driver [1].
> >>
> >> The selection criteria of the correct CIO2 mode are not specified, and
> >> for the time being, always use the sensor maximum resolution at the
> >> expense of frame rate and bus bandwidth to allow the pipeline to successfully
> >> support smaller modes for the OV5670 sensor and solves pipeline stalls
> >
> > s/solves/solve/
> >
> >> when capturing with both sensors.
> >
> > I would make it clearer this is a temporary workaround. Maybe "For the
> > time being, as a workaround, ..." ?
> >
> >>
> >> [1] See the <sensor_modes> enumeration in:
> >> https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/master/baseboard-poppy/media-libs/cros-camera-hal-configs-poppy/files/gcss/graph_settings_ov5670.xml
> >>
> >> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> >> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >> ---
> >> RFC->v1:
> >> - Add Niklas' tag
> >> - Expand the \todo comment and remove the existing comment block
> >>
> >> Just have a look at the \todo block wording, if no big questions I'll push this
> >> one soon.
> >>
> >> Thanks
> >> j
> >> ---
> >> src/libcamera/pipeline/ipu3/ipu3.cpp | 31 ++++++++++++----------------
> >> 1 file changed, 13 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> index 2d881fe28f98..2ba00a2ca211 100644
> >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> @@ -144,25 +144,16 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> >> status = Adjusted;
> >> }
> >>
> >> - /*
> >> - * 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, as the IPU3 can downscale only. If
> >> - * no resolution is requested for the RAW stream, use the one from the
> >> - * largest YUV stream, plus margins pixels for the IF and BDS to scale.
> >> - * If no resolution is requested for any stream, pick the largest one.
> >> - */
> >> + /* Validate the requested stream configuration */
> >> unsigned int rawCount = 0;
> >> unsigned int yuvCount = 0;
> >> Size maxYuvSize;
> >> - Size maxRawSize;
> >>
> >> for (const StreamConfiguration &cfg : config_) {
> >> const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
> >>
> >> if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {
> >> rawCount++;
> >> - maxRawSize.expandTo(cfg.size);
> >> } else {
> >> yuvCount++;
> >> maxYuvSize.expandTo(cfg.size);
> >> @@ -174,18 +165,22 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> >> return Invalid;
> >> }
> >>
> >> - if (maxRawSize.isNull())
> >> - maxRawSize = maxYuvSize.alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,
> >> - IMGU_OUTPUT_HEIGHT_MARGIN)
> >> - .boundedTo(data_->cio2_.sensor()->resolution());
> >> -
> >> /*
> >> * Generate raw configuration from CIO2.
> >> *
> >> - * The output YUV streams will be limited in size to the maximum
> >> - * frame size requested for the RAW stream.
> >> + * \todo The image sensor frame size should be calculated as the
> >> + * smallest possible resolution larger enough to accommodate the
> >> + * requested stream sizes. However such a selection makes the pipeline
> >
> > Smallest possible size may not always be the best, we haven't really
> > thought about this. I would write "The image sensor output size should
> > be selected to optimize operation based on the sizes of the requested
> > streams.".
>
> Indeed, we really need to think about how we present this as an option
> to the users/applications as well.
>
> This is /use case/ dependant.
>
> A mobile platform wants to reduce power for instance, and might want the
> lowest reasonable sensor size to capture the required results, but other
> use-cases (perhaps a digital microscope) would want to use as high a
> resolution as possible from the sensor to capture as much light
> information as is possible, and deal with any size constraints at the
> rescaler.
It's actually the other way around, you'll get a better SNR if you scale
on the sensor with binning.
> And of course it can also depend on how the resolution selection affects
> the field of view ...
That's a separate issue, scaling doesn't affect the field of view, and
cropping shouldn't be applied behind the scenes to lower the resolution.
> >> + * configuration procedure fail for small resolution (in example:
>
> s/resolution/resolutions/
>
> > s/in example/for example/
> >
> >> + * 640x480 with OV5670) and causes the capture operations to stall for
> >> + * some streams size combinations (see the commit message of the patch
> >
> > s/streams/stream/
> >
> >> + * that introduced this comment for more failure examples).
> >> + *
> >> + * Until the sensor frame size calculation criteria are not clarified,
> >
> > s/are not clarified/are clarified/
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> >> + * always use the largest possible one which guarantees better results
> >> + * at the expense of the frame rate and CSI-2 bus bandwidth.
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> >> */
> >> - cio2Configuration_ = data_->cio2_.generateConfiguration(maxRawSize);
> >> + cio2Configuration_ = data_->cio2_.generateConfiguration({});
> >> if (!cio2Configuration_.pixelFormat.isValid())
> >> return Invalid;
> >>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list