[PATCH v5 00/13] Add video damage tracking

[Alper Nebi Yasak]

Hi all,

On 2023-08-29 09:27 +03:00, Alexander Graf wrote:
> On 29.08.23 00:08, Simon Glass wrote:
>> On Mon, 28 Aug 2023 at 14:24, Alexander Graf <agraf at csgraf.de> wrote:
>>> On 28.08.23 19:54, Simon Glass wrote:
>>>> U-Boot does have video_sync() but it doesn't know when to call it. If
>>>> it does not call it, then any amount of single-threaded code can run
>>>> after that, which may update the framebuffer. In other words, U-Boot
>>>> is in exactly the same boat as UEFI. It has to decide whether to call
>>>> video_sync() based on some sort of heuristic.
>>>>
>>>> That is the only point I am trying to make here. Does that make sense?
>>>
>>> Oh, I thought you mentioned above that U-Boot is in a better spot or
>>> "has it solved already". I agree - it's in the same boat and the only
>>> safe thing it can really do today that is fully cross-platform
>>> compatible is to call video_sync() after every character.
>>>
>>> I don't understand what you mean by "any amount of single-threaded code
>>> can run after that, which may update the framebuffer". Any framebuffer
>>> modification is U-Boot internal code which then again can apply
>>> video_sync() to tell the system "I want what I wrote to screen actually
>>> be on screen now". I don't think that's necessarily bad design. A bit
>>> clunky, but we're in a pre-boot environment after all.
>>>
>>> Since we're aligned now: What exactly did you refer to with "but we have
>>> managed to work out something"?
>>>> So now we are on the same page about that point. The next step is my
>> heuristic point:
>>
>> vidconsole_putc_xy() - does not call video_sync()
>> vidconsole_newline() - does
>>
>> I am simply suggesting that UEFI should do the same thing.
> 
> 
> I think the better analogy is with
> 
> vidconsole_puts() - does
> 
> and that's exactly the function that the UEFI code calls. The UEFI 
> interface is "write this long string to screen". All the UEFI code does 
> is call vidconsole_puts() to do that which then (rightfully) calls 
> video_sync().
> 
> The reason we flush after every character with grub is grub: Grub abuses 
> the "write long string to screen" function and instead only writes a 
> single character on each call, which then means we flush on every 
> character write.

There's another reason I discovered empirically as I tried to implement
cyclic video_sync()s instead of calling it whenever. The writes will go
through eventually (as the cache is filled by other work?) even if *we*
don't explicitly flush it. That means partial data gets pushed to the
display in an uncontrolled way, resulting in bad graphical artifacts.

And I mean very noticeable things like a blocky waterfall effect when
filling a blue rectangle background in GRUB menu, or half-rendered
letters in U-Boot console (very obvious if I get it to panic-and-hang).

I think that locks it down, and there's two reasonable things we can do:

- Write and immediately flush to fb (hardware) every time
- Batch writes in fb, periodically write-and-flush to copy_fb (hardware)

Both can utilize a damage tracking feature to minimize the amount of
copy/flush done. And maybe we can implement the two simultaneously if we
skip flushing when damaged region is zero-sized already-flushed.

There's a flaw with the second approach though, EFI can have direct
access to the hardware copy_fb. When it has directly written to the
framebuffer, we would need to at least stop overwriting that, and
ideally copy backwards to the non-hardware fb. Is there some kind of a
locking API that EFI apps use to get/release the framebuffer? We could
hook that into something like that.

Note that I've been using "flush" and not "sync" to avoid talking about
when a driver ops->video_sync() should be called. Is that fundamentally
incompatible with EFI, can we even call that after e.g. ExitBootServices
where the OS wants to use it as efifb? Maybe we should always call that
periodically at 60Hz (16666us) or so?

(I'm testing on rk3399-gru-kevin with a 2400x1600 eDP screen by the way,
and attaching some WIP patches if you want to test. Debian arm64 netinst
iso uses text-mode GRUB by default, in case you need a payload to test.)

>>>>>> Also I notice that EFI calls notify? all the time, so U-Boot probably
>>>>>> does have the ability to sync the video every 10ms if we wanted to.

BTW, with attached cyclic patch on chromebook_kevin, I immediately get a
warning that it takes too long, at 2-8ms without/with VIDEO_COPY. It's
about 11ms for both on sandbox on my PC.

>>>>> I fail to see how invalidating the frame buffer for the screen every
>>>>> 10ms is any better than doing flush+invalidate operations only on screen
>>>>> areas that changed? It's more fragile, more difficult to understand and
>>>>> also significantly more expensive given that most of the time very
>>>>> little on the screen actually changes.
>>>>>>>> I am not suggesting it is better, at all. I am just trying to explain
>>>> that the U-Boot EFI implementation should not be calling video_sync()
>>>> after every character, before or after this series.
>>>
>>> Ah, it doesn't :). It just calls the normal U-Boot "write character on
>>> screen" function. What that does is up to U-Boot internals - and those
>>> basically today dictate that something needs to call video_sync() to
>>> make FB modifications actually pop up on screen.
>>>
>> Hmmm, so what function is it calling, then?  I think we are getting
>> closer to the 'fix' I am trying to tease out.
> 
> It literally calls vidconsole_puts():
> 
> https://github.com/u-boot/u-boot/blob/master/lib/efi_loader/efi_console.c#L185

I'd think "sync after a whole string is printed" is an OK heuristic, and
GRUB is abusing it... But maybe GRUB is doing these things as an ad-hoc
double buffering implementation with forced syncs at the expense of
performance to avoid buggy firmware causing graphical artifacts.

In any case, apologies but I'll be unable to work on U-Boot things until
late September, may also be unable to respond. (Going to DebConf soon)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-video-Replace-all-video_sync-calls-with-a-cyclic.patch
Type: text/x-patch
Size: 7338 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20230830/e9e4eea9/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-video-rockchip-Implement-copy-framebuffer-suppor.patch
Type: text/x-patch
Size: 2107 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20230830/e9e4eea9/attachment-0001.bin>