diff mbox series

[v5,3/9] vivid: add dynamic array test control

Message ID 20220503093925.876640-4-xavier.roumegue@oss.nxp.com (mailing list archive)
State Superseded
Delegated to: Hans Verkuil
Headers
Series i.MX8MP DW100 dewarper driver |

Commit Message

Xavier Roumegue (OSS) May 3, 2022, 9:39 a.m. UTC 
  From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Add a dynamic array test control to help test support for this
feature.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/test-drivers/vivid/vivid-ctrls.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Laurent Pinchart June 14, 2022, 9 p.m. UTC | #1 
Hi Xavier and Hans,

Thank you for the patch.

On Tue, May 03, 2022 at 11:39:19AM +0200, Xavier Roumegue wrote:
> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> 
> Add a dynamic array test control to help test support for this
> feature.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/test-drivers/vivid/vivid-ctrls.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/media/test-drivers/vivid/vivid-ctrls.c b/drivers/media/test-drivers/vivid/vivid-ctrls.c
> index e7516dc1227b..7267892dc18a 100644
> --- a/drivers/media/test-drivers/vivid/vivid-ctrls.c
> +++ b/drivers/media/test-drivers/vivid/vivid-ctrls.c
> @@ -34,6 +34,7 @@
>  #define VIVID_CID_U8_4D_ARRAY		(VIVID_CID_CUSTOM_BASE + 10)
>  #define VIVID_CID_AREA			(VIVID_CID_CUSTOM_BASE + 11)
>  #define VIVID_CID_RO_INTEGER		(VIVID_CID_CUSTOM_BASE + 12)
> +#define VIVID_CID_U32_DYN_ARRAY		(VIVID_CID_CUSTOM_BASE + 13)
>  
>  #define VIVID_CID_VIVID_BASE		(0x00f00000 | 0xf000)
>  #define VIVID_CID_VIVID_CLASS		(0x00f00000 | 1)
> @@ -189,6 +190,19 @@ static const struct v4l2_ctrl_config vivid_ctrl_u32_array = {
>  	.dims = { 1 },
>  };
>  
> +static const struct v4l2_ctrl_config vivid_ctrl_u32_dyn_array = {
> +	.ops = &vivid_user_gen_ctrl_ops,
> +	.id = VIVID_CID_U32_DYN_ARRAY,
> +	.name = "U32 Dynamic Array",
> +	.type = V4L2_CTRL_TYPE_U32,
> +	.flags = V4L2_CTRL_FLAG_DYNAMIC_ARRAY,
> +	.def = 50,
> +	.min = 10,
> +	.max = 90,
> +	.step = 1,
> +	.dims = { 100 },
> +};

To meaningfully test this, don't we need the vivid driver to change the
dimension ? Or is it meant to only test changes made by the application
?

> +
>  static const struct v4l2_ctrl_config vivid_ctrl_u16_matrix = {
>  	.ops = &vivid_user_gen_ctrl_ops,
>  	.id = VIVID_CID_U16_MATRIX,
> @@ -1612,6 +1626,7 @@ int vivid_create_controls(struct vivid_dev *dev, bool show_ccs_cap,
>  	dev->ro_int32 = v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_ro_int32, NULL);
>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_area, NULL);
>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u32_array, NULL);
> +	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u32_dyn_array, NULL);
>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u16_matrix, NULL);
>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u8_4d_array, NULL);
>
Hans Verkuil June 15, 2022, 7:39 a.m. UTC | #2 
On 6/14/22 23:00, Laurent Pinchart wrote:
> Hi Xavier and Hans,
> 
> Thank you for the patch.
> 
> On Tue, May 03, 2022 at 11:39:19AM +0200, Xavier Roumegue wrote:
>> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>
>> Add a dynamic array test control to help test support for this
>> feature.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>>  drivers/media/test-drivers/vivid/vivid-ctrls.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/media/test-drivers/vivid/vivid-ctrls.c b/drivers/media/test-drivers/vivid/vivid-ctrls.c
>> index e7516dc1227b..7267892dc18a 100644
>> --- a/drivers/media/test-drivers/vivid/vivid-ctrls.c
>> +++ b/drivers/media/test-drivers/vivid/vivid-ctrls.c
>> @@ -34,6 +34,7 @@
>>  #define VIVID_CID_U8_4D_ARRAY		(VIVID_CID_CUSTOM_BASE + 10)
>>  #define VIVID_CID_AREA			(VIVID_CID_CUSTOM_BASE + 11)
>>  #define VIVID_CID_RO_INTEGER		(VIVID_CID_CUSTOM_BASE + 12)
>> +#define VIVID_CID_U32_DYN_ARRAY		(VIVID_CID_CUSTOM_BASE + 13)
>>  
>>  #define VIVID_CID_VIVID_BASE		(0x00f00000 | 0xf000)
>>  #define VIVID_CID_VIVID_CLASS		(0x00f00000 | 1)
>> @@ -189,6 +190,19 @@ static const struct v4l2_ctrl_config vivid_ctrl_u32_array = {
>>  	.dims = { 1 },
>>  };
>>  
>> +static const struct v4l2_ctrl_config vivid_ctrl_u32_dyn_array = {
>> +	.ops = &vivid_user_gen_ctrl_ops,
>> +	.id = VIVID_CID_U32_DYN_ARRAY,
>> +	.name = "U32 Dynamic Array",
>> +	.type = V4L2_CTRL_TYPE_U32,
>> +	.flags = V4L2_CTRL_FLAG_DYNAMIC_ARRAY,
>> +	.def = 50,
>> +	.min = 10,
>> +	.max = 90,
>> +	.step = 1,
>> +	.dims = { 100 },
>> +};
> 
> To meaningfully test this, don't we need the vivid driver to change the
> dimension ? Or is it meant to only test changes made by the application
> ?

