[PATCH v4 0/5] Fix legacy DPMS changes with MST

[PATCH v4 0/5] Fix legacy DPMS changes with MST

[Lyude Paul]

Next version of https://patchwork.freedesktop.org/series/49878/ . No
changes, except that these patches are against master so hopefully
intel's CI doesn't get confused this time.

Lyude Paul (5):
  drm/atomic_helper: Disallow new modesets on unregistered connectors
  drm/nouveau: Fix nv50_mstc->best_encoder()
  drm/i915: Leave intel_conn->mst_port set, use mst_port_gone instead
  drm/i915: Skip vcpi allocation for MSTB ports that are gone
  drm/i915: Fix intel_dp_mst_best_encoder()

 drivers/gpu/drm/drm_atomic_helper.c     | 21 ++++++++++++++++-
 drivers/gpu/drm/i915/intel_dp_mst.c     | 31 ++++++++++++++-----------
 drivers/gpu/drm/i915/intel_drv.h        |  6 +++++
 drivers/gpu/drm/nouveau/dispnv50/disp.c | 14 ++++-------
 4 files changed, 47 insertions(+), 25 deletions(-)

-- 
2.17.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH v4 0/5] Fix legacy DPMS changes with MST

[Lyude Paul]

Next version of https://patchwork.freedesktop.org/series/49878/ . No
changes, except that these patches are against master so hopefully
intel's CI doesn't get confused this time.

Lyude Paul (5):
  drm/atomic_helper: Disallow new modesets on unregistered connectors
  drm/nouveau: Fix nv50_mstc->best_encoder()
  drm/i915: Leave intel_conn->mst_port set, use mst_port_gone instead
  drm/i915: Skip vcpi allocation for MSTB ports that are gone
  drm/i915: Fix intel_dp_mst_best_encoder()

 drivers/gpu/drm/drm_atomic_helper.c     | 21 ++++++++++++++++-
 drivers/gpu/drm/i915/intel_dp_mst.c     | 31 ++++++++++++++-----------
 drivers/gpu/drm/i915/intel_drv.h        |  6 +++++
 drivers/gpu/drm/nouveau/dispnv50/disp.c | 14 ++++-------
 4 files changed, 47 insertions(+), 25 deletions(-)

-- 
2.17.1

[PATCH v4 1/5] drm/atomic_helper: Disallow new modesets on unregistered connectors

[Lyude Paul]

With the exception of modesets which would switch the DPMS state of a
connector from on to off, we want to make sure that we disallow all
modesets which would result in enabling a new monitor or a new mode
configuration on a monitor if the connector for the display in question
is no longer registered. This allows us to stop userspace from trying to
enable new displays on connectors for an MST topology that were just
removed from the system, without preventing userspace from disabling
DPMS on those connectors.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/drm_atomic_helper.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 80be74df7ba6..ce2decfc6826 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -307,6 +307,26 @@ update_connector_routing(struct drm_atomic_state *state,
 		return 0;
 	}
 
+	crtc_state = drm_atomic_get_new_crtc_state(state,
+						   new_connector_state->crtc);
+	/*
+	 * For compatibility with legacy users, we want to make sure that
+	 * we allow DPMS On->Off modesets on unregistered connectors. Modesets
+	 * which would result in anything else must be considered invalid, to
+	 * avoid turning on new displays on dead connectors.
+	 *
+	 * Since the connector can be unregistered at any point during an
+	 * atomic check or commit, this is racy. But that's OK: all we care
+	 * about is ensuring that userspace can't do anything but shut off the
+	 * display on a connector that was destroyed after it's been notified,
+	 * not before.
+	 */
+	if (!READ_ONCE(connector->registered) && crtc_state->active) {
+		DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] is not registered\n",
+				 connector->base.id, connector->name);
+		return -EINVAL;
+	}
+
 	funcs = connector->helper_private;
 
 	if (funcs->atomic_best_encoder)
@@ -351,7 +371,6 @@ update_connector_routing(struct drm_atomic_state *state,
 
 	set_best_encoder(state, new_connector_state, new_encoder);
 
-	crtc_state = drm_atomic_get_new_crtc_state(state, new_connector_state->crtc);
 	crtc_state->connectors_changed = true;
 
 	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d:%s]\n",
-- 
2.17.1

Re: [PATCH v4 1/5] drm/atomic_helper: Disallow new modesets on unregistered connectors

[Daniel Vetter]

