[PATCH] ALSA: hda - Do not assign streams in reverse order

[PATCH] ALSA: hda - Do not assign streams in reverse order

[Anssi Hannula]

Currently stream numbers are assigned in reverse order.

Unfortunately commit 7546abfb8e1f9933b5 ("ALSA: hda - Increment
default stream numbers for AMD HDMI controllers") assumed this was not
the case (specifically, it had the "old cards had single device only"
=> "extra unused stream numbers do not matter" assumption), causing
non-working audio regressions for AMD Radeon HDMI users on v3.14.

Change the stream numbers to be assigned in increasing order instead.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=77002
Reported-by: Christian Güdel <cg@dmesg.ch>
Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
Tested-by: Christian Güdel <cg@dmesg.ch>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: <stable@vger.kernel.org> # 3.14
---

Takashi, please check I didn't miss anything (I'm not that familiar
with this low-level HDA code). :)


 sound/pci/hda/hda_intel.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index e354ab1ec20f..7a7b7b10dddf 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1856,13 +1856,14 @@ azx_assign_device(struct azx *chip, struct snd_pcm_substream *substream)
 		struct azx_dev *azx_dev = &chip->azx_dev[dev];
 		dsp_lock(azx_dev);
 		if (!azx_dev->opened && !dsp_is_locked(azx_dev)) {
-			res = azx_dev;
-			if (res->assigned_key == key) {
-				res->opened = 1;
-				res->assigned_key = key;
+			if (azx_dev->assigned_key == key) {
+				azx_dev->opened = 1;
+				azx_dev->assigned_key = key;
 				dsp_unlock(azx_dev);
 				return azx_dev;
 			}
+			if (!res)
+				res = azx_dev;
 		}
 		dsp_unlock(azx_dev);
 	}
-- 
1.8.4.5

Re: [PATCH] ALSA: hda - Do not assign streams in reverse order

[Anssi Hannula]

Anssi Hannula wrote:
> Currently stream numbers are assigned in reverse order.
> 
> Unfortunately commit 7546abfb8e1f9933b5 ("ALSA: hda - Increment
> default stream numbers for AMD HDMI controllers") assumed this was not
> the case (specifically, it had the "old cards had single device only"
> => "extra unused stream numbers do not matter" assumption), causing
> non-working audio regressions for AMD Radeon HDMI users on v3.14.
> 
> Change the stream numbers to be assigned in increasing order instead.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=77002
> Reported-by: Christian Güdel <cg@dmesg.ch>
> Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
> Tested-by: Christian Güdel <cg@dmesg.ch>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: <stable@vger.kernel.org> # 3.14
> ---
> 
> Takashi, please check I didn't miss anything (I'm not that familiar
> with this low-level HDA code). :)

Alex wrote in the above bugreport now that the stream counts are:
Kaveri:                4 streams, 7 endpoints
Kabini:                2 streams, 3 endpoints
Bonaire/Hawaii:        6 streams, 7 endpoints
Trinity/Richland:      4 streams, 6 endpoints
Oland:                 2 streams, 2 endpoints
Tahiti/Pitcairn/Verde: 6 streams, 6 endpoints

So we have cards with less streams than converters, if I understood
correctly.

I'd think my patch is still reasonable even if we'll have to fix the
stream counts anyway, the pros and cons I see are:
+ safer in case we/manufacturer get the stream count wrong for some card
- easily hides too high stream count

But your call :)


>  sound/pci/hda/hda_intel.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index e354ab1ec20f..7a7b7b10dddf 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -1856,13 +1856,14 @@ azx_assign_device(struct azx *chip, struct snd_pcm_substream *substream)
>  		struct azx_dev *azx_dev = &chip->azx_dev[dev];
>  		dsp_lock(azx_dev);
>  		if (!azx_dev->opened && !dsp_is_locked(azx_dev)) {
> -			res = azx_dev;
> -			if (res->assigned_key == key) {
> -				res->opened = 1;
> -				res->assigned_key = key;
> +			if (azx_dev->assigned_key == key) {
> +				azx_dev->opened = 1;
> +				azx_dev->assigned_key = key;
>  				dsp_unlock(azx_dev);
>  				return azx_dev;
>  			}
> +			if (!res)
> +				res = azx_dev;
>  		}
>  		dsp_unlock(azx_dev);
>  	}
> -- 
> 1.8.4.5

-- 
Anssi Hannula

Re: [PATCH] ALSA: hda - Do not assign streams in reverse order

[Takashi Iwai]

At Mon,  7 Apr 2014 22:36:38 +0300,
Anssi Hannula wrote:
> 
> Currently stream numbers are assigned in reverse order.
> 
> Unfortunately commit 7546abfb8e1f9933b5 ("ALSA: hda - Increment
> default stream numbers for AMD HDMI controllers") assumed this was not
> the case (specifically, it had the "old cards had single device only"
> => "extra unused stream numbers do not matter" assumption), causing
> non-working audio regressions for AMD Radeon HDMI users on v3.14.
> 
> Change the stream numbers to be assigned in increasing order instead.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=77002
> Reported-by: Christian Güdel <cg@dmesg.ch>
> Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
> Tested-by: Christian Güdel <cg@dmesg.ch>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: <stable@vger.kernel.org> # 3.14
> ---
> 
> Takashi, please check I didn't miss anything (I'm not that familiar
> with this low-level HDA code). :)

In that case, let's revert the commit instead.  There was a reason to
behave in that way, although I don't remember well.  That said, this
change might lead to another regression.  Thus, if a fix is needed for
stable, the revert would be the best choice as a first aid.  Of
course, this change is good to have, so let's implement this for 3.15
and see whether it works well for all nasty controllers.

Besides, this patch can't be applied any longer to the current code
base since it's been rewritten a lot; most of codes have been moved to
another file hda_controller.c.  Could you rebase the patch for the
latest tree?  Meanwhile I'll prepare a revert that is applied only to
3.14.x.


thanks,

Takashi

> 
> 
>  sound/pci/hda/hda_intel.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index e354ab1ec20f..7a7b7b10dddf 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -1856,13 +1856,14 @@ azx_assign_device(struct azx *chip, struct snd_pcm_substream *substream)
>  		struct azx_dev *azx_dev = &chip->azx_dev[dev];
>  		dsp_lock(azx_dev);
>  		if (!azx_dev->opened && !dsp_is_locked(azx_dev)) {
> -			res = azx_dev;
> -			if (res->assigned_key == key) {
> -				res->opened = 1;
> -				res->assigned_key = key;
> +			if (azx_dev->assigned_key == key) {
> +				azx_dev->opened = 1;
> +				azx_dev->assigned_key = key;
>  				dsp_unlock(azx_dev);
>  				return azx_dev;
>  			}
> +			if (!res)
> +				res = azx_dev;
>  		}
>  		dsp_unlock(azx_dev);
>  	}
> -- 
> 1.8.4.5
> 

Re: [PATCH] ALSA: hda - Do not assign streams in reverse order

[Anssi Hannula]

08.04.2014 10:35, Takashi Iwai kirjoitti:
> At Mon,  7 Apr 2014 22:36:38 +0300,
> Anssi Hannula wrote:
>>
>> Currently stream numbers are assigned in reverse order.
>>
>> Unfortunately commit 7546abfb8e1f9933b5 ("ALSA: hda - Increment
>> default stream numbers for AMD HDMI controllers") assumed this was not
>> the case (specifically, it had the "old cards had single device only"
>> => "extra unused stream numbers do not matter" assumption), causing
>> non-working audio regressions for AMD Radeon HDMI users on v3.14.
>>
>> Change the stream numbers to be assigned in increasing order instead.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=77002
>> Reported-by: Christian Güdel <cg@dmesg.ch>
>> Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
>> Tested-by: Christian Güdel <cg@dmesg.ch>
>> Cc: Alex Deucher <alexander.deucher@amd.com>
>> Cc: <stable@vger.kernel.org> # 3.14
>> ---
>>
>> Takashi, please check I didn't miss anything (I'm not that familiar
>> with this low-level HDA code). :)
> 
> In that case, let's revert the commit instead.  There was a reason to
> behave in that way, although I don't remember well.  That said, this
> change might lead to another regression.  Thus, if a fix is needed for
> stable, the revert would be the best choice as a first aid.  Of
> course, this change is good to have, so let's implement this for 3.15
> and see whether it works well for all nasty controllers.

We tested reverting (on top of v3.14) with Christian and it works as
well, so you can add:
Tested-by: Christian Güdel <cg@dmesg.ch>

> Besides, this patch can't be applied any longer to the current code
> base since it's been rewritten a lot; most of codes have been moved to
> another file hda_controller.c.  Could you rebase the patch for the
> latest tree?  Meanwhile I'll prepare a revert that is applied only to
> 3.14.x.

Will do.

> 
> thanks,
> 
> Takashi
> 
>>
>>
>>  sound/pci/hda/hda_intel.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
>> index e354ab1ec20f..7a7b7b10dddf 100644
>> --- a/sound/pci/hda/hda_intel.c
>> +++ b/sound/pci/hda/hda_intel.c
>> @@ -1856,13 +1856,14 @@ azx_assign_device(struct azx *chip, struct snd_pcm_substream *substream)
>>  		struct azx_dev *azx_dev = &chip->azx_dev[dev];
>>  		dsp_lock(azx_dev);
>>  		if (!azx_dev->opened && !dsp_is_locked(azx_dev)) {
>> -			res = azx_dev;
>> -			if (res->assigned_key == key) {
>> -				res->opened = 1;
>> -				res->assigned_key = key;
>> +			if (azx_dev->assigned_key == key) {
>> +				azx_dev->opened = 1;
>> +				azx_dev->assigned_key = key;
>>  				dsp_unlock(azx_dev);
>>  				return azx_dev;
>>  			}
>> +			if (!res)
>> +				res = azx_dev;
>>  		}
>>  		dsp_unlock(azx_dev);
>>  	}
>> -- 
>> 1.8.4.5
>>


-- 
Anssi Hannula

Re: [PATCH] ALSA: hda - Do not assign streams in reverse order

[Takashi Iwai]

At Tue, 08 Apr 2014 10:50:18 +0300,
Anssi Hannula wrote:
> 
> 08.04.2014 10:35, Takashi Iwai kirjoitti:
> > At Mon,  7 Apr 2014 22:36:38 +0300,
> > Anssi Hannula wrote:
> >>
> >> Currently stream numbers are assigned in reverse order.
> >>
> >> Unfortunately commit 7546abfb8e1f9933b5 ("ALSA: hda - Increment
> >> default stream numbers for AMD HDMI controllers") assumed this was not
> >> the case (specifically, it had the "old cards had single device only"
> >> => "extra unused stream numbers do not matter" assumption), causing
> >> non-working audio regressions for AMD Radeon HDMI users on v3.14.
> >>
> >> Change the stream numbers to be assigned in increasing order instead.
> >>
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=77002
> >> Reported-by: Christian Güdel <cg@dmesg.ch>
> >> Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
> >> Tested-by: Christian Güdel <cg@dmesg.ch>
> >> Cc: Alex Deucher <alexander.deucher@amd.com>
> >> Cc: <stable@vger.kernel.org> # 3.14
> >> ---
> >>
> >> Takashi, please check I didn't miss anything (I'm not that familiar
> >> with this low-level HDA code). :)
> > 
> > In that case, let's revert the commit instead.  There was a reason to
> > behave in that way, although I don't remember well.  That said, this
> > change might lead to another regression.  Thus, if a fix is needed for
> > stable, the revert would be the best choice as a first aid.  Of
> > course, this change is good to have, so let's implement this for 3.15
> > and see whether it works well for all nasty controllers.
> 
> We tested reverting (on top of v3.14) with Christian and it works as
> well, so you can add:
> Tested-by: Christian Güdel <cg@dmesg.ch>

OK, great.

> > Besides, this patch can't be applied any longer to the current code
> > base since it's been rewritten a lot; most of codes have been moved to
> > another file hda_controller.c.  Could you rebase the patch for the
> > latest tree?  Meanwhile I'll prepare a revert that is applied only to
> > 3.14.x.
> 
> Will do.

Thanks!


Takashi

[PATCH] ALSA: hda - Do not assign streams in reverse order

[Anssi Hannula]

Currently stream numbers are assigned in reverse order.

Unfortunately commit 7546abfb8e1f9933b5 ("ALSA: hda - Increment
default stream numbers for AMD HDMI controllers") assumed this was not
the case (specifically, it had the "old cards had single device only"
=> "extra unused stream numbers do not matter" assumption), causing
non-working audio regressions for AMD Radeon HDMI users.

Change the stream numbers to be assigned in forward order.

The benefit is that regular audio playback will still work even if the
assumed stream count is too high, downside is that a too high stream
count may remain hidden.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=77002
Reported-by: Christian Güdel <cg@dmesg.ch>
Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
Tested-by: Christian Güdel <cg@dmesg.ch> # 3.14
Cc: Alex Deucher <alexander.deucher@amd.com>
---

Rebased for v3.14.

 sound/pci/hda/hda_controller.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
index 97993e17f46a..3e005e509131 100644
--- a/sound/pci/hda/hda_controller.c
+++ b/sound/pci/hda/hda_controller.c
@@ -187,13 +187,14 @@ azx_assign_device(struct azx *chip, struct snd_pcm_substream *substream)
 		struct azx_dev *azx_dev = &chip->azx_dev[dev];
 		dsp_lock(azx_dev);
 		if (!azx_dev->opened && !dsp_is_locked(azx_dev)) {
-			res = azx_dev;
-			if (res->assigned_key == key) {
-				res->opened = 1;
-				res->assigned_key = key;
+			if (azx_dev->assigned_key == key) {
+				azx_dev->opened = 1;
+				azx_dev->assigned_key = key;
 				dsp_unlock(azx_dev);
 				return azx_dev;
 			}
+			if (!res)
+				res = azx_dev;
 		}
 		dsp_unlock(azx_dev);
 	}
-- 
1.8.4.5

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

Re: [PATCH] ALSA: hda - Do not assign streams in reverse order

[Takashi Iwai]

At Tue,  8 Apr 2014 12:36:42 +0300,
Anssi Hannula wrote:
> 
> Currently stream numbers are assigned in reverse order.
> 
> Unfortunately commit 7546abfb8e1f9933b5 ("ALSA: hda - Increment
> default stream numbers for AMD HDMI controllers") assumed this was not
> the case (specifically, it had the "old cards had single device only"
> => "extra unused stream numbers do not matter" assumption), causing
> non-working audio regressions for AMD Radeon HDMI users.
> 
> Change the stream numbers to be assigned in forward order.
> 
> The benefit is that regular audio playback will still work even if the
> assumed stream count is too high, downside is that a too high stream
> count may remain hidden.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=77002
> Reported-by: Christian Güdel <cg@dmesg.ch>
> Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
> Tested-by: Christian Güdel <cg@dmesg.ch> # 3.14
> Cc: Alex Deucher <alexander.deucher@amd.com>

Thanks, applied.

> ---
> 
> Rebased for v3.14.

It's for 3.15 :)


Takashi

> 
>  sound/pci/hda/hda_controller.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
> index 97993e17f46a..3e005e509131 100644
> --- a/sound/pci/hda/hda_controller.c
> +++ b/sound/pci/hda/hda_controller.c
> @@ -187,13 +187,14 @@ azx_assign_device(struct azx *chip, struct snd_pcm_substream *substream)
>  		struct azx_dev *azx_dev = &chip->azx_dev[dev];
>  		dsp_lock(azx_dev);
>  		if (!azx_dev->opened && !dsp_is_locked(azx_dev)) {
> -			res = azx_dev;
> -			if (res->assigned_key == key) {
> -				res->opened = 1;
> -				res->assigned_key = key;
> +			if (azx_dev->assigned_key == key) {
> +				azx_dev->opened = 1;
> +				azx_dev->assigned_key = key;
>  				dsp_unlock(azx_dev);
>  				return azx_dev;
>  			}
> +			if (!res)
> +				res = azx_dev;
>  		}
>  		dsp_unlock(azx_dev);
>  	}
> -- 
> 1.8.4.5
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel