diff mbox

[bug] Volume at maximum when track with different frequency is played with my RME sound card

Message ID s5h8u5ayzyb.wl-tiwai@suse.de (mailing list archive)
State New, archived
Headers show

Commit Message

Takashi Iwai Dec. 4, 2015, 3:51 p.m. UTC 
On Fri, 04 Dec 2015 16:14:56 +0100,
Maeda wrote:
> 
> OK :)
> 
> Tested-by: Sylvain LABOISNE <maeda1@free.fr>

Thanks, the official fix is below.  It'll be included in 4.4-rc5
(slipped from rc4) and backported to stable kernels later.


Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: rme96: Fix unexpected volume reset after rate changes

rme96 driver needs to reset DAC depending on the sample rate, and this
results in resetting to the max volume suddenly.  It's because of the
missing call of snd_rme96_apply_dac_volume().

However, calling this function right after the DAC reset still may not
work, and we need some delay before this call.  Since the DAC reset
and the procedure after that are performed in the spinlock, we delay
the DAC volume restore at the end after the spinlock.

Reported-and-tested-by: Sylvain LABOISNE <maeda1@free.fr>
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/rme96.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

Comments

Maeda Dec. 4, 2015, 5:13 p.m. UTC | #1 
Good job :)
Thanks for help and debug.

I'm waiting for the release then.
I don't log anything to the bug report I did 
(https://bugzilla.kernel.org/show_bug.cgi?id=105771). Let me know if I 
have to do something in addition.

Le 04/12/2015 16:51, Takashi Iwai a écrit :
> On Fri, 04 Dec 2015 16:14:56 +0100,
> Maeda wrote:
>> OK :)
>>
>> Tested-by: Sylvain LABOISNE <maeda1@free.fr>
> Thanks, the official fix is below.  It'll be included in 4.4-rc5
> (slipped from rc4) and backported to stable kernels later.
>
>
> Takashi
>
> -- 8< --
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] ALSA: rme96: Fix unexpected volume reset after rate changes
>
> rme96 driver needs to reset DAC depending on the sample rate, and this
> results in resetting to the max volume suddenly.  It's because of the
> missing call of snd_rme96_apply_dac_volume().
>
> However, calling this function right after the DAC reset still may not
> work, and we need some delay before this call.  Since the DAC reset
> and the procedure after that are performed in the spinlock, we delay
> the DAC volume restore at the end after the spinlock.
>
> Reported-and-tested-by: Sylvain LABOISNE <maeda1@free.fr>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>   sound/pci/rme96.c | 37 +++++++++++++++++++++++--------------
>   1 file changed, 23 insertions(+), 14 deletions(-)
>
> diff --git a/sound/pci/rme96.c b/sound/pci/rme96.c
> index 714df906249e..e4229d01cf6a 100644
> --- a/sound/pci/rme96.c
> +++ b/sound/pci/rme96.c
> @@ -741,10 +741,11 @@ snd_rme96_playback_setrate(struct rme96 *rme96,
>   	{
>   		/* change to/from double-speed: reset the DAC (if available) */
>   		snd_rme96_reset_dac(rme96);
> +		return 1; /* need to restore volume */
>   	} else {
>   		writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
> +		return 0;
>   	}
> -	return 0;
>   }
>   
>   static int
> @@ -980,6 +981,7 @@ snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
>   	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
>   	struct snd_pcm_runtime *runtime = substream->runtime;
>   	int err, rate, dummy;
> +	bool apply_dac_volume = false;
>   
>   	runtime->dma_area = (void __force *)(rme96->iobase +
>   					     RME96_IO_PLAY_BUFFER);
> @@ -993,24 +995,24 @@ snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
>   	{
>                   /* slave clock */
>                   if ((int)params_rate(params) != rate) {
> -			spin_unlock_irq(&rme96->lock);
> -			return -EIO;
> -                }
> -	} else if ((err = snd_rme96_playback_setrate(rme96, params_rate(params))) < 0) {
> -		spin_unlock_irq(&rme96->lock);
> -		return err;
> -	}
> -	if ((err = snd_rme96_playback_setformat(rme96, params_format(params))) < 0) {
> -		spin_unlock_irq(&rme96->lock);
> -		return err;
> +			err = -EIO;
> +			goto error;
> +		}
> +	} else {
> +		err = snd_rme96_playback_setrate(rme96, params_rate(params));
> +		if (err < 0)
> +			goto error;
> +		apply_dac_volume = err > 0; /* need to restore volume later? */
>   	}
> +	if ((err = snd_rme96_playback_setformat(rme96, params_format(params))) < 0)
> +		goto error;
>   	snd_rme96_setframelog(rme96, params_channels(params), 1);
>   	if (rme96->capture_periodsize != 0) {
>   		if (params_period_size(params) << rme96->playback_frlog !=
>   		    rme96->capture_periodsize)
>   		{
> -			spin_unlock_irq(&rme96->lock);
> -			return -EBUSY;
> +			err = -EBUSY;
> +			goto error;
>   		}
>   	}
>   	rme96->playback_periodsize =
> @@ -1022,8 +1024,15 @@ snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
>   		writel(rme96->wcreg |= rme96->wcreg_spdif_stream, rme96->iobase + RME96_IO_CONTROL_REGISTER);
>   	}
>   	spin_unlock_irq(&rme96->lock);
> +	err = 0;
>   		
> -	return 0;
> + error:
> +	if (apply_dac_volume) {
> +		usleep_range(3000, 10000);
> +		snd_rme96_apply_dac_volume(rme96);
> +	}
> +
> +	return err;
>   }
>   
>   static int
diff mbox

