| Message ID | 20240318-billion-v1-3-2f7bc0ee2030@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: uvc: Probe PLF limits at start-up | expand |
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 ? > + /* 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. 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. > + 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[] = {
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
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 <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. 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[] = {
Hi Laurent On Tue, 11 Jun 2024 at 01:48, Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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 <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. > > 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!
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; + 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)); + /* 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; + } + + /* 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)); + 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[] = {
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(-)