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

[Jacopo Mondi]

Hi Umang

On Wed, Sep 28, 2022 at 04:04:32PM +0300, Laurent Pinchart wrote:
> 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
> >

Oh, missed the fact we already handle errorIdx >= numControls


> > > 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;

Afaict 'const' has no purpose.

Apart from that
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>


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