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

Extend Minimal TMEM cache implementation. #8350

Open
wants to merge 3 commits into
base: master
from

Conversation

@phire
Copy link
Member

commented Sep 6, 2019

Now works with games that deliberately avoid invalidating TMEM because they know textures are too large to fit:

  • Sonic Riders
  • Metal Arms: Glitch in the System
  • Godzilla: Destroy All Monsters Melee
  • NHL Slapshot
  • Tak and the Power of Juju
  • Night at the Museum: Battle of the Smithsonian
  • 428: Fūsa Sareta Shibuya de

This is just a first pass, while it should work, I'm not happy with how the code is structured.
Mostly commiting for fifoci run and potential feedback.

@phire phire added the WIP label Sep 6, 2019

Extend Minimal TMEM cache implementation
Now works with games that deliberately avoid invalidating TMEM because
they know textures are too large to fit:

 * Sonic Riders
 * Metal Arms: Glitch in the System
 * Godzilla: Destroy All Monsters Melee
 * NHL Slapshot
 * Tak and the Power of Juju
 * Night at the Museum: Battle of the Smithsonian
 * 428: Fūsa Sareta Shibuya de

This is just a first pass, while it should work, I'm not happy with
how the code is structured.

@phire phire force-pushed the phire:fix-tmem branch from c7ca06e to b1b4095 Sep 6, 2019

@phire

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2019

Rebased for clean fifo-ci run.

Looks like I broke Spyro... This is going to need some more work
https://fifoci.dolphin-emu.org/compare/4964251-4953629/

// On TMEM configuration changed:
// 1. invalidate stage.

struct texture_unit_state

This comment has been minimized.

Copy link
@lioncash

lioncash Sep 6, 2019

Member
Suggested change
struct texture_unit_state
struct TextureUnitState

struct texture_unit_state
{
enum State

This comment has been minimized.

Copy link
@lioncash

lioncash Sep 6, 2019

Member
Suggested change
enum State
enum class State
};
static std::array<texture_unit_state, 8> s_unit;

void ConfigurationChanged(int bp_addr, int config)

This comment has been minimized.

Copy link
@lioncash

lioncash Sep 6, 2019

Member

bp_addr should likely be a u32, given its an address.

// On invalidate cache
// 1. invalidate all

void Invalidate(u32 param)

This comment has been minimized.

Copy link
@lioncash

lioncash Sep 6, 2019

Member
Suggested change
void Invalidate(u32 param)
void Invalidate([[maybe_unused]] u32 param)
// 2. if texture size is small enough to fit in region AND there is no overlap, mark as cached.
// 3. If there is cached overlap, invalidate.

static inline u32 calculate_unit_size(TexImage1 config)

This comment has been minimized.

Copy link
@lioncash

lioncash Sep 6, 2019

Member

inline is superfluous here given static is already specified. This should also be named CalculateUnitSize

return 512 * (1 << config.cache_width) * (1 << config.cache_height);
}

static inline u32 calculate_unit_size(TexImage2 config)

This comment has been minimized.

Copy link
@lioncash

lioncash Sep 6, 2019

Member

Ditto

