[alsa-devel] [PATCH 3/4] drm/i915: Add display audio routing APIs for ALSA

[Daniel Vetter]

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?

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