diff mbox series

[1/2] media: uvcvideo: UVC minimum relative pan/tilt/zoom speed fix

Message ID 20240328-uvc-fix-relative-ptz-speed-v1-1-17373eb8b2be@securitylive.com
State Superseded
Headers show
Series media: uvcvideo: UVC minimum relative pan/tilt/zoom speed fix | expand

Commit Message

John Bauer via B4 Relay March 28, 2024, 11:18 p.m. UTC 
From: John Bauer <johnebgood@securitylive.com>

The minimum UVC control value for the relative pan/tilt/zoom speeds
cannot be probed as the implementation condenses the pan and tilt
direction and speed into two 16 bit values. The minimum cannot be
set at probe time because it is probed first and the maximum is not
yet known. With this fix if a relative speed control is queried
or set the minimum is set and checked based on the additive inverse of
the maximum at that time.

Signed-off-by: John Bauer <johnebgood@securitylive.com>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 37 ++++++++++++++++++++++++++++++++++---
 1 file changed, 34 insertions(+), 3 deletions(-)

Comments

kernel test robot March 29, 2024, 11:59 p.m. UTC | #1 
Hi John,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 23956900041d968f9ad0f30db6dede4daccd7aa9]

url:    https://github.com/intel-lab-lkp/linux/commits/John-Bauer-via-B4-Relay/media-uvcvideo-UVC-minimum-relative-pan-tilt-zoom-speed-fix/20240329-071938
base:   23956900041d968f9ad0f30db6dede4daccd7aa9
patch link:    https://lore.kernel.org/r/20240328-uvc-fix-relative-ptz-speed-v1-1-17373eb8b2be%40securitylive.com
patch subject: [PATCH 1/2] media: uvcvideo: UVC minimum relative pan/tilt/zoom speed fix
config: arm-defconfig (https://download.01.org/0day-ci/archive/20240330/202403300745.omqy1CdY-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project.git f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240330/202403300745.omqy1CdY-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403300745.omqy1CdY-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/media/usb/uvc/uvc_ctrl.c:1944:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
                   default:
                   ^
   drivers/media/usb/uvc/uvc_ctrl.c:1944:3: note: insert 'break;' to avoid fall-through
                   default:
                   ^
                   break; 
   1 warning generated.


vim +1944 drivers/media/usb/uvc/uvc_ctrl.c

  1898	
  1899	int uvc_ctrl_set(struct uvc_fh *handle,
  1900		struct v4l2_ext_control *xctrl)
  1901	{
  1902		struct uvc_video_chain *chain = handle->chain;
  1903		struct uvc_control *ctrl;
  1904		struct uvc_control_mapping *mapping;
  1905		s32 value;
  1906		u32 step;
  1907		s32 min;
  1908		s32 max;
  1909		int ret;
  1910	
  1911		if (__uvc_query_v4l2_class(chain, xctrl->id, 0) >= 0)
  1912			return -EACCES;
  1913	
  1914		ctrl = uvc_find_control(chain, xctrl->id, &mapping);
  1915		if (ctrl == NULL)
  1916			return -EINVAL;
  1917		if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR))
  1918			return -EACCES;
  1919	
  1920		/* Clamp out of range values. */
  1921		switch (mapping->v4l2_type) {
  1922		case V4L2_CTRL_TYPE_INTEGER:
  1923			if (!ctrl->cached) {
  1924				ret = uvc_ctrl_populate_cache(chain, ctrl);
  1925				if (ret < 0)
  1926					return ret;
  1927			}
  1928	
  1929			min = mapping->get(mapping, UVC_GET_MIN,
  1930					   uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN));
  1931			max = mapping->get(mapping, UVC_GET_MAX,
  1932					   uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX));
  1933	
  1934			/*
  1935			 * For the relative speed implementation the minimum
  1936			 * value cannot be probed so it becomes the additive
  1937			 * inverse of maximum.
  1938			 */
  1939			switch (xctrl->id) {
  1940			case V4L2_CID_ZOOM_CONTINUOUS:
  1941			case V4L2_CID_PAN_SPEED:
  1942			case V4L2_CID_TILT_SPEED:
  1943				min = max * -1;
> 1944			default:
  1945				break;
  1946			}
  1947	
  1948			step = mapping->get(mapping, UVC_GET_RES,
  1949					    uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES));
  1950			if (step == 0)
  1951				step = 1;
  1952	
  1953			xctrl->value = min + DIV_ROUND_CLOSEST((u32)(xctrl->value - min),
  1954								step) * step;
  1955			if (mapping->data_type == UVC_CTRL_DATA_TYPE_SIGNED)
  1956				xctrl->value = clamp(xctrl->value, min, max);
  1957			else
  1958				xctrl->value = clamp_t(u32, xctrl->value, min, max);
  1959			value = xctrl->value;
  1960			break;
  1961	
  1962		case V4L2_CTRL_TYPE_BITMASK:
  1963			if (!ctrl->cached) {
  1964				ret = uvc_ctrl_populate_cache(chain, ctrl);
  1965				if (ret < 0)
  1966					return ret;
  1967			}
  1968	
  1969			xctrl->value &= uvc_get_ctrl_bitmap(ctrl, mapping);
  1970			value = xctrl->value;
  1971			break;
  1972	
  1973		case V4L2_CTRL_TYPE_BOOLEAN:
  1974			xctrl->value = clamp(xctrl->value, 0, 1);
  1975			value = xctrl->value;
  1976			break;
  1977	
  1978		case V4L2_CTRL_TYPE_MENU:
  1979			if (xctrl->value < (ffs(mapping->menu_mask) - 1) ||
  1980			    xctrl->value > (fls(mapping->menu_mask) - 1))
  1981				return -ERANGE;
  1982	
  1983			if (!test_bit(xctrl->value, &mapping->menu_mask))
  1984				return -EINVAL;
  1985	
  1986			value = uvc_mapping_get_menu_value(mapping, xctrl->value);
  1987	
  1988			/*
  1989			 * Valid menu indices are reported by the GET_RES request for
  1990			 * UVC controls that support it.
  1991			 */
  1992			if (mapping->data_type == UVC_CTRL_DATA_TYPE_BITMASK) {
  1993				if (!ctrl->cached) {
  1994					ret = uvc_ctrl_populate_cache(chain, ctrl);
  1995					if (ret < 0)
  1996						return ret;
  1997				}
  1998	
  1999				if (!(uvc_get_ctrl_bitmap(ctrl, mapping) & value))
  2000					return -EINVAL;
  2001			}
  2002	
  2003			break;
  2004	
  2005		default:
  2006			value = xctrl->value;
  2007			break;
  2008		}
  2009	
  2010		/*
  2011		 * If the mapping doesn't span the whole UVC control, the current value
  2012		 * needs to be loaded from the device to perform the read-modify-write
  2013		 * operation.
  2014		 */
  2015		if ((ctrl->info.size * 8) != mapping->size) {
  2016			ret = __uvc_ctrl_load_cur(chain, ctrl);
  2017			if (ret < 0)
  2018				return ret;
  2019		}
  2020	
  2021		/* Backup the current value in case we need to rollback later. */
  2022		if (!ctrl->dirty) {
  2023			memcpy(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_BACKUP),
  2024			       uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
  2025			       ctrl->info.size);
  2026		}
  2027	
  2028		mapping->set(mapping, value,
  2029			uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
  2030	
  2031		if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
  2032			ctrl->handle = handle;
  2033	
  2034		ctrl->dirty = 1;
  2035		ctrl->modified = 1;
  2036		return 0;
  2037	}
  2038
