| Message ID | 1449218769-16577-29-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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;
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(+)