[PATCH v5 11/13] video: Use VIDEO_DAMAGE for VIDEO_COPY
Alper Nebi Yasak
alpernebiyasak at gmail.com
Wed Aug 30 21:07:38 CEST 2023
On 2023-08-21 23:06 +03:00, Alexander Graf wrote:
>
> 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.
Yep, see patch 06/13 for size of the regions. E.g. for putc_xy()s it's
fontdata->height * fontdata->width, for rows it's like fontdata->height
* vid_priv->xsize * count...
>>
>>> 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);
> + }
> + }
> +}
I think Simon was asking how and when video_sync() is called outside the
tests. The tests use lower-level functions that are ops->putc_xy() in
each console, and normally vidconsole calls higher on the call-chain
also maybe do a video_sync() when they think it's worth updating the
display.
>>
>>> 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.
More specifically, this batches the copying work video_sync_copy() does
per console-op into video_flush_copy() called once per video_sync().
Then, since vidconsole_memmove() is only used to memmove() and invoke
that copy mechanism, we can also reduce it to just memmove().
More information about the U-Boot
mailing list