Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220617103645.71560-1-ribalda@chromium.org> <20220617103645.71560-3-ribalda@chromium.org>
 <YqyFIq8iv/FZ6tNe@pendragon.ideasonboard.com>
In-Reply-To: <YqyFIq8iv/FZ6tNe@pendragon.ideasonboard.com>
From:   Ricardo Ribalda <ribalda@chromium.org>
Date:   Fri, 17 Jun 2022 16:07:20 +0200
Message-ID: <CANiDSCuZ0O3iv2Kk3b7Ui9ZWYM3PVsZAsF2DAL54XY9MkyHuLg@mail.gmail.com>
Subject: Re: [PATCH v7 2/8] media: uvcvideo: Add support for per-device
 control mapping overrides
To:     Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc:     Mauro Carvalho Chehab <mchehab@kernel.org>,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        tfiga@chromium.org, senozhatsky@chromium.org, yunkec@google.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Laurent

On Fri, 17 Jun 2022 at 15:44, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> 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@chromium.org>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  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