[PATCH v5 00/13] Add video damage tracking

[Alexander Graf]

On 30.08.23 20:27, Alper Nebi Yasak wrote:
> 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.


Edk2 also has a shadow frame buffer that it uses for VGA because 
otherwise the NC read accesses from VRAM would be too expensive. I don't 
believe there's any UEFI locking mechanism, but clear understanding that 
if you want to access the frame buffer while anything else but you could 
potentially access it too, you better use the UEFI APIs instead of 
scribbling on it yourself.


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


If you actually need to do something actively frequently, then that 
won't work anymore after ExitBootServices. At that point, all "normal" 
U-Boot code is gone.


> (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 would expect explicit damage flushes like the patch set originally 
does to be a lot more performant than anything dumber, but timer based.


Alex