Skip to content

Cleanup debug #90

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Cleanup debug #90

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
331013b
video: Replace SetDisplayStart with SetFB
JayFoxRox May 28, 2020
aed5f68
video: Allocate the framebuffer
JayFoxRox May 28, 2020
ea5c293
video: Fill current mode with zeros until it is set
JayFoxRox May 28, 2020
4fd6a55
debug: Synchronize current mode with XVideo
JayFoxRox May 28, 2020
661af3a
debug: Ensure that draw operations don't get stuck in buffers
JayFoxRox May 28, 2020
ad7554a
video: Make variables static
JayFoxRox May 28, 2020
cf459ee
samples: Don't use hidden debug.h variables
JayFoxRox May 28, 2020
842617a
debug: Make variables static
JayFoxRox May 28, 2020
afd47d6
debug: Remove colors from header
JayFoxRox May 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 44 additions & 14 deletions lib/hal/debug.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,19 @@
#define MARGIN 25
#define MARGINS 50 // MARGIN*2

int SCREEN_WIDTH = 640;
int SCREEN_HEIGHT = 480;
int SCREEN_BPP = 32;
#define WHITE_32BPP 0x00FFFFFF
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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.

#define BLACK_32BPP 0x00000000

#define WHITE_16BPP 0xFFFF
#define BLACK_16BPP 0x0000

#define WHITE_15BPP 0x7FFF
#define BLACK_15BPP 0x0000

static unsigned char *SCREEN_FB = NULL;
static int SCREEN_WIDTH = 0;
static int SCREEN_HEIGHT = 0;
static int SCREEN_BPP = 0;

int nextRow = MARGIN;
int nextCol = MARGIN;
Expand All @@ -27,9 +37,23 @@ static const unsigned char systemFont[] =
#include "font_unscii_16.h"
};

static void synchronizeVideoMode()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
}

