[libcamera-devel] [PATCH 04/11] pipeline: ipu3: Identify sensors that do not need the Imgu

[Laurent Pinchart]

Hi Dan,

Thank you for the patch.

I think it's "ImgU", not "Imgu". That applies to the subject line,
commit message and patch, and possibly to other patches in the series.

On Sat, Mar 18, 2023 at 11:40:07PM +0000, Daniel Scally via libcamera-devel wrote:
> Some sensors connected to the CIO2 device do not need to be connected
> to the Imgu - for example the OmniVision 7251 outputs Y10 data which
> needn't be debayered and which is unsupported by the kernel's ipu3-imgu
> driver.
> 
> To be able to handle those sensors we need to be able to skip the Imgu
> related parts of the processing in the IPU3 pipeline. As a preliminary
> step, check whether the sensor needs the Imgu during CIO2Device::init()
> and provide a method to check that later.
> 
> Signed-off-by: Daniel Scally <dan.scally at ideasonboard.com>
> ---
>  src/libcamera/pipeline/ipu3/cio2.cpp | 20 ++++++++++++++++++++
>  src/libcamera/pipeline/ipu3/cio2.h   |  4 ++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index 7400cb0b..d100df57 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -17,6 +17,7 @@
>  #include <libcamera/stream.h>
>  #include <libcamera/transform.h>
>  
> +#include "libcamera/internal/bayer_format.h"
>  #include "libcamera/internal/camera_sensor.h"
>  #include "libcamera/internal/framebuffer.h"
>  #include "libcamera/internal/media_device.h"
> @@ -160,6 +161,25 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * A camera sensor connected to the CIO2 device need not necessarily be
> +	 * connected to the Imgu. This is the case for some IR cameras for
> +	 * example, which output IPU3-packed Y10 data rather than packed data in
> +	 * bayer patterns. In the case of those sensors we need to be able to
> +	 * skip dealing with the Imgu during the pipeline, so we need to be able
> +	 * to identify them.
> +	 *
> +	 * Check whether this sensor needs to be connected to the Imgu.

YUV sensors would also fall in that category, so we could prepare for
that by explaining that the ImgU can only deal with bayer formats at its
input, but I don't think we'll ever see an IPU3-based system with a YUV
sensor.

We could also talk about whether the sensor can be handled by the ImgU
instead of whether it needs the ImgU, as capturing raw frames only
without running any IPA algorithm can also be done with bayer sensors.
As with my other point, this is likely a niche use cases.

I'm fine with the text above.

> +	 */
> +	needsImgu_ = false;
> +	for (unsigned int mbusCode : sensorCodes) {
> +		const BayerFormat &bayerFormat = BayerFormat::fromMbusCode(mbusCode);
> +		if (bayerFormat.isValid() && bayerFormat.order < BayerFormat::MONO) {

Could we white-list the formats supported by the ImgU instead ? The ImgU
supports 10-bit bayer formats only, while the BayerFormat class supports
more.

> +			needsImgu_ = true;
> +			break;
> +		}
> +	}
> +
>  	/*
>  	 * \todo Define when to open and close video device nodes, as they
>  	 * might impact on power consumption.
> diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
> index bbd87eb8..064673d9 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.h
> +++ b/src/libcamera/pipeline/ipu3/cio2.h
> @@ -63,6 +63,8 @@ public:
>  
>  	Signal<> bufferAvailable;
>  
> +	bool needsImgu() { return needsImgu_; }
> +
>  private:
>  	void freeBuffers();
>  
> @@ -74,6 +76,8 @@ private:
>  
>  	std::vector<std::unique_ptr<FrameBuffer>> buffers_;
>  	std::queue<FrameBuffer *> availableBuffers_;
> +
> +	bool needsImgu_;
>  };
>  
>  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart