Re: [PATCH v3 00/28] Introduce QC USB SND audio offloading support

[Wesley Cheng <quic_wcheng@quicinc.com>]

Hi Pierre,

On 3/13/2023 7:22 PM, Pierre-Louis Bossart wrote:
> 
> <snip>
> 
>>>
>>>>
>>>>        USB                          |            ASoC
>>>> --------------------------------------------------------------------
>>>>                                     |  _________________________
>>>>                                     | |sm8250 platform card     |
>>>>                                     | |_________________________|
>>>>                                     |         |           |
>>>>                                     |      ___V____   ____V____
>>>>                                     |     |Q6USB   | |Q6AFE    |  #5
>>>>                                     |     |"codec" | |"cpu"    |
>>>>                                     |     |________| |_________|
>>>>                                     |         ^
>>>>                                     |         |  #6
>>>>                                     |      ___V____
>>>>                                     |     |SOC-USB |
>>>>     ________   #1  ________         #7    |        |
>>>>    |USB SND |<--->|QC offld|<------------>|________|
>>>>    |(card.c)|     |        |<----------       ^
>>>>    |________|     |________|___ #4  | |       |
>>>>        ^               ^       |    | |    ___V__________________
>>>>        | #2            |  #2   |    | |   |APR/GLINK             |
>>>>     __ V_______________V_____  |    | |   |______________________|
>>>>    |USB SND (endpoint.c)     | |    | |              ^
>>>>    |_________________________| |    | | #8           |
>>>>                ^               |    | |   ___________V___________
>>>>                | #3            |    | |->|audio DSP              |
>>>>     ___________V_____________  |    |    |_______________________|
>>>>    |XHCI HCD                 |<-    |
>>>>    |_________________________|      |
>>>>
>>>>
>>>> #1 - USB SND and QC offload:
>>>> Initialization:
>>>> - Register platform operations, to receive connect/disconnect events
>>>>     from USB SND.
>>>> - QC offload creates a QMI handle, in order to receive QMI requests
>>>>     from the audio DSP.
>>>>
>>>> Runtime:
>>>> - USB SND passes along "struct snd_usb_audio" in order for QC offload
>>>>     to reference USB UAC desc parsing/USB SND helper APIs.
>>>> - USB device disconnection events will result in clearing of the chip
>>>>     entry.
>>>>
>>>> #2 - USB SND and QC offload endpoints:
>>>> Runtime:
>>>> - In the non-offloaded path, USB snd will utilize functions exposed by
>>>>     USB SND endpoint, to help with fetching USB EP references and queuing
>>>>     URBs.
>>>> - In the offload path, qc offload will utilize the functions to fetch
>>>>     USB EP references, so that it can use that information to query the
>>>>     XHCI HCD.
>>>> - Likewise, both will clean up endpoints when audio stream is not in
>>>> use.
>>>>
>>>> #3 - XHCI HCD:
>>>> Initialization:
>>>> - During XHCI probe timing, when the USB HCD is added to the system, it
>>>>     will also initialize the secondary event rings.
>>>>
>>>> Runtime:
>>>> - During USB device plug ins/outs, allocates device slot, assigns eps,
>>>>     and initializes transfer rings.
>>>>
>>>> #4 - QC offload and XHCI:
>>>> Runtime:
>>>> - QC offload needs to reference the transfer ring and secondary event
>>>> ring
>>>>     addresses by executing XHCI offload management APIs.
>>>> - This happens when audio DSP receives a USB QMI stream request.
>>>>
>>>> #5 - ASoC components:
>>>> Initialization:
>>>> - The SM8250 platform sound card driver fetches DT node entries defining
>>>>     the ASoC links. This chain/link has the components involved for a
>>>>     particular Q6AFE path. (not only USB offload)
>>>>       - "cpu" - this is the ASoC CPU DAI that handles interaction with
>>>> the
>>>>                 Q6 DSP's audio protocol. (AFE ports)
>>>>       - "codec" - the ASoC codec (backend) DAI defined
>>>> - Registers ASoC platform sound card based on links defined in the DT
>>>> node.
>>>>     - Probes DAI components involved, ie Q6USB and Q6AFE
>>>>
>>>> Runtime:
>>>> - Q6AFE has the bulk of the interaction w/ the audio DSP to start an
>>>> audio
>>>>     session, such as issuing AFE port start commands (part of the
>>>> protocol
>>>>     used to communicate the audio session info)
>>>> - Q6USB will be there to now check for if format requested is
>>>> supported by
>>>>     the device, and maintain offloading status.
>>>>
>>>> #6 - Q6USB and SOC-USB:
>>>> Initialization:
>>>> - Q6USB will query QC offload for USB device connection states. (through
>>>>     soc-usb)
>>>> - Creates a SOC USB entry, that carries information about resources,
>>>>     such as audio DSP memory information and requested XHCI event ring
>>>>     index.
>>>>
>>>> Runtime:
>>>> - SOC-USB will receive connect/disconnect events and propagate to Q6USB.
>>>>     - Q6USB makes devices available for offloading based on these events.
>>>> - Sets Q6AFE port configurations to select the USB SND card# and PCM#.
>>>>
>>>> #7 - SOC-USB and QC offload:
>>>> Initialization:
>>>> - Rediscover USB SND devices when the SOC-USB entry is created (if
>>>> needed)
>>>>       - For situations where the Q6USB DAI hasn't been probed.
>>>>
>>>> Runtime:
>>>> - Propagate connect/disconnect events.
>>>
>>> Is the SOC-USB module or building blocks intended to be generic or
>>> Qualcomm agnostic?
>>>
>>
>> This should be generic.
> 
> ok, but then how would it communicate with APR/GLINK described below [1]

