Received-SPF: pass (google.com: domain of linux-kernel+bounces-119168-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240326-uvc-relative-ptz-speed-fix-v1-1-453fd5ccfd37@securitylive.com>
In-Reply-To: <20240326-uvc-relative-ptz-speed-fix-v1-1-453fd5ccfd37@securitylive.com>
From: Ricardo Ribalda <ribalda@chromium.org>
Date: Tue, 26 Mar 2024 15:22:52 +0100
Message-ID: <CANiDSCtdgQhAyeLm89W-zNsMsNAVY0Ey5tZ4XpemzaZHXf8ZFQ@mail.gmail.com>
Subject: Re: [PATCH] media: uvcvideo: UVC minimum relative pan/tilt/zoom speed fix.
To: johnebgood@securitylive.com
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>, 
	Mauro Carvalho Chehab <mchehab@kernel.org>, linux-media@vger.kernel.org, 
	linux-kernel@vger.kernel.org, Gergo Koteles <soyer@irl.hu>, linh.tp.vu@gmail.com
Content-Type: text/plain; charset="UTF-8"

Hi John,

Thanks for your patch. If we confirm that there are no devices with
ranges [-A,B], with A!=B, then it looks like the way to go

I would have removed the

 return -data[first+1];

from uvc_ctrl_get_rel_speed(), as it becomes now dead code.

And I would have done it a little bit different with the switch, but I
have no idea what Laurent prefers ;):

@@ -1334,6 +1333,18 @@ static int __uvc_query_v4l2_ctrl(struct
uvc_video_chain *chain,
                v4l2_ctrl->step = mapping->get(mapping, UVC_GET_RES,
                                  uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES));

+       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 = -v4l2_ctrl->maximum;
+       }
+
        return 0;
 }

@@ -1919,6 +1930,18 @@ int uvc_ctrl_set(struct uvc_fh *handle,
                if (step == 0)
                        step = 1;

+               switch (xctrl->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.
+                        */
+                       min = -max;
+               }
+
                xctrl->value = min +
DIV_ROUND_CLOSEST((u32)(xctrl->value - min),
                                                        step) * step;
                if (mapping->data_type == UVC_CTRL_DATA_TYPE_SIGNED)


On Tue, 26 Mar 2024 at 10:02, 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>
> ---
> Gergo noticed that a quirk fix would not be needed and the just
> the minimum value needed to be set. Thanks Ricardo, Linh and Gergo
> for helping me along here. The reason the problem presented is that
> the minimum probe is done before the maximum however the way the
> driver has condensed direction and speed controls
> (with great simplification benefit to the user) the minimum
> value must be derived from the maximum and negated. This
> fix gets/checks/sets the correct relative minimum value when
> needed instead of of at probe time. This minimum value does not
> reflect the UVC 1.5 spec but the simplified condensed
> implementation of the driver; it's beautiful.
> ---
>  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)
>
> ---
> base-commit: e8f897f4afef0031fe618a8e94127a0934896aba
> change-id: 20240325-uvc-relative-ptz-speed-fix-2011aea68b5f
>
> Best regards,
> --
> John Bauer <johnebgood@securitylive.com>
>
>


-- 
Ricardo Ribalda