Skip to contentSkip to conversation

dp/mst: add SDP stream support

PATCH Superseded [email protected] 4 participants 9 messages Nov 11, 2015 – Nov 24, 2015

Subsystems (3)
DRM drivers DRM misc Intel i915

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH v2 1/2 dp/mst: add SDP stream support

From: Libin Yang <[email protected]>

This adds code to initialise the SDP streams for a sink in the simplest ordering.

I've no idea how you'd want to control the ordering at this level, so don't bother
until someone comes up with a use case.

Signed-off-by Libin Yang <[email protected]>
Signed-off-by Dave Airlie <[email protected]>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 38 ++++++++++++++++++++++++++++++++---
 include/drm/drm_dp_mst_helper.h       |  7 +++++--
 2 files changed, 40 insertions(+), 5 deletions(-)
drivers/gpu/drm/drm_dp_mst_topology.c +35 -3
index 809959d..f50eb7b 100644
@@ -666,7 +666,9 @@ static int build_enum_path_resources(struct drm_dp_sideband_msg_tx *msg, int por
 }
 
 static int build_allocate_payload(struct drm_dp_sideband_msg_tx *msg, int port_num,
-				  u8 vcpi, uint16_t pbn)
+				  u8 vcpi, uint16_t pbn,
+				  u8 number_sdp_streams,
+				  u8 *sdp_stream_sink)
 {
 	struct drm_dp_sideband_msg_req_body req;
 	memset(&req, 0, sizeof(req));

@@ -674,6 +676,9 @@ static int build_allocate_payload(struct drm_dp_sideband_msg_tx *msg, int port_n
 	req.u.allocate_payload.port_number = port_num;
 	req.u.allocate_payload.vcpi = vcpi;
 	req.u.allocate_payload.pbn = pbn;
+	req.u.allocate_payload.number_sdp_streams = number_sdp_streams;
+	memcpy(req.u.allocate_payload.sdp_stream_sink, sdp_stream_sink,
+		   number_sdp_streams);
 	drm_dp_encode_sideband_req(&req, msg);
 	msg->path_msg = true;
 	return 0;

@@ -1562,6 +1567,8 @@ static int drm_dp_payload_send_msg(struct drm_dp_mst_topology_mgr *mgr,
 	struct drm_dp_sideband_msg_tx *txmsg;
 	struct drm_dp_mst_branch *mstb;
 	int len, ret;
+	u8 sinks[DRM_DP_MAX_SDP_STREAMS];
+	int i;
 
 	mstb = drm_dp_get_validated_mstb_ref(mgr, port->parent);
 	if (!mstb)

@@ -1573,10 +1580,13 @@ static int drm_dp_payload_send_msg(struct drm_dp_mst_topology_mgr *mgr,
 		goto fail_put;
 	}
 
+	for (i = 0; i < port->num_sdp_streams; i++)
+		sinks[i] = i;
+
 	txmsg->dst = mstb;
 	len = build_allocate_payload(txmsg, port->port_num,
 				     id,
-				     pbn);
+				     pbn, port->num_sdp_streams, sinks);
 
 	drm_dp_queue_down_tx(mgr, txmsg);
 

@@ -2259,6 +2269,27 @@ out:
 EXPORT_SYMBOL(drm_dp_mst_detect_port);
 
 /**
+ * drm_dp_mst_port_has_audio() - Check whether port has audio capability or not
+ * @mgr: manager for this port
+ * @port: unverified pointer to a port.
+ *
+ * This returns whether the port supports audio or not.
+ */
+bool drm_dp_mst_port_has_audio(struct drm_dp_mst_topology_mgr *mgr,
+					struct drm_dp_mst_port *port)
+{
+	bool ret = false;
+
+	port = drm_dp_get_validated_port_ref(mgr, port);
+	if (!port)
+		return ret;
+	ret = port->has_audio;
+	drm_dp_put_port(port);
+	return ret;
+}
+EXPORT_SYMBOL(drm_dp_mst_port_has_audio);
+
+/**
  * drm_dp_mst_get_edid() - get EDID for an MST port
  * @connector: toplevel connector to get EDID for
  * @mgr: manager for this port

@@ -2283,6 +2314,7 @@ struct edid *drm_dp_mst_get_edid(struct drm_connector *connector, struct drm_dp_
 		edid = drm_get_edid(connector, &port->aux.ddc);
 		drm_mode_connector_set_tile_property(connector);
 	}
+	port->has_audio = drm_detect_monitor_audio(edid);
 	drm_dp_put_port(port);
 	return edid;
 }

@@ -2566,7 +2598,7 @@ static void drm_dp_mst_dump_mstb(struct seq_file *m,
 
 	seq_printf(m, "%smst: %p, %d\n", prefix, mstb, mstb->num_ports);
 	list_for_each_entry(port, &mstb->ports, next) {
-		seq_printf(m, "%sport: %d: ddps: %d ldps: %d, %p, conn: %p\n", prefix, port->port_num, port->ddps, port->ldps, port, port->connector);
+		seq_printf(m, "%sport: %d: ddps: %d ldps: %d, sdp: %d/%d, %p, conn: %p\n", prefix, port->port_num, port->ddps, port->ldps, port->num_sdp_streams, port->num_sdp_stream_sinks, port, port->connector);
 		if (port->mstb)
 			drm_dp_mst_dump_mstb(m, port->mstb);
 	}
include/drm/drm_dp_mst_helper.h +5 -2
index 5340099..74b5888 100644
@@ -94,6 +94,7 @@ struct drm_dp_mst_port {
 	struct drm_dp_mst_topology_mgr *mgr;
 
 	struct edid *cached_edid; /* for DP logical ports - make tiling work */
+	bool has_audio;
 };
 
 /**

@@ -215,13 +216,13 @@ struct drm_dp_sideband_msg_rx {
 	struct drm_dp_sideband_msg_hdr initial_hdr;
 };
 
-
+#define DRM_DP_MAX_SDP_STREAMS 16
 struct drm_dp_allocate_payload {
 	u8 port_number;
 	u8 number_sdp_streams;
 	u8 vcpi;
 	u16 pbn;
-	u8 sdp_stream_sink[8];
+	u8 sdp_stream_sink[DRM_DP_MAX_SDP_STREAMS];
 };
 
 struct drm_dp_allocate_payload_ack_reply {

@@ -484,6 +485,8 @@ int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
 
 enum drm_connector_status drm_dp_mst_detect_port(struct drm_connector *connector, struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port);
 
+bool drm_dp_mst_port_has_audio(struct drm_dp_mst_topology_mgr *mgr,
+					struct drm_dp_mst_port *port);
 struct edid *drm_dp_mst_get_edid(struct drm_connector *connector, struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port);
Signature · 6 lines
1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
author
#1 lore raw

PATCH v2 2/2 drm/i915: start adding dp mst audio

From: Libin Yang <[email protected]>

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 <[email protected]>
Signed-off-by Dave Airlie <[email protected]>
---
 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(-)
drivers/gpu/drm/i915/i915_debugfs.c +14 -0
index 5659d4c..38c7a5d 100644
@@ -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");
drivers/gpu/drm/i915/intel_audio.c +6 -3
index 63d4706..07b2aa6 100644
@@ -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;
drivers/gpu/drm/i915/intel_dp_mst.c +25 -0
index 0639275..4ded0fb 100644
@@ -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;
Signature · 6 lines
1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
#2 lore raw

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

(Cc'ing Paulo for the audio power domain question)

Quoted 34 lines · On Wed, 2015-11-11 at 13:33 +0800, [email protected] wrote:

From: Libin Yang <[email protected]>

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 <[email protected]>
Signed-off-by Dave Airlie <[email protected]>
---
 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(-)
drivers/gpu/drm/i915/i915_debugfs.c +10 -0
index 5659d4c..38c7a5d 100644
@@ -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",
Quoted 2 lines

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));

Quoted 20 lines
drivers/gpu/drm/i915/i915_debugfs.c +4 -0
+}
+
 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");
drivers/gpu/drm/i915/intel_audio.c +0 -0
index 63d4706..07b2aa6 100644

drivers/gpu/drm/i915/intel_audio.c static void hsw_audio_codec_disable(struct intel_encoder · line 267

Quoted 7 lines
drivers/gpu/drm/i915/intel_audio.c +2 -1
@@ -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 '('.

drivers/gpu/drm/i915/intel_audio.c static void ilk_audio_codec_enable(struct drm_connector · line 484

Quoted 11 lines
drivers/gpu/drm/i915/intel_audio.c +2 -1
 		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.

drivers/gpu/drm/i915/intel_audio.c void intel_audio_codec_enable(struct intel_encoder · line 524

Quoted 12 lines
drivers/gpu/drm/i915/intel_audio.c +0 -0
 		tmp |= AUD_CONFIG_N_VALUE_INDEX;
 	else
drivers/gpu/drm/i915/intel_audio.c +2 -1
 		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.

Quoted 31 lines
drivers/gpu/drm/i915/intel_audio.c +0 -0
 	connector->eld[6] = drm_av_sync_delay(connector, adjusted_mode) / 2;
drivers/gpu/drm/i915/intel_dp_mst.c +2 -0
index 0639275..4ded0fb 100644
@@ -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);
drivers/gpu/drm/i915/intel_dp_mst.c +5 -0
 	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;
drivers/gpu/drm/i915/intel_dp_mst.c +0 -0
 	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);

drivers/gpu/drm/i915/intel_dp_mst.c static void intel_mst_disable_dp(struct intel_encoder · line 126

Quoted 8 lines
drivers/gpu/drm/i915/intel_dp_mst.c +4 -0
@@ -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?

Quoted 28 lines
drivers/gpu/drm/i915/intel_dp_mst.c +0 -0
 }

static void intel_mst_post_disable_dp(struct intel_encoder *encoder)

drivers/gpu/drm/i915/intel_dp_mst.c +8 -0
@@ -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,

drivers/gpu/drm/i915/intel_dp_mst.c static void intel_dp_mst_enc_get_config(struct · line 279

Quoted 10 lines
drivers/gpu/drm/i915/intel_dp_mst.c +6 -0
@@ -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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

author
#3 lore raw

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

Quoted 43 lines · Ander Conselvan De Oliveira

(Cc'ing Paulo for the audio power domain question)

Quoted 34 lines · On Wed, 2015-11-11 at 13:33 +0800, [email protected] wrote:

From: Libin Yang <[email protected]>

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 <[email protected]>
Signed-off-by Dave Airlie <[email protected]>
---
  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(-)
drivers/gpu/drm/i915/i915_debugfs.c +10 -0
index 5659d4c..38c7a5d 100644
@@ -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",
Quoted 2 lines

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:

Quoted 2 lines

bool has_audio = drm_dp_mst_port_has_audio(...);
seq_printf(..., yesno(has_audio));

Get it. Thanks.

Quoted 15 lines
Quoted 15 lines
drivers/gpu/drm/i915/i915_debugfs.c +4 -0
+}
+
  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);
  	}
Quoted 16 lines
Quoted 7 lines
drivers/gpu/drm/i915/i915_debugfs.c +0 -0
  	seq_printf(m, "\tmodes:\n");
drivers/gpu/drm/i915/intel_audio.c +0 -0
index 63d4706..07b2aa6 100644

drivers/gpu/drm/i915/intel_audio.c static void hsw_audio_codec_disable(struct intel_encoder · line 267

Quoted 7 lines
drivers/gpu/drm/i915/intel_audio.c +2 -1
@@ -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.

Quoted 105 lines

drivers/gpu/drm/i915/intel_audio.c static void ilk_audio_codec_enable(struct drm_connector · line 484

Quoted 12 lines
drivers/gpu/drm/i915/intel_audio.c +2 -1
  		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.

drivers/gpu/drm/i915/intel_audio.c void intel_audio_codec_enable(struct intel_encoder · line 524

Quoted 12 lines
drivers/gpu/drm/i915/intel_audio.c +0 -0
  		tmp |= AUD_CONFIG_N_VALUE_INDEX;
  	else

  		tmp |= audio_config_hdmi_pixel_clock(adjusted_mode);
drivers/gpu/drm/i915/intel_audio.c +2 -1
@@ -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.

Quoted 31 lines
drivers/gpu/drm/i915/intel_audio.c +0 -0
  	connector->eld[6] = drm_av_sync_delay(connector, adjusted_mode) / 2;
drivers/gpu/drm/i915/intel_dp_mst.c +2 -0
index 0639275..4ded0fb 100644
@@ -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;
drivers/gpu/drm/i915/intel_dp_mst.c +5 -0
@@ -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);

drivers/gpu/drm/i915/intel_dp_mst.c static void intel_mst_disable_dp(struct intel_encoder · line 126

Quoted 8 lines
drivers/gpu/drm/i915/intel_dp_mst.c +4 -0
@@ -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?

Quoted 32 lines
drivers/gpu/drm/i915/intel_dp_mst.c +0 -0
  }

static void intel_mst_post_disable_dp(struct intel_encoder *encoder)

drivers/gpu/drm/i915/intel_dp_mst.c +8 -0
@@ -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,

drivers/gpu/drm/i915/intel_dp_mst.c +0 -0
@@ -251,6 +270,12 @@ static void intel_dp_mst_enc_get_config(struct
 intel_encoder *encoder,
 
  	pipe_config->has_dp_encoder = true;
Quoted 11 lines

drivers/gpu/drm/i915/intel_dp_mst.c static void intel_dp_mst_enc_get_config(struct intel_encoder *encoder, · line 278

Quoted 6 lines
drivers/gpu/drm/i915/intel_dp_mst.c +6 -0
+	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

Quoted 3 lines

Ander

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

#4 lore raw

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

Quoted 210 lines · Libin Yang
Quoted 45 lines · Ander Conselvan De Oliveira

(Cc'ing Paulo for the audio power domain question)

Quoted 34 lines · On Wed, 2015-11-11 at 13:33 +0800, [email protected] wrote:

From: Libin Yang <[email protected]>

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 <[email protected]>
Signed-off-by Dave Airlie <[email protected]>
---
  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(-)
drivers/gpu/drm/i915/i915_debugfs.c +10 -0
index 5659d4c..38c7a5d 100644
@@ -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",
Quoted 3 lines

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:

Quoted 2 lines

bool has_audio = drm_dp_mst_port_has_audio(...);
seq_printf(..., yesno(has_audio));

Get it. Thanks.

Quoted 24 lines
Quoted 24 lines
drivers/gpu/drm/i915/i915_debugfs.c +4 -0
+}
+
  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");
drivers/gpu/drm/i915/intel_audio.c +0 -0
index 63d4706..07b2aa6 100644
@@ -262,7 +262,8 @@ static void hsw_audio_codec_disable(struct

intel_encoder
*encoder)

Quoted 8 lines

drivers/gpu/drm/i915/intel_audio.c static void hsw_audio_codec_disable(struct intel_encoder *encoder) · line 266

Quoted 6 lines
  	tmp |= AUD_CONFIG_N_PROG_ENABLE;
  	tmp &= ~AUD_CONFIG_UPPER_N_MASK;
  	tmp &= ~AUD_CONFIG_LOWER_N_MASK;
drivers/gpu/drm/i915/intel_audio.c +2 -1
-	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.

Quoted 110 lines

drivers/gpu/drm/i915/intel_audio.c static void hsw_audio_codec_disable(struct intel_encoder *encoder) · line 266

Quoted 13 lines
  		tmp |= AUD_CONFIG_N_VALUE_INDEX;
  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
drivers/gpu/drm/i915/intel_audio.c +0 -0
@@ -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;

drivers/gpu/drm/i915/intel_audio.c +2 -1
-	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.

drivers/gpu/drm/i915/intel_audio.c void intel_audio_codec_enable(struct intel_encoder · line 524

Quoted 12 lines
  		tmp |= AUD_CONFIG_N_VALUE_INDEX;
  	else
  		tmp |= audio_config_hdmi_pixel_clock(adjusted_mode);
drivers/gpu/drm/i915/intel_audio.c +2 -1
@@ -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.

Quoted 34 lines
drivers/gpu/drm/i915/intel_audio.c +0 -0
  	connector->eld[6] = drm_av_sync_delay(connector, adjusted_mode)

/ 2;

drivers/gpu/drm/i915/intel_dp_mst.c +0 -0
index 0639275..4ded0fb 100644
@@ -78,6 +78,8 @@ static bool intel_dp_mst_compute_config(struct

intel_encoder
*encoder,
return false;
}

drivers/gpu/drm/i915/intel_dp_mst.c +7 -1
+	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);

drivers/gpu/drm/i915/intel_dp_mst.c static void intel_mst_disable_dp(struct intel_encoder · line 126

Quoted 8 lines
drivers/gpu/drm/i915/intel_dp_mst.c +4 -0
@@ -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?

Quoted 32 lines
drivers/gpu/drm/i915/intel_dp_mst.c +0 -0
  }

static void intel_mst_post_disable_dp(struct intel_encoder *encoder)

drivers/gpu/drm/i915/intel_dp_mst.c +8 -0
@@ -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,

drivers/gpu/drm/i915/intel_dp_mst.c +0 -0
@@ -251,6 +270,12 @@ static void intel_dp_mst_enc_get_config(struct
 intel_encoder *encoder,
 
  	pipe_config->has_dp_encoder = true;
Quoted 13 lines

drivers/gpu/drm/i915/intel_dp_mst.c static void intel_dp_mst_enc_get_config(struct intel_encoder *encoder, · line 278

Quoted 7 lines
drivers/gpu/drm/i915/intel_dp_mst.c +6 -0
+	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.

Quoted 2 lines

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
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

#5 lore raw

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

In reply to Ander Conselvan De Oliveira

Em Qui, 2015-11-19 às 13:07 +0200, Ander Conselvan De Oliveira escreveu:

Quoted 169 lines

(Cc'ing Paulo for the audio power domain question)

Quoted 39 lines · On Wed, 2015-11-11 at 13:33 +0800, [email protected] wrote:

From: Libin Yang <[email protected]>

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 <[email protected]>
Signed-off-by Dave Airlie <[email protected]>

---
 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(-)
drivers/gpu/drm/i915/i915_debugfs.c +0 -0
index 5659d4c..38c7a5d 100644
@@ -2873,6 +2873,18 @@ static void intel_dp_info(struct seq_file
 *m,
  		intel_panel_info(m, &intel_connector->panel);
  }

drivers/gpu/drm/i915/i915_debugfs.c +4 -0
+static void intel_dp_mst_info(struct seq_file *m,
+			  struct intel_connector *intel_connector)
+{
+	struct intel_encoder *intel_encoder = intel_connector-
Quoted 1 line

encoder;

drivers/gpu/drm/i915/i915_debugfs.c +3 -0
+	struct intel_dp_mst_encoder *intel_mst =
+		enc_to_mst(&intel_encoder->base);
+	struct intel_digital_port *intel_dig_port = intel_mst-
Quoted 1 line

primary;

drivers/gpu/drm/i915/i915_debugfs.c +3 -0
+	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-

Quoted 2 lines
Quoted 1 line

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));

Quoted 33 lines
drivers/gpu/drm/i915/i915_debugfs.c +2 -0
+}
+

 static void intel_hdmi_info(struct seq_file *m, struct intel_connector *intel_connector)
 {

drivers/gpu/drm/i915/i915_debugfs.c +2 -0
@@ -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");

drivers/gpu/drm/i915/intel_audio.c +0 -0
index 63d4706..07b2aa6 100644
@@ -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;

drivers/gpu/drm/i915/intel_audio.c +0 -1
-	if (intel_pipe_has_type(intel_crtc,

INTEL_OUTPUT_DISPLAYPORT))

drivers/gpu/drm/i915/intel_audio.c +1 -0
+	if (intel_pipe_has_type(intel_crtc,

INTEL_OUTPUT_DISPLAYPORT) ||

drivers/gpu/drm/i915/intel_audio.c +1 -0
+		intel_pipe_has_type(intel_crtc,
Quoted 1 line

INTEL_OUTPUT_DP_MST))

The second line should be aligned with '('.

Quoted 14 lines

  tmp |= AUD_CONFIG_N_VALUE_INDEX;
  I915_WRITE(HSW_AUD_CFG(pipe), tmp);

drivers/gpu/drm/i915/intel_audio.c +0 -0
@@ -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;

drivers/gpu/drm/i915/intel_audio.c +0 -1
-	if (intel_pipe_has_type(intel_crtc,

INTEL_OUTPUT_DISPLAYPORT))

drivers/gpu/drm/i915/intel_audio.c +1 -0
+	if (intel_pipe_has_type(intel_crtc,

INTEL_OUTPUT_DISPLAYPORT) ||

drivers/gpu/drm/i915/intel_audio.c +1 -0
+		intel_pipe_has_type(intel_crtc,
Quoted 1 line

INTEL_OUTPUT_DP_MST))

Same here.

Quoted 13 lines

  tmp |= AUD_CONFIG_N_VALUE_INDEX;
  else
  tmp |=
audio_config_hdmi_pixel_clock(adjusted_mode);

drivers/gpu/drm/i915/intel_audio.c +0 -0
@@ -516,7 +518,8 @@ void intel_audio_codec_enable(struct

intel_encoder
*intel_encoder)

  /* ELD Conn_Type */
  connector->eld[5] &= ~(3 << 2);

drivers/gpu/drm/i915/intel_audio.c +2 -1
-	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))
Quoted 1 line

  connector->eld[5] |= (1 << 2);

