LKML Archive on
help / color / mirror / Atom feed
From: Pierre-Louis Bossart <>
To: "Péter Ujfalusi" <>,
	"Sameer Pujar" <>,,,,,,,,,,
Subject: Re: [PATCH 01/13] ASoC: soc-pcm: Don't reconnect an already active BE
Date: Wed, 29 Sep 2021 09:52:17 -0500	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

>>> But in addition we'd need to agree on what an 'active BE' is. Why can't
>>> we connect a second stream while the first one is already in HW_PARAMS
>>> or PAUSED or STOP? It's perfectly legal in ALSA/ASoC to have multiple
>>> HW_PARAMS calls, and when we reach STOP we have to do a prepare again.
>>> And more fundamentally, the ability to add a second FE on a 'active' BE
>>> in START state is a basic requirement for a mixer, e.g. to play a
>>> notification on one FE while listening to music on another. What needs
>>> to happen is only to make sure that the FE and BE are compatible in
>>> terms of HW_PARAMS and not sending a second TRIGGER_STOP, only checking
>>> the BE NEW or CLOSE state is way too restrictive.
>> Sorry for the trouble to your system.
>> Idea was to avoid reconfiguration of the same BE DAI again, but not to
>> stop the provision to add a subsequent FE. In fact I had tested mixing
>> of streams coming from 10 different FEs.

Can you describe the sequence that you used to start them? That may be
useful to understand the criteria you used?

>> In your case, because of this patch, looks like the subsequent FE is not
>> finding a BE DAI since it is already active due to a prior FE. The
>> reason it works at my end is because the mixer input and output DAIs are
>> separated. Any new FE would just configure the mixer input DAI to which
>> it is attached and skip already running/configured output DAI. 

If you want to visualize the topology we're using in our 'nocodec'
tests, see

and use the sof-adl-nocodec.

it's a bog-standard topology with processing entities connected by buffers.

I don't fully understand the notion of mixer input DAI, in our case we
have a bunch of PCM devices connected to a mixer. The mixer is not
directly connected to a DAI.

> The problem as I see is that with this patch one can not connect a new
> FE to a BE which is _not_ in NEW or CLOSE state.
> The FE and BE needs to be connected to have DPCM working and this patch
> makes the code to skip the dpcm_be_connect().
> Consider this simple setup:
> FE1 -->|
>        | --> BE -->
> FE2- ->|
> First we start FE1, dpcm_be_connect(FE1, BE, stream) is made.
> Later FE2 is started but dpcm_be_connect(FE2, BE, stream) would be not
> made because BE is no longer in NEW/CLOSE state.

I share Peter's analysis, there cannot be any restrictions on
connections - at any time. A mixer input might become active and be
added to the mix. We might have a temporary lock to delay new
connections but cannot not reject them outright based on BE state.

>> I am just
>> curious to know, if originally you were reconfiguring the BE DAI again
>> with same parameters (for a second FE) or some additional configuration
>> is done?

That's a different question - and a good one.

In the case of a mixer, the propagation of hw_params is a broken
concept. It leads to the first FE configuring the BE to define its
preferred parameters, e.g. S16_LE format. If later on a second FE is
started which could play S24_LE, the mixer and BE are already configured
to a lower resolution. A mixer should really have its own parameters and
be the start of a new 'domain' - as Lars described it several years ago
at the audio miniconference.

For now, the only restriction that we could enforce is that the BE
cannot be reconfigured after the prepare step.

Note that our DAIs tolerate multiple calls to hw_params. If you have a
case where the hw_params allocates resources, maybe you should consider
moving that allocation to the prepare step, or free them if you detect a
reconfiguration. That would be something needed even outside of the DPCM
scope. Similarly you need to support the case where the DAI hw_free is
called without hw_params ever being called, it's a known sequence that
can happen if the FE hw-params fails.

>>> I can send a revert with the explanations in the commit message if there
>>> is a consensus that this patch needs to be revisited.
>> May be this can be revisited since it appears to be a critical problem
>> for your system. But I hope this discussion can be alive on following
>> points for a better fix.
>> 1. The original issue at my end was not just a configuration redundancy.
>> I realize now that with more stream addition following error print is seen.
>>    "ASoC: too many users playback at open 4"
>>    This is because the max DPCM users is capped at 8. Increasing this
>> may help (need to see what number is better), but does not address the
>> redundancy problem.

