Skip to content
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.

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

nv2a: Handle SIZE_IN > SIZE_OUT case #1178

Conversation

abaire
Copy link
Contributor

@abaire abaire commented Jul 8, 2022

NV_PVIDEO_SIZE_IN is set to 0xFFFFFFFF during initialization and teardown of the PVIDEO overlay. In some cases this can happen before the overlay is explicitly stopped, leading to an assert.

On hardware any SIZE_IN > SIZE_OUT is silently capped to SIZE_OUT.

Fixes #330

Test

@Triticum0

This comment was marked as spam.

@abaire
Copy link
Contributor Author

abaire commented Jul 8, 2022

It worked for https://xemu.app/titles/57450001/#Ultimate-Beach-Soccer

Excellent. I was able to figure out why the tests were breaking, but fixing that also slightly changed the behavior; rather than immediately tearing down the overlay it looks like it just freezes the frame. Will push a new change in a couple hours when I can get back to this.

UPDATE: It doesn't freeze the frame, it looks like it actually just treats the input size as the same as the output size and that this is generally the case (setting SIZE_IN to 2x SIZE_OUT acts as though they're equal, it does not scale the content to fit SIZE_OUT)

@abaire abaire force-pushed the fix/330/handle_sentinel_value_pvideo_size_in branch from 2be5703 to b5ce084 Compare July 8, 2022 19:58
@abaire abaire changed the title nv2a: Handle sentinel value for SIZE_IN register nv2a: Handle SIZE_IN > SIZE_OUT case Jul 8, 2022
@abaire abaire marked this pull request as ready for review July 8, 2022 19:59
@Triticum0

This comment was marked as outdated.

@Triticum0

This comment was marked as outdated.

@Triticum0

This comment was marked as off-topic.

@Triticum0

This comment was marked as off-topic.

@abaire
Copy link
Contributor Author

abaire commented Jul 8, 2022

This is what I get from midway arcade

  0x00007ff7054b751f in pgraph_render_display_pvideo_overlay (
    d=d@entry=0x21405219040) at ../hw/xbox/nv2a/pgraph.c:5114
        pg = 0x2140523bc98
        enabled = <optimized out>
        base = 0
        limit = 134217727
        offset = 60874752
        in_width = 720
        in_height = 480
        in_pitch = 1280
        in_color = <optimized out>
        out_width = 720
        out_height = 480
        out_x = 0
        out_y = 0
        color_key_enabled = <optimized out>
        color_key = <optimized out>
        end = <optimized out>

That one is unusual since the in_size is already equal to the out_size and all of the values other than pitch look reasonable. I think this is a different bug and should be filed separately. I'll write a separate test that lies about the pitch and see how the HW behaves.

I also just ordered the game and can look at it in a couple weeks when it arrives.

UPDATE: from my test it looks like the hardware can handle the pitch being < the expected pitch just fine. It still respects the pitch, so you can get a staircase effect.

@abaire abaire marked this pull request as draft July 8, 2022 21:53
@abaire
Copy link
Contributor Author

abaire commented Jul 8, 2022

Also Trying to go in-game on Ultimate Beach Soccer by memory also shows the whole screen corrupted previously you could make it in-game and actually see what was going on.

Can you enable the nv2a_reg_write trace and capture the end of the video for one of those games? I need to see what is happening to the PVIDEO register when the overlay should be torn down.

As far as I can see from my test case, it is valid to give an incorrect (larger than output) size and the hardware does not tear down the display, so something else must be happening for these games that exhibit issues without the need to eject the disc or reset.

@Triticum0

This comment was marked as off-topic.

@abaire
Copy link
Contributor Author

abaire commented Jul 8, 2022

How do you specify with trace-event to only log pvideo register writes. Also, how do I step through the could without hitting an assertion? Do I have to place a breakpoint and then trace from there because when I run Xemu on symbols using gdb, it seems I can't type any commands while it running through the terminal?

In the monitor trace-event nv2a_reg_write on will turn on the reg write events.

If you're running with this PR it sounds like you shouldn't hit the assertion (for Ultimate Beach Soccer, let's ignore the Midway case for the moment, I'll have a prospective fix later as I can already see that the HW handles pitch < expected gracefully).

@Triticum0

This comment was marked as outdated.

@abaire
Copy link
Contributor Author

abaire commented Jul 8, 2022

Here you go. I figure out how to do it eventually. Is stepping through the code per frame possible without your debug pr. Thank for the help Ultimate-Beach-Soccer.log

You can in GDB; set a breakpoint in nv2a_profile_flip_stall in pgraph.c and it'll be close to the same behavior as with my debugger window.

@Triticum0

This comment was marked as off-topic.

`NV_PVIDEO_SIZE_IN` is set to 0xFFFFFFFF during initialization and teardown
of the PVIDEO overlay. In some cases this can happen before the overlay is
explicitly stopped, leading to an assert. On hardware SIZE_IN values larger
than SIZE_OUT are capped (content is not scaled).

Fixes xemu-project#330

[Test](https://github.com/abaire/nxdk_pgraph_tests/blob/main/src/tests/pvideo_tests.cpp)
@abaire abaire force-pushed the fix/330/handle_sentinel_value_pvideo_size_in branch from b5ce084 to fd2b0d4 Compare July 8, 2022 23:16
@abaire
Copy link
Contributor Author

abaire commented Jul 8, 2022

how do you do that

Ping #general or #help, it's not something related to this code review :)

Can you check Midway again with the latest version of this PR? I confirmed that a pitch less than a full row is supported by the hardware and removed the assert. The output doesn't exactly match HW but is close.

Also looking at the log you posted, I don't see an explicit teardown of the PVIDEO, but I do see some writes to PVIDEO registers that I've not seen before so I'm guessing that may be what's missing. Will write another test case to see what happens when I duplicate what I see logged.

UPDATE: Writing to those registers has no effect, so I'm not sure what causes the PVIDEO to be hidden in that game. As far as I can see the overlay should remain up forever...

@abaire abaire marked this pull request as ready for review July 8, 2022 23:44
@abaire
Copy link
Contributor Author

abaire commented Jul 8, 2022

@Triticum0 can you test all games again with the commit I just now pushed (sorry to keep asking for retests!)

I don't see anything in the Ultimate Beach Soccer log that looks like it should tear down the overlay, but I also have checked a number of games and only see 0xFFFFFFFF being used when video is about to be shown (immediately replaced with a valid size) or about to be torn down (promptly followed by a write to the STOP register with the low bit set). Thus I think it's safe for now to treat it as a command that should just hide the video. If we find a real world case where this causes a problem (e.g., a video overlay blinking or disappearing) we can dig into how UBS is causing the overlay to be hidden and remove the FIXME that I added.

@Triticum0

This comment was marked as outdated.

@Triticum0

This comment was marked as off-topic.

@Triticum0

This comment was marked as outdated.

@abaire
Copy link
Contributor Author

abaire commented Jul 9, 2022

Won't be able to test until later tonight, but I should also mention the misalignment of the FMV on masters only happens once the FMV ends or if you try to the bailout of the FMV manually to go to the menus, which causes the assert to hit. In the recent pr where you try to handle the SIZE_IN > SIZE_OUT case is what causes the misalignment to happen during the whole FMV.

If you can let me know if the behavior is the same after the latest commit I can take a look. From the log you sent I'm not sure how this could happen, as the size in is clearly less than the size out except for the end of the video when it sets it to 0xFFFFFFFF

nv2a_reg_write PVIDEO [0x704] = 0x00000000: Stop: Overlay active: False, Stop immediately: True
nv2a_reg_write PVIDEO [0xb00] = 0x00000000: color key: 0x00000000
nv2a_reg_write PVIDEO [0x920] = 0x0337d000: offset[0]: Offset: 0x000cdf40
nv2a_reg_write PVIDEO [0x930] = 0x00000000: point_in[0]: 0, 0
nv2a_reg_write PVIDEO [0x928] = 0x024002d0: size_in[0]: 360 x 576
nv2a_reg_write PVIDEO [0x938] = 0x001200cd: ds/dx[0]: 0x00024019 (147481)
nv2a_reg_write PVIDEO [0x940] = 0x001334e9: dt/dy[0]: 0x0001334e (78670)
nv2a_reg_write PVIDEO [0x948] = 0x00000000: point_out[0]: 0, 0
nv2a_reg_write PVIDEO [0x950] = 0x01e00280: size_out[0]: 640 x 480
nv2a_reg_write PVIDEO [0x958] = 0x000105c0: format[0]: Pitch: 1472, LE_CR8YB8CB8YA8, Display always
nv2a_reg_write PVIDEO [0x908] = 0x07ffffff: limit[0]: 2097151 (0x001fffff)
nv2a_reg_write PVIDEO [0x140] = 0x00000001: Interrupt enabled: Buffer0: True, Buffer1: False
nv2a_reg_write PVIDEO [0x700] = 0x00000001: Use: Buffer0: True, Buffer1: False

So for the frames in the log, the change should have no effect at all until the part that would've asserted. It may be that there is some initial frame that isn't in the log that has things mismatched, but the output size and position are controlled by the out register and not influenced by this PR.

If it's still doing this on with the latest commit, can you get a nv2a_reg_write trace of the entire video playback? (Start the trace before you start the video)

@Triticum0

This comment was marked as outdated.

@Triticum0

This comment was marked as outdated.

@abaire abaire force-pushed the fix/330/handle_sentinel_value_pvideo_size_in branch from 3d5b8e7 to 9a39898 Compare July 11, 2022 22:08
@abaire
Copy link
Contributor Author

abaire commented Jul 11, 2022

@Triticum0 there was a silly mistake in the way I was doing the scaling, fixed now. I don't think it matches hardware but hopefully looks better now.

@Triticum0
Copy link

Still has the orginal issue as before im using a usa verison i redumped it off my xbox again just to make sure the dump wasn't causing any issue hopfully this log will be more useful

Here another log
first.txt

2022-07-12.00-19-51.-.Copy.mp4

@Triticum0

This comment was marked as off-topic.

@abaire
Copy link
Contributor Author

abaire commented Jul 11, 2022

I'm confused as to how it's behaving so differently from my tests using the exact same parameters.. You don't have to capture any more logs, none of the changes I'm making to attempt to fix the scaling will have any effect on what is logged so there's nothing useful in there.

What are your View menu settings (resolution scale and display mode)?

@Triticum0
Copy link

Also this fixes breaks other games with seeing over y axis top boarders as well as misalignment on fmv other than the games affected.

@abaire
Copy link
Contributor Author

abaire commented Jul 11, 2022

Also this fixes breaks other games with seeing over y axis top boarders as well as misalignment on fmv other than the games affected.

Can you give me a few examples? That'll probably help me figure out where things are different. I've been using Serious Sam as a spot test, the intro FMV looks correct to me, as does the video that plays on the loading screen.

@abaire
Copy link
Contributor Author

abaire commented Jul 11, 2022

Update: I found a video that ends up being wrong, the THQ intro for Spongebob. Will debug and push another fix.

@Triticum0
Copy link

scaled and 1x internal resoultion. The seeing over y axis top boarders only occurs on pal region game i tested call England International Football on the codemaster intro

@abaire abaire force-pushed the fix/330/handle_sentinel_value_pvideo_size_in branch from 9a39898 to f241ce1 Compare July 12, 2022 00:48
@abaire
Copy link
Contributor Author

abaire commented Jul 12, 2022

There was a conflict between the way I was blindly capping width/height in the older commit and the correct handling of the dIn/dOut registers. The output of the pgraph test now matches HW and the Spongebob THQ video (720x480=>640x480) is no longer wrapping incorrectly.

@Triticum0 can you check again with this new commit?

@Triticum0
Copy link

yes, give me 15 mins

@Triticum0
Copy link

I retested it looks correct there are just now 4 diffrent screens simultaneously

2022-07-12.02-34-27.-.Copy.mp4

@abaire
Copy link
Contributor Author

abaire commented Jul 12, 2022

I retested it looks correct there are just now 4 diffrent screens simultaneously

2022-07-12.02-34-27.-.Copy.mp4

I went ahead and ordered the game, it's not clear to me why the behavior differs so much between what you're seeing and what I see in other games and my test program. Will come back to this in a couple weeks when it arrives.

I am somewhat curious what you see if you run the "PVIDEO" > "PAL into NTSC" test from https://github.com/abaire/nxdk_pgraph_tests/releases, but I expect I'll be able to figure out whatever is broken once I can test directly.

@Triticum0
Copy link

It is the same thing that happens with the video in the in the commit
xemu-2022-07-12-02-54-28

@abaire
Copy link
Contributor Author

abaire commented Jul 12, 2022

Thanks for testing, there is something very different between the behavior on your machine and my my two, I don't see doubling on either.

I assume you're still testing in Windows on your nvidia card?

Just to rule it out, can you also try rebooting?

@Triticum0
Copy link

yes, I'm using Nvidia 30 series and windows updated to the latest drivers + reboot did nothing.

@abaire
Copy link
Contributor Author

abaire commented Jul 12, 2022

yes, I'm using Nvidia 30 series and windows updated to the latest drivers + reboot did nothing.

Thanks for checking. I have one last thing to try, I don't expect it to make any difference but want to rule out the possibility that textureSize does something different in your config than mine. Mind doing one last test?

Thanks again for all your help remote-debugging this :)

