ALSA: oxygen: Fix S/PDIF muting
diff mbox

Message ID 20150628131337.d8077aed73a9851b194bdc30@tuffmail.com
State New
Headers show

Commit Message

Laurence Darby June 28, 2015, 12:13 p.m. UTC
The S/PDIF output was muted whenever audio wasn't playing which resulted
in a clicking noise from the DAC when resuming (similar to the issue in
3bef1c377d1b (ALSA: hda - stop setup_dig_out_stream() causing clicks)).
Also, the mixer control wasn't actually controlling the S/PDIF output.

 - Fix the initial state of the mixer to match the initial muted state.
 - Mute the output at shutdown.
 - Delete the code that modifies OXYGEN_SPDIF_OUT_ENABLE, and modify it
   only in oxygen_update_spdif_source(), so only the mixer control
   controls it.

Signed-off-by: Laurence Darby <ldarby@tuffmail.com>
---
Patch is against master, tested on 4.0.

 sound/pci/oxygen/oxygen_lib.c   |  5 ++++-
 sound/pci/oxygen/oxygen_mixer.c |  9 +++++----
 sound/pci/oxygen/oxygen_pcm.c   | 15 +--------------
 3 files changed, 10 insertions(+), 19 deletions(-)

Comments

Clemens Ladisch June 28, 2015, 6:44 p.m. UTC | #1
Laurence Darby wrote:
> The S/PDIF output was muted whenever audio wasn't playing which resulted
> in a clicking noise from the DAC when resuming

It indeed appears that there are too many oxygen_clear_bits32() calls.
However, I don't trust the hardware; please confirm that the S/PDIF
output, without an active stream, play zeros and not the last sample.

> Also, the mixer control wasn't actually controlling the S/PDIF output.

Because that is not its purpose to begin with.  >:-)

The mixer control enables copying the "Multichannel" device to the
S/PDIF output.  The "Digital" device has no mute, and takes precedence
over the multichannel device when both are active.


Regards,
Clemens
Laurence Darby June 29, 2015, 8:01 p.m. UTC | #2
On Sun, 28 Jun 2015 20:44:26 +0200 Clemens Ladisch wrote:

> Laurence Darby wrote:
> > The S/PDIF output was muted whenever audio wasn't playing which
> > resulted in a clicking noise from the DAC when resuming
> 
> It indeed appears that there are too many oxygen_clear_bits32() calls.
> However, I don't trust the hardware; please confirm that the S/PDIF
> output, without an active stream, play zeros and not the last sample.

Well, to test that I created a wav file of about a quarter of a sine
wave (at about -15dB), consisting of 5520 samples, so it matched the
alsa buffer size, otherwise aplay pads the buffer with silence.

The chip does continue playing the last sample value, then the speakers
pop later when something else is played.  Is this really an issue
though? I just found my intel hda chip behaves the same way, and that
chip's driver leaves it un-muted.

If this is still going to prevent fixing the popping noise for the
oxygen chip, what about writing a single 0x00 sample to it in the
driver instead of muting it?

> 
> > Also, the mixer control wasn't actually controlling the S/PDIF
> > output.
> 
> Because that is not its purpose to begin with.  >:-)
> 
> The mixer control enables copying the "Multichannel" device to the
> S/PDIF output.  The "Digital" device has no mute, and takes precedence
> over the multichannel device when both are active.
> 

All my other sound cards (hda, cmedia & emu10k) have an S/PDIF mixer
control that mutes the output (i.e. causes the DAC to lose the signal),
in the same way as disabling OXYGEN_SPDIF_OUT_ENABLE does, so I would
say that it does have mute, and I thought it should be controlled by
the variable called "spdif_playback_enable"...

