| Message ID | 20230124091046.2500682-1-jouni.hogander@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/fbdev: Implement wrappers for callbacks used by fbcon | expand |
Hi Am 24.01.23 um 10:10 schrieb Jouni Högander: > After disconnecting damage worker from update logic our dirty callback > is not called on fbcon events. This is causing problems to features > (PSR, FBC, DRRS) relying on dirty callback getting called and breaking > fb console when these features are in use. > > Implement wrappers for callbacks used by fbcon and call our dirty > callback in those. > > Fixes: f231af498c29 ("drm/fb-helper: Disconnect damage worker from update logic") > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: Jani Nikula <jani.nikula@intel.com> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> This is the better solution wrt what fbdev wants. Acked-by: Thomas Zimmermann <tzimmermann@suse.de> Best regards Thomas > --- > drivers/gpu/drm/i915/display/intel_fbdev.c | 53 ++++++++++++++++++++-- > 1 file changed, 49 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c > index 19f3b5d92a55..b1653624552e 100644 > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c > @@ -77,6 +77,18 @@ static void intel_fbdev_invalidate(struct intel_fbdev *ifbdev) > intel_frontbuffer_invalidate(to_frontbuffer(ifbdev), ORIGIN_CPU); > } > > +static void intel_fbdev_dirty(struct fb_info *info) > +{ > + struct drm_fb_helper *helper = info->par; > + > + /* > + * Intel_fb dirty implementation doesn't use damage clips -> > + * no need to pass them here > + */ > + if (helper->fb->funcs->dirty) > + helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, NULL, 0); > +} > + > static int intel_fbdev_set_par(struct fb_info *info) > { > struct drm_fb_helper *fb_helper = info->par; > @@ -91,6 +103,39 @@ static int intel_fbdev_set_par(struct fb_info *info) > return ret; > } > > +static ssize_t intel_fbdev_write(struct fb_info *info, const char __user *buf, > + size_t count, loff_t *ppos) > +{ > + int ret; > + > + ret = drm_fb_helper_cfb_write(info, buf, count, ppos); > + if (ret > 0) > + intel_fbdev_dirty(info); > + > + return ret; > +} > + > +static void intel_fbdev_fillrect(struct fb_info *info, > + const struct fb_fillrect *rect) > +{ > + drm_fb_helper_cfb_fillrect(info, rect); > + intel_fbdev_dirty(info); > +} > + > +static void intel_fbdev_copyarea(struct fb_info *info, > + const struct fb_copyarea *area) > +{ > + drm_fb_helper_cfb_copyarea(info, area); > + intel_fbdev_dirty(info); > +} > + > +static void intel_fbdev_imageblit(struct fb_info *info, > + const struct fb_image *image) > +{ > + drm_fb_helper_cfb_imageblit(info, image); > + intel_fbdev_dirty(info); > +} > + > static int intel_fbdev_blank(int blank, struct fb_info *info) > { > struct drm_fb_helper *fb_helper = info->par; > @@ -125,10 +170,10 @@ static const struct fb_ops intelfb_ops = { > DRM_FB_HELPER_DEFAULT_OPS, > .fb_set_par = intel_fbdev_set_par, > .fb_read = drm_fb_helper_cfb_read, > - .fb_write = drm_fb_helper_cfb_write, > - .fb_fillrect = drm_fb_helper_cfb_fillrect, > - .fb_copyarea = drm_fb_helper_cfb_copyarea, > - .fb_imageblit = drm_fb_helper_cfb_imageblit, > + .fb_write = intel_fbdev_write, > + .fb_fillrect = intel_fbdev_fillrect, > + .fb_copyarea = intel_fbdev_copyarea, > + .fb_imageblit = intel_fbdev_imageblit, > .fb_pan_display = intel_fbdev_pan_display, > .fb_blank = intel_fbdev_blank, > };
Greeting,
FYI, we noticed BUG:sleeping_function_called_from_invalid_context_at_kernel/locking/mutex.c due to commit (built with gcc-11):
commit: 60177838fe0528a3be445b18883191256c548e6b ("[Intel-gfx] [PATCH] drm/i915/fbdev: Implement wrappers for callbacks used by fbcon")
url: https://github.com/intel-lab-lkp/linux/commits/Jouni-H-gander/drm-i915-fbdev-Implement-wrappers-for-callbacks-used-by-fbcon/20230124-171233
base: git://anongit.freedesktop.org/drm/drm-tip drm-tip
patch link: https://lore.kernel.org/all/20230124091046.2500682-1-jouni.hogander@intel.com/
patch subject: [Intel-gfx] [PATCH] drm/i915/fbdev: Implement wrappers for callbacks used by fbcon
in testcase: igt
version: igt-x86_64-a978df79-1_20230128
with following parameters:
group: group-20
on test machine: 20 threads 1 sockets (Commet Lake) with 16G memory
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
user :notice: [ 52.579833] i915_vma_resource 126 126 384 42 4 : tunables 0 0 0 : slabdata 3 3 0
user :notice: [ 52.602229] i915_vma 126 126 768 42 8 : tunables 0 0 0 : slabdata 3 3 0
kern :info : [ 52.615934] fbcon: i915drmfb (fb0) is primary device
kern :info : [ 52.616084] Console: switching to colour frame buffer device 240x67
user :notice: [ 52.617420] i915_priolist 128 128 64 64 1 : tunables 0 0 0 : slabdata 2 2 0
kern :info : [ 52.617825] i915 0000:00:02.0: [drm] Reducing the compressed framebuffer size. This may lead to less power savings than a non-reduced-size. Try to increase stolen memory size if available in BIOS.
user :notice: [ 52.618848] i915_dependency 0 0 128 32 1 : tunables 0 0 0 : slabdata 0 0 0
kern :info : [ 52.624256] i915 0000:00:02.0: [drm] fb0: i915drmfb frame buffer device
kern :err : [ 52.644689] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:1099
kern :err : [ 52.644691] in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 423, name: sed
kern :err : [ 52.644693] preempt_count: 2, expected: 0
kern :warn : [ 52.644694] CPU: 12 PID: 423 Comm: sed Not tainted 6.2.0-rc5-00875-g60177838fe05 #9
kern :warn : [ 52.644697] Hardware name: Intel Corporation CometLake Client Platform/CometLake S UDIMM (ERB/CRB), BIOS CMLSFWR1.R00.2212.D00.2104290922 04/29/2021
kern :warn : [ 52.644698] Call Trace:
kern :warn : [ 52.644699] <TASK>
kern :warn : [ 52.644700] dump_stack_lvl (kbuild/src/x86_64-3/lib/dump_stack.c:107 (discriminator 1))
kern :warn : [ 52.644704] __might_resched.cold (kbuild/src/x86_64-3/kernel/sched/core.c:10037)
kern :warn : [ 52.644707] ? kasan_set_track (kbuild/src/x86_64-3/mm/kasan/common.c:52)
kern :warn : [ 52.644710] ww_mutex_lock (kbuild/src/x86_64-3/kernel/locking/mutex.c:1099)
kern :warn : [ 52.644713] ? __ww_mutex_lock_slowpath (kbuild/src/x86_64-3/kernel/locking/mutex.c:1098)
kern :warn : [ 52.644715] ? fill_ptr_key (kbuild/src/x86_64-3/lib/vsprintf.c:2504)
kern :warn : [ 52.644717] ? vfs_write (kbuild/src/x86_64-3/include/linux/fs.h:2189 kbuild/src/x86_64-3/fs/read_write.c:491 kbuild/src/x86_64-3/fs/read_write.c:584)
kern :warn : [ 52.644719] i915_gem_object_flush_if_display (kbuild/src/x86_64-3/drivers/gpu/drm/i915/gem/i915_gem_object.h:176 kbuild/src/x86_64-3/drivers/gpu/drm/i915/gem/i915_gem_object.h:187 kbuild/src/x86_64-3/drivers/gpu/drm/i915/gem/i915_gem_domain.c:106) i915
kern :warn : [ 52.644805] intel_user_framebuffer_dirty (kbuild/src/x86_64-3/drivers/gpu/drm/i915/display/intel_display_types.h:2062 (discriminator 1) kbuild/src/x86_64-3/drivers/gpu/drm/i915/display/intel_fb.c:1865 (discriminator 1)) i915
kern :warn : [ 52.644890] soft_cursor (kbuild/src/x86_64-3/drivers/video/fbdev/core/softcursor.c:75)
kern :warn : [ 52.644893] ? desc_read_finalized_seq (kbuild/src/x86_64-3/kernel/printk/printk_ringbuffer.c:1770)
kern :warn : [ 52.644897] bit_cursor (kbuild/src/x86_64-3/drivers/video/fbdev/core/bitblit.c:377)
kern :warn : [ 52.644899] ? skb_csum_hwoffload_help (kbuild/src/x86_64-3/net/core/dev.c:3329)
kern :warn : [ 52.644902] ? bit_putcs (kbuild/src/x86_64-3/drivers/video/fbdev/core/bitblit.c:238)
kern :warn : [ 52.644903] ? copy_data (kbuild/src/x86_64-3/kernel/printk/printk_ringbuffer.c:1798)
kern :warn : [ 52.644905] ? __kasan_kmalloc (kbuild/src/x86_64-3/mm/kasan/common.c:381)
kern :warn : [ 52.644907] ? get_color (kbuild/src/x86_64-3/drivers/video/fbdev/core/fbcon.c:287)
kern :warn : [ 52.644909] ? bit_putcs (kbuild/src/x86_64-3/drivers/video/fbdev/core/bitblit.c:238)
kern :warn : [ 52.644911] ? fbcon_cursor (kbuild/src/x86_64-3/drivers/video/fbdev/core/fbcon.c:1325)
kern :warn : [ 52.644913] hide_cursor (kbuild/src/x86_64-3/drivers/tty/vt/vt.c:893 kbuild/src/x86_64-3/drivers/tty/vt/vt.c:908)
kern :warn : [ 52.644915] vt_console_print (kbuild/src/x86_64-3/drivers/tty/vt/vt.c:3108)
kern :warn : [ 52.644917] ? find_first_fitting_seq (kbuild/src/x86_64-3/kernel/printk/printk.c:1415)
kern :warn : [ 52.644919] ? _raw_read_unlock_irqrestore (kbuild/src/x86_64-3/kernel/locking/spinlock.c:161)
kern :warn : [ 52.644922] ? lf (kbuild/src/x86_64-3/drivers/tty/vt/vt.c:3079)
kern :warn : [ 52.644924] ? _raw_spin_lock (kbuild/src/x86_64-3/arch/x86/include/asm/atomic.h:202 kbuild/src/x86_64-3/include/linux/atomic/atomic-instrumented.h:543 kbuild/src/x86_64-3/include/asm-generic/qspinlock.h:111 kbuild/src/x86_64-3/include/linux/spinlock.h:186 kbuild/src/x86_64-3/include/linux/spinlock_api_smp.h:134 kbuild/src/x86_64-3/kernel/locking/spinlock.c:154)
kern :warn : [ 52.644926] ? _raw_write_lock_irq (kbuild/src/x86_64-3/kernel/locking/spinlock.c:153)
kern :warn : [ 52.644928] ? prb_final_commit (kbuild/src/x86_64-3/kernel/printk/printk_ringbuffer.c:1939)
kern :warn : [ 52.644931] console_emit_next_record+0x2b5/0x6c0
kern :warn : [ 52.644933] ? devkmsg_read (kbuild/src/x86_64-3/kernel/printk/printk.c:2769)
kern :warn : [ 52.644936] ? printk_sprint (kbuild/src/x86_64-3/kernel/printk/printk.c:2208)
kern :warn : [ 52.644937] ? __kasan_kmalloc (kbuild/src/x86_64-3/mm/kasan/common.c:381)
kern :warn : [ 52.644939] ? devkmsg_write (kbuild/src/x86_64-3/include/linux/slab.h:584 kbuild/src/x86_64-3/kernel/printk/printk.c:762)
kern :warn : [ 52.644941] console_flush_all (kbuild/src/x86_64-3/kernel/printk/printk.c:2889)
kern :warn : [ 52.644943] console_unlock (kbuild/src/x86_64-3/kernel/printk/printk.c:2967)
kern :warn : [ 52.644945] ? console_flush_all (kbuild/src/x86_64-3/kernel/printk/printk.c:2939)
kern :warn : [ 52.644947] vprintk_emit (kbuild/src/x86_64-3/arch/x86/include/asm/preempt.h:85 kbuild/src/x86_64-3/kernel/printk/printk.c:2360)
kern :warn : [ 52.644950] devkmsg_emit+0xab/0xdc
kern :warn : [ 52.644952] ? suspend_console.cold (kbuild/src/x86_64-3/kernel/printk/printk.c:727)
kern :warn : [ 52.644954] ? check_heap_object (kbuild/src/x86_64-3/arch/x86/include/asm/bitops.h:207 kbuild/src/x86_64-3/arch/x86/include/asm/bitops.h:239 kbuild/src/x86_64-3/include/asm-generic/bitops/instrumented-non-atomic.h:142 kbuild/src/x86_64-3/include/linux/page-flags.h:485 kbuild/src/x86_64-3/mm/usercopy.c:194)
kern :warn : [ 52.644957] ? kasan_set_track (kbuild/src/x86_64-3/mm/kasan/common.c:52)
kern :warn : [ 52.644958] devkmsg_write.cold (kbuild/src/x86_64-3/kernel/printk/printk.c:797)
kern :warn : [ 52.644961] ? vprintk_default (kbuild/src/x86_64-3/kernel/printk/printk.c:740)
kern :warn : [ 52.644963] vfs_write (kbuild/src/x86_64-3/include/linux/fs.h:2189 kbuild/src/x86_64-3/fs/read_write.c:491 kbuild/src/x86_64-3/fs/read_write.c:584)
kern :warn : [ 52.644964] ? kernel_write (kbuild/src/x86_64-3/fs/read_write.c:565)
kern :warn : [ 52.644966] ? rcu_report_qs_rdp (kbuild/src/x86_64-3/kernel/rcu/tree.c:2050)
kern :warn : [ 52.644968] ? __fget_light (kbuild/src/x86_64-3/include/linux/atomic/atomic-arch-fallback.h:227 kbuild/src/x86_64-3/include/linux/atomic/atomic-instrumented.h:35 kbuild/src/x86_64-3/fs/file.c:1015)
kern :warn : [ 52.644971] ksys_write (kbuild/src/x86_64-3/fs/read_write.c:637)
kern :warn : [ 52.644972] ? __ia32_sys_read (kbuild/src/x86_64-3/fs/read_write.c:627)
kern :warn : [ 52.644974] do_syscall_64 (kbuild/src/x86_64-3/arch/x86/entry/common.c:50 kbuild/src/x86_64-3/arch/x86/entry/common.c:80)
kern :warn : [ 52.644976] entry_SYSCALL_64_after_hwframe (kbuild/src/x86_64-3/arch/x86/entry/entry_64.S:120)
kern :warn : [ 52.644978] RIP: 0033:0x7fe4a9bd5833
kern :warn : [ 52.644981] Code: 8b 15 61 26 0e 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 64 8b 04 25 18 00 00 00 85 c0 75 14 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 55 c3 0f 1f 40 00 48 83 ec 28 48 89 54 24 18
All code
========
0: 8b 15 61 26 0e 00 mov 0xe2661(%rip),%edx # 0xe2667
6: f7 d8 neg %eax
8: 64 89 02 mov %eax,%fs:(%rdx)
b: 48 c7 c0 ff ff ff ff mov $0xffffffffffffffff,%rax
12: eb b7 jmp 0xffffffffffffffcb
14: 0f 1f 00 nopl (%rax)
17: 64 8b 04 25 18 00 00 mov %fs:0x18,%eax
1e: 00
1f: 85 c0 test %eax,%eax
21: 75 14 jne 0x37
23: b8 01 00 00 00 mov $0x1,%eax
28: 0f 05 syscall
2a:* 48 3d 00 f0 ff ff cmp $0xfffffffffffff000,%rax <-- trapping instruction
30: 77 55 ja 0x87
32: c3 retq
33: 0f 1f 40 00 nopl 0x0(%rax)
37: 48 83 ec 28 sub $0x28,%rsp
3b: 48 89 54 24 18 mov %rdx,0x18(%rsp)
Code starting with the faulting instruction
===========================================
0: 48 3d 00 f0 ff ff cmp $0xfffffffffffff000,%rax
6: 77 55 ja 0x5d
8: c3 retq
9: 0f 1f 40 00 nopl 0x0(%rax)
d: 48 83 ec 28 sub $0x28,%rsp
11: 48 89 54 24 18 mov %rdx,0x18(%rsp)
kern :warn : [ 52.644982] RSP: 002b:00007ffca32f1068 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
kern :warn : [ 52.644985] RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00007fe4a9bd5833
kern :warn : [ 52.644986] RDX: 0000000000000001 RSI: 000056242854c448 RDI: 0000000000000001
kern :warn : [ 52.644987] RBP: 000056242854c448 R08: 00005624301bc090 R09: 0000000000000000
kern :warn : [ 52.644988] R10: 000000000000006a R11: 0000000000000246 R12: 0000000000000001
kern :warn : [ 52.644989] R13: 00007fe4a9cb96a0 R14: 0000000000000001 R15: 00007fe4a9cb4880
kern :warn : [ 52.644991] </TASK>
If you fix the issue, kindly add following tag
| Reported-by: kernel test robot <yujie.liu@intel.com>
| Link: https://lore.kernel.org/oe-lkp/202302030918.58d54238-yujie.liu@intel.com
To reproduce:
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
sudo bin/lkp install job.yaml # job file is attached in this email
bin/lkp split-job --compatible job.yaml # generate the yaml file for lkp run
sudo bin/lkp run generated-yaml-file
# if come across any failure that blocks the test,
# please remove ~/.lkp and /lkp dir to run from a clean state.
On Tue, 2023-01-24 at 13:27 +0100, Thomas Zimmermann wrote: > Hi > > Am 24.01.23 um 10:10 schrieb Jouni Högander: > > After disconnecting damage worker from update logic our dirty > > callback > > is not called on fbcon events. This is causing problems to features > > (PSR, FBC, DRRS) relying on dirty callback getting called and > > breaking > > fb console when these features are in use. > > > > Implement wrappers for callbacks used by fbcon and call our dirty > > callback in those. > > > > Fixes: f231af498c29 ("drm/fb-helper: Disconnect damage worker from > > update logic") > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > Cc: Jani Nikula <jani.nikula@intel.com> > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > This is the better solution wrt what fbdev wants. There was a failure from testing robot. drivers/tty/vt/vt.c is using spinlock and in our dirty callback we are taking mutex. Do you have any suggestions? Shall we fallback to original fix which was setting the dirty callback where call is made from workqueue? > > Acked-by: Thomas Zimmermann <tzimmermann@suse.de> > > Best regards > Thomas > > > --- > > drivers/gpu/drm/i915/display/intel_fbdev.c | 53 > > ++++++++++++++++++++-- > > 1 file changed, 49 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c > > b/drivers/gpu/drm/i915/display/intel_fbdev.c > > index 19f3b5d92a55..b1653624552e 100644 > > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c > > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c > > @@ -77,6 +77,18 @@ static void intel_fbdev_invalidate(struct > > intel_fbdev *ifbdev) > > intel_frontbuffer_invalidate(to_frontbuffer(ifbdev), > > ORIGIN_CPU); > > } > > > > +static void intel_fbdev_dirty(struct fb_info *info) > > +{ > > + struct drm_fb_helper *helper = info->par; > > + > > + /* > > + * Intel_fb dirty implementation doesn't use damage clips - > > > > > + * no need to pass them here > > + */ > > + if (helper->fb->funcs->dirty) > > + helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, > > NULL, 0); > > +} > > + > > static int intel_fbdev_set_par(struct fb_info *info) > > { > > struct drm_fb_helper *fb_helper = info->par; > > @@ -91,6 +103,39 @@ static int intel_fbdev_set_par(struct fb_info > > *info) > > return ret; > > } > > > > +static ssize_t intel_fbdev_write(struct fb_info *info, const char > > __user *buf, > > + size_t count, loff_t *ppos) > > +{ > > + int ret; > > + > > + ret = drm_fb_helper_cfb_write(info, buf, count, ppos); > > + if (ret > 0) > > + intel_fbdev_dirty(info); > > + > > + return ret; > > +} > > + > > +static void intel_fbdev_fillrect(struct fb_info *info, > > + const struct fb_fillrect *rect) > > +{ > > + drm_fb_helper_cfb_fillrect(info, rect); > > + intel_fbdev_dirty(info); > > +} > > + > > +static void intel_fbdev_copyarea(struct fb_info *info, > > + const struct fb_copyarea *area) > > +{ > > + drm_fb_helper_cfb_copyarea(info, area); > > + intel_fbdev_dirty(info); > > +} > > + > > +static void intel_fbdev_imageblit(struct fb_info *info, > > + const struct fb_image *image) > > +{ > > + drm_fb_helper_cfb_imageblit(info, image); > > + intel_fbdev_dirty(info); > > +} > > + > > static int intel_fbdev_blank(int blank, struct fb_info *info) > > { > > struct drm_fb_helper *fb_helper = info->par; > > @@ -125,10 +170,10 @@ static const struct fb_ops intelfb_ops = { > > DRM_FB_HELPER_DEFAULT_OPS, > > .fb_set_par = intel_fbdev_set_par, > > .fb_read = drm_fb_helper_cfb_read, > > - .fb_write = drm_fb_helper_cfb_write, > > - .fb_fillrect = drm_fb_helper_cfb_fillrect, > > - .fb_copyarea = drm_fb_helper_cfb_copyarea, > > - .fb_imageblit = drm_fb_helper_cfb_imageblit, > > + .fb_write = intel_fbdev_write, > > + .fb_fillrect = intel_fbdev_fillrect, > > + .fb_copyarea = intel_fbdev_copyarea, > > + .fb_imageblit = intel_fbdev_imageblit, > > .fb_pan_display = intel_fbdev_pan_display, > > .fb_blank = intel_fbdev_blank, > > }; > > -- > 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
On Fri, Feb 03, 2023 at 07:21:27AM +0000, Hogander, Jouni wrote: > On Tue, 2023-01-24 at 13:27 +0100, Thomas Zimmermann wrote: > > Hi > > > > Am 24.01.23 um 10:10 schrieb Jouni Högander: > > > After disconnecting damage worker from update logic our dirty > > > callback > > > is not called on fbcon events. This is causing problems to features > > > (PSR, FBC, DRRS) relying on dirty callback getting called and > > > breaking > > > fb console when these features are in use. > > > > > > Implement wrappers for callbacks used by fbcon and call our dirty > > > callback in those. > > > > > > Fixes: f231af498c29 ("drm/fb-helper: Disconnect damage worker from > > > update logic") > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > > > This is the better solution wrt what fbdev wants. > > There was a failure from testing robot. drivers/tty/vt/vt.c is using > spinlock and in our dirty callback we are taking mutex. > > Do you have any suggestions? Shall we fallback to original fix which > was setting the dirty callback where call is made from workqueue? Please just fix the original regression as straightforwardly as possible. > > > > > Acked-by: Thomas Zimmermann <tzimmermann@suse.de> > > > > Best regards > > Thomas > > > > > --- > > > drivers/gpu/drm/i915/display/intel_fbdev.c | 53 > > > ++++++++++++++++++++-- > > > 1 file changed, 49 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c > > > b/drivers/gpu/drm/i915/display/intel_fbdev.c > > > index 19f3b5d92a55..b1653624552e 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c > > > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c > > > @@ -77,6 +77,18 @@ static void intel_fbdev_invalidate(struct > > > intel_fbdev *ifbdev) > > > intel_frontbuffer_invalidate(to_frontbuffer(ifbdev), > > > ORIGIN_CPU); > > > } > > > > > > +static void intel_fbdev_dirty(struct fb_info *info) > > > +{ > > > + struct drm_fb_helper *helper = info->par; > > > + > > > + /* > > > + * Intel_fb dirty implementation doesn't use damage clips - > > > > > > > + * no need to pass them here > > > + */ > > > + if (helper->fb->funcs->dirty) > > > + helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, > > > NULL, 0); > > > +} > > > + > > > static int intel_fbdev_set_par(struct fb_info *info) > > > { > > > struct drm_fb_helper *fb_helper = info->par; > > > @@ -91,6 +103,39 @@ static int intel_fbdev_set_par(struct fb_info > > > *info) > > > return ret; > > > } > > > > > > +static ssize_t intel_fbdev_write(struct fb_info *info, const char > > > __user *buf, > > > + size_t count, loff_t *ppos) > > > +{ > > > + int ret; > > > + > > > + ret = drm_fb_helper_cfb_write(info, buf, count, ppos); > > > + if (ret > 0) > > > + intel_fbdev_dirty(info); > > > + > > > + return ret; > > > +} > > > + > > > +static void intel_fbdev_fillrect(struct fb_info *info, > > > + const struct fb_fillrect *rect) > > > +{ > > > + drm_fb_helper_cfb_fillrect(info, rect); > > > + intel_fbdev_dirty(info); > > > +} > > > + > > > +static void intel_fbdev_copyarea(struct fb_info *info, > > > + const struct fb_copyarea *area) > > > +{ > > > + drm_fb_helper_cfb_copyarea(info, area); > > > + intel_fbdev_dirty(info); > > > +} > > > + > > > +static void intel_fbdev_imageblit(struct fb_info *info, > > > + const struct fb_image *image) > > > +{ > > > + drm_fb_helper_cfb_imageblit(info, image); > > > + intel_fbdev_dirty(info); > > > +} > > > + > > > static int intel_fbdev_blank(int blank, struct fb_info *info) > > > { > > > struct drm_fb_helper *fb_helper = info->par; > > > @@ -125,10 +170,10 @@ static const struct fb_ops intelfb_ops = { > > > DRM_FB_HELPER_DEFAULT_OPS, > > > .fb_set_par = intel_fbdev_set_par, > > > .fb_read = drm_fb_helper_cfb_read, > > > - .fb_write = drm_fb_helper_cfb_write, > > > - .fb_fillrect = drm_fb_helper_cfb_fillrect, > > > - .fb_copyarea = drm_fb_helper_cfb_copyarea, > > > - .fb_imageblit = drm_fb_helper_cfb_imageblit, > > > + .fb_write = intel_fbdev_write, > > > + .fb_fillrect = intel_fbdev_fillrect, > > > + .fb_copyarea = intel_fbdev_copyarea, > > > + .fb_imageblit = intel_fbdev_imageblit, > > > .fb_pan_display = intel_fbdev_pan_display, > > > .fb_blank = intel_fbdev_blank, > > > }; > > > > -- > > 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 >
On Fri, 2023-02-03 at 12:42 +0200, Ville Syrjälä wrote: > On Fri, Feb 03, 2023 at 07:21:27AM +0000, Hogander, Jouni wrote: > > On Tue, 2023-01-24 at 13:27 +0100, Thomas Zimmermann wrote: > > > Hi > > > > > > Am 24.01.23 um 10:10 schrieb Jouni Högander: > > > > After disconnecting damage worker from update logic our dirty > > > > callback > > > > is not called on fbcon events. This is causing problems to > > > > features > > > > (PSR, FBC, DRRS) relying on dirty callback getting called and > > > > breaking > > > > fb console when these features are in use. > > > > > > > > Implement wrappers for callbacks used by fbcon and call our > > > > dirty > > > > callback in those. > > > > > > > > Fixes: f231af498c29 ("drm/fb-helper: Disconnect damage worker > > > > from > > > > update logic") > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > > > > > This is the better solution wrt what fbdev wants. > > > > There was a failure from testing robot. drivers/tty/vt/vt.c is > > using > > spinlock and in our dirty callback we are taking mutex. > > > > Do you have any suggestions? Shall we fallback to original fix > > which > > was setting the dirty callback where call is made from workqueue? > > Please just fix the original regression as straightforwardly as > possible. Here is the original patch by me: https://patchwork.freedesktop.org/series/111433/ which was eventually nacked by Thomas. Maybe we could continue with that one and solve these problems when/if fb_dirty is removed? > > > > > > > > > Acked-by: Thomas Zimmermann <tzimmermann@suse.de> > > > > > > Best regards > > > Thomas > > > > > > > --- > > > > drivers/gpu/drm/i915/display/intel_fbdev.c | 53 > > > > ++++++++++++++++++++-- > > > > 1 file changed, 49 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c > > > > b/drivers/gpu/drm/i915/display/intel_fbdev.c > > > > index 19f3b5d92a55..b1653624552e 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c > > > > @@ -77,6 +77,18 @@ static void intel_fbdev_invalidate(struct > > > > intel_fbdev *ifbdev) > > > > intel_frontbuffer_invalidate(to_frontbuffer(ifbdev), > > > > ORIGIN_CPU); > > > > } > > > > > > > > +static void intel_fbdev_dirty(struct fb_info *info) > > > > +{ > > > > + struct drm_fb_helper *helper = info->par; > > > > + > > > > + /* > > > > + * Intel_fb dirty implementation doesn't use damage > > > > clips - > > > > > > > > > + * no need to pass them here > > > > + */ > > > > + if (helper->fb->funcs->dirty) > > > > + helper->fb->funcs->dirty(helper->fb, NULL, 0, > > > > 0, > > > > NULL, 0); > > > > +} > > > > + > > > > static int intel_fbdev_set_par(struct fb_info *info) > > > > { > > > > struct drm_fb_helper *fb_helper = info->par; > > > > @@ -91,6 +103,39 @@ static int intel_fbdev_set_par(struct > > > > fb_info > > > > *info) > > > > return ret; > > > > } > > > > > > > > +static ssize_t intel_fbdev_write(struct fb_info *info, const > > > > char > > > > __user *buf, > > > > + size_t count, loff_t *ppos) > > > > +{ > > > > + int ret; > > > > + > > > > + ret = drm_fb_helper_cfb_write(info, buf, count, ppos); > > > > + if (ret > 0) > > > > + intel_fbdev_dirty(info); > > > > + > > > > + return ret; > > > > +} > > > > + > > > > +static void intel_fbdev_fillrect(struct fb_info *info, > > > > + const struct fb_fillrect > > > > *rect) > > > > +{ > > > > + drm_fb_helper_cfb_fillrect(info, rect); > > > > + intel_fbdev_dirty(info); > > > > +} > > > > + > > > > +static void intel_fbdev_copyarea(struct fb_info *info, > > > > + const struct fb_copyarea > > > > *area) > > > > +{ > > > > + drm_fb_helper_cfb_copyarea(info, area); > > > > + intel_fbdev_dirty(info); > > > > +} > > > > + > > > > +static void intel_fbdev_imageblit(struct fb_info *info, > > > > + const struct fb_image *image) > > > > +{ > > > > + drm_fb_helper_cfb_imageblit(info, image); > > > > + intel_fbdev_dirty(info); > > > > +} > > > > + > > > > static int intel_fbdev_blank(int blank, struct fb_info *info) > > > > { > > > > struct drm_fb_helper *fb_helper = info->par; > > > > @@ -125,10 +170,10 @@ static const struct fb_ops intelfb_ops = > > > > { > > > > DRM_FB_HELPER_DEFAULT_OPS, > > > > .fb_set_par = intel_fbdev_set_par, > > > > .fb_read = drm_fb_helper_cfb_read, > > > > - .fb_write = drm_fb_helper_cfb_write, > > > > - .fb_fillrect = drm_fb_helper_cfb_fillrect, > > > > - .fb_copyarea = drm_fb_helper_cfb_copyarea, > > > > - .fb_imageblit = drm_fb_helper_cfb_imageblit, > > > > + .fb_write = intel_fbdev_write, > > > > + .fb_fillrect = intel_fbdev_fillrect, > > > > + .fb_copyarea = intel_fbdev_copyarea, > > > > + .fb_imageblit = intel_fbdev_imageblit, > > > > .fb_pan_display = intel_fbdev_pan_display, > > > > .fb_blank = intel_fbdev_blank, > > > > }; > > > > > > -- > > > 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 > > >
Hi Am 03.02.23 um 12:09 schrieb Hogander, Jouni: > On Fri, 2023-02-03 at 12:42 +0200, Ville Syrjälä wrote: >> On Fri, Feb 03, 2023 at 07:21:27AM +0000, Hogander, Jouni wrote: >>> On Tue, 2023-01-24 at 13:27 +0100, Thomas Zimmermann wrote: >>>> Hi >>>> >>>> Am 24.01.23 um 10:10 schrieb Jouni Högander: >>>>> After disconnecting damage worker from update logic our dirty >>>>> callback >>>>> is not called on fbcon events. This is causing problems to >>>>> features >>>>> (PSR, FBC, DRRS) relying on dirty callback getting called and >>>>> breaking >>>>> fb console when these features are in use. >>>>> >>>>> Implement wrappers for callbacks used by fbcon and call our >>>>> dirty >>>>> callback in those. >>>>> >>>>> Fixes: f231af498c29 ("drm/fb-helper: Disconnect damage worker >>>>> from >>>>> update logic") >>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de> >>>>> Cc: Jani Nikula <jani.nikula@intel.com> >>>>> Signed-off-by: Jouni Högander <jouni.hogander@intel.com> >>>> >>>> This is the better solution wrt what fbdev wants. >>> >>> There was a failure from testing robot. drivers/tty/vt/vt.c is >>> using >>> spinlock and in our dirty callback we are taking mutex. I think I've seen this problem before in the context of vc4. We'd probably have to add a damage worker to i915, which seems a bit much for this fix. >>> >>> Do you have any suggestions? Shall we fallback to original fix >>> which >>> was setting the dirty callback where call is made from workqueue? >> >> Please just fix the original regression as straightforwardly as >> possible. > > Here is the original patch by me: > > https://patchwork.freedesktop.org/series/111433/ Given the circumstances, Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> for this older patch. Maybe add a TODO comment that says that the fb_dirty worker needs to be moved into i915 and that the fb_ write ops need to be reimplemented for a good solution. Best regards Thomas > > which was eventually nacked by Thomas. Maybe we could continue with > that one and solve these problems when/if fb_dirty is removed? > >> >>> >>>> >>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de> >>>> >>>> Best regards >>>> Thomas >>>> >>>>> --- >>>>> drivers/gpu/drm/i915/display/intel_fbdev.c | 53 >>>>> ++++++++++++++++++++-- >>>>> 1 file changed, 49 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c >>>>> b/drivers/gpu/drm/i915/display/intel_fbdev.c >>>>> index 19f3b5d92a55..b1653624552e 100644 >>>>> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c >>>>> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c >>>>> @@ -77,6 +77,18 @@ static void intel_fbdev_invalidate(struct >>>>> intel_fbdev *ifbdev) >>>>> intel_frontbuffer_invalidate(to_frontbuffer(ifbdev), >>>>> ORIGIN_CPU); >>>>> } >>>>> >>>>> +static void intel_fbdev_dirty(struct fb_info *info) >>>>> +{ >>>>> + struct drm_fb_helper *helper = info->par; >>>>> + >>>>> + /* >>>>> + * Intel_fb dirty implementation doesn't use damage >>>>> clips - >>>>>> >>>>> + * no need to pass them here >>>>> + */ >>>>> + if (helper->fb->funcs->dirty) >>>>> + helper->fb->funcs->dirty(helper->fb, NULL, 0, >>>>> 0, >>>>> NULL, 0); >>>>> +} >>>>> + >>>>> static int intel_fbdev_set_par(struct fb_info *info) >>>>> { >>>>> struct drm_fb_helper *fb_helper = info->par; >>>>> @@ -91,6 +103,39 @@ static int intel_fbdev_set_par(struct >>>>> fb_info >>>>> *info) >>>>> return ret; >>>>> } >>>>> >>>>> +static ssize_t intel_fbdev_write(struct fb_info *info, const >>>>> char >>>>> __user *buf, >>>>> + size_t count, loff_t *ppos) >>>>> +{ >>>>> + int ret; >>>>> + >>>>> + ret = drm_fb_helper_cfb_write(info, buf, count, ppos); >>>>> + if (ret > 0) >>>>> + intel_fbdev_dirty(info); >>>>> + >>>>> + return ret; >>>>> +} >>>>> + >>>>> +static void intel_fbdev_fillrect(struct fb_info *info, >>>>> + const struct fb_fillrect >>>>> *rect) >>>>> +{ >>>>> + drm_fb_helper_cfb_fillrect(info, rect); >>>>> + intel_fbdev_dirty(info); >>>>> +} >>>>> + >>>>> +static void intel_fbdev_copyarea(struct fb_info *info, >>>>> + const struct fb_copyarea >>>>> *area) >>>>> +{ >>>>> + drm_fb_helper_cfb_copyarea(info, area); >>>>> + intel_fbdev_dirty(info); >>>>> +} >>>>> + >>>>> +static void intel_fbdev_imageblit(struct fb_info *info, >>>>> + const struct fb_image *image) >>>>> +{ >>>>> + drm_fb_helper_cfb_imageblit(info, image); >>>>> + intel_fbdev_dirty(info); >>>>> +} >>>>> + >>>>> static int intel_fbdev_blank(int blank, struct fb_info *info) >>>>> { >>>>> struct drm_fb_helper *fb_helper = info->par; >>>>> @@ -125,10 +170,10 @@ static const struct fb_ops intelfb_ops = >>>>> { >>>>> DRM_FB_HELPER_DEFAULT_OPS, >>>>> .fb_set_par = intel_fbdev_set_par, >>>>> .fb_read = drm_fb_helper_cfb_read, >>>>> - .fb_write = drm_fb_helper_cfb_write, >>>>> - .fb_fillrect = drm_fb_helper_cfb_fillrect, >>>>> - .fb_copyarea = drm_fb_helper_cfb_copyarea, >>>>> - .fb_imageblit = drm_fb_helper_cfb_imageblit, >>>>> + .fb_write = intel_fbdev_write, >>>>> + .fb_fillrect = intel_fbdev_fillrect, >>>>> + .fb_copyarea = intel_fbdev_copyarea, >>>>> + .fb_imageblit = intel_fbdev_imageblit, >>>>> .fb_pan_display = intel_fbdev_pan_display, >>>>> .fb_blank = intel_fbdev_blank, >>>>> }; >>>> >>>> -- >>>> 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 >>> >> >
diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c index 19f3b5d92a55..b1653624552e 100644 --- a/drivers/gpu/drm/i915/display/intel_fbdev.c +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c @@ -77,6 +77,18 @@ static void intel_fbdev_invalidate(struct intel_fbdev *ifbdev) intel_frontbuffer_invalidate(to_frontbuffer(ifbdev), ORIGIN_CPU); } +static void intel_fbdev_dirty(struct fb_info *info) +{ + struct drm_fb_helper *helper = info->par; + + /* + * Intel_fb dirty implementation doesn't use damage clips -> + * no need to pass them here + */ + if (helper->fb->funcs->dirty) + helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, NULL, 0); +} + static int intel_fbdev_set_par(struct fb_info *info) { struct drm_fb_helper *fb_helper = info->par; @@ -91,6 +103,39 @@ static int intel_fbdev_set_par(struct fb_info *info) return ret; } +static ssize_t intel_fbdev_write(struct fb_info *info, const char __user *buf, + size_t count, loff_t *ppos) +{ + int ret; + + ret = drm_fb_helper_cfb_write(info, buf, count, ppos); + if (ret > 0) + intel_fbdev_dirty(info); + + return ret; +} + +static void intel_fbdev_fillrect(struct fb_info *info, + const struct fb_fillrect *rect) +{ + drm_fb_helper_cfb_fillrect(info, rect); + intel_fbdev_dirty(info); +} + +static void intel_fbdev_copyarea(struct fb_info *info, + const struct fb_copyarea *area) +{ + drm_fb_helper_cfb_copyarea(info, area); + intel_fbdev_dirty(info); +} + +static void intel_fbdev_imageblit(struct fb_info *info, + const struct fb_image *image) +{ + drm_fb_helper_cfb_imageblit(info, image); + intel_fbdev_dirty(info); +} + static int intel_fbdev_blank(int blank, struct fb_info *info) { struct drm_fb_helper *fb_helper = info->par; @@ -125,10 +170,10 @@ static const struct fb_ops intelfb_ops = { DRM_FB_HELPER_DEFAULT_OPS, .fb_set_par = intel_fbdev_set_par, .fb_read = drm_fb_helper_cfb_read, - .fb_write = drm_fb_helper_cfb_write, - .fb_fillrect = drm_fb_helper_cfb_fillrect, - .fb_copyarea = drm_fb_helper_cfb_copyarea, - .fb_imageblit = drm_fb_helper_cfb_imageblit, + .fb_write = intel_fbdev_write, + .fb_fillrect = intel_fbdev_fillrect, + .fb_copyarea = intel_fbdev_copyarea, + .fb_imageblit = intel_fbdev_imageblit, .fb_pan_display = intel_fbdev_pan_display, .fb_blank = intel_fbdev_blank, };
After disconnecting damage worker from update logic our dirty callback is not called on fbcon events. This is causing problems to features (PSR, FBC, DRRS) relying on dirty callback getting called and breaking fb console when these features are in use. Implement wrappers for callbacks used by fbcon and call our dirty callback in those. Fixes: f231af498c29 ("drm/fb-helper: Disconnect damage worker from update logic") Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: Jani Nikula <jani.nikula@intel.com> Signed-off-by: Jouni Högander <jouni.hogander@intel.com> --- drivers/gpu/drm/i915/display/intel_fbdev.c | 53 ++++++++++++++++++++-- 1 file changed, 49 insertions(+), 4 deletions(-)