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

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
  xingchao.w...@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.w...@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

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
   xingchao.w...@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.w...@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

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
 xingchao.w...@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.w...@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.

 - 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

[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 xingchao.w...@linux.intel.com
---
 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;

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
xingchao.w...@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.w...@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.

- 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

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

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

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

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

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

5 matches

Site Navigation

Mail list logo

Footer information