dims[0] is the maximum number of elements allowed, it is not the actual
size of the control. The application sets the control to a size between
1 and dims[0] (100 in this case) elements.

When the control is created it will have just 1 element containing the
default control value.

Regards,

	Hans

> 
>> +
>>  static const struct v4l2_ctrl_config vivid_ctrl_u16_matrix = {
>>  	.ops = &vivid_user_gen_ctrl_ops,
>>  	.id = VIVID_CID_U16_MATRIX,
>> @@ -1612,6 +1626,7 @@ int vivid_create_controls(struct vivid_dev *dev, bool show_ccs_cap,
>>  	dev->ro_int32 = v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_ro_int32, NULL);
>>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_area, NULL);
>>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u32_array, NULL);
>> +	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u32_dyn_array, NULL);
>>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u16_matrix, NULL);
>>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u8_4d_array, NULL);
>>  
>
Hans Verkuil June 15, 2022, 7:46 a.m. UTC | #3 
On 6/15/22 09:39, Hans Verkuil wrote:
> 
> 
> On 6/14/22 23:00, Laurent Pinchart wrote:
>> Hi Xavier and Hans,
>>
>> Thank you for the patch.
>>
>> On Tue, May 03, 2022 at 11:39:19AM +0200, Xavier Roumegue wrote:
>>> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>>
>>> Add a dynamic array test control to help test support for this
>>> feature.
>>>
>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>> ---
>>>  drivers/media/test-drivers/vivid/vivid-ctrls.c | 15 +++++++++++++++
>>>  1 file changed, 15 insertions(+)
>>>
>>> diff --git a/drivers/media/test-drivers/vivid/vivid-ctrls.c b/drivers/media/test-drivers/vivid/vivid-ctrls.c
>>> index e7516dc1227b..7267892dc18a 100644
>>> --- a/drivers/media/test-drivers/vivid/vivid-ctrls.c
>>> +++ b/drivers/media/test-drivers/vivid/vivid-ctrls.c
>>> @@ -34,6 +34,7 @@
>>>  #define VIVID_CID_U8_4D_ARRAY		(VIVID_CID_CUSTOM_BASE + 10)
>>>  #define VIVID_CID_AREA			(VIVID_CID_CUSTOM_BASE + 11)
>>>  #define VIVID_CID_RO_INTEGER		(VIVID_CID_CUSTOM_BASE + 12)
>>> +#define VIVID_CID_U32_DYN_ARRAY		(VIVID_CID_CUSTOM_BASE + 13)
>>>  
>>>  #define VIVID_CID_VIVID_BASE		(0x00f00000 | 0xf000)
>>>  #define VIVID_CID_VIVID_CLASS		(0x00f00000 | 1)
>>> @@ -189,6 +190,19 @@ static const struct v4l2_ctrl_config vivid_ctrl_u32_array = {
>>>  	.dims = { 1 },
>>>  };
>>>  
>>> +static const struct v4l2_ctrl_config vivid_ctrl_u32_dyn_array = {
>>> +	.ops = &vivid_user_gen_ctrl_ops,
>>> +	.id = VIVID_CID_U32_DYN_ARRAY,
>>> +	.name = "U32 Dynamic Array",
>>> +	.type = V4L2_CTRL_TYPE_U32,
>>> +	.flags = V4L2_CTRL_FLAG_DYNAMIC_ARRAY,
>>> +	.def = 50,
>>> +	.min = 10,
>>> +	.max = 90,
>>> +	.step = 1,
>>> +	.dims = { 100 },
>>> +};
>>
>> To meaningfully test this, don't we need the vivid driver to change the
>> dimension ? Or is it meant to only test changes made by the application
>> ?
> 
> dims[0] is the maximum number of elements allowed, it is not the actual
> size of the control. The application sets the control to a size between
> 1 and dims[0] (100 in this case) elements.
> 
> When the control is created it will have just 1 element containing the
> default control value.

