Re: [PATCH] media: v4l2-ctrl: Add p_def to v4l2_ctrl_config

[Ricardo Ribalda Delgado]

Hi Hans

On Wed, Oct 16, 2019 at 2:57 PM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote:
>
> On 10/16/19 2:43 PM, Hans Verkuil wrote:
> > On 10/16/19 2:39 PM, Ricardo Ribalda Delgado wrote:
> >> Hi Hans:
> >>
> >> On Wed, Oct 16, 2019 at 2:32 PM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote:
> >>>
> >>> On 10/16/19 2:20 PM, Ricardo Ribalda Delgado wrote:
> >>>> Hi Hans
> >>>>
> >>>> Not that awkward, the user has to use the brand new
> >>>> v4l2_ctrl_ptr_create() ;). But if you prefer void * I can make the
> >>>> change.
> >>>
> >>> Well, a struct v4l2_ctrl_config is typically a static const, so you can't use
> >>> v4l2_ctrl_ptr_create().
> >>>
> >>> Hmm, perhaps it is as easy as:
> >>>
> >>> static const struct v4l2_area def_area = {
> >>>         ...
> >>> };
> >>>
> >>> static const struct v4l2_ctrl_config ctrl = {
> >>>         ...
> >>>
> >>>         .p_def.p_area = &def_area,
> >>>         ...
> >>> };
> >>>
> >>> Can you do a quick compile check that I am not overlooking anything?
> >>>
> >>> If this works, then I'll take this patch.
> >>
> >> Testing with gcc 9.2.1
> >>
> >> This works fine, no warning/error:
> >>
> >> static struct v4l2_area unit_size = {
> >> .width = UNIT_SIZE,
> >> .height = UNIT_SIZE,
> >> };
> >> static struct v4l2_ctrl_config area_ctrl = {
> >> .type = V4L2_CTRL_TYPE_AREA,
> >> .flags = V4L2_CTRL_FLAG_READ_ONLY,
> >> .p_def.p_area = &unit_size,
> >> };
> >>
> >> but if unit_size is set as CONST:
> >> static const struct v4l2_area
> >>
> >> Then:
> >> drivers/qtec/qtec_sony.c: In function âqtec_sony_probeâ:
> >> drivers/qtec/qtec_sony.c:3151:19: warning: initialization discards
> >> âconstâ qualifier from pointer target type [-Wdiscarded-qualifiers]
> >>  3151 |   .p_def.p_area = &unit_size,
> >>       |
> >
> > Hmm. So we need a const void *p_def instead.
>
> Ah, v4l2_ctrl_ptr_create() expects a non-const pointer.
>
> What happens if union v4l2_ctrl_ptr p_def; in struct v4l2_ctrl changes
> to const union v4l2_ctrl_ptr p_def; ?
>
> You'll need to add const elsewhere as well, but since the default value
> is const, this might work.
>
> I'm not entirely sure this is correct code, though, but it's worth trying
> it.
>

This might be tricky. p_def needs to be allocated "in run time", and
we need to set p_def afterwards.
I am afraid we will get a  " assignment of read-only member" error

> Regards,
>
>         Hans
>
> >
> > Regards,
> >
> >       Hans
> >
> >>
> >>>
> >>> Regards,
> >>>
> >>>         Hans
> >>>
> >>>>
> >>>> Regards
> >>>>
> >>>> On Wed, Oct 16, 2019 at 2:17 PM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote:
> >>>>>
> >>>>> On 10/14/19 4:14 PM, Ricardo Ribalda Delgado wrote:
> >>>>>> This allows setting the default value on compound controls created via
> >>>>>> v4l2_ctrl_new_custom.
> >>>>>>
> >>>>>> Signed-off-by: Ricardo Ribalda Delgado <ribalda@xxxxxxxxxx>
> >>>>>> ---
> >>>>>>  drivers/media/v4l2-core/v4l2-ctrls.c | 2 +-
> >>>>>>  include/media/v4l2-ctrls.h           | 2 ++
> >>>>>>  2 files changed, 3 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> >>>>>> index bf50d37ef6c1..12cf38f73f7b 100644
> >>>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> >>>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> >>>>>> @@ -2583,7 +2583,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, ptr_null, priv);
> >>>>>> +                     flags, qmenu, qmenu_int, cfg->p_def, priv);
> >>>>>>       if (ctrl)
> >>>>>>               ctrl->is_private = cfg->is_private;
> >>>>>>       return ctrl;
> >>>>>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> >>>>>> index 26205ba3a0a0..2fca5b823961 100644
> >>>>>> --- a/include/media/v4l2-ctrls.h
> >>>>>> +++ b/include/media/v4l2-ctrls.h
> >>>>>> @@ -375,6 +375,7 @@ struct v4l2_ctrl_handler {
> >>>>>>   * @max:     The control's maximum value.
> >>>>>>   * @step:    The control's step value for non-menu controls.
> >>>>>>   * @def:     The control's default value.
> >>>>>> + * @p_def:   The control's default value for compound controls.
> >>>>>>   * @dims:    The size of each dimension.
> >>>>>>   * @elem_size:       The size in bytes of the control.
> >>>>>>   * @flags:   The control's flags.
> >>>>>> @@ -403,6 +404,7 @@ struct v4l2_ctrl_config {
> >>>>>>       s64 max;
> >>>>>>       u64 step;
> >>>>>>       s64 def;
> >>>>>> +     union v4l2_ctrl_ptr p_def;
> >>>>>>       u32 dims[V4L2_CTRL_MAX_DIMS];
> >>>>>>       u32 elem_size;
> >>>>>>       u32 flags;
> >>>>>>
> >>>>>
> >>>>> I'm not sure about this. It might be a bit awkward to initialize p_def given that it is a union.
> >>>>>
> >>>>> Perhaps a simple void pointer would be easier?
> >>>>>
> >>>>> Regards,
> >>>>>
> >>>>>         Hans
> >>>
> >
>