[Intel-gfx] Intel-gfx Digest, Vol 57, Issue 7

[Mario Kleiner]

> ------------------------------
>
> Message: 5
> Date: Tue,  2 Oct 2012 17:54:35 +0200
> From: Daniel Vetter <daniel.vetter at ffwll.ch>
> To: Intel Graphics Development <intel-gfx at lists.freedesktop.org>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>, stable at vger.kernel.org
> Subject: [Intel-gfx] [PATCH 1/2] drm/i915: call drm_handle_vblank
> 	before	finish_page_flip
> Message-ID: <1349193276-13908-1-git-send-email-daniel.vetter at ffwll.ch>
>
> ... 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.

Hi Daniel,

as the author of the gross hack in finish_page_flip, the reasoning 
behind that admittedly gross guess-o-matic was to make that part of 
timestamping maintenance-free, with not much need for frequent testing, 
especially given the lack of good tests for it and my lack of test hw 
for ironlake and later.

Papering over such copy & paste bugs was basically the whole idea behind 
it, because many neuro-scientists nowadays use Linux + Intel-KMS with 
recent Intel gpus and absolutely have to rely on the precision and 
robustness of those timestamps. At the same time, most of them would not 
be computer savy enough to compile and install drivers themselves or to 
apply patches, so the driver was meant to be self-healing wrt. timestamps.

I'm fine with removing the hack and fixing this properly, especially if 
you say that it didn't work realiably in some cases. But i hope this 
means that timestamping sanity tests via flip_test are a part of your 
regular QA testing before you release a new kms driver, so bugs get 
caught pre-release? Otherwise many scientists would get pretty nervous 
when using Intel gpus in the future.

Btw. if you remove the hack in the followup patch, you can also remove 
the do_gettimeofday(&tnow).

thanks,
-mario

>
> Cc: stable at vger.kernel.org
> Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
>  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)
> -- 1.7.10 ------------------------------ Message: 6 Date: Tue, 2 Oct 2012 17:54:36 +0200 From: Daniel Vetter <daniel.vetter at ffwll.ch> To: Intel Graphics Development <intel-gfx at lists.freedesktop.org> Cc: Daniel Vetter <daniel.vetter at ffwll.ch> Subject: [Intel-gfx] [PATCH 2/2] drm/i915: don't frob the vblank ts in finish_page_flip Message-ID: <1349193276-13908-2-git-send-email-daniel.vetter at ffwll.ch> 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 at 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

------------------------------

Message: 6
Date: Tue,  2 Oct 2012 17:54:36 +0200
From: Daniel Vetter <daniel.vetter at ffwll.ch>
To: Intel Graphics Development <intel-gfx at lists.freedesktop.org>
Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
Subject: [Intel-gfx] [PATCH 2/2] drm/i915: don't frob the vblank ts in
	finish_page_flip
Message-ID: <1349193276-13908-2-git-send-email-daniel.vetter at ffwll.ch>

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 at 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