And here.

Quoted 44 lines


  connector->eld[6] = drm_av_sync_delay(connector, adjusted_mode) / 2;

drivers/gpu/drm/i915/intel_dp_mst.c +0 -0
index 0639275..4ded0fb 100644
@@ -78,6 +78,8 @@ static bool intel_dp_mst_compute_config(struct

intel_encoder
*encoder,
  return false;
  }

drivers/gpu/drm/i915/intel_dp_mst.c +1 -0
+	if (drm_dp_mst_port_has_audio(&intel_dp->mst_mgr, found-
Quoted 1 line

port))

drivers/gpu/drm/i915/intel_dp_mst.c +1 -0
+		pipe_config->has_audio = true;

  mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);

  pipe_config->pbn = mst_pbn;

drivers/gpu/drm/i915/intel_dp_mst.c +0 -0
@@ -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-

Quoted 1 line

primary;

  struct intel_dp *intel_dp = &intel_dig_port->dp;

drivers/gpu/drm/i915/intel_dp_mst.c +5 -0
+	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);

drivers/gpu/drm/i915/intel_dp_mst.c +0 -0
@@ -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); }

drivers/gpu/drm/i915/intel_dp_mst.c +1 -0
+	if (intel_crtc->config->has_audio) {
Quoted 8 lines

drivers/gpu/drm/i915/intel_dp_mst.c

Quoted 4 lines
drivers/gpu/drm/i915/intel_dp_mst.c +2 -0
+		intel_audio_codec_disable(encoder);
+		intel_display_power_put(dev_priv,

POWER_DOMAIN_AUDIO);

drivers/gpu/drm/i915/intel_dp_mst.c +1 -0
+	}

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.

