| Message ID | 20191013232755.3292-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your work. On 2019-10-14 02:27:47 +0300, Laurent Pinchart wrote: > We don't support V4L2 compound controls, the size field is thus unused. > Remove it to ease merging of the libcamera and V4L2 control info > classes. Support for array controls can then be added later on top, and > would be useful for libcamera controls too. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/include/v4l2_controls.h | 4 ---- > src/libcamera/v4l2_controls.cpp | 8 -------- > src/libcamera/v4l2_device.cpp | 14 ++++++++++++-- > 3 files changed, 12 insertions(+), 14 deletions(-) > > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h > index 89cc74485e6f..133f5262febf 100644 > --- a/src/libcamera/include/v4l2_controls.h > +++ b/src/libcamera/include/v4l2_controls.h > @@ -31,14 +31,10 @@ public: > V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl); > > const ControlId &id() const { return id_; } > - size_t size() const { return size_; } > - > const ControlRange &range() const { return range_; } > > private: > V4L2ControlId id_; > - size_t size_; > - > ControlRange range_; > }; > > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp > index c45d3fda2e1f..dcf31b7a8f26 100644 > --- a/src/libcamera/v4l2_controls.cpp > +++ b/src/libcamera/v4l2_controls.cpp > @@ -127,8 +127,6 @@ V4L2ControlId::V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl) > V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) > : id_(ctrl) > { > - size_ = ctrl.elem_size * ctrl.elems; > - > if (ctrl.type == V4L2_CTRL_TYPE_INTEGER64) > range_ = ControlRange(static_cast<int64_t>(ctrl.minimum), > static_cast<int64_t>(ctrl.maximum)); > @@ -143,12 +141,6 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) > * \return The V4L2 control ID > */ > > -/** > - * \fn V4L2ControlInfo::size() > - * \brief Retrieve the control value data size (in bytes) > - * \return The V4L2 control value data size > - */ > - > /** > * \fn V4L2ControlInfo::range() > * \brief Retrieve the control value range > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index b47ba448f354..54cc214ecce9 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -8,11 +8,13 @@ > #include "v4l2_device.h" > > #include <fcntl.h> > +#include <iomanip> > #include <string.h> > #include <sys/ioctl.h> > #include <unistd.h> > > #include "log.h" > +#include "utils.h" > #include "v4l2_controls.h" > > /** > @@ -360,6 +362,13 @@ void V4L2Device::listControls() > ctrl.flags & V4L2_CTRL_FLAG_DISABLED) > continue; > > + if (ctrl.elems != 1 || ctrl.nr_of_dims) { > + LOG(V4L2, Debug) > + << "Array control " << utils::hex(ctrl.id) > + << " not supported"; > + continue; > + } > + > switch (ctrl.type) { > case V4L2_CTRL_TYPE_INTEGER: > case V4L2_CTRL_TYPE_BOOLEAN: > @@ -371,8 +380,9 @@ void V4L2Device::listControls() > break; > /* \todo Support compound controls. */ > default: > - LOG(V4L2, Debug) << "Control type '" << ctrl.type > - << "' not supported"; > + LOG(V4L2, Debug) > + << "Control " << utils::hex(ctrl.id) > + << " has unsupported type " << ctrl.type; > continue; This looks like something for a separate patch ;-) With or without this fixed, Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > } > > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, On Mon, Oct 14, 2019 at 02:27:47AM +0300, Laurent Pinchart wrote: > We don't support V4L2 compound controls, the size field is thus unused. > Remove it to ease merging of the libcamera and V4L2 control info > classes. Support for array controls can then be added later on top, and > would be useful for libcamera controls too. I understand for now it's trivial to get the size of the data from the type, but we'll need this soon so I'm not sure it is worth removing this. I would rather keep it and at the end of this series rework move it to ControlId. For libcamera controls the size would be the size of the type (unless otherwise specified in the Control<> definition). For V4L2 controls the size comes from the kernel, and V4L2ControlId is exactly contructed from a v4l2_query_ext_ctrl. What do you think ? > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/include/v4l2_controls.h | 4 ---- > src/libcamera/v4l2_controls.cpp | 8 -------- > src/libcamera/v4l2_device.cpp | 14 ++++++++++++-- > 3 files changed, 12 insertions(+), 14 deletions(-) > > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h > index 89cc74485e6f..133f5262febf 100644 > --- a/src/libcamera/include/v4l2_controls.h > +++ b/src/libcamera/include/v4l2_controls.h > @@ -31,14 +31,10 @@ public: > V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl); > > const ControlId &id() const { return id_; } > - size_t size() const { return size_; } > - > const ControlRange &range() const { return range_; } > > private: > V4L2ControlId id_; > - size_t size_; > - > ControlRange range_; > }; > > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp > index c45d3fda2e1f..dcf31b7a8f26 100644 > --- a/src/libcamera/v4l2_controls.cpp > +++ b/src/libcamera/v4l2_controls.cpp > @@ -127,8 +127,6 @@ V4L2ControlId::V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl) > V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) > : id_(ctrl) > { > - size_ = ctrl.elem_size * ctrl.elems; > - > if (ctrl.type == V4L2_CTRL_TYPE_INTEGER64) > range_ = ControlRange(static_cast<int64_t>(ctrl.minimum), > static_cast<int64_t>(ctrl.maximum)); > @@ -143,12 +141,6 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) > * \return The V4L2 control ID > */ > > -/** > - * \fn V4L2ControlInfo::size() > - * \brief Retrieve the control value data size (in bytes) > - * \return The V4L2 control value data size > - */ > - > /** > * \fn V4L2ControlInfo::range() > * \brief Retrieve the control value range > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index b47ba448f354..54cc214ecce9 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -8,11 +8,13 @@ > #include "v4l2_device.h" > > #include <fcntl.h> > +#include <iomanip> > #include <string.h> > #include <sys/ioctl.h> > #include <unistd.h> > > #include "log.h" > +#include "utils.h" > #include "v4l2_controls.h" > > /** > @@ -360,6 +362,13 @@ void V4L2Device::listControls() > ctrl.flags & V4L2_CTRL_FLAG_DISABLED) > continue; > > + if (ctrl.elems != 1 || ctrl.nr_of_dims) { > + LOG(V4L2, Debug) > + << "Array control " << utils::hex(ctrl.id) > + << " not supported"; > + continue; > + } > + > switch (ctrl.type) { > case V4L2_CTRL_TYPE_INTEGER: > case V4L2_CTRL_TYPE_BOOLEAN: > @@ -371,8 +380,9 @@ void V4L2Device::listControls() > break; > /* \todo Support compound controls. */ > default: > - LOG(V4L2, Debug) << "Control type '" << ctrl.type > - << "' not supported"; > + LOG(V4L2, Debug) > + << "Control " << utils::hex(ctrl.id) > + << " has unsupported type " << ctrl.type; > continue; > } > > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Tue, Oct 15, 2019 at 04:50:54PM +0200, Jacopo Mondi wrote: > On Mon, Oct 14, 2019 at 02:27:47AM +0300, Laurent Pinchart wrote: > > We don't support V4L2 compound controls, the size field is thus unused. > > Remove it to ease merging of the libcamera and V4L2 control info > > classes. Support for array controls can then be added later on top, and > > would be useful for libcamera controls too. > > I understand for now it's trivial to get the size of the data from the > type, but we'll need this soon so I'm not sure it is worth removing > this. > > I would rather keep it and at the end of this series rework move it to > ControlId. For libcamera controls the size would be the size of the > type (unless otherwise specified in the Control<> definition). For > V4L2 controls the size comes from the kernel, and V4L2ControlId is > exactly contructed from a v4l2_query_ext_ctrl. > > What do you think ? I would prefer adding it back on top :-) Beside the obvious reason that it would be easier to avoid a complete rework (but that's hardly an excuse for not doing it), I think it would be best to add the size field back when needed, when we will know what we need it for. Currently it stores the size of a compound V4L2 control, but we don't support those, and generalising it to ControlId will require us to think about how to support compound (or at least array) controls for libcamera controls too. I would rather not go through that design phase as a dependency for this series. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/libcamera/include/v4l2_controls.h | 4 ---- > > src/libcamera/v4l2_controls.cpp | 8 -------- > > src/libcamera/v4l2_device.cpp | 14 ++++++++++++-- > > 3 files changed, 12 insertions(+), 14 deletions(-) > > > > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h > > index 89cc74485e6f..133f5262febf 100644 > > --- a/src/libcamera/include/v4l2_controls.h > > +++ b/src/libcamera/include/v4l2_controls.h > > @@ -31,14 +31,10 @@ public: > > V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl); > > > > const ControlId &id() const { return id_; } > > - size_t size() const { return size_; } > > - > > const ControlRange &range() const { return range_; } > > > > private: > > V4L2ControlId id_; > > - size_t size_; > > - > > ControlRange range_; > > }; > > > > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp > > index c45d3fda2e1f..dcf31b7a8f26 100644 > > --- a/src/libcamera/v4l2_controls.cpp > > +++ b/src/libcamera/v4l2_controls.cpp > > @@ -127,8 +127,6 @@ V4L2ControlId::V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl) > > V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) > > : id_(ctrl) > > { > > - size_ = ctrl.elem_size * ctrl.elems; > > - > > if (ctrl.type == V4L2_CTRL_TYPE_INTEGER64) > > range_ = ControlRange(static_cast<int64_t>(ctrl.minimum), > > static_cast<int64_t>(ctrl.maximum)); > > @@ -143,12 +141,6 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) > > * \return The V4L2 control ID > > */ > > > > -/** > > - * \fn V4L2ControlInfo::size() > > - * \brief Retrieve the control value data size (in bytes) > > - * \return The V4L2 control value data size > > - */ > > - > > /** > > * \fn V4L2ControlInfo::range() > > * \brief Retrieve the control value range > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > index b47ba448f354..54cc214ecce9 100644 > > --- a/src/libcamera/v4l2_device.cpp > > +++ b/src/libcamera/v4l2_device.cpp > > @@ -8,11 +8,13 @@ > > #include "v4l2_device.h" > > > > #include <fcntl.h> > > +#include <iomanip> > > #include <string.h> > > #include <sys/ioctl.h> > > #include <unistd.h> > > > > #include "log.h" > > +#include "utils.h" > > #include "v4l2_controls.h" > > > > /** > > @@ -360,6 +362,13 @@ void V4L2Device::listControls() > > ctrl.flags & V4L2_CTRL_FLAG_DISABLED) > > continue; > > > > + if (ctrl.elems != 1 || ctrl.nr_of_dims) { > > + LOG(V4L2, Debug) > > + << "Array control " << utils::hex(ctrl.id) > > + << " not supported"; > > + continue; > > + } > > + > > switch (ctrl.type) { > > case V4L2_CTRL_TYPE_INTEGER: > > case V4L2_CTRL_TYPE_BOOLEAN: > > @@ -371,8 +380,9 @@ void V4L2Device::listControls() > > break; > > /* \todo Support compound controls. */ > > default: > > - LOG(V4L2, Debug) << "Control type '" << ctrl.type > > - << "' not supported"; > > + LOG(V4L2, Debug) > > + << "Control " << utils::hex(ctrl.id) > > + << " has unsupported type " << ctrl.type; > > continue; > > } > >
Hi Laurent, On Tue, Oct 15, 2019 at 05:54:56PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > On Tue, Oct 15, 2019 at 04:50:54PM +0200, Jacopo Mondi wrote: > > On Mon, Oct 14, 2019 at 02:27:47AM +0300, Laurent Pinchart wrote: > > > We don't support V4L2 compound controls, the size field is thus unused. > > > Remove it to ease merging of the libcamera and V4L2 control info > > > classes. Support for array controls can then be added later on top, and > > > would be useful for libcamera controls too. > > > > I understand for now it's trivial to get the size of the data from the > > type, but we'll need this soon so I'm not sure it is worth removing > > this. > > > > I would rather keep it and at the end of this series rework move it to > > ControlId. For libcamera controls the size would be the size of the > > type (unless otherwise specified in the Control<> definition). For > > V4L2 controls the size comes from the kernel, and V4L2ControlId is > > exactly contructed from a v4l2_query_ext_ctrl. > > > > What do you think ? > > I would prefer adding it back on top :-) Beside the obvious reason that > it would be easier to avoid a complete rework (but that's hardly an > excuse for not doing it), I think it would be best to add the size field > back when needed, when we will know what we need it for. Currently it > stores the size of a compound V4L2 control, but we don't support those, > and generalising it to ControlId will require us to think about how to > support compound (or at least array) controls for libcamera controls > too. I would rather not go through that design phase as a dependency for > this series. > Makes sense, so please have my tag on this one Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > src/libcamera/include/v4l2_controls.h | 4 ---- > > > src/libcamera/v4l2_controls.cpp | 8 -------- > > > src/libcamera/v4l2_device.cpp | 14 ++++++++++++-- > > > 3 files changed, 12 insertions(+), 14 deletions(-) > > > > > > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h > > > index 89cc74485e6f..133f5262febf 100644 > > > --- a/src/libcamera/include/v4l2_controls.h > > > +++ b/src/libcamera/include/v4l2_controls.h > > > @@ -31,14 +31,10 @@ public: > > > V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl); > > > > > > const ControlId &id() const { return id_; } > > > - size_t size() const { return size_; } > > > - > > > const ControlRange &range() const { return range_; } > > > > > > private: > > > V4L2ControlId id_; > > > - size_t size_; > > > - > > > ControlRange range_; > > > }; > > > > > > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp > > > index c45d3fda2e1f..dcf31b7a8f26 100644 > > > --- a/src/libcamera/v4l2_controls.cpp > > > +++ b/src/libcamera/v4l2_controls.cpp > > > @@ -127,8 +127,6 @@ V4L2ControlId::V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl) > > > V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) > > > : id_(ctrl) > > > { > > > - size_ = ctrl.elem_size * ctrl.elems; > > > - > > > if (ctrl.type == V4L2_CTRL_TYPE_INTEGER64) > > > range_ = ControlRange(static_cast<int64_t>(ctrl.minimum), > > > static_cast<int64_t>(ctrl.maximum)); > > > @@ -143,12 +141,6 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) > > > * \return The V4L2 control ID > > > */ > > > > > > -/** > > > - * \fn V4L2ControlInfo::size() > > > - * \brief Retrieve the control value data size (in bytes) > > > - * \return The V4L2 control value data size > > > - */ > > > - > > > /** > > > * \fn V4L2ControlInfo::range() > > > * \brief Retrieve the control value range > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > > index b47ba448f354..54cc214ecce9 100644 > > > --- a/src/libcamera/v4l2_device.cpp > > > +++ b/src/libcamera/v4l2_device.cpp > > > @@ -8,11 +8,13 @@ > > > #include "v4l2_device.h" > > > > > > #include <fcntl.h> > > > +#include <iomanip> > > > #include <string.h> > > > #include <sys/ioctl.h> > > > #include <unistd.h> > > > > > > #include "log.h" > > > +#include "utils.h" > > > #include "v4l2_controls.h" > > > > > > /** > > > @@ -360,6 +362,13 @@ void V4L2Device::listControls() > > > ctrl.flags & V4L2_CTRL_FLAG_DISABLED) > > > continue; > > > > > > + if (ctrl.elems != 1 || ctrl.nr_of_dims) { > > > + LOG(V4L2, Debug) > > > + << "Array control " << utils::hex(ctrl.id) > > > + << " not supported"; > > > + continue; > > > + } > > > + > > > switch (ctrl.type) { > > > case V4L2_CTRL_TYPE_INTEGER: > > > case V4L2_CTRL_TYPE_BOOLEAN: > > > @@ -371,8 +380,9 @@ void V4L2Device::listControls() > > > break; > > > /* \todo Support compound controls. */ > > > default: > > > - LOG(V4L2, Debug) << "Control type '" << ctrl.type > > > - << "' not supported"; > > > + LOG(V4L2, Debug) > > > + << "Control " << utils::hex(ctrl.id) > > > + << " has unsupported type " << ctrl.type; > > > continue; > > > } > > > > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h index 89cc74485e6f..133f5262febf 100644 --- a/src/libcamera/include/v4l2_controls.h +++ b/src/libcamera/include/v4l2_controls.h @@ -31,14 +31,10 @@ public: V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl); const ControlId &id() const { return id_; } - size_t size() const { return size_; } - const ControlRange &range() const { return range_; } private: V4L2ControlId id_; - size_t size_; - ControlRange range_; }; diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp index c45d3fda2e1f..dcf31b7a8f26 100644 --- a/src/libcamera/v4l2_controls.cpp +++ b/src/libcamera/v4l2_controls.cpp @@ -127,8 +127,6 @@ V4L2ControlId::V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl) V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) : id_(ctrl) { - size_ = ctrl.elem_size * ctrl.elems; - if (ctrl.type == V4L2_CTRL_TYPE_INTEGER64) range_ = ControlRange(static_cast<int64_t>(ctrl.minimum), static_cast<int64_t>(ctrl.maximum)); @@ -143,12 +141,6 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) * \return The V4L2 control ID */ -/** - * \fn V4L2ControlInfo::size() - * \brief Retrieve the control value data size (in bytes) - * \return The V4L2 control value data size - */ - /** * \fn V4L2ControlInfo::range() * \brief Retrieve the control value range diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index b47ba448f354..54cc214ecce9 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -8,11 +8,13 @@ #include "v4l2_device.h" #include <fcntl.h> +#include <iomanip> #include <string.h> #include <sys/ioctl.h> #include <unistd.h> #include "log.h" +#include "utils.h" #include "v4l2_controls.h" /** @@ -360,6 +362,13 @@ void V4L2Device::listControls() ctrl.flags & V4L2_CTRL_FLAG_DISABLED) continue; + if (ctrl.elems != 1 || ctrl.nr_of_dims) { + LOG(V4L2, Debug) + << "Array control " << utils::hex(ctrl.id) + << " not supported"; + continue; + } + switch (ctrl.type) { case V4L2_CTRL_TYPE_INTEGER: case V4L2_CTRL_TYPE_BOOLEAN: @@ -371,8 +380,9 @@ void V4L2Device::listControls() break; /* \todo Support compound controls. */ default: - LOG(V4L2, Debug) << "Control type '" << ctrl.type - << "' not supported"; + LOG(V4L2, Debug) + << "Control " << utils::hex(ctrl.id) + << " has unsupported type " << ctrl.type; continue; }
We don't support V4L2 compound controls, the size field is thus unused. Remove it to ease merging of the libcamera and V4L2 control info classes. Support for array controls can then be added later on top, and would be useful for libcamera controls too. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/include/v4l2_controls.h | 4 ---- src/libcamera/v4l2_controls.cpp | 8 -------- src/libcamera/v4l2_device.cpp | 14 ++++++++++++-- 3 files changed, 12 insertions(+), 14 deletions(-)