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 <ag...@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)
From 5144e9479c6b31cac33e98b5ae00a6d103f19462 Mon Sep 17 00:00:00 2001
From: Alper Nebi Yasak <alpernebiya...@gmail.com>
Date: Tue, 22 Aug 2023 18:05:29 +0300
Subject: [PATCH] WIP: video: Replace all video_sync calls with a cyclic
function
---
boot/expo.c | 2 --
cmd/video.c | 2 --
drivers/serial/sandbox.c | 2 --
drivers/video/Kconfig | 1 +
drivers/video/vidconsole-uclass.c | 35 +++----------------------------
drivers/video/video-uclass.c | 28 ++++++++++++++++++++++++-
drivers/video/video_bmp.c | 2 +-
include/video.h | 2 ++
lib/efi_loader/efi_gop.c | 2 --
9 files changed, 34 insertions(+), 42 deletions(-)
diff --git a/boot/expo.c b/boot/expo.c
index 139d684f8e6e..03e858c03c80 100644
--- a/boot/expo.c
+++ b/boot/expo.c
@@ -208,8 +208,6 @@ int expo_render(struct expo *exp)
return log_msg_ret("ren", ret);
}
- video_sync(dev, true);
-
return scn ? 0 : -ECHILD;
}
diff --git a/cmd/video.c b/cmd/video.c
index 942f81c16336..98de2c9f1b8d 100644
--- a/cmd/video.c
+++ b/cmd/video.c
@@ -42,8 +42,6 @@ static int do_video_puts(struct cmd_tbl *cmdtp, int flag, int argc,
if (uclass_first_device_err(UCLASS_VIDEO_CONSOLE, &dev))
return CMD_RET_FAILURE;
ret = vidconsole_put_string(dev, argv[1]);
- if (!ret)
- ret = video_sync(dev->parent, false);
return ret ? CMD_RET_FAILURE : 0;
}
diff --git a/drivers/serial/sandbox.c b/drivers/serial/sandbox.c
index f4003811ee75..586e1154a327 100644
--- a/drivers/serial/sandbox.c
+++ b/drivers/serial/sandbox.c
@@ -139,8 +139,6 @@ static int sandbox_serial_pending(struct udevice *dev, bool input)
return 0;
os_usleep(100);
- if (IS_ENABLED(CONFIG_VIDEO) && !IS_ENABLED(CONFIG_SPL_BUILD))
- video_sync_all();
avail = membuff_putraw(&priv->buf, 100, false, &data);
if (!avail)
return 1; /* buffer full */
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 09f2cb1a7321..530485102c78 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -7,6 +7,7 @@ menu "Graphics support"
config VIDEO
bool "Enable driver model support for LCD/video"
depends on DM
+ select CYCLIC
help
This enables driver model for LCD and video devices. These support
a bitmap display of various sizes and depths which can be drawn on
diff --git a/drivers/video/vidconsole-uclass.c b/drivers/video/vidconsole-uclass.c
index b5b3b6625902..876e80f9ebe5 100644
--- a/drivers/video/vidconsole-uclass.c
+++ b/drivers/video/vidconsole-uclass.c
@@ -77,7 +77,7 @@ static int vidconsole_back(struct udevice *dev)
if (priv->ycur < 0)
priv->ycur = 0;
}
- return video_sync(dev->parent, false);
+ return 0;
}
/* Move to a newline, scrolling the display if necessary */
@@ -87,7 +87,7 @@ static void vidconsole_newline(struct udevice *dev)
struct udevice *vid_dev = dev->parent;
struct video_priv *vid_priv = dev_get_uclass_priv(vid_dev);
const int rows = CONFIG_VAL(CONSOLE_SCROLL_LINES);
- int i, ret;
+ int i;
priv->xcur_frac = priv->xstart_frac;
priv->ycur += priv->y_charsize;
@@ -101,13 +101,6 @@ static void vidconsole_newline(struct udevice *dev)
priv->ycur -= rows * priv->y_charsize;
}
priv->last_ch = 0;
-
- ret = video_sync(dev->parent, false);
- if (ret) {
-#ifdef DEBUG
- console_puts_select_stderr(true, "[vc err: video_sync]");
-#endif
- }
}
static char *parsenum(char *s, int *num)
@@ -298,15 +291,7 @@ static void vidconsole_escape_char(struct udevice *dev, char ch)
parsenum(priv->escape_buf + 1, &mode);
if (mode == 2) {
- int ret;
-
video_clear(dev->parent);
- ret = video_sync(dev->parent, false);
- if (ret) {
-#ifdef DEBUG
- console_puts_select_stderr(true, "[vc err: video_sync]");
-#endif
- }
priv->ycur = 0;
priv->xcur_frac = priv->xstart_frac;
} else {
@@ -520,12 +505,6 @@ static void vidconsole_putc(struct stdio_dev *sdev, const char ch)
if (ret) {
#ifdef DEBUG
console_puts_select_stderr(true, "[vc err: putc]");
-#endif
- }
- ret = video_sync(dev->parent, false);
- if (ret) {
-#ifdef DEBUG
- console_puts_select_stderr(true, "[vc err: video_sync]");
#endif
}
}
@@ -542,12 +521,6 @@ static void vidconsole_puts(struct stdio_dev *sdev, const char *s)
snprintf(str, sizeof(str), "[vc err: puts %d]", ret);
console_puts_select_stderr(true, str);
-#endif
- }
- ret = video_sync(dev->parent, false);
- if (ret) {
-#ifdef DEBUG
- console_puts_select_stderr(true, "[vc err: video_sync]");
#endif
}
}
@@ -685,9 +658,7 @@ UCLASS_DRIVER(vidconsole) = {
#ifdef CONFIG_VIDEO_COPY
int vidconsole_sync_copy(struct udevice *dev, void *from, void *to)
{
- struct udevice *vid = dev_get_parent(dev);
-
- return video_sync_copy(vid, from, to);
+ return 0;
}
int vidconsole_memmove(struct udevice *dev, void *dst, const void *src,
diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
index f743ed74c818..099ba5acaa9b 100644
--- a/drivers/video/video-uclass.c
+++ b/drivers/video/video-uclass.c
@@ -6,6 +6,7 @@
#define LOG_CATEGORY UCLASS_VIDEO
#include <common.h>
+#include <cyclic.h>
#include <bloblist.h>
#include <console.h>
#include <cpu_func.h>
@@ -249,7 +250,7 @@ int video_fill(struct udevice *dev, u32 colour)
if (ret)
return ret;
- return video_sync(dev, false);
+ return 0;
}
int video_clear(struct udevice *dev)
@@ -351,6 +352,24 @@ void video_set_default_colors(struct udevice *dev, bool invert)
priv->colour_bg = video_index_to_colour(priv, back);
}
+static void video_cyclic(void *ctx)
+{
+ struct udevice *dev = ctx;
+ int ret;
+
+ if (!device_active(dev))
+ return;
+
+ video_sync_copy_all(dev);
+
+ ret = video_sync(dev, true);
+ if (ret) {
+#ifdef DEBUG
+ console_puts_select_stderr(true, "[vc err: video_sync]");
+#endif
+ }
+}
+
/* Flush video activity to the caches */
int video_sync(struct udevice *vid, bool force)
{
@@ -603,6 +622,13 @@ static int video_post_probe(struct udevice *dev)
}
}
+ /* Register video sync as a cyclic function at 60Hz */
+ priv->cyclic = cyclic_register(video_cyclic, 16666, dev->name, dev);
+ if (!priv->cyclic) {
+ log_err("cyclic_register for %s video sync failed\n", dev->name);
+ return -ENODEV;
+ }
+
return 0;
};
diff --git a/drivers/video/video_bmp.c b/drivers/video/video_bmp.c
index 45f003c8251a..90a0e4e98b11 100644
--- a/drivers/video/video_bmp.c
+++ b/drivers/video/video_bmp.c
@@ -466,5 +466,5 @@ int video_bmp_display(struct udevice *dev, ulong bmp_image, int x, int y,
if (ret)
return log_ret(ret);
- return video_sync(dev, false);
+ return 0;
}
diff --git a/include/video.h b/include/video.h
index 66d109ca5da6..31aa8df171fa 100644
--- a/include/video.h
+++ b/include/video.h
@@ -8,6 +8,7 @@
#define _VIDEO_H_
#include <stdio_dev.h>
+#include <cyclic.h>
struct udevice;
@@ -118,6 +119,7 @@ struct video_priv {
bool flush_dcache;
u8 fg_col_idx;
u8 bg_col_idx;
+ struct cyclic_info *cyclic;
};
/**
diff --git a/lib/efi_loader/efi_gop.c b/lib/efi_loader/efi_gop.c
index 778b693f983a..56ca0482d871 100644
--- a/lib/efi_loader/efi_gop.c
+++ b/lib/efi_loader/efi_gop.c
@@ -451,8 +451,6 @@ static efi_status_t EFIAPI gop_blt(struct efi_gop *this,
if (ret != EFI_SUCCESS)
return EFI_EXIT(ret);
- video_sync_all();
-
return EFI_EXIT(EFI_SUCCESS);
}
--
2.40.1
From 87771cf765e6cb1096d6487e4dcc3c6972df0c8e Mon Sep 17 00:00:00 2001
From: Alper Nebi Yasak <alpernebiya...@gmail.com>
Date: Fri, 25 Aug 2023 21:29:52 +0300
Subject: [PATCH] WIP: video: rockchip: Implement copy framebuffer support
---
configs/chromebook_kevin_defconfig | 3 +--
drivers/video/rockchip/rk_vop.c | 11 ++++++++++-
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/configs/chromebook_kevin_defconfig b/configs/chromebook_kevin_defconfig
index 6ce69f419e17..0baa73cc62db 100644
--- a/configs/chromebook_kevin_defconfig
+++ b/configs/chromebook_kevin_defconfig
@@ -5,8 +5,6 @@ CONFIG_ARCH_ROCKCHIP=y
CONFIG_TEXT_BASE=0x00200000
CONFIG_SPL_GPIO=y
CONFIG_NR_DRAM_BANKS=1
-CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
-CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x300000
CONFIG_SF_DEFAULT_SPEED=20000000
CONFIG_ENV_SIZE=0x8000
CONFIG_DEFAULT_DEVICE_TREE="rk3399-gru-kevin"
@@ -129,6 +127,7 @@ CONFIG_USB_ETHER_SMSC95XX=y
CONFIG_VIDEO=y
# CONFIG_VIDEO_FONT_8X16 is not set
CONFIG_VIDEO_FONT_16X32=y
+CONFIG_VIDEO_COPY=y
CONFIG_DISPLAY=y
CONFIG_VIDEO_ROCKCHIP=y
CONFIG_VIDEO_ROCKCHIP_MAX_XRES=2400
diff --git a/drivers/video/rockchip/rk_vop.c b/drivers/video/rockchip/rk_vop.c
index c514e2a0e449..e70689851a61 100644
--- a/drivers/video/rockchip/rk_vop.c
+++ b/drivers/video/rockchip/rk_vop.c
@@ -464,10 +464,16 @@ int rk_vop_probe(struct udevice *dev)
return -EINVAL;
}
+ if (IS_ENABLED(CONFIG_VIDEO_COPY))
+ plat->copy_base = plat->base + plat->size / 2;
+
for (node = ofnode_first_subnode(port);
ofnode_valid(node);
node = dev_read_next_subnode(node)) {
- ret = rk_display_init(dev, plat->base, node);
+ if (IS_ENABLED(CONFIG_VIDEO_COPY))
+ ret = rk_display_init(dev, plat->copy_base, node);
+ else
+ ret = rk_display_init(dev, plat->base, node);
if (ret)
debug("Device failed: ret=%d\n", ret);
if (!ret)
@@ -485,5 +491,8 @@ int rk_vop_bind(struct udevice *dev)
plat->size = 4 * (CONFIG_VIDEO_ROCKCHIP_MAX_XRES *
CONFIG_VIDEO_ROCKCHIP_MAX_YRES);
+ if (IS_ENABLED(CONFIG_VIDEO_COPY))
+ plat->size *= 2;
+
return 0;
}
--
2.40.1
