On Thu, 13 Oct 2022 at 11:54, Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote:
>
>
>
> On 10/13/22 11:52, Ricardo Ribalda wrote:
> > Hi Hans
> >
> >
> > On Thu, 13 Oct 2022 at 09:55, Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote:
> >>
> >> Hi Yunke,
> >>
> >> On 9/29/22 06:11, Yunke Cao wrote:
> >>> Removing priv_user_controls and its related checks.
> >>>
> >>> I suspect this is wrong because:
> >>>
> >>> 1. priv_user_controls == priv_user_controls_check is not always true.
> >>>
> >>> priv_user_controls counts the number of controls with
> >>> id >= V4L2_CID_PRIVATE_BASE (0x08000000).
> >>> priv_user_controls_check uses V4L2_CTRL_DRIVER_PRIV ((id) & 0xffff) >= 0x1000).
> >>>
> >>> The private controls defined in V4L2_CID_USER_BASE + 0x1000 will count towards
> >>> priv_user_controls_check, but not priv_user_controls. For example,
> >>> V4L2_CID_USER_MEYE_BASE (include/uapi/linux/v4l2-controls.h#n158).
> >>>
> >>> 2. Line 205 returns error for id >= V4L2_CID_PRIVATE_BASE. Counting
> >>> priv_user_controls will not happen.
> >>
> >> A long time ago all private controls in a driver started at ID V4L2_CID_PRIVATE_BASE.
> >> When the control framework was created, all private controls were changed to start
> >> at a control class base + 0x1000, and to stay compatible with old userspace the
> >> control framework emulated enumerating such controls from V4L2_CID_PRIVATE_BASE.
> >
> > The emulated controls are also enumerated with?
> >
> > qctrl.id = id | V4L2_CTRL_FLAG_NEXT_CTRL | V4L2_CTRL_FLAG_NEXT_COMPOUND; ?
>
> No, they are not.
>
> >
> > Because if so, they wont pass the test:
> >
> > if (id >= V4L2_CID_PRIVATE_BASE)
> > return fail("no V4L2_CID_PRIVATE_BASE allowed\n");
>
> Exactly: they should never be enumerated that way, if they appear, then the
> emulation is broken and this test fails.
Gotcha... I was not aware of that emulation.
So that means we have to add that emulation to uvc control framework :(
Thanks!
>
> Regards,
>
> Hans
>
> >
> > Thanks!
> >
> >
> >
> >>
> >> These compliance tests verify that that emulation is still working correctly.
> >>
> >> So this code is OK. If you have an example of where it fails, then that is likely
> >> to be a bug elsewhere. I would need more information to see what could be the cause
> >> in that case.
> >>
> >> For the record:
> >>
> >> Rejected-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> >>
> >> Regards,
> >>
> >> Hans
> >>
> >>>
> >>> Signed-off-by: Yunke Cao <yunkec@xxxxxxxxxxxx>
> >>> ---
> >>> ---
> >>> utils/v4l2-compliance/v4l2-test-controls.cpp | 22 +---------------------
> >>> 1 file changed, 1 insertion(+), 21 deletions(-)
> >>>
> >>> diff --git a/utils/v4l2-compliance/v4l2-test-controls.cpp b/utils/v4l2-compliance/v4l2-test-controls.cpp
> >>> index 999dbcd7..18c9f638 100644
> >>> --- a/utils/v4l2-compliance/v4l2-test-controls.cpp
> >>> +++ b/utils/v4l2-compliance/v4l2-test-controls.cpp
> >>> @@ -182,7 +182,6 @@ int testQueryExtControls(struct node *node)
> >>> __u32 which = 0;
> >>> bool found_ctrl_class = false;
> >>> unsigned user_controls = 0;
> >>> - unsigned priv_user_controls = 0;
> >>> unsigned user_controls_check = 0;
> >>> unsigned priv_user_controls_check = 0;
> >>> unsigned class_count = 0;
> >>> @@ -299,30 +298,11 @@ int testQueryExtControls(struct node *node)
> >>> user_controls++;
> >>> }
> >>>
> >>> - for (id = V4L2_CID_PRIVATE_BASE; ; id++) {
> >>> - memset(&qctrl, 0xff, sizeof(qctrl));
> >>> - qctrl.id = id;
> >>> - ret = doioctl(node, VIDIOC_QUERY_EXT_CTRL, &qctrl);
> >>> - if (ret && ret != EINVAL)
> >>> - return fail("invalid query_ext_ctrl return code (%d)\n", ret);
> >>> - if (ret)
> >>> - break;
> >>> - if (qctrl.id != id)
> >>> - return fail("qctrl.id (%08x) != id (%08x)\n",
> >>> - qctrl.id, id);
> >>> - if (checkQCtrl(node, qctrl))
> >>> - return fail("invalid control %08x\n", qctrl.id);
> >>> - priv_user_controls++;
> >>> - }
> >>> -
> >>> - if (priv_user_controls + user_controls && node->controls.empty())
> >>> + if (user_controls && node->controls.empty())
> >>> return fail("does not support V4L2_CTRL_FLAG_NEXT_CTRL\n");
> >>> if (user_controls != user_controls_check)
> >>> return fail("expected %d user controls, got %d\n",
> >>> user_controls_check, user_controls);
> >>> - if (priv_user_controls != priv_user_controls_check)
> >>> - return fail("expected %d private controls, got %d\n",
> >>> - priv_user_controls_check, priv_user_controls);
> >>> return result;
> >>> }
> >>>
> >>>
> >>> ---
> >>> base-commit: 7f560aede797b659b585f063ed1f143f58b03df5
> >>> change-id: 20220929-remove_private_control_check-ab8cc38a1b9e
> >>>
> >>> Best regards,
> >
> >
> >
--
Ricardo Ribalda
