2014-02-12 16:06 GMT-02:00 Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>: > 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. I always saw this as "it works by design since enable_primary_plane() has the wait". > > 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 -- Paulo Zanoni _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
