Bug 8811

post your patch so it can be reviewed for inclusion.  Make sure you are not
merging any GPLed code.

it's already there, in the url field

the usual coding style issues, and apparently dri + xv = crash.

(In reply to comment #2)
> it's already there, in the url field
> 

Ah missed that.

> the usual coding style issues, and apparently dri + xv = crash.

that's probably the same bug that was in there when Vladimir released a similar
patch two years ago:
http://lists.freedesktop.org/pipermail/xorg/2004-December/005371.html
http://lists.freedesktop.org/pipermail/xorg/2005-January/005609.html
Probably a CCE wait-for-idle before the CCE is initialized.

How can I fix that?
Sorry, I'm a beginner hacking the x.org-code :-)

(In reply to comment #4)
> How can I fix that?
> Sorry, I'm a beginner hacking the x.org-code :-)

Go through the patched r128 driver code and look for macros that try and idle
the CCE before it is initialized.  I'd start with r128preinit() in r128_driver.c
and move out from there.  You can take a look at the radeon code for reference.

Sorry about the phenomenal bug spam, guys.  Adding xorg-team@ to the QA contact so bugs don't get lost in future.

Created attachment 8969 [details] [review]
xf86-video-ati-6.6.3-r128-xvideo.patch

Here is my patch based on the same code.
Not knowing about this bug, I have created another patch from scratch.

I spent several days in diff clean-up and removed all code, where GATOS chunk contained older code. My code does not change any file without r128_ in name.

It works on my AIW R128 32MB. Tested on TV-input.

It is a patch against xf86-video-ati-6.6.3 from openSUSE 10.2.

Known issue:
- No support for stereo decoder

My patch works with DRI.

Support for Rage Theatre 200 was not added to my patch. I doubt there is any R128 + RT200 card.

Another issue I found. I am not sure that I will be able to fix it:
- Use of XRandR or screen mode change freezes overlay video.

I will verify these issues known from older versions of R128 drivers (not GATOS related):
- Use of GL or overlay video sometimes corrupts hw cursor colormap.
- Ugly graphic effects while starting the server (cosmetical regression sometimes between xorg 6.8.1 and 6.9.0).
- Display corruption when overlay video is completely off-screen (not seen a long time, probably does not occur any more).
- Compare Radeon and R128 XV_* controls and scales and sync them.
- Improve video size XV_* controls for capture to allow arbitrary sizes.
- Verify, whether it can still be compiled without XVideo support.

Some of the known issues are serious:

- Use of XRandR or screen mode change freezes overlay video.
- Use of GL or overlay video sometimes corrupts hw cursor colormap.

As long as these are not fixed I hesitate from applying. At least RandR shouldn't freeze anything. Does RandR *only* freeze the overlay video, and can it be restarted? In that case we could think about it.


Stanislavs comment:

>- Use of XRandR or screen mode change freezes overlay video.
Problem of my patch. After xrandr call Xvideo window displays static
image. It is enough to restart application, which was using Xvideo; no
crash or server freeze.
Problem is not critical. I can try to fix it in future by disabling
Xvideo before screen mode change.

>- Use of GL or overlay video sometimes corrupts hw cursor colormap.
Perfectly reproducible without my patch.

[...] same with the other issues.


So no regressions, except for the RandR problem. I would like to have that fixed (so it at least works as before). Also I have a couple of other points with that patch.

Given that R128 isn't that widely used any more, and we probably won't get better integration of the GATOS features in the near future, I would commit the changes after the issues have been addressed - if nobody objects.

Michael, any chance you could try Stanislav's patch instead of yours and try whether it works for you as well? It's much cleaner, and will probably provide less headaches after being committed...

Sorry for the delay, I have a lot of abi-stress atm. Next weak it will be done :-)
Whiletime, X.org was upgraded to 7.2 (ubuntu), and if I saw right this patch was for X.org 6.8?

I'll try it next week.

Many thanks,
Michael Fritscher

Created attachment 10492 [details] [review]
xf86-video-ati-6.6.3-compile-cfb.patch

This incremental patch makes cfb compiling again. But it does not mean, that it will work. In 8bpp mode, all applications crash, in 16bpp and 24bpp mode nothing crashes, but some elements are imporperly half scaled and the result is unusable.
I guess everybody can live without cfb code, because it is disabled for years.

Created attachment 10493 [details] [review]
xf86-video-ati-6.6.3-if0.patch

Incremental patch to explain code I was pointed to by Matthias Hopf:

I'm a bit startled by this part of the patch:

@@ -4454,11 +4647,14 @@
     if (info->DGAModes)          xfree(info->DGAModes);
     info->DGAModes               = NULL;
 
+#if 0
     if (info->adaptor) {
+       R128ShutdownVideo(pScrn, info->adaptor->pPortPrivates[0].ptr);
         xfree(info->adaptor->pPortPrivates[0].ptr);
        xf86XVFreeVideoAdaptorRec(info->adaptor);
        info->adaptor = NULL;
     }
+#endif
 
     pScrn->vtSema = FALSE;

I don't remove this code, because I did not traced, what is the call sequence during card shutdown and whether this code is bad or only needs fix.

The crash is:

0: X(xf86SigHandler+0x81) [0x80cbf81]
1: [0xb7f0d420]
2: /usr/lib/xorg/modules/multimedia//theatre_drv.so [0xb7adaf69]
3: /usr/lib/xorg/modules/multimedia//theatre_drv.so [0xb7adb011]
4: /usr/lib/xorg/modules/multimedia//theatre_drv.so(ShutdownTheatre+0x1b) [0xb7adc13b]
5: /usr/lib/xorg/modules/drivers//r128_drv.so(R128ShutdownVideo+0x74) [0xb7b28d59]
6: /usr/lib/xorg/modules/drivers//r128_drv.so [0xb7b28237]
7: X [0x81549d7]
8: X [0x80c6bfc]
9: X [0x80c0e59]
10: X [0x812d698]
11: X [0x8151865]
12: X [0x80f0a25]
13: X(main+0x50b) [0x806e8cb]
14: /lib/libc.so.6(__libc_start_main+0xdc) [0xb7c8cf9c]
15: X(FontFileCompleteXLFD+0x1e1) [0x806db71]

Ok, so you disabled the code because it crashed on your hardware? Fair enough :)

I guess this is just the usual bitrot.

Both codes in last two patches were already disabled for years. Searching in GATOS and linuxvideo CVS:


For shutting down the card:

Adding R128ShutDownVideo:
revision 1.8
date: 2001-06-01 13:50:25 +0200;  author: volodya;  state: Exp;  lines: +1 -0;
Further down Marcs and Kevin suggestions list.

Commenting out:
revision 1.9
date: 2001-06-02 00:40:42 +0200;  author: volodya;  state: Exp;  lines: +10 -0;
Further cleanups. Radeon (verified) and R128(untested) behave ok during VT switches.

I only added comment, why it is disabled. I have not enough insight to guess, whether it is disabled because it is obsolete or broken.


For USE_FB:

As far as CVS history can show, CFB code was disabled (with comment "not until overlays and 24->32 code added", later only "not until overlays"). The code was completely broken again by upgrade to XFree-4.0.0.

I only made the cfb code to compile again (if it is enabled) and added comment, that it is broken.

Please don't bring back cfb.  Is there any reason you need it?

I don't bring it back. It is still disabled and broken. It was only a research why these parts of code are disabled and what happens if I try to enable disabled code.

After finishing the research I can say:

- The default setting coming from GATOS works

- The setting with XVideo disabled works

- Enabling other disabled parts of code may break the driver

Apart from the RandR issue I have no problems applying this patch any more. I just would like to hear from Michael first, whether he has additional issues.

It would be great, of course, if the RandR problem could be fixed as well...

Do you have a binary module? I'm afraid my x.org devel-tree is borged atm...
Else I must clean up this mess first...
I'm using ubuntu 7.04 with its xorg 7.2 now.

When you switch modes the overlay gets reset (see R128InitCommonRegisters() and R128RestoreCommonRegisters() in r128_driver.c).  If R128PutImage() doesn't restore enough overlay state, you'll need to restart the overlay to get the right values back in the overlay regs.  radeon may have the same issue.

Stanislav, any updates from your side?
Otherwise I would suggest merging after 7.3 is out. The remaining issues don't sound sever.

Please check in as is. It works and does most things one should expect and does not break anything.

If anybody can decide abot years-old bitrots in the code, I would be glad, but I am keeping all these commented-out parts of code there.

I plan to look at some remaining problems in future (but it's a low priority task). I could open separate bugs for them:

- Properly work after XRandR change.

- Properly support suspend/resume (not related to my patch).

- By one bug in brigthness settings. I can confirm it, but it has no practical impact. AFAIK Radeon driver has the same problem. Feel free to add a change proposed by you.

You wrote:

Your brightness + saturation controls are integer based, but don't have
rounding included. That may lead to strange effects, I suggest adding
half the denominator before doing the division - but of course you have
to check that no overflows can happen. BTW - brightness == 1000 will
already have the same representation (& 0x7f) as brightness == -1000...

Created attachment 14281 [details] [review]
xf86-video-ati-6.7.197-r128-xvideo.patch

Patch ported to the latest git snapshot (replaces all three patches above).

Ping. Is there any reason, why this patch was not yet accepted?

Comment #20 suggest to merge after 7.3. Minor fixes can come later.

The patch was already included in openSUSE 11.0.

BTW, the patch no longer applies due to the driver split (ati --> radeon/r128/mach64).

Updated patch from comment #22 applies on the current git://anongit.freedesktop.org/git/xorg/driver/xf86-video-r128.git without any reject.

Well, it applies, but it doesn't build ...

r128_video.c:25:25: error: generic_bus.h: No such file or directory
r128_video.c:26:25: error: theatre_reg.h: No such file or directory
r128_video.c:27:21: error: theatre.h: No such file or directory
r128_video.c:28:28: error: theatre_detect.h: No such file or directory

... since these files are in seperate radeon driver tree.

Created attachment 17108 [details] [review]
xf86-video-ati-theatre.patch

Additional patch to Comment #22. With this patch, the code compiles and works.

Before applying both these patches I did following:
git clone git://anongit.freedesktop.org/git/xorg/driver/xf86-video-r128.git
git clone git://anongit.freedesktop.org/git/xorg/driver/xf86-video-ati.git
cp -a xf86-video-ati/src/generic_bus.h xf86-video-r128/src/
cp -a xf86-video-ati/src/theatre_reg.h xf86-video-r128/src/
cp -a xf86-video-ati/src/theatre.h xf86-video-r128/src/
cp -a xf86-video-ati/src/theatre_detect.h xf86-video-r128/src/
cp -a xf86-video-ati/src/theatre_detect.c xf86-video-r128/src/
cp -a xf86-video-ati/src/theatre_detect_module.c xf86-video-r128/src/
cp -a xf86-video-ati/src/theatre.c xf86-video-r128/src/
cp -a xf86-video-ati/src/theatre_module.c xf86-video-r128/src/

Cloning sources seems to be sub-optimal (and causes file conflict), so I would suggest one of the following:

1) Keep the theatre code as is and move it to the directory with other multimedia chips in xserver. Edit radeon driver accordingly.

2) Clone files, rename radeon modules and its references in the r128 driver and remove support for ATI Rage Theatre 200 from them (AFAIK there was never manufactured any card combing ATI Rage 128 and ATI Rage Theatre 200).

What would you prefer?

I've just added both patches to openSUSE's r128 driver (and an additional one to fix the switch to libpciaccess) + the copy action. Obviously nobody else is interested into TV-in support for All in Wonder 128. Hence closing as WONTFIX.

Internal information for Stanislav:

 47898  State:new     By:sndirsch     When:2010-09-11T22:20:23
        submit:       X11:XOrg/xorg-x11-driver-video  ->  openSUSE:Factory
        Descr: - xf86-video-ati-6.7.197-r128-xvideo.patch/
                 xf86-video-ati-theatre.patch
                 * readded TV-in support for All in Wonder 128 (bfo #8811)
               - xf86-video-ati-theatre-pciaccess.patch
                 * fixes build with libpciaccess

Created attachment 38631 [details] [review]
xf86-video-ati-theatre-pciaccess.patch

Required since the switch to libpciaccess.