diff mbox series

[v7,1/5] drm/atomic_helper: Disallow new modesets on unregistered connectors

Message ID 20181008232437.5571-2-lyude@redhat.com (mailing list archive)
State New, archived
Headers show
Series Fix legacy DPMS changes with MST | expand

Commit Message

Lyude Paul Oct. 8, 2018, 11:24 p.m. UTC 
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(-)

Comments

Ville Syrjälä Oct. 9, 2018, 5:20 p.m. UTC | #1 
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
Daniel Vetter Oct. 11, 2018, 8:40 a.m. UTC | #2 
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
diff mbox series

Patch

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",