There is actually one thing that I would like to improve: allow dynamic arrays
to be empty (0 elements). Such a control should probably be empty in the
beginning as well.

I'll take a stab at this.

Regards,

	Hans

> 
> Regards,
> 
> 	Hans
> 
>>
>>> +
>>>  static const struct v4l2_ctrl_config vivid_ctrl_u16_matrix = {
>>>  	.ops = &vivid_user_gen_ctrl_ops,
>>>  	.id = VIVID_CID_U16_MATRIX,
>>> @@ -1612,6 +1626,7 @@ int vivid_create_controls(struct vivid_dev *dev, bool show_ccs_cap,
>>>  	dev->ro_int32 = v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_ro_int32, NULL);
>>>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_area, NULL);
>>>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u32_array, NULL);
>>> +	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u32_dyn_array, NULL);
>>>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u16_matrix, NULL);
>>>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u8_4d_array, NULL);
>>>  
>>
Hans Verkuil June 15, 2022, 9:14 a.m. UTC | #4 
Hi Laurent, Xavier,

Ignore what I wrote before, I read it with the HEVC patch series in mind, not the dw100
series.

So let me try again :-)

On 6/14/22 23:00, Laurent Pinchart wrote:
> Hi Xavier and Hans,
> 
> Thank you for the patch.
> 
> On Tue, May 03, 2022 at 11:39:19AM +0200, Xavier Roumegue wrote:
>> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>
>> Add a dynamic array test control to help test support for this
>> feature.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>>  drivers/media/test-drivers/vivid/vivid-ctrls.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/media/test-drivers/vivid/vivid-ctrls.c b/drivers/media/test-drivers/vivid/vivid-ctrls.c
>> index e7516dc1227b..7267892dc18a 100644
>> --- a/drivers/media/test-drivers/vivid/vivid-ctrls.c
>> +++ b/drivers/media/test-drivers/vivid/vivid-ctrls.c
>> @@ -34,6 +34,7 @@
>>  #define VIVID_CID_U8_4D_ARRAY		(VIVID_CID_CUSTOM_BASE + 10)
>>  #define VIVID_CID_AREA			(VIVID_CID_CUSTOM_BASE + 11)
>>  #define VIVID_CID_RO_INTEGER		(VIVID_CID_CUSTOM_BASE + 12)
>> +#define VIVID_CID_U32_DYN_ARRAY		(VIVID_CID_CUSTOM_BASE + 13)
>>  
>>  #define VIVID_CID_VIVID_BASE		(0x00f00000 | 0xf000)
>>  #define VIVID_CID_VIVID_CLASS		(0x00f00000 | 1)
>> @@ -189,6 +190,19 @@ static const struct v4l2_ctrl_config vivid_ctrl_u32_array = {
>>  	.dims = { 1 },
>>  };
>>  
>> +static const struct v4l2_ctrl_config vivid_ctrl_u32_dyn_array = {
>> +	.ops = &vivid_user_gen_ctrl_ops,
>> +	.id = VIVID_CID_U32_DYN_ARRAY,
>> +	.name = "U32 Dynamic Array",
>> +	.type = V4L2_CTRL_TYPE_U32,
>> +	.flags = V4L2_CTRL_FLAG_DYNAMIC_ARRAY,
>> +	.def = 50,
>> +	.min = 10,
>> +	.max = 90,
>> +	.step = 1,
>> +	.dims = { 100 },
>> +};
> 
> To meaningfully test this, don't we need the vivid driver to change the
> dimension ? Or is it meant to only test changes made by the application
> ?

As I understand it the dw100 driver needs a 2 dimensional array control.
The size is fixed for each resolution, but if the resolution changes, then
this control changes size as well, and it makes sense that when that happens
it is also reset to default values.

So this isn't a dynamic array at all. It is a standard 2 dimensional array.

