| Message ID | 1390221974-28194-3-git-send-email-hverkuil@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/20/2014 01:45 PM, Hans Verkuil wrote: > From: Hans Verkuil<hans.verkuil@cisco.com> > > The upcoming VIDIOC_QUERY_EXT_CTRL adds support for a unit string. This > allows userspace to show the unit belonging to a particular control. > > This patch adds support for the unit string to the control framework. > > Signed-off-by: Hans Verkuil<hans.verkuil@cisco.com> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com> -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, Thanks for the patchset! On Mon, Jan 20, 2014 at 01:45:55PM +0100, Hans Verkuil wrote: > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h > index 0b347e8..3998049 100644 > --- a/include/media/v4l2-ctrls.h > +++ b/include/media/v4l2-ctrls.h > @@ -85,6 +85,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv); > * @ops: The control ops. > * @id: The control ID. > * @name: The control name. > + * @unit: The control's unit. May be NULL. > * @type: The control type. > * @minimum: The control's minimum value. > * @maximum: The control's maximum value. > @@ -130,6 +131,7 @@ struct v4l2_ctrl { > const struct v4l2_ctrl_ops *ops; > u32 id; > const char *name; > + const char *unit; What would you think of using a numeric value (with the standardised units #defined)? I think using a string begs for unmanaged unit usage. Code that deals with units might work with one driver but not with another since it uses a slightly different string for unit x. A prefix could be potentially nice, too, so ms and µs would still have the same unit but a different prefix.
On 01/24/2014 11:35 AM, Sakari Ailus wrote: > Hi Hans, > > Thanks for the patchset! > > On Mon, Jan 20, 2014 at 01:45:55PM +0100, Hans Verkuil wrote: >> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h >> index 0b347e8..3998049 100644 >> --- a/include/media/v4l2-ctrls.h >> +++ b/include/media/v4l2-ctrls.h >> @@ -85,6 +85,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv); >> * @ops: The control ops. >> * @id: The control ID. >> * @name: The control name. >> + * @unit: The control's unit. May be NULL. >> * @type: The control type. >> * @minimum: The control's minimum value. >> * @maximum: The control's maximum value. >> @@ -130,6 +131,7 @@ struct v4l2_ctrl { >> const struct v4l2_ctrl_ops *ops; >> u32 id; >> const char *name; >> + const char *unit; > > What would you think of using a numeric value (with the standardised units > #defined)? I think using a string begs for unmanaged unit usage. Code that > deals with units might work with one driver but not with another since it > uses a slightly different string for unit x. First of all, you always need a string. You don't want GUIs like qv4l2 to have to switch on a unit in order to generate the unit strings. That's impossible to keep up to date. In addition, private controls can have really strange custom units, so you want to have a string there as well. Standard controls can have their unit string set in v4l2-ctrls.c, just as their name is set there these days, thus ensuring consistency. What I had in mind is that videodev2.h defines a list of standardized unit strings, e.g.: #define V4L2_CTRL_UNIT_USECS "usecs" #define V4L2_CTRL_UNIT_MSECS "msecs" and apps can do strcmp(qc->unit, V4L2_CTRL_UNIT_USECS) to see what the unit is. If a driver doesn't use one of those standardized unit strings, then it is a driver bug. > A prefix could be potentially nice, too, so ms and µs would still have the > same unit but a different prefix. Can you give an example of a prefix? I don't really follow what you want to achieve. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, On Fri, Jan 24, 2014 at 12:19:30PM +0100, Hans Verkuil wrote: > On 01/24/2014 11:35 AM, Sakari Ailus wrote: > > Hi Hans, > > > > Thanks for the patchset! > > > > On Mon, Jan 20, 2014 at 01:45:55PM +0100, Hans Verkuil wrote: > >> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h > >> index 0b347e8..3998049 100644 > >> --- a/include/media/v4l2-ctrls.h > >> +++ b/include/media/v4l2-ctrls.h > >> @@ -85,6 +85,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv); > >> * @ops: The control ops. > >> * @id: The control ID. > >> * @name: The control name. > >> + * @unit: The control's unit. May be NULL. > >> * @type: The control type. > >> * @minimum: The control's minimum value. > >> * @maximum: The control's maximum value. > >> @@ -130,6 +131,7 @@ struct v4l2_ctrl { > >> const struct v4l2_ctrl_ops *ops; > >> u32 id; > >> const char *name; > >> + const char *unit; > > > > What would you think of using a numeric value (with the standardised units > > #defined)? I think using a string begs for unmanaged unit usage. Code that > > deals with units might work with one driver but not with another since it > > uses a slightly different string for unit x. > > First of all, you always need a string. You don't want GUIs like qv4l2 to have > to switch on a unit in order to generate the unit strings. That's impossible to > keep up to date. That's true when when you want to show that to the user, yes. But when you have an application which tries to figure out which value to put to the control, a numeric value is more convenient. Kernel interfaces seldom use strings and that's for a good reason. > In addition, private controls can have really strange custom units, so you want > to have a string there as well. Good point as well. > Standard controls can have their unit string set in v4l2-ctrls.c, just as their > name is set there these days, thus ensuring consistency. > > What I had in mind is that videodev2.h defines a list of standardized unit strings, > e.g.: > > #define V4L2_CTRL_UNIT_USECS "usecs" > #define V4L2_CTRL_UNIT_MSECS "msecs" That's possible as well, but requires the user to e.g. use if (strcmp()) ... instead of just plain switch (unit) { ... }. I'd also very much prefer to stick to SI units and prefixes where applicable if we end up using strings. Combining the unit and prefix could make sense. > and apps can do strcmp(qc->unit, V4L2_CTRL_UNIT_USECS) to see what the unit is. > If a driver doesn't use one of those standardized unit strings, then it is a > driver bug. > > > A prefix could be potentially nice, too, so ms and µs would still have the > > same unit but a different prefix. > > Can you give an example of a prefix? I don't really follow what you want to > achieve. You use them in your own example above. :-) <URL:http://en.wikipedia.org/wiki/SI_prefix>
Hi Sakari, On 01/24/2014 04:54 PM, Sakari Ailus wrote: > Hi Hans, > > On Fri, Jan 24, 2014 at 12:19:30PM +0100, Hans Verkuil wrote: >> On 01/24/2014 11:35 AM, Sakari Ailus wrote: >>> Hi Hans, >>> >>> Thanks for the patchset! >>> >>> On Mon, Jan 20, 2014 at 01:45:55PM +0100, Hans Verkuil wrote: >>>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h >>>> index 0b347e8..3998049 100644 >>>> --- a/include/media/v4l2-ctrls.h >>>> +++ b/include/media/v4l2-ctrls.h >>>> @@ -85,6 +85,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv); >>>> * @ops: The control ops. >>>> * @id: The control ID. >>>> * @name: The control name. >>>> + * @unit: The control's unit. May be NULL. >>>> * @type: The control type. >>>> * @minimum: The control's minimum value. >>>> * @maximum: The control's maximum value. >>>> @@ -130,6 +131,7 @@ struct v4l2_ctrl { >>>> const struct v4l2_ctrl_ops *ops; >>>> u32 id; >>>> const char *name; >>>> + const char *unit; >>> >>> What would you think of using a numeric value (with the standardised units >>> #defined)? I think using a string begs for unmanaged unit usage. Code that >>> deals with units might work with one driver but not with another since it >>> uses a slightly different string for unit x. >> >> First of all, you always need a string. You don't want GUIs like qv4l2 to have >> to switch on a unit in order to generate the unit strings. That's impossible to >> keep up to date. > > That's true when when you want to show that to the user, yes. But when you > have an application which tries to figure out which value to put to the > control, a numeric value is more convenient. > > Kernel interfaces seldom use strings and that's for a good reason. Well, actually, the kernel is in many places moving away from IDs to strings. Headers full of IDs are surprisingly hard to maintain. > >> In addition, private controls can have really strange custom units, so you want >> to have a string there as well. > > Good point as well. > >> Standard controls can have their unit string set in v4l2-ctrls.c, just as their >> name is set there these days, thus ensuring consistency. >> >> What I had in mind is that videodev2.h defines a list of standardized unit strings, >> e.g.: >> >> #define V4L2_CTRL_UNIT_USECS "usecs" >> #define V4L2_CTRL_UNIT_MSECS "msecs" > > That's possible as well, but requires the user to e.g. use if (strcmp()) > ... instead of just plain switch (unit) { ... }. > > I'd also very much prefer to stick to SI units and prefixes where applicable > if we end up using strings. Combining the unit and prefix could make sense. > >> and apps can do strcmp(qc->unit, V4L2_CTRL_UNIT_USECS) to see what the unit is. >> If a driver doesn't use one of those standardized unit strings, then it is a >> driver bug. >> >>> A prefix could be potentially nice, too, so ms and µs would still have the >>> same unit but a different prefix. >> >> Can you give an example of a prefix? I don't really follow what you want to >> achieve. > > You use them in your own example above. :-) > > <URL:http://en.wikipedia.org/wiki/SI_prefix> > Ah, that sort of prefix. I don't think the prefix makes sense, for a number of reasons: first I think it makes life even harder for applications, since they now have to factor in a SI prefix as well. Secondly, what to do with units like km/s? There are two prefixes there (e.g. mm/usecs is also a valid speed representation). I think that for the initial version we just add the unit string as that is needed anyway. There are more than enough reserved fields available to add a unit_id field later, but frankly, I'd like to see some real-life use-cases first. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c index ccaa38f..ee8ea66 100644 --- a/drivers/media/v4l2-core/v4l2-common.c +++ b/drivers/media/v4l2-core/v4l2-common.c @@ -114,12 +114,13 @@ EXPORT_SYMBOL(v4l2_ctrl_check); int v4l2_ctrl_query_fill(struct v4l2_queryctrl *qctrl, s32 _min, s32 _max, s32 _step, s32 _def) { const char *name; + const char *unit = NULL; s64 min = _min; s64 max = _max; u64 step = _step; s64 def = _def; - v4l2_ctrl_fill(qctrl->id, &name, &qctrl->type, + v4l2_ctrl_fill(qctrl->id, &name, &unit, &qctrl->type, &min, &max, &step, &def, &qctrl->flags); if (name == NULL) diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c index 5b60b53..0b9246b 100644 --- a/drivers/media/v4l2-core/v4l2-ctrls.c +++ b/drivers/media/v4l2-core/v4l2-ctrls.c @@ -863,8 +863,9 @@ const char *v4l2_ctrl_get_name(u32 id) } EXPORT_SYMBOL(v4l2_ctrl_get_name); -void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, - s64 *min, s64 *max, u64 *step, s64 *def, u32 *flags) +void v4l2_ctrl_fill(u32 id, const char **name, const char **unit, + enum v4l2_ctrl_type *type, s64 *min, s64 *max, + u64 *step, s64 *def, u32 *flags) { *name = v4l2_ctrl_get_name(id); *flags = 0; @@ -1627,7 +1628,8 @@ unlock: /* Add a new control */ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, const struct v4l2_ctrl_ops *ops, - u32 id, const char *name, enum v4l2_ctrl_type type, + u32 id, const char *name, const char *unit, + enum v4l2_ctrl_type type, s64 min, s64 max, u64 step, s64 def, u32 flags, const char * const *qmenu, const s64 *qmenu_int, void *priv) @@ -1675,6 +1677,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, ctrl->ops = ops; ctrl->id = id; ctrl->name = name; + ctrl->unit = unit; ctrl->type = type; ctrl->flags = flags; ctrl->minimum = min; @@ -1709,6 +1712,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl, bool is_menu; struct v4l2_ctrl *ctrl; const char *name = cfg->name; + const char *unit = cfg->unit; const char * const *qmenu = cfg->qmenu; const s64 *qmenu_int = cfg->qmenu_int; enum v4l2_ctrl_type type = cfg->type; @@ -1719,8 +1723,8 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl, s64 def = cfg->def; if (name == NULL) - v4l2_ctrl_fill(cfg->id, &name, &type, &min, &max, &step, - &def, &flags); + v4l2_ctrl_fill(cfg->id, &name, &unit, &type, + &min, &max, &step, &def, &flags); is_menu = (cfg->type == V4L2_CTRL_TYPE_MENU || cfg->type == V4L2_CTRL_TYPE_INTEGER_MENU); @@ -1736,7 +1740,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl, return NULL; } - ctrl = v4l2_ctrl_new(hdl, cfg->ops, cfg->id, name, + ctrl = v4l2_ctrl_new(hdl, cfg->ops, cfg->id, name, unit, type, min, max, is_menu ? cfg->menu_skip_mask : step, def, flags, qmenu, qmenu_int, priv); @@ -1752,16 +1756,17 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl, u32 id, s64 min, s64 max, u64 step, s64 def) { const char *name; + const char *unit = NULL; enum v4l2_ctrl_type type; u32 flags; - v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags); + v4l2_ctrl_fill(id, &name, &unit, &type, &min, &max, &step, &def, &flags); if (type == V4L2_CTRL_TYPE_MENU || type == V4L2_CTRL_TYPE_INTEGER_MENU) { handler_set_err(hdl, -EINVAL); return NULL; } - return v4l2_ctrl_new(hdl, ops, id, name, type, + return v4l2_ctrl_new(hdl, ops, id, name, unit, type, min, max, step, def, flags, NULL, NULL, NULL); } EXPORT_SYMBOL(v4l2_ctrl_new_std); @@ -1775,6 +1780,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl, const s64 *qmenu_int = NULL; unsigned int qmenu_int_len = 0; const char *name; + const char *unit = NULL; enum v4l2_ctrl_type type; s64 min; s64 max = _max; @@ -1782,7 +1788,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl, u64 step; u32 flags; - v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags); + v4l2_ctrl_fill(id, &name, &unit, &type, &min, &max, &step, &def, &flags); if (type == V4L2_CTRL_TYPE_MENU) qmenu = v4l2_ctrl_get_menu(id); @@ -1793,7 +1799,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl, handler_set_err(hdl, -EINVAL); return NULL; } - return v4l2_ctrl_new(hdl, ops, id, name, type, + return v4l2_ctrl_new(hdl, ops, id, name, unit, type, 0, max, mask, def, flags, qmenu, qmenu_int, NULL); } EXPORT_SYMBOL(v4l2_ctrl_new_std_menu); @@ -1805,6 +1811,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, { enum v4l2_ctrl_type type; const char *name; + const char *unit = NULL; u32 flags; u64 step; s64 min; @@ -1819,12 +1826,12 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, return NULL; } - v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags); + v4l2_ctrl_fill(id, &name, &unit, &type, &min, &max, &step, &def, &flags); if (type != V4L2_CTRL_TYPE_MENU || qmenu == NULL) { handler_set_err(hdl, -EINVAL); return NULL; } - return v4l2_ctrl_new(hdl, ops, id, name, type, 0, max, mask, def, + return v4l2_ctrl_new(hdl, ops, id, name, unit, type, 0, max, mask, def, flags, qmenu, NULL, NULL); } @@ -1836,6 +1843,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, u32 id, u8 _max, u8 _def, const s64 *qmenu_int) { const char *name; + const char *unit = NULL; enum v4l2_ctrl_type type; s64 min; u64 step; @@ -1843,12 +1851,12 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, s64 def = _def; u32 flags; - v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags); + v4l2_ctrl_fill(id, &name, &unit, &type, &min, &max, &step, &def, &flags); if (type != V4L2_CTRL_TYPE_INTEGER_MENU) { handler_set_err(hdl, -EINVAL); return NULL; } - return v4l2_ctrl_new(hdl, ops, id, name, type, + return v4l2_ctrl_new(hdl, ops, id, name, unit, type, 0, max, 0, def, flags, NULL, qmenu_int, NULL); } EXPORT_SYMBOL(v4l2_ctrl_new_int_menu); diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h index 0b347e8..3998049 100644 --- a/include/media/v4l2-ctrls.h +++ b/include/media/v4l2-ctrls.h @@ -85,6 +85,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv); * @ops: The control ops. * @id: The control ID. * @name: The control name. + * @unit: The control's unit. May be NULL. * @type: The control type. * @minimum: The control's minimum value. * @maximum: The control's maximum value. @@ -130,6 +131,7 @@ struct v4l2_ctrl { const struct v4l2_ctrl_ops *ops; u32 id; const char *name; + const char *unit; enum v4l2_ctrl_type type; s64 minimum, maximum, default_value; union { @@ -207,6 +209,7 @@ struct v4l2_ctrl_handler { * @ops: The control ops. * @id: The control ID. * @name: The control name. + * @unit: The control's unit. * @type: The control type. * @min: The control's minimum value. * @max: The control's maximum value. @@ -230,6 +233,7 @@ struct v4l2_ctrl_config { const struct v4l2_ctrl_ops *ops; u32 id; const char *name; + const char *unit; enum v4l2_ctrl_type type; s64 min; s64 max; @@ -249,15 +253,16 @@ struct v4l2_ctrl_config { * and @name will be NULL. * * This function will overwrite the contents of @name, @type and @flags. - * The contents of @min, @max, @step and @def may be modified depending on - * the type. + * The contents of @unit, @min, @max, @step and @def may be modified depending + * on the type. * * Do not use in drivers! It is used internally for backwards compatibility * control handling only. Once all drivers are converted to use the new * control framework this function will no longer be exported. */ -void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, - s64 *min, s64 *max, u64 *step, s64 *def, u32 *flags); +void v4l2_ctrl_fill(u32 id, const char **name, const char **unit, + enum v4l2_ctrl_type *type, s64 *min, s64 *max, + u64 *step, s64 *def, u32 *flags); /** v4l2_ctrl_handler_init_class() - Initialize the control handler.