Would an acceptable fix be to create another mixer control for muting
(and also rename the existing S/PDIF to e.g. "S/PDIF Multichannel"
so its clearer what that's for)?

Regards,
Laurence
Clemens Ladisch June 30, 2015, 9:43 a.m. UTC | #3
Laurence Darby wrote:
> On Sun, 28 Jun 2015 20:44:26 +0200 Clemens Ladisch wrote:
>> Laurence Darby wrote:
>>> The S/PDIF output was muted whenever audio wasn't playing which
>>> resulted in a clicking noise from the DAC when resuming
>>
>> It indeed appears that there are too many oxygen_clear_bits32() calls.
>> However, I don't trust the hardware; please confirm that the S/PDIF
>> output, without an active stream, play zeros and not the last sample.
>
> Well, to test that I created a wav file of about a quarter of a sine
> wave (at about -15dB), consisting of 5520 samples, so it matched the
> alsa buffer size, otherwise aplay pads the buffer with silence.
>
> The chip does continue playing the last sample value, then the speakers
> pop later when something else is played.  Is this really an issue
> though? I just found my intel hda chip behaves the same way, and that
> chip's driver leaves it un-muted.

The current problem is that the output gets disabled, then later, when
the driver is about to start the new stream, it enables the output again
(which outputs the last old sample), and then the new stream starts
(usually with zero samples).  Going from disabled(=zero) to old-sample
to new-sample(=zero) is an extra pop.

In any case, putting a DC offset on the speakers is a bad idea.

> If this is still going to prevent fixing the popping noise for the
> oxygen chip, what about writing a single 0x00 sample to it in the
> driver instead of muting it?

This is likely to work (but the zero sample has to go through DMA).

At the moment, oxygen_hw_free() pokes at the OXYGEN_DMA_FLUSH register.
Please check if there is an improvement if it also pokes at the
OXYGEN_DMA_RESET register.

>>> Also, the mixer control wasn't actually controlling the S/PDIF
>>> output.
>>
>> Because that is not its purpose to begin with.  >:-)
>>
>> The mixer control enables copying the "Multichannel" device to the
>> S/PDIF output.  The "Digital" device has no mute, and takes precedence
>> over the multichannel device when both are active.
>
> All my other sound cards (hda, cmedia & emu10k) have an S/PDIF mixer
> control that mutes the output (i.e. causes the DAC to lose the signal),
> in the same way as disabling OXYGEN_SPDIF_OUT_ENABLE does, so I would
> say that it does have mute, and I thought it should be controlled by
> the variable called "spdif_playback_enable"...

I don't remember what the reason for the behaviour of the current
controls was; I probably copied them from some other driver.

> Would an acceptable fix be to create another mixer control for muting
> (and also rename the existing S/PDIF to e.g. "S/PDIF Multichannel"
> so its clearer what that's for)?

AFAICS the usual name would be "PCM".


When making separate changes, please send separate patches.


Regards,
Clemens
Laurence Darby July 5, 2015, 6:56 p.m. UTC | #4
On Tue, 30 Jun 2015 11:43:52 +0200 Clemens Ladisch wrote:

> Laurence Darby wrote:
> > On Sun, 28 Jun 2015 20:44:26 +0200 Clemens Ladisch wrote:
> >> Laurence Darby wrote:
> >>> The S/PDIF output was muted whenever audio wasn't playing which
> >>> resulted in a clicking noise from the DAC when resuming
> >>
> >> It indeed appears that there are too many oxygen_clear_bits32()
> >> calls. However, I don't trust the hardware; please confirm that
> >> the S/PDIF output, without an active stream, play zeros and not
> >> the last sample.
> >
> > Well, to test that I created a wav file of about a quarter of a sine
> > wave (at about -15dB), consisting of 5520 samples, so it matched the
> > alsa buffer size, otherwise aplay pads the buffer with silence.
> >
> > The chip does continue playing the last sample value, then the
> > speakers pop later when something else is played.  Is this really
> > an issue though? I just found my intel hda chip behaves the same
> > way, and that chip's driver leaves it un-muted.
> 
> The current problem is that the output gets disabled, then later, when
> the driver is about to start the new stream, it enables the output
> again (which outputs the last old sample), and then the new stream
> starts (usually with zero samples).  Going from disabled(=zero) to
> old-sample to new-sample(=zero) is an extra pop.
> 
> In any case, putting a DC offset on the speakers is a bad idea.
> 
> > If this is still going to prevent fixing the popping noise for the
> > oxygen chip, what about writing a single 0x00 sample to it in the
> > driver instead of muting it?
> 
> This is likely to work (but the zero sample has to go through DMA).

Ok, I eventually got this to work, with this in oxygen_spdif_hw_free():

	memset (runtime->dma_area, 0, runtime->dma_bytes);
	oxygen_trigger(substream,SNDRV_PCM_TRIGGER_START);
	msleep (1);
	oxygen_trigger(substream,SNDRV_PCM_TRIGGER_STOP);
	
instead of disabling OXYGEN_SPDIF_OUT_ENABLE.  The 1ms sleep is
necessary otherwise it intermittently still leaves DC on the output.

If I create a proper patch for that (it should probably use
snd_pcm_format_set_silence() instead of memset) would that be
acceptable? 

> At the moment, oxygen_hw_free() pokes at the OXYGEN_DMA_FLUSH
> register. Please check if there is an improvement if it also pokes at
> the OXYGEN_DMA_RESET register.

Unfortunately not.  I did some archeology and found your CVS commit
from 2007: http://sourceforge.net/p/alsa/mailman/message/15439076/
which has this code:

+	if (chip->CMI8788IC_revision == CMI8788IC_Revision1)
+		DMARestRegister = PCI_DMA_Reset;
+	if (chip->CMI8788IC_revision == CMI8788IC_Revision2)
+		DMARestRegister = PCI_DMA_FLUSH;
...
+		/* Reset DMA Channel*/
+		reset = snd_cmipci_read_b(chip, DMARestRegister);
+		reset |= cmi_subs->DMA_chan_reset; /* set bit */

So I understand why you asked if the RESET works :) 

