linux-kernel - Re: [PATCH v2] usb: gadget: uvc: Add H264 frame format support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6ec972b0-a971-4d35-9d8f-4a0fe3660514@quicinc.com>
Date: Mon, 26 Aug 2024 21:00:04 +0530
From: AKASH KUMAR <quic_akakum@...cinc.com>
To: Michael Riesch <michael.riesch@...fvision.net>,
        Greg Kroah-Hartman
	<gregkh@...uxfoundation.org>,
        Daniel Scally <dan.scally@...asonboard.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>
CC: Thinh Nguyen <Thinh.Nguyen@...opsys.com>, Felipe Balbi <balbi@...nel.org>,
        Jack Pham <quic_jackp@...cinc.com>, <kernel@...cinc.com>,
        Wesley Cheng
	<quic_wcheng@...cinc.com>,
        Vijayavardhan Vennapusa
	<quic_vvreddy@...cinc.com>,
        Krishna Kurapati <quic_kriskura@...cinc.com>,
        <linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] usb: gadget: uvc: Add H264 frame format support

Hi Michael,

On 8/26/2024 3:06 PM, Michael Riesch wrote:
> Hi Akash,
>
> Thanks for your patches. Very interesting to see H.264 UVC gadget
> support coming to life. May I ask how you tested your patches? What does
> the user space stack on the gadget side look like? And what is the USB
> host in your setup (OS, application, ...)?
We have added support in our application code which opens uvc node 
during enumeration,
e2e application POTPLAYER can be used to test which has h264 decoder 
support, with my changes
you can see in device setting option along with mjpeg,uncompressed , 
h264 and all frames and fps
which we can be added in any init service, which can create and populate 
uvc properties.
You need to create sysfs entries as shared in Documentation,
With format names, driver creates parameters and we need to populate all 
paramters which are writable.
we have validated in linux, android and ubuntu OS.
>
> On 8/1/24 08:15, AKASH KUMAR wrote:
>> Hi Greg,Daniel,Laurent,
>>
>> On 7/11/2024 3:13 PM, AKASH KUMAR wrote:
>>> On 7/11/2024 2:37 PM, Greg Kroah-Hartman wrote:
>>>> On Thu, Jul 11, 2024 at 01:53:04PM +0530, Akash Kumar wrote:
>>>>> Add support for framebased frame format which can be used to support>>>> multiple formats like H264 or H265 other than mjpeg and YUV frames.
>>>>>
>>>>> Framebased format is set to H264 by default, which can be updated to
>>>>> other formats by updating the GUID through guid configfs attribute.
>>>>> Using Different structures for all 3 formats as H264 has different
>>>>> structure than mjpeg and uncompressed which will be paased to
>>>>> frame make func based on active format instead of common frame
>>>>> structure, have updated all apis in driver accordingly.
>>>>> h264 is not recognized by hosts machine during enumeration
>>>>> with common frame structure, so we need to pass h264 frame
>>>>> structure separately.
>>>>>
>>>>> Signed-off-by: Akash Kumar <quic_akakum@...cinc.com>
>>>>> ---
>>>>>    .../ABI/testing/configfs-usb-gadget-uvc       |  88 ++-
>>>>>    drivers/usb/gadget/function/uvc_configfs.c    | 570
>>>>> +++++++++++++++---
>>>>>    drivers/usb/gadget/function/uvc_configfs.h    |  34 +-
>>>>>    drivers/usb/gadget/function/uvc_v4l2.c        |  80 ++-
>>>>>    include/uapi/linux/usb/video.h                |  62 ++
>>>>>    5 files changed, 714 insertions(+), 120 deletions(-)
>>>>>
>>>>> Changes for v2:
>>>>> - Added H264 frame format Details in Documentation/ABI/
>>>>>     and new configsfs attribute path for mjpeg and
>>>>>     uncompresseed formats.
>>>>>
>>>>> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc
>>>>> b/Documentation/ABI/testing/configfs-usb-gadget-uvc
>>>>> index 4feb692c4c1d..2580083cdcc5 100644
>>>>> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
>>>>> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
>>>>> @@ -224,13 +224,13 @@ Description:    Additional color matching
>>>>> descriptors
>>>>>                          white
>>>>>            ========================
>>>>> ======================================
>>>>>    -What: /config/usb-gadget/gadget/functions/uvc.name/streaming/mjpeg
>>>>> -Date:        Dec 2014
>>>>> +What:
>>>>> /config/usb-gadget/gadget/functions/uvc.name/streaming/mjpeg/name
>>>> You are changing an existing api, how will all existing code handle
>>>> this? Will it not break? What is ensuring that this will work as-is ok?
>>>> I have modified all existing apis in kernel and have handled it and
>>>> all existing formats
>>> are working along with H264 in this change. Only user needs to change
>>> configfs parameter
>>> path according to updated path in documentation in Userspace.Currently
>>> H264 doesn't work with same
>>> structure and we need add it differently as a result these configfs
>>> paths are getting updated.
>>> Daniel and Laurent can you suggest if it ok?
>>>>> -#define UVCG_FRAME_ATTR(cname, aname, bits) \
>>>>> -static ssize_t uvcg_frame_##cname##_show(struct config_item *item,
>>>>> char *page)\
>>>>> +#define UVCG_FRAME_ATTR(cname, fname, bits) \
>>>>> +static ssize_t uvcg_frame_##fname##_##cname##_show(struct
>>>>> config_item *item, char *page)\
>>>>>    {                                    \
>>>>>        struct uvcg_frame *f = to_uvcg_frame(item);            \
>>>>>        struct f_uvc_opts *opts;                    \
>>>>> @@ -1936,14 +1941,14 @@ static ssize_t
>>>>> uvcg_frame_##cname##_show(struct config_item *item, char *page)\
>>>>>        opts = to_f_uvc_opts(opts_item);                \
>>>>>                                        \
>>>>>        mutex_lock(&opts->lock);                    \
>>>>> -    result = sprintf(page, "%u\n", f->frame.cname);            \
>>>>> +    result = scnprintf(page, PAGE_SIZE, "%u\n",
>>>>> f->frame.fname.cname);\
>>>> sysfs_emit() is made for this.
>>> Sure, will change.
>>>
>>>
>> can you suggest how to support H264 format without changing userspace
>> nodes,
>> as H264 format structure is different from mjpeg and uncompressed format
>> and
>> using same structure show issue as host is not able to recognize H264
>> format frames.
>>
>> Thanks,
>> Akash
>>
> After only a quick look at the USB Video Payload H264 1.5 document, I
> think there should be a folder
>    /config/usb-gadget/gadget/functions/uvc.name/streaming/h264/
> with the formats
>    /config/usb-gadget/gadget/functions/uvc.name/streaming/h264/name
> and the frames
>    /config/usb-gadget/gadget/functions/uvc.name/streaming/h264/name/name
> in analogy to the other payloads, namely mjpeg and uncompressed.
> Naturally, the attributes will be different to the existing formats.
>
> What exactly does not work with this approach?
We need to add additional name for mjpeg and uncompressed due to 
different structure
as we cannot support all 3 formats with same frame struct type.
This name can be any dummy word and formats are added inside this dummy 
name instead
of mjpeg and uncompressed.
>
> Best regards,
> Michael
Thanks,
Akash