* [RFC PATCH] ALSA : hda - not use assigned converters for all unused pins
@ 2013-09-16 2:40 mengdong.lin
2013-09-16 22:40 ` David Henningsson
0 siblings, 1 reply; 5+ messages in thread
From: mengdong.lin @ 2013-09-16 2:40 UTC (permalink / raw)
To: alsa-devel, tiwai; +Cc: Mengdong Lin
From: Mengdong Lin <mengdong.lin@intel.com>
BIOS can mark a pin as "no physical connection" if the port is used by an
integrated display which is not audio capable. And audio driver will overlook
such pins.
On Haswell, such a disconneted pin will keep muted and connected to the 1st
converter by default. But if the 1st convertor is assigned to a connected pin
for audio streaming. The muted disconnected pin can make the connected pin
no sound output.
So this patch avoids using assigned converters for all unused pins for Haswell,
including the disconected pins.
Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 3d8cd044..4960695 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1152,29 +1152,34 @@ static void haswell_config_cvts(struct hda_codec *codec,
int pin_id, int mux_id)
{
struct hdmi_spec *spec = codec->spec;
- struct hdmi_spec_per_pin *per_pin;
- int pin_idx, mux_idx;
+ hda_nid_t pin_nid;
+ int pin_idx, cvt_idx;
int curr;
- int err;
-
- for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
- per_pin = get_pin(spec, pin_idx);
+ struct hdmi_spec_per_cvt *per_cvt;
+ /* configure all 3 pins, including "no physical connection" ones */
+ for (pin_nid = 5; pin_nid < 8; pin_nid++) {
+ pin_idx = pin_nid_to_pin_index(spec, pin_nid);
if (pin_idx == pin_id)
continue;
- curr = snd_hda_codec_read(codec, per_pin->pin_nid, 0,
+ curr = snd_hda_codec_read(codec, pin_nid, 0,
AC_VERB_GET_CONNECT_SEL, 0);
+ if (curr != mux_id)
+ continue;
+
+ for (cvt_idx = 0; cvt_idx < spec->num_cvts; cvt_idx++) {
+ per_cvt = get_cvt(spec, cvt_idx);
- /* Choose another unused converter */
- if (curr == mux_id) {
- err = hdmi_choose_cvt(codec, pin_idx, NULL, &mux_idx);
- if (err < 0)
- return;
- snd_printdd("HDMI: choose converter %d for pin %d\n", mux_idx, pin_idx);
- snd_hda_codec_write_cache(codec, per_pin->pin_nid, 0,
+ /* Must not already be assigned */
+ if (!per_cvt->assigned) {
+ snd_printdd("choose converter %d for pin %d\n",
+ cvt_idx, pin_idx);
+ snd_hda_codec_write_cache(codec, pin_nid, 0,
AC_VERB_SET_CONNECT_SEL,
- mux_idx);
+ cvt_idx);
+ break;
+ }
}
}
}
--
1.8.1.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC PATCH] ALSA : hda - not use assigned converters for all unused pins
2013-09-16 2:40 [RFC PATCH] ALSA : hda - not use assigned converters for all unused pins mengdong.lin
@ 2013-09-16 22:40 ` David Henningsson
2013-09-20 8:00 ` Lin, Mengdong
0 siblings, 1 reply; 5+ messages in thread
From: David Henningsson @ 2013-09-16 22:40 UTC (permalink / raw)
To: mengdong.lin; +Cc: tiwai, alsa-devel
On 09/16/2013 04:40 AM, mengdong.lin@intel.com wrote:
> From: Mengdong Lin <mengdong.lin@intel.com>
>
> BIOS can mark a pin as "no physical connection" if the port is used by an
> integrated display which is not audio capable. And audio driver will overlook
> such pins.
>
> On Haswell, such a disconneted pin will keep muted and connected to the 1st
> converter by default. But if the 1st convertor is assigned to a connected pin
> for audio streaming. The muted disconnected pin can make the connected pin
> no sound output.
>
> So this patch avoids using assigned converters for all unused pins for Haswell,
> including the disconected pins.
Thanks for the patch!
>
> Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>
>
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index 3d8cd044..4960695 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -1152,29 +1152,34 @@ static void haswell_config_cvts(struct hda_codec *codec,
> int pin_id, int mux_id)
> {
> struct hdmi_spec *spec = codec->spec;
> - struct hdmi_spec_per_pin *per_pin;
> - int pin_idx, mux_idx;
> + hda_nid_t pin_nid;
> + int pin_idx, cvt_idx;
> int curr;
> - int err;
> -
> - for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
> - per_pin = get_pin(spec, pin_idx);
> + struct hdmi_spec_per_cvt *per_cvt;
>
> + /* configure all 3 pins, including "no physical connection" ones */
> + for (pin_nid = 5; pin_nid < 8; pin_nid++) {
It might not be so future proof to hard code nid IDs here (even if it
should be safe right now?). Perhaps more elegant to go through the
entire codec->nids array like being done here:
http://lxr.linux.no/linux+*/sound/pci/hda/hda_auto_parser.c#L188
The nid's wcaps is cached so it does not mean additional reads from the
codec.
> + pin_idx = pin_nid_to_pin_index(spec, pin_nid);
> if (pin_idx == pin_id)
> continue;
>
> - curr = snd_hda_codec_read(codec, per_pin->pin_nid, 0,
> + curr = snd_hda_codec_read(codec, pin_nid, 0,
> AC_VERB_GET_CONNECT_SEL, 0);
> + if (curr != mux_id)
> + continue;
> +
> + for (cvt_idx = 0; cvt_idx < spec->num_cvts; cvt_idx++) {
> + per_cvt = get_cvt(spec, cvt_idx);
>
> - /* Choose another unused converter */
> - if (curr == mux_id) {
> - err = hdmi_choose_cvt(codec, pin_idx, NULL, &mux_idx);
> - if (err < 0)
> - return;
> - snd_printdd("HDMI: choose converter %d for pin %d\n", mux_idx, pin_idx);
> - snd_hda_codec_write_cache(codec, per_pin->pin_nid, 0,
> + /* Must not already be assigned */
> + if (!per_cvt->assigned) {
> + snd_printdd("choose converter %d for pin %d\n",
> + cvt_idx, pin_idx);
> + snd_hda_codec_write_cache(codec, pin_nid, 0,
> AC_VERB_SET_CONNECT_SEL,
> - mux_idx);
> + cvt_idx);
> + break;
> + }
> }
> }
> }
>
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH] ALSA : hda - not use assigned converters for all unused pins
2013-09-16 22:40 ` David Henningsson
@ 2013-09-20 8:00 ` Lin, Mengdong
2013-09-20 9:32 ` David Henningsson
0 siblings, 1 reply; 5+ messages in thread
From: Lin, Mengdong @ 2013-09-20 8:00 UTC (permalink / raw)
To: David Henningsson; +Cc: tiwai@suse.de, alsa-devel@alsa-project.org
Hi David,
Sorry, I missed your mail. Please see comments below ...
> -----Original Message-----
> From: David Henningsson [mailto:david.henningsson@canonical.com]
> Sent: Tuesday, September 17, 2013 6:40 AM
> > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> > index 3d8cd044..4960695 100644
> > --- a/sound/pci/hda/patch_hdmi.c
> > +++ b/sound/pci/hda/patch_hdmi.c
> > @@ -1152,29 +1152,34 @@ static void haswell_config_cvts(struct
> hda_codec *codec,
> > int pin_id, int mux_id)
> > {
> > struct hdmi_spec *spec = codec->spec;
> > - struct hdmi_spec_per_pin *per_pin;
> > - int pin_idx, mux_idx;
> > + hda_nid_t pin_nid;
> > + int pin_idx, cvt_idx;
> > int curr;
> > - int err;
> > -
> > - for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
> > - per_pin = get_pin(spec, pin_idx);
> > + struct hdmi_spec_per_cvt *per_cvt;
> >
> > + /* configure all 3 pins, including "no physical connection" ones */
> > + for (pin_nid = 5; pin_nid < 8; pin_nid++) {
>
> It might not be so future proof to hard code nid IDs here (even if it should be
> safe right now?). Perhaps more elegant to go through the entire codec->nids
> array like being done here:
>
> http://lxr.linux.no/linux+*/sound/pci/hda/hda_auto_parser.c#L188
>
> The nid's wcaps is cached so it does not mean additional reads from the codec.
Xingchao defined haswell_config_cvts() to configure converters on codec->pins before.
But codec->pins does not include the pins programmed by BIOS as "No physical connections" (like port B in the OEM machine).
So I change the code from using codec->pins to hard coded NIDs. It's not graceful and the NIDs may change in the future.
There is another option to change add_pin() to not overlook such "no connection" pins for Haswell.
But there would be a side effect: the 1st pin can be "no connection" and then the following function will be affected:
- simple_playback_build_pcms()
- simple_playback_init()
Both of them assume the 1st pin is valid and use get_pin(spec, 0) to operates on the 1st pin.
And in addition, I don't want more PCM and controls to be created on the "No connection" pins.
So finally I gave up to change add_pin() but just modify haswell_config_cvts().
Thanks
Mengdong
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH] ALSA : hda - not use assigned converters for all unused pins
2013-09-20 8:00 ` Lin, Mengdong
@ 2013-09-20 9:32 ` David Henningsson
2013-09-20 14:48 ` Lin, Mengdong
0 siblings, 1 reply; 5+ messages in thread
From: David Henningsson @ 2013-09-20 9:32 UTC (permalink / raw)
To: Lin, Mengdong; +Cc: tiwai@suse.de, alsa-devel@alsa-project.org
On 09/20/2013 10:00 AM, Lin, Mengdong wrote:
> Hi David,
>
> Sorry, I missed your mail. Please see comments below ...
>
>> -----Original Message-----
>> From: David Henningsson [mailto:david.henningsson@canonical.com]
>> Sent: Tuesday, September 17, 2013 6:40 AM
>
>>> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
>>> index 3d8cd044..4960695 100644
>>> --- a/sound/pci/hda/patch_hdmi.c
>>> +++ b/sound/pci/hda/patch_hdmi.c
>>> @@ -1152,29 +1152,34 @@ static void haswell_config_cvts(struct
>> hda_codec *codec,
>>> int pin_id, int mux_id)
>>> {
>>> struct hdmi_spec *spec = codec->spec;
>>> - struct hdmi_spec_per_pin *per_pin;
>>> - int pin_idx, mux_idx;
>>> + hda_nid_t pin_nid;
>>> + int pin_idx, cvt_idx;
>>> int curr;
>>> - int err;
>>> -
>>> - for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
>>> - per_pin = get_pin(spec, pin_idx);
>>> + struct hdmi_spec_per_cvt *per_cvt;
>>>
>>> + /* configure all 3 pins, including "no physical connection" ones */
>>> + for (pin_nid = 5; pin_nid < 8; pin_nid++) {
>>
>> It might not be so future proof to hard code nid IDs here (even if it should be
>> safe right now?). Perhaps more elegant to go through the entire codec->nids
>> array like being done here:
>>
>> http://lxr.linux.no/linux+*/sound/pci/hda/hda_auto_parser.c#L188
>>
>> The nid's wcaps is cached so it does not mean additional reads from the codec.
>
> Xingchao defined haswell_config_cvts() to configure converters on codec->pins before.
> But codec->pins does not include the pins programmed by BIOS as "No physical connections" (like port B in the OEM machine).
> So I change the code from using codec->pins to hard coded NIDs. It's not graceful and the NIDs may change in the future.
>
> There is another option to change add_pin() to not overlook such "no connection" pins for Haswell.
I was suggesting to do something like this in haswell_config_cvts:
int end_nid = codec->start_nid + codec->num_nodes;
for (nid = codec->start_nid; nid < end_nid; nid++) {
unsigned int wid_caps = get_wcaps(codec, nid);
if (get_wcaps_type(wid_caps) != AC_WID_PIN)
continue;
/* etc */
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH] ALSA : hda - not use assigned converters for all unused pins
2013-09-20 9:32 ` David Henningsson
@ 2013-09-20 14:48 ` Lin, Mengdong
0 siblings, 0 replies; 5+ messages in thread
From: Lin, Mengdong @ 2013-09-20 14:48 UTC (permalink / raw)
To: David Henningsson; +Cc: tiwai@suse.de, alsa-devel@alsa-project.org
> -----Original Message-----
> From: David Henningsson [mailto:david.henningsson@canonical.com]
> Sent: Friday, September 20, 2013 5:32 PM
> To: Lin, Mengdong
> Cc: alsa-devel@alsa-project.org; tiwai@suse.de
> Subject: Re: [alsa-devel] [RFC PATCH] ALSA : hda - not use assigned converters
> for all unused pins
>
> On 09/20/2013 10:00 AM, Lin, Mengdong wrote:
> > Hi David,
> >
> > Sorry, I missed your mail. Please see comments below ...
> >
> >> -----Original Message-----
> >> From: David Henningsson [mailto:david.henningsson@canonical.com]
> >> Sent: Tuesday, September 17, 2013 6:40 AM
> >
> >>> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> >>> index 3d8cd044..4960695 100644
> >>> --- a/sound/pci/hda/patch_hdmi.c
> >>> +++ b/sound/pci/hda/patch_hdmi.c
> >>> @@ -1152,29 +1152,34 @@ static void haswell_config_cvts(struct
> >> hda_codec *codec,
> >>> int pin_id, int mux_id)
> >>> {
> >>> struct hdmi_spec *spec = codec->spec;
> >>> - struct hdmi_spec_per_pin *per_pin;
> >>> - int pin_idx, mux_idx;
> >>> + hda_nid_t pin_nid;
> >>> + int pin_idx, cvt_idx;
> >>> int curr;
> >>> - int err;
> >>> -
> >>> - for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
> >>> - per_pin = get_pin(spec, pin_idx);
> >>> + struct hdmi_spec_per_cvt *per_cvt;
> >>>
> >>> + /* configure all 3 pins, including "no physical connection" ones */
> >>> + for (pin_nid = 5; pin_nid < 8; pin_nid++) {
> >>
> >> It might not be so future proof to hard code nid IDs here (even if it
> >> should be safe right now?). Perhaps more elegant to go through the
> >> entire codec->nids array like being done here:
> >>
> >> http://lxr.linux.no/linux+*/sound/pci/hda/hda_auto_parser.c#L188
> >>
> >> The nid's wcaps is cached so it does not mean additional reads from the
> codec.
> >
> > Xingchao defined haswell_config_cvts() to configure converters on
> codec->pins before.
> > But codec->pins does not include the pins programmed by BIOS as "No
> physical connections" (like port B in the OEM machine).
> > So I change the code from using codec->pins to hard coded NIDs. It's not
> graceful and the NIDs may change in the future.
> >
> > There is another option to change add_pin() to not overlook such "no
> connection" pins for Haswell.
>
> I was suggesting to do something like this in haswell_config_cvts:
>
> int end_nid = codec->start_nid + codec->num_nodes;
> for (nid = codec->start_nid; nid < end_nid; nid++) {
> unsigned int wid_caps = get_wcaps(codec, nid);
> if (get_wcaps_type(wid_caps) != AC_WID_PIN)
> continue;
> /* etc */
>
>
Yes, this's elegant. I misunderstood your point in the afternoon.
I'll revise the patch as you suggested on Sunday, now out of office on holiday.
Thanks
Mengdong
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-09-20 14:48 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-16 2:40 [RFC PATCH] ALSA : hda - not use assigned converters for all unused pins mengdong.lin
2013-09-16 22:40 ` David Henningsson
2013-09-20 8:00 ` Lin, Mengdong
2013-09-20 9:32 ` David Henningsson
2013-09-20 14:48 ` Lin, Mengdong
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).