[libcamera-devel] [PATCH v2 08/13] libcamera: ipu3: imgu: Use specific functions to configure each sink
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Jun 28 13:58:46 CEST 2020
Hi Niklas,
On Sun, Jun 28, 2020 at 01:56:45PM +0200, Niklas Söderlund wrote:
> On 2020-06-28 09:51:30 +0300, Laurent Pinchart wrote:
> > 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.
> */
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > + * 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
More information about the libcamera-devel
mailing list