| Message ID | 20231120184529.730565-1-alain.volmat@foss.st.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Alain, Quoting Alain Volmat via libcamera-devel (2023-11-20 18:45:29) > Perform the HBLANK readonly check by looking at the v4l2_query_ext_ctrl > struct for the V4L2_CID_HBLANK instead of checking for min/max values. > Aha, you beat me to it. (ref: https://patchwork.libcamera.org/patch/19189/) I would say we need to fix both here and src/ipa/rpi/common/ipa_base.cpp with the same change though. Could you also update that and compile test please? > Signed-off-by: Alain Volmat <alain.volmat@foss.st.com> > --- > src/libcamera/camera_sensor.cpp | 17 ++++------------- > 1 file changed, 4 insertions(+), 13 deletions(-) > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index 0ef78d9c..3281c1f9 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -188,21 +188,12 @@ int CameraSensor::init() > * Set HBLANK to the minimum to start with a well-defined line length, > * allowing IPA modules that do not modify HBLANK to use the sensor > * minimum line length in their calculations. > - * > - * At present, there is no way of knowing if a control is read-only. > - * As a workaround, assume that if the minimum and maximum values of > - * the V4L2_CID_HBLANK control are the same, it implies the control > - * is read-only. > - * > - * \todo The control API ought to have a flag to specify if a control > - * is read-only which could be used below. > */ > if (ctrls.infoMap()->find(V4L2_CID_HBLANK) != ctrls.infoMap()->end()) { > - const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK); > - const int32_t hblankMin = hblank.min().get<int32_t>(); > - const int32_t hblankMax = hblank.max().get<int32_t>(); > - > - if (hblankMin != hblankMax) { > + const struct v4l2_query_ext_ctrl *hblankInfo = subdev_->controlInfo(V4L2_CID_HBLANK); > + if (!(hblankInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { I don't know if it actually helps or makes sense yet, but if I got here first I was going to see if it would make sense to put a helper into the v4l2_device class to make this more succinct (but still V4L2 specific rather than Control specific). Either with or without a helper - both locations covered would earn a tag from me. -- Kieran > + const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK); > + const int32_t hblankMin = hblank.min().get<int32_t>(); > ControlList ctrl(subdev_->controls()); > > ctrl.set(V4L2_CID_HBLANK, hblankMin); > -- > 2.25.1 >
Hi Kieran On Mon, Nov 20, 2023 at 10:27:32PM +0000, Kieran Bingham via libcamera-devel wrote: > Hi Alain, > > Quoting Alain Volmat via libcamera-devel (2023-11-20 18:45:29) > > Perform the HBLANK readonly check by looking at the v4l2_query_ext_ctrl > > struct for the V4L2_CID_HBLANK instead of checking for min/max values. > > > > Aha, you beat me to it. (ref: > https://patchwork.libcamera.org/patch/19189/) > Thanks for digging it out > I would say we need to fix both here and src/ipa/rpi/common/ipa_base.cpp > with the same change though. Could you also update that and compile test > please? > Why not resume the work done by Naush then and re-propose his series ? Alain could you maybe consider that ? > > > > Signed-off-by: Alain Volmat <alain.volmat@foss.st.com> > > --- > > src/libcamera/camera_sensor.cpp | 17 ++++------------- > > 1 file changed, 4 insertions(+), 13 deletions(-) > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > index 0ef78d9c..3281c1f9 100644 > > --- a/src/libcamera/camera_sensor.cpp > > +++ b/src/libcamera/camera_sensor.cpp > > @@ -188,21 +188,12 @@ int CameraSensor::init() > > * Set HBLANK to the minimum to start with a well-defined line length, > > * allowing IPA modules that do not modify HBLANK to use the sensor > > * minimum line length in their calculations. > > - * > > - * At present, there is no way of knowing if a control is read-only. > > - * As a workaround, assume that if the minimum and maximum values of > > - * the V4L2_CID_HBLANK control are the same, it implies the control > > - * is read-only. > > - * > > - * \todo The control API ought to have a flag to specify if a control > > - * is read-only which could be used below. > > */ > > if (ctrls.infoMap()->find(V4L2_CID_HBLANK) != ctrls.infoMap()->end()) { > > - const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK); > > - const int32_t hblankMin = hblank.min().get<int32_t>(); > > - const int32_t hblankMax = hblank.max().get<int32_t>(); > > - > > - if (hblankMin != hblankMax) { > > + const struct v4l2_query_ext_ctrl *hblankInfo = subdev_->controlInfo(V4L2_CID_HBLANK); > > + if (!(hblankInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > > I don't know if it actually helps or makes sense yet, but if I got here > first I was going to see if it would make sense to put a helper into the > v4l2_device class to make this more succinct (but still V4L2 specific > rather than Control specific). > > Either with or without a helper - both locations covered would earn a > tag from me. > > -- > Kieran > > > > + const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK); > > + const int32_t hblankMin = hblank.min().get<int32_t>(); > > ControlList ctrl(subdev_->controls()); > > > > ctrl.set(V4L2_CID_HBLANK, hblankMin); > > -- > > 2.25.1 > >
Quoting Jacopo Mondi (2023-11-21 09:47:28) > Hi Kieran > > On Mon, Nov 20, 2023 at 10:27:32PM +0000, Kieran Bingham via libcamera-devel wrote: > > Hi Alain, > > > > Quoting Alain Volmat via libcamera-devel (2023-11-20 18:45:29) > > > Perform the HBLANK readonly check by looking at the v4l2_query_ext_ctrl > > > struct for the V4L2_CID_HBLANK instead of checking for min/max values. > > > > > > > Aha, you beat me to it. (ref: > > https://patchwork.libcamera.org/patch/19189/) > > > > Thanks for digging it out > > > I would say we need to fix both here and src/ipa/rpi/common/ipa_base.cpp > > with the same change though. Could you also update that and compile test > > please? > > > > Why not resume the work done by Naush then and re-propose his series ? > Alain could you maybe consider that ? I did. It sounded rejected to me. https://patchwork.libcamera.org/cover/19187/ """ > Only modified one ControlInfo constructor is modified which is used by the > V4L2Device class to allow this flag to be set, as setting it for a non-v4l2 > control probably does not make sense at this point. This is the part that bothers me a bit. If the feature is only used internally, it shouldn't be exposed in the public API. One possible workaround would be to add flag controls as being settable in a request and as being reported in metadata. This is a feature that is useful for applications, and it could then be used internally do indicate read-only internal controls. """ -- Kieran > > > > > > > > Signed-off-by: Alain Volmat <alain.volmat@foss.st.com> > > > --- > > > src/libcamera/camera_sensor.cpp | 17 ++++------------- > > > 1 file changed, 4 insertions(+), 13 deletions(-) > > > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > > index 0ef78d9c..3281c1f9 100644 > > > --- a/src/libcamera/camera_sensor.cpp > > > +++ b/src/libcamera/camera_sensor.cpp > > > @@ -188,21 +188,12 @@ int CameraSensor::init() > > > * Set HBLANK to the minimum to start with a well-defined line length, > > > * allowing IPA modules that do not modify HBLANK to use the sensor > > > * minimum line length in their calculations. > > > - * > > > - * At present, there is no way of knowing if a control is read-only. > > > - * As a workaround, assume that if the minimum and maximum values of > > > - * the V4L2_CID_HBLANK control are the same, it implies the control > > > - * is read-only. > > > - * > > > - * \todo The control API ought to have a flag to specify if a control > > > - * is read-only which could be used below. > > > */ > > > if (ctrls.infoMap()->find(V4L2_CID_HBLANK) != ctrls.infoMap()->end()) { > > > - const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK); > > > - const int32_t hblankMin = hblank.min().get<int32_t>(); > > > - const int32_t hblankMax = hblank.max().get<int32_t>(); > > > - > > > - if (hblankMin != hblankMax) { > > > + const struct v4l2_query_ext_ctrl *hblankInfo = subdev_->controlInfo(V4L2_CID_HBLANK); > > > + if (!(hblankInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > > > > I don't know if it actually helps or makes sense yet, but if I got here > > first I was going to see if it would make sense to put a helper into the > > v4l2_device class to make this more succinct (but still V4L2 specific > > rather than Control specific). > > > > Either with or without a helper - both locations covered would earn a > > tag from me. > > > > -- > > Kieran > > > > > > > + const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK); > > > + const int32_t hblankMin = hblank.min().get<int32_t>(); > > > ControlList ctrl(subdev_->controls()); > > > > > > ctrl.set(V4L2_CID_HBLANK, hblankMin); > > > -- > > > 2.25.1 > > >
Hello, Resurecting this thread, as I've recently sent a similar patch (https://patchwork.libcamera.org/patch/19608/). On Tue, Nov 21, 2023 at 01:12:05PM +0000, 📷-dev wrote: > Quoting Jacopo Mondi (2023-11-21 09:47:28) > > On Mon, Nov 20, 2023 at 10:27:32PM +0000, Kieran Bingham via libcamera-devel wrote: > > > Quoting Alain Volmat via libcamera-devel (2023-11-20 18:45:29) > > > > Perform the HBLANK readonly check by looking at the v4l2_query_ext_ctrl > > > > struct for the V4L2_CID_HBLANK instead of checking for min/max values. > > > > > > Aha, you beat me to it. (ref: > > > https://patchwork.libcamera.org/patch/19189/) > > > > Thanks for digging it out > > > > > I would say we need to fix both here and src/ipa/rpi/common/ipa_base.cpp > > > with the same change though. Could you also update that and compile test > > > please? > > > > Why not resume the work done by Naush then and re-propose his series ? > > Alain could you maybe consider that ? > > I did. It sounded rejected to me. > > https://patchwork.libcamera.org/cover/19187/ > > """ > > Only modified one ControlInfo constructor is modified which is used by the > > V4L2Device class to allow this flag to be set, as setting it for a non-v4l2 > > control probably does not make sense at this point. > > This is the part that bothers me a bit. If the feature is only used > internally, it shouldn't be exposed in the public API. > > One possible workaround would be to add flag controls as being settable > in a request and as being reported in metadata. This is a feature that > is useful for applications, and it could then be used internally do > indicate read-only internal controls. > """ I think it makes sense to decouple the two issues, fixing the hack as done in this patch, and considering extensions to ControlInfo separately. > > > > Signed-off-by: Alain Volmat <alain.volmat@foss.st.com> > > > > --- > > > > src/libcamera/camera_sensor.cpp | 17 ++++------------- > > > > 1 file changed, 4 insertions(+), 13 deletions(-) > > > > > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > > > index 0ef78d9c..3281c1f9 100644 > > > > --- a/src/libcamera/camera_sensor.cpp > > > > +++ b/src/libcamera/camera_sensor.cpp > > > > @@ -188,21 +188,12 @@ int CameraSensor::init() > > > > * Set HBLANK to the minimum to start with a well-defined line length, > > > > * allowing IPA modules that do not modify HBLANK to use the sensor > > > > * minimum line length in their calculations. > > > > - * > > > > - * At present, there is no way of knowing if a control is read-only. > > > > - * As a workaround, assume that if the minimum and maximum values of > > > > - * the V4L2_CID_HBLANK control are the same, it implies the control > > > > - * is read-only. > > > > - * > > > > - * \todo The control API ought to have a flag to specify if a control > > > > - * is read-only which could be used below. > > > > */ > > > > if (ctrls.infoMap()->find(V4L2_CID_HBLANK) != ctrls.infoMap()->end()) { > > > > - const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK); > > > > - const int32_t hblankMin = hblank.min().get<int32_t>(); > > > > - const int32_t hblankMax = hblank.max().get<int32_t>(); > > > > - > > > > - if (hblankMin != hblankMax) { > > > > + const struct v4l2_query_ext_ctrl *hblankInfo = subdev_->controlInfo(V4L2_CID_HBLANK); > > > > + if (!(hblankInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { The patch I sent drops the ctrls.infoMap()->find(V4L2_CID_HBLANK) check above, and instead checks that hblankInfo is not null. The rest of the series drops the ctrls variable from the context where tha HBLANK handling is located, so I would prefer keeping that. > > > > > > I don't know if it actually helps or makes sense yet, but if I got here > > > first I was going to see if it would make sense to put a helper into the > > > v4l2_device class to make this more succinct (but still V4L2 specific > > > rather than Control specific). > > > > > > Either with or without a helper - both locations covered would earn a > > > tag from me. > > > > > > > + const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK); > > > > + const int32_t hblankMin = hblank.min().get<int32_t>(); > > > > ControlList ctrl(subdev_->controls()); > > > > > > > > ctrl.set(V4L2_CID_HBLANK, hblankMin);
diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp index 0ef78d9c..3281c1f9 100644 --- a/src/libcamera/camera_sensor.cpp +++ b/src/libcamera/camera_sensor.cpp @@ -188,21 +188,12 @@ int CameraSensor::init() * Set HBLANK to the minimum to start with a well-defined line length, * allowing IPA modules that do not modify HBLANK to use the sensor * minimum line length in their calculations. - * - * At present, there is no way of knowing if a control is read-only. - * As a workaround, assume that if the minimum and maximum values of - * the V4L2_CID_HBLANK control are the same, it implies the control - * is read-only. - * - * \todo The control API ought to have a flag to specify if a control - * is read-only which could be used below. */ if (ctrls.infoMap()->find(V4L2_CID_HBLANK) != ctrls.infoMap()->end()) { - const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK); - const int32_t hblankMin = hblank.min().get<int32_t>(); - const int32_t hblankMax = hblank.max().get<int32_t>(); - - if (hblankMin != hblankMax) { + const struct v4l2_query_ext_ctrl *hblankInfo = subdev_->controlInfo(V4L2_CID_HBLANK); + if (!(hblankInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { + const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK); + const int32_t hblankMin = hblank.min().get<int32_t>(); ControlList ctrl(subdev_->controls()); ctrl.set(V4L2_CID_HBLANK, hblankMin);
Perform the HBLANK readonly check by looking at the v4l2_query_ext_ctrl struct for the V4L2_CID_HBLANK instead of checking for min/max values. Signed-off-by: Alain Volmat <alain.volmat@foss.st.com> --- src/libcamera/camera_sensor.cpp | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-)