What is missing in the control framework is a function similar to
v4l2_ctrl_modify_range() that can resize an array.

v4l2_ctrl_modify_dimensions() would be a good name.

I can make something for that if you both agree with this proposal.

Regards,

	Hans

> 
>> +
>>  static const struct v4l2_ctrl_config vivid_ctrl_u16_matrix = {
>>  	.ops = &vivid_user_gen_ctrl_ops,
>>  	.id = VIVID_CID_U16_MATRIX,
>> @@ -1612,6 +1626,7 @@ int vivid_create_controls(struct vivid_dev *dev, bool show_ccs_cap,
>>  	dev->ro_int32 = v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_ro_int32, NULL);
>>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_area, NULL);
>>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u32_array, NULL);
>> +	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u32_dyn_array, NULL);
>>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u16_matrix, NULL);
>>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u8_4d_array, NULL);
>>  
>
Laurent Pinchart June 15, 2022, 11:53 a.m. UTC | #5 
Hi Hans,

On Wed, Jun 15, 2022 at 11:14:43AM +0200, Hans Verkuil wrote:
> Hi Laurent, Xavier,
> 
> Ignore what I wrote before, I read it with the HEVC patch series in mind, not the dw100
> series.
> 
> So let me try again :-)
> 
> On 6/14/22 23:00, Laurent Pinchart wrote:
> > Hi Xavier and Hans,
> > 
> > Thank you for the patch.
> > 
> > On Tue, May 03, 2022 at 11:39:19AM +0200, Xavier Roumegue wrote:
> >> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >>
> >> Add a dynamic array test control to help test support for this
> >> feature.
> >>
> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >> ---
> >>  drivers/media/test-drivers/vivid/vivid-ctrls.c | 15 +++++++++++++++
> >>  1 file changed, 15 insertions(+)
> >>
> >> diff --git a/drivers/media/test-drivers/vivid/vivid-ctrls.c b/drivers/media/test-drivers/vivid/vivid-ctrls.c
> >> index e7516dc1227b..7267892dc18a 100644
> >> --- a/drivers/media/test-drivers/vivid/vivid-ctrls.c
> >> +++ b/drivers/media/test-drivers/vivid/vivid-ctrls.c
> >> @@ -34,6 +34,7 @@
> >>  #define VIVID_CID_U8_4D_ARRAY		(VIVID_CID_CUSTOM_BASE + 10)
> >>  #define VIVID_CID_AREA			(VIVID_CID_CUSTOM_BASE + 11)
> >>  #define VIVID_CID_RO_INTEGER		(VIVID_CID_CUSTOM_BASE + 12)
> >> +#define VIVID_CID_U32_DYN_ARRAY		(VIVID_CID_CUSTOM_BASE + 13)
> >>  
> >>  #define VIVID_CID_VIVID_BASE		(0x00f00000 | 0xf000)
> >>  #define VIVID_CID_VIVID_CLASS		(0x00f00000 | 1)
> >> @@ -189,6 +190,19 @@ static const struct v4l2_ctrl_config vivid_ctrl_u32_array = {
> >>  	.dims = { 1 },
> >>  };
> >>  
> >> +static const struct v4l2_ctrl_config vivid_ctrl_u32_dyn_array = {
> >> +	.ops = &vivid_user_gen_ctrl_ops,
> >> +	.id = VIVID_CID_U32_DYN_ARRAY,
> >> +	.name = "U32 Dynamic Array",
> >> +	.type = V4L2_CTRL_TYPE_U32,
> >> +	.flags = V4L2_CTRL_FLAG_DYNAMIC_ARRAY,
> >> +	.def = 50,
> >> +	.min = 10,
> >> +	.max = 90,
> >> +	.step = 1,
> >> +	.dims = { 100 },
> >> +};
> > 
> > To meaningfully test this, don't we need the vivid driver to change the
> > dimension ? Or is it meant to only test changes made by the application
> > ?
> 
> As I understand it the dw100 driver needs a 2 dimensional array control.
> The size is fixed for each resolution, but if the resolution changes, then
> this control changes size as well, and it makes sense that when that happens
> it is also reset to default values.
> 
> So this isn't a dynamic array at all. It is a standard 2 dimensional array.
> 
> What is missing in the control framework is a function similar to
> v4l2_ctrl_modify_range() that can resize an array.
> 
> v4l2_ctrl_modify_dimensions() would be a good name.
> 
> I can make something for that if you both agree with this proposal.

