[libcamera-devel] [PATCH v2 08/13] libcamera: ipu3: imgu: Use specific functions to configure each sink
Niklas Söderlund
niklas.soderlund at ragnatech.se
Sun Jun 28 13:56:45 CEST 2020
Hi Laurent,
Thanks for your feedback.
On 2020-06-28 09:51:30 +0300, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> On Sun, Jun 28, 2020 at 02:15:27AM +0200, Niklas Söderlund wrote:
> > When the IPU3 pipeline only provided streams to applications that came
> > from the ImgU it made sense to have a generic function to configure all
> > the different outputs. With the addition of the RAW stream this begins
> > to be cumbersome to read and make sense of in the PipelineHandlerIPU3
> > 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.
>
> s/paves way/paves the way/
>
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > * Changes since v1
> > - s/sens/sense/
> > - Retain comment for configureVideoDevice()
> > - Inline the accessors for configureVideoDevice() in the .h file
> > - Update comment in PipelineHandlerIPU3::configure()
> > ---
> > src/libcamera/pipeline/ipu3/imgu.cpp | 20 +++++++--------
> > src/libcamera/pipeline/ipu3/imgu.h | 28 ++++++++++++++++++--
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 38 +++++++++++++++-------------
> > 3 files changed, 57 insertions(+), 29 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> > index 46d56a5d18dc3687..981239cba975f92e 100644
> > --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> > @@ -139,18 +139,17 @@ int ImgUDevice::configureInput(const Size &size,
> > }
> >
> > /**
> > - * \brief Configure the ImgU unit \a id video output
> > - * \param[in] output The ImgU output device to configure
> > + * \brief Configure a video device on the ImgU
> > + * \param[in] dev The video device to configure
> > + * \param[in] pad The pad of the ImgU subdevice
> > * \param[in] cfg The requested configuration
> > + * \param[out] outputFormat The format set on the video device
> > * \return 0 on success or a negative error code otherwise
> > */
> > -int ImgUDevice::configureOutput(ImgUOutput *output,
> > - const StreamConfiguration &cfg,
> > - V4L2DeviceFormat *outputFormat)
> > +int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,
> > + const StreamConfiguration &cfg,
> > + V4L2DeviceFormat *outputFormat)
> > {
> > - V4L2VideoDevice *dev = output->dev;
> > - unsigned int pad = output->pad;
> > -
> > V4L2SubdeviceFormat imguFormat = {};
> > imguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;
> > imguFormat.size = cfg.size;
> > @@ -160,7 +159,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 = {};
> > @@ -172,7 +171,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..5f1dbfd8f45f7924 100644
> > --- a/src/libcamera/pipeline/ipu3/imgu.h
> > +++ b/src/libcamera/pipeline/ipu3/imgu.h
> > @@ -49,9 +49,29 @@ public:
> > }
> >
> > int init(MediaDevice *media, unsigned int index);
> > +
> > int configureInput(const Size &size, V4L2DeviceFormat *inputFormat);
> > - int configureOutput(ImgUOutput *output, const StreamConfiguration &cfg,
> > - V4L2DeviceFormat *outputFormat);
> > +
> > + int configureOutput(const StreamConfiguration &cfg,
> > + V4L2DeviceFormat *outputFormat)
> > + {
> > + return configureVideoDevice(output_.dev, PAD_OUTPUT, cfg,
> > + outputFormat);
> > + }
> > +
> > + int configureViewfinder(const StreamConfiguration &cfg,
> > + V4L2DeviceFormat *outputFormat)
> > + {
> > + return configureVideoDevice(viewfinder_.dev, PAD_VF, cfg,
> > + outputFormat);
> > + }
> > +
> > + int configureStat(const StreamConfiguration &cfg,
> > + V4L2DeviceFormat *outputFormat)
> > + {
> > + return configureVideoDevice(stat_.dev, PAD_STAT, cfg,
> > + outputFormat);
> > + }
> >
> > int allocateBuffers(unsigned int bufferCount);
> > void freeBuffers();
> > @@ -78,6 +98,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..e817f842f1216a7f 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 only uses en externel
>
> s/ en / an /
>
> and an external what ?
>
> > + * for the already configured CIO2 sink and doesn't need
>
> Isn't the CIO2 a source ?
>
> I know what you mean here as I know the code, but for someone who's not
> familiar with the IPU3 pipeline handler that would be a bit confusing.
How about?
/*
* The RAW stream is configured as part of the CIO2 and
* no configuration is needed for the ImgU.
*/
>
> > + * any specific configuration of the ImgU.
> > + */
> > 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;
> >
>
> --
> Regards,
>
> Laurent Pinchart
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list