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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 7675 bytes --]

Hi

Am 17.11.22 um 13:57 schrieb Marek Szyprowski:
> 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@suse.de>
>> Reviewed-by: Daniel Vetter <daniel.vetter@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

Thank you so much for reporting. That line should never be executed with 
vc4 et al.

If you have the time, could you please try the attached patch and report 
the results. Thanks a lot.

Best regards
Thomas

> 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

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #1.1.1.2: 0001-drm-fb-helper-Only-do-damage-handling-if-deferred-I-.patch --]
[-- Type: text/x-patch, Size: 4075 bytes --]

From 6697f9c4d0dbcda6e9dd37c1d232a255b23bcf93 Mon Sep 17 00:00:00 2001
From: Thomas Zimmermann <tzimmermann@suse.de>
Date: Thu, 17 Nov 2022 16:54:03 +0100
Subject: [PATCH] drm/fb-helper: Only do damage handling if deferred I/O is
 active

Make damage handling dependent on fbdev deferred I/O, instead of the
existence of the fb_dirty callback. The generic fbdev emulation always
sets the fb_dirty callback, even if it's unused. For drivers that don't
use deferred I/O, this leads to error messages as shown below.

 WARNING: CPU: 0 PID: 220 at drivers/video/fbdev/core/fb_defio.c:340

A bug report is at [1].

Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: 7f5cc4a3e5e4 ("drm/fb-helper: Schedule deferred-I/O worker after writing to framebuffer")
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Jaya Kumar <jayalk@intworks.biz>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Helge Deller <deller@gmx.de>
Cc: linux-fbdev@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Link: https://lore.kernel.org/dri-devel/20221115115819.23088-1-tzimmermann@suse.de/T/#m06eedc0a468940e4cbbd14ca026733b639bc445a
---
 drivers/gpu/drm/drm_fb_helper.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index a1f86e436ae8e..ba5bd85035610 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -816,7 +816,7 @@ ssize_t drm_fb_helper_sys_write(struct fb_info *info, const char __user *buf,
 	if (ret <= 0)
 		return ret;
 
-	if (helper->funcs->fb_dirty) {
+	if (info->fbdefio) {
 		drm_fb_helper_memory_range_to_clip(info, pos, ret, &damage_area);
 		drm_fb_helper_damage(helper, damage_area.x1, damage_area.y1,
 				     drm_rect_width(&damage_area),
@@ -841,7 +841,7 @@ void drm_fb_helper_sys_fillrect(struct fb_info *info,
 
 	sys_fillrect(info, rect);
 
-	if (helper->funcs->fb_dirty)
+	if (info->fbdefio)
 		drm_fb_helper_damage(helper, rect->dx, rect->dy, rect->width, rect->height);
 }
 EXPORT_SYMBOL(drm_fb_helper_sys_fillrect);
@@ -860,7 +860,7 @@ void drm_fb_helper_sys_copyarea(struct fb_info *info,
 
 	sys_copyarea(info, area);
 
-	if (helper->funcs->fb_dirty)
+	if (info->fbdefio)
 		drm_fb_helper_damage(helper, area->dx, area->dy, area->width, area->height);
 }
 EXPORT_SYMBOL(drm_fb_helper_sys_copyarea);
@@ -879,7 +879,7 @@ void drm_fb_helper_sys_imageblit(struct fb_info *info,
 
 	sys_imageblit(info, image);
 
-	if (helper->funcs->fb_dirty)
+	if (info->fbdefio)
 		drm_fb_helper_damage(helper, image->dx, image->dy, image->width, image->height);
 }
 EXPORT_SYMBOL(drm_fb_helper_sys_imageblit);
@@ -989,7 +989,7 @@ ssize_t drm_fb_helper_cfb_write(struct fb_info *info, const char __user *buf,
 	if (ret <= 0)
 		return ret;
 
-	if (helper->funcs->fb_dirty) {
+	if (info->fbdefio) {
 		drm_fb_helper_memory_range_to_clip(info, pos, ret, &damage_area);
 		drm_fb_helper_damage(helper, damage_area.x1, damage_area.y1,
 				     drm_rect_width(&damage_area),
@@ -1014,7 +1014,7 @@ void drm_fb_helper_cfb_fillrect(struct fb_info *info,
 
 	cfb_fillrect(info, rect);
 
-	if (helper->funcs->fb_dirty)
+	if (info->fbdefio)
 		drm_fb_helper_damage(helper, rect->dx, rect->dy, rect->width, rect->height);
 }
 EXPORT_SYMBOL(drm_fb_helper_cfb_fillrect);
@@ -1033,7 +1033,7 @@ void drm_fb_helper_cfb_copyarea(struct fb_info *info,
 
 	cfb_copyarea(info, area);
 
-	if (helper->funcs->fb_dirty)
+	if (info->fbdefio)
 		drm_fb_helper_damage(helper, area->dx, area->dy, area->width, area->height);
 }
 EXPORT_SYMBOL(drm_fb_helper_cfb_copyarea);
@@ -1052,7 +1052,7 @@ void drm_fb_helper_cfb_imageblit(struct fb_info *info,
 
 	cfb_imageblit(info, image);
 
-	if (helper->funcs->fb_dirty)
+	if (info->fbdefio)
 		drm_fb_helper_damage(helper, image->dx, image->dy, image->width, image->height);
 }
 EXPORT_SYMBOL(drm_fb_helper_cfb_imageblit);
-- 
2.38.1


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic