[libcamera-devel] [PATCH v2 6/6] [HACK] ipu3: Set and get a few sensor controls

[Jacopo Mondi]

Hi Kieran,

On Tue, Jun 11, 2019 at 03:09:51PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 10/06/2019 17:40, Jacopo Mondi wrote:
> > Not to merge patch to demonstrate how to set controls on the image
> > sensor device.
> >
> > Not-Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 80 ++++++++++++++++++++++++++++
> >  1 file changed, 80 insertions(+)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index b3e7fb0e9c64..59c1fe3c56fd 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -688,6 +688,86 @@ int PipelineHandlerIPU3::start(Camera *camera)
> >  	ImgUDevice *imgu = data->imgu_;
> >  	int ret;
> >
> > +	/* --- Get control values --- */
> > +	std::vector<unsigned int> controlIds = {
> > +		V4L2_CID_EXPOSURE, V4L2_CID_ANALOGUE_GAIN,
> > +	};
> > +	V4L2Controls controls;
> > +	ret = cio2->sensor_->dev()->getControls(controlIds, &controls);
> > +	if (ret) {
> > +		LOG(Error) << "Failed to get control values";
> > +		return -EINVAL;
> > +	}
> > +
>
> Moving some logic for printing the values (or working out how to use
> ControlValue class in a V4L2Control) would simplify any debug prints
> greatly:
>
> for (V4L2Control *ctrl : controls) {
> 	LOG(Error) << "Control : " << id
> 		   << " - value: " << ctrl->value();
> }
>

Sorry, I might have missed how that would change using ControlValue.
That class has getInt, getBool etc, right?

>
> > +	for (V4L2Control *ctrl : controls) {
> > +		unsigned int id = ctrl->id();
> > +
> > +		switch(ctrl->type()) {
> > +		case V4L2_CTRL_TYPE_INTEGER:
> > +		case V4L2_CTRL_TYPE_BOOLEAN:
> > +		{
> > +			uint32_t val = controls.getInt(id);
> > +			LOG(Error) << "Control : " << id
> > +				   << " - value: " << val;
> > +		}
> > +			break;
> > +		case V4L2_CTRL_TYPE_INTEGER64:
> > +		{
> > +			uint64_t val = controls.getInt64(id);
> > +			LOG(Error) << "Control : " << id
> > +				   << " - value: " << val;
> > +		}
> > +			break;
> > +		default:
> > +			LOG(Error) << "Unsupported type: " << ctrl->type();
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
>
>
>
> > +	/* --- Set control values --- */
> > +	V4L2Controls setControls;
> > +	setControls.set(V4L2_CID_EXPOSURE, 2046);
> > +	setControls.set(V4L2_CID_ANALOGUE_GAIN, 1024);
> > +
> > +	ret = cio2->sensor_->dev()->setControls(setControls);
> > +	if (ret) {
> > +		LOG(IPU3, Error) << "Failed to set controls";
> > +		return ret;
> > +	}
> > +
> > +	/* --- Get control values back again and verify they have changed --- */
> > +	V4L2Controls newcontrols;
> > +	ret = cio2->sensor_->dev()->getControls(controlIds, &newcontrols);
> > +	if (ret) {
> > +		LOG(Error) << "Failed to get control values";
> > +		return -EINVAL;
> > +	}
> > +
> > +	for (V4L2Control *ctrl : newcontrols) {
> > +		unsigned int id = ctrl->id();
> > +
> > +		switch(ctrl->type()) {
> > +		case V4L2_CTRL_TYPE_INTEGER:
> > +		case V4L2_CTRL_TYPE_BOOLEAN:
> > +		{
> > +			uint32_t val = newcontrols.getInt(id);
>
> Eeep, Here we are iterating new controls to print them (I get that this
> is just a print) - but we are then 'getting' the int from the list
> container instead of the V4L2Control, which is involving 'finding' the
> control again, and iterating the 'newcontrols' container for each print?
>
> Iterating the list twice seems painful to me... but perhaps that's just
> because of this particular demo use case, and it might not be such an
> issue in real usage.

Accessing the V4L2Control value would require the user to cast the
V4L2Control * to the appropriate specialized derived class, something
we don't want.

In this case yes, using a ControlValue object which has a field for
all possible value types and provides a getInt() getBool() etc would
remove the casting requirement and would allow to access a control
value with a single call (provided the user knows the control value
type, but we have the same issue here, where user should call the
opportune get$TYPE() anyhow.


>
>
> > +			LOG(Error) << "Control : " << id
> > +				   << " - value: " << val;
> > +		}
> > +			break;
> > +		case V4L2_CTRL_TYPE_INTEGER64:
> > +		{
> > +			uint64_t val = newcontrols.getInt64(id);
> > +			LOG(Error) << "Control : " << id
> > +				   << " - value: " << val;
> > +		}
> > +			break;
> > +		default:
> > +			LOG(Error) << "Unsupported type: " << ctrl->type();
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> >  	/*
> >  	 * Start the ImgU video devices, buffers will be queued to the
> >  	 * ImgU output and viewfinder when requests will be queued.
> >
>
> --
> Regards
> --
> Kieran
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190611/7a1ae2c5/attachment.sig>