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;
}
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
