[alsa-devel] [ANNOUNCE] patch for INTEL HDMI codec

[Takashi Iwai]

At Tue, 28 Oct 2008 16:07:39 +0800,
Wu Fengguang wrote:
> 
> Hello,
> 
> We have recently enabled sound output for Intel HDMI codecs,
> and would like to share the good news and some early code :-)

Great!

> The audio enabling code includes both ALSA and X.Org patches.
> Attached is one single patch against today's sound-2.6 tree.
> It contains very basic code for handling
>         - audio infoframe
>         - ELD(E-EDID Like Data)
>         - unsolicited response
> 
> The patch is full enough to produce stereo sound via HDMI.
> But it still lacks features and contains many 'defined but unused' staffs.

Yeah I see.

> It was tested OK on ASUS P5E-VM board and HP 2230s notebook.
> It _should_ also work on Intel DG45ID board.

Does the ALSA patch work alone without changes in x.org side?

> Takashi, I can prepare a trimmed-down patch series, in doing so I'd like
> to know if you are interested in the basic DIP/ELD/UNSOL routines?

Yes.  I wanted to write it by myself, but forgot for long time since
I have no HDMI hardware for testing :)
Good to see someone finally adding it!

> Or should I only submit bare code comparable to the current patch_atihdmi.c?

If the codec is supposed to be (almost) compatible, it'd be a good
idea.  Otherwise (if you are uncertain), we can keep it separated as
your original patch.  I don't mind much about it.

And, maybe we'll need to move EDID handling to the common place,
anyway.


> 
> PS: the patch can now produce the following ELD information:
> 
> [10340.656719] eldv = 1, pinp = 1
> [10340.660711] ELD buffer size  is 64
> [10340.660716] ELD baseline len is 10*4
> [10340.660720] vendor block len is 20
> [10340.660724] ELD version      is CEA-861D or below
> [10340.660728] CEA-EDID version is CEA-861-B, C or D
> [10340.660732] manufacture name is 0x4544
> [10340.660736] product code     is 0xa02c
> [10340.660740] port id          is 0x0
> [10340.660743] HDCP support     is 0
> [10340.660747] AI support       is 0
> [10340.660751] SAD count        is 0
> [10340.660754] audio sync delay is 0
> [10340.660758] connection type  is HDMI
> [10340.660763] speaker allocations:
> [10340.660766] monitor name     is DELL 2408WFP

Looks good.


> --- /dev/null
> +++ sound-2.6/sound/pci/hda/patch_intelhdmi.c
(snip)
> +struct ELD_fixed_fields {
> +	__u8 rsv_0		:3;
> +	__u8 ELD_ver		:5;

We should avoid bitfields if it's used for communication with the
hardware in general for portability reason.  Simply take it as bytes,
and use normal bit shift ops.

Also, no reason to use "__" prefix there.


> +static void hdmi_debug_slot_mapping(struct hda_codec *codec)
> +{
> +#ifdef CONFIG_SND_DEBUG

I'd use CONFIG_SND_DEBUG_VERBOSE here.  Most distros turn on
CONFIG_SND_DEBUG=y, and this will be annoying.

> +	int i;
> +	int slot;
> +
> +	for (i = 0; i < 8; i++) {
> +		slot = snd_hda_codec_read(codec, CVT_NID, 0,
> +						AC_VERB_GET_HDMI_CHAN_SLOT, i);
> +		printk(KERN_DEBUG "ASP channel %d => slot %d\n",
> +				slot >> 4, slot & 0x7);
> +	}
> +#endif
> +}
> +static void hdmi_debug_present_sense(struct hda_codec *codec)
> +{
> +#ifdef CONFIG_SND_DEBUG

Ditto.

> +static int hdmi_get_eldd(struct hda_codec *codec, char **eldd)
> +{
> +	int i;
> +	int size;
> +	char *buf;
> +
> +	i = hdmi_present_sense(codec) & AC_PINSENSE_ELDV;
> +	if (!i)
> +		return 0;
> +
> +	size = snd_hda_codec_read(codec, PIN_NID, 0, AC_VERB_GET_HDMI_DIP_SIZE,
> +						     AC_DIPSIZE_ELD_BUF);
> +	if (size == 0) {
> +		/* wfg: workaround for ASUS P5E-VM HDMI board */
> +		snd_printk(KERN_INFO "ELD buf size is 0, force 64\n");
> +		size = 64;
> +	}
> +	if (size < sizeof(struct ELD_fixed_fields)) {
> +		snd_printk(KERN_WARNING "Invalid ELD buf size %d\n", size);
> +		return 0;
> +	}
> +	WARN_ON(size > PAGE_SIZE);

Better to check more explicity and show the value in the error
message, and restrict the size.

> +static void hdmi_parse_short_audio_desc(struct ELD_SAD *a)
> +{
> +	};

Unnecessary semicolon.

> --- sound-2.6.orig/sound/pci/hda/hda_intel.c
> +++ sound-2.6/sound/pci/hda/hda_intel.c
> @@ -1196,7 +1196,7 @@ static unsigned int azx_max_codecs[AZX_N
>   * report wrongly the non-existing 4th slot availability
>   */
>  static unsigned int azx_default_codecs[AZX_NUM_DRIVERS] __devinitdata = {
> -	[AZX_DRIVER_ICH] = 3,
> +	[AZX_DRIVER_ICH] = 4,
>  	[AZX_DRIVER_ATI] = 3,

Well, this change should be done in a separate patch.
I guess your hardware has the HDMI code on the 4th slot, and the audio
codec on other?

This number 3 was basically to avoid lock-up by a wrong detection on
some hardwares such as thinkpad X60 with some old BIOS.  I remember
there were others, too.

I guess, however, this workaround is no longer necessary because we
clear the slot bits beforehand properly.  If any, a user can add
probe_mask option manually, and we can put it in the blacklist.

So, I'd say, let's remove this azx_default_codecs[] stuff completely
now.


thanks,

Takashi