Automatic horizontal integer scaling implementation on SMS/Genesis/potentially others

[bootsector]

PR is merged!

Can't wait for new core releases based on this! Thank you, Yim!!!!

Edit: I looked at the wrong repo. It wasn't merged yet. Hope the concerns sorg raised can be fixed!

[Yim]

Yeah, the comment from sorgelig is that division and multiplication are costly in terms of logic and reduce the fmax (the maximum clock frequency), so this is not a good addition for the framework. If it works ok in some cores then it could be added to them individually, though I’d have to figure out how. I think I wouldn’t be able to do it in the sys files as I do presently because it would get wiped whenever the framework updates. I could maintain a series of parallel updates by reinserting it after each update and doing unofficial releases, but that sounds like a lot of work.

I have got ideas about alternative framework implementations that don’t use multiplication or division, but I’m not sure how much logic they would consume or what the effect would be on fmax. I might give it a go and see what sorgelig says, but otherwise I’ll have to look at doing it on a core by core basis (or admit defeat).

What I’m thinking is either a for loop that would check the integer output size against the non-integer output size and increment it by the input size until it exceeded it (or a set number of repetitions had passed), or a series of ifs that did the same thing. Wouldn’t be elegant and I’m not sure what it would do to fmax, but should be fairly easily done without any multiplication or division.

[dfilskov]

I've added auto to the menu, and here's the new version of the scaler code:

GREAT work!! - much appreciated! - fingers crossed for a fix and the pull request to be accepted!

Which repository is it? - this one? - https://github.com/MiSTer-devel/Main_MiSTer/pulls ?

[morf77]

I have got ideas about alternative framework implementations that don’t use multiplication or division, but I’m not sure how much logic they would consume or what the effect would be on fmax. I might give it a go and see what sorgelig says, but otherwise I’ll have to look at doing it on a core by core basis (or admit defeat).

What I’m thinking is either a for loop that would check the integer output size against the non-integer output size and increment it by the input size until it exceeded it (or a set number of repetitions had passed), or a series of ifs that did the same thing. Wouldn’t be elegant and I’m not sure what it would do to fmax, but should be fairly easily done without any multiplication or division.

What about RPC-FPGA? This could offload the division and multiplication to the cpu.

https://ieeexplore.ieee.org/document/8280158
https://core.ac.uk/download/pdf/153399229.pdf

Possibly there are downsides to this as well I'm not aware of. Just brainstorming.

[Yim]

GREAT work!! - much appreciated! - fingers crossed for a fix and the pull request to be accepted!

Which repository is it? - this one? - https://github.com/MiSTer-devel/Main_MiSTer/pulls ?

Thanks! My request was on the template, with the goal of getting it into the framework. It looks like that isn't going to happen unless I can figure out a much more optimised way to do it. I did kind of think that this change was too easy to not have been done already - looks like it's harder than I thought.

Here's the build of the genesis core I did last night with the auto option added. It seems to work ok there.

I've taken a shot at a different method of working out the integer scale size, but it seems to be making things worse. I'm starting to think I should leave this stuff to people who know what they're doing instead of fumbling around like I've been doing. If I can't get it to work in a well-optimised way in the framework, then I'll take a look at implementing it in individual cores, but I dunno if I'll be able to pull anything off there.

What about RPC-FPGA? This could offload the division and multiplication to the cpu.

https://ieeexplore.ieee.org/document/8280158
https://core.ac.uk/download/pdf/153399229.pdf

Possibly there are downsides to this as well I'm not aware of. Just brainstorming.

Uhh... maybe? Offloading that stuff makes sense, but I have no idea how to do it or if it would work and figuring it out looks complicated. I have no prior experience with this stuff, only what I've figured out as I go working on this.

[flynnsbit]

@Yim Keep going! You are at least starting something, even if it is a conversation, with a nice Proof of concept, you are learning, and working to understand the MiSTer framework and cores. This is how new and exciting things get added.

[dfilskov]

It looks like that isn't going to happen unless I can figure out a much more optimised way to do it.