From a userspace point of view, we only need to be able to set the
control after setting the format. There's no need for control change
events (but I don't mind if they're there of course, even if I think
they won't be very usable in practice).

From an API point of view, I'd like a clear and documented behaviour for
what happens to the control value when the format is changed. It can be
a global behaviour, or a control-specific behaviour, I don't mind much.

> >> +
> >>  static const struct v4l2_ctrl_config vivid_ctrl_u16_matrix = {
> >>  	.ops = &vivid_user_gen_ctrl_ops,
> >>  	.id = VIVID_CID_U16_MATRIX,
> >> @@ -1612,6 +1626,7 @@ int vivid_create_controls(struct vivid_dev *dev, bool show_ccs_cap,
> >>  	dev->ro_int32 = v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_ro_int32, NULL);
> >>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_area, NULL);
> >>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u32_array, NULL);
> >> +	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u32_dyn_array, NULL);
> >>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u16_matrix, NULL);
> >>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u8_4d_array, NULL);
> >>
Hans Verkuil June 15, 2022, 12:27 p.m. UTC | #6 
On 6/15/22 13:53, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Wed, Jun 15, 2022 at 11:14:43AM +0200, Hans Verkuil wrote:
>> Hi Laurent, Xavier,
>>
>> Ignore what I wrote before, I read it with the HEVC patch series in mind, not the dw100
>> series.
>>
>> So let me try again :-)
>>
>> On 6/14/22 23:00, Laurent Pinchart wrote:
>>> Hi Xavier and Hans,
>>>
>>> Thank you for the patch.
>>>
>>> On Tue, May 03, 2022 at 11:39:19AM +0200, Xavier Roumegue wrote:
>>>> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>>>
>>>> Add a dynamic array test control to help test support for this
>>>> feature.
>>>>
>>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>>> ---
>>>>  drivers/media/test-drivers/vivid/vivid-ctrls.c | 15 +++++++++++++++
>>>>  1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/drivers/media/test-drivers/vivid/vivid-ctrls.c b/drivers/media/test-drivers/vivid/vivid-ctrls.c
>>>> index e7516dc1227b..7267892dc18a 100644
>>>> --- a/drivers/media/test-drivers/vivid/vivid-ctrls.c
>>>> +++ b/drivers/media/test-drivers/vivid/vivid-ctrls.c
>>>> @@ -34,6 +34,7 @@
>>>>  #define VIVID_CID_U8_4D_ARRAY		(VIVID_CID_CUSTOM_BASE + 10)
>>>>  #define VIVID_CID_AREA			(VIVID_CID_CUSTOM_BASE + 11)
>>>>  #define VIVID_CID_RO_INTEGER		(VIVID_CID_CUSTOM_BASE + 12)
>>>> +#define VIVID_CID_U32_DYN_ARRAY		(VIVID_CID_CUSTOM_BASE + 13)
>>>>  
>>>>  #define VIVID_CID_VIVID_BASE		(0x00f00000 | 0xf000)
>>>>  #define VIVID_CID_VIVID_CLASS		(0x00f00000 | 1)
>>>> @@ -189,6 +190,19 @@ static const struct v4l2_ctrl_config vivid_ctrl_u32_array = {
>>>>  	.dims = { 1 },
>>>>  };
>>>>  
>>>> +static const struct v4l2_ctrl_config vivid_ctrl_u32_dyn_array = {
>>>> +	.ops = &vivid_user_gen_ctrl_ops,
>>>> +	.id = VIVID_CID_U32_DYN_ARRAY,
>>>> +	.name = "U32 Dynamic Array",
>>>> +	.type = V4L2_CTRL_TYPE_U32,
>>>> +	.flags = V4L2_CTRL_FLAG_DYNAMIC_ARRAY,
>>>> +	.def = 50,
>>>> +	.min = 10,
>>>> +	.max = 90,
>>>> +	.step = 1,
>>>> +	.dims = { 100 },
>>>> +};
>>>
>>> To meaningfully test this, don't we need the vivid driver to change the
>>> dimension ? Or is it meant to only test changes made by the application
>>> ?
>>
>> As I understand it the dw100 driver needs a 2 dimensional array control.
>> The size is fixed for each resolution, but if the resolution changes, then
>> this control changes size as well, and it makes sense that when that happens
>> it is also reset to default values.
>>
>> So this isn't a dynamic array at all. It is a standard 2 dimensional array.
>>
>> What is missing in the control framework is a function similar to
>> v4l2_ctrl_modify_range() that can resize an array.
>>
>> v4l2_ctrl_modify_dimensions() would be a good name.
>>
>> I can make something for that if you both agree with this proposal.
> 
> From a userspace point of view, we only need to be able to set the
> control after setting the format. There's no need for control change
> events (but I don't mind if they're there of course, even if I think
> they won't be very usable in practice).
> 
> From an API point of view, I'd like a clear and documented behaviour for
> what happens to the control value when the format is changed. It can be
> a global behaviour, or a control-specific behaviour, I don't mind much.

I'd say it is control specific. In this case you would reset the contents
to default values (presumably values that don't do any warping in this
case).

Regards,

	Hans

> 
>>>> +
>>>>  static const struct v4l2_ctrl_config vivid_ctrl_u16_matrix = {
>>>>  	.ops = &vivid_user_gen_ctrl_ops,
>>>>  	.id = VIVID_CID_U16_MATRIX,
>>>> @@ -1612,6 +1626,7 @@ int vivid_create_controls(struct vivid_dev *dev, bool show_ccs_cap,
>>>>  	dev->ro_int32 = v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_ro_int32, NULL);
>>>>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_area, NULL);
>>>>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u32_array, NULL);
>>>> +	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u32_dyn_array, NULL);
>>>>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u16_matrix, NULL);
>>>>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u8_4d_array, NULL);
>>>>  
>
Laurent Pinchart June 15, 2022, 12:39 p.m. UTC | #7 
Hi Hans,

