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; }