{
bool has_overlap = false;
// We are just going to do a linear search
for (int i = 0; i < s_unit.size(); i++)

This comment has been minimized.

Copy link
@lioncash

lioncash Sep 6, 2019

Member

i should be a size_t, otherwise this introduces a signed/unsigned mismatch.

{
void Invalidate(u32 param);
void ConfigurationChanged(int bp_addr, int config);
void Bind(u8 unit, int num_blocks_width, int num_blocks_height, bool is_mipmapped, bool is_32_bit);

This comment has been minimized.

Copy link
@lioncash

lioncash Sep 6, 2019

Member

These u8s should probably be u32. u32 is slightly more flexible when it comes to interface changes outside of this source file, given it's very easy to implicitly truncate to u8, causing warnings.

@@ -0,0 +1,16 @@
// Copyright 2010 Dolphin Emulator Project

This comment has been minimized.

Copy link
@lioncash
int odd_base;
int odd_size;
};
static std::array<texture_unit_state, 8> s_unit;

This comment has been minimized.

Copy link
@lioncash

lioncash Sep 6, 2019

Member

Is there really no other place to place this (like in the actual cache)? This introduces more file-scope/global mutable state.

This comment has been minimized.

Copy link
@phire

phire Sep 6, 2019

Author Member

I feel really uneasy about how complicated texture cache is getting and just how complicated it is to understand.

This part is still WIP, this state will need to be stored somewhere, and it will need initialized to zero, and something will need to be done on DoState (either we invalidate the whole thing, or for correctness we save anything that's tagged as "cached")

@BhaaLseN
Copy link
Member

left a comment

Do you intend to address those FIXME in here, or in a follow-up PR?

@@ -88,6 +88,8 @@ add_library(videocommon
TextureDecoder.h
TextureDecoder_Common.cpp
TextureDecoder_Util.h
TmemBase.cpp
TmemBase.h

This comment has been minimized.

Copy link
@BhaaLseN

BhaaLseN Sep 6, 2019

Member

This line needs more space ;)

case BPMEM_TX_SETIMAGE3:
case BPMEM_TX_SETIMAGE3_4:
TextureCacheBase::InvalidateAllBindPoints();
TmemBase::ConfigurationChanged(bp.address, bp.newvalue);

This comment has been minimized.

Copy link
@BhaaLseN

BhaaLseN Sep 6, 2019

Member

It feels odd that 0 and 3 do something differently to 1 and 2. Maybe it would be a good idea to leave a few comments on why they're different?

if (IsValidBindPoint(stage) && bound_textures[stage])
{
return bound_textures[stage];
TCacheEntry* entry = bound_textures[stage];
// If the TMEM configuration is such that this texture is more or less guarneteed to still

This comment has been minimized.

Copy link
@BhaaLseN

BhaaLseN Sep 6, 2019

Member
Suggested change
// If the TMEM configuration is such that this texture is more or less guarneteed to still
// If the TMEM configuration is such that this texture is more or less guaranteed to still
{
enum State
{
INVALID, // Configuration has changed

This comment has been minimized.

Copy link
@BhaaLseN

BhaaLseN Sep 6, 2019

Member

Thoughts on putting the comment in the line before rather than at the end of the line? At least Visual Studio shows those in Intellisense if they're in the line before.


void ConfigurationChanged(int bp_addr, int config)
{
// Extract bits encoding texture unit

This comment has been minimized.

Copy link
@BhaaLseN

BhaaLseN Sep 6, 2019

Member

Would you mind extending that comment with more info on what the bits in bp_addr mean (or, if it is easier: why the shifting and masking is done)?


void Invalidate(u32 param)
{
// The excat arguments of Invalidate commands is currently unknown.

This comment has been minimized.

Copy link
@BhaaLseN

BhaaLseN Sep 6, 2019

Member
Suggested change
// The excat arguments of Invalidate commands is currently unknown.
// The exact arguments of Invalidate commands is currently unknown.
return (a <= b && (a + a_size) >= b) || (b <= a && (b + b_size) >= a);
}

static bool process_overlaps_single(u8 unit_id, int base, int size)

This comment has been minimized.

Copy link
@BhaaLseN

BhaaLseN Sep 6, 2019

Member

Would there be any benefit in having a struct parameter instead of separate base/size and reuse the same as TextureUnitState.even/TextureUnitState.odd? That way you could move the overlap method into that struct; potentially as operator if it makes sense; plus get the added benefit of less ambiguity when faced with 2 or 4 int parameters (like, is it base or size first; or are they next to each other in the overlap case?)
It could also be the home for CalculateUnitSize (and perhaps others) if it makes sense in there.

This comment has been minimized.

Copy link
@phire

phire Sep 6, 2019

Author Member

I'm really unhappy with this code and the way implementation details (finding overlaps) is mixed with the higher level concepts.

What I want to do is implement a generic interval tree implementation in common, which appears to be the correct data structure for storing and checking for overlaps.

Then I can just implement tmem in terms of the interval tree.

@phire

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2019

FIFOci update:

Good news:

Average news:

Bad news:

@JMC47

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2019

Are we sure those last two are really broken? Those are ancient fifologs. And Sonic's gravity boost was already broken.

@JMC47

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2019

The two games mentioned as broken in those fifologs are only broken in EFB2Tex. In EFB2RAM, they render more correctly than they did before. It'd be nice for them to not black out or act weird in EFB2Tex, but that's a far, far, far less severe issue than we have now.

@JMC47

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2019

Blacked out player colors are because of the fifolog.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.