Cleanup debug

[JayFoxRox]

Continuation after accidentally closing #77 (and being unable to restore or re-open that PR)

Apparently I was still staging this when I was leaving XboxDev, so this might still be necessary.
Pinging worker-bees @Teufelchen1 and friends to consider looking into it.

TODO:

• Review if this is still a good idea
• What was said in Cleanup debug #77

[Teufelchen1]

Why did you move the color defines into the c file? I would assume they were better suited in the header. Reason for that thought is, that one might include the header and switch the color of the text. A contradiction to that is that color switching is not supported by the debugPrint interface. This might be the reason for the change?

[JayFoxRox]

• Because they often conflict with other constants in other projects (people also called their macros for Xbox buttons WHITE and BLACK or red-black trees used it).
  • The new names make this less critical, but still.. why expose weird unprefixed names?
• There's no use for them in the header, as there's no API to use them with.
• It's not very beginner friendly to manually select between 32BPP / 16BPP / 15BPP variants.

= Generic macro names are not a good API, so it should not be exposed.

Adding color support to debugPrint should be avoided; consider the API soft-deprecated.

[Teufelchen1]

Did you think about adding a if (SCREEN_FB == NULL) { panic(); }? Its not necessary the responsibility to check for it, as it can't fail once the user successfully run XVideoSetMode();(as they should). But it wouldn't hurt either. 🤔

[JayFoxRox]

Why would SCREEN_FB at NULL be an issue? NULL is just memory address 0, which is a perfectly valid address in physical memory.

[Teufelchen1]

oh oh. Yes you are right. I completely forgot that. Sry!

[Teufelchen1]

This is already upstream as

static void synchronizeFramebuffer(void)
{
	VIDEO_MODE vm = XVideoGetMode();
	SCREEN_WIDTH = vm.width;
	SCREEN_HEIGHT = vm.height;
	SCREEN_BPP = vm.bpp;
	SCREEN_FB = XVideoGetFB();
}

[Teufelchen1]

This is missing upstream.
I have a hard time understanding what it does. I read up this felixcloutier.com/x86/sfence. It helped a bit but some of braincells are still resisting the enlightenment.

Let me try to explain it:

So, because the CPU & compiler are "smart", they try to improve the performance of the given execution by doing out-of-order execution(I'm familiar with this) and also write-combining(also familiar for me).
This is all great and stuff, but sometimes you really want changes(i.e. a write to memory) to be completely done, before the next operations are executed.
This is was sfence ensures. Before the cpu continues execution(or more precise: access from the last touched memory addresses), it waits and ensures the last write-combines and out-of-order executions are done.
I could construct a theoretical case, where an executable writes a lot via debugPrint and then crashes while halting/stopping the cpu simultaneously. sfence could ensure in such a scenario, that everything is written(and therefore an error message readable), prior halting the system?

[JayFoxRox]

I don't fully remember when it was a problem, but you can actually manage to break the displayed image if you don't do this.

Basically if you get unlucky, your pixels will not be flushed to memory and instead they remain in the CPU write-combine-buffer, where the GPU won't see them. I believe flushing them to CPU cache was enough so the GPU can see that it needs to update?

So this might be incorrect or I might have gotten lucky with sfence (it might have to do some other instruction). But I'm convinced that something like this is necessary, depending on the cache mode of the framebuffer.

[Teufelchen1]

Has already been fixed in upstream in this commit.

(valid for all three samples)

[Teufelchen1]

all changes in this file are already upstream.