On Wed, Jun 15, 2022 at 02:27:57PM +0200, Hans Verkuil wrote:
> On 6/15/22 13:53, Laurent Pinchart wrote:
> > On Wed, Jun 15, 2022 at 11:14:43AM +0200, Hans Verkuil wrote:
> >> Hi Laurent, Xavier,
> >>
> >> Ignore what I wrote before, I read it with the HEVC patch series in mind, not the dw100
> >> series.
> >>
> >> So let me try again :-)
> >>
> >> On 6/14/22 23:00, Laurent Pinchart wrote:
> >>> Hi Xavier and Hans,
> >>>
> >>> Thank you for the patch.
> >>>
> >>> On Tue, May 03, 2022 at 11:39:19AM +0200, Xavier Roumegue wrote:
> >>>> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >>>>
> >>>> Add a dynamic array test control to help test support for this
> >>>> feature.
> >>>>
> >>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >>>> ---
> >>>>  drivers/media/test-drivers/vivid/vivid-ctrls.c | 15 +++++++++++++++
> >>>>  1 file changed, 15 insertions(+)
> >>>>
> >>>> diff --git a/drivers/media/test-drivers/vivid/vivid-ctrls.c b/drivers/media/test-drivers/vivid/vivid-ctrls.c
> >>>> index e7516dc1227b..7267892dc18a 100644
> >>>> --- a/drivers/media/test-drivers/vivid/vivid-ctrls.c
> >>>> +++ b/drivers/media/test-drivers/vivid/vivid-ctrls.c
> >>>> @@ -34,6 +34,7 @@
> >>>>  #define VIVID_CID_U8_4D_ARRAY		(VIVID_CID_CUSTOM_BASE + 10)
> >>>>  #define VIVID_CID_AREA			(VIVID_CID_CUSTOM_BASE + 11)
> >>>>  #define VIVID_CID_RO_INTEGER		(VIVID_CID_CUSTOM_BASE + 12)
> >>>> +#define VIVID_CID_U32_DYN_ARRAY		(VIVID_CID_CUSTOM_BASE + 13)
> >>>>  
> >>>>  #define VIVID_CID_VIVID_BASE		(0x00f00000 | 0xf000)
> >>>>  #define VIVID_CID_VIVID_CLASS		(0x00f00000 | 1)
> >>>> @@ -189,6 +190,19 @@ static const struct v4l2_ctrl_config vivid_ctrl_u32_array = {
> >>>>  	.dims = { 1 },
> >>>>  };
> >>>>  
> >>>> +static const struct v4l2_ctrl_config vivid_ctrl_u32_dyn_array = {
> >>>> +	.ops = &vivid_user_gen_ctrl_ops,
> >>>> +	.id = VIVID_CID_U32_DYN_ARRAY,
> >>>> +	.name = "U32 Dynamic Array",
> >>>> +	.type = V4L2_CTRL_TYPE_U32,
> >>>> +	.flags = V4L2_CTRL_FLAG_DYNAMIC_ARRAY,
> >>>> +	.def = 50,
> >>>> +	.min = 10,
> >>>> +	.max = 90,
> >>>> +	.step = 1,
> >>>> +	.dims = { 100 },
> >>>> +};
> >>>
> >>> To meaningfully test this, don't we need the vivid driver to change the
> >>> dimension ? Or is it meant to only test changes made by the application
> >>> ?
> >>
> >> As I understand it the dw100 driver needs a 2 dimensional array control.
> >> The size is fixed for each resolution, but if the resolution changes, then
> >> this control changes size as well, and it makes sense that when that happens
> >> it is also reset to default values.
> >>
> >> So this isn't a dynamic array at all. It is a standard 2 dimensional array.
> >>
> >> What is missing in the control framework is a function similar to
> >> v4l2_ctrl_modify_range() that can resize an array.
> >>
> >> v4l2_ctrl_modify_dimensions() would be a good name.
> >>
> >> I can make something for that if you both agree with this proposal.
> > 
> > From a userspace point of view, we only need to be able to set the
> > control after setting the format. There's no need for control change
> > events (but I don't mind if they're there of course, even if I think
> > they won't be very usable in practice).
> > 
> > From an API point of view, I'd like a clear and documented behaviour for
> > what happens to the control value when the format is changed. It can be
> > a global behaviour, or a control-specific behaviour, I don't mind much.
> 
> I'd say it is control specific. In this case you would reset the contents
> to default values (presumably values that don't do any warping in this
> case).

Works for me. Let's then note in the core API that any control whose
dimensions can change need to document how its value is affected.

> >>>> +
> >>>>  static const struct v4l2_ctrl_config vivid_ctrl_u16_matrix = {
> >>>>  	.ops = &vivid_user_gen_ctrl_ops,
> >>>>  	.id = VIVID_CID_U16_MATRIX,
> >>>> @@ -1612,6 +1626,7 @@ int vivid_create_controls(struct vivid_dev *dev, bool show_ccs_cap,
> >>>>  	dev->ro_int32 = v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_ro_int32, NULL);
> >>>>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_area, NULL);
> >>>>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u32_array, NULL);
> >>>> +	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u32_dyn_array, NULL);
> >>>>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u16_matrix, NULL);
> >>>>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u8_4d_array, NULL);
> >>>>
Xavier Roumegue (OSS) June 16, 2022, 6:08 a.m. UTC | #8 
Hi Hans,

