From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id CA7A121480C; Mon, 3 Mar 2025 15:16:47 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=213.167.242.64 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741015009; cv=none; b=UGYZKkv7S+AOX9UACg4Sh6Ws1gpSJNKc6pzD2aHY/VLGHQJiH30zzKr9/+Oai6RLtzni/f8KOn2CXF2NqVAp6Mg/cGO0E5CZBj3lyQWh3822lvOZxkBBMpZsXL4meNtyeCPce7QcnTTyuiRk/MkYcvg4oevMYgmA4hnLdOCaJJM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741015009; c=relaxed/simple; bh=w8Gzg6G5ATgIFqx6VGug6piru5S1PG7aH5DSn3kk6r4=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=EJkWmiHdMZ5/adRBZYe2Q495njGljVPf5qdmi83vC0f9KRYQHGPB2Z1XZvM9Tjmrx8ZSgc9oZ2V4wHKLrZsqJuy4BASb08/Sha3esIe/KZ25yLBsFR786P+Yy9JgYMnlPS+wQ9z4Szs2ma0JoVrob7q4QoHoZxQK+VnBbQB0yfc= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ideasonboard.com; spf=pass smtp.mailfrom=ideasonboard.com; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b=TXmHJojT; arc=none smtp.client-ip=213.167.242.64 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ideasonboard.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=ideasonboard.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="TXmHJojT" Received: from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi [81.175.209.231]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id E82EF2D5; Mon, 3 Mar 2025 16:15:14 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1741014915; bh=w8Gzg6G5ATgIFqx6VGug6piru5S1PG7aH5DSn3kk6r4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=TXmHJojTpEXCWiFno0DYkQwRYZIbU3WFuXteOya/ro4iH3viPqdzU5uk2NST7244J +HA/DJKGoJJgXObYfX/0gSNSUYmno0vRjQZxa0j33i0S1prA/pQG9Xrasp82//zYiT WxVv2IcivblwlO/6NlRK3NxEbc745EBA24PaJ6AM= Date: Mon, 3 Mar 2025 17:16:25 +0200 From: Laurent Pinchart To: Ricardo Ribalda Cc: Hans Verkuil , Hans de Goede , Mauro Carvalho Chehab , linux-media@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v5] media: uvcvideo: Set V4L2_CTRL_FLAG_DISABLED during queryctrl errors Message-ID: <20250303151625.GE32048@pendragon.ideasonboard.com> References: <20250224-uvc-eaccess-v5-1-690d73bcef28@chromium.org> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20250224-uvc-eaccess-v5-1-690d73bcef28@chromium.org> On Mon, Feb 24, 2025 at 09:40:06AM +0000, Ricardo Ribalda wrote: > To implement VIDIOC_QUERYCTRL, we need to know the minimum, maximum, > step and flags of the control. For some of the controls, this involves > querying the actual hardware. > > Some non-compliant cameras produce errors when we query them. These > error can be triggered every time, sometimes, or when other controls do > not have the "right value". Right now, we populate that error to userspace. > When an error happens, the v4l2 framework does not copy the v4l2_queryctrl > struct to userspace. Also, userspace apps are not ready to handle any > other error than -EINVAL. > > One of the main usecases of VIDIOC_QUERYCTRL is enumerating the controls > of a device. This is done using the V4L2_CTRL_FLAG_NEXT_CTRL flag. In > that usecase, a non-compliant control will make it almost impossible to > enumerate all controls of the device. > > A control with an invalid max/min/step/flags is better than non being > able to enumerate the rest of the controls. > > This patch: > - Retries for an extra attempt to read the control, to avoid spurious > errors. More attempts do not seem to produce better results in the > tested hardware. > - Makes VIDIOC_QUERYCTRL return 0 in all the error cases different than > -EINVAL. > - Introduces a warning in dmesg so we can have a trace of what has happened > and sets the V4L2_CTRL_FLAG_DISABLED. > > Signed-off-by: Ricardo Ribalda > --- > Hi 2*Hans and Laurent! > > I came around a device that was listing just a couple of controls when > it should be listing much more. > > Some debugging latter I found that the device was returning -EIO when > all the focal controls were read. > > Lots of good arguments in favor/against this patch in the v1. Please > check! Let's continue the discussion on v4. > Without this patch: > $ v4l2-ctl --list-ctrls > auto_exposure 0x009a0901 (menu) : min=0 max=3 default=3 value=3 (Aperture Priority Mode) > exposure_time_absolute 0x009a0902 (int) : min=50 max=10000 step=1 default=166 value=166 flags=inactive > exposure_dynamic_framerate 0x009a0903 (bool) : default=0 value=0 > region_of_interest_auto_control 0x009a1902 (bitmask): max=0x00000001 default=0x00000001 value=1 > > With this patch: > $ v4l2-ctl --list-ctrls > auto_exposure 0x009a0901 (menu) : min=0 max=3 default=3 value=3 (Aperture Priority Mode) > exposure_time_absolute 0x009a0902 (int) : min=50 max=10000 step=1 default=166 value=166 flags=inactive > exposure_dynamic_framerate 0x009a0903 (bool) : default=0 value=0 > error 5 getting ext_ctrl Focus, Absolute > error 5 getting ext_ctrl Focus, Automatic Continuous > region_of_interest_rectangle 0x009a1901 (unknown): type=107 value=unsupported payload type flags=has-payload > region_of_interest_auto_control 0x009a1902 (bitmask): max=0x00000001 default=0x00000001 value=1 > -- > --- > Changes in v5: > - Explain the retry in the commit message (Thanks Laurent). > - Link to v4: https://lore.kernel.org/r/20250111-uvc-eaccess-v4-1-c7759bfd1bd4@chromium.org > > Changes in v4: > - Display control name (Thanks Hans) > - Link to v3: https://lore.kernel.org/r/20250107-uvc-eaccess-v3-1-99f3335d5133@chromium.org > > Changes in v3: > - Add a retry mechanism during error. > - Set V4L2_CTRL_FLAG_DISABLED flag. > - Link to v2: https://lore.kernel.org/r/20241219-uvc-eaccess-v2-1-bf6520c8b86d@chromium.org > > Changes in v2: > - Never return error, even if we are not enumerating the controls > - Improve commit message. > - Link to v1: https://lore.kernel.org/r/20241213-uvc-eaccess-v1-1-62e0b4fcc634@chromium.org > --- > drivers/media/usb/uvc/uvc_ctrl.c | 43 ++++++++++++++++++++++++++++++++-------- > 1 file changed, 35 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index 4e58476d305e..9d7812e8572d 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -1280,6 +1280,8 @@ static u32 uvc_get_ctrl_bitmap(struct uvc_control *ctrl, > return ~0; > } > > +#define MAX_QUERY_RETRIES 2 > + > static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > struct uvc_control *ctrl, > struct uvc_control_mapping *mapping, > @@ -1305,19 +1307,44 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > __uvc_find_control(ctrl->entity, mapping->master_id, > &master_map, &master_ctrl, 0); > if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) { > + unsigned int retries; > s32 val; > - int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val); > - if (ret < 0) > - return ret; > + int ret; > > - if (val != mapping->master_manual) > - v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE; > + for (retries = 0; retries < MAX_QUERY_RETRIES; retries++) { > + ret = __uvc_ctrl_get(chain, master_ctrl, master_map, > + &val); > + if (ret >= 0) > + break; > + } > + > + if (ret < 0) { > + dev_warn_ratelimited(&chain->dev->udev->dev, > + "UVC non compliance: Error %d querying master control %x (%s)\n", > + ret, master_map->id, > + uvc_map_get_name(master_map)); > + } else if (val != mapping->master_manual) { > + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE; > + } > } > > if (!ctrl->cached) { > - int ret = uvc_ctrl_populate_cache(chain, ctrl); > - if (ret < 0) > - return ret; > + unsigned int retries; > + int ret; > + > + for (retries = 0; retries < MAX_QUERY_RETRIES; retries++) { > + ret = uvc_ctrl_populate_cache(chain, ctrl); > + if (ret >= 0) > + break; > + } > + > + if (ret < 0) { > + dev_warn_ratelimited(&chain->dev->udev->dev, > + "UVC non compliance: Error %d populating cache of control %x (%s)\n", > + ret, mapping->id, > + uvc_map_get_name(mapping)); > + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_DISABLED; > + } > } > > if (ctrl->info.flags & UVC_CTRL_FLAG_GET_DEF) { > > --- > base-commit: c2b96a6818159fba8a3bcc38262da9e77f9b3ec7 > change-id: 20241213-uvc-eaccess-755cc061a360 > > Best regards, > -- > Ricardo Ribalda > -- Regards, Laurent Pinchart