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

["Xavier Roumegue (OSS)" <xavier.roumegue@oss.nxp.com>]

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);
>>>   
>>