diff mbox

Intel-gfx Digest, Vol 57, Issue 7

Message ID 506B2834.7060601@tuebingen.mpg.de (mailing list archive)
State New, archived
Headers show

Commit Message

Mario Kleiner Oct. 2, 2012, 5:45 p.m. UTC
> ------------------------------
>
> Message: 5
> Date: Tue,  2 Oct 2012 17:54:35 +0200
> From: Daniel Vetter <daniel.vetter@ffwll.ch>
> To: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>, stable@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@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@vger.kernel.org
> Signed-off-by: Daniel Vetter <daniel.vetter@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@ffwll.ch> To: Intel Graphics Development <intel-gfx@lists.freedesktop.org> Cc: Daniel Vetter <daniel.vetter@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@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@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@ffwll.ch>
To: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Cc: Daniel Vetter <daniel.vetter@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@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@ffwll.ch>
---
  drivers/gpu/drm/i915/intel_display.c |   19 -------------------
  1 file changed, 19 deletions(-)

  		e->event.tv_usec = tvbl.tv_usec;

-- 1.7.10

Comments

Daniel Vetter Oct. 2, 2012, 6:08 p.m. UTC | #1
On Tue, Oct 2, 2012 at 7:45 PM, Mario Kleiner
<mario.kleiner@tuebingen.mpg.de> wrote:
>
> 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?

Running i-g-t tests is part of our nightly testrun. If you add in the
machines Chris&me have we pretty much cover every platform ever
shipped by Intel ;-)

> Otherwise many scientists would get pretty nervous when using Intel gpus in
> the future.

... so I hope your scientists won't get nervous ;-) I still have a few
things I need to add to the flip test though:
- interactions with wait_vblank (both the blocking version and the
event version)
- checking whether the timestamps are accurately space (since we can
at least read out the line counter, we should be far accurater that 1%
there).
- if you have any ideas for further test, their' highly welcome. One
thing we unfortunately can't really test is whether the timestamp is
really when the hw starts scanning out the first line of the new
frame. But extremely low jitter and whether we pick the right frame
(or accidentally overcorrect by an entire frame like this code
sometimes managed to do) should able be covered.

Eventually I want to also get rid of the delayed vblank disabling and
make the frame counter readout race-free (we probably need to move a
bit of drm core logic into the driver for that). Again, I'll only
merge patches once I'm happy with the evilness of the tests ...

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

Indeed, thanks for spotting this. I'll resend v2 of this patch.

Yours, Daniel
Mario Kleiner Oct. 2, 2012, 9:27 p.m. UTC | #2
On 02.10.12 20:08, Daniel Vetter wrote:
> On Tue, Oct 2, 2012 at 7:45 PM, Mario Kleiner
> <mario.kleiner@tuebingen.mpg.de> wrote:
>>
>> 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?
>
> Running i-g-t tests is part of our nightly testrun. If you add in the
> machines Chris&me have we pretty much cover every platform ever
> shipped by Intel ;-)
>

Cool, that makes me happy :)

>> Otherwise many scientists would get pretty nervous when using Intel gpus in
>> the future.
>
> ... so I hope your scientists won't get nervous ;-) I still have a few
> things I need to add to the flip test though:

I'll have a closer look at flip_test in the near future and see if i 
have some ideas. Psychtoolbox has many built-in tests for timestamp 
consistency. I'll have a look how it compares to flip_test and if some 
of the simple tests could be transplanted. Many of the more complex 
offline tests are unfortunately in Octave scripting language and require 
lots of human intervention, interpretation and special test equipment, 
so not really suitable for quick QA testing.

> - interactions with wait_vblank (both the blocking version and the
> event version)
> - checking whether the timestamps are accurately space (since we can
> at least read out the line counter, we should be far accurater that 1%
> there).

My testing on a Mini Atom Netbook with Intel GMA-950 in 2010 confirmed 
that. The OML_sync_control timestamps were off by less than 0.2 msecs 
wrt. photo-diode measurements on a fast CRT monitor and jitter in the 
low microsecond range < 1 scanline duration. Basically perfect within 
measurement accuracy of the external equipment.

Here's a poster from 2010, for a vision scientist audience, if you are 
interested in the procedure i used and some results:

https://github.com/kleinerm/Psychtoolbox-3/blob/master/Psychtoolbox/PsychDocumentation/ECVP2010Poster_VisualTimingPrecision.pdf

> - if you have any ideas for further test, their' highly welcome. One
> thing we unfortunately can't really test is whether the timestamp is
> really when the hw starts scanning out the first line of the new
> frame. But extremely low jitter and whether we pick the right frame
> (or accidentally overcorrect by an entire frame like this code
> sometimes managed to do) should able be covered.
>

See above: I have the equipment and methods to test that, but 
unfortunately difficulty getting my hands on Intel gpus in our lab, but 
at least during testing at the end of 2010, when i could borrow a few, 
the kms timestamping was basically perfect.

> Eventually I want to also get rid of the delayed vblank disabling and
> make the frame counter readout race-free (we probably need to move a
> bit of drm core logic into the driver for that). Again, I'll only
> merge patches once I'm happy with the evilness of the tests ...
>

Happy to review that once you get at it. I spent a lot of time staring 
at that code after Matthew Garret proposed some patches for early vblank 
disabling a while ago. If i'm not mistaken, the vblank disable/enable 
code in the core should be almost race-free. If you could guarantee that 
the driver->get_vblank_count() function returns a count "as if" the hw 
counter increments always at a fixed point in the scanout, identical for 
all kms drivers, e.g., defined to increment always at the 1st or last 
scanline of vblank, it should be race free. The enable/disable code in 
the core makes the implicit assumption that the hw counter increments 
either at beginning or end of vblank, can't remember atm. which one it was.

Maybe there are better ways on intel hardware, but my sketchy idea was 
to do something like read scanline, read hw vblank counter, read 
scanline, do some comparisons and find out if either +1 needs to be 
added to the returned counter value to fudge it, or if the query needs 
to be repeated once in very rare cases. It worked at least on paper. I 
should really stop making notes only on scrap paper ;-) -- maybe i can 
find it somewhere in my paper pile.

>> Btw. if you remove the hack in the followup patch, you can also remove the
>> do_gettimeofday(&tnow).
>
> Indeed, thanks for spotting this. I'll resend v2 of this patch.
>

Fwiw, you have my

Reviewed-by: mario.kleiner@tuebingen.mpg.de

thanks,
-mario
diff mbox

Patch

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;