Re: [PATCH 2/5] v4l: controls: Add support for exponential bases, prefixes and units
Hi Sakari,
On Fri, Sep 28, 2018 at 11:00 PM Hans Verkuil wrote:
>
> On 09/25/2018 12:14 PM, Sakari Ailus wrote:
> > Add support for exponential bases, prefixes as well as units for V4L2
> > controls. This makes it possible to convey information on the relation
> > between the control value and the hardware feature being controlled.
> >
Sorry for being late to the party.
Thanks for the series. I think it has a potential to be very useful.
Please see my comments below.
> > Signed-off-by: Sakari Ailus
> > ---
> > include/uapi/linux/videodev2.h | 32 +++-
> > 1 file changed, 31 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index ae083978988f1..23b02f2db85a1 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -1652,6 +1652,32 @@ struct v4l2_queryctrl {
> > __u32reserved[2];
> > };
> >
> > +/* V4L2 control exponential bases */
> > +#define V4L2_CTRL_BASE_UNDEFINED 0
> > +#define V4L2_CTRL_BASE_LINEAR1
>
> I'm not really sure you need BASE_LINEAR. That is effectively the same
> as UNDEFINED since what else can you do? It's also weird to have this
> as 'base' if the EXPONENTIAL flag is set.
>
> I don't see why you need the EXPONENTIAL flag at all: if this is non-0,
> then you know the exponential base.
Or vice versa, we could remove UNDEFINED and LINEAR altogether and
have the EXPONENTIAL flag actually signify the presence of a valid
base? Besides that, "linear exponential base" just doesn't sound right
or am I missing some basic maths? ;)
Then we could actually have a LOGARITHMIC flag and it could reuse the
same bases enum.
>
> > +#define V4L2_CTRL_BASE_2 2
> > +#define V4L2_CTRL_BASE_1010
> > +
> > +/* V4L2 control unit prefixes */
> > +#define V4L2_CTRL_PREFIX_NANO-9
> > +#define V4L2_CTRL_PREFIX_MICRO -6
> > +#define V4L2_CTRL_PREFIX_MILLI -3
> > +#define V4L2_CTRL_PREFIX_1 0
>
> I would prefer PREFIX_NONE, since there is no prefix in this case.
>
> I assume this prefix is only valid if the unit is not UNDEFINED and not
> NONE?
>
> Is 'base' also dependent on a valid unit? (it doesn't appear to be)
>
> > +#define V4L2_CTRL_PREFIX_KILO3
> > +#define V4L2_CTRL_PREFIX_MEGA6
> > +#define V4L2_CTRL_PREFIX_GIGA9
> > +
> > +/* V4L2 control units */
> > +#define V4L2_CTRL_UNIT_UNDEFINED 0
> > +#define V4L2_CTRL_UNIT_NONE 1
Hmm, what's the meaning of NONE? How does it differ from UNDEFINED?
> > +#define V4L2_CTRL_UNIT_SECOND2
> > +#define V4L2_CTRL_UNIT_AMPERE3
> > +#define V4L2_CTRL_UNIT_LINE 4
> > +#define V4L2_CTRL_UNIT_PIXEL 5
> > +#define V4L2_CTRL_UNIT_PIXELS_PER_SEC6
> > +#define V4L2_CTRL_UNIT_HZ7
> > +
> > +
> > /* Used in the VIDIOC_QUERY_EXT_CTRL ioctl for querying extended controls
> > */
> > struct v4l2_query_ext_ctrl {
> > __u32id;
> > @@ -1666,7 +1692,10 @@ struct v4l2_query_ext_ctrl {
> > __u32elems;
> > __u32nr_of_dims;
> > __u32dims[V4L2_CTRL_MAX_DIMS];
> > - __u32reserved[32];
> > + __u8 base;
> > + __s8 prefix;
Should we make those bigger just in case, or leave some reserved
fields around so we can make them bigger when we need it?
Best regards,
Tomasz
Re: [PATCH 2/5] v4l: controls: Add support for exponential bases, prefixes and units
Hi! > Add support for exponential bases, prefixes as well as units for V4L2 > controls. This makes it possible to convey information on the relation > between the control value and the hardware feature being controlled. > > Signed-off-by: Sakari Ailus Sounds good. > +/* V4L2 control exponential bases */ > +#define V4L2_CTRL_BASE_UNDEFINED 0 > +#define V4L2_CTRL_BASE_LINEAR1 > +#define V4L2_CTRL_BASE_2 2 > +#define V4L2_CTRL_BASE_1010 Do we also want to support logarithmic and 1/x? For example focus is usually in diopters and thats 1/meter... > +/* V4L2 control units */ > +#define V4L2_CTRL_UNIT_UNDEFINED 0 > +#define V4L2_CTRL_UNIT_NONE 1 > +#define V4L2_CTRL_UNIT_SECOND2 > +#define V4L2_CTRL_UNIT_AMPERE3 > +#define V4L2_CTRL_UNIT_LINE 4 > +#define V4L2_CTRL_UNIT_PIXEL 5 > +#define V4L2_CTRL_UNIT_PIXELS_PER_SEC6 > +#define V4L2_CTRL_UNIT_HZ7 And Hz is 1/second. Should we also have some support for ISO-sensitivity and f/ aperture numbers? Thanks, Pavel
Re: [PATCH 2/5] v4l: controls: Add support for exponential bases, prefixes and units
On 10/01/2018 11:27 AM, Helmut Grohne wrote: > On Fri, Sep 28, 2018 at 04:00:17PM +0200, Hans Verkuil wrote: >> On 09/25/2018 12:14 PM, Sakari Ailus wrote: >>> +/* V4L2 control unit prefixes */ >>> +#define V4L2_CTRL_PREFIX_NANO -9 >>> +#define V4L2_CTRL_PREFIX_MICRO -6 >>> +#define V4L2_CTRL_PREFIX_MILLI -3 >>> +#define V4L2_CTRL_PREFIX_1 0 >> >> I would prefer PREFIX_NONE, since there is no prefix in this case. >> >> I assume this prefix is only valid if the unit is not UNDEFINED and not >> NONE? > > Why should it? The prefix is concerned with rescaling a value prior to > presenting it to a user. Even a unitless quantity or a value of > undefined unit can be reasonably scaled. Displaying a unit and scaling > look like orthogonal concepts to me. What's the point? If I have a unit-less control with values 1-1000, then what would a prefix 'milli' tell me as a user? Why would 0.001-1 be better compared to 1-1000? Without a unit it is just an integer range and scaling is meaningless. > >> Is 'base' also dependent on a valid unit? (it doesn't appear to be) > > I'd argue it should not depend on a valid unit like the prefix. I think I agree with that, although I am dubious about the value of the base field as I commented on elsewhere. Regards, Hans
Re: [PATCH 2/5] v4l: controls: Add support for exponential bases, prefixes and units
On Fri, Sep 28, 2018 at 04:00:17PM +0200, Hans Verkuil wrote: > On 09/25/2018 12:14 PM, Sakari Ailus wrote: > > +/* V4L2 control unit prefixes */ > > +#define V4L2_CTRL_PREFIX_NANO -9 > > +#define V4L2_CTRL_PREFIX_MICRO -6 > > +#define V4L2_CTRL_PREFIX_MILLI -3 > > +#define V4L2_CTRL_PREFIX_1 0 > > I would prefer PREFIX_NONE, since there is no prefix in this case. > > I assume this prefix is only valid if the unit is not UNDEFINED and not > NONE? Why should it? The prefix is concerned with rescaling a value prior to presenting it to a user. Even a unitless quantity or a value of undefined unit can be reasonably scaled. Displaying a unit and scaling look like orthogonal concepts to me. > Is 'base' also dependent on a valid unit? (it doesn't appear to be) I'd argue it should not depend on a valid unit like the prefix. Helmut
Re: [PATCH 2/5] v4l: controls: Add support for exponential bases, prefixes and units
On 09/25/2018 12:14 PM, Sakari Ailus wrote:
> Add support for exponential bases, prefixes as well as units for V4L2
> controls. This makes it possible to convey information on the relation
> between the control value and the hardware feature being controlled.
>
> Signed-off-by: Sakari Ailus
> ---
> include/uapi/linux/videodev2.h | 32 +++-
> 1 file changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index ae083978988f1..23b02f2db85a1 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1652,6 +1652,32 @@ struct v4l2_queryctrl {
> __u32reserved[2];
> };
>
> +/* V4L2 control exponential bases */
> +#define V4L2_CTRL_BASE_UNDEFINED 0
> +#define V4L2_CTRL_BASE_LINEAR1
I'm not really sure you need BASE_LINEAR. That is effectively the same
as UNDEFINED since what else can you do? It's also weird to have this
as 'base' if the EXPONENTIAL flag is set.
I don't see why you need the EXPONENTIAL flag at all: if this is non-0,
then you know the exponential base.
> +#define V4L2_CTRL_BASE_2 2
> +#define V4L2_CTRL_BASE_1010
> +
> +/* V4L2 control unit prefixes */
> +#define V4L2_CTRL_PREFIX_NANO-9
> +#define V4L2_CTRL_PREFIX_MICRO -6
> +#define V4L2_CTRL_PREFIX_MILLI -3
> +#define V4L2_CTRL_PREFIX_1 0
I would prefer PREFIX_NONE, since there is no prefix in this case.
I assume this prefix is only valid if the unit is not UNDEFINED and not
NONE?
Is 'base' also dependent on a valid unit? (it doesn't appear to be)
> +#define V4L2_CTRL_PREFIX_KILO3
> +#define V4L2_CTRL_PREFIX_MEGA6
> +#define V4L2_CTRL_PREFIX_GIGA9
> +
> +/* V4L2 control units */
> +#define V4L2_CTRL_UNIT_UNDEFINED 0
> +#define V4L2_CTRL_UNIT_NONE 1
> +#define V4L2_CTRL_UNIT_SECOND2
> +#define V4L2_CTRL_UNIT_AMPERE3
> +#define V4L2_CTRL_UNIT_LINE 4
> +#define V4L2_CTRL_UNIT_PIXEL 5
> +#define V4L2_CTRL_UNIT_PIXELS_PER_SEC6
> +#define V4L2_CTRL_UNIT_HZ7
> +
> +
> /* Used in the VIDIOC_QUERY_EXT_CTRL ioctl for querying extended controls */
> struct v4l2_query_ext_ctrl {
> __u32id;
> @@ -1666,7 +1692,10 @@ struct v4l2_query_ext_ctrl {
> __u32elems;
> __u32nr_of_dims;
> __u32dims[V4L2_CTRL_MAX_DIMS];
> - __u32reserved[32];
> + __u8 base;
> + __s8 prefix;
> + __u16unit;
> + __u32reserved[31];
> };
>
> /* Used in the VIDIOC_QUERYMENU ioctl for querying menu items */
> @@ -1692,6 +1721,7 @@ struct v4l2_querymenu {
> #define V4L2_CTRL_FLAG_HAS_PAYLOAD 0x0100
> #define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE 0x0200
> #define V4L2_CTRL_FLAG_MODIFY_LAYOUT 0x0400
> +#define V4L2_CTRL_FLAG_EXPONENTIAL 0x0800
>
> /* Query flags, to be ORed with the control ID */
> #define V4L2_CTRL_FLAG_NEXT_CTRL 0x8000
>
Regards,
Hans
[PATCH 2/5] v4l: controls: Add support for exponential bases, prefixes and units
Add support for exponential bases, prefixes as well as units for V4L2
controls. This makes it possible to convey information on the relation
between the control value and the hardware feature being controlled.
Signed-off-by: Sakari Ailus
---
include/uapi/linux/videodev2.h | 32 +++-
1 file changed, 31 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index ae083978988f1..23b02f2db85a1 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1652,6 +1652,32 @@ struct v4l2_queryctrl {
__u32reserved[2];
};
+/* V4L2 control exponential bases */
+#define V4L2_CTRL_BASE_UNDEFINED 0
+#define V4L2_CTRL_BASE_LINEAR 1
+#define V4L2_CTRL_BASE_2 2
+#define V4L2_CTRL_BASE_10 10
+
+/* V4L2 control unit prefixes */
+#define V4L2_CTRL_PREFIX_NANO -9
+#define V4L2_CTRL_PREFIX_MICRO -6
+#define V4L2_CTRL_PREFIX_MILLI -3
+#define V4L2_CTRL_PREFIX_1 0
+#define V4L2_CTRL_PREFIX_KILO 3
+#define V4L2_CTRL_PREFIX_MEGA 6
+#define V4L2_CTRL_PREFIX_GIGA 9
+
+/* V4L2 control units */
+#define V4L2_CTRL_UNIT_UNDEFINED 0
+#define V4L2_CTRL_UNIT_NONE1
+#define V4L2_CTRL_UNIT_SECOND 2
+#define V4L2_CTRL_UNIT_AMPERE 3
+#define V4L2_CTRL_UNIT_LINE4
+#define V4L2_CTRL_UNIT_PIXEL 5
+#define V4L2_CTRL_UNIT_PIXELS_PER_SEC 6
+#define V4L2_CTRL_UNIT_HZ 7
+
+
/* Used in the VIDIOC_QUERY_EXT_CTRL ioctl for querying extended controls */
struct v4l2_query_ext_ctrl {
__u32id;
@@ -1666,7 +1692,10 @@ struct v4l2_query_ext_ctrl {
__u32elems;
__u32nr_of_dims;
__u32dims[V4L2_CTRL_MAX_DIMS];
- __u32reserved[32];
+ __u8 base;
+ __s8 prefix;
+ __u16unit;
+ __u32reserved[31];
};
/* Used in the VIDIOC_QUERYMENU ioctl for querying menu items */
@@ -1692,6 +1721,7 @@ struct v4l2_querymenu {
#define V4L2_CTRL_FLAG_HAS_PAYLOAD 0x0100
#define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE0x0200
#define V4L2_CTRL_FLAG_MODIFY_LAYOUT 0x0400
+#define V4L2_CTRL_FLAG_EXPONENTIAL 0x0800
/* Query flags, to be ORed with the control ID */
#define V4L2_CTRL_FLAG_NEXT_CTRL 0x8000
--
2.11.0
