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; ? Because if so, they wont pass the test: if (id >= V4L2_CID_PRIVATE_BASE) return fail("no V4L2_CID_PRIVATE_BASE allowed\n"); 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