| Message ID | 20220925122324.260251-1-umang.jain@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | ed591e705c451d0ce14988ae96829a31a2ae2f9a |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang 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: 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... Thanks j > > Signed-off-by: Umang Jain <umang.jain@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); > > 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); > -- > 2.37.3 >
Hi Jacopo, On 9/28/22 11:30 AM, Jacopo Mondi wrote: > Hi Umang > > 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) .... > > Thanks > j > >> Signed-off-by: Umang Jain <umang.jain@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); >> >> 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); >> -- >> 2.37.3 >>
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@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@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);
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@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@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@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
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); 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);
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. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- src/libcamera/v4l2_device.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)