Hi Dan,
Thank you for the patch.
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 ?
> 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.
> +
> + /*
> + * 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().
> + 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