Quoted 56 lines
Quoted 41 lines

 }

 static void intel_mst_post_disable_dp(struct intel_encoder *encoder)

drivers/gpu/drm/i915/intel_dp_mst.c +0 -0
@@ -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;

drivers/gpu/drm/i915/intel_dp_mst.c +1 -0
+	struct intel_crtc *crtc = to_intel_crtc(encoder-
Quoted 1 line

base.crtc);

  enum port port = intel_dig_port->port;
  int ret;

drivers/gpu/drm/i915/intel_dp_mst.c +0 -0
@@ -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);

drivers/gpu/drm/i915/intel_dp_mst.c +5 -0
+
+	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);

drivers/gpu/drm/i915/intel_dp_mst.c +2 -0
+		intel_audio_codec_enable(encoder);
+	}

 }

 static bool intel_dp_mst_enc_get_hw_state(struct intel_encoder *encoder,

drivers/gpu/drm/i915/intel_dp_mst.c +0 -0
@@ -251,6 +270,12 @@ static void intel_dp_mst_enc_get_config(struct
 intel_encoder *encoder,


  pipe_config->has_dp_encoder = true;

drivers/gpu/drm/i915/intel_dp_mst.c +1 -0
+	if (intel_display_power_is_enabled(dev_priv,

POWER_DOMAIN_AUDIO)) {

drivers/gpu/drm/i915/intel_dp_mst.c static void intel_dp_mst_enc_get_config(struct intel_encoder *encoder, · line 278

Quoted 5 lines
drivers/gpu/drm/i915/intel_dp_mst.c +5 -0
+		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.

Quoted 1 line

Ander

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

#6 lore raw

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

Quoted 180 lines · Paulo R

Em Qui, 2015-11-19 às 13:07 +0200, Ander Conselvan De Oliveira escreveu:

Quoted 161 lines

(Cc'ing Paulo for the audio power domain question)

Quoted 39 lines · On Wed, 2015-11-11 at 13:33 +0800, [email protected] wrote:

From: Libin Yang <[email protected]>

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 <[email protected]>
Signed-off-by Dave Airlie <[email protected]>
---
 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(-)
drivers/gpu/drm/i915/i915_debugfs.c +10 -0
index 5659d4c..38c7a5d 100644
@@ -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-

Quoted 2 lines
Quoted 1 line

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));

Quoted 33 lines
drivers/gpu/drm/i915/i915_debugfs.c +2 -0
+}
+
 static void intel_hdmi_info(struct seq_file *m,
 			    struct intel_connector

*intel_connector)
{

drivers/gpu/drm/i915/i915_debugfs.c +2 -0
@@ -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);
 	}
drivers/gpu/drm/i915/i915_debugfs.c +0 -0
 	seq_printf(m, "\tmodes:\n");
drivers/gpu/drm/i915/intel_audio.c +0 -0
index 63d4706..07b2aa6 100644
@@ -262,7 +262,8 @@ static void hsw_audio_codec_disable(struct

intel_encoder
*encoder)

drivers/gpu/drm/i915/intel_audio.c +0 -1
 	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))

