Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201104180734.286789-1-ribalda@chromium.org> <20201104180734.286789-3-ribalda@chromium.org>
 <20201106060602.GA6926@pendragon.ideasonboard.com>
In-Reply-To: <20201106060602.GA6926@pendragon.ideasonboard.com>
From:   Ricardo Ribalda <ribalda@chromium.org>
Date:   Fri, 6 Nov 2020 09:45:27 +0100
Message-ID: <CANiDSCuZ_euz1tb35ETffN_NxLxW_N-7hBCpk-HhuRuRJBFxkA@mail.gmail.com>
Subject: Re: [PATCH v2 2/7] media: uvcvideo: Move guid to entity
To:     Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc:     Mauro Carvalho Chehab <mchehab@kernel.org>,
        linux-media@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Laurent

Thanks for the review

On Fri, Nov 6, 2020 at 7:06 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Wed, Nov 04, 2020 at 07:07:29PM +0100, Ricardo Ribalda wrote:
> > Instead of having multiple copies of the entity guid on the code, move
> > it to the entity structure.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_ctrl.c   | 30 ++++--------------------------
> >  drivers/media/usb/uvc/uvc_driver.c | 21 +++++++++++++++++++--
> >  drivers/media/usb/uvc/uvcvideo.h   |  2 +-
> >  3 files changed, 24 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index f479d8971dfb..0e480b75e724 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -826,31 +826,10 @@ static void uvc_set_le_value(struct uvc_control_mapping *mapping,
> >   * Terminal and unit management
> >   */
> >
> > -static const u8 uvc_processing_guid[16] = UVC_GUID_UVC_PROCESSING;
> > -static const u8 uvc_camera_guid[16] = UVC_GUID_UVC_CAMERA;
> > -static const u8 uvc_media_transport_input_guid[16] =
> > -     UVC_GUID_UVC_MEDIA_TRANSPORT_INPUT;
> > -
> >  static int uvc_entity_match_guid(const struct uvc_entity *entity,
> > -     const u8 guid[16])
> > +                              const u8 guid[16])
> >  {
> > -     switch (UVC_ENTITY_TYPE(entity)) {
> > -     case UVC_ITT_CAMERA:
> > -             return memcmp(uvc_camera_guid, guid, 16) == 0;
> > -
> > -     case UVC_ITT_MEDIA_TRANSPORT_INPUT:
> > -             return memcmp(uvc_media_transport_input_guid, guid, 16) == 0;
> > -
> > -     case UVC_VC_PROCESSING_UNIT:
> > -             return memcmp(uvc_processing_guid, guid, 16) == 0;
> > -
> > -     case UVC_VC_EXTENSION_UNIT:
> > -             return memcmp(entity->extension.guidExtensionCode,
> > -                           guid, 16) == 0;
> > -
> > -     default:
> > -             return 0;
> > -     }
> > +     return memcmp(entity->guid, guid, sizeof(entity->guid)) == 0;
> >  }
> >
> >  /* ------------------------------------------------------------------------
> > @@ -1776,8 +1755,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
> >       if (data == NULL)
> >               return -ENOMEM;
> >
> > -     memcpy(info->entity, ctrl->entity->extension.guidExtensionCode,
> > -            sizeof(info->entity));
> > +     memcpy(info->entity, ctrl->entity->guid, sizeof(info->entity));
> >       info->index = ctrl->index;
> >       info->selector = ctrl->index + 1;
> >
> > @@ -1883,7 +1861,7 @@ int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
> >
> >       if (!found) {
> >               uvc_trace(UVC_TRACE_CONTROL, "Control %pUl/%u not found.\n",
> > -                     entity->extension.guidExtensionCode, xqry->selector);
> > +                     entity->guid, xqry->selector);
> >               return -ENOENT;
> >       }
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 9fc0b600eab1..77fea26faa9a 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -1019,6 +1019,11 @@ static int uvc_parse_streaming(struct uvc_device *dev,
> >       return ret;
> >  }
> >
> > +static const u8 uvc_camera_guid[16] = UVC_GUID_UVC_CAMERA;
> > +static const u8 uvc_media_transport_input_guid[16] =
> > +     UVC_GUID_UVC_MEDIA_TRANSPORT_INPUT;
> > +static const u8 uvc_processing_guid[16] = UVC_GUID_UVC_PROCESSING;
> > +
> >  static struct uvc_entity *uvc_alloc_entity(u16 type, u8 id,
> >               unsigned int num_pads, unsigned int extra_size)
> >  {
> > @@ -1038,6 +1043,18 @@ static struct uvc_entity *uvc_alloc_entity(u16 type, u8 id,
> >       entity->id = id;
> >       entity->type = type;
> >
> > +     switch (type) {
> > +     case UVC_ITT_CAMERA:
> > +             memcpy(entity->guid, uvc_camera_guid, 16);
> > +             break;
> > +     case UVC_ITT_MEDIA_TRANSPORT_INPUT:
> > +             memcpy(entity->guid, uvc_media_transport_input_guid, 16);
> > +             break;
> > +     case UVC_VC_PROCESSING_UNIT:
> > +             memcpy(entity->guid, uvc_processing_guid, 16);
> > +             break;
> > +     }
>
> Given that the GUID is set in uvc_parse_vendor_control() and
> uvc_parse_standard_control() for extension units, I'm wondering if it
> would make sense to move it there for the other entity types too. Up to
> you. Otherwise, I'd add the following comment above the switch:
>
>         /*
>          * Set the GUID for standard entity types. For extension units, the GUID
>          * is initialized by the caller.
>          */

I added the comment. So far I am working on

https://github.com/ribalda/linux/tree/uvctest-v3

Please let me know when you are ready with v2, to send v3 to the mailing list.

Thanks!

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > +
> >       entity->num_links = 0;
> >       entity->num_pads = num_pads;
> >       entity->pads = ((void *)(entity + 1)) + extra_size;
> > @@ -1109,7 +1126,7 @@ static int uvc_parse_vendor_control(struct uvc_device *dev,
> >               if (unit == NULL)
> >                       return -ENOMEM;
> >
> > -             memcpy(unit->extension.guidExtensionCode, &buffer[4], 16);
> > +             memcpy(unit->guid, &buffer[4], 16);
> >               unit->extension.bNumControls = buffer[20];
> >               memcpy(unit->baSourceID, &buffer[22], p);
> >               unit->extension.bControlSize = buffer[22+p];
> > @@ -1368,7 +1385,7 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
> >               if (unit == NULL)
> >                       return -ENOMEM;
> >
> > -             memcpy(unit->extension.guidExtensionCode, &buffer[4], 16);
> > +             memcpy(unit->guid, &buffer[4], 16);
> >               unit->extension.bNumControls = buffer[20];
> >               memcpy(unit->baSourceID, &buffer[22], p);
> >               unit->extension.bControlSize = buffer[22+p];
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index a3dfacf069c4..df7bf2d104a3 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -304,6 +304,7 @@ struct uvc_entity {
> >       u8 id;
> >       u16 type;
> >       char name[64];
> > +     u8 guid[16];
> >
> >       /* Media controller-related fields. */
> >       struct video_device *vdev;
> > @@ -342,7 +343,6 @@ struct uvc_entity {
> >               } selector;
> >
> >               struct {
> > -                     u8  guidExtensionCode[16];
> >                       u8  bNumControls;
> >                       u8  bControlSize;
> >                       u8  *bmControls;
>
> --
> Regards,
>
> Laurent Pinchart


-- 
Ricardo Ribalda