Hi Laurent
On Fri, 17 Jun 2022 at 15:44, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
>
> 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.
ups, sorry about that :(
. Will send as a new version
>
> > + 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
--
Ricardo Ribalda
