Hi Ricardo,
Thank you for the patch.
On Fri, Jun 17, 2022 at 12:36:39PM +0200, Ricardo Ribalda wrote:
> Some devices do not implement all their controls in a way that complies
> with the UVC specification. This is for instance the case for several
> devices that do not support the disabled mode for the power line
> frequency control. Add a mechanism to allow per-device control mapping
> overrides to avoid errors when accessing non-compliant controls.
>
> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> ---
> drivers/media/usb/uvc/uvc_ctrl.c | 35 ++++++++++++++++++++++++++------
> drivers/media/usb/uvc/uvcvideo.h | 1 +
> 2 files changed, 30 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index a709ebbb4d69..092decfdaa62 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -2403,9 +2403,8 @@ static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain,
> {
> const struct uvc_control_info *info = uvc_ctrls;
> const struct uvc_control_info *iend = info + ARRAY_SIZE(uvc_ctrls);
> - const struct uvc_control_mapping *mapping = uvc_ctrl_mappings;
> - const struct uvc_control_mapping *mend =
> - mapping + ARRAY_SIZE(uvc_ctrl_mappings);
> + const struct uvc_control_mapping *mapping;
> + const struct uvc_control_mapping *mend;
>
> /* XU controls initialization requires querying the device for control
> * information. As some buggy UVC devices will crash when queried
> @@ -2433,14 +2432,38 @@ static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain,
> if (!ctrl->initialized)
> return;
>
> - /* Process common mappings first. */
> - for (; mapping < mend; ++mapping) {
> + /*
> + * First check if the device provides a custom mapping for this control,
> + * used to override standard mappings for non-conformant devices. Don't
> + * process standard mappings if a custom mapping is found. This
> + * mechanism doesn't support combining standard and custom mappings for
> + * a single control.
> + */
> + if (chain->dev->info->mappings) {
> + bool custom = false;
> + unsigned int i;
> +
> + for (i = 0; (mapping = chain->dev->info->mappings[i]); ++i) {
> + if (uvc_entity_match_guid(ctrl->entity, mapping->entity) &&
> + ctrl->info.selector == mapping->selector) {
> + __uvc_ctrl_add_mapping(chain, ctrl, mapping);
> + custom = true;
> + }
> + }
> +
> + if (custom)
> + return;
> + }
> +
> + /* Process common mappings next. */
> + mend = mapping + ARRAY_SIZE(uvc_ctrl_mappings);
I don't think mapping has the right value here, it could even be
uninitialized. Let's make this
mapping = uvc_ctrl_mappings;
mend = mapping + ARRAY_SIZE(uvc_ctrl_mappings);
for (; mapping < mend; ++mapping) {
to match the code below.
> + for (mapping = uvc_ctrl_mappings; mapping < mend; ++mapping) {
> if (uvc_entity_match_guid(ctrl->entity, mapping->entity) &&
> ctrl->info.selector == mapping->selector)
> __uvc_ctrl_add_mapping(chain, ctrl, mapping);
> }
>
> - /* And then version-specific mappings. */
> + /* Finally process version-specific mappings. */
> if (chain->dev->uvc_version < 0x0150) {
> mapping = uvc_ctrl_mappings_uvc11;
> mend = mapping + ARRAY_SIZE(uvc_ctrl_mappings_uvc11);
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index c5b4febd2d94..fff5c5c99a3d 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -667,6 +667,7 @@ struct uvc_device_info {
> u32 quirks;
> u32 meta_format;
> u16 uvc_version;
> + const struct uvc_control_mapping **mappings;
> };
>
> struct uvc_device {
--
Regards,
Laurent Pinchart
