[libcamera-devel] [PATCH v2 19/20] lbcamera: ipu3: Configure ImgU with the computed parameters

[Jacopo Mondi]

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 at 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 at 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