[PATCH v4 1/9] dm: video: Add damage tracking API

[Simon Glass]

Hi Alex,

On Tue, 3 Jan 2023 at 14:50, Alexander Graf <agraf at csgraf.de> wrote:
>
> We are going to introduce image damage tracking to fasten up screen
> refresh on large displays. This patch adds damage tracking for up to
> one rectangle of the screen which is typically enough to hold blt or
> text print updates. Callers into this API and a reduced dcache flush
> code path will follow in later patches.
>
> Signed-off-by: Alexander Graf <agraf at csgraf.de>
> Reported-by: Da Xue <da at libre.computer>
>
> ---
>
> v1 -> v2:
>
>   - Remove ifdefs
>
> v2 -> v3:
>
>   - Adapt Kconfig to DM always
>
> v3 -> v4:
>
>   - Move damage clear from patch 6 to this one
>   - Simplify first damage logic
>   - Remove VIDEO_DAMAGE default for ARM
> ---
>  drivers/video/Kconfig        | 13 ++++++++++++
>  drivers/video/video-uclass.c | 41 ++++++++++++++++++++++++++++++++++++
>  include/video.h              | 29 +++++++++++++++++++++++--
>  3 files changed, 81 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index f539977d9b..826a1a6587 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -62,6 +62,19 @@ config VIDEO_COPY
>           To use this, your video driver must set @copy_base in
>           struct video_uc_plat.
>
> +config VIDEO_DAMAGE
> +       bool "Enable damage tracking of frame buffer regions"
> +       help
> +         On some machines (most ARM), the display frame buffer resides in
> +         RAM. To make the display controller pick up screen updates, we
> +         have to flush frame buffer contents from CPU caches into RAM which
> +         can be a slow operation.
> +
> +         This feature adds damage tracking to collect information about regions
> +         that received updates. When we want to sync, we then only flush
> +         regions of the frame buffer that were modified before, speeding up
> +         screen refreshes significantly.
> +
>  config BACKLIGHT_PWM
>         bool "Generic PWM based Backlight Driver"
>         depends on BACKLIGHT && DM_PWM
> diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
> index 0ce376ca3f..d18c8cd2b1 100644
> --- a/drivers/video/video-uclass.c
> +++ b/drivers/video/video-uclass.c
> @@ -21,6 +21,8 @@
>  #include <dm/device_compat.h>
>  #include <dm/device-internal.h>
>  #include <dm/uclass-internal.h>
> +#include <linux/types.h>
> +#include <linux/bitmap.h>
>  #ifdef CONFIG_SANDBOX
>  #include <asm/sdl.h>
>  #endif
> @@ -254,6 +256,37 @@ void video_set_default_colors(struct udevice *dev, bool invert)
>         priv->colour_bg = video_index_to_colour(priv, back);
>  }
>
> +/* Notify about changes in the frame buffer */
> +int video_damage(struct udevice *vid, int x, int y, int width, int height)
> +{
> +       struct video_priv *priv = dev_get_uclass_priv(vid);
> +       int endx = x + width;
> +       int endy = y + height;
> +
> +       if (!CONFIG_IS_ENABLED(VIDEO_DAMAGE))
> +               return 0;
> +
> +       if (x > priv->xsize)
> +               return 0;
> +
> +       if (y > priv->ysize)
> +               return 0;
> +
> +       if (endx > priv->xsize)
> +               endx = priv->xsize;
> +
> +       if (endy > priv->ysize)
> +               endy = priv->ysize;
> +
> +       /* Span a rectangle across all old and new damage */
> +       priv->damage.x = min(x, priv->damage.x);
> +       priv->damage.y = min(y, priv->damage.y);
> +       priv->damage.endx = max(endx, priv->damage.endx);
> +       priv->damage.endy = max(endy, priv->damage.endy);
> +
> +       return 0;

Should we make this void, then?

> +}
> +
>  /* Flush video activity to the caches */
>  int video_sync(struct udevice *vid, bool force)
>  {
> @@ -288,6 +321,14 @@ int video_sync(struct udevice *vid, bool force)
>                 last_sync = get_timer(0);
>         }
>  #endif
> +
> +       if (CONFIG_IS_ENABLED(VIDEO_DAMAGE)) {
> +               priv->damage.x = priv->xsize;
> +               priv->damage.y = priv->ysize;
> +               priv->damage.endx = 0;
> +               priv->damage.endy = 0;
> +       }
> +
>         return 0;
>  }
>
> diff --git a/include/video.h b/include/video.h
> index 43f2e2c02f..4b35e97f79 100644
> --- a/include/video.h
> +++ b/include/video.h
> @@ -109,6 +109,12 @@ struct video_priv {

Please update the comment when you add struct members.

>         void *fb;
>         int fb_size;
>         void *copy_fb;
> +       struct {
> +               int x;
> +               int y;
> +               int endx;
> +               int endy;
> +       } damage;
>         int line_length;
>         u32 colour_fg;
>         u32 colour_bg;
> @@ -211,8 +217,9 @@ int video_fill(struct udevice *dev, u32 colour);
>   * @return: 0 on success, error code otherwise
>   *
>   * Some frame buffers are cached or have a secondary frame buffer. This
> - * function syncs these up so that the current contents of the U-Boot frame
> - * buffer are displayed to the user.
> + * function syncs the damaged parts of them up so that the current contents
> + * of the U-Boot frame buffer are displayed to the user. It clears the damage
> + * buffer.
>   */
>  int video_sync(struct udevice *vid, bool force);
>
> @@ -332,6 +339,24 @@ static inline int video_sync_copy_all(struct udevice *dev)
>
>  #endif
>
> +/**
> + * video_damage() - Notify the video subsystem about screen updates.
> + *
> + * @vid:       Device to sync
> + * @x:         Upper left X coordinate of the damaged rectangle
> + * @y:         Upper left Y coordinate of the damaged rectangle
> + * @width:     Width of the damaged rectangle
> + * @height:    Height of the damaged rectangle
> + *
> + * @return: 0

Returns: 0

(that is the style now, sadly meaning that much of U-Boot needs to be updated)

> + *
> + * Some frame buffers are cached or have a secondary frame buffer. This
> + * function notifies the video subsystem about rectangles that were updated
> + * within the frame buffer. They may only get written to the screen on the
> + * next call to video_sync().
> + */
> +int video_damage(struct udevice *vid, int x, int y, int width, int height);
> +
>  /**
>   * video_is_active() - Test if one video device it active
>   *
> --
> 2.37.1 (Apple Git-137.1)
>

Regards,
Simon