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@xxxxxxxxx> 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.
>>
> 
> 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@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
>>>>
>>