Re: [PATCH 1/2] drm/i915: call drm_handle_vblank before finish_page_flip

[Chris Wilson <chris@chris-wilson.co.uk>]

On Wed, 03 Oct 2012 11:08:40 +0300, Imre Deak <imre.deak@intel.com> wrote:
> On Tue, 2012-10-02 at 17:54 +0200, Daniel Vetter wrote:
> > ... since finish_page_flip needs the vblank timestamp generated
> > in drm_handle_vblank. Somehow all the gmch platforms get it right,
> > but all the pch platform irq handlers get is wrong. Hooray for copy&
> > pasting!
> > 
> > Currently this gets papered over by a gross hack in finish_page_flip.
> > A second patch will remove that.
> > 
> > Note that without this, the new timestamp sanity checks in flip_test
> > occasionally get tripped up, hence the cc: stable tag.
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Tested-by: Imre Deak <imre.deak@intel.com>
> 
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c |   16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index d7f0066..1fc2489 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -698,12 +698,12 @@ static irqreturn_t ivybridge_irq_handler(DRM_IRQ_ARGS)
> >  			intel_opregion_gse_intr(dev);
> >  
> >  		for (i = 0; i < 3; i++) {
> > +			if (de_iir & (DE_PIPEA_VBLANK_IVB << (5 * i)))
> > +				drm_handle_vblank(dev, i);
> >  			if (de_iir & (DE_PLANEA_FLIP_DONE_IVB << (5 * i))) {
> >  				intel_prepare_page_flip(dev, i);
> >  				intel_finish_page_flip_plane(dev, i);
> >  			}
> > -			if (de_iir & (DE_PIPEA_VBLANK_IVB << (5 * i)))
> > -				drm_handle_vblank(dev, i);
> >  		}
> >  
> >  		/* check event from PCH */
> > @@ -785,6 +785,12 @@ static irqreturn_t ironlake_irq_handler(DRM_IRQ_ARGS)
> >  	if (de_iir & DE_GSE)
> >  		intel_opregion_gse_intr(dev);
> >  
> > +	if (de_iir & DE_PIPEA_VBLANK)
> > +		drm_handle_vblank(dev, 0);
> > +
> > +	if (de_iir & DE_PIPEB_VBLANK)
> > +		drm_handle_vblank(dev, 1);
> > +
> >  	if (de_iir & DE_PLANEA_FLIP_DONE) {
> >  		intel_prepare_page_flip(dev, 0);
> >  		intel_finish_page_flip_plane(dev, 0);
> > @@ -795,12 +801,6 @@ static irqreturn_t ironlake_irq_handler(DRM_IRQ_ARGS)
> >  		intel_finish_page_flip_plane(dev, 1);
> >  	}
> >  
> > -	if (de_iir & DE_PIPEA_VBLANK)
> > -		drm_handle_vblank(dev, 0);
> > -
> > -	if (de_iir & DE_PIPEB_VBLANK)
> > -		drm_handle_vblank(dev, 1);
> > -
> >  	/* check event from PCH */
> >  	if (de_iir & DE_PCH_EVENT) {
> >  		if (pch_iir & hotplug_mask)
> 
> 
From: Chris Wilson <chris@chris-wilson.co.uk>
Subject: Re: [PATCH 2/2] drm/i915: don't frob the vblank ts in finish_page_flip
To: Daniel Vetter <daniel.vetter@ffwll.ch>, Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Cc: Imre Deak <imre.deak@intel.com>, Daniel Vetter <daniel.vetter@ffwll.ch>
In-Reply-To: <1349193276-13908-2-git-send-email-daniel.vetter@ffwll.ch>
References: <1349193276-13908-1-git-send-email-daniel.vetter@ffwll.ch> <1349193276-13908-2-git-send-email-daniel.vetter@ffwll.ch>

On Tue,  2 Oct 2012 17:54:36 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Now that we correctly generate it, this hack is no longer required (and
> might actually paper over a serious bug).
> 
> pageflip timestamps are sanity check in the latest version of the flip-test
> in intel-gpu-tools.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   19 -------------------
>  1 file changed, 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 57c1309..87f825c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6203,25 +6203,6 @@ static void do_intel_finish_page_flip(struct drm_device *dev,
>  		e = work->event;
>  		e->event.sequence = drm_vblank_count_and_time(dev, intel_crtc->pipe, &tvbl);
>  
> -		/* Called before vblank count and timestamps have
> -		 * been updated for the vblank interval of flip
> -		 * completion? Need to increment vblank count and
> -		 * add one videorefresh duration to returned timestamp
> -		 * to account for this. We assume this happened if we
> -		 * get called over 0.9 frame durations after the last
> -		 * timestamped vblank.
> -		 *
> -		 * This calculation can not be used with vrefresh rates
> -		 * below 5Hz (10Hz to be on the safe side) without
> -		 * promoting to 64 integers.
> -		 */
> -		if (10 * (timeval_to_ns(&tnow) - timeval_to_ns(&tvbl)) >
> -		    9 * crtc->framedur_ns) {
> -			e->event.sequence++;
> -			tvbl = ns_to_timeval(timeval_to_ns(&tvbl) +
> -					     crtc->framedur_ns);
> -		}
> -
>  		e->event.tv_sec = tvbl.tv_sec;
>  		e->event.tv_usec = tvbl.tv_usec;
>  
> -- 
> 1.7.10
> 
From: Chris Wilson <chris@chris-wilson.co.uk>
Subject: Re: [PATCH 1/2] drm/i915: call drm_handle_vblank before finish_page_flip
To: Daniel Vetter <daniel.vetter@ffwll.ch>, Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Cc: Imre Deak <imre.deak@intel.com>, Daniel Vetter <daniel.vetter@ffwll.ch>, stable@vger.kernel.org
In-Reply-To: <1349193276-13908-1-git-send-email-daniel.vetter@ffwll.ch>
References: <1349193276-13908-1-git-send-email-daniel.vetter@ffwll.ch>

On Tue,  2 Oct 2012 17:54:35 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> ... since finish_page_flip needs the vblank timestamp generated
> in drm_handle_vblank. Somehow all the gmch platforms get it right,
> but all the pch platform irq handlers get is wrong. Hooray for copy&
> pasting!
> 
> Currently this gets papered over by a gross hack in finish_page_flip.
> A second patch will remove that.
> 
> Note that without this, the new timestamp sanity checks in flip_test
> occasionally get tripped up, hence the cc: stable tag.

Just a touch concerned because I recall no details specifying that the
vblank interrupt will be delivered before or at the same time as the
pageflip-complete interrupt. Not that it would be possible for the
pageflip to complete without a vblank!
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre