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

Remove constraint that virtual framebuffers have to represent VRAM. #11553

Merged
merged 4 commits into from
Nov 12, 2018

Conversation

hrydgard
Copy link
Owner

@hrydgard hrydgard commented Nov 11, 2018

Prerequisite for #11531, virtual readbacks.

Seems to work fine now.

@hrydgard hrydgard added this to the v1.8.0 milestone Nov 11, 2018
@hrydgard hrydgard changed the title Remove constraint that virtual framebuffers have to represent VRAM. WIP: Remove constraint that virtual framebuffers have to represent VRAM. Nov 11, 2018
@hrydgard hrydgard changed the title WIP: Remove constraint that virtual framebuffers have to represent VRAM. Remove constraint that virtual framebuffers have to represent VRAM. Nov 11, 2018
@hrydgard
Copy link
Owner Author

This seems to work now.

Copy link
Collaborator

@unknownbrackets unknownbrackets left a comment

Choose a reason for hiding this comment

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

I think we should just kill off MaskedEqual() entirely. Still a bit worried about what render-to-texture heuristics, but we'll see what happens.

-[Unknown]

@@ -253,10 +253,10 @@ void FramebufferManagerCommon::EstimateDrawingSize(u32 fb_address, GEBufferForma

if (viewport_width != region_width) {
// The majority of the time, these are equal. If not, let's check what we know.
const u32 fb_normalized_address = fb_address | 0x44000000;
const u32 fb_normalized_address = fb_address & 0x3FFFFFFF;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm. This heuristic probably isn't valid outside VRAM anyway - since we're setting fb_address in GetFramebufferHeuristicInputs, maybe the simpler thing is just not to mask this at all (removing this line entirely)?

If fb_address needs masking, then there's a bug in GetFramebufferHeuristicInputs. vfbs_[i]->fb_address possibly still needs masking, of course.

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, you're right - GetFramebufferHeuristic should only be called when we're actually rendering which means it must be VRAM because the GPU is incapable of rendering ot RAM.

I'll get rid of the unnecessary variable here.

@@ -700,7 +700,7 @@ void FramebufferManagerCommon::UpdateFromMemory(u32 addr, int size, bool safe) {
// If we're not rendering to it, format may be wrong. Use displayFormat_ instead.
fmt = displayFormat_;
}
DrawPixels(vfb, 0, 0, Memory::GetPointer(addr | 0x04000000), fmt, vfb->fb_stride, vfb->width, vfb->height);
DrawPixels(vfb, 0, 0, Memory::GetPointer(addr), fmt, vfb->fb_stride, vfb->width, vfb->height);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is inside a MaskedEqual() check which is probably dangerous and needs to be changed. I think MaskedEqual needs removing entirely, or it needs to check the VRAM bit and not mask it out (probably better to just ensure we never put anything in with kernel/cached bits.)

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

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

Removing MaskedEqual entirely. I think this should be fine as-is now.

// These checks are mainly to reduce scanning all textures.
const u32 addr = (address | 0x04000000) & 0x3F9FFFFF;
const u32 addr = address & 0x3F9FFFFF;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We might need to only do this mask when it's in VRAM now, right?

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, that's right..

@@ -750,8 +750,7 @@ bool TextureCacheCommon::AttachFramebuffer(TexCacheEntry *entry, u32 address, Vi
AttachedFramebufferInfo fbInfo = { 0 };

const u64 mirrorMask = 0x00600000;
// Must be in VRAM so | 0x04000000 it is. Also, ignore memory mirrors.
const u32 addr = (address | 0x04000000) & 0x3FFFFFFF & ~mirrorMask;
const u32 addr = (address & 0x3FFFFFFF) & ~mirrorMask;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yup

@hrydgard
Copy link
Owner Author

It's a little bit more sane now. Generally we're probably still doing some unneeded masking here and there since some inputs are guaranteed to not have the kernel/uncached bits but I don't think it matters too much, and it's best to defend against inputs where we don't have that guarantee, like the HLE memcpy etc.

@ghost
Copy link

ghost commented Nov 12, 2018

does this addition work also on android devices??

@hrydgard
Copy link
Owner Author

@owengque This particular change is just preparation for #11531, but yes, it will work and drastically speed up Digimon on all devices including Android.

@ghost
Copy link

ghost commented Nov 12, 2018

I just tried Digimon Adventure, but it's still slow when walking near his friends. if away from many characters it is stable 30fps.

@ghost
Copy link

ghost commented Nov 12, 2018

SD 625.
ADRENO 506.
ANDROID OREO 8.1.
OPEN GL.

@hrydgard
Copy link
Owner Author

First of all, the change hasn't been applied to the current builds yet. Are you building yourself and if so are you including #11531? Secondly it seems you have turned off buffer effects. The point of this is to allow them, making the outlines visible, while still not murdering performance. It's possible the game still isn't fast enough on your device, but this is still going to be a huge improvement with the buffer effects on.

@unknownbrackets unknownbrackets merged commit 256d5e0 into master Nov 12, 2018
@hrydgard hrydgard deleted the framebuffers-outside-vram branch November 12, 2018 14:46
unknownbrackets added a commit to unknownbrackets/ppsspp that referenced this pull request Jun 2, 2019
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.

None yet

2 participants