drm/atomic: Reject events for inactive crtc's.

Commit Message

Maarten Lankhorst July 30, 2015, 7:03 a.m. UTC

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 <[e-mail]>
---

Comments

Daniel Stone Aug. 6, 2015, 9:47 a.m. UTC | #1

Hi,

On 30 July 2015 at 08:03, Maarten Lankhorst
<[e-mail]> 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 <[e-mail]>
> ---
> 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

Maarten Lankhorst Aug. 6, 2015, 11:19 a.m. UTC | #2

Hey,

Op 06-08-15 om 11:47 schreef Daniel Stone:
> Hi,
>
> On 30 July 2015 at 08:03, Maarten Lankhorst
> <[e-mail]> 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 <[e-mail]>
>> ---
>> 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

Daniel Vetter Aug. 6, 2015, 12:49 p.m. UTC | #3

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
> > <[e-mail]> 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 <[e-mail]>
> >> ---
> >> 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 Stone Aug. 27, 2015, 2:36 p.m. UTC | #4

Hi,

On 6 August 2015 at 13:49, Daniel Vetter <[e-mail]> 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
>> > <[e-mail]> 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

Daniel Vetter Aug. 27, 2015, 3:22 p.m. UTC | #5

On Thu, Aug 27, 2015 at 03:36:09PM +0100, Daniel Stone wrote:
> Hi,
> 
> On 6 August 2015 at 13:49, Daniel Vetter <[e-mail]> 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
> >> > <[e-mail]> 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

Maarten Lankhorst Aug. 31, 2015, 7:48 a.m. UTC | #6

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 <[e-mail]> 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
>>>>> <[e-mail]> 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

Patch

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. */