[PATCH v5 11/13] video: Use VIDEO_DAMAGE for VIDEO_COPY

[Simon Glass]

Hi Alper,

On Mon, 21 Aug 2023 at 07:51, Alper Nebi Yasak <alpernebiyasak at gmail.com> wrote:
>
> From: Alexander Graf <agraf at csgraf.de>
>
> CONFIG_VIDEO_COPY implemented a range-based copying mechanism: If we
> print a single character, it will always copy the full range of bytes
> from the top left corner of the character to the lower right onto the
> uncached frame buffer. This includes pretty much the full line contents
> of the printed character.
>
> Since we now have proper damage tracking, let's make use of that to reduce
> the amount of data we need to copy. With this patch applied, we will only
> copy the tiny rectangle surrounding characters when we print them,
> speeding up the video console.

I suppose for rotated consoles it copies whole lines, but otherwise it
does a lot of small copies?

>
> After this, changes to the main frame buffer are not immediately copied
> to the copy frame buffer, but postponed until the next video device
> sync. So issue an explicit sync before inspecting the copy frame buffer
> contents for the video tests.

So how does the sync get done in this case?

>
> Signed-off-by: Alexander Graf <agraf at csgraf.de>
> [Alper: Rebase for fontdata->height/w, fill_part(), fix memmove(dev),
>         drop from defconfig, use damage.xstart/yend, use IS_ENABLED(),
>         call video_sync() before copy_fb check, update video_copy test]
> Co-developed-by: Alper Nebi Yasak <alpernebiyasak at gmail.com>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak at gmail.com>
> ---
>
> Changes in v5:
> - Remove video_sync_copy() also from video_fill(), video_fill_part()
> - Fix memmove() calls by removing the extra dev argument
> - Call video_sync() before checking copy_fb in video tests
> - Use xstart, ystart, xend, yend as names for damage region
> - Use met->baseline instead of priv->baseline
> - Use fontdata->height/width instead of VIDEO_FONT_HEIGHT/WIDTH
> - Use xstart, ystart, xend, yend as names for damage region
> - Use IS_ENABLED() instead of CONFIG_IS_ENABLED()
> - Drop VIDEO_DAMAGE from sandbox defconfig added in a new patch
> - Update dm_test_video_copy test added in a new patch
>
> Changes in v3:
> - Make VIDEO_COPY always select VIDEO_DAMAGE
>
> Changes in v2:
> - Add patch "video: Use VIDEO_DAMAGE for VIDEO_COPY"
>
>  configs/sandbox_defconfig         |  1 -
>  drivers/video/Kconfig             |  5 ++
>  drivers/video/console_normal.c    | 13 +----
>  drivers/video/console_rotate.c    | 44 +++-----------
>  drivers/video/console_truetype.c  | 16 +----
>  drivers/video/vidconsole-uclass.c | 16 -----
>  drivers/video/video-uclass.c      | 97 ++++++++-----------------------
>  drivers/video/video_bmp.c         |  7 ---
>  include/video.h                   | 37 ------------
>  include/video_console.h           | 52 -----------------
>  test/dm/video.c                   |  3 +-
>  11 files changed, 43 insertions(+), 248 deletions(-)
>
> diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
> index 51b820f13121..259f31f26cee 100644
> --- a/configs/sandbox_defconfig
> +++ b/configs/sandbox_defconfig
> @@ -307,7 +307,6 @@ CONFIG_USB_ETH_CDC=y
>  CONFIG_VIDEO=y
>  CONFIG_VIDEO_FONT_SUN12X22=y
>  CONFIG_VIDEO_COPY=y
> -CONFIG_VIDEO_DAMAGE=y
>  CONFIG_CONSOLE_ROTATION=y
>  CONFIG_CONSOLE_TRUETYPE=y
>  CONFIG_CONSOLE_TRUETYPE_CANTORAONE=y
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index 97f494a1340b..b3fbd9d7d9ca 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -83,11 +83,14 @@ config VIDEO_PCI_DEFAULT_FB_SIZE
>
>  config VIDEO_COPY
>         bool "Enable copying the frame buffer to a hardware copy"
> +       select VIDEO_DAMAGE
>         help
>           On some machines (e.g. x86), reading from the frame buffer is very
>           slow because it is uncached. To improve performance, this feature
>           allows the frame buffer to be kept in cached memory (allocated by
>           U-Boot) and then copied to the hardware frame-buffer as needed.
> +         It uses the VIDEO_DAMAGE feature to keep track of regions to copy
> +         and will only copy actually touched regions.
>
>           To use this, your video driver must set @copy_base in
>           struct video_uc_plat.
> @@ -105,6 +108,8 @@ config VIDEO_DAMAGE
>           regions of the frame buffer that were modified before, speeding up
>           screen refreshes significantly.
>
> +         It is also used by VIDEO_COPY to identify which regions changed.
> +
>  config BACKLIGHT_PWM
>         bool "Generic PWM based Backlight Driver"
>         depends on BACKLIGHT && DM_PWM
> diff --git a/drivers/video/console_normal.c b/drivers/video/console_normal.c
> index a19ce6a2bc11..c44aa09473a3 100644
> --- a/drivers/video/console_normal.c
> +++ b/drivers/video/console_normal.c
> @@ -35,10 +35,6 @@ static int console_set_row(struct udevice *dev, uint row, int clr)
>                 fill_pixel_and_goto_next(&dst, clr, pbytes, pbytes);
>         end = dst;
>
> -       ret = vidconsole_sync_copy(dev, line, end);
> -       if (ret)
> -               return ret;
> -
>         video_damage(dev->parent,
>                      0,
>                      fontdata->height * row,
> @@ -57,14 +53,11 @@ static int console_move_rows(struct udevice *dev, uint rowdst,
>         void *dst;
>         void *src;
>         int size;
> -       int ret;
>
>         dst = vid_priv->fb + rowdst * fontdata->height * vid_priv->line_length;
>         src = vid_priv->fb + rowsrc * fontdata->height * vid_priv->line_length;
>         size = fontdata->height * vid_priv->line_length * count;
> -       ret = vidconsole_memmove(dev, dst, src, size);
> -       if (ret)
> -               return ret;
> +       memmove(dst, src, size);

Why are you making that change?

Regards,
Simon