alsa-devel.alsa-project.org archive mirror
 help / color / mirror / Atom feed
* [alsa-devel] [PATCH] ALSA: hda: Modify stream stripe mask only when needed
@ 2019-12-02  7:49 Takashi Iwai
  2019-12-15 16:36 ` Martin Regner
  2019-12-15 18:57 ` Martin Regner
  0 siblings, 2 replies; 5+ messages in thread
From: Takashi Iwai @ 2019-12-02  7:49 UTC (permalink / raw)
  To: alsa-devel

The recent in HD-audio stream management for changing the stripe
control seems causing a regression on some platforms.  The stripe
control is currently used only by HDMI codec, and applying the stripe
mask unconditionally may lead to scratchy and static noises as seen on
some MacBooks.

For addressing the regression, this patch changes the stream
management code to apply the stripe mask conditionally only when the
codec driver requested.

Fixes: 9b6f7e7a296e ("ALSA: hda: program stripe bits for controller")
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=204477
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/hdaudio.h    |  1 +
 sound/hda/hdac_stream.c    | 19 ++++++++++++-------
 sound/pci/hda/patch_hdmi.c |  5 +++++
 3 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index b260c5fd2337..e05b95e83d5a 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -493,6 +493,7 @@ struct hdac_stream {
 	bool prepared:1;
 	bool no_period_wakeup:1;
 	bool locked:1;
+	bool stripe:1;			/* apply stripe control */
 
 	/* timestamp */
 	unsigned long start_wallclk;	/* start + minimum wallclk */
diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c
index d8fe7ff0cd58..f9707fb05efe 100644
--- a/sound/hda/hdac_stream.c
+++ b/sound/hda/hdac_stream.c
@@ -96,12 +96,14 @@ void snd_hdac_stream_start(struct hdac_stream *azx_dev, bool fresh_start)
 			      1 << azx_dev->index,
 			      1 << azx_dev->index);
 	/* set stripe control */
-	if (azx_dev->substream)
-		stripe_ctl = snd_hdac_get_stream_stripe_ctl(bus, azx_dev->substream);
-	else
-		stripe_ctl = 0;
-	snd_hdac_stream_updateb(azx_dev, SD_CTL_3B, SD_CTL_STRIPE_MASK,
-				stripe_ctl);
+	if (azx_dev->stripe) {
+		if (azx_dev->substream)
+			stripe_ctl = snd_hdac_get_stream_stripe_ctl(bus, azx_dev->substream);
+		else
+			stripe_ctl = 0;
+		snd_hdac_stream_updateb(azx_dev, SD_CTL_3B, SD_CTL_STRIPE_MASK,
+					stripe_ctl);
+	}
 	/* set DMA start and interrupt mask */
 	snd_hdac_stream_updateb(azx_dev, SD_CTL,
 				0, SD_CTL_DMA_START | SD_INT_MASK);
@@ -118,7 +120,10 @@ void snd_hdac_stream_clear(struct hdac_stream *azx_dev)
 	snd_hdac_stream_updateb(azx_dev, SD_CTL,
 				SD_CTL_DMA_START | SD_INT_MASK, 0);
 	snd_hdac_stream_writeb(azx_dev, SD_STS, SD_INT_MASK); /* to be sure */
-	snd_hdac_stream_updateb(azx_dev, SD_CTL_3B, SD_CTL_STRIPE_MASK, 0);
+	if (azx_dev->stripe) {
+		snd_hdac_stream_updateb(azx_dev, SD_CTL_3B, SD_CTL_STRIPE_MASK, 0);
+		azx_dev->stripe = 0;
+	}
 	azx_dev->running = false;
 }
 EXPORT_SYMBOL_GPL(snd_hdac_stream_clear);
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 0032bba8cc9d..ed4e98a1057a 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -32,6 +32,7 @@
 #include <sound/hda_codec.h>
 #include "hda_local.h"
 #include "hda_jack.h"
+#include "hda_controller.h"
 
 static bool static_hdmi_pcm;
 module_param(static_hdmi_pcm, bool, 0644);
@@ -1249,6 +1250,10 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,
 	per_pin->cvt_nid = per_cvt->cvt_nid;
 	hinfo->nid = per_cvt->cvt_nid;
 
+	/* flip stripe flag for the assigned stream if supported */
+	if (get_wcaps(codec, per_cvt->cvt_nid) & AC_WCAP_STRIPE)
+		azx_stream(get_azx_dev(substream))->stripe = 1;
+
 	snd_hda_set_dev_select(codec, per_pin->pin_nid, per_pin->dev_id);
 	snd_hda_codec_write_cache(codec, per_pin->pin_nid, 0,
 			    AC_VERB_SET_CONNECT_SEL,
-- 
2.16.4

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

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [alsa-devel] [PATCH] ALSA: hda: Modify stream stripe mask only when needed
  2019-12-02  7:49 [alsa-devel] [PATCH] ALSA: hda: Modify stream stripe mask only when needed Takashi Iwai
@ 2019-12-15 16:36 ` Martin Regner
  2019-12-15 19:49   ` Takashi Iwai
  2019-12-15 18:57 ` Martin Regner
  1 sibling, 1 reply; 5+ messages in thread
From: Martin Regner @ 2019-12-15 16:36 UTC (permalink / raw)
  To: tiwai; +Cc: alsa-devel

Hi there,

since i upgraded my gentoo system to kernel 5.4.3 the sound over hdmi is
distorted, loud and seems to come only from one speaker. I checked the 
changes
between 5.4.2 and 5.4.3 and as a shot in the dark, i reverted the commit
e38e486d66e2a3b902768fd71c32dbf10f77e1cb, and the problem was gone.

I'll try to find out if the issue is on my system, but there is may 
something
wrong with the condition which enables the stripe mask.

It would be really great if you could take a look into this issue again.

Kind regards
Martin

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [alsa-devel] [PATCH] ALSA: hda: Modify stream stripe mask only when needed
  2019-12-02  7:49 [alsa-devel] [PATCH] ALSA: hda: Modify stream stripe mask only when needed Takashi Iwai
  2019-12-15 16:36 ` Martin Regner
@ 2019-12-15 18:57 ` Martin Regner
  2019-12-15 19:13   ` Martin Regner
  1 sibling, 1 reply; 5+ messages in thread
From: Martin Regner @ 2019-12-15 18:57 UTC (permalink / raw)
  To: tiwai; +Cc: alsa-devel

Hi again,

i did a little bit of research and my current solution is this patch:


diff -ru a/include/sound/hdaudio.h b/include/sound/hdaudio.h
--- a/include/sound/hdaudio.h   2019-12-15 19:49:15.775689873 +0100
+++ b/include/sound/hdaudio.h   2019-12-15 19:48:23.474688545 +0100
@@ -493,7 +493,7 @@
         bool prepared:1;
         bool no_period_wakeup:1;
         bool locked:1;
-       bool stripe:1;                  /* apply stripe control */
+       bool stripe:0;                  /* apply stripe control */

         /* timestamp */
         unsigned long start_wallclk;    /* start + minimum wallclk */
diff -ru a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c
--- a/sound/hda/hdac_stream.c   2019-12-15 19:15:37.512583523 +0100
+++ b/sound/hda/hdac_stream.c   2019-12-15 19:15:48.976584590 +0100
@@ -122,7 +122,6 @@
         snd_hdac_stream_writeb(azx_dev, SD_STS, SD_INT_MASK); /* to be 
sure */
         if (azx_dev->stripe) {
                 snd_hdac_stream_updateb(azx_dev, SD_CTL_3B, 
SD_CTL_STRIPE_MASK, 0);
-               azx_dev->stripe = 0;
         }
         azx_dev->running = false;
  }

I don't really understand the approach in the original commit. As far as 
i understand this, the stripe property is initially set to 1.
In the patch_hdmi.c it is set to 1 if the AC_WCAP_STRIPE flag is set. 
Finally it is set to 0 in the function snd_hdac_stream_clear.

With this patch the stripe property is initially 0, set to 1 if the 
AC_WCAP_STRIPE flag is set and never touched again.

Unfortunatly i can't test this on another machine.

Kind regards

Martin

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [alsa-devel] [PATCH] ALSA: hda: Modify stream stripe mask only when needed
  2019-12-15 18:57 ` Martin Regner
@ 2019-12-15 19:13   ` Martin Regner
  0 siblings, 0 replies; 5+ messages in thread
From: Martin Regner @ 2019-12-15 19:13 UTC (permalink / raw)
  To: tiwai; +Cc: alsa-devel

Sorry, the patch below did not compile. The working version is just this:

--- a/sound/hda/hdac_stream.c   2019-12-15 19:15:37.512583523 +0100
+++ b/sound/hda/hdac_stream.c   2019-12-15 19:15:48.976584590 +0100
@@ -122,7 +122,6 @@
     snd_hdac_stream_writeb(azx_dev, SD_STS, SD_INT_MASK); /* to be sure */
     if (azx_dev->stripe) {
         snd_hdac_stream_updateb(azx_dev, SD_CTL_3B, SD_CTL_STRIPE_MASK, 0);
-       azx_dev->stripe = 0;
     }
     azx_dev->running = false;
  }

Sorry for bothering.



On 15.12.19 19:57, Martin Regner wrote:
> Hi again,
>
> i did a little bit of research and my current solution is this patch:
>
>
> diff -ru a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> --- a/include/sound/hdaudio.h   2019-12-15 19:49:15.775689873 +0100
> +++ b/include/sound/hdaudio.h   2019-12-15 19:48:23.474688545 +0100
> @@ -493,7 +493,7 @@
>         bool prepared:1;
>         bool no_period_wakeup:1;
>         bool locked:1;
> -       bool stripe:1;                  /* apply stripe control */
> +       bool stripe:0;                  /* apply stripe control */
>
>         /* timestamp */
>         unsigned long start_wallclk;    /* start + minimum wallclk */
> diff -ru a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c
> --- a/sound/hda/hdac_stream.c   2019-12-15 19:15:37.512583523 +0100
> +++ b/sound/hda/hdac_stream.c   2019-12-15 19:15:48.976584590 +0100
> @@ -122,7 +122,6 @@
>         snd_hdac_stream_writeb(azx_dev, SD_STS, SD_INT_MASK); /* to be 
> sure */
>         if (azx_dev->stripe) {
>                 snd_hdac_stream_updateb(azx_dev, SD_CTL_3B, 
> SD_CTL_STRIPE_MASK, 0);
> -               azx_dev->stripe = 0;
>         }
>         azx_dev->running = false;
>  }
>
> I don't really understand the approach in the original commit. As far 
> as i understand this, the stripe property is initially set to 1.
> In the patch_hdmi.c it is set to 1 if the AC_WCAP_STRIPE flag is set. 
> Finally it is set to 0 in the function snd_hdac_stream_clear.
>
> With this patch the stripe property is initially 0, set to 1 if the 
> AC_WCAP_STRIPE flag is set and never touched again.
>
> Unfortunatly i can't test this on another machine.
>
> Kind regards
>
> Martin
>

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [alsa-devel] [PATCH] ALSA: hda: Modify stream stripe mask only when needed
  2019-12-15 16:36 ` Martin Regner
@ 2019-12-15 19:49   ` Takashi Iwai
  0 siblings, 0 replies; 5+ messages in thread
From: Takashi Iwai @ 2019-12-15 19:49 UTC (permalink / raw)
  To: Martin Regner; +Cc: alsa-devel

On Sun, 15 Dec 2019 17:36:57 +0100,
Martin Regner wrote:
> 
> Hi there,
> 
> since i upgraded my gentoo system to kernel 5.4.3 the sound over hdmi is
> distorted, loud and seems to come only from one speaker. I checked the
> changes
> between 5.4.2 and 5.4.3 and as a shot in the dark, i reverted the commit
> e38e486d66e2a3b902768fd71c32dbf10f77e1cb, and the problem was gone.
> 
> I'll try to find out if the issue is on my system, but there is may
> something
> wrong with the condition which enables the stripe mask.
> 
> It would be really great if you could take a look into this issue again.

This should have been fixed by the patch below:
  https://lore.kernel.org/r/20191214175217.31852-1-tiwai@suse.de

It's already in my sound git tree.

Let me know if you still see the issue after this patch.


thanks,

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-12-15 19:54 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-02  7:49 [alsa-devel] [PATCH] ALSA: hda: Modify stream stripe mask only when needed Takashi Iwai
2019-12-15 16:36 ` Martin Regner
2019-12-15 19:49   ` Takashi Iwai
2019-12-15 18:57 ` Martin Regner
2019-12-15 19:13   ` Martin Regner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).