Hi Dan,
On Mon, Dec 19, 2022 at 09:44:53AM +0000, Dan Scally wrote:
> On 18/12/2022 23:17, Laurent Pinchart wrote:
> > On Thu, Dec 15, 2022 at 12:03:03PM +0000, Dan Scally wrote:
> >> 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;
> >
> > As you store a pointer to the target, don't you need to keep a reference
> > to it somehow, to avoid the target being deleted ? Or is that handled by
> > configfs itself ?
>
> configfs is doing it in configfs_symlink()
>
> > It also looks like you need to refcount the number of links to the
> > target, to disallow changes to the color matching attributes when the
> > descriptor is linked.
>
> Yes; I had thought that that was unnecessary so skipped it, but turns
> out I'm wrong - I'll add that in.
>
> > This being said, are links the best option here ? Why can't we create
> > color matching descriptors as children of format descriptors the same
> > way that frame descriptors are handled ?
>
> We can do it that way, but I thought that it made more sense to expand
> the current implementation because
>
>
> 1) I'm reluctant to get rid of the default descriptor and I wouldn't
> want to have them in multiple places.
>
> 2) We're planning to expand the uncompressed format to support more than
> just yuy2, which for some cameras might result in _a lot_ of available
> formats (vivid has 80!). It seems easier to have color matching
> descriptors that you can set once and simply link to than have to
> recreate them for each format.
The first point doesn't convince me, as I think we should remove the
default. As the UVC specification doesn't mandate a color matching
descriptor, I think it should be left to the system integrator whether
to include one or not.
I had considered the second point as well. In practice I expect we'll
have few formats, but still more than one. I also expect them to have
identical color matching data. Links could avoid recreating the same
descriptors, but the gain isn't very big. Consider the format
descriptors as well, we could have identifical format descriptors for
different formats, and we don't use links.
I thus still think links are overkill, but if the general consensus is
that they're better, I'm fine with that.
> >>>> +
> >>>> +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 = {
> >
> > Not a candidate for this patch (or this series), but I wonder if this
> > could be made const. It would involve making several pointers in struct
> > config_item_type const, which may or may not be straightforward.
> >
> >>>> + .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",
--
Regards,
Laurent Pinchart
