On Wed, Feb 12, 2014 at 03:02:13PM -0200, Paulo Zanoni wrote: > 2014-02-12 9:26 GMT-02:00 Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>: > > On Wed, Feb 12, 2014 at 01:13:17PM +0200, Ville Syrjälä wrote: > >> On Thu, Dec 19, 2013 at 07:12:31PM -0200, Paulo Zanoni wrote: > >> > From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > >> > > >> > When I forked haswell_crtc_enable I copied all the code from > >> > ironlake_crtc_enable. The last piece of the function contains a big > >> > comment with a call to intel_wait_for_vblank. After this fork, we > >> > rearranged the Haswell code so that it enables the planes as the very > >> > last step of the modeset sequence, so we're sure that we call > >> > intel_enable_primary_plane after the pipe is really running, so the > >> > vblank waiting functions work as expected. I really believe this is > >> > what fixes the problem described by the big comment, so let's give it > >> > a try and get rid of that intel_wait_for_vblank, saving around 16ms > >> > per modeset (and init/resume). We can always revert if needed :) > >> > >> I noticed this got merged, but I'd actually prefer we go the other way. > >> Ie. remove the vblank wait from enable_primary_plane(). We're going to > >> want to use the atomic update also for enabling the planes at modeset > >> soon enough, so I think this change is going in the wrong direction. > > The enable_primary_plane() wait happens on all gens, and it's there by > design. The code removed by this patch is only for HSW and, considered > the comment block involved, it's just a workaround. It's not a workaround. It's there to prevent completing a subsequent pageflip before it even had a chance to happen. That's still needed. Now it sort of works by accident since enable_primary_plane has the wait, but there's not even a comment saying why the wait is needed there. But I admit, even the original comment was quite vague. Hmm. Now that I think about it more, I think we need to go for the flip counter approach anyway, since if you currently do a set_base w/o actually changing the fb, and then follow that with a page flip, we hit the same problem where the page flip might be completed too early. > So removing the > enable_primary_plane() wait would have been a totally different patch, > affecting all gens, instead of a HSW+ only patch. > > So if you want to move the wait form enable_primary_plane() to the end > of crtc_enable() on _all_ gens, it's fine, but it's a separate patch > with a different purpose. I have a patch for that somewhere. Maybe it was part of the plane enable/disable reordering that only got partially applied (just for HSW). Or it might be a part of my remaining PCH watermark patches. Also we don't need the wait at all on gmch platforms (except vlv) since those don't have a flip done interrupt that gets triggred by mmio flips. So having the wait in enable_primary is a bit wasteful on those platforms. But if you don't want to undo the change, I'll just stick a note to my TODO file to fix it all up at some point. -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
