On 9/20/19 12:33 PM, Ricardo Ribalda Delgado wrote: > Hello Hans > > Thanks for your review! I will implement the changes and resend, just > one comment. > > > On Fri, Sep 20, 2019 at 12:07 PM Hans Verkuil <hverkuil@xxxxxxxxx> wrote: >> >> On 9/17/19 6:21 PM, Ricardo Ribalda Delgado wrote: >>> Hi Hans >>> >>> Is this something close to what you were having in mind? Right now it >>> sits on https://github.com/ribalda/linux/commit/de21dbc2f57b58b22f5d73bc314dd8e59dff5c7d >>> but I will make it as the beginning of my patchset if you think that I >>> am on the right track. >>> >>> Thanks! >>> >>> On Tue, Sep 17, 2019 at 6:19 PM Ricardo Ribalda Delgado >>> <ribalda@xxxxxxxxxx> wrote: >>>> >>>> Implement v4l2_ctrl_new_std_compound. This is just a discussion patch, >>>> do not merge as is, and be gentle with the author ;P. >>>> >>>> Signed-off-by: Ricardo Ribalda Delgado <ribalda@xxxxxxxxxx> >>>> --- >>>> drivers/media/i2c/imx214.c | 13 +++-- >>>> drivers/media/v4l2-core/v4l2-ctrls.c | 79 +++++++++++++++++----------- >>>> include/media/v4l2-ctrls.h | 25 +++++++++ >>>> 3 files changed, 81 insertions(+), 36 deletions(-) >>>> >>>> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c >>>> index 625617d4c81a..e18fed9f31f1 100644 >>>> --- a/drivers/media/i2c/imx214.c >>>> +++ b/drivers/media/i2c/imx214.c >>>> @@ -953,6 +953,9 @@ static int imx214_probe(struct i2c_client *client) >>>> .width = 1120, >>>> .height = 1120, >>>> }; >>>> + union v4l2_ctrl_ptr p_def = { >>>> + .p_area = &unit_size, >>>> + }; >>>> int ret; >>>> >>>> ret = imx214_parse_fwnode(dev); >>>> @@ -1034,11 +1037,11 @@ static int imx214_probe(struct i2c_client *client) >>>> V4L2_CID_EXPOSURE, >>>> 0, 3184, 1, 0x0c70); >>>> >>>> - imx214->unit_size = v4l2_ctrl_new_area(&imx214->ctrls, >>>> - &imx214_ctrl_ops, >>>> - V4L2_CID_UNIT_CELL_SIZE, >>>> - &unit_size); >>>> - >>>> + imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls, >>>> + NULL, >>>> + V4L2_CID_UNIT_CELL_SIZE, >>>> + 0, 0x7ffffff, 1, 0, >>>> + p_def); >>>> ret = imx214->ctrls.error; >>>> if (ret) { >>>> dev_err(&client->dev, "%s control init failed (%d)\n", >>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c >>>> index 3d2fa1868982..04813ba2262b 100644 >>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c >>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c >>>> @@ -1555,6 +1555,10 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx, >>>> p_mpeg2_slice_params->picture.picture_coding_type = >>>> V4L2_MPEG2_PICTURE_CODING_TYPE_I; >>>> break; >>>> + default: >>>> + if (ctrl->has_p_def) >>>> + memcpy(p, ctrl->p_def.p, ctrl->elem_size); >>>> + break; >> >> It makes more sense to do this at the beginning of this function: >> >> if (ctrl->p_def.p) >> memcpy(p, ctrl->p_def.p, ctrl->elem_size); >> else >> memset(p, 0, ctrl->elem_size); >> >> and then enter the switch. >> >> This avoids calling memset followed by a memcpy. >> >>>> } >>>> } >>>> >>>> @@ -2369,7 +2373,8 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, >>>> s64 min, s64 max, u64 step, s64 def, >>>> const u32 dims[V4L2_CTRL_MAX_DIMS], u32 elem_size, >>>> u32 flags, const char * const *qmenu, >>>> - const s64 *qmenu_int, void *priv) >>>> + const s64 *qmenu_int, const union v4l2_ctrl_ptr p_def, >>>> + void *priv) >>>> { >>>> struct v4l2_ctrl *ctrl; >>>> unsigned sz_extra; >>>> @@ -2478,6 +2483,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, >>>> is_array) >>>> sz_extra += 2 * tot_ctrl_size; >>>> >>>> + if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p) >>>> + sz_extra += elem_size; >>>> + >>>> ctrl = kvzalloc(sizeof(*ctrl) + sz_extra, GFP_KERNEL); >>>> if (ctrl == NULL) { >>>> handler_set_err(hdl, -ENOMEM); >>>> @@ -2521,6 +2529,13 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, >>>> ctrl->p_new.p = &ctrl->val; >>>> ctrl->p_cur.p = &ctrl->cur.val; >>>> } >>>> + >>>> + if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p) { >>>> + ctrl->p_def.p = ctrl->p_cur.p + tot_ctrl_size; >>>> + memcpy(ctrl->p_def.p, p_def.p, elem_size); >>>> + ctrl->has_p_def = true; >> >> Is this needed? ctrl->p_def.p would be NULL if there is no p_def. >> >>>> + } >>>> + >>>> for (idx = 0; idx < elems; idx++) { >>>> ctrl->type_ops->init(ctrl, idx, ctrl->p_cur); >>>> ctrl->type_ops->init(ctrl, idx, ctrl->p_new); >>>> @@ -2550,6 +2565,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl, >>>> s64 max = cfg->max; >>>> u64 step = cfg->step; >>>> s64 def = cfg->def; >>>> + const union v4l2_ctrl_ptr p_def = {}; >> >> I think it is cleaner to have a 'static const union v4l2_ctrl_ptr ptr_null;' >> at the start of the source and just use that. >> >>>> >>>> if (name == NULL) >>>> v4l2_ctrl_fill(cfg->id, &name, &type, &min, &max, &step, >>>> @@ -2572,7 +2588,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl, >>>> type, min, max, >>>> is_menu ? cfg->menu_skip_mask : step, def, >>>> cfg->dims, cfg->elem_size, >>>> - flags, qmenu, qmenu_int, priv); >>>> + flags, qmenu, qmenu_int, p_def, priv); >>>> if (ctrl) >>>> ctrl->is_private = cfg->is_private; >>>> return ctrl; >>>> @@ -2587,6 +2603,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl, >>>> const char *name; >>>> enum v4l2_ctrl_type type; >>>> u32 flags; >>>> + const union v4l2_ctrl_ptr p_def = {}; >>>> >>>> v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags); >>>> if (type == V4L2_CTRL_TYPE_MENU || >>>> @@ -2597,7 +2614,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl, >>>> } >>>> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, >>>> min, max, step, def, NULL, 0, >>>> - flags, NULL, NULL, NULL); >>>> + flags, NULL, NULL, p_def, NULL); >>>> } >>>> EXPORT_SYMBOL(v4l2_ctrl_new_std); >>>> >>>> @@ -2616,6 +2633,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl, >>>> s64 def = _def; >>>> u64 step; >>>> u32 flags; >>>> + const union v4l2_ctrl_ptr p_def = {}; >>>> >>>> v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags); >>>> >>>> @@ -2630,7 +2648,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl, >>>> } >>>> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, >>>> 0, max, mask, def, NULL, 0, >>>> - flags, qmenu, qmenu_int, NULL); >>>> + flags, qmenu, qmenu_int, p_def, NULL); >>>> } >>>> EXPORT_SYMBOL(v4l2_ctrl_new_std_menu); >>>> >>>> @@ -2646,6 +2664,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, >>>> s64 min; >>>> s64 max = _max; >>>> s64 def = _def; >>>> + const union v4l2_ctrl_ptr p_def = {}; >>>> >>>> /* v4l2_ctrl_new_std_menu_items() should only be called for >>>> * standard controls without a standard menu. >>>> @@ -2662,11 +2681,33 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, >>>> } >>>> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, >>>> 0, max, mask, def, NULL, 0, >>>> - flags, qmenu, NULL, NULL); >>>> + flags, qmenu, NULL, p_def, NULL); >>>> >>>> } >>>> EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items); >>>> >>>> +/* Helper function for standard compound controls */ >>>> +struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl, >>>> + const struct v4l2_ctrl_ops *ops, >>>> + u32 id, s64 min, s64 max, u64 step, s64 def, >> >> The def arg makes no sense, since that's superseded by p_def. >> > > If def does not make sense, then for the same reason min, max and step > should not make any sense (because they are not a pointer but a > integer). > What about removing them completely from the function prototype and > add them later if we change our mind, it is a internal API. I've been going back and forth about whether or not min/max/step is useful for e.g. this AREA control as it would still help in validating a range for the width and height. But I think you are right, and it is better to just drop this and mark min/max/step/def as N/A in the documentation for these compound controls. We DO need basic validation for AREA (i.e. the width/height shall be > 0), but that's easy enough. Regards, Hans > >>>> + const union v4l2_ctrl_ptr p_def) >>>> +{ >>>> + const char *name; >>>> + enum v4l2_ctrl_type type; >>>> + u32 flags; >> >> Add: >> >> s64 def = min; >> >> It will be discarded anyway. >> >>>> + >>>> + v4l2_ctrl_fill(id, &name, &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; >>>> + } >> >> This makes no sense. It should just check that the type is a compound type and >> return an error if it isn't. >> >>>> + return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, >>>> + min, max, step, def, NULL, 0, >>>> + flags, NULL, NULL, p_def, NULL); >>>> +} >>>> +EXPORT_SYMBOL(v4l2_ctrl_new_std_compound); >>>> + >>>> /* Helper function for standard integer menu controls */ >>>> struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, >>>> const struct v4l2_ctrl_ops *ops, >>>> @@ -2679,6 +2720,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, >>>> s64 max = _max; >>>> s64 def = _def; >>>> u32 flags; >>>> + const union v4l2_ctrl_ptr p_def = {}; >>>> >>>> v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags); >>>> if (type != V4L2_CTRL_TYPE_INTEGER_MENU) { >>>> @@ -2687,7 +2729,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, >>>> } >>>> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, >>>> 0, max, 0, def, NULL, 0, >>>> - flags, NULL, qmenu_int, NULL); >>>> + flags, NULL, qmenu_int, p_def, NULL); >>>> } >>>> EXPORT_SYMBOL(v4l2_ctrl_new_int_menu); >>>> >>>> @@ -4030,31 +4072,6 @@ static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 ch_flags) >>>> return try_or_set_cluster(fh, master, true, ch_flags); >>>> } >>>> >>>> -/* Helper function for area controls */ >>>> -struct v4l2_ctrl *v4l2_ctrl_new_area(struct v4l2_ctrl_handler *hdl, >>>> - const struct v4l2_ctrl_ops *ops, >>>> - u32 id, const struct v4l2_area *area) >>>> -{ >>>> - static struct v4l2_ctrl_config ctrl = {}; >>>> - struct v4l2_ctrl *c; >>>> - int ret; >>>> - >>>> - ctrl.id = id; >>>> - c = v4l2_ctrl_new_custom(hdl, &ctrl, (void *)area); >>>> - if (!c) >>>> - return NULL; >>>> - >>>> - memcpy(c->p_new.p_area, area, sizeof(*area)); >>>> - ret = set_ctrl(NULL, c, 0); >>>> - if (ret){ >>>> - hdl->error = ret; >>>> - return NULL; >>>> - } >>>> - >>>> - return c; >>>> -} >>>> -EXPORT_SYMBOL(v4l2_ctrl_new_area); >>>> - >>>> /* Helper function for VIDIOC_S_CTRL compatibility */ >>>> static int set_ctrl_lock(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, >>>> struct v4l2_ext_control *c) >>>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h >>>> index b5b42777a756..8dc7e9827056 100644 >>>> --- a/include/media/v4l2-ctrls.h >>>> +++ b/include/media/v4l2-ctrls.h >>>> @@ -164,6 +164,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv); >>>> * manual mode. So if the value of the auto control equals this >>>> * value, then the whole cluster is in manual mode. Drivers should >>>> * never set this flag directly. >>>> + * @has_p_def: If set, the p_def field points to the default value of the control. >>>> * @ops: The control ops. >>>> * @type_ops: The control type ops. >>>> * @id: The control ID. >>>> @@ -230,6 +231,7 @@ struct v4l2_ctrl { >>>> unsigned int has_volatiles:1; >>>> unsigned int call_notify:1; >>>> unsigned int manual_mode_value:8; >>>> + unsigned int has_p_def:1; >>>> >>>> const struct v4l2_ctrl_ops *ops; >>>> const struct v4l2_ctrl_type_ops *type_ops; >>>> @@ -256,6 +258,7 @@ struct v4l2_ctrl { >>>> s32 val; >>>> } cur; >>>> >>>> + union v4l2_ctrl_ptr p_def; >>>> union v4l2_ctrl_ptr p_new; >>>> union v4l2_ctrl_ptr p_cur; >>>> }; >>>> @@ -647,6 +650,28 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, >>>> u32 id, u8 max, >>>> u64 mask, u8 def, >>>> const char * const *qmenu); >>>> +/** >>>> + * v4l2_ctrl_new_std_compound() - Allocate and initialize a new standard V4L2 >>>> + * compound control. >>>> + * >>>> + * @hdl: The control handler. >>>> + * @ops: The control ops. >>>> + * @id: The control ID. >>>> + * @min: The control's minimum value. >>>> + * @max: The control's maximum value. >>>> + * @step: The control's step value >>>> + * @def: The control's default value. >>>> + * @p_def: The control's p_def value. >>>> + * >>>> + * Sames as v4l2_ctrl_new_std(), but with support to compound controls, thanks to >>>> + * the @p_def field. >>>> + * >>>> + */ >>>> +struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl, >>>> + const struct v4l2_ctrl_ops *ops, >>>> + u32 id, s64 min, s64 max, u64 step, >>>> + s64 def, const union v4l2_ctrl_ptr p_def); >>>> + >>>> >>>> /** >>>> * v4l2_ctrl_new_int_menu() - Create a new standard V4L2 integer menu control. >>>> -- >>>> 2.23.0 >>>> >> >> So other than these fairly minor points, this is indeed what I was looking for. >> >> Regards, >> >> Hans
