From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 3/3] drm/i915: remove the vblank_wait hack from HSW+ Date: Wed, 12 Feb 2014 20:06:20 +0200 Message-ID: <20140212180620.GG3891@intel.com> References: <1387487551-1612-1-git-send-email-przanoni@gmail.com> <1387487551-1612-3-git-send-email-przanoni@gmail.com> <20140212111317.GC3891@intel.com> <20140212112636.GD3891@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 537A0FAF2C for ; Wed, 12 Feb 2014 10:06:25 -0800 (PST) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Paulo Zanoni Cc: Intel Graphics Development , Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org On Wed, Feb 12, 2014 at 03:02:13PM -0200, Paulo Zanoni wrote: > 2014-02-12 9:26 GMT-02:00 Ville Syrj=E4l=E4 : > > On Wed, Feb 12, 2014 at 01:13:17PM +0200, Ville Syrj=E4l=E4 wrote: > >> On Thu, Dec 19, 2013 at 07:12:31PM -0200, Paulo Zanoni wrote: > >> > From: Paulo Zanoni > >> > > >> > 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=E4l=E4 Intel OTC