[libcamera-devel] [PATCH 09/12] libcamera: ipu3: Handle ScalerCrop

[Jacopo Mondi]

Hi Laurent,

On Mon, Jan 11, 2021 at 01:53:38AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Jan 05, 2021 at 08:05:19PM +0100, Jacopo Mondi wrote:
> > Add support for caching the value of control::ScalerCrop to return
> > it in the request metadata.
> >
> > No cropping is currently applied on the input video device, the control
> > value is returned in the metadata pack as it is received.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 22 ++++++++++++++++++++--
> >  1 file changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index f1329ffb0463..381524bb3499 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -66,6 +66,7 @@ public:
> >  	Stream rawStream_;
> >
> >  	int32_t exposureTime_;
> > +	Rectangle scalerCrop_;
> >  };
> >
> >  class IPU3CameraConfiguration : public CameraConfiguration
> > @@ -75,6 +76,7 @@ public:
> >
> >  	Status validate() override;
> >
> > +	const Size &cio2Size() const { return cio2Configuration_.size; }
>
> Do we need this, can't we keep using config->cio2Format().size ?
>
> >  	const StreamConfiguration &cio2Format() const { return cio2Configuration_; }
> >  	const ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; }
> >
> > @@ -468,12 +470,19 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> >  	 * Pass the requested stream size to the CIO2 unit and get back the
> >  	 * adjusted format to be propagated to the ImgU output devices.
> >  	 */
> > -	const Size &sensorSize = config->cio2Format().size;
> >  	V4L2DeviceFormat cio2Format;
> > -	ret = cio2->configure(sensorSize, &cio2Format);
> > +	ret = cio2->configure(config->cio2Size(), &cio2Format);
> >  	if (ret)
> >  		return ret;
> >
> > +	/* Initialize the scaler crop using the sensor's analogue crop. */
> > +	CameraSensorInfo sensorInfo;
> > +	ret = cio2->sensor()->sensorInfo(&sensorInfo);
> > +	if (ret)
> > +		/* Use the requested CIO2 output size as fallback. */
> > +		sensorInfo.analogCrop = Rectangle(config->cio2Size());
> > +	data->scalerCrop_ = sensorInfo.analogCrop;
> > +
> >  	/*
> >  	 * If the ImgU gets configured, its driver seems to expect that
> >  	 * buffers will be queued to its outputs, as otherwise the next
> > @@ -656,6 +665,14 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
> >  	IPU3CameraData *data = cameraData(camera);
> >  	int error = 0;
> >
> > +	ControlList &controls = request->controls();
> > +	if (controls.contains(controls::ScalerCrop))
> > +		/*
> > +		 * \todo No scaling is applied. Just return the value in the
> > +		 * request metadata as it is.
> > +		 */
> > +		data->scalerCrop_ = controls.get(controls::ScalerCrop);
>
> This means that request completing after the this request is queued will
> all have the new scaler crop rectangle. That's not right, it should be
> synchronized with this request.
>

Correct -.-

should I rather take the scaler crop value to be set in metadata from
the Request's controls ?

> > +
> >  	/*
> >  	 * Queue a buffer on the CIO2, using the raw stream buffer provided in
> >  	 * the request, if any, or a CIO2 internal buffer otherwise.
> > @@ -991,6 +1008,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
> >  	/* Mark the request as complete. */
> >  	request->metadata().set(controls::draft::PipelineDepth, 3);
> >  	request->metadata().set(controls::ExposureTime, exposureTime_);
> > +	request->metadata().set(controls::ScalerCrop, scalerCrop_);
> >  	pipe_->completeRequest(request);
> >  }
> >
>
> --
> Regards,
>
> Laurent Pinchart