[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 08:51:30 CEST 2020
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.
> + * 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