Re: [Intel-gfx] [PATCH 3/4] drm/i915: Add display audio routing APIs for ALSA

2013-06-18 Thread Wang, Xingchao 
Hi Daniel,

> -Original Message-
> From: daniel.vet...@ffwll.ch [mailto:daniel.vet...@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-de...@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 +, Wang, Xingchao wrote:
> > Hi Daniel,
> >
> > > -Original Message-
> > > From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch]
> > > Sent: Saturday, June 15, 2013 3:18 AM
> > > To: Wang Xingchao
> > > Cc: Takashi Iwai; alsa-de...@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
> > >  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 
> > >
> > >
> > > 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

Re: [Intel-gfx] [PATCH 3/4] drm/i915: Add display audio routing APIs for ALSA

2013-06-18 Thread Daniel Vetter 
On Mon, Jun 17, 2013 at 12:52:41PM +, Wang, Xingchao wrote:
> Hi Daniel,
>
> > -Original Message-
> > From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch]
> > Sent: Saturday, June 15, 2013 3:18 AM
> > To: Wang Xingchao
> > Cc: Takashi Iwai; alsa-de...@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
> >  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 
> >
> >
> > 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

Re: [Intel-gfx] [PATCH 3/4] drm/i915: Add display audio routing APIs for ALSA

2013-06-17 Thread Wang, Xingchao 
Hi Daniel,

> -Original Message-
> From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch]
> Sent: Saturday, June 15, 2013 3:18 AM
> To: Wang Xingchao
> Cc: Takashi Iwai; alsa-de...@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
>  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 
> 
> 
> 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.
  
> - 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

Re: [Intel-gfx] [PATCH 3/4] drm/i915: Add display audio routing APIs for ALSA

2013-06-14 Thread Daniel Vetter 
On Fri, Jun 14, 2013 at 5:20 PM, Wang Xingchao
 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 


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.

- 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.

Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2013-06-14 Thread Wang Xingchao 
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 
---
 drivers/gpu/drm/i915/i915_drv.h  |  18 +
 drivers/gpu/drm/i915/intel_ddi.c | 131 +--
 drivers/gpu/drm/i915/intel_display.c |   7 +-
 drivers/gpu/drm/i915/intel_drv.h |   1 +
 include/drm/i915_powerwell.h |   5 ++
 5 files changed, 157 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 10a56c9..8248048 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -87,6 +87,7 @@ enum port {
I915_MAX_PORTS
 };
 #define port_name(p) ((p) + 'A')
+#define pin2port(p) ((p) + PORT_B)
 
 enum intel_display_power_domain {
POWER_DOMAIN_PIPE_A,
@@ -785,6 +786,20 @@ struct i915_power_well {
int i915_request;
 };
 
+#define DEFAULT_PIPE   -1
+/* Dp1.2 mode, one DDI port can choose multiple pipes */
+struct dp12_port {
+   int pipes[I915_MAX_PIPES];
+   int count;
+};
+
+/* audio routing info for haswell */
+struct i915_audio {
+   /* route map between pipe and DDI port */
+   struct dp12_port active_pipes[I915_MAX_PORTS];
+   int pin_eld;
+};
+
 struct i915_dri1_state {
unsigned allow_batchbuffer : 1;
u32 __iomem *gfx_hws_cpu_addr;
@@ -1147,6 +1162,9 @@ typedef struct drm_i915_private {
/* Haswell power well */
struct i915_power_well power_well;
 
+   /* Haswell audio routing */
+   struct i915_audio audio_route;
+
enum no_fbc_reason no_fbc_reason;
 
struct drm_mm_node *compressed_fb;
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 224ce25..82823b8 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -286,8 +286,11 @@ static void intel_ddi_mode_set(struct drm_encoder *encoder,
   struct drm_display_mode *adjusted_mode)
 {
struct drm_crtc *crtc = encoder->crtc;
+   struct drm_i915_private *dev_priv = crtc->dev->dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
struct intel_encoder *intel_encoder = to_intel_encoder(encoder);
+   struct i915_audio *hsw_audio = &dev_priv->audio_route;
+   struct dp12_port *hsw_port;
int port = intel_ddi_get_encoder_port(intel_encoder);
int pipe = intel_crtc->pipe;
int type = intel_encoder->type;
@@ -296,6 +299,12 @@ static void intel_ddi_mode_set(struct drm_encoder *encoder,
  port_name(port), pipe_name(pipe));
 
intel_crtc->eld_vld = false;
+
+   /* store pipe routing info */
+   hsw_port = &hsw_audio->active_pipes[port];
+   hsw_port->pipes[0] = pipe;
+   hsw_port->count = 1;
+
if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
struct intel_digital_port *intel_dig_port =
@@ -306,8 +315,8 @@ static void intel_ddi_mode_set(struct drm_encoder *encoder,
intel_dp->DP |= DDI_PORT_WIDTH(intel_dp->lane_count);
 
if (intel_dp->has_audio) {
-   DRM_DEBUG_DRIVER("DP audio on pipe %c on DDI\n",
-pipe_name(intel_crtc->pipe));
+   DRM_DEBUG_DRIVER("DP audio on pipe %c on DDI %c\n",
+pipe_name(intel_crtc->pipe), 
port_name(port));
 
/* write eld */
DRM_DEBUG_DRIVER("DP audio: write eld information\n");
@@ -324,8 +333,8 @@ static void intel_ddi_mode_set(struct drm_encoder *encoder,
 * and a new set of registers, so we leave it for future
 * patch bombing.
 */
-   DRM_DEBUG_DRIVER("HDMI audio on pipe %c on DDI\n",
-pipe_name(intel_crtc->pipe));
+   DRM_DEBUG_DRIVER("HDMI audio on pipe %c on DDI %c \n",
+pipe_name(intel_crtc->pipe), 
port_name(port));
 
/* write eld */
DRM_DEBUG_DRIVER("HDMI audio: write eld information\n");
@@ -1135,6 +1144,9 @@ static void intel_disable_ddi(struct intel_encoder 
*intel_encoder)
int type = intel_encoder->type;
struct drm_device *dev = encoder->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
+   struct i915_audio *hsw_audio = &dev_priv->audio_route;
+   struct dp12_port *hsw_port;
+   enum port port = intel_ddi_get_encoder_port(intel_encoder);
uint32_t tmp;