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. 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 think the only issue with dropping the vblank wait from primary >> enable is the IPS enable, but that should be made async anyway. For >> now we could just move the vblank wait into enable_ips(). > > Oh and additionally, if we want to, we could get rid of the vblank wait > (at least on some platforms) by using the hardware flip counter to make > sure we don't prematurely complete a subsequent page flip. > >> >> > >> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> >> > --- >> > drivers/gpu/drm/i915/intel_display.c | 10 ---------- >> > 1 file changed, 10 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> > index f0f78d3..4f933f2 100644 >> > --- a/drivers/gpu/drm/i915/intel_display.c >> > +++ b/drivers/gpu/drm/i915/intel_display.c >> > @@ -3720,16 +3720,6 @@ static void haswell_crtc_enable(struct drm_crtc *crtc) >> > * to change the workaround. */ >> > haswell_mode_set_planes_workaround(intel_crtc); >> > haswell_crtc_enable_planes(crtc); >> > - >> > - /* >> > - * There seems to be a race in PCH platform hw (at least on some >> > - * outputs) where an enabled pipe still completes any pageflip right >> > - * away (as if the pipe is off) instead of waiting for vblank. As soon >> > - * as the first vblank happend, everything works as expected. Hence just >> > - * wait for one vblank before returning to avoid strange things >> > - * happening. >> > - */ >> > - intel_wait_for_vblank(dev, intel_crtc->pipe); >> > } >> > >> > static void ironlake_pfit_disable(struct intel_crtc *crtc) >> > -- >> > 1.8.3.1 >> > >> > _______________________________________________ >> > Intel-gfx mailing list >> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> -- >> Ville Syrjälä >> Intel OTC > > -- > Ville Syrjälä > Intel OTC -- Paulo Zanoni _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
