Re: [V2 PATCH 2/2] drm/i915: start adding dp mst audio

["Zanoni, Paulo R" <paulo.r.zanoni@intel.com>]

Em Qui, 2015-11-19 às 13:07 +0200, Ander Conselvan De Oliveira
escreveu:
> (Cc'ing Paulo for the audio power domain question)
> 
> On Wed, 2015-11-11 at 13:33 +0800, libin.yang@linux.intel.com wrote:
> > From: Libin Yang <libin.yang@linux.intel.com>
> > 
> > This patch adds support for DP MST audio in i915.
> > 
> > Enable audio codec when DP MST is enabled if has_audio flag is set.
> > Disable audio codec when DP MST is disabled if has_audio flag is
> > set.
> > 
> > Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> > Signed-off-by: Dave Airlie <airlied@redhat.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 14 ++++++++++++++
> >  drivers/gpu/drm/i915/intel_audio.c  |  9 ++++++---
> >  drivers/gpu/drm/i915/intel_dp_mst.c | 25 +++++++++++++++++++++++++
> >  3 files changed, 45 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 5659d4c..38c7a5d 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2873,6 +2873,18 @@ static void intel_dp_info(struct seq_file
> > *m,
> >  		intel_panel_info(m, &intel_connector->panel);
> >  }
> >  
> > +static void intel_dp_mst_info(struct seq_file *m,
> > +			  struct intel_connector *intel_connector)
> > +{
> > +	struct intel_encoder *intel_encoder = intel_connector-
> > >encoder;
> > +	struct intel_dp_mst_encoder *intel_mst =
> > +		enc_to_mst(&intel_encoder->base);
> > +	struct intel_digital_port *intel_dig_port = intel_mst-
> > >primary;
> > +	struct intel_dp *intel_dp = &intel_dig_port->dp;
> > +
> > +	seq_printf(m, "\taudio support: %s\n",
> > drm_dp_mst_port_has_audio(&intel_dp->mst_mgr, intel_connector-
> > >port) ? "yes" :
> > "no");
> 
> Too much going on in just one line. You can replace the ternary
> operator with
> yesno(). You could also add a has_audio bool:
> 
> bool has_audio = drm_dp_mst_port_has_audio(...);
> seq_printf(..., yesno(has_audio));
> 
> 
> > +}
> > +
> >  static void intel_hdmi_info(struct seq_file *m,
> >  			    struct intel_connector
> > *intel_connector)
> >  {
> > @@ -2916,6 +2928,8 @@ static void intel_connector_info(struct
> > seq_file *m,
> >  			intel_hdmi_info(m, intel_connector);
> >  		else if (intel_encoder->type == INTEL_OUTPUT_LVDS)
> >  			intel_lvds_info(m, intel_connector);
> > +		else if (intel_encoder->type ==
> > INTEL_OUTPUT_DP_MST)
> > +			intel_dp_mst_info(m, intel_connector);
> >  	}
> >  
> >  	seq_printf(m, "\tmodes:\n");
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> > b/drivers/gpu/drm/i915/intel_audio.c
> > index 63d4706..07b2aa6 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -262,7 +262,8 @@ static void hsw_audio_codec_disable(struct
> > intel_encoder
> > *encoder)
> >  	tmp |= AUD_CONFIG_N_PROG_ENABLE;
> >  	tmp &= ~AUD_CONFIG_UPPER_N_MASK;
> >  	tmp &= ~AUD_CONFIG_LOWER_N_MASK;
> > -	if (intel_pipe_has_type(intel_crtc,
> > INTEL_OUTPUT_DISPLAYPORT))
> > +	if (intel_pipe_has_type(intel_crtc,
> > INTEL_OUTPUT_DISPLAYPORT) ||
> > +		intel_pipe_has_type(intel_crtc,
> > INTEL_OUTPUT_DP_MST))
> 
> The second line should be aligned with '('.
> 
> >  		tmp |= AUD_CONFIG_N_VALUE_INDEX;
> >  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> >  
> > @@ -478,7 +479,8 @@ static void ilk_audio_codec_enable(struct
> > drm_connector
> > *connector,
> >  	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
> >  	tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> >  	tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK;
> > -	if (intel_pipe_has_type(intel_crtc,
> > INTEL_OUTPUT_DISPLAYPORT))
> > +	if (intel_pipe_has_type(intel_crtc,
> > INTEL_OUTPUT_DISPLAYPORT) ||
> > +		intel_pipe_has_type(intel_crtc,
> > INTEL_OUTPUT_DP_MST))
> 
> Same here.
> 
> >  		tmp |= AUD_CONFIG_N_VALUE_INDEX;
> >  	else
> >  		tmp |=
> > audio_config_hdmi_pixel_clock(adjusted_mode);
> > @@ -516,7 +518,8 @@ void intel_audio_codec_enable(struct
> > intel_encoder
> > *intel_encoder)
> >  
> >  	/* ELD Conn_Type */
> >  	connector->eld[5] &= ~(3 << 2);
> > -	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT))
> > +	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT) ||
> > +		intel_pipe_has_type(crtc, INTEL_OUTPUT_DP_MST))
> >  		connector->eld[5] |= (1 << 2);
> 
> And here.
> 
> >  
> >  	connector->eld[6] = drm_av_sync_delay(connector,
> > adjusted_mode) / 2;
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c
> > b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index 0639275..4ded0fb 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -78,6 +78,8 @@ static bool intel_dp_mst_compute_config(struct
> > intel_encoder
> > *encoder,
> >  		return false;
> >  	}
> >  
> > +	if (drm_dp_mst_port_has_audio(&intel_dp->mst_mgr, found-
> > >port))
> > +		pipe_config->has_audio = true;
> >  	mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock,
> > bpp);
> >  
> >  	pipe_config->pbn = mst_pbn;
> > @@ -102,6 +104,11 @@ static void intel_mst_disable_dp(struct
> > intel_encoder
> > *encoder)
> >  	struct intel_dp_mst_encoder *intel_mst =
> > enc_to_mst(&encoder->base);
> >  	struct intel_digital_port *intel_dig_port = intel_mst-
> > >primary;
> >  	struct intel_dp *intel_dp = &intel_dig_port->dp;
> > +	struct drm_device *dev = encoder->base.dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct drm_crtc *crtc = encoder->base.crtc;
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +
> >  	int ret;
> >  
> >  	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> > @@ -112,6 +119,10 @@ static void intel_mst_disable_dp(struct
> > intel_encoder
> > *encoder)
> >  	if (ret) {
> >  		DRM_ERROR("failed to update payload %d\n", ret);
> >  	}
> > +	if (intel_crtc->config->has_audio) {
> > +		intel_audio_codec_disable(encoder);
> > +		intel_display_power_put(dev_priv,
> > POWER_DOMAIN_AUDIO);
> > +	}
> 
> It seems the power domain is only need for DDI platforms. Perhaps it
> would make
> sense to move that into hsw_audio_codec_{en,dis}able. Paulo?