drivers/gpu/drm/i915/intel_audio.c +1 -0
+	if (intel_pipe_has_type(intel_crtc,

INTEL_OUTPUT_DISPLAYPORT) ||

drivers/gpu/drm/i915/intel_audio.c +1 -0
+		intel_pipe_has_type(intel_crtc,
Quoted 1 line

INTEL_OUTPUT_DP_MST))

The second line should be aligned with '('.

Quoted 14 lines
drivers/gpu/drm/i915/intel_audio.c +0 -0
 		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,

drivers/gpu/drm/i915/intel_audio.c +0 -1
 	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))

drivers/gpu/drm/i915/intel_audio.c +1 -0
+	if (intel_pipe_has_type(intel_crtc,

INTEL_OUTPUT_DISPLAYPORT) ||

drivers/gpu/drm/i915/intel_audio.c +1 -0
+		intel_pipe_has_type(intel_crtc,
Quoted 1 line

INTEL_OUTPUT_DP_MST))

Same here.

Quoted 8 lines
drivers/gpu/drm/i915/intel_audio.c +0 -0
 		tmp |= AUD_CONFIG_N_VALUE_INDEX;
 	else
 		tmp |=

audio_config_hdmi_pixel_clock(adjusted_mode);

drivers/gpu/drm/i915/intel_audio.c +0 -0
@@ -516,7 +518,8 @@ void intel_audio_codec_enable(struct

intel_encoder
*intel_encoder)

