[libcamera-devel] [PATCH 7/7] libcamera: ipu3: Add support for a RAW still capture stream
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Mar 26 15:34:09 CET 2020
Hi Niklas,
Thank you for the patch.
On Tue, Mar 24, 2020 at 04:51:45PM +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>
> ---
> * Changes from RFC
> - Add definition for IPU3_MAX_STREAMS.
> - Deal with all IPU3 Bayer patterns.
> - Rework size logic.
> ---
> src/libcamera/pipeline/ipu3/ipu3.cpp | 99 +++++++++++++++++++++++++---
> 1 file changed, 91 insertions(+), 8 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index cc602834f24108a7..c1b143e58d0a536e 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -140,11 +140,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_;
> };
> @@ -166,6 +167,7 @@ public:
>
> IPU3Stream outStream_;
> IPU3Stream vfStream_;
> + IPU3Stream rawStream_;
> };
>
> class IPU3CameraConfiguration : public CameraConfiguration
> @@ -180,6 +182,7 @@ public:
>
> private:
> static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> + static constexpr unsigned int IPU3_MAX_STREAMS = 3;
>
> void adjustStream(StreamConfiguration &cfg, bool scale);
>
> @@ -304,8 +307,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() > IPU3_MAX_STREAMS) {
> + config_.resize(IPU3_MAX_STREAMS);
> status = Adjusted;
> }
>
> @@ -345,6 +348,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> std::set<const IPU3Stream *> availableStreams = {
> &data_->outStream_,
> &data_->vfStream_,
> + &data_->rawStream_,
> };
>
> streams_.clear();
> @@ -356,7 +360,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_;
> @@ -367,8 +373,32 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> LOG(IPU3, Debug)
> << "Assigned '" << stream->name_ << "' to stream " << i;
>
> - bool scale = stream == &data_->vfStream_;
> - adjustStream(config_[i], scale);
> + if (stream->raw_) {
> + unsigned int drmfourcc;
> + switch (sensorFormat_.mbus_code) {
> + case MEDIA_BUS_FMT_SBGGR10_1X10:
> + drmfourcc = DRM_FORMAT_SBGGR10;
> + break;
> + case MEDIA_BUS_FMT_SGBRG10_1X10:
> + drmfourcc = DRM_FORMAT_SGBRG10;
> + break;
> + case MEDIA_BUS_FMT_SGRBG10_1X10:
> + drmfourcc = DRM_FORMAT_SGRBG10;
> + break;
> + case MEDIA_BUS_FMT_SRGGB10_1X10:
> + drmfourcc = DRM_FORMAT_SRGGB10;
> + break;
Put this in a statically declared std::map<uint32_t, PixelFormat>, it
will not only simplify lookup but also allow reuse in
generateConfiguration().
> + default:
> + return Invalid;
> + }
> + cfg.pixelFormat = PixelFormat(drmfourcc,
> + { IPU3_FORMAT_MOD_PACKED });
> + cfg.size = sensorFormat_.size;
> + cfg.bufferCount = IPU3_BUFFER_COUNT;
I would move this line after the else below, and remove the
corresponding linve from adjustStream().
> + } else {
> + bool scale = stream == &data_->vfStream_;
> + adjustStream(config_[i], scale);
> + }
>
> if (cfg.pixelFormat != pixelFormat || cfg.size != size) {
> LOG(IPU3, Debug)
> @@ -397,6 +427,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> std::set<IPU3Stream *> streams = {
> &data->outStream_,
> &data->vfStream_,
> + &data->rawStream_,
> };
>
> config = new IPU3CameraConfiguration(camera, data);
> @@ -438,6 +469,21 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
>
> break;
>
> + case StreamRole::StillCaptureRaw: {
As you don't declare local variables, the { } are not necessary.
> + if (streams.find(&data->rawStream_) == streams.end()) {
> + LOG(IPU3, Error)
> + << "No stream available for requested role "
> + << role;
> + break;
> + }
> +
> + stream = &data->rawStream_;
> +
> + cfg.pixelFormat = PixelFormat(DRM_FORMAT_SRGGB10, { IPU3_FORMAT_MOD_PACKED });
Line folding ?
You should look up the DRM format based on the Bayer pattern produced by
the sensor, exactly as in validate().
> + cfg.size = data->cio2_.sensor_->resolution();
> + break;
> + }
> +
> case StreamRole::Viewfinder:
> case StreamRole::VideoRecording: {
> /*
> @@ -571,6 +617,9 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> stream->active_ = true;
> cfg.setStream(stream);
>
I think this is worth a comment to explain why.
/*
* The RAW still capture stream just copies buffers from the
* internal queue and doesn't need any specific configuration.
*/
How about the interesting corner case of an application requesting the
raw stream only ? :-) This can be fixed later, but please add a \todo.
> + if (stream->raw_)
> + continue;
> +
> ret = imgu->configureOutput(stream->device_, cfg);
> if (ret)
> return ret;
> @@ -621,9 +670,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;
I don't like special cases much, but I think it's OK for now, we'll do
more refactoring later.
>
> return video->exportBuffers(count, buffers);
> }
> @@ -737,6 +792,10 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
> IPU3Stream *stream = static_cast<IPU3Stream *>(it.first);
> buffer = it.second;
>
> + /* Skip raw streams, they are copied from the CIO2 buffer. */
> + if (stream->raw_)
> + continue;
> +
> int ret = stream->device_->dev->queueBuffer(buffer);
> if (ret < 0)
> error = ret;
> @@ -831,6 +890,7 @@ int PipelineHandlerIPU3::registerCameras()
> std::set<Stream *> streams = {
> &data->outStream_,
> &data->vfStream_,
> + &data->rawStream_,
> };
> CIO2Device *cio2 = &data->cio2_;
>
> @@ -852,6 +912,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
> @@ -941,7 +1003,28 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
> if (buffer->metadata().status == FrameMetadata::FrameCancelled)
> return;
>
> - imgu_->input_->queueBuffer(buffer);
> + Request *request = buffer->request();
> + 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 (raw->copyFrom(buffer))
> + LOG(IPU3, Debug) << "Memcopy of FrameBuffer Failed";
s/Memcopy/Copy/ ?
s/Failed/failed/
Pretty nice overall, only small changes needed :-)
> +
> + pipe_->completeBuffer(camera_, request, raw);
> +
> + if (request->buffers().size() == 1) {
> + cio2_.putBuffer(buffer);
> + pipe_->completeRequest(camera_, request);
> + }
> }
>
> /* -----------------------------------------------------------------------------
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list