[libcamera-devel] [PATCH 08/13] libcamera: ipu3: imgu: Use specific functions to configure each sink
Niklas Söderlund
niklas.soderlund at ragnatech.se
Sat Jun 27 12:40:16 CEST 2020
Hi Jacopo,
Thanks for your feedback.
On 2020-06-27 12:02:48 +0200, Jacopo Mondi wrote:
> On Sat, Jun 27, 2020 at 05:00:38AM +0200, Niklas Söderlund wrote:
> > When the IPU3 pipeline only provided streams to applications that came
> > from the ImgU it made sens to have a generic function to configure all
>
> sense
>
> > the different outputs. With the addition of the RAW stream this begins
> > to be cumbersome to read and make sens of in the PipelineHandlerIPU3
>
> sense
>
> > code. Replace the generic function that takes a specific argument for
> > which sink to configure with a specific function for each sink.
> >
> > This makes the code easier to follow as it's always clear which of the
> > ImgU sinks are being configured without knowing the content of a
> > generically named variable. It also paves way for future improvements.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > src/libcamera/pipeline/ipu3/imgu.cpp | 34 ++++++++++++++++---------
> > src/libcamera/pipeline/ipu3/imgu.h | 11 +++++++-
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 38 +++++++++++++++-------------
> > 3 files changed, 53 insertions(+), 30 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> > index 5d11539605f26303..d54e844d9bfc5147 100644
> > --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> > @@ -139,19 +139,28 @@ int ImgUDevice::configureInput(const Size &size,
> > return 0;
> > }
> >
> > -/**
> > - * \brief Configure the ImgU unit \a id video output
> > - * \param[in] output The ImgU output device to configure
> > - * \param[in] cfg The requested configuration
> > - * \return 0 on success or a negative error code otherwise
> > - */
> > -int ImgUDevice::configureOutput(ImgUOutput *output,
> > - const StreamConfiguration &cfg,
> > +int ImgUDevice::configureOutput(const StreamConfiguration &cfg,
> > V4L2DeviceFormat *outputFormat)
> > {
> > - V4L2VideoDevice *dev = output->dev;
> > - unsigned int pad = output->pad;
> > + return configureVideoDevice(output_.dev, PAD_OUTPUT, cfg, outputFormat);
> > +}
> >
> > +int ImgUDevice::configureViewfinder(const StreamConfiguration &cfg,
> > + V4L2DeviceFormat *outputFormat)
>
> No documentation but no complains in the build. Is doxygen ignoreing
> this file ?
We don't run Doxygen for pipeline handlers.
>
> > +{
> > + return configureVideoDevice(viewfinder_.dev, PAD_VF, cfg, outputFormat);
> > +}
> > +
> > +int ImgUDevice::configureStat(const StreamConfiguration &cfg,
> > + V4L2DeviceFormat *outputFormat)
> > +{
> > + return configureVideoDevice(stat_.dev, PAD_STAT, cfg, outputFormat);
> > +}
> > +
> > +int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,
> > + const StreamConfiguration &cfg,
> > + V4L2DeviceFormat *outputFormat)
> > +{
> > V4L2SubdeviceFormat imguFormat = {};
> > imguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;
> > imguFormat.size = cfg.size;
> > @@ -161,7 +170,7 @@ int ImgUDevice::configureOutput(ImgUOutput *output,
> > return ret;
> >
> > /* No need to apply format to the stat node. */
> > - if (output == &stat_)
> > + if (dev == stat_.dev)
> > return 0;
> >
> > *outputFormat = {};
> > @@ -173,7 +182,8 @@ int ImgUDevice::configureOutput(ImgUOutput *output,
> > if (ret)
> > return ret;
> >
> > - LOG(IPU3, Debug) << "ImgU " << output->name << " format = "
> > + const char *name = dev == output_.dev ? "output" : "viewfinder";
> > + LOG(IPU3, Debug) << "ImgU " << name << " format = "
> > << outputFormat->toString();
> >
> > return 0;
> > diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h
> > index b6b08b4fef2d3a9d..f8fd54089753b40e 100644
> > --- a/src/libcamera/pipeline/ipu3/imgu.h
> > +++ b/src/libcamera/pipeline/ipu3/imgu.h
> > @@ -49,9 +49,14 @@ public:
> > }
> >
> > int init(MediaDevice *media, unsigned int index);
> > +
> > int configureInput(const Size &size, V4L2DeviceFormat *inputFormat);
> > - int configureOutput(ImgUOutput *output, const StreamConfiguration &cfg,
> > + int configureOutput(const StreamConfiguration &cfg,
> > V4L2DeviceFormat *outputFormat);
> > + int configureViewfinder(const StreamConfiguration &cfg,
> > + V4L2DeviceFormat *outputFormat);
> > + int configureStat(const StreamConfiguration &cfg,
> > + V4L2DeviceFormat *outputFormat);
> >
> > int allocateBuffers(unsigned int bufferCount);
> > void freeBuffers();
> > @@ -78,6 +83,10 @@ private:
> > const std::string &sink, unsigned int sinkPad,
> > bool enable);
> >
> > + int configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,
> > + const StreamConfiguration &cfg,
> > + V4L2DeviceFormat *outputFormat);
> > +
> > std::string name_;
> > MediaDevice *media_;
> > };
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index b41a789e8dc2a7b2..ae7a01b81dacf498 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -504,19 +504,25 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> > stream->active_ = true;
> > cfg.setStream(stream);
> >
> > - /*
> > - * The RAW still capture stream just copies buffers from the
> > - * internal queue and doesn't need any specific configuration.
> > - */
> > - if (stream->raw_) {
> > + if (stream == outStream) {
> > + ret = imgu->configureOutput(cfg, &outputFormat);
> > + if (ret)
> > + return ret;
> > +
> > + cfg.stride = outputFormat.planes[0].bpl;
> > + } else if (stream == vfStream) {
> > + ret = imgu->configureViewfinder(cfg, &outputFormat);
> > + if (ret)
> > + return ret;
> > +
> > + cfg.stride = outputFormat.planes[0].bpl;
> > + } else {
> > + /*
> > + * The RAW still capture stream just copies buffers from
> > + * the internal queue and doesn't need any specific
> > + * configuration.
> > + */
> > cfg.stride = cio2Format.planes[0].bpl;
> > - } else {
> > - ret = imgu->configureOutput(stream->device_, cfg,
> > - &outputFormat);
> > - if (ret)
> > - return ret;
> > -
> > - cfg.stride = outputFormat.planes[0].bpl;
> > }
> > }
> >
> > @@ -526,15 +532,13 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> > * be at least one active stream in the configuration request).
> > */
> > if (!outStream->active_) {
> > - ret = imgu->configureOutput(outStream->device_, config->at(0),
> > - &outputFormat);
> > + ret = imgu->configureOutput(config->at(0), &outputFormat);
> > if (ret)
> > return ret;
> > }
> >
> > if (!vfStream->active_) {
> > - ret = imgu->configureOutput(vfStream->device_, config->at(0),
> > - &outputFormat);
> > + ret = imgu->configureViewfinder(config->at(0), &outputFormat);
> > if (ret)
> > return ret;
> > }
> > @@ -546,7 +550,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> > StreamConfiguration statCfg = {};
> > statCfg.size = cio2Format.size;
> >
> > - ret = imgu->configureOutput(&imgu->stat_, statCfg, &outputFormat);
> > + ret = imgu->configureStat(statCfg, &outputFormat);
> > if (ret)
> > return ret;
> >
> > --
> > 2.27.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list