[alsa-devel] Unifying sound/pci/hda/patch_*hdmi.c
Hi,
the below is a patch for unifying three patch_*hdmi.c into a single file. I'd like to push this to 2.6.37, so please review, especially whether it can break Nvidia hardware (since I have no h/w available).
In future, we can reduce ATI- and Nvidia-specific codes gradually after confirming the generic parser works.
The patch is found in sound-unstable git tree, too. The corresponding alsa-driver-unstable tarball can be used for external builds.
thanks,
Takashi
Thanks. I'll test it on Intel hardware tomorrow :)
On Wed, Sep 15, 2010 at 04:48:55PM +0200, Takashi Iwai wrote:
Hi,
the below is a patch for unifying three patch_*hdmi.c into a single file. I'd like to push this to 2.6.37, so please review, especially whether it can break Nvidia hardware (since I have no h/w available).
In future, we can reduce ATI- and Nvidia-specific codes gradually after confirming the generic parser works.
The patch is found in sound-unstable git tree, too. The corresponding alsa-driver-unstable tarball can be used for external builds.
thanks,
Takashi
Takashi,
Since the driver will also be supporting DisplayPort audio, would you add "DisplayPort" too in the following chunk?
+config SND_HDA_CODEC_HDMI ==> + bool "Build HDMI HD-audio codec support" select SND_DYNAMIC_MINORS default y help - Say Y here to include INTEL HDMI HD-audio codec support in - snd-hda-intel driver, such as Eaglelake integrated HDMI. ==> + Say Y here to include HDMI HD-audio codec support in + snd-hda-intel driver. This includes all AMD/ATI, Intel and ==> + Nvidia HDMI codecs.
Thanks, Fengguang
Takashi Iwai wrote:
the below is a patch for unifying three patch_*hdmi.c into a single file. I'd like to push this to 2.6.37, so please review, especially whether it can break Nvidia hardware (since I have no h/w available).
First off, in general, I think working towards unifying all the HDMI code is a great goal.
However, this patch worries me a bit because it significantly changes the way some parts of the code work at the same time as performing the refactoring...
In particular, after a brief skim, I see the following issues:
+static int patch_nvhdmi_8ch_89(struct hda_codec *codec) +{ + struct hdmi_spec *spec; + int err = patch_generic_hdmi(codec); + + if (err < 0) + return err; + spec = codec->spec; + spec->old_pin_detect = 1; + spec->pcm_playback = &nvhdmi_pcm_playback_8ch_7x;
That last line is definitely wrong as far as I can tell. Just delete it?
+ return 0; +}
+static int patch_nvhdmi_8ch_7x(struct hda_codec *codec) +{ + struct hdmi_spec *spec; + int err = patch_generic_hdmi(codec);
The old nvhdmi_8ch_7x code was custom like nvhdmi_2ch. Unfortunately, I'm not familiar with the old HW nor why it used custom code instead of the generic parser. This seems like a risky change without testing.
One behavioral change I see is that patch_generic_hdmi will call snd_hda_eld_proc_new, which wasn't previously called for this codec type. Yet, nvhdmi_patch_ops_8ch_7x doesn't point unsol_event at hdmi_unsol_event, so the ELD stuff won't work. I don't know if the unsolicited event logic in the HW works on this chip or not, given the Linux driver hasn't used it before. I suppose it has to work, but who knows...
Related,
+static struct hda_codec_ops nvhdmi_patch_ops_2ch = { + .build_controls = generic_hdmi_build_controls, + .build_pcms = generic_hdmi_build_pcms, + .init = nvhdmi_7x_init, + .free = generic_hdmi_free, +};
Generic_hdmi_free calls snd_hda_eld_proc_free. Since nvhdmi_7x_init doesn't call generic_hdmi_init, will that attempt to free ELD structures that aren't allocated?
Once the above issues are thought through, I can try to find time to test this on HW. Unfortunately, I definitely don't have any of the "2ch" HW available to test at all. I do have a small subset of the "8ch_7x" and "8ch_89" HW (although my only HDMI sink at work only supports 2-channel audio). Also, my allotted time to work on the audio driver is very small right now, plus I'll be away from the office next week and hence won't have HW access to test anything anyway. Sorry I suck at being a tester!
In future, we can reduce ATI- and Nvidia-specific codes gradually after confirming the generic parser works.
The patch is found in sound-unstable git tree, too. The corresponding alsa-driver-unstable tarball can be used for external builds.
thanks,
Takashi
At Thu, 16 Sep 2010 11:10:13 -0700, Stephen Warren wrote:
Takashi Iwai wrote:
the below is a patch for unifying three patch_*hdmi.c into a single file. I'd like to push this to 2.6.37, so please review, especially whether it can break Nvidia hardware (since I have no h/w available).
First off, in general, I think working towards unifying all the HDMI code is a great goal.
However, this patch worries me a bit because it significantly changes the way some parts of the code work at the same time as performing the refactoring...
In particular, after a brief skim, I see the following issues:
+static int patch_nvhdmi_8ch_89(struct hda_codec *codec) +{
- struct hdmi_spec *spec;
- int err = patch_generic_hdmi(codec);
- if (err < 0)
return err;- spec = codec->spec;
- spec->old_pin_detect = 1;
- spec->pcm_playback = &nvhdmi_pcm_playback_8ch_7x;
That last line is definitely wrong as far as I can tell. Just delete it?
Ah, crap, this has to be removed.
- return 0;
+}
+static int patch_nvhdmi_8ch_7x(struct hda_codec *codec) +{
- struct hdmi_spec *spec;
- int err = patch_generic_hdmi(codec);
The old nvhdmi_8ch_7x code was custom like nvhdmi_2ch. Unfortunately, I'm not familiar with the old HW nor why it used custom code instead of the generic parser. This seems like a risky change without testing.
Looks like I did wrong copy&paste here and there. It was intended to call patch_nvhdmi_2ch() instead. The additional fix patch is below.
One behavioral change I see is that patch_generic_hdmi will call snd_hda_eld_proc_new, which wasn't previously called for this codec type. Yet, nvhdmi_patch_ops_8ch_7x doesn't point unsol_event at hdmi_unsol_event, so the ELD stuff won't work. I don't know if the unsolicited event logic in the HW works on this chip or not, given the Linux driver hasn't used it before. I suppose it has to work, but who knows...
Related,
+static struct hda_codec_ops nvhdmi_patch_ops_2ch = {
- .build_controls = generic_hdmi_build_controls,
- .build_pcms = generic_hdmi_build_pcms,
- .init = nvhdmi_7x_init,
- .free = generic_hdmi_free,
+};
Generic_hdmi_free calls snd_hda_eld_proc_free. Since nvhdmi_7x_init doesn't call generic_hdmi_init, will that attempt to free ELD structures that aren't allocated?
The call of snd_hda_eld_proc_free() is harmless, so I chose one function to reduce rather than having two.
Once the above issues are thought through, I can try to find time to test this on HW. Unfortunately, I definitely don't have any of the "2ch" HW available to test at all. I do have a small subset of the "8ch_7x" and "8ch_89" HW (although my only HDMI sink at work only supports 2-channel audio). Also, my allotted time to work on the audio driver is very small right now, plus I'll be away from the office next week and hence won't have HW access to test anything anyway. Sorry I suck at being a tester!
OK, no problem. Thanks for review, anyway.
Takashi
--- diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index c6d183e..7d4ed7f 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -1352,7 +1352,6 @@ static int patch_nvhdmi_8ch_89(struct hda_codec *codec) return err; spec = codec->spec; spec->old_pin_detect = 1; - spec->pcm_playback = &nvhdmi_pcm_playback_8ch_7x; return 0; }
@@ -1382,12 +1381,13 @@ static int patch_nvhdmi_2ch(struct hda_codec *codec) static int patch_nvhdmi_8ch_7x(struct hda_codec *codec) { struct hdmi_spec *spec; - int err = patch_generic_hdmi(codec); + int err = patch_nvhdmi_2ch(codec);
if (err < 0) return err; spec = codec->spec; spec->multiout.max_channels = 8; + spec->pcm_playback = &nvhdmi_pcm_playback_8ch_7x; codec->patch_ops = nvhdmi_patch_ops_8ch_7x; return 0; }
On Wed, Sep 15, 2010 at 04:48:55PM +0200, Takashi Iwai wrote:
Hi,
the below is a patch for unifying three patch_*hdmi.c into a single file. I'd like to push this to 2.6.37, so please review, especially whether it can break Nvidia hardware (since I have no h/w available).
It works on Eaglelake (G45) and CougarPoint. I cannot find a IbexPeak box for test for now, however its codec file is almost the same as CougarPoint, so there should be no big problem.
Thanks, Fengguang
participants (3)
-
Stephen Warren -
Takashi Iwai -
Wu Fengguang