Received-SPF: pass (google.com: domain of linux-kernel+bounces-208949-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240318-billion-v1-0-2f7bc0ee2030@chromium.org>
 <20240318-billion-v1-3-2f7bc0ee2030@chromium.org> <20240610141402.GB13744@pendragon.ideasonboard.com>
In-Reply-To: <20240610141402.GB13744@pendragon.ideasonboard.com>
From: Ricardo Ribalda <ribalda@chromium.org>
Date: Mon, 10 Jun 2024 23:51:55 +0200
Message-ID: <CANiDSCsNdZyKMM82q9SqZ-hQH2Pch24rVCqSs=L6mFL=RbJqQg@mail.gmail.com>
Subject: Re: [PATCH 3/5] media: uvcvideo: Probe the PLF characteristics
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>, Sergey Senozhatsky <senozhatsky@chromium.org>, 
	linux-media@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

Hi Laurent

On Mon, 10 Jun 2024 at 16:14, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Mon, Mar 18, 2024 at 11:55:25PM +0000, Ricardo Ribalda wrote:
> > The UVC 1.5 standard defines 4 values for the PLF control: Off, 50Hz,
> > 60Hz and Auto. But it does not clearly define if all the values must be
> > implemented or not.
> >
> > Instead of just using the UVC version to determine what the PLF control
> > can do, probe it.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_ctrl.c | 54 +++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 50 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 67522143c6c85..9a0b81aca30d1 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -501,12 +501,58 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
> >  static int uvc_ctrl_add_plf_mapping(struct uvc_video_chain *chain,
> >       struct uvc_control *ctrl, const struct uvc_control_mapping *mapping)
> >  {
> > +     const struct uvc_control_mapping *out_mapping =
> > +                                     &uvc_ctrl_power_line_mapping_uvc11;
> > +     u8 init_val;
> > +     u8 *buf;
>
>         u8 *buf __free(kfree) = NULL;
>
> will simplify the exit paths.
>
> > +     int ret;
> > +
> > +     buf = kmalloc(sizeof(*buf), GFP_KERNEL);
> > +     if (!buf)
> > +             return -ENOMEM;
> > +
> > +     /* Save the default PLF value, so we can restore it. */
> > +     ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, ctrl->entity->id,
> > +                          chain->dev->intfnum, ctrl->info.selector,
> > +                          buf, sizeof(*buf));
>
> That's the current value, not the default. Is that intended ?

Yes, the driver does not init the other controls to the default value.
So I'd rather be consistent.

>
> > +     /* If we cannot read the control skip it. */
> > +     if (ret) {
> > +             kfree(buf);
> > +             return ret;
> > +     }
> > +     init_val = *buf;
> > +
> > +     /* If PLF value cannot be set to off, it is limited. */
> > +     *buf = V4L2_CID_POWER_LINE_FREQUENCY_DISABLED;
> > +     ret = uvc_query_ctrl(chain->dev, UVC_SET_CUR, ctrl->entity->id,
> > +                          chain->dev->intfnum, ctrl->info.selector,
> > +                          buf, sizeof(*buf));
> > +     if (ret) {
> > +             out_mapping = &uvc_ctrl_power_line_mapping_limited;
> > +             goto end;
>
> If setting the value failed you don't need to restore it, do you ?
>
> > +     }
> > +
> > +     /* UVC 1.1 does not define auto, we can exit. */
> >       if (chain->dev->uvc_version < 0x150)
> > -             return __uvc_ctrl_add_mapping(chain, ctrl,
> > -                                           &uvc_ctrl_power_line_mapping_uvc11);
> > +             goto end;
> > +
> > +     /* Check if the device supports auto. */
> > +     *buf = V4L2_CID_POWER_LINE_FREQUENCY_AUTO;
> > +     ret = uvc_query_ctrl(chain->dev, UVC_SET_CUR, ctrl->entity->id,
> > +                          chain->dev->intfnum, ctrl->info.selector,
> > +                          buf, sizeof(*buf));
>
> Now for the real annoying question. I've encountered quite a few devices
> that would crash when the driver tried to get/set lots of controls at
> probe time. This is why the control cache is populated the first time a
> control is queried, and not when the device is probed. I'm always
> worried when adding more control accesses at probe time that some
> devices will behave improperly.

If we encounter a device like that we could quirk it.

>
> Given the number of UVC users I tend to be on the conservative side, but
> obviously, if we were to strictly avoid new access patterns to the
> device, the driver wouldn't be able to evolve. I do like patches 4/5 and
> 5/5, so I'm tempted to take the risk and revert this series later if
> needed. That would likely make some other users unhappy if they rely on
> the heuristic.
>
> Another issue is how to get vendors to implement the power line
> frequency control correctly. With the series applied, vendors won't
> notice they're doing something wrong. Of course, they probably don't
> check the behaviour of their devices with Linux in the first place, so
> I'm not sure what we could do.

We can add the check to v4l2-compliance....

Although I would love to see a uvc-compliance tool. If the tool can be
easily run in windows/linux without a driver I guess ISP vendors will
run it to validate their code.
Right now there is no way to validate a usb camera beyond: it runs in
windows and in cheese.

>
> > +     if (!ret)
> > +             out_mapping = &uvc_ctrl_power_line_mapping_uvc15;
> > +
> > +end:
> > +     /* Restore initial value and add mapping. */
> > +     *buf = init_val;
> > +     uvc_query_ctrl(chain->dev, UVC_SET_CUR, ctrl->entity->id,
> > +                    chain->dev->intfnum, ctrl->info.selector,
> > +                    buf, sizeof(*buf));
> >
> > -     return __uvc_ctrl_add_mapping(chain, ctrl,
> > -                                   &uvc_ctrl_power_line_mapping_uvc15);
> > +     kfree(buf);
> > +     return __uvc_ctrl_add_mapping(chain, ctrl, out_mapping);
> >  }
> >
> >  static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>
> --
> Regards,
>
> Laurent Pinchart


-- 
Ricardo Ribalda