[PATCH 1/6] drm/dp_mst: Introduce drm_dp_mst_connector_atomic_check()

[PATCH 1/6] drm/dp_mst: Introduce drm_dp_mst_connector_atomic_check()

[Lyude Paul]

Currently the way that we prevent userspace from performing new modesets
on MST connectors that have just been destroyed is rather broken.
There's nothing in the actual DRM DP MST topology helpers that checks
whether or not a connector still exists, instead each DRM driver does
this on it's own, usually by returning NULL from the best_encoder
callback which in turn, causes the atomic commit to fail.

However, this is wrong in a rather subtle way. If ->best_encoder()
returns NULL, this makes ALL modesets involving the connector fail. This
includes modesets from userspace that would shut off the CRTCs being
used by the connector. Since this results in blocking any changes to a
connector's DPMS prop, it has the sideaffect of preventing legacy
modesetting users from ever disabling a CRTC that was previously enabled
for use in an MST topology. An example of this, where X tries to
change the DPMS property of an MST connector that was just detached from
the system:

[ 2908.320131] [drm:drm_helper_probe_single_connector_modes [drm_kms_helper]] [CONNECTOR:82:DP-6]
[ 2908.320148] [drm:drm_helper_probe_single_connector_modes [drm_kms_helper]] [CONNECTOR:82:DP-6] status updated from connected to disconnected
[ 2908.320166] [drm:drm_helper_probe_single_connector_modes [drm_kms_helper]] [CONNECTOR:82:DP-6] disconnected
[ 2908.320193] [drm:drm_mode_object_put.part.2 [drm]] OBJ ID: 111 (1)
[ 2908.320230] [drm:drm_sysfs_hotplug_event [drm]] generating hotplug event
...
[ 2908.638539] [drm:drm_ioctl [drm]] pid=12928, dev=0xe201, auth=1, DRM_IOCTL_MODE_SETPROPERTY
[ 2908.638546] [drm:drm_atomic_state_init [drm]] Allocated atomic state 000000007155ba49
[ 2908.638553] [drm:drm_mode_object_get [drm]] OBJ ID: 114 (1)
[ 2908.638560] [drm:drm_mode_object_get [drm]] OBJ ID: 108 (1)
[ 2908.638568] [drm:drm_atomic_get_crtc_state [drm]] Added [CRTC:41:head-0] 0000000097a6396e state to 000000007155ba49
[ 2908.638575] [drm:drm_atomic_add_affected_connectors [drm]] Adding all current connectors for [CRTC:41:head-0] to 000000007155ba49
[ 2908.638582] [drm:drm_mode_object_get [drm]] OBJ ID: 82 (3)
[ 2908.638589] [drm:drm_mode_object_get [drm]] OBJ ID: 82 (4)
[ 2908.638596] [drm:drm_atomic_get_connector_state [drm]] Added [CONNECTOR:82:DP-6] 0000000087427144 state to 000000007155ba49
[ 2908.638603] [drm:drm_atomic_check_only [drm]] checking 000000007155ba49
[ 2908.638609] [drm:drm_atomic_helper_check_modeset [drm_kms_helper]] [CRTC:41:head-0] active changed
[ 2908.638613] [drm:drm_atomic_helper_check_modeset [drm_kms_helper]] Updating routing for [CONNECTOR:82:DP-6]
[ 2908.638616] [drm:drm_atomic_helper_check_modeset [drm_kms_helper]] No suitable encoder found for [CONNECTOR:82:DP-6]
[ 2908.638623] [drm:drm_atomic_check_only [drm]] atomic driver check for 000000007155ba49 failed: -22
[ 2908.638630] [drm:drm_atomic_state_default_clear [drm]] Clearing atomic state 000000007155ba49
[ 2908.638637] [drm:drm_mode_object_put.part.2 [drm]] OBJ ID: 82 (4)
[ 2908.638643] [drm:drm_mode_object_put.part.2 [drm]] OBJ ID: 82 (3)
[ 2908.638650] [drm:drm_mode_object_put.part.2 [drm]] OBJ ID: 114 (2)
[ 2908.638656] [drm:drm_mode_object_put.part.2 [drm]] OBJ ID: 108 (2)
[ 2908.638663] [drm:__drm_atomic_state_free [drm]] Freeing atomic state 000000007155ba49
[ 2908.638669] [drm:drm_mode_object_put.part.2 [drm]] OBJ ID: 82 (2)
[ 2908.638676] [drm:drm_ioctl [drm]] pid=12928, ret = -22

While this doesn't usually result in any errors that would be obvious to
the user, it does result in us leaving display resources on. This in
turn leads to unwanted sideaffects like inactive GPUs being left on
(usually from the resulting leaked runtime PM ref).

So, provide an easier way of doing this that doesn't require breaking
->best_encoder(): add a common drm_dp_mst_connector_atomic_check()
function that DRM drivers can call in order to have CRTC enabling
commits fail automatically if the MST port driving the connector no
longer exists. We'll also be able to expand upon this later as well once
we add MST fallback retraining support.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 76 +++++++++++++++++++++++++++
 include/drm/drm_dp_mst_helper.h       |  3 ++
 2 files changed, 79 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 7780567aa669..0162d4bf2549 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -3129,6 +3129,82 @@ static const struct drm_private_state_funcs mst_state_funcs = {
 	.atomic_destroy_state = drm_dp_mst_destroy_state,
 };
 
