Re: [PATCH 3/3] drm/i915: remove the vblank_wait hack from HSW+

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@linux.intel.com>:
> > 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@intel.com>
> >> >
> >> > 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