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