[libcamera-devel] [PATCH 08/13] libcamera: ipu3: imgu: Use specific functions to configure each sink
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Jun 27 18:10:53 CEST 2020
Hi Niklas,
Thank you for the patch.
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
> the different outputs. With the addition of the RAW stream this begins
> to be cumbersome to read and make sens 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.
>
> 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
> - */
Should we retain the documentation ? It may not be directly clear from
the function prototype what outputFormat is used for.
> -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)
> +{
> + return configureVideoDevice(viewfinder_.dev, PAD_VF, cfg, outputFormat);
> +}
> +
> +int ImgUDevice::configureStat(const StreamConfiguration &cfg,
> + V4L2DeviceFormat *outputFormat)
> +{
> + return configureVideoDevice(stat_.dev, PAD_STAT, cfg, outputFormat);
> +}
Should these functions be inlined in the .h file for efficiency ?
> +
> +int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,
> + const StreamConfiguration &cfg,
> + V4L2DeviceFormat *outputFormat)
> +{
When I read the commit message I got worried you would duplicate quite a
bit of code, I'm relieved now :-)
> 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.
s/internal queue/internal CIO2 queue/ ?
s/configuration/configuration of the ImgU/ ?
And we don't copy buffers anymore, do we ?
> + */
> cfg.stride = cio2Format.planes[0].bpl;
> - } else {
> - ret = imgu->configureOutput(stream->device_, cfg,
> - &outputFormat);
> - if (ret)
> - return ret;
> -
> - cfg.stride = outputFormat.planes[0].bpl;
> }
I'm not sure I find the result more readable, but I'm also not opposed
to it.
> }
>
> @@ -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