All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] stop setup_dig_out_stream() causing clicks
@ 2012-10-04 20:20 Laurence Darby
  2012-10-08 12:38 ` Takashi Iwai
  0 siblings, 1 reply; 6+ messages in thread
From: Laurence Darby @ 2012-10-04 20:20 UTC (permalink / raw)
  To: alsa-devel


Starting audio or seeking in various music players causes
setup_dig_out_stream() to be called, which resets the SPDIF stream,
which caused one DAC (but not another) to make a clicking noise every
time.

This patch turns off codec->spdif_status_reset after one reset which
stops further clicks.  One reset is still necessary to initialise the
codec properly.

Signed-off-by: Laurence Darby <ldarby@tuffmail.com>
---
2nd attempt at this, I hope I'm not doing anything wrong this time.

 sound/pci/hda/hda_codec.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 1c65cc5..7742a77 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -4731,9 +4731,11 @@ static void setup_dig_out_stream(struct hda_codec *codec, hda_nid_t nid,
 						   format);
 	}
 	/* turn on again (if needed) */
-	if (codec->spdif_status_reset && (spdif->ctls & AC_DIG1_ENABLE))
+	if (codec->spdif_status_reset && (spdif->ctls & AC_DIG1_ENABLE)) {
 		set_dig_out_convert(codec, nid,
 				    spdif->ctls & 0xff, -1);
+		codec->spdif_status_reset = 0;
+	}
 }
 
 static void cleanup_dig_out_stream(struct hda_codec *codec, hda_nid_t nid)
-- 
1.7.12

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] stop setup_dig_out_stream() causing clicks
  2012-10-04 20:20 [PATCH] stop setup_dig_out_stream() causing clicks Laurence Darby
@ 2012-10-08 12:38 ` Takashi Iwai
  2012-11-01  2:53   ` [PATCH v2] " Laurence Darby
  0 siblings, 1 reply; 6+ messages in thread
From: Takashi Iwai @ 2012-10-08 12:38 UTC (permalink / raw)
  To: Laurence Darby; +Cc: alsa-devel

At Thu, 4 Oct 2012 21:20:08 +0100,
Laurence Darby wrote:
> 
> 
> Starting audio or seeking in various music players causes
> setup_dig_out_stream() to be called, which resets the SPDIF stream,
> which caused one DAC (but not another) to make a clicking noise every
> time.
> 
> This patch turns off codec->spdif_status_reset after one reset which
> stops further clicks.  One reset is still necessary to initialise the
> codec properly.

The flag isn't supposed to be changed in such a dynamic way.
Certain codecs seem requiring resetting the SPDIF status bits, and
this flag indicates that.

That is, blindly clearing this flag is risky (and likely broken).

One better option would be to do the SPDIF status reset only when the
value is really changed.


thanks,

Takashi

> 
> Signed-off-by: Laurence Darby <ldarby@tuffmail.com>
> ---
> 2nd attempt at this, I hope I'm not doing anything wrong this time.
> 
>  sound/pci/hda/hda_codec.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index 1c65cc5..7742a77 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -4731,9 +4731,11 @@ static void setup_dig_out_stream(struct hda_codec *codec, hda_nid_t nid,
>  						   format);
>  	}
>  	/* turn on again (if needed) */
> -	if (codec->spdif_status_reset && (spdif->ctls & AC_DIG1_ENABLE))
> +	if (codec->spdif_status_reset && (spdif->ctls & AC_DIG1_ENABLE)) {
>  		set_dig_out_convert(codec, nid,
>  				    spdif->ctls & 0xff, -1);
> +		codec->spdif_status_reset = 0;
> +	}
>  }
>  
>  static void cleanup_dig_out_stream(struct hda_codec *codec, hda_nid_t nid)
> -- 
> 1.7.12
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] stop setup_dig_out_stream() causing clicks
  2012-10-08 12:38 ` Takashi Iwai
@ 2012-11-01  2:53   ` Laurence Darby
  2012-11-02  9:04     ` Takashi Iwai
  0 siblings, 1 reply; 6+ messages in thread
From: Laurence Darby @ 2012-11-01  2:53 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: alsa-devel

Starting audio or seeking in various music players causes
setup_dig_out_stream() to be called, which resets the SPDIF stream,
which caused one DAC (but not another) to make a clicking noise every
time.

This patch ensures the reset only happens when it needs to, which is
when the format changes, and makes the code a little more readable.

Signed-off-by: Laurence Darby <ldarby@tuffmail.com>
---
Takashi Iwai wrote:

> At Thu, 4 Oct 2012 21:20:08 +0100,
> Laurence Darby wrote:
> > 
> > 
> > Starting audio or seeking in various music players causes
> > setup_dig_out_stream() to be called, which resets the SPDIF stream,
> > which caused one DAC (but not another) to make a clicking noise
> > every time.
> > 
> > This patch turns off codec->spdif_status_reset after one reset which
> > stops further clicks.  One reset is still necessary to initialise
> > the codec properly.
> 
> The flag isn't supposed to be changed in such a dynamic way.
> Certain codecs seem requiring resetting the SPDIF status bits, and
> this flag indicates that.
> 
> That is, blindly clearing this flag is risky (and likely broken).
> 
> One better option would be to do the SPDIF status reset only when the
> value is really changed.
> 

Hi Takashi,

Thanks, you are right, if another audio player used a different format,
only noise was produced...  So v2 of the patch does the reset if the
format changes, (so changing players still results in a click, but that
seems to be unavoidable) and has some other changes which I think make
the code a bit more readable.  Could you please review this?

It's against the 3.6.4 stable branch (which is what I'm running), or
does it need to be against Linus's tree?

Thanks,
Laurence

 sound/pci/hda/hda_codec.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 38fdefc..6f7800a 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -4718,10 +4718,20 @@ EXPORT_SYMBOL_HDA(snd_hda_input_mux_put);
 static void setup_dig_out_stream(struct hda_codec *codec, hda_nid_t nid,
 				 unsigned int stream_tag, unsigned int format)
 {
-	struct hda_spdif_out *spdif = snd_hda_spdif_out_of_nid(codec, nid);
-
-	/* turn off SPDIF once; otherwise the IEC958 bits won't be updated */
-	if (codec->spdif_status_reset && (spdif->ctls & AC_DIG1_ENABLE))
+	struct hda_spdif_out *spdif;
+	unsigned int curr_fmt;
+	bool reset;
+
+	spdif = snd_hda_spdif_out_of_nid(codec, nid);
+	curr_fmt = snd_hda_codec_read(codec, nid, 0,
+				      AC_VERB_GET_STREAM_FORMAT, 0);
+	reset = codec->spdif_status_reset &&
+		spdif->ctls & AC_DIG1_ENABLE &&
+		curr_fmt != format;
+
+	/* turn off SPDIF if needed; otherwise the IEC958 bits won't be
+	   updated */
+	if (reset)
 		set_dig_out_convert(codec, nid,
 				    spdif->ctls & ~AC_DIG1_ENABLE & 0xff,
 				    -1);
@@ -4733,7 +4743,7 @@ static void setup_dig_out_stream(struct hda_codec *codec, hda_nid_t nid,
 						   format);
 	}
 	/* turn on again (if needed) */
-	if (codec->spdif_status_reset && (spdif->ctls & AC_DIG1_ENABLE))
+	if (reset)
 		set_dig_out_convert(codec, nid,
 				    spdif->ctls & 0xff, -1);
 }
