[libcamera-devel] [PATCH] libcamera: v4l2_device: Log control id instead of errorIdx

[Laurent Pinchart]

Hello,

On Wed, Sep 28, 2022 at 11:59:02AM +0530, Umang Jain via libcamera-devel wrote:
> On 9/28/22 11:30 AM, Jacopo Mondi wrote:
> > On Sun, Sep 25, 2022 at 05:53:24PM +0530, Umang Jain via libcamera-devel wrote:
> >> v4l2_ext_controls.errorIdx (in the case of single failing control for
> >> VIDIOC_*_EXT_CTRLS calls) represents the index of that control.
> >> Since it is a single control, we can print the control id rather than
> >> its index. This improves logging as the id can be easily co-related
> >> with the controls while reading the log.
> > 
> > If we do that we lose the ability to report errors during the
> > valiation step:
> 
> Validation errors are reported separately, just above this patch's hunk(s).
> 
> See here: 
> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/v4l2_device.cpp#n239
> 
> > https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-g-ext-ctrls.html?highlight=ext_ctrl#c.V4L.VIDIOC_S_EXT_CTRLS
> > If the error is associated with a particular control, then error_idx
> > is set to the index of that control. If the error is not related to a
> > specific control, or the validation step failed (see below), then
> > error_idx is set to count. The value is undefined if the ioctl
> > returned 0 (success).
> >
> > There's more context in the documentation page about validation.
> >
> > Also, if a single control has failed, error_idx will report the index
> > of such control already, so I'm missing what we gain here...
> 
> index for e.g.  '3' is less readable than control id, say : (0x009819e2)
> 
> Hence we already print controls (in the debug mode, see below) with 
> their control-ids, one can easily figure out which (specific) control is 
> failing just by looking at the log. Hence, we "gain" read-ability here.
> ...
> [1:40:41.345462669] [16014] DEBUG V4L2 v4l2_device.cpp:625 
> /dev/video13[16:cap]: Control: Blue Balance (0x0098090f)
> [1:40:41.345590216] [16014] DEBUG V4L2 v4l2_device.cpp:625 
> /dev/video13[16:cap]: Control: Colour Correction Matrix (0x009819e1)
> [1:40:41.345707893] [16014] DEBUG V4L2 v4l2_device.cpp:625 
> /dev/video13[16:cap]: Control: Lens Shading (0x009819e2)
> [1:40:41.345818848] [16014] DEBUG V4L2 v4l2_device.cpp:625 
> /dev/video13[16:cap]: Control: Black Level (0x009819e3)
> ....
> 
> >> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> >> ---
> >>   src/libcamera/v4l2_device.cpp | 6 ++++--
> >>   1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> >> index 83901763..c60f7c91 100644
> >> --- a/src/libcamera/v4l2_device.cpp
> >> +++ b/src/libcamera/v4l2_device.cpp
> >> @@ -244,7 +244,8 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
> >>   		}
> >>
> >>   		/* A specific control failed. */
> >> -		LOG(V4L2, Error) << "Unable to read control " << errorIdx
> >> +		const unsigned int id = v4l2Ctrls[errorIdx].id;
> >> +		LOG(V4L2, Error) << "Unable to read control " << utils::hex(id)
> >>   				 << ": " << strerror(-ret);

This wouldn't almpw us to differentiate between entries for controls
with identical IDs, but that's such a corner case that I don't mind.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> >>
> >>   		v4l2Ctrls.resize(errorIdx);
> >> @@ -354,7 +355,8 @@ int V4L2Device::setControls(ControlList *ctrls)
> >>   		}
> >>
> >>   		/* A specific control failed. */
> >> -		LOG(V4L2, Error) << "Unable to set control " << errorIdx
> >> +		const unsigned int id = v4l2Ctrls[errorIdx].id;
> >> +		LOG(V4L2, Error) << "Unable to set control " << utils::hex(id)
> >>   				 << ": " << strerror(-ret);
> >>
> >>   		v4l2Ctrls.resize(errorIdx);

-- 
Regards,

Laurent Pinchart