Re: [PATCH 1/4] ASoC: soc-pcm: dpcm: fix playback/capture checks

From: Jerome Brunet <jbrunet@baylibre.com>
To: Mark Brown <broonie@kernel.org>,
	Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>,
	alsa-devel@alsa-project.org,
	Stephan Gerhold <stephan@gerhold.net>,
	tiwai@suse.de, Daniel Baluta <daniel.baluta@gmail.com>,
	Ranjani Sridharan <ranjani.sridharan@linux.intel.com>,
	Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
	Bard Liao <yung-chuan.liao@linux.intel.com>
Subject: Re: [PATCH 1/4] ASoC: soc-pcm: dpcm: fix playback/capture checks
Date: Fri, 17 Jul 2020 15:42:13 +0200	[thread overview]
Message-ID: <1j1rla9s22.fsf@starbuckisacylon.baylibre.com> (raw)
In-Reply-To: <20200616164255.GR4447@sirena.org.uk>

On Tue 16 Jun 2020 at 18:42, Mark Brown <broonie@kernel.org> wrote:

> On Tue, Jun 16, 2020 at 10:05:39AM -0500, Pierre-Louis Bossart wrote:
>> On 6/16/20 9:52 AM, Mark Brown wrote:
>> > On Tue, Jun 16, 2020 at 09:23:25AM -0500, Pierre-Louis Bossart wrote:
>
>> > > Doesn't simple-card rely on DT blobs that can also be updated?
>
>> > DT is an ABI just like ACPI - it's just more featureful.  Many systems
>> > can easily update their DTs but not all of them and users don't always
>> > want to try to keep it in lock step with the kernel.  Stuff like this is
>> > why I've been dubious about putting DPCM things in there, it's too much
>> > of a hard coding of internal APIs.
>
>> ok, but is there any actual use of dpcm_playback/capture outside of C code?
>
>> simple-card.c and audio-graph-card do hard-code but that's done with C in
>> the driver:
>
> ...
>
>> that that should be fixed based on the DAI format used in that dai_link - in
>> other words we can make sure the capabilities of the dailink are aligned
>> with the dais while parsing the DT blobs.
>
> Right, just heading off the idea that we can fix things by updating DTs.

Hi everyone,

Getting to this a bit late in the game but ...

This patch breaks things for me as well. The Amlogic S400 (axg-card) and
Libretech S905x card (gx-card) are not probing anymore after this
change. Both have some BEs handling only one direction.

Like for Stephan (and simple-card), the related cards used to set
dpcm_capture/dpcm_playback to 1.

Before this patch, these flags just applied an additionnal restiction on
the link. So the card was setting it to 1 and let soc-pcm.c figure out
what was actually needed. Whether it is usefull to have such flags is
certainly up for debate ...

However, with patch, the meaning of the flags changed from "retrict" to
"require" which is something else entirely.

Commit 25612477d20b ("ASoC: soc-dai: set dai_link dpcm_ flags with a
helper") makes things worse (for me) by requiring all the elements on
the link to support the stream direction for the direction to be enabled.

This breaks platforms where there is multiple codecs with different
capabilities on a link. For example, we may have 2 codecs on a link, one
doing playback, the other capture. This is the case on several Amlogic
platforms.

With the new meaning of those flag, the card driver has to set something
that ASoC core will also compute on its own, and verify it agrees with
the card. This is redundant.
What is the point of this ? Can't we just cut the middle man then ?

The old meaning at least allowed to pass the additional information that
a direction was to be forcefully disabled. This is also redundant with
capture_only/playback_only though ...

Can we revert this change until we figure out to do with those flags ?

I could propose a patch to move on but I'm not entirely clear what it kind
of restriction we were trying to put on Multi-CPU links

IMO, on a Multi-CPU/Multi-Codec link, we should allow the direction as
long as at least one CPU and one Codec on the link are capable of
handling the direction (not all of them, as it seems to be set now)

Cheers
Jerome