On Tue, Apr 09, 2019 at 11:00:10PM +0300, Ville Syrjälä wrote: > On Tue, Apr 09, 2019 at 05:40:49PM +0300, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > Our SDVO audio support is pretty bogus. We can't push audio over the > > SDVO bus, so trying to enable audio in the SDVO control register doesn't > > do anything. In fact it looks like the SDVO encoder will always mix in > > the audio coming over HDA, and there's no (at least documented) way to > > disable that from our side. So HDMI audio does work currently but only by > > luck really. What is missing though is the ELD. > > Hmm. Looks like I forgot to update this text after the gen3 bug was > reported. The situation is that audio works on gen4 by luck. On gen3 > it got broken by the referenced commit since we no longer enable > HDMI encoding on the SDVO device (that will stop audio transmission > entirely). > > > > > To pass the ELD to the audio driver we need to write it to magic buffer > > in the SDVO encoder hardware which then gets pulled out via HDA in the > > other end. Ie. pretty much the same thing we had for native HDMI before > > we started to just pass the ELD between the drivers. This sort of > > explains why we even have that silly hardware buffer with native HDMI. > > > > $ cat /proc/asound/card0/eld#1.0 > > -monitor_present 0 > > -eld_valid 0 > > +monitor_present 1 > > +eld_valid 1 > > +monitor_name LG TV > > +connection_type HDMI > > +... > > > > This also fixes our state readout since we can now query the SDVO > > encoder about the state of the "ELD valid" and "presence detect" > > bits. As mentioned those don't actually control whether audio > > gets sent over the HDMI cable, but it's the best we can do. > > > > Cc: stable@xxxxxxxxxxxxxxx > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx> > > Cc: zardam@xxxxxxxxx > > Tested-by: zardam@xxxxxxxxx > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108976 > > Fixes: de44e256b92c ("drm/i915/sdvo: Shut up state checker with hdmi cards on gen3") > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> Matches the sdvo specs and bspec (SDVO_AUDIO_ENABLE is a reserved/MBZ bit on GEN3,3.5, and on GEN4 it's probably HDMI specific, since there is no audio traffic over the SDVO bus): Reviewed-by: Imre Deak <imre.deak@xxxxxxxxx> Btw, is it guaranteed that we have a valid ELD when force_audio == HDMI_AUDIO_ON ? > > --- > > drivers/gpu/drm/i915/intel_sdvo.c | 58 +++++++++++++++++++++----- > > drivers/gpu/drm/i915/intel_sdvo_regs.h | 3 ++ > > 2 files changed, 50 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c > > index 61db07244296..7f64352a3413 100644 > > --- a/drivers/gpu/drm/i915/intel_sdvo.c > > +++ b/drivers/gpu/drm/i915/intel_sdvo.c > > @@ -916,6 +916,13 @@ static bool intel_sdvo_set_colorimetry(struct intel_sdvo *intel_sdvo, > > return intel_sdvo_set_value(intel_sdvo, SDVO_CMD_SET_COLORIMETRY, &mode, 1); > > } > > > > +static bool intel_sdvo_set_audio_state(struct intel_sdvo *intel_sdvo, > > + u8 audio_state) > > +{ > > + return intel_sdvo_set_value(intel_sdvo, SDVO_CMD_SET_AUDIO_STAT, > > + &audio_state, 1); > > +} > > + > > #if 0 > > static void intel_sdvo_dump_hdmi_buf(struct intel_sdvo *intel_sdvo) > > { > > @@ -1487,11 +1494,6 @@ static void intel_sdvo_pre_enable(struct intel_encoder *intel_encoder, > > else > > sdvox |= SDVO_PIPE_SEL(crtc->pipe); > > > > - if (crtc_state->has_audio) { > > - WARN_ON_ONCE(INTEL_GEN(dev_priv) < 4); > > - sdvox |= SDVO_AUDIO_ENABLE; > > - } > > - > > if (INTEL_GEN(dev_priv) >= 4) { > > /* done in crtc_mode_set as the dpll_md reg must be written early */ > > } else if (IS_I945G(dev_priv) || IS_I945GM(dev_priv) || > > @@ -1635,8 +1637,13 @@ static void intel_sdvo_get_config(struct intel_encoder *encoder, > > if (sdvox & HDMI_COLOR_RANGE_16_235) > > pipe_config->limited_color_range = true; > > > > - if (sdvox & SDVO_AUDIO_ENABLE) > > - pipe_config->has_audio = true; > > + if (intel_sdvo_get_value(intel_sdvo, SDVO_CMD_GET_AUDIO_STAT, > > + &val, 1)) { > > + u8 mask = SDVO_AUDIO_ELD_VALID | SDVO_AUDIO_PRESENCE_DETECT; > > + > > + if ((val & mask) == mask) > > + pipe_config->has_audio = true; > > + } > > > > if (intel_sdvo_get_value(intel_sdvo, SDVO_CMD_GET_ENCODE, > > &val, 1)) { > > @@ -1647,6 +1654,32 @@ static void intel_sdvo_get_config(struct intel_encoder *encoder, > > intel_sdvo_get_avi_infoframe(intel_sdvo, pipe_config); > > } > > > > +static void intel_sdvo_disable_audio(struct intel_sdvo *intel_sdvo) > > +{ > > + intel_sdvo_set_audio_state(intel_sdvo, 0); > > +} > > + > > +static void intel_sdvo_enable_audio(struct intel_sdvo *intel_sdvo, > > + const struct intel_crtc_state *crtc_state, > > + const struct drm_connector_state *conn_state) > > +{ > > + const struct drm_display_mode *adjusted_mode = > > + &crtc_state->base.adjusted_mode; > > + struct drm_connector *connector = conn_state->connector; > > + u8 *eld = connector->eld; > > + > > + eld[6] = drm_av_sync_delay(connector, adjusted_mode) / 2; > > + > > + intel_sdvo_set_audio_state(intel_sdvo, 0); > > + > > + intel_sdvo_write_infoframe(intel_sdvo, SDVO_HBUF_INDEX_ELD, > > + SDVO_HBUF_TX_DISABLED, > > + eld, drm_eld_size(eld)); > > + > > + intel_sdvo_set_audio_state(intel_sdvo, SDVO_AUDIO_ELD_VALID | > > + SDVO_AUDIO_PRESENCE_DETECT); > > +} > > + > > static void intel_disable_sdvo(struct intel_encoder *encoder, > > const struct intel_crtc_state *old_crtc_state, > > const struct drm_connector_state *conn_state) > > @@ -1656,6 +1689,9 @@ static void intel_disable_sdvo(struct intel_encoder *encoder, > > struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc); > > u32 temp; > > > > + if (old_crtc_state->has_audio) > > + intel_sdvo_disable_audio(intel_sdvo); > > + > > intel_sdvo_set_active_outputs(intel_sdvo, 0); > > if (0) > > intel_sdvo_set_encoder_power_state(intel_sdvo, > > @@ -1741,6 +1777,9 @@ static void intel_enable_sdvo(struct intel_encoder *encoder, > > intel_sdvo_set_encoder_power_state(intel_sdvo, > > DRM_MODE_DPMS_ON); > > intel_sdvo_set_active_outputs(intel_sdvo, intel_sdvo->attached_output); > > + > > + if (pipe_config->has_audio) > > + intel_sdvo_enable_audio(intel_sdvo, pipe_config, conn_state); > > } > > > > static enum drm_mode_status > > @@ -2603,7 +2642,6 @@ static bool > > intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device) > > { > > struct drm_encoder *encoder = &intel_sdvo->base.base; > > - struct drm_i915_private *dev_priv = to_i915(encoder->dev); > > struct drm_connector *connector; > > struct intel_encoder *intel_encoder = to_intel_encoder(encoder); > > struct intel_connector *intel_connector; > > @@ -2640,9 +2678,7 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device) > > encoder->encoder_type = DRM_MODE_ENCODER_TMDS; > > connector->connector_type = DRM_MODE_CONNECTOR_DVID; > > > > - /* gen3 doesn't do the hdmi bits in the SDVO register */ > > - if (INTEL_GEN(dev_priv) >= 4 && > > - intel_sdvo_is_hdmi_connector(intel_sdvo, device)) { > > + if (intel_sdvo_is_hdmi_connector(intel_sdvo, device)) { > > connector->connector_type = DRM_MODE_CONNECTOR_HDMIA; > > intel_sdvo_connector->is_hdmi = true; > > } > > diff --git a/drivers/gpu/drm/i915/intel_sdvo_regs.h b/drivers/gpu/drm/i915/intel_sdvo_regs.h > > index db0ed499268a..e9ba3b047f93 100644 > > --- a/drivers/gpu/drm/i915/intel_sdvo_regs.h > > +++ b/drivers/gpu/drm/i915/intel_sdvo_regs.h > > @@ -707,6 +707,9 @@ struct intel_sdvo_enhancements_arg { > > #define SDVO_CMD_GET_AUDIO_ENCRYPT_PREFER 0x90 > > #define SDVO_CMD_SET_AUDIO_STAT 0x91 > > #define SDVO_CMD_GET_AUDIO_STAT 0x92 > > + #define SDVO_AUDIO_ELD_VALID (1 << 0) > > + #define SDVO_AUDIO_PRESENCE_DETECT (1 << 1) > > + #define SDVO_AUDIO_CP_READY (1 << 2) > > #define SDVO_CMD_SET_HBUF_INDEX 0x93 > > #define SDVO_HBUF_INDEX_ELD 0 > > #define SDVO_HBUF_INDEX_AVI_IF 1 > > -- > > 2.21.0 > > -- > Ville Syrjälä > Intel > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
