[alsa-devel] Unifying sound/pci/hda/patch_*hdmi.c

[Stephen Warren]

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

-- 
nvpublic