drivers/gpu/drm/i915/intel_audio.c void intel_audio_codec_enable(struct intel_encoder *intel_encoder) · line 523

Quoted 6 lines
drivers/gpu/drm/i915/intel_audio.c +2 -1
 	/* 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.

Quoted 36 lines

connector->eld[6] = drm_av_sync_delay(connector,
adjusted_mode) / 2;

drivers/gpu/drm/i915/intel_dp_mst.c +0 -0
index 0639275..4ded0fb 100644
@@ -78,6 +78,8 @@ static bool intel_dp_mst_compute_config(struct

intel_encoder
*encoder,

drivers/gpu/drm/i915/intel_dp_mst.c +1 -0
 		return false;
 	}
 
+	if (drm_dp_mst_port_has_audio(&intel_dp->mst_mgr, found-
Quoted 1 line

port))

drivers/gpu/drm/i915/intel_dp_mst.c +1 -0
+		pipe_config->has_audio = true;
 	mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock,

bpp);

drivers/gpu/drm/i915/intel_dp_mst.c +0 -0
 	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-

Quoted 1 line

primary;

drivers/gpu/drm/i915/intel_dp_mst.c +5 -0
 	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;
Quoted 16 lines

drivers/gpu/drm/i915/intel_dp_mst.c

Quoted 12 lines
drivers/gpu/drm/i915/intel_dp_mst.c +0 -0
 	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);

@@ -112,6 +119,10 @@ static void intel_mst_disable_dp(struct

intel_encoder
*encoder)

drivers/gpu/drm/i915/intel_dp_mst.c +3 -0
 	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);

drivers/gpu/drm/i915/intel_dp_mst.c +1 -0
+	}

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?

Quoted 9 lines

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

Quoted 40 lines
Quoted 40 lines
Quoted 40 lines

}

static void intel_mst_post_disable_dp(struct intel_encoder
*encoder)

drivers/gpu/drm/i915/intel_dp_mst.c +0 -0
@@ -214,6 +225,7 @@ static void intel_mst_enable_dp(struct

intel_encoder
*encoder)

drivers/gpu/drm/i915/intel_dp_mst.c +1 -0
 	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-
Quoted 1 line

base.crtc);

drivers/gpu/drm/i915/intel_dp_mst.c +0 -0
 	enum port port = intel_dig_port->port;
 	int ret;

@@ -226,6 +238,13 @@ static void intel_mst_enable_dp(struct

intel_encoder
*encoder)

drivers/gpu/drm/i915/intel_dp_mst.c +5 -0
 	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);

drivers/gpu/drm/i915/intel_dp_mst.c +2 -0
+		intel_audio_codec_enable(encoder);
+	}
 }
 
 static bool intel_dp_mst_enc_get_hw_state(struct intel_encoder

*encoder,

drivers/gpu/drm/i915/intel_dp_mst.c +0 -0
@@ -251,6 +270,12 @@ static void intel_dp_mst_enc_get_config(struct
 intel_encoder *encoder,
 
 	pipe_config->has_dp_encoder = true;
Quoted 17 lines
Quoted 8 lines

drivers/gpu/drm/i915/intel_dp_mst.c static void intel_dp_mst_enc_get_config(struct intel_encoder *encoder, · line 278

Quoted 7 lines
drivers/gpu/drm/i915/intel_dp_mst.c +6 -0
+	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;
+	}
+
Quoted 7 lines

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.

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

author
#7 lore raw

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

Best Regards,
Libin

Quoted 191 lines · Ander Conselvan De Oliveira
Quoted 180 lines · Paulo R

Em Qui, 2015-11-19 às 13:07 +0200, Ander Conselvan De Oliveira escreveu:

Quoted 161 lines

(Cc'ing Paulo for the audio power domain question)

Quoted 39 lines · On Wed, 2015-11-11 at 13:33 +0800, [email protected] wrote:

From: Libin Yang <[email protected]>

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 <[email protected]>
Signed-off-by Dave Airlie <[email protected]>
---
  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(-)
drivers/gpu/drm/i915/i915_debugfs.c +10 -0
index 5659d4c..38c7a5d 100644
@@ -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-

Quoted 2 lines
Quoted 1 line

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));

Quoted 33 lines
drivers/gpu/drm/i915/i915_debugfs.c +2 -0
+}
+
  static void intel_hdmi_info(struct seq_file *m,
  			    struct intel_connector

*intel_connector)
{

drivers/gpu/drm/i915/i915_debugfs.c +2 -0
@@ -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");
drivers/gpu/drm/i915/intel_audio.c +0 -0
index 63d4706..07b2aa6 100644
@@ -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;

drivers/gpu/drm/i915/intel_audio.c +0 -1
-	if (intel_pipe_has_type(intel_crtc,

INTEL_OUTPUT_DISPLAYPORT))

drivers/gpu/drm/i915/intel_audio.c +1 -0
+	if (intel_pipe_has_type(intel_crtc,

INTEL_OUTPUT_DISPLAYPORT) ||

drivers/gpu/drm/i915/intel_audio.c +1 -0
+		intel_pipe_has_type(intel_crtc,
Quoted 1 line

INTEL_OUTPUT_DP_MST))

The second line should be aligned with '('.

Quoted 14 lines
  		tmp |= AUD_CONFIG_N_VALUE_INDEX;
  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
drivers/gpu/drm/i915/intel_audio.c +0 -0
@@ -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;

drivers/gpu/drm/i915/intel_audio.c +0 -1
-	if (intel_pipe_has_type(intel_crtc,

INTEL_OUTPUT_DISPLAYPORT))

drivers/gpu/drm/i915/intel_audio.c +1 -0
+	if (intel_pipe_has_type(intel_crtc,

INTEL_OUTPUT_DISPLAYPORT) ||

drivers/gpu/drm/i915/intel_audio.c +1 -0
+		intel_pipe_has_type(intel_crtc,
Quoted 1 line

INTEL_OUTPUT_DP_MST))

Same here.

Quoted 10 lines

tmp |= AUD_CONFIG_N_VALUE_INDEX;
else
tmp |=
audio_config_hdmi_pixel_clock(adjusted_mode);

drivers/gpu/drm/i915/intel_audio.c +0 -0
@@ -516,7 +518,8 @@ void intel_audio_codec_enable(struct

intel_encoder
*intel_encoder)

  	/* ELD Conn_Type */
  	connector->eld[5] &= ~(3 << 2);

