| Message ID | 20200627030043.2088585-9-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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 ? > +{ > + 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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
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@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; >
Hi Laurent, Thanks for your feedback. On 2020-06-27 19:10:53 +0300, Laurent Pinchart wrote: > 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@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. I find little value in this type of documentation in pipeline handlers. But it's already there so I will update it and retain it for v2. > > > -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 ? Good idea. > > > + > > +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 ? Good point, I will rewrite it for v2. > > > + */ > > 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
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) +{ + 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;
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@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(-)