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 32FDB49658 for ; Sun, 16 Jun 2024 23:21:01 +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=1718580064; cv=none; b=J20/mGPmur+AfTnxd7tNNVyPSwKfa/9gRQz+x8rO7kpChWHVtGFc7rAhMayxETGivnn5Hr2m0aWW33DdXKrNWVy4Fv/FLuJ+xB+kPzmIIWRLSBL9xTvjEptWomep2PCCFitYJf/Dmph0VDJ6D58AC3rd0Jhj00Kzz6M4doxXfQg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718580064; c=relaxed/simple; bh=JD3Pp6Wkg+Llnof5Fmpvb6JudwCKyIizWDZSOV4GR8I=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=ja30qigjznDAQSpRSi3YaYNUa6jKfQE3zBIb1b4RBHgxLN2M+jWa89ETTyCsFsPzZ6RaXmiHoA6pu1PSFvuBHJIwWDWc174NV72nbmiiPRYOApPtVdjEmCe5qm1+qQs6jaqBTSi8/C3GfOCofxcQi211G6Qxk/vbKtGbCoRbFOE= 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=IHE4Kisy; 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="IHE4Kisy" Received: from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi [81.175.209.231]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id E7FD42D5; Mon, 17 Jun 2024 01:20:43 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1718580044; bh=JD3Pp6Wkg+Llnof5Fmpvb6JudwCKyIizWDZSOV4GR8I=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=IHE4Kisy/ENmgWlbQh6GcKGjVweP5AHwl5Jf4qmOTkhrwfE83SFosNroJlFtCiX0r JCSX39hOKqKp9xtEm4df7RxpUVqXBha2DYOBvxhj2cNMxlp9e6Hf+DUVw6ZzX3tmFo oKdCgYwnGF/2FTUr7hsN958RfQm7erDRCkd8PB8E= Date: Mon, 17 Jun 2024 02:20:39 +0300 From: Laurent Pinchart To: Ricardo Ribalda Cc: Daniel Schaefer , linux-media@vger.kernel.org, Edgar Thier , Kieran Bingham , Mauro Carvalho Chehab , Kieran Levin Subject: Re: [PATCH] media: uvcvideo: Override default flags Message-ID: <20240616232039.GF4782@pendragon.ideasonboard.com> References: <20240602065053.36850-1-dhs@frame.work> Precedence: bulk X-Mailing-List: linux-media@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: On Mon, Jun 03, 2024 at 01:49:51PM +0200, Ricardo Ribalda wrote: > Hi Daniel > > Thanks for the patch. Some minor nits. > > Feel free to ignore it if you prefer your style. > > On Sun, 2 Jun 2024 at 08:52, Daniel Schaefer wrote: > > > > When the UVC device has a control that is readonly it doesn't set the > > SET_CUR flag. For example the privacy control has SET_CUR flag set in > > the defaults in the `uvc_ctrls` variable. Even if the device does not > > have it set, it's not cleared by uvc_ctrl_get_flags. > > > > Originally written with assignment in commit 859086ae3636 ("media: > > uvcvideo: Apply flags from device to actual properties"). But changed to > > |= in commit 0dc68cabdb62 ("media: uvcvideo: Prevent setting unavailable > > flags"). It would not clear the default flags. > > > > With this patch applied the correct flags are reported to user space. > > Tested with: > > > > ``` > > > v4l2-ctl --list-ctrls | grep privacy > > privacy 0x009a0910 (bool) : default=0 value=0 flags=read-only > > ``` > > > > Cc: Edgar Thier > > Cc: Kieran Bingham > > Cc: Mauro Carvalho Chehab > > Cc: Laurent Pinchart > > Cc: Kieran Levin > > Signed-off-by: Daniel Schaefer > Fixes: 0dc68cabdb62 ("media: uvcvideo: Prevent setting unavailable flags") > > Reviewed-by: Ricardo Ribalda > > --- > > drivers/media/usb/uvc/uvc_ctrl.c | 26 +++++++++++++++++--------- > > 1 file changed, 17 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > index 4b685f883e4d..f50542e26542 100644 > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > @@ -2031,15 +2031,23 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev, > > else > > ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, > > dev->intfnum, info->selector, data, 1); > > - if (!ret) > > - info->flags |= (data[0] & UVC_CONTROL_CAP_GET ? > > - UVC_CTRL_FLAG_GET_CUR : 0) > > - | (data[0] & UVC_CONTROL_CAP_SET ? > > - UVC_CTRL_FLAG_SET_CUR : 0) > > - | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ? > > - UVC_CTRL_FLAG_AUTO_UPDATE : 0) > > - | (data[0] & UVC_CONTROL_CAP_ASYNCHRONOUS ? > > - UVC_CTRL_FLAG_ASYNCHRONOUS : 0); > > + if (!ret) { > > + info->flags = (data[0] & UVC_CONTROL_CAP_GET) > > + ? (info->flags | UVC_CTRL_FLAG_GET_CUR) > > + : (info->flags & ~UVC_CTRL_FLAG_GET_CUR); > > + > > + info->flags = (data[0] & UVC_CONTROL_CAP_SET) > > + ? (info->flags | UVC_CTRL_FLAG_SET_CUR) > > + : (info->flags & ~UVC_CTRL_FLAG_SET_CUR); > > + > > + info->flags = (data[0] & UVC_CONTROL_CAP_AUTOUPDATE) > > + ? (info->flags | UVC_CTRL_FLAG_AUTO_UPDATE) > > + : (info->flags & ~UVC_CTRL_FLAG_AUTO_UPDATE); > > + > > + info->flags = (data[0] & UVC_CONTROL_CAP_ASYNCHRONOUS) > > + ? (info->flags | UVC_CTRL_FLAG_ASYNCHRONOUS) > > + : (info->flags & ~UVC_CTRL_FLAG_ASYNCHRONOUS); > > + } > > nit: I would have done it as > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index 4b685f883e4d..c453a67e1407 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -2031,7 +2031,12 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev, > else > ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, > dev->intfnum, info->selector, data, 1); > - if (!ret) > + if (!ret) { > + info->flags &= ~(UVC_CTRL_FLAG_GET_CUR | > + UVC_CTRL_FLAG_SET_CUR | > + UVC_CTRL_FLAG_AUTO_UPDATE | > + UVC_CTRL_FLAG_ASYNCHRONOUS); > + > info->flags |= (data[0] & UVC_CONTROL_CAP_GET ? > UVC_CTRL_FLAG_GET_CUR : 0) > | (data[0] & UVC_CONTROL_CAP_SET ? > @@ -2040,6 +2045,7 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev, > UVC_CTRL_FLAG_AUTO_UPDATE : 0) > | (data[0] & UVC_CONTROL_CAP_ASYNCHRONOUS ? > UVC_CTRL_FLAG_ASYNCHRONOUS : 0); > + } I prefer that slightly too. Reviewed-by: Laurent Pinchart I'll make the change in my tree, no need to send a v2. > > > > kfree(data); > > return ret; -- Regards, Laurent Pinchart