* drm/atomic: Reject events for inactive crtc's. @ 2015-07-30 7:03 Maarten Lankhorst 2015-08-06 9:47 ` Daniel Stone 0 siblings, 1 reply; 7+ messages in thread From: Maarten Lankhorst @ 2015-07-30 7:03 UTC (permalink / raw) To: Daniel Vetter, dri-devel@lists.freedesktop.org; +Cc: Intel Graphics Development This will cause drm_atomic_helper_page_flip and drm_mode_atomic_ioctl to fail with -EINVAL if a event is requested on a inactive crtc. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index d719ce0b10a0..679577e8e02d 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -476,6 +476,12 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc, return -EINVAL; } + if (!state->active && state->event) { + DRM_DEBUG_ATOMIC("[CRTC:%d] requesting event and not active\n", + crtc->base.id); + return -EINVAL; + } + /* The state->enable vs. state->mode_blob checks can be WARN_ON, * as this is a kernel-internal detail that userspace should never * be able to trigger. */ _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: drm/atomic: Reject events for inactive crtc's. 2015-07-30 7:03 drm/atomic: Reject events for inactive crtc's Maarten Lankhorst @ 2015-08-06 9:47 ` Daniel Stone 2015-08-06 11:19 ` Maarten Lankhorst 0 siblings, 1 reply; 7+ messages in thread From: Daniel Stone @ 2015-08-06 9:47 UTC (permalink / raw) To: Maarten Lankhorst Cc: Intel Graphics Development, dri-devel@lists.freedesktop.org Hi, On 30 July 2015 at 08:03, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote: > This will cause drm_atomic_helper_page_flip and drm_mode_atomic_ioctl to > fail with -EINVAL if a event is requested on a inactive crtc. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index d719ce0b10a0..679577e8e02d 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -476,6 +476,12 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc, > return -EINVAL; > } > > + if (!state->active && state->event) { > + DRM_DEBUG_ATOMIC("[CRTC:%d] requesting event and not active\n", > + crtc->base.id); > + return -EINVAL; > + } Hmm, even if disabling? Maybe (!crtc->state->active && !state->active) && state->event. Cheers, Daniel _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: drm/atomic: Reject events for inactive crtc's. 2015-08-06 9:47 ` Daniel Stone @ 2015-08-06 11:19 ` Maarten Lankhorst 2015-08-06 12:49 ` Daniel Vetter 0 siblings, 1 reply; 7+ messages in thread From: Maarten Lankhorst @ 2015-08-06 11:19 UTC (permalink / raw) To: Daniel Stone; +Cc: Intel Graphics Development, dri-devel@lists.freedesktop.org Hey, Op 06-08-15 om 11:47 schreef Daniel Stone: > Hi, > > On 30 July 2015 at 08:03, Maarten Lankhorst > <maarten.lankhorst@linux.intel.com> wrote: >> This will cause drm_atomic_helper_page_flip and drm_mode_atomic_ioctl to >> fail with -EINVAL if a event is requested on a inactive crtc. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >> index d719ce0b10a0..679577e8e02d 100644 >> --- a/drivers/gpu/drm/drm_atomic.c >> +++ b/drivers/gpu/drm/drm_atomic.c >> @@ -476,6 +476,12 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc, >> return -EINVAL; >> } >> >> + if (!state->active && state->event) { >> + DRM_DEBUG_ATOMIC("[CRTC:%d] requesting event and not active\n", >> + crtc->base.id); >> + return -EINVAL; >> + } > Hmm, even if disabling? Maybe (!crtc->state->active && !state->active) > && state->event. How do you want to send a vblank event after disabling? I think the only thing that makes sense would be not creating events when crtc_state->enable = false, so when a crtc is removed and an event is requested you won't return -EINVAL. ~Maarten _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: drm/atomic: Reject events for inactive crtc's. 2015-08-06 11:19 ` Maarten Lankhorst @ 2015-08-06 12:49 ` Daniel Vetter 2015-08-27 14:36 ` Daniel Stone 0 siblings, 1 reply; 7+ messages in thread From: Daniel Vetter @ 2015-08-06 12:49 UTC (permalink / raw) To: Maarten Lankhorst Cc: dri-devel@lists.freedesktop.org, Intel Graphics Development On Thu, Aug 06, 2015 at 01:19:35PM +0200, Maarten Lankhorst wrote: > Hey, > > Op 06-08-15 om 11:47 schreef Daniel Stone: > > Hi, > > > > On 30 July 2015 at 08:03, Maarten Lankhorst > > <maarten.lankhorst@linux.intel.com> wrote: > >> This will cause drm_atomic_helper_page_flip and drm_mode_atomic_ioctl to > >> fail with -EINVAL if a event is requested on a inactive crtc. > >> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> --- > >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > >> index d719ce0b10a0..679577e8e02d 100644 > >> --- a/drivers/gpu/drm/drm_atomic.c > >> +++ b/drivers/gpu/drm/drm_atomic.c > >> @@ -476,6 +476,12 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc, > >> return -EINVAL; > >> } > >> > >> + if (!state->active && state->event) { > >> + DRM_DEBUG_ATOMIC("[CRTC:%d] requesting event and not active\n", > >> + crtc->base.id); > >> + return -EINVAL; > >> + } > > Hmm, even if disabling? Maybe (!crtc->state->active && !state->active) > > && state->event. > How do you want to send a vblank event after disabling? The event would be when we stop scanning out, but yeah that's a bit a tricky one. I guess for now (until we have someone who needs this) we could just merge Maarten's patch as the easier thing to do right now? Maybe with a small code comment that this is intentional? Atomic is a ridiculously complex interface, restricting it to just the features we actually need is imo the right approach. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: drm/atomic: Reject events for inactive crtc's. 2015-08-06 12:49 ` Daniel Vetter @ 2015-08-27 14:36 ` Daniel Stone 2015-08-27 15:22 ` [Intel-gfx] " Daniel Vetter 0 siblings, 1 reply; 7+ messages in thread From: Daniel Stone @ 2015-08-27 14:36 UTC (permalink / raw) To: Daniel Vetter; +Cc: Intel Graphics Development, dri-devel@lists.freedesktop.org Hi, On 6 August 2015 at 13:49, Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Aug 06, 2015 at 01:19:35PM +0200, Maarten Lankhorst wrote: >> Op 06-08-15 om 11:47 schreef Daniel Stone: >> > On 30 July 2015 at 08:03, Maarten Lankhorst >> > <maarten.lankhorst@linux.intel.com> wrote: >> >> + if (!state->active && state->event) { >> >> + DRM_DEBUG_ATOMIC("[CRTC:%d] requesting event and not active\n", >> >> + crtc->base.id); >> >> + return -EINVAL; >> >> + } >> > Hmm, even if disabling? Maybe (!crtc->state->active && !state->active) >> > && state->event. >> How do you want to send a vblank event after disabling? > > The event would be when we stop scanning out, but yeah that's a bit a > tricky one. I guess for now (until we have someone who needs this) we > could just merge Maarten's patch as the easier thing to do right now? > Maybe with a small code comment that this is intentional? Exactly that. Surely this (when the CRTC actually goes dark) is something we already know? Assuming you don't have atomic_disable / instant-scanout-stop, and have to wait until the next vblank to kill it, then how does this work? - create FB - assign FB to plane on CRTC - unreference FB such that plane holds the last remaining reference - set plane->fb == NULL + crtc->active = 0, i.e. SetCrtc(NULL, 0, 0, 0, 0, ...) You can't immediately unpin/destroy the FB since you might still be mid-scanout. So you already need to defer this, and that would be the point at which you stop scanout. In a lot of hardware, this is just another latched register which takes effect on the next vblank, for which you still catch an IRQ - at which point you send the event. If you actually have atomic_disable hardware that stops scanout immediately, you can just send the event immediately. What am I missing? Cheers, Daniel _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Intel-gfx] drm/atomic: Reject events for inactive crtc's. 2015-08-27 14:36 ` Daniel Stone @ 2015-08-27 15:22 ` Daniel Vetter 2015-08-31 7:48 ` Maarten Lankhorst 0 siblings, 1 reply; 7+ messages in thread From: Daniel Vetter @ 2015-08-27 15:22 UTC (permalink / raw) To: Daniel Stone; +Cc: Intel Graphics Development, dri-devel@lists.freedesktop.org On Thu, Aug 27, 2015 at 03:36:09PM +0100, Daniel Stone wrote: > Hi, > > On 6 August 2015 at 13:49, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Thu, Aug 06, 2015 at 01:19:35PM +0200, Maarten Lankhorst wrote: > >> Op 06-08-15 om 11:47 schreef Daniel Stone: > >> > On 30 July 2015 at 08:03, Maarten Lankhorst > >> > <maarten.lankhorst@linux.intel.com> wrote: > >> >> + if (!state->active && state->event) { > >> >> + DRM_DEBUG_ATOMIC("[CRTC:%d] requesting event and not active\n", > >> >> + crtc->base.id); > >> >> + return -EINVAL; > >> >> + } > >> > Hmm, even if disabling? Maybe (!crtc->state->active && !state->active) > >> > && state->event. > >> How do you want to send a vblank event after disabling? > > > > The event would be when we stop scanning out, but yeah that's a bit a > > tricky one. I guess for now (until we have someone who needs this) we > > could just merge Maarten's patch as the easier thing to do right now? > > Maybe with a small code comment that this is intentional? > > Exactly that. Surely this (when the CRTC actually goes dark) is > something we already know? Assuming you don't have atomic_disable / > instant-scanout-stop, and have to wait until the next vblank to kill > it, then how does this work? > - create FB > - assign FB to plane on CRTC > - unreference FB such that plane holds the last remaining reference > - set plane->fb == NULL + crtc->active = 0, i.e. SetCrtc(NULL, 0, 0, > 0, 0, ...) > > You can't immediately unpin/destroy the FB since you might still be > mid-scanout. So you already need to defer this, and that would be the > point at which you stop scanout. In a lot of hardware, this is just > another latched register which takes effect on the next vblank, for > which you still catch an IRQ - at which point you send the event. If > you actually have atomic_disable hardware that stops scanout > immediately, you can just send the event immediately. > > What am I missing? The userspace which actually wants this and the testcases which makes sure it works. Until we have that I'd really prefer to just close that undefined behaviour gap in the atomic api, similar to how we just merged a patch to disallow switching live planes. We can always extend this later on easily, there's lots of little features like this that people suggested for atomic. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Intel-gfx] drm/atomic: Reject events for inactive crtc's. 2015-08-27 15:22 ` [Intel-gfx] " Daniel Vetter @ 2015-08-31 7:48 ` Maarten Lankhorst 0 siblings, 0 replies; 7+ messages in thread From: Maarten Lankhorst @ 2015-08-31 7:48 UTC (permalink / raw) To: Daniel Vetter, Daniel Stone Cc: Intel Graphics Development, dri-devel@lists.freedesktop.org Op 27-08-15 om 17:22 schreef Daniel Vetter: > On Thu, Aug 27, 2015 at 03:36:09PM +0100, Daniel Stone wrote: >> Hi, >> >> On 6 August 2015 at 13:49, Daniel Vetter <daniel@ffwll.ch> wrote: >>> On Thu, Aug 06, 2015 at 01:19:35PM +0200, Maarten Lankhorst wrote: >>>> Op 06-08-15 om 11:47 schreef Daniel Stone: >>>>> On 30 July 2015 at 08:03, Maarten Lankhorst >>>>> <maarten.lankhorst@linux.intel.com> wrote: >>>>>> + if (!state->active && state->event) { >>>>>> + DRM_DEBUG_ATOMIC("[CRTC:%d] requesting event and not active\n", >>>>>> + crtc->base.id); >>>>>> + return -EINVAL; >>>>>> + } >>>>> Hmm, even if disabling? Maybe (!crtc->state->active && !state->active) >>>>> && state->event. >>>> How do you want to send a vblank event after disabling? >>> The event would be when we stop scanning out, but yeah that's a bit a >>> tricky one. I guess for now (until we have someone who needs this) we >>> could just merge Maarten's patch as the easier thing to do right now? >>> Maybe with a small code comment that this is intentional? >> Exactly that. Surely this (when the CRTC actually goes dark) is >> something we already know? Assuming you don't have atomic_disable / >> instant-scanout-stop, and have to wait until the next vblank to kill >> it, then how does this work? >> - create FB >> - assign FB to plane on CRTC >> - unreference FB such that plane holds the last remaining reference >> - set plane->fb == NULL + crtc->active = 0, i.e. SetCrtc(NULL, 0, 0, >> 0, 0, ...) >> >> You can't immediately unpin/destroy the FB since you might still be >> mid-scanout. So you already need to defer this, and that would be the >> point at which you stop scanout. In a lot of hardware, this is just >> another latched register which takes effect on the next vblank, for >> which you still catch an IRQ - at which point you send the event. If >> you actually have atomic_disable hardware that stops scanout >> immediately, you can just send the event immediately. >> >> What am I missing? > The userspace which actually wants this and the testcases which makes sure > it works. Until we have that I'd really prefer to just close that > undefined behaviour gap in the atomic api, similar to how we just merged a > patch to disallow switching live planes. > > We can always extend this later on easily, there's lots of little features > like this that people suggested for atomic. > -Daniel Or in this case it's really solved in the kernel. Old fb's don't get unpinned until the update, so why should userspace care about any of it, assuming kernel refcount is correct? ~Maarten _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-08-31 7:48 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-07-30 7:03 drm/atomic: Reject events for inactive crtc's Maarten Lankhorst 2015-08-06 9:47 ` Daniel Stone 2015-08-06 11:19 ` Maarten Lankhorst 2015-08-06 12:49 ` Daniel Vetter 2015-08-27 14:36 ` Daniel Stone 2015-08-27 15:22 ` [Intel-gfx] " Daniel Vetter 2015-08-31 7:48 ` Maarten Lankhorst
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.