[libcamera-devel] [PATCH 01/10] libcamera: v4l2_controls: Remove V4L2ControlInfo::size()

[Jacopo Mondi]

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 at jmondi.org>

Thanks
   j

> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20191015/e5ad1397/attachment-0001.sig>