Need help fixing pop/click artifacts in an ASOC driver

Need help fixing pop/click artifacts in an ASOC driver

[Dimitris Papavasiliou]

Hi,

A (commercial) DAC board, I use with a Raspberry Pi is plagued by clicks 
and pops and I'm trying to fix it at the driver level.  I have mostly 
fixed it, but I could use some guidance on the problem described below. 
I apologize in advance for the long read, but the situation is a bit 
convoluted (or my understanding of it is).

The board (a HifiBerry DAC+ Pro) uses a PCM5122 chip which is driven by 
the pcm512x codec driver, but the Pro version also includes a set of 
external clocks on the board (one each for 48Khz and 44.1Khz sample 
rates and multiples thereof).

There are two kinds of clicks and pops: on the one hand the board clicks 
when the device is opened/closed, mostly because the pcm512x codec 
driver doesn't support the digital_mute operation.  These are relatively 
mild clicks, which are an annoyance at most and I have a separate patch 
for the pcm512x, that addresses them, which I'm currently testing and 
will (hopefully) submit soon.

A separate case is a pop that's generated when the clock source is 
changed.  This pop is independent of the digital volume setting and very 
loud.  As far as I could determine, when the hw_params callback 
specified in the snd_soc_dai_link struct is called, it figures out what 
clock is needed for the giver sample rate and powers the appropriate 
clock through a GPIO on the PCM5122.  This essentially means that the 
external clock source fed into the 5122 is stopped and restarted at a 
different rate, potentially while the DAC chip is operating.  Later the 
hw_params callback defined in the pcm512x driver is called and it 
reconfigure the clock dividers for the new rate.  It is at this point 
that the pop seems to be generated, provided that the card is not 
powered down or muted.  If I comment out either the clock power-up/down 
in the dai_link hw_params, or the clock divider reconfiguration in the 
codec's hw_params, no pop is generated.

Now, the digital_mute patch described before, also happens to fix this 
loud pop.  I'm not sure whether that's a happy coincidence that might 
cease to be the case in the future though and was considering fixing the 
pop problem separately, at its source: the clock change.  One way to fix 
the problem is to switch the 5122 into standby before changing the clock 
source in the dai_link hw_params callback and switch it back into normal 
operation afterwards.  This seems to work regardless of whether 
digital_mute is implemented or not, but has the following problem: The 
register that's used to put the chip into/out of standby, is also used 
by the pcm512x driver, in the set_bias_level operation.  Since the 
hw_params operation is, as far as I can tell, not atomic, I'm concerned 
that race conditions may arise, causing the card to be left powered up, 
when it should be off and vice-versa.

Is there some way to synchronize the use of common chip registers in the 
DAI and codec drivers?  Is the digital_mute somehow guaranteed to fix 
the pop originating at the DAI/machine driver level (whatever the 
hw_params operation in the dai_link struct is supposed to be)? Am I 
perhaps missing something entirely?

Any help or guidance is welcome.

Thanks,
Dimitris

Re: Need help fixing pop/click artifacts in an ASOC driver

[Pierre-Louis Bossart]

On 11/6/18 10:37 AM, Dimitris Papavasiliou wrote:
> Hi,
>
> A (commercial) DAC board, I use with a Raspberry Pi is plagued by 
> clicks and pops and I'm trying to fix it at the driver level.  I have 
> mostly fixed it, but I could use some guidance on the problem 
> described below. I apologize in advance for the long read, but the 
> situation is a bit convoluted (or my understanding of it is).
>
> The board (a HifiBerry DAC+ Pro) uses a PCM5122 chip which is driven 
> by the pcm512x codec driver, but the Pro version also includes a set 
> of external clocks on the board (one each for 48Khz and 44.1Khz sample 
> rates and multiples thereof).
>
> There are two kinds of clicks and pops: on the one hand the board 
> clicks when the device is opened/closed, mostly because the pcm512x 
> codec driver doesn't support the digital_mute operation. These are 
> relatively mild clicks, which are an annoyance at most and I have a 
> separate patch for the pcm512x, that addresses them, which I'm 
> currently testing and will (hopefully) submit soon.
>
> A separate case is a pop that's generated when the clock source is 
> changed.  This pop is independent of the digital volume setting and 
> very loud.  As far as I could determine, when the hw_params callback 
> specified in the snd_soc_dai_link struct is called, it figures out 
> what clock is needed for the giver sample rate and powers the 
> appropriate clock through a GPIO on the PCM5122.  This essentially 
> means that the external clock source fed into the 5122 is stopped and 
> restarted at a different rate, potentially while the DAC chip is 
> operating.  Later the hw_params callback defined in the pcm512x driver 
> is called and it reconfigure the clock dividers for the new rate.  It 
> is at this point that the pop seems to be generated, provided that the 
> card is not powered down or muted.  If I comment out either the clock 
> power-up/down in the dai_link hw_params, or the clock divider 
> reconfiguration in the codec's hw_params, no pop is generated.
>
> Now, the digital_mute patch described before, also happens to fix this 
> loud pop.  I'm not sure whether that's a happy coincidence that might 
> cease to be the case in the future though and was considering fixing 
> the pop problem separately, at its source: the clock change.  One way 
> to fix the problem is to switch the 5122 into standby before changing 
> the clock source in the dai_link hw_params callback and switch it back 
> into normal operation afterwards.  This seems to work regardless of 
> whether digital_mute is implemented or not, but has the following 
> problem: The register that's used to put the chip into/out of standby, 
> is also used by the pcm512x driver, in the set_bias_level operation.  
> Since the hw_params operation is, as far as I can tell, not atomic, 
> I'm concerned that race conditions may arise, causing the card to be 
> left powered up, when it should be off and vice-versa.
>
> Is there some way to synchronize the use of common chip registers in 
> the DAI and codec drivers?  Is the digital_mute somehow guaranteed to 
> fix the pop originating at the DAI/machine driver level (whatever the 
> hw_params operation in the dai_link struct is supposed to be)? Am I 
> perhaps missing something entirely?
>
> Any help or guidance is welcome.

Thanks for starting this thread. You are not the only one fighting to 
make those Hifiberry DAC+ PRO boards work without convoluted hacks. Our 
team uses them for validation of the Sound Open Firmware on the UP^2 
boards and we are not quite happy with the clock definitions and don't 
really have a line of sight to upstream our changes.

The current implementation we have is ripped from the Raspberry kernel:

The PCM512x codec uses the clock framework to request an "sclk", and if 
it's not available will fall back to slave mode (SoC master). The codec 
driver uses this clock handle to request the current rates with 
clk_get_rate() and program the various dividers.

In the Raspberry kernel there is a pretend clock which does nothing but 
store a value on clk_set_rate() and provides the stored value with 
clock_get_rate().

The machine driver hw_params calls clk_set_rate(), but the silly part is 
that it then directly plays with the codec GPIO registers to change the 
clock sources without any sort of stabilization time/mute.

My view is that it's a broken model, and your point about the clock 
transitions not taking into account the codec state reinforces my 
diagnosis. Ideally we'd need a clk API implementation which is closely 
tied with codec driver implementation for the cases where the codec 
GPIOs are used to select clock sources.

For now our code uses a hack which bypasses the clock API completely. I 
shared it on this list [1] but was told the clock API was the way to go. 
I don't have a clear vision though on how to deal with a case where the 
codec driver is both client and provider of a clock.

I am told that Suse supports the Raspberry Pi now, maybe Takashi will 
chime in :-)?

-Pierre

[1] https://patchwork.kernel.org/patch/10444387/


_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: Need help fixing pop/click artifacts in an ASOC driver

[Takashi Iwai]

On Thu, 08 Nov 2018 02:42:52 +0100,
Pierre-Louis Bossart wrote:
> 
> I am told that Suse supports the Raspberry Pi now, maybe Takashi will
> chime in :-)?

Unfortunately, I already left my Pi box, and we don't work on the
extension board in general...


Takashi

Re: Need help fixing pop/click artifacts in an ASOC driver

[Dimitris Papavasiliou]

On 11/08/2018 03:42 AM, Pierre-Louis Bossart wrote:
> Thanks for starting this thread. You are not the only one fighting to 
> make those Hifiberry DAC+ PRO boards work without convoluted hacks. Our 
> team uses them for validation of the Sound Open Firmware on the UP^2 
> boards and we are not quite happy with the clock definitions and don't 
> really have a line of sight to upstream our changes.

Thanks for responding; I was starting to worry this thread would
go by unnoticed.  I've noticed the less than elegant clock setup
as well, and considered patching it, so that the clocks are
actually selected (i.e. powered up/down inside the clock
driver). I'm not sure whether it would have any practical
significance though, as after probing the card, the CODEC driver
only ever stops or starts the clocks inside PM-related callbacks,
which I don't think the Pi supports.

