[PATCH] drm/fb-helper: flush damage before unblanking

[PATCH] drm/fb-helper: flush damage before unblanking

[Angelo Compagnucci]

With a shadow buffer and deferred I/O, what userspace draws in fbdev
reaches the DRM framebuffer only once the deferred I/O work and then the
damage work have run. Unblanking turns the display on with the
framebuffer as it is: a program drawing its first picture and then
unblanking, such as a boot splash taking over the picture a bootloader
left on a panel, shows the previous framebuffer contents for a moment.

Seen on an Arietta G25 with an ILI9341 panel (tinydrm ili9341): enabling
the pipe sends the whole framebuffer to the panel, still black, and the
splash only replaces it once the damage work runs, a visible flicker.

On unblank, flush the deferred I/O and the damage work first.

Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com>
---
 drivers/gpu/drm/clients/drm_fbdev_helper.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/clients/drm_fbdev_helper.c b/drivers/gpu/drm/clients/drm_fbdev_helper.c
index 415764ee086a..5c584dee3e55 100644
--- a/drivers/gpu/drm/clients/drm_fbdev_helper.c
+++ b/drivers/gpu/drm/clients/drm_fbdev_helper.c
@@ -150,6 +150,22 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
 	mutex_unlock(&fb_helper->lock);
 }
 
+/*
+ * Turning a display on sends it the framebuffer: first get there what was
+ * drawn in fbdev, still pending in deferred I/O or damage handling, else the
+ * display shows the framebuffer as it was before for a moment.
+ */
+static void drm_fb_helper_flush_damage(struct fb_info *info)
+{
+	struct drm_fb_helper *helper = info->par;
+
+#ifdef CONFIG_FB_DEFERRED_IO
+	if (info->fbdefio)
+		flush_delayed_work(&info->deferred_work);
+#endif
+	flush_work(&helper->damage_work);
+}
+
 /**
  * drm_fb_helper_blank - implementation for &fb_ops.fb_blank
  * @blank: desired blanking state
@@ -163,6 +179,7 @@ int drm_fb_helper_blank(int blank, struct fb_info *info)
 	switch (blank) {
 	/* Display: On; HSync: On, VSync: On */
 	case FB_BLANK_UNBLANK:
+		drm_fb_helper_flush_damage(info);
 		drm_fb_helper_dpms(info, DRM_MODE_DPMS_ON);
 		break;
 	/* Display: Off; HSync: On, VSync: On */
-- 
2.53.0

Re: [PATCH] drm/fb-helper: flush damage before unblanking

[Thomas Zimmermann]

Hi

Am 12.11.26 um 10:16 schrieb Angelo Compagnucci:
> With a shadow buffer and deferred I/O, what userspace draws in fbdev
> reaches the DRM framebuffer only once the deferred I/O work and then the
> damage work have run. Unblanking turns the display on with the
> framebuffer as it is: a program drawing its first picture and then
> unblanking, such as a boot splash taking over the picture a bootloader
> left on a panel, shows the previous framebuffer contents for a moment.
>
> Seen on an Arietta G25 with an ILI9341 panel (tinydrm ili9341): enabling
> the pipe sends the whole framebuffer to the panel, still black, and the
> splash only replaces it once the damage work runs, a visible flicker.
>
> On unblank, flush the deferred I/O and the damage work first.
>
> Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com>
> ---
>   drivers/gpu/drm/clients/drm_fbdev_helper.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
>
> diff --git a/drivers/gpu/drm/clients/drm_fbdev_helper.c b/drivers/gpu/drm/clients/drm_fbdev_helper.c
> index 415764ee086a..5c584dee3e55 100644
> --- a/drivers/gpu/drm/clients/drm_fbdev_helper.c
> +++ b/drivers/gpu/drm/clients/drm_fbdev_helper.c
> @@ -150,6 +150,22 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
>   	mutex_unlock(&fb_helper->lock);
>   }
>   
> +/*
> + * Turning a display on sends it the framebuffer: first get there what was
> + * drawn in fbdev, still pending in deferred I/O or damage handling, else the
> + * display shows the framebuffer as it was before for a moment.
> + */
> +static void drm_fb_helper_flush_damage(struct fb_info *info)
> +{
> +	struct drm_fb_helper *helper = info->par;
> +
> +#ifdef CONFIG_FB_DEFERRED_IO
> +	if (info->fbdefio)
> +		flush_delayed_work(&info->deferred_work);
> +#endif
> +	flush_work(&helper->damage_work);
> +}
> +
>   /**
>    * drm_fb_helper_blank - implementation for &fb_ops.fb_blank
>    * @blank: desired blanking state
> @@ -163,6 +179,7 @@ int drm_fb_helper_blank(int blank, struct fb_info *info)
>   	switch (blank) {
>   	/* Display: On; HSync: On, VSync: On */
>   	case FB_BLANK_UNBLANK:
> +		drm_fb_helper_flush_damage(info);
>   		drm_fb_helper_dpms(info, DRM_MODE_DPMS_ON);

