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

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

[Ricardo Ribalda Delgado]

This allows setting the default value on compound controls created via
v4l2_ctrl_new_custom.

Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
---
 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;
-- 
2.23.0

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

[Hans Verkuil]

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

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

[Ricardo Ribalda Delgado]

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.

Regards

On Wed, Oct 16, 2019 at 2:17 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> 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@kernel.org>
> > ---
> >  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

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

[Hans Verkuil]

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.

Regards,

	Hans

> 
> Regards
> 
> On Wed, Oct 16, 2019 at 2:17 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> 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@kernel.org>
>>> ---
>>>  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

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

[Ricardo Ribalda Delgado]

Hi Hans:

On Wed, Oct 16, 2019 at 2:32 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> 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,
      |

>
> Regards,
>
>         Hans
>
> >
> > Regards
> >
> > On Wed, Oct 16, 2019 at 2:17 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> 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@kernel.org>
> >>> ---
> >>>  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
>

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

[Hans Verkuil]

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

Regards,

	Hans

> 
>>
>> Regards,
>>
>>         Hans
>>
>>>
>>> Regards
>>>
>>> On Wed, Oct 16, 2019 at 2:17 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> 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@kernel.org>
>>>>> ---
>>>>>  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
>>

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

[Hans Verkuil]

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

Regards,

	Hans

> 
> Regards,
> 
> 	Hans
> 
>>
>>>
>>> Regards,
>>>
>>>         Hans
>>>
>>>>
>>>> Regards
>>>>
>>>> On Wed, Oct 16, 2019 at 2:17 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> 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@kernel.org>
>>>>>> ---
>>>>>>  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
>>>
> 

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

[Ricardo Ribalda Delgado]

Hi Hans

On Wed, Oct 16, 2019 at 2:43 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> 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@xs4all.nl> 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.
>

If we make p_def in ctrl_config const then this will fail:

drivers/qtec/qtec_sony.c:3273:18: error: assignment of read-only member ‘p_def’
 3273 |  area_ctrl.p_def = v4l2_ctrl_ptr_create((void *)&unit_size);


I think we need to leave it as in the proposed patch.

> Regards,
>
>         Hans
>
> >
> >>
> >> Regards,
> >>
> >>         Hans
> >>
> >>>
> >>> Regards
> >>>
> >>> On Wed, Oct 16, 2019 at 2:17 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> 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@kernel.org>
> >>>>> ---
> >>>>>  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
> >>
>

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

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

[Hans Verkuil]

On 10/16/19 3:12 PM, Ricardo Ribalda Delgado wrote:
> Hi Hans
> 
> On Wed, Oct 16, 2019 at 2:43 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> 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@xs4all.nl> 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.
>>
> 
> If we make p_def in ctrl_config const then this will fail:
> 
> drivers/qtec/qtec_sony.c:3273:18: error: assignment of read-only member ‘p_def’
>  3273 |  area_ctrl.p_def = v4l2_ctrl_ptr_create((void *)&unit_size);

area_ctrl should be const as well, I think. And then initialize this field in
the structure with .p_def.p_area = &unit_size,

Regards,

	Hans

> 
> 
> I think we need to leave it as in the proposed patch.
> 
>> Regards,
>>
>>         Hans
>>
>>>
>>>>
>>>> Regards,
>>>>
>>>>         Hans
>>>>
>>>>>
>>>>> Regards
>>>>>
>>>>> On Wed, Oct 16, 2019 at 2:17 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> 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@kernel.org>
>>>>>>> ---
>>>>>>>  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
>>>>
>>