-- 
1.7.12.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] stop setup_dig_out_stream() causing clicks
  2012-11-01  2:53   ` [PATCH v2] " Laurence Darby
@ 2012-11-02  9:04     ` Takashi Iwai
  2012-11-03 17:00       ` [PATCH v3] " Laurence Darby
  0 siblings, 1 reply; 6+ messages in thread
From: Takashi Iwai @ 2012-11-02  9:04 UTC (permalink / raw)
  To: Laurence Darby; +Cc: alsa-devel

At Thu, 1 Nov 2012 02:53:43 +0000,
Laurence Darby wrote:
> 
> Starting audio or seeking in various music players causes
> setup_dig_out_stream() to be called, which resets the SPDIF stream,
> which caused one DAC (but not another) to make a clicking noise every
> time.
> 
> This patch ensures the reset only happens when it needs to, which is
> when the format changes, and makes the code a little more readable.
> 
> Signed-off-by: Laurence Darby <ldarby@tuffmail.com>
> ---
> Takashi Iwai wrote:
> 
> > At Thu, 4 Oct 2012 21:20:08 +0100,
> > Laurence Darby wrote:
> > > 
> > > 
> > > Starting audio or seeking in various music players causes
> > > setup_dig_out_stream() to be called, which resets the SPDIF stream,
> > > which caused one DAC (but not another) to make a clicking noise
> > > every time.
> > > 
> > > This patch turns off codec->spdif_status_reset after one reset which
> > > stops further clicks.  One reset is still necessary to initialise
> > > the codec properly.
> > 
> > The flag isn't supposed to be changed in such a dynamic way.
> > Certain codecs seem requiring resetting the SPDIF status bits, and
> > this flag indicates that.
> > 
> > That is, blindly clearing this flag is risky (and likely broken).
> > 
> > One better option would be to do the SPDIF status reset only when the
> > value is really changed.
> > 
> 
> Hi Takashi,
> 
> Thanks, you are right, if another audio player used a different format,
> only noise was produced...  So v2 of the patch does the reset if the
> format changes, (so changing players still results in a click, but that
> seems to be unavoidable) and has some other changes which I think make
> the code a bit more readable.  Could you please review this?

Thanks, this looks much better now.
Just a nitpicking:

> 
> It's against the 3.6.4 stable branch (which is what I'm running), or
> does it need to be against Linus's tree?
> 
> Thanks,
> Laurence
> 
>  sound/pci/hda/hda_codec.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index 38fdefc..6f7800a 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -4718,10 +4718,20 @@ EXPORT_SYMBOL_HDA(snd_hda_input_mux_put);
>  static void setup_dig_out_stream(struct hda_codec *codec, hda_nid_t nid,
>  				 unsigned int stream_tag, unsigned int format)
>  {
> -	struct hda_spdif_out *spdif = snd_hda_spdif_out_of_nid(codec, nid);
> -
> -	/* turn off SPDIF once; otherwise the IEC958 bits won't be updated */
> -	if (codec->spdif_status_reset && (spdif->ctls & AC_DIG1_ENABLE))
> +	struct hda_spdif_out *spdif;
> +	unsigned int curr_fmt;
> +	bool reset;
> +
> +	spdif = snd_hda_spdif_out_of_nid(codec, nid);
> +	curr_fmt = snd_hda_codec_read(codec, nid, 0,
> +				      AC_VERB_GET_STREAM_FORMAT, 0);
> +	reset = codec->spdif_status_reset &&
> +		spdif->ctls & AC_DIG1_ENABLE &&
> +		curr_fmt != format;

Better to put parentheses around the bit operations when combined with
logical operations.  gcc would suggest it, too, I thought.



Takashi

> +
> +	/* turn off SPDIF if needed; otherwise the IEC958 bits won't be
> +	   updated */
> +	if (reset)
>  		set_dig_out_convert(codec, nid,
>  				    spdif->ctls & ~AC_DIG1_ENABLE & 0xff,
>  				    -1);
> @@ -4733,7 +4743,7 @@ static void setup_dig_out_stream(struct hda_codec *codec, hda_nid_t nid,
>  						   format);
>  	}
>  	/* turn on again (if needed) */
> -	if (codec->spdif_status_reset && (spdif->ctls & AC_DIG1_ENABLE))
> +	if (reset)
>  		set_dig_out_convert(codec, nid,
>  				    spdif->ctls & 0xff, -1);
>  }
> -- 
> 1.7.12.1
> 
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v3] stop setup_dig_out_stream() causing clicks
  2012-11-02  9:04     ` Takashi Iwai
@ 2012-11-03 17:00       ` Laurence Darby
  2012-11-04  8:17         ` Takashi Iwai
  0 siblings, 1 reply; 6+ messages in thread
From: Laurence Darby @ 2012-11-03 17:00 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: alsa-devel

Starting audio or seeking in various music players causes
setup_dig_out_stream() to be called, which resets the SPDIF stream,
which caused one DAC (but not another) to make a clicking noise every
time.

This patch ensures the reset only happens when it needs to, which is
when the format changes, and makes the code a little more readable.

Signed-off-by: Laurence Darby <ldarby@tuffmail.com>
---

Takashi Iwai wrote:

> > +	reset = codec->spdif_status_reset &&
> > +		spdif->ctls & AC_DIG1_ENABLE &&
> > +		curr_fmt != format;
> 
> Better to put parentheses around the bit operations when combined with
> logical operations.  gcc would suggest it, too, I thought.
> 

gcc doesn't warn about it, but it does on code like this:

	reset = codec->spdif_status_reset &&
		spdif->ctls & AC_DIG1_ENABLE + 0 &&
		curr_fmt != format;

which suggests putting them around the +. But since you spotted it, I've put them back in.

Regards,
Laurence


 sound/pci/hda/hda_codec.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 38fdefc..2b21906 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -4718,10 +4718,20 @@ EXPORT_SYMBOL_HDA(snd_hda_input_mux_put);
 static void setup_dig_out_stream(struct hda_codec *codec, hda_nid_t nid,
 				 unsigned int stream_tag, unsigned int format)
 {
-	struct hda_spdif_out *spdif = snd_hda_spdif_out_of_nid(codec, nid);
-
-	/* turn off SPDIF once; otherwise the IEC958 bits won't be updated */
-	if (codec->spdif_status_reset && (spdif->ctls & AC_DIG1_ENABLE))
+	struct hda_spdif_out *spdif;
+	unsigned int curr_fmt;
+	bool reset;
+
+	spdif = snd_hda_spdif_out_of_nid(codec, nid);
+	curr_fmt = snd_hda_codec_read(codec, nid, 0,
+				      AC_VERB_GET_STREAM_FORMAT, 0);
+	reset = codec->spdif_status_reset &&
+		(spdif->ctls & AC_DIG1_ENABLE) &&
+		curr_fmt != format;
+	
+	/* turn off SPDIF if needed; otherwise the IEC958 bits won't be
+	   updated */
+	if (reset)
 		set_dig_out_convert(codec, nid,
 				    spdif->ctls & ~AC_DIG1_ENABLE & 0xff,
 				    -1);
@@ -4733,7 +4743,7 @@ static void setup_dig_out_stream(struct hda_codec *codec, hda_nid_t nid,
 						   format);
 	}
 	/* turn on again (if needed) */
-	if (codec->spdif_status_reset && (spdif->ctls & AC_DIG1_ENABLE))
+	if (reset)
 		set_dig_out_convert(codec, nid,
 				    spdif->ctls & 0xff, -1);
 }
-- 
1.7.12.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH v3] stop setup_dig_out_stream() causing clicks
  2012-11-03 17:00       ` [PATCH v3] " Laurence Darby
@ 2012-11-04  8:17         ` Takashi Iwai
  0 siblings, 0 replies; 6+ messages in thread
From: Takashi Iwai @ 2012-11-04  8:17 UTC (permalink / raw)
  To: Laurence Darby; +Cc: alsa-devel

At Sat, 3 Nov 2012 17:00:06 +0000,
Laurence Darby wrote:
> 
> Starting audio or seeking in various music players causes
> setup_dig_out_stream() to be called, which resets the SPDIF stream,
> which caused one DAC (but not another) to make a clicking noise every
> time.
> 
> This patch ensures the reset only happens when it needs to, which is
> when the format changes, and makes the code a little more readable.
> 
> Signed-off-by: Laurence Darby <ldarby@tuffmail.com>
> ---
> 
> Takashi Iwai wrote:
> 
> > > +	reset = codec->spdif_status_reset &&
> > > +		spdif->ctls & AC_DIG1_ENABLE &&
> > > +		curr_fmt != format;
> > 
> > Better to put parentheses around the bit operations when combined with
> > logical operations.  gcc would suggest it, too, I thought.
> > 
> 
> gcc doesn't warn about it, but it does on code like this:
> 
> 	reset = codec->spdif_status_reset &&
> 		spdif->ctls & AC_DIG1_ENABLE + 0 &&
> 		curr_fmt != format;
> 
> which suggests putting them around the +. But since you spotted it, I've put them back in.

Thanks, applied now.


Takashi

> 
> Regards,
> Laurence
> 
> 
>  sound/pci/hda/hda_codec.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index 38fdefc..2b21906 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -4718,10 +4718,20 @@ EXPORT_SYMBOL_HDA(snd_hda_input_mux_put);
>  static void setup_dig_out_stream(struct hda_codec *codec, hda_nid_t nid,
>  				 unsigned int stream_tag, unsigned int format)
>  {
> -	struct hda_spdif_out *spdif = snd_hda_spdif_out_of_nid(codec, nid);
> -
> -	/* turn off SPDIF once; otherwise the IEC958 bits won't be updated */
> -	if (codec->spdif_status_reset && (spdif->ctls & AC_DIG1_ENABLE))
> +	struct hda_spdif_out *spdif;
> +	unsigned int curr_fmt;
> +	bool reset;
> +
> +	spdif = snd_hda_spdif_out_of_nid(codec, nid);
> +	curr_fmt = snd_hda_codec_read(codec, nid, 0,
> +				      AC_VERB_GET_STREAM_FORMAT, 0);
> +	reset = codec->spdif_status_reset &&
> +		(spdif->ctls & AC_DIG1_ENABLE) &&
> +		curr_fmt != format;
> +	
> +	/* turn off SPDIF if needed; otherwise the IEC958 bits won't be
> +	   updated */
> +	if (reset)
>  		set_dig_out_convert(codec, nid,
>  				    spdif->ctls & ~AC_DIG1_ENABLE & 0xff,
>  				    -1);
> @@ -4733,7 +4743,7 @@ static void setup_dig_out_stream(struct hda_codec *codec, hda_nid_t nid,
>  						   format);
>  	}
>  	/* turn on again (if needed) */
> -	if (codec->spdif_status_reset && (spdif->ctls & AC_DIG1_ENABLE))
> +	if (reset)
>  		set_dig_out_convert(codec, nid,
>  				    spdif->ctls & 0xff, -1);
>  }
> -- 
> 1.7.12.1
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-11-04  8:17 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-10-04 20:20 [PATCH] stop setup_dig_out_stream() causing clicks Laurence Darby
2012-10-08 12:38 ` Takashi Iwai
2012-11-01  2:53   ` [PATCH v2] " Laurence Darby
2012-11-02  9:04     ` Takashi Iwai
2012-11-03 17:00       ` [PATCH v3] " Laurence Darby
2012-11-04  8:17         ` Takashi Iwai

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.