looks like theres a typo in the second last section: if (IS_R300_VARIANT) shouldn't it look like the last section? if (!IS_R300_VARIANT) Index: xc/programs/Xserver/hw/xfree86/drivers/ati/radeon_accel.c =================================================================== RCS file: /cvs/xorg/xc/programs/Xserver/hw/xfree86/drivers/ati/radeon_accel.c,v retrieving revision 1.7 retrieving revision 1.8 diff -u -r1.7 -r1.8 --- xc/programs/Xserver/hw/xfree86/drivers/ati/radeon_accel.c 30 Jul 2004 20:30:51 -0000 1.7 +++ xc/programs/Xserver/hw/xfree86/drivers/ati/radeon_accel.c 30 Jul 2004 22:20:21 -0000 1.8 @@ -211,9 +211,7 @@ host_path_cntl = INREG(RADEON_HOST_PATH_CNTL); rbbm_soft_reset = INREG(RADEON_RBBM_SOFT_RESET); - if ((info->ChipFamily == CHIP_FAMILY_R300) || - (info->ChipFamily == CHIP_FAMILY_R350) || - (info->ChipFamily == CHIP_FAMILY_RV350)) { + if (IS_R300_VARIANT) { CARD32 tmp; OUTREG(RADEON_RBBM_SOFT_RESET, (rbbm_soft_reset | @@ -227,7 +225,6 @@ } else { OUTREG(RADEON_RBBM_SOFT_RESET, (rbbm_soft_reset | RADEON_SOFT_RESET_CP | - RADEON_SOFT_RESET_HI | RADEON_SOFT_RESET_SE | RADEON_SOFT_RESET_RE | RADEON_SOFT_RESET_PP | @@ -236,7 +233,6 @@ INREG(RADEON_RBBM_SOFT_RESET); OUTREG(RADEON_RBBM_SOFT_RESET, (rbbm_soft_reset & (CARD32) ~(RADEON_SOFT_RESET_CP | - RADEON_SOFT_RESET_HI | RADEON_SOFT_RESET_SE | RADEON_SOFT_RESET_RE | RADEON_SOFT_RESET_PP | @@ -249,9 +245,7 @@ INREG(RADEON_HOST_PATH_CNTL); OUTREG(RADEON_HOST_PATH_CNTL, host_path_cntl); - if ((info->ChipFamily != CHIP_FAMILY_R300) && - (info->ChipFamily != CHIP_FAMILY_R350) && - (info->ChipFamily != CHIP_FAMILY_RV350)) + if (IS_R300_VARIANT) OUTREG(RADEON_RBBM_SOFT_RESET, rbbm_soft_reset); OUTREG(RADEON_CLOCK_CNTL_INDEX, clock_cntl_index); @@ -279,9 +273,7 @@ */ /* Turn of all automatic flushing - we'll do it all */ - if ((info->ChipFamily != CHIP_FAMILY_R300) && - (info->ChipFamily != CHIP_FAMILY_R350) && - (info->ChipFamily != CHIP_FAMILY_RV350)) + if (!IS_R300_VARIANT) OUTREG(RADEON_RB2D_DSTCACHE_MODE, 0); pitch64 = ((pScrn->displayWidth * (pScrn->bitsPerPixel / 8) + 0x3f)) >> 6;
cc'ing hui yu, this was his patch.
possible patch (if the change of the logic wasn't intented) Index: xc/programs/Xserver/hw/xfree86/drivers/ati/radeon_accel.c =================================================================== RCS file: /cvs/xorg/xc/programs/Xserver/hw/xfree86/drivers/ati/radeon_accel.c,v retrieving revision 1.10 diff -u -r1.10 radeon_accel.c --- xc/programs/Xserver/hw/xfree86/drivers/ati/radeon_accel.c 12 Aug 2004 05:00:22 -0000 1.10 +++ xc/programs/Xserver/hw/xfree86/drivers/ati/radeon_accel.c 9 Oct 2004 21:36:05 -0000 @@ -245,7 +245,7 @@ INREG(RADEON_HOST_PATH_CNTL); OUTREG(RADEON_HOST_PATH_CNTL, host_path_cntl); - if (IS_R300_VARIANT) + if (!IS_R300_VARIANT) OUTREG(RADEON_RBBM_SOFT_RESET, rbbm_soft_reset); OUTREG(RADEON_CLOCK_CNTL_INDEX, clock_cntl_index);
Any updates on this? The latest version of radeon_accel.c still seems to contain this typo, and if we're doing the reset improperly on R300 cards, it may be the cause of bugs like #3510.
I'd say check the fix in, as it looks like an obvious cut and paste error. Or at least try adding the patch, building it and testing against the bug #3510 referenced above.
> looks like theres a typo > cc'ing hui yu, this was his patch. hyu@ati.com: So do you think that there is an error here?
Yes, it's my fault. Thanks for catching this.
Created attachment 3601 [details] [review] radeon_typo_r300_patch.txt
CVSROOT: /cvs/xorg Module name: xc Changes by: daenzer@gabe.freedesktop.org 05/11/08 08:30:48 Log message: 2005-11-08 Michel Daenzer <michel@daenzer.net> * programs/Xserver/hw/xfree86/drivers/ati/radeon_accel.c: (RADEONEngineReset): bugzilla #988 (https://bugs.freedesktop.org/show_bug.cgi?id=988) Fix typo which may or may not have had a negative impact on stability with R300 class cards. Modified files: ./: ChangeLog xc/programs/Xserver/hw/xfree86/drivers/ati/: radeon_accel.c Revision Changes Path 1.1502 +8 -0 xc/ChangeLog http://cvs.freedesktop.org/xorg/xc/ChangeLog 1.22 +1 -1 xc/programs/Xserver/hw/xfree86/drivers/ati/radeon_accel.c http://cvs.freedesktop.org/xorg/xc/programs/Xserver/hw/xfree86/drivers/ati/radeon_accel.c
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.