The power domains code usually tries to be platform-independent,
abstracting away the different power wells, which are the platform-
dependent thing. So I'm fine with every platform getting the audio
power domain when it needs to use audio, even if the platform doesn't
have power wells. So if some other platform in the future introduces
some new odd power well for audio, we can rely on the fact that we
already have the proper power domain get/put calls, and focus on just
implementing the correct translation from "audio power domain" to
proper power wells.

> 
> >  }
> >  
> >  static void intel_mst_post_disable_dp(struct intel_encoder
> > *encoder)
> > @@ -214,6 +225,7 @@ static void intel_mst_enable_dp(struct
> > intel_encoder
> > *encoder)
> >  	struct intel_dp *intel_dp = &intel_dig_port->dp;
> >  	struct drm_device *dev = intel_dig_port->base.base.dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct intel_crtc *crtc = to_intel_crtc(encoder-
> > >base.crtc);
> >  	enum port port = intel_dig_port->port;
> >  	int ret;
> >  
> > @@ -226,6 +238,13 @@ static void intel_mst_enable_dp(struct
> > intel_encoder
> > *encoder)
> >  	ret = drm_dp_check_act_status(&intel_dp->mst_mgr);
> >  
> >  	ret = drm_dp_update_payload_part2(&intel_dp->mst_mgr);
> > +
> > +	if (crtc->config->has_audio) {
> > +		DRM_DEBUG_DRIVER("Enabling DP audio on pipe %c\n",
> > +				 pipe_name(crtc->pipe));
> > +		intel_display_power_get(dev_priv,
> > POWER_DOMAIN_AUDIO);
> > +		intel_audio_codec_enable(encoder);
> > +	}
> >  }
> >  
> >  static bool intel_dp_mst_enc_get_hw_state(struct intel_encoder
> > *encoder,
> > @@ -251,6 +270,12 @@ static void intel_dp_mst_enc_get_config(struct
> > intel_encoder *encoder,
> >  
> >  	pipe_config->has_dp_encoder = true;
> >  
> > +	if (intel_display_power_is_enabled(dev_priv,
> > POWER_DOMAIN_AUDIO)) {
> > +		temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> > +		if (temp & AUDIO_OUTPUT_ENABLE(crtc->pipe))
> > +			pipe_config->has_audio = true;
> > +	}
> > +
> 
> This code is DDI specific. That shouldn't be a problem since we only
> support DP
> MST on DDI platforms, but we shouldn't duplicate that logic here. So
> I think
> this should be added to a function that can be called both from here
> and
> intel_ddi.c.


> Ander
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx