Bug 21076 - [945GME] [KMS] XV_SYNC_TO_VBLANK does not prevent tearing of xv video
Summary: [945GME] [KMS] XV_SYNC_TO_VBLANK does not prevent tearing of xv video
Status: RESOLVED FIXED
Alias: None
Product: xorg
Classification: Unclassified
Component: Driver/intel (show other bugs)
Version: git
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: Carl Worth
QA Contact: Xorg Project Team
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-04-06 18:54 UTC by Eric Smith
Modified: 2009-05-26 16:38 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Xorg.0.log (13.58 KB, text/plain)
2009-04-06 18:54 UTC, Eric Smith
no flags Details
xorg.conf (1.03 KB, text/plain)
2009-04-06 18:54 UTC, Eric Smith
no flags Details
dmesg (15.09 KB, text/plain)
2009-04-06 18:56 UTC, Eric Smith
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Smith 2009-04-06 18:54:23 UTC
Created attachment 24619 [details]
Xorg.0.log

I am using latest xorg and intel drives on 2.6.29-zen2.  When I run video in mplayer with xv video output, I get very obvious tearing.  Changing the value of XV_SYNC_TO_VBLANK does not affect it.  I have tried running mplayer in just X (no window or compositing manager) and that doesn't help either.
Comment 1 Eric Smith 2009-04-06 18:54:54 UTC
Created attachment 24620 [details]
xorg.conf
Comment 2 Eric Smith 2009-04-06 18:56:06 UTC
Created attachment 24621 [details]
dmesg
Comment 3 haihao 2009-04-06 19:55:53 UTC
Does this commit work for you?

commit bc3312fd7c03d09a231dfebfe390fe668ad15d1e
Author: Carl Worth <cworth@cworth.org>
Date:   Mon Apr 6 11:16:40 2009 -0700

    Use WAIT_FOR_SCAN_LINE instead of WAIT_FOR_VBLANK
