[Intel-gfx] drm/atomic: Reject events for inactive crtc's.

[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 at 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 at 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