Not sure if it helps but a friend and I am/are working on a spreadsheet that calculates the best vscale_border and custom_aspect_ratio settings in MiSTer.ini 's core sections for getting integer scaling in any HDMI resolution: https://docs.google.com/spreadsheets/d/ ... edit#gid=0

Do let us know it you have any feedback.

[Yim]

It could be helpful, depends where I go with this. Thanks.

I’ve made a comment on the pull request asking if there’s any chance of getting my code in if I can improve it. If sorgelig doesn’t want me messing with the scaler (and fair enough if so) then there’s not much point pursuing it. Maybe I could do it somewhere else in the framework, but again that would impact all cores and would need to be well done.

I’ve started thinking about putting it in individual cores, but the problem is that all the relevant information seems to be in the framework. I went with modifying the scaler because that’s where the input and output sizes are, which are the most relevant data to the problem. Custom aspect ratios seem to be handled entirely in the framework, so I’ve got no idea how to react to them by modifying only core-specific code.

The default aspect ratio settings are in the core-specific code. I could probably put in a menu option for integer scaling and use it to set specific aspect ratios to get that result, but the problem is that the aspect ratio needed to get integer horizontal scaling changes depending on the vertical resolution. There is access to HDMI_HEIGHT in the core specific code, so I could maybe do a case statement for each vertical resolution in the standard video modes with an assumption that vertical integer scaling is on. Wouldn’t be pretty, but it might work in some cores at least.

I did come up with what I thought was a less fmax reducing way of doing the integer scaling in the framework - I had it add the input resolution to a variable once per clock cycle whenever it was below the planned output resolution, and then subtracted again if it took it over and narrow scaling was required. Unfortunately it actually blew out the timing worse than my other method, I think because I used too many if statements to figure out whether or not to subtract. Timing stuff is really hard for me to get my head around. I might try a simplified version that only does narrow integer scaling. Wide would be even simpler, but would break when set to full screen.

If I don’t figure things out, maybe I’ll just do a series of builds of various cores using my rejected code and upload them somewhere on a “user beware” basis. It seems to work in the Genesis core, there’s a decent chance it’ll be ok in some of the others too.

[zakk4223]

I have a stupid question.

Can't we just dynamically calculate the correct AR in Main_Mister when it detects a resolution change and send that to the core?
See video.cpp: show_video_info(), video_mode_adjust() and setScaler()

Edit: Hmm, looking deeper into it you can't just dynamically twiddle the AR, you have to 'reinit' the scaler to change the available AR, so that's not too useful. You can however, dynamically set the max vertical and horizontal resolution, although the selected AR is still enforced.

[Yim]

That seems to be how vertical integer scaling is done:

Code: Select all

 uint32_t div = 1 << (cfg.vscale_mode - 1);
uint32_t mag = (scrh*div) / height;
scrh = (height * mag) / div;

Which is essentially the same as how I was trying to do horizontal integer scaling (leaving aside div, which is there to facilitate half and quarter integer scales). I’m gonna have to spend a while figuring things out before I have any chance of making progress this way though. It seems like you could just repeat the above but for horizontal values and tweak the verilog to not implement the aspect ratio, but if it were that easy (“easy”) it’d probably be done already.

[dfilskov]

Did you guys see this? - from Alexey's Patreon

[Zorlac]

With a 4K LG CX OLED monitor, will it be best to enable this running MiSTer at 1920x1080, or would 1920x1440 be better?

[dfilskov]

Both should work with Alexey's new integer Genesis core ... but you should always use a reso that best matches you screen's if possible - 1920x1080 is exactly half 4k afaik - so that's better than 1920x1440 ... we just have to hope that LG doesn't blur the image when upscaling it to double size. Otherwise there's not much to gain from using integer.

[Yim]

I’m glad to see this feature coming, though a little disappointed I didn’t manage to find a way to do it myself that would be accepted. Looking forward to seeing the code and how someone who knows what they’re doing implements it.

[LamerDeluxe]

I’m glad to see this feature coming, though a little disappointed I didn’t manage to find a way to do it myself that would be accepted. Looking forward to seeing the code and how someone who knows what they’re doing implements it.

