Re: [hatari-devel] defaulting to SMALL_MEM

[Thomas Huth <th.huth@xxxxxxxxx>]

Am Thu, 17 Feb 2022 11:14:25 +0100
schrieb Nicolas Pomarède <npomarede@xxxxxxxxxxxx>:

> Le 16/02/2022 à 22:02, Eero Tamminen a écrit :
> > Hi,
> > 
> > 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 into it.
> > 
> > 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.) 
> 
> Hi
> 
> 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.

I agree, this is getting very ugly that way.

> 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.

After thinking about this for a while, I agree, it's like the best
solution to add some margin to the allocation for this instead, to
avoid that the code gets too ugly and to avoid regressions with the
performance of the rendering functions.

But shouldn't we rather allocate NUM_VISIBLE_LINE_PIXELS *
NUM_VISIBLE_LINES / 2 bytes instead? ... in case a program sets the
memory base to the very end of the RAM and then does some fullscreen
tricks?

Anyway, we should add a proper comment to the allocation, otherwise
we'll wonder in a couple of years why we did that change ;-)

 Thomas