drivers/gpu/drm/i915/intel_audio.c void intel_audio_codec_enable(struct intel_encoder *intel_encoder) · line 523

Quoted 4 lines
drivers/gpu/drm/i915/intel_audio.c +2 -1
-	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.

Quoted 36 lines

connector->eld[6] = drm_av_sync_delay(connector,
adjusted_mode) / 2;

drivers/gpu/drm/i915/intel_dp_mst.c +0 -0
index 0639275..4ded0fb 100644
@@ -78,6 +78,8 @@ static bool intel_dp_mst_compute_config(struct

intel_encoder
*encoder,
return false;
}

drivers/gpu/drm/i915/intel_dp_mst.c +1 -0
+	if (drm_dp_mst_port_has_audio(&intel_dp->mst_mgr, found-
Quoted 1 line

port))

drivers/gpu/drm/i915/intel_dp_mst.c +1 -0
+		pipe_config->has_audio = true;
  	mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock,

bpp);

  	pipe_config->pbn = mst_pbn;
drivers/gpu/drm/i915/intel_dp_mst.c +0 -0
@@ -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-

Quoted 1 line

primary;

struct intel_dp *intel_dp = &intel_dig_port->dp;

drivers/gpu/drm/i915/intel_dp_mst.c +5 -0
+	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;
Quoted 16 lines

