Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
From: Hans Verkuil <hverkuil@xs4all.nl>
To: Ricardo Ribalda Delgado <ribalda@kernel.org>
Cc: linux-media <linux-media@vger.kernel.org>
Subject: Re: [PATCH] RFC: v4l2-ctrls: Inmplement v4l2_ctrl_new_std_compound()
Date: Fri, 20 Sep 2019 12:38:55 +0200
Message-ID: <e46616a9-c13d-ecc0-33bc-3fbb50c292e4@xs4all.nl> (raw)
In-Reply-To: <CAPybu_299E0ZG6B-kOj4vM15fhKLi-18Mm25b5c6w3_bSVy3Xg@mail.gmail.com>

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@xs4all.nl> 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@kernel.org> 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@kernel.org>
>>>> ---
>>>>  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


      reply index

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-12 13:35 Proposal for a v4l2_ctrl_new_std_compound() function Hans Verkuil
2019-09-13 16:08 ` kbuild test robot
2019-09-17 16:19 ` [PATCH] RFC: v4l2-ctrls: Inmplement v4l2_ctrl_new_std_compound() Ricardo Ribalda Delgado
2019-09-17 16:21   ` Ricardo Ribalda Delgado
2019-09-20 10:06     ` Hans Verkuil
2019-09-20 10:33       ` Ricardo Ribalda Delgado
2019-09-20 10:38         ` Hans Verkuil [this message]

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e46616a9-c13d-ecc0-33bc-3fbb50c292e4@xs4all.nl \
    --to=hverkuil@xs4all.nl \
    --cc=linux-media@vger.kernel.org \
    --cc=ribalda@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org linux-media@archiver.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/ public-inbox