Re: [PATCHv2 3/3] usb: gadget: f_uac*: Support multiple sampling rates

[Eugeniu Rosca]

On Tue, Jul 03, 2018 at 01:29:54AM +0300, Ruslan Bilovol wrote:
> Hi guys,
> 
> On Mon, Jul 2, 2018 at 7:30 PM, Eugeniu Rosca <erosca@xxxxxxxxxxxxxx> wrote:
> > Hi Julian,
> >
> > [CC:Takashi, since we are discussing sound-related parts of USB]
> >
> > On Mon, Jul 02, 2018 at 04:07:19PM +0200, Julian Scheel wrote:
> >> Hi Eugeniu,
> >>
> >> On 30.06.2018 20:16, Eugeniu Rosca wrote:
> >> > Would it be possible to revive the uac2 multiple sampling rate
> >> > patch-set [1] by rebasing it onto the most recent kernel? If you
> >> > don't have time for this, I could help you.
> >>
> >> I have this on my todo-list for a while now... In fact I fixed the build
> >> errors reported by the robot last year, but didn't had the time to
> >> verify all of it and push again.
> >> Still, I'd be happy to get this merged, so I'll try to check the state
> >> of this within the next days and either post to the list or get back to
> >> you if more work needs to be done.
> >
> > We've been living with an internally developed uac2 multiple rate
> > support since June 2015, initially written on top of v3.14. Due to
> > significant refactoring of uac2 driver brought by v4.13 commit [1], I
> > went through the comparison between the in-house implementation (which
> > no more applied cleanly to post-[1] vanilla) and your proposal from [2].
> >
> 
> When Julian posted his patches, I've been working on UAC3 gadget
> implementation which I posted later [1]. While I originally tried to make
> UAC3 function to have same configfs files as UAC1/2, now, preparing
> UAC3 gadget patch I see it doesn't fit this approach, and patch
> "usb: gadget: f_uac*: Reduce code duplication" isn't applicable for UAC3
> case. Especially for channels configuration which in new spec can be
> done only through clusters descriptions, which makes configfs interface
> more complicated and not so straightforward as UAC1/2 have.
> 
> I didn't finish my UAC3 gadget patch v2 yet, but if you can try to avoid
> adding patch "Reduce code duplication" or wait for a few weeks, when
> I'll post UAC3 patches, it would be great; so we will be able to take into
> account new spec as well.
> 
> [1]: https://lkml.org/lkml/2017/11/6/1514

Hello Ruslan, hello Julian,

I took some time to study the implementations of existing uac drivers,
including uac3 v1 [1] and would like to share some thoughts with you.

My main thesis is that, w/o going too deep into the semantics and specs,
there is just a lot of code shared between the drivers. Why I am
pointing this out is because features like [2] and possibly a number of
other upcoming features which are agnostic on the exact UAC spec (just
like multiple rate support is) will need a copy-paste (and individual
maintenance) in every single UAC{1,2,3} driver.

To quantify the degree of similarity (the percentage of common
code), I took the uac2 driver as reference and compared it to uac1
and to uac3 implementations.

Some comments/assumptions:
 - To get relevant diff output between the drivers, I needed to reorder
   the functions some *.c files slightly (this already can be a source
   of bugs, since contributors can't see the difference between the
   drivers clearly).
 - To highlight as much common code as possible, I needed to reorder
   statements inside functions (some functions performing the same role
   are doing things in slightly different order, which again can be
   source of subtle bugs).
 - I used v4.18-rc3, on top of which patch [1] applies cleanly.
 - I only compared *.c code (functions and macros), not data structs.
 - I ignored the differences in the names of local variables.
 - I ignored the differences in the prints.
 - I considered 'struct f_uac{1,2,3}_opts' identical, since they
   really are.
 - I ignored the difference between 'struct f_uac{1,2,3}' since they
   can be easily unified.
 - I ignored the values behind the 'UAC{1,2,3}_DEF_*' macros, since they
   are not essential.
 - Obviously, I ignored any whitespace difference.
 - I paired the functions/macros by role.
 - The actual % values are estimated *visually*, therefore might be not
   precise (unless it is a 0% or 100%).