I think your efforts have played a big part in getting official support, which is really great and much appreciated. I enjoyed coming up with solutions to get integer horizontal scaling with and without a correct aspect-ratio, but they all shouldn't be needed anymore soon. At least we've learned some new things.

[dfilskov]

Hey! - official integer scaling ... yehaaa!

https://www.patreon.com/posts/genesis-megacd-47866110

[Yim]

Nice! I look forward to trying it out. Thanks Sorgelig!

[Zorlac]

Gratz Yim and thank you!! Cant wait to check this out!

[Milspex]

does this mean you dont need custom aspect ratios anymore ?

[Zorlac]

I think in theory this would eliminate worrying about aspect ratio setting if Sorg implements Yim’s idea of having an “auto” option that will choose narrow or wide based on which setting is closer to the correct AR.

[Yim]

I've been playing around with the new horizontal integer scaling code, and I think I've got a decent addition to make it work with aspect ratios other than the default and to add the option to auto select between narrow and wide, whichever is closer to the aspect ratio setting. I've put a Mega Drive fork up on github, including an RBF in the releases folder if you want to try it out. The template version has changed while I've been working on it so I'll need to take a look and maybe adapt what I've done to the latest version before I can make a pull request. As before, I'll describe how I've gone about it, with the assumption that the reader knows about as much as I did before working on it:

First up, a small change to sys_top.v, when it communicates with the module emu, I've added a few inputs as lines 1560-1564:

.ARC1X(arc1x[11:0]),
.ARC1Y(arc1y[11:0]),
.ARC2X(arc2x[11:0]),
.ARC2Y(arc2y[11:0]),

This passes the custom aspect ratios from the .ini file to that module, which is contained in genesis.sv. Speaking of which, I needed to add those inputs to that file, at lines 47-50:

input [11:0] ARC1X,
input [11:0] ARC1Y,
input [11:0] ARC2X,
input [11:0] ARC2Y,

At line 199 I increased the size of the aspect ratio wires, previously 8 bits, to 12:

wire [11:0] arx,ary;

This was necessary because depending on the settings they might wind up carrying the custom aspect ratios, which are 12 bit numbers. Speaking of which, this sections starts at line 202:

if (!ar) begin
case(res) // {V30, H40}
2'b00: begin // 256 x 224
arx = 8'd64;
ary = 8'd49;
end

2'b01: begin // 320 x 224
arx = status[30] ? 8'd10: 8'd64;
ary = status[30] ? 8'd7 : 8'd49;
end

2'b10: begin // 256 x 240
arx = 8'd128;
ary = 8'd105;
end

2'b11: begin // 320 x 240
arx = status[30] ? 8'd4 : 8'd128;
ary = status[30] ? 8'd3 : 8'd105;
end
endcase
end else if (ar==1) begin
arx = HDMI_WIDTH;
ary = HDMI_HEIGHT;
end else if (ar==2) begin
arx = ARC1X;
ary = ARC1Y;
end else begin
arx = ARC2X;
ary = ARC2Y;
end

The first chunk of this is unchanged beyond putting it behind an if statement. If ar is set to zero, it means the aspect ratio is set to "Original" in the OSD. Consequently, the x and y values for the aspect ratio are set to the normal values depending on the output resolution of the mega drive (which can be 256x224, 256x240, 320x224, or 320x240). The bottom part is what I added - if ar=1, then the aspect ratio is set to "Full Screen", so I assign the full size of the screen to be the aspect ratio. If it's 2, that's custom aspect ratio 1. If it's 3, then custom ratio 2. Up next is the bit that passes information to the video_freak module (starting from line 250):

