by Laurent Pinchart

[permalink] [raw]

Subject: Re: [PATCH 3/5] media: uvcvideo: Probe the PLF characteristics

Hi Ricardo,

On Mon, Jun 10, 2024 at 11:51:55PM +0200, Ricardo Ribalda wrote:
> On Mon, 10 Jun 2024 at 16:14, Laurent Pinchart wrote:
> > 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 <[email protected]>
> > > ---
> > > 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.

I'm fine with that, let's update the comment to "Save the current PLF
value".

> > > + /* 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.

Now we could place bets on what is less likely to scale, quirking
devices that have a bad PLF implementation, or quirking devices whose
firmware will crash when queried too much at probe time :-)

> > 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.

*without a driver* is doable with libusb but would be *lots* of work,
basically duplicating the whole uvcvideo driver in userspace. That's not
a project I would start, but it would be interesting.

> Right now there is no way to validate a usb camera beyond: it runs in
> windows and in cheese.

Isn't the USB-IF supposed to have a compliance test suite ? Given all
the broken devices we hear about, it must either not be very good, or
not be used by vendors.

I think a compliance tool based on top of the uvcvideo kernel driver
would already be a good step forward.

> > > + 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

2024-06-11 08:23:16

by Ricardo Ribalda

[permalink] [raw]

Subject: Re: [PATCH 3/5] media: uvcvideo: Probe the PLF characteristics

Hi Laurent

On Tue, 11 Jun 2024 at 01:48, Laurent Pinchart
<[email protected]> wrote:
>
> Hi Ricardo,
>
> On Mon, Jun 10, 2024 at 11:51:55PM +0200, Ricardo Ribalda wrote:
> > On Mon, 10 Jun 2024 at 16:14, Laurent Pinchart wrote:
> > > 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 <[email protected]>
> > > > ---
> > > > 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.
>
> I'm fine with that, let's update the comment to "Save the current PLF
> value".

done in my tree, if there is a v3 I will fix it, otherwise you are
free to modify it if you want ;)

>
> > > > + /* 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.
>
> Now we could place bets on what is less likely to scale, quirking
> devices that have a bad PLF implementation, or quirking devices whose
> firmware will crash when queried too much at probe time :-)

:)

>
> > > 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.
>
> *without a driver* is doable with libusb but would be *lots* of work,
> basically duplicating the whole uvcvideo driver in userspace. That's not
> a project I would start, but it would be interesting.
>
> > Right now there is no way to validate a usb camera beyond: it runs in
> > windows and in cheese.
>
> Isn't the USB-IF supposed to have a compliance test suite ? Given all
> the broken devices we hear about, it must either not be very good, or
> not be used by vendors.

At least there is no public compliance test suite, and if third
parties cannot validate the results the test is not very useful.

>
> I think a compliance tool based on top of the uvcvideo kernel driver
> would already be a good step forward.
>
> > > > + 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

Regards!

--
Ricardo Ribalda