Bug 72352

Summary:	[HSW] Incorrect i915.ko audio enabling code, bad vblank wait
Product:	DRI	Reporter:	Paulo Zanoni <przanoni>
Component:	DRM/Intel	Assignee:	Paulo Zanoni <przanoni>
Status:	CLOSED FIXED	QA Contact:	Intel GFX Bugs mailing list <intel-gfx-bugs>
Severity:	normal
Priority:	medium	CC:	intel-gfx-bugs, jocelyn.li, mengdong.lin, przanoni
Version:	unspecified
Hardware:	Other
OS:	Linux (All)
Whiteboard:
i915 platform:		i915 features:
Bug Depends on:
Bug Blocks:	72211

Description Paulo Zanoni 2013-12-05 13:16:22 UTC

Hi

(this bug was reported somewhere else, and I was asked by the Audio team  to move it to freedesktop.org)

The function haswell_write_eld() from intel_display.c is responsible for enabling Audio on Haswell. This function is called by intel_write_eld(), which is called at mode_set time. When these functions are called, we're still at the very beginning of the mode set sequence, so the pipes, ports and planes are all stopped.

One of the problems I see is that this function calls intel_wait_for_vblank. Since the pipes are stopped, there's no vblank to wait, so what happens is that we sleep for the full 50ms timeout that's inside intel_wait_for_vblank, and we get a "vblank wait timed out" message every time. Waiting for a vblank at this point of the mode set sequence doesn't really make any sense.

I can imagine two possible solutions for this problem, but I'm not sure which one is correct:

Possible solution 1:
The docs suggests that Audio should only be enabled after the whole video mode set sequence is done, so we could perhaps call intel_write_eld just after the pipe is really running. I don't really know if we need to move more code besides intel_write_eld.

Possible solution 2:
If we conclude that intel_write_eld is called at the right place, we should consider replacing the intel_wait_for_vblank call with something that makes more sense and waits just for the amount of time we actually need, not for full 50ms. We should really take into consideration that the pipe is stopped.

One of the goals of the i915.ko driver is to startup as fast as possible, so useless 50ms waits should really be removed.

Here's a sample backtrace from haswell_write_eld():
[ 5.447629] [<ffffffffa00a9d48>] haswell_write_eld+0x58/0x420 [i915]
[ 5.447666] [<ffffffffa00b3e42>] intel_write_eld+0xc2/0xe0 [i915]
[ 5.447703] [<ffffffffa00bf959>] intel_ddi_mode_set+0x1a9/0x1b0 [i915]
[ 5.447733] [<ffffffffa00b1eea>] __intel_set_mode+0x77a/0x1470 [i915]
[ 5.447762] [<ffffffffa00b50f6>] intel_set_mode+0x16/0x30 [i915]
[ 5.447788] [<ffffffffa00b5a06>] intel_crtc_set_config+0x7f6/0x9f0 [i915]


I also sent a patch on September to remove the wait since it doesn't make sense: http://lists.freedesktop.org/archives/intel-gfx/2013-September/033454.html , but the patch was not merged.

Thanks,
Paulo

Comment 1 Jocelyn Li 2013-12-06 00:49:42 UTC

Mengdong,

Could you take a look at this?

Thanks,
Jocelyn

Comment 2 Mengdong Lin 2014-02-07 09:04:29 UTC

Hi Paulo,

We're checking with HW owner, Joseph and Abid, about the audio enable sequence position and whether it's okay to remove intel_wait_for_vblank. 

We've including you in that mail - "Need your clarfication about position of audio codec enable sequence in HSW/BDW b-spec".


Thanks
Mengdong

Comment 3 Mengdong Lin 2014-02-20 07:59:11 UTC

Hi Paulo,

I think your patch below is okay:
http://lists.freedesktop.org/archives/intel-gfx/2013-September/033454.html

And I tested it on HSW/BDW and found no regression.


We don't want to move intel_write_eld() atm, because it's called after the port:pipe mapping is established. Although we enables audio before pipe and port are finally enabled, the unsol responses of the eld valid/presence detect are not lost to the audio driver.

Comment 4 Paulo Zanoni 2014-04-29 13:20:27 UTC

Looks like we merged "drm/i915: Remove vblank wait from haswell_write_eld" from Daniel instead of my patch. Closing bug.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.