Hi Daniel, > -----Original Message----- > From: daniel.vetter at ffwll.ch [mailto:daniel.vetter at ffwll.ch] On Behalf Of > Daniel Vetter > Sent: Tuesday, June 18, 2013 3:13 PM > To: Wang, Xingchao > Cc: Daniel Vetter; Wang Xingchao; Takashi Iwai; alsa-devel at alsa-project.org; > intel-gfx; David Henningsson > Subject: Re: [PATCH 3/4] drm/i915: Add display audio routing APIs for ALSA > > On Mon, Jun 17, 2013 at 12:52:41PM +0000, Wang, Xingchao wrote: > > Hi Daniel, > > > > > -----Original Message----- > > > From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] > > > Sent: Saturday, June 15, 2013 3:18 AM > > > To: Wang Xingchao > > > Cc: Takashi Iwai; alsa-devel at alsa-project.org; intel-gfx; David > > > Henningsson; Wang, Xingchao > > > Subject: Re: [PATCH 3/4] drm/i915: Add display audio routing APIs > > > for ALSA > > > > > > On Fri, Jun 14, 2013 at 5:20 PM, Wang Xingchao > > > <xingchao.wang at linux.intel.com> wrote: > > > > ALSA audio driver need know current audio routing infomation. > > > > i.e. Route map between codec pins(DDI ports) and Transcoders(Pipe). > > > > > > > > Also the new API let audio driver disable unused audio pin's output. > > > > This fixed the bug when three pins *ALL* have monitors connected, > > > > playing audio on one pin would cause audio output to all minitors. > > > > > > > > Signed-off-by: Wang Xingchao <xingchao.wang at linux.intel.com> > > > > > > > > > So I've started to have a real look at audio on haswell and audio on > > > i915 in general, and I'm seriously confused. Random observations, > > > but I fear this isn't the real story by far yet: > > > > > > - On haswell we now have a hooping 3 places that set up these audio > > > routing > > > register: haswell_write_eld (called from the connector enable > > > callbacks), intel_enable/disable_ddi and now the new hooks you're > > > adding here. That's 2 places too many. If we really need all three > > > places those need to be refactored so that the bit-frobbing logic is > > > all in one place. But I seriously doubt that we need them all. > > > > > > > Yeah I agree to put audio stuff in One place. Let me give you some > > background about the issue this Patch fixed. > > Basically audio side need such API in gfx side to enable/disable > > audio, that's three PIPE based audio enable bits. The issue comes > > when two monitors connected on DDI port B and port C, like: > > DDI B(pin 0) --> DP monitor > > DDI C(pin 1) --> HDMI minitor > > > > In haswell the pins default choose converter 0(hardware level) as data source, > so it's like: > > > > Converter 0 --> pin 0 + pin 1 + pin2 > > > > And when play audio on pin 0, pin 1 could get audio data too. > > Meanwhile pin 1 has HDMi minitor connected, you can hear audio. To > > fix this issue, I tried several solutions: > > 1) Disable pin 1/2 when play audio on pin 0, or disable pin 0/2 when > > play audio on pin 1 > > 2) mute pin 1/2 when play audio on pin 0, or mute pin 0/2 when play > > audio on pin 1 > > 3) configure pin 1/2 to choose other converters when play audio on pin 0. > > 4) disable audio output enable in gfx side, this is implemented in current > patchset. > > > > I prefer 1) or 2) it's very simple, but it doesnot work on Haswell. > > It's the issue I'm trying to fix: Pin 0 must be output-eanbe/unmute > > when playing audio on pin 1. Seems pin 1 has dependency on pin 0, and > > I have to disable audio output in gfx side. > > > > I don't think it's bad if we implement such API in gfx side, the > > question is you point out, to make it clean and simple. If Dp1.2 is > > needed in future, audio side need the pipe->DDI port connections map > > too. We can reuse power-well module to export such information. > > > > To implement solution 4) above, audio side handle request to disable audio > output in gfx side: > > - if only pin 0 is busy playing audio and using valid pipe, disable audio output > for pin 1/2. > > It's the same logic when only pin1 or pin 2 is busy playing audio. > > - if both pin0 and pin 1 are busy playing audio, only disable pin 2. > > - if all pins are playing audio, do nothing. > > > > In above case when both Pin 0 and pin 1 are connected with Dp and HDMI > > monitor, if only playing audio on pin 0, disable pin 1's audio output > > in gfx side Would cause eld info refresh in audio driver side, but > > that doesnot matter as when you need playing audio on pin 1, the audio > > output enable bit would be set again. > > > > The patch could fix the issue when playing audio on one pin, audio > > would route to another pin too. In such case, we can drop the second > > patch in fact. > > Just reading through your description I prefer option 3) since that should be > possible to implement in the audio side only. > > To reiterate why I don't really like 4) is that touching these bits will result in > unsolicted even interrupts on the audio side. So your patch doesn't seem to just > disable/enable audio, but there's a big chain of follow-up events going on. So > I'm afraid that there's some really subtile dependency in there making your > current solution fragile. > > So what's the downside of option 3? I've rework the patch and sent for Takashi's review. The new patch only has changes in audio side. Thanks --xingchao > > Yours, Daniel > > > > > - I've quickly read through the haswell audio modeset sequence. On a > > > glance I could see no reason why we need to have 3 different places > > > to set up the gfx side of the audio support, since it's much simpler > > > apparently: > > > > > > Enable sequence: 1. gfx side sets up registers 2. gfx side sets the > > > audio enable bit, which generates the interrup 3. audio side completes the > setup on its side. > > > > > > Disable sequence is just the inverse. So I don't think we need 3 > > > different places for this. > > > > > > - Both on ibx/cpt and also on haswell here we seem to rely on BIOS > > > preset values way too often. Or at least I didn't figure out where > > > these registes get initialized (pretty sure nowhere in i915.ko). We > > > have countless bugs where the BIOS tried to be fancy and set up > > > something we don't actually support. So please, if there's no really > > > good reason why we need to do things differently, have explicit > > > register setups. If there's something we need to preserve from the BIOS, it > needs to be done explicitly and must have a comment. > > > > > > - No global state or stuffing random things into dev_priv/crtc any > > > more. Our modeset infrastructure has a transactional state machine > > > now: First we compute state parameters in the various compute_config > > > functions and store all that into a struct intel_crtc_config > > > pipe_config. Then the modeset functions apply that state. Finally at > > > the end the hw state is cross-checked with the sw state. We need > > > this to properly support atomic modeset and fastboot. Yes, this > > > means that recent additions for haswell audio support like crtc->eld_vld > need to go away and be moved into pipe_config. > > > > > > - Changing the OUTPUT_ENABLE bits will result in interrupts on the audio > side. > > > But these functions here are only called from the audio side, so we > > > have a really complicated feedback loop. Given that your patches > > > need much better explanation of what's going on (preferably with > > > time/state diagrams). Also I think I need a more detailed > > > explanation of what exactly is broken currently on Haswell audio and how > these patches fix this. > > > > Hope above explanation is helpful for you, if it's not enough, I would > > draw some graph case by case to let you know. :) > > > > Thanks > > --xingchao > > > > > > Cheers, Daniel > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