Regards,
Laurence
Clemens Ladisch July 5, 2015, 7:23 p.m. UTC | #5
Laurence Darby wrote:
> On Tue, 30 Jun 2015 11:43:52 +0200 Clemens Ladisch wrote:
>> Laurence Darby wrote:
>>> If this is still going to prevent fixing the popping noise for the
>>> oxygen chip, what about writing a single 0x00 sample to it in the
>>> driver instead of muting it?
>>
>> This is likely to work (but the zero sample has to go through DMA).
>
> Ok, I eventually got this to work, with this in oxygen_spdif_hw_free():
>
> 	memset (runtime->dma_area, 0, runtime->dma_bytes);
> 	oxygen_trigger(substream,SNDRV_PCM_TRIGGER_START);
> 	msleep (1);
> 	oxygen_trigger(substream,SNDRV_PCM_TRIGGER_STOP);

Please note that hw_free can be called without a valid configuration.

Calling oxygen_trigger() might affect other streams.

> If I create a proper patch for that (it should probably use
> snd_pcm_format_set_silence() instead of memset)

All supported sample formats uses plain zeros.


Regards,
Clemens

Patch
diff mbox

diff --git a/sound/pci/oxygen/oxygen_lib.c b/sound/pci/oxygen/oxygen_lib.c
index b4ef580..2cca908 100644
--- a/sound/pci/oxygen/oxygen_lib.c
+++ b/sound/pci/oxygen/oxygen_lib.c
@@ -373,7 +373,7 @@  static void oxygen_init(struct oxygen *chip)
 	for (i = 0; i < 8; ++i)
 		chip->dac_volume[i] = chip->model.dac_volume_min;
 	chip->dac_mute = 1;
-	chip->spdif_playback_enable = 1;
+	chip->spdif_playback_enable = 0;
 	chip->spdif_bits = OXYGEN_SPDIF_C | OXYGEN_SPDIF_ORIGINAL |
 		(IEC958_AES1_CON_PCM_CODER << OXYGEN_SPDIF_CATEGORY_SHIFT);
 	chip->spdif_pcm_bits = chip->spdif_bits;
@@ -575,6 +575,9 @@  static void oxygen_shutdown(struct oxygen *chip)
 	spin_lock_irq(&chip->reg_lock);
 	chip->interrupt_mask = 0;
 	chip->pcm_running = 0;
+	chip->spdif_playback_enable = 0;
+	oxygen_clear_bits32(chip, OXYGEN_SPDIF_CONTROL,
+			    OXYGEN_SPDIF_OUT_ENABLE);
 	oxygen_write16(chip, OXYGEN_DMA_STATUS, 0);
 	oxygen_write16(chip, OXYGEN_INTERRUPT_MASK, 0);
 	spin_unlock_irq(&chip->reg_lock);