Are you also getting loud pops when switching between clocks?  I
use the following Python script to reproduce the problem (and
also disable auto-mute via the mixer, as it can mask it):

------------------------------

import alsaaudio, time, random

d = alsaaudio.PCM()
p = True

while True:
     time.sleep(1)

     f = p and 48000 or 44100
     n = p and 2 or 1
     m = p and alsaaudio.PCM_FORMAT_S16_LE or alsaaudio.PCM_FORMAT_S24_LE

     d.setrate(f)
     d.setchannels(n)
     d.setformat(m)

     if random.random() > 0*0.25:
         print "pop!"
         p = not p

------------------------------

I've done some more testing regarding the pop, including:

* Trying to switch the DAC clock reference away from SCK (via
   PCM512x_DAC_REF), prior to switching the clocks (to the BCK, or
   to a reserved value that's supposed to cause a mute according
   to the datasheet).

* Halting the clocks, prior to switching the clock source (via
   PCM512x_SYNCHRONIZE) and letting the CODEC driver resynchronize
   them in its hw_params callback, or resynchronizing after the
   switch.

* Enabling clock error detection, which the CODEC driver mostly
   disables, in the hope that it will force a mute.

None of this fixes the pop, although the first two can lower its
volume somewhat.  All in all, the only changes that silence the
pop are:

* Putting the chip into standby before switching clocks in the
   machine driver and resuming afterwards.

* Muting the output before switching clocks in the machine driver
   and unmuting afterwards.

With any of the above, I get no pop after hundreds of clock
switches (using the above script), which otherwise pop every
time.

Muting the output, or suspending the chip at the beginning of the
CODEC driver (pcm512x) and unmuting, or resuming at the end
doesn't help.  This suggests to me that a spike is generated at
the DAC input sometime during/after the clock switch and that
suspending the chip during the switch avoids it altogether, while
muting the output suppresses it.

Implementing digital_mute in the CODEC driver probably helps
because the output is muted during the machine driver's hw_params
callback, but it would make sense to me to fix the machine
driver, so that the spike is never generated in the first place.
My problem is that the PCM512x_POWER register is manipulated at
the CODEC level, inside DAPM-related callbacks and, as far as I
can see, hw_params, a PCM-related callback, and DAPM callbacks
aren't synchronized through some mutex.  Race conditions should
be possible.

The best solution I've found, is to force the chip into standby
via something like the following:

struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component);

snd_soc_dapm_mutex_lock(dapm);
snd_soc_dapm_force_bias_level(dapm, SND_SOC_BIAS_OFF);

/* Switch clock source here. */

snd_soc_dapm_sync_unlocked(dapm);
snd_soc_dapm_mutex_unlock(dapm);

This seems to work fine, but I'm not sure if it's the appropriate
approach, or if it has unwanted side-effects.  I've CCed the
author of pcm512x, who's also a maintainer on ASOC and
DAPM-related stuff.  Perhaps he can advise on this.

-Dimitris

Re: Need help fixing pop/click artifacts in an ASOC driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 426 bytes --]

On Sat, Nov 10, 2018 at 05:46:33PM +0200, Dimitris Papavasiliou wrote:

> Are you also getting loud pops when switching between clocks?  I
> use the following Python script to reproduce the problem (and
> also disable auto-mute via the mixer, as it can mask it):

Switching between clocks while the device isn't completely idle is
unlikely to work well with any device - I'd have questions about a use
case that's doing that.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Need help fixing pop/click artifacts in an ASOC driver

[Dimitris Papavasiliou]

On 11/15/2018 12:06 AM, Mark Brown wrote:
> Switching between clocks while the device isn't completely idle is
> unlikely to work well with any device - I'd have questions about a use
> case that's doing that.

The clocks are switched inside the hw_params callback of the
machine driver, when the sampling rate changes from a multiple of
48kHz to a multiple of 44.1kHz and vice versa, usually after
opening the device and before starting playback.  That has to be
so, as far as I can see.

If by idle you mean, not playing, then the device is idle during
the switch.  If by idle you mean biased off, then it depends on
circumstance.  For instance if playback at 44.1kHz is stopped and
quickly restarted at 48kHz, as often happens, the device won't
have had the chance to sit long enough to be biased off (and even
if it has, I'm not entirely sure whether it won't be biased on
before the hw_params callback).  The pop may still be avoided if
auto-mute is used, but that's not always the case.  My proposed
fix on the other hand consistently avoids generating the pop and
does not rely on muting.  I'm just not sure how to best implement
it.

Re: Need help fixing pop/click artifacts in an ASOC driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1341 bytes --]

On Thu, Nov 15, 2018 at 12:50:30AM +0200, Dimitris Papavasiliou wrote:

> The clocks are switched inside the hw_params callback of the
> machine driver, when the sampling rate changes from a multiple of
> 48kHz to a multiple of 44.1kHz and vice versa, usually after
> opening the device and before starting playback.  That has to be
> so, as far as I can see.

That's fairly normal, yes.  You can also do SRC in software and lock the
rate at one.

> If by idle you mean, not playing, then the device is idle during
> the switch.  If by idle you mean biased off, then it depends on
> circumstance.  For instance if playback at 44.1kHz is stopped and
> quickly restarted at 48kHz, as often happens, the device won't
> have had the chance to sit long enough to be biased off (and even
> if it has, I'm not entirely sure whether it won't be biased on
> before the hw_params callback).  The pop may still be avoided if

Usually it's only the digital that will notice clocking changes, so long
as none of the digital stuff is active you're normally fine.  It does
depend on the hardware though.

> auto-mute is used, but that's not always the case.  My proposed
> fix on the other hand consistently avoids generating the pop and
> does not rely on muting.  I'm just not sure how to best implement
> it.

I've no idea what you're proposing, sorry.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Need help fixing pop/click artifacts in an ASOC driver

[Dimitris Papavasiliou]

Thank you for taking the time to help out.

On 11/15/2018 01:02 AM, Mark Brown wrote:
> I've no idea what you're proposing, sorry.

I mentioned it in the message you replied to initially, so you
can consult that for more details if you want, but I'll give a
summary below:

As far as I could determine experimentally, switching clocks when
the DAC is not suspended seems to result in a spike at the input
of the DAC, the level of which doesn't depend on the digital
volume setting (so that it's always very loud).  I tried various
potential fixes, based on guesses about probable causes of the
spike (see first message where I CCed you for a brief list of
attempted fixes) and it seems that a pop can be avoided either by
suppressing it, by muting the DAC, or by avoiding generating it
in the first place by switching the PCM5122 into standby (via bit
PCM512x_RQST of the PCM512x_POWER register) before switching the
external clock and switching it back out of standby afterwards.

Avoiding the spike instead of relying on muting seems preferable,
but, since the power state of the PCM5122 is manipulated by the
CODEC driver, in response to requests to set the bias level, I'm
concerned about potential race conditions.

Right now I have the following implementation for switching the
external clock:


/* Figure out if a clock switch is necessary based on currently
    used clock and requested sample rate . */

...

if (no_switch_needed)
     return;

snd_soc_dapm_mutex_lock(dapm);
force = (snd_soc_dapm_get_bias_level(dapm) != SND_SOC_BIAS_OFF);

if (force)
     snd_soc_dapm_force_bias_level(dapm, SND_SOC_BIAS_OFF);

/* Switch the clock here. */

...

if (force)
     snd_soc_dapm_sync_unlocked(dapm);

snd_soc_dapm_mutex_unlock(dapm);


It seems to work fine, both in consistently avoiding the pop and
in not producing any obvious side-effects.  Does that look
reasonable, or is there a better way to temporarily switch the
PCM5122 into standby in a safe way?

Re: Need help fixing pop/click artifacts in an ASOC driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1961 bytes --]

On Thu, Nov 15, 2018 at 01:55:06AM +0200, Dimitris Papavasiliou wrote:
> On 11/15/2018 01:02 AM, Mark Brown wrote:
> > I've no idea what you're proposing, sorry.

> I mentioned it in the message you replied to initially, so you
> can consult that for more details if you want, but I'll give a
> summary below:

That was rather a long mail and I got a bit lost about what the proposal
was.

> As far as I could determine experimentally, switching clocks when
> the DAC is not suspended seems to result in a spike at the input
> of the DAC, the level of which doesn't depend on the digital
> volume setting (so that it's always very loud).  I tried various

This is fairly normal; the DAC is partly digital.

> Avoiding the spike instead of relying on muting seems preferable,
> but, since the power state of the PCM5122 is manipulated by the
> CODEC driver, in response to requests to set the bias level, I'm
> concerned about potential race conditions.

Given that there's no substantial delays in the power up/down paths of
the driver you probably want to set idle_bias_off and possibly also
configuring ignore_pmdown_time to force immediate poweroff.  That should
get the device powered down rapidly, though bouncing the power on and
off all the time isn't great.  If you do need ignore_pmdown_time then
it's probably better to add a higher level interface that allows the
machine driver to cancel all power down timers.

> if (no_switch_needed)
>     return;
> 
> snd_soc_dapm_mutex_lock(dapm);
> force = (snd_soc_dapm_get_bias_level(dapm) != SND_SOC_BIAS_OFF);
> 
> if (force)
>     snd_soc_dapm_force_bias_level(dapm, SND_SOC_BIAS_OFF);
> 
> /* Switch the clock here. */
> 
> ...
> 
> if (force)
>     snd_soc_dapm_sync_unlocked(dapm);
> 
> snd_soc_dapm_mutex_unlock(dapm);

This is not a good idea, fiddling around with DAPM internals from
drivers is going to be very fragile as things change in future.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Need help fixing pop/click artifacts in an ASOC driver

[Dimitris Papavasiliou]

On 11/15/18 2:33 AM, Mark Brown wrote:
> On Thu, Nov 15, 2018 at 01:55:06AM +0200, Dimitris Papavasiliou wrote:
>> On 11/15/2018 01:02 AM, Mark Brown wrote:
>>> I've no idea what you're proposing, sorry.
> 
>> I mentioned it in the message you replied to initially, so you
>> can consult that for more details if you want, but I'll give a
>> summary below:
> 
> That was rather a long mail and I got a bit lost about what the proposal
> was.

I understand completely and I apologize for that.  I try to keep the
length down as much as possible, but the situation is a bit
complicated (at least with my current familiarity of the ASOC layer).
Please don't interpret what I said as a complaint; I merely wanted to
point out that more details were available, should you require them.

> Given that there's no substantial delays in the power up/down paths of
> the driver you probably want to set idle_bias_off and possibly also
> configuring ignore_pmdown_time to force immediate poweroff.  That should
> get the device powered down rapidly, though bouncing the power on and
> off all the time isn't great.  If you do need ignore_pmdown_time then
> it's probably better to add a higher level interface that allows the
> machine driver to cancel all power down timers.

Although I think I've mentioned it in previous messages, you may have
missed it: What I'm trying to do, is fix the driver for a commercial
DAC board (the HifiBerry DAC+ Pro) which was designed for the
Raspberry Pi.  It uses the pcm512x CODEC driver (by specifying
.codec_dai_name = "pcm512x-hifi" in the dai_link struct) and simply
supplies a separate machine driver and a rudimentary clock driver, to
handle the necessary clock switching.

(If it would help, you can see the relevant code here:

https://github.com/raspberrypi/linux/blob/rpi-4.19.y/sound/soc/bcm/hifiberry_dacplus.c

Clock switching takes place in snd_rpi_hifiberry_dacplus_select_clk
which is eventually called from snd_rpi_hifiberry_dacplus_hw_params.)

Unless I misunderstand, I can't change the CODEC driver's settings
(such as ignore_pmdown_time) from the machine driver.  Even if I
could, I'm not entirely sure it would consistently fix the problem.
For instance, if the auto-mute is set to 21ms, the DAC output is muted
automatically by the chip after 21ms of no input and the pop still
manages to get through before that on occasion.

It also seems like a roundabout solution, that depends on proper
sequencing, i.e. it is assumed that the device is always biased
off before the hw_params callback of the machine driver is
called, something which might change in the future and cause a
regression.

All that's really necessary, is calling

snd_soc_component_update_bits(component, PCM512x_POWER, PCM512x_RQSY, 
PCM512x_RQSY);

before the clock switch and

snd_soc_component_update_bits(component, PCM512x_POWER, PCM512x_RQSY, 0);

afterwards.  Isn't there some way to lock the component/CODEC's
regmap, so as to perform an operation involving more than one
register change atomically? Something like
snd_soc_component_lock/unlock_regmap sounds like it might be
generally useful, to synchronize access to a chip's registers
across CODE/machine/etc. drivers.

Re: Need help fixing pop/click artifacts in an ASOC driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1851 bytes --]

On Thu, Nov 15, 2018 at 01:25:28PM +0200, Dimitris Papavasiliou wrote:

> Unless I misunderstand, I can't change the CODEC driver's settings
> (such as ignore_pmdown_time) from the machine driver.  Even if I

You can get to the CODEC driver from the machine driver, and for
idle_bias_off there's probably a good case for just setting it
unconditionally given the lack of delays.

> could, I'm not entirely sure it would consistently fix the problem.
> For instance, if the auto-mute is set to 21ms, the DAC output is muted
> automatically by the chip after 21ms of no input and the pop still
> manages to get through before that on occasion.

That's why I'm suggesting ignore_pmdown_time as a first thing to try -
it should cause the powerdown to happen synchronously so there's no
chance for anything else to go in and change the clocking to cause a
pop.

> It also seems like a roundabout solution, that depends on proper
> sequencing, i.e. it is assumed that the device is always biased
> off before the hw_params callback of the machine driver is
> called, something which might change in the future and cause a
> regression.

The whole goal with the combination of idle_bias_off and
ignore_pmdown_time is to ensure that this happens in an orderly
fashion in a way that the core knows about.

> afterwards.  Isn't there some way to lock the component/CODEC's
> regmap, so as to perform an operation involving more than one
> register change atomically? Something like
> snd_soc_component_lock/unlock_regmap sounds like it might be
> generally useful, to synchronize access to a chip's registers
> across CODE/machine/etc. drivers.

No, there's no facility for that and it's such a niche case that I'm not
convinced it's a good idea to do it.  Another thing you could do here
though is to do the bounce as part of set_sysclk() in the CODEC driver.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Need help fixing pop/click artifacts in an ASOC driver

[Dimitris Papavasiliou]

On 11/15/2018 09:04 PM, Mark Brown wrote:
> You can get to the CODEC driver from the machine driver, and for
> idle_bias_off there's probably a good case for just setting it
> unconditionally given the lack of delays.
> 
> ...
> 
> That's why I'm suggesting ignore_pmdown_time as a first thing to try -
> it should cause the powerdown to happen synchronously so there's no
> chance for anything else to go in and change the clocking to cause a
> pop.

Thanks for clarifying about idle_bias_off/ignore_pmdown_time.  As
far as I can see, idle_bias_off seems to be set by default, as
the pcm512x driver is now a component driver and idle_bias_on is
not set to true.  I've also tried to explicitly set it to false,
as well as setting use_pmdown_time to false. As far as
ignore_pmdown_time is concerned, it seems to be settable at the
dai_link level now, which is convenient for my use case.

Unfortunately none of this helps.  Although the chip is turned
off without delay, it's turned off only while the device is
closed.  As soon as the device is opened, it is turned on and
kept on during all subsequent hw_params calls, where clock
switching takes place.  The pops always get through.

> No, there's no facility for that and it's such a niche case that I'm not
> convinced it's a good idea to do it.  Another thing you could do here
> though is to do the bounce as part of set_sysclk() in the CODEC driver.

Regarding your suggestion to do the bounce as part of
set_sysclk() in the CODEC driver, I'm unsure how to proceed.  The
set_sysclk callback is virtually undocumented, as far as I could
see, and not supported by the pcm512x component driver.  Even if
I could support it, either my somehow filling in the component
driver callbacks from the machine driver, or by patching the
pcm512x component driver itself, if such a patch would be
acceptable, I don't see how it could help with my problem.  Since
both set_sysclk callbacks seem to operate at the CODEC or CODEC
DAI level, which knows nothing about how to switch clocks on this
particular card, I don't see how I can do the switch there, where
I can safely wrap it inside a bounce.

Maybe I'm just missing something, but as far as I can see,
there's no way to get around the need to synchronize access to
the POWER register, so as to be able to reliably power bounce the
chip while the clock selection takes place outside the
CODEC/component driver.  If such synchronization is impossible,
I'm forced to accept that the pop will be generated and rely on
suppressing it via a mute.  This can be done by implementing the
.digital_mute callback in the pcm512x component driver, for which
I have a patch, which seems to work reliably for suppressing both
the loud pop, as well as various other lower volume clicks and
pops generated by the chip.  I still feel that not generating the
pop in the first place would be preferable though.

Re: Need help fixing pop/click artifacts in an ASOC driver

[Dimitris Papavasiliou]

I've submitted a patch for the pcm512x CODEC, that is relevant to
this problem.  The patch implements the digital_mute interface,
suppressing various clicks and pops, including the loud pop under
discussion here.  I still feel that fixing the machine driver, so
that it doesn't generate the pop at all would be desirable, as
I'm not sure whether the digital_mute callback is guaranteed to
have muted the device when the clock source takes place.