Patch

diff --git a/sound/pci/rme96.c b/sound/pci/rme96.c
index 714df906249e..e4229d01cf6a 100644
--- a/sound/pci/rme96.c
+++ b/sound/pci/rme96.c
@@ -741,10 +741,11 @@  snd_rme96_playback_setrate(struct rme96 *rme96,
 	{
 		/* change to/from double-speed: reset the DAC (if available) */
 		snd_rme96_reset_dac(rme96);
+		return 1; /* need to restore volume */
 	} else {
 		writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
+		return 0;
 	}
-	return 0;
 }
 
 static int
@@ -980,6 +981,7 @@  snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
 	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	int err, rate, dummy;
+	bool apply_dac_volume = false;
 
 	runtime->dma_area = (void __force *)(rme96->iobase +
 					     RME96_IO_PLAY_BUFFER);
@@ -993,24 +995,24 @@  snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
 	{
                 /* slave clock */
                 if ((int)params_rate(params) != rate) {
-			spin_unlock_irq(&rme96->lock);
-			return -EIO;                    
-                }
-	} else if ((err = snd_rme96_playback_setrate(rme96, params_rate(params))) < 0) {
-		spin_unlock_irq(&rme96->lock);
-		return err;
-	}
-	if ((err = snd_rme96_playback_setformat(rme96, params_format(params))) < 0) {
-		spin_unlock_irq(&rme96->lock);
-		return err;
+			err = -EIO;
+			goto error;
+		}
+	} else {
+		err = snd_rme96_playback_setrate(rme96, params_rate(params));
+		if (err < 0)
+			goto error;
+		apply_dac_volume = err > 0; /* need to restore volume later? */
 	}
+	if ((err = snd_rme96_playback_setformat(rme96, params_format(params))) < 0)
+		goto error;
 	snd_rme96_setframelog(rme96, params_channels(params), 1);
 	if (rme96->capture_periodsize != 0) {
 		if (params_period_size(params) << rme96->playback_frlog !=
 		    rme96->capture_periodsize)
 		{
-			spin_unlock_irq(&rme96->lock);
-			return -EBUSY;
+			err = -EBUSY;
+			goto error;
 		}
 	}
 	rme96->playback_periodsize =
@@ -1022,8 +1024,15 @@  snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
 		writel(rme96->wcreg |= rme96->wcreg_spdif_stream, rme96->iobase + RME96_IO_CONTROL_REGISTER);
 	}
 	spin_unlock_irq(&rme96->lock);
+	err = 0;
 		
-	return 0;
+ error:
+	if (apply_dac_volume) {
+		usleep_range(3000, 10000);
+		snd_rme96_apply_dac_volume(rme96);
+	}
+
+	return err;
 }
 
 static int