diff --git a/sound/pci/oxygen/oxygen_mixer.c b/sound/pci/oxygen/oxygen_mixer.c
index 6492bca..20a2781 100644
--- a/sound/pci/oxygen/oxygen_mixer.c
+++ b/sound/pci/oxygen/oxygen_mixer.c
@@ -252,7 +252,6 @@  void oxygen_update_spdif_source(struct oxygen *chip)
 	old_control = oxygen_read32(chip, OXYGEN_SPDIF_CONTROL);
 	old_routing = oxygen_read16(chip, OXYGEN_PLAY_ROUTING);
 	if (chip->pcm_active & (1 << PCM_SPDIF)) {
-		new_control = old_control | OXYGEN_SPDIF_OUT_ENABLE;
 		new_routing = (old_routing & ~OXYGEN_PLAY_SPDIF_MASK)
 			| OXYGEN_PLAY_SPDIF_SPDIF;
 		oxygen_rate = (old_control >> OXYGEN_SPDIF_OUT_RATE_SHIFT)
@@ -265,13 +264,15 @@  void oxygen_update_spdif_source(struct oxygen *chip)
 		oxygen_rate = oxygen_read16(chip, OXYGEN_I2S_MULTICH_FORMAT)
 			& OXYGEN_I2S_RATE_MASK;
 		new_control = (old_control & ~OXYGEN_SPDIF_OUT_RATE_MASK) |
-			(oxygen_rate << OXYGEN_SPDIF_OUT_RATE_SHIFT) |
-			OXYGEN_SPDIF_OUT_ENABLE;
+			(oxygen_rate << OXYGEN_SPDIF_OUT_RATE_SHIFT);
 	} else {
-		new_control = old_control & ~OXYGEN_SPDIF_OUT_ENABLE;
 		new_routing = old_routing;
 		oxygen_rate = OXYGEN_RATE_44100;
 	}
+	if (chip->spdif_playback_enable)
+		new_control = old_control | OXYGEN_SPDIF_OUT_ENABLE;
+	else
+		new_control = old_control & ~OXYGEN_SPDIF_OUT_ENABLE;
 	if (old_routing != new_routing) {
 		oxygen_write32(chip, OXYGEN_SPDIF_CONTROL,
 			       new_control & ~OXYGEN_SPDIF_OUT_ENABLE);
diff --git a/sound/pci/oxygen/oxygen_pcm.c b/sound/pci/oxygen/oxygen_pcm.c
index aa2ebd1..f4a4a48 100644
--- a/sound/pci/oxygen/oxygen_pcm.c
+++ b/sound/pci/oxygen/oxygen_pcm.c
@@ -477,8 +477,6 @@  static int oxygen_spdif_hw_params(struct snd_pcm_substream *substream,
 
 	mutex_lock(&chip->mutex);
 	spin_lock_irq(&chip->reg_lock);
-	oxygen_clear_bits32(chip, OXYGEN_SPDIF_CONTROL,
-			    OXYGEN_SPDIF_OUT_ENABLE);
 	oxygen_write8_masked(chip, OXYGEN_PLAY_FORMAT,
 			     oxygen_format(hw_params) << OXYGEN_SPDIF_FORMAT_SHIFT,
 			     OXYGEN_SPDIF_FORMAT_MASK);
@@ -544,17 +542,6 @@  static int oxygen_hw_free(struct snd_pcm_substream *substream)
 	return snd_pcm_lib_free_pages(substream);
 }
 
-static int oxygen_spdif_hw_free(struct snd_pcm_substream *substream)
-{
-	struct oxygen *chip = snd_pcm_substream_chip(substream);
-
-	spin_lock_irq(&chip->reg_lock);
-	oxygen_clear_bits32(chip, OXYGEN_SPDIF_CONTROL,
-			    OXYGEN_SPDIF_OUT_ENABLE);
-	spin_unlock_irq(&chip->reg_lock);
-	return oxygen_hw_free(substream);
-}
-
 static int oxygen_prepare(struct snd_pcm_substream *substream)
 {
 	struct oxygen *chip = snd_pcm_substream_chip(substream);
@@ -669,7 +656,7 @@  static struct snd_pcm_ops oxygen_spdif_ops = {
 	.close     = oxygen_close,
 	.ioctl     = snd_pcm_lib_ioctl,
 	.hw_params = oxygen_spdif_hw_params,
-	.hw_free   = oxygen_spdif_hw_free,
+	.hw_free   = oxygen_hw_free,
 	.prepare   = oxygen_prepare,
 	.trigger   = oxygen_trigger,
 	.pointer   = oxygen_pointer,