Hi Dan,
On Sun, Jan 01, 2023 at 08:55:43PM +0000, Dan Scally wrote:
> On 28/12/2022 02:50, Laurent Pinchart wrote:
> > On Mon, Dec 19, 2022 at 02:43:16PM +0000, Daniel Scally wrote:
> >> 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>
> >> ---
> >> Changes in v2:
> >>
> >> - New section of the ABI documentation
> >> - uvcg_format_allow_link() now checks to see if an existing link is
> >> already there
> >> - .allow_link() and .drop_link() track color_matching->refcnt
> >>
> >> .../ABI/testing/configfs-usb-gadget-uvc | 17 ++++
> >> drivers/usb/gadget/function/uvc_configfs.c | 99 ++++++++++++++++++-
> >> 2 files changed, 114 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> >> index 53258b7c6f2d..e7753b2cb11b 100644
> >> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> >> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> >> @@ -177,6 +177,23 @@ Description: Default color matching descriptors
> >> white
> >> ======================== ======================================
> >>
> >> +What: /config/usb-gadget/gadget/functions/uvc.name/streaming/color_matching/name
> >> +Date: Dec 2022
> >> +KernelVersion: 6.3
> >> +Description: Additional color matching descriptors
> >> +
> >> + All attributes read/write:
> >> +
> >> + ======================== ======================================
> >> + bMatrixCoefficients matrix used to compute luma and
> >> + chroma values from the color primaries
> >> + bTransferCharacteristics optoelectronic transfer
> >> + characteristic of the source picture,
> >> + also called the gamma function
> >> + bColorPrimaries color primaries and the reference
> >> + white
> >> + ======================== ======================================
> >> +
> >
> > Should the link also be documented somewhere ?
>
> I actually couldn't see that any of the links were described in this
> document, so I skipped it. I'm working on a more comprehensive piece of
> documentation which describes the UVC Gadget more fully, and my plan was
> to do it there.
>
> >
> >> What: /config/usb-gadget/gadget/functions/uvc.name/streaming/mjpeg
> >> Date: Dec 2014
> >> KernelVersion: 4.0
> >> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> >> index ef5d75942f24..3be6ca936851 100644
> >> --- a/drivers/usb/gadget/function/uvc_configfs.c
> >> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> >> @@ -771,6 +771,77 @@ uvcg_format_get_default_color_match(struct config_item *streaming)
> >> return color_match;
> >> }
> >>
> >> +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 uvcg_color_matching *color_matching_desc;
> >> + struct config_item *streaming, *color_matching;
> >> + 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;
> >> + }
> >> +
> >> + fmt = to_uvcg_format(src);
> >
> > It's been a long time since I worked with configfs, so I may be wrong,
> > but shouldn't we check the name of the source here to make sure it's
> > equal to "color_matching" ? Or do you want to allow the user to name the
> > source freely ? That would be a bit confusing I think.
>
> The source will be either streaming/uncompressed/<name> or
> streaming/mjpeg/<name>. I don't think we need to check that, as this
> function will only be called if that's where the user is attempting to
> link from. So it'll be a link from streaming/uncompressed/u to
> streaming/color_matching/yuy2 for example.
Ah so the source is the directory in which the link is created, not the
link itself ? In that case you indeed can't check the link name as that
information isn't available.
> >> +
> >> + /*
> >> + * There's always a color matching descriptor associated with the format
> >> + * but without a symlink it should only ever be the default one. If it's
> >> + * not the default, there's already a symlink and we should bail out.
> >> + */
> >> + color_matching_desc = uvcg_format_get_default_color_match(streaming);
> >> + if (fmt->color_matching != color_matching_desc) {
> >
> > If you check the source link name, I suppose this could be dropped. Then
> > you coud just write
> >
> > fmt->color_matching->refcnt--;
> >
> > and avoid the call to uvcg_format_get_default_color_match().
>
> Not sure I follow here I'm afraid. As I see it, to retain the current
> functionality (sending the 1/1/4 descriptor when no other is specified)
> we need to link the default descriptor when none other is linked, so we
> need to check whether or not that's the one that's currently linked to
> know if there's another symlink hanging around already. This check is
> designed to avoid streaming/uncompressed/u being linked to both
> streaming/color_matching/yuy2 and streaming/color_matching/mjpeg for
> example.
The idea is that if we could restrict the link name to "color_matching",
we could only reach this point in the code when no link exists yet as
the configfs core wouldn't allow creating a second link with the same
name.
> >> + ret = -EBUSY;
> >> + goto out_put_cm;
> >> + }
> >> +
> >> + color_matching_desc->refcnt--;
> >> +
> >> + color_matching_desc = to_uvcg_color_matching(to_config_group(tgt));
> >> + fmt->color_matching = color_matching_desc;
> >> + color_matching_desc->refcnt++;
> >
> > And this could become
> >
> > fmt->color_matching = to_uvcg_color_matching(to_config_group(tgt));
> > fmt->color_matching->refcnt++;
> >
> >> +
> >> +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 mutex *su_mutex = &src->ci_group->cg_subsys->su_mutex;
> >> + struct uvcg_color_matching *color_matching_desc;
> >> + struct config_item *streaming;
> >> + struct uvcg_format *fmt;
> >> +
> >> + mutex_lock(su_mutex);
> >> +
> >> + color_matching_desc = to_uvcg_color_matching(to_config_group(tgt));
> >> + color_matching_desc->refcnt--;
> >> +
> >> + streaming = src->ci_parent->ci_parent;
> >> + color_matching_desc = uvcg_format_get_default_color_match(streaming);
> >> +
> >> + fmt = to_uvcg_format(src);
> >> + fmt->color_matching = color_matching_desc;
> >> + color_matching_desc->refcnt++;
> >
> > fmt->color_matching = uvcg_format_get_default_color_match(streaming);
> > fmt->color_matching->refcnt++;
> >
> > although if you increase the refcnt in
> > uvcg_format_get_default_color_match() as I proposed in a previous patch
> > in this series, this would just be
> >
> > fmt->color_matching = uvcg_format_get_default_color_match(streaming);
> >
> >> +
> >> + 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;
> >> @@ -1571,7 +1642,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,
> >> @@ -1767,7 +1838,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,
> >> @@ -1922,6 +1993,29 @@ 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_color_matching *color_match;
> >> +
> >> + color_match = kzalloc(sizeof(*color_match), GFP_KERNEL);
> >> + if (!color_match)
> >> + return ERR_PTR(-ENOMEM);
> >> +
> >> + color_match->desc.bLength = UVC_DT_COLOR_MATCHING_SIZE;
> >> + color_match->desc.bDescriptorType = USB_DT_CS_INTERFACE;
> >> + color_match->desc.bDescriptorSubType = UVC_VS_COLORFORMAT;
> >> +
> >> + config_group_init_type_name(&color_match->group, name,
> >> + &uvcg_color_matching_type);
> >> +
> >> + return &color_match->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_color_matching *color_match;
> >> @@ -1947,6 +2041,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
