[Bug 228755] libvgl under syscons causes system reboot (via SDL 1.2)

[Bruce Evans]

On Tue, 5 Jun 2018, Mahmoud Al-Qudsi wrote:

> On Tue, Jun 5, 2018 at 7:32 AM, Bruce Evans <brde at optusnet.com.au> wrote:
>> This is the driver getting the check wrong for all frame buffer sizes that
>> are not a multiple of PAGE_SIZE.  The frame buffer size is not a multiple
>> of PAGE SIZE mainly for the interesting geometries 800x600x8 and
>> 1920x1080x8.  I use this fix:
>>
>> Index: vesa.c
>> ===================================================================
>> --- vesa.c      (revision 334595)
>> +++ vesa.c      (working copy)
>> @@ -1642,7 +1642,7 @@
>>             (adp->va_info.vi_flags & V_INFO_LINEAR) != 0) {
>>                 /* va_window_size == va_buffer_size/vi_planes */
>>                 /* XXX: is this correct? */
>> -               if (offset > adp->va_window_size - PAGE_SIZE)
>> +               if (offset > trunc_page(adp->va_window_size))
>>                         return (-1);
>>                 *paddr = adp->va_info.vi_buffer + offset;
>>  #ifdef VM_MEMATTR_WRITE_COMBINING
>>
>> After fixing this, even 1920x1080x8 works.
>
> I submitted a (different) patch [0] that we've been using in production since
> 2014 for that particular code multiple times, most recently in January of 2018
> [1]. It's not a libvgl-specific issue and honestly deserves to be fixed
> already.  With it, we've been able to use sc as a pretty much bulletproof X
> target, with the caveat that it's insanely slow at higher (native) resolutions.

I will check that.

The slowness is not too bad here.  On Haswell 4GHz with CPU graphics,
in 1920x1080x8 mode, the 3*1000 random operations at the end of libvgl's
demo take 1 second.  At 800MHz, this takes 1.8 seconds.  This shows
that the it is the video bandwidth that is the main limit.  However,
AthlonXP 2GHz with old Radeon is insanely slow.  It takes 43 seconds
in 1280x1024x8 mode.  In fact, vgl is slow in all modes are slow on this
system.  However, this system isn't much slower in my video bandwidth
tests, and it is fast enough under old X with an old radeon driver, even
for an application a bit like vgl that sometimes draws single pixels.

> This particular issue is actually already tracked in kern/162373, the same
> patch was submitted there by Mikhail Kupchik way back in November of 2011.
>
> Since you seem to know what you're doing - is the comparison with
> `trunc_page(adp->va_window_size)` here the way to go or the alternative
> approach with `offset >= adp->va_info.vi_buffer_size` range checking instead?

I think the tests are equivalent under the assumptions that the caller has
rounded down 'offset' to a page boundary and that the hardware frame buffer
size is a multiple of the page size.  The trunc_page() test is clearer but
still cyptic.  The range being checked is from offset to offset + PAGE_SIZE -1
under the simplifying assumpting that offset has been rounded.  It is the
end of this range, not the start, that should be compared with the rounded-uo
buffer size.  But after rounding, '<' means at least 1 page smaller, not just
at least 1 byte smaller, so we can take the shortcut of replacing
offset + PAGE_SIZE - 1 by that less about PAGE_SIZE, but it is still not
obvious that this doesn't have an off-by-1-byte error.

> ...
> [0]: https://github.com/neosmart/freebsd/commit/b3af2d1ed4a63fe1a5d771ed3a052242250098d7
> [1]: http://freebsd.1045724.x6.nabble.com/PATCH-validated-against-50k-i686-machines-for-vmparam-h-and-vesa-c-td6235962.html

Bruce