On Thu, Oct 04, 2018 at 08:29:50PM -0400, Lyude Paul wrote:
> With the exception of modesets which would switch the DPMS state of a
> connector from on to off, we want to make sure that we disallow all
> modesets which would result in enabling a new monitor or a new mode
> configuration on a monitor if the connector for the display in question
> is no longer registered. This allows us to stop userspace from trying to
> enable new displays on connectors for an MST topology that were just
> removed from the system, without preventing userspace from disabling
> DPMS on those connectors.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Cc: stable@vger.kernel.org

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 80be74df7ba6..ce2decfc6826 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -307,6 +307,26 @@ update_connector_routing(struct drm_atomic_state *state,
>  		return 0;
>  	}
>  
> +	crtc_state = drm_atomic_get_new_crtc_state(state,
> +						   new_connector_state->crtc);
> +	/*
> +	 * For compatibility with legacy users, we want to make sure that
> +	 * we allow DPMS On->Off modesets on unregistered connectors. Modesets
> +	 * which would result in anything else must be considered invalid, to
> +	 * avoid turning on new displays on dead connectors.
> +	 *
> +	 * Since the connector can be unregistered at any point during an
> +	 * atomic check or commit, this is racy. But that's OK: all we care
> +	 * about is ensuring that userspace can't do anything but shut off the
> +	 * display on a connector that was destroyed after it's been notified,
> +	 * not before.
> +	 */
> +	if (!READ_ONCE(connector->registered) && crtc_state->active) {
> +		DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] is not registered\n",
> +				 connector->base.id, connector->name);
> +		return -EINVAL;
> +	}
> +
>  	funcs = connector->helper_private;
>  
>  	if (funcs->atomic_best_encoder)
> @@ -351,7 +371,6 @@ update_connector_routing(struct drm_atomic_state *state,
>  
>  	set_best_encoder(state, new_connector_state, new_encoder);
>  
> -	crtc_state = drm_atomic_get_new_crtc_state(state, new_connector_state->crtc);
>  	crtc_state->connectors_changed = true;
>  
>  	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d:%s]\n",
> -- 
> 2.17.1
> 

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

[PATCH v4 2/5] drm/nouveau: Fix 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 by just always returning a valid encoder.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: stable@vger.kernel.org

Changes since v2:
- Remove usage of atomic MST helper for now, since that got replaced
  with a much simpler solution

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 drivers/gpu/drm/nouveau/dispnv50/disp.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 5691dfa1db6f..63a23a80f279 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -843,22 +843,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
-- 
2.17.1

Re: [PATCH v4 2/5] drm/nouveau: Fix nv50_mstc->best_encoder()

[Daniel Vetter]

On Thu, Oct 04, 2018 at 08:29:51PM -0400, Lyude Paul wrote:
> 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 by just always returning a valid encoder.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Cc: stable@vger.kernel.org
> 
> Changes since v2:
> - Remove usage of atomic MST helper for now, since that got replaced
>   with a much simpler solution
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/nouveau/dispnv50/disp.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index 5691dfa1db6f..63a23a80f279 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -843,22 +843,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
> -- 
> 2.17.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 2/5] drm/nouveau: Fix nv50_mstc->best_encoder()

[Daniel Vetter]

On Thu, Oct 04, 2018 at 08:29:51PM -0400, Lyude Paul wrote:
> 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 by just always returning a valid encoder.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Cc: stable@vger.kernel.org
> 
> Changes since v2:
> - Remove usage of atomic MST helper for now, since that got replaced
>   with a much simpler solution
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/nouveau/dispnv50/disp.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index 5691dfa1db6f..63a23a80f279 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -843,22 +843,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
> -- 
> 2.17.1
> 

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

[PATCH v4 3/5] 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.

Changes since v2:
- Add a comment to mst_port_gone (Jani Nikula)
- Change mst_port_gone to a u8 instead of a bool, per the kernel bot.
  Apparently bool is discouraged in structs these days

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    |  6 ++++++
 2 files changed, 13 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..8261d4579452 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -410,6 +410,12 @@ struct intel_connector {
 
 	struct intel_dp *mst_port;
 
+	/*
+	 * Set to 1 if this is an MST connector that was removed from the
+	 * system and unregistered from sysfs
+	 */
+	u8 mst_port_gone;
+
 	/* Work struct to schedule a uevent on link train failure */
 	struct work_struct modeset_retry_work;
 
-- 
2.17.1

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

[Daniel Vetter]

On Thu, Oct 04, 2018 at 08:29:52PM -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.
> 
> Changes since v2:
> - Add a comment to mst_port_gone (Jani Nikula)
> - Change mst_port_gone to a u8 instead of a bool, per the kernel bot.
>   Apparently bool is discouraged in structs these days
> 
> 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    |  6 ++++++
>  2 files changed, 13 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)

Just use the same READ_ONCE(conn->registered) trick as in the core? Imo no
need to track this twice ...

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

I think if we've move the conn->registered check into drm_probe_helper.c
we could drop all these checks here too. And fixup nouveau/amdgpu while at
it - trying to get modes for a disconnected mst sink is somewhat silly :-)

From a functional pov the patch lgtm. With mst_port_gone removed and the
READ_ON(!conn->registered) trick everywhere instead this has my

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Cheers, Daniel

>  
>  	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..8261d4579452 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -410,6 +410,12 @@ struct intel_connector {
>  
>  	struct intel_dp *mst_port;
>  
> +	/*
> +	 * Set to 1 if this is an MST connector that was removed from the
> +	 * system and unregistered from sysfs
> +	 */
> +	u8 mst_port_gone;
-Daniel
> +
>  	/* Work struct to schedule a uevent on link train failure */
>  	struct work_struct modeset_retry_work;
>  
> -- 
> 2.17.1
> 

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

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

[Daniel Vetter]

On Thu, Oct 04, 2018 at 08:29:52PM -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.
> 
> Changes since v2:
> - Add a comment to mst_port_gone (Jani Nikula)
> - Change mst_port_gone to a u8 instead of a bool, per the kernel bot.
>   Apparently bool is discouraged in structs these days
> 
> 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    |  6 ++++++
>  2 files changed, 13 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)

Just use the same READ_ONCE(conn->registered) trick as in the core? Imo no
need to track this twice ...

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

I think if we've move the conn->registered check into drm_probe_helper.c
we could drop all these checks here too. And fixup nouveau/amdgpu while at
it - trying to get modes for a disconnected mst sink is somewhat silly :-)

>From a functional pov the patch lgtm. With mst_port_gone removed and the
READ_ON(!conn->registered) trick everywhere instead this has my

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Cheers, Daniel

>  
>  	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..8261d4579452 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -410,6 +410,12 @@ struct intel_connector {
>  
>  	struct intel_dp *mst_port;
>  
> +	/*
> +	 * Set to 1 if this is an MST connector that was removed from the
> +	 * system and unregistered from sysfs
> +	 */
> +	u8 mst_port_gone;
-Daniel
> +
>  	/* Work struct to schedule a uevent on link train failure */
>  	struct work_struct modeset_retry_work;
>  
> -- 
> 2.17.1
> 

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

[PATCH v4 4/5] 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

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

[Daniel Vetter]

On Thu, Oct 04, 2018 at 08:29:53PM -0400, Lyude Paul wrote:
> 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) {

Wondered why you don't need this for nouveau/amdgpu, but a bit of grepping
later says they don't even bother to check that in their atomic_check
functions. So if a multi-stream cable runs out of vcpi slots, they just
toss up their hands, atomic be damned :-(

With the s/mst_port_gone/READ_ONCE(!conn->registered)/ bikeshed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I wondered a bit whether ths fuction shouldn't just return 0 if the
connector is gone, but we'd need to audit/fix other drivers first.
-Daniel

> +		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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

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

[Daniel Vetter]

On Thu, Oct 04, 2018 at 08:29:53PM -0400, Lyude Paul wrote:
> 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) {

Wondered why you don't need this for nouveau/amdgpu, but a bit of grepping
later says they don't even bother to check that in their atomic_check
functions. So if a multi-stream cable runs out of vcpi slots, they just
toss up their hands, atomic be damned :-(

With the s/mst_port_gone/READ_ONCE(!conn->registered)/ bikeshed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I wondered a bit whether ths fuction shouldn't just return 0 if the
connector is gone, but we'd need to audit/fix other drivers first.
-Daniel

> +		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
> 

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

[PATCH v4 5/5] 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.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: stable@vger.kernel.org

Changes since v1:
- Remove mst atomic helper, since this got replaced with a much simpler
  solution

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index a366f32b048a..daade60c5714 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -407,8 +407,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

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

[Daniel Vetter]

On Thu, Oct 04, 2018 at 08:29:54PM -0400, Lyude Paul wrote:
> 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.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Cc: stable@vger.kernel.org
> 
> Changes since v1:
> - Remove mst atomic helper, since this got replaced with a much simpler
>   solution
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>

With the obvious rebase onto my bikeshed for patch 3:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index a366f32b048a..daade60c5714 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -407,8 +407,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
> 

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

✗ Fi.CI.BAT: failure for Fix legacy DPMS changes with MST (rev4)

[Patchwork]

== Series Details ==

Series: Fix legacy DPMS changes with MST (rev4)
URL   : https://patchwork.freedesktop.org/series/49878/
State : failure

== Summary ==

Applying: drm/atomic_helper: Disallow new modesets on unregistered connectors
Applying: drm/nouveau: Fix nv50_mstc->best_encoder()
Applying: drm/i915: Leave intel_conn->mst_port set, use mst_port_gone instead
Applying: drm/i915: Skip vcpi allocation for MSTB ports that are gone
error: sha1 information is lacking or useless (drivers/gpu/drm/i915/intel_dp_mst.c).
error: could not build fake ancestor
Patch failed at 0004 drm/i915: Skip vcpi allocation for MSTB ports that are gone
Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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