Re: [PATCH 6/6] usb: gadget: uvc: Allow creating new color matching descriptors

[Dan Scally <dan.scally@xxxxxxxxxxxxxxxx>]

Hi Kieran

On 15/12/2022 12:00, Kieran Bingham wrote:
> Quoting Daniel Scally (2022-12-13 08:37:36)
> > Allow users to create new color matching descriptors in addition to
> > the default one. These must be associated with a UVC format in order
> > to be transmitted to the host, which is achieved by symlinking from
> > the format to the newly created color matching descriptor - extend
> > the uncompressed and mjpeg formats to support that linking operation.
> >
> > Signed-off-by: Daniel Scally <dan.scally@xxxxxxxxxxxxxxxx>
> > ---
> >   .../ABI/testing/configfs-usb-gadget-uvc       |  4 +-
> >   drivers/usb/gadget/function/uvc_configfs.c    | 79 ++++++++++++++++++-
> >   2 files changed, 79 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> > index 3512f4899fe3..ce629f0880a9 100644
> > --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> > +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> > @@ -160,10 +160,10 @@ Date:             Dec 2014
> >   KernelVersion: 4.0
> >   Description:   Color matching descriptors
> >   
> > -What:          /config/usb-gadget/gadget/functions/uvc.name/streaming/color_matching/default
> > +What:          /config/usb-gadget/gadget/functions/uvc.name/streaming/color_matching/name
> Hrm... do we need to keep 'default' (and keep it available in patch
> 1/6?) so that the default is still kept accesible?


It's still there - it's just no longer necessarily the only entry

> That would mean others are added as siblings to the default, and then
> the one that gets linked is the one that is used perhaps?

Yep that's how it's implemented
> Feels like that would overcomplicate 'the default case' maybe ... but
> I'm weary that this is 'removing' the default from ABI...?
>
> >   Date:          Dec 2014
> >   KernelVersion: 4.0
> Hrm ... and this would leave the documentation stating that you could
> provide custom color matching descriptors from v4.0 onwards, which would
> be inaccurate ?


Ah, good point...fair enough, I'll add a new entry for the custom named 
ones.

