Re: [PATCH 28/28] drm/atomic-helper: Reject legacy flips on a disabled pipe

From: Daniel Vetter <daniel@ffwll.ch>
To: Daniel Stone <daniel@fooishbar.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Daniel Stone <daniels@collabora.com>,
	DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 28/28] drm/atomic-helper: Reject legacy flips on a disabled pipe
Date: Tue, 8 Dec 2015 09:23:23 +0100	[thread overview]
Message-ID: <20151208082323.GC20822@phenom.ffwll.local> (raw)
In-Reply-To: <CAPj87rNvWYVrSpn5MDfE=eC4DZgBEkcXHSrMKUbt1ExTGk28KA@mail.gmail.com>

On Mon, Dec 07, 2015 at 03:33:00PM +0000, Daniel Stone wrote:
> Hi,
> 
> On 4 December 2015 at 08:46, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > +       /*
> > +        * 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
> > +        * compositors drawing loop. This is consistent with the vblank ioctl
> > +        * which also rejects service on a disabled pipe.
> > +        */
> 
> Sorry to keep whingeing, but this isn't actually related to event
> generation. To the best of my knowledge, this change does a few
> things. Firstly, comments the check above (enforcing that (flags &
> ALLOW_MODESET) == {crtcs}->allow_modeset), which is good. But the
> comment is incorrect, because whether or not an event is requested is
> wholly unrelated. Secondly, it disables allow_modeset on pageflip,
> which shouldn't be changing DPMS stage (good!). Thirdly, it enforces
> something like the above statement on pageflips, except again it has
> no regard to events: it just enforces the no-DPMS-on-flip rule, for
> which event generation is a subset.

Well the comment is completely misplace, I wanted to put it next to the
check for event generation, not here.

> If you fix the above comment to more accurately note that this just
> enforces that you need the ALLOW_MODESET flag to change active, mode
> or connector routing, and (as Thierry asked), add a comment below to
> note that we enforce existing no-DPMS-on-flip semantics in the helper,
> then you're welcome to my R-b. But please don't mention events in the
> new comment. :)

Hm, I didn't really want to type a comment for ALLOW_MODESET - imo it's
pretty clear what it does and why it's useful. I'll try again at making
something coheren here ;-)
-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