Add support for VBE_DISPI_INDEX_X_OFFSET

[chschnell]

Adds support for horizontal offset in Bochs VESA BIOS Extension.

Should fix issue #1088.

[copy]

I don't believe this change is correct. The vga code copies memory linearly (and thus only supports offsets on the y axis). See https://github.com/copy/v86/blob/master/src/rust/cpu/vga.rs

Note that virtual width/height are also not supported. Writes to the corresponding registers (6 and 7) are ignored, and reads simply return full width/height.

[chschnell]

I don't believe this change is correct. The vga code copies memory linearly (and thus only supports offsets on the y axis). See https://github.com/copy/v86/blob/master/src/rust/cpu/vga.rs

This PR does not change the linear memory layout in VGA RAM.

What it changes is the calculation of the fixed offset in VGA memory this.svga_offset where this linear block of memory begins.

As you pointed out, this.svga_offset can currently only be a multiple of the horizontal screen size, in vga.js lines 2126-2136 it is effectively calculated as:

this.svga_offset = this.svga_offset_y * this.svga_width;

This PR only extends this calculation to:

this.svga_offset = this.svga_offset_y * this.svga_width + this.svga_offset_x;

This does not change the linear nature of the memory block that starts at this.svga_offset, only the fixed offset where it starts.

Note that virtual width/height are also not supported. Writes to the corresponding registers (6 and 7) are ignored, and reads simply return full width/height.

Yes, and I have not changed that, this PR is independent of virtual width and height. I understand that SVGA in V86 is linear-only.

Let's just take a step back and look at what motivated this PR.

To better see this defect I've made two new 1280x960 screenshots, both show a maximized terminal window running the top command (to compare it's best to open these two screenshots in separate browser tabs).

Before this PR:

After this PR:

It helps to zoom in into the top-right corner of the first screenshot, notice the top scanline and the 1px Y-offset:

This looks exactly like the visual glitch I would expect if an offset into VGA memory was off by some value X_OFFSET being less than X_WIDTH (the length of a single scanline which is 1280 in this case). If the difference was larger than X_WIDTH then we would see only a section or even nothing in the first screenshot, but we see all lines (although shifted by X_OFFSET).

When I looked into this I noticed two things:

1. the "cut" on the right side of the first screenshot is at X = 1024, and
2. Alpine attempts to write 1024 into the VBE_DISPI_INDEX_X_OFFSET register when setting this 1280x960 video mode.

Hence I included VBE_DISPI_INDEX_X_OFFSET in the calculation of this.svga_offset as described above, which worked and eventually led to this PR.

I've looked over the rust code but fail to see how it is affected by this PR other than being passed a different value in argument svga_dest_offset (being this.svga_offset) which it simply uses as the linear offset for the copy destination buffer. My rust knowledge is probably insufficient, I'd kindly ask you to point me more directly to the problem that this PR causes in cpu/vga.rs.

[chschnell]

Bochs handles VBE_DISPI_INDEX_Y_OFFSET and VBE_DISPI_INDEX_X_OFFSET structurally the same as in my interpretation (the details are a bit different because Bochs implements VBE_DISPI_INDEX_VIRT_WIDTH), see:

https://sourceforge.net/p/bochs/code/HEAD/tree/trunk/bochs/iodev/display/vga.cc#l1180

In addition, when the SVGA gets switched into enabled state in the handler of VBE_DISPI_INDEX_ENABLE, Bochs resets offset_x (VBE_DISPI_INDEX_X_OFFSET) and offset_y to 0, I think that's a good idea and missing from this PR, if you want I can add similar code to this PR at the equivalent spot in vga.js.

[copy]

I see, thanks for the explanation. I was under the impression that fixing the bug required virtual size support, and that x offset only made sense in conjunction with virtual size support. Alpine's behaviour here is a bit weird, but I see how the fix is correct.

In addition, when the SVGA gets switched into enabled state in the handler of VBE_DISPI_INDEX_ENABLE, Bochs resets offset_x (VBE_DISPI_INDEX_X_OFFSET) and offset_y to 0, I think that's a good idea and missing from this PR, if you want I can add similar code to this PR at the equivalent spot in vga.js.

Yes, feel free to include that change in this PR, then I'll merge.

[chschnell]

I added the three lines to the PR.

It's a big pleasure for me to contribute to this great project!

[chschnell]

Alpine's behaviour here is a bit weird, but I see how the fix is correct.

I was also wondering why it's doing that.

IIRC that X_OFFSET changes when you modify the screen resolution in Alpine, and it didn't appear plausible to me in any way.

However, this PR fixes that, all screen resolutions in Alpine work now.

[copy]

It's a big pleasure for me to contribute to this great project!

Appreciate your contributions :-)