Does that sound reasonable, or would it be preferable to simply
rely on the digital_mute mechanism (assuming the patch is
acceptable)?

Re: Need help fixing pop/click artifacts in an ASOC driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 517 bytes --]

On Sat, Nov 24, 2018 at 10:17:49PM +0200, Dimitris Papavasiliou wrote:

> discussion here.  I still feel that fixing the machine driver, so
> that it doesn't generate the pop at all would be desirable, as
> I'm not sure whether the digital_mute callback is guaranteed to
> have muted the device when the clock source takes place.

Making the machine driver more robust is definitely better, it's more
robust in case the mute is for example just a very low gain which might
still let the pop through at a lower level.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Need help fixing pop/click artifacts in an ASOC driver

[Dimitris Papavasiliou]

On 12/13/18 7:42 PM, Mark Brown wrote:
> On Sat, Nov 24, 2018 at 10:17:49PM +0200, Dimitris Papavasiliou wrote:
> 
>> discussion here.  I still feel that fixing the machine driver, so
>> that it doesn't generate the pop at all would be desirable, as
>> I'm not sure whether the digital_mute callback is guaranteed to
>> have muted the device when the clock source takes place.
> 
> Making the machine driver more robust is definitely better, it's more
> robust in case the mute is for example just a very low gain which might
> still let the pop through at a lower level.

Even worse, it would let the pop through at full volume, as it
doesn't depend on the gain.  Still, there doesn't seem to be a
safe way to avoid the pop altogether, as far as I can see, since
the only way I have found to avoid it, is to suspend the chip
during the switch, and I can't synchronize access to the relevant
register by the machine and CODEC drivers.

If you have any other ideas/pointers on approaches I could
investigate, please let me know.

Re: Need help fixing pop/click artifacts in an ASOC driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 643 bytes --]

On Mon, Dec 17, 2018 at 02:17:45PM +0200, Dimitris Papavasiliou wrote:

> Even worse, it would let the pop through at full volume, as it
> doesn't depend on the gain.  Still, there doesn't seem to be a
> safe way to avoid the pop altogether, as far as I can see, since
> the only way I have found to avoid it, is to suspend the chip
> during the switch, and I can't synchronize access to the relevant
> register by the machine and CODEC drivers.

> If you have any other ideas/pointers on approaches I could
> investigate, please let me know.

If you're doing this during DAPM there shouldn't be anything else
running on the card at the time.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Need help fixing pop/click artifacts in an ASOC driver

[Dimitris Papavasiliou]

On 12/17/18 2:37 PM, Mark Brown wrote:
> On Mon, Dec 17, 2018 at 02:17:45PM +0200, Dimitris Papavasiliou wrote:
> 
>> Even worse, it would let the pop through at full volume, as it
>> doesn't depend on the gain.  Still, there doesn't seem to be a
>> safe way to avoid the pop altogether, as far as I can see, since
>> the only way I have found to avoid it, is to suspend the chip
>> during the switch, and I can't synchronize access to the relevant
>> register by the machine and CODEC drivers.
> 
>> If you have any other ideas/pointers on approaches I could
>> investigate, please let me know.
> 
> If you're doing this during DAPM there shouldn't be anything else
> running on the card at the time.

In general the clocks need to be switched during the hw_params
callback (when the new callback specifies a new sample rate,
which needs the other clock) and, as far as I can see, this can
happen at more or less any time, i.e. in all power states and
regardless of whether the device is opened or closed.

I can't therefore depend on DAPM functionality to suspend the
chip when needed, as far as I can see at least and apart from
asking the DAPM to force-suspend the chip, as I've already
suggested.

Re: Need help fixing pop/click artifacts in an ASOC driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 495 bytes --]

On Mon, Dec 17, 2018 at 02:58:03PM +0200, Dimitris Papavasiliou wrote:

> In general the clocks need to be switched during the hw_params
> callback (when the new callback specifies a new sample rate,
> which needs the other clock) and, as far as I can see, this can
> happen at more or less any time, i.e. in all power states and
> regardless of whether the device is opened or closed.

It shouldn't be called while the stream is running, if it is it'd be
entirely fine to just return an error.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Need help fixing pop/click artifacts in an ASOC driver

[Dimitris Papavasiliou]

On 12/17/18 4:10 PM, Mark Brown wrote:
> On Mon, Dec 17, 2018 at 02:58:03PM +0200, Dimitris Papavasiliou wrote:
> 
>> In general the clocks need to be switched during the hw_params
>> callback (when the new callback specifies a new sample rate,
>> which needs the other clock) and, as far as I can see, this can
>> happen at more or less any time, i.e. in all power states and
>> regardless of whether the device is opened or closed.
> 
> It shouldn't be called while the stream is running, if it is it'd be
> entirely fine to just return an error.

It's not called while the stream is running, or at least I
haven't seen that happen.  It can be called though, while the
device is opened and fully powered up, for instance when
switching tracks during playback, where the next track has a
different sample rate.  This scenario is relatively frequent
during normal use of a DAC and can't be avoided, as far as I can
see.

Re: Need help fixing pop/click artifacts in an ASOC driver

[Pierre-Louis Bossart]

On 12/17/18 6:17 AM, Dimitris Papavasiliou wrote:
> On 12/13/18 7:42 PM, Mark Brown wrote:
>> On Sat, Nov 24, 2018 at 10:17:49PM +0200, Dimitris Papavasiliou wrote:
>>
>>> discussion here.  I still feel that fixing the machine driver, so
>>> that it doesn't generate the pop at all would be desirable, as
>>> I'm not sure whether the digital_mute callback is guaranteed to
>>> have muted the device when the clock source takes place.
>>
>> Making the machine driver more robust is definitely better, it's more
>> robust in case the mute is for example just a very low gain which might
>> still let the pop through at a lower level.
>
> Even worse, it would let the pop through at full volume, as it
> doesn't depend on the gain.  Still, there doesn't seem to be a
> safe way to avoid the pop altogether, as far as I can see, since
> the only way I have found to avoid it, is to suspend the chip
> during the switch, and I can't synchronize access to the relevant
> register by the machine and CODEC drivers.
>
> If you have any other ideas/pointers on approaches I could
> investigate, please let me know.
>
I started prototyping a different approach where the codec driver passes 
the regmap information to the clock driver. What's missing in the 
patchset is the addition of a clock control in the machine driver, and 
logic added so that rate  change can only be done in a hw_params if 
there was a complete stop and reset on a DAPM_OFF event. compile-tested 
only for now.

https://github.com/plbossart/sound/commits/hifiberry/clks


_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: Need help fixing pop/click artifacts in an ASOC driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 695 bytes --]

On Mon, Dec 17, 2018 at 05:03:04PM +0200, Dimitris Papavasiliou wrote:
> On 12/17/18 4:10 PM, Mark Brown wrote:

> > It shouldn't be called while the stream is running, if it is it'd be
> > entirely fine to just return an error.

> It's not called while the stream is running, or at least I
> haven't seen that happen.  It can be called though, while the
> device is opened and fully powered up, for instance when
> switching tracks during playback, where the next track has a
> different sample rate.  This scenario is relatively frequent
> during normal use of a DAC and can't be avoided, as far as I can
> see.

That's why I was suggesting that you should be configuring the power
down time.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Need help fixing pop/click artifacts in an ASOC driver

[Dimitris Papavasiliou]

On 12/17/18 5:40 PM, Mark Brown wrote:
> That's why I was suggesting that you should be configuring the power
> down time.

I've already tried your suggestion, but unfortunately it doesn't
work in the general case, as it doesn't help when the hw_params
callback is called while the device is opened (but not playing).

I'm quoting my initial reply below for your convenience; you can look
up the whole message if needed.  Apologies in advance, if you've
replied to this and I've somehow missed it.

On 11/18/18 3:37 PM, Dimitris Papavasiliou wrote:
 > Thanks for clarifying about idle_bias_off/ignore_pmdown_time.  As
 > far as I can see, idle_bias_off seems to be set by default, as
 > the pcm512x driver is now a component driver and idle_bias_on is
 > not set to true.  I've also tried to explicitly set it to false,
 > as well as setting use_pmdown_time to false. As far as
 > ignore_pmdown_time is concerned, it seems to be settable at the
 > dai_link level now, which is convenient for my use case.
 >
 > Unfortunately none of this helps.  Although the chip is turned
 > off without delay, it's turned off only while the device is
 > closed.  As soon as the device is opened, it is turned on and
 > kept on during all subsequent hw_params calls, where clock
 > switching takes place.  The pops always get through.

Re: Need help fixing pop/click artifacts in an ASOC driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 945 bytes --]

On Mon, Dec 17, 2018 at 06:23:43PM +0200, Dimitris Papavasiliou wrote:
> On 12/17/18 5:40 PM, Mark Brown wrote:
> > That's why I was suggesting that you should be configuring the power
> > down time.