On 6/15/22 11:14, Hans Verkuil wrote:
> Hi Laurent, Xavier,
> 
> Ignore what I wrote before, I read it with the HEVC patch series in mind, not the dw100
> series.
> 
> So let me try again :-)
> 
> On 6/14/22 23:00, Laurent Pinchart wrote:
>> Hi Xavier and Hans,
>>
>> Thank you for the patch.
>>
>> On Tue, May 03, 2022 at 11:39:19AM +0200, Xavier Roumegue wrote:
>>> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>>
>>> Add a dynamic array test control to help test support for this
>>> feature.
>>>
>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>> ---
>>>   drivers/media/test-drivers/vivid/vivid-ctrls.c | 15 +++++++++++++++
>>>   1 file changed, 15 insertions(+)
>>>
>>> diff --git a/drivers/media/test-drivers/vivid/vivid-ctrls.c b/drivers/media/test-drivers/vivid/vivid-ctrls.c
>>> index e7516dc1227b..7267892dc18a 100644
>>> --- a/drivers/media/test-drivers/vivid/vivid-ctrls.c
>>> +++ b/drivers/media/test-drivers/vivid/vivid-ctrls.c
>>> @@ -34,6 +34,7 @@
>>>   #define VIVID_CID_U8_4D_ARRAY		(VIVID_CID_CUSTOM_BASE + 10)
>>>   #define VIVID_CID_AREA			(VIVID_CID_CUSTOM_BASE + 11)
>>>   #define VIVID_CID_RO_INTEGER		(VIVID_CID_CUSTOM_BASE + 12)
>>> +#define VIVID_CID_U32_DYN_ARRAY		(VIVID_CID_CUSTOM_BASE + 13)
>>>   
>>>   #define VIVID_CID_VIVID_BASE		(0x00f00000 | 0xf000)
>>>   #define VIVID_CID_VIVID_CLASS		(0x00f00000 | 1)
>>> @@ -189,6 +190,19 @@ static const struct v4l2_ctrl_config vivid_ctrl_u32_array = {
>>>   	.dims = { 1 },
>>>   };
>>>   
>>> +static const struct v4l2_ctrl_config vivid_ctrl_u32_dyn_array = {
>>> +	.ops = &vivid_user_gen_ctrl_ops,
>>> +	.id = VIVID_CID_U32_DYN_ARRAY,
>>> +	.name = "U32 Dynamic Array",
>>> +	.type = V4L2_CTRL_TYPE_U32,
>>> +	.flags = V4L2_CTRL_FLAG_DYNAMIC_ARRAY,
>>> +	.def = 50,
>>> +	.min = 10,
>>> +	.max = 90,
>>> +	.step = 1,
>>> +	.dims = { 100 },
>>> +};
>>
>> To meaningfully test this, don't we need the vivid driver to change the
>> dimension ? Or is it meant to only test changes made by the application
>> ?
> 
> As I understand it the dw100 driver needs a 2 dimensional array control.
Considering the semantic of the array control, indeed, this is 2 
dimensional array control. Nevertheless, the driver does not need to 
access individual array items, as the hardware is the only consumer.
Hence, the driver rather considers the array control as a binary blob 
with varying size.
 From this perspective, the dynamic array control was a good candidate.

