[alsa-devel] ASoC:Question rate constraint between the dais

[Trent Piepho]

On Sat, Mar 17, 2012 at 7:48 AM, Mark Brown
<broonie at opensource.wolfsonmicro.com> wrote:
>
> On Fri, Mar 16, 2012 at 06:01:44PM -0400, Trent Piepho wrote:
> > On Fri, Mar 16, 2012 at 3:22 PM, Mark Brown
>
> > > There's a way to do this, and it's the way that the WM8988 driver is
> > > doing it - obviously you'll have different criteria for choosing
> > > constraints but the actual application of the constraints is going to be
> > > done in the same way.
>
> > Since the constraint is based on the current configuration of another
> > DAI, the two DAIs need some way to query each other's parameters.  The
>
> Since they're both provided by the same driver this is trivial.

It's not that trivial with the sub-streams in question aren't in the
same instance of the driver, e.g. playback and record.  Say you had
two WM8988 codecs, how does one codec get a handle to the other?
There's no global list of codecs in the driver, so you have to add
something like that. Or change the device tree binding to give the
slave codec a phandle to the master codec.

> > And as I said before, ALSA doesn't handle this 100%.  There is a race
> > if both streams are initialized at the same time.  If you open one
>
> Yes, I did reply to your previous mail about that pointing out that this
> is impossible...

...which didn't arrive until after I started writing my email.

> > I think what's necessary is for ALSA to let hw_params return an error
> > code that means lets the driver say, "Sorry, the constraints have
> > changed and the hw_params you have chosen are no longer valid.  Here
> > are new constraints, please try again."
>
> Thinking about this slightly further than in my reply to Lars-Peter a
> few minutes ago...
>
> I think just returning -EINVAL on bad parameters (which is pretty much
> what we do) is sufficient for this if the driver is good about keeping
> the constraints up to date.  Applications should already be able to take
> the hint, though I imagine most of them wouldn't trust the drivers to
> provide accurate constraints and aren't as widely deployed as PulseAudio
> (which forced fixing the DMA stuff by virtue of everyone using it).
> Though just spinning a small number of times will probably deal with
> lying drivers well enough.

Does PulseAudio handle it if hw_params fails?  From what I've seen,
most applications just fail if hw_params fails.

I think there is still a race in the design of ALSA, in that the
driver checking the parameters in its hw_params() method is not atomic
with the substream being assigned those parameters, w.r.t. another
substream.

The problem is in the code for snd_pcm_hw_params(),

 405        err = snd_pcm_hw_params_choose(substream, params);
 406        if (err < 0)
 407                goto _error;
 408
 409        if (substream->ops->hw_params != NULL) {
 410                err = substream->ops->hw_params(substream, params);
 411                if (err < 0)
 412                        goto _error;
 413        }
 414
 415        runtime->access = params_access(params);
 ...
 455        runtime->status->state = SNDRV_PCM_STATE_SETUP;

At line 410 the driver is given a chance to approve the selected
hw_params.  It can look at any of the joint substreams, and if they
are in state >= SETUP, verify that the hw_params are still valid and
if not, update the constraints and return an error.  But it is not
until line 455 that the substream has actually finished getting the
parameters and entered the SETUP state. There is no lock to prevent
another substream from entering this code at the same time.  To fix
the race, there would need to be a critical section from line 408-455
that only one substream of the group with joint constraints can enter
at one time.