[28/28] drm/atomic-helper: Reject legacy flips on a disabled pipe
diff mbox

Message ID 1449218769-16577-29-git-send-email-daniel.vetter@ffwll.ch
State New
Headers show

Commit Message

Daniel Vetter Dec. 4, 2015, 8:46 a.m. UTC
We want this for consistency with existing page_flip semantics.

Since this spurred quite a discussion on IRC also document why we
reject even generation when the pipe is off: It's not that it's hard
to implement, but userspace has a track recording proofing that it's
way too easy to accidentally abuse and cause havoc. We want to make
sure userspace doesn't get away with that.

Cc: Daniel Stone <daniels@collabora.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_atomic.c        | 9 +++++++++
 drivers/gpu/drm/drm_atomic_helper.c | 8 ++++++++
 2 files changed, 17 insertions(+)

Comments

Ville Syrjälä Dec. 7, 2015, 1:42 p.m. UTC | #1
On Fri, Dec 04, 2015 at 09:46:09AM +0100, Daniel Vetter wrote:
> We want this for consistency with existing page_flip semantics.
> 
> Since this spurred quite a discussion on IRC also document why we
> reject even generation when the pipe is off: It's not that it's hard
> to implement, but userspace has a track recording proofing that it's
> way too easy to accidentally abuse and cause havoc. We want to make
> sure userspace doesn't get away with that.
> 
> Cc: Daniel Stone <daniels@collabora.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_atomic.c        | 9 +++++++++
>  drivers/gpu/drm/drm_atomic_helper.c | 8 ++++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 7426d40017a0..06cdb52907da 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1265,6 +1265,15 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>  	if (config->funcs->atomic_check)
>  		ret = config->funcs->atomic_check(state->dev, state);
>  
> +	/*
> +	 * Reject event generation for when a CRTC is off and stays off. It
> +	 * wouldn't be hard to implement this, but userspace has a track record
> +	 * of happily burning through 100% cpu (or worse, crash) when the
> +	 * display pipe is suspended. To avoid all that fun just reject updates
> +	 * that ask for events since likely that indicates a bug in the
> +	 * compositors drawing loop. This is consistent with the vblank ioctl
> +	 * which also rejects service on a disabled pipe.
> +	 */
>  	if (!state->allow_modeset) {
>  		for_each_crtc_in_state(state, crtc, crtc_state, i) {
>  			if (drm_atomic_crtc_needs_modeset(crtc_state)) {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 110f3db8dd05..8e281a96c35f 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2283,6 +2283,14 @@ retry:

Argh! Once again that stupid 'put goto labes at col 0' rule making
it impossible to review a patch. Could we *please* change that to
put the labels at col 1 instead?

>  		goto fail;
>  	drm_atomic_set_fb_for_plane(plane_state, fb);
>  
> +	state->allow_modeset = false;
> +	if (!crtc_state->active) {
> +		DRM_DEBUG_ATOMIC("[CRTC:%d] disabled, rejecting legacy flip\n",
> +				 crtc->base.id);
> +		ret = -EINVAL;
> +		goto fail;
> +	}
> +
>  	ret = drm_atomic_async_commit(state);
>  	if (ret != 0)
>  		goto fail;
> -- 
> 2.5.1
Thierry Reding Dec. 7, 2015, 3:25 p.m. UTC | #2
On Fri, Dec 04, 2015 at 09:46:09AM +0100, Daniel Vetter wrote:
> We want this for consistency with existing page_flip semantics.
> 
> Since this spurred quite a discussion on IRC also document why we
> reject even generation when the pipe is off: It's not that it's hard
> to implement, but userspace has a track recording proofing that it's
> way too easy to accidentally abuse and cause havoc. We want to make
> sure userspace doesn't get away with that.
> 
> Cc: Daniel Stone <daniels@collabora.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_atomic.c        | 9 +++++++++
>  drivers/gpu/drm/drm_atomic_helper.c | 8 ++++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 7426d40017a0..06cdb52907da 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1265,6 +1265,15 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>  	if (config->funcs->atomic_check)
>  		ret = config->funcs->atomic_check(state->dev, state);
>  
> +	/*
> +	 * Reject event generation for when a CRTC is off and stays off. It
> +	 * wouldn't be hard to implement this, but userspace has a track record
> +	 * of happily burning through 100% cpu (or worse, crash) when the
> +	 * display pipe is suspended. To avoid all that fun just reject updates
> +	 * that ask for events since likely that indicates a bug in the
> +	 * compositors drawing loop. This is consistent with the vblank ioctl

"compositor's".

> +	 * which also rejects service on a disabled pipe.
> +	 */
>  	if (!state->allow_modeset) {
>  		for_each_crtc_in_state(state, crtc, crtc_state, i) {
>  			if (drm_atomic_crtc_needs_modeset(crtc_state)) {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 110f3db8dd05..8e281a96c35f 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2283,6 +2283,14 @@ retry:
>  		goto fail;
>  	drm_atomic_set_fb_for_plane(plane_state, fb);
>  
> +	state->allow_modeset = false;

Perhaps explain the reason for setting this?

Thierry
Daniel Stone Dec. 7, 2015, 3:33 p.m. UTC | #3
Hi,

On 4 December 2015 at 08:46, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> +       /*
> +        * Reject event generation for when a CRTC is off and stays off. It
> +        * wouldn't be hard to implement this, but userspace has a track record
> +        * of happily burning through 100% cpu (or worse, crash) when the
> +        * display pipe is suspended. To avoid all that fun just reject updates
> +        * that ask for events since likely that indicates a bug in the
> +        * compositors drawing loop. This is consistent with the vblank ioctl
> +        * which also rejects service on a disabled pipe.
> +        */

Sorry to keep whingeing, but this isn't actually related to event
generation. To the best of my knowledge, this change does a few
things. Firstly, comments the check above (enforcing that (flags &
ALLOW_MODESET) == {crtcs}->allow_modeset), which is good. But the
comment is incorrect, because whether or not an event is requested is
wholly unrelated. Secondly, it disables allow_modeset on pageflip,
which shouldn't be changing DPMS stage (good!). Thirdly, it enforces
something like the above statement on pageflips, except again it has
no regard to events: it just enforces the no-DPMS-on-flip rule, for
which event generation is a subset.

If you fix the above comment to more accurately note that this just
enforces that you need the ALLOW_MODESET flag to change active, mode
or connector routing, and (as Thierry asked), add a comment below to
note that we enforce existing no-DPMS-on-flip semantics in the helper,
then you're welcome to my R-b. But please don't mention events in the
new comment. :)

Cheers,
Daniel
Daniel Vetter Dec. 8, 2015, 8:23 a.m. UTC | #4
On Mon, Dec 07, 2015 at 03:33:00PM +0000, Daniel Stone wrote:
> Hi,
> 
> On 4 December 2015 at 08:46, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > +       /*
> > +        * Reject event generation for when a CRTC is off and stays off. It
> > +        * wouldn't be hard to implement this, but userspace has a track record
> > +        * of happily burning through 100% cpu (or worse, crash) when the
> > +        * display pipe is suspended. To avoid all that fun just reject updates
> > +        * that ask for events since likely that indicates a bug in the
> > +        * compositors drawing loop. This is consistent with the vblank ioctl
> > +        * which also rejects service on a disabled pipe.
> > +        */
> 
> Sorry to keep whingeing, but this isn't actually related to event
> generation. To the best of my knowledge, this change does a few
> things. Firstly, comments the check above (enforcing that (flags &
> ALLOW_MODESET) == {crtcs}->allow_modeset), which is good. But the
> comment is incorrect, because whether or not an event is requested is
> wholly unrelated. Secondly, it disables allow_modeset on pageflip,
> which shouldn't be changing DPMS stage (good!). Thirdly, it enforces
> something like the above statement on pageflips, except again it has
> no regard to events: it just enforces the no-DPMS-on-flip rule, for
> which event generation is a subset.

Well the comment is completely misplace, I wanted to put it next to the
check for event generation, not here.

> If you fix the above comment to more accurately note that this just
> enforces that you need the ALLOW_MODESET flag to change active, mode
> or connector routing, and (as Thierry asked), add a comment below to
> note that we enforce existing no-DPMS-on-flip semantics in the helper,
> then you're welcome to my R-b. But please don't mention events in the
> new comment. :)

Hm, I didn't really want to type a comment for ALLOW_MODESET - imo it's
pretty clear what it does and why it's useful. I'll try again at making
something coheren here ;-)
-Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 7426d40017a0..06cdb52907da 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1265,6 +1265,15 @@  int drm_atomic_check_only(struct drm_atomic_state *state)
 	if (config->funcs->atomic_check)
 		ret = config->funcs->atomic_check(state->dev, state);
 
+	/*
+	 * Reject event generation for when a CRTC is off and stays off. It
+	 * wouldn't be hard to implement this, but userspace has a track record
+	 * of happily burning through 100% cpu (or worse, crash) when the
+	 * display pipe is suspended. To avoid all that fun just reject updates
+	 * that ask for events since likely that indicates a bug in the
+	 * compositors drawing loop. This is consistent with the vblank ioctl
+	 * which also rejects service on a disabled pipe.
+	 */
 	if (!state->allow_modeset) {
 		for_each_crtc_in_state(state, crtc, crtc_state, i) {
 			if (drm_atomic_crtc_needs_modeset(crtc_state)) {
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 110f3db8dd05..8e281a96c35f 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2283,6 +2283,14 @@  retry:
 		goto fail;
 	drm_atomic_set_fb_for_plane(plane_state, fb);
 
+	state->allow_modeset = false;
+	if (!crtc_state->active) {
+		DRM_DEBUG_ATOMIC("[CRTC:%d] disabled, rejecting legacy flip\n",
+				 crtc->base.id);
+		ret = -EINVAL;
+		goto fail;
+	}
+
 	ret = drm_atomic_async_commit(state);
 	if (ret != 0)
 		goto fail;