{
VIDEO_MODE vm = XVideoGetMode();
SCREEN_WIDTH = vm.width;
SCREEN_HEIGHT = vm.height;
SCREEN_BPP = vm.bpp;
SCREEN_FB = XVideoGetFB();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🤔

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

static void writebackBuffers()
{
asm __volatile__("sfence");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

static void drawChar(unsigned char c, int x, int y, int fgColour, int bgColour)
{
unsigned char *videoBuffer = XVideoGetFB();
unsigned char *videoBuffer = SCREEN_FB;
videoBuffer += (y * SCREEN_WIDTH + x) * ((SCREEN_BPP+7)/8);

unsigned char mask;
Expand Down Expand Up @@ -146,17 +170,15 @@ void debugPrint(const char *format, ...)
va_start(argList, format);
vsprintf(buffer, format, argList);
va_end(argList);

VIDEO_MODE vm = XVideoGetMode();
SCREEN_WIDTH = vm.width;
SCREEN_HEIGHT = vm.height;
SCREEN_BPP = vm.bpp;

synchronizeVideoMode();

int fgColour;
int bgColour;
switch (SCREEN_BPP) {
case 32:
fgColour = WHITE;
bgColour = BLACK;
fgColour = WHITE_32BPP;
bgColour = BLACK_32BPP;
break;
case 16:
fgColour = WHITE_16BPP;
Expand Down Expand Up @@ -194,30 +216,38 @@ void debugPrint(const char *format, ...)

s++;
}

writebackBuffers();
}

void advanceScreen( void )
{
synchronizeVideoMode();

int pixelSize = (SCREEN_BPP+7)/8;
int screenSize = SCREEN_WIDTH * (SCREEN_HEIGHT - MARGINS) * pixelSize;
int lineSize = SCREEN_WIDTH * (FONT_HEIGHT + 1) * pixelSize;

unsigned char* thisScreen = XVideoGetFB() + (SCREEN_WIDTH * MARGIN) * pixelSize;
unsigned char* thisScreen = SCREEN_FB + (SCREEN_WIDTH * MARGIN) * pixelSize;
unsigned char* prevScreen = thisScreen+lineSize;

memmove(thisScreen, prevScreen, screenSize);

nextRow -= (FONT_HEIGHT+1);
nextCol = MARGIN;

writebackBuffers();
}

void debugClearScreen( void )
{
unsigned char* videoBuffer = XVideoGetFB();
synchronizeVideoMode();

memset( videoBuffer, 0, ((SCREEN_BPP+7)/8) * (SCREEN_WIDTH * SCREEN_HEIGHT) );
memset( SCREEN_FB, 0, ((SCREEN_BPP+7)/8) * (SCREEN_WIDTH * SCREEN_HEIGHT) );
nextRow = MARGIN;
nextCol = MARGIN;

writebackBuffers();
}

void debugPrintHex(const char *buffer, int length)
Expand Down
12 changes: 0 additions & 12 deletions lib/hal/debug.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,6 @@ extern "C"
{
#endif

#define WHITE 0x00FFFFFF
#define BLACK 0x00000000
#define RED 0x00FF0000
#define GREEN 0x0000FF00
#define BLUE 0x000000FF

#define WHITE_16BPP 0xFFFF
#define BLACK_16BPP 0x0000

#define WHITE_15BPP 0x7FFF
#define BLACK_15BPP 0x0000

/**
* Prints a message to whatever debug facilities might
* be available.
Expand Down
55 changes: 35 additions & 20 deletions lib/hal/video.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,15 @@
#define VIDEO_R5G6B5 0x00000011
#define VIDEO_A8R8G8B8 0x00000012


unsigned char* _fb;
DWORD dwEncoderSettings = 0;
VIDEO_MODE vmCurrent;
int flickerLevel = 5;
BOOL flickerSet = FALSE;
BOOL softenFilter = TRUE;
BOOL softenSet = FALSE;
GAMMA_RAMP_ENTRY gammaRampEntries[256];
static unsigned char* framebufferMemory = NULL;
static unsigned char* _fb = NULL;
static DWORD dwEncoderSettings = 0;
static VIDEO_MODE vmCurrent = { 0, 0, 0, 0 };
static int flickerLevel = 5;
static BOOL flickerSet = FALSE;
static BOOL softenFilter = TRUE;
static BOOL softenSet = FALSE;
static GAMMA_RAMP_ENTRY gammaRampEntries[256];

static KINTERRUPT InterruptObject;
static KDPC DPCObject;
Expand All @@ -62,7 +62,7 @@ typedef struct _VIDEO_MODE_SETTING
DWORD dwFlags;
} VIDEO_MODE_SETTING;

VIDEO_MODE_SETTING vidModes[] =
static VIDEO_MODE_SETTING vidModes[] =
{
{0x44030307,640,480,50,VIDEO_REGION_PAL,AV_PACK_STANDARD}, //640x480 PAL 50Hz
{0x44040408,720,480,50,VIDEO_REGION_PAL,AV_PACK_STANDARD}, //720x480 PAL 50Hz
Expand Down Expand Up @@ -122,7 +122,7 @@ VIDEO_MODE_SETTING vidModes[] =
{0x04020204,720,480,60,VIDEO_REGION_NTSCJ,AV_PACK_SVIDEO}, //720x480 NTSCJ 60Hz
};

int iVidModes = sizeof(vidModes) / sizeof(VIDEO_MODE_SETTING);
static int iVidModes = sizeof(vidModes) / sizeof(VIDEO_MODE_SETTING);

static void __stdcall DPC(PKDPC Dpc,
PVOID DeferredContext,
Expand Down Expand Up @@ -292,6 +292,13 @@ unsigned char* XVideoGetFB(void)
return _fb;
}

void XVideoSetFB(unsigned char *fb)
{
assert(((unsigned int)fb & ~0x7FFFFFFF) == 0);
_fb = fb;
VIDEOREG(PCRTC_START) = (unsigned int)_fb & 0x7FFFFFFF;
}

VIDEO_MODE XVideoGetMode(void)
{
return vmCurrent;
Expand Down Expand Up @@ -324,12 +331,28 @@ void XVideoInit(DWORD dwMode, int width, int height, int bpp)

XVideoSetVideoEnable(FALSE);

if (framebufferMemory != NULL) {
MmFreeContiguousMemory(framebufferMemory);
}
framebufferMemory = MmAllocateContiguousMemoryEx(screenSize,
0x00000000, 0x7FFFFFFF,
0x1000,
PAGE_READWRITE |
PAGE_WRITECOMBINE);
assert(framebufferMemory != NULL);
memset(framebufferMemory, 0x00, screenSize);
asm __volatile__("sfence");

do
{
Step = AvSetDisplayMode((PVOID)VIDEO_BASE, Step,
dwMode, dwFormat, pitch, VIDEO_FRAMEBUFFER);
dwMode, dwFormat, pitch, (unsigned int)framebufferMemory & 0x7FFFFFFF);
} while(Step);

/* Store the framebuffer that we set; normally this is done in XVideoSetFB.
However, the first one is set using the kernel, so we do it here, too. */
_fb = framebufferMemory;

XVideoSetFlickerFilter(5);
XVideoSetSoftenFilter(TRUE);

Expand All @@ -341,9 +364,6 @@ void XVideoInit(DWORD dwMode, int width, int height, int bpp)
XVideoSetGammaRamp(0, defaultGammaRampEntries, 256);

XVideoSetVideoEnable(TRUE);

_fb = (unsigned char*)(0xF0000000+VIDEOREG(PCRTC_START));
memset(_fb, 0x00, screenSize);
}


Expand Down Expand Up @@ -442,11 +462,6 @@ void XVideoWaitForVBlank()
VIDEOREG(PCRTC_INTR)=PCRTC_INTR_VBLANK_RESET;
}

void XVideoSetDisplayStart(unsigned int offset)
{
VIDEOREG(PCRTC_START) = (unsigned int) (_fb - 0xF0000000 + offset);
}

unsigned char* XVideoGetVideoBase()
{
return (unsigned char *)VIDEO_BASE;
Expand Down
3 changes: 1 addition & 2 deletions lib/hal/video.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ extern "C"

// Defines for frame buffer
#define VIDEO_BASE 0xFD000000
#define VIDEO_FRAMEBUFFER 0x03c00000

// Hardware access macros
#define VIDEOREG(x) (*(volatile unsigned int*)(VIDEO_BASE + (x)))
Expand Down Expand Up @@ -52,6 +51,7 @@ typedef struct _GAMMA_RAMP_ENTRY

DWORD XVideoGetEncoderSettings(void);
unsigned char* XVideoGetFB(void);
void XVideoSetFB(unsigned char *fb);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all changes in this file are already upstream.

VIDEO_MODE XVideoGetMode(void);

void XVideoSetFlickerFilter(int level);
Expand All @@ -74,7 +74,6 @@ If a value of 0 is provided for the bpp a default value of 32bpp is used.
BOOLEAN XVideoListModes(VIDEO_MODE *vm, int bpp, int refresh, void **p);

void XVideoWaitForVBlank();
void XVideoSetDisplayStart(unsigned int offset);
unsigned char* XVideoGetVideoBase();
int XVideoVideoMemorySize();

Expand Down
5 changes: 1 addition & 4 deletions samples/sdl/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,9 +173,6 @@ static const struct {
"\0\0\0\0\0\0\0\0\0\0\0",
};

//Screen dimension constants
const extern int SCREEN_WIDTH;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has already been fixed in upstream in this commit.

(valid for all three samples)

const extern int SCREEN_HEIGHT;
#define NUM_SPRITES 10
#define MAX_SPEED 5

Expand Down Expand Up @@ -284,7 +281,7 @@ void demo(void)
window = SDL_CreateWindow("Demo",
SDL_WINDOWPOS_UNDEFINED,
SDL_WINDOWPOS_UNDEFINED,
SCREEN_WIDTH, SCREEN_HEIGHT,
640, 480,
SDL_WINDOW_SHOWN);
if(window == NULL)
{
Expand Down
6 changes: 1 addition & 5 deletions samples/sdl_image/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,6 @@ static void printIMGErrorAndReboot(void)
XReboot();
}

// Screen dimension constants
const extern int SCREEN_WIDTH;
const extern int SCREEN_HEIGHT;

void demo(void)
{
int done = 0;
Expand All @@ -45,7 +41,7 @@ void demo(void)
window = SDL_CreateWindow("Demo",
SDL_WINDOWPOS_UNDEFINED,
SDL_WINDOWPOS_UNDEFINED,
SCREEN_WIDTH, SCREEN_HEIGHT,
640, 480,
SDL_WINDOW_SHOWN);
if(window == NULL)
{
Expand Down
5 changes: 1 addition & 4 deletions samples/sdl_ttf/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,6 @@
#include <windows.h>
#include <stdbool.h>

const extern int SCREEN_WIDTH;
const extern int SCREEN_HEIGHT;

int main(void) {
int initialized_SDL = -1;
int initialized_TTF = -1;
Expand Down Expand Up @@ -37,7 +34,7 @@ int main(void) {
window = SDL_CreateWindow("nxdk SDL_ttf sample",
SDL_WINDOWPOS_UNDEFINED,
SDL_WINDOWPOS_UNDEFINED,
SCREEN_WIDTH, SCREEN_HEIGHT,
640, 480,
SDL_WINDOW_SHOWN);
if (window == NULL) {
debugPrint("Window creation failed: %s", SDL_GetError());
Expand Down