ds_dx and dt_dy describe how the PVIDEO content should be scaled to fit the
output area. Each is calculated via `((in - 1) << 20) / (out - 1)`, this
commit calculates the full frame scale (in / out) and applies that when
determining the texture coordinates for the overlay.
@abaire abaire force-pushed the fix/330/handle_sentinel_value_pvideo_size_in branch from c2d1134 to 353de20 Compare July 12, 2022 15:02
@abaire
Copy link
Contributor Author

abaire commented Jul 12, 2022

@Triticum0 I was able to reproduce the quadrupling of output by setting my render scale to 2x, can you double check that your render scale was still set to 1x in your last round of testing?

I also just pushed a fix that handles higher scales, so hopefully that'll also make the problem go away.

@Triticum0
Copy link

Triticum0 commented Jul 13, 2022

@abaire Yeah can say with full confidence that you fixed all the issues that I was having on the pr. It seems to match hardware now. Thank you for your patients and hard work.
PR:

2022-07-13.21-36-43.mp4

Hardware:
https://www.youtube.com/watch?v=tbx3iUc26kk

Note: retested all games on the list not work properly and other issues i have are fixed

@abaire abaire marked this pull request as ready for review July 13, 2022 20:48
@Triticum0
Copy link

Note: special case doesn't fix Broken-Sword-The-Sleeping-Dragon and Disney-s-The-Haunted-Mansion. haven't test with both pr combined and squash commits when made the test build don't know if it has an effect?

@abaire
Copy link
Contributor Author

abaire commented Jul 13, 2022

Note: special case doesn't fix Broken-Sword-The-Sleeping-Dragon and Disney-s-The-Haunted-Mansion. haven't test with both pr combined and squash commits when made the test build don't know if it has an effect?

I suspect they're just different underlying issues, can you file separate title bugs for them?
The fact that Broken Sword interacts poorly with #1146 is in indication that it's doing some interesting things with CPU blitting which may contribute to or cause whatever the current behavior is. I'm not sure what the behavior is for the haunted mansion so I'd need to see a video of that to guess at what might be happening (ideally in a new title-specific bug).

@Triticum0
Copy link

Triticum0 commented Jul 13, 2022

Edit #1183 has I video so you can look at what it looks like. Ping me if you need any help

@mborgerson mborgerson merged commit f29c2ff into xemu-project:master Jul 18, 2022
@abaire abaire deleted the fix/330/handle_sentinel_value_pvideo_size_in branch July 18, 2022 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Assertion] in_pitch >= in_width * 2
4 participants