| Message ID | 20200709084128.5316-20-jacopo@jmondi.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your patch. On 2020-07-09 10:41:27 +0200, Jacopo Mondi wrote: > Instrument the ImgUDevice::configureInput() function to use the provided > pipe configuration parameters to configure the IF, BDS and GDC > rectangles. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> s/lbcamera/libcamera/ in Subject. Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/ipu3/imgu. | 0 > src/libcamera/pipeline/ipu3/imgu.cpp | 35 ++++++++++++++++------------ > src/libcamera/pipeline/ipu3/imgu.h | 2 +- > src/libcamera/pipeline/ipu3/ipu3.cpp | 3 ++- > 4 files changed, 23 insertions(+), 17 deletions(-) > create mode 100644 src/libcamera/pipeline/ipu3/imgu. > > diff --git a/src/libcamera/pipeline/ipu3/imgu. b/src/libcamera/pipeline/ipu3/imgu. > new file mode 100644 > index 000000000000..e69de29bb2d1 > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp > index d1addb7d84d1..69bcc4f30962 100644 > --- a/src/libcamera/pipeline/ipu3/imgu.cpp > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp > @@ -441,11 +441,11 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe) > > /** > * \brief Configure the ImgU unit input > - * \param[in] size The ImgU input frame size > + * \param[in] config The ImgU pipe configuration parameters > * \param[in] inputFormat The format to be applied to ImgU input > * \return 0 on success or a negative error code otherwise > */ > -int ImgUDevice::configureInput(const Size &size, > +int ImgUDevice::configureInput(const PipeConfig &pipeConfig, > V4L2DeviceFormat *inputFormat) > { > /* Configure the ImgU input video device with the requested sizes. */ > @@ -465,32 +465,37 @@ int ImgUDevice::configureInput(const Size &size, > * to configure the crop/compose rectangles, contradicting the > * V4L2 specification. > */ > - Rectangle rect = { > + Rectangle iif = { > .x = 0, > .y = 0, > - .width = inputFormat->size.width, > - .height = inputFormat->size.height, > + .width = pipeConfig.iif.width, > + .height = pipeConfig.iif.height, > }; > - ret = imgu_->setSelection(PAD_INPUT, V4L2_SEL_TGT_CROP, &rect); > + ret = imgu_->setSelection(PAD_INPUT, V4L2_SEL_TGT_CROP, &iif); > if (ret) > return ret; > + LOG(IPU3, Debug) << "ImgU input feeder rectangle = " << iif.toString(); > > - ret = imgu_->setSelection(PAD_INPUT, V4L2_SEL_TGT_COMPOSE, &rect); > + Rectangle bds = { > + .x = 0, > + .y = 0, > + .width = pipeConfig.bds.width, > + .height = pipeConfig.bds.height, > + }; > + ret = imgu_->setSelection(PAD_INPUT, V4L2_SEL_TGT_COMPOSE, &bds); > if (ret) > return ret; > + LOG(IPU3, Debug) << "ImgU BDS rectangle = " << bds.toString(); > > - LOG(IPU3, Debug) << "ImgU input feeder and BDS rectangle = " > - << rect.toString(); > - > - V4L2SubdeviceFormat imguFormat = {}; > - imguFormat.mbus_code = MEDIA_BUS_FMT_FIXED; > - imguFormat.size = size; > + V4L2SubdeviceFormat gdcFormat = {}; > + gdcFormat.mbus_code = MEDIA_BUS_FMT_FIXED; > + gdcFormat.size = pipeConfig.gdc; > > - ret = imgu_->setFormat(PAD_INPUT, &imguFormat); > + ret = imgu_->setFormat(PAD_INPUT, &gdcFormat); > if (ret) > return ret; > > - LOG(IPU3, Debug) << "ImgU GDC format = " << imguFormat.toString(); > + LOG(IPU3, Debug) << "ImgU GDC format = " << gdcFormat.toString(); > > return 0; > } > diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h > index 15ee9a7f5698..6193c84bf35d 100644 > --- a/src/libcamera/pipeline/ipu3/imgu.h > +++ b/src/libcamera/pipeline/ipu3/imgu.h > @@ -45,7 +45,7 @@ public: > > PipeConfig calculatePipeConfig(Pipe *pipe); > > - int configureInput(const Size &size, V4L2DeviceFormat *inputFormat); > + int configureInput(const PipeConfig &pipeConfig, V4L2DeviceFormat *inputFormat); > > int configureOutput(const StreamConfiguration &cfg, > V4L2DeviceFormat *outputFormat) > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 72261d16e9f8..d33aa7aee5e5 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -66,6 +66,7 @@ public: > Status validate() override; > > const StreamConfiguration &cio2Format() const { return cio2Configuration_; }; > + const ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; } > > private: > IPU3CameraData *data_; > @@ -444,7 +445,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > if (ret) > return ret; > > - ret = imgu->configureInput(sensorSize, &cio2Format); > + ret = imgu->configureInput(config->imguConfig(), &cio2Format); > 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, Thank you for the patch. On Thu, Jul 09, 2020 at 10:41:27AM +0200, Jacopo Mondi wrote: > Instrument the ImgUDevice::configureInput() function to use the provided > pipe configuration parameters to configure the IF, BDS and GDC > rectangles. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/imgu. | 0 > src/libcamera/pipeline/ipu3/imgu.cpp | 35 ++++++++++++++++------------ > src/libcamera/pipeline/ipu3/imgu.h | 2 +- > src/libcamera/pipeline/ipu3/ipu3.cpp | 3 ++- > 4 files changed, 23 insertions(+), 17 deletions(-) > create mode 100644 src/libcamera/pipeline/ipu3/imgu. > > diff --git a/src/libcamera/pipeline/ipu3/imgu. b/src/libcamera/pipeline/ipu3/imgu. > new file mode 100644 > index 000000000000..e69de29bb2d1 > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp > index d1addb7d84d1..69bcc4f30962 100644 > --- a/src/libcamera/pipeline/ipu3/imgu.cpp > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp > @@ -441,11 +441,11 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe) > > /** > * \brief Configure the ImgU unit input > - * \param[in] size The ImgU input frame size > + * \param[in] config The ImgU pipe configuration parameters > * \param[in] inputFormat The format to be applied to ImgU input > * \return 0 on success or a negative error code otherwise > */ > -int ImgUDevice::configureInput(const Size &size, > +int ImgUDevice::configureInput(const PipeConfig &pipeConfig, > V4L2DeviceFormat *inputFormat) > { > /* Configure the ImgU input video device with the requested sizes. */ > @@ -465,32 +465,37 @@ int ImgUDevice::configureInput(const Size &size, > * to configure the crop/compose rectangles, contradicting the > * V4L2 specification. > */ > - Rectangle rect = { > + Rectangle iif = { The documentation mentions the IF module (Image Feeder), but the code often uses iif. What does iif stand for ? > .x = 0, > .y = 0, > - .width = inputFormat->size.width, > - .height = inputFormat->size.height, > + .width = pipeConfig.iif.width, > + .height = pipeConfig.iif.height, > }; > - ret = imgu_->setSelection(PAD_INPUT, V4L2_SEL_TGT_CROP, &rect); > + ret = imgu_->setSelection(PAD_INPUT, V4L2_SEL_TGT_CROP, &iif); > if (ret) > return ret; > + LOG(IPU3, Debug) << "ImgU input feeder rectangle = " << iif.toString(); > > - ret = imgu_->setSelection(PAD_INPUT, V4L2_SEL_TGT_COMPOSE, &rect); > + Rectangle bds = { > + .x = 0, > + .y = 0, > + .width = pipeConfig.bds.width, > + .height = pipeConfig.bds.height, > + }; > + ret = imgu_->setSelection(PAD_INPUT, V4L2_SEL_TGT_COMPOSE, &bds); > if (ret) > return ret; > + LOG(IPU3, Debug) << "ImgU BDS rectangle = " << bds.toString(); > > - LOG(IPU3, Debug) << "ImgU input feeder and BDS rectangle = " > - << rect.toString(); > - > - V4L2SubdeviceFormat imguFormat = {}; > - imguFormat.mbus_code = MEDIA_BUS_FMT_FIXED; > - imguFormat.size = size; > + V4L2SubdeviceFormat gdcFormat = {}; > + gdcFormat.mbus_code = MEDIA_BUS_FMT_FIXED; > + gdcFormat.size = pipeConfig.gdc; > > - ret = imgu_->setFormat(PAD_INPUT, &imguFormat); > + ret = imgu_->setFormat(PAD_INPUT, &gdcFormat); > if (ret) > return ret; > > - LOG(IPU3, Debug) << "ImgU GDC format = " << imguFormat.toString(); > + LOG(IPU3, Debug) << "ImgU GDC format = " << gdcFormat.toString(); This change appears small, but knowing how much work went into the calculation behind the scene, it's very nice to see the pieces fitting together. Great work ! Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > return 0; > } > diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h > index 15ee9a7f5698..6193c84bf35d 100644 > --- a/src/libcamera/pipeline/ipu3/imgu.h > +++ b/src/libcamera/pipeline/ipu3/imgu.h > @@ -45,7 +45,7 @@ public: > > PipeConfig calculatePipeConfig(Pipe *pipe); > > - int configureInput(const Size &size, V4L2DeviceFormat *inputFormat); > + int configureInput(const PipeConfig &pipeConfig, V4L2DeviceFormat *inputFormat); > > int configureOutput(const StreamConfiguration &cfg, > V4L2DeviceFormat *outputFormat) > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 72261d16e9f8..d33aa7aee5e5 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -66,6 +66,7 @@ public: > Status validate() override; > > const StreamConfiguration &cio2Format() const { return cio2Configuration_; }; > + const ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; } > > private: > IPU3CameraData *data_; > @@ -444,7 +445,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > if (ret) > return ret; > > - ret = imgu->configureInput(sensorSize, &cio2Format); > + ret = imgu->configureInput(config->imguConfig(), &cio2Format); > if (ret) > return ret; >
Hi Laurent, On Fri, Jul 10, 2020 at 03:55:24PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Thu, Jul 09, 2020 at 10:41:27AM +0200, Jacopo Mondi wrote: > > Instrument the ImgUDevice::configureInput() function to use the provided > > pipe configuration parameters to configure the IF, BDS and GDC > > rectangles. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/imgu. | 0 > > src/libcamera/pipeline/ipu3/imgu.cpp | 35 ++++++++++++++++------------ > > src/libcamera/pipeline/ipu3/imgu.h | 2 +- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 3 ++- > > 4 files changed, 23 insertions(+), 17 deletions(-) > > create mode 100644 src/libcamera/pipeline/ipu3/imgu. > > > > diff --git a/src/libcamera/pipeline/ipu3/imgu. b/src/libcamera/pipeline/ipu3/imgu. > > new file mode 100644 > > index 000000000000..e69de29bb2d1 > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp > > index d1addb7d84d1..69bcc4f30962 100644 > > --- a/src/libcamera/pipeline/ipu3/imgu.cpp > > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp > > @@ -441,11 +441,11 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe) > > > > /** > > * \brief Configure the ImgU unit input > > - * \param[in] size The ImgU input frame size > > + * \param[in] config The ImgU pipe configuration parameters > > * \param[in] inputFormat The format to be applied to ImgU input > > * \return 0 on success or a negative error code otherwise > > */ > > -int ImgUDevice::configureInput(const Size &size, > > +int ImgUDevice::configureInput(const PipeConfig &pipeConfig, > > V4L2DeviceFormat *inputFormat) > > { > > /* Configure the ImgU input video device with the requested sizes. */ > > @@ -465,32 +465,37 @@ int ImgUDevice::configureInput(const Size &size, > > * to configure the crop/compose rectangles, contradicting the > > * V4L2 specification. > > */ > > - Rectangle rect = { > > + Rectangle iif = { > > The documentation mentions the IF module (Image Feeder), but the code > often uses iif. What does iif stand for ? if is a keyword, iif seems the most similar term I could find, and it's nicely aligned to the other two components names (bds and gdc) being 3 letters in lenght. > > > .x = 0, > > .y = 0, > > - .width = inputFormat->size.width, > > - .height = inputFormat->size.height, > > + .width = pipeConfig.iif.width, > > + .height = pipeConfig.iif.height, > > }; > > - ret = imgu_->setSelection(PAD_INPUT, V4L2_SEL_TGT_CROP, &rect); > > + ret = imgu_->setSelection(PAD_INPUT, V4L2_SEL_TGT_CROP, &iif); > > if (ret) > > return ret; > > + LOG(IPU3, Debug) << "ImgU input feeder rectangle = " << iif.toString(); > > > > - ret = imgu_->setSelection(PAD_INPUT, V4L2_SEL_TGT_COMPOSE, &rect); > > + Rectangle bds = { > > + .x = 0, > > + .y = 0, > > + .width = pipeConfig.bds.width, > > + .height = pipeConfig.bds.height, > > + }; > > + ret = imgu_->setSelection(PAD_INPUT, V4L2_SEL_TGT_COMPOSE, &bds); > > if (ret) > > return ret; > > + LOG(IPU3, Debug) << "ImgU BDS rectangle = " << bds.toString(); > > > > - LOG(IPU3, Debug) << "ImgU input feeder and BDS rectangle = " > > - << rect.toString(); > > - > > - V4L2SubdeviceFormat imguFormat = {}; > > - imguFormat.mbus_code = MEDIA_BUS_FMT_FIXED; > > - imguFormat.size = size; > > + V4L2SubdeviceFormat gdcFormat = {}; > > + gdcFormat.mbus_code = MEDIA_BUS_FMT_FIXED; > > + gdcFormat.size = pipeConfig.gdc; > > > > - ret = imgu_->setFormat(PAD_INPUT, &imguFormat); > > + ret = imgu_->setFormat(PAD_INPUT, &gdcFormat); > > if (ret) > > return ret; > > > > - LOG(IPU3, Debug) << "ImgU GDC format = " << imguFormat.toString(); > > + LOG(IPU3, Debug) << "ImgU GDC format = " << gdcFormat.toString(); > > This change appears small, but knowing how much work went into the > calculation behind the scene, it's very nice to see the pieces fitting > together. Great work ! Slowly getting there! thanks! > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > return 0; > > } > > diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h > > index 15ee9a7f5698..6193c84bf35d 100644 > > --- a/src/libcamera/pipeline/ipu3/imgu.h > > +++ b/src/libcamera/pipeline/ipu3/imgu.h > > @@ -45,7 +45,7 @@ public: > > > > PipeConfig calculatePipeConfig(Pipe *pipe); > > > > - int configureInput(const Size &size, V4L2DeviceFormat *inputFormat); > > + int configureInput(const PipeConfig &pipeConfig, V4L2DeviceFormat *inputFormat); > > > > int configureOutput(const StreamConfiguration &cfg, > > V4L2DeviceFormat *outputFormat) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 72261d16e9f8..d33aa7aee5e5 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -66,6 +66,7 @@ public: > > Status validate() override; > > > > const StreamConfiguration &cio2Format() const { return cio2Configuration_; }; > > + const ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; } > > > > private: > > IPU3CameraData *data_; > > @@ -444,7 +445,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > > if (ret) > > return ret; > > > > - ret = imgu->configureInput(sensorSize, &cio2Format); > > + ret = imgu->configureInput(config->imguConfig(), &cio2Format); > > if (ret) > > return ret; > > > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/pipeline/ipu3/imgu. b/src/libcamera/pipeline/ipu3/imgu. new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp index d1addb7d84d1..69bcc4f30962 100644 --- a/src/libcamera/pipeline/ipu3/imgu.cpp +++ b/src/libcamera/pipeline/ipu3/imgu.cpp @@ -441,11 +441,11 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe) /** * \brief Configure the ImgU unit input - * \param[in] size The ImgU input frame size + * \param[in] config The ImgU pipe configuration parameters * \param[in] inputFormat The format to be applied to ImgU input * \return 0 on success or a negative error code otherwise */ -int ImgUDevice::configureInput(const Size &size, +int ImgUDevice::configureInput(const PipeConfig &pipeConfig, V4L2DeviceFormat *inputFormat) { /* Configure the ImgU input video device with the requested sizes. */ @@ -465,32 +465,37 @@ int ImgUDevice::configureInput(const Size &size, * to configure the crop/compose rectangles, contradicting the * V4L2 specification. */ - Rectangle rect = { + Rectangle iif = { .x = 0, .y = 0, - .width = inputFormat->size.width, - .height = inputFormat->size.height, + .width = pipeConfig.iif.width, + .height = pipeConfig.iif.height, }; - ret = imgu_->setSelection(PAD_INPUT, V4L2_SEL_TGT_CROP, &rect); + ret = imgu_->setSelection(PAD_INPUT, V4L2_SEL_TGT_CROP, &iif); if (ret) return ret; + LOG(IPU3, Debug) << "ImgU input feeder rectangle = " << iif.toString(); - ret = imgu_->setSelection(PAD_INPUT, V4L2_SEL_TGT_COMPOSE, &rect); + Rectangle bds = { + .x = 0, + .y = 0, + .width = pipeConfig.bds.width, + .height = pipeConfig.bds.height, + }; + ret = imgu_->setSelection(PAD_INPUT, V4L2_SEL_TGT_COMPOSE, &bds); if (ret) return ret; + LOG(IPU3, Debug) << "ImgU BDS rectangle = " << bds.toString(); - LOG(IPU3, Debug) << "ImgU input feeder and BDS rectangle = " - << rect.toString(); - - V4L2SubdeviceFormat imguFormat = {}; - imguFormat.mbus_code = MEDIA_BUS_FMT_FIXED; - imguFormat.size = size; + V4L2SubdeviceFormat gdcFormat = {}; + gdcFormat.mbus_code = MEDIA_BUS_FMT_FIXED; + gdcFormat.size = pipeConfig.gdc; - ret = imgu_->setFormat(PAD_INPUT, &imguFormat); + ret = imgu_->setFormat(PAD_INPUT, &gdcFormat); if (ret) return ret; - LOG(IPU3, Debug) << "ImgU GDC format = " << imguFormat.toString(); + LOG(IPU3, Debug) << "ImgU GDC format = " << gdcFormat.toString(); return 0; } diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h index 15ee9a7f5698..6193c84bf35d 100644 --- a/src/libcamera/pipeline/ipu3/imgu.h +++ b/src/libcamera/pipeline/ipu3/imgu.h @@ -45,7 +45,7 @@ public: PipeConfig calculatePipeConfig(Pipe *pipe); - int configureInput(const Size &size, V4L2DeviceFormat *inputFormat); + int configureInput(const PipeConfig &pipeConfig, V4L2DeviceFormat *inputFormat); int configureOutput(const StreamConfiguration &cfg, V4L2DeviceFormat *outputFormat) diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 72261d16e9f8..d33aa7aee5e5 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -66,6 +66,7 @@ public: Status validate() override; const StreamConfiguration &cio2Format() const { return cio2Configuration_; }; + const ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; } private: IPU3CameraData *data_; @@ -444,7 +445,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) if (ret) return ret; - ret = imgu->configureInput(sensorSize, &cio2Format); + ret = imgu->configureInput(config->imguConfig(), &cio2Format); if (ret) return ret;
Instrument the ImgUDevice::configureInput() function to use the provided pipe configuration parameters to configure the IF, BDS and GDC rectangles. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/imgu. | 0 src/libcamera/pipeline/ipu3/imgu.cpp | 35 ++++++++++++++++------------ src/libcamera/pipeline/ipu3/imgu.h | 2 +- src/libcamera/pipeline/ipu3/ipu3.cpp | 3 ++- 4 files changed, 23 insertions(+), 17 deletions(-) create mode 100644 src/libcamera/pipeline/ipu3/imgu.