[libcamera-devel] [PATCH v4 02/12] libcamera: ipu3: Create camera with 2 streams
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Apr 15 13:08:33 CEST 2019
Hi Jacopo,
Thank you for the patch.
On Tue, Apr 09, 2019 at 09:25:38PM +0200, Jacopo Mondi wrote:
> Create each IPU3 camera with two streams: 'output' and 'viewfinder'
> which represents the video stream from main and secondary ImgU output
> respectively.
>
> Re-work stream configuration to handle the two video streams 'output'
> and 'viewfinder' separately.
>
> As the IPU3 driver requires viewfinder and stat video nodes to be
> started not to stall ImgU processing: for this reason configure
s/As the/The/ or s/: for this reason/,/
> 'output', 'viewfinder' and 'stat' regardless of the user requested active
s/user requested/user-requested/
> streams.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> src/libcamera/pipeline/ipu3/ipu3.cpp | 180 +++++++++++++++++++--------
> 1 file changed, 125 insertions(+), 55 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 00907bb53891..712e57c5a459 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -61,7 +61,7 @@ public:
> }
>
> int init(MediaDevice *media, unsigned int index);
> - int configureInput(const StreamConfiguration &config,
> + int configureInput(const Size &config,
> V4L2DeviceFormat *inputFormat);
> int configureOutput(ImgUOutput *output,
> const StreamConfiguration &config);
> @@ -111,7 +111,7 @@ public:
> }
>
> int init(const MediaDevice *media, unsigned int index);
> - int configure(const StreamConfiguration &config,
> + int configure(const Size &config,
> V4L2DeviceFormat *outputFormat);
>
> BufferPool *exportBuffers();
> @@ -137,12 +137,14 @@ class IPU3Stream : public Stream
> {
> public:
> IPU3Stream()
> - : active_(false)
> + : active_(false), device_(nullptr), cfg_(nullptr)
> {
> }
>
> bool active_;
> std::string name_;
> + ImgUDevice::ImgUOutput *device_;
> + const StreamConfiguration *cfg_;
> };
>
> class PipelineHandlerIPU3 : public PipelineHandler
> @@ -183,7 +185,8 @@ private:
> CIO2Device cio2_;
> ImgUDevice *imgu_;
>
> - IPU3Stream stream_;
> + IPU3Stream outStream_;
> + IPU3Stream vfStream_;
> };
>
> static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> @@ -222,7 +225,7 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera,
> {
> CameraConfiguration configs;
> IPU3CameraData *data = cameraData(camera);
> - StreamConfiguration *config = &configs[&data->stream_];
> + StreamConfiguration config = {};
>
> /*
> * FIXME: Soraka: the maximum resolution reported by both sensors
> @@ -232,15 +235,22 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera,
> *
> * \todo Clarify ImgU alignement requirements.
> */
> - config->width = 2560;
> - config->height = 1920;
> - config->pixelFormat = V4L2_PIX_FMT_NV12;
> - config->bufferCount = IPU3_BUFFER_COUNT;
> + config.width = 2560;
> + config.height = 1920;
> + config.pixelFormat = V4L2_PIX_FMT_NV12;
> + config.bufferCount = IPU3_BUFFER_COUNT;
>
> + configs[&data->outStream_] = config;
> LOG(IPU3, Debug)
> - << "Stream format set to " << config->width << "x"
> - << config->height << "-0x" << std::hex << std::setfill('0')
> - << std::setw(8) << config->pixelFormat;
> + << "Stream 'output' format set to " << config.width << "x"
> + << config.height << "-0x" << std::hex << std::setfill('0')
> + << std::setw(8) << config.pixelFormat;
> +
> + configs[&data->vfStream_] = config;
> + LOG(IPU3, Debug)
> + << "Stream 'viewfinder' format set to " << config.width << "x"
Shouldn't this be 'output' instead of 'viewfinder' as you set the
configuration for data->outStream_ ? You could use the stream name
instead of a hardcoded string to fix this.
> + << config.height << "-0x" << std::hex << std::setfill('0')
> + << std::setw(8) << config.pixelFormat;
>
> return configs;
> }
> @@ -249,35 +259,59 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
> const CameraConfiguration &config)
> {
> IPU3CameraData *data = cameraData(camera);
> - const StreamConfiguration &cfg = config[&data->stream_];
> + IPU3Stream *outStream = &data->outStream_;
> + IPU3Stream *vfStream = &data->vfStream_;
> CIO2Device *cio2 = &data->cio2_;
> ImgUDevice *imgu = data->imgu_;
> + Size cio2Config = {};
s/cio2Config/cio2Size/
Or maybe sensorSize ?
> int ret;
>
> - LOG(IPU3, Info)
> - << "Requested image format " << cfg.width << "x"
> - << cfg.height << "-0x" << std::hex << std::setfill('0')
> - << std::setw(8) << cfg.pixelFormat << " on camera '"
> - << camera->name() << "'";
> + outStream->active_ = false;
> + vfStream->active_ = false;
The Stream class has a StreamConfiguration member. I wonder if we could
reuse that instead of adding an active_ field, by making the
configuration invalid to signal that the stream is not active. That way
all streams could be reset to inactive in the Camera class instead of
manually in the pipeline handler. This could be done on top of this
series too.
> + for (const auto &s : config) {
for (Stream *s : config) {
> + IPU3Stream *stream = static_cast<IPU3Stream *>(s);
> + const StreamConfiguration &cfg = config[stream];
>
> - /*
> - * Verify that the requested size respects the IPU3 alignement
> - * requirements (the image width shall be a multiple of 8 pixels and
> - * its height a multiple of 4 pixels) and the camera maximum sizes.
> - *
> - * \todo: consider the BDS scaling factor requirements:
> - * "the downscaling factor must be an integer value multiple of 1/32"
> - */
> - if (cfg.width % 8 || cfg.height % 4) {
> - LOG(IPU3, Error) << "Invalid stream size: bad alignment";
> - return -EINVAL;
> - }
> + /*
> + * Verify that the requested size respects the IPU3 alignment
> + * requirements (the image width shall be a multiple of 8
> + * pixels and its height a multiple of 4 pixels) and the camera
> + * maximum sizes.
> + *
> + * \todo: Consider the BDS scaling factor requirements: "the
> + * downscaling factor must be an integer value multiple of
> + * 1/32"
You can move this to the end of the previous line.
> + */
> + if (cfg.width % 8 || cfg.height % 4) {
> + LOG(IPU3, Error)
> + << "Invalid stream size: bad alignment";
> + return -EINVAL;
> + }
>
> - if (cfg.width > cio2->maxSize_.width ||
> - cfg.height > cio2->maxSize_.height) {
> - LOG(IPU3, Error)
> - << "Invalid stream size: larger than sensor resolution";
> - return -EINVAL;
> + if (cfg.width > cio2->maxSize_.width ||
> + cfg.height > cio2->maxSize_.height) {
> + LOG(IPU3, Error)
> + << "Invalid stream size: larger than sensor resolution";
> + return -EINVAL;
> + }
> +
> + LOG(IPU3, Info)
> + << "Stream '" << stream->name_ << "' "
> + << cfg.width << "x" << cfg.height << "-0x"
> + << std::hex << std::setw(8) << cfg.pixelFormat
> + << " on camera'" << camera->name() << "'";
I have a patch in my rkisp1 pipeline handler implementation that moves
this to the caller, as the code is common to all pipeline handlers. If
we want to print the stream name we need to move it to the Stream class.
Do you think that would be useful ?
> + /*
> + * Collect the maximum width and height: IPU3 can downscale
> + * only.
> + */
> + if (cfg.width > cio2Config.width)
> + cio2Config.width = cfg.width;
> + if (cfg.height > cio2Config.height)
> + cio2Config.height = cfg.height;
> +
> + stream->active_ = true;
> + stream->cfg_ = &cfg;
> }
>
> /*
> @@ -293,24 +327,51 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
> * adjusted format to be propagated to the ImgU output devices.
> */
> V4L2DeviceFormat cio2Format = {};
> - ret = cio2->configure(cfg, &cio2Format);
> + ret = cio2->configure(cio2Config, &cio2Format);
> if (ret)
> return ret;
>
> - ret = imgu->configureInput(cfg, &cio2Format);
> + ret = imgu->configureInput(cio2Config, &cio2Format);
> if (ret)
> return ret;
>
> - /* Apply the format to the ImgU output, viewfinder and stat. */
> - ret = imgu->configureOutput(&imgu->output_, cfg);
> - if (ret)
> - return ret;
> + /* Apply the format to the configured streams output devices. */
> + for (auto const &s : config) {
for (Stream *s : config) {
> + IPU3Stream *stream = static_cast<IPU3Stream *>(s);
>
> - ret = imgu->configureOutput(&imgu->viewfinder_, cfg);
> - if (ret)
> - return ret;
> + ret = imgu->configureOutput(stream->device_, *stream->cfg_);
> + if (ret)
> + return ret;
> + }
>
> - ret = imgu->configureOutput(&imgu->stat_, cfg);
> + /*
> + * As we need to set format also on the non-active streams, use
> + * the configuration of the active one for that purpose (there should
> + * be at least one active stream in the configuration request).
> + */
> + if (!outStream->active_) {
> + ret = imgu->configureOutput(outStream->device_,
> + *vfStream->cfg_);
> + if (ret)
> + return ret;
> + }
> +
> + if (!vfStream->active_) {
> + ret = imgu->configureOutput(vfStream->device_,
> + *outStream->cfg_);
> + if (ret)
> + return ret;
> + }
I like this much better than the previous version, thank you for the
great rework.
> +
> + /*
> + * Apply the largest available format to the stat node.
> + * \todo Revise this when we'll actually use the stat node.
> + */
> + StreamConfiguration statConfig = {};
> + statConfig.width = cio2Format.width;
> + statConfig.height = cio2Format.height;
> +
> + ret = imgu->configureOutput(&imgu->stat_, statConfig);
> if (ret)
> return ret;
>
> @@ -416,7 +477,7 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)
> {
> IPU3CameraData *data = cameraData(camera);
> V4L2Device *output = data->imgu_->output_.dev;
> - IPU3Stream *stream = &data->stream_;
> + IPU3Stream *stream = &data->outStream_;
>
> /* Queue a buffer to the ImgU output for capture. */
> Buffer *buffer = request->findBuffer(stream);
> @@ -567,7 +628,10 @@ int PipelineHandlerIPU3::registerCameras()
> for (unsigned int id = 0; id < 4 && numCameras < 2; ++id) {
> std::unique_ptr<IPU3CameraData> data =
> utils::make_unique<IPU3CameraData>(this);
> - std::set<Stream *> streams{ &data->stream_ };
> + std::set<Stream *> streams = {
> + &data->outStream_,
> + &data->vfStream_,
> + };
> CIO2Device *cio2 = &data->cio2_;
>
> ret = cio2->init(cio2MediaDev_.get(), id);
> @@ -575,11 +639,14 @@ int PipelineHandlerIPU3::registerCameras()
> continue;
>
> /**
> - * \todo Dynamically assign ImgU devices; as of now, limit
> - * support to two cameras only, and assign imgu0 to the first
> - * one and imgu1 to the second.
> + * \todo Dynamically assign ImgU and output devices to each
> + * stream and camera; as of now, limit support to two cameras
> + * only, and assign imgu0 to the first one and imgu1 to the
> + * second.
> */
> data->imgu_ = numCameras ? &imgu1_ : &imgu0_;
> + data->outStream_.device_ = &data->imgu_->output_;
> + data->vfStream_.device_ = &data->imgu_->viewfinder_;
>
> /*
> * Connect video devices' 'bufferReady' signals to their
> @@ -596,7 +663,10 @@ int PipelineHandlerIPU3::registerCameras()
> data->imgu_->output_.dev->bufferReady.connect(data.get(),
> &IPU3CameraData::imguOutputBufferReady);
>
> - /* Create and register the Camera instance. */
> +
Extra blank line.
> + /* Initialize and register the Camera and its streams. */
> + data->outStream_.name_ = "output";
> + data->vfStream_.name_ = "viewfinder";
I would move those two lines to the previous hunk to group all
initialisation of IPU3Stream (and thus remove the comment change in this
hunk).
> std::string cameraName = cio2->sensor_->entityName() + " "
> + std::to_string(id);
> std::shared_ptr<Camera> camera = Camera::create(this,
> @@ -730,12 +800,12 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)
>
> /**
> * \brief Configure the ImgU unit input
> - * \param[in] config The requested stream configuration
> + * \param[in] config The requested stream sizes
s/The requested stream sizes/The ImgU input frame size/
Let's rename the parameter from config to size.
> * \param[in] inputFormat The format to be applied to ImgU input
> *
> * \return 0 on success or a negative error code otherwise
> */
> -int ImgUDevice::configureInput(const StreamConfiguration &config,
> +int ImgUDevice::configureInput(const Size &config,
> V4L2DeviceFormat *inputFormat)
> {
> /* Configure the ImgU input video device with the requested sizes. */
> @@ -1096,12 +1166,12 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
>
> /**
> * \brief Configure the CIO2 unit
> - * \param[in] config The requested configuration
> + * \param[in] config The requested image sizes
s/config The requested stream sizes/size The CIO2 output frame size/
> * \param[out] outputFormat The CIO2 unit output image format
> *
> * \return 0 on success or a negative error code otherwise
> */
> -int CIO2Device::configure(const StreamConfiguration &config,
> +int CIO2Device::configure(const Size &config,
> V4L2DeviceFormat *outputFormat)
> {
> unsigned int imageSize = config.width * config.height;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list