linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v7 0/5] Fix legacy DPMS changes with MST
@ 2018-10-08 23:24 Lyude Paul
  2018-10-08 23:24 ` [PATCH v7 1/5] drm/atomic_helper: Disallow new modesets on unregistered connectors Lyude Paul
                   ` (4 more replies)
  0 siblings, 5 replies; 8+ messages in thread
From: Lyude Paul @ 2018-10-08 23:24 UTC (permalink / raw)
  To: intel-gfx, nouveau, dri-devel
  Cc: David Airlie, Maarten Lankhorst, linux-kernel, Sean Paul,
	Gustavo Padovan, Sean Paul, Daniel Vetter, Ben Skeggs,
	Ville Syrjälä,
	Lyude Paul, Rodrigo Vivi, Jani Nikula, Joonas Lahtinen

Next version of https://patchwork.freedesktop.org/series/49878/

Still no functional changes, just removing a duplicate s-b to make CI
happy.

Lyude Paul (5):
  drm/atomic_helper: Disallow new modesets on unregistered connectors
  drm/nouveau: Fix nv50_mstc->best_encoder()
  drm/i915: Don't unset intel_connector->mst_port
  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     | 41 ++++++++++++-------------
 drivers/gpu/drm/nouveau/dispnv50/disp.c | 14 +++------
 3 files changed, 44 insertions(+), 32 deletions(-)

-- 
2.17.1


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

* [PATCH v7 1/5] drm/atomic_helper: Disallow new modesets on unregistered connectors
  2018-10-08 23:24 [PATCH v7 0/5] Fix legacy DPMS changes with MST Lyude Paul
@ 2018-10-08 23:24 ` Lyude Paul
  2018-10-09 17:20   ` Ville Syrjälä
  2018-10-08 23:24 ` [PATCH v7 2/5] drm/nouveau: Fix nv50_mstc->best_encoder() Lyude Paul
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 8+ messages in thread
From: Lyude Paul @ 2018-10-08 23:24 UTC (permalink / raw)
  To: intel-gfx, nouveau, dri-devel
  Cc: stable, Gustavo Padovan, Maarten Lankhorst, Sean Paul,
	David Airlie, linux-kernel

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.

Changes since v5:
- Fix typo in comment, nothing else

Signed-off-by: Lyude Paul <lyude@redhat.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
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 6f66777dca4b..e6a2cf72de5e 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -319,6 +319,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 its 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)
@@ -363,7 +383,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


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

* [PATCH v7 2/5] drm/nouveau: Fix nv50_mstc->best_encoder()
  2018-10-08 23:24 [PATCH v7 0/5] Fix legacy DPMS changes with MST Lyude Paul
  2018-10-08 23:24 ` [PATCH v7 1/5] drm/atomic_helper: Disallow new modesets on unregistered connectors Lyude Paul
@ 2018-10-08 23:24 ` Lyude Paul
  2018-10-08 23:24 ` [PATCH v7 3/5] drm/i915: Don't unset intel_connector->mst_port Lyude Paul
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Lyude Paul @ 2018-10-08 23:24 UTC (permalink / raw)
  To: intel-gfx, nouveau, dri-devel
  Cc: stable, Ben Skeggs, David Airlie, Daniel Vetter,
	Ville Syrjälä,
	Thierry Reding, linux-kernel

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.

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>
Cc: stable@vger.kernel.org
---
 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 9f32b10c7c29..31b94bc9ec90 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


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

* [PATCH v7 3/5] drm/i915: Don't unset intel_connector->mst_port
  2018-10-08 23:24 [PATCH v7 0/5] Fix legacy DPMS changes with MST Lyude Paul
  2018-10-08 23:24 ` [PATCH v7 1/5] drm/atomic_helper: Disallow new modesets on unregistered connectors Lyude Paul
  2018-10-08 23:24 ` [PATCH v7 2/5] drm/nouveau: Fix nv50_mstc->best_encoder() Lyude Paul
@ 2018-10-08 23:24 ` Lyude Paul
  2018-10-08 23:24 ` [PATCH v7 4/5] drm/i915: Skip vcpi allocation for MSTB ports that are gone Lyude Paul
  2018-10-08 23:24 ` [PATCH v7 5/5] drm/i915: Fix intel_dp_mst_best_encoder() Lyude Paul
  4 siblings, 0 replies; 8+ messages in thread
From: Lyude Paul @ 2018-10-08 23:24 UTC (permalink / raw)
  To: intel-gfx, nouveau, dri-devel
  Cc: stable, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, David Airlie,
	linux-kernel

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 just check the status of
drm_connector->regustered 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
Changes since v4:
- Don't use mst_port_gone at all! Just check if the connector is
  registered or not - Daniel Vetter

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

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 43db2e9ac575..aa21742d8634 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -307,9 +307,8 @@ static int intel_dp_mst_get_ddc_modes(struct drm_connector *connector)
 	struct edid *edid;
 	int ret;
 
-	if (!intel_dp) {
+	if (!READ_ONCE(connector->registered))
 		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);
@@ -324,9 +323,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 (!READ_ONCE(connector->registered))
 		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
@@ -366,7 +366,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 (!READ_ONCE(connector->registered))
 		return MODE_ERROR;
 
 	if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
@@ -398,7 +398,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 (!READ_ONCE(connector->registered))
 		return NULL;
 	return &intel_dp->mst_encoders[crtc->pipe]->base.base;
 }
@@ -499,7 +499,6 @@ static void intel_dp_register_mst_connector(struct drm_connector *connector)
 static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 					   struct drm_connector *connector)
 {
-	struct intel_connector *intel_connector = to_intel_connector(connector);
 	struct drm_i915_private *dev_priv = to_i915(connector->dev);
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, connector->name);
@@ -508,10 +507,6 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 	if (dev_priv->fbdev)
 		drm_fb_helper_remove_one_connector(&dev_priv->fbdev->helper,
 						   connector);
-	/* prevent race with the check in ->detect */
-	drm_modeset_lock(&connector->dev->mode_config.connection_mutex, NULL);
-	intel_connector->mst_port = NULL;
-	drm_modeset_unlock(&connector->dev->mode_config.connection_mutex);
 
 	drm_connector_put(connector);
 }
-- 
2.17.1


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

* [PATCH v7 4/5] drm/i915: Skip vcpi allocation for MSTB ports that are gone
  2018-10-08 23:24 [PATCH v7 0/5] Fix legacy DPMS changes with MST Lyude Paul
                   ` (2 preceding siblings ...)
  2018-10-08 23:24 ` [PATCH v7 3/5] drm/i915: Don't unset intel_connector->mst_port Lyude Paul
@ 2018-10-08 23:24 ` Lyude Paul
  2018-10-08 23:24 ` [PATCH v7 5/5] drm/i915: Fix intel_dp_mst_best_encoder() Lyude Paul
  4 siblings, 0 replies; 8+ messages in thread
From: Lyude Paul @ 2018-10-08 23:24 UTC (permalink / raw)
  To: intel-gfx, nouveau, dri-devel
  Cc: stable, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, David Airlie,
	linux-kernel

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.

Changes since V4:
- Don't use mst_port_gone at all, just check whether or not the drm
  connector is registered - Daniel Vetter

Signed-off-by: Lyude Paul <lyude@redhat.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index aa21742d8634..0f14c0d1669c 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -38,11 +38,11 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 	struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder->base);
 	struct intel_digital_port *intel_dig_port = intel_mst->primary;
 	struct intel_dp *intel_dp = &intel_dig_port->dp;
-	struct intel_connector *connector =
-		to_intel_connector(conn_state->connector);
+	struct drm_connector *connector = conn_state->connector;
+	void *port = to_intel_connector(connector)->port;
 	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 constant_n = drm_dp_has_quirk(&intel_dp->desc,
@@ -70,17 +70,23 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 
 	pipe_config->port_clock = intel_dp_max_link_rate(intel_dp);
 
-	if (drm_dp_mst_port_has_audio(&intel_dp->mst_mgr, connector->port))
+	if (drm_dp_mst_port_has_audio(&intel_dp->mst_mgr, port))
 		pipe_config->has_audio = true;
 
 	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;
+	/* Zombie connectors can't have VCPI slots */
+	if (READ_ONCE(connector->registered)) {
+		slots = drm_dp_atomic_find_vcpi_slots(state,
+						      &intel_dp->mst_mgr,
+						      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


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

* [PATCH v7 5/5] drm/i915: Fix intel_dp_mst_best_encoder()
  2018-10-08 23:24 [PATCH v7 0/5] Fix legacy DPMS changes with MST Lyude Paul
                   ` (3 preceding siblings ...)
  2018-10-08 23:24 ` [PATCH v7 4/5] drm/i915: Skip vcpi allocation for MSTB ports that are gone Lyude Paul
@ 2018-10-08 23:24 ` Lyude Paul
  4 siblings, 0 replies; 8+ messages in thread
From: Lyude Paul @ 2018-10-08 23:24 UTC (permalink / raw)
  To: intel-gfx, nouveau, dri-devel
  Cc: stable, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, David Airlie,
	linux-kernel

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.

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

Signed-off-by: Lyude Paul <lyude@redhat.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: stable@vger.kernel.org
---
 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 0f14c0d1669c..7f155b4f1a7d 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -404,8 +404,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 (!READ_ONCE(connector->registered))
-		return NULL;
 	return &intel_dp->mst_encoders[crtc->pipe]->base.base;
 }
 
-- 
2.17.1


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

* Re: [PATCH v7 1/5] drm/atomic_helper: Disallow new modesets on unregistered connectors
  2018-10-08 23:24 ` [PATCH v7 1/5] drm/atomic_helper: Disallow new modesets on unregistered connectors Lyude Paul
@ 2018-10-09 17:20   ` Ville Syrjälä
  2018-10-11  8:40     ` Daniel Vetter
  0 siblings, 1 reply; 8+ messages in thread
From: Ville Syrjälä @ 2018-10-09 17:20 UTC (permalink / raw)
  To: Lyude Paul
  Cc: intel-gfx, nouveau, dri-devel, David Airlie, linux-kernel,
	stable, Sean Paul, Daniel Vetter

On Mon, Oct 08, 2018 at 07:24:30PM -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.
> 
> Changes since v5:
> - Fix typo in comment, nothing else
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 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 6f66777dca4b..e6a2cf72de5e 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -319,6 +319,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 its 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;
> +	}

This broke my ilk (and presumably snb-bdw as well).

[   25.593121] [drm:drm_atomic_helper_check_modeset [drm_kms_helper]] Updating routing for [CONNECTOR:55:eDP-1]
[   25.593131] [drm:drm_atomic_helper_check_modeset [drm_kms_helper]] [CONNECTOR:55:eDP-1] is not registered
[   25.593133] ------------[ cut here ]------------
[   25.593134] Could not determine valid watermarks for inherited state
[   25.593212] WARNING: CPU: 0 PID: 3060 at ../drivers/gpu/drm/i915/intel_display.c:14983 intel_modeset_init+0x12cf/0x1980 [i915]

Also I can't see that any of the repostings of this has undergone the
full CI run (just BAT results are visible). Not sure why that is.
Not that the full run would have caught this because we unwisely 
load the module before the tests start. Which means any failures
during initial readout/takeover will not be flagged :(

> +
>  	funcs = connector->helper_private;
>  
>  	if (funcs->atomic_best_encoder)
> @@ -363,7 +383,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
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel

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

* Re: [PATCH v7 1/5] drm/atomic_helper: Disallow new modesets on unregistered connectors
  2018-10-09 17:20   ` Ville Syrjälä
@ 2018-10-11  8:40     ` Daniel Vetter
  0 siblings, 0 replies; 8+ messages in thread
From: Daniel Vetter @ 2018-10-11  8:40 UTC (permalink / raw)
  To: Ville Syrjälä
  Cc: Lyude Paul, intel-gfx, nouveau, dri-devel, David Airlie,
	linux-kernel, stable, Sean Paul, Daniel Vetter

On Tue, Oct 09, 2018 at 08:20:27PM +0300, Ville Syrjälä wrote:
> On Mon, Oct 08, 2018 at 07:24:30PM -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.
> > 
> > Changes since v5:
> > - Fix typo in comment, nothing else
> > 
> > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 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 6f66777dca4b..e6a2cf72de5e 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -319,6 +319,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 its 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;
> > +	}
> 
> This broke my ilk (and presumably snb-bdw as well).
> 
> [   25.593121] [drm:drm_atomic_helper_check_modeset [drm_kms_helper]] Updating routing for [CONNECTOR:55:eDP-1]
> [   25.593131] [drm:drm_atomic_helper_check_modeset [drm_kms_helper]] [CONNECTOR:55:eDP-1] is not registered
> [   25.593133] ------------[ cut here ]------------
> [   25.593134] Could not determine valid watermarks for inherited state
> [   25.593212] WARNING: CPU: 0 PID: 3060 at ../drivers/gpu/drm/i915/intel_display.c:14983 intel_modeset_init+0x12cf/0x1980 [i915]
> 
> Also I can't see that any of the repostings of this has undergone the
> full CI run (just BAT results are visible). Not sure why that is.
> Not that the full run would have caught this because we unwisely 
> load the module before the tests start. Which means any failures
> during initial readout/takeover will not be flagged :(

Ugh, so we need a special bool for "unplugged", to differentiate it from
the "it's real, but not yet registered because the driver isn't fully
loaded yet" case.
-Daniel

> 
> > +
> >  	funcs = connector->helper_private;
> >  
> >  	if (funcs->atomic_best_encoder)
> > @@ -363,7 +383,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
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrjälä
> Intel

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

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

end of thread, other threads:[~2018-10-11  8:40 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-08 23:24 [PATCH v7 0/5] Fix legacy DPMS changes with MST Lyude Paul
2018-10-08 23:24 ` [PATCH v7 1/5] drm/atomic_helper: Disallow new modesets on unregistered connectors Lyude Paul
2018-10-09 17:20   ` Ville Syrjälä
2018-10-11  8:40     ` Daniel Vetter
2018-10-08 23:24 ` [PATCH v7 2/5] drm/nouveau: Fix nv50_mstc->best_encoder() Lyude Paul
2018-10-08 23:24 ` [PATCH v7 3/5] drm/i915: Don't unset intel_connector->mst_port Lyude Paul
2018-10-08 23:24 ` [PATCH v7 4/5] drm/i915: Skip vcpi allocation for MSTB ports that are gone Lyude Paul
2018-10-08 23:24 ` [PATCH v7 5/5] drm/i915: Fix intel_dp_mst_best_encoder() Lyude Paul

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).