Bugzilla – Bug 113810
S3 Savage notebook LCD very dark
Last modified: 2005-09-07 17:43:45 UTC
updating 9.3 to 10.0 beta-3 on a Toshiba Portege 3480: the LCD display is very dark (almost unusable) in X11, not on text mode. changing gamma doesn't help, it looks like the display illumination got dimmed. after replacing only savage_drv.o with the driver from 9.3, X11 display brightness is ok again. shall I test savage_drv.o from beta-1/2 too ? if you have a new savage driver for testing before beta-4, pls let me know!
Please attatch /var/log/Xorg.*.log.
Created attachment 48147 [details] X server log with 10.0 beta-3 savage driver I've created two server logs, on with the old working 9.3 driver on display :93 and one with the new dimm display on :103 (that last one is attached). as you can see in the output of diff, there is no significant difference :-( diff Xorg.93.log Xorg.103.log 14c14 < (==) Log file: "/var/log/Xorg.93.log", Time: Tue Aug 30 12:01:04 2005 --- > (==) Log file: "/var/log/Xorg.103.log", Time: Tue Aug 30 12:01:32 2005
Hmm. We added a patch since 9.3. That's the only difference I can see. I'll attach the patch. ------------------------------------------------------------------- Tue May 17 18:34:13 CEST 2005 - sndirsch@suse.de - adjusted pc_savage.diff; enabled it ------------------------------------------------------------------ Tue May 17 14:30:10 CEST 2005 - sndirsch@suse.de [...] - pc_savage.diff (X.Org Bugzilla #3309); disabled for now! Set the hardware to 8bit CLUT mode in 8 bpp. We used to flag an 8 bit DAC to DDX but did not make sure it actually was running in 8 bit mode. On Savage4 the HW cursor doesn't seem to be truecolor in 8bpp - not even with the streams engine running. Fix SavageProbeDDC() to load vbe module and initialize it before using it. [...]
Created attachment 48237 [details] savage patch new in 10.0
Harald, nice to hear from you again! This is something for you to debug. I only have Savage4 and Savage4 docu for this so I cannot do this myself - and Felix Kuehling - our Savage guru - went to ATi. Here is the deal: I fixed the code so it sets SR 0x1b bit 4 to set the DAC depth to 8 bit. Doing this I removed the mask for bit 5 which is reserved according to my docs. Maybe you can investigate if this is the problem.
I'll provide a savage driver without the mentioned patch applied to make sure that it is really related to this patch.
Stefan, please try this patch on top of the other. diff -u -r1.38 savage_driver.c --- savage_driver.c 5 Aug 2005 23:04:33 -0000 1.38 +++ savage_driver.c 31 Aug 2005 17:36:13 -0000 @@ -2372,7 +2372,7 @@ VGAOUT8(0x3c4, 0x1b); if( (pScrn->bitsPerPixel == 32) && !psav->DGAactive && ! psav->FBStart2nd ) - VGAOUT8(0x3c5, 0x18 ); + VGAOUT8(0x3c5, 0x38 ); else VGAOUT8(0x3c5, 0x10 ); @@ -2673,7 +2673,7 @@ VGAOUT8(0x3c5, restore->SR18); VGAOUT8(0x3c4, 0x1b); if( psav->DGAactive ) - VGAOUT8(0x3c5, restore->SR1B & ~0x08 ); + VGAOUT8(0x3c5, restore->SR1B & ~0x28 ); else VGAOUT8(0x3c5, restore->SR1B);
*** Bug 113706 has been marked as a duplicate of this bug. ***
Created attachment 48403 [details] New driver Please test and give me feedback.
Stefan, thanks for making the new driver. I only have a Savage4 so I cannot test this. I'm not sure if our QA still tests systems with Savage chipsets so Harald et al. please test!
Hi Stefan! unfortuneately I tried to reply via email and didn't realize that replying to "bugzilla_noreply@novell.com" is just wrting to a black hole... only phone call with Egbert showed, that you didn't get the new facts from my tests of last night (see reply to #7) thanks for the new driver for testing! unfortuneately I didn't check bugzilla directly but only checked my mails and your mail got delivered on 03:55:10 :-(( > Please test and give me feedback. as expected from my tests last night and now confirmed by a real test: with that new driver, the display is still dark (SR1B is now 0x38) and there are still those stripes in the blue gradient of xfce4. thinking a but more about the issue of LUT resolution reminds me that a few times in the past I wondered why some image/movie/... doesn't seem to display with full 16M colors -- the LUT setup now explains what was wrong here too...
OK, I've looked at it again. Harald, could you please try to set UseBIOS to NO and see if it makes a difference? I've noticed that the SR1B register only gets written when the BIOS isn't used. I may have to tweak the CLUT depth bit in the case where it is used, too.
(In reply to comment #7) > Stefan, please try this patch on top of the other On Aug 31, bugzilla_noreply@novell.com wrote: > ------- Additional Comments From eich@suse.de 2005-08-31 11:38 MST ------- > Stefan, please try this patch on top of the other. > > - VGAOUT8(0x3c5, 0x18 ); > + VGAOUT8(0x3c5, 0x38 ); update & more data -- I was playing a bit more with SR1B and found the following behaviour running xfce4 as window manager I get a blue background gradient going from left to right. with depth 24 this gradient shows significant steps with SR1B bit 3 (0x08) set! only with SR1B bit 3 cleared, I really get full 24bpp on the LCD... that's with 9.3 driver. now I started the 10.0 driver again, and guess what: with SR1B_3 set, I get a dimm screen which shows the same bad gradient resolution! with SR1B_0 cleared, the gradient is ok AND -- the LCD is bright again! so it has to do with the settings of the DAC! I used a small old tool do read out the full LUT -- and realized/remembered that LUT readout depends on SR1B_4 setting (6/8 bit LUT). all dumps (driver 93 and 10 and SR1B settings 0x08 and 0x18) are attached. as you can see, the LUT is initialized differently in 10.0 driver! 9.3 sets up a 6bit LUT while 10.0 only sets a 4 bit LUT gradient. that's why the display is so dark... browsing pc_savage.diff again, I don't see what might cause this different initialisation, might be a global change in Xserver ? one reason might be a sequence change during initialisation ?! the correct sequeze has to be: - first set SR1B to 0x18 - only then load LUT with full 8 bit gradient from 0x00 to 0xff if SR1B_4 is cleared when LUT gets loaded, the values are shifted 2 bit left. what's the reason that the LUT is loaded with a 6 bit gradient even in the 9.3 driver ? hope this helps to fix the savage driver ?
please get the LUT dump attachments from bug id #113812. I have no idea how I managed to switch over to another id between replying and sending those four attachments ?!?! I hate bugzilla!!! (and bugzilla seems to hate me, too :-(
(In reply to comment #12) > OK, I've looked at it again. Harald, could you please try to set UseBIOS to NO > and see if it makes a difference? I've noticed that the SR1B register only gets > written when the BIOS isn't used. I may have to tweak the CLUT depth bit in the > case where it is used, too. sorry, no change: 9.3 is still bright and 10.0 and test driver give dark display. I can't comment on the steps in the gradient right now because it's way to bright in my room -- damn sun shine in summer time ;-)
Phil wrote in the duplicate bugreport that this problem only occurs with 24bit and not with 16bit. Can you confirm this? Currently we use 16bit as default for the savage driver.
(In reply to comment #16) > Phil wrote in the duplicate bugreport that this problem only occurs with 24bit > and not with 16bit. Can you confirm this? yes and no:) depth 8/15/16 all set SR1B to 0x10, so the gamma LUT is disabled. so brightness is ok for 15/16bpp, but gamma correction via LUT doesn't work anymore. for 8bpp, again the LCD is very dark... the bahaviour for depth 15/16 (ok,no gamma) is the same for 9.3 driver, but the 9.3 shows 8bpp with correct itensity too. I think this is another pointer that filling the LUT seems to be at least part of the problem... > Currently we use 16bit as default for > the savage driver. hmm, using 16bpp as default really hides the problem and isn't really what you should offer L.User as "good" default for this great chip;)
Created attachment 48901 [details] Test patch for Harald The attached patch needs to be applied after the patch in attachment #48237 [details] has been backed out. I've done some reading in the savage specs that Harald had sent me and was able to locate some more bits that may make a difference. Harald, could you please test this patch? It also adds debugging stuff to the palette loading code which dumps the state of the SR1B register at palette loading time. If this patch doesn't help I would like to see the output. This debugging stuff needs to be removed before this patch goes into production.
Egbert, is this patch against current X.Org CVS? I backed out the patch in attachment #48237 [details] but still got 3 hunks failed.
I had to edit the patch by hand. You may have to apply the missing hunks by hand. The patch is rather small.
(In reply to comment #18) > Created an attachment (id=48901) [edit] > Test patch for Harald > > The attached patch needs to be applied after the patch in attachment #48237 [details] [edit] has > been backed out. I've done some reading in the savage specs that Harald had > sent me and was able to locate some more bits that may make a difference. > Harald, could you please test this patch? > It also adds debugging stuff to the palette loading code which dumps the state > of the SR1B register at palette loading time. If this patch doesn't help I > would like to see the output. > This debugging stuff needs to be removed before this patch goes into > production. don't bother about debugging code, this patch won't get into production ;-) the result of my test is: nothing has changed :-( 24bpp and 8bpp are still dark and xgamma works, 15/16bpp are bright but don't support xgamma. I've written the server output for all depths to xout.$depth and grepped for your debug output. as you can see, SR1B always has the same value per depth: harald:drivers/savage # grep SavageLoadPalette xout.* | uniq -c 7 xout.15:SavageLoadPalette - SR1B: 0x57 9 xout.16:SavageLoadPalette - SR1B: 0x57 7 xout.24:SavageLoadPalette - SR1B: 0x7f 771 xout.8:SavageLoadPalette - SR1B: 0x57 please note: I've run "xgamma" in each depth and got another "SavageLoadPalette ..." output, which shows that SR1B seems to be constant. even with "reloaded" gamma curve, I still only can read out LUT 6 bit values from 0x00 to 0x3f. next step: using 24bpp I poked SR1B from 0x7f to 0x6f (set LUT 1 to 18 bit) while X11 is running and run "xgamma -gamma 1" again and surprise -- your debug output shows 0x6f (as poked) and the display is bright! the LUT/gamma code seems to be the problem! next step: I stepped back to original 10.0 savage driver code and did some more code reading and I came up with the following beautiful patch: --- savage_driver.c-0 2005-09-05 10:16:42.941936000 +0200 +++ savage_driver.c 2005-09-06 19:39:30.641365750 +0200 @@ -2498,7 +2498,7 @@ NULL, colormapFlags )) return FALSE; } else { - if (!xf86HandleColormaps(pScreen, 256, 6, SavageLoadPalette, NULL, + if (!xf86HandleColormaps(pScreen, 256, 8, SavageLoadPalette, NULL, colormapFlags )) return FALSE; } with this small and obvious change, 24 and 8 bpp are bright and gamma works!!! now only 15/16 bpp gamma support is missing. sone preliminary test results here: savage/ix needs a full 3*8 bit LUT for 15/16bpp gamma correction. common/xf86cmap.c CMapRefreshColors() seems to need a new flag which shows that the LUT is full 8 bit instead of 5/6 bits for depth 15/16. maybe I'll have a look into that tomorrow, I have to leave now and I'm very happy with that small patch above. Egbert: are you really sure that Savage4 has only 18 bit LUT ? if not, the other call to xf86HandleColormaps() for savage4 just above my patch should be changed too... CU!
So what's the conclusion of this now? I guess we still get a savage patch in for RC2. But for this I need the final patch I need to apply ASAP. Please comment.
Harald, thanks for the investigation! You're right. This happens when you need to set the LUT depth in two different places. I assume my previous fix is OK - apart from this one missing change. I'm going to add a patch that should be applied on top of the existing savage patch (and maybe combined into one). The reason why it always worked for me on SAV4 was that I was using CVS head which already had the change to HandleColorMap().
Created attachment 49018 [details] Fix
So this should be applied on top of attachment #48237 [details], right?
At least the patch applies on top of it.
New package with this new patch submitted. Will be fixed for RC2.
(In reply to comment #24) > Created an attachment (id=49018) [edit] > Fix I've rebuild the driver from clean 10.0 sources with only this patch applied on top, applied cleanly. LCD brightness is OK for all depths, gamma correction works for depth 8/24, no gamma for depth 15/16 yet (it's disabled in the LUT and the LUT contents is wrong). seems to be OK for me for 10.0, and for 10.1 we're looking into gamma correction for depth 15/16... it's already too late for this patch into RC1 ?!?