we haven't used more than 2 users, but it's already broken at 2 with
race conditions left and right. I am really surprised you managed to
have more than 2 without hitting inconsistent states - our automated
play/stop/pause monkey tests reliably break DPCM in less than 20s.

>> 2. If reconfiguration of the same BE is not necessary for a subsequent
>> FE run, shouldn't we avoid the reconfig itself and somehow avoid FE
>> failure?
> I'm not sure, but it might be possible to just skip the
> dpcm_set_be_update_state(be, stream, SND_SOC_DPCM_UPDATE_BE);
> call at the end of the loop, but the question is under which condition?
> Can a BE asked to be reconfigured when STOP/OPEN/HW_PARAMS?
> Skipping the connect does not sound right for a new FE-BE connection.

The reconfiguration is one problem, but what also happens is that the BE
dailink will see multiple triggers. I've been playing with refcounts to
force consistency and make sure there is only one TRIGGER_START send to
the dailink, and conversely there are cases where the TRIGGER_STOP is
never sent...

  parent reply	other threads:[~2021-09-29 15:30 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-27  9:33 [PATCH 00/13] Extend AHUB audio support for Tegra210 and later Sameer Pujar
2021-08-27  9:33 ` [PATCH 01/13] ASoC: soc-pcm: Don't reconnect an already active BE Sameer Pujar
2021-09-28 21:25   ` Pierre-Louis Bossart
2021-09-29  7:43     ` Sameer Pujar
     [not found]       ` <>
2021-09-29 14:52         ` Pierre-Louis Bossart [this message]
2021-09-30  7:57           ` Sameer Pujar
2021-09-30 14:34             ` Pierre-Louis Bossart
2021-09-30 15:35               ` Sameer Pujar
2021-09-30 16:13                 ` Pierre-Louis Bossart
2021-09-30 19:00       ` Pierre-Louis Bossart
2021-10-04  4:38         ` Sameer Pujar
2021-08-27  9:33 ` [PATCH 02/13] ASoC: simple-card-utils: Increase maximum DAI links limit to 512 Sameer Pujar
2021-08-27  9:33 ` [PATCH 03/13] ASoC: audio-graph: Fixup CPU endpoint hw_params in a BE<->BE link Sameer Pujar
2021-08-27  9:33 ` [PATCH 04/13] ASoC: dt-bindings: tegra: Few more Tegra210 AHUB modules Sameer Pujar
2021-08-31 20:21   ` Rob Herring
2021-09-01  7:09     ` Sameer Pujar
2021-08-27  9:33 ` [PATCH 05/13] ASoC: tegra: Add routes for few " Sameer Pujar
2021-08-27  9:33 ` [PATCH 06/13] ASoC: tegra: Add Tegra210 based MVC driver Sameer Pujar
2021-09-03 18:13   ` Mark Brown
     [not found]     ` <>
     [not found]       ` <>
2021-09-09 14:20         ` Mark Brown
     [not found]     ` <>
2021-09-13 14:23       ` Mark Brown
2021-08-27  9:33 ` [PATCH 07/13] ASoC: tegra: Add Tegra210 based SFC driver Sameer Pujar
2021-08-27  9:33 ` [PATCH 08/13] ASoC: tegra: Add Tegra210 based AMX driver Sameer Pujar
2021-08-27  9:33 ` [PATCH 09/13] ASoC: tegra: Add Tegra210 based ADX driver Sameer Pujar
2021-08-27  9:33 ` [PATCH 10/13] ASoC: tegra: Add Tegra210 based Mixer driver Sameer Pujar
2021-08-27  9:33 ` [PATCH 11/13] arm64: defconfig: Enable few Tegra210 based AHUB drivers Sameer Pujar
2021-08-27  9:33 ` [PATCH 12/13] arm64: tegra: Add few AHUB devices for Tegra210 and later Sameer Pujar
2021-08-27  9:33 ` [PATCH 13/13] arm64: tegra: Extend APE audio support on Jetson platforms Sameer Pujar
2021-09-08  4:56 ` [PATCH 00/13] Extend AHUB audio support for Tegra210 and later Sameer Pujar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \
    --subject='Re: [PATCH 01/13] ASoC: soc-pcm: Don'\''t reconnect an already active BE' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).