From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1E1A5C54EAA for ; Mon, 30 Jan 2023 12:20:30 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231340AbjA3MU3 (ORCPT ); Mon, 30 Jan 2023 07:20:29 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51868 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236628AbjA3MU1 (ORCPT ); Mon, 30 Jan 2023 07:20:27 -0500 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E175636FE1 for ; Mon, 30 Jan 2023 04:20:25 -0800 (PST) Received: from [192.168.0.43] (cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id CFCE18B8; Mon, 30 Jan 2023 13:20:23 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1675081224; bh=EON7GaVkzXrbhq+qwTMKdFiUhNWkMr3JuL/Pxhs6+5g=; h=Date:Subject:From:To:Cc:References:In-Reply-To:From; b=UE3d6pThm/l6aivR5o9xHeW0UWSR5alL0xUr4Fr6beBqZRIlz6RHweYYmynbasImt aW3J0g0eLrnbHcJV0RSLzAwKtz2qV6lYot6ZUAAAOwSIIHPjIAlZxmKOsMGiZQH+BX G9fQO1hQ9Cdc6/GVODsYJUTuYYNRtCq9m/MmD3Ug= Message-ID: <1cf61684-e095-dbe5-2967-0fcdeabc65f9@ideasonboard.com> Date: Mon, 30 Jan 2023 12:20:20 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.2 Subject: Re: [PATCH v2 4/7] usb: gadget: uvc: Copy color matching descriptor for each frame Content-Language: en-US From: Dan Scally To: Laurent Pinchart Cc: linux-usb@vger.kernel.org, gregkh@linuxfoundation.org, w36195@motorola.com, m.grzeschik@pengutronix.de, kieran.bingham@ideasonboard.com, torleiv@huddly.com References: <20221219144316.757680-1-dan.scally@ideasonboard.com> <20221219144316.757680-5-dan.scally@ideasonboard.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-usb@vger.kernel.org On 30/01/2023 12:01, Dan Scally wrote: > Hi Laurent > > On 28/12/2022 02:33, Laurent Pinchart wrote: >> Hi Dan, >> >> Thank you for the patch. >> >> On Mon, Dec 19, 2022 at 02:43:13PM +0000, Daniel Scally wrote: >>> As currently implemented the default color matching descriptor is >>> appended after _all_ the formats and frames that the gadget is >>> configured with. According to the UVC specifications however this >>> is supposed to be on a per-format basis (section 3.9.2.6): >>> >>> "Only one instance is allowed for a given format and if present, >>> the Color Matching descriptor shall be placed following the Video >>> and Still Image Frame descriptors for that format." >>> >>> Associate the default color matching descriptor with struct >>> uvcg_format and copy it once-per-format instead of once only. >>> >>> Signed-off-by: Daniel Scally >>> --- >>> Changes in v2: >>> >>>     - Renamed uvcg_cmd and associated variables. >>>     - Formatting >>>     - Increased the refcnt variable for the color matching struct in >>>     the format make() functions >>> >>>   drivers/usb/gadget/function/uvc_configfs.c | 64 >>> +++++++++++++++++++++- >>>   drivers/usb/gadget/function/uvc_configfs.h | 13 +++-- >>>   2 files changed, 68 insertions(+), 9 deletions(-) >>> >>> diff --git a/drivers/usb/gadget/function/uvc_configfs.c >>> b/drivers/usb/gadget/function/uvc_configfs.c >>> index 147d3def24dd..6fb7ac8366fe 100644 >>> --- a/drivers/usb/gadget/function/uvc_configfs.c >>> +++ b/drivers/usb/gadget/function/uvc_configfs.c >>> @@ -748,6 +748,29 @@ static const char * const uvcg_format_names[] = { >>>       "mjpeg", >>>   }; >>>   +static struct uvcg_color_matching * >>> +uvcg_format_get_default_color_match(struct config_item *streaming) >>> +{ >>> +    struct config_item *color_matching_item, *cm_default; >>> +    struct uvcg_color_matching *color_match; >>> + >>> +    color_matching_item = >>> config_group_find_item(to_config_group(streaming), >>> +                             "color_matching"); >>> +    if (!color_matching_item) >>> +        return NULL; >>> + >>> +    cm_default = >>> config_group_find_item(to_config_group(color_matching_item), >>> +                        "default"); >>> +    config_item_put(color_matching_item); >>> +    if (!cm_default) >>> +        return NULL; >>> + >>> +    color_match = to_uvcg_color_matching(to_config_group(cm_default)); >>> +    config_item_put(cm_default); >>> + >>> +    return color_match; >>> +} >>> + >>>   static ssize_t uvcg_format_bma_controls_show(struct uvcg_format >>> *f, char *page) >>>   { >>>       struct f_uvc_opts *opts; >>> @@ -1561,8 +1584,15 @@ static struct config_group >>> *uvcg_uncompressed_make(struct config_group *group, >>>           'Y',  'U',  'Y',  '2', 0x00, 0x00, 0x10, 0x00, >>>            0x80, 0x00, 0x00, 0xaa, 0x00, 0x38, 0x9b, 0x71 >>>       }; >>> +    struct uvcg_color_matching *color_match; >>> +    struct config_item *streaming; >>>       struct uvcg_uncompressed *h; >>>   +    streaming = group->cg_item.ci_parent; >>> +    color_match = uvcg_format_get_default_color_match(streaming); >>> +    if (!color_match) >>> +        return ERR_PTR(-EINVAL); >>> + >>>       h = kzalloc(sizeof(*h), GFP_KERNEL); >>>       if (!h) >>>           return ERR_PTR(-ENOMEM); >>> @@ -1580,6 +1610,8 @@ static struct config_group >>> *uvcg_uncompressed_make(struct config_group *group, >>>         INIT_LIST_HEAD(&h->fmt.frames); >>>       h->fmt.type = UVCG_UNCOMPRESSED; >>> +    h->fmt.color_matching = color_match; >>> +    color_match->refcnt++; >> Does this need to be protected by a lock ? I suppose it may not matter >> too much for the default as it will never be dropped, but it would still >> be nice to get locking right overall. > > > I don't think the locking is necessary here as in practice it's done > by configfs core when you symlink between the config items. The refcnt > here is only being used to protect a color matching descriptor's > attributes from being changed when it's already been linked to a > format, we rely on core to protect the descriptor from being dropped > if it's in use somewhere. > >> >> I'm tempted to increase the refcnt in >> uvcg_format_get_default_color_match(), as function that look up and >> return a pointer to a refcounted object should take a reference. That's >> not a hard requirement here if it makes the rest of the code too >> complex. Sorry forgot to respond here; in fact it does take a reference the the colour matching descriptors config_item, through config_group_find_item() in configfs core. >> >>> config_group_init_type_name(&h->fmt.group, name, >>>                       &uvcg_uncompressed_type); >>>   @@ -1744,8 +1776,15 @@ static const struct config_item_type >>> uvcg_mjpeg_type = { >>>   static struct config_group *uvcg_mjpeg_make(struct config_group >>> *group, >>>                              const char *name) >>>   { >>> +    struct uvcg_color_matching *color_match; >>> +    struct config_item *streaming; >>>       struct uvcg_mjpeg *h; >>>   +    streaming = group->cg_item.ci_parent; >>> +    color_match = uvcg_format_get_default_color_match(streaming); >>> +    if (!color_match) >>> +        return ERR_PTR(-EINVAL); >>> + >>>       h = kzalloc(sizeof(*h), GFP_KERNEL); >>>       if (!h) >>>           return ERR_PTR(-ENOMEM); >>> @@ -1761,6 +1800,8 @@ static struct config_group >>> *uvcg_mjpeg_make(struct config_group *group, >>>         INIT_LIST_HEAD(&h->fmt.frames); >>>       h->fmt.type = UVCG_MJPEG; >>> +    h->fmt.color_matching = color_match; >>> +    color_match->refcnt++; >>>       config_group_init_type_name(&h->fmt.group, name, >>>                       &uvcg_mjpeg_type); >>>   @@ -1909,7 +1950,8 @@ static inline struct uvc_descriptor_header >>>   enum uvcg_strm_type { >>>       UVCG_HEADER = 0, >>>       UVCG_FORMAT, >>> -    UVCG_FRAME >>> +    UVCG_FRAME, >>> +    UVCG_CMD, >> s/UVCG_CMD/UVCG_COLOR_MATCHING/ >> >>>   }; >>>     /* >>> @@ -1959,6 +2001,10 @@ static int __uvcg_iter_strm_cls(struct >>> uvcg_streaming_header *h, >>>               if (ret) >>>                   return ret; >>>           } >>> + >>> +        ret = fun(f->fmt->color_matching, priv2, priv3, 0, UVCG_CMD); >>> +        if (ret) >>> +            return ret; >>>       } >>>         return ret; >>> @@ -2014,6 +2060,12 @@ static int __uvcg_cnt_strm(void *priv1, void >>> *priv2, void *priv3, int n, >>>           *size += frm->frame.b_frame_interval_type * sz; >>>       } >>>       break; >>> +    case UVCG_CMD: { >>> +        struct uvcg_color_matching *color_match = priv1; >>> + >>> +        *size += sizeof(color_match->desc); >>> +    } >>> +    break; >>>       } >>>         ++*count; >>> @@ -2099,6 +2151,13 @@ static int __uvcg_fill_strm(void *priv1, void >>> *priv2, void *priv3, int n, >>>                   frm->frame.b_frame_interval_type); >>>       } >>>       break; >>> +    case UVCG_CMD: { >>> +        struct uvcg_color_matching *color_match = priv1; >>> + >>> +        memcpy(*dest, &color_match->desc, sizeof(color_match->desc)); >>> +        *dest += sizeof(color_match->desc); >>> +    } >>> +    break; >>>       } >>>         return 0; >>> @@ -2138,7 +2197,7 @@ static int >>> uvcg_streaming_class_allow_link(struct config_item *src, >>>       if (ret) >>>           goto unlock; >>>   -    count += 2; /* color_matching, NULL */ >>> +    count += 1; /* NULL */ >>>       *class_array = kcalloc(count, sizeof(void *), GFP_KERNEL); >>>       if (!*class_array) { >>>           ret = -ENOMEM; >>> @@ -2165,7 +2224,6 @@ static int >>> uvcg_streaming_class_allow_link(struct config_item *src, >>>           kfree(data_save); >>>           goto unlock; >>>       } >>> -    *cl_arr = (struct uvc_descriptor_header >>> *)&opts->uvc_color_matching; >>>         ++target_hdr->linked; >>>       ret = 0; >>> diff --git a/drivers/usb/gadget/function/uvc_configfs.h >>> b/drivers/usb/gadget/function/uvc_configfs.h >>> index c7392c9b840e..174ee691302b 100644 >>> --- a/drivers/usb/gadget/function/uvc_configfs.h >>> +++ b/drivers/usb/gadget/function/uvc_configfs.h >>> @@ -52,12 +52,13 @@ enum uvcg_format_type { >>>   }; >>>     struct uvcg_format { >>> -    struct config_group    group; >>> -    enum uvcg_format_type    type; >>> -    unsigned        linked; >>> -    struct list_head    frames; >>> -    unsigned        num_frames; >>> -    __u8            bmaControls[UVCG_STREAMING_CONTROL_SIZE]; >>> +    struct config_group        group; >>> +    enum uvcg_format_type        type; >>> +    unsigned            linked; >>> +    struct list_head        frames; >>> +    unsigned            num_frames; >>> +    __u8 bmaControls[UVCG_STREAMING_CONTROL_SIZE]; >>> +    struct uvcg_color_matching    *color_matching; >>>   }; >>>     struct uvcg_format_ptr {