I was scanning through the The-Ultimate-MiSTer2MEGA65-Porting-Guide to check if there were simple things I had omitted for my MegaPET.
It recommends to use avm_cache together with qnice2hyperram, to reduce latency. I had tried some other stuff in this area already (with some mysterious failures that seemed to happen rarely and randomly), so I wanted to try it.
My use case is where one disk image is stored in the low byte of each hyperram word, and the other disk image is stored in the high byte. So for each request there is just a single bit enabled in byteenable.
From this experiment I have the following findings.
-
A read request from the qnice side doesn't appear to set m_avm_byteenable_o. Depending on use case it would probably need to be either 11 or s_avm_byteenable_i. I chose for the latter (because it is the same in all requests, for each cache). Maybe this is a candidate for a generic parametrisation.
-
The read-ahead logic at the comment "Half the cache has now been read." is not triggered for my use case. It works if you either sequentially read words with byteenable = 11, or sequentially read bytes with byteenable = 01 first and 10 next. I chose to remove this part of the condition, but there could be another generic parametrisation for this.
-
The upstream avm_fifo apparently needs to be strictly larger than the cache size (which means twice as large). It failed for me when I set the sizes the same: the last word transfer of a burst of G_CACHE_SIZE never completed. This is a trap that could be mentioned. I fell into the trap since I used a cache size larger than in the example cases.
-
When qnice reads the word that triggers the read-ahead, that word is still a cache hit and as such s_avm_readdatavalid_o is set. However at the same time, s_avm_waitrequest_o is also set since a new request can't be accepted at this time. The downstream qnice2hyperram interprets this situation as "I have to wait for completion of my request", unfortunately. This leads to sub-optimal behaviour (but the cache is robust enough that it is correct!). So qnice didn't accept the data and kept the request active. Meanwhile the cache was fetching ahead and dropping the word from its cache. When the read-ahead finished, it was robust enough to see that it was now a cache miss, so it went and fetched 8 words from the address which was still being requested. 3 read requests later the story repeats.
Apparently this is a bug in qnice2hyperram (the combination of readdatavalid and waitrequest is legitimate). It contains s_qnice_wait_o <= ((m_avm_write_o or m_avm_read_o) and m_avm_waitrequest_i) or reading which probably already makes sure that if both are set, the waitrequest essentially wins. It seems likely that this needs to be s_qnice_wait_o <= ((m_avm_write_o or m_avm_read_o) and m_avm_waitrequest_i and m_avm_readdatavalid = '0') or reading;
However, since I already had made some changes in the avm_cache, I put the change there. In the complicated expression that determines s_avm_waitrequest_o, I replaced '0' when cache_rd_hit_s = '1' and s_avm_write_i = '0' and state = READING_ST with the simpler '0' when s_avm_readdatavalid_o = '1'. The original condition represents one case where readdatavalid gets set, I generalised it to all cases, which might have been the original intention anyway.
avm_cache_rhi.vhd.txt
Discussion (mostly monologue) on the discord: https://discord.com/channels/719326990221574164/1411396798572003388/1515343023087685773
I was scanning through the The-Ultimate-MiSTer2MEGA65-Porting-Guide to check if there were simple things I had omitted for my MegaPET.
It recommends to use
avm_cachetogether withqnice2hyperram, to reduce latency. I had tried some other stuff in this area already (with some mysterious failures that seemed to happen rarely and randomly), so I wanted to try it.My use case is where one disk image is stored in the low byte of each hyperram word, and the other disk image is stored in the high byte. So for each request there is just a single bit enabled in
byteenable.From this experiment I have the following findings.
A read request from the qnice side doesn't appear to set
m_avm_byteenable_o. Depending on use case it would probably need to be either11ors_avm_byteenable_i. I chose for the latter (because it is the same in all requests, for each cache). Maybe this is a candidate for a generic parametrisation.The read-ahead logic at the comment "Half the cache has now been read." is not triggered for my use case. It works if you either sequentially read words with byteenable =
11, or sequentially read bytes with byteenable =01first and10next. I chose to remove this part of the condition, but there could be another generic parametrisation for this.The upstream avm_fifo apparently needs to be strictly larger than the cache size (which means twice as large). It failed for me when I set the sizes the same: the last word transfer of a burst of G_CACHE_SIZE never completed. This is a trap that could be mentioned. I fell into the trap since I used a cache size larger than in the example cases.
When qnice reads the word that triggers the read-ahead, that word is still a cache hit and as such
s_avm_readdatavalid_ois set. However at the same time,s_avm_waitrequest_ois also set since a new request can't be accepted at this time. The downstreamqnice2hyperraminterprets this situation as "I have to wait for completion of my request", unfortunately. This leads to sub-optimal behaviour (but the cache is robust enough that it is correct!). So qnice didn't accept the data and kept the request active. Meanwhile the cache was fetching ahead and dropping the word from its cache. When the read-ahead finished, it was robust enough to see that it was now a cache miss, so it went and fetched 8 words from the address which was still being requested. 3 read requests later the story repeats.Apparently this is a bug in qnice2hyperram (the combination of readdatavalid and waitrequest is legitimate). It contains
s_qnice_wait_o <= ((m_avm_write_o or m_avm_read_o) and m_avm_waitrequest_i) or readingwhich probably already makes sure that if both are set, the waitrequest essentially wins. It seems likely that this needs to bes_qnice_wait_o <= ((m_avm_write_o or m_avm_read_o) and m_avm_waitrequest_i and m_avm_readdatavalid = '0') or reading;However, since I already had made some changes in the
avm_cache, I put the change there. In the complicated expression that determiness_avm_waitrequest_o, I replaced'0' when cache_rd_hit_s = '1' and s_avm_write_i = '0' and state = READING_STwith the simpler'0' when s_avm_readdatavalid_o = '1'. The original condition represents one case where readdatavalid gets set, I generalised it to all cases, which might have been the original intention anyway.avm_cache_rhi.vhd.txt
Discussion (mostly monologue) on the discord: https://discord.com/channels/719326990221574164/1411396798572003388/1515343023087685773