> So I'm not sure we can just rename this documentation segment eitherway.
>
>
> > -Description:   Default color matching descriptors
> > +Description:   color matching descriptors
> >   
> >                  All attributes read/write:
> >   
> > diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> > index 4fbc42d738a4..82c10f0dab71 100644
> > --- a/drivers/usb/gadget/function/uvc_configfs.c
> > +++ b/drivers/usb/gadget/function/uvc_configfs.c
> > @@ -769,6 +769,58 @@ static struct uvcg_cmd *uvcg_format_get_default_cmd(struct config_item *streamin
> >          return cmd;
> >   }
> >   
> > +static int uvcg_format_allow_link(struct config_item *src, struct config_item *tgt)
> > +{
> > +       struct mutex *su_mutex = &src->ci_group->cg_subsys->su_mutex;
> > +       struct config_item *streaming, *color_matching;
> > +       struct uvcg_cmd *color_matching_desc;
> > +       struct uvcg_format *fmt;
> > +       int ret = 0;
> > +
> > +       mutex_lock(su_mutex);
> > +
> > +       streaming = src->ci_parent->ci_parent;
> > +       color_matching = config_group_find_item(to_config_group(streaming), "color_matching");
> > +       if (!color_matching || color_matching != tgt->ci_parent) {
> > +               ret = -EINVAL;
> > +               goto out_put_cm;
> > +       }
> > +
> > +       color_matching_desc = to_uvcg_cmd(to_config_group(tgt));
> > +       fmt = to_uvcg_format(src);
> > +       fmt->color_matching = color_matching_desc;
> > +
> > +out_put_cm:
> > +       config_item_put(color_matching);
> > +       mutex_unlock(su_mutex);
> > +
> > +       return ret;
> > +}
> > +
> > +static void uvcg_format_drop_link(struct config_item *src, struct config_item *tgt)
> > +{
> > +       struct config_item *streaming;
> > +       struct mutex *su_mutex = &src->ci_group->cg_subsys->su_mutex;
> > +       struct uvcg_format *fmt;
> > +       struct uvcg_cmd *cmd;
> > +
> > +       mutex_lock(su_mutex);
> > +
> > +       streaming = src->ci_parent->ci_parent;
> > +       cmd = uvcg_format_get_default_cmd(streaming);
> > +
> > +       fmt = to_uvcg_format(src);
> > +       fmt->color_matching = cmd;
> > +
> > +       mutex_unlock(su_mutex);
> > +}
> > +
> > +static struct configfs_item_operations uvcg_format_item_operations = {
> > +       .release        = uvcg_config_item_release,
> > +       .allow_link     = uvcg_format_allow_link,
> > +       .drop_link      = uvcg_format_drop_link,
> > +};
> > +
> >   static ssize_t uvcg_format_bma_controls_show(struct uvcg_format *f, char *page)
> >   {
> >          struct f_uvc_opts *opts;
> > @@ -1569,7 +1621,7 @@ static struct configfs_attribute *uvcg_uncompressed_attrs[] = {
> >   };
> >   
> >   static const struct config_item_type uvcg_uncompressed_type = {
> > -       .ct_item_ops    = &uvcg_config_item_ops,
> > +       .ct_item_ops    = &uvcg_format_item_operations,
> >          .ct_group_ops   = &uvcg_uncompressed_group_ops,
> >          .ct_attrs       = uvcg_uncompressed_attrs,
> >          .ct_owner       = THIS_MODULE,
> > @@ -1764,7 +1816,7 @@ static struct configfs_attribute *uvcg_mjpeg_attrs[] = {
> >   };
> >   
> >   static const struct config_item_type uvcg_mjpeg_type = {
> > -       .ct_item_ops    = &uvcg_config_item_ops,
> > +       .ct_item_ops    = &uvcg_format_item_operations,
> >          .ct_group_ops   = &uvcg_mjpeg_group_ops,
> >          .ct_attrs       = uvcg_mjpeg_attrs,
> >          .ct_owner       = THIS_MODULE,
> > @@ -1912,6 +1964,28 @@ static const struct config_item_type uvcg_color_matching_type = {
> >    * streaming/color_matching
> >    */
> >   
> > +static struct config_group *uvcg_color_matching_make(struct config_group *group,
> > +                                                    const char *name)
> > +{
> > +       struct uvcg_cmd *cmd;
> > +
> > +       cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
> > +       if (!cmd)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       cmd->desc.bLength = UVC_DT_COLOR_MATCHING_SIZE;
> > +       cmd->desc.bDescriptorType = USB_DT_CS_INTERFACE;
> > +       cmd->desc.bDescriptorSubType = UVC_VS_COLORFORMAT;
> > +
> > +       config_group_init_type_name(&cmd->group, name, &uvcg_color_matching_type);
> > +
> > +       return &cmd->group;
> > +}
> > +
> > +static struct configfs_group_operations uvcg_color_matching_grp_group_ops = {
> > +       .make_group     = uvcg_color_matching_make,
> > +};
> > +
> >   static int uvcg_color_matching_create_children(struct config_group *parent)
> >   {
> >          struct uvcg_cmd *cmd;
> > @@ -1937,6 +2011,7 @@ static int uvcg_color_matching_create_children(struct config_group *parent)
> >   static const struct uvcg_config_group_type uvcg_color_matching_grp_type = {
> >          .type = {
> >                  .ct_item_ops    = &uvcg_config_item_ops,
> > +               .ct_group_ops   = &uvcg_color_matching_grp_group_ops,
> >                  .ct_owner       = THIS_MODULE,
> >          },
> >          .name = "color_matching",
> > --
> > 2.34.1