drivers/gpu/drm/i915/intel_dp_mst.c

Quoted 12 lines
  	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
drivers/gpu/drm/i915/intel_dp_mst.c +0 -0
@@ -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);
}

drivers/gpu/drm/i915/intel_dp_mst.c +3 -0
+	if (intel_crtc->config->has_audio) {
+		intel_audio_codec_disable(encoder);
+		intel_display_power_put(dev_priv,

POWER_DOMAIN_AUDIO);

drivers/gpu/drm/i915/intel_dp_mst.c +1 -0
+	}

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?

Quoted 9 lines

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.

Quoted 3 lines

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

Quoted 60 lines

Ander

Quoted 57 lines
Quoted 55 lines
Quoted 37 lines

  }

static void intel_mst_post_disable_dp(struct intel_encoder
*encoder)

drivers/gpu/drm/i915/intel_dp_mst.c +0 -0
@@ -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;

drivers/gpu/drm/i915/intel_dp_mst.c +1 -0
+	struct intel_crtc *crtc = to_intel_crtc(encoder-
Quoted 1 line

base.crtc);

enum port port = intel_dig_port->port;
int ret;

drivers/gpu/drm/i915/intel_dp_mst.c +0 -0
@@ -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);
drivers/gpu/drm/i915/intel_dp_mst.c +5 -0
+
+	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);

