[v2,5/6] drm/fb-helper: Schedule deferred-I/O worker after writing to framebuffer

Thu Nov 17 04:57:59 PST 2022

Hi Thomas,

On 15.11.2022 12:58, Thomas Zimmermann wrote:
> Schedule the deferred-I/O worker instead of the damage worker after
> writing to the fbdev framebuffer. The deferred-I/O worker then performs
> the dirty-fb update. The fbdev emulation will initialize deferred I/O
> for all drivers that require damage updates. It is therefore a valid
> assumption that the deferred-I/O worker is present.
>
> It would be possible to perform the damage handling directly from within
> the write operation. But doing this could increase the overhead of the
> write or interfere with a concurrently scheduled deferred-I/O worker.
> Instead, scheduling the deferred-I/O worker with its regular delay of
> 50 ms removes load off the write operation and allows the deferred-I/O
> worker to handle multiple write operations that arrived during the delay
> time window.
>
> v2:
> 	* keep drm_fb_helper_damage() (Daniel)
> 	* use fb_deferred_io_schedule_flush() (Daniel)
> 	* clarify comments (Daniel)
>
> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>

This patch, merged into today's linux-next as commit 7f5cc4a3e5e4 
("drm/fb-helper: Schedule deferred-I/O worker after writing to 
framebuffer"), triggers a following warning on Raspberry Pi 3 & 4 as 
well as all Amlogic Meson G12A/B based boards:

------------[ cut here ]------------
WARNING: CPU: 0 PID: 220 at drivers/video/fbdev/core/fb_defio.c:340 
soft_cursor+0x180/0x1f0
Modules linked in: brcmfmac brcmutil vc4(+) sha256_generic libsha256 
snd_soc_hdmi_codec sha256_arm cfg80211 snd_soc_core ac97_bus 
snd_pcm_dmaengine hci_uart btbcm snd_pcm snd_timer snd crc32_arm_ce 
soundcore raspberrypi_hwmon drm_dma_helper bluetooth bcm2835_thermal 
ecdh_generic ecc libaes
CPU: 0 PID: 220 Comm: systemd-udevd Not tainted 
6.1.0-rc5-next-20221117-00041-g13334c897c2b #5953
Hardware name: BCM2835
  unwind_backtrace from show_stack+0x10/0x14
  show_stack from dump_stack_lvl+0x40/0x4c
  dump_stack_lvl from __warn+0xc8/0x13c
  __warn from warn_slowpath_fmt+0x5c/0xb8
  warn_slowpath_fmt from soft_cursor+0x180/0x1f0
  soft_cursor from bit_cursor+0x320/0x4d0
  bit_cursor from fbcon_cursor+0xf4/0x124
  fbcon_cursor from hide_cursor+0x30/0x98
  hide_cursor from redraw_screen+0x1e8/0x230
  redraw_screen from fbcon_prepare_logo+0x390/0x44c
  fbcon_prepare_logo from fbcon_init+0x494/0x5ac
  fbcon_init from visual_init+0xc0/0x108
  visual_init from do_bind_con_driver+0x1b8/0x3a8
  do_bind_con_driver from do_take_over_console+0x13c/0x1e8
  do_take_over_console from do_fbcon_takeover+0x70/0xd0
  do_fbcon_takeover from fbcon_fb_registered+0x19c/0x1ac
  fbcon_fb_registered from register_framebuffer+0x1ec/0x2ec
  register_framebuffer from 
__drm_fb_helper_initial_config_and_unlock+0x3f0/0x5b8
  __drm_fb_helper_initial_config_and_unlock from 
drm_fbdev_client_hotplug+0xbc/0x120
  drm_fbdev_client_hotplug from drm_fbdev_generic_setup+0x88/0x174
  drm_fbdev_generic_setup from vc4_drm_bind+0x1fc/0x294 [vc4]
  vc4_drm_bind [vc4] from try_to_bring_up_aggregate_device+0x160/0x1bc
  try_to_bring_up_aggregate_device from 
component_master_add_with_match+0xc4/0xf8
  component_master_add_with_match from vc4_platform_drm_probe+0xa0/0xc0 
[vc4]
  vc4_platform_drm_probe [vc4] from platform_probe+0x5c/0xb8
  platform_probe from really_probe+0xc8/0x2f0
  really_probe from __driver_probe_device+0x84/0xe4
  __driver_probe_device from driver_probe_device+0x30/0x104
  driver_probe_device from __driver_attach+0x90/0x174
  __driver_attach from bus_for_each_dev+0x70/0xb0
  bus_for_each_dev from bus_add_driver+0x164/0x1f0
  bus_add_driver from driver_register+0x88/0x11c
  driver_register from vc4_drm_register+0x44/0x1000 [vc4]
  vc4_drm_register [vc4] from do_one_initcall+0x40/0x1e0
  do_one_initcall from do_init_module+0x44/0x1d4
  do_init_module from sys_finit_module+0xbc/0xf8
  sys_finit_module from ret_fast_syscall+0x0/0x54
Exception stack(0xf0d85fa8 to 0xf0d85ff0)
...
---[ end trace 0000000000000000 ]---
Console: switching to colour frame buffer device 90x30

It looks that at least the VC4 DRM and Meson DRM drivers needs some 
adjustments to avoid this warning. Am I right?

> ---
>   drivers/gpu/drm/drm_fb_helper.c     | 10 +++++++++-
>   drivers/video/fbdev/core/fb_defio.c | 16 ++++++++++++++++
>   include/linux/fb.h                  |  1 +
>   3 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index cdbf03e941b2b..fbb9088f7defc 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -599,9 +599,17 @@ static void drm_fb_helper_add_damage_clip(struct drm_fb_helper *helper, u32 x, u
>   static void drm_fb_helper_damage(struct drm_fb_helper *helper, u32 x, u32 y,
>   				 u32 width, u32 height)
>   {
> +	struct drm_device *dev = helper->dev;
> +	struct fb_info *info = helper->info;
> +
>   	drm_fb_helper_add_damage_clip(helper, x, y, width, height);
>   
> -	schedule_work(&helper->damage_work);
> +	/*
> +	 * The current fbdev emulation only flushes buffers if a damage
> +	 * update is necessary. And we can assume that deferred I/O has
> +	 * been enabled as damage updates require deferred I/O for mmap.
> +	 */
> +	fb_deferred_io_schedule_flush(info);
>   }
>   
>   /*
> diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
> index c730253ab85ce..dec678f72a42f 100644
> --- a/drivers/video/fbdev/core/fb_defio.c
> +++ b/drivers/video/fbdev/core/fb_defio.c
> @@ -332,3 +332,19 @@ void fb_deferred_io_cleanup(struct fb_info *info)
>   	mutex_destroy(&fbdefio->lock);
>   }
>   EXPORT_SYMBOL_GPL(fb_deferred_io_cleanup);
> +
> +void fb_deferred_io_schedule_flush(struct fb_info *info)
> +{
> +	struct fb_deferred_io *fbdefio = info->fbdefio;
> +
> +	if (WARN_ON_ONCE(!fbdefio))
> +		return; /* bug in driver logic */
> +
> +	/*
> +	 * There's no requirement from callers to schedule the
> +	 * flush immediately. Rather schedule the worker with a
> +	 * delay and let a few more writes pile up.
> +	 */
> +	schedule_delayed_work(&info->deferred_work, fbdefio->delay);
> +}
> +EXPORT_SYMBOL_GPL(fb_deferred_io_schedule_flush);
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index bcb8658f5b64d..172f271520c78 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -663,6 +663,7 @@ extern void fb_deferred_io_open(struct fb_info *info,
>   				struct inode *inode,
>   				struct file *file);
>   extern void fb_deferred_io_cleanup(struct fb_info *info);
> +extern void fb_deferred_io_schedule_flush(struct fb_info *info);
>   extern int fb_deferred_io_fsync(struct file *file, loff_t start,
>   				loff_t end, int datasync);
>   

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland