| Message ID | 20210205170317.267605-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Rejected |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Fri, Feb 05, 2021 at 05:03:17PM +0000, Kieran Bingham wrote: > We can not obtain the control names directly from V4L2 so create > a map of the control name, when declaring the list of mandatory > controls to enable a human readable print of any missing requirements. If we want to print V4L2 controls by name to ease debugging, it would be best not to limit the feature to this particular instance. I've been toying for a long time with the idea of creating Control instances for V4L2 controls, which would also allow the usage of a nicer get() and set() in the ControlList class. > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/camera_sensor.cpp | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index c9e8d49b7935..9a510108f171 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -346,18 +346,23 @@ int CameraSensor::validateSensorDriver() > * For raw sensors, make sure the sensor driver supports the controls > * required by the CameraSensor class. > */ > - static constexpr uint32_t mandatoryControls[] = { > - V4L2_CID_EXPOSURE, > - V4L2_CID_HBLANK, > - V4L2_CID_PIXEL_RATE, > - V4L2_CID_VBLANK, > + static struct { > + uint32_t ctrl; > + std::string name; > + } mandatoryControls[] = { > +#define MANDATORY_CONTROL(__ctrl) { __ctrl, #__ctrl } > + MANDATORY_CONTROL(V4L2_CID_EXPOSURE), > + MANDATORY_CONTROL(V4L2_CID_HBLANK), > + MANDATORY_CONTROL(V4L2_CID_PIXEL_RATE), > + MANDATORY_CONTROL(V4L2_CID_VBLANK), > +#undef MANDATORY_CONTROL > }; > > err = 0; > - for (uint32_t ctrl : mandatoryControls) { > - if (!controls.count(ctrl)) { > + for (auto &c : mandatoryControls) { > + if (!controls.count(c.ctrl)) { > LOG(CameraSensor, Error) > - << "Mandatory V4L2 control " << utils::hex(ctrl) > + << "Mandatory V4L2 control " << c.name > << " not available"; > err = -EINVAL; > }
Hi Laurent, On 06/02/2021 07:43, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Fri, Feb 05, 2021 at 05:03:17PM +0000, Kieran Bingham wrote: >> We can not obtain the control names directly from V4L2 so create >> a map of the control name, when declaring the list of mandatory >> controls to enable a human readable print of any missing requirements. > > If we want to print V4L2 controls by name to ease debugging, it would be > best not to limit the feature to this particular instance. I've been > toying for a long time with the idea of creating Control instances for > V4L2 controls, which would also allow the usage of a nicer get() and > set() in the ControlList class. But we already establish controls with names from V4L2 when they are available don't we? In src/libcamera/v4l2_controls.cpp, there is a helper std::string v4l2_ctrl_name(const struct v4l2_query_ext_ctrl &ctrl) which is used to establish a V4L2ControlId after it has been queried from the kernel. The key issue here is that at the point of reporting the Mandatory Controls are not available on the device - well - they don't exist and thus haven't been queried, so there is no name available. Do you envisage a static list of *all* v4l2 controls, over just the 4 which are 'required' here? Or that we add all v4l2 controls we use to the static declarations perhaps? (That could be handled just like the other controls then) Right now - with the recent addition of mandatory controls and many sensor drivers not supporting all the required controls, we have several instances of people hitting failures and being told an obscure hex value in the error reporting, which is not very helpful to an end user trying to debug the issue of why their capture which worked a couple of weeks ago now fails. I don't expect that we wish to provide a manual lookup service every time someone reports or queries what those values are - so I'd hope we can have a solution to this quite quickly. -- Kieran >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> src/libcamera/camera_sensor.cpp | 21 +++++++++++++-------- >> 1 file changed, 13 insertions(+), 8 deletions(-) >> >> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp >> index c9e8d49b7935..9a510108f171 100644 >> --- a/src/libcamera/camera_sensor.cpp >> +++ b/src/libcamera/camera_sensor.cpp >> @@ -346,18 +346,23 @@ int CameraSensor::validateSensorDriver() >> * For raw sensors, make sure the sensor driver supports the controls >> * required by the CameraSensor class. >> */ >> - static constexpr uint32_t mandatoryControls[] = { >> - V4L2_CID_EXPOSURE, >> - V4L2_CID_HBLANK, >> - V4L2_CID_PIXEL_RATE, >> - V4L2_CID_VBLANK, >> + static struct { >> + uint32_t ctrl; >> + std::string name; >> + } mandatoryControls[] = { >> +#define MANDATORY_CONTROL(__ctrl) { __ctrl, #__ctrl } >> + MANDATORY_CONTROL(V4L2_CID_EXPOSURE), >> + MANDATORY_CONTROL(V4L2_CID_HBLANK), >> + MANDATORY_CONTROL(V4L2_CID_PIXEL_RATE), >> + MANDATORY_CONTROL(V4L2_CID_VBLANK), >> +#undef MANDATORY_CONTROL >> }; >> >> err = 0; >> - for (uint32_t ctrl : mandatoryControls) { >> - if (!controls.count(ctrl)) { >> + for (auto &c : mandatoryControls) { >> + if (!controls.count(c.ctrl)) { >> LOG(CameraSensor, Error) >> - << "Mandatory V4L2 control " << utils::hex(ctrl) >> + << "Mandatory V4L2 control " << c.name >> << " not available"; >> err = -EINVAL; >> } >
On 06/02/2021 10:12, Kieran Bingham wrote: > Hi Laurent, > > On 06/02/2021 07:43, Laurent Pinchart wrote: >> Hi Kieran, >> >> Thank you for the patch. >> >> On Fri, Feb 05, 2021 at 05:03:17PM +0000, Kieran Bingham wrote: >>> We can not obtain the control names directly from V4L2 so create >>> a map of the control name, when declaring the list of mandatory >>> controls to enable a human readable print of any missing requirements. >> >> If we want to print V4L2 controls by name to ease debugging, it would be >> best not to limit the feature to this particular instance. I've been >> toying for a long time with the idea of creating Control instances for >> V4L2 controls, which would also allow the usage of a nicer get() and >> set() in the ControlList class. > > But we already establish controls with names from V4L2 when they are > available don't we? > > In src/libcamera/v4l2_controls.cpp, there is a helper > > std::string v4l2_ctrl_name(const struct v4l2_query_ext_ctrl &ctrl) > > which is used to establish a V4L2ControlId after it has been queried > from the kernel. > > The key issue here is that at the point of reporting the Mandatory > Controls are not available on the device - well - they don't exist and > thus haven't been queried, so there is no name available. > > > Do you envisage a static list of *all* v4l2 controls, over just the 4 > which are 'required' here? Or that we add all v4l2 controls we use to > the static declarations perhaps? (That could be handled just like the > other controls then) > > Right now - with the recent addition of mandatory controls and many > sensor drivers not supporting all the required controls, we have several > instances of people hitting failures and being told an obscure hex value > in the error reporting, which is not very helpful to an end user trying > to debug the issue of why their capture which worked a couple of weeks > ago now fails. > > I don't expect that we wish to provide a manual lookup service every > time someone reports or queries what those values are - so I'd hope we > can have a solution to this quite quickly. Indeed of course this is more than just the mandatory controls: > [0:51:10.226221013] [7894] DEBUG CameraSensor camera_sensor.cpp:291 'imx258 8-001a': Optional V4L2 control 0x009a0922 not supported > [0:51:10.226238875] [7894] DEBUG CameraSensor camera_sensor.cpp:291 'imx258 8-001a': Optional V4L2 control 0x009a0923 not supported It might be optional, but it would be very nice to know what it is ;-) -- Kieran >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> --- >>> src/libcamera/camera_sensor.cpp | 21 +++++++++++++-------- >>> 1 file changed, 13 insertions(+), 8 deletions(-) >>> >>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp >>> index c9e8d49b7935..9a510108f171 100644 >>> --- a/src/libcamera/camera_sensor.cpp >>> +++ b/src/libcamera/camera_sensor.cpp >>> @@ -346,18 +346,23 @@ int CameraSensor::validateSensorDriver() >>> * For raw sensors, make sure the sensor driver supports the controls >>> * required by the CameraSensor class. >>> */ >>> - static constexpr uint32_t mandatoryControls[] = { >>> - V4L2_CID_EXPOSURE, >>> - V4L2_CID_HBLANK, >>> - V4L2_CID_PIXEL_RATE, >>> - V4L2_CID_VBLANK, >>> + static struct { >>> + uint32_t ctrl; >>> + std::string name; >>> + } mandatoryControls[] = { >>> +#define MANDATORY_CONTROL(__ctrl) { __ctrl, #__ctrl } >>> + MANDATORY_CONTROL(V4L2_CID_EXPOSURE), >>> + MANDATORY_CONTROL(V4L2_CID_HBLANK), >>> + MANDATORY_CONTROL(V4L2_CID_PIXEL_RATE), >>> + MANDATORY_CONTROL(V4L2_CID_VBLANK), >>> +#undef MANDATORY_CONTROL >>> }; >>> >>> err = 0; >>> - for (uint32_t ctrl : mandatoryControls) { >>> - if (!controls.count(ctrl)) { >>> + for (auto &c : mandatoryControls) { >>> + if (!controls.count(c.ctrl)) { >>> LOG(CameraSensor, Error) >>> - << "Mandatory V4L2 control " << utils::hex(ctrl) >>> + << "Mandatory V4L2 control " << c.name >>> << " not available"; >>> err = -EINVAL; >>> } >> >
Hi Kieran, On Mon, Feb 08, 2021 at 12:20:57PM +0000, Kieran Bingham wrote: > On 06/02/2021 10:12, Kieran Bingham wrote: > > Hi Laurent, > > > > On 06/02/2021 07:43, Laurent Pinchart wrote: > >> Hi Kieran, > >> > >> Thank you for the patch. > >> > >> On Fri, Feb 05, 2021 at 05:03:17PM +0000, Kieran Bingham wrote: > >>> We can not obtain the control names directly from V4L2 so create > >>> a map of the control name, when declaring the list of mandatory > >>> controls to enable a human readable print of any missing requirements. > >> > >> If we want to print V4L2 controls by name to ease debugging, it would be > >> best not to limit the feature to this particular instance. I've been > >> toying for a long time with the idea of creating Control instances for > >> V4L2 controls, which would also allow the usage of a nicer get() and > >> set() in the ControlList class. > > > > But we already establish controls with names from V4L2 when they are > > available don't we? > > > > In src/libcamera/v4l2_controls.cpp, there is a helper > > > > std::string v4l2_ctrl_name(const struct v4l2_query_ext_ctrl &ctrl) > > > > which is used to establish a V4L2ControlId after it has been queried > > from the kernel. > > > > The key issue here is that at the point of reporting the Mandatory > > Controls are not available on the device - well - they don't exist and > > thus haven't been queried, so there is no name available. > > > > > > Do you envisage a static list of *all* v4l2 controls, over just the 4 > > which are 'required' here? Or that we add all v4l2 controls we use to > > the static declarations perhaps? (That could be handled just like the > > other controls then) > > > > Right now - with the recent addition of mandatory controls and many > > sensor drivers not supporting all the required controls, we have several > > instances of people hitting failures and being told an obscure hex value > > in the error reporting, which is not very helpful to an end user trying > > to debug the issue of why their capture which worked a couple of weeks > > ago now fails. > > > > I don't expect that we wish to provide a manual lookup service every > > time someone reports or queries what those values are - so I'd hope we > > can have a solution to this quite quickly. > > Indeed of course this is more than just the mandatory controls: > > > > [0:51:10.226221013] [7894] DEBUG CameraSensor camera_sensor.cpp:291 'imx258 8-001a': Optional V4L2 control 0x009a0922 not supported > > [0:51:10.226238875] [7894] DEBUG CameraSensor camera_sensor.cpp:291 'imx258 8-001a': Optional V4L2 control 0x009a0923 not supported > > It might be optional, but it would be very nice to know what it is ;-) Personal opinion: if one is interesting in knowing why the driver validation fails, I hope she would not only rely on the debug printouts but rather turn to the code and cross-check for the controls presence in the driver. An ad-hoc solution for this is nice, but it's a bit a waste of effort, considering how much nicer would our control framework be if we could get rid of the V4L2Control indexed by id and treat all controls we deal with (libcamera and v4l2) as ControlId * We import v4l2-controls.h so we have all we need to parse and create ControlId instances at build time from there. It's not a 2 hours task unfortunately as it require quite some some of parsing and substitution. > > -- > Kieran > > > >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>> --- > >>> src/libcamera/camera_sensor.cpp | 21 +++++++++++++-------- > >>> 1 file changed, 13 insertions(+), 8 deletions(-) > >>> > >>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > >>> index c9e8d49b7935..9a510108f171 100644 > >>> --- a/src/libcamera/camera_sensor.cpp > >>> +++ b/src/libcamera/camera_sensor.cpp > >>> @@ -346,18 +346,23 @@ int CameraSensor::validateSensorDriver() > >>> * For raw sensors, make sure the sensor driver supports the controls > >>> * required by the CameraSensor class. > >>> */ > >>> - static constexpr uint32_t mandatoryControls[] = { > >>> - V4L2_CID_EXPOSURE, > >>> - V4L2_CID_HBLANK, > >>> - V4L2_CID_PIXEL_RATE, > >>> - V4L2_CID_VBLANK, > >>> + static struct { > >>> + uint32_t ctrl; > >>> + std::string name; > >>> + } mandatoryControls[] = { > >>> +#define MANDATORY_CONTROL(__ctrl) { __ctrl, #__ctrl } > >>> + MANDATORY_CONTROL(V4L2_CID_EXPOSURE), > >>> + MANDATORY_CONTROL(V4L2_CID_HBLANK), > >>> + MANDATORY_CONTROL(V4L2_CID_PIXEL_RATE), > >>> + MANDATORY_CONTROL(V4L2_CID_VBLANK), > >>> +#undef MANDATORY_CONTROL > >>> }; > >>> > >>> err = 0; > >>> - for (uint32_t ctrl : mandatoryControls) { > >>> - if (!controls.count(ctrl)) { > >>> + for (auto &c : mandatoryControls) { > >>> + if (!controls.count(c.ctrl)) { > >>> LOG(CameraSensor, Error) > >>> - << "Mandatory V4L2 control " << utils::hex(ctrl) > >>> + << "Mandatory V4L2 control " << c.name > >>> << " not available"; > >>> err = -EINVAL; > >>> } > >> > > > > -- > Regards > -- > Kieran > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hello, On Mon, Feb 08, 2021 at 05:15:29PM +0100, Jacopo Mondi wrote: > On Mon, Feb 08, 2021 at 12:20:57PM +0000, Kieran Bingham wrote: > > On 06/02/2021 10:12, Kieran Bingham wrote: > > > On 06/02/2021 07:43, Laurent Pinchart wrote: > > >> On Fri, Feb 05, 2021 at 05:03:17PM +0000, Kieran Bingham wrote: > > >>> We can not obtain the control names directly from V4L2 so create > > >>> a map of the control name, when declaring the list of mandatory > > >>> controls to enable a human readable print of any missing requirements. > > >> > > >> If we want to print V4L2 controls by name to ease debugging, it would be > > >> best not to limit the feature to this particular instance. I've been > > >> toying for a long time with the idea of creating Control instances for > > >> V4L2 controls, which would also allow the usage of a nicer get() and > > >> set() in the ControlList class. > > > > > > But we already establish controls with names from V4L2 when they are > > > available don't we? > > > > > > In src/libcamera/v4l2_controls.cpp, there is a helper > > > > > > std::string v4l2_ctrl_name(const struct v4l2_query_ext_ctrl &ctrl) > > > > > > which is used to establish a V4L2ControlId after it has been queried > > > from the kernel. > > > > > > The key issue here is that at the point of reporting the Mandatory > > > Controls are not available on the device - well - they don't exist and > > > thus haven't been queried, so there is no name available. > > > > > > > > > Do you envisage a static list of *all* v4l2 controls, over just the 4 > > > which are 'required' here? Or that we add all v4l2 controls we use to > > > the static declarations perhaps? (That could be handled just like the > > > other controls then) > > > > > > Right now - with the recent addition of mandatory controls and many > > > sensor drivers not supporting all the required controls, we have several > > > instances of people hitting failures and being told an obscure hex value > > > in the error reporting, which is not very helpful to an end user trying > > > to debug the issue of why their capture which worked a couple of weeks > > > ago now fails. > > > > > > I don't expect that we wish to provide a manual lookup service every > > > time someone reports or queries what those values are - so I'd hope we > > > can have a solution to this quite quickly. > > > > Indeed of course this is more than just the mandatory controls: > > > > > > > [0:51:10.226221013] [7894] DEBUG CameraSensor camera_sensor.cpp:291 'imx258 8-001a': Optional V4L2 control 0x009a0922 not supported > > > [0:51:10.226238875] [7894] DEBUG CameraSensor camera_sensor.cpp:291 'imx258 8-001a': Optional V4L2 control 0x009a0923 not supported > > > > It might be optional, but it would be very nice to know what it is ;-) > > Personal opinion: if one is interesting in knowing why the driver > validation fails, I hope she would not only rely on the debug > printouts but rather turn to the code and cross-check for the controls > presence in the driver. An ad-hoc solution for this is nice, but it's > a bit a waste of effort, considering how much nicer would our control > framework be if we could get rid of the V4L2Control indexed by id and > treat all controls we deal with (libcamera and v4l2) as ControlId * I think I generally agree. > We import v4l2-controls.h so we have all we need to parse and create > ControlId instances at build time from there. It's not a 2 hours task > unfortunately as it require quite some some of parsing and > substitution. We need type information too, which we can't get from the v4l2-controls.h header. I was thus considering a YAML file listing the V4L2 controls we need. > > >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >>> --- > > >>> src/libcamera/camera_sensor.cpp | 21 +++++++++++++-------- > > >>> 1 file changed, 13 insertions(+), 8 deletions(-) > > >>> > > >>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > >>> index c9e8d49b7935..9a510108f171 100644 > > >>> --- a/src/libcamera/camera_sensor.cpp > > >>> +++ b/src/libcamera/camera_sensor.cpp > > >>> @@ -346,18 +346,23 @@ int CameraSensor::validateSensorDriver() > > >>> * For raw sensors, make sure the sensor driver supports the controls > > >>> * required by the CameraSensor class. > > >>> */ > > >>> - static constexpr uint32_t mandatoryControls[] = { > > >>> - V4L2_CID_EXPOSURE, > > >>> - V4L2_CID_HBLANK, > > >>> - V4L2_CID_PIXEL_RATE, > > >>> - V4L2_CID_VBLANK, > > >>> + static struct { > > >>> + uint32_t ctrl; > > >>> + std::string name; > > >>> + } mandatoryControls[] = { > > >>> +#define MANDATORY_CONTROL(__ctrl) { __ctrl, #__ctrl } > > >>> + MANDATORY_CONTROL(V4L2_CID_EXPOSURE), > > >>> + MANDATORY_CONTROL(V4L2_CID_HBLANK), > > >>> + MANDATORY_CONTROL(V4L2_CID_PIXEL_RATE), > > >>> + MANDATORY_CONTROL(V4L2_CID_VBLANK), > > >>> +#undef MANDATORY_CONTROL > > >>> }; > > >>> > > >>> err = 0; > > >>> - for (uint32_t ctrl : mandatoryControls) { > > >>> - if (!controls.count(ctrl)) { > > >>> + for (auto &c : mandatoryControls) { > > >>> + if (!controls.count(c.ctrl)) { > > >>> LOG(CameraSensor, Error) > > >>> - << "Mandatory V4L2 control " << utils::hex(ctrl) > > >>> + << "Mandatory V4L2 control " << c.name > > >>> << " not available"; > > >>> err = -EINVAL; > > >>> }
Hi, On 08/02/2021 16:21, Laurent Pinchart wrote: > Hello, > > On Mon, Feb 08, 2021 at 05:15:29PM +0100, Jacopo Mondi wrote: >> On Mon, Feb 08, 2021 at 12:20:57PM +0000, Kieran Bingham wrote: >>> On 06/02/2021 10:12, Kieran Bingham wrote: >>>> On 06/02/2021 07:43, Laurent Pinchart wrote: >>>>> On Fri, Feb 05, 2021 at 05:03:17PM +0000, Kieran Bingham wrote: >>>>>> We can not obtain the control names directly from V4L2 so create >>>>>> a map of the control name, when declaring the list of mandatory >>>>>> controls to enable a human readable print of any missing requirements. >>>>> >>>>> If we want to print V4L2 controls by name to ease debugging, it would be >>>>> best not to limit the feature to this particular instance. I've been >>>>> toying for a long time with the idea of creating Control instances for >>>>> V4L2 controls, which would also allow the usage of a nicer get() and >>>>> set() in the ControlList class. >>>> >>>> But we already establish controls with names from V4L2 when they are >>>> available don't we? >>>> >>>> In src/libcamera/v4l2_controls.cpp, there is a helper >>>> >>>> std::string v4l2_ctrl_name(const struct v4l2_query_ext_ctrl &ctrl) >>>> >>>> which is used to establish a V4L2ControlId after it has been queried >>>> from the kernel. >>>> >>>> The key issue here is that at the point of reporting the Mandatory >>>> Controls are not available on the device - well - they don't exist and >>>> thus haven't been queried, so there is no name available. >>>> >>>> >>>> Do you envisage a static list of *all* v4l2 controls, over just the 4 >>>> which are 'required' here? Or that we add all v4l2 controls we use to >>>> the static declarations perhaps? (That could be handled just like the >>>> other controls then) >>>> >>>> Right now - with the recent addition of mandatory controls and many >>>> sensor drivers not supporting all the required controls, we have several >>>> instances of people hitting failures and being told an obscure hex value >>>> in the error reporting, which is not very helpful to an end user trying >>>> to debug the issue of why their capture which worked a couple of weeks >>>> ago now fails. >>>> >>>> I don't expect that we wish to provide a manual lookup service every >>>> time someone reports or queries what those values are - so I'd hope we >>>> can have a solution to this quite quickly. >>> >>> Indeed of course this is more than just the mandatory controls: >>> >>> >>>> [0:51:10.226221013] [7894] DEBUG CameraSensor camera_sensor.cpp:291 'imx258 8-001a': Optional V4L2 control 0x009a0922 not supported >>>> [0:51:10.226238875] [7894] DEBUG CameraSensor camera_sensor.cpp:291 'imx258 8-001a': Optional V4L2 control 0x009a0923 not supported >>> >>> It might be optional, but it would be very nice to know what it is ;-) >> >> Personal opinion: if one is interesting in knowing why the driver >> validation fails, I hope she would not only rely on the debug >> printouts but rather turn to the code and cross-check for the controls The problem is the hex values are opaque from a human-readable perspective. The VBLANK control was only added at the end of last week, and there was already a misunderstanding that it was a missing HBLANK control preventing the running of mainline master, until it was clear that actually we were now failing on a missing VBLANK. Printing hex values just feels like we're saying "Oh we don't care about users, or others having to debug the situation". >> presence in the driver. An ad-hoc solution for this is nice, but it's >> a bit a waste of effort, considering how much nicer would our control >> framework be if we could get rid of the V4L2Control indexed by id and >> treat all controls we deal with (libcamera and v4l2) as ControlId * Waste of effort or interim solution? It was a 15 minute patch + discussion (hence RFC of course). > I think I generally agree. > >> We import v4l2-controls.h so we have all we need to parse and create >> ControlId instances at build time from there. It's not a 2 hours task >> unfortunately as it require quite some some of parsing and >> substitution. > > We need type information too, which we can't get from the > v4l2-controls.h header. I was thus considering a YAML file listing the > V4L2 controls we need. So how do we make this happen? I agree that a full control interface would be nicer, but that is going to take time that none of us have right now. (Though we did have volunteers looking for tasks recently didn't we?) This is why I would like to still consider an interim solution, because we have actively chosen to cause breakage - and that breakage results in people seeing hex codes as the 'summary' which isn't friendly or helpful to end users (or ... me....) -- Kieran >>>>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>>>> --- >>>>>> src/libcamera/camera_sensor.cpp | 21 +++++++++++++-------- >>>>>> 1 file changed, 13 insertions(+), 8 deletions(-) >>>>>> >>>>>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp >>>>>> index c9e8d49b7935..9a510108f171 100644 >>>>>> --- a/src/libcamera/camera_sensor.cpp >>>>>> +++ b/src/libcamera/camera_sensor.cpp >>>>>> @@ -346,18 +346,23 @@ int CameraSensor::validateSensorDriver() >>>>>> * For raw sensors, make sure the sensor driver supports the controls >>>>>> * required by the CameraSensor class. >>>>>> */ >>>>>> - static constexpr uint32_t mandatoryControls[] = { >>>>>> - V4L2_CID_EXPOSURE, >>>>>> - V4L2_CID_HBLANK, >>>>>> - V4L2_CID_PIXEL_RATE, >>>>>> - V4L2_CID_VBLANK, >>>>>> + static struct { >>>>>> + uint32_t ctrl; >>>>>> + std::string name; >>>>>> + } mandatoryControls[] = { >>>>>> +#define MANDATORY_CONTROL(__ctrl) { __ctrl, #__ctrl } >>>>>> + MANDATORY_CONTROL(V4L2_CID_EXPOSURE), >>>>>> + MANDATORY_CONTROL(V4L2_CID_HBLANK), >>>>>> + MANDATORY_CONTROL(V4L2_CID_PIXEL_RATE), >>>>>> + MANDATORY_CONTROL(V4L2_CID_VBLANK), >>>>>> +#undef MANDATORY_CONTROL >>>>>> }; >>>>>> >>>>>> err = 0; >>>>>> - for (uint32_t ctrl : mandatoryControls) { >>>>>> - if (!controls.count(ctrl)) { >>>>>> + for (auto &c : mandatoryControls) { >>>>>> + if (!controls.count(c.ctrl)) { >>>>>> LOG(CameraSensor, Error) >>>>>> - << "Mandatory V4L2 control " << utils::hex(ctrl) >>>>>> + << "Mandatory V4L2 control " << c.name >>>>>> << " not available"; >>>>>> err = -EINVAL; >>>>>> } >
Hi Kieran, On Mon, Feb 08, 2021 at 04:47:46PM +0000, Kieran Bingham wrote: > On 08/02/2021 16:21, Laurent Pinchart wrote: > > On Mon, Feb 08, 2021 at 05:15:29PM +0100, Jacopo Mondi wrote: > >> On Mon, Feb 08, 2021 at 12:20:57PM +0000, Kieran Bingham wrote: > >>> On 06/02/2021 10:12, Kieran Bingham wrote: > >>>> On 06/02/2021 07:43, Laurent Pinchart wrote: > >>>>> On Fri, Feb 05, 2021 at 05:03:17PM +0000, Kieran Bingham wrote: > >>>>>> We can not obtain the control names directly from V4L2 so create > >>>>>> a map of the control name, when declaring the list of mandatory > >>>>>> controls to enable a human readable print of any missing requirements. > >>>>> > >>>>> If we want to print V4L2 controls by name to ease debugging, it would be > >>>>> best not to limit the feature to this particular instance. I've been > >>>>> toying for a long time with the idea of creating Control instances for > >>>>> V4L2 controls, which would also allow the usage of a nicer get() and > >>>>> set() in the ControlList class. > >>>> > >>>> But we already establish controls with names from V4L2 when they are > >>>> available don't we? > >>>> > >>>> In src/libcamera/v4l2_controls.cpp, there is a helper > >>>> > >>>> std::string v4l2_ctrl_name(const struct v4l2_query_ext_ctrl &ctrl) > >>>> > >>>> which is used to establish a V4L2ControlId after it has been queried > >>>> from the kernel. > >>>> > >>>> The key issue here is that at the point of reporting the Mandatory > >>>> Controls are not available on the device - well - they don't exist and > >>>> thus haven't been queried, so there is no name available. > >>>> > >>>> > >>>> Do you envisage a static list of *all* v4l2 controls, over just the 4 > >>>> which are 'required' here? Or that we add all v4l2 controls we use to > >>>> the static declarations perhaps? (That could be handled just like the > >>>> other controls then) > >>>> > >>>> Right now - with the recent addition of mandatory controls and many > >>>> sensor drivers not supporting all the required controls, we have several > >>>> instances of people hitting failures and being told an obscure hex value > >>>> in the error reporting, which is not very helpful to an end user trying > >>>> to debug the issue of why their capture which worked a couple of weeks > >>>> ago now fails. > >>>> > >>>> I don't expect that we wish to provide a manual lookup service every > >>>> time someone reports or queries what those values are - so I'd hope we > >>>> can have a solution to this quite quickly. > >>> > >>> Indeed of course this is more than just the mandatory controls: > >>> > >>>> [0:51:10.226221013] [7894] DEBUG CameraSensor camera_sensor.cpp:291 'imx258 8-001a': Optional V4L2 control 0x009a0922 not supported > >>>> [0:51:10.226238875] [7894] DEBUG CameraSensor camera_sensor.cpp:291 'imx258 8-001a': Optional V4L2 control 0x009a0923 not supported > >>> > >>> It might be optional, but it would be very nice to know what it is ;-) > >> > >> Personal opinion: if one is interesting in knowing why the driver > >> validation fails, I hope she would not only rely on the debug > >> printouts but rather turn to the code and cross-check for the controls > > The problem is the hex values are opaque from a human-readable > perspective. The VBLANK control was only added at the end of last week, > and there was already a misunderstanding that it was a missing HBLANK > control preventing the running of mainline master, until it was clear > that actually we were now failing on a missing VBLANK. > > Printing hex values just feels like we're saying "Oh we don't care about > users, or others having to debug the situation". I think that Jacopo's point, and mine, was that this message is really for developers, not end users, and developers who would need to add controls to a camera sensor kernel driver are expected to be able to read v4l2-controls.h. That being said, a name is of course nicer to read, and not just here. My concern was thus whether printing a name here was important enough to call for a ad-hoc short term fix, or if we should instead take one extra step. In hindsight, I've probably underestimated the lack of user-friendliness of hex values in this case. > >> presence in the driver. An ad-hoc solution for this is nice, but it's > >> a bit a waste of effort, considering how much nicer would our control > >> framework be if we could get rid of the V4L2Control indexed by id and > >> treat all controls we deal with (libcamera and v4l2) as ControlId * > > Waste of effort or interim solution? It was a 15 minute patch + > discussion (hence RFC of course). Turns out the discussion is the most time consuming part :-) > > I think I generally agree. > > > >> We import v4l2-controls.h so we have all we need to parse and create > >> ControlId instances at build time from there. It's not a 2 hours task > >> unfortunately as it require quite some some of parsing and > >> substitution. > > > > We need type information too, which we can't get from the > > v4l2-controls.h header. I was thus considering a YAML file listing the > > V4L2 controls we need. > > So how do we make this happen? Please see the RFC series on the list, let's continue the discussion there. > I agree that a full control interface would be nicer, but that is going > to take time that none of us have right now. > > (Though we did have volunteers looking for tasks recently didn't we?) > > > This is why I would like to still consider an interim solution, because > we have actively chosen to cause breakage - and that breakage results in > people seeing hex codes as the 'summary' which isn't friendly or helpful > to end users (or ... me....) > > >>>>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>>>>> --- > >>>>>> src/libcamera/camera_sensor.cpp | 21 +++++++++++++-------- > >>>>>> 1 file changed, 13 insertions(+), 8 deletions(-) > >>>>>> > >>>>>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > >>>>>> index c9e8d49b7935..9a510108f171 100644 > >>>>>> --- a/src/libcamera/camera_sensor.cpp > >>>>>> +++ b/src/libcamera/camera_sensor.cpp > >>>>>> @@ -346,18 +346,23 @@ int CameraSensor::validateSensorDriver() > >>>>>> * For raw sensors, make sure the sensor driver supports the controls > >>>>>> * required by the CameraSensor class. > >>>>>> */ > >>>>>> - static constexpr uint32_t mandatoryControls[] = { > >>>>>> - V4L2_CID_EXPOSURE, > >>>>>> - V4L2_CID_HBLANK, > >>>>>> - V4L2_CID_PIXEL_RATE, > >>>>>> - V4L2_CID_VBLANK, > >>>>>> + static struct { > >>>>>> + uint32_t ctrl; > >>>>>> + std::string name; > >>>>>> + } mandatoryControls[] = { > >>>>>> +#define MANDATORY_CONTROL(__ctrl) { __ctrl, #__ctrl } > >>>>>> + MANDATORY_CONTROL(V4L2_CID_EXPOSURE), > >>>>>> + MANDATORY_CONTROL(V4L2_CID_HBLANK), > >>>>>> + MANDATORY_CONTROL(V4L2_CID_PIXEL_RATE), > >>>>>> + MANDATORY_CONTROL(V4L2_CID_VBLANK), > >>>>>> +#undef MANDATORY_CONTROL > >>>>>> }; > >>>>>> > >>>>>> err = 0; > >>>>>> - for (uint32_t ctrl : mandatoryControls) { > >>>>>> - if (!controls.count(ctrl)) { > >>>>>> + for (auto &c : mandatoryControls) { > >>>>>> + if (!controls.count(c.ctrl)) { > >>>>>> LOG(CameraSensor, Error) > >>>>>> - << "Mandatory V4L2 control " << utils::hex(ctrl) > >>>>>> + << "Mandatory V4L2 control " << c.name > >>>>>> << " not available"; > >>>>>> err = -EINVAL; > >>>>>> }
diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp index c9e8d49b7935..9a510108f171 100644 --- a/src/libcamera/camera_sensor.cpp +++ b/src/libcamera/camera_sensor.cpp @@ -346,18 +346,23 @@ int CameraSensor::validateSensorDriver() * For raw sensors, make sure the sensor driver supports the controls * required by the CameraSensor class. */ - static constexpr uint32_t mandatoryControls[] = { - V4L2_CID_EXPOSURE, - V4L2_CID_HBLANK, - V4L2_CID_PIXEL_RATE, - V4L2_CID_VBLANK, + static struct { + uint32_t ctrl; + std::string name; + } mandatoryControls[] = { +#define MANDATORY_CONTROL(__ctrl) { __ctrl, #__ctrl } + MANDATORY_CONTROL(V4L2_CID_EXPOSURE), + MANDATORY_CONTROL(V4L2_CID_HBLANK), + MANDATORY_CONTROL(V4L2_CID_PIXEL_RATE), + MANDATORY_CONTROL(V4L2_CID_VBLANK), +#undef MANDATORY_CONTROL }; err = 0; - for (uint32_t ctrl : mandatoryControls) { - if (!controls.count(ctrl)) { + for (auto &c : mandatoryControls) { + if (!controls.count(c.ctrl)) { LOG(CameraSensor, Error) - << "Mandatory V4L2 control " << utils::hex(ctrl) + << "Mandatory V4L2 control " << c.name << " not available"; err = -EINVAL; }
We can not obtain the control names directly from V4L2 so create a map of the control name, when declaring the list of mandatory controls to enable a human readable print of any missing requirements. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/camera_sensor.cpp | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)