[libcamera-devel] [PATCH v3 2/8] libcamera: ipu3: Create camera with 2 streams
Jacopo Mondi
jacopo at jmondi.org
Mon Apr 8 09:54:08 CEST 2019
Hi Laurent,
thanks for review,
On Fri, Apr 05, 2019 at 06:44:05PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Apr 03, 2019 at 05:07:29PM +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, keep track of which streams have
> > been requested by the application, and configure 'output', 'viewfinder'
> > and 'stat' regardless of the user requests.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 216 +++++++++++++++++++++------
> > 1 file changed, 170 insertions(+), 46 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 164e187c769d..caf1051c58ab 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -171,17 +171,61 @@ private:
> > CIO2Device cio2_;
> > ImgUDevice *imgu_;
> >
> > - Stream stream_;
> > + Stream outStream_;
> > + Stream vfStream_;
> > +
> > + unsigned int activeStreamsMask;
> > };
> >
> > static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> >
> > + static constexpr unsigned int IPU3_STREAM_OUTPUT = BIT(0);
> > + static constexpr unsigned int IPU3_STREAM_VF = BIT(1);
> > +
> > IPU3CameraData *cameraData(const Camera *camera)
> > {
> > return static_cast<IPU3CameraData *>(
> > PipelineHandler::cameraData(camera));
> > }
> >
> > + bool isOutput(IPU3CameraData *data, Stream *stream)
> > + {
> > + return &data->outStream_ == stream;
> > + }
>
> You're missing blank lines between functions.
>
That was a style choice, to make those functions look more 'compact'
together. But since both you and Niklas pointed this out, I'll change
it..
> > + bool isOutputActive(IPU3CameraData *data)
> > + {
> > + return (data->activeStreamsMask & IPU3_STREAM_OUTPUT) ?
> > + true : false;
> > + }
> > + void setOutputActive(IPU3CameraData *data)
> > + {
> > + data->activeStreamsMask |= IPU3_STREAM_OUTPUT;
> > + }
> > + bool isViewfinder(IPU3CameraData *data, Stream *stream)
> > + {
> > + return &data->vfStream_ == stream;
> > + }
> > + bool isViewfinderActive(IPU3CameraData *data)
> > + {
> > + return (data->activeStreamsMask & IPU3_STREAM_VF) ?
> > + true : false;
> > + }
> > + void setViewfinderActive(IPU3CameraData *data)
> > + {
> > + data->activeStreamsMask |= IPU3_STREAM_VF;
> > + }
> > + bool isStreamActive(IPU3CameraData *data, Stream *stream)
> > + {
> > + if (isOutput(data, stream) &&
> > + isOutputActive(data))
> > + return true;
> > + if (isViewfinder(data, stream) &&
> > + isViewfinderActive(data))
> > + return true;
>
> How about subclassing the Stream class and adding an active flag,
> instead of storing that in a mask at the camera data level ? I think
> you'll have more data to store in the IPU3CameraStream class.
>
By sub-classing Stream I can collect per-Stream informations, like an
'active' flag, what I need here is a global status that makes possible
to do choices like "should I configure viewfinder now that I'm
configuring output too, as viewfinder is not part of the active stream
list?"
True I can store a per-stream flag and check for that in each stream
the pipeline handler supports. I'll see how many other things could
fit in a Stream sub-class and consider this option, thanks!
> > +
> > + return false;
> > + }
> > +
> > int registerCameras();
> >
> > ImgUDevice imgu0_;
> > @@ -210,7 +254,7 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera,
> > {
> > std::map<Stream *, StreamConfiguration> configs;
> > IPU3CameraData *data = cameraData(camera);
> > - StreamConfiguration *config = &configs[&data->stream_];
> > + StreamConfiguration config = {};
> >
> > /*
> > * FIXME: Soraka: the maximum resolution reported by both sensors
> > @@ -220,52 +264,93 @@ 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;
>
> A bit inefficient as you end up copying the data twice, but probably
> not the end of the world.
>
> > + LOG(IPU3, Debug)
> > + << "Stream 'output' format set to " << config.width << "x"
> > + << config.height << "-0x" << std::hex << std::setfill('0')
> > + << std::setw(8) << config.pixelFormat;
>
> If you also stored the stream name in the IPU3CameraStream (or
> IPU3Stream ?) class I think you could avoid code duplication.
>
> >
> > + configs[&data->vfStream_] = config;
>
> Can both streams scale ?
>
Can't they? Why is it relevant here?
> > LOG(IPU3, Debug)
> > - << "Stream format set to " << config->width << "x"
> > - << config->height << "-0x" << std::hex << std::setfill('0')
> > - << std::setw(8) << config->pixelFormat;
> > + << "Stream 'viewfinder' format set to " << config.width << "x"
> > + << config.height << "-0x" << std::hex << std::setfill('0')
> > + << std::setw(8) << config.pixelFormat;
> >
> > return configs;
> > }
> >
> > int PipelineHandlerIPU3::configureStreams(Camera *camera,
> > - std::map<Stream *, StreamConfiguration> &config)
> > + std::map<Stream *,
> > + StreamConfiguration> &config)
>
> That doesn't look nice, I think you could keep the longer line instead.
>
I like 80-cols :(
> > {
> > IPU3CameraData *data = cameraData(camera);
> > - const StreamConfiguration &cfg = config[&data->stream_];
> > + StreamConfiguration CIO2Config = {};
>
> Variables should start with lower case.
>
Ack.. cio2Config then
> > CIO2Device *cio2 = &data->cio2_;
> > ImgUDevice *imgu = data->imgu_;
> > 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() << "'";
> > + /* Remove previously configured stream masks to store the new ones. */
> > + data->activeStreamsMask = 0;
> > + for (auto const &streamConfig : config) {
> > + Stream *stream = streamConfig.first;
> > + const StreamConfiguration &cfg = streamConfig.second;
> >
> > - /*
> > - * 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 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"
>
> While at it, s/consider/Consider/ and s/32"/32"./
>
> > + */
> > + 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)
> > + << "Requested image format " << cfg.width << "x"
> > + << cfg.height << "-0x" << std::hex << std::setw(8)
> > + << cfg.pixelFormat << " on camera'"
> > + << camera->name() << "'";
>
> That will be a bit confusing if you don't print the stream name, again,
> with a Stream subclass, you could easily get the name :-)
>
> > +
> > + /*
> > + * FIXME: As viewfinder should be operated even when
> > + * applications do not intend to use it, we need to keep track
> > + * of which streams have to be configured, to make meaningful
> > + * decisions at configure and request queueing time.
> > + *
> > + * Walk here all the streams to configure and collect the
> > + * active ones in a bitmaks.
> > + */
> > + if (isOutput(data, stream))
> > + setOutputActive(data);
> > + if (isViewfinder(data, stream))
> > + setViewfinderActive(data);
> > +
> > + /*
> > + * 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;
>
> Sounds good, but it's a bit strange to store that in a
> StreamConfiguration, given that you only use it to configure the CIO2
> and the ImgU input. How about replacing that by Size ?
>
That's also a possibility, I only use the width and height (I now
wonder what should I do with the configuration's pixelformat fields though)
> > }
> >
> > /*
> > @@ -281,26 +366,62 @@ 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;
> > + for (auto const &streamConfig : config) {
> > + Stream *stream = streamConfig.first;
> > + const StreamConfiguration &cfg = streamConfig.second;
> >
> > - ret = imgu->configureOutput(&imgu->viewfinder_, cfg);
> > - if (ret)
> > - return ret;
> > + if (isOutput(data, stream)) {
> > + ret = imgu->configureOutput(&imgu->output_, cfg);
> > + if (ret)
> > + return ret;
> >
> > - ret = imgu->configureOutput(&imgu->stat_, cfg);
> > - if (ret)
> > - return ret;
> > + ret = imgu->configureOutput(&imgu->stat_, cfg);
> > + if (ret)
> > + return ret;
> > +
> > +
> > + if (isViewfinderActive(data))
> > + continue;
> > +
> > + /*
> > + * Configure viewfinder using the output stream
> > + * configuration if it is not part of the active
> > + * streams list.
> > + */
> > + ret = imgu->configureOutput(&imgu->viewfinder_, cfg);
> > + if (ret)
> > + return ret;
> > + } else if (isViewfinder(data, stream)) {
> > + ret = imgu->configureOutput(&imgu->viewfinder_, cfg);
> > + if (ret)
> > + return ret;
> > +
> > + if (isOutputActive(data))
> > + continue;
> > +
> > + /*
> > + * Configure output using the viewfinder stream
> > + * configuration if it is not part of the active
> > + * streams list.
> > + */
> > + ret = imgu->configureOutput(&imgu->output_, cfg);
> > + if (ret)
> > + return ret;
> > +
> > + ret = imgu->configureOutput(&imgu->stat_, cfg);
> > + if (ret)
> > + return ret;
> > + }
> > + }
>
> This looks quite complicated. I think you can configure the streams
> included in the configuration in the loop without caring about their
> type, and then configure the streams that are not included outside of
> the loop. The code would look simpler.
>
Niklas had a similar idea, I'll try to see how it looks like.
Thanks
j
> >
> > return 0;
> > }
> > @@ -404,7 +525,7 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)
> > {
> > IPU3CameraData *data = cameraData(camera);
> > V4L2Device *output = data->imgu_->output_.dev;
> > - Stream *stream = &data->stream_;
> > + Stream *stream = &data->outStream_;
> >
> > /* Queue a buffer to the ImgU output for capture. */
> > Buffer *buffer = request->findBuffer(stream);
> > @@ -554,7 +675,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);
>
> --
> Regards,
>
> Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190408/f1f056bb/attachment.sig>
More information about the libcamera-devel
mailing list