|Re: [hatari-devel] defaulting to SMALL_MEM|
[ Thread Index |
| More lists.tuxfamily.org/hatari-devel Archives
Le 16/02/2022 à 22:02, Eero Tamminen a écrit :
On 15.2.2022 21.01, Thomas Huth wrote:
Am Sun, 13 Feb 2022 19:10:44 +0200
schrieb Eero Tamminen <oak@xxxxxxxxxxxxxx>:
I.e. if screen pointer would be in such RAM area, should the other
video related counters still change or not?
Well, likely yes, but in this case I wonder if that'd buy us much. It
certainly would make the current ST-low rendering code (which is
already quite complex) way more complicated (and likely slower) if we
add checks there all over the place. And for what? As long as we don't
know of any program that relies on such weird behavior, it does not
have any benefit for us, does it?
For the mono function it's trivial, but I hadn't realized how much
things would need to be changed or the color one before actually looking
Attached is patch handling all the memcpy() cases,
which is already quite large. However, that is missing all the
additional word accesses through pVideoRasterEndLine pointer, used for
STE special cases, which would be even trickier to handle.
=> I concur that it's not really workable for Video_CopyScreenLineColor().
(That function really is one scary monster. Would be nice if somebody
were able to split it into more manageable smaller functions.)
I agree this function is rather complicated in the "color" case due to
all the possible overscan tricks to handle. I'm planning to rewrite it
differently one day to handle cases that are even not supported today
(such as mixing low res and med res on the same line).
In the meantime, I'm not sure it would be wise to apply your patch, it
makes it even harder to read the code.
Regarding this possibility of reading after end of ram when converting
screen pixels, maybe a simpler solution could be to add a "security
margin" buffer after end of STRam in case we compile with SMALL_MEM ?
That is, instead of allocating STRamEnd bytes for STRam, we allocate
STRamEnd + SCREENBYTES_LINE*10 bytes and fill this extra space of
SCREENBYTES_LINE*10 bytes with 0.
This way we can remove the tests like "> &STRam[STRamEnd]" + memset to 0
when above the limit and happily copy whatever pVideoRaster points too.
I agree this may not be as clean as really checking all accesses are not
above the limit, but in term of optimisation it would be much more
efficient, considering the Video_CopyScreenLineXXX functions are called
rather often ; as long as this extra security margin is documented when
allocating RAM at start and in every Video_CopyScreenLineXXX functions,
this would look like a good trade off to me.