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@xxxxxxxxx>
> >>>>
> >>>> Add a dynamic array test control to help test support for this
> >>>> feature.
> >>>>
> >>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> >>>> ---
> >>>> 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);
> >>>>
--
Regards,
Laurent Pinchart
