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

[Dan Scally <dan.scally@ideasonboard.com>]

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@ideasonboard.com>
>> ---
>>   .../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
>>