[Intel-gfx] [PATCH 3/3] drm/i915: remove the vblank_wait hack from HSW+

[Paulo Zanoni]

2014-02-12 16:06 GMT-02:00 Ville Syrjälä <ville.syrjala at 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 at 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 at 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.

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