The results are drawn in the following two tables.
 
 ###### UAC2 vs UAC1

 | uac2                   | %   | uac1                   |
 |------------------------|-----|------------------------|
 | afunc_bind             | 50  | f_audio_bind           |
 | afunc_unbind           | 100 | f_audio_unbind         |
 | afunc_set_alt          | 100 | f_audio_set_alt        |
 | afunc_get_alt          | 100 | f_audio_get_alt        |
 | afunc_disable          | 100 | f_audio_disable        |
 | in_rq_cur              | 0   | -                      |
 | in_rq_range            | 0   | -                      |
 | out_rq_cur             | 0   | -                      |
 | ac_rq_in               | 0   | -                      |
 | setup_rq_inf           | 0   | -                      |
 | afunc_setup            | 50  | f_audio_setup          |
 | to_f_uac2_opts         | 100 | to_f_uac1_opts         |
 | f_uac2_attr_release    | 100 | f_uac1_attr_release    |
 | UAC2_ATTRIBUTE         | 100 | UAC1_ATTRIBUTE         |
 | afunc_free_inst        | 100 | f_audio_free_inst      |
 | afunc_alloc_inst       | 100 | f_audio_alloc_inst     |
 | afunc_free             | 100 | f_audio_free           |
 | afunc_alloc            | 100 | f_audio_alloc          |
 | set_ep_max_packet_size | 0   | -                      |
 | -                      | 0   | audio_get_endpoint_req |
 | -                      | 0   | audio_set_endpoint_req |

 ###### UAC2 vs UAC3

 | uac2                   | %   | uac3                     |
 |------------------------|-----|--------------------------|
 | afunc_bind             | 50  | f_audio_bind             |
 | afunc_unbind           | 50  | f_audio_unbind           |
 | afunc_set_alt          | 100 | f_audio_set_alt          |
 | afunc_get_alt          | 100 | f_audio_get_alt          |
 | afunc_disable          | 100 | f_audio_disable          |
 | in_rq_cur              | 30  | in_rq_cur                |
 | in_rq_range            | 30  | in_rq_range              |
 | out_rq_cur             | 20  | out_rq_cur               |
 | ac_rq_in               | 50  | ac_rq_in                 |
 | setup_rq_inf           | 90  | setup_rq_inf             |
 | afunc_setup            | 100 | f_audio_setup            |
 | to_f_uac2_opts         | 100 | to_f_uac3_opts           |
 | f_uac2_attr_release    | 100 | f_uac3_attr_release      |
 | UAC2_ATTRIBUTE         | 100 | UAC3_ATTRIBUTE           |
 | afunc_free_inst        | 100 | f_audio_free_inst        |
 | afunc_alloc_inst       | 100 | f_audio_alloc_inst       |
 | afunc_free             | 100 | f_audio_free             |
 | afunc_alloc            | 100 | f_audio_alloc            |
 | set_ep_max_packet_size | 100 | set_ep_max_packet_size   |
 | -                      | 0   | in_rq_hc_desc            |
 | -                      | 0   | uac3_copy_descriptors    |
 | -                      | 0   | alloc_fu_desc            |
 | -                      | 0   | build_cluster_descriptor |
 | -                      | 0   | UAC3_ATTRIBUTE_CHMASK    |

The message I wanted to share here is that, in my opinion, there is
really a lot of common code across UAC drivers. AFAIK features like
multiple sampling rate support really don't care about the exact UAC
spec, which means that w/o any attempt to reduce code duplication, we
would end up with duplicated (and diverging over time) implementations
of such features in several places.

Can I have your view on that?
Since you likely have more experience with uac* code, do you think is
feasible to isolate the common parts into a separate file?

Thanks,
Eugeniu.

[1] https://lkml.org/lkml/2017/11/6/1514 ("[PATCH 1/1]
    usb: gadget: add USB Audio Device Class 3.0 gadget support")
[2] https://lkml.org/lkml/2017/6/30/276 ("[PATCHv2 0/3]
    USB Audio Gadget: Support multiple sampling rates")
[3] https://lkml.org/lkml/2017/6/30/270 ("[PATCHv2 2/3]
    usb: gadget: f_uac*: Reduce code duplication")