.ARX(((status[56:54]) || (!ar)) ? arx : (ar - 1'd1)),
.ARY(((status[56:54]) || (!ar)) ? ary : 12'd0),
.CROP_SIZE((en216p & vcrop_en) ? 10'd216 : 10'd0),
.CROP_OFF(voff),
.SCALE(status[56:54]),
.FULLSCREEN(ar==2'b01)

I've put the bits I added in bold. status[56:54] carries the integer scaling setting from the OSD. The normal behaviour is that if ar, the aspect ratio setting from the menu, is zero, then arx is passed to video_freak as the aspect ratio's horizontal component and ary as the vertical component. Otherwise, ARX is 0,1, or 2, signifying full screen or custom aspect ratios (which are implemented elsewhere normally), and y is set to zero. My change here is that if I've set arx and ary as described above and the user has turned on integer scaling in the OSD then I want to pass the values I set above through to video_freak so it can determine the required size. I added fullscreen at the end there. It gets set to 1 if the aspect ratio is set to "Full Screen", and zero otherwise. I missed having this on the first pass, with the result that if integer scaling was on the horizontal size would be scaled for aspect ratio relative to the integer scaled vertical size and often wind up smaller than the full screen.

"P1oMO,Scale,Normal,V-Integer,Narrower HV-Integer,Wider HV-Integer,Auto HV-Integer;",

This last change in genesis.sv is at line 290, and adds the option for automatic narrow or wide integer selection. This increases the number of options from 4 to five, so I need a three bit number to store the selection, so P1oMN became P1oMO. Changing this changes status[56:54], mentioned above. I got totally stuck for several days because I missed putting a comma between "HV-Integer" and "Auto" at this point. That left only three menu options, one of which was "Wider HV-IntegerAuto HV-Integer". Having a menu option that's too long causes the OSD to freeze, which I knew already from having the same problem earlier but which I didn't twig to for a couple days when it happened again.

Last up is video_freak.sv. Prior to the introduction of horizontal integer scaling, this file was video_crop.sv. I guess having more functions required a name change. Not sure if "freak" is intended as a comment on people who wanted horizontal integer scaling or if it means something else. Anyways, at line 32:

input [2:0] SCALE,
input FULLSCREEN

The inputs for the scale selection and whether the aspect ratio is set to "Full Screen" or not. SCALE was there already, but I had to increase its size to accommodate the new option. The first half or so of this file determines the size of the output from the core (i.e. the resolution of the original system), then does the crop setting, which I didn't fiddle with. The second part does the calculations for integer scaling. My original implementation of horizontal scaling just stuck some divisions and multiplications into the main scaler. These are costly operations in hardware, and so risked upsetting the timer of the scaler. Sorgelig's version is much cleverer than mine: it puts in a bunch of registers to hold multiplication and division arguments and results, and wires to show if a calculation is running, and then breaks the process into steps and waits at each step until the previous calculation is finished before moving on to the next. I'm probably not describing it accurately. Basically the information is processed on the side rather than in the middle of other stuff, I think. The process is:

0: divide screen height by core vertical resolution, rounding down. This gives the vertical integer scaling multiple.

1: multiply that multiple by the core vertical resolution, which gives the integer scaled vertical resolution.

2: store that resolution in a register for later, then multiply it by the horizontal aspect ratio setting

3: divide the result of that by the vertical aspect ratio setting. This gives the non-integer scaled horizontal resolution.

4:

w_nonint <= FULLSCREEN ? HDMI_WIDTH : div_res[11:0];
div_num <= div_res;
div_den <= hsize;
div_start <= 1;

store that resolution in a register (which I added - the original version was able to get this value from the register holding the division numerator in this next step, but I'm gonna add another division which will wipe that value so I need to store it elsewhere. If FULLSCREEN is set, then I instead set the non-scaled width to the size of the screen), then divide it by the core horizontal aspect ratio rounding down to get the narrow integer scaling multiple.

5: multiply that multiple by the core horizontal resolution to get the narrow integer scaled horizontal resolution. The wider resolution is obtained by adding the core resolution to this figure.

6:

6: if((mul_res <= HDMI_WIDTH) && !FULLSCREEN) begin
div_num <= w_nonint[11:0] - mul_res[11:0];
div_den <= hsize[11:1];
div_start <= 1;
cnt <= 8;
end else begin
div_num <= HDMI_WIDTH;
div_den <= hsize;
div_start <= 1;

I fiddled with this step a bit. This is the check that the output resolution isn't wider than the screen. mul_res at the start is the result of the multiplication in the previous step. If it's less wide than the screen (HDMI_WIDTH), then all is good and we can check whether the auto scaling will go narrow or wide, which is what the first set of div_whatevers does here - w_nonint (the non-integer scaled resolution) minus mul_res (the integer scaled resolution) is the remainder of the division (the division module has a remainder output available, but it doesn't seem to give the remainder of the division). I divide that by half of the core resolution (by only looking at the first eleven bits rather than all twelve the number is halved). If the result (which rounds down) is zero, then the remainder is less than half of another multiple, so I want narrow scaling. If it's non-zero, then wide scaling. After finding that step 7 is skipped.

The second half of this code, only used if the resolution is too wide or FULLSCREEN is on, corrects for the output resolution being too wide - it finds the largest multiple for integer scaling which will fit in the screen.

7: Step seven multiplies the multiple from the second part of 6 by the core horizontal resolution to get the output resolution.

8:

arxf <= {1'b1, !SCALE[2:1] ? w_nonint[11:0] : ((div_res && SCALE[2] || SCALE[0]) && (wideres <= HDMI_WIDTH)) ? wideres : mul_res[11:0]};
aryf <= {1'b1, oheight};

This determines the output aspect ratio figures. 1'b1 at the start sets the most significant bit to 1. This tells the rest of the code that the aspect ratio figure contains a resolution rather than an aspect ratio. Then there's the logic for choosing which aspect ratio to use. It's kind of hard to read, so I'll try to explain. First it checks if the first two bits of SCALE are both zero. If so, then horizontal integer scaling is turned off and it uses w_nonint, the register I set at step 4. Next, it checks if div_res and the first bit of SCALE both have a non-zero value. If so, then auto width selection is turned on and wide is the correct choice (dis_res is the result of dividing the remainder by half the resolution, unless the horizontal resolution was wider than the screen in which case I actually want narrow, but there's another check coming which will clean that up). Alternatively, if the last bit of SCALE is set to 1, that means wide scaling is turned on (given that we've already checked that we are using horizontal integer scaling). Then we check if wideres (which is the narrow resolution plus the core resolution) is wider than the screen. If it's not and wide scaling is on, that's used as the resolution value. If it is or we're chasing narrow resolution, then the latest multiplication result is used. Since the last multiplication was to find the integer scaled resolution, that's the value we want.

The vertical figure is simpler. It's the height stored back in step 2. This whole process is skipped if vertical integer scaling is turned off, so there's nothing else to check at this point.

And that's it! Hope it makes sense. Like I said, I've got a build of this for mega drive up on github. Now I just need to make a fork of the template, stick my changes in there, and do a pull request. video_freak has changed on the template since I started working on the MD version though, so I'll have to take a look and see if any further adaptations are required.

[Harrumph]

Thanks for documenting your progress and thought process, it’s super valuable for a programming novice!

[Yim]

I've put in a pull request. Had to adjust things a little for the differences in the template version, hopefully I haven't screwed anything up.

Thanks for documenting your progress and thought process, it’s super valuable for a programming novice!

Thanks! I'm very much a novice myself, so I hope I haven't said too much that isn't correct along the way.

[flynnsbit]

I've put in a pull request. Had to adjust things a little for the differences in the template version, hopefully I haven't screwed anything up.

Thanks for documenting your progress and thought process, it’s super valuable for a programming novice!

Thanks! I'm very much a novice myself, so I hope I haven't said too much that isn't correct along the way.

You should drop into the MiSTer Discord #dev-talk channel. There is also a section for test builds and unstable nightlies. It might benefit you to be able to interact and ask questions to the other core devs. It's a really good group and specifically the dev channel for you.
https://discord.gg/MYGaRnHk

[Yim]

My pull request is closed, not accepted, but Sorgelig has added auto narrow/wide to the framework and says he’ll add an integer option for full screen. Automatically adjusting custom aspect ratios to be integer scaled isn’t going to happen. Pretty good outcome on the whole, I think. Thanks Sorgelig!