Re: Wrong colors on ARAnyM

[Michael Schmitz <schmitzmic@gmail.com>]

Hi Geert,

On 15/02/22 21:11, Geert Uytterhoeven wrote:
>>> 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.

The atafb driver references a 'linux/tools/hardware.txt' file for a 
description of the Videl registers. Does anyone still have a copy of 
that file?

Failing that - how would I go about setting specific text colours for 
the console (or draw a test pattern showing the result of the four 
palette entries)?

>>
>> 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.
> Oops.
>
>> 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.
> I recall seeing an odd blue console before, too.
> I always dismissed it as some artefact of redrawing the screen in
> a different color mode, like we used to have when switching from a
> 16-color to a monochrome text console (remember the underlined text?).
Can't say I do ... but I don't use the console much at all (keyboard 
beginning to act up).
> Did/do you see it on the real Falcon, too?
Didn't try yet, sorry.
> Initially, I suspected the real Falcon always using the Falcon palette,
> so the bug was never seen on real hardware. But the following comment
> makes me think the colors have always been wrong if bpp == 2:
>
>          /* 2 planes must use STE compatibility mode */
>          par->hw.falcon.ste_mode = bpp == 2;
>
>> 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.
> Yes, it is quite reproducible, but it doesn't always happen every
> second switch. As my Atari DRM driver cannot switch mode on the fly,
> I have to initialize the driver with the mode I want to use. Booting
> with a mode with 2 bitplanes gives me the wrong colors in ca. 20%
> of the cases. So it is random.
Yes, the 'every second switch' does appear to be a little random, too. 
One boot I get that behavior, the next it's behaving as it should.
>> Now what would cause us to miss every other palette update or depth change?
> That's a good question! Why does ARAnyM sometimes decide to use the
> Falcon palette, and sometimes the STe palette, and not only with
> bpp == 2, but also with other color depths?

I guess we'll have to wait and see whether I see that random behaviour 
on the actual hardware.

Looking at getBpp() in the ARAnyM source, I see that ST shifter modes 
are used if bits 10, 8 and 4 in f_shift are clear, but the selection of 
mono vs. STe 2 bpp modes is different from what's used in the kernel driver:

The kernel uses videl.st_shift & 0x300 == 0x100 for STe mode, and == 
0x200 for mono. ARAnyM uses == 0x01 for STe mode in getBpp(). In 
useStPalette(), == 0x100 is used. Both read from the same register 
offset. The kernel never sets 0x01 for st_shift, so the code in getBpp() 
can't work as far as I can see.

>
>> I can confirm that the bug is gone with additional parentheses to apply
>> the shifts (<<8 for red, <<4 for green) to the result of the bit-wise
>> or, not just the right-hand portion of it.
>>
>> Do you want me to prepare a patch, or would you prefer to do it yourself?
> Feel free to send a patch.
> If you don't get to it, I'll make one, eventually.
> Thanks!

My pleasure - checkpatch may grumble a litte over the additonal line 
length but I think we can ignore that!

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