Message-ID: <378af3f1-b5b0-4f7a-ab62-f5c891feb7b5@quicinc.com>
Date: Tue, 1 Apr 2025 16:47:41 -0700
From: Wesley Cheng <quic_wcheng@...cinc.com>
To: Stephan Gerhold <stephan.gerhold@...aro.org>
CC: <srinivas.kandagatla@...aro.org>, <mathias.nyman@...el.com>,
        <perex@...ex.cz>, <conor+dt@...nel.org>, <dmitry.torokhov@...il.com>,
        <corbet@....net>, <broonie@...nel.org>, <lgirdwood@...il.com>,
        <krzk+dt@...nel.org>, <pierre-louis.bossart@...ux.intel.com>,
        <Thinh.Nguyen@...opsys.com>, <tiwai@...e.com>, <robh@...nel.org>,
        <gregkh@...uxfoundation.org>, <linux-kernel@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-sound@...r.kernel.org>,
        <linux-input@...r.kernel.org>, <linux-usb@...r.kernel.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-doc@...r.kernel.org>,
        Luca Weiss
	<luca.weiss@...rphone.com>
Subject: Re: [PATCH v36 22/31] ASoC: qcom: qdsp6: Introduce USB AFE port to
 q6dsp

Hi Stephan,

On 4/1/2025 1:16 AM, Stephan Gerhold wrote:
> Hi Wesley,
> 
> On Mon, Mar 31, 2025 at 12:52:19PM -0700, Wesley Cheng wrote:
>> On 3/26/2025 2:57 AM, Stephan Gerhold wrote:
>>> On Tue, Mar 25, 2025 at 04:18:03PM -0700, Wesley Cheng wrote:
>>>> On 3/25/2025 2:24 AM, Stephan Gerhold wrote:
>>>>> On Tue, Mar 18, 2025 at 05:51:32PM -0700, Wesley Cheng wrote:
>>>>>> The QC ADSP is able to support USB playback endpoints, so that the main
>>>>>> application processor can be placed into lower CPU power modes.  This adds
>>>>>> the required AFE port configurations and port start command to start an
>>>>>> audio session.
>>>>>>
>>>>>> Specifically, the QC ADSP can support all potential endpoints that are
>>>>>> exposed by the audio data interface.  This includes isochronous data
>>>>>> endpoints, in either synchronous mode or asynchronous mode. In the latter
>>>>>> case both implicit or explicit feedback endpoints are supported.  The size
>>>>>> of audio samples sent per USB frame (microframe) will be adjusted based on
>>>>>> information received on the feedback endpoint.
>>>>>>
>>>>>> Some pre-requisites are needed before issuing the AFE port start command,
>>>>>> such as setting the USB AFE dev_token.  This carries information about the
>>>>>> available USB SND cards and PCM devices that have been discovered on the
>>>>>> USB bus.  The dev_token field is used by the audio DSP to notify the USB
>>>>>> offload driver of which card and PCM index to enable playback on.
>>>>>>
>>>>>> Signed-off-by: Wesley Cheng <quic_wcheng@...cinc.com>
>>>>>> ---
>>>>>>  sound/soc/qcom/qdsp6/q6afe-dai.c         |  60 +++++++
>>>>>>  sound/soc/qcom/qdsp6/q6afe.c             | 192 ++++++++++++++++++++++-
>>>>>>  sound/soc/qcom/qdsp6/q6afe.h             |  36 ++++-
>>>>>>  sound/soc/qcom/qdsp6/q6dsp-lpass-ports.c |  23 +++
>>>>>>  sound/soc/qcom/qdsp6/q6dsp-lpass-ports.h |   1 +
>>>>>>  sound/soc/qcom/qdsp6/q6routing.c         |  32 +++-
>>>>>>  6 files changed, 341 insertions(+), 3 deletions(-)
>>>>>>
>>>> [...]
>>>>>> diff --git a/sound/soc/qcom/qdsp6/q6routing.c b/sound/soc/qcom/qdsp6/q6routing.c
>>>>>> index 90228699ba7d..b7439420b425 100644
>>>>>> --- a/sound/soc/qcom/qdsp6/q6routing.c
>>>>>> +++ b/sound/soc/qcom/qdsp6/q6routing.c
>>>>>> @@ -435,6 +435,26 @@ static struct session_data *get_session_from_id(struct msm_routing_data *data,
>>>>>>  	return NULL;
>>>>>>  }
>>>>>> +
>>>>>> +static bool is_usb_routing_enabled(struct msm_routing_data *data)
>>>>>> +{
>>>>>> +	int i;
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * Loop through current sessions to see if there are active routes
>>>>>> +	 * to the USB_RX backend DAI.  The USB offload routing is designed
>>>>>> +	 * similarly to the non offload path.  If there are multiple PCM
>>>>>> +	 * devices associated with the ASoC platform card, only one active
>>>>>> +	 * path can be routed to the USB offloaded endpoint.
>>>>>> +	 */
>>>>>> +	for (i = 0; i < MAX_SESSIONS; i++) {
>>>>>> +		if (data->sessions[i].port_id == USB_RX)
>>>>>> +			return true;
>>>>>> +	}
>>>>>> +
>>>>>> +	return false;
>>>>>> +}
>>>>>
>>>>> What is different about USB_RX compared to other output ports we have in
>>>>> Q6AFE? Obviously, we can only play one stream on an output port. But
>>>>> doesn't the ADSP mix streams together when you have multiple routes?
>>>>>
>>>>
>>>> This patch will limit the USB_RX from being able to be mixed to multiple
>>>> q6adm paths.
>>>>
>>>>> Also, this doesn't actually check for *active* routes only. It just
>>>>> looks if any other MultiMedia DAI is configured to output to USB_RX.
>>>>> That doesn't mean they will ever be active at the same time.
>>>>>
>>>>
>>>> Yes, the main reason being that that is the mechanism we use to populate
>>>> the active offload path within the USB SND card mixer.
>>>>
>>>>> I might for example want to have MultiMedia1 and MultiMedia2 both
>>>>> configured to output to USB_RX. Let's assume MultiMedia1 is a normal PCM
>>>>> DAI, MultiMedia2 is a compress offload DAI. When I want to playback
>>>>> normal audio, I go through MultiMedia1, when I want to play compressed
>>>>> audio, I go through MultiMedia2. Only one of them active at a time.
>>>>> Why can't I set this up statically in the mixers?
>>>>>
>>>>> If you confirm that it is really impossible to have multiple streams
>>>>> mixed together to the USB_RX output in the ADSP, then this should be a
>>>>> runtime check instead when starting the stream IMO.
>>>>>
>>>>
>>>> We can have multiple streams being mixed together, but it will get
>>>> confusing because it changes the definition that we had discussed about in
>>>> the past about the overall design for the interaction w/ userspace.
>>>> Although we (QC) only support a single USB audio device for offloading,
>>>> there could be other situations where the audio DSP can support multiple
>>>> devices.  The assumption is that each MM path is assigned to a USB device.
>>>>
>>>
>>> Are you referring to the "USB Offload Playback Route PCM#*" mixers here?
>>> They could just refer to first of the configured MM paths, if someone
>>> decides to route multiple paths to the USB backend. Looking at
>>> q6usb_update_offload_route(), I think the implementation does that
>>> already.
>>>
>>> I think it's fine that the userspace API for automatically "probing" the
>>> PCM device supports only a single path to the USB backend. But if
>>> someone wants to bypass the automatic probing and configure a more
>>> advanced setup, do we need to forbid that?
>>>
>>> Asked differently: what would happen if we remove this check here and
>>> handle USB_RX like any other Q6AFE output port? Would anything break for
>>> the userspace interface?
>>>
>>
>> So I took a look at seeing how the Q6ADM/ASM interactions would work for
>> the situation where if user tried to start both MM1/2 streams at the same
>> time over the USB offload path.  In this scenario, we see that the Q6USB BE
>> DAI operations, ie startup, hw_params, etc... gets called one time for the
>> initial stream.  For example, if I start playback on MM1, then that
>> triggers the USB BE DAI to be brought up.
>>
>> When I start playback on MM2, since MM1 already called
>> dpcm_be_dai_startup(), then be->dpcm[stream].users will be greater than
>> zero.  This would cause the __soc_pcm_open() to be skipped for the USB BE
>> DAI, so I wouldn't be able to check the runtime status at the Q6USB backend
>> DAI.  However, we do track current streaming sessions done over Q6 ADM and
>> it does save the AFE port associated to each COPP allocation, so I think its
>> reasonable to see if there is already a COPP entry for the USB AFE port, to
>> fail the open() call associated to the FE DAI.
>>
> 
> This sounds like a reasonable approach *if* we have to prevent multiple
> MM DAIs from streaming to the USB AFE port at the same time.
> 
> It's still unclear to me why we have to introduce this limitation in the
> first place. I think the questions from my previous email are still
> open. Can you check them again?
> 