> I've already tried your suggestion, but unfortunately it doesn't
> work in the general case, as it doesn't help when the hw_params
> callback is called while the device is opened (but not playing).

The device being open shouldn't have any impact on the power state?

> > Unfortunately none of this helps.  Although the chip is turned
> > off without delay, it's turned off only while the device is
> > closed.  As soon as the device is opened, it is turned on and
> > kept on during all subsequent hw_params calls, where clock
> > switching takes place.  The pops always get through.

I would expect the stream to be closed and reopened by most applications
in between reconfigurations like that, it certainly used to be the
common pattern.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Need help fixing pop/click artifacts in an ASOC driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 689 bytes --]

On Mon, Dec 17, 2018 at 09:03:33AM -0600, Pierre-Louis Bossart wrote:

> I started prototyping a different approach where the codec driver passes the
> regmap information to the clock driver. What's missing in the patchset is
> the addition of a clock control in the machine driver, and logic added so
> that rate  change can only be done in a hw_params if there was a complete
> stop and reset on a DAPM_OFF event. compile-tested only for now.

> https://github.com/plbossart/sound/commits/hifiberry/clks

That looks a lot like the CODEC should be exporting a GPIO driver so the
machine driver doesn't actually need the regmap?  The only register
touched is _GPIO_CONTROL_1.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Need help fixing pop/click artifacts in an ASOC driver

[Pierre-Louis Bossart]

On 12/17/18 11:39 AM, Mark Brown wrote:
> On Mon, Dec 17, 2018 at 09:03:33AM -0600, Pierre-Louis Bossart wrote:
>
>> I started prototyping a different approach where the codec driver passes the
>> regmap information to the clock driver. What's missing in the patchset is
>> the addition of a clock control in the machine driver, and logic added so
>> that rate  change can only be done in a hw_params if there was a complete
>> stop and reset on a DAPM_OFF event. compile-tested only for now.
>> https://github.com/plbossart/sound/commits/hifiberry/clks
> That looks a lot like the CODEC should be exporting a GPIO driver so the
> machine driver doesn't actually need the regmap?  The only register
> touched is _GPIO_CONTROL_1.

I am not sure what you meant by 'exporting a GPIO driver' (mostly 
because I am not familiar with any GPIO framework) but indeed the local 
oscillator choice is controlled by a single register accessible through 
regmap - and changes to that register should only happen when the device 
is a specific state to prevent click/pops.

The machine driver should use clk_set_rate() and not directly handle 
regmap or codec stuff. If it does, or if the clock framework isn't 
relevant here then we can simplify all this as suggested in 
https://patchwork.kernel.org/patch/10444387/. What I was trying to do 
with the github update is to keep the clock framework, tie it closer 
with the codec parts with a state variable that prevents wild changes 
without going back to a 'safe' idle state (similar idea as PulseAudio 
clock changes, which can only happen when the PCM is not opened and used).

Re: Need help fixing pop/click artifacts in an ASOC driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1935 bytes --]

On Mon, Dec 17, 2018 at 12:08:36PM -0600, Pierre-Louis Bossart wrote:
> On 12/17/18 11:39 AM, Mark Brown wrote:

> > That looks a lot like the CODEC should be exporting a GPIO driver so the
> > machine driver doesn't actually need the regmap?  The only register
> > touched is _GPIO_CONTROL_1.

> I am not sure what you meant by 'exporting a GPIO driver' (mostly because I
> am not familiar with any GPIO framework) but indeed the local oscillator
> choice is controlled by a single register accessible through regmap - and
> changes to that register should only happen when the device is a specific
> state to prevent click/pops.

The GPIO framework provides a fairly simple view of GPIOs - from a user
point of view it's just getting or setting the value of a line.  It
looks like the register you're controlling isn't actually controlling a
chip feature directly but rather is setting a GPIO on the chip which
controls an external clock generator.  I could be misparsing things,
though.  I did glance at the pcm512x datasheet and didn't see pins or
anything that looked like an oscillator but I could've missed something.

> The machine driver should use clk_set_rate() and not directly handle regmap
> or codec stuff. If it does, or if the clock framework isn't relevant here
> then we can simplify all this as suggested in
> https://patchwork.kernel.org/patch/10444387/. What I was trying to do with
> the github update is to keep the clock framework, tie it closer with the
> codec parts with a state variable that prevents wild changes without going
> back to a 'safe' idle state (similar idea as PulseAudio clock changes, which
> can only happen when the PCM is not opened and used).

Right, I think bringing in the clock framework more is good -
effectively all I'm suggesting is changing the control interface used to
set the clock to add an indirection through gpiolib so you don't need to
pass the raw register map around.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Need help fixing pop/click artifacts in an ASOC driver

[Pierre-Louis Bossart]

On 12/17/18 1:02 PM, Mark Brown wrote:
> On Mon, Dec 17, 2018 at 12:08:36PM -0600, Pierre-Louis Bossart wrote:
>> On 12/17/18 11:39 AM, Mark Brown wrote:
>>> That looks a lot like the CODEC should be exporting a GPIO driver so the
>>> machine driver doesn't actually need the regmap?  The only register
>>> touched is _GPIO_CONTROL_1.
>> I am not sure what you meant by 'exporting a GPIO driver' (mostly because I
>> am not familiar with any GPIO framework) but indeed the local oscillator
>> choice is controlled by a single register accessible through regmap - and
>> changes to that register should only happen when the device is a specific
>> state to prevent click/pops.
> The GPIO framework provides a fairly simple view of GPIOs - from a user
> point of view it's just getting or setting the value of a line.  It
> looks like the register you're controlling isn't actually controlling a
> chip feature directly but rather is setting a GPIO on the chip which
> controls an external clock generator.  I could be misparsing things,
> though.  I did glance at the pcm512x datasheet and didn't see pins or
> anything that looked like an oscillator but I could've missed something.
You are correct, there are two variants of the Hifiberry DAC+, the 'PRO' 
version with two oscillators on board (SoC configured as bitclock and 
fsync slave) and the regular without (all clocks provided by the SoC in 
that case). It's indeed a GPIO control of an external component, not an 
on-chip capability.
>
>> The machine driver should use clk_set_rate() and not directly handle regmap
>> or codec stuff. If it does, or if the clock framework isn't relevant here
>> then we can simplify all this as suggested in
>> https://patchwork.kernel.org/patch/10444387/. What I was trying to do with
>> the github update is to keep the clock framework, tie it closer with the
>> codec parts with a state variable that prevents wild changes without going
>> back to a 'safe' idle state (similar idea as PulseAudio clock changes, which
>> can only happen when the PCM is not opened and used).
> Right, I think bringing in the clock framework more is good -
> effectively all I'm suggesting is changing the control interface used to
> set the clock to add an indirection through gpiolib so you don't need to
> pass the raw register map around.
this looks elegant indeed, will look into it. I'll have to learn more on 
gpios :-)

Re: Need help fixing pop/click artifacts in an ASOC driver

[Dimitris Papavasiliou]

On 12/17/18 6:52 PM, Mark Brown wrote:
> On Mon, Dec 17, 2018 at 06:23:43PM +0200, Dimitris Papavasiliou wrote:
>> I've already tried your suggestion, but unfortunately it doesn't
>> work in the general case, as it doesn't help when the hw_params
>> callback is called while the device is opened (but not playing).
> 
> The device being open shouldn't have any impact on the power state?

It's been a while since I tried it, but, as far as I recall, that was
the observed behavior (determined via printk statements in the CODEC's
set_bias_level callback).  To test potential fixes, I have a script
that opens the device and switches sampling rate between 44.1kHz and
48kHz every one second.  It popped almost every time.

(I've also tried variations of the script where a sample is played
every one second, again switching between the above sample rates and
where the device is closed an reopened at every switch.  The behavior
is mostly the same.)

If you're positive that the expected behavior should be different, I
could try it again, providing more details.

>>> Unfortunately none of this helps.  Although the chip is turned
>>> off without delay, it's turned off only while the device is
>>> closed.  As soon as the device is opened, it is turned on and
>>> kept on during all subsequent hw_params calls, where clock
>>> switching takes place.  The pops always get through.

> I would expect the stream to be closed and reopened by most applications
> in between reconfigurations like that, it certainly used to be the
> common pattern.

If by stream you mean the device, then perhaps they do.  As explained
above, I test using a script, instead of using applications, such as a
media player, as it's easier to reproduce the problem repeatedly at
will and under controlled conditions.  At any rate, a potential fix of
the problem, shouldn't depend on application behavior.

Re: Need help fixing pop/click artifacts in an ASOC driver

[Dimitris Papavasiliou]

On 12/17/18 8:08 PM, Pierre-Louis Bossart wrote:

