Created attachment 20318 [details] [review]
Alex  patch rebased to git master.

Created attachment 20319 [details] [review]
Changed flag patch.

Note EXA part of this patch is a hack.

You should just drop the non-XVideo-related parts for now. Avoiding tearing for 2D operations in general is a much more difficult problem and not necessary for XVideo.

Also, if I understand these patches correctly, they always wait for vertical blank. However, they would really only need to wait for scanout to be outside of the vertical range being rendered to.

Patch heavily reduces tearing here, but does not remove it. There is still a triangle tear in top 1/5th of the screen. With a movie exceeding screen refresh rate I got a horisontal tear at roughly the same height.

The problem is that the rendering commands may not finish before the display controller starts scanning again.  Changing between waiting for the rising and falling edge of the vline will probably cause different results depending on the size of the command buffer and data and the time it takes to finish rendering and flush the results out to vram.

(In reply to comment #4)
> Patch heavily reduces tearing here, but does not remove it. There is still a
> triangle tear in top 1/5th of the screen. With a movie exceeding screen refresh
> rate I got a horisontal tear at roughly the same height.
> 

FWIW I noticed some triangular artifacts on my r500 (haven't tried your patches).

If you are running recent git then you get the new bicubic scale by default.

Turning it off with xvattr make these disappear for me.

Created attachment 20454 [details] [review]
Previous two patches merged correctly.

(In reply to comment #5)
> (...) Changing between waiting for the rising and
> falling edge of the vline will probably cause different results depending on
> the size of the command buffer and data and the time it takes to finish
> rendering and flush the results out to vram.

In my case waiting for falling edge was no-op. It was clearly visible in Exa:
waiting for FE had no effect but RE slowed Exa rendering greatly. But 
it's certainly good to hear more test results with both scenarios.

(In reply to comment #4)
> Patch heavily reduces tearing here, but does not remove it. There is still a
> triangle tear in top 1/5th of the screen. With a movie exceeding screen refresh
> rate I got a horisontal tear at roughly the same height.

Could you please test just first Alex patch (without changed flag)? In 
your case FE might be better than RE.

I did some testing previously with just the first patch and it had no real effect on HEAD. I'll try to do some more thorough testing tonight.

I am a bit puzzled why I am getting tearing when it waits for the start of the vblank though. The only running application is mplayer, so there should be extremely little sitting in the command queue for the GPU.

You aren't waiting for vblank, you are waiting for the rising for falling edge of a particular vline.  The line in question is specified in one of the crtc registers.

Also, for those that are interested, I've also played around a bit with double buffered rendering:
http://www.botchco.com/alex/xorg/exa_double_buffer.diff

Ah, I see. So the time we have is just the period between scan lines, not between screen updates? I guess that can be quite brief, especially with reduced blanking (or does that just affect vblank?).

If I'm going to go elbow deep into this, any good pointer into the reference docs about this?

(In reply to comment #11)
> Ah, I see. So the time we have is just the period between scan lines, not
> between screen updates? I guess that can be quite brief, especially with
> reduced blanking (or does that just affect vblank?).
> 

Basically what the patch does is at the start of an acceleration call, I stall the engine until we get to the rising or falling edge or a particular vline.  The vline I chose was the last one of the visible portion of the scanout buffer.  At that point the display controller should be at the start of the blanking period; the stall drops and rendering continues.  Hopefully it finishes before the blanking period ends.  If not you get tearing again; possibly in a different place.  There's no real clean way to do this without pageflipping.  Reduced blanking does reduce the length of the blanking period so you have less time to work with.

> If I'm going to go elbow deep into this, any good pointer into the reference
> docs about this?
> 

The driver code and the chip documentation here:
http://www.x.org/docs/AMD/

(In reply to comment #12)
> Basically what the patch does is at the start of an acceleration call, I stall
> the engine until we get to the rising or falling edge or a particular vline. 
> The vline I chose was the last one of the visible portion of the scanout
> buffer.  At that point the display controller should be at the start of the
> blanking period; the stall drops and rendering continues.  Hopefully it
> finishes before the blanking period ends.  If not you get tearing again;
> possibly in a different place.
Are you sure it really needs to finish before scanout starts again? Since vblank is something like 4% of total time (with non-reduced blanking), that would mean those blits would need to be fast enough for a sustained rate of 1500 fps. I doubt  (at least for the slower chips) that's possible at all (for reasonably sized full-screen video). I thought it should be sufficient if rendering is just ahead of scanout, since AFAIK rasterization happens (roughly) top to bottom. So those 4% should pretty much always be enough for no tearing to happen I think in theory... Though of course it won't do much if compositing is used, at least not without the 2d parts of the patch I guess...

For reference, the nouveau driver does the same thing and it works just fine.

The rendering is done through two triangles, not through a blit. So it isn't just top to bottom, but two passes, once for each triangle.

I've been fiddling around a bit, and it seems like the chip is only able to trigger on the rising edge, nothing else. I can determine this by the fact that a) the tear occurs at random places between runs, indicating that it does not have a stable reference point, and b) a torture test does not get rate limited to the refresh rate.

(a and b are of course giving stable results with the rising edge trigger)

I've been unable to find documentation for this register, so I have no idea how this is supposed to work except for Alex' code.

(In reply to comment #14)
> For reference, the nouveau driver does the same thing and it works just fine.
> 
> The rendering is done through two triangles, not through a blit.
I meant really "3d blit". But you're wrong about the two tris - the code indeed used to do that, and indeed I got full-screen diagonal tearing with this. Currently it uses a rect or quad. (FYI, nouveau seems to use one big tri scissored to a quad for nv40, and a quad for nv50, though I didn't look to closely.)

> So it isn't
> just top to bottom, but two passes, once for each triangle.
Nope.

> 
> I've been fiddling around a bit, and it seems like the chip is only able to
> trigger on the rising edge, nothing else.
No idea, but since you can wait for any vline I don't really see why you'd need to be able to wait for both falling and rising edge anyway.

The driver might be sending a quad, but the hardware is definitely handling it as triangles as there is a huge diagonal tear on my screen. :)

The "nouveau method" of clipping one huge triangle was done after observing how the blob did it IIRC. I have no idea how to implement it here though, but someone who is familiar with the R300 3D engine might.

I've done some more testing of the patches here and concluded that this VLINE waiting is far from obvious. The closest I've come to removing the tears is to set a range from line 650 to 655 (setting a too small interval caused the wait to lock up permanently) and waiting for "VLINE on" (bit 3). I still get tears, but they are equally distributed on the top and bottom.

Not sure how to proceed...

(In reply to comment #16)
> The driver might be sending a quad, but the hardware is definitely handling it
> as triangles as there is a huge diagonal tear on my screen. :)
That's strange as I no longer see this. Maybe could even be dependent on the hardware?

For reference, I have a RS690.

(In reply to comment #17)
> That's strange as I no longer see this. Maybe could even be dependent on the
> hardware?

Yeah, as of R300 the 3D engine no longer seems to provide any primitive that allows rendering a rectangle top to bottom in one pass. So the 'large triangle plus scissor' trick would be needed.

As for the VLINE wait, the way it should be done I think is:

* Set up the CRTC_GUI_TRIG_VLINE register to start at the top vertical line rendered to and end at the bottom line rendered to (these may need to be converted/clipped to the CRTC active vertical range etc.) and set the CRTC_GUI_TRIG_VLINE_INV bit. The register reference suggests that the CRTC_GUI_TRIG_VLINE_STALL bit may also need to be set due to the below.
* Write WAIT_CRTC_VLINE (not *_RE_* or *_FE_*) to WAIT_UNTIL immediately before emitting the 3D primitive. This write should stall the CP while scanout is within the specified range.

Note that the waiting should only be done if the window isn't redirected, i.e. if the destination pixmap is the screen pixmap.

(In reply to comment #19)
> (In reply to comment #17)
> > That's strange as I no longer see this. Maybe could even be dependent on the
> > hardware?
> 
> Yeah, as of R300 the 3D engine no longer seems to provide any primitive that
> allows rendering a rectangle top to bottom in one pass. So the 'large triangle
> plus scissor' trick would be needed.

How difficult is this? Could I, who have no insight into the R300 3D engine, pull it off in an afternoon, or is it a matter of first spending a week learning the hardware?

> 
> As for the VLINE wait, the way it should be done I think is:
> 
> * Set up the CRTC_GUI_TRIG_VLINE register to start at the top vertical line
> rendered to and end at the bottom line rendered to (these may need to be
> converted/clipped to the CRTC active vertical range etc.) and set the
> CRTC_GUI_TRIG_VLINE_INV bit. The register reference suggests that the
> CRTC_GUI_TRIG_VLINE_STALL bit may also need to be set due to the below.
> * Write WAIT_CRTC_VLINE (not *_RE_* or *_FE_*) to WAIT_UNTIL immediately before
> emitting the 3D primitive. This write should stall the CP while scanout is
> within the specified range.
> 

Those bits are for the legacy CRTC control. Is there an equivalent STALL bit for AtomBIOS, and if so, which bit is it?

> Note that the waiting should only be done if the window isn't redirected, i.e.
> if the destination pixmap is the screen pixmap.
> 

Ths wait helper already has this check (and requires the relevant pixmap as an argument). There is also the problem of mplayer getting tossed offscreen when composite is active, but that's another issue (which also should probably be solved in mplayer).

(In reply to comment #20)
> > Yeah, as of R300 the 3D engine no longer seems to provide any primitive that
> > allows rendering a rectangle top to bottom in one pass. So the 'large triangle
> > plus scissor' trick would be needed.
> 
> How difficult is this? Could I, who have no insight into the R300 3D engine,
> pull it off in an afternoon, or is it a matter of first spending a week
> learning the hardware?

I think it should be possible; the only 3D engine specific part should be setting up the scissor rectangle, which you can steal from the RADEONInit3DEngine function (the R300_SC_SCISSOR0/1 registers). You may not even need to set up the scissor rectangle for an initial proof of concept in fullscreen.


> Those bits are for the legacy CRTC control. Is there an equivalent STALL bit
> for AtomBIOS, and if so, which bit is it?

What matters isn't ATOM but AVIVO vs. the legacy display engine. Looking at the patch, there's the AVIVO_D1MODE_VLINE_START_END register.


> There is also the problem of mplayer getting tossed offscreen when composite
> is active, but that's another issue (which also should probably be solved in
> mplayer).

There's nothing mplayer can do about it. With compositing, (avoiding) tearing is up to the compositing manager. Note that every decent compositing manager should be able to unredirect fullscreen windows. I know at least compiz and xfwm4 can.

(In reply to comment #21)
> 
> What matters isn't ATOM but AVIVO vs. the legacy display engine. Looking at the
> patch, there's the AVIVO_D1MODE_VLINE_START_END register.
> 

My question was more along the line of "which bit is STALL". The current radeon_regs.h does not define it.

> 
> > There is also the problem of mplayer getting tossed offscreen when composite
> > is active, but that's another issue (which also should probably be solved in
> > mplayer).
> 
> There's nothing mplayer can do about it. With compositing, (avoiding) tearing
> is up to the compositing manager. Note that every decent compositing manager
> should be able to unredirect fullscreen windows. I know at least compiz and
> xfwm4 can.
> 

Right, the bug is that even without a compositing manager it gets broken. Backingstore (IIRC) was rewritten to use offscreen pixmaps when COMPOSITE is active. And since mplayer asks for such a window, all of its output is put offscreen and not subjected to vsync by most drivers.

I haven't really had time to follow up on this, but someone else might be able to? :)

As for the bug here, I can play around with it tonight and see what I come up with.

(In reply to comment #22)
> My question was more along the line of "which bit is STALL". The current
> radeon_regs.h does not define it.

Maybe it's just not necessary with AVIVO.

In that case I believe the method you described has already been tested without decent results (I'll double check though).

OTOH, there is no define for STALL in the legacy CRTC part either.

(In reply to comment #24)
> In that case I believe the method you described has already been tested without
> decent results (I'll double check though).

The large triangle trick hasn't been tried, has it? I don't think there's any hope of avoiding tearing without it.

Quite right. Results will follow in a few hours (hopefully). :)

Created attachment 20563 [details] [review]
crude proof of concept

This is a crude proof of concept of the ideas above. If someone has an abundance of free time to clean this up, then feel free, otherwise I'll probably have a look at it closer to the weekend. I don't think I have any code in the radeon driver yet, so it should be about time. :)

Outstanding issues/questions:

- VLINE line counting. Is line 0 the first image line?

- Should we allow a write very close to the actual image. I.e. should we have some grace lines between the "ok-to-render area" and the image. Otherwise we might get a small tear if we're unlucky.

- Scissoring needs to be cleaned up by someone who understands it (or provide me with enough info to do it):

  - Why the 1024 + 64 offset?

  - Do we need to restore it once we're done? Perhaps we should use a clip box instead?

  - Can it be set outside the while loop, before the wait?

-  <=R200 and bilinear code of course needs to be fixed. :)

There was probably more things, but I'm too tired to think of them now.

Created attachment 20689 [details] [review]
cleaned up patch

A cleaned up implementation. I was unable to test the bicubic portion as it does not work on RS690. :/

Other outstanding issues:

- The odd offset for scissor coordinates
- What to do about R200. The naming of the scissor registers imply that it isn't available on R200. Is that so? Should we have a third code path for that?

As Roland suggested i think we should just wait for some vline to pass and then start the blit ie not wait for vblank. Little ASCII art to illustrate this :
(not full screen wait)

-- screen
** window for which we wants to avoid tearing


 --------------
|              |
|    *****     |
|    |   |     | <- 
|    |   |     |   |__ stall engine until crtc is in this area then start blit
|    |   |     |   |
|    *****     | <-
|              |
 --------------

Of course the tricky question is the margin we choose btw the first line at which we blit and btw the line we wait, if we wait for same line then blit might out run scanout so we will weird tearing effect. Anyway i think this is the best solution to shorten the amount of time we stall the engine and also to give more time for the blit to happen.

Complete patch at:

http://git.infradead.org/users/drzeus/xf86-video-ati?a=shortlog;h=refs/heads/vsync

Alex tested it on R200 but had problems where the state changes caused problems elsewhere. There was also some issue with fullscreen. Could you describe it in more detail though, Alex?

It is currently untested on R500, so feel free. :)

I have not implemented Jerome's suggestion of waiting for just the target area. Right now it avoids the entire visible screen.

(In reply to comment #30)
> Complete patch at:
> 
> http://git.infradead.org/users/drzeus/xf86-video-ati?a=shortlog;h=refs/heads/vsync
> 
> Alex tested it on R200 but had problems where the state changes caused problems
> elsewhere. There was also some issue with fullscreen. Could you describe it in
> more detail though, Alex?
> 
> It is currently untested on R500, so feel free. :)
> 
> I have not implemented Jerome's suggestion of waiting for just the target area.
> Right now it avoids the entire visible screen.
> 

Dear Pierre & others,
Firstly, thank you for your efforts to resolve this problem.  I am trying to use a Radeon card in my MythTV system, and I've completely given up on the proprietary fglrx drivers and their associated tearing problems.

I've been trying out the patches on this thread on my machine, which uses a Radeon x1250 (RS690) IGP.  I have been attempting to watch MythTV recordings (standard definition HD video) and h264 HDTV rips.

With the patch posted on 2008-11-15 ("Alex's patch rebased to git-master") I got tear-free video except for a triangle in the top right of the screen.  This is a vast improvement on ATI's efforts!

With the "cleaned up patch" posted on 2008-11-29 (applied to git-master of the same day), I had completely working tear-free video (at last!) but there were some problems after exiting full-screen video display -- all of the text on the MythTV menus had disappeared and there were large black strips along all four sides of the screen.  Perhaps something isn't being reset to its initial state when the fullscreen display exits?  Sorry, I'm an ex-C programmer but know next to nothing about graphics hardware!

With the most recent patch posted on 2008-12-04, the tearing was sadly back.

So, for me at least on the RS690, the patch of 2008-11-29 represents the best tear-free video.  If the screen corruption on exit was fixed it would be perfect!

Needless to say, I am happy to test any further patches on my hardware and will report back.

Thanks,
George

That seems odd. It's the same code. The only difference is added support for R200 and R500, but those code paths should not be executed on your card. In fact, the card used for development is an RS690.

There is one more difference though, and that's that EXA vsync is not disabled in the final patch. Perhaps Myth is doing some rendering in parallel with the video that is messing things up.

Alex, should we perhaps drop the EXA stuff for now?

(In reply to comment #32)
> 
> Alex, should we perhaps drop the EXA stuff for now?
> 

yeah, you can just comment out the calls to FUNC_NAME(RADEONWaitForVLine) in radeon_exa_funcs.c and radeon_exa_render.c

(In reply to comment #30)
> Complete patch at:
> 
> http://git.infradead.org/users/drzeus/xf86-video-ati?a=shortlog;h=refs/heads/vsync
> 
> Alex tested it on R200 but had problems where the state changes caused problems
> elsewhere. There was also some issue with fullscreen. Could you describe it in
> more detail though, Alex?

On r200, you have to reset the scissors when you switch to EXA or your rendering gets clipped.  Also you can only scale the video to a certain size before it stops rendering.  I suspect we are hitting a coordinate limit somewhere due to the huge triangle.  All chips support rect lists (not just r100).  That might be a better solution if it renders then as rects rather than as triangles.

(In reply to comment #32)
> That seems odd. It's the same code. The only difference is added support for
> R200 and R500, but those code paths should not be executed on your card. In
> fact, the card used for development is an RS690.

Very sorry, I made a mistake.  I'd compiled the wrong code for what I thought was the patch of 2008-12-04.  Now that I've got the correct code, I can confirm that the patches of 2008-11-29 and 2008-12-04 are functionally equivalent -- that is, tear-free but with strange behaviour after exiting full-screen video.  Will try to get a shot of the screen.

(In reply to comment #34)

> On r200, you have to reset the scissors when you switch to EXA or your
> rendering gets clipped.  Also you can only scale the video to a certain size
> before it stops rendering.  I suspect we are hitting a coordinate limit
> somewhere due to the huge triangle.  All chips support rect lists (not just
> r100).  That might be a better solution if it renders then as rects rather than
> as triangles.
> 

We need to reset the scissors on r300+ as well or it clips subsequent rendering.

If you update your copy of the tree at

http://git.infradead.org/users/drzeus/xf86-video-ati?a=shortlog;h=refs/heads/vsync

you should now have something that works very well. Alex fixed so that the vsync stuff is configurable. It's on by default for video, but off for EXA.

Merged.  thanks!

Created attachment 20832 [details]
Xv corruption with latest ad2579f8898251105a6b36b745afd1ce1dab103e

I tested latest commit ad2579f8898251105a6b36b745afd1ce1dab103e, but I am getting screen corruption on my RV530 on a MacBook Pro. I am using xserver 1.5.3 on a Ubuntu 8.10 with the -ati driver compiled from git source.

See also these screnshots:
with vlc: http://img126.imageshack.us/img126/1809/schermataet8.png
with mplayer: http://img154.imageshack.us/img154/3315/schermata1qq2.png

As discussed with ossman on IRC, the problem go away when disabling compiz:
(14:48:32) ossman: does the problem go away if you turn off compiz?
(14:49:45) fpoibaf: OK. Disabling compiz make the problem disappear
(14:50:37) ossman: great. so we need to do something differently when we're rendering offscreen

Just want to add that there are problems even with compiz disabled, when overlapping other windows (or the menu of the video player) on the Xv area:
http://img126.imageshack.us/img126/6047/schermata2ji5.png

Created attachment 20842 [details] [review]
switch back to quads

something's up with the scissors and offscreen buffers.  Switching back to quads fixes it for me, and doesn't seem to introduce any diagonal tearing.

(In reply to comment #42)
> Created an attachment (id=20842) [details]
> switch back to quads
> 
> something's up with the scissors and offscreen buffers.  Switching back to
> quads fixes it for me, and doesn't seem to introduce any diagonal tearing.
> 

I had exactly the same corruptions as Fabio (my card is a X 1900XT).
Your patch fixed them for me. And as far as I can tell there's still no tearing with disabled compositing. But when compositing is enabled the video's tearing again.

(In reply to comment #43)
> I had exactly the same corruptions as Fabio (my card is a X 1900XT).
> Your patch fixed them for me. And as far as I can tell there's still no tearing
> with disabled compositing. But when compositing is enabled the video's tearing
> again.
> 

When composite is enabled, the video gets drawn to an offscreen buffer rather than to the front buffer, so this won't help in that case.  Enabling EXA and the EXAVSync option should help in the composite case.

Ok, an improper setup of the scissoring caused all of the reported issues. Please try with the current HEAD and things should be working.

OK, I had a chance to try the latest git and I can confirm the corruption problems are gone.

I noticed, however, that while tear is fixed in the non composited case, there is still tearing with compiz, no matter if I use the
Option "EXAVSync" "on"
in my xorg.conf and enable "Sync to vblank" in compiz config.

I would also ask a thing: from what I have understood the current tear-avoiding code is a temporary solution (with reduced performance) in meantime a definitive solution is developed. Is it right? What would be the perfect solution? Double buffering? DRI2? Does the perfect solution require changes to X server or DRM?

Thanks

(In reply to comment #46)
> I noticed, however, that while tear is fixed in the non composited case, there
> is still tearing with compiz,

Yes, as was pointed out before, with compositing it's up to the compositing manager, not the textured video blit itself.

> no matter if I use the Option "EXAVSync" "on"

That only affects X11 rendering, whereas compiz uses OpenGL.

> in my xorg.conf and enable "Sync to vblank" in compiz config.

The sync to vblank method used by compiz can't work with indirect rendering. You can try setting vblank_mode to 2 or 3 in /etc/drirc.

Also note that if you enable the 'Unredirect fullscreen windows' option in compiz, the current solution should work for fullscreen windows even with compiz. Other compositing managers have similar options.


> I would also ask a thing: from what I have understood the current tear-avoiding
> code is a temporary solution (with reduced performance) in meantime a
> definitive solution is developed. Is it right?

I'm not sure there's a better solution for the non-compositing case. In the long term, compiz with DRI2 direct rendering will probably be a better solution.

> The sync to vblank method used by compiz can't work with indirect rendering.
> You can try setting vblank_mode to 2 or 3 in /etc/drirc.
> 
> Also note that if you enable the 'Unredirect fullscreen windows' option in
> compiz, the current solution should work for fullscreen windows even with
> compiz. Other compositing managers have similar options.

I tried setting vblank_mode to 2 or 3 in my .drirc, but I still get tear (note that while glxgears drops to ~60fps, the compiz benchmark still shows a lot higher fps).

Also the 'Unredirect fullscreen windows' option in compiz did not help even in fullscreen mode.

> Also note that if you enable the 'Unredirect fullscreen windows' option in
> compiz, the current solution should work for fullscreen windows even with
> compiz. Other compositing managers have similar options.
Thanks, you're right. I just yesterday installed KDE 4.2 Beta, and know fullscreen-apps seem to be rendered directly by default, and VSync is working nicely (that's without EXAVsync enabled). KDE 4.1's KWIN afaik had no option to unredirect fullscreen windows.

(In reply to comment #48)
> Also the 'Unredirect fullscreen windows' option in compiz did not help even in
> fullscreen mode.
I suspect this is a compiz-bug. I remember trying to use this option awhile ago when I was using fglrx, and iirc it didn't work as well.

(In reply to comment #48)
> I tried setting vblank_mode to 2 or 3 in my .drirc, but I still get tear (note
> that while glxgears drops to ~60fps, the compiz benchmark still shows a lot
> higher fps).

Only some compiz plugin effects use 'normal' buffer swaps, normally compiz uses the GLX_MESA_copy_sub_buffer extension for efficient incremental updates, but that can't be automatically synchronized to the refresh.


> Also the 'Unredirect fullscreen windows' option in compiz did not help even in
> fullscreen mode.

As noted by Henning, compiz doesn't 100% reliably recognize fullscreen windows yet. The compiz 'Extra WM Actions' plugin allows manually (un)redirecting windows, though unfortunately IME that doesn't always work as expected either.