On Tue, Apr 01, 2025 at 04:47:41PM -0700, Wesley Cheng wrote:
On 4/1/2025 1:16 AM, Stephan Gerhold wrote:
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@xxxxxxxxxxx>
---
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.
Thanks a lot for testing this!
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.
Yeah, this sounds like a bug to me...
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.
I'm fine with this approach. My only two concerns/comments are:
1. Let's make sure we don't waste too much time finding the ideal
approach to implement the additional blocking, if this will likely
end up getting removed again anyway.
2. How can we justify making this change only for USB? Your analysis
sounds like the issue applies to all types of output ports. This was
exactly my main concern with the check you added in this patch:
I would expect QDSP6 treats all ports exactly the same. This means
we should add this additional check for all the output ports...