I think there's no guarantee that we can flush anything before 
unblanking the display.

Best regards
Thomas

>   		break;
>   	/* Display: Off; HSync: On, VSync: On */

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Stefan Gaiser, Jochen Jaser, Abhinav Puri, (HRB 36809, AG Nürnberg)

[PATCH v2] drm/fbdev-dma: Copy the shadow buffer on unblank

[Angelo Compagnucci]

With a shadow buffer, what userspace draws in fbdev reaches the
framebuffer memory only when the damage worker runs. Turning the display
on shows the framebuffer memory as it is. So a program that draws its
first picture and then unblanks, such as a boot splash, shows an older
picture for a moment.

Seen on an ILI9341 panel (tinydrm ili9341): enabling the pipe sends the
whole framebuffer memory to the panel, still black, and the splash only
replaces it once the damage worker runs.

Nothing can be flushed to a display that is off. But the copy from the
shadow buffer into the framebuffer memory does not need the display. On
unblank, copy the whole shadow buffer into the framebuffer memory before
turning the display on. The damage worker still flushes the damage once
the display is on.

Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com>
---
Changes v1 -> v2:
  - Do not flush the damage before unblanking: nothing can reach a
    display that is off. Instead, in fbdev-dma, copy the whole shadow
    buffer into the framebuffer memory on unblank, which needs no
    display [Thomas Zimmermann]
  - The fbdev helpers are unchanged, so the title changed too; v1 was
    "drm/fb-helper: flush damage before unblanking"

 drivers/gpu/drm/drm_fbdev_dma.c | 44 ++++++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_fbdev_dma.c b/drivers/gpu/drm/drm_fbdev_dma.c
index 452f84400350..cc8813538bd9 100644
--- a/drivers/gpu/drm/drm_fbdev_dma.c
+++ b/drivers/gpu/drm/drm_fbdev_dma.c
@@ -93,15 +93,6 @@ static void drm_fbdev_dma_shadowed_fb_destroy(struct fb_info *info)
 	drm_client_release(&fb_helper->client);
 }
 
-static const struct fb_ops drm_fbdev_dma_shadowed_fb_ops = {
-	.owner = THIS_MODULE,
-	.fb_open = drm_fbdev_dma_fb_open,
-	.fb_release = drm_fbdev_dma_fb_release,
-	FB_DEFAULT_DEFERRED_OPS(drm_fbdev_dma_shadowed),
-	DRM_FB_HELPER_DEFAULT_OPS,
-	.fb_destroy = drm_fbdev_dma_shadowed_fb_destroy,
-};
-
 /*
  * struct drm_fb_helper
  */