> The machine driver should use clk_set_rate() and not directly handle 
> regmap or codec stuff. If it does, or if the clock framework isn't 
> relevant here then we can simplify all this as suggested in 
> https://patchwork.kernel.org/patch/10444387/. What I was trying to do 
> with the github update is to keep the clock framework, tie it closer 
> with the codec parts with a state variable that prevents wild changes 
> without going back to a 'safe' idle state (similar idea as PulseAudio 
> clock changes, which can only happen when the PCM is not opened and used).

I have documented, in a previous message, various approaches I tried
to prevent the pop, which were mostly based on the assumption that the
clock source was changed too "abruptly", or wasn't allowed to settle.
In the end, the only state, where the clocks can be switched without a
pop seemed to be when the chip is suspended.

It would of course be great if this could be achieved in a natural
manner, by waiting for the card to be suspended and only switching
clocks in such a state, but I don't see how this can be achieved
robustly.  The power management behavior is outside of the control of
the machine driver, so it can only refuse to switch sample rate when
it's not suspended.  Perhaps this will work in practice, but I fear it
would break applications depending on less restrained control of the
hardware.

Re: Need help fixing pop/click artifacts in an ASOC driver

[Pierre-Louis Bossart]

On 12/18/18 5:32 AM, Dimitris Papavasiliou wrote:
> On 12/17/18 8:08 PM, Pierre-Louis Bossart wrote:
>
>> The machine driver should use clk_set_rate() and not directly handle 
>> regmap or codec stuff. If it does, or if the clock framework isn't 
>> relevant here then we can simplify all this as suggested in 
>> https://patchwork.kernel.org/patch/10444387/. What I was trying to do 
>> with the github update is to keep the clock framework, tie it closer 
>> with the codec parts with a state variable that prevents wild changes 
>> without going back to a 'safe' idle state (similar idea as PulseAudio 
>> clock changes, which can only happen when the PCM is not opened and 
>> used).
>
> I have documented, in a previous message, various approaches I tried
> to prevent the pop, which were mostly based on the assumption that the
> clock source was changed too "abruptly", or wasn't allowed to settle.
> In the end, the only state, where the clocks can be switched without a
> pop seemed to be when the chip is suspended.
>
> It would of course be great if this could be achieved in a natural
> manner, by waiting for the card to be suspended and only switching
> clocks in such a state, but I don't see how this can be achieved
> robustly.  The power management behavior is outside of the control of
> the machine driver, so it can only refuse to switch sample rate when
> it's not suspended.  Perhaps this will work in practice, but I fear it
> would break applications depending on less restrained control of the
> hardware.

My point was that the machine driver can track DAPM events and put the 
device in a 'safe' state on SND_SOC_DAPM_EVENT_OFF so that the rate can 
be changed when playback restarts. I don't see what prevents us from 
using the same config as during suspend? It's pretty common to play with 
clocks with these events, and calling clk_disable_unprepare() could lead 
to whatever configuration sequence is needed for pop-free restart on 
startup.

Re: Need help fixing pop/click artifacts in an ASOC driver

[Dimitris Papavasiliou]

On 12/18/18 4:12 PM, Pierre-Louis Bossart wrote:
> My point was that the machine driver can track DAPM events and put the 
> device in a 'safe' state on SND_SOC_DAPM_EVENT_OFF so that the rate can 
> be changed when playback restarts. I don't see what prevents us from 
> using the same config as during suspend? It's pretty common to play with 
> clocks with these events, and calling clk_disable_unprepare() could lead 
> to whatever configuration sequence is needed for pop-free restart on 
> startup.

I only wanted to point out that, given the current DAPM configuration
of the pcm512x CODEC driver (as determined from monitoring calls to
the .set_bias_level callback, which seems to be the only place where
the power state of the chip is actually manipulated), it seemed like
it wasn't necessarily turned off between sample rate changes, and that
I'm not sure there's a 'safe' state you can put it in, so that it
won't pop on the next change to the clock source, if it's powered up
at the time.

The DAPM subsystem is complex and documentation on it is scarce, but
it seems the former can be fixed.  At least it is implied that the DAC
can be turned on/off at the stream domain level, which seems to be
tied to playback starting/stopping.  If the DAC can be suspended right
after playback stops and powered back up right before playback starts
again, then I don't think anything else needs to be done to avoid
pops, as all clock switching will have been done with the device
suspended.

If this is not possible, then I'm not sure if you can put the device
in a 'safe' state, say with both clocks turned off, or the clock
reference switched away from SCK, or something like that, so that it
won't pop when the sample rate (and hence clock source) is later
changed, while the DAC is powered up.

Anyway, calling my familiarity with the internals of ASOC drivers
superficial, would probably be stretching it, so the above is
(hopefully somewhat educated) guesswork at best.  Ignore it if it
doesn't make sense.

Re: Need help fixing pop/click artifacts in an ASOC driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1612 bytes --]

On Tue, Dec 18, 2018 at 12:39:09PM +0200, Dimitris Papavasiliou wrote:
> On 12/17/18 6:52 PM, Mark Brown wrote:

> > The device being open shouldn't have any impact on the power state?

> It's been a while since I tried it, but, as far as I recall, that was
> the observed behavior (determined via printk statements in the CODEC's
> set_bias_level callback).  To test potential fixes, I have a script
> that opens the device and switches sampling rate between 44.1kHz and
> 48kHz every one second.  It popped almost every time.

If you're saying you're keeping the device open while you're doing this
(rather than closing the stream and reconfiguring) then I'd expect
you're going to find other hardware which has similar issues, reclocking
the device isn't something I'd expect to be able to do glitch free.

> > I would expect the stream to be closed and reopened by most applications
> > in between reconfigurations like that, it certainly used to be the
> > common pattern.

> If by stream you mean the device, then perhaps they do.  As explained
> above, I test using a script, instead of using applications, such as a
> media player, as it's easier to reproduce the problem repeatedly at
> will and under controlled conditions.  At any rate, a potential fix of
> the problem, shouldn't depend on application behavior.

The application does have some responsibility for ensuring that things
work cleanly, there are limiations on what hardware can reasonably do
and often trying to do things behind the hardware to avoid problems in
unusual situations results in a lot of effort that's perhaps not worth
it.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Need help fixing pop/click artifacts in an ASOC driver

[Dimitris Papavasiliou]

On 12/19/18 6:19 PM, Mark Brown wrote:
> If you're saying you're keeping the device open while you're doing this
> (rather than closing the stream and reconfiguring) then I'd expect
> you're going to find other hardware which has similar issues, reclocking
> the device isn't something I'd expect to be able to do glitch free.

I got pops even if I opened the stream, called hw_params and closed it
on each iteration, but, as it turns out, that was in part because I
was being lazy, using a Python script to do the above, which sneaked
in a hw_params call immediately after opening the stream, to "reset"
it to default values.

So I've retried your suggested approach.  It looks like setting
.ignore_pmdown_time = 1 in the snd_soc_dai_link of the machine driver
is enough to cause immediate power ups/downs, but I get the following
behavior: The device gets powered up immediately after the first
hw_params call (even if nothing gets played subsequently) and it gets
powered down immediately after it's closed.  This does avoid the pop
when the device gets configured right after opening it, which, as you
say, seems to be what usually happens (I checked with the SoX play
utility and the MPD), but a pop is generated if there are multiple
hw_params calls and the sample rate is changed in any but the first one
(where the device is still powered down).

This can conceivably happen (for instance the Python library I was
using, forces you to make multiple hw_params calls, in order to set
more than one parameters).  Is there some way to configure the DAPM to
power up the card right before playback starts, and power it down
right after it stops?  This would prevent pops even in such
pathological cases.

>From what I could glean from the documentation on DAPM it seems like
that might be possible via stream domain widgets, but the CODEC driver
already defines those and the device is still powered up before
playback is requested.

Re: Need help fixing pop/click artifacts in an ASOC driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2091 bytes --]

On Wed, Dec 19, 2018 at 11:44:46PM +0200, Dimitris Papavasiliou wrote:
> On 12/19/18 6:19 PM, Mark Brown wrote:
> > If you're saying you're keeping the device open while you're doing this
> > (rather than closing the stream and reconfiguring) then I'd expect
> > you're going to find other hardware which has similar issues, reclocking
> > the device isn't something I'd expect to be able to do glitch free.

> I got pops even if I opened the stream, called hw_params and closed it
> on each iteration, but, as it turns out, that was in part because I
> was being lazy, using a Python script to do the above, which sneaked
> in a hw_params call immediately after opening the stream, to "reset"
> it to default values.

Wow, that's...  interesting.