Ricardo Ribalda April 2, 2024, 11:53 a.m. UTC | #2 
Hi John

Maybe you could add a function like


static bool is_relative_ptz_ctr(__u32 id)
{
          switch (xctrl->id) {
           case V4L2_CID_ZOOM_CONTINUOUS:
           case V4L2_CID_PAN_SPEED:
            case V4L2_CID_TILT_SPEED:
                     return true;
          }
          return false;
}

to figure out if a control is relative or not, and avoid code duplication.



On Fri, 29 Mar 2024 at 00:18, John Bauer via B4 Relay
<devnull+johnebgood.securitylive.com@kernel.org> wrote:
>
> From: John Bauer <johnebgood@securitylive.com>
>
> The minimum UVC control value for the relative pan/tilt/zoom speeds
> cannot be probed as the implementation condenses the pan and tilt
> direction and speed into two 16 bit values. The minimum cannot be
> set at probe time because it is probed first and the maximum is not
> yet known. With this fix if a relative speed control is queried
> or set the minimum is set and checked based on the additive inverse of
> the maximum at that time.
>
> Signed-off-by: John Bauer <johnebgood@securitylive.com>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 37 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 34 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index e59a463c2761..b389ab3ee05d 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1322,9 +1322,25 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>                 break;
>         }
>
> -       if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MIN)
> -               v4l2_ctrl->minimum = mapping->get(mapping, UVC_GET_MIN,
> -                                    uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN));
> +       if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MIN) {
> +               switch (v4l2_ctrl->id) {
> +               case V4L2_CID_ZOOM_CONTINUOUS:
> +               case V4L2_CID_PAN_SPEED:
> +               case V4L2_CID_TILT_SPEED:
> +                       /*
> +                        * For the relative speed implementation the minimum
> +                        * value cannot be probed so it becomes the additive
> +                        * inverse of maximum.
> +                        */
> +                       v4l2_ctrl->minimum = -1 * mapping->get(mapping, UVC_GET_MAX,
> +                                               uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX));
> +                       break;
> +               default:
> +                       v4l2_ctrl->minimum = mapping->get(mapping, UVC_GET_MIN,
> +                                               uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN));
> +                       break;
> +               }
> +       }
>
>         if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MAX)
>                 v4l2_ctrl->maximum = mapping->get(mapping, UVC_GET_MAX,
> @@ -1914,6 +1930,21 @@ int uvc_ctrl_set(struct uvc_fh *handle,
>                                    uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN));
>                 max = mapping->get(mapping, UVC_GET_MAX,
>                                    uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX));
> +
> +               /*
> +                * For the relative speed implementation the minimum
> +                * value cannot be probed so it becomes the additive
> +                * inverse of maximum.
> +                */
> +               switch (xctrl->id) {
> +               case V4L2_CID_ZOOM_CONTINUOUS:
> +               case V4L2_CID_PAN_SPEED:
> +               case V4L2_CID_TILT_SPEED:
> +                       min = max * -1;
> +               default:
> +                       break;
> +               }
> +
>                 step = mapping->get(mapping, UVC_GET_RES,
>                                     uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES));
>                 if (step == 0)
>
> --
> 2.34.1
>
>
diff mbox series

Patch

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index e59a463c2761..b389ab3ee05d 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1322,9 +1322,25 @@  static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
 		break;
 	}
 
-	if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MIN)
-		v4l2_ctrl->minimum = mapping->get(mapping, UVC_GET_MIN,
-				     uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN));
+	if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MIN) {
+		switch (v4l2_ctrl->id) {
+		case V4L2_CID_ZOOM_CONTINUOUS:
+		case V4L2_CID_PAN_SPEED:
+		case V4L2_CID_TILT_SPEED:
+			/*
+			 * For the relative speed implementation the minimum
+			 * value cannot be probed so it becomes the additive
+			 * inverse of maximum.
+			 */
+			v4l2_ctrl->minimum = -1 * mapping->get(mapping, UVC_GET_MAX,
+						uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX));
+			break;
+		default:
+			v4l2_ctrl->minimum = mapping->get(mapping, UVC_GET_MIN,
+						uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN));
+			break;
+		}
+	}
 
 	if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MAX)
 		v4l2_ctrl->maximum = mapping->get(mapping, UVC_GET_MAX,
@@ -1914,6 +1930,21 @@  int uvc_ctrl_set(struct uvc_fh *handle,
 				   uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN));
 		max = mapping->get(mapping, UVC_GET_MAX,
 				   uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX));
+
+		/*
+		 * For the relative speed implementation the minimum
+		 * value cannot be probed so it becomes the additive
+		 * inverse of maximum.
+		 */
+		switch (xctrl->id) {
+		case V4L2_CID_ZOOM_CONTINUOUS:
+		case V4L2_CID_PAN_SPEED:
+		case V4L2_CID_TILT_SPEED:
+			min = max * -1;
+		default:
+			break;
+		}
+
 		step = mapping->get(mapping, UVC_GET_RES,
 				    uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES));
 		if (step == 0)