| Message ID | 20190610164052.30741-7-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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(); } > + 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. > + 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. >
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@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
Hi Jacopo, On 11/06/2019 16:05, Jacopo Mondi wrote: > 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@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? The class ControlValue provides toString() and << overrides, so you can do a direct << on a ControlValue. It simplifies debug prints at least. Re-reading my statement, it looks like I hadn't completed my sentence fully. I was trynig to say, "If you move the logic for printing the values into the V4L2Control class or V4L2ControlValue or such ..." >> >>> + 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. Yes, that will depend on where the type is needed to be stored. But one thing it does do is allow value exchange without caring about the type. One reason why I'd really like to see this class be common (i.e. use ControlValue) is because then pipeline handlers do not need to care about the types in the most part. They could do direct copies or stores of the ControlValue object, or have that object (from the application) filled in directly. The application must already know about the Type for dealing with ControlValue class (which is application facing). Pipelines could potentially find themselves dealing with lists of controls to either get or set them, and they won't care about the data-type; they will just be the bridge between the Application and the V4L2 layer. The application however, will know that if it is looking at a Brightness control it is an integer, and if it's dealing with an Enable control it's boolean. Perhaps we might even enable signals and slots on controls so that the application gets updated directly when a control is updated, allowing it to update the GUI or such. >> >>> + 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
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; + } + + 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); + 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.