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

[sashiko-bot@kernel.org]

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