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@xxxxxxx> > Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx> 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