-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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
Fix s2 and s3 Cache_Count_Flash_Pages rom function wrapper (IDFGH-14492) #15261
Fix s2 and s3 Cache_Count_Flash_Pages rom function wrapper (IDFGH-14492) #15261
Conversation
The rom function on the s2 and s3 only counts one page for any pages which are mapped to page 0 of flash as the Cache_Flash_To_SPIRAM_Copy function attempts to map all flash page 0 mapped pages to one PSRAM page. As this function can be called for multiple regions, it needs to track if a page mapped to page 0 has previously been accounted for by a previous call. It does this using the page0_mapped in-out parameter. This logic contains an error: ``` if (*page0_mapped == 0) { // BUG: If page0_count is 0, 1 is still added count = valid_flash_count + 1 - page0_count; } else { count = valid_flash_count - page0_count; } *page0_mapped += page0_count; return count; ``` The current Cache_Count_Flash_Pages wrapper in the idf attempts to compensate for this bug by checking if the page0_mapped parameter was changed by a call to the function and reducing the count if it has not. This, however, will incorrectly over-compensate in situations where the initial value of page0_mapped was not zero as the code above only miscounts when it was zero. This patch addresses the issue in this wrapper function by correctly compensating for the bug only in cases where the final page0_mapped value is 0.
👋 Hello EliteTK, we appreciate your contribution to this project! 📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project. Click to see more instructions ...
Review and merge process you can expect ...
|
Description
The
Cache_Count_Flash_Pages
rom function on the s2 and s3 only counts one page for any pages which are mapped to page 0 of flash as theCache_Flash_To_SPIRAM_Copy
function attempts to map all flash page 0 mapped pages to one PSRAM page.As this function can be called for multiple regions, it needs to track if a page mapped to page 0 has previously been accounted for by a previous call. It does this using the
page0_mapped
in-out parameter. This logic contains an error[0]:The current
Cache_Count_Flash_Pages
wrapper in the idf attempts to compensate for this bug by checking if thepage0_mapped
parameter was changed by a call to the function and reducing the count if it has not.This, however, will incorrectly over-compensate in situations where the initial value of
page0_mapped
was not zero as the code above only miscounts when it was zero.This patch addresses the issue in this wrapper function by correctly compensating for the bug only in cases where the final
page0_mapped
value is 0.Testing
I used a ESP32-S3 based LCD board to run code which made use of
CONFIG_SPIRAM_FETCH_INSTRUCTIONS
andCONFIG_SPIRAM_RODATA
, I added some additional verbose logging inmmu_config_psram_text_segment
andmmu_config_psram_rodata_segment
to check the calculated page count and compare it to the change inpage_id
after the SPIRAM copy is performed.Since my code did not result in page0 getting mapped (I'm not sure how to arrange for that organically), I manually triggered the issue by initialising
page0_mapped
to 1 which resulted in the original code under-counting the total number of required pages by 1 in each instance of the function being called.With the fix applied, the count matched the jump in page number that subsequent calls to
Cache_Flash_To_SPIRAM_Copy
resulted in. This was tested both in cases which did and cases which did not trigger the bug.Checklist
Before submitting a Pull Request, please ensure the following:
NOTE: This change would only break code which would already break due to a lack of PSRAM space.