+static bool
+drm_dp_mst_connector_still_exists(struct drm_connector *connector,
+				  struct drm_dp_mst_topology_mgr *mgr,
+				  struct drm_dp_mst_branch *mstb)
+{
+	struct drm_dp_mst_port *port;
+	bool exists = false;
+
+	mstb = drm_dp_get_validated_mstb_ref(mgr, mstb);
+	if (!mstb)
+		return false;
+
+	list_for_each_entry(port, &mstb->ports, next) {
+		port = drm_dp_get_validated_port_ref(mgr, port);
+		if (!port)
+			continue;
+
+		exists = (port->connector == connector ||
+			  (port->mstb &&
+			   drm_dp_mst_connector_still_exists(connector, mgr,
+							     port->mstb)));
+
+		drm_dp_put_port(port);
+		if (exists)
+			break;
+	}
+
+	drm_dp_put_mst_branch_device(mstb);
+	return exists;
+}
+
+/**
+ * drm_dp_mst_connector_atomic_check - Helper for validating a new atomic
+ *                                     state on an MST connector
+ * @connector: drm connector
+ * @connector_state: the new atomic state of @connector
+ * @mgr: the MST topology mgr for @connector
+ *
+ * This function performs various atomic checks that apply to all drivers
+ * using the DRM DP MST helpers. This should be called by all drivers at the
+ * start of the atomic_check function for their MST connectors.
+ *
+ * Return 0 for success, or negative error code on failure.
+ */
+int
+drm_dp_mst_connector_atomic_check(struct drm_connector *connector,
+				  struct drm_connector_state *connector_state,
+				  struct drm_dp_mst_topology_mgr *mgr)
+{
+	struct drm_atomic_state *state = connector_state->state;
+	struct drm_crtc *crtc = connector_state->crtc;
+	struct drm_crtc_state *new_crtc_state;
+
+	if (!crtc)
+		return 0;
+
+	new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+	if (!new_crtc_state)
+		return 0;
+
+	if (!drm_atomic_crtc_needs_modeset(new_crtc_state) ||
+	    !new_crtc_state->active)
+		return 0;
+
+	/* Make sure that the port for this MST connector still exists */
+	if (!drm_dp_mst_connector_still_exists(connector, mgr,
+					       mgr->mst_primary)) {
+		DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] has disappeared from the MST topology\n",
+				 connector->base.id, connector->name);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_dp_mst_connector_atomic_check);
+
 /**
  * drm_atomic_get_mst_topology_state: get MST topology state
  *
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 7f78d26a0766..8e33c2c85d1e 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -625,6 +625,9 @@ void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr);
 int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr);
 struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
 								    struct drm_dp_mst_topology_mgr *mgr);
+int drm_dp_mst_connector_atomic_check(struct drm_connector *connector,
+				      struct drm_connector_state *connector_state,
+				      struct drm_dp_mst_topology_mgr *mgr);
 int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,
 				  struct drm_dp_mst_topology_mgr *mgr,
 				  struct drm_dp_mst_port *port, int pbn);
-- 
2.17.1

Re: [PATCH 1/6] drm/dp_mst: Introduce drm_dp_mst_connector_atomic_check()

[Sasha Levin]

Hi,

[This is an automated email]

This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all

The bot has tested the following trees: v4.18.8, v4.14.70, v4.9.127, v4.4.156, v3.18.122, 

v4.18.8: Build OK!
v4.14.70: Build OK!
v4.9.127: Failed to apply! Possible dependencies:
    3f3353b7e121 ("drm/dp: Introduce MST topology state to track available link bandwidth")
    edb1ed1ab7d3 ("drm/dp: Add DP MST helpers to atomically find and release vcpi slots")

v4.4.156: Failed to apply! Possible dependencies:
    3f3353b7e121 ("drm/dp: Introduce MST topology state to track available link bandwidth")
    edb1ed1ab7d3 ("drm/dp: Add DP MST helpers to atomically find and release vcpi slots")

v3.18.122: Failed to apply! Possible dependencies:
    3f3353b7e121 ("drm/dp: Introduce MST topology state to track available link bandwidth")
    edb1ed1ab7d3 ("drm/dp: Add DP MST helpers to atomically find and release vcpi slots")


Please let us know how to resolve this.

--
Thanks,
Sasha

Re: [PATCH 1/6] drm/dp_mst: Introduce drm_dp_mst_connector_atomic_check()

[Lyude Paul]

On Wed, 2018-09-19 at 18:58 +0000, Sasha Levin wrote:
> Hi,
> 
> [This is an automated email]
> 
> This commit has been processed because it contains a -stable tag.
> The stable tag indicates that it's relevant for the following trees: all
> 
> The bot has tested the following trees: v4.18.8, v4.14.70, v4.9.127, v4.4.156,
> v3.18.122, 
> 
> v4.18.8: Build OK!
> v4.14.70: Build OK!
> v4.9.127: Failed to apply! Possible dependencies:
>     3f3353b7e121 ("drm/dp: Introduce MST topology state to track available
> link bandwidth")
>     edb1ed1ab7d3 ("drm/dp: Add DP MST helpers to atomically find and release
> vcpi slots")
> 
> v4.4.156: Failed to apply! Possible dependencies:
>     3f3353b7e121 ("drm/dp: Introduce MST topology state to track available
> link bandwidth")
>     edb1ed1ab7d3 ("drm/dp: Add DP MST helpers to atomically find and release
> vcpi slots")
> 
> v3.18.122: Failed to apply! Possible dependencies:
>     3f3353b7e121 ("drm/dp: Introduce MST topology state to track available
> link bandwidth")
>     edb1ed1ab7d3 ("drm/dp: Add DP MST helpers to atomically find and release
> vcpi slots")
> 
> 
> Please let us know how to resolve this.
...is this the email address I'm supposed to "let you know how to resolve this"
with? if that is the case, it's 100% OK to simply ignore all of the versions
that don't apply with this patch.

> 
> --
> Thanks,
> Sasha

Re: [PATCH 1/6] drm/dp_mst: Introduce drm_dp_mst_connector_atomic_check()

[Dan Carpenter]

Hi Lyude,

Thank you for the patch! Perhaps something to improve:

url:    https://github.com/0day-ci/linux/commits/Lyude-Paul/Fix-legacy-DPMS-changes-with-MST/20180919-203434
base:   git://anongit.freedesktop.org/drm-intel for-linux-next

smatch warnings:
drivers/gpu/drm/drm_dp_mst_topology.c:3144 drm_dp_mst_connector_still_exists() error: we previously assumed 'port' could be null (see line 3146)

# https://github.com/0day-ci/linux/commit/f8df31d5221b9a6da6698d4a37e622253bb17cdc
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout f8df31d5221b9a6da6698d4a37e622253bb17cdc
vim +/port +3144 drivers/gpu/drm/drm_dp_mst_topology.c

3f3353b7 Pandiyan, Dhinakaran 2017-04-20  3131  
f8df31d5 Lyude Paul           2018-09-18  3132  static bool
f8df31d5 Lyude Paul           2018-09-18  3133  drm_dp_mst_connector_still_exists(struct drm_connector *connector,
f8df31d5 Lyude Paul           2018-09-18  3134  				  struct drm_dp_mst_topology_mgr *mgr,
f8df31d5 Lyude Paul           2018-09-18  3135  				  struct drm_dp_mst_branch *mstb)
f8df31d5 Lyude Paul           2018-09-18  3136  {
f8df31d5 Lyude Paul           2018-09-18  3137  	struct drm_dp_mst_port *port;
f8df31d5 Lyude Paul           2018-09-18  3138  	bool exists = false;
f8df31d5 Lyude Paul           2018-09-18  3139  
f8df31d5 Lyude Paul           2018-09-18  3140  	mstb = drm_dp_get_validated_mstb_ref(mgr, mstb);
f8df31d5 Lyude Paul           2018-09-18  3141  	if (!mstb)
f8df31d5 Lyude Paul           2018-09-18  3142  		return false;
f8df31d5 Lyude Paul           2018-09-18  3143  
f8df31d5 Lyude Paul           2018-09-18 @3144  	list_for_each_entry(port, &mstb->ports, next) {
                                                                            ^^^^
We need to use a different loop iterator, or possibly
list_for_each_entry_safe() because it looks like we're freeing
something.  Maybe it's cleanest to do both.

f8df31d5 Lyude Paul           2018-09-18  3145  		port = drm_dp_get_validated_port_ref(mgr, port);
f8df31d5 Lyude Paul           2018-09-18 @3146  		if (!port)
f8df31d5 Lyude Paul           2018-09-18  3147  			continue;
f8df31d5 Lyude Paul           2018-09-18  3148  
f8df31d5 Lyude Paul           2018-09-18  3149  		exists = (port->connector == connector ||
f8df31d5 Lyude Paul           2018-09-18  3150  			  (port->mstb &&
f8df31d5 Lyude Paul           2018-09-18  3151  			   drm_dp_mst_connector_still_exists(connector, mgr,
f8df31d5 Lyude Paul           2018-09-18  3152  							     port->mstb)));
f8df31d5 Lyude Paul           2018-09-18  3153  
f8df31d5 Lyude Paul           2018-09-18  3154  		drm_dp_put_port(port);
f8df31d5 Lyude Paul           2018-09-18  3155  		if (exists)
f8df31d5 Lyude Paul           2018-09-18  3156  			break;
f8df31d5 Lyude Paul           2018-09-18  3157  	}
f8df31d5 Lyude Paul           2018-09-18  3158  
f8df31d5 Lyude Paul           2018-09-18  3159  	drm_dp_put_mst_branch_device(mstb);
f8df31d5 Lyude Paul           2018-09-18  3160  	return exists;
f8df31d5 Lyude Paul           2018-09-18  3161  }
f8df31d5 Lyude Paul           2018-09-18  3162  

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH 1/6] drm/dp_mst: Introduce drm_dp_mst_connector_atomic_check()

[Harry Wentland]

On 2018-09-18 07:06 PM, Lyude Paul wrote:
> Currently the way that we prevent userspace from performing new modesets
> on MST connectors that have just been destroyed is rather broken.
> There's nothing in the actual DRM DP MST topology helpers that checks
> whether or not a connector still exists, instead each DRM driver does
> this on it's own, usually by returning NULL from the best_encoder
> callback which in turn, causes the atomic commit to fail.
> 
> However, this is wrong in a rather subtle way. If ->best_encoder()
> returns NULL, this makes ALL modesets involving the connector fail. This
> includes modesets from userspace that would shut off the CRTCs being
> used by the connector. Since this results in blocking any changes to a
> connector's DPMS prop, it has the sideaffect of preventing legacy
> modesetting users from ever disabling a CRTC that was previously enabled
> for use in an MST topology. An example of this, where X tries to
> change the DPMS property of an MST connector that was just detached from
> the system:
> 
> [ 2908.320131] [drm:drm_helper_probe_single_connector_modes [drm_kms_helper]] [CONNECTOR:82:DP-6]
> [ 2908.320148] [drm:drm_helper_probe_single_connector_modes [drm_kms_helper]] [CONNECTOR:82:DP-6] status updated from connected to disconnected
> [ 2908.320166] [drm:drm_helper_probe_single_connector_modes [drm_kms_helper]] [CONNECTOR:82:DP-6] disconnected
> [ 2908.320193] [drm:drm_mode_object_put.part.2 [drm]] OBJ ID: 111 (1)
> [ 2908.320230] [drm:drm_sysfs_hotplug_event [drm]] generating hotplug event
> ...
> [ 2908.638539] [drm:drm_ioctl [drm]] pid=12928, dev=0xe201, auth=1, DRM_IOCTL_MODE_SETPROPERTY
> [ 2908.638546] [drm:drm_atomic_state_init [drm]] Allocated atomic state 000000007155ba49
> [ 2908.638553] [drm:drm_mode_object_get [drm]] OBJ ID: 114 (1)
> [ 2908.638560] [drm:drm_mode_object_get [drm]] OBJ ID: 108 (1)
> [ 2908.638568] [drm:drm_atomic_get_crtc_state [drm]] Added [CRTC:41:head-0] 0000000097a6396e state to 000000007155ba49
> [ 2908.638575] [drm:drm_atomic_add_affected_connectors [drm]] Adding all current connectors for [CRTC:41:head-0] to 000000007155ba49
> [ 2908.638582] [drm:drm_mode_object_get [drm]] OBJ ID: 82 (3)
> [ 2908.638589] [drm:drm_mode_object_get [drm]] OBJ ID: 82 (4)
> [ 2908.638596] [drm:drm_atomic_get_connector_state [drm]] Added [CONNECTOR:82:DP-6] 0000000087427144 state to 000000007155ba49
> [ 2908.638603] [drm:drm_atomic_check_only [drm]] checking 000000007155ba49
> [ 2908.638609] [drm:drm_atomic_helper_check_modeset [drm_kms_helper]] [CRTC:41:head-0] active changed
> [ 2908.638613] [drm:drm_atomic_helper_check_modeset [drm_kms_helper]] Updating routing for [CONNECTOR:82:DP-6]
> [ 2908.638616] [drm:drm_atomic_helper_check_modeset [drm_kms_helper]] No suitable encoder found for [CONNECTOR:82:DP-6]
> [ 2908.638623] [drm:drm_atomic_check_only [drm]] atomic driver check for 000000007155ba49 failed: -22
> [ 2908.638630] [drm:drm_atomic_state_default_clear [drm]] Clearing atomic state 000000007155ba49
> [ 2908.638637] [drm:drm_mode_object_put.part.2 [drm]] OBJ ID: 82 (4)
> [ 2908.638643] [drm:drm_mode_object_put.part.2 [drm]] OBJ ID: 82 (3)
> [ 2908.638650] [drm:drm_mode_object_put.part.2 [drm]] OBJ ID: 114 (2)
> [ 2908.638656] [drm:drm_mode_object_put.part.2 [drm]] OBJ ID: 108 (2)
> [ 2908.638663] [drm:__drm_atomic_state_free [drm]] Freeing atomic state 000000007155ba49
> [ 2908.638669] [drm:drm_mode_object_put.part.2 [drm]] OBJ ID: 82 (2)
> [ 2908.638676] [drm:drm_ioctl [drm]] pid=12928, ret = -22
> 
> While this doesn't usually result in any errors that would be obvious to
> the user, it does result in us leaving display resources on. This in
> turn leads to unwanted sideaffects like inactive GPUs being left on
> (usually from the resulting leaked runtime PM ref).
> 
> So, provide an easier way of doing this that doesn't require breaking
> ->best_encoder(): add a common drm_dp_mst_connector_atomic_check()
> function that DRM drivers can call in order to have CRTC enabling
> commits fail automatically if the MST port driving the connector no
> longer exists. We'll also be able to expand upon this later as well once
> we add MST fallback retraining support.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Cc: stable@vger.kernel.org

This does seem like a saner way to handle the case when the MST connector is gone. As this doesn't currently seem to affect amdgpu directly and I therefore might miss something I'll leave the RB to someone else, but you have my
Acked-by: Harry Wentland <harry.wentland@amd.com>

Harry

> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 76 +++++++++++++++++++++++++++
>  include/drm/drm_dp_mst_helper.h       |  3 ++
>  2 files changed, 79 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 7780567aa669..0162d4bf2549 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -3129,6 +3129,82 @@ static const struct drm_private_state_funcs mst_state_funcs = {
>  	.atomic_destroy_state = drm_dp_mst_destroy_state,
>  };
>  
> +static bool
> +drm_dp_mst_connector_still_exists(struct drm_connector *connector,
> +				  struct drm_dp_mst_topology_mgr *mgr,
> +				  struct drm_dp_mst_branch *mstb)
> +{
> +	struct drm_dp_mst_port *port;
> +	bool exists = false;
> +
> +	mstb = drm_dp_get_validated_mstb_ref(mgr, mstb);
> +	if (!mstb)
> +		return false;
> +
> +	list_for_each_entry(port, &mstb->ports, next) {
> +		port = drm_dp_get_validated_port_ref(mgr, port);
> +		if (!port)
> +			continue;
> +
> +		exists = (port->connector == connector ||
> +			  (port->mstb &&
> +			   drm_dp_mst_connector_still_exists(connector, mgr,
> +							     port->mstb)));
> +
> +		drm_dp_put_port(port);
> +		if (exists)
> +			break;
> +	}
> +
> +	drm_dp_put_mst_branch_device(mstb);
> +	return exists;
> +}
> +
> +/**
> + * drm_dp_mst_connector_atomic_check - Helper for validating a new atomic
> + *                                     state on an MST connector
> + * @connector: drm connector
> + * @connector_state: the new atomic state of @connector
> + * @mgr: the MST topology mgr for @connector
> + *
> + * This function performs various atomic checks that apply to all drivers
> + * using the DRM DP MST helpers. This should be called by all drivers at the
> + * start of the atomic_check function for their MST connectors.
> + *
> + * Return 0 for success, or negative error code on failure.
> + */
> +int
> +drm_dp_mst_connector_atomic_check(struct drm_connector *connector,
> +				  struct drm_connector_state *connector_state,
> +				  struct drm_dp_mst_topology_mgr *mgr)
> +{
> +	struct drm_atomic_state *state = connector_state->state;
> +	struct drm_crtc *crtc = connector_state->crtc;
> +	struct drm_crtc_state *new_crtc_state;
> +
> +	if (!crtc)
> +		return 0;
> +
> +	new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +	if (!new_crtc_state)
> +		return 0;
> +
> +	if (!drm_atomic_crtc_needs_modeset(new_crtc_state) ||
> +	    !new_crtc_state->active)
> +		return 0;
> +
> +	/* Make sure that the port for this MST connector still exists */
> +	if (!drm_dp_mst_connector_still_exists(connector, mgr,
> +					       mgr->mst_primary)) {
> +		DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] has disappeared from the MST topology\n",
> +				 connector->base.id, connector->name);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_dp_mst_connector_atomic_check);
> +
>  /**
>   * drm_atomic_get_mst_topology_state: get MST topology state
>   *
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index 7f78d26a0766..8e33c2c85d1e 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -625,6 +625,9 @@ void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr);
>  int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr);
>  struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
>  								    struct drm_dp_mst_topology_mgr *mgr);
> +int drm_dp_mst_connector_atomic_check(struct drm_connector *connector,
> +				      struct drm_connector_state *connector_state,
> +				      struct drm_dp_mst_topology_mgr *mgr);
>  int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,
>  				  struct drm_dp_mst_topology_mgr *mgr,
>  				  struct drm_dp_mst_port *port, int pbn);
> 

[PATCH 2/6] drm/nouveau: Unbreak nv50_mstc->best_encoder()

[Lyude Paul]

As mentioned in the previous commit, we currently prevent new modesets
on recently-removed MST connectors by returning no encoder from our
->best_encoder() callback once the MST port has disappeared. This is
wrong however, because it prevents legacy modesetting users from being
able to disable CRTCs on MST connectors after the connector's respective
topology has disappeared.

So, fix this by instead using the new
drm_dp_mst_connector_atomic_check() helper instead while always
returning a valid encoder from ->best_encoder().

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/nouveau/dispnv50/disp.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 9da0bdfe1e1c..8d6f6ee9bc75 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -881,22 +881,16 @@ nv50_mstc_atomic_best_encoder(struct drm_connector *connector,
 {
 	struct nv50_head *head = nv50_head(connector_state->crtc);
 	struct nv50_mstc *mstc = nv50_mstc(connector);
-	if (mstc->port) {
-		struct nv50_mstm *mstm = mstc->mstm;
-		return &mstm->msto[head->base.index]->encoder;
-	}
-	return NULL;
+
+	return &mstc->mstm->msto[head->base.index]->encoder;
 }
 
 static struct drm_encoder *
 nv50_mstc_best_encoder(struct drm_connector *connector)
 {
 	struct nv50_mstc *mstc = nv50_mstc(connector);
-	if (mstc->port) {
-		struct nv50_mstm *mstm = mstc->mstm;
-		return &mstm->msto[0]->encoder;
-	}
-	return NULL;
+
+	return &mstc->mstm->msto[0]->encoder;
 }
 
 static enum drm_mode_status
@@ -926,10 +920,21 @@ nv50_mstc_get_modes(struct drm_connector *connector)
 	return ret;
 }
 
+static int
+nv50_mstc_atomic_check(struct drm_connector *connector,
+		       struct drm_connector_state *state)
+{
+	struct nv50_mstc *mstc = nv50_mstc(connector);
+
+	return drm_dp_mst_connector_atomic_check(connector, state,
+						 &mstc->mstm->mgr);
+}
+
 static const struct drm_connector_helper_funcs
 nv50_mstc_help = {
 	.get_modes = nv50_mstc_get_modes,
 	.mode_valid = nv50_mstc_mode_valid,
+	.atomic_check = nv50_mstc_atomic_check,
 	.best_encoder = nv50_mstc_best_encoder,
 	.atomic_best_encoder = nv50_mstc_atomic_best_encoder,
 };
-- 
2.17.1

[PATCH 3/6] drm/i915: Leave intel_conn->mst_port set, use mst_port_gone instead

[Lyude Paul]

Currently we set intel_connector->mst_port to NULL to signify that the
MST port has been removed from the system so that we can prevent further
action on the port such as connector probes, mode probing, etc.
However, we're going to need access to intel_connector->mst_port in
order to fixup ->best_encoder() so that it can always return the correct
encoder for an MST port to prevent legacy DPMS prop changes from
failing. This should be safe, so instead keep intel_connector->mst_port
always set and instead add intel_connector->mst_port_gone in order to
signify whether or not the connector has disappeared from the system.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 14 +++++++-------
 drivers/gpu/drm/i915/intel_drv.h    |  1 +
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 4ecd65375603..fcb9b87b9339 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -311,9 +311,8 @@ static int intel_dp_mst_get_ddc_modes(struct drm_connector *connector)
 	struct edid *edid;
 	int ret;
 
-	if (!intel_dp) {
+	if (intel_connector->mst_port_gone)
 		return intel_connector_update_modes(connector, NULL);
-	}
 
 	edid = drm_dp_mst_get_edid(connector, &intel_dp->mst_mgr, intel_connector->port);
 	ret = intel_connector_update_modes(connector, edid);
@@ -328,9 +327,10 @@ intel_dp_mst_detect(struct drm_connector *connector, bool force)
 	struct intel_connector *intel_connector = to_intel_connector(connector);
 	struct intel_dp *intel_dp = intel_connector->mst_port;
 
-	if (!intel_dp)
+	if (intel_connector->mst_port_gone)
 		return connector_status_disconnected;
-	return drm_dp_mst_detect_port(connector, &intel_dp->mst_mgr, intel_connector->port);
+	return drm_dp_mst_detect_port(connector, &intel_dp->mst_mgr,
+				      intel_connector->port);
 }
 
 static void
@@ -370,7 +370,7 @@ intel_dp_mst_mode_valid(struct drm_connector *connector,
 	int bpp = 24; /* MST uses fixed bpp */
 	int max_rate, mode_rate, max_lanes, max_link_clock;
 
-	if (!intel_dp)
+	if (intel_connector->mst_port_gone)
 		return MODE_ERROR;
 
 	if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
@@ -402,7 +402,7 @@ static struct drm_encoder *intel_mst_atomic_best_encoder(struct drm_connector *c
 	struct intel_dp *intel_dp = intel_connector->mst_port;
 	struct intel_crtc *crtc = to_intel_crtc(state->crtc);
 
-	if (!intel_dp)
+	if (intel_connector->mst_port_gone)
 		return NULL;
 	return &intel_dp->mst_encoders[crtc->pipe]->base.base;
 }
@@ -514,7 +514,7 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 						   connector);
 	/* prevent race with the check in ->detect */
 	drm_modeset_lock(&connector->dev->mode_config.connection_mutex, NULL);
-	intel_connector->mst_port = NULL;
+	intel_connector->mst_port_gone = true;
 	drm_modeset_unlock(&connector->dev->mode_config.connection_mutex);
 
 	drm_connector_put(connector);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 8fc61e96754f..87ce772ae7f8 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -409,6 +409,7 @@ struct intel_connector {
 	void *port; /* store this opaque as its illegal to dereference it */
 
 	struct intel_dp *mst_port;
+	bool mst_port_gone;
 
 	/* Work struct to schedule a uevent on link train failure */
 	struct work_struct modeset_retry_work;
-- 
2.17.1

Re: [PATCH 3/6] drm/i915: Leave intel_conn->mst_port set, use mst_port_gone instead

[Sasha Levin]

Hi,

[This is an automated email]

This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all

The bot has tested the following trees: v4.18.8, v4.14.70, v4.9.127, v4.4.156, v3.18.122, 

v4.18.8: Build OK!
v4.14.70: Build OK!
v4.9.127: Failed to apply! Possible dependencies:
    06bfe5b0d892 ("drm/i915: Avoid null dereference if mst_port is unset.")
    22a2c8e0457f ("drm/i915: Validate mode against max. link data rate for DP MST")
    9301397a63b3 ("drm/i915: Implement Link Rate fallback on Link training failure")
    af2405af07d1 ("drm/fb-helper: Push down modeset lock into FB helpers")

v4.4.156: Failed to apply! Possible dependencies:
    0552f7651bc2 ("drm/i915/mst: use reference counted connectors. (v3)")
    06bfe5b0d892 ("drm/i915: Avoid null dereference if mst_port is unset.")
    22a2c8e0457f ("drm/i915: Validate mode against max. link data rate for DP MST")
    9301397a63b3 ("drm/i915: Implement Link Rate fallback on Link training failure")
    af2405af07d1 ("drm/fb-helper: Push down modeset lock into FB helpers")

v3.18.122: Failed to apply! Possible dependencies:
    0552f7651bc2 ("drm/i915/mst: use reference counted connectors. (v3)")
    06bfe5b0d892 ("drm/i915: Avoid null dereference if mst_port is unset.")
    22a2c8e0457f ("drm/i915: Validate mode against max. link data rate for DP MST")
    459485ad3513 ("drm/i915: Fixup dp mst encoder selection")
    9301397a63b3 ("drm/i915: Implement Link Rate fallback on Link training failure")
    af2405af07d1 ("drm/fb-helper: Push down modeset lock into FB helpers")
    c6a0aed4d493 ("drm/mst: cached EDID for logical ports (v2)")


Please let us know how to resolve this.

--
Thanks,
Sasha

Re: [PATCH 3/6] drm/i915: Leave intel_conn->mst_port set, use mst_port_gone instead

[Lyude Paul]

On Wed, 2018-09-19 at 18:58 +0000, Sasha Levin wrote:
> Hi,
> 
> [This is an automated email]
> 
> This commit has been processed because it contains a -stable tag.
> The stable tag indicates that it's relevant for the following trees: all
> 
> The bot has tested the following trees: v4.18.8, v4.14.70, v4.9.127, v4.4.156,
> v3.18.122, 
> 
> v4.18.8: Build OK!
> v4.14.70: Build OK!
> v4.9.127: Failed to apply! Possible dependencies:
>     06bfe5b0d892 ("drm/i915: Avoid null dereference if mst_port is unset.")
>     22a2c8e0457f ("drm/i915: Validate mode against max. link data rate for DP
> MST")
>     9301397a63b3 ("drm/i915: Implement Link Rate fallback on Link training
> failure")
>     af2405af07d1 ("drm/fb-helper: Push down modeset lock into FB helpers")
> 
> v4.4.156: Failed to apply! Possible dependencies:
>     0552f7651bc2 ("drm/i915/mst: use reference counted connectors. (v3)")
>     06bfe5b0d892 ("drm/i915: Avoid null dereference if mst_port is unset.")
>     22a2c8e0457f ("drm/i915: Validate mode against max. link data rate for DP
> MST")
>     9301397a63b3 ("drm/i915: Implement Link Rate fallback on Link training
> failure")
>     af2405af07d1 ("drm/fb-helper: Push down modeset lock into FB helpers")
> 
> v3.18.122: Failed to apply! Possible dependencies:
>     0552f7651bc2 ("drm/i915/mst: use reference counted connectors. (v3)")
>     06bfe5b0d892 ("drm/i915: Avoid null dereference if mst_port is unset.")
>     22a2c8e0457f ("drm/i915: Validate mode against max. link data rate for DP
> MST")
>     459485ad3513 ("drm/i915: Fixup dp mst encoder selection")
>     9301397a63b3 ("drm/i915: Implement Link Rate fallback on Link training
> failure")
>     af2405af07d1 ("drm/fb-helper: Push down modeset lock into FB helpers")
>     c6a0aed4d493 ("drm/mst: cached EDID for logical ports (v2)")
> 
> 
> Please let us know how to resolve this.

...is this the email address I'm supposed to "let you know how to resolve this"
with? if that is the case, it's 100% OK to simply ignore all of the versions
that don't apply with this patch.
> 
> --
> Thanks,
> Sasha

Re: [Intel-gfx] [PATCH 3/6] drm/i915: Leave intel_conn->mst_port set, use mst_port_gone instead

[Daniel Vetter]

On Tue, Sep 18, 2018 at 07:06:19PM -0400, Lyude Paul wrote:
> Currently we set intel_connector->mst_port to NULL to signify that the
> MST port has been removed from the system so that we can prevent further
> action on the port such as connector probes, mode probing, etc.
> However, we're going to need access to intel_connector->mst_port in
> order to fixup ->best_encoder() so that it can always return the correct
> encoder for an MST port to prevent legacy DPMS prop changes from
> failing. This should be safe, so instead keep intel_connector->mst_port
> always set and instead add intel_connector->mst_port_gone in order to
> signify whether or not the connector has disappeared from the system.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Cc: stable@vger.kernel.org

Hm, how exactly do legacy DPMS setprop blow up here? Or at least I can't
come up with a scenario that's specific to legacy props, atomic should be
equally affected. By the time we land in this code, the core code already
remapping it to be purely an atomic update.

Another thought: Should we handle at least some of this in the probe
helpers? I.e. once you unplugged a drm_connector, it always shows
disconnected and mode list is gone, instead of duplicating this over all
drivers. We could just check connector->registered.

Thanks, Daniel
> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 14 +++++++-------
>  drivers/gpu/drm/i915/intel_drv.h    |  1 +
>  2 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 4ecd65375603..fcb9b87b9339 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -311,9 +311,8 @@ static int intel_dp_mst_get_ddc_modes(struct drm_connector *connector)
>  	struct edid *edid;
>  	int ret;
>  
> -	if (!intel_dp) {
> +	if (intel_connector->mst_port_gone)
>  		return intel_connector_update_modes(connector, NULL);
> -	}
>  
>  	edid = drm_dp_mst_get_edid(connector, &intel_dp->mst_mgr, intel_connector->port);
>  	ret = intel_connector_update_modes(connector, edid);
> @@ -328,9 +327,10 @@ intel_dp_mst_detect(struct drm_connector *connector, bool force)
>  	struct intel_connector *intel_connector = to_intel_connector(connector);
>  	struct intel_dp *intel_dp = intel_connector->mst_port;
>  
> -	if (!intel_dp)
> +	if (intel_connector->mst_port_gone)
>  		return connector_status_disconnected;
> -	return drm_dp_mst_detect_port(connector, &intel_dp->mst_mgr, intel_connector->port);
> +	return drm_dp_mst_detect_port(connector, &intel_dp->mst_mgr,
> +				      intel_connector->port);
>  }
>  
>  static void
> @@ -370,7 +370,7 @@ intel_dp_mst_mode_valid(struct drm_connector *connector,
>  	int bpp = 24; /* MST uses fixed bpp */
>  	int max_rate, mode_rate, max_lanes, max_link_clock;
>  
> -	if (!intel_dp)
> +	if (intel_connector->mst_port_gone)
>  		return MODE_ERROR;
>  
>  	if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
> @@ -402,7 +402,7 @@ static struct drm_encoder *intel_mst_atomic_best_encoder(struct drm_connector *c
>  	struct intel_dp *intel_dp = intel_connector->mst_port;
>  	struct intel_crtc *crtc = to_intel_crtc(state->crtc);
>  
> -	if (!intel_dp)
> +	if (intel_connector->mst_port_gone)
>  		return NULL;
>  	return &intel_dp->mst_encoders[crtc->pipe]->base.base;
>  }
> @@ -514,7 +514,7 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
>  						   connector);
>  	/* prevent race with the check in ->detect */
>  	drm_modeset_lock(&connector->dev->mode_config.connection_mutex, NULL);
> -	intel_connector->mst_port = NULL;
> +	intel_connector->mst_port_gone = true;
>  	drm_modeset_unlock(&connector->dev->mode_config.connection_mutex);
>  
>  	drm_connector_put(connector);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 8fc61e96754f..87ce772ae7f8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -409,6 +409,7 @@ struct intel_connector {
>  	void *port; /* store this opaque as its illegal to dereference it */
>  
>  	struct intel_dp *mst_port;
> +	bool mst_port_gone;
>  
>  	/* Work struct to schedule a uevent on link train failure */
>  	struct work_struct modeset_retry_work;
> -- 
> 2.17.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 3/6] drm/i915: Leave intel_conn->mst_port set, use mst_port_gone instead

[Lyude Paul]

On Fri, 2018-09-21 at 11:27 +0200, Daniel Vetter wrote:
> On Tue, Sep 18, 2018 at 07:06:19PM -0400, Lyude Paul wrote:
> > Currently we set intel_connector->mst_port to NULL to signify that the
> > MST port has been removed from the system so that we can prevent further
> > action on the port such as connector probes, mode probing, etc.
> > However, we're going to need access to intel_connector->mst_port in
> > order to fixup ->best_encoder() so that it can always return the correct
> > encoder for an MST port to prevent legacy DPMS prop changes from
> > failing. This should be safe, so instead keep intel_connector->mst_port
> > always set and instead add intel_connector->mst_port_gone in order to
> > signify whether or not the connector has disappeared from the system.
> > 
> > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > Cc: stable@vger.kernel.org
> 
> Hm, how exactly do legacy DPMS setprop blow up here? Or at least I can't
> come up with a scenario that's specific to legacy props, atomic should be
> equally affected. By the time we land in this code, the core code already
> remapping it to be purely an atomic update.
So, what I've been seeing with X that's been blowing this up:
 * Hotplug event gets received, X does reprobe
 * Notices MST connectors are now disconnected, sets DPMS from on->off
 * During atomic_check, drm_atomic_helper_check_modeset() is called
 * update_connector_routing() gets called
 * funcs->best_encoder() returns NULL for the encoder
 * update_connector_routing() fails atomic commit with "No suitable encoder
   found", line 320 of drm_atomic_helper
> 
> Another thought: Should we handle at least some of this in the probe
> helpers? I.e. once you unplugged a drm_connector, it always shows
> disconnected and mode list is gone, instead of duplicating this over all
> drivers. We could just check connector->registered.
oooh, good idea! I will certainly go do that
> 
> Thanks, Daniel
> > ---
> >  drivers/gpu/drm/i915/intel_dp_mst.c | 14 +++++++-------
> >  drivers/gpu/drm/i915/intel_drv.h    |  1 +
> >  2 files changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c
> > b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index 4ecd65375603..fcb9b87b9339 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -311,9 +311,8 @@ static int intel_dp_mst_get_ddc_modes(struct
> > drm_connector *connector)
> >  	struct edid *edid;
> >  	int ret;
> >  
> > -	if (!intel_dp) {
> > +	if (intel_connector->mst_port_gone)
> >  		return intel_connector_update_modes(connector, NULL);
> > -	}
> >  
> >  	edid = drm_dp_mst_get_edid(connector, &intel_dp->mst_mgr,
> > intel_connector->port);
> >  	ret = intel_connector_update_modes(connector, edid);
> > @@ -328,9 +327,10 @@ intel_dp_mst_detect(struct drm_connector *connector,
> > bool force)
> >  	struct intel_connector *intel_connector =
> > to_intel_connector(connector);
> >  	struct intel_dp *intel_dp = intel_connector->mst_port;
> >  
> > -	if (!intel_dp)
> > +	if (intel_connector->mst_port_gone)
> >  		return connector_status_disconnected;
> > -	return drm_dp_mst_detect_port(connector, &intel_dp->mst_mgr,
> > intel_connector->port);
> > +	return drm_dp_mst_detect_port(connector, &intel_dp->mst_mgr,
> > +				      intel_connector->port);
> >  }
> >  
> >  static void
> > @@ -370,7 +370,7 @@ intel_dp_mst_mode_valid(struct drm_connector
> > *connector,
> >  	int bpp = 24; /* MST uses fixed bpp */
> >  	int max_rate, mode_rate, max_lanes, max_link_clock;
> >  
> > -	if (!intel_dp)
> > +	if (intel_connector->mst_port_gone)
> >  		return MODE_ERROR;
> >  
> >  	if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
> > @@ -402,7 +402,7 @@ static struct drm_encoder
> > *intel_mst_atomic_best_encoder(struct drm_connector *c
> >  	struct intel_dp *intel_dp = intel_connector->mst_port;
> >  	struct intel_crtc *crtc = to_intel_crtc(state->crtc);
> >  
> > -	if (!intel_dp)
> > +	if (intel_connector->mst_port_gone)
> >  		return NULL;
> >  	return &intel_dp->mst_encoders[crtc->pipe]->base.base;
> >  }
> > @@ -514,7 +514,7 @@ static void intel_dp_destroy_mst_connector(struct
> > drm_dp_mst_topology_mgr *mgr,
> >  						   connector);
> >  	/* prevent race with the check in ->detect */
> >  	drm_modeset_lock(&connector->dev->mode_config.connection_mutex, NULL);
> > -	intel_connector->mst_port = NULL;
> > +	intel_connector->mst_port_gone = true;
> >  	drm_modeset_unlock(&connector->dev->mode_config.connection_mutex);
> >  
> >  	drm_connector_put(connector);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 8fc61e96754f..87ce772ae7f8 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -409,6 +409,7 @@ struct intel_connector {
> >  	void *port; /* store this opaque as its illegal to dereference it */
> >  
> >  	struct intel_dp *mst_port;
> > +	bool mst_port_gone;
> >  
> >  	/* Work struct to schedule a uevent on link train failure */
> >  	struct work_struct modeset_retry_work;
> > -- 
> > 2.17.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
-- 
Cheers,
	Lyude Paul

Re: [Intel-gfx] [PATCH 3/6] drm/i915: Leave intel_conn->mst_port set, use mst_port_gone instead

[Daniel Vetter]

On Fri, Sep 21, 2018 at 04:17:16PM -0400, Lyude Paul wrote:
> On Fri, 2018-09-21 at 11:27 +0200, Daniel Vetter wrote:
> > On Tue, Sep 18, 2018 at 07:06:19PM -0400, Lyude Paul wrote:
> > > Currently we set intel_connector->mst_port to NULL to signify that the
> > > MST port has been removed from the system so that we can prevent further
> > > action on the port such as connector probes, mode probing, etc.
> > > However, we're going to need access to intel_connector->mst_port in
> > > order to fixup ->best_encoder() so that it can always return the correct
> > > encoder for an MST port to prevent legacy DPMS prop changes from
> > > failing. This should be safe, so instead keep intel_connector->mst_port
> > > always set and instead add intel_connector->mst_port_gone in order to
> > > signify whether or not the connector has disappeared from the system.
> > > 
> > > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > > Cc: stable@vger.kernel.org
> > 
> > Hm, how exactly do legacy DPMS setprop blow up here? Or at least I can't
> > come up with a scenario that's specific to legacy props, atomic should be
> > equally affected. By the time we land in this code, the core code already
> > remapping it to be purely an atomic update.
> So, what I've been seeing with X that's been blowing this up:
>  * Hotplug event gets received, X does reprobe
>  * Notices MST connectors are now disconnected, sets DPMS from on->off
>  * During atomic_check, drm_atomic_helper_check_modeset() is called
>  * update_connector_routing() gets called
>  * funcs->best_encoder() returns NULL for the encoder
>  * update_connector_routing() fails atomic commit with "No suitable encoder
>    found", line 320 of drm_atomic_helper

Uh ... And X then doesn't try to fully shut down the CRTC? That would be a
SETCRTC with num_connectors=0, mode=0.

The failure mode has nothing to do with legacy dpms, it's just anything
that touches the modeset state will fail (except when you fully turn
things off, not just dpms off). If we fix this, then a DPMS on will
probably also "work", with possibly some hilarious dmesg noise and driver
bugs. So we need to be careful that this only allows disabling, not
re-enabling afterwards.

Hm ... thinking a bit more, I think a full disable also will not quite
work. Definitely need to test that.

> > Another thought: Should we handle at least some of this in the probe
> > helpers? I.e. once you unplugged a drm_connector, it always shows
> > disconnected and mode list is gone, instead of duplicating this over all
> > drivers. We could just check connector->registered.
> oooh, good idea! I will certainly go do that

We'll probably also need some checks in atomic helpers to prevent
re-enabling of a ghost connector.
-Daniel

> > 
> > Thanks, Daniel
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp_mst.c | 14 +++++++-------
> > >  drivers/gpu/drm/i915/intel_drv.h    |  1 +
> > >  2 files changed, 8 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > index 4ecd65375603..fcb9b87b9339 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > @@ -311,9 +311,8 @@ static int intel_dp_mst_get_ddc_modes(struct
> > > drm_connector *connector)
> > >  	struct edid *edid;
> > >  	int ret;
> > >  
> > > -	if (!intel_dp) {
> > > +	if (intel_connector->mst_port_gone)
> > >  		return intel_connector_update_modes(connector, NULL);
> > > -	}
> > >  
> > >  	edid = drm_dp_mst_get_edid(connector, &intel_dp->mst_mgr,
> > > intel_connector->port);
> > >  	ret = intel_connector_update_modes(connector, edid);
> > > @@ -328,9 +327,10 @@ intel_dp_mst_detect(struct drm_connector *connector,
> > > bool force)
> > >  	struct intel_connector *intel_connector =
> > > to_intel_connector(connector);
> > >  	struct intel_dp *intel_dp = intel_connector->mst_port;
> > >  
> > > -	if (!intel_dp)
> > > +	if (intel_connector->mst_port_gone)
> > >  		return connector_status_disconnected;
> > > -	return drm_dp_mst_detect_port(connector, &intel_dp->mst_mgr,
> > > intel_connector->port);
> > > +	return drm_dp_mst_detect_port(connector, &intel_dp->mst_mgr,
> > > +				      intel_connector->port);
> > >  }
> > >  
> > >  static void
> > > @@ -370,7 +370,7 @@ intel_dp_mst_mode_valid(struct drm_connector
> > > *connector,
> > >  	int bpp = 24; /* MST uses fixed bpp */
> > >  	int max_rate, mode_rate, max_lanes, max_link_clock;
> > >  
> > > -	if (!intel_dp)
> > > +	if (intel_connector->mst_port_gone)
> > >  		return MODE_ERROR;
> > >  
> > >  	if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
> > > @@ -402,7 +402,7 @@ static struct drm_encoder
> > > *intel_mst_atomic_best_encoder(struct drm_connector *c
> > >  	struct intel_dp *intel_dp = intel_connector->mst_port;
> > >  	struct intel_crtc *crtc = to_intel_crtc(state->crtc);
> > >  
> > > -	if (!intel_dp)
> > > +	if (intel_connector->mst_port_gone)
> > >  		return NULL;
> > >  	return &intel_dp->mst_encoders[crtc->pipe]->base.base;
> > >  }
> > > @@ -514,7 +514,7 @@ static void intel_dp_destroy_mst_connector(struct
> > > drm_dp_mst_topology_mgr *mgr,
> > >  						   connector);
> > >  	/* prevent race with the check in ->detect */
> > >  	drm_modeset_lock(&connector->dev->mode_config.connection_mutex, NULL);
> > > -	intel_connector->mst_port = NULL;
> > > +	intel_connector->mst_port_gone = true;
> > >  	drm_modeset_unlock(&connector->dev->mode_config.connection_mutex);
> > >  
> > >  	drm_connector_put(connector);
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 8fc61e96754f..87ce772ae7f8 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -409,6 +409,7 @@ struct intel_connector {
> > >  	void *port; /* store this opaque as its illegal to dereference it */
> > >  
> > >  	struct intel_dp *mst_port;
> > > +	bool mst_port_gone;
> > >  
> > >  	/* Work struct to schedule a uevent on link train failure */
> > >  	struct work_struct modeset_retry_work;
> > > -- 
> > > 2.17.1
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > 
> -- 
> Cheers,
> 	Lyude Paul
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 4/6] drm/i915: Skip vcpi allocation for MSTB ports that are gone

[Lyude Paul]

Since we need to be able to allow DPMS on->off prop changes after an MST
port has disappeared from the system, we need to be able to make sure we
can compute a config for the resulting atomic commit. Currently this is
impossible when the port has disappeared, since the VCPI slot searching
we try to do in intel_dp_mst_compute_config() will fail with -EINVAL.

Since the only commits we want to allow on no-longer-present MST ports
are ones that shut off display hardware, we already know that no VCPI
allocations are needed. So, hardcode the VCPI slot count to 0 when
intel_dp_mst_compute_config() is called on an MST port that's gone.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index fcb9b87b9339..a366f32b048a 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -42,7 +42,7 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 		to_intel_connector(conn_state->connector);
 	struct drm_atomic_state *state = pipe_config->base.state;
 	int bpp;
-	int lane_count, slots;
+	int lane_count, slots = 0;
 	const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
 	int mst_pbn;
 	bool reduce_m_n = drm_dp_has_quirk(&intel_dp->desc,
@@ -76,11 +76,16 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 	mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
 	pipe_config->pbn = mst_pbn;
 
-	slots = drm_dp_atomic_find_vcpi_slots(state, &intel_dp->mst_mgr,
-					      connector->port, mst_pbn);
-	if (slots < 0) {
-		DRM_DEBUG_KMS("failed finding vcpi slots:%d\n", slots);
-		return false;
+	if (!connector->mst_port_gone) {
+		slots = drm_dp_atomic_find_vcpi_slots(state,
+						      &intel_dp->mst_mgr,
+						      connector->port,
+						      mst_pbn);
+		if (slots < 0) {
+			DRM_DEBUG_KMS("failed finding vcpi slots:%d\n",
+				      slots);
+			return false;
+		}
 	}
 
 	intel_link_compute_m_n(bpp, lane_count,
-- 
2.17.1

[PATCH 5/6] drm/i915: Fix intel_dp_mst_best_encoder()

[Lyude Paul]

Currently, i915 appears to rely on blocking modesets on
no-longer-present MSTB ports by simply returning NULL for
->best_encoder(), which in turn causes any new atomic commits that don't
disable the CRTC to fail. This is wrong however, since we still want to
allow userspace to disable CRTCs on no-longer-present MSTB ports by
changing the DPMS state to off and this still requires that we retrieve
an encoder.

So, fix this by always returning a valid encoder regardless of the state
of the MST port. Additionally, make intel_dp_mst_atomic_check() simply
rely on drm_dp_mst_connector_atomic_check() to prevent new modesets on
no-longer-present MSTB ports.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index a366f32b048a..2b798d4592f0 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -106,14 +106,21 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 }
 
 static int intel_dp_mst_atomic_check(struct drm_connector *connector,
-		struct drm_connector_state *new_conn_state)
+				     struct drm_connector_state *new_conn_state)
 {
 	struct drm_atomic_state *state = new_conn_state->state;
 	struct drm_connector_state *old_conn_state;
 	struct drm_crtc *old_crtc;
 	struct drm_crtc_state *crtc_state;
+	struct drm_dp_mst_topology_mgr *mgr =
+		&to_intel_connector(connector)->mst_port->mst_mgr;
 	int slots, ret = 0;
 
+	ret = drm_dp_mst_connector_atomic_check(connector, new_conn_state,
+						mgr);
+	if (ret)
+		return ret;
+
 	old_conn_state = drm_atomic_get_old_connector_state(state, connector);
 	old_crtc = old_conn_state->crtc;
 	if (!old_crtc)
@@ -122,12 +129,6 @@ static int intel_dp_mst_atomic_check(struct drm_connector *connector,
 	crtc_state = drm_atomic_get_new_crtc_state(state, old_crtc);
 	slots = to_intel_crtc_state(crtc_state)->dp_m_n.tu;
 	if (drm_atomic_crtc_needs_modeset(crtc_state) && slots > 0) {
-		struct drm_dp_mst_topology_mgr *mgr;
-		struct drm_encoder *old_encoder;
-
-		old_encoder = old_conn_state->best_encoder;
-		mgr = &enc_to_mst(old_encoder)->primary->dp.mst_mgr;
-
 		ret = drm_dp_atomic_release_vcpi_slots(state, mgr, slots);
 		if (ret)
 			DRM_DEBUG_KMS("failed releasing %d vcpi slots:%d\n", slots, ret);
@@ -407,8 +408,6 @@ static struct drm_encoder *intel_mst_atomic_best_encoder(struct drm_connector *c
 	struct intel_dp *intel_dp = intel_connector->mst_port;
 	struct intel_crtc *crtc = to_intel_crtc(state->crtc);
 
-	if (intel_connector->mst_port_gone)
-		return NULL;
 	return &intel_dp->mst_encoders[crtc->pipe]->base.base;
 }
 
-- 
2.17.1