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


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


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