> The size is fixed for each resolution, but if the resolution changes, then
> this control changes size as well, and it makes sense that when that happens
> it is also reset to default values.
> 
> So this isn't a dynamic array at all. It is a standard 2 dimensional array.
> 
> What is missing in the control framework is a function similar to
> v4l2_ctrl_modify_range() that can resize an array.
> 
> v4l2_ctrl_modify_dimensions() would be a goo
> I can make something for that if you both agree with this proposal.
I am ok with that. Thanks !
Regards,
  Xavier
> 
> Regards,
> 
> 	Hans
> 
>>
>>> +
>>>   static const struct v4l2_ctrl_config vivid_ctrl_u16_matrix = {
>>>   	.ops = &vivid_user_gen_ctrl_ops,
>>>   	.id = VIVID_CID_U16_MATRIX,
>>> @@ -1612,6 +1626,7 @@ int vivid_create_controls(struct vivid_dev *dev, bool show_ccs_cap,
>>>   	dev->ro_int32 = v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_ro_int32, NULL);
>>>   	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_area, NULL);
>>>   	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u32_array, NULL);
>>> +	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u32_dyn_array, NULL);
>>>   	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u16_matrix, NULL);
>>>   	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u8_4d_array, NULL);
>>>   
>>
diff mbox series

Patch

diff --git a/drivers/media/test-drivers/vivid/vivid-ctrls.c b/drivers/media/test-drivers/vivid/vivid-ctrls.c
index e7516dc1227b..7267892dc18a 100644
--- a/drivers/media/test-drivers/vivid/vivid-ctrls.c
+++ b/drivers/media/test-drivers/vivid/vivid-ctrls.c
@@ -34,6 +34,7 @@ 
 #define VIVID_CID_U8_4D_ARRAY		(VIVID_CID_CUSTOM_BASE + 10)
 #define VIVID_CID_AREA			(VIVID_CID_CUSTOM_BASE + 11)
 #define VIVID_CID_RO_INTEGER		(VIVID_CID_CUSTOM_BASE + 12)
+#define VIVID_CID_U32_DYN_ARRAY		(VIVID_CID_CUSTOM_BASE + 13)
 
 #define VIVID_CID_VIVID_BASE		(0x00f00000 | 0xf000)
 #define VIVID_CID_VIVID_CLASS		(0x00f00000 | 1)
@@ -189,6 +190,19 @@  static const struct v4l2_ctrl_config vivid_ctrl_u32_array = {
 	.dims = { 1 },
 };
 
+static const struct v4l2_ctrl_config vivid_ctrl_u32_dyn_array = {
+	.ops = &vivid_user_gen_ctrl_ops,
+	.id = VIVID_CID_U32_DYN_ARRAY,
+	.name = "U32 Dynamic Array",
+	.type = V4L2_CTRL_TYPE_U32,
+	.flags = V4L2_CTRL_FLAG_DYNAMIC_ARRAY,
+	.def = 50,
+	.min = 10,
+	.max = 90,
+	.step = 1,
+	.dims = { 100 },
+};
+
 static const struct v4l2_ctrl_config vivid_ctrl_u16_matrix = {
 	.ops = &vivid_user_gen_ctrl_ops,
 	.id = VIVID_CID_U16_MATRIX,
@@ -1612,6 +1626,7 @@  int vivid_create_controls(struct vivid_dev *dev, bool show_ccs_cap,
 	dev->ro_int32 = v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_ro_int32, NULL);
 	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_area, NULL);
 	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u32_array, NULL);
+	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u32_dyn_array, NULL);
 	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u16_matrix, NULL);
 	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u8_4d_array, NULL);