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

Submitted by libin.yang@linux.intel.com on Nov. 11, 2015, 5:33 a.m.

Details

Message ID 1447220002-82030-2-git-send-email-libin.yang@linux.intel.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

libin.yang@linux.intel.com Nov. 11, 2015, 5:33 a.m.
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(-)

Patch hide | download patch | download mbox

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");
+}
+
 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))
 		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))
 		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);
 
 	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);
+	}
 }
 
 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;
+	}
+
 	temp = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder));
 	if (temp & TRANS_DDI_PHSYNC)
 		flags |= DRM_MODE_FLAG_PHSYNC;

Comments

(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?

>  }
>  
>  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
On 11/19/2015 07:07 PM, Ander Conselvan De Oliveira wrote:
> (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));

Get it. Thanks.

>
>
>> +}
>> +
>>   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 '('.

OK, I see.

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

OK, I will try to add a helper function. Which file do you suggest 
adding the function it?

Best Regards,
Libin

>
> Ander
>
On Mon, 2015-11-23 at 10:23 +0800, Libin Yang wrote:
> On 11/19/2015 07:07 PM, Ander Conselvan De Oliveira wrote:
> > (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));
> 
> Get it. Thanks.
> 
> > 
> > 
> > > +}
> > > +
> > >   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 '('.
> 
> OK, I see.
> 
> > 
> > >   		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?
> > 
> > >   }
> > > 
> > >   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.
> 
> OK, I will try to add a helper function. Which file do you suggest 
> adding the function it?

Add it to intel_ddi.c, where the original occurrence of that code is.

Ander
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
On Mon, 2015-11-23 at 14:04 +0000, Zanoni, Paulo R wrote:
> 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.

Hmm, ok. So I think the way to go is to move the power domain handling into
intel_audio_codec_enable/disable. But that would be a separate patch, so Libin,
I think you can leave this as is.

Ander

> 
> > 
> > >  }
> > >  
> > >  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.
> 
>
Best Regards,
Libin

On 11/23/2015 11:01 PM, Ander Conselvan De Oliveira wrote:
> On Mon, 2015-11-23 at 14:04 +0000, Zanoni, Paulo R wrote:
>> 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.
>
> Hmm, ok. So I think the way to go is to move the power domain handling into
> intel_audio_codec_enable/disable. But that would be a separate patch, so Libin,
> I think you can leave this as is.

get it. Thanks.

Regards,
Libin

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