[PATCH 0/5] sandbox: video: Refactor out of uclass, try partial screen updates
While working on the video damage tracking series [1], I noticed the 10Hz limitation on the sandbox screen refresh rate, and thought maybe applying the damage tracking idea to SDL would make it reasonable to increase the rate or even remove the limit completely.
Experimenting on that idea resulted in a bit of a refactoring to pull sandbox out of the video uclass, so I'm sending it as a series in case some of it could be useful.
As for the partial update idea itself, I found that having the delay around 1/refresh-rate appears to be the most visually smooth (about 7ms for 144Hz, 16ms for 60Hz etc.), but couldn't exactly evaluate CPU usage for which the original 10Hz rate limit was set. In any case, the visual appeal of sandbox doesn't really matter, so I don't exactly care.
Of course, this applies onto the video damage tracking series that I recently updated and sent [1], but only the "partial updates" patch is functionally dependent on it.
[1] https://lore.kernel.org/u-boot/20230821135111.3558478-1-alpernebiyasak@gmail...
Alper Nebi Yasak (5): sandbox: video: Display copy framebuffer if enabled video: Allow deferring and retrying driver-specific video sync sandbox: video: Move sync rate limit into SDL code sandbox: video: Move sandbox video sync to a driver operation RFC: sandbox: video: Use partial updates for SDL display
arch/sandbox/cpu/sdl.c | 34 ++++++++++++++++++++++++++-------- arch/sandbox/include/asm/sdl.h | 13 ++++++++++--- drivers/video/sandbox_sdl.c | 30 ++++++++++++++++++++++++++++++ drivers/video/video-uclass.c | 14 ++------------ include/video.h | 3 ++- test/dm/video.c | 2 +- 6 files changed, 71 insertions(+), 25 deletions(-)
base-commit: 3881c9fbb7fdd98f6eae5cd33f7e9abe9455a585 prerequisite-patch-id: aad206b8942d0e8654ba1b11f28104950baf1518 prerequisite-patch-id: ee3d21bb91062ebbcf0c3a75342d7fa37d5630ce prerequisite-patch-id: 7fcaec0bb8b5bbb5b3813bda896034c9b98b67b9 prerequisite-patch-id: aa4173ebedc3f6d0f04f72bf49bed642614f42a7 prerequisite-patch-id: 06676907c4d262880f7904fed33de11057e886f0 prerequisite-patch-id: 63bbfd7808bddea8b39120d50bb61ac1c4b3eeb8 prerequisite-patch-id: 8941f56b57f7ece319eed67ce8584e7769a36a3b prerequisite-patch-id: b89b375a3221ac1f89f3eb15fa20a2140ea5687c prerequisite-patch-id: 950508eaecbbc01560a2f24778786dd50e7c20e8 prerequisite-patch-id: c30b2a60881bc813e4d8a7e1a99fd99c368957d2 prerequisite-patch-id: 963770b5a43c750bac620788562ce549ac48dacd prerequisite-patch-id: 500af25e869687516da147ce7c819c6b32a9bc92 prerequisite-patch-id: b064cd6b7db9cd122f10c2ae2573328ac945b3be
When VIDEO_COPY is enabled, the "main" framebuffer is a cached work area in U-Boot allocated memory and the "copy" framebuffer is the hardware frame buffer displayed on the screen. The sandbox SDL video driver sets copy_base to indicate support for this, but it displays the work area instead. Change it to display the copy buffer if enabled.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com ---
drivers/video/video-uclass.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c index 3f9ddaadd15d..7cec6362570f 100644 --- a/drivers/video/video-uclass.c +++ b/drivers/video/video-uclass.c @@ -454,9 +454,13 @@ int video_sync(struct udevice *vid, bool force)
#if defined(CONFIG_VIDEO_SANDBOX_SDL) static ulong last_sync; + void *fb = priv->fb; + + if (IS_ENABLED(CONFIG_VIDEO_COPY)) + fb = priv->copy_fb;
if (force || get_timer(last_sync) > 100) { - sandbox_sdl_sync(priv->fb); + sandbox_sdl_sync(fb); last_sync = get_timer(0); } #endif
On Mon, 21 Aug 2023 at 12:13, Alper Nebi Yasak alpernebiyasak@gmail.com wrote:
When VIDEO_COPY is enabled, the "main" framebuffer is a cached work area in U-Boot allocated memory and the "copy" framebuffer is the hardware frame buffer displayed on the screen. The sandbox SDL video driver sets copy_base to indicate support for this, but it displays the work area instead. Change it to display the copy buffer if enabled.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com
drivers/video/video-uclass.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
The sandbox SDL driver has some code in the video uclass to rate limit video syncs by postponing them, and forcing a sync nonetheless with a "force" argument.
Add infrastructure for doing this through driver ops, where the driver can request to defer a sync with -EAGAIN, and the uclass can force it by calling the op again it until it does something.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com --- Alternatively, add "force" argument into the driver ops->video_sync(). But I think it should go away instead. The problem is video_sync() being called irregularly and too frequently, maybe we can call it only from cyclic at the hardware refresh rate?
drivers/video/video-uclass.c | 2 ++ include/video.h | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c index 7cec6362570f..accf486509cb 100644 --- a/drivers/video/video-uclass.c +++ b/drivers/video/video-uclass.c @@ -446,6 +446,8 @@ int video_sync(struct udevice *vid, bool force)
if (ops && ops->video_sync) { ret = ops->video_sync(vid); + while (force && ret == -EAGAIN) + ret = ops->video_sync(vid); if (ret) return ret; } diff --git a/include/video.h b/include/video.h index 42e57b44188d..5c4327d4e455 100644 --- a/include/video.h +++ b/include/video.h @@ -137,7 +137,8 @@ struct video_priv { * displays needs synchronization when data in an FB is available. * For these devices implement video_sync hook to call a sync * function. vid is pointer to video device udevice. Function - * should return 0 on success video_sync and error code otherwise + * should return 0 on success video_sync, -EAGAIN if it was + * deferred and should be tried again, and error code otherwise */ struct video_ops { int (*video_sync)(struct udevice *vid);
Hi Alper,
On Mon, 21 Aug 2023 at 12:13, Alper Nebi Yasak alpernebiyasak@gmail.com wrote:
The sandbox SDL driver has some code in the video uclass to rate limit video syncs by postponing them, and forcing a sync nonetheless with a "force" argument.
Add infrastructure for doing this through driver ops, where the driver can request to defer a sync with -EAGAIN, and the uclass can force it by calling the op again it until it does something.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com
Alternatively, add "force" argument into the driver ops->video_sync(). But I think it should go away instead. The problem is video_sync() being called irregularly and too frequently, maybe we can call it only from cyclic at the hardware refresh rate?
Yes I like that idea better. Calling it in a tight loop until it does something seems wrong to me.
drivers/video/video-uclass.c | 2 ++ include/video.h | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c index 7cec6362570f..accf486509cb 100644 --- a/drivers/video/video-uclass.c +++ b/drivers/video/video-uclass.c @@ -446,6 +446,8 @@ int video_sync(struct udevice *vid, bool force)
if (ops && ops->video_sync) { ret = ops->video_sync(vid);
while (force && ret == -EAGAIN)ret = ops->video_sync(vid); if (ret) return ret; }diff --git a/include/video.h b/include/video.h index 42e57b44188d..5c4327d4e455 100644 --- a/include/video.h +++ b/include/video.h @@ -137,7 +137,8 @@ struct video_priv {
displays needs synchronization when data in an FB is available.For these devices implement video_sync hook to call a syncfunction. vid is pointer to video device udevice. Function
should return 0 on success video_sync and error code otherwise
should return 0 on success video_sync, -EAGAIN if it was*/
deferred and should be tried again, and error code otherwisestruct video_ops { int (*video_sync)(struct udevice *vid); -- 2.40.1
Regards, Simon
As a first step of removing sandbox-specific code from the video uclass, move the sync rate limiter into the SDL code. We lose the ability to immediately force a screen refresh, but can retry until it does.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com ---
arch/sandbox/cpu/sdl.c | 9 +++++++++ arch/sandbox/include/asm/sdl.h | 4 +++- drivers/video/video-uclass.c | 8 +++----- 3 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/arch/sandbox/cpu/sdl.c b/arch/sandbox/cpu/sdl.c index 590e406517bf..48fae20b4c2d 100644 --- a/arch/sandbox/cpu/sdl.c +++ b/arch/sandbox/cpu/sdl.c @@ -48,6 +48,7 @@ struct buf_info { * @screen: SDL window to use * @src_depth: Number of bits per pixel in the source frame buffer (that we read * from and render to SDL) + * @last_sync: Timestamp of last display update in milliseconds */ static struct sdl_info { int width; @@ -67,6 +68,7 @@ static struct sdl_info { SDL_Renderer *renderer; SDL_Window *screen; int src_depth; + int last_sync; } sdl;
static void sandbox_sdl_poll_events(void) @@ -148,6 +150,7 @@ int sandbox_sdl_init_display(int width, int height, int log2_bpp, sdl.vis_width = sdl.width; sdl.vis_height = sdl.height; } + sdl.last_sync = 0;
if (!SDL_SetHint(SDL_HINT_RENDER_SCALE_QUALITY, "1")) printf("Unable to init hinting: %s", SDL_GetError()); @@ -245,6 +248,10 @@ int sandbox_sdl_sync(void *lcd_base)
if (!sdl.texture) return 0; + + if (SDL_GetTicks() - sdl.last_sync < 100) + return -EAGAIN; + SDL_RenderClear(sdl.renderer); ret = copy_to_texture(lcd_base); if (ret) { @@ -268,6 +275,8 @@ int sandbox_sdl_sync(void *lcd_base) SDL_RenderDrawRect(sdl.renderer, &rect);
SDL_RenderPresent(sdl.renderer); + sdl.last_sync = SDL_GetTicks(); + sandbox_sdl_poll_events();
return 0; diff --git a/arch/sandbox/include/asm/sdl.h b/arch/sandbox/include/asm/sdl.h index ee4991f7c24a..1ace7d1a1217 100644 --- a/arch/sandbox/include/asm/sdl.h +++ b/arch/sandbox/include/asm/sdl.h @@ -37,10 +37,12 @@ int sandbox_sdl_remove_display(void); * sandbox_sdl_sync() - Sync current U-Boot LCD frame buffer to SDL * * This must be called periodically to update the screen for SDL so that the - * user can see it. + * user can see it. It is internally rate limited to 10 Hz to avoid using + * system resources too much. * * @lcd_base: Base of frame buffer * Return: 0 if screen was updated, -ENODEV is there is no screen. + * -EAGAIN if screen was not updated due to sync rate limit. */ int sandbox_sdl_sync(void *lcd_base);
diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c index accf486509cb..d867185da539 100644 --- a/drivers/video/video-uclass.c +++ b/drivers/video/video-uclass.c @@ -455,16 +455,14 @@ int video_sync(struct udevice *vid, bool force) video_flush_dcache(vid);
#if defined(CONFIG_VIDEO_SANDBOX_SDL) - static ulong last_sync; void *fb = priv->fb;
if (IS_ENABLED(CONFIG_VIDEO_COPY)) fb = priv->copy_fb;
- if (force || get_timer(last_sync) > 100) { - sandbox_sdl_sync(fb); - last_sync = get_timer(0); - } + ret = sandbox_sdl_sync(fb); + while (force && ret == -EAGAIN) + ret = sandbox_sdl_sync(fb); #endif
if (IS_ENABLED(CONFIG_VIDEO_DAMAGE)) {
The sandbox SDL video sync is handled in the uclass because there has been a sync rate limiter and a way to bypass that. Previous patches move the rate limit code into SDL-specific files, and provide a generic way to defer and force video syncs.
Sandbox code shouldn't be in the uclasses if possible. Move the remaining sandbox sync call into the driver ops. Now that sandbox video sync attempts can defer without resetting video damage, force a video sync before checking that video syncs reset video damage.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com ---
drivers/video/sandbox_sdl.c | 16 ++++++++++++++++ drivers/video/video-uclass.c | 14 -------------- test/dm/video.c | 2 +- 3 files changed, 17 insertions(+), 15 deletions(-)
diff --git a/drivers/video/sandbox_sdl.c b/drivers/video/sandbox_sdl.c index 9081c7da62e4..7dc2787a5d25 100644 --- a/drivers/video/sandbox_sdl.c +++ b/drivers/video/sandbox_sdl.c @@ -99,6 +99,17 @@ int sandbox_sdl_set_bpp(struct udevice *dev, enum video_log2_bpp l2bpp) return 0; }
+static int sandbox_sdl_video_sync(struct udevice *dev) +{ + struct video_priv *priv = dev_get_uclass_priv(dev); + void *fb = priv->fb; + + if (IS_ENABLED(CONFIG_VIDEO_COPY)) + fb = priv->copy_fb; + + return sandbox_sdl_sync(fb); +} + static int sandbox_sdl_remove(struct udevice *dev) { /* @@ -133,6 +144,10 @@ static const struct udevice_id sandbox_sdl_ids[] = { { } };
+static struct video_ops sandbox_sdl_ops = { + .video_sync = sandbox_sdl_video_sync, +}; + U_BOOT_DRIVER(sandbox_lcd_sdl) = { .name = "sandbox_lcd_sdl", .id = UCLASS_VIDEO, @@ -141,4 +156,5 @@ U_BOOT_DRIVER(sandbox_lcd_sdl) = { .probe = sandbox_sdl_probe, .remove = sandbox_sdl_remove, .plat_auto = sizeof(struct sandbox_sdl_plat), + .ops = &sandbox_sdl_ops, }; diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c index d867185da539..2632216c05ae 100644 --- a/drivers/video/video-uclass.c +++ b/drivers/video/video-uclass.c @@ -23,9 +23,6 @@ #include <dm/device_compat.h> #include <dm/device-internal.h> #include <dm/uclass-internal.h> -#ifdef CONFIG_SANDBOX -#include <asm/sdl.h> -#endif
/* * Theory of operation: @@ -454,17 +451,6 @@ int video_sync(struct udevice *vid, bool force)
video_flush_dcache(vid);
-#if defined(CONFIG_VIDEO_SANDBOX_SDL) - void *fb = priv->fb; - - if (IS_ENABLED(CONFIG_VIDEO_COPY)) - fb = priv->copy_fb; - - ret = sandbox_sdl_sync(fb); - while (force && ret == -EAGAIN) - ret = sandbox_sdl_sync(fb); -#endif - if (IS_ENABLED(CONFIG_VIDEO_DAMAGE)) { priv->damage.xstart = priv->xsize; priv->damage.ystart = priv->ysize; diff --git a/test/dm/video.c b/test/dm/video.c index 4c3bcd26e94f..487e4d4a73a2 100644 --- a/test/dm/video.c +++ b/test/dm/video.c @@ -756,7 +756,7 @@ static int dm_test_video_damage(struct unit_test_state *uts) ut_asserteq(1280, priv->damage.xend); ut_asserteq(510, priv->damage.yend);
- video_sync(dev, false); + video_sync(dev, true); ut_asserteq(priv->xsize, priv->damage.xstart); ut_asserteq(priv->ysize, priv->damage.ystart); ut_asserteq(0, priv->damage.xend);
On Mon, 21 Aug 2023 at 12:13, Alper Nebi Yasak alpernebiyasak@gmail.com wrote:
The sandbox SDL video sync is handled in the uclass because there has been a sync rate limiter and a way to bypass that. Previous patches move the rate limit code into SDL-specific files, and provide a generic way to defer and force video syncs.
Sandbox code shouldn't be in the uclasses if possible. Move the remaining sandbox sync call into the driver ops. Now that sandbox video sync attempts can defer without resetting video damage, force a video sync before checking that video syncs reset video damage.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com
drivers/video/sandbox_sdl.c | 16 ++++++++++++++++ drivers/video/video-uclass.c | 14 -------------- test/dm/video.c | 2 +- 3 files changed, 17 insertions(+), 15 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Now that we have video damage tracking, try to reduce the SDL display work by copying only the updated regions onto the SDL texture instead of the entire framebuffer. We still have to do RenderClear and RenderCopy the whole texture onto the renderer, but that allegedly happens in the GPU.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com --- The second half of copy_to_texture is untested.
arch/sandbox/cpu/sdl.c | 25 +++++++++++++++++-------- arch/sandbox/include/asm/sdl.h | 9 +++++++-- drivers/video/sandbox_sdl.c | 16 +++++++++++++++- 3 files changed, 39 insertions(+), 11 deletions(-)
diff --git a/arch/sandbox/cpu/sdl.c b/arch/sandbox/cpu/sdl.c index 48fae20b4c2d..3a3221d89066 100644 --- a/arch/sandbox/cpu/sdl.c +++ b/arch/sandbox/cpu/sdl.c @@ -192,8 +192,10 @@ int sandbox_sdl_init_display(int width, int height, int log2_bpp, return 0; }
-static int copy_to_texture(void *lcd_base) +static int copy_to_texture(void *lcd_base, int xstart, int ystart, + int xend, int yend) { + struct SDL_Rect rect; char *dest; int pitch, x, y; int src_pitch; @@ -201,8 +203,15 @@ static int copy_to_texture(void *lcd_base) char *src; int ret;
+ rect.x = xstart; + rect.y = ystart; + rect.w = xend - xstart + 1; + rect.h = yend - ystart + 1; + if (sdl.src_depth == sdl.depth) { - SDL_UpdateTexture(sdl.texture, NULL, lcd_base, sdl.pitch); + src_pitch = sdl.width * sdl.src_depth / 8; + src = lcd_base + src_pitch * rect.y + rect.x * sdl.src_depth / 8; + SDL_UpdateTexture(sdl.texture, &rect, src, src_pitch); return 0; }
@@ -215,7 +224,7 @@ static int copy_to_texture(void *lcd_base) return -EINVAL; }
- ret = SDL_LockTexture(sdl.texture, NULL, &pixels, &pitch); + ret = SDL_LockTexture(sdl.texture, &rect, &pixels, &pitch); if (ret) { printf("SDL lock %d: %s\n", ret, SDL_GetError()); return ret; @@ -223,12 +232,12 @@ static int copy_to_texture(void *lcd_base)
/* Copy the pixels one by one */ src_pitch = sdl.width * sdl.src_depth / 8; - for (y = 0; y < sdl.height; y++) { + for (y = 0; y < rect.h; y++) { char val;
dest = pixels + y * pitch; - src = lcd_base + src_pitch * y; - for (x = 0; x < sdl.width; x++, dest += 4) { + src = lcd_base + src_pitch * (ystart + y) + xstart; + for (x = 0; x < rect.w; x++, dest += 4) { val = *src++; dest[0] = val; dest[1] = val; @@ -241,7 +250,7 @@ static int copy_to_texture(void *lcd_base) return 0; }
-int sandbox_sdl_sync(void *lcd_base) +int sandbox_sdl_sync(void *lcd_base, int xstart, int ystart, int xend, int yend) { struct SDL_Rect rect; int ret; @@ -253,7 +262,7 @@ int sandbox_sdl_sync(void *lcd_base) return -EAGAIN;
SDL_RenderClear(sdl.renderer); - ret = copy_to_texture(lcd_base); + ret = copy_to_texture(lcd_base, xstart, ystart, xend, yend); if (ret) { printf("copy_to_texture: %d: %s\n", ret, SDL_GetError()); return -EIO; diff --git a/arch/sandbox/include/asm/sdl.h b/arch/sandbox/include/asm/sdl.h index 1ace7d1a1217..c7c73ef3a3e6 100644 --- a/arch/sandbox/include/asm/sdl.h +++ b/arch/sandbox/include/asm/sdl.h @@ -41,10 +41,14 @@ int sandbox_sdl_remove_display(void); * system resources too much. * * @lcd_base: Base of frame buffer + * @xstart: X start position of updated region in pixels from the left + * @ystart: Y start position of updated region in pixels from the top + * @xend: X end position of updated region in pixels from the left + * @yend: Y end position of updated region in pixels from the top * Return: 0 if screen was updated, -ENODEV is there is no screen. * -EAGAIN if screen was not updated due to sync rate limit. */ -int sandbox_sdl_sync(void *lcd_base); +int sandbox_sdl_sync(void *lcd_base, int xstart, int ystart, int xend, int yend);
/** * sandbox_sdl_scan_keys() - scan for pressed keys @@ -118,7 +122,8 @@ static inline int sandbox_sdl_remove_display(void) return -ENODEV; }
-static inline int sandbox_sdl_sync(void *lcd_base) +static inline int sandbox_sdl_sync(void *lcd_base, int xstart, int ystart, + int xend, int yend) { return -ENODEV; } diff --git a/drivers/video/sandbox_sdl.c b/drivers/video/sandbox_sdl.c index 7dc2787a5d25..eb424072b6fe 100644 --- a/drivers/video/sandbox_sdl.c +++ b/drivers/video/sandbox_sdl.c @@ -103,11 +103,25 @@ static int sandbox_sdl_video_sync(struct udevice *dev) { struct video_priv *priv = dev_get_uclass_priv(dev); void *fb = priv->fb; + int xstart = 0; + int ystart = 0; + int xend = priv->xsize; + int yend = priv->ysize;
if (IS_ENABLED(CONFIG_VIDEO_COPY)) fb = priv->copy_fb;
- return sandbox_sdl_sync(fb); + if (IS_ENABLED(CONFIG_VIDEO_DAMAGE)) { + if (!priv->damage.xend && !priv->damage.yend) + return 0; + + xstart = priv->damage.xstart; + ystart = priv->damage.ystart; + xend = priv->damage.xend; + yend = priv->damage.yend; + } + + return sandbox_sdl_sync(fb, xstart, ystart, xend, yend); }
static int sandbox_sdl_remove(struct udevice *dev)
On Mon, 21 Aug 2023 at 12:13, Alper Nebi Yasak alpernebiyasak@gmail.com wrote:
Now that we have video damage tracking, try to reduce the SDL display work by copying only the updated regions onto the SDL texture instead of the entire framebuffer. We still have to do RenderClear and RenderCopy the whole texture onto the renderer, but that allegedly happens in the GPU.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com
The second half of copy_to_texture is untested.
arch/sandbox/cpu/sdl.c | 25 +++++++++++++++++-------- arch/sandbox/include/asm/sdl.h | 9 +++++++-- drivers/video/sandbox_sdl.c | 16 +++++++++++++++- 3 files changed, 39 insertions(+), 11 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
participants (2)
-
Alper Nebi Yasak -
Simon Glass