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

[Laurent Pinchart]

Hi Jacopo,

On Mon, Jan 18, 2021 at 11:55:33AM +0100, Jacopo Mondi wrote:
> On Mon, Jan 11, 2021 at 01:53:38AM +0200, Laurent Pinchart wrote:
> > 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 ?

Yes that should fix the issue.

> > > +
> > >  	/*
> > >  	 * 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