@@ -196,6 +187,41 @@ static const struct drm_fb_helper_funcs drm_fbdev_dma_helper_funcs = {
  * struct drm_fb_helper
  */
 
+static int drm_fbdev_dma_shadowed_fb_blank(int blank, struct fb_info *info)
+{
+	struct drm_fb_helper *fb_helper = info->par;
+	struct drm_clip_rect clip = {
+		.x2 = fb_helper->fb->width,
+		.y2 = fb_helper->fb->height,
+	};
+
+	/*
+	 * Turning the display on shows the framebuffer memory. Copy the
+	 * shadow buffer into it first, so it holds what was drawn. This
+	 * does not need the display; the damage worker still flushes the
+	 * damage once the display is on.
+	 */
+	if (blank == FB_BLANK_UNBLANK)
+		drm_fbdev_dma_damage_blit(fb_helper, &clip);
+
+	return drm_fb_helper_blank(blank, info);
+}
+
+static const struct fb_ops drm_fbdev_dma_shadowed_fb_ops = {
+	.owner = THIS_MODULE,
+	.fb_open = drm_fbdev_dma_fb_open,
+	.fb_release = drm_fbdev_dma_fb_release,
+	FB_DEFAULT_DEFERRED_OPS(drm_fbdev_dma_shadowed),
+	/* DRM_FB_HELPER_DEFAULT_OPS, with a blank that copies the shadow */
+	.fb_check_var = drm_fb_helper_check_var,
+	.fb_set_par = drm_fb_helper_set_par,
+	.fb_setcmap = drm_fb_helper_setcmap,
+	.fb_blank = drm_fbdev_dma_shadowed_fb_blank,
+	.fb_pan_display = drm_fb_helper_pan_display,
+	.fb_ioctl = drm_fb_helper_ioctl,
+	.fb_destroy = drm_fbdev_dma_shadowed_fb_destroy,
+};
+
 static int drm_fbdev_dma_driver_fbdev_probe_tail(struct drm_fb_helper *fb_helper,
 						 struct drm_fb_helper_surface_size *sizes)
 {
-- 
2.53.0

Re: [PATCH v2] drm/fbdev-dma: Copy the shadow buffer on unblank

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] drm/fbdev-dma: sleeping lock in
  drm_fbdev_dma_shadowed_fb_blank() on oops
- [Low] drm/fbdev-dma: missing Fixes trailer for shadow buffer unblank
  fix
--

commit a63597b4af664e3cb6d1b39bfd1dd1e573a6fdb2
Author: Angelo Compagnucci <angelo.compagnucci@gmail.com>

> drm/fbdev-dma: Copy the shadow buffer on unblank
>
> With a shadow buffer, what userspace draws in fbdev reaches the
> framebuffer memory only when the damage worker runs. Turning the display
> on shows the framebuffer memory as it is. So a program that draws its
> first picture and then unblanks, such as a boot splash, shows an older
> picture for a moment.
>
> Seen on an ILI9341 panel (tinydrm ili9341): enabling the pipe sends the
> whole framebuffer memory to the panel, still black, and the splash only
> replaces it once the damage worker runs.
>
> Nothing can be flushed to a display that is off. But the copy from the
> shadow buffer into the framebuffer memory does not need the display. On
> unblank, copy the whole shadow buffer into the framebuffer memory before
> turning the display on. The damage worker still flushes the damage once
> the display is on.
>
> Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com>

[Severity: Low]
This fixes a bug introduced with shadow buffering.  Should the commit
message include a Fixes tag?

  Fixes: 360399643299 ("drm/fbdev-dma: Add shadow buffering for deferred I/O")

> diff --git a/drivers/gpu/drm/drm_fbdev_dma.c b/drivers/gpu/drm/drm_fbdev_dma.c
> index 452f844003504..cc8813538bd90 100644
> --- a/drivers/gpu/drm/drm_fbdev_dma.c
> +++ b/drivers/gpu/drm/drm_fbdev_dma.c

[ ... ]

> @@ -196,6 +187,41 @@ static const struct drm_fb_helper_funcs drm_fbdev_dma_helper_funcs = {
>   * struct drm_fb_helper
>   */
>  
> +static int drm_fbdev_dma_shadowed_fb_blank(int blank, struct fb_info *info)
> +{
> +	struct drm_fb_helper *fb_helper = info->par;
> +	struct drm_clip_rect clip = {
> +		.x2 = fb_helper->fb->width,
> +		.y2 = fb_helper->fb->height,
> +	};
> +
> +	/*
> +	 * Turning the display on shows the framebuffer memory. Copy the
> +	 * shadow buffer into it first, so it holds what was drawn. This
> +	 * does not need the display; the damage worker still flushes the
> +	 * damage once the display is on.
> +	 */
> +	if (blank == FB_BLANK_UNBLANK)
> +		drm_fbdev_dma_damage_blit(fb_helper, &clip);
> +
> +	return drm_fb_helper_blank(blank, info);
> +}

[Severity: Medium]
Can calling drm_fbdev_dma_damage_blit() before checking oops_in_progress
sleep in atomic context or deadlock during an oops or panic?

When the VT console is blanked during an oops or panic, bust_spinlocks(0)
and panic() call console_unblank() -> ... -> fb_blank(info,
FB_BLANK_UNBLANK) with oops_in_progress set and interrupts disabled.
drm_fb_helper_blank() guards against this at entry:

drivers/gpu/drm/clients/drm_fbdev_helper.c:drm_fb_helper_blank() {
	if (oops_in_progress)
		return -EBUSY;
    ...
}

Calling drm_fbdev_dma_damage_blit() before reaching that check
unconditionally acquires the sleeping mutex fb_helper->lock, which sleeps in
atomic context and can deadlock if the crashing context or a stopped CPU
already holds fb_helper->lock.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113110708.927665-1-angelo.compagnucci@gmail.com?part=1