Skip to content

Rewrite cache management functions #758

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

Ziemas
Copy link
Contributor

@Ziemas Ziemas commented May 1, 2025

First of all, I totally understand if people think it's best to leave this well enough alone.

The previous implementation looped over and read both ways of every cache line, doing a manual comparison of the address and then doing index invalidate on match.

This replaces that with a simple loop over each cache line using the Hit invalidate cache ops, which will tell the hardware to check for a cache hit and invalidate.

Doing things this way should mean it's safe to do without disabling interrupts, which is how sceSifWriteBackDCache worked. Because of that I've made it use the same shared implementation, this does miss some loop unrolling from that function because I don't imagine it matters much?

@Ziemas Ziemas force-pushed the cache branch 3 times, most recently from f500950 to c3f3cfd Compare May 2, 2025 15:37
@rickgaiser
Copy link
Member

Nice improvement in code readability!
As for the functional change, I think you're right, but I've also been experimenting with this stuff a while back and it's very tricky to get right, and easy to get wrong.

I see you've made a few changes after making the PR. Take your time and let me know when it's ready to merge.

Ziemas added 4 commits May 9, 2025 16:43
The previous implementation looped over and read both ways of every
cache line, doing a manual comparison of the address and then doing
index invalidate on match.

This replaces that with a simple loop over each cache line using the Hit
invalidate cache ops, which will tell the hardware to check for a cache
hit and invalidate.
sceSifWriteBackDCache used Hit writeback invalidate ops, this makes it a
good match to use SyncDCache now that it also does.

This misses some loop unrolling that was there for large invalidations,
not sure that particularly matters.
This should be safe now that we use Hit invalidate cache ops.
sceSifWriteBackDCache left them and has been fine.
SyncDCache/InvalidDCache are annoying to use because of their provided
range being inclusive at both ends, it's too easy to accidentally affect
an extra cache line at the end.

This provides a new cache API and implements the older ones using it.
@Ziemas
Copy link
Contributor Author

Ziemas commented May 9, 2025

It kept bugging me how easy it is to accidentally affect one more line than you intended with SyncDCache/InvalidDCache, since the range end is inclusive SyncDCache(0, 64) is syncing two cache lines and I see most code using it just add a size without subtracting 1. That's not so bad for writeback, but could be bad for invalidate.

So I added a cache.h with inline cache management functions that make sense to me.

@uyjulian
Copy link
Member

uyjulian commented May 9, 2025

Looks good

@Ziemas Ziemas changed the title [RFC] Rewrite cache management functions Rewrite cache management functions May 10, 2025
@sp193
Copy link
Member

sp193 commented May 10, 2025

The existing cache functionality is designed and documented by Sony and so the limitation of writing back in units of 64 is known. If you have nice macros for improving its behaviour, I suppose that would be a nice addition.

Personally, I used to make use the uncached or even uncached+accelerated memory region where its characteristics fit better better than writing back/invalidating the data cache lines, since there is no need to write in blocks of 64. However, we are cautioned against mixing access modes within a cache line.

I am retired, but I hope to share my 2-cents. But feel free to carry on if we can manage this.
I feel that it is possible that undocumented/poorly-understood hardware issues/behaviour to occur if we deviate from the original design. For this reason, I used to work towards eliminating code that worked differently from the Sony original.

@Ziemas
Copy link
Contributor Author

Ziemas commented May 10, 2025

The existing cache functionality is designed and documented by Sony and so the limitation of writing back in units of 64 is known. If you have nice macros for improving its behaviour, I suppose that would be a nice addition.

My main problem is that I see it misused all over PS2SDK when it does something like SyncDCache(ptr, ptr+size) where if the size is a multiple of the line size it'll write back an unrelated line at the end.

Personally, I used to make use the uncached or even uncached+accelerated memory region where its characteristics fit better better than writing back/invalidating the data cache lines, since there is no need to write in blocks of 64. However, we are cautioned against mixing access modes within a cache line.

I agree, using UCAB memory for writing DMA lists is great.

I am retired, but I hope to share my 2-cents. But feel free to carry on if we can manage this. I feel that it is possible that undocumented/poorly-understood hardware issues/behaviour to occur if we deviate from the original design. For this reason, I used to work towards eliminating code that worked differently from the Sony original.

In this case sceSifWriteBackDCache() was already using the hit writeback invalidate function that i use here.

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.

4 participants