Re: [ANNOUNCE] patch for INTEL HDMI codec

[Takashi Iwai <tiwai@suse.de>]

At Tue, 28 Oct 2008 17:50:16 +0800,
Wu, Fengguang wrote:
> 
> On Tue, Oct 28, 2008 at 02:01:08AM -0700, Takashi Iwai wrote:
> > 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?
> 
> - P5E-VM:   works in console, ALSA patch is enough
> - HP 2230s: needs intel video driver to produce sound at all
> 
> - DG45ID:   (if I remember it right)
>   - produces noise in console since booting(this is why I call
>     hdmi_disable_output() in intel_hdmi_init(), will comment this case)
>   - sound OK both with vesa/intel video drivers

Thanks for detailed information.
If the ALSA patch doesn't do anything harmful, I'd happily apply it :)

> > > 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!
> 
> Yeah, hardware is critical.
> To do this work, we collected several HDMI boxes and monitors :-)

Wow.

Right now my colleagues have some HDMI devices for testing, but they
are ATI graphic boards (no surprise :)

> > > 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.
> 
> OK. The main logic for ati/nv/intel HDMI codecs should be the same,
> except some possible quirks.  One fact is, we migrated SiI1392 HDMI
> support from patch_atihdmi.c to patch_intelhdmi.c to make it work for
> our P5E-VM board.

Right.  I think SII1392 never worked properly with the current code.
So, it's no problem to move it.

> > And, maybe we'll need to move EDID handling to the common place,
> > anyway.
> 
> Yes. But what if the intel driver evolve/expand fast enough to be the
> one real driver for HDMI/Display Port codecs? ;-)

Heh, my grand plan is to have a perfect generic codec-parser.
IOW, the codec-specific code should be merged into the generic parser
in future.

> > > 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.
> 
> I think those kernel messages would be valuable informations for both
> developers and end users. Such information are also good candidates
> for sysfs(or procfs) exports, I guess.

I thought of that, too.  If you need a sysfs entry for a specific
codec, you can easily attach to hwdep sysfs directory.  See
hda_hwdep.c:snd_hda_hwdep_add_sysfs().


> > > --- /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.
> 
> This structure is for communication with intel's video driver.
> ie. intel video driver reuses that struct for filling the ELD buffer,
> so I think it should be OK.

Hm... Then this should work, although I don't agree fully with
bitfields for any communication protocols.

Anyway, it's no big problem.  This can be fixed if we see any
incompatibilities.

> > Also, no reason to use "__" prefix there.
> 
> OK.
> 
> > > +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.
> 
> Will use it - it wasn't only because I cannot find the _VERBOSE
> version at the time ;-)

That was renamed from CONFIG_SND_DEBUG_DETECT.


> > > --- 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.
> 
> OK, it's a temp solution - I read about the same patch from you in
> latest linux-2.6 tree :-)

Oh yes, there were some issues around it.

> > I guess your hardware has the HDMI code on the 4th slot, and the audio
> > codec on other?
> 
> It's required for DG45ID, however I no longer have access to it now :-(
> 
> > 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.
> 
> Agreed, I wondered why probe_mask and max limits exist side by side...

OK, I'll fix that now.


thanks,

Takashi