So I checked with our audio DSP folks, and they mentioned there isn't
technically a limitation from mixing multiple ADM streams from their end.
My observations are as follows:
- Using tinyplay to open and play on different FE PCM devices (ie MM1 and
MM2), both streams are audible on the USB headset (intermixed).
- If starting playback on MM1 first, before MM2, then once playback is
complete on MM1, the ADM close is also affecting the MM2 stream.
Basically, MM2 stops when the MM1 audio file is completed.
- Due to the abrupt/incomplete closing of the MM2 ADM stream, looks like
the audio DSP is not handling that case well, so subsequent playbacks fail.

I did find a possible reason for this, and it seems to be due to some code
unrelated to the USB offload path directly.  It looks like the Q6ADM is
currently built in a way that you can only route streams to a single
endpoint, even though we do have reference counting for each COPP profile.
So even after the first MM1 ADM stream completes and the PCM device is
closed, the MM1 ADM close callback will issue a q6adm_device_close() for
the USB AFE port.

I made some test changes to account for the refcount before issuing the
q6adm_device_close(), and that seemed to work.  Once the MM1 stream closes,
it allows for the MM2 stream to close/finish before issuing the command,
and that allows for proper cleanup of the audio data.

IMO, I would like to keep the initial behavior (ie, blocking the additional
stream open from the kernel) until I can get some more testing done, and
figure out if this is the correct approach.  If it is, I can submit a
follow up series to address it.

Thanks
Wesley Cheng

Powered by blists - more mailing lists