Re: Wrong colors on ARAnyM

[Michael Schmitz <schmitzmic@gmail.com>]

Hi Geert,

On 14/02/22 20:41, Geert Uytterhoeven wrote:
> Hi Michael,
>
> (replying in plain text mode ;-)
Sorry about that - hopefully fixed now.
>
> On Sun, Feb 13, 2022 at 9:29 PM Michael Schmitz <schmitzmic@gmail.com> wrote:
>>
>>   2. Change the color depth a few times:
>>
>>      $ fbset -depth 1
>>      $ fbset -depth 2
>>
>> Depth 2 forces STe palette mode (in decode_var; activated in the Videl in the vbl switcher).
>>
>> [...]
>> It _looks_ as though changes to f_shift and st_shift are not pushed to hardware in the VBL interrupt. Does ARAnyM run the VBL interrupt regularly?
> Yes it does.
>
>> But maybe the kernel and ARAnyM disagree because useStPalette() has this:
>>
>> int hreg = handleReadW(HW + 0x82); // Too lame!
>> // Better way how to make out ST LOW mode wanted
>> if (hreg == 0x10 || hreg == 0x17 || hreg == 0x3e) {
>> useStPal = true;
>>
>> }
>>
>> for st_shift == 0
> Linux' falcon_setcolreg() writes to both the Falcon palette registers
> and shifter_tt.color_reg[], so shouldn't the STE color palette contain
> the correct colors, too?


I don't think it will:

The STe palette is set like this:

                 shifter_tt.color_reg[regno] =
                         (((red & 0xe) >> 1) | ((red & 1) << 3) << 8) |
                         (((green & 0xe) >> 1) | ((green & 1) << 3) << 4) |
                         ((blue & 0xe) >> 1) | ((blue & 1) << 3);

with all colours downshifted by 12 bits already. Rewriting that in the 
same form as is used in falcon_setcolreg(), i.e. without the downshift:

                 shifter_tt.color_reg[regno] =
                         (((red & 0xe000) >> 13) | ((red & 1000) >> 9) 
<< 8) |
                         (((green & 0xe000) >> 13) | ((green & 1000) >> 
9) << 4) |
                         ((blue & 0xe000) >> 13) | ((blue & 1000) >> 9);


Now look at falcon_setcolreg:

                 shifter_tt.color_reg[regno] =
                         (((red & 0xe000) >> 13) | ((red & 0x1000) >> 
12) << 8) |
                         (((green & 0xe000) >> 13) | ((green & 0x1000) 
 >> 12) << 4) |
                         ((blue & 0xe000) >> 13) | ((blue & 0x1000) >> 12);


My guess would be the two ought to be identical, assuming the STe 
palette registers provide backwards hardware compatibility.

Either way, I think we've got operator precedence wrong here. Shift 
takes precedence over bitwise and / or? With that precedence, what we 
have in summary is

                 shifter_tt.color_reg[regno] =
                         (((red & 0xe000) >> 13) | ((red & 0x1000) >> 4)) |
                         (((green & 0xe000) >> 13) | ((green & 0x1000) 
 >> 8)) |
                         ((blue & 0xe000) >> 13) | ((blue & 0x1000) >> 12);

Only the blue bits ever entirely seem to end up where they ought to. 
Testing this hypothesis on ARAnyM (without getting it to boot all the 
way, for some odd reason likely to do with my current .config) shows 
that the current code results in the shifter_tt.color_reg entries either 
5 or 0x17, 0x113 or 0x117. Adding the missing parentheses results in 
what I'd expect to see.

This bug has been present since at least 2.4.30. I do recall seeing this 
odd blue console before on the Falcon, so it's been a real bug for ages. 
Did the operator precedence change sometime since?

>
>> As the problem is not 100% reproducible, it looks like a race condition
>> in ARAnyM, or a wrong initialization order in atafb.
>>
>> I'd have to test this on hardware but haven't got fbset on the Falcon. I'll try to locate a binary.
> Sorry, you do not need fbset to reproduce this:
>
>      echo N > /sys/class/graphics/fb0/bits_per_pixel

Thanks, that's easy enough to try.

Playing around with that command a bit, it does appear that the second 
switch after selecting depth 2 turns the console blue (regardless of 
what depth is set in that step). And the second switch after that (even 
if setting depth 2) restores normal colours. No race that I can see 
there, it's quite reproducible.

Now what would cause us to miss every other palette update or depth change?

I'll verify that the bug is gone once I've rebuilt with a sane .config. 
But I think we've got enough to fix at least the annoying STe palette 
bug. Unless you want to keep that alive until we've found out what 
causes us to miss odd updates...

Cheers,

     Michael


>
> Gr{oetje,eeting}s,
>
>                          Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                  -- Linus Torvalds