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

[Alexander Graf]

On 21.08.23 21:11, Simon Glass wrote:
Hi Alper,

On Mon, 21 Aug 2023 at 07:51, Alper Nebi Yasak <alpernebiya...@gmail.com> wrote:
From: Alexander Graf <ag...@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?


I tried to keep the code as simple as possible and only track an "upper 
left" and "lower right" corner of modifications. So sync will always 
copy/flush a single rectangle.



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?


It gets called as part of video_sync():

+static void video_flush_copy(struct udevice *vid)
+{
+       struct video_priv *priv = dev_get_uclass_priv(vid);
+
+       if (!priv->copy_fb)
+               return;
+
+       if (priv->damage.xend && priv->damage.yend) {
+               int lstart = priv->damage.xstart * VNBYTES(priv->bpix);
+               int lend = priv->damage.xend * VNBYTES(priv->bpix);
+               int y;
+
+               for (y = priv->damage.ystart; y < priv->damage.yend; y++) {
+                       ulong offset = (y * priv->line_length) + lstart;
+                       ulong len = lend - lstart;
+
+                       memcpy(priv->copy_fb + offset, priv->fb + offset, len);
+               }
+       }
+}



Signed-off-by: Alexander Graf <ag...@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 <alpernebiya...@gmail.com>
Signed-off-by: Alper Nebi Yasak <alpernebiya...@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?


There is no point in keeping a special vidconsole_memmove() around 
anymore, since we don't actually need to call vidconsole_sync_copy() 
after the move. The damage call that we introduced to all call sites in 
combination with a video_sync() call takes over the job of the sync copy.


Alex