Comment 4 Shuang He 2009-04-07 00:07:02 UTC
(In reply to comment #3)
> Does this commit work for you?
> 
> commit bc3312fd7c03d09a231dfebfe390fe668ad15d1e
> Author: Carl Worth <cworth@cworth.org>
> Date:   Mon Apr 6 11:16:40 2009 -0700
> 
>     Use WAIT_FOR_SCAN_LINE instead of WAIT_FOR_VBLANK
> 

I have tried this commit on 2.7 branch, it turns out it's working on pipe 1, but not working on pipe 0. Here's the results:
on 915GM, with a LVDS output on pipe 1, it's working
on Q965, with a VGA output on pipe 0, it's not working
on 945GME, with LVDS and VGA, it's working, if turn off LVDS, VGA is not working.

And if I roll back to commit 5944f5e32511984b11decc0df6074600e1989934, both pipes are working. 

Carl, could you have a look at this?

Comment 5 Shuang He 2009-04-07 00:28:17 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > Does this commit work for you?
> > 
> > commit bc3312fd7c03d09a231dfebfe390fe668ad15d1e
> > Author: Carl Worth <cworth@cworth.org>
> > Date:   Mon Apr 6 11:16:40 2009 -0700
> > 
> >     Use WAIT_FOR_SCAN_LINE instead of WAIT_FOR_VBLANK
> > 
> 
> I have tried this commit on 2.7 branch, it turns out it's working on pipe 1,
> but not working on pipe 0. Here's the results:
> on 915GM, with a LVDS output on pipe 1, it's working
> on Q965, with a VGA output on pipe 0, it's not working
> on 945GME, with LVDS and VGA, it's working, if turn off LVDS, VGA is not
> working.
> 
> And if I roll back to commit 5944f5e32511984b11decc0df6074600e1989934, both
> pipes are working. 
> 
> Carl, could you have a look at this?
> 

Just found the MI_WAIT_FOR_PIPEA_SCAN_LINE_WINDOW should be bit 1. 

diff --git a/src/i810_reg.h b/src/i810_reg.h
index c964569..5211400 100644
--- a/src/i810_reg.h
+++ b/src/i810_reg.h
@@ -2442,7 +2442,7 @@ SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
 #define MI_WAIT_FOR_PIPEB_VBLANK               (1<<7)
 #define MI_WAIT_FOR_PIPEB_SCAN_LINE_WINDOW     (1<<5)
 #define MI_WAIT_FOR_PIPEA_VBLANK               (1<<3)
-#define MI_WAIT_FOR_PIPEA_SCAN_LINE_WINDOW     (1<<2)
+#define MI_WAIT_FOR_PIPEA_SCAN_LINE_WINDOW     (1<<1)

 /* Set the scan line for MI_WAIT_FOR_PIPE?_SCAN_LINE_WINDOW */
 #define MI_LOAD_SCAN_LINES_INCL      
Comment 6 Eric Smith 2009-04-07 09:56:31 UTC
(In reply to comment #3)
> Does this commit work for you?
> 
> commit bc3312fd7c03d09a231dfebfe390fe668ad15d1e
> Author: Carl Worth <cworth@cworth.org>
> Date:   Mon Apr 6 11:16:40 2009 -0700
> 
>     Use WAIT_FOR_SCAN_LINE instead of WAIT_FOR_VBLANK
> 

No.  It didn't work before that commit and it isn't working after it either.
Comment 7 Carl Worth 2009-04-07 12:38:42 UTC
(In reply to comment #5)
> Just found the MI_WAIT_FOR_PIPEA_SCAN_LINE_WINDOW should be bit 1. 
> 
> diff --git a/src/i810_reg.h b/src/i810_reg.h
> index c964569..5211400 100644
> --- a/src/i810_reg.h
> +++ b/src/i810_reg.h
> @@ -2442,7 +2442,7 @@ SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
>  #define MI_WAIT_FOR_PIPEB_VBLANK               (1<<7)
>  #define MI_WAIT_FOR_PIPEB_SCAN_LINE_WINDOW     (1<<5)
>  #define MI_WAIT_FOR_PIPEA_VBLANK               (1<<3)
> -#define MI_WAIT_FOR_PIPEA_SCAN_LINE_WINDOW     (1<<2)
> +#define MI_WAIT_FOR_PIPEA_SCAN_LINE_WINDOW     (1<<1)
> 
>  /* Set the scan line for MI_WAIT_FOR_PIPE?_SCAN_LINE_WINDOW */
>  #define MI_LOAD_SCAN_LINES_INCL      

Thanks for the catch!

I had also noticed that my fix wasn't working on both pipes, and I double-checked the values for MI_LOAD_SCAN_LINES_DISPLAY_PIPEA vs. MI_LOAD_SCAN_LINES_DISPLAY_PIPEB, but forgot to look at these values as well.

I just tested with this fix, and I now get tear-free video on either pipe, (whether only a single pipe is enabled or both). Of course, if I have clone-mode enabled, then I only get tear-free video on one of the two pipes, but that is expected, (and is the same as the behavior before my changes).

I've now pushed this patch out to both master and the 2.7 branch.

Eric, can you see what you get with this change as well, (on top of my previous change mentioned earlier).

-Carl
Comment 8 Eric Smith 2009-04-07 18:36:18 UTC
No, still no change.  Is there any additional information I could provide that would help?
Comment 9 Carl Worth 2009-04-08 13:21:12 UTC
(In reply to comment #8)
> No, still no change.  Is there any additional information I could provide that
> would help?

Hi Eric,

It's strange that things aren't working for you.

From your Xorg.log file it looks like you have two displays enabled, (one LVDS---such as a laptop panel, and one VGA---such as an external monitor). Is that correct? If so, does the tearing occur on both displays?

It's actually expected that tearing will occur on one of the displays if both are enabled. If you want tear-free video on a specific display, one option to get that is to turn the other display off, with a command such as:

    xrandr --output VGA --off

or:

    xrandr --output LVDS --off

Give those a try and see if you don't have any luck.

Otherwise, I'm stumped as to what the difference might be that things are working for me but not for you.

-Carl
Comment 10 Eric Smith 2009-04-08 15:14:12 UTC
(In reply to comment #9)
> (In reply to comment #8)
> > No, still no change.  Is there any additional information I could provide that
> > would help?
> 
> Hi Eric,
> 
> It's strange that things aren't working for you.
> 
> From your Xorg.log file it looks like you have two displays enabled, (one
> LVDS---such as a laptop panel, and one VGA---such as an external monitor). Is
> that correct? If so, does the tearing occur on both displays?
> 
> It's actually expected that tearing will occur on one of the displays if both
> are enabled. If you want tear-free video on a specific display, one option to
> get that is to turn the other display off, with a command such as:
> 
>     xrandr --output VGA --off
> 
> or:
> 
>     xrandr --output LVDS --off
> 
> Give those a try and see if you don't have any luck.
> 
> Otherwise, I'm stumped as to what the difference might be that things are
> working for me but not for you.
> 
> -Carl
> 


I only have a VGA connected, but an LVDS shows up as connected because of a bug in the detection code. (Bug #20089 had fixed this, but the patch was later removed because it caused other problems). I now manually disable it as you can see in my xorg.conf.  Here is what my xrandr shows:

gentoo ~ # xrandr --verbose
Screen 0: minimum 320 x 200, current 1280 x 1024, maximum 2048 x 2048
VGA1 connected 1280x1024+0+0 (0x3c) normal (normal left inverted right x axis y axis) 338mm x 270mm
        Identifier: 0x3b                                                                           
        Timestamp:  52520126                                                                       
        Subpixel:   unknown                                                                        
        Clones:                                                                                    
        CRTC:       0                                                                              
        CRTCs:      0 1                                                                            
        Transform:  1.000000 0.000000 0.000000                                                     
                    0.000000 1.000000 0.000000                                                     
                    0.000000 0.000000 1.000000                                                     
                   filter:                                                                         
  1280x1024 (0x3c)  108.0MHz +HSync +VSync *current +preferred                                     
        h: width  1280 start 1328 end 1440 total 1688 skew    0 clock   64.0KHz                    
        v: height 1024 start 1025 end 1028 total 1066           clock   60.0Hz
  1280x1024 (0x3d)  135.0MHz +HSync +VSync
        h: width  1280 start 1296 end 1440 total 1688 skew    0 clock   80.0KHz
        v: height 1024 start 1025 end 1028 total 1066           clock   75.0Hz
...
Comment 11 Carl Worth 2009-04-08 16:25:24 UTC
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > No, still no change.  Is there any additional information I could provide that
> > > would help?
> I only have a VGA connected, but an LVDS shows up as connected because of a bug
> in the detection code. (Bug #20089 had fixed this, but the patch was later
> removed because it caused other problems). I now manually disable it as you can
> see in my xorg.conf.

OK. I was just taking a guess, which is all I can do from here, I'm afraid.

I think your next best bet is to go sprinkling some ErrorF calls into src/i830_video.c to see if the logic is wrong and its not even attempting to sync, or if the logic is correct, but for some reason the sync commands aren't behaving on your GPU.

In particular, it's the I830PutImage function that you'll want to instrument, and the critical code is happening in the "if (sync)" block. So you might even start with just:

    if (sync) {
        ErrorF ("In if (sync) block");
        ...
    }

and then look in your log file to see if that message is getting there.

And then poke around similarly to ensure its syncing on the correct pipe, etc.

-Carl
Comment 12 Eric Smith 2009-04-09 16:07:36 UTC
> OK. I was just taking a guess, which is all I can do from here, I'm afraid.
> 
> I think your next best bet is to go sprinkling some ErrorF calls into
> src/i830_video.c to see if the logic is wrong and its not even attempting to
> sync, or if the logic is correct, but for some reason the sync commands aren't
> behaving on your GPU.
> 
> In particular, it's the I830PutImage function that you'll want to instrument,
> and the critical code is happening in the "if (sync)" block. So you might even
> start with just:
> 
>     if (sync) {
>         ErrorF ("In if (sync) block");
>         ...
>     }
> 
> and then look in your log file to see if that message is getting there.
> 
> And then poke around similarly to ensure its syncing on the correct pipe, etc.
> 
> -Carl
> 

Ok, so I added some ErrorF calls and it does get inside of "if (sync)".  I don't really understand how it works, but I was expecting pipe to be set to either 0 or 1 based on the output for crtc in xrandr --verbose.  But crtc->pipe has value 8585296 (5 lines into "if (sync)"), and pipe is set to 1048576 (6 lines later).  Could this indicate a problem?
Comment 13 Carl Worth 2009-04-15 11:31:06 UTC
(In reply to comment #12) 
> Ok, so I added some ErrorF calls and it does get inside of "if (sync)".  I
> don't really understand how it works, but I was expecting pipe to be set to
> either 0 or 1 based on the output for crtc in xrandr --verbose.  But crtc->pipe
> has value 8585296 (5 lines into "if (sync)"), and pipe is set to 1048576 (6
> lines later).  Could this indicate a problem?

Interesting. So that value of 8585296 is something we need to figure out where it's coming from. The code in the driver is certainly written to expect that intel_crtc == 0 means pipe A, and any non-zero value means pipe B.

Meanwhile, the value for "pipe" of 1048576 (== 1<<20) is expected for pipe B, (see the definition of MI_LOAD_SCAN_LINES_DISPLAY_PIPEB in i810_reg.h.

So it could be that the problem is just that the driver is trying to sync on pipe B when it should be syncing on pipe A. Why don't you try changing the conditional inside the "if (sync)" block so that both "event" and "pipe" are set for pipe A, and see if that fixes your problem.

If so, we'll next just need to figure out why the code isn't getting what it expects in intel_crtc->pipe.

-Carl
Comment 14 Eric Smith 2009-04-16 10:26:35 UTC
(In reply to comment #13)
> Interesting. So that value of 8585296 is something we need to figure out where
> it's coming from. The code in the driver is certainly written to expect that
> intel_crtc == 0 means pipe A, and any non-zero value means pipe B.
> 
> Meanwhile, the value for "pipe" of 1048576 (== 1<<20) is expected for pipe B,
> (see the definition of MI_LOAD_SCAN_LINES_DISPLAY_PIPEB in i810_reg.h.
> 
> So it could be that the problem is just that the driver is trying to sync on
> pipe B when it should be syncing on pipe A. Why don't you try changing the
> conditional inside the "if (sync)" block so that both "event" and "pipe" are
> set for pipe A, and see if that fixes your problem.
> 
> If so, we'll next just need to figure out why the code isn't getting what it
> expects in intel_crtc->pipe.
> 
> -Carl
> 

Yes, that seems to be the problem.  Forcing "event" and "pipe" to be set to pipe A gives me beautiful tear-free video! :)  I'll try to figure out where intel_crtc->pipe gets the bad value, but any hints on where to look would be be helpful!
Thanks,
Eric

Comment 15 Carl Worth 2009-04-16 10:42:47 UTC
(In reply to comment #14)
> Yes, that seems to be the problem.  Forcing "event" and "pipe" to be set to
> pipe A gives me beautiful tear-free video! :)  I'll try to figure out where
> intel_crtc->pipe gets the bad value, but any hints on where to look would be be
> helpful!

Excellent news!

You can start in i830_crtc_init where it should first be getting assigned. I don't see any other assignments to this value in a quick glance at the code. So if it's correct there, then you'll likely need to next attach a debugger and set a watch on the *(&intel_crtc->pipe) value to get the debugger to stop at the point where the value is getting smashed.

Thanks again for your help here. Let me know if you need anything more.

-Carl
Comment 16 Eric Smith 2009-04-20 15:59:15 UTC
(In reply to comment #15)
> (In reply to comment #14)
> > Yes, that seems to be the problem.  Forcing "event" and "pipe" to be set to
> > pipe A gives me beautiful tear-free video! :)  I'll try to figure out where
> > intel_crtc->pipe gets the bad value, but any hints on where to look would be be
> > helpful!
> 
> Excellent news!
> 
> You can start in i830_crtc_init where it should first be getting assigned. I
> don't see any other assignments to this value in a quick glance at the code. So
> if it's correct there, then you'll likely need to next attach a debugger and
> set a watch on the *(&intel_crtc->pipe) value to get the debugger to stop at
> the point where the value is getting smashed.
> 
> Thanks again for your help here. Let me know if you need anything more.
> 
> -Carl
> 

I finally had some time to look at this, but I'm a little confused.  It seems like the only place that i830_crtc_init is actually called is inside of I830AccelMethodInit on line 1680 of i830_driver.c.  But I830AccelMethodInit is only called if pI830->use_drm_mode is false (which it isn't, see line 1913 of i830_driver.c).
A simple grep of the source shows that i830_crtc_init is the only place intel_crtc->pipe is assigned.  So since i830_crtc_init doesn't seem to be called, where else should I look? 
Sorry if I'm missing something obvious because of my nonexistent programming skills..

Eric
Comment 17 Carl Worth 2009-04-21 15:25:19 UTC
(In reply to comment #16)
> I finally had some time to look at this, but I'm a little confused.  It seems
> like the only place that i830_crtc_init is actually called is inside of
> I830AccelMethodInit on line 1680 of i830_driver.c.  But I830AccelMethodInit is
> only called if pI830->use_drm_mode is false (which it isn't, see line 1913 of
> i830_driver.c).
> A simple grep of the source shows that i830_crtc_init is the only place
> intel_crtc->pipe is assigned.  So since i830_crtc_init doesn't seem to be
> called, where else should I look? 
> Sorry if I'm missing something obvious because of my nonexistent programming
> skills..

It's not a problem with your programming skills. I'm sorry I led you astray.

So there are two different paths in the driver---one for kernel mode-setting (KMS) and one for user mode-setting (UMS), as controlled by the "use_drm_mode" flag, (which would be more clearly named use_kms, but there you have it).

So, anyway, if you are using the KMS case, then the code that initializes intel_crtc->pipe is instead in the kernel source, at drivers/gpu/drm/i915/intel_display.c:intel_crtc_init.

One thing you could do before diving into the kernel is to see if you get tear-free XV with KMS turned off. For this, before starting the X server, try the following:

    sudo rmmod i915
    sudo modprobe i915 modeset=0

Then start X and see if intel_crtc->pipe is set correctly, and see if you get the desired tear-free behavior.

-Carl
Comment 18 Carl Worth 2009-04-21 15:43:19 UTC
(In reply to comment #17)
> One thing you could do before diving into the kernel is to see if you get
> tear-free XV with KMS turned off. For this, before starting the X server, try
> the following:

I've done this now the opposite direction.

I enabled KMS and have now successfully reproduced the bug (hurrah!). The bug is definitely KMS-specific, so I've re-titled it that way.

Maybe the kernel and user-space code are seeing different definitions for the intel_crtc struct somehow? Anyway, I'll investigate and keep you posted.

Thanks again for your debugging efforts that pointed us the right direction to find where the bug is at least.

-Carl
Comment 19 Carl Worth 2009-04-21 16:32:56 UTC
I've talked with Jesse a bit about this bug.

The code in i830_video.c is definitely very broken for the KMS case. It's misinterpreting some memory so getting effectively random data and acting on it.

Also, what the code wants is the pipe value corresponding to a given CRTC, but in the KMS case, the kernel driver doesn't expose this information at all.

So we're likely going to need some new interface in the kernel driver to do what we want. And we already knew that we wanted a way for arbitrary applications, (other than XV users---compositing managers for example), to ask for a tearing-free copyregion operation. This will be necessary to allow for tear-free XV while a compositing manager is running.

So hopefulyl we can do one piece of work and eliminate XV tearing for both the KMS and the compositing-manager cases.

I'll try to learn this week exactly what will be needed to pull that off.

-Carl
Comment 20 Francesco R 2009-04-27 07:56:20 UTC
Option		"Device"	"/dev/input/event2
^^^ this line in the original xorg.conf has a syntax error, the missing '"', does it matter ?
Comment 21 Eric Smith 2009-04-27 09:13:10 UTC
(In reply to comment #20)
> Option          "Device"        "/dev/input/event2
> ^^^ this line in the original xorg.conf has a syntax error, the missing '"',
> does it matter ?
> 

It's no problem. I must have accidentally deleted it while I was removing some commented lines.
Comment 22 Carl Worth 2009-05-26 16:38:26 UTC
(In reply to comment #19) 
> So we're likely going to need some new interface in the kernel driver to do
> what we want. And we already knew that we wanted a way for arbitrary
> applications, (other than XV users---compositing managers for example), to ask
> for a tearing-free copyregion operation. This will be necessary to allow for
> tear-free XV while a compositing manager is running.
> 
> So hopefulyl we can do one piece of work and eliminate XV tearing for both the
> KMS and the compositing-manager cases.
> 
> I'll try to learn this week exactly what will be needed to pull that off.

The code to enable tear-free syncing of video-playback with KMS has now all landed in the xf86-video-intel driver repository as well as in the upstream kernel repository, (ie. Linus' repository as of v2.6.30-rc6), and in the 2.4.11 libdrm (which has already been released as well).

So getting the code for each of those from git will fix the bug. And once the following releases happen, they will contain the fix as well:

      Linux: 2.6.30
      libdrm: 2.4.11
      xf86-video-intel: 2.8.0

-Carl


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.