To make the diagram a bit more compact, I left out our Q6ASM DAI, which 
is the path that handles the PCM data/buffers. (not really involved in 
initializing any part of the offload path)

Sorry made a mistake on the diagram.  There is no connection from 
SOC-USB to the APR/GLINK.  The Q6USB driver will be the one that is 
going to configure some of the Q6AFE ports along withe the Q6AFE DAI driver.

|            ASoC
----------------------------------
|  _________________________
| |sm8250 platform card     |
| |_________________________|
|         |           |
|      ___V____   ____V____
|     |Q6USB   | |Q6AFE    |  #5
|     |"codec" | |"cpu"    |
|     |________| |_________|
|         ^  ^        ^
|      #6 |  |________|
|      ___V____     |
|     |SOC-USB |    |
#7    |        |    |
----->|________|    |
---                 |
| |                 |
| |    _____________V________
| |   |APR/GLINK             |
| |   |______________________|
| |              ^
| | #8           |
| |   ___________V___________
| |->|audio DSP              |
|    |_______________________|
|
|

>>
>>> It's not clear to me how it would handle "audio DSP memory information
>>> and requested XHCI event ring index."
>>>
>>
>> Each soc-usb entry that is created by the ASoC DPCM backend DAI (q6usb)
>> will be able to hold "private data" that, in QC case, is defined as:
>> struct q6usb_offload
>>
>> This is passed within the snd_soc_usb_add_port() call:
>> snd_soc_usb_add_port(component->dev, &data->priv,
>> q6usb_alsa_connection_cb);
>>
>> So depending on the user, the private data can contain their own struct
>> with the information they require.
> 
> ok, so "handle private data such as audio DSP memory information
>>> and requested XHCI event ring index" is what you meant. That'd fine.
> 
>>> In addition, it seems to be the "bridge" or means of communication
>>> between qc_audio_offload and q6usb, is this not based on custom events
>>> or triggers?
>>>
>>
>> Ideally, no, it shouldn't be based on custom events.  Intention for the
>> connect_cb() that is defined is just to receive USB device discovery
>> events from USB SND.  From the qc_audio_offload, we call
>> snd_soc_usb_connect() within our platform op that we register to USB SND.
>>
>> //Platform connect_cb() - called from USB SND probe (device connected)
>> static void qc_usb_audio_offload_probe(struct snd_usb_audio *chip)
>> {
>> ...
>> snd_soc_usb_connect(usb_get_usb_backend(udev), chip->card->number,
>>                  chip->index, chip->pcm_devs);
>>
>> In the QC situation, we used this to build a list of active devices
>> connected.
>>
>>> Along the same lines, this SOC-USB entity interfaces with APR/GLINK
>>> which doesn't speak to me so it must be a QCOM interface?>>
>>
>> Sorry for not labeling those in the diagram, but yes, those are QC
>> specific interfaces.  You can just think of it as a type of IPC transport.
> 
> [1] ... something's not clear on how a generic 'soc-usb' component can
> directly talk to a vendor-specific IPC.
> 
> Is there a missing layer?
> 

:) hopefully the updated diagram clarifies this.

>>> I am trying to see if this design could be used for other architectures,
>>> and the QCOM-specific and generic parts are not obvious.
>>>
>>>> #8 - audio DSP and QC offload:
>>>> Runtime:
>>>> - Handle QMI requests coming from audio DSP.  These requests come AFTER
>>>>     the Q6AFE port is opened by the Q6AFE DAI(#6)
>>>> - Returns memory information about resources allocated by XHCI.
>>>> - Enables audio playback when this QMI transaction is completed.
>>>>
>>>>>>
>>>>>>>> When the audio DSP wants to enable a playback stream, the request is
>>>>>>>> first
>>>>>>>> received by the ASoC platform sound card.  Depending on the selected
>>>>>>>> route,
>>>>>>>> ASoC will bring up the individual DAIs in the path.  The Q6USB
>>>>>>>> backend DAI
>>>>>>>> will send an AFE port start command (with enabling the USB playback
>>>>>>>> path), and
>>>>>>>> the audio DSP will handle the request accordingly.
>>>>>>>>
>>>>>>>> Part of the AFE USB port start handling will have an exchange of
>>>>>>>> control
>>>>>>>> messages using the QMI protocol.  The qc_audio_offload driver will
>>>>>>>> populate the
>>>>>>>> buffer information:
>>>>>>>> - Event ring base address
>>>>>>>> - EP transfer ring base address
>>>>>>>>
>>>>>>>> and pass it along to the audio DSP.  All endpoint management will
>>>>>>>> now
>>>>>>>> be handed
>>>>>>>> over to the DSP, and the main processor is not involved in
>>>>>>>> transfers.
>>>>>>>>
>>>>>>>> Overall, implementing this feature will still expose separate sound
>>>>>>>> card and PCM
>>>>>>>> devices for both the platorm card and USB audio device:
>>>>>>>>      0 [SM8250MTPWCD938]: sm8250 - SM8250-MTP-WCD9380-WSA8810-VA-D
>>>>>>>>                           SM8250-MTP-WCD9380-WSA8810-VA-DMIC
>>>>>>>>      1 [Audio          ]: USB-Audio - USB Audio
>>>>>>>>                           Generic USB Audio at
>>>>>>>> usb-xhci-hcd.1.auto-1.4,
>>>>>>>> high speed
>>>>>>>>
>>>>>>>> This is to ensure that userspace ALSA entities can decide which
>>>>>>>> route
>>>>>>>> to take
>>>>>>>> when executing the audio playback.  In the above, if card#1 is
>>>>>>>> selected, then
>>>>>>>> USB audio data will take the legacy path over the USB PCM drivers,
>>>>>>>> etc...
>>>>>>>
>>>>>>> I already voiced my concerns about exposing two cards, each with
>>>>>>> their
>>>>>>> own set of volume controls with the same device. It would be much
>>>>>>> better
>>>>>>> to have an additional offloaded PCM device for card0...
>>>>>>>
>>>>>>> But if the consensus is to have two cards, it's still not clear
>>>>>>> how the
>>>>>>> routing would be selected. In the case where there are two USB audio
>>>>>>> devices attached, the offloaded path would only support one of the
>>>>>>> two.
>>>>>>> How would userspace know which of the two is selected?
>>>>>>>
>>>>>>
>>>>>> With patch#24:
>>>>>> https://lore.kernel.org/linux-usb/20230308235751.495-25-quic_wcheng@quicinc.com/T/#u
>>>>>>
>>>>>> Now, userspace can at least choose which device it wants to offload.
>>>>>> Part of doing that would mean userspace knows what USB SND card
>>>>>> devices
>>>>>> are available, so it is aware of which devices are shared (between the
>>>>>> offload and USB SND path)
>>>>>>
>>>>>>> And how would userspace know the difference anyways between two
>>>>>>> physical
>>>>>>> devices attached to the platform with no offload, and one physical
>>>>>>> device with one additional offload path? The names you selected
>>>>>>> can't be
>>>>>>> used to identify that card1 is the optimized version of card0.
>>>>>>>
>>>>>>
>>>>>> Is userspace currently able to differentiate between cards that are
>>>>>> created by USB SND versus ASoC?  How complex can the userspace card
>>>>>> discovery be?  Can it query kcontrols at this point in time?  If so,
>>>>>> maybe we can change the names of the newly added ones to reflect
>>>>>> that it
>>>>>> is an offload device?
>>>>>>
>>>>>> SND kcontrol names are currently:
>>>>>> Q6USB offload status
>>>>>> Q6USB offload SND device select
>>>>>
>>>>> I must admit I've never seen kcontrols being used to identify what the
>>>>> card is, and in this case it's a pretend-card that's just an improved
>>>>> version of another. It might be easier to use something else, such as
>>>>> the component strings.
>>>>
>>>> Its not exactly a pretend card, right?  This is part of the overall
>>>> platform sound card we have in the system.  At the moment, I'm only
>>>> testing by adding the USB audio routing, but there can be several ASoC
>>>> links defined in the overall platform card.
>>>
>>> Sorry, I misunderstood the proposal. I thought there would be one card
>>> for "generic USB Audio", and another one for "DSP-offloaded USB Audio".
>>> I assumed, probably mistakenly, that all local audio endpoints
>>> (speaker,mics) would be exposed as a separate card.
>>>
>>
>> Ah got it.  No, that isn't the case here.
>>
>>> It looks like it's more "generic USB Audio" and "DSP Audio", with the
>>> USB offload being exposed as a PCM device of the latter.
>>>
>>> Did I get this right? In this case, presumably there can be some sort of
>>
>> Yep that's correct!
> 
> ok, that's good.
> 
> My initial thought was to add a 'DSP offload' PCM to the USB card, you
> added a "USB offload" PCM to the DSP card. Nice logical swap!
> 
> Your proposal might be easier in practice since there's typically a
> vendor-specific configuration file (UCM or custom) file for the DSP,
> where USB information can be added.
> 
> It's more problematic to change a generic USB card as we know it today
> and bolt vendor-specific DSP information on top.
> 
> The only open I have with your option is that there are still two
> control paths to e.g. set the volume. It would be so much easier for
> userspace if there was a single volume control no matter what path is
> used for data, or make sure the kcontrols are 'mirrored' somehow. If we
> found a way to address this issue that would be ideal.
> 

Got it.  Let me look to see if that is something we can address/add.  I 
think the current implementation is that USB SND will expose some mixer 
controls based on the UAC descriptor parsing.  Then when they want to 
change the volume (for example) it will result in a USB SETUP transaction.

So USB SND will eventually be the entity controlling these changes.

>>> UCM file for the "DSP Audio card" that contains the configuration or
>>> knows which kcontrols to look for. But my point about detection hold.
>>> You could perfectly well have a 'Jack control' that tells userspace when
>>> a device is connected. That way there's no guess work, it's similar to
>>> HDMI for Intel: the device is exposed but only valid when the jack
>>> control is set.
>>>
>>
>> Hmm, ok.  Let me see if I can switch up some things.  Maybe replace the
>> current snd_soc_dapm_enable_pin() calls in the q6usb connection_cb and
>> replace that with a snd jack report. (the snd jack implementation
>> already takes care of updating the pin if needed)
> 
> The jack is useful to let userspace know if a PCM device, i.e. a
> Front-End, can be used. But if you expose a PCM device, nothing prevents
> an application from trying to open and use it, we recently had such an
> issue due to a change in PipeWire that tried to open a non-functional
> HDMI device. So you do need something to bail if the PCM device is
> mistakenly used.
> 
> DAPM pin management seems different, it will turn-on/off parts of the
> graph connected to an endpoint. Userpace will typically not know
> anything about pin management, it's an in-kernel concept.
> 
> Not sure if you have to choose, those are two different layers, no?

Will review this more and fix it in the next rev.  Thanks for the inputs!

Thanks
Wesley Cheng