Bug 113810

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 ?!?