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. */
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
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
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
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
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
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@lists.freedesktop.org
-
Daniel Stone -
Daniel Vetter -
Maarten Lankhorst