drivers/gpu/drm/i915/intel_dp_mst.c +2 -0
+		intel_audio_codec_enable(encoder);
+	}
  }
 
  static bool intel_dp_mst_enc_get_hw_state(struct intel_encoder

*encoder,

drivers/gpu/drm/i915/intel_dp_mst.c +0 -0
@@ -251,6 +270,12 @@ static void intel_dp_mst_enc_get_config(struct
 intel_encoder *encoder,

drivers/gpu/drm/i915/intel_dp_mst.c static void intel_dp_mst_enc_get_config(struct · line 280

Quoted 10 lines
drivers/gpu/drm/i915/intel_dp_mst.c +6 -0
 
  	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.

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

#8 lore raw
In reply to [email protected]

Hi Jani,

Do you have time to have a review? Thanks.

Regards,
Libin

Quoted 182 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]]
Sent: Wednesday, November 11, 2015 1:33 PM
To: [email protected]; [email protected]; Vetter, Daniel; [email protected]
Cc: Yang, Libin; Libin Yang; Dave Airlie
Subject: [V2 PATCH 1/2] dp/mst: add SDP stream support

From: Libin Yang <[email protected]>

This adds code to initialise the SDP streams for a sink in the simplest ordering.

I've no idea how you'd want to control the ordering at this level, so don't bother
until someone comes up with a use case.

Signed-off-by Libin Yang <[email protected]>
Signed-off-by Dave Airlie <[email protected]>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 38
++++++++++++++++++++++++++++++++---
 include/drm/drm_dp_mst_helper.h       |  7 +++++--
 2 files changed, 40 insertions(+), 5 deletions(-)
drivers/gpu/drm/drm_dp_mst_topology.c +0 -0
index 809959d..f50eb7b 100644
@@ -666,7 +666,9 @@ static int build_enum_path_resources(struct

drm_dp_sideband_msg_tx *msg, int por
}

static int build_allocate_payload(struct drm_dp_sideband_msg_tx *msg,
int port_num,

drivers/gpu/drm/drm_dp_mst_topology.c +3 -1
-				  u8 vcpi, uint16_t pbn)
+				  u8 vcpi, uint16_t pbn,
+				  u8 number_sdp_streams,
+				  u8 *sdp_stream_sink)
 {
 	struct drm_dp_sideband_msg_req_body req;
 	memset(&req, 0, sizeof(req));

@@ -674,6 +676,9 @@ static int build_allocate_payload(struct

drm_dp_sideband_msg_tx *msg, int port_n

drivers/gpu/drm/drm_dp_mst_topology.c +1 -0
 	req.u.allocate_payload.port_number = port_num;
 	req.u.allocate_payload.vcpi = vcpi;
 	req.u.allocate_payload.pbn = pbn;
+	req.u.allocate_payload.number_sdp_streams =

number_sdp_streams;

drivers/gpu/drm/drm_dp_mst_topology.c +1 -0
+	memcpy(req.u.allocate_payload.sdp_stream_sink,

sdp_stream_sink,

drivers/gpu/drm/drm_dp_mst_topology.c +3 -0
+		   number_sdp_streams);
 	drm_dp_encode_sideband_req(&req, msg);
 	msg->path_msg = true;
 	return 0;

@@ -1562,6 +1567,8 @@ static int drm_dp_payload_send_msg(struct
 drm_dp_mst_topology_mgr *mgr,
 	struct drm_dp_sideband_msg_tx *txmsg;
 	struct drm_dp_mst_branch *mstb;
 	int len, ret;
+	u8 sinks[DRM_DP_MAX_SDP_STREAMS];
+	int i;
 
 	mstb = drm_dp_get_validated_mstb_ref(mgr, port->parent);
drivers/gpu/drm/drm_dp_mst_topology.c +5 -1
 	if (!mstb)

@@ -1573,10 +1580,13 @@ static int
 drm_dp_payload_send_msg(struct drm_dp_mst_topology_mgr *mgr,
 		goto fail_put;
 	}
 
+	for (i = 0; i < port->num_sdp_streams; i++)
+		sinks[i] = i;
+
 	txmsg->dst = mstb;
 	len = build_allocate_payload(txmsg, port->port_num,
 				     id,
-				     pbn);
+				     pbn, port->num_sdp_streams, sinks);
 
 	drm_dp_queue_down_tx(mgr, txmsg);

@@ -2259,6 +2269,27 @@ out:
 EXPORT_SYMBOL(drm_dp_mst_detect_port);
 
 /**
+ * drm_dp_mst_port_has_audio() - Check whether port has audio

capability or not
+ * @mgr: manager for this port
+ * @port: unverified pointer to a port.
+ *
+ * This returns whether the port supports audio or not.
+ */
+bool drm_dp_mst_port_has_audio(struct drm_dp_mst_topology_mgr *mgr,

drivers/gpu/drm/drm_dp_mst_topology.c +14 -0
+					struct drm_dp_mst_port *port)
+{
+	bool ret = false;
+
+	port = drm_dp_get_validated_port_ref(mgr, port);
+	if (!port)
+		return ret;
+	ret = port->has_audio;
+	drm_dp_put_port(port);
+	return ret;
+}
+EXPORT_SYMBOL(drm_dp_mst_port_has_audio);
+
+/**
  * drm_dp_mst_get_edid() - get EDID for an MST port
  * @connector: toplevel connector to get EDID for
  * @mgr: manager for this port

@@ -2283,6 +2314,7 @@ struct edid *drm_dp_mst_get_edid(struct

drm_connector *connector, struct drm_dp_

drivers/gpu/drm/drm_dp_mst_topology.c +1 -1
 		edid = drm_get_edid(connector, &port->aux.ddc);
 		drm_mode_connector_set_tile_property(connector);
 	}
+	port->has_audio = drm_detect_monitor_audio(edid);
 	drm_dp_put_port(port);
 	return edid;
 }

@@ -2566,7 +2598,7 @@ static void drm_dp_mst_dump_mstb(struct
 seq_file *m,
 
 	seq_printf(m, "%smst: %p, %d\n", prefix, mstb, mstb-
 >num_ports);
 	list_for_each_entry(port, &mstb->ports, next) {
-		seq_printf(m, "%sport: %d: ddps: %d ldps: %d, %p,

conn: %p\n", prefix, port->port_num, port->ddps, port->ldps, port, port-

Quoted 1 line

connector);

drivers/gpu/drm/drm_dp_mst_topology.c +1 -0
+		seq_printf(m, "%sport: %d: ddps: %d ldps: %d,

sdp: %d/%d, %p, conn: %p\n", prefix, port->port_num, port->ddps, port-

Quoted 1 line

ldps, port->num_sdp_streams, port->num_sdp_stream_sinks, port,

port->connector);

drivers/gpu/drm/drm_dp_mst_topology.c +0 -0
 		if (port->mstb)
 			drm_dp_mst_dump_mstb(m, port->mstb);
 	}
include/drm/drm_dp_mst_helper.h +0 -0
index 5340099..74b5888 100644
@@ -94,6 +94,7 @@ struct drm_dp_mst_port {
 	struct drm_dp_mst_topology_mgr *mgr;
 
 	struct edid *cached_edid; /* for DP logical ports - make tiling

work */

include/drm/drm_dp_mst_helper.h +3 -2
+	bool has_audio;
 };
 
 /**

@@ -215,13 +216,13 @@ struct drm_dp_sideband_msg_rx {
 	struct drm_dp_sideband_msg_hdr initial_hdr;
 };
 
-
+#define DRM_DP_MAX_SDP_STREAMS 16
 struct drm_dp_allocate_payload {
 	u8 port_number;
 	u8 number_sdp_streams;
 	u8 vcpi;
 	u16 pbn;
-	u8 sdp_stream_sink[8];
+	u8 sdp_stream_sink[DRM_DP_MAX_SDP_STREAMS];
 };
 
 struct drm_dp_allocate_payload_ack_reply {

@@ -484,6 +485,8 @@ int drm_dp_mst_hpd_irq(struct

drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl

enum drm_connector_status drm_dp_mst_detect_port(struct
drm_connector *connector, struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port);

+bool drm_dp_mst_port_has_audio(struct drm_dp_mst_topology_mgr *mgr,

include/drm/drm_dp_mst_helper.h +1 -0
+					struct drm_dp_mst_port *port);
 struct edid *drm_dp_mst_get_edid(struct drm_connector *connector,

struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port);

Quoted 2 lines

--
1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx