Bug 1889 - r128_dri: isn't patient enough waiting for idle (4.2->4.3 regression)
Summary: r128_dri: isn't patient enough waiting for idle (4.2->4.3 regression)
Status: RESOLVED FIXED
Alias: None
Product: xorg
Classification: Unclassified
Component: Driver/rage128 (show other bugs)
Version: git
Hardware: All All
: high major
Assignee: Xorg Project Team
QA Contact:
URL: http://bugs.debian.org/236187
Whiteboard:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2004-11-22 09:41 UTC by Daniel Stone
Modified: 2006-06-03 03:36 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
[FIXED_X11R68x] simple patch (1.29 KB, patch)
2004-11-22 09:42 UTC, Daniel Stone
roland.mainz: 6.8-branch+
Details | Splinter Review
New r128_accel.c idle timeout fix (1018 bytes, patch)
2006-05-05 21:33 UTC, Conn
no flags Details | Splinter Review
New r128_accel.c idle timeout fix (375 bytes, patch)
2006-05-05 21:38 UTC, Conn
no flags Details | Splinter Review

Description Daniel Stone 2004-11-22 09:41:00 UTC
In some cases, the r128 engine can take far longer than the suggested timeout to
return to idle, and the driver gets into a neat infinite loop of 'let me try to
wait for it to idle; ah, no, let me reset it; let me try to wait for it to idle;
ah, no, let me reset it ...'.  In 4.2.x, the default timeout was 10,000 *
2,000,000 * 32 waits; in 4.3.x, the timeout changed to 10,000 * 32 waits, and
this has broken things severely.

The attached patch changes the timeout to wait longer for the engine to return
to idle, and makes sure it gets cleanly reset also.

Patch by Daniel Jacobwitz for Debian, not licence-infected, blah, blah. 
Debian/Ubuntu #009, has been present for a very, very long time with no known
regressions and a very big solved problem.
Comment 1 Daniel Stone 2004-11-22 09:42:04 UTC
Created attachment 1330 [details] [review]
[FIXED_X11R68x] simple patch
Comment 2 Daniel Stone 2004-11-23 11:01:53 UTC
Comment on attachment 1330 [details] [review]
[FIXED_X11R68x] simple patch

make roland less angry
Comment 3 Daniel Stone 2004-12-04 14:28:04 UTC
Comment on attachment 1330 [details] [review]
[FIXED_X11R68x] simple patch

fixed in HEAD, please consider for 6.8.2
Comment 4 Roland Mainz 2004-12-11 15:27:39 UTC
Comment on attachment 1330 [details] [review]
[FIXED_X11R68x] simple patch

Approval for X11R6.8.x stable branch granted in the 2004-12-08
release-wranglers phone call.
Please don't commit, I'll do that myself...
Comment 5 Roland Mainz 2004-12-15 01:22:46 UTC
Comment on attachment 1330 [details] [review]
[FIXED_X11R68x] simple patch

Patch checked-in into X11R6.8.x stable branch:

/cvs/xorg/xc/ChangeLog,v  <--  ChangeLog
new revision: 1.365.2.89; previous revision: 1.365.2.88
cvs commit: Using deprecated info format strings.  Convert your scripts to use
the new argument format and remove '1's from your info file format strings.
/cvs/xorg/xc/programs/Xserver/hw/xfree86/drivers/ati/r128_accel.c,v  <-- 
r128_accel.c
new revision: 1.3.4.1; previous revision: 1.3
cvs commit: Using deprecated info format strings.  Convert your scripts to use
the new argument format and remove '1's from your info file format strings.
Mailing the commit message to xorg-commit@lists.freedesktop.org...
Comment 6 Roland Mainz 2004-12-15 01:23:41 UTC
Patch checked-in into X11R6.8.x stable branch and Xorg trunk...

... marking bug as FIXED.
Comment 7 Conn 2006-05-05 21:28:21 UTC
(In reply to comment #6)
> Patch checked-in into X11R6.8.x stable branch and Xorg trunk...
> 
> ... marking bug as FIXED.

I hope it's ok that I reopen this bug. I've been suffering with CCE idle
timeouts on my system (Dell Inspiron 8000, ATI Mobility M4 8mb) with the latest
Xorg 7.0. I looked through r128_accel.c and noticed that there are two instances
in which the engine gets reset on idle, but the previous patch only addressed
one part. I looked through XFree86's cvs and found this:
http://cvsweb.xfree86.org/cvsweb/xc/programs/Xserver/hw/xfree86/drivers/ati/r128_accel.c.diff?r1=1.19&r2=1.21

I will post a diff modified for Xorg, (but the credit goes to Michel Dänzer),
and on my system I can verify it has reduced the occurance of system lockups*
due to screensavers and specific opengl applications (gl-117 for example). I
previously had success with changing the CCEUsecTimeout to 20000 in my
xorg.conf, but this fix seems a better solution.

*Note that it crashes still occur, most notably with foobillard. However, I
think that's an issue of broken software fallbacks, so unrelated to this fix.
Comment 8 Conn 2006-05-05 21:33:52 UTC
Created attachment 5561 [details] [review]
New r128_accel.c idle timeout fix

I'm not sure if the ifdef lines are necessary, but I included them as they were
added to XFree86's cvs; my system was more stable when I added just the line
containing "R128CCE_STOP(pScrn, info);".
Comment 9 Conn 2006-05-05 21:38:53 UTC
Created attachment 5562 [details] [review]
New r128_accel.c idle timeout fix

Sorry for the spam, I submitted the wrong patch (that diff didn't apply
cleanly, this one does).
Comment 10 Conn 2006-05-05 21:42:02 UTC
Comment on attachment 5562 [details] [review]
New r128_accel.c idle timeout fix

--- r128_accel.c.orig	2005-12-14 19:41:28.000000000 +0000
+++ r128_accel.c	2006-05-05 12:19:44.000000000 +0100
@@ -219,6 +219,9 @@
		   INREG(R128_GUI_PROBE)));
	xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
		   "Idle timed out, resetting engine...\n");
+#ifdef XF86DRI
+	R128CCE_STOP(pScrn, info);
+#endif
	R128EngineReset(pScrn);
 #ifdef XF86DRI
	R128CCE_RESET(pScrn, info);
Comment 11 Daniel Stone 2006-06-03 02:28:14 UTC
michel, okay if I commit this one?
Comment 12 Michel Dänzer 2006-06-03 02:33:12 UTC
(In reply to comment #11)
> michel, okay if I commit this one?

Sure. We're currently discussing on the dri-devel list how to handle this even
better in the radeon driver, but if anything comes out of that, it can always be
backported to r128.
Comment 13 Daniel Stone 2006-06-03 03:36:24 UTC
committed to HEAD


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.