Re: [PATCH v5] drm/i915: pass ELD to HDMI/DP audio driver

[Takashi Iwai <tiwai@suse.de>]

At Fri, 11 Nov 2011 17:24:21 +0800,
Wu Fengguang wrote:
> 
> On Fri, Nov 11, 2011 at 04:49:57PM +0800, Takashi Iwai wrote:
> > At Fri, 11 Nov 2011 16:22:41 +0800,
> > Wu Fengguang wrote:
> > > 
> > > On Fri, Nov 11, 2011 at 03:40:37PM +0800, Takashi Iwai wrote:
> > > > At Fri, 11 Nov 2011 10:29:25 +0800,
> > > > Wu Fengguang wrote:
> > > > > 
> > > > > On Thu, Nov 10, 2011 at 10:28:19PM +0800, Takashi Iwai wrote:
> > > > > > At Thu, 10 Nov 2011 21:51:50 +0800,
> > > > > > Wu Fengguang wrote:
> > > > > > > 
> > > > > > > > > > > So maybe the hardware is in some state that is unable to provide the
> > > > > > > > > > > real ELD content?
> > > > > > > > > > That's my guess as well. I think the hardware may still be doing some 
> > > > > > > > > > form of data negotiation with the HDMI display device at that stage, and 
> > > > > > > > > > doesn't have the copy of the EDID+ELD buffer until a tiny bit later. 
> > > > > > > > > > Possibly?
> > > > > > > > > 
> > > > > > > > > Look at the below dmesg. The ELD seem to available immediately after the DPMS
> > > > > > > > > state setting..
> > > > > > > > 
> > > > > > > > Interesting.  Does HDMI audio work at all while HDMI DPMS off?
> > > > > > > > It clears SDVO_ENABLE bit, so this might turn off both video and
> > > > > > > > audio?
> > > > > > > 
> > > > > > > We normally see transient blank screen and silence of audio when
> > > > > > > switching the video mode.
> > > > > > 
> > > > > > Well, what I suspected is that ELD won't be transferred while
> > > > > > SDVO_ENABLE is cleared.
> > > > > 
> > > > > It's not about SDVO_ENABLE. The transient ELD invalid state I see in
> > > > > dmesg is caused by the graphics driver doing
> > > > > 
> > > > >         ELD_Valid = 0           => trigger 1st unsolicited event
> > > > 
> > > > But why this triggers at *plugging*?  Wasn't it zero beforehand?
> > > > If I understand correctly, changing AUD_CNTL_ST register triggers the
> > > > HD-audio codec unsol event.  Writing even the same value matters?
> > > 
> > > Sorry I assumed the "mode switching" context.
> > 
> > Ah, I see.  But this could be suppressed by your patch
> >    drm/i915: don't trigger hotplug events on unchanged ELD
> > no?
> 
> Mostly. The ELD may still change if the mode switching changes the
> av-sync-delay field, or suppresses the bandwidth available to audio
> samples.

OK, that's possible.

> > (snip)
> > > > > Depending on the timing, the 1st unsolicited event may see
> > > > > ELD_Valid=0 (if it's fast enough) or ELD_Valid=1 (if the event
> > > > > handling is delayed after the graphics driver sets ELD_Valid=1).
> > > > > 
> > > > > I know that because I literally saw both cases happening in dmesg.
> > > > > The 1st hot plug event itself will send ELD_Valid=0, however the audio
> > > > > driver is not trusting this and always do a status query (the HDMI
> > > > > status line) whose result depends on the timing.
> > > > 
> > > > The problem in this procedure is that this looks as if you are
> > > > re-connecting the HDMI from the audio-codec POV.
> > > 
> > > The re-connecting events can be distinguished from the
> > > video-mode-switching events by the Presence_Detect bit.
> > 
> > Hm, OK, so the codec driver can simply ignore (or postpone) the case
> > when the connection is kept but ELD is invalidated.
> 
> Yes.
> 
> > > Here is one hot removal event (I just wrote a patch to trigger this),
> > > with Presence_Detect=0:
> > 
> > One note that we don't rely on PD bit because not all (non-Intel)
> > hardware report it correctly.
> 
> Oops. Do you imply ELDV is reliable on all platforms? ;-)

Oh hell, no :)
The driver tries to probe explicitly via GET_PIN_SENSE HD-audio verb.


> > > [   91.777028] [drm:ironlake_write_eld], ELD on pipe B 
> > > [   91.778561] HDMI hot plug event: Codec=3 Pin=6 Presence_Detect=0 ELD_Valid=1
> > > [   91.783078] HDMI status: Codec=3 Pin=6 Presence_Detect=0 ELD_Valid=0
> > > [   91.783083] [drm:ironlake_write_eld], Audio directed to unknown port
> > > [   91.783095] [drm:output_poll_execute], [CONNECTOR:12:HDMI-A-2] status updated from 1 to 2
> > > 
> > > The HDA spec even mentioned doing some timeout mechanism for the
> > > "Presence_Detect=1 ELD_Valid=0" state. Well it may help some corner
> > > cases, but perhaps not an urgent feature.
> > 
> > Yeah, this sounds like the workaround for such a case.
> 
> Yeah, your mentioned DVI case may be always in state
> 
>         "Presence_Detect=1 ELD_Valid=0"
> 
> where audio playback should be denied.
> 
> And there might be the error case that the 2nd event is lost or not
> generated at all for changing
> 
>         "Presence_Detect=1 ELD_Valid=0"
> to
>         "Presence_Detect=1 ELD_Valid=1"
> 
> > > > We might end up with some delayed probe with a dedicated work_struct
> > > > (because it's bad to have a too long delay in unsol event handler
> > > >  that run on a single workq).
> > > 
> > > Understand. What if the graphics driver can delay the ELD writing (I
> > > can try that), so that the audio driver only need to wait for
> > > something like 10ms? 
> > 
> > Or, we can introduce a dirty flag, and set it when ELD is changed,
> > but don't prase ELD contents yet.  First upon the next access, the
> > driver updates the status, and clear the dirty flag.  We may put a
> > small delay at this update, too.
> 
> It should work fine for "cat /proc/asound/card0/eld*" and other
> interfaces, however it could still delay the printks' significantly.

Well, this reminds me of another question -- do we need these printks
unconditionally?

> And it feels not good that accessing ELD may be blocked for some time..

Understood.

> So I now prefer to avoid the msleep totally and schedule a delayed
> work for parsing ELD.

OK, let me know if you have some test material.


thanks,

Takashi