> So I've retried your suggested approach.  It looks like setting
> .ignore_pmdown_time = 1 in the snd_soc_dai_link of the machine driver
> is enough to cause immediate power ups/downs, but I get the following
> behavior: The device gets powered up immediately after the first
> hw_params call (even if nothing gets played subsequently) and it gets

Are you sure it's the hw_params() call itself?

> powered down immediately after it's closed.  This does avoid the pop
> when the device gets configured right after opening it, which, as you
> say, seems to be what usually happens (I checked with the SoX play
> utility and the MPD), but a pop is generated if there are multiple
> hw_params calls and the sample rate is changed in any but the first one
> (where the device is still powered down).

> This can conceivably happen (for instance the Python library I was
> using, forces you to make multiple hw_params calls, in order to set
> more than one parameters).  Is there some way to configure the DAPM to
> power up the card right before playback starts, and power it down
> right after it stops?  This would prevent pops even in such
> pathological cases.

This is what's supposed to happen.  DAPM is triggered in the prepare()
callback, you can call hw_params() as often as you like and it should
have no interaction with DAPM at all.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Need help fixing pop/click artifacts in an ASOC driver

[Dimitris Papavasiliou]

On 12/20/18 5:36 PM, Mark Brown wrote:
> On Wed, Dec 19, 2018 at 11:44:46PM +0200, Dimitris Papavasiliou wrote:
>> I got pops even if I opened the stream, called hw_params and closed it
>> on each iteration, but, as it turns out, that was in part because I
>> was being lazy, using a Python script to do the above, which sneaked
>> in a hw_params call immediately after opening the stream, to "reset"
>> it to default values.
> 
> Wow, that's...  interesting.

I'm not sure if you're referring to the Python module's behavior, or to
the fact that I was so careless and am now boring you with it, so I'm
going to assume the former. :)

>> So I've retried your suggested approach.  It looks like setting
>> .ignore_pmdown_time = 1 in the snd_soc_dai_link of the machine driver
>> is enough to cause immediate power ups/downs, but I get the following
>> behavior: The device gets powered up immediately after the first
>> hw_params call (even if nothing gets played subsequently) and it gets
> 
> Are you sure it's the hw_params() call itself?
> 
>[...]
> 
> This is what's supposed to happen.  DAPM is triggered in the prepare()
> callback, you can call hw_params() as often as you like and it should
> have no interaction with DAPM at all.

Ah, well, the explanation then probably lies in the fact that
snd_pcm_hw_params, according to the documentation also prepares the
stream:

> Install one PCM hardware configuration chosen from a configuration
> space and snd_pcm_prepare it.

As far as I can see there's no other way to call hw_params from
userspace, except for snd_pcm_set_params, which is just a wrapper that
calls snd_pcm_hw_params internally, so I suppose calling hw_params to
configure the stream, must inevitably lead to the device being powered
up.

Unfortunately the documentation ("Writing an ALSA Driver") on the
prepare callback even warns explicitly, to:

> Be careful that this callback will be called many times at each
> setup, too.

I'm not sure if this implies that configuring the card in multiple
steps when setting up the stream is expected behavior, but it could be
interpreted that way.

Should we then accept, that some pops will be generated and hope that
they'll be suppressed by the digital_mute callback?

Re: Need help fixing pop/click artifacts in an ASOC driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1510 bytes --]

On Thu, Dec 20, 2018 at 10:41:40PM +0200, Dimitris Papavasiliou wrote:
> On 12/20/18 5:36 PM, Mark Brown wrote:

> > Install one PCM hardware configuration chosen from a configuration
> > space and snd_pcm_prepare it.

> As far as I can see there's no other way to call hw_params from
> userspace, except for snd_pcm_set_params, which is just a wrapper that
> calls snd_pcm_hw_params internally, so I suppose calling hw_params to
> configure the stream, must inevitably lead to the device being powered
> up.

OK, that's fun...

> Unfortunately the documentation ("Writing an ALSA Driver") on the
> prepare callback even warns explicitly, to:

> > Be careful that this callback will be called many times at each
> > setup, too.

> I'm not sure if this implies that configuring the card in multiple
> steps when setting up the stream is expected behavior, but it could be
> interpreted that way.

It's certainly possible if there needs to be negotation about the
parameters (but then you wouldn't prepare as the stream as you'd fail to
set the parameters) but it's not normal.  The repeated configurations
used to be much more common as OSS had separate calls to set each
parameter rather than the atomic configuration that ALSA has so you got
a hw_params() call for each OSS ioctl(), with native ALSA applications
it is more usual to get it right first time.

> Should we then accept, that some pops will be generated and hope that
> they'll be suppressed by the digital_mute callback?

It's probably easiest.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Need help fixing pop/click artifacts in an ASOC driver

[Dimitris Papavasiliou]

On 12/21/18 12:57 PM, Mark Brown wrote:
 >> Should we then accept, that some pops will be generated and hope that
 >> they'll be suppressed by the digital_mute callback?
 > It's probably easiest.

All right then; I'll just add the one line, setting
.ignore_pmdown_time = 1 to the machine driver and leave it at that.
I've been using the card with just the digital_mute patch for several
weeks and no pop got through, so it seems to be good enough, even with
just that.

First, I'd like to thank you for your support both in troubleshooting
this issue and in reviewing the patch I submitted.  I fully appreciate
I wasn't the easiest man to work with, being new to all of this and
mostly clueless, so your help is very much appreciated.

Now, I apologize in advance, but I'll ask you if you can comment on the
following final issue I have with this driver, which I was determined
to ignore, but can't: I've already mentioned that I'm using a card
made for the Raspberry Pi, and I can't really apply my patch there
as-is, because they seem to have changed the pcm512x CODEC driver,
without pushing the changes upstream.  You can see the patch in
question here:

https://github.com/raspberrypi/linux/commit/c53a137bd151130e29d7fc43947ac3e579a29ce4#diff-723fa079c49ec85d48e290fe84994b36

(My patch, by the way, does run without the above change if you use
the card without the external crystals, so I was able to test it in
the form I submitted it in.)

I can apply the patch via merge, so bothering with this is not at all
necessary, but if this change is useful, perhaps it's worth submitting
another patch for it.  I doubt this though, as it looks a bit hackish
to me.

To provide you with some context: The card uses a 24.576MHz crystal
for 48kHz-derived sampling rates, and it supports the S24_LE format,
which leads to non-integer LRCK dividers, and somewhat
faster-than-normal playback.  A solution to this, would be to play
24bit samples as 32-bit with the MSB zeroed out, as the chip supports
this.  As far as I understand it, the patch implements this, by
sending stereo streams as TDM data with two slots, one for each
channel, wide enough for the physical size of the sample, which
"happens" to be 32bits.  On the machine driver size the following is
done:

width = snd_pcm_format_physical_width(params_format(params));
ret = snd_soc_dai_set_tdm_slot(rtd->cpu_dai, 0x03, 0x03,
                                channels, width);
ret = snd_soc_dai_set_tdm_slot(rtd->codec_dai, 0x03, 0x03,
                                channels, width);

This works, but I'm not sure TDM is meant for things like that.
Anyway, if you think it's worth disucssing this, I can start a new
thread if you prefer.  If there's a straightforward way to fix this at
the machine driver level, that'd be even better, as it would allow me
to get rid of the divergence between the Pi and mainline kernel,
without bothering the latter.  If none of the above is the case, I can
happily just merge the patch as-is.

Re: Need help fixing pop/click artifacts in an ASOC driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1141 bytes --]

On Fri, Dec 21, 2018 at 03:05:51PM +0200, Dimitris Papavasiliou wrote:

> without pushing the changes upstream.  You can see the patch in
> question here:

> https://github.com/raspberrypi/linux/commit/c53a137bd151130e29d7fc43947ac3e579a29ce4#diff-723fa079c49ec85d48e290fe84994b36

That's a change half implementing TDM modes with the goal of allowing a
bodge to force 32 bit frame sizes where that's required to get clocks to
divide out neatly.

> I can apply the patch via merge, so bothering with this is not at all
> necessary, but if this change is useful, perhaps it's worth submitting
> another patch for it.  I doubt this though, as it looks a bit hackish
> to me.

It's useful (that's a genuine issue) but it shouldn't be bodged by TDM
mode but should instead just be done as standard without the machine
driver needing to do TDM - check if the clock rate required is actually
generated then fall back to a less exact frame if that helps.  That way
everyone gets the benefit without needing custom configuration or code.
Basically it's doing the right thing in terms of configuring the
hardware but should be triggered differently.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Need help fixing pop/click artifacts in an ASOC driver

[Matthias Reichl]

On Fri, Dec 21, 2018 at 03:05:51PM +0200, Dimitris Papavasiliou wrote:
> Now, I apologize in advance, but I'll ask you if you can comment on the
> following final issue I have with this driver, which I was determined
> to ignore, but can't: I've already mentioned that I'm using a card
> made for the Raspberry Pi, and I can't really apply my patch there
> as-is, because they seem to have changed the pcm512x CODEC driver,
> without pushing the changes upstream.  You can see the patch in
> question here:
> 
> https://github.com/raspberrypi/linux/commit/c53a137bd151130e29d7fc43947ac3e579a29ce4#diff-723fa079c49ec85d48e290fe84994b36

I didn't upstream the patch as I wasn't really satisfied with it
and also think it's quite hackish.

Back then when the Hifiberry folks submitted their machine driver
to the RPi kernel they sneaked in a change to the pcm512x which
forced S24 data to use 64fs clocking
https://github.com/raspberrypi/linux/pull/1152

This change had the nasty side effect that using pcm512x eg in slave
mode with the simple card driver broke as the setup on both sides
no longer matched. bcm2835 by default uses 32/48/64fs for 16/24/32
bit data - like most drivers do (also upstream pcm512x driver). pcm512x
with the change used 32/64/64fs - something that's not possible to
express in simple-card's binding.

So that change had to go and after dropping it we realized some control
over fs was needed if the machine driver wanted to support S24_LE
(just dropping S24_LE support and relying on alsalib to do automatic
24->32bit conversion would have been another option).

Doing that via the set_bclk_ratio interface would probably have been
the cleaner approach, OTOH set_tdm_slot had the nice feature that
it already had simple card DT bindings - so I (ab)used that.

I don't have a Hifiberry DAC+ card (or other cards with on-board
oscillators plus pcm512x in master mode) so I haven't looked into
better solutions. Main focus was to have some workaround that didn't
cause collateral damage but allowed the card to keep working.

I wouldn't mind at all if you do a proper implementation and submit it
upstream so the downstream patch can be dropped. 

so long,

Hias

> 
> (My patch, by the way, does run without the above change if you use
> the card without the external crystals, so I was able to test it in
> the form I submitted it in.)
> 
> I can apply the patch via merge, so bothering with this is not at all
> necessary, but if this change is useful, perhaps it's worth submitting
> another patch for it.  I doubt this though, as it looks a bit hackish
> to me.
> 
> To provide you with some context: The card uses a 24.576MHz crystal
> for 48kHz-derived sampling rates, and it supports the S24_LE format,
> which leads to non-integer LRCK dividers, and somewhat
> faster-than-normal playback.  A solution to this, would be to play
> 24bit samples as 32-bit with the MSB zeroed out, as the chip supports
> this.  As far as I understand it, the patch implements this, by
> sending stereo streams as TDM data with two slots, one for each
> channel, wide enough for the physical size of the sample, which
> "happens" to be 32bits.  On the machine driver size the following is
> done:
> 
> width = snd_pcm_format_physical_width(params_format(params));
> ret = snd_soc_dai_set_tdm_slot(rtd->cpu_dai, 0x03, 0x03,
>                                channels, width);
> ret = snd_soc_dai_set_tdm_slot(rtd->codec_dai, 0x03, 0x03,
>                                channels, width);
> 
> This works, but I'm not sure TDM is meant for things like that.
> Anyway, if you think it's worth disucssing this, I can start a new
> thread if you prefer.  If there's a straightforward way to fix this at
> the machine driver level, that'd be even better, as it would allow me
> to get rid of the divergence between the Pi and mainline kernel,
> without bothering the latter.  If none of the above is the case, I can
> happily just merge the patch as-is.
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: Need help fixing pop/click artifacts in an ASOC driver

[Dimitris Papavasiliou]

On Fri, Dec 21, 2018 at 7:33 PM Mark Brown <broonie@kernel.org> wrote:
>
> > I can apply the patch via merge, so bothering with this is not at all
> > necessary, but if this change is useful, perhaps it's worth submitting
> > another patch for it.  I doubt this though, as it looks a bit hackish
> > to me.
>
> It's useful (that's a genuine issue) but it shouldn't be bodged by TDM
> mode but should instead just be done as standard without the machine
> driver needing to do TDM - check if the clock rate required is actually
> generated then fall back to a less exact frame if that helps.  That way
> everyone gets the benefit without needing custom configuration or code.
> Basically it's doing the right thing in terms of configuring the
> hardware but should be triggered differently.

I suspected as much; I'll need to read up a bit on clocking though if
I'm going to give this a shot.  I'll be away from my computer during
the holidays so, if time permits, I'll give it a try with the new year
and open a new thread if I need feedback.  Thanks again for all your
help so far.

Re: Need help fixing pop/click artifacts in an ASOC driver

[Dimitris Papavasiliou]

On Sat, Dec 22, 2018 at 4:44 PM Matthias Reichl <hias@horus.com> wrote:
> I didn't upstream the patch as I wasn't really satisfied with it
> and also think it's quite hackish.
>
> [...]
>
> I wouldn't mind at all if you do a proper implementation and submit it
> upstream so the downstream patch can be dropped.

Thanks for chiming in Matthias!

Re: Need help fixing pop/click artifacts in an ASOC driver

[Pierre-Louis Bossart]

On 12/17/18 1:14 PM, Pierre-Louis Bossart wrote:
>
> On 12/17/18 1:02 PM, Mark Brown wrote:
>> On Mon, Dec 17, 2018 at 12:08:36PM -0600, Pierre-Louis Bossart wrote:
>>> On 12/17/18 11:39 AM, Mark Brown wrote:
>>>> That looks a lot like the CODEC should be exporting a GPIO driver 
>>>> so the
>>>> machine driver doesn't actually need the regmap?  The only register
>>>> touched is _GPIO_CONTROL_1.
>>> I am not sure what you meant by 'exporting a GPIO driver' (mostly 
>>> because I
>>> am not familiar with any GPIO framework) but indeed the local 
>>> oscillator
>>> choice is controlled by a single register accessible through regmap 
>>> - and
>>> changes to that register should only happen when the device is a 
>>> specific
>>> state to prevent click/pops.
>> The GPIO framework provides a fairly simple view of GPIOs - from a user
>> point of view it's just getting or setting the value of a line. It
>> looks like the register you're controlling isn't actually controlling a
>> chip feature directly but rather is setting a GPIO on the chip which
>> controls an external clock generator.  I could be misparsing things,
>> though.  I did glance at the pcm512x datasheet and didn't see pins or
>> anything that looked like an oscillator but I could've missed something.
> You are correct, there are two variants of the Hifiberry DAC+, the 
> 'PRO' version with two oscillators on board (SoC configured as 
> bitclock and fsync slave) and the regular without (all clocks provided 
> by the SoC in that case). It's indeed a GPIO control of an external 
> component, not an on-chip capability.
>>
>>> The machine driver should use clk_set_rate() and not directly handle 
>>> regmap
>>> or codec stuff. If it does, or if the clock framework isn't relevant 
>>> here
>>> then we can simplify all this as suggested in
>>> https://patchwork.kernel.org/patch/10444387/. What I was trying to 
>>> do with
>>> the github update is to keep the clock framework, tie it closer with 
>>> the
>>> codec parts with a state variable that prevents wild changes without 
>>> going
>>> back to a 'safe' idle state (similar idea as PulseAudio clock 
>>> changes, which
>>> can only happen when the PCM is not opened and used).
>> Right, I think bringing in the clock framework more is good -
>> effectively all I'm suggesting is changing the control interface used to
>> set the clock to add an indirection through gpiolib so you don't need to
>> pass the raw register map around.
> this looks elegant indeed, will look into it. I'll have to learn more 
> on gpios :-)

I was able to expose the 6 PCM512x GPIOs, toggle them from userspace as 
well as from the clock driver, it's indeed simple enough and could be 
useful for other usages, but there are still two issues with the concept:

1. After toggling a GPIO to enable the 44.1 or 48kHz oscillators, the 
clock driver cannot verify that the codec is locked, for this it'd need 
access to the regmap to check the PCM512x_RATE_DET_4 register. This is 
really needed to detect if the local oscillators are populated or not 
and hence detect the DAC+ PRO vs. the plain vanilla DAC+. An alternative 
would be to let the clock driver program GPIOs, and check in the codec 
driver is the clock is valid or not after the call to 
clk_prepare_enable() and fall back to slave mode in the latter case. I 
believe it'd be more elegant to do all these checks in the clock driver 
itself, and only register a clock if those local oscillators actually 
exist. I also can't figure out how to pass a regmap as platform data, 
the size argument can't be just a pointer and sizeof(*regmap) won't work 
either.

2. there is a mutual dependency between codec and clock driver. The 
codec exposes the gpios needed by the clock driver which in turn 
registers a clock needed by the codec driver. is it acceptable to create 
the clock platform device from the codec driver to remove any sort of 
race conditions or need for deferred probes?

Thanks

-Pierre


_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel