[PATCH] ALSA: hda: Disable odd numbered channels on HDMI controllers.

[PATCH] ALSA: hda: Disable odd numbered channels on HDMI controllers.

[Nitin Daga]

Added code in hda_intel.c to disable odd numbered channels
not supported by HDMI controllers.

Signed-off-by: Nitin Daga <ndaga@nvidia.com>
Acked-By: Stephen Warren <swarren@nvidia.com>
---
 pci/hda/hda_intel.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/pci/hda/hda_intel.c b/pci/hda/hda_intel.c
index a78ea34..293c685 100644
--- a/pci/hda/hda_intel.c
+++ b/pci/hda/hda_intel.c
@@ -1539,6 +1539,16 @@ struct azx_pcm {
 	struct hda_pcm_stream *hinfo[2];
 };
 
+static unsigned int channels_2_4_6_8[] = {
+	2, 4, 6, 8
+};
+
+static struct snd_pcm_hw_constraint_list hw_constraints_2_4_6_8_channels = {
+	.count = ARRAY_SIZE(channels_2_4_6_8),
+	.list = channels_2_4_6_8,
+	.mask = 0,
+};
+
 static int azx_pcm_open(struct snd_pcm_substream *substream)
 {
 	struct azx_pcm *apcm = snd_pcm_substream_chip(substream);
@@ -1566,6 +1576,9 @@ static int azx_pcm_open(struct snd_pcm_substream *substream)
 				   128);
 	snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES,
 				   128);
+	snd_pcm_hw_constraint_list(runtime, 0,
+				   SNDRV_PCM_HW_PARAM_CHANNELS,
+				   &hw_constraints_2_4_6_8_channels);
 	snd_hda_power_up(apcm->codec);
 	err = hinfo->ops.open(hinfo, apcm->codec, substream);
 	if (err < 0) {
-- 
1.7.0.4


-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Re: [PATCH] ALSA: hda: Disable odd numbered channels on HDMI controllers.

[Anssi Hannula]

On 13.01.2011 22:19, Nitin Daga wrote:
> Added code in hda_intel.c to disable odd numbered channels
> not supported by HDMI controllers.
> 
> Signed-off-by: Nitin Daga <ndaga@nvidia.com>
> Acked-By: Stephen Warren <swarren@nvidia.com>
> ---
>  pci/hda/hda_intel.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/pci/hda/hda_intel.c b/pci/hda/hda_intel.c
> index a78ea34..293c685 100644
> --- a/pci/hda/hda_intel.c
> +++ b/pci/hda/hda_intel.c
> @@ -1539,6 +1539,16 @@ struct azx_pcm {
>  	struct hda_pcm_stream *hinfo[2];
>  };
>  
> +static unsigned int channels_2_4_6_8[] = {
> +	2, 4, 6, 8
> +};
> +
> +static struct snd_pcm_hw_constraint_list hw_constraints_2_4_6_8_channels = {
> +	.count = ARRAY_SIZE(channels_2_4_6_8),
> +	.list = channels_2_4_6_8,
> +	.mask = 0,
> +};

Hmm, can't these be const? OTOH none of the other drivers use const for
snd_pcm_hw_constraint_list either, so I guess it is ok.

>  static int azx_pcm_open(struct snd_pcm_substream *substream)
>  {
>  	struct azx_pcm *apcm = snd_pcm_substream_chip(substream);
> @@ -1566,6 +1576,9 @@ static int azx_pcm_open(struct snd_pcm_substream *substream)
>  				   128);
>  	snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES,
>  				   128);
> +	snd_pcm_hw_constraint_list(runtime, 0,
> +				   SNDRV_PCM_HW_PARAM_CHANNELS,
> +				   &hw_constraints_2_4_6_8_channels);

This would seem simpler and more generic (there are codecs that have
over 8 channels):
snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS, 2);

However, either of those would AFAICS break Si3054 Modem codec which is
mono-only.

Maybe just add the constraint to patch_hdmi.c?
Or use the patch as is but add 1, 10, 12, 14, 16 (or so) to the allowed
channel counts list.

Not really sure what is the preferred approach, though.

>  	snd_hda_power_up(apcm->codec);
>  	err = hinfo->ops.open(hinfo, apcm->codec, substream);
>  	if (err < 0) {

-- 
Anssi Hannula

Re: [PATCH] ALSA: hda: Disable odd numbered channels on HDMI controllers.

[Takashi Iwai]

At Thu, 13 Jan 2011 22:42:39 +0200,
Anssi Hannula wrote:
> 
> On 13.01.2011 22:19, Nitin Daga wrote:
> > Added code in hda_intel.c to disable odd numbered channels
> > not supported by HDMI controllers.
> > 
> > Signed-off-by: Nitin Daga <ndaga@nvidia.com>
> > Acked-By: Stephen Warren <swarren@nvidia.com>
> > ---
> >  pci/hda/hda_intel.c |   13 +++++++++++++
> >  1 files changed, 13 insertions(+), 0 deletions(-)
> > 
> > diff --git a/pci/hda/hda_intel.c b/pci/hda/hda_intel.c
> > index a78ea34..293c685 100644
> > --- a/pci/hda/hda_intel.c
> > +++ b/pci/hda/hda_intel.c
> > @@ -1539,6 +1539,16 @@ struct azx_pcm {
> >  	struct hda_pcm_stream *hinfo[2];
> >  };
> >  
> > +static unsigned int channels_2_4_6_8[] = {
> > +	2, 4, 6, 8
> > +};
> > +
> > +static struct snd_pcm_hw_constraint_list hw_constraints_2_4_6_8_channels = {
> > +	.count = ARRAY_SIZE(channels_2_4_6_8),
> > +	.list = channels_2_4_6_8,
> > +	.mask = 0,
> > +};
> 
> Hmm, can't these be const? OTOH none of the other drivers use const for
> snd_pcm_hw_constraint_list either, so I guess it is ok.
> 
> >  static int azx_pcm_open(struct snd_pcm_substream *substream)
> >  {
> >  	struct azx_pcm *apcm = snd_pcm_substream_chip(substream);
> > @@ -1566,6 +1576,9 @@ static int azx_pcm_open(struct snd_pcm_substream *substream)
> >  				   128);
> >  	snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES,
> >  				   128);
> > +	snd_pcm_hw_constraint_list(runtime, 0,
> > +				   SNDRV_PCM_HW_PARAM_CHANNELS,
> > +				   &hw_constraints_2_4_6_8_channels);
> 
> This would seem simpler and more generic (there are codecs that have
> over 8 channels):
> snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS, 2);
> 
> However, either of those would AFAICS break Si3054 Modem codec which is
> mono-only.
> 
> Maybe just add the constraint to patch_hdmi.c?

Yes, looks so.  I applied the following patch instead.

Alternatively, we can check channels_min and channels_max that are
already set in hda_intel.c::azx_pcm_open(), and apply the hw_constraint
only when channels_min%2==0 or such.

But, as we aren't 100% sure whether the odd number of channels work
with other (analog) codecs, it's safer to limit only for HDMI for the
time being.


thanks,

Takashi

---
>From ad09fc9d2156f3d37537b34418a6b79309013d33 Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Fri, 14 Jan 2011 09:42:27 +0100
Subject: [PATCH] ALSA: hda - Suppress the odd number of channels for HDMI

It looks like that HDMI codecs don't support the odd number of channels
although HD-audio spec doesn't have the restriction.  Add the
hw_constraint to limit to only the even number of channels.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/patch_hdmi.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index f29b97b..2d28879 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1238,6 +1238,9 @@ static int simple_playback_pcm_open(struct hda_pcm_stream *hinfo,
 		snd_pcm_hw_constraint_list(substream->runtime, 0,
 				SNDRV_PCM_HW_PARAM_CHANNELS,
 				hw_constraints_channels);
+	} else {
+		snd_pcm_hw_constraint_step(substream->runtime, 0,
+					   SNDRV_PCM_HW_PARAM_CHANNELS, 2);
 	}
 
 	return snd_hda_multi_out_dig_open(codec, &spec->multiout);
-- 
1.7.3.4

Re: [PATCH] ALSA: hda: Disable odd numbered channels on HDMI controllers.

[Anssi Hannula]

On 14.01.2011 10:49, Takashi Iwai wrote:
> At Thu, 13 Jan 2011 22:42:39 +0200,
> Anssi Hannula wrote:
>>
>> On 13.01.2011 22:19, Nitin Daga wrote:
>>> Added code in hda_intel.c to disable odd numbered channels
>>> not supported by HDMI controllers.
>>>
>>> Signed-off-by: Nitin Daga <ndaga@nvidia.com>
>>> Acked-By: Stephen Warren <swarren@nvidia.com>
>>> ---
>>>  pci/hda/hda_intel.c |   13 +++++++++++++
>>>  1 files changed, 13 insertions(+), 0 deletions(-)
[...]
>> This would seem simpler and more generic (there are codecs that have
>> over 8 channels):
>> snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS, 2);
>>
>> However, either of those would AFAICS break Si3054 Modem codec which is
>> mono-only.
>>
>> Maybe just add the constraint to patch_hdmi.c?
> 
> Yes, looks so.  I applied the following patch instead.

It is ineffective, the generic hdmi devices use hdmi_pcm_open() instead
of simple_playback_pcm_open().

> Alternatively, we can check channels_min and channels_max that are
> already set in hda_intel.c::azx_pcm_open(), and apply the hw_constraint
> only when channels_min%2==0 or such.
> 
> But, as we aren't 100% sure whether the odd number of channels work
> with other (analog) codecs, it's safer to limit only for HDMI for the
> time being.

Well, snd_hda_multi_out_analog_open() already sets such a constraint,
but I didn't check if all the analog multichannel codecs use that.

> thanks,
> 
> Takashi
> 
> ---
> From ad09fc9d2156f3d37537b34418a6b79309013d33 Mon Sep 17 00:00:00 2001
> From: Takashi Iwai <tiwai@suse.de>
> Date: Fri, 14 Jan 2011 09:42:27 +0100
> Subject: [PATCH] ALSA: hda - Suppress the odd number of channels for HDMI
> 
> It looks like that HDMI codecs don't support the odd number of channels
> although HD-audio spec doesn't have the restriction.  Add the
> hw_constraint to limit to only the even number of channels.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/pci/hda/patch_hdmi.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index f29b97b..2d28879 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -1238,6 +1238,9 @@ static int simple_playback_pcm_open(struct hda_pcm_stream *hinfo,
>  		snd_pcm_hw_constraint_list(substream->runtime, 0,
>  				SNDRV_PCM_HW_PARAM_CHANNELS,
>  				hw_constraints_channels);
> +	} else {
> +		snd_pcm_hw_constraint_step(substream->runtime, 0,
> +					   SNDRV_PCM_HW_PARAM_CHANNELS, 2);
>  	}
>  
>  	return snd_hda_multi_out_dig_open(codec, &spec->multiout);


-- 
Anssi Hannula

Re: [PATCH] ALSA: hda: Disable odd numbered channels on HDMI controllers.

[Takashi Iwai]

At Fri, 14 Jan 2011 11:14:16 +0200,
Anssi Hannula wrote:
> 
> On 14.01.2011 10:49, Takashi Iwai wrote:
> > At Thu, 13 Jan 2011 22:42:39 +0200,
> > Anssi Hannula wrote:
> >>
> >> On 13.01.2011 22:19, Nitin Daga wrote:
> >>> Added code in hda_intel.c to disable odd numbered channels
> >>> not supported by HDMI controllers.
> >>>
> >>> Signed-off-by: Nitin Daga <ndaga@nvidia.com>
> >>> Acked-By: Stephen Warren <swarren@nvidia.com>
> >>> ---
> >>>  pci/hda/hda_intel.c |   13 +++++++++++++
> >>>  1 files changed, 13 insertions(+), 0 deletions(-)
> [...]
> >> This would seem simpler and more generic (there are codecs that have
> >> over 8 channels):
> >> snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS, 2);
> >>
> >> However, either of those would AFAICS break Si3054 Modem codec which is
> >> mono-only.
> >>
> >> Maybe just add the constraint to patch_hdmi.c?
> > 
> > Yes, looks so.  I applied the following patch instead.
> 
> It is ineffective, the generic hdmi devices use hdmi_pcm_open() instead
> of simple_playback_pcm_open().

Ah, indeed.  Fixed now.

Meanwhile, I found another bug in patch_hdmi.c.  The updated PCM
parameters aren't reflected properly at the first time because
runtime->hw was already set.  Fixed by patch below.

> > Alternatively, we can check channels_min and channels_max that are
> > already set in hda_intel.c::azx_pcm_open(), and apply the hw_constraint
> > only when channels_min%2==0 or such.
> > 
> > But, as we aren't 100% sure whether the odd number of channels work
> > with other (analog) codecs, it's safer to limit only for HDMI for the
> > time being.
> 
> Well, snd_hda_multi_out_analog_open() already sets such a constraint,
> but I didn't check if all the analog multichannel codecs use that.

Right.  OTOH, the spec doesn't restrict it, so limiting it in the
controller driver side (hda_intel.c) doesn't sound right.
It's rather likely a codec-specific issue.  So, restriction should be
applied in the codec side, IMO.


thanks,

Takashi

---
>From 639cef0eb6df05d5516520aa89b0c9fe62ee2d3b Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Fri, 14 Jan 2011 10:30:46 +0100
Subject: [PATCH] ALSA: hda - Store PCM parameters properly in HDMI open callback

In hdmi_pcm_open(), the evaluated PCM hw parameters are stored in
hinfo, but these aren't properly set back to the current runtime
record since these have been set beforehand in azx_pcm_open().
This patch fixes the behavior.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/patch_hdmi.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 2d28879..5980552 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -817,6 +817,7 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,
 	struct hdmi_spec *spec = codec->spec;
 	struct hdmi_eld *eld;
 	struct hda_pcm_stream *codec_pars;
+	struct snd_pcm_runtime *runtime = substream->runtime;
 	unsigned int idx;
 
 	for (idx = 0; idx < spec->num_cvts; idx++)
@@ -844,6 +845,11 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,
 		hinfo->formats = codec_pars->formats;
 		hinfo->maxbps = codec_pars->maxbps;
 	}
+	/* store the updated parameters */
+	runtime->hw.channels_min = hinfo->channels_min;
+	runtime->hw.channels_max = hinfo->channels_max;
+	runtime->hw.formats = hinfo->formats;
+	runtime->hw.rates = hinfo->rates;
 	return 0;
 }
 
-- 
1.7.3.4