| Message ID | 1449564561-3896-4-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 8 December 2015 at 08:49, Daniel Vetter <daniel.vetter@ffwll.ch> 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. > > v2: Somehow thought we do reject events already, but that code only > existed in my imagination ... Also suggestions from Thierry. What a beautifully-coloured shed. Reviewed-by: Daniel Stone <daniels@collabora.com> Cheers, Daniel
On Tue, Dec 8, 2015 at 3:49 AM, Daniel Vetter <daniel.vetter@ffwll.ch> 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 event generation? > to implement, but userspace has a track recording proofing that it's has a track record which proves that it's > way too easy to accidentally abuse and cause havoc. We want to make > sure userspace doesn't get away with that. > > v2: Somehow thought we do reject events already, but that code only > existed in my imagination ... Also suggestions from Thierry. > > Cc: Daniel Stone <daniels@collabora.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Thierry Reding <thierry.reding@gmail.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/drm_atomic.c | 16 ++++++++++++++++ > drivers/gpu/drm/drm_atomic_helper.c | 9 +++++++++ > include/drm/drm_crtc.h | 3 ++- > 3 files changed, 27 insertions(+), 1 deletion(-)
On Tue, Dec 08, 2015 at 12:01:57PM +0000, Daniel Stone wrote: > Hi, > > On 8 December 2015 at 08:49, Daniel Vetter <daniel.vetter@ffwll.ch> 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. > > > > v2: Somehow thought we do reject events already, but that code only > > existed in my imagination ... Also suggestions from Thierry. > > What a beautifully-coloured shed. > > Reviewed-by: Daniel Stone <daniels@collabora.com> Added Ilia's spelling fixes and merged to drm-misc, thanks for the review. -Daniel
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 07ab75e22b2b..029377513b1d 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -508,6 +508,22 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc, return -EINVAL; } + /* + * 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 + * compositor's drawing loop. This is consistent with the vblank IOCTL + * and legacy page_flip IOCTL which also reject service on a disabled + * pipe. + */ + if (state->event && !state->active && !crtc->state->active) { + DRM_DEBUG_ATOMIC("[CRTC:%d] requesting event but off\n", + crtc->base.id); + return -EINVAL; + } + return 0; } diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 76c124b2a775..9eb367cad790 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2282,6 +2282,15 @@ retry: goto fail; drm_atomic_set_fb_for_plane(plane_state, fb); + /* Make sure we don't accidentally do a full modeset. */ + 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; diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 6fe14a773def..6da847a6cb2f 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -548,7 +548,8 @@ struct drm_crtc_funcs { * ->page_flip() operation is already pending the callback should return * -EBUSY. Pageflips on a disabled CRTC (either by setting a NULL mode * or just runtime disabled through DPMS respectively the new atomic - * "ACTIVE" state) should result in an -EINVAL error code. + * "ACTIVE" state) should result in an -EINVAL error code. Note that + * drm_atomic_helper_page_flip() checks this already for atomic drivers. */ int (*page_flip)(struct drm_crtc *crtc, struct drm_framebuffer *fb,
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. v2: Somehow thought we do reject events already, but that code only existed in my imagination ... Also suggestions from Thierry. Cc: Daniel Stone <daniels@collabora.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Thierry Reding <thierry.reding@gmail.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/drm_atomic.c | 16 ++++++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 9 +++++++++ include/drm/drm_crtc.h | 3 ++- 3 files changed, 27 insertions(+), 1 deletion(-)