All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] drm/dp/mst: Sideband message transaction to power up/down nodes
@ 2017-09-06  1:26 Dhinakaran Pandiyan
  2017-09-06  1:26 ` [PATCH 2/2] drm/i915/mst: Use MST sideband message transaction for dpms Dhinakaran Pandiyan
                   ` (4 more replies)
  0 siblings, 5 replies; 23+ messages in thread
From: Dhinakaran Pandiyan @ 2017-09-06  1:26 UTC (permalink / raw)
  To: intel-gfx; +Cc: Dhinakaran Pandiyan, dri-devel, Harry Wentland

The POWER_DOWN_PHY and POWER_UP_PHY sideband message transactions allow
the source to reqest any node in a mst path or a whole path to be
powered down or up. This allows drivers to target a specific sink in the
MST topology, an improvement over just power managing the imediate
downstream device. Secondly, since the request-reply protocol waits for an
ACK, we can be sure that a downstream sink has enough time to respond to a
power up/down request.

Cc: Lyude <lyude@redhat.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 73 +++++++++++++++++++++++++++++++++++
 include/drm/drm_dp_mst_helper.h       |  2 +
 2 files changed, 75 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 41b492f99955..a9f12708a046 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -294,6 +294,12 @@ static void drm_dp_encode_sideband_req(struct drm_dp_sideband_msg_req_body *req,
 		memcpy(&buf[idx], req->u.i2c_write.bytes, req->u.i2c_write.num_bytes);
 		idx += req->u.i2c_write.num_bytes;
 		break;
+
+	case DP_POWER_DOWN_PHY:
+	case DP_POWER_UP_PHY:
+		buf[idx] = (req->u.port_num.port_number & 0xf) << 4;
+		idx++;
+		break;
 	}
 	raw->cur_len = idx;
 }
@@ -538,6 +544,22 @@ static bool drm_dp_sideband_parse_query_payload_ack(struct drm_dp_sideband_msg_r
 	return false;
 }
 
+
+static bool drm_dp_sideband_parse_power_updown_phy_ack(struct drm_dp_sideband_msg_rx *raw,
+						       struct drm_dp_sideband_msg_reply_body *repmsg)
+{
+	int idx = 1;
+
+	repmsg->u.port_number.port_number = (raw->msg[idx] >> 4) & 0xf;
+	idx++;
+	if (idx > raw->curlen) {
+		DRM_DEBUG_KMS("power up/down phy parse length fail %d %d\n",
+			      idx, raw->curlen);
+		return false;
+	}
+	return true;
+}
+
 static bool drm_dp_sideband_parse_reply(struct drm_dp_sideband_msg_rx *raw,
 					struct drm_dp_sideband_msg_reply_body *msg)
 {
@@ -567,6 +589,9 @@ static bool drm_dp_sideband_parse_reply(struct drm_dp_sideband_msg_rx *raw,
 		return drm_dp_sideband_parse_enum_path_resources_ack(raw, msg);
 	case DP_ALLOCATE_PAYLOAD:
 		return drm_dp_sideband_parse_allocate_payload_ack(raw, msg);
+	case DP_POWER_DOWN_PHY:
+	case DP_POWER_UP_PHY:
+		return drm_dp_sideband_parse_power_updown_phy_ack(raw, msg);
 	default:
 		DRM_ERROR("Got unknown reply 0x%02x\n", msg->req_type);
 		return false;
@@ -693,6 +718,22 @@ static int build_allocate_payload(struct drm_dp_sideband_msg_tx *msg, int port_n
 	return 0;
 }
 
+static int build_power_updown_phy(struct drm_dp_sideband_msg_tx *msg,
+				  int port_num, bool power_up)
+{
+	struct drm_dp_sideband_msg_req_body req;
+
+	if (power_up)
+		req.req_type = DP_POWER_UP_PHY;
+	else
+		req.req_type = DP_POWER_DOWN_PHY;
+
+	req.u.port_num.port_number = port_num;
+	drm_dp_encode_sideband_req(&req, msg);
+	msg->path_msg = true;
+	return 0;
+}
+
 static int drm_dp_mst_assign_payload_id(struct drm_dp_mst_topology_mgr *mgr,
 					struct drm_dp_vcpi *vcpi)
 {
@@ -1724,6 +1765,38 @@ static int drm_dp_payload_send_msg(struct drm_dp_mst_topology_mgr *mgr,
 	return ret;
 }
 
+int drm_dp_send_power_updown_phy(struct drm_dp_mst_topology_mgr *mgr,
+				 struct drm_dp_mst_port *port, bool power_up)
+{
+	struct drm_dp_sideband_msg_tx *txmsg;
+	int len, ret;
+
+	txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL);
+	if (!txmsg)
+		return -ENOMEM;
+
+	port = drm_dp_get_validated_port_ref(mgr, port);
+	if (!port)
+		return -EINVAL;
+
+	txmsg->dst = port->parent;
+	len = build_power_updown_phy(txmsg, port->port_num, power_up);
+	drm_dp_queue_down_tx(mgr, txmsg);
+
+	ret = drm_dp_mst_wait_tx_reply(port->parent, txmsg);
+	if (ret > 0) {
+		if (txmsg->reply.reply_type == 1)
+			ret = -EINVAL;
+		else
+			ret = 0;
+	}
+	kfree(txmsg);
+	drm_dp_put_port(port);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_dp_send_power_updown_phy);
+
 static int drm_dp_create_payload_step1(struct drm_dp_mst_topology_mgr *mgr,
 				       int id,
 				       struct drm_dp_payload *payload)
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index d55abb75f29a..7f78d26a0766 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -631,5 +631,7 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,
 int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
 				     struct drm_dp_mst_topology_mgr *mgr,
 				     int slots);
+int drm_dp_send_power_updown_phy(struct drm_dp_mst_topology_mgr *mgr,
+				 struct drm_dp_mst_port *port, bool power_up);
 
 #endif
-- 
2.11.0

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

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 2/2] drm/i915/mst: Use MST sideband message transaction for dpms
  2017-09-06  1:26 [PATCH 1/2] drm/dp/mst: Sideband message transaction to power up/down nodes Dhinakaran Pandiyan
@ 2017-09-06  1:26 ` Dhinakaran Pandiyan
  2017-09-07  5:51   ` Stefan Assmann
  2017-09-12 16:11   ` Ville Syrjälä
  2017-09-06  1:46 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/dp/mst: Sideband message transaction to power up/down nodes Patchwork
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 23+ messages in thread
From: Dhinakaran Pandiyan @ 2017-09-06  1:26 UTC (permalink / raw)
  To: intel-gfx; +Cc: Dhinakaran Pandiyan, dri-devel

Use the POWER_DOWN_PHY and POWER_UP_PHY sideband message trasactions to
set power states for downstream sinks. Apart from giving us the ability
to set power state for individual sinks, this fixes the below test for
me

$ xrandr --display :0 --output DP-2-2-8 --off
$ xrandr --display :0 --output DP-2-2-1 --off
$ xrandr --display :0 --output DP-2-2-8 --auto #Black screen
$ xrandr --display :0 --output DP-2-2-1 --auto

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Lyude <lyude@redhat.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c    | 6 ++++--
 drivers/gpu/drm/i915/intel_dp_mst.c | 8 ++++----
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 1da3bb2cc4b4..8aebacc0aa31 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2161,7 +2161,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
 		intel_prepare_dp_ddi_buffers(encoder);
 
 	intel_ddi_init_dp_buf_reg(encoder);
-	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
+	if (!link_mst)
+		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
 	intel_dp_start_link_train(intel_dp);
 	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
 		intel_dp_stop_link_train(intel_dp);
@@ -2240,7 +2241,8 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder,
 	if (type == INTEL_OUTPUT_DP || type == INTEL_OUTPUT_EDP) {
 		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 
-		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
+		if (old_crtc_state && old_conn_state)
+			intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
 	}
 
 	val = I915_READ(DDI_BUF_CTL(port));
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 8e3aad0ea60b..81e63724e24b 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -167,12 +167,11 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
 	intel_dp->active_mst_links--;
 
 	intel_mst->connector = NULL;
-	if (intel_dp->active_mst_links == 0) {
+	if (intel_dp->active_mst_links == 0)
 		intel_dig_port->base.post_disable(&intel_dig_port->base,
 						  NULL, NULL);
-
-		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
-	}
+	drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port,
+				     false);
 }
 
 static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
@@ -197,6 +196,7 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
 
 	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
 
+	drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port, true);
 	if (intel_dp->active_mst_links == 0)
 		intel_dig_port->base.pre_enable(&intel_dig_port->base,
 						pipe_config, NULL);
-- 
2.11.0

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

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/dp/mst: Sideband message transaction to power up/down nodes
  2017-09-06  1:26 [PATCH 1/2] drm/dp/mst: Sideband message transaction to power up/down nodes Dhinakaran Pandiyan
  2017-09-06  1:26 ` [PATCH 2/2] drm/i915/mst: Use MST sideband message transaction for dpms Dhinakaran Pandiyan
@ 2017-09-06  1:46 ` Patchwork
  2017-09-06 15:40 ` [PATCH 1/2] " Lyude Paul
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 23+ messages in thread
From: Patchwork @ 2017-09-06  1:46 UTC (permalink / raw)
  To: Dhinakaran Pandiyan; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/2] drm/dp/mst: Sideband message transaction to power up/down nodes
URL   : https://patchwork.freedesktop.org/series/29853/
State : warning

== Summary ==

Series 29853v1 series starting with [1/2] drm/dp/mst: Sideband message transaction to power up/down nodes
https://patchwork.freedesktop.org/api/1.0/series/29853/revisions/1/mbox/

Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-atomic:
                fail       -> PASS       (fi-snb-2600) fdo#100215 +1
Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                pass       -> SKIP       (fi-skl-x1585l) fdo#101781
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> SKIP       (fi-cfl-s)
Test drv_module_reload:
        Subgroup basic-reload-inject:
                dmesg-fail -> DMESG-WARN (fi-cfl-s) k.org#196765

fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215
fdo#101781 https://bugs.freedesktop.org/show_bug.cgi?id=101781
k.org#196765 https://bugzilla.kernel.org/show_bug.cgi?id=196765

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:457s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:443s
fi-blb-e6850     total:289  pass:224  dwarn:1   dfail:0   fail:0   skip:64  time:361s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:556s
fi-bwr-2160      total:289  pass:184  dwarn:0   dfail:0   fail:0   skip:105 time:255s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:521s
fi-byt-j1900     total:289  pass:254  dwarn:1   dfail:0   fail:0   skip:34  time:519s
fi-byt-n2820     total:289  pass:250  dwarn:1   dfail:0   fail:0   skip:38  time:510s
fi-cfl-s         total:289  pass:249  dwarn:4   dfail:0   fail:0   skip:36  time:461s
fi-elk-e7500     total:289  pass:230  dwarn:0   dfail:0   fail:0   skip:59  time:434s
fi-glk-2a        total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:610s
fi-hsw-4770      total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:457s
fi-hsw-4770r     total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:424s
fi-ilk-650       total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:428s
fi-ivb-3520m     total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:500s
fi-ivb-3770      total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:473s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:517s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:598s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:601s
fi-pnv-d510      total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:533s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:474s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:529s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:512s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:440s
fi-skl-x1585l    total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:489s
fi-snb-2520m     total:289  pass:251  dwarn:0   dfail:0   fail:0   skip:38  time:555s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:1   skip:39  time:409s

22fe7d7fe4c7a6e04c7f7f99d6b8985008616546 drm-tip: 2017y-09m-05d-19h-15m-06s UTC integration manifest
f61f7362822c drm/i915/mst: Use MST sideband message transaction for dpms
4dd49840e139 drm/dp/mst: Sideband message transaction to power up/down nodes

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5589/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/2] drm/dp/mst: Sideband message transaction to power up/down nodes
  2017-09-06  1:26 [PATCH 1/2] drm/dp/mst: Sideband message transaction to power up/down nodes Dhinakaran Pandiyan
  2017-09-06  1:26 ` [PATCH 2/2] drm/i915/mst: Use MST sideband message transaction for dpms Dhinakaran Pandiyan
  2017-09-06  1:46 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/dp/mst: Sideband message transaction to power up/down nodes Patchwork
@ 2017-09-06 15:40 ` Lyude Paul
  2017-09-06 16:36   ` Pandiyan, Dhinakaran
  2017-09-07  0:14   ` [PATCH v2] " Dhinakaran Pandiyan
  2017-09-07  0:35 ` ✗ Fi.CI.BAT: failure for series starting with [v2] drm/dp/mst: Sideband message transaction to power up/down nodes (rev2) Patchwork
  2017-09-07  5:49 ` [PATCH 1/2] drm/dp/mst: Sideband message transaction to power up/down nodes Stefan Assmann
  4 siblings, 2 replies; 23+ messages in thread
From: Lyude Paul @ 2017-09-06 15:40 UTC (permalink / raw)
  To: Dhinakaran Pandiyan, intel-gfx; +Cc: dri-devel

On Tue, 2017-09-05 at 18:26 -0700, Dhinakaran Pandiyan wrote:
> The POWER_DOWN_PHY and POWER_UP_PHY sideband message transactions
> allow
> the source to reqest any node in a mst path or a whole path to be
> powered down or up. This allows drivers to target a specific sink in
> the
> MST topology, an improvement over just power managing the imediate
> downstream device. Secondly, since the request-reply protocol waits
> for an
> ACK, we can be sure that a downstream sink has enough time to respond
> to a
> power up/down request.
> 
> Cc: Lyude <lyude@redhat.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 73
> +++++++++++++++++++++++++++++++++++
>  include/drm/drm_dp_mst_helper.h       |  2 +
>  2 files changed, 75 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 41b492f99955..a9f12708a046 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -294,6 +294,12 @@ static void drm_dp_encode_sideband_req(struct
> drm_dp_sideband_msg_req_body *req,
>  		memcpy(&buf[idx], req->u.i2c_write.bytes, req-
> >u.i2c_write.num_bytes);
>  		idx += req->u.i2c_write.num_bytes;
>  		break;
> +
> +	case DP_POWER_DOWN_PHY:
> +	case DP_POWER_UP_PHY:
> +		buf[idx] = (req->u.port_num.port_number & 0xf) << 4;
> +		idx++;
> +		break;
>  	}
>  	raw->cur_len = idx;
>  }
> @@ -538,6 +544,22 @@ static bool
> drm_dp_sideband_parse_query_payload_ack(struct drm_dp_sideband_msg_r
>  	return false;
>  }
>  
> +
> +static bool drm_dp_sideband_parse_power_updown_phy_ack(struct
> drm_dp_sideband_msg_rx *raw,
> +						       struct
> drm_dp_sideband_msg_reply_body *repmsg)
> +{
> +	int idx = 1;
> +
> +	repmsg->u.port_number.port_number = (raw->msg[idx] >> 4) &
> 0xf;
> +	idx++;
> +	if (idx > raw->curlen) {
> +		DRM_DEBUG_KMS("power up/down phy parse length fail
> %d %d\n",
> +			      idx, raw->curlen);
> +		return false;
> +	}
> +	return true;
> +}
> +
>  static bool drm_dp_sideband_parse_reply(struct
> drm_dp_sideband_msg_rx *raw,
>  					struct
> drm_dp_sideband_msg_reply_body *msg)
>  {
> @@ -567,6 +589,9 @@ static bool drm_dp_sideband_parse_reply(struct
> drm_dp_sideband_msg_rx *raw,
>  		return
> drm_dp_sideband_parse_enum_path_resources_ack(raw, msg);
>  	case DP_ALLOCATE_PAYLOAD:
>  		return
> drm_dp_sideband_parse_allocate_payload_ack(raw, msg);
> +	case DP_POWER_DOWN_PHY:
> +	case DP_POWER_UP_PHY:
> +		return
> drm_dp_sideband_parse_power_updown_phy_ack(raw, msg);
>  	default:
>  		DRM_ERROR("Got unknown reply 0x%02x\n", msg-
> >req_type);
>  		return false;
> @@ -693,6 +718,22 @@ static int build_allocate_payload(struct
> drm_dp_sideband_msg_tx *msg, int port_n
>  	return 0;
>  }
>  
> +static int build_power_updown_phy(struct drm_dp_sideband_msg_tx
> *msg,
> +				  int port_num, bool power_up)
> +{
> +	struct drm_dp_sideband_msg_req_body req;
> +
> +	if (power_up)
> +		req.req_type = DP_POWER_UP_PHY;
> +	else
> +		req.req_type = DP_POWER_DOWN_PHY;
> +
> +	req.u.port_num.port_number = port_num;
> +	drm_dp_encode_sideband_req(&req, msg);
> +	msg->path_msg = true;
> +	return 0;
> +}
> +
>  static int drm_dp_mst_assign_payload_id(struct
> drm_dp_mst_topology_mgr *mgr,
>  					struct drm_dp_vcpi *vcpi)
>  {
> @@ -1724,6 +1765,38 @@ static int drm_dp_payload_send_msg(struct
> drm_dp_mst_topology_mgr *mgr,
>  	return ret;
>  }
>  
> +int drm_dp_send_power_updown_phy(struct drm_dp_mst_topology_mgr
> *mgr,
> +				 struct drm_dp_mst_port *port, bool
> power_up)
> +{
> +	struct drm_dp_sideband_msg_tx *txmsg;
> +	int len, ret;
> +
> +	txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL);
> +	if (!txmsg)
> +		return -ENOMEM;
> +
> +	port = drm_dp_get_validated_port_ref(mgr, port);
> +	if (!port)
> +		return -EINVAL;
if (!port), then you leak memory by not freeing txmsg
> +
> +	txmsg->dst = port->parent;
> +	len = build_power_updown_phy(txmsg, port->port_num,
> power_up);
> +	drm_dp_queue_down_tx(mgr, txmsg);
> +
> +	ret = drm_dp_mst_wait_tx_reply(port->parent, txmsg);
> +	if (ret > 0) {
> +		if (txmsg->reply.reply_type == 1)
> +			ret = -EINVAL;
> +		else
> +			ret = 0;
> +	}
> +	kfree(txmsg);
> +	drm_dp_put_port(port);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_dp_send_power_updown_phy);
> +
>  static int drm_dp_create_payload_step1(struct
> drm_dp_mst_topology_mgr *mgr,
>  				       int id,
>  				       struct drm_dp_payload
> *payload)
> diff --git a/include/drm/drm_dp_mst_helper.h
> b/include/drm/drm_dp_mst_helper.h
> index d55abb75f29a..7f78d26a0766 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -631,5 +631,7 @@ int drm_dp_atomic_find_vcpi_slots(struct
> drm_atomic_state *state,
>  int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
>  				     struct drm_dp_mst_topology_mgr
> *mgr,
>  				     int slots);
> +int drm_dp_send_power_updown_phy(struct drm_dp_mst_topology_mgr
> *mgr,
> +				 struct drm_dp_mst_port *port, bool
> power_up);
>  
>  #endif
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/2] drm/dp/mst: Sideband message transaction to power up/down nodes
  2017-09-06 15:40 ` [PATCH 1/2] " Lyude Paul
@ 2017-09-06 16:36   ` Pandiyan, Dhinakaran
  2017-09-07  0:14   ` [PATCH v2] " Dhinakaran Pandiyan
  1 sibling, 0 replies; 23+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-09-06 16:36 UTC (permalink / raw)
  To: lyude; +Cc: intel-gfx, harry.wentland, dri-devel

On Wed, 2017-09-06 at 11:40 -0400, Lyude Paul wrote:
> On Tue, 2017-09-05 at 18:26 -0700, Dhinakaran Pandiyan wrote:
> > The POWER_DOWN_PHY and POWER_UP_PHY sideband message transactions
> > allow
> > the source to reqest any node in a mst path or a whole path to be
> > powered down or up. This allows drivers to target a specific sink in
> > the
> > MST topology, an improvement over just power managing the imediate
> > downstream device. Secondly, since the request-reply protocol waits
> > for an
> > ACK, we can be sure that a downstream sink has enough time to respond
> > to a
> > power up/down request.
> > 
> > Cc: Lyude <lyude@redhat.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Harry Wentland <harry.wentland@amd.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 73
> > +++++++++++++++++++++++++++++++++++
> >  include/drm/drm_dp_mst_helper.h       |  2 +
> >  2 files changed, 75 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 41b492f99955..a9f12708a046 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -294,6 +294,12 @@ static void drm_dp_encode_sideband_req(struct
> > drm_dp_sideband_msg_req_body *req,
> >  		memcpy(&buf[idx], req->u.i2c_write.bytes, req-
> > >u.i2c_write.num_bytes);
> >  		idx += req->u.i2c_write.num_bytes;
> >  		break;
> > +
> > +	case DP_POWER_DOWN_PHY:
> > +	case DP_POWER_UP_PHY:
> > +		buf[idx] = (req->u.port_num.port_number & 0xf) << 4;
> > +		idx++;
> > +		break;
> >  	}
> >  	raw->cur_len = idx;
> >  }
> > @@ -538,6 +544,22 @@ static bool
> > drm_dp_sideband_parse_query_payload_ack(struct drm_dp_sideband_msg_r
> >  	return false;
> >  }
> >  
> > +
> > +static bool drm_dp_sideband_parse_power_updown_phy_ack(struct
> > drm_dp_sideband_msg_rx *raw,
> > +						       struct
> > drm_dp_sideband_msg_reply_body *repmsg)
> > +{
> > +	int idx = 1;
> > +
> > +	repmsg->u.port_number.port_number = (raw->msg[idx] >> 4) &
> > 0xf;
> > +	idx++;
> > +	if (idx > raw->curlen) {
> > +		DRM_DEBUG_KMS("power up/down phy parse length fail
> > %d %d\n",
> > +			      idx, raw->curlen);
> > +		return false;
> > +	}
> > +	return true;
> > +}
> > +
> >  static bool drm_dp_sideband_parse_reply(struct
> > drm_dp_sideband_msg_rx *raw,
> >  					struct
> > drm_dp_sideband_msg_reply_body *msg)
> >  {
> > @@ -567,6 +589,9 @@ static bool drm_dp_sideband_parse_reply(struct
> > drm_dp_sideband_msg_rx *raw,
> >  		return
> > drm_dp_sideband_parse_enum_path_resources_ack(raw, msg);
> >  	case DP_ALLOCATE_PAYLOAD:
> >  		return
> > drm_dp_sideband_parse_allocate_payload_ack(raw, msg);
> > +	case DP_POWER_DOWN_PHY:
> > +	case DP_POWER_UP_PHY:
> > +		return
> > drm_dp_sideband_parse_power_updown_phy_ack(raw, msg);
> >  	default:
> >  		DRM_ERROR("Got unknown reply 0x%02x\n", msg-
> > >req_type);
> >  		return false;
> > @@ -693,6 +718,22 @@ static int build_allocate_payload(struct
> > drm_dp_sideband_msg_tx *msg, int port_n
> >  	return 0;
> >  }
> >  
> > +static int build_power_updown_phy(struct drm_dp_sideband_msg_tx
> > *msg,
> > +				  int port_num, bool power_up)
> > +{
> > +	struct drm_dp_sideband_msg_req_body req;
> > +
> > +	if (power_up)
> > +		req.req_type = DP_POWER_UP_PHY;
> > +	else
> > +		req.req_type = DP_POWER_DOWN_PHY;
> > +
> > +	req.u.port_num.port_number = port_num;
> > +	drm_dp_encode_sideband_req(&req, msg);
> > +	msg->path_msg = true;
> > +	return 0;
> > +}
> > +
> >  static int drm_dp_mst_assign_payload_id(struct
> > drm_dp_mst_topology_mgr *mgr,
> >  					struct drm_dp_vcpi *vcpi)
> >  {
> > @@ -1724,6 +1765,38 @@ static int drm_dp_payload_send_msg(struct
> > drm_dp_mst_topology_mgr *mgr,
> >  	return ret;
> >  }
> >  
> > +int drm_dp_send_power_updown_phy(struct drm_dp_mst_topology_mgr
> > *mgr,
> > +				 struct drm_dp_mst_port *port, bool
> > power_up)
> > +{
> > +	struct drm_dp_sideband_msg_tx *txmsg;
> > +	int len, ret;
> > +
> > +	txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL);
> > +	if (!txmsg)
> > +		return -ENOMEM;
> > +
> > +	port = drm_dp_get_validated_port_ref(mgr, port);
> > +	if (!port)
> > +		return -EINVAL;
> if (!port), then you leak memory by not freeing txmsg

Right, I'll fix that up in the next version. Thanks!

> > +
> > +	txmsg->dst = port->parent;
> > +	len = build_power_updown_phy(txmsg, port->port_num,
> > power_up);
> > +	drm_dp_queue_down_tx(mgr, txmsg);
> > +
> > +	ret = drm_dp_mst_wait_tx_reply(port->parent, txmsg);
> > +	if (ret > 0) {
> > +		if (txmsg->reply.reply_type == 1)
> > +			ret = -EINVAL;
> > +		else
> > +			ret = 0;
> > +	}
> > +	kfree(txmsg);
> > +	drm_dp_put_port(port);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_dp_send_power_updown_phy);
> > +
> >  static int drm_dp_create_payload_step1(struct
> > drm_dp_mst_topology_mgr *mgr,
> >  				       int id,
> >  				       struct drm_dp_payload
> > *payload)
> > diff --git a/include/drm/drm_dp_mst_helper.h
> > b/include/drm/drm_dp_mst_helper.h
> > index d55abb75f29a..7f78d26a0766 100644
> > --- a/include/drm/drm_dp_mst_helper.h
> > +++ b/include/drm/drm_dp_mst_helper.h
> > @@ -631,5 +631,7 @@ int drm_dp_atomic_find_vcpi_slots(struct
> > drm_atomic_state *state,
> >  int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
> >  				     struct drm_dp_mst_topology_mgr
> > *mgr,
> >  				     int slots);
> > +int drm_dp_send_power_updown_phy(struct drm_dp_mst_topology_mgr
> > *mgr,
> > +				 struct drm_dp_mst_port *port, bool
> > power_up);
> >  
> >  #endif
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v2] drm/dp/mst: Sideband message transaction to power up/down nodes
  2017-09-06 15:40 ` [PATCH 1/2] " Lyude Paul
  2017-09-06 16:36   ` Pandiyan, Dhinakaran
@ 2017-09-07  0:14   ` Dhinakaran Pandiyan
  2017-09-07 18:04     ` Lyude Paul
  2017-09-11 13:33     ` Ville Syrjälä
  1 sibling, 2 replies; 23+ messages in thread
From: Dhinakaran Pandiyan @ 2017-09-07  0:14 UTC (permalink / raw)
  To: intel-gfx; +Cc: Dhinakaran Pandiyan, dri-devel

The POWER_DOWN_PHY and POWER_UP_PHY sideband message transactions allow
the source to reqest any node in a mst path or a whole path to be
powered down or up. This allows drivers to target a specific sink in the
MST topology, an improvement over just power managing the imediate
downstream device. Secondly, since the request-reply protocol waits for an
ACK, we can be sure that a downstream sink has enough time to respond to a
power up/down request.

v2: Fix memory leak (Lyude)
Cc: Lyude <lyude@redhat.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 75 +++++++++++++++++++++++++++++++++++
 include/drm/drm_dp_mst_helper.h       |  2 +
 2 files changed, 77 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 41b492f99955..9bc5049e7e59 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -294,6 +294,12 @@ static void drm_dp_encode_sideband_req(struct drm_dp_sideband_msg_req_body *req,
 		memcpy(&buf[idx], req->u.i2c_write.bytes, req->u.i2c_write.num_bytes);
 		idx += req->u.i2c_write.num_bytes;
 		break;
+
+	case DP_POWER_DOWN_PHY:
+	case DP_POWER_UP_PHY:
+		buf[idx] = (req->u.port_num.port_number & 0xf) << 4;
+		idx++;
+		break;
 	}
 	raw->cur_len = idx;
 }
@@ -538,6 +544,22 @@ static bool drm_dp_sideband_parse_query_payload_ack(struct drm_dp_sideband_msg_r
 	return false;
 }
 
+
+static bool drm_dp_sideband_parse_power_updown_phy_ack(struct drm_dp_sideband_msg_rx *raw,
+						       struct drm_dp_sideband_msg_reply_body *repmsg)
+{
+	int idx = 1;
+
+	repmsg->u.port_number.port_number = (raw->msg[idx] >> 4) & 0xf;
+	idx++;
+	if (idx > raw->curlen) {
+		DRM_DEBUG_KMS("power up/down phy parse length fail %d %d\n",
+			      idx, raw->curlen);
+		return false;
+	}
+	return true;
+}
+
 static bool drm_dp_sideband_parse_reply(struct drm_dp_sideband_msg_rx *raw,
 					struct drm_dp_sideband_msg_reply_body *msg)
 {
@@ -567,6 +589,9 @@ static bool drm_dp_sideband_parse_reply(struct drm_dp_sideband_msg_rx *raw,
 		return drm_dp_sideband_parse_enum_path_resources_ack(raw, msg);
 	case DP_ALLOCATE_PAYLOAD:
 		return drm_dp_sideband_parse_allocate_payload_ack(raw, msg);
+	case DP_POWER_DOWN_PHY:
+	case DP_POWER_UP_PHY:
+		return drm_dp_sideband_parse_power_updown_phy_ack(raw, msg);
 	default:
 		DRM_ERROR("Got unknown reply 0x%02x\n", msg->req_type);
 		return false;
@@ -693,6 +718,22 @@ static int build_allocate_payload(struct drm_dp_sideband_msg_tx *msg, int port_n
 	return 0;
 }
 
+static int build_power_updown_phy(struct drm_dp_sideband_msg_tx *msg,
+				  int port_num, bool power_up)
+{
+	struct drm_dp_sideband_msg_req_body req;
+
+	if (power_up)
+		req.req_type = DP_POWER_UP_PHY;
+	else
+		req.req_type = DP_POWER_DOWN_PHY;
+
+	req.u.port_num.port_number = port_num;
+	drm_dp_encode_sideband_req(&req, msg);
+	msg->path_msg = true;
+	return 0;
+}
+
 static int drm_dp_mst_assign_payload_id(struct drm_dp_mst_topology_mgr *mgr,
 					struct drm_dp_vcpi *vcpi)
 {
@@ -1724,6 +1765,40 @@ static int drm_dp_payload_send_msg(struct drm_dp_mst_topology_mgr *mgr,
 	return ret;
 }
 
+int drm_dp_send_power_updown_phy(struct drm_dp_mst_topology_mgr *mgr,
+				 struct drm_dp_mst_port *port, bool power_up)
+{
+	struct drm_dp_sideband_msg_tx *txmsg;
+	int len, ret;
+
+	port = drm_dp_get_validated_port_ref(mgr, port);
+	if (!port)
+		return -EINVAL;
+
+	txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL);
+	if (!txmsg) {
+		drm_dp_put_port(port);
+		return -ENOMEM;
+	}
+
+	txmsg->dst = port->parent;
+	len = build_power_updown_phy(txmsg, port->port_num, power_up);
+	drm_dp_queue_down_tx(mgr, txmsg);
+
+	ret = drm_dp_mst_wait_tx_reply(port->parent, txmsg);
+	if (ret > 0) {
+		if (txmsg->reply.reply_type == 1)
+			ret = -EINVAL;
+		else
+			ret = 0;
+	}
+	kfree(txmsg);
+	drm_dp_put_port(port);
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_dp_send_power_updown_phy);
+
 static int drm_dp_create_payload_step1(struct drm_dp_mst_topology_mgr *mgr,
 				       int id,
 				       struct drm_dp_payload *payload)
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index d55abb75f29a..7f78d26a0766 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -631,5 +631,7 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,
 int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
 				     struct drm_dp_mst_topology_mgr *mgr,
 				     int slots);
+int drm_dp_send_power_updown_phy(struct drm_dp_mst_topology_mgr *mgr,
+				 struct drm_dp_mst_port *port, bool power_up);
 
 #endif
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* ✗ Fi.CI.BAT: failure for series starting with [v2] drm/dp/mst: Sideband message transaction to power up/down nodes (rev2)
  2017-09-06  1:26 [PATCH 1/2] drm/dp/mst: Sideband message transaction to power up/down nodes Dhinakaran Pandiyan
                   ` (2 preceding siblings ...)
  2017-09-06 15:40 ` [PATCH 1/2] " Lyude Paul
@ 2017-09-07  0:35 ` Patchwork
  2017-09-07  5:49 ` [PATCH 1/2] drm/dp/mst: Sideband message transaction to power up/down nodes Stefan Assmann
  4 siblings, 0 replies; 23+ messages in thread
From: Patchwork @ 2017-09-07  0:35 UTC (permalink / raw)
  To: Dhinakaran Pandiyan; +Cc: intel-gfx

== Series Details ==

Series: series starting with [v2] drm/dp/mst: Sideband message transaction to power up/down nodes (rev2)
URL   : https://patchwork.freedesktop.org/series/29853/
State : failure

== Summary ==

Series 29853v2 series starting with [v2] drm/dp/mst: Sideband message transaction to power up/down nodes
https://patchwork.freedesktop.org/api/1.0/series/29853/revisions/2/mbox/

Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-legacy:
                fail       -> PASS       (fi-snb-2600) fdo#100215
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> SKIP       (fi-cfl-s) fdo#102294
Test drv_module_reload:
        Subgroup basic-reload-inject:
                pass       -> INCOMPLETE (fi-bwr-2160)

fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215
fdo#102294 https://bugs.freedesktop.org/show_bug.cgi?id=102294

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:456s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:439s
fi-blb-e6850     total:289  pass:224  dwarn:1   dfail:0   fail:0   skip:64  time:365s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:554s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:104
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:520s
fi-byt-j1900     total:289  pass:254  dwarn:1   dfail:0   fail:0   skip:34  time:524s
fi-byt-n2820     total:289  pass:250  dwarn:1   dfail:0   fail:0   skip:38  time:512s
fi-cfl-s         total:289  pass:249  dwarn:4   dfail:0   fail:0   skip:36  time:475s
fi-elk-e7500     total:289  pass:230  dwarn:0   dfail:0   fail:0   skip:59  time:431s
fi-glk-2a        total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:610s
fi-hsw-4770      total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:445s
fi-hsw-4770r     total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:426s
fi-ilk-650       total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:425s
fi-ivb-3520m     total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:506s
fi-ivb-3770      total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:476s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:516s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:608s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:597s
fi-pnv-d510      total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:530s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:476s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:533s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:512s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:450s
fi-skl-x1585l    total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:489s
fi-snb-2520m     total:289  pass:251  dwarn:0   dfail:0   fail:0   skip:38  time:558s
fi-snb-2600      total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:410s

d987a6e6aed64b97877546c110149c16bca64804 drm-tip: 2017y-09m-06d-23h-36m-35s UTC integration manifest
86fb8d256f2b drm/i915/mst: Use MST sideband message transaction for dpms
f7f3476b4cfd drm/dp/mst: Sideband message transaction to power up/down nodes

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5601/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/2] drm/dp/mst: Sideband message transaction to power up/down nodes
  2017-09-06  1:26 [PATCH 1/2] drm/dp/mst: Sideband message transaction to power up/down nodes Dhinakaran Pandiyan
                   ` (3 preceding siblings ...)
  2017-09-07  0:35 ` ✗ Fi.CI.BAT: failure for series starting with [v2] drm/dp/mst: Sideband message transaction to power up/down nodes (rev2) Patchwork
@ 2017-09-07  5:49 ` Stefan Assmann
  4 siblings, 0 replies; 23+ messages in thread
From: Stefan Assmann @ 2017-09-07  5:49 UTC (permalink / raw)
  To: Dhinakaran Pandiyan; +Cc: intel-gfx, dri-devel

On 2017-09-05 18:26, Dhinakaran Pandiyan wrote:
> The POWER_DOWN_PHY and POWER_UP_PHY sideband message transactions allow
> the source to reqest any node in a mst path or a whole path to be
> powered down or up. This allows drivers to target a specific sink in the
> MST topology, an improvement over just power managing the imediate
> downstream device. Secondly, since the request-reply protocol waits for an
> ACK, we can be sure that a downstream sink has enough time to respond to a
> power up/down request.
> 
> Cc: Lyude <lyude@redhat.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

Tested-by: Stefan Assmann <sassmann@redhat.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/2] drm/i915/mst: Use MST sideband message transaction for dpms
  2017-09-06  1:26 ` [PATCH 2/2] drm/i915/mst: Use MST sideband message transaction for dpms Dhinakaran Pandiyan
@ 2017-09-07  5:51   ` Stefan Assmann
  2017-09-12 16:11   ` Ville Syrjälä
  1 sibling, 0 replies; 23+ messages in thread
From: Stefan Assmann @ 2017-09-07  5:51 UTC (permalink / raw)
  To: Dhinakaran Pandiyan; +Cc: intel-gfx, dri-devel

On 2017-09-05 18:26, Dhinakaran Pandiyan wrote:
> Use the POWER_DOWN_PHY and POWER_UP_PHY sideband message trasactions to
> set power states for downstream sinks. Apart from giving us the ability
> to set power state for individual sinks, this fixes the below test for
> me
> 
> $ xrandr --display :0 --output DP-2-2-8 --off
> $ xrandr --display :0 --output DP-2-2-1 --off
> $ xrandr --display :0 --output DP-2-2-8 --auto #Black screen
> $ xrandr --display :0 --output DP-2-2-1 --auto
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Lyude <lyude@redhat.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

Tested-by: Stefan Assmann <sassmann@redhat.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2] drm/dp/mst: Sideband message transaction to power up/down nodes
  2017-09-07  0:14   ` [PATCH v2] " Dhinakaran Pandiyan
@ 2017-09-07 18:04     ` Lyude Paul
  2017-09-07 18:54       ` [Intel-gfx] " Pandiyan, Dhinakaran
  2017-09-11 13:33     ` Ville Syrjälä
  1 sibling, 1 reply; 23+ messages in thread
From: Lyude Paul @ 2017-09-07 18:04 UTC (permalink / raw)
  To: Dhinakaran Pandiyan, intel-gfx; +Cc: Harry Wentland, dri-devel

Looks good to me.

Reviewed-by: Lyude Paul <lyude@redhat.com>

On Wed, 2017-09-06 at 17:14 -0700, Dhinakaran Pandiyan wrote:
> The POWER_DOWN_PHY and POWER_UP_PHY sideband message transactions
> allow
> the source to reqest any node in a mst path or a whole path to be
> powered down or up. This allows drivers to target a specific sink in
> the
> MST topology, an improvement over just power managing the imediate
> downstream device. Secondly, since the request-reply protocol waits
> for an
> ACK, we can be sure that a downstream sink has enough time to respond
> to a
> power up/down request.
> 
> v2: Fix memory leak (Lyude)
> Cc: Lyude <lyude@redhat.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 75
> +++++++++++++++++++++++++++++++++++
>  include/drm/drm_dp_mst_helper.h       |  2 +
>  2 files changed, 77 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 41b492f99955..9bc5049e7e59 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -294,6 +294,12 @@ static void drm_dp_encode_sideband_req(struct
> drm_dp_sideband_msg_req_body *req,
>  		memcpy(&buf[idx], req->u.i2c_write.bytes, req-
> >u.i2c_write.num_bytes);
>  		idx += req->u.i2c_write.num_bytes;
>  		break;
> +
> +	case DP_POWER_DOWN_PHY:
> +	case DP_POWER_UP_PHY:
> +		buf[idx] = (req->u.port_num.port_number & 0xf) << 4;
> +		idx++;
> +		break;
>  	}
>  	raw->cur_len = idx;
>  }
> @@ -538,6 +544,22 @@ static bool
> drm_dp_sideband_parse_query_payload_ack(struct drm_dp_sideband_msg_r
>  	return false;
>  }
>  
> +
> +static bool drm_dp_sideband_parse_power_updown_phy_ack(struct
> drm_dp_sideband_msg_rx *raw,
> +						       struct
> drm_dp_sideband_msg_reply_body *repmsg)
> +{
> +	int idx = 1;
> +
> +	repmsg->u.port_number.port_number = (raw->msg[idx] >> 4) &
> 0xf;
> +	idx++;
> +	if (idx > raw->curlen) {
> +		DRM_DEBUG_KMS("power up/down phy parse length fail
> %d %d\n",
> +			      idx, raw->curlen);
> +		return false;
> +	}
> +	return true;
> +}
> +
>  static bool drm_dp_sideband_parse_reply(struct
> drm_dp_sideband_msg_rx *raw,
>  					struct
> drm_dp_sideband_msg_reply_body *msg)
>  {
> @@ -567,6 +589,9 @@ static bool drm_dp_sideband_parse_reply(struct
> drm_dp_sideband_msg_rx *raw,
>  		return
> drm_dp_sideband_parse_enum_path_resources_ack(raw, msg);
>  	case DP_ALLOCATE_PAYLOAD:
>  		return
> drm_dp_sideband_parse_allocate_payload_ack(raw, msg);
> +	case DP_POWER_DOWN_PHY:
> +	case DP_POWER_UP_PHY:
> +		return
> drm_dp_sideband_parse_power_updown_phy_ack(raw, msg);
>  	default:
>  		DRM_ERROR("Got unknown reply 0x%02x\n", msg-
> >req_type);
>  		return false;
> @@ -693,6 +718,22 @@ static int build_allocate_payload(struct
> drm_dp_sideband_msg_tx *msg, int port_n
>  	return 0;
>  }
>  
> +static int build_power_updown_phy(struct drm_dp_sideband_msg_tx
> *msg,
> +				  int port_num, bool power_up)
> +{
> +	struct drm_dp_sideband_msg_req_body req;
> +
> +	if (power_up)
> +		req.req_type = DP_POWER_UP_PHY;
> +	else
> +		req.req_type = DP_POWER_DOWN_PHY;
> +
> +	req.u.port_num.port_number = port_num;
> +	drm_dp_encode_sideband_req(&req, msg);
> +	msg->path_msg = true;
> +	return 0;
> +}
> +
>  static int drm_dp_mst_assign_payload_id(struct
> drm_dp_mst_topology_mgr *mgr,
>  					struct drm_dp_vcpi *vcpi)
>  {
> @@ -1724,6 +1765,40 @@ static int drm_dp_payload_send_msg(struct
> drm_dp_mst_topology_mgr *mgr,
>  	return ret;
>  }
>  
> +int drm_dp_send_power_updown_phy(struct drm_dp_mst_topology_mgr
> *mgr,
> +				 struct drm_dp_mst_port *port, bool
> power_up)
> +{
> +	struct drm_dp_sideband_msg_tx *txmsg;
> +	int len, ret;
> +
> +	port = drm_dp_get_validated_port_ref(mgr, port);
> +	if (!port)
> +		return -EINVAL;
> +
> +	txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL);
> +	if (!txmsg) {
> +		drm_dp_put_port(port);
> +		return -ENOMEM;
> +	}
> +
> +	txmsg->dst = port->parent;
> +	len = build_power_updown_phy(txmsg, port->port_num,
> power_up);
> +	drm_dp_queue_down_tx(mgr, txmsg);
> +
> +	ret = drm_dp_mst_wait_tx_reply(port->parent, txmsg);
> +	if (ret > 0) {
> +		if (txmsg->reply.reply_type == 1)
> +			ret = -EINVAL;
> +		else
> +			ret = 0;
> +	}
> +	kfree(txmsg);
> +	drm_dp_put_port(port);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_dp_send_power_updown_phy);
> +
>  static int drm_dp_create_payload_step1(struct
> drm_dp_mst_topology_mgr *mgr,
>  				       int id,
>  				       struct drm_dp_payload
> *payload)
> diff --git a/include/drm/drm_dp_mst_helper.h
> b/include/drm/drm_dp_mst_helper.h
> index d55abb75f29a..7f78d26a0766 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -631,5 +631,7 @@ int drm_dp_atomic_find_vcpi_slots(struct
> drm_atomic_state *state,
>  int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
>  				     struct drm_dp_mst_topology_mgr
> *mgr,
>  				     int slots);
> +int drm_dp_send_power_updown_phy(struct drm_dp_mst_topology_mgr
> *mgr,
> +				 struct drm_dp_mst_port *port, bool
> power_up);
>  
>  #endif
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Intel-gfx] [PATCH v2] drm/dp/mst: Sideband message transaction to power up/down nodes
  2017-09-07 18:04     ` Lyude Paul
@ 2017-09-07 18:54       ` Pandiyan, Dhinakaran
  0 siblings, 0 replies; 23+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-09-07 18:54 UTC (permalink / raw)
  To: lyude; +Cc: intel-gfx, dri-devel




On Thu, 2017-09-07 at 14:04 -0400, Lyude Paul wrote:
> Looks good to me.
> 
> Reviewed-by: Lyude Paul <lyude@redhat.com>
> 


Thanks for the review.
-DK


> On Wed, 2017-09-06 at 17:14 -0700, Dhinakaran Pandiyan wrote:
> > The POWER_DOWN_PHY and POWER_UP_PHY sideband message transactions
> > allow
> > the source to reqest any node in a mst path or a whole path to be
> > powered down or up. This allows drivers to target a specific sink in
> > the
> > MST topology, an improvement over just power managing the imediate
> > downstream device. Secondly, since the request-reply protocol waits
> > for an
> > ACK, we can be sure that a downstream sink has enough time to respond
> > to a
> > power up/down request.
> > 
> > v2: Fix memory leak (Lyude)
> > Cc: Lyude <lyude@redhat.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Harry Wentland <harry.wentland@amd.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 75
> > +++++++++++++++++++++++++++++++++++
> >  include/drm/drm_dp_mst_helper.h       |  2 +
> >  2 files changed, 77 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 41b492f99955..9bc5049e7e59 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -294,6 +294,12 @@ static void drm_dp_encode_sideband_req(struct
> > drm_dp_sideband_msg_req_body *req,
> >  		memcpy(&buf[idx], req->u.i2c_write.bytes, req-
> > >u.i2c_write.num_bytes);
> >  		idx += req->u.i2c_write.num_bytes;
> >  		break;
> > +
> > +	case DP_POWER_DOWN_PHY:
> > +	case DP_POWER_UP_PHY:
> > +		buf[idx] = (req->u.port_num.port_number & 0xf) << 4;
> > +		idx++;
> > +		break;
> >  	}
> >  	raw->cur_len = idx;
> >  }
> > @@ -538,6 +544,22 @@ static bool
> > drm_dp_sideband_parse_query_payload_ack(struct drm_dp_sideband_msg_r
> >  	return false;
> >  }
> >  
> > +
> > +static bool drm_dp_sideband_parse_power_updown_phy_ack(struct
> > drm_dp_sideband_msg_rx *raw,
> > +						       struct
> > drm_dp_sideband_msg_reply_body *repmsg)
> > +{
> > +	int idx = 1;
> > +
> > +	repmsg->u.port_number.port_number = (raw->msg[idx] >> 4) &
> > 0xf;
> > +	idx++;
> > +	if (idx > raw->curlen) {
> > +		DRM_DEBUG_KMS("power up/down phy parse length fail
> > %d %d\n",
> > +			      idx, raw->curlen);
> > +		return false;
> > +	}
> > +	return true;
> > +}
> > +
> >  static bool drm_dp_sideband_parse_reply(struct
> > drm_dp_sideband_msg_rx *raw,
> >  					struct
> > drm_dp_sideband_msg_reply_body *msg)
> >  {
> > @@ -567,6 +589,9 @@ static bool drm_dp_sideband_parse_reply(struct
> > drm_dp_sideband_msg_rx *raw,
> >  		return
> > drm_dp_sideband_parse_enum_path_resources_ack(raw, msg);
> >  	case DP_ALLOCATE_PAYLOAD:
> >  		return
> > drm_dp_sideband_parse_allocate_payload_ack(raw, msg);
> > +	case DP_POWER_DOWN_PHY:
> > +	case DP_POWER_UP_PHY:
> > +		return
> > drm_dp_sideband_parse_power_updown_phy_ack(raw, msg);
> >  	default:
> >  		DRM_ERROR("Got unknown reply 0x%02x\n", msg-
> > >req_type);
> >  		return false;
> > @@ -693,6 +718,22 @@ static int build_allocate_payload(struct
> > drm_dp_sideband_msg_tx *msg, int port_n
> >  	return 0;
> >  }
> >  
> > +static int build_power_updown_phy(struct drm_dp_sideband_msg_tx
> > *msg,
> > +				  int port_num, bool power_up)
> > +{
> > +	struct drm_dp_sideband_msg_req_body req;
> > +
> > +	if (power_up)
> > +		req.req_type = DP_POWER_UP_PHY;
> > +	else
> > +		req.req_type = DP_POWER_DOWN_PHY;
> > +
> > +	req.u.port_num.port_number = port_num;
> > +	drm_dp_encode_sideband_req(&req, msg);
> > +	msg->path_msg = true;
> > +	return 0;
> > +}
> > +
> >  static int drm_dp_mst_assign_payload_id(struct
> > drm_dp_mst_topology_mgr *mgr,
> >  					struct drm_dp_vcpi *vcpi)
> >  {
> > @@ -1724,6 +1765,40 @@ static int drm_dp_payload_send_msg(struct
> > drm_dp_mst_topology_mgr *mgr,
> >  	return ret;
> >  }
> >  
> > +int drm_dp_send_power_updown_phy(struct drm_dp_mst_topology_mgr
> > *mgr,
> > +				 struct drm_dp_mst_port *port, bool
> > power_up)
> > +{
> > +	struct drm_dp_sideband_msg_tx *txmsg;
> > +	int len, ret;
> > +
> > +	port = drm_dp_get_validated_port_ref(mgr, port);
> > +	if (!port)
> > +		return -EINVAL;
> > +
> > +	txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL);
> > +	if (!txmsg) {
> > +		drm_dp_put_port(port);
> > +		return -ENOMEM;
> > +	}
> > +
> > +	txmsg->dst = port->parent;
> > +	len = build_power_updown_phy(txmsg, port->port_num,
> > power_up);
> > +	drm_dp_queue_down_tx(mgr, txmsg);
> > +
> > +	ret = drm_dp_mst_wait_tx_reply(port->parent, txmsg);
> > +	if (ret > 0) {
> > +		if (txmsg->reply.reply_type == 1)
> > +			ret = -EINVAL;
> > +		else
> > +			ret = 0;
> > +	}
> > +	kfree(txmsg);
> > +	drm_dp_put_port(port);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL(drm_dp_send_power_updown_phy);
> > +
> >  static int drm_dp_create_payload_step1(struct
> > drm_dp_mst_topology_mgr *mgr,
> >  				       int id,
> >  				       struct drm_dp_payload
> > *payload)
> > diff --git a/include/drm/drm_dp_mst_helper.h
> > b/include/drm/drm_dp_mst_helper.h
> > index d55abb75f29a..7f78d26a0766 100644
> > --- a/include/drm/drm_dp_mst_helper.h
> > +++ b/include/drm/drm_dp_mst_helper.h
> > @@ -631,5 +631,7 @@ int drm_dp_atomic_find_vcpi_slots(struct
> > drm_atomic_state *state,
> >  int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
> >  				     struct drm_dp_mst_topology_mgr
> > *mgr,
> >  				     int slots);
> > +int drm_dp_send_power_updown_phy(struct drm_dp_mst_topology_mgr
> > *mgr,
> > +				 struct drm_dp_mst_port *port, bool
> > power_up);
> >  
> >  #endif
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2] drm/dp/mst: Sideband message transaction to power up/down nodes
  2017-09-07  0:14   ` [PATCH v2] " Dhinakaran Pandiyan
  2017-09-07 18:04     ` Lyude Paul
@ 2017-09-11 13:33     ` Ville Syrjälä
  2017-09-11 19:10       ` Pandiyan, Dhinakaran
  1 sibling, 1 reply; 23+ messages in thread
From: Ville Syrjälä @ 2017-09-11 13:33 UTC (permalink / raw)
  To: Dhinakaran Pandiyan; +Cc: Harry Wentland, intel-gfx, dri-devel

On Wed, Sep 06, 2017 at 05:14:58PM -0700, Dhinakaran Pandiyan wrote:
> The POWER_DOWN_PHY and POWER_UP_PHY sideband message transactions allow
> the source to reqest any node in a mst path or a whole path to be
> powered down or up. This allows drivers to target a specific sink in the
> MST topology, an improvement over just power managing the imediate
> downstream device. Secondly, since the request-reply protocol waits for an
> ACK, we can be sure that a downstream sink has enough time to respond to a
> power up/down request.

I was a bit worried how this handles multiple streams going through
the same MST device, but looks like the spec has accounted for this
by having the device skip the D3 if any active streams are present.

I guess that also means we have to do this after we've turned off the
stream, assuming we want things actually reach D3. Looks like that does
match our current order of things.

Pushed this one to drm-misc-next. Thanks for the patch and review.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2] drm/dp/mst: Sideband message transaction to power up/down nodes
  2017-09-11 13:33     ` Ville Syrjälä
@ 2017-09-11 19:10       ` Pandiyan, Dhinakaran
  0 siblings, 0 replies; 23+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-09-11 19:10 UTC (permalink / raw)
  To: ville.syrjala; +Cc: intel-gfx, harry.wentland, cpaul, dri-devel

On Mon, 2017-09-11 at 16:33 +0300, Ville Syrjälä wrote:
> On Wed, Sep 06, 2017 at 05:14:58PM -0700, Dhinakaran Pandiyan wrote:
> > The POWER_DOWN_PHY and POWER_UP_PHY sideband message transactions allow
> > the source to reqest any node in a mst path or a whole path to be
> > powered down or up. This allows drivers to target a specific sink in the
> > MST topology, an improvement over just power managing the imediate
> > downstream device. Secondly, since the request-reply protocol waits for an
> > ACK, we can be sure that a downstream sink has enough time to respond to a
> > power up/down request.
> 
> I was a bit worried how this handles multiple streams going through
> the same MST device, but looks like the spec has accounted for this
> by having the device skip the D3 if any active streams are present.
> 
> I guess that also means we have to do this after we've turned off the
> stream, assuming we want things actually reach D3. Looks like that does
> match our current order of things.
> 
> Pushed this one to drm-misc-next. Thanks for the patch and review.
> 
Thanks, do you see any potential problems patch with Patch 2/2?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/2] drm/i915/mst: Use MST sideband message transaction for dpms
  2017-09-06  1:26 ` [PATCH 2/2] drm/i915/mst: Use MST sideband message transaction for dpms Dhinakaran Pandiyan
  2017-09-07  5:51   ` Stefan Assmann
@ 2017-09-12 16:11   ` Ville Syrjälä
  2017-09-12 16:17     ` Ville Syrjälä
  1 sibling, 1 reply; 23+ messages in thread
From: Ville Syrjälä @ 2017-09-12 16:11 UTC (permalink / raw)
  To: Dhinakaran Pandiyan; +Cc: intel-gfx, dri-devel

On Tue, Sep 05, 2017 at 06:26:34PM -0700, Dhinakaran Pandiyan wrote:
> Use the POWER_DOWN_PHY and POWER_UP_PHY sideband message trasactions to
> set power states for downstream sinks. Apart from giving us the ability
> to set power state for individual sinks, this fixes the below test for
> me
> 
> $ xrandr --display :0 --output DP-2-2-8 --off
> $ xrandr --display :0 --output DP-2-2-1 --off
> $ xrandr --display :0 --output DP-2-2-8 --auto #Black screen
> $ xrandr --display :0 --output DP-2-2-1 --auto
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Lyude <lyude@redhat.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c    | 6 ++++--
>  drivers/gpu/drm/i915/intel_dp_mst.c | 8 ++++----
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 1da3bb2cc4b4..8aebacc0aa31 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2161,7 +2161,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
>  		intel_prepare_dp_ddi_buffers(encoder);
>  
>  	intel_ddi_init_dp_buf_reg(encoder);
> -	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> +	if (!link_mst)
> +		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
>  	intel_dp_start_link_train(intel_dp);
>  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
>  		intel_dp_stop_link_train(intel_dp);
> @@ -2240,7 +2241,8 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder,
>  	if (type == INTEL_OUTPUT_DP || type == INTEL_OUTPUT_EDP) {
>  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>  
> -		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> +		if (old_crtc_state && old_conn_state)
> +			intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);

I would make that
!intel_crtc_has_type(DP_MST)

>  	}
>  
>  	val = I915_READ(DDI_BUF_CTL(port));
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 8e3aad0ea60b..81e63724e24b 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -167,12 +167,11 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
>  	intel_dp->active_mst_links--;
>  
>  	intel_mst->connector = NULL;
> -	if (intel_dp->active_mst_links == 0) {
> +	if (intel_dp->active_mst_links == 0)
>  		intel_dig_port->base.post_disable(&intel_dig_port->base,
>  						  NULL, NULL);
> -
> -		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> -	}
> +	drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port,
> +				     false);

One thing that bothers me here is whether we'll start getting bogus link
training requests during shutdown since we'll do the D3 only after the
stream has been stopped. If you'll look at commit 7618138d8b40
("drm/i915/ddi: Avoid long delays during system suspend / eDP
disabling") Imre explicitly moved the D3 earlier for SST to avoid this.

Hmm. Actually by the time we call mst_post_disable() we should have
deallocated the VC etc. so the current stream should be already
terminated at that point. So I think we should be able to do the
PHY_POWER_DOWN before we shut down the link, just like we do for SST.

>  }
>  
>  static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
> @@ -197,6 +196,7 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
>  
>  	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
>  
> +	drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port, true);
>  	if (intel_dp->active_mst_links == 0)
>  		intel_dig_port->base.pre_enable(&intel_dig_port->base,
>  						pipe_config, NULL);
> -- 
> 2.11.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/2] drm/i915/mst: Use MST sideband message transaction for dpms
  2017-09-12 16:11   ` Ville Syrjälä
@ 2017-09-12 16:17     ` Ville Syrjälä
  2017-09-12 19:08       ` Pandiyan, Dhinakaran
  0 siblings, 1 reply; 23+ messages in thread
From: Ville Syrjälä @ 2017-09-12 16:17 UTC (permalink / raw)
  To: Dhinakaran Pandiyan; +Cc: intel-gfx, dri-devel

On Tue, Sep 12, 2017 at 07:11:32PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 05, 2017 at 06:26:34PM -0700, Dhinakaran Pandiyan wrote:
> > Use the POWER_DOWN_PHY and POWER_UP_PHY sideband message trasactions to
> > set power states for downstream sinks. Apart from giving us the ability
> > to set power state for individual sinks, this fixes the below test for
> > me
> > 
> > $ xrandr --display :0 --output DP-2-2-8 --off
> > $ xrandr --display :0 --output DP-2-2-1 --off
> > $ xrandr --display :0 --output DP-2-2-8 --auto #Black screen
> > $ xrandr --display :0 --output DP-2-2-1 --auto
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Lyude <lyude@redhat.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c    | 6 ++++--
> >  drivers/gpu/drm/i915/intel_dp_mst.c | 8 ++++----
> >  2 files changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 1da3bb2cc4b4..8aebacc0aa31 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -2161,7 +2161,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> >  		intel_prepare_dp_ddi_buffers(encoder);
> >  
> >  	intel_ddi_init_dp_buf_reg(encoder);
> > -	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> > +	if (!link_mst)
> > +		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> >  	intel_dp_start_link_train(intel_dp);
> >  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
> >  		intel_dp_stop_link_train(intel_dp);
> > @@ -2240,7 +2241,8 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder,
> >  	if (type == INTEL_OUTPUT_DP || type == INTEL_OUTPUT_EDP) {
> >  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >  
> > -		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> > +		if (old_crtc_state && old_conn_state)
> > +			intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> 
> I would make that
> !intel_crtc_has_type(DP_MST)
> 
> >  	}
> >  
> >  	val = I915_READ(DDI_BUF_CTL(port));
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index 8e3aad0ea60b..81e63724e24b 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -167,12 +167,11 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
> >  	intel_dp->active_mst_links--;
> >  
> >  	intel_mst->connector = NULL;
> > -	if (intel_dp->active_mst_links == 0) {
> > +	if (intel_dp->active_mst_links == 0)
> >  		intel_dig_port->base.post_disable(&intel_dig_port->base,
> >  						  NULL, NULL);
> > -
> > -		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> > -	}
> > +	drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port,
> > +				     false);
> 
> One thing that bothers me here is whether we'll start getting bogus link
> training requests during shutdown since we'll do the D3 only after the
> stream has been stopped. If you'll look at commit 7618138d8b40
> ("drm/i915/ddi: Avoid long delays during system suspend / eDP
> disabling") Imre explicitly moved the D3 earlier for SST to avoid this.
> 
> Hmm. Actually by the time we call mst_post_disable() we should have
> deallocated the VC etc. so the current stream should be already
> terminated at that point. So I think we should be able to do the
> PHY_POWER_DOWN before we shut down the link, just like we do for SST.

Might also be nice to add comments near all the D3 calls to document
that the order we use is there to prevent the sink from complaining
about link loss when we shut down the link. Otherwise someone might
get tempted to reorder things as a cleanup or something.

> 
> >  }
> >  
> >  static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
> > @@ -197,6 +196,7 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
> >  
> >  	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> >  
> > +	drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port, true);
> >  	if (intel_dp->active_mst_links == 0)
> >  		intel_dig_port->base.pre_enable(&intel_dig_port->base,
> >  						pipe_config, NULL);
> > -- 
> > 2.11.0
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/2] drm/i915/mst: Use MST sideband message transaction for dpms
  2017-09-12 16:17     ` Ville Syrjälä
@ 2017-09-12 19:08       ` Pandiyan, Dhinakaran
  2017-09-12 20:11         ` Pandiyan, Dhinakaran
  0 siblings, 1 reply; 23+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-09-12 19:08 UTC (permalink / raw)
  To: ville.syrjala; +Cc: intel-gfx, dri-devel

