[libcamera-devel,RFC,6/6] libcamera: ipu3: Add support for a RAW still capture stream

3115 diff mbox series

Allow the RAW buffer cycling between CIO2 and IMGU to be memory copied
to a new FrameBuffer in a new RAW stream. This allows users to capture
the raw Bayer format coming from the sensor.

As the RAW frame is memory copied queueing requests with the
StillCaptureRaw stream might impact performance.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 83 +++++++++++++++++++++++++---
 1 file changed, 75 insertions(+), 8 deletions(-)

Hi Niklas,

Thank you for the patch.

On Mon, Mar 16, 2020 at 03:41:46AM +0100, Niklas Söderlund wrote:
> Allow the RAW buffer cycling between CIO2 and IMGU to be memory copied
> to a new FrameBuffer in a new RAW stream. This allows users to capture
> the raw Bayer format coming from the sensor.
> 
> As the RAW frame is memory copied queueing requests with the
> StillCaptureRaw stream might impact performance.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 83 +++++++++++++++++++++++++---
>  1 file changed, 75 insertions(+), 8 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index dbb0c4328aa0efe5..18d9ad4d7318a034 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -142,11 +142,12 @@ class IPU3Stream : public Stream
>  {
>  public:
>  	IPU3Stream()
> -		: active_(false), device_(nullptr)
> +		: active_(false), raw_(false), device_(nullptr)
>  	{
>  	}
>  
>  	bool active_;
> +	bool raw_;
>  	std::string name_;
>  	ImgUDevice::ImgUOutput *device_;
>  };
> @@ -170,6 +171,7 @@ public:
>  
>  	IPU3Stream outStream_;
>  	IPU3Stream vfStream_;
> +	IPU3Stream rawStream_;
>  };
>  
>  class IPU3CameraConfiguration : public CameraConfiguration
> @@ -308,8 +310,8 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  		return Invalid;
>  
>  	/* Cap the number of entries to the available streams. */
> -	if (config_.size() > 2) {
> -		config_.resize(2);
> +	if (config_.size() > 3) {

Time to add a constexpr unsigned int MAX_STREAMS = 3 ?

> +		config_.resize(3);
>  		status = Adjusted;
>  	}
>  
> @@ -322,6 +324,8 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  	Size size = {};
>  
>  	for (const StreamConfiguration &cfg : config_) {
> +		if (cfg.pixelFormat.modifiers().count(IPU3_FORMAT_MOD_PACKED))
> +			continue;

This calls for some comments I think, I'm not sure to understand the
logic. The validate() function is getting hard to read, I think we'll
have to refactor it in the near future.

>  		if (cfg.size.width > size.width)
>  			size.width = cfg.size.width;
>  		if (cfg.size.height > size.height)
> @@ -349,6 +353,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  	std::set<const IPU3Stream *> availableStreams = {
>  		&data_->outStream_,
>  		&data_->vfStream_,
> +		&data_->rawStream_,
>  	};
>  
>  	streams_.clear();
> @@ -360,7 +365,9 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  		const Size size = cfg.size;
>  		const IPU3Stream *stream;
>  
> -		if (cfg.size == sensorFormat_.size)
> +		if (cfg.pixelFormat.modifiers().count(IPU3_FORMAT_MOD_PACKED))
> +			stream = &data_->rawStream_;
> +		else if (cfg.size == sensorFormat_.size)
>  			stream = &data_->outStream_;
>  		else
>  			stream = &data_->vfStream_;
> @@ -371,8 +378,15 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  		LOG(IPU3, Debug)
>  			<< "Assigned '" << stream->name_ << "' to stream " << i;
>  
> -		bool scale = stream == &data_->vfStream_;
> -		adjustStream(config_[i], scale);
> +		if (stream == &data_->rawStream_) {

Maybe if stream->raw_ ? Or maybe it make no difference.

> +			cfg.pixelFormat = PixelFormat(DRM_FORMAT_SRGGB10,

What if the sensor produces a different Bayer pattern ?

> +						      { IPU3_FORMAT_MOD_PACKED });
> +			cfg.size = sensor->resolution();
> +			cfg.bufferCount = IPU3_BUFFER_COUNT;
> +		} else {
> +			bool scale = stream == &data_->vfStream_;
> +			adjustStream(config_[i], scale);
> +		}
>  
>  		if (cfg.pixelFormat != pixelFormat || cfg.size != size) {
>  			LOG(IPU3, Debug)
> @@ -401,6 +415,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
>  	std::set<IPU3Stream *> streams = {
>  		&data->outStream_,
>  		&data->vfStream_,
> +		&data->rawStream_,
>  	};
>  
>  	config = new IPU3CameraConfiguration(camera, data);
> @@ -442,6 +457,21 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
>  
>  			break;
>  
> +		case StreamRole::StillCaptureRaw: {
> +			if (streams.find(&data->rawStream_) == streams.end()) {
> +				LOG(IPU3, Error)
> +					<< "No stream available for requested role "
> +					<< role;

Can this happen ? Maybe if the raw role is specified multiple times ?

> +				break;
> +			}
> +
> +			stream = &data->rawStream_;
> +
> +			cfg.pixelFormat = PixelFormat(DRM_FORMAT_SRGGB10, { IPU3_FORMAT_MOD_PACKED });
> +			cfg.size = data->cio2_.sensor_->resolution();
> +			break;
> +		}
> +
>  		case StreamRole::Viewfinder:
>  		case StreamRole::VideoRecording: {
>  			/*
> @@ -575,6 +605,9 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  		stream->active_ = true;
>  		cfg.setStream(stream);
>  
> +		if (stream->raw_)
> +			continue;
> +
>  		ret = imgu->configureOutput(stream->device_, cfg);
>  		if (ret)
>  			return ret;
> @@ -625,9 +658,15 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,
>  					    std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>  {
> +	IPU3CameraData *data = cameraData(camera);
>  	IPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream);
> -	V4L2VideoDevice *video = ipu3stream->device_->dev;
>  	unsigned int count = stream->configuration().bufferCount;
> +	V4L2VideoDevice *video;
> +
> +	if (ipu3stream->raw_)
> +		video = data->cio2_.output_;
> +	else
> +		video = ipu3stream->device_->dev;
>  
>  	return video->exportBuffers(count, buffers);
>  }
> @@ -740,6 +779,9 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
>  		IPU3Stream *stream = static_cast<IPU3Stream *>(it.first);
>  		buffer = it.second;
>  
> +		if (stream->raw_)
> +			continue;

This needs a comment too. Overall we need to start documenting this code
properly, it's becoming unreadable :-S

> +
>  		int ret = stream->device_->dev->queueBuffer(buffer);
>  		if (ret < 0)
>  			error = ret;
> @@ -834,6 +876,7 @@ int PipelineHandlerIPU3::registerCameras()
>  		std::set<Stream *> streams = {
>  			&data->outStream_,
>  			&data->vfStream_,
> +			&data->rawStream_,
>  		};
>  		CIO2Device *cio2 = &data->cio2_;
>  
> @@ -855,6 +898,8 @@ int PipelineHandlerIPU3::registerCameras()
>  		data->outStream_.name_ = "output";
>  		data->vfStream_.device_ = &data->imgu_->viewfinder_;
>  		data->vfStream_.name_ = "viewfinder";
> +		data->rawStream_.raw_ = true;
> +		data->rawStream_.name_ = "raw";
>  
>  		/*
>  		 * Connect video devices' 'bufferReady' signals to their
> @@ -946,7 +991,29 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
>  	if (buffer->metadata().status == FrameMetadata::FrameCancelled)
>  		return;
>  
> -	imgu_->input_->queueBuffer(buffer);
> +	Request *request = cio2ToRequest_[buffer];
> +	FrameBuffer *raw = request->findBuffer(&rawStream_);
> +
> +	if (!raw) {
> +		/* No RAW buffers present, just queue to IMGU. */
> +		imgu_->input_->queueBuffer(buffer);
> +		return;
> +	}
> +
> +	/* RAW buffers present, special care is needed. */
> +	if (request->buffers().size() > 1)
> +		imgu_->input_->queueBuffer(buffer);
> +
> +	if (FrameBufferMemCopy(raw, buffer))
> +		LOG(IPU3, Debug) << "Memcopy of FrameBuffer Failed";

I suppose that's OK for now, but it's a costly operation, we'll likely
have to defer this to a separate thread.

Shouldn't you set the raw buffer status to error if the copy fails ?

> +
> +	pipe_->completeBuffer(camera_, request, raw);
> +
> +	if (request->buffers().size() == 1) {
> +		cio2ToRequest_.erase(buffer);
> +		cio2_.putBuffer(buffer);
> +		pipe_->completeRequest(camera_, request);
> +	}
>  }
>  
>  /* -----------------------------------------------------------------------------