[libcamera-devel] [RFC 6/6] libcamera: ipu3: Add support for a RAW still capture stream
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Mar 23 14:32:56 CET 2020
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 at 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);
> + }
> }
>
> /* -----------------------------------------------------------------------------
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list