On Tue, 2017-09-12 at 19:17 +0300, Ville Syrjälä wrote:
> On Tue, Sep 12, 2017 at 07:11:32PM +0300, Ville Syrjälä wrote:
> > On Tue, Sep 05, 2017 at 06:26:34PM -0700, Dhinakaran Pandiyan wrote:
> > > Use the POWER_DOWN_PHY and POWER_UP_PHY sideband message trasactions to
> > > set power states for downstream sinks. Apart from giving us the ability
> > > to set power state for individual sinks, this fixes the below test for
> > > me
> > > 
> > > $ xrandr --display :0 --output DP-2-2-8 --off
> > > $ xrandr --display :0 --output DP-2-2-1 --off
> > > $ xrandr --display :0 --output DP-2-2-8 --auto #Black screen
> > > $ xrandr --display :0 --output DP-2-2-1 --auto
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Lyude <lyude@redhat.com>
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_ddi.c    | 6 ++++--
> > >  drivers/gpu/drm/i915/intel_dp_mst.c | 8 ++++----
> > >  2 files changed, 8 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > index 1da3bb2cc4b4..8aebacc0aa31 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -2161,7 +2161,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> > >  		intel_prepare_dp_ddi_buffers(encoder);
> > >  
> > >  	intel_ddi_init_dp_buf_reg(encoder);
> > > -	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> > > +	if (!link_mst)
> > > +		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> > >  	intel_dp_start_link_train(intel_dp);
> > >  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
> > >  		intel_dp_stop_link_train(intel_dp);
> > > @@ -2240,7 +2241,8 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder,
> > >  	if (type == INTEL_OUTPUT_DP || type == INTEL_OUTPUT_EDP) {
> > >  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > >  
> > > -		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> > > +		if (old_crtc_state && old_conn_state)
> > > +			intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> > 
> > I would make that
> > !intel_crtc_has_type(DP_MST)
> > 
> > >  	}
> > >  
> > >  	val = I915_READ(DDI_BUF_CTL(port));
> > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > index 8e3aad0ea60b..81e63724e24b 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > @@ -167,12 +167,11 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
> > >  	intel_dp->active_mst_links--;
> > >  
> > >  	intel_mst->connector = NULL;
> > > -	if (intel_dp->active_mst_links == 0) {
> > > +	if (intel_dp->active_mst_links == 0)
> > >  		intel_dig_port->base.post_disable(&intel_dig_port->base,
> > >  						  NULL, NULL);
> > > -
> > > -		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> > > -	}
> > > +	drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port,
> > > +				     false);
> > 
> > One thing that bothers me here is whether we'll start getting bogus link
> > training requests during shutdown since we'll do the D3 only after the
> > stream has been stopped. If you'll look at commit 7618138d8b40
> > ("drm/i915/ddi: Avoid long delays during system suspend / eDP
> > disabling") Imre explicitly moved the D3 earlier for SST to avoid this.
> > 

Interesting, that patch makes a lot of sense.

> > Hmm. Actually by the time we call mst_post_disable() we should have
> > deallocated the VC etc. so the current stream should be already
> > terminated at that point. So I think we should be able to do the
> > PHY_POWER_DOWN before we shut down the link, just like we do for SST.
> 
> Might also be nice to add comments near all the D3 calls to document
> that the order we use is there to prevent the sink from complaining
> about link loss when we shut down the link. Otherwise someone might
> get tempted to reorder things as a cleanup or something.
> 

Sure.

> > 
> > >  }
> > >  
> > >  static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
> > > @@ -197,6 +196,7 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
> > >  
> > >  	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> > >  
> > > +	drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port, true);
> > >  	if (intel_dp->active_mst_links == 0)
> > >  		intel_dig_port->base.pre_enable(&intel_dig_port->base,
> > >  						pipe_config, NULL);
> > > -- 
> > > 2.11.0
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/2] drm/i915/mst: Use MST sideband message transaction for dpms
  2017-09-12 19:08       ` Pandiyan, Dhinakaran
@ 2017-09-12 20:11         ` Pandiyan, Dhinakaran
  2017-09-13  7:32           ` Maarten Lankhorst
  0 siblings, 1 reply; 23+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-09-12 20:11 UTC (permalink / raw)
  To: ville.syrjala; +Cc: intel-gfx, dri-devel




On Tue, 2017-09-12 at 19:08 +0000, Pandiyan, Dhinakaran wrote:
> On Tue, 2017-09-12 at 19:17 +0300, Ville Syrjälä wrote:
> > On Tue, Sep 12, 2017 at 07:11:32PM +0300, Ville Syrjälä wrote:
> > > On Tue, Sep 05, 2017 at 06:26:34PM -0700, Dhinakaran Pandiyan wrote:
> > > > Use the POWER_DOWN_PHY and POWER_UP_PHY sideband message trasactions to
> > > > set power states for downstream sinks. Apart from giving us the ability
> > > > to set power state for individual sinks, this fixes the below test for
> > > > me
> > > > 
> > > > $ xrandr --display :0 --output DP-2-2-8 --off
> > > > $ xrandr --display :0 --output DP-2-2-1 --off
> > > > $ xrandr --display :0 --output DP-2-2-8 --auto #Black screen
> > > > $ xrandr --display :0 --output DP-2-2-1 --auto
> > > > 
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Lyude <lyude@redhat.com>
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_ddi.c    | 6 ++++--
> > > >  drivers/gpu/drm/i915/intel_dp_mst.c | 8 ++++----
> > > >  2 files changed, 8 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > > index 1da3bb2cc4b4..8aebacc0aa31 100644
> > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > @@ -2161,7 +2161,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> > > >  		intel_prepare_dp_ddi_buffers(encoder);
> > > >  
> > > >  	intel_ddi_init_dp_buf_reg(encoder);
> > > > -	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> > > > +	if (!link_mst)
> > > > +		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> > > >  	intel_dp_start_link_train(intel_dp);
> > > >  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
> > > >  		intel_dp_stop_link_train(intel_dp);
> > > > @@ -2240,7 +2241,8 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder,
> > > >  	if (type == INTEL_OUTPUT_DP || type == INTEL_OUTPUT_EDP) {
> > > >  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > >  
> > > > -		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> > > > +		if (old_crtc_state && old_conn_state)
> > > > +			intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> > > 
> > > I would make that
> > > !intel_crtc_has_type(DP_MST)
> > > 

old_crtc_state, which intel_crtc_has_type() needs, is NULL for MST and
the reason Maarten provided in his commit is

" We also shouldn't pass crtc_state, because in that case the
  disabling sequence may potentially be different depending on
  which crtc is disabled last. Nice way to introduce bugs.
"
I am not 100% sure I understand the concern. But since that change was
intentional I guess we can use the NULL values, like I've done, to
identify the mst sinks. I am open to ideas.



> > > >  	}
> > > >  
> > > >  	val = I915_READ(DDI_BUF_CTL(port));
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > index 8e3aad0ea60b..81e63724e24b 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > @@ -167,12 +167,11 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
> > > >  	intel_dp->active_mst_links--;
> > > >  
> > > >  	intel_mst->connector = NULL;
> > > > -	if (intel_dp->active_mst_links == 0) {
> > > > +	if (intel_dp->active_mst_links == 0)
> > > >  		intel_dig_port->base.post_disable(&intel_dig_port->base,
> > > >  						  NULL, NULL);
> > > > -
> > > > -		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> > > > -	}
> > > > +	drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port,
> > > > +				     false);
> > > 
> > > One thing that bothers me here is whether we'll start getting bogus link
> > > training requests during shutdown since we'll do the D3 only after the
> > > stream has been stopped. If you'll look at commit 7618138d8b40
> > > ("drm/i915/ddi: Avoid long delays during system suspend / eDP
> > > disabling") Imre explicitly moved the D3 earlier for SST to avoid this.
> > > 
> 
> Interesting, that patch makes a lot of sense.
> 
> > > Hmm. Actually by the time we call mst_post_disable() we should have
> > > deallocated the VC etc. so the current stream should be already
> > > terminated at that point. So I think we should be able to do the
> > > PHY_POWER_DOWN before we shut down the link, just like we do for SST.
> > 
> > Might also be nice to add comments near all the D3 calls to document
> > that the order we use is there to prevent the sink from complaining
> > about link loss when we shut down the link. Otherwise someone might
> > get tempted to reorder things as a cleanup or something.
> > 
> 
> Sure.
> 
> > > 
> > > >  }
> > > >  
> > > >  static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
> > > > @@ -197,6 +196,7 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
> > > >  
> > > >  	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> > > >  
> > > > +	drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port, true);
> > > >  	if (intel_dp->active_mst_links == 0)
> > > >  		intel_dig_port->base.pre_enable(&intel_dig_port->base,
> > > >  						pipe_config, NULL);
> > > > -- 
> > > > 2.11.0
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel OTC
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/2] drm/i915/mst: Use MST sideband message transaction for dpms
  2017-09-12 20:11         ` Pandiyan, Dhinakaran
@ 2017-09-13  7:32           ` Maarten Lankhorst
  2017-09-13 10:37             ` [Intel-gfx] " Ville Syrjälä
  0 siblings, 1 reply; 23+ messages in thread
From: Maarten Lankhorst @ 2017-09-13  7:32 UTC (permalink / raw)
  To: Pandiyan, Dhinakaran, ville.syrjala; +Cc: intel-gfx, dri-devel

Op 12-09-17 om 22:11 schreef Pandiyan, Dhinakaran:
>
>
> On Tue, 2017-09-12 at 19:08 +0000, Pandiyan, Dhinakaran wrote:
>> On Tue, 2017-09-12 at 19:17 +0300, Ville Syrjälä wrote:
>>> On Tue, Sep 12, 2017 at 07:11:32PM +0300, Ville Syrjälä wrote:
>>>> On Tue, Sep 05, 2017 at 06:26:34PM -0700, Dhinakaran Pandiyan wrote:
>>>>> Use the POWER_DOWN_PHY and POWER_UP_PHY sideband message trasactions to
>>>>> set power states for downstream sinks. Apart from giving us the ability
>>>>> to set power state for individual sinks, this fixes the below test for
>>>>> me
>>>>>
>>>>> $ xrandr --display :0 --output DP-2-2-8 --off
>>>>> $ xrandr --display :0 --output DP-2-2-1 --off
>>>>> $ xrandr --display :0 --output DP-2-2-8 --auto #Black screen
>>>>> $ xrandr --display :0 --output DP-2-2-1 --auto
>>>>>
>>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>> Cc: Lyude <lyude@redhat.com>
>>>>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>>>>> ---
>>>>>  drivers/gpu/drm/i915/intel_ddi.c    | 6 ++++--
>>>>>  drivers/gpu/drm/i915/intel_dp_mst.c | 8 ++++----
>>>>>  2 files changed, 8 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>>>>> index 1da3bb2cc4b4..8aebacc0aa31 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>>>> @@ -2161,7 +2161,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
>>>>>  		intel_prepare_dp_ddi_buffers(encoder);
>>>>>  
>>>>>  	intel_ddi_init_dp_buf_reg(encoder);
>>>>> -	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
>>>>> +	if (!link_mst)
>>>>> +		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
>>>>>  	intel_dp_start_link_train(intel_dp);
>>>>>  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
>>>>>  		intel_dp_stop_link_train(intel_dp);
>>>>> @@ -2240,7 +2241,8 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder,
>>>>>  	if (type == INTEL_OUTPUT_DP || type == INTEL_OUTPUT_EDP) {
>>>>>  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>>>>>  
>>>>> -		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
>>>>> +		if (old_crtc_state && old_conn_state)
>>>>> +			intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
>>>> I would make that
>>>> !intel_crtc_has_type(DP_MST)
>>>>
> old_crtc_state, which intel_crtc_has_type() needs, is NULL for MST and
> the reason Maarten provided in his commit is
>
> " We also shouldn't pass crtc_state, because in that case the
>   disabling sequence may potentially be different depending on
>   which crtc is disabled last. Nice way to introduce bugs.
> "
> I am not 100% sure I understand the concern. But since that change was
> intentional I guess we can use the NULL values, like I've done, to
> identify the mst sinks. I am open to ideas.
I think checking for NULL crtc_state is enough, in case we ever decide to pass the real connector state.
For clarity, I would add something like bool is_dsp_mst = !crtc_state; /* When enabling the link for MST, this connector is never bound to a crtc */.
>
>
>>>>>  	}
>>>>>  
>>>>>  	val = I915_READ(DDI_BUF_CTL(port));
>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
>>>>> index 8e3aad0ea60b..81e63724e24b 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
>>>>> @@ -167,12 +167,11 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
>>>>>  	intel_dp->active_mst_links--;
>>>>>  
>>>>>  	intel_mst->connector = NULL;
>>>>> -	if (intel_dp->active_mst_links == 0) {
>>>>> +	if (intel_dp->active_mst_links == 0)
>>>>>  		intel_dig_port->base.post_disable(&intel_dig_port->base,
>>>>>  						  NULL, NULL);
>>>>> -
>>>>> -		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
>>>>> -	}
>>>>> +	drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port,
>>>>> +				     false);
>>>> One thing that bothers me here is whether we'll start getting bogus link
>>>> training requests during shutdown since we'll do the D3 only after the
>>>> stream has been stopped. If you'll look at commit 7618138d8b40
>>>> ("drm/i915/ddi: Avoid long delays during system suspend / eDP
>>>> disabling") Imre explicitly moved the D3 earlier for SST to avoid this.
>>>>
>> Interesting, that patch makes a lot of sense.
>>
>>>> Hmm. Actually by the time we call mst_post_disable() we should have
>>>> deallocated the VC etc. so the current stream should be already
>>>> terminated at that point. So I think we should be able to do the
>>>> PHY_POWER_DOWN before we shut down the link, just like we do for SST.
>>> Might also be nice to add comments near all the D3 calls to document
>>> that the order we use is there to prevent the sink from complaining
>>> about link loss when we shut down the link. Otherwise someone might
>>> get tempted to reorder things as a cleanup or something.
>>>
>> Sure.
>>
>>>>>  }
>>>>>  
>>>>>  static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
>>>>> @@ -197,6 +196,7 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
>>>>>  
>>>>>  	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
>>>>>  
>>>>> +	drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port, true);
>>>>>  	if (intel_dp->active_mst_links == 0)
>>>>>  		intel_dig_port->base.pre_enable(&intel_dig_port->base,
>>>>>  						pipe_config, NULL);
>>>>> -- 
>>>>> 2.11.0
>>>> -- 
>>>> Ville Syrjälä
>>>> Intel OTC
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx


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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Intel-gfx] [PATCH 2/2] drm/i915/mst: Use MST sideband message transaction for dpms
  2017-09-13  7:32           ` Maarten Lankhorst
@ 2017-09-13 10:37             ` Ville Syrjälä
  2017-09-13 10:46               ` Maarten Lankhorst
  0 siblings, 1 reply; 23+ messages in thread
From: Ville Syrjälä @ 2017-09-13 10:37 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx, Pandiyan, Dhinakaran, dri-devel

On Wed, Sep 13, 2017 at 09:32:40AM +0200, Maarten Lankhorst wrote:
> Op 12-09-17 om 22:11 schreef Pandiyan, Dhinakaran:
> >
> >
> > On Tue, 2017-09-12 at 19:08 +0000, Pandiyan, Dhinakaran wrote:
> >> On Tue, 2017-09-12 at 19:17 +0300, Ville Syrjälä wrote:
> >>> On Tue, Sep 12, 2017 at 07:11:32PM +0300, Ville Syrjälä wrote:
> >>>> On Tue, Sep 05, 2017 at 06:26:34PM -0700, Dhinakaran Pandiyan wrote:
> >>>>> Use the POWER_DOWN_PHY and POWER_UP_PHY sideband message trasactions to
> >>>>> set power states for downstream sinks. Apart from giving us the ability
> >>>>> to set power state for individual sinks, this fixes the below test for
> >>>>> me
> >>>>>
> >>>>> $ xrandr --display :0 --output DP-2-2-8 --off
> >>>>> $ xrandr --display :0 --output DP-2-2-1 --off
> >>>>> $ xrandr --display :0 --output DP-2-2-8 --auto #Black screen
> >>>>> $ xrandr --display :0 --output DP-2-2-1 --auto
> >>>>>
> >>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>>> Cc: Lyude <lyude@redhat.com>
> >>>>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> >>>>> ---
> >>>>>  drivers/gpu/drm/i915/intel_ddi.c    | 6 ++++--
> >>>>>  drivers/gpu/drm/i915/intel_dp_mst.c | 8 ++++----
> >>>>>  2 files changed, 8 insertions(+), 6 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >>>>> index 1da3bb2cc4b4..8aebacc0aa31 100644
> >>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >>>>> @@ -2161,7 +2161,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> >>>>>  		intel_prepare_dp_ddi_buffers(encoder);
> >>>>>  
> >>>>>  	intel_ddi_init_dp_buf_reg(encoder);
> >>>>> -	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> >>>>> +	if (!link_mst)
> >>>>> +		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> >>>>>  	intel_dp_start_link_train(intel_dp);
> >>>>>  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
> >>>>>  		intel_dp_stop_link_train(intel_dp);
> >>>>> @@ -2240,7 +2241,8 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder,
> >>>>>  	if (type == INTEL_OUTPUT_DP || type == INTEL_OUTPUT_EDP) {
> >>>>>  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >>>>>  
> >>>>> -		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> >>>>> +		if (old_crtc_state && old_conn_state)
> >>>>> +			intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> >>>> I would make that
> >>>> !intel_crtc_has_type(DP_MST)
> >>>>
> > old_crtc_state, which intel_crtc_has_type() needs, is NULL for MST and
> > the reason Maarten provided in his commit is
> >
> > " We also shouldn't pass crtc_state, because in that case the
> >   disabling sequence may potentially be different depending on
> >   which crtc is disabled last. Nice way to introduce bugs.
> > "
> > I am not 100% sure I understand the concern. But since that change was
> > intentional I guess we can use the NULL values, like I've done, to
> > identify the mst sinks. I am open to ideas.
> I think checking for NULL crtc_state is enough, in case we ever decide to pass the real connector state.
> For clarity, I would add something like bool is_dsp_mst = !crtc_state; /* When enabling the link for MST, this connector is never bound to a crtc */.

The NULL state is rather ugly IMO. With a comment I might be able to
stomach it. However, after another look I see that we already pass the
crtc state to ddi_pre_enable() from the MST code, so even just from a
symmetry POV it would make sense to pass it to ddi_post_disable as well.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Intel-gfx] [PATCH 2/2] drm/i915/mst: Use MST sideband message transaction for dpms
  2017-09-13 10:37             ` [Intel-gfx] " Ville Syrjälä
@ 2017-09-13 10:46               ` Maarten Lankhorst
  2017-09-13 11:48                 ` Ville Syrjälä
  0 siblings, 1 reply; 23+ messages in thread
From: Maarten Lankhorst @ 2017-09-13 10:46 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx, Pandiyan, Dhinakaran, dri-devel

Op 13-09-17 om 12:37 schreef Ville Syrjälä:
> On Wed, Sep 13, 2017 at 09:32:40AM +0200, Maarten Lankhorst wrote:
>> Op 12-09-17 om 22:11 schreef Pandiyan, Dhinakaran:
>>>
>>> On Tue, 2017-09-12 at 19:08 +0000, Pandiyan, Dhinakaran wrote:
>>>> On Tue, 2017-09-12 at 19:17 +0300, Ville Syrjälä wrote:
>>>>> On Tue, Sep 12, 2017 at 07:11:32PM +0300, Ville Syrjälä wrote:
>>>>>> On Tue, Sep 05, 2017 at 06:26:34PM -0700, Dhinakaran Pandiyan wrote:
>>>>>>> Use the POWER_DOWN_PHY and POWER_UP_PHY sideband message trasactions to
>>>>>>> set power states for downstream sinks. Apart from giving us the ability
>>>>>>> to set power state for individual sinks, this fixes the below test for
>>>>>>> me
>>>>>>>
>>>>>>> $ xrandr --display :0 --output DP-2-2-8 --off
>>>>>>> $ xrandr --display :0 --output DP-2-2-1 --off
>>>>>>> $ xrandr --display :0 --output DP-2-2-8 --auto #Black screen
>>>>>>> $ xrandr --display :0 --output DP-2-2-1 --auto
>>>>>>>
>>>>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>>> Cc: Lyude <lyude@redhat.com>
>>>>>>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>>>>>>> ---
>>>>>>>  drivers/gpu/drm/i915/intel_ddi.c    | 6 ++++--
>>>>>>>  drivers/gpu/drm/i915/intel_dp_mst.c | 8 ++++----
>>>>>>>  2 files changed, 8 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>>>>>>> index 1da3bb2cc4b4..8aebacc0aa31 100644
>>>>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>>>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>>>>>> @@ -2161,7 +2161,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
>>>>>>>  		intel_prepare_dp_ddi_buffers(encoder);
>>>>>>>  
>>>>>>>  	intel_ddi_init_dp_buf_reg(encoder);
>>>>>>> -	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
>>>>>>> +	if (!link_mst)
>>>>>>> +		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
>>>>>>>  	intel_dp_start_link_train(intel_dp);
>>>>>>>  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
>>>>>>>  		intel_dp_stop_link_train(intel_dp);
>>>>>>> @@ -2240,7 +2241,8 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder,
>>>>>>>  	if (type == INTEL_OUTPUT_DP || type == INTEL_OUTPUT_EDP) {
>>>>>>>  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>>>>>>>  
>>>>>>> -		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
>>>>>>> +		if (old_crtc_state && old_conn_state)
>>>>>>> +			intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
>>>>>> I would make that
>>>>>> !intel_crtc_has_type(DP_MST)
>>>>>>
>>> old_crtc_state, which intel_crtc_has_type() needs, is NULL for MST and
>>> the reason Maarten provided in his commit is
>>>
>>> " We also shouldn't pass crtc_state, because in that case the
>>>   disabling sequence may potentially be different depending on
>>>   which crtc is disabled last. Nice way to introduce bugs.
>>> "
>>> I am not 100% sure I understand the concern. But since that change was
>>> intentional I guess we can use the NULL values, like I've done, to
>>> identify the mst sinks. I am open to ideas.
>> I think checking for NULL crtc_state is enough, in case we ever decide to pass the real connector state.
>> For clarity, I would add something like bool is_dsp_mst = !crtc_state; /* When enabling the link for MST, this connector is never bound to a crtc */.
> The NULL state is rather ugly IMO. With a comment I might be able to
> stomach it. However, after another look I see that we already pass the
> crtc state to ddi_pre_enable() from the MST code, so even just from a
> symmetry POV it would make sense to pass it to ddi_post_disable as well.
>
It doesn't make sense to pass crtc_state, there's no guarantee that the crtc_state
from the same crtc is passed into first MST enable, and last MST disable.
To be sure we don't rely on it NULL was passed in, which is the only valid thing here.

~Maarten

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/2] drm/i915/mst: Use MST sideband message transaction for dpms
  2017-09-13 10:46               ` Maarten Lankhorst
@ 2017-09-13 11:48                 ` Ville Syrjälä
  2017-09-13 11:55                   ` Maarten Lankhorst
  0 siblings, 1 reply; 23+ messages in thread
From: Ville Syrjälä @ 2017-09-13 11:48 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx, Pandiyan, Dhinakaran, dri-devel

On Wed, Sep 13, 2017 at 12:46:47PM +0200, Maarten Lankhorst wrote:
> Op 13-09-17 om 12:37 schreef Ville Syrjälä:
> > On Wed, Sep 13, 2017 at 09:32:40AM +0200, Maarten Lankhorst wrote:
> >> Op 12-09-17 om 22:11 schreef Pandiyan, Dhinakaran:
> >>>
> >>> On Tue, 2017-09-12 at 19:08 +0000, Pandiyan, Dhinakaran wrote:
> >>>> On Tue, 2017-09-12 at 19:17 +0300, Ville Syrjälä wrote:
> >>>>> On Tue, Sep 12, 2017 at 07:11:32PM +0300, Ville Syrjälä wrote:
> >>>>>> On Tue, Sep 05, 2017 at 06:26:34PM -0700, Dhinakaran Pandiyan wrote:
> >>>>>>> Use the POWER_DOWN_PHY and POWER_UP_PHY sideband message trasactions to
> >>>>>>> set power states for downstream sinks. Apart from giving us the ability
> >>>>>>> to set power state for individual sinks, this fixes the below test for
> >>>>>>> me
> >>>>>>>
> >>>>>>> $ xrandr --display :0 --output DP-2-2-8 --off
> >>>>>>> $ xrandr --display :0 --output DP-2-2-1 --off
> >>>>>>> $ xrandr --display :0 --output DP-2-2-8 --auto #Black screen
> >>>>>>> $ xrandr --display :0 --output DP-2-2-1 --auto
> >>>>>>>
> >>>>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>>>>> Cc: Lyude <lyude@redhat.com>
> >>>>>>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> >>>>>>> ---
> >>>>>>>  drivers/gpu/drm/i915/intel_ddi.c    | 6 ++++--
> >>>>>>>  drivers/gpu/drm/i915/intel_dp_mst.c | 8 ++++----
> >>>>>>>  2 files changed, 8 insertions(+), 6 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >>>>>>> index 1da3bb2cc4b4..8aebacc0aa31 100644
> >>>>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >>>>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >>>>>>> @@ -2161,7 +2161,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> >>>>>>>  		intel_prepare_dp_ddi_buffers(encoder);
> >>>>>>>  
> >>>>>>>  	intel_ddi_init_dp_buf_reg(encoder);
> >>>>>>> -	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> >>>>>>> +	if (!link_mst)
> >>>>>>> +		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> >>>>>>>  	intel_dp_start_link_train(intel_dp);
> >>>>>>>  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
> >>>>>>>  		intel_dp_stop_link_train(intel_dp);
> >>>>>>> @@ -2240,7 +2241,8 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder,
> >>>>>>>  	if (type == INTEL_OUTPUT_DP || type == INTEL_OUTPUT_EDP) {
> >>>>>>>  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >>>>>>>  
> >>>>>>> -		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> >>>>>>> +		if (old_crtc_state && old_conn_state)
> >>>>>>> +			intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> >>>>>> I would make that
> >>>>>> !intel_crtc_has_type(DP_MST)
> >>>>>>
> >>> old_crtc_state, which intel_crtc_has_type() needs, is NULL for MST and
> >>> the reason Maarten provided in his commit is
> >>>
> >>> " We also shouldn't pass crtc_state, because in that case the
> >>>   disabling sequence may potentially be different depending on
> >>>   which crtc is disabled last. Nice way to introduce bugs.
> >>> "
> >>> I am not 100% sure I understand the concern. But since that change was
> >>> intentional I guess we can use the NULL values, like I've done, to
> >>> identify the mst sinks. I am open to ideas.
> >> I think checking for NULL crtc_state is enough, in case we ever decide to pass the real connector state.
> >> For clarity, I would add something like bool is_dsp_mst = !crtc_state; /* When enabling the link for MST, this connector is never bound to a crtc */.
> > The NULL state is rather ugly IMO. With a comment I might be able to
> > stomach it. However, after another look I see that we already pass the
> > crtc state to ddi_pre_enable() from the MST code, so even just from a
> > symmetry POV it would make sense to pass it to ddi_post_disable as well.
> >
> It doesn't make sense to pass crtc_state, there's no guarantee that the crtc_state
> from the same crtc is passed into first MST enable, and last MST disable.

Doesn't really matter. If they don't match sufficiently we've already
messed up somewhere.

Ideally we should have some kind of separate state for the DP link, but
since we don't atm we just abuse the crtc state to contain the link
parameters.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/2] drm/i915/mst: Use MST sideband message transaction for dpms
  2017-09-13 11:48                 ` Ville Syrjälä
@ 2017-09-13 11:55                   ` Maarten Lankhorst
  2017-09-13 13:52                     ` Ville Syrjälä
  0 siblings, 1 reply; 23+ messages in thread
From: Maarten Lankhorst @ 2017-09-13 11:55 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx, Pandiyan, Dhinakaran, dri-devel

Op 13-09-17 om 13:48 schreef Ville Syrjälä:
> On Wed, Sep 13, 2017 at 12:46:47PM +0200, Maarten Lankhorst wrote:
>> Op 13-09-17 om 12:37 schreef Ville Syrjälä:
>>> On Wed, Sep 13, 2017 at 09:32:40AM +0200, Maarten Lankhorst wrote:
>>>> Op 12-09-17 om 22:11 schreef Pandiyan, Dhinakaran:
>>>>> On Tue, 2017-09-12 at 19:08 +0000, Pandiyan, Dhinakaran wrote:
>>>>>> On Tue, 2017-09-12 at 19:17 +0300, Ville Syrjälä wrote:
>>>>>>> On Tue, Sep 12, 2017 at 07:11:32PM +0300, Ville Syrjälä wrote:
>>>>>>>> On Tue, Sep 05, 2017 at 06:26:34PM -0700, Dhinakaran Pandiyan wrote:
>>>>>>>>> Use the POWER_DOWN_PHY and POWER_UP_PHY sideband message trasactions to
>>>>>>>>> set power states for downstream sinks. Apart from giving us the ability
>>>>>>>>> to set power state for individual sinks, this fixes the below test for
>>>>>>>>> me
>>>>>>>>>
>>>>>>>>> $ xrandr --display :0 --output DP-2-2-8 --off
>>>>>>>>> $ xrandr --display :0 --output DP-2-2-1 --off
>>>>>>>>> $ xrandr --display :0 --output DP-2-2-8 --auto #Black screen
>>>>>>>>> $ xrandr --display :0 --output DP-2-2-1 --auto
>>>>>>>>>
>>>>>>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>>>>> Cc: Lyude <lyude@redhat.com>
>>>>>>>>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>>>>>>>>> ---
>>>>>>>>>  drivers/gpu/drm/i915/intel_ddi.c    | 6 ++++--
>>>>>>>>>  drivers/gpu/drm/i915/intel_dp_mst.c | 8 ++++----
>>>>>>>>>  2 files changed, 8 insertions(+), 6 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>>>>>>>>> index 1da3bb2cc4b4..8aebacc0aa31 100644
>>>>>>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>>>>>>>> @@ -2161,7 +2161,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
>>>>>>>>>  		intel_prepare_dp_ddi_buffers(encoder);
>>>>>>>>>  
>>>>>>>>>  	intel_ddi_init_dp_buf_reg(encoder);
>>>>>>>>> -	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
>>>>>>>>> +	if (!link_mst)
>>>>>>>>> +		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
>>>>>>>>>  	intel_dp_start_link_train(intel_dp);
>>>>>>>>>  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
>>>>>>>>>  		intel_dp_stop_link_train(intel_dp);
>>>>>>>>> @@ -2240,7 +2241,8 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder,
>>>>>>>>>  	if (type == INTEL_OUTPUT_DP || type == INTEL_OUTPUT_EDP) {
>>>>>>>>>  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>>>>>>>>>  
>>>>>>>>> -		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
>>>>>>>>> +		if (old_crtc_state && old_conn_state)
>>>>>>>>> +			intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
>>>>>>>> I would make that
>>>>>>>> !intel_crtc_has_type(DP_MST)
>>>>>>>>
>>>>> old_crtc_state, which intel_crtc_has_type() needs, is NULL for MST and
>>>>> the reason Maarten provided in his commit is
>>>>>
>>>>> " We also shouldn't pass crtc_state, because in that case the
>>>>>   disabling sequence may potentially be different depending on
>>>>>   which crtc is disabled last. Nice way to introduce bugs.
>>>>> "
>>>>> I am not 100% sure I understand the concern. But since that change was
>>>>> intentional I guess we can use the NULL values, like I've done, to
>>>>> identify the mst sinks. I am open to ideas.
>>>> I think checking for NULL crtc_state is enough, in case we ever decide to pass the real connector state.
>>>> For clarity, I would add something like bool is_dsp_mst = !crtc_state; /* When enabling the link for MST, this connector is never bound to a crtc */.
>>> The NULL state is rather ugly IMO. With a comment I might be able to
>>> stomach it. However, after another look I see that we already pass the
>>> crtc state to ddi_pre_enable() from the MST code, so even just from a
>>> symmetry POV it would make sense to pass it to ddi_post_disable as well.
>>>
>> It doesn't make sense to pass crtc_state, there's no guarantee that the crtc_state
>> from the same crtc is passed into first MST enable, and last MST disable.
> Doesn't really matter. If they don't match sufficiently we've already
> messed up somewhere.
>
> Ideally we should have some kind of separate state for the DP link, but
> since we don't atm we just abuse the crtc state to contain the link
> parameters.

Could we stuff it in the real DP's connector state, and then make sure we add the master connector state?

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/2] drm/i915/mst: Use MST sideband message transaction for dpms
  2017-09-13 11:55                   ` Maarten Lankhorst
@ 2017-09-13 13:52                     ` Ville Syrjälä
  0 siblings, 0 replies; 23+ messages in thread
From: Ville Syrjälä @ 2017-09-13 13:52 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx, Pandiyan, Dhinakaran, dri-devel

On Wed, Sep 13, 2017 at 01:55:34PM +0200, Maarten Lankhorst wrote:
> Op 13-09-17 om 13:48 schreef Ville Syrjälä:
> > On Wed, Sep 13, 2017 at 12:46:47PM +0200, Maarten Lankhorst wrote:
> >> Op 13-09-17 om 12:37 schreef Ville Syrjälä:
> >>> On Wed, Sep 13, 2017 at 09:32:40AM +0200, Maarten Lankhorst wrote:
> >>>> Op 12-09-17 om 22:11 schreef Pandiyan, Dhinakaran:
> >>>>> On Tue, 2017-09-12 at 19:08 +0000, Pandiyan, Dhinakaran wrote:
> >>>>>> On Tue, 2017-09-12 at 19:17 +0300, Ville Syrjälä wrote:
> >>>>>>> On Tue, Sep 12, 2017 at 07:11:32PM +0300, Ville Syrjälä wrote:
> >>>>>>>> On Tue, Sep 05, 2017 at 06:26:34PM -0700, Dhinakaran Pandiyan wrote:
> >>>>>>>>> Use the POWER_DOWN_PHY and POWER_UP_PHY sideband message trasactions to
> >>>>>>>>> set power states for downstream sinks. Apart from giving us the ability
> >>>>>>>>> to set power state for individual sinks, this fixes the below test for
> >>>>>>>>> me
> >>>>>>>>>
> >>>>>>>>> $ xrandr --display :0 --output DP-2-2-8 --off
> >>>>>>>>> $ xrandr --display :0 --output DP-2-2-1 --off
> >>>>>>>>> $ xrandr --display :0 --output DP-2-2-8 --auto #Black screen
> >>>>>>>>> $ xrandr --display :0 --output DP-2-2-1 --auto
> >>>>>>>>>
> >>>>>>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>>>>>>> Cc: Lyude <lyude@redhat.com>
> >>>>>>>>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> >>>>>>>>> ---
> >>>>>>>>>  drivers/gpu/drm/i915/intel_ddi.c    | 6 ++++--
> >>>>>>>>>  drivers/gpu/drm/i915/intel_dp_mst.c | 8 ++++----
> >>>>>>>>>  2 files changed, 8 insertions(+), 6 deletions(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >>>>>>>>> index 1da3bb2cc4b4..8aebacc0aa31 100644
> >>>>>>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >>>>>>>>> @@ -2161,7 +2161,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> >>>>>>>>>  		intel_prepare_dp_ddi_buffers(encoder);
> >>>>>>>>>  
> >>>>>>>>>  	intel_ddi_init_dp_buf_reg(encoder);
> >>>>>>>>> -	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> >>>>>>>>> +	if (!link_mst)
> >>>>>>>>> +		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> >>>>>>>>>  	intel_dp_start_link_train(intel_dp);
> >>>>>>>>>  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
> >>>>>>>>>  		intel_dp_stop_link_train(intel_dp);
> >>>>>>>>> @@ -2240,7 +2241,8 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder,
> >>>>>>>>>  	if (type == INTEL_OUTPUT_DP || type == INTEL_OUTPUT_EDP) {
> >>>>>>>>>  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >>>>>>>>>  
> >>>>>>>>> -		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> >>>>>>>>> +		if (old_crtc_state && old_conn_state)
> >>>>>>>>> +			intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> >>>>>>>> I would make that
> >>>>>>>> !intel_crtc_has_type(DP_MST)
> >>>>>>>>
> >>>>> old_crtc_state, which intel_crtc_has_type() needs, is NULL for MST and
> >>>>> the reason Maarten provided in his commit is
> >>>>>
> >>>>> " We also shouldn't pass crtc_state, because in that case the
> >>>>>   disabling sequence may potentially be different depending on
> >>>>>   which crtc is disabled last. Nice way to introduce bugs.
> >>>>> "
> >>>>> I am not 100% sure I understand the concern. But since that change was
> >>>>> intentional I guess we can use the NULL values, like I've done, to
> >>>>> identify the mst sinks. I am open to ideas.
> >>>> I think checking for NULL crtc_state is enough, in case we ever decide to pass the real connector state.
> >>>> For clarity, I would add something like bool is_dsp_mst = !crtc_state; /* When enabling the link for MST, this connector is never bound to a crtc */.
> >>> The NULL state is rather ugly IMO. With a comment I might be able to
> >>> stomach it. However, after another look I see that we already pass the
> >>> crtc state to ddi_pre_enable() from the MST code, so even just from a
> >>> symmetry POV it would make sense to pass it to ddi_post_disable as well.
> >>>
> >> It doesn't make sense to pass crtc_state, there's no guarantee that the crtc_state
> >> from the same crtc is passed into first MST enable, and last MST disable.
> > Doesn't really matter. If they don't match sufficiently we've already
> > messed up somewhere.
> >
> > Ideally we should have some kind of separate state for the DP link, but
> > since we don't atm we just abuse the crtc state to contain the link
> > parameters.
> 
> Could we stuff it in the real DP's connector state, and then make sure we add the master connector state?

Not sure we want a disabled connector to affect the entire thing. I was
thinking that maybe we want some kind of encoder state thing. Not really
sure.

At some point I did have the idea that maybe we could stick the main MST
connector into the state, and actually use that to turn on the link,
leaving the MST encoder hooks to only enable/disable the streams. But
that would need careful ordering to make sure the main link gets
enabled/disabled as needed. And it would probably hit the design
issue in atomic that we'd end up leaking some of the main link state
to user space. We'd also need some kind of fake crtc for the main link I
suppose.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2017-09-13 13:52 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-06  1:26 [PATCH 1/2] drm/dp/mst: Sideband message transaction to power up/down nodes Dhinakaran Pandiyan
2017-09-06  1:26 ` [PATCH 2/2] drm/i915/mst: Use MST sideband message transaction for dpms Dhinakaran Pandiyan
2017-09-07  5:51   ` Stefan Assmann
2017-09-12 16:11   ` Ville Syrjälä
2017-09-12 16:17     ` Ville Syrjälä
2017-09-12 19:08       ` Pandiyan, Dhinakaran
2017-09-12 20:11         ` Pandiyan, Dhinakaran
2017-09-13  7:32           ` Maarten Lankhorst
2017-09-13 10:37             ` [Intel-gfx] " Ville Syrjälä
2017-09-13 10:46               ` Maarten Lankhorst
2017-09-13 11:48                 ` Ville Syrjälä
2017-09-13 11:55                   ` Maarten Lankhorst
2017-09-13 13:52                     ` Ville Syrjälä
2017-09-06  1:46 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/dp/mst: Sideband message transaction to power up/down nodes Patchwork
2017-09-06 15:40 ` [PATCH 1/2] " Lyude Paul
2017-09-06 16:36   ` Pandiyan, Dhinakaran
2017-09-07  0:14   ` [PATCH v2] " Dhinakaran Pandiyan
2017-09-07 18:04     ` Lyude Paul
2017-09-07 18:54       ` [Intel-gfx] " Pandiyan, Dhinakaran
2017-09-11 13:33     ` Ville Syrjälä
2017-09-11 19:10       ` Pandiyan, Dhinakaran
2017-09-07  0:35 ` ✗ Fi.CI.BAT: failure for series starting with [v2] drm/dp/mst: Sideband message transaction to power up/down nodes (rev2) Patchwork
2017-09-07  5:49 ` [PATCH 1/2] drm/dp/mst: Sideband message transaction to power up/down nodes Stefan Assmann

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.