| Message ID | 20190111201206.45018-5-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/tinydrm: Use damage helper for dirtyfb | expand |
On 1/11/19 2:12 PM, Noralf Trønnes wrote: > This switches to drm_atomic_helper_dirtyfb() as the framebuffer dirty > handler. All flushing will now happen in the pipe functions. > > Also enable the damage plane property for all except repaper which can > only do full updates. > > ili9225: > This change made ili9225_init() equal to mipi_dbi_init() so use it. > > v2: Remove fb check in mipi_dbi_enable_flush() it can't be NULL > (kbuild test robot) > > Cc: David Lechner <david@lechnology.com> > Cc: Eric Anholt <eric@anholt.net> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > Acked-by: Sam Ravnborg <sam@ravnborg.org> > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- Tested-by: David Lechner <david@lechnology.com> Reviewed-by: David Lechner <david@lechnology.com> Tested with ST7586 on LEGO MINDSTORMS EV3 and ILI9225 on BeagleBone Green. (FWIW, I encounter other issues that had to be fixed to make things work on both of these, but none were actually related to this series.)
On 1/13/19 3:19 PM, David Lechner wrote: > On 1/11/19 2:12 PM, Noralf Trønnes wrote: >> This switches to drm_atomic_helper_dirtyfb() as the framebuffer dirty >> handler. All flushing will now happen in the pipe functions. >> >> Also enable the damage plane property for all except repaper which can >> only do full updates. >> >> ili9225: >> This change made ili9225_init() equal to mipi_dbi_init() so use it. >> >> v2: Remove fb check in mipi_dbi_enable_flush() it can't be NULL >> (kbuild test robot) >> >> Cc: David Lechner <david@lechnology.com> >> Cc: Eric Anholt <eric@anholt.net> >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> >> Acked-by: Sam Ravnborg <sam@ravnborg.org> >> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> --- > > Tested-by: David Lechner <david@lechnology.com> > Reviewed-by: David Lechner <david@lechnology.com> > > Tested with ST7586 on LEGO MINDSTORMS EV3 and ILI9225 on > BeagleBone Green. (FWIW, I encounter other issues that had to > be fixed to make things work on both of these, but none were > actually related to this series.) > Actually, I have to take this back. It appears that the problems that I had on LEGO MINDSTORMS EV3 are actually caused by this series. I did a git bisect to be sure and ended up with this patch as the bad commit. I'm guessing that the damage helper must be causing some kind of memory corruption. Here is the crash I am getting: Unable to handle kernel NULL pointer dereference at virtual address 00000004 pgd = (ptrval) [00000004] *pgd=00000000 Internal error: Oops: 5 [#1] PREEMPT ARM Modules linked in: CPU: 0 PID: 1 Comm: swapper Not tainted 5.0.0-rc1-00125-ge9acadba5409 #1389 Hardware name: Generic DA850/OMAP-L138/AM18x PC is at rb_insert_color+0x1c/0x1a4 LR is at kernfs_link_sibling+0x94/0xcc pc : [<c04b95ec>] lr : [<c014bfdc>] psr: 60000013 sp : c2831b38 ip : 00000000 fp : c06b762c r10: 00000000 r9 : c06b835c r8 : 00000000 r7 : c2963f00 r6 : c066b028 r5 : c2016cc0 r4 : 00000000 r3 : 00000000 r2 : c2983010 r1 : c2963f2c r0 : c2016cd0 Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none Control: 0005317f Table: c0004000 DAC: 00000053 Process swapper (pid: 1, stack limit = 0x(ptrval)) Stack: (0xc2831b38 to 0xc2832000) 1b20: 00000000 c2016cc0 1b40: c066b028 c014bfdc c2016cc0 c2963f00 c066b028 c014c768 c2963f00 c014c448 1b60: 00000000 c2016cc0 c2963f00 c066b028 c2963f00 c014c860 00000000 00000001 1b80: c0589938 c2b4b408 00000000 c014ec70 00000000 c2b4b408 00000000 c04c4cb0 1ba0: 0000070f 00000000 00000000 9fd04dd9 00000000 c2b4b408 c066b028 00000000 1bc0: c293ac98 c04b58f8 c059081c 00000000 c2b4b408 c066b028 00000000 00000000 1be0: 00000000 c04b5d64 c2831c08 9fd04dd9 c2b4b3c0 c293ac80 c2bd16c0 c2b4b408 1c00: c00d650c c059081c c2bd16c0 c28e3a80 c2b4b3c0 00000000 c2b4b3c0 00000000 1c20: c06b7634 00020000 c06b835c c00d72f8 00000000 c00b0c24 00000000 00000074 1c40: c0590728 00000000 c0590728 c2b4b3c0 00000074 c0590728 00020000 00000000 1c60: c06b762c c00b0958 00000000 00380bc6 00000000 c06bbf1c c2bd9c00 c2bfe000 1c80: c06bbf14 0000000c 00000000 00000000 c2831e0c c00b0a90 00000000 00000000 1ca0: 00000000 00000000 003b2580 c017d6e4 00000000 00000000 00000000 c2bfe000 1cc0: c2bd9c00 00000000 003b2580 00000000 00000000 c01971a0 00001103 00000000 1ce0: c2bd8400 00000400 00000400 9fd04dd9 00000000 0000000a 00000002 00000000 1d00: ffffffff 00000000 ffffffff 00000000 00000000 00000001 00000a04 00000032 1d20: 00000000 0000000c 00000004 c00af550 c2bd9ecc 9fd04dd9 c2404480 c2bd9eb4 1d40: c2bd8400 c2404480 00000002 00000001 00000000 00000000 00000001 00000000 1d60: 00000000 00000000 00000000 00000000 00000000 00000000 00000001 00000000 1d80: 00380bc6 00000000 003b257f 00000000 00000077 0000ffff 00000200 00000002 1da0: 00000001 0000ffff 00000000 00000401 c2bfe22c 00000000 00000000 00000000 1dc0: 00000000 00000000 c2012400 c24be100 00001000 00000000 c2404480 00000000 1de0: 00004003 9fd04dd9 00000000 c2bd9c00 c2404480 00000083 c24044fc 00008000 1e00: 00000000 00000020 00008000 c00e4d88 c2404480 00000000 00000000 c0190afc 1e20: c2bd0be0 c0692898 c0692898 00000000 00000020 c0190b10 c0194f48 c2bd0be0 1e40: c066b370 c00e568c c29f5380 c01002d4 c29f5380 c2bd0be0 00008000 c0100488 1e60: c0692898 00000000 c066b028 c2bd0be0 c2bd0bc0 c01033ec 00000000 ffffff00 1e80: ffff0a00 c0575ff4 c2bd0be0 c281a010 c2417098 c2bd0be0 0000000a 00000000 1ea0: 0000000a 9fd04dd9 0000000a c2bd0be0 c2bd0bc0 00000000 c0575ff8 00008000 1ec0: c066b028 00008000 c0575ff4 c0104178 00000000 00000000 c2014000 c2014005 1ee0: c0575ff8 c3fb1280 c0652868 c062b1ec 00000000 c01013d8 c24c3558 00000000 1f00: 00000000 00006180 c24c3558 c00f17c4 e10c76ac 0b300002 c2858015 c281a010 1f20: c2415da8 9fd04dd9 00000000 00000002 c06ad310 c066b028 c066da68 00000000 1f40: 00000000 00000000 00000000 c062b478 00000000 c0037200 00306b6c 9fd00000 1f60: c0576098 9fd04dd9 00000002 c06ad310 c0652878 00000000 00000000 00000000 1f80: 00000000 00000000 00000000 c062b5e4 00000000 00000000 00000000 00000000 1fa0: c04c7860 c04c7868 00000000 c00090e0 00000000 00000000 00000000 00000000 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000 [<c04b95ec>] (rb_insert_color) from [<c014bfdc>] (kernfs_link_sibling+0x94/0xcc) [<c014bfdc>] (kernfs_link_sibling) from [<c014c768>] (kernfs_add_one+0x90/0x140) [<c014c768>] (kernfs_add_one) from [<c014c860>] (kernfs_create_dir_ns+0x48/0x74) [<c014c860>] (kernfs_create_dir_ns) from [<c014ec70>] (sysfs_create_dir_ns+0x68/0xd0) [<c014ec70>] (sysfs_create_dir_ns) from [<c04b58f8>] (kobject_add_internal+0x9c/0x2c4) [<c04b58f8>] (kobject_add_internal) from [<c04b5d64>] (kobject_init_and_add+0x54/0x94) [<c04b5d64>] (kobject_init_and_add) from [<c00d650c>] (sysfs_slab_add+0x10c/0x220) [<c00d650c>] (sysfs_slab_add) from [<c00d72f8>] (__kmem_cache_create+0x1d8/0x338) [<c00d72f8>] (__kmem_cache_create) from [<c00b0958>] (kmem_cache_create_usercopy+0x180/0x298) [<c00b0958>] (kmem_cache_create_usercopy) from [<c00b0a90>] (kmem_cache_create+0x20/0x28) [<c00b0a90>] (kmem_cache_create) from [<c017d6e4>] (ext4_mb_init+0x374/0x44c) [<c017d6e4>] (ext4_mb_init) from [<c01971a0>] (ext4_fill_super+0x2258/0x2ef0) [<c01971a0>] (ext4_fill_super) from [<c00e4d88>] (mount_bdev+0x154/0x18c) [<c00e4d88>] (mount_bdev) from [<c0190b10>] (ext4_mount+0x14/0x20) [<c0190b10>] (ext4_mount) from [<c00e568c>] (mount_fs+0x14/0xa8) [<c00e568c>] (mount_fs) from [<c0100488>] (vfs_kern_mount+0x48/0xf0) [<c0100488>] (vfs_kern_mount) from [<c01033ec>] (do_mount+0x180/0xb9c) [<c01033ec>] (do_mount) from [<c0104178>] (ksys_mount+0x8c/0xb4) [<c0104178>] (ksys_mount) from [<c062b1ec>] (mount_block_root+0x128/0x2a4) [<c062b1ec>] (mount_block_root) from [<c062b478>] (mount_root+0x110/0x154) [<c062b478>] (mount_root) from [<c062b5e4>] (prepare_namespace+0x128/0x188) [<c062b5e4>] (prepare_namespace) from [<c04c7868>] (kernel_init+0x8/0xf4) [<c04c7868>] (kernel_init) from [<c00090e0>] (ret_from_fork+0x14/0x34) Exception stack(0xc2831fb0 to 0xc2831ff8) 1fa0: 00000000 00000000 00000000 00000000 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 Code: e5923000 e3130001 1a000054 e92d4070 (e593c004) ---[ end trace 2c8fc104914a532b ]--- Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ]---
Den 14.01.2019 03.15, skrev David Lechner: > On 1/13/19 3:19 PM, David Lechner wrote: >> On 1/11/19 2:12 PM, Noralf Trønnes wrote: >>> This switches to drm_atomic_helper_dirtyfb() as the framebuffer dirty >>> handler. All flushing will now happen in the pipe functions. >>> >>> Also enable the damage plane property for all except repaper which can >>> only do full updates. >>> >>> ili9225: >>> This change made ili9225_init() equal to mipi_dbi_init() so use it. >>> >>> v2: Remove fb check in mipi_dbi_enable_flush() it can't be NULL >>> (kbuild test robot) >>> >>> Cc: David Lechner <david@lechnology.com> >>> Cc: Eric Anholt <eric@anholt.net> >>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> >>> Acked-by: Sam Ravnborg <sam@ravnborg.org> >>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> >>> --- >> >> Tested-by: David Lechner <david@lechnology.com> >> Reviewed-by: David Lechner <david@lechnology.com> >> >> Tested with ST7586 on LEGO MINDSTORMS EV3 and ILI9225 on >> BeagleBone Green. (FWIW, I encounter other issues that had to >> be fixed to make things work on both of these, but none were >> actually related to this series.) >> > > Actually, I have to take this back. It appears that the problems > that I had on LEGO MINDSTORMS EV3 are actually caused by this > series. I did a git bisect to be sure and ended up with this > patch as the bad commit. I'm guessing that the damage helper > must be causing some kind of memory corruption. Here is the > crash I am getting: > > Unable to handle kernel NULL pointer dereference at virtual address > 00000004 > pgd = (ptrval) > [00000004] *pgd=00000000 > Internal error: Oops: 5 [#1] PREEMPT ARM > Modules linked in: > CPU: 0 PID: 1 Comm: swapper Not tainted 5.0.0-rc1-00125-ge9acadba5409 #1389 > Hardware name: Generic DA850/OMAP-L138/AM18x > PC is at rb_insert_color+0x1c/0x1a4 > LR is at kernfs_link_sibling+0x94/0xcc > pc : [<c04b95ec>] lr : [<c014bfdc>] psr: 60000013 > sp : c2831b38 ip : 00000000 fp : c06b762c > r10: 00000000 r9 : c06b835c r8 : 00000000 > r7 : c2963f00 r6 : c066b028 r5 : c2016cc0 r4 : 00000000 > r3 : 00000000 r2 : c2983010 r1 : c2963f2c r0 : c2016cd0 > Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none > Control: 0005317f Table: c0004000 DAC: 00000053 > Process swapper (pid: 1, stack limit = 0x(ptrval)) > Stack: (0xc2831b38 to 0xc2832000) > 1b20: 00000000 > c2016cc0 > 1b40: c066b028 c014bfdc c2016cc0 c2963f00 c066b028 c014c768 c2963f00 > c014c448 > 1b60: 00000000 c2016cc0 c2963f00 c066b028 c2963f00 c014c860 00000000 > 00000001 > 1b80: c0589938 c2b4b408 00000000 c014ec70 00000000 c2b4b408 00000000 > c04c4cb0 > 1ba0: 0000070f 00000000 00000000 9fd04dd9 00000000 c2b4b408 c066b028 > 00000000 > 1bc0: c293ac98 c04b58f8 c059081c 00000000 c2b4b408 c066b028 00000000 > 00000000 > 1be0: 00000000 c04b5d64 c2831c08 9fd04dd9 c2b4b3c0 c293ac80 c2bd16c0 > c2b4b408 > 1c00: c00d650c c059081c c2bd16c0 c28e3a80 c2b4b3c0 00000000 c2b4b3c0 > 00000000 > 1c20: c06b7634 00020000 c06b835c c00d72f8 00000000 c00b0c24 00000000 > 00000074 > 1c40: c0590728 00000000 c0590728 c2b4b3c0 00000074 c0590728 00020000 > 00000000 > 1c60: c06b762c c00b0958 00000000 00380bc6 00000000 c06bbf1c c2bd9c00 > c2bfe000 > 1c80: c06bbf14 0000000c 00000000 00000000 c2831e0c c00b0a90 00000000 > 00000000 > 1ca0: 00000000 00000000 003b2580 c017d6e4 00000000 00000000 00000000 > c2bfe000 > 1cc0: c2bd9c00 00000000 003b2580 00000000 00000000 c01971a0 00001103 > 00000000 > 1ce0: c2bd8400 00000400 00000400 9fd04dd9 00000000 0000000a 00000002 > 00000000 > 1d00: ffffffff 00000000 ffffffff 00000000 00000000 00000001 00000a04 > 00000032 > 1d20: 00000000 0000000c 00000004 c00af550 c2bd9ecc 9fd04dd9 c2404480 > c2bd9eb4 > 1d40: c2bd8400 c2404480 00000002 00000001 00000000 00000000 00000001 > 00000000 > 1d60: 00000000 00000000 00000000 00000000 00000000 00000000 00000001 > 00000000 > 1d80: 00380bc6 00000000 003b257f 00000000 00000077 0000ffff 00000200 > 00000002 > 1da0: 00000001 0000ffff 00000000 00000401 c2bfe22c 00000000 00000000 > 00000000 > 1dc0: 00000000 00000000 c2012400 c24be100 00001000 00000000 c2404480 > 00000000 > 1de0: 00004003 9fd04dd9 00000000 c2bd9c00 c2404480 00000083 c24044fc > 00008000 > 1e00: 00000000 00000020 00008000 c00e4d88 c2404480 00000000 00000000 > c0190afc > 1e20: c2bd0be0 c0692898 c0692898 00000000 00000020 c0190b10 c0194f48 > c2bd0be0 > 1e40: c066b370 c00e568c c29f5380 c01002d4 c29f5380 c2bd0be0 00008000 > c0100488 > 1e60: c0692898 00000000 c066b028 c2bd0be0 c2bd0bc0 c01033ec 00000000 > ffffff00 > 1e80: ffff0a00 c0575ff4 c2bd0be0 c281a010 c2417098 c2bd0be0 0000000a > 00000000 > 1ea0: 0000000a 9fd04dd9 0000000a c2bd0be0 c2bd0bc0 00000000 c0575ff8 > 00008000 > 1ec0: c066b028 00008000 c0575ff4 c0104178 00000000 00000000 c2014000 > c2014005 > 1ee0: c0575ff8 c3fb1280 c0652868 c062b1ec 00000000 c01013d8 c24c3558 > 00000000 > 1f00: 00000000 00006180 c24c3558 c00f17c4 e10c76ac 0b300002 c2858015 > c281a010 > 1f20: c2415da8 9fd04dd9 00000000 00000002 c06ad310 c066b028 c066da68 > 00000000 > 1f40: 00000000 00000000 00000000 c062b478 00000000 c0037200 00306b6c > 9fd00000 > 1f60: c0576098 9fd04dd9 00000002 c06ad310 c0652878 00000000 00000000 > 00000000 > 1f80: 00000000 00000000 00000000 c062b5e4 00000000 00000000 00000000 > 00000000 > 1fa0: c04c7860 c04c7868 00000000 c00090e0 00000000 00000000 00000000 > 00000000 > 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 > 00000000 > 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 > 00000000 > [<c04b95ec>] (rb_insert_color) from [<c014bfdc>] > (kernfs_link_sibling+0x94/0xcc) > [<c014bfdc>] (kernfs_link_sibling) from [<c014c768>] > (kernfs_add_one+0x90/0x140) > [<c014c768>] (kernfs_add_one) from [<c014c860>] > (kernfs_create_dir_ns+0x48/0x74) > [<c014c860>] (kernfs_create_dir_ns) from [<c014ec70>] > (sysfs_create_dir_ns+0x68/0xd0) > [<c014ec70>] (sysfs_create_dir_ns) from [<c04b58f8>] > (kobject_add_internal+0x9c/0x2c4) > [<c04b58f8>] (kobject_add_internal) from [<c04b5d64>] > (kobject_init_and_add+0x54/0x94) > [<c04b5d64>] (kobject_init_and_add) from [<c00d650c>] > (sysfs_slab_add+0x10c/0x220) > [<c00d650c>] (sysfs_slab_add) from [<c00d72f8>] > (__kmem_cache_create+0x1d8/0x338) > [<c00d72f8>] (__kmem_cache_create) from [<c00b0958>] > (kmem_cache_create_usercopy+0x180/0x298) > [<c00b0958>] (kmem_cache_create_usercopy) from [<c00b0a90>] > (kmem_cache_create+0x20/0x28) > [<c00b0a90>] (kmem_cache_create) from [<c017d6e4>] > (ext4_mb_init+0x374/0x44c) > [<c017d6e4>] (ext4_mb_init) from [<c01971a0>] > (ext4_fill_super+0x2258/0x2ef0) > [<c01971a0>] (ext4_fill_super) from [<c00e4d88>] (mount_bdev+0x154/0x18c) > [<c00e4d88>] (mount_bdev) from [<c0190b10>] (ext4_mount+0x14/0x20) > [<c0190b10>] (ext4_mount) from [<c00e568c>] (mount_fs+0x14/0xa8) > [<c00e568c>] (mount_fs) from [<c0100488>] (vfs_kern_mount+0x48/0xf0) > [<c0100488>] (vfs_kern_mount) from [<c01033ec>] (do_mount+0x180/0xb9c) > [<c01033ec>] (do_mount) from [<c0104178>] (ksys_mount+0x8c/0xb4) > [<c0104178>] (ksys_mount) from [<c062b1ec>] (mount_block_root+0x128/0x2a4) > [<c062b1ec>] (mount_block_root) from [<c062b478>] (mount_root+0x110/0x154) > [<c062b478>] (mount_root) from [<c062b5e4>] (prepare_namespace+0x128/0x188) > [<c062b5e4>] (prepare_namespace) from [<c04c7868>] (kernel_init+0x8/0xf4) > [<c04c7868>] (kernel_init) from [<c00090e0>] (ret_from_fork+0x14/0x34) > Exception stack(0xc2831fb0 to 0xc2831ff8) > 1fa0: 00000000 00000000 00000000 > 00000000 > 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 > 00000000 > 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 > Code: e5923000 e3130001 1a000054 e92d4070 (e593c004) > ---[ end trace 2c8fc104914a532b ]--- > Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b > ---[ end Kernel panic - not syncing: Attempted to kill init! > exitcode=0x0000000b ]--- > First step is to test if it's the damage clip allocation/freeing that's causing problems. Setting clips to NULL avoids allocation and results in a full update each time: diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c index e16aa5ae00b4..943ae17b8b1c 100644 --- a/drivers/gpu/drm/drm_damage_helper.c +++ b/drivers/gpu/drm/drm_damage_helper.c @@ -182,6 +182,7 @@ int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb, } state->acquire_ctx = &ctx; + clips = NULL; if (clips) { uint32_t inc = 1; If that helps, try clearing/freeing the damage explicitly: diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c index e16aa5ae00b4..0a59263e4733 100644 --- a/drivers/gpu/drm/drm_damage_helper.c +++ b/drivers/gpu/drm/drm_damage_helper.c @@ -371,6 +371,8 @@ bool drm_atomic_helper_damage_merged(const struct drm_plane_state *old_state, valid = true; } + drm_property_replace_blob(&state->fb_damage_clips, NULL); + return valid; } EXPORT_SYMBOL(drm_atomic_helper_damage_merged); If that doesn't help, try disabling the call to st7586_fb_dirty() so we can rule out the driver. If it helps, verify that the rectangle is within bounds. Noralf.
Den 14.01.2019 16.00, skrev Noralf Trønnes: > > > Den 14.01.2019 03.15, skrev David Lechner: >> On 1/13/19 3:19 PM, David Lechner wrote: >>> On 1/11/19 2:12 PM, Noralf Trønnes wrote: >>>> This switches to drm_atomic_helper_dirtyfb() as the framebuffer dirty >>>> handler. All flushing will now happen in the pipe functions. >>>> >>>> Also enable the damage plane property for all except repaper which can >>>> only do full updates. >>>> >>>> ili9225: >>>> This change made ili9225_init() equal to mipi_dbi_init() so use it. >>>> >>>> v2: Remove fb check in mipi_dbi_enable_flush() it can't be NULL >>>> (kbuild test robot) >>>> >>>> Cc: David Lechner <david@lechnology.com> >>>> Cc: Eric Anholt <eric@anholt.net> >>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> >>>> Acked-by: Sam Ravnborg <sam@ravnborg.org> >>>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> >>>> --- >>> >>> Tested-by: David Lechner <david@lechnology.com> >>> Reviewed-by: David Lechner <david@lechnology.com> >>> >>> Tested with ST7586 on LEGO MINDSTORMS EV3 and ILI9225 on >>> BeagleBone Green. (FWIW, I encounter other issues that had to >>> be fixed to make things work on both of these, but none were >>> actually related to this series.) >>> >> >> Actually, I have to take this back. It appears that the problems >> that I had on LEGO MINDSTORMS EV3 are actually caused by this >> series. I did a git bisect to be sure and ended up with this >> patch as the bad commit. I'm guessing that the damage helper >> must be causing some kind of memory corruption. Here is the >> crash I am getting: >> >> Unable to handle kernel NULL pointer dereference at virtual address >> 00000004 >> pgd = (ptrval) >> [00000004] *pgd=00000000 >> Internal error: Oops: 5 [#1] PREEMPT ARM >> Modules linked in: >> CPU: 0 PID: 1 Comm: swapper Not tainted 5.0.0-rc1-00125-ge9acadba5409 #1389 >> Hardware name: Generic DA850/OMAP-L138/AM18x >> PC is at rb_insert_color+0x1c/0x1a4 >> LR is at kernfs_link_sibling+0x94/0xcc >> pc : [<c04b95ec>] lr : [<c014bfdc>] psr: 60000013 >> sp : c2831b38 ip : 00000000 fp : c06b762c >> r10: 00000000 r9 : c06b835c r8 : 00000000 >> r7 : c2963f00 r6 : c066b028 r5 : c2016cc0 r4 : 00000000 >> r3 : 00000000 r2 : c2983010 r1 : c2963f2c r0 : c2016cd0 >> Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none >> Control: 0005317f Table: c0004000 DAC: 00000053 >> Process swapper (pid: 1, stack limit = 0x(ptrval)) >> Stack: (0xc2831b38 to 0xc2832000) >> 1b20: 00000000 >> c2016cc0 >> 1b40: c066b028 c014bfdc c2016cc0 c2963f00 c066b028 c014c768 c2963f00 >> c014c448 >> 1b60: 00000000 c2016cc0 c2963f00 c066b028 c2963f00 c014c860 00000000 >> 00000001 >> 1b80: c0589938 c2b4b408 00000000 c014ec70 00000000 c2b4b408 00000000 >> c04c4cb0 >> 1ba0: 0000070f 00000000 00000000 9fd04dd9 00000000 c2b4b408 c066b028 >> 00000000 >> 1bc0: c293ac98 c04b58f8 c059081c 00000000 c2b4b408 c066b028 00000000 >> 00000000 >> 1be0: 00000000 c04b5d64 c2831c08 9fd04dd9 c2b4b3c0 c293ac80 c2bd16c0 >> c2b4b408 >> 1c00: c00d650c c059081c c2bd16c0 c28e3a80 c2b4b3c0 00000000 c2b4b3c0 >> 00000000 >> 1c20: c06b7634 00020000 c06b835c c00d72f8 00000000 c00b0c24 00000000 >> 00000074 >> 1c40: c0590728 00000000 c0590728 c2b4b3c0 00000074 c0590728 00020000 >> 00000000 >> 1c60: c06b762c c00b0958 00000000 00380bc6 00000000 c06bbf1c c2bd9c00 >> c2bfe000 >> 1c80: c06bbf14 0000000c 00000000 00000000 c2831e0c c00b0a90 00000000 >> 00000000 >> 1ca0: 00000000 00000000 003b2580 c017d6e4 00000000 00000000 00000000 >> c2bfe000 >> 1cc0: c2bd9c00 00000000 003b2580 00000000 00000000 c01971a0 00001103 >> 00000000 >> 1ce0: c2bd8400 00000400 00000400 9fd04dd9 00000000 0000000a 00000002 >> 00000000 >> 1d00: ffffffff 00000000 ffffffff 00000000 00000000 00000001 00000a04 >> 00000032 >> 1d20: 00000000 0000000c 00000004 c00af550 c2bd9ecc 9fd04dd9 c2404480 >> c2bd9eb4 >> 1d40: c2bd8400 c2404480 00000002 00000001 00000000 00000000 00000001 >> 00000000 >> 1d60: 00000000 00000000 00000000 00000000 00000000 00000000 00000001 >> 00000000 >> 1d80: 00380bc6 00000000 003b257f 00000000 00000077 0000ffff 00000200 >> 00000002 >> 1da0: 00000001 0000ffff 00000000 00000401 c2bfe22c 00000000 00000000 >> 00000000 >> 1dc0: 00000000 00000000 c2012400 c24be100 00001000 00000000 c2404480 >> 00000000 >> 1de0: 00004003 9fd04dd9 00000000 c2bd9c00 c2404480 00000083 c24044fc >> 00008000 >> 1e00: 00000000 00000020 00008000 c00e4d88 c2404480 00000000 00000000 >> c0190afc >> 1e20: c2bd0be0 c0692898 c0692898 00000000 00000020 c0190b10 c0194f48 >> c2bd0be0 >> 1e40: c066b370 c00e568c c29f5380 c01002d4 c29f5380 c2bd0be0 00008000 >> c0100488 >> 1e60: c0692898 00000000 c066b028 c2bd0be0 c2bd0bc0 c01033ec 00000000 >> ffffff00 >> 1e80: ffff0a00 c0575ff4 c2bd0be0 c281a010 c2417098 c2bd0be0 0000000a >> 00000000 >> 1ea0: 0000000a 9fd04dd9 0000000a c2bd0be0 c2bd0bc0 00000000 c0575ff8 >> 00008000 >> 1ec0: c066b028 00008000 c0575ff4 c0104178 00000000 00000000 c2014000 >> c2014005 >> 1ee0: c0575ff8 c3fb1280 c0652868 c062b1ec 00000000 c01013d8 c24c3558 >> 00000000 >> 1f00: 00000000 00006180 c24c3558 c00f17c4 e10c76ac 0b300002 c2858015 >> c281a010 >> 1f20: c2415da8 9fd04dd9 00000000 00000002 c06ad310 c066b028 c066da68 >> 00000000 >> 1f40: 00000000 00000000 00000000 c062b478 00000000 c0037200 00306b6c >> 9fd00000 >> 1f60: c0576098 9fd04dd9 00000002 c06ad310 c0652878 00000000 00000000 >> 00000000 >> 1f80: 00000000 00000000 00000000 c062b5e4 00000000 00000000 00000000 >> 00000000 >> 1fa0: c04c7860 c04c7868 00000000 c00090e0 00000000 00000000 00000000 >> 00000000 >> 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 >> 00000000 >> 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 >> 00000000 >> [<c04b95ec>] (rb_insert_color) from [<c014bfdc>] >> (kernfs_link_sibling+0x94/0xcc) >> [<c014bfdc>] (kernfs_link_sibling) from [<c014c768>] >> (kernfs_add_one+0x90/0x140) >> [<c014c768>] (kernfs_add_one) from [<c014c860>] >> (kernfs_create_dir_ns+0x48/0x74) >> [<c014c860>] (kernfs_create_dir_ns) from [<c014ec70>] >> (sysfs_create_dir_ns+0x68/0xd0) >> [<c014ec70>] (sysfs_create_dir_ns) from [<c04b58f8>] >> (kobject_add_internal+0x9c/0x2c4) >> [<c04b58f8>] (kobject_add_internal) from [<c04b5d64>] >> (kobject_init_and_add+0x54/0x94) >> [<c04b5d64>] (kobject_init_and_add) from [<c00d650c>] >> (sysfs_slab_add+0x10c/0x220) >> [<c00d650c>] (sysfs_slab_add) from [<c00d72f8>] >> (__kmem_cache_create+0x1d8/0x338) >> [<c00d72f8>] (__kmem_cache_create) from [<c00b0958>] >> (kmem_cache_create_usercopy+0x180/0x298) >> [<c00b0958>] (kmem_cache_create_usercopy) from [<c00b0a90>] >> (kmem_cache_create+0x20/0x28) >> [<c00b0a90>] (kmem_cache_create) from [<c017d6e4>] >> (ext4_mb_init+0x374/0x44c) >> [<c017d6e4>] (ext4_mb_init) from [<c01971a0>] >> (ext4_fill_super+0x2258/0x2ef0) >> [<c01971a0>] (ext4_fill_super) from [<c00e4d88>] (mount_bdev+0x154/0x18c) >> [<c00e4d88>] (mount_bdev) from [<c0190b10>] (ext4_mount+0x14/0x20) >> [<c0190b10>] (ext4_mount) from [<c00e568c>] (mount_fs+0x14/0xa8) >> [<c00e568c>] (mount_fs) from [<c0100488>] (vfs_kern_mount+0x48/0xf0) >> [<c0100488>] (vfs_kern_mount) from [<c01033ec>] (do_mount+0x180/0xb9c) >> [<c01033ec>] (do_mount) from [<c0104178>] (ksys_mount+0x8c/0xb4) >> [<c0104178>] (ksys_mount) from [<c062b1ec>] (mount_block_root+0x128/0x2a4) >> [<c062b1ec>] (mount_block_root) from [<c062b478>] (mount_root+0x110/0x154) >> [<c062b478>] (mount_root) from [<c062b5e4>] (prepare_namespace+0x128/0x188) >> [<c062b5e4>] (prepare_namespace) from [<c04c7868>] (kernel_init+0x8/0xf4) >> [<c04c7868>] (kernel_init) from [<c00090e0>] (ret_from_fork+0x14/0x34) >> Exception stack(0xc2831fb0 to 0xc2831ff8) >> 1fa0: 00000000 00000000 00000000 >> 00000000 >> 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 >> 00000000 >> 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 >> Code: e5923000 e3130001 1a000054 e92d4070 (e593c004) >> ---[ end trace 2c8fc104914a532b ]--- >> Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b >> ---[ end Kernel panic - not syncing: Attempted to kill init! >> exitcode=0x0000000b ]--- >> > > First step is to test if it's the damage clip allocation/freeing that's > causing problems. Setting clips to NULL avoids allocation and results in > a full update each time: > > diff --git a/drivers/gpu/drm/drm_damage_helper.c > b/drivers/gpu/drm/drm_damage_helper.c > index e16aa5ae00b4..943ae17b8b1c 100644 > --- a/drivers/gpu/drm/drm_damage_helper.c > +++ b/drivers/gpu/drm/drm_damage_helper.c > @@ -182,6 +182,7 @@ int drm_atomic_helper_dirtyfb(struct drm_framebuffer > *fb, > } > state->acquire_ctx = &ctx; > > + clips = NULL; > if (clips) { > uint32_t inc = 1; > > If that helps, try clearing/freeing the damage explicitly: > > diff --git a/drivers/gpu/drm/drm_damage_helper.c > b/drivers/gpu/drm/drm_damage_helper.c > index e16aa5ae00b4..0a59263e4733 100644 > --- a/drivers/gpu/drm/drm_damage_helper.c > +++ b/drivers/gpu/drm/drm_damage_helper.c > @@ -371,6 +371,8 @@ bool drm_atomic_helper_damage_merged(const struct > drm_plane_state *old_state, > valid = true; > } > > + drm_property_replace_blob(&state->fb_damage_clips, NULL); > + > return valid; > } > EXPORT_SYMBOL(drm_atomic_helper_damage_merged); > > If that doesn't help, try disabling the call to st7586_fb_dirty() so we > can rule out the driver. > If it helps, verify that the rectangle is within bounds. > I see that you have this call chain: st7586_pipe_enable() -> mipi_dbi_enable_flush() -> mipi_dbi_fb_dirty(). That doesn't look safe. The st7586 driver allocates a tx_buf with size: size_t bufsize = (mode->vdisplay + 2) / 3 * mode->hdisplay; whereas mipi_dbi_enable_flush() will trigger a copy to tx_buf with len: fb->width * fb->height * 2 It looks like you're writing zeroes way past the end of the buffer. Noralf.
On 1/14/19 10:13 AM, Noralf Trønnes wrote: > > I see that you have this call chain: > st7586_pipe_enable() -> mipi_dbi_enable_flush() -> mipi_dbi_fb_dirty(). > > That doesn't look safe. The st7586 driver allocates a tx_buf with size: > size_t bufsize = (mode->vdisplay + 2) / 3 * mode->hdisplay; > > whereas mipi_dbi_enable_flush() will trigger a copy to tx_buf with len: > fb->width * fb->height * 2 > > It looks like you're writing zeroes way past the end of the buffer. > > Noralf. > Thanks! That does indeed seem to be the problem. I'll put together a patch to fix this. I'm thinking it will be easier to make the fix before applying this series so that it will be easier to backport. FWIW, here is the change I made on top of this series. diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c index 50e370ce5a59..a6cad8f1d2c9 100644 --- a/drivers/gpu/drm/tinydrm/st7586.c +++ b/drivers/gpu/drm/tinydrm/st7586.c @@ -176,6 +176,12 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe, { struct tinydrm_device *tdev = pipe_to_tinydrm(pipe); struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev); + struct drm_rect rect = { + .x1 = 0, + .x2 = plane_state->fb->width, + .y1 = 0, + .y2 = plane_state->fb->height, + }; int ret; u8 addr_mode; @@ -234,7 +240,7 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe, mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON); - mipi_dbi_enable_flush(mipi, crtc_state, plane_state); + st7586_fb_dirty(plane_state->fb, &rect); } static void st7586_pipe_disable(struct drm_simple_display_pipe *pipe)
On 1/11/19 2:12 PM, Noralf Trønnes wrote: > This switches to drm_atomic_helper_dirtyfb() as the framebuffer dirty > handler. All flushing will now happen in the pipe functions. > > Also enable the damage plane property for all except repaper which can > only do full updates. > > ili9225: > This change made ili9225_init() equal to mipi_dbi_init() so use it. > > v2: Remove fb check in mipi_dbi_enable_flush() it can't be NULL > (kbuild test robot) > > Cc: David Lechner <david@lechnology.com> > Cc: Eric Anholt <eric@anholt.net> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > Acked-by: Sam Ravnborg <sam@ravnborg.org> > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- ... > diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c > index a52bc3b02606..50e370ce5a59 100644 > --- a/drivers/gpu/drm/tinydrm/st7586.c > +++ b/drivers/gpu/drm/tinydrm/st7586.c > @@ -17,6 +17,7 @@ > #include <linux/spi/spi.h> > #include <video/mipi_display.h> > > +#include <drm/drm_damage_helper.h> > #include <drm/drm_drv.h> > #include <drm/drm_fb_cma_helper.h> > #include <drm/drm_gem_cma_helper.h> > @@ -112,23 +113,15 @@ static int st7586_buf_copy(void *dst, struct drm_framebuffer *fb, > return ret; > } > > -static int st7586_fb_dirty(struct drm_framebuffer *fb, > - struct drm_file *file_priv, unsigned int flags, > - unsigned int color, struct drm_clip_rect *clips, > - unsigned int num_clips) > +static void st7586_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect) > { > struct tinydrm_device *tdev = fb->dev->dev_private; > struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev); > - struct drm_rect clip; > - struct drm_rect *rect = &clip; This function later modifies the fields of the struct pointed to by rect. Are all callers of this function OK with having it modified or should we make a local copy of rect and modify that instead?
On 1/14/19 3:50 PM, David Lechner wrote: > On 1/14/19 10:13 AM, Noralf Trønnes wrote: >> >> I see that you have this call chain: >> st7586_pipe_enable() -> mipi_dbi_enable_flush() -> mipi_dbi_fb_dirty(). >> >> That doesn't look safe. The st7586 driver allocates a tx_buf with size: >> size_t bufsize = (mode->vdisplay + 2) / 3 * mode->hdisplay; >> >> whereas mipi_dbi_enable_flush() will trigger a copy to tx_buf with len: >> fb->width * fb->height * 2 >> >> It looks like you're writing zeroes way past the end of the buffer. >> >> Noralf. >> > > Thanks! That does indeed seem to be the problem. I'll put together > a patch to fix this. I'm thinking it will be easier to make the > fix before applying this series so that it will be easier to > backport. > Well, now that I am looking into it more, I see that the problem was not preexisting. This patch ("drm/tinydrm: Use damage helper for dirtyfb") also changes mipi_dbi_enable_flush() from calling tdev->fb_dirty() to mipi_dbi_fb_dirty(). Perhaps we should not be dropping tdev->fb_dirty()?
Den 14.01.2019 23.33, skrev David Lechner: > On 1/14/19 3:50 PM, David Lechner wrote: >> On 1/14/19 10:13 AM, Noralf Trønnes wrote: >>> >>> I see that you have this call chain: >>> st7586_pipe_enable() -> mipi_dbi_enable_flush() -> mipi_dbi_fb_dirty(). >>> >>> That doesn't look safe. The st7586 driver allocates a tx_buf with size: >>> size_t bufsize = (mode->vdisplay + 2) / 3 * mode->hdisplay; >>> >>> whereas mipi_dbi_enable_flush() will trigger a copy to tx_buf with len: >>> fb->width * fb->height * 2 >>> >>> It looks like you're writing zeroes way past the end of the buffer. >>> >>> Noralf. >>> >> >> Thanks! That does indeed seem to be the problem. I'll put together >> a patch to fix this. I'm thinking it will be easier to make the >> fix before applying this series so that it will be easier to >> backport. >> > > Well, now that I am looking into it more, I see that the problem > was not preexisting. This patch ("drm/tinydrm: Use damage helper > for dirtyfb") also changes mipi_dbi_enable_flush() from calling > tdev->fb_dirty() to mipi_dbi_fb_dirty(). > > Perhaps we should not be dropping tdev->fb_dirty()? Ah, now it makes sense. I thought it strange that you hadn't experienced the corruption before. How about this change? diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c index d39417b74e8b..01a8077954b3 100644 --- a/drivers/gpu/drm/tinydrm/st7586.c +++ b/drivers/gpu/drm/tinydrm/st7586.c @@ -177,6 +177,13 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe, { struct tinydrm_device *tdev = pipe_to_tinydrm(pipe); struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev); + struct drm_framebuffer *fb = plane_state->fb; + struct drm_rect rect = { + .x1 = 0, + .x2 = fb->width, + .y1 = 0, + .y2 = fb->height, + }; int ret; u8 addr_mode; @@ -233,9 +240,10 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe, msleep(100); - mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON); + mipi->enabled = true; + st7586_fb_dirty(fb, &rect); - mipi_dbi_enable_flush(mipi, crtc_state, plane_state); + mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON); } static void st7586_pipe_disable(struct drm_simple_display_pipe *pipe)
Den 14.01.2019 23.06, skrev David Lechner: > On 1/11/19 2:12 PM, Noralf Trønnes wrote: >> This switches to drm_atomic_helper_dirtyfb() as the framebuffer dirty >> handler. All flushing will now happen in the pipe functions. >> >> Also enable the damage plane property for all except repaper which can >> only do full updates. >> >> ili9225: >> This change made ili9225_init() equal to mipi_dbi_init() so use it. >> >> v2: Remove fb check in mipi_dbi_enable_flush() it can't be NULL >> (kbuild test robot) >> >> Cc: David Lechner <david@lechnology.com> >> Cc: Eric Anholt <eric@anholt.net> >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> >> Acked-by: Sam Ravnborg <sam@ravnborg.org> >> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> --- > > > ... > >> diff --git a/drivers/gpu/drm/tinydrm/st7586.c >> b/drivers/gpu/drm/tinydrm/st7586.c >> index a52bc3b02606..50e370ce5a59 100644 >> --- a/drivers/gpu/drm/tinydrm/st7586.c >> +++ b/drivers/gpu/drm/tinydrm/st7586.c >> @@ -17,6 +17,7 @@ >> #include <linux/spi/spi.h> >> #include <video/mipi_display.h> >> +#include <drm/drm_damage_helper.h> >> #include <drm/drm_drv.h> >> #include <drm/drm_fb_cma_helper.h> >> #include <drm/drm_gem_cma_helper.h> >> @@ -112,23 +113,15 @@ static int st7586_buf_copy(void *dst, struct >> drm_framebuffer *fb, >> return ret; >> } >> -static int st7586_fb_dirty(struct drm_framebuffer *fb, >> - struct drm_file *file_priv, unsigned int flags, >> - unsigned int color, struct drm_clip_rect *clips, >> - unsigned int num_clips) >> +static void st7586_fb_dirty(struct drm_framebuffer *fb, struct >> drm_rect *rect) >> { >> struct tinydrm_device *tdev = fb->dev->dev_private; >> struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev); >> - struct drm_rect clip; >> - struct drm_rect *rect = &clip; > > This function later modifies the fields of the struct pointed to by rect. > Are all callers of this function OK with having it modified or should we > make a local copy of rect and modify that instead? It's safe to modify it. st7586_pipe_update() owns the structure and it doesn't use it after calling st7586_fb_dirty(). Noralf.
Den 14.01.2019 23.33, skrev David Lechner: > On 1/14/19 3:50 PM, David Lechner wrote: >> On 1/14/19 10:13 AM, Noralf Trønnes wrote: >>> >>> I see that you have this call chain: >>> st7586_pipe_enable() -> mipi_dbi_enable_flush() -> mipi_dbi_fb_dirty(). >>> >>> That doesn't look safe. The st7586 driver allocates a tx_buf with size: >>> size_t bufsize = (mode->vdisplay + 2) / 3 * mode->hdisplay; >>> >>> whereas mipi_dbi_enable_flush() will trigger a copy to tx_buf with len: >>> fb->width * fb->height * 2 >>> >>> It looks like you're writing zeroes way past the end of the buffer. >>> >>> Noralf. >>> >> >> Thanks! That does indeed seem to be the problem. I'll put together >> a patch to fix this. I'm thinking it will be easier to make the >> fix before applying this series so that it will be easier to >> backport. >> > > Well, now that I am looking into it more, I see that the problem > was not preexisting. This patch ("drm/tinydrm: Use damage helper > for dirtyfb") also changes mipi_dbi_enable_flush() from calling > tdev->fb_dirty() to mipi_dbi_fb_dirty(). > > Perhaps we should not be dropping tdev->fb_dirty()? I want to get rid of tinydrm_device, to avoid tinydrm being like a mid-layer. My goal is to make tinydrm just a collection of tiny regular DRM drivers.
On 1/14/19 4:43 PM, Noralf Trønnes wrote: > > > Den 14.01.2019 23.33, skrev David Lechner: >> On 1/14/19 3:50 PM, David Lechner wrote: >>> On 1/14/19 10:13 AM, Noralf Trønnes wrote: >>>> >>>> I see that you have this call chain: >>>> st7586_pipe_enable() -> mipi_dbi_enable_flush() -> mipi_dbi_fb_dirty(). >>>> >>>> That doesn't look safe. The st7586 driver allocates a tx_buf with size: >>>> size_t bufsize = (mode->vdisplay + 2) / 3 * mode->hdisplay; >>>> >>>> whereas mipi_dbi_enable_flush() will trigger a copy to tx_buf with len: >>>> fb->width * fb->height * 2 >>>> >>>> It looks like you're writing zeroes way past the end of the buffer. >>>> >>>> Noralf. >>>> >>> >>> Thanks! That does indeed seem to be the problem. I'll put together >>> a patch to fix this. I'm thinking it will be easier to make the >>> fix before applying this series so that it will be easier to >>> backport. >>> >> >> Well, now that I am looking into it more, I see that the problem >> was not preexisting. This patch ("drm/tinydrm: Use damage helper >> for dirtyfb") also changes mipi_dbi_enable_flush() from calling >> tdev->fb_dirty() to mipi_dbi_fb_dirty(). >> >> Perhaps we should not be dropping tdev->fb_dirty()? > > Ah, now it makes sense. I thought it strange that you hadn't experienced > the corruption before. > > How about this change? > > diff --git a/drivers/gpu/drm/tinydrm/st7586.c > b/drivers/gpu/drm/tinydrm/st7586.c > index d39417b74e8b..01a8077954b3 100644 > --- a/drivers/gpu/drm/tinydrm/st7586.c > +++ b/drivers/gpu/drm/tinydrm/st7586.c > @@ -177,6 +177,13 @@ static void st7586_pipe_enable(struct > drm_simple_display_pipe *pipe, > { > struct tinydrm_device *tdev = pipe_to_tinydrm(pipe); > struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev); > + struct drm_framebuffer *fb = plane_state->fb; > + struct drm_rect rect = { > + .x1 = 0, > + .x2 = fb->width, > + .y1 = 0, > + .y2 = fb->height, > + }; > int ret; > u8 addr_mode; > > @@ -233,9 +240,10 @@ static void st7586_pipe_enable(struct > drm_simple_display_pipe *pipe, > > msleep(100); > > - mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON); > + mipi->enabled = true; > + st7586_fb_dirty(fb, &rect); > > - mipi_dbi_enable_flush(mipi, crtc_state, plane_state); > + mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON); > } > > static void st7586_pipe_disable(struct drm_simple_display_pipe *pipe) > > That looks good. I also noticed that ili9225 has a custom dirty function, so it will need a similar change as well.
Den 15.01.2019 00.03, skrev David Lechner: > On 1/14/19 4:43 PM, Noralf Trønnes wrote: >> >> >> Den 14.01.2019 23.33, skrev David Lechner: >>> On 1/14/19 3:50 PM, David Lechner wrote: >>>> On 1/14/19 10:13 AM, Noralf Trønnes wrote: >>>>> >>>>> I see that you have this call chain: >>>>> st7586_pipe_enable() -> mipi_dbi_enable_flush() -> >>>>> mipi_dbi_fb_dirty(). >>>>> >>>>> That doesn't look safe. The st7586 driver allocates a tx_buf with >>>>> size: >>>>> size_t bufsize = (mode->vdisplay + 2) / 3 * mode->hdisplay; >>>>> >>>>> whereas mipi_dbi_enable_flush() will trigger a copy to tx_buf with >>>>> len: >>>>> fb->width * fb->height * 2 >>>>> >>>>> It looks like you're writing zeroes way past the end of the buffer. >>>>> >>>>> Noralf. >>>>> >>>> >>>> Thanks! That does indeed seem to be the problem. I'll put together >>>> a patch to fix this. I'm thinking it will be easier to make the >>>> fix before applying this series so that it will be easier to >>>> backport. >>>> >>> >>> Well, now that I am looking into it more, I see that the problem >>> was not preexisting. This patch ("drm/tinydrm: Use damage helper >>> for dirtyfb") also changes mipi_dbi_enable_flush() from calling >>> tdev->fb_dirty() to mipi_dbi_fb_dirty(). >>> >>> Perhaps we should not be dropping tdev->fb_dirty()? >> >> Ah, now it makes sense. I thought it strange that you hadn't experienced >> the corruption before. >> >> How about this change? >> >> diff --git a/drivers/gpu/drm/tinydrm/st7586.c >> b/drivers/gpu/drm/tinydrm/st7586.c >> index d39417b74e8b..01a8077954b3 100644 >> --- a/drivers/gpu/drm/tinydrm/st7586.c >> +++ b/drivers/gpu/drm/tinydrm/st7586.c >> @@ -177,6 +177,13 @@ static void st7586_pipe_enable(struct >> drm_simple_display_pipe *pipe, >> { >> struct tinydrm_device *tdev = pipe_to_tinydrm(pipe); >> struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev); >> + struct drm_framebuffer *fb = plane_state->fb; >> + struct drm_rect rect = { >> + .x1 = 0, >> + .x2 = fb->width, >> + .y1 = 0, >> + .y2 = fb->height, >> + }; >> int ret; >> u8 addr_mode; >> >> @@ -233,9 +240,10 @@ static void st7586_pipe_enable(struct >> drm_simple_display_pipe *pipe, >> >> msleep(100); >> >> - mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON); >> + mipi->enabled = true; >> + st7586_fb_dirty(fb, &rect); >> >> - mipi_dbi_enable_flush(mipi, crtc_state, plane_state); >> + mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON); >> } >> >> static void st7586_pipe_disable(struct drm_simple_display_pipe *pipe) >> >> > > > That looks good. I also noticed that ili9225 has a custom dirty > function, so it will need a similar change as well. > Thanks for tracking this down, I'll spin a new version tomorrow. Noralf.
On 1/14/19 4:53 PM, Noralf Trønnes wrote: > > > Den 14.01.2019 23.33, skrev David Lechner: >> On 1/14/19 3:50 PM, David Lechner wrote: >>> On 1/14/19 10:13 AM, Noralf Trønnes wrote: >>>> >>>> I see that you have this call chain: >>>> st7586_pipe_enable() -> mipi_dbi_enable_flush() -> mipi_dbi_fb_dirty(). >>>> >>>> That doesn't look safe. The st7586 driver allocates a tx_buf with size: >>>> size_t bufsize = (mode->vdisplay + 2) / 3 * mode->hdisplay; >>>> >>>> whereas mipi_dbi_enable_flush() will trigger a copy to tx_buf with len: >>>> fb->width * fb->height * 2 >>>> >>>> It looks like you're writing zeroes way past the end of the buffer. >>>> >>>> Noralf. >>>> >>> >>> Thanks! That does indeed seem to be the problem. I'll put together >>> a patch to fix this. I'm thinking it will be easier to make the >>> fix before applying this series so that it will be easier to >>> backport. >>> >> >> Well, now that I am looking into it more, I see that the problem >> was not preexisting. This patch ("drm/tinydrm: Use damage helper >> for dirtyfb") also changes mipi_dbi_enable_flush() from calling >> tdev->fb_dirty() to mipi_dbi_fb_dirty(). >> >> Perhaps we should not be dropping tdev->fb_dirty()? > > I want to get rid of tinydrm_device, to avoid tinydrm being like a > mid-layer. My goal is to make tinydrm just a collection of tiny regular > DRM drivers. > OK. Perhaps we should add a comment to mipi_dbi_enable_flush() to say that it should not be used by drivers with a custom dirty function. Or perhaps we could pass an optional parameter with the custom dirty function to mipi_dbi_enable_flush() like this: diff --git a/drivers/gpu/drm/tinydrm/hx8357d.c b/drivers/gpu/drm/tinydrm/hx8357d.c index 8bbd0beafc6a..4b9981ffe5a4 100644 --- a/drivers/gpu/drm/tinydrm/hx8357d.c +++ b/drivers/gpu/drm/tinydrm/hx8357d.c @@ -170,7 +170,7 @@ static void yx240qv29_enable(struct drm_simple_display_pipe *pipe, break; } mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode); - mipi_dbi_enable_flush(mipi, crtc_state, plane_state); + mipi_dbi_enable_flush(mipi, crtc_state, plane_state, NULL); } static const struct drm_simple_display_pipe_funcs hx8357d_pipe_funcs = { diff --git a/drivers/gpu/drm/tinydrm/ili9225.c b/drivers/gpu/drm/tinydrm/ili9225.c index aa1376a22bda..bebfd0d01340 100644 --- a/drivers/gpu/drm/tinydrm/ili9225.c +++ b/drivers/gpu/drm/tinydrm/ili9225.c @@ -270,7 +270,7 @@ static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe, ili9225_command(mipi, ILI9225_DISPLAY_CONTROL_1, 0x1017); - mipi_dbi_enable_flush(mipi, crtc_state, plane_state); + mipi_dbi_enable_flush(mipi, crtc_state, plane_state, ili9225_fb_dirty); } static void ili9225_pipe_disable(struct drm_simple_display_pipe *pipe) diff --git a/drivers/gpu/drm/tinydrm/ili9341.c b/drivers/gpu/drm/tinydrm/ili9341.c index 713bb2dd7e04..ecfb8ff02a40 100644 --- a/drivers/gpu/drm/tinydrm/ili9341.c +++ b/drivers/gpu/drm/tinydrm/ili9341.c @@ -126,7 +126,7 @@ static void yx240qv29_enable(struct drm_simple_display_pipe *pipe, } addr_mode |= ILI9341_MADCTL_BGR; mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode); - mipi_dbi_enable_flush(mipi, crtc_state, plane_state); + mipi_dbi_enable_flush(mipi, crtc_state, plane_state, NULL); } static const struct drm_simple_display_pipe_funcs ili9341_pipe_funcs = { diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c index 82a92ec9ae3c..c81aa1fbc2ad 100644 --- a/drivers/gpu/drm/tinydrm/mi0283qt.c +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c @@ -134,7 +134,7 @@ static void mi0283qt_enable(struct drm_simple_display_pipe *pipe, } addr_mode |= ILI9341_MADCTL_BGR; mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode); - mipi_dbi_enable_flush(mipi, crtc_state, plane_state); + mipi_dbi_enable_flush(mipi, crtc_state, plane_state, NULL); } static const struct drm_simple_display_pipe_funcs mi0283qt_pipe_funcs = { diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c index 680692a83f94..399c66b3f0d7 100644 --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c @@ -287,6 +287,7 @@ EXPORT_SYMBOL(mipi_dbi_pipe_update); * @mipi: MIPI DBI structure * @crtc_state: CRTC state * @plane_state: Plane state + * @dirty: Optional custom dirty function * * This function sets &mipi_dbi->enabled, flushes the whole framebuffer and * enables the backlight. Drivers can use this in their @@ -294,7 +295,9 @@ EXPORT_SYMBOL(mipi_dbi_pipe_update); */ void mipi_dbi_enable_flush(struct mipi_dbi *mipi, struct drm_crtc_state *crtc_state, - struct drm_plane_state *plane_state) + struct drm_plane_state *plane_state, + void (*dirty)(struct drm_framebuffer *fb, + struct drm_rect *rect)) { struct drm_framebuffer *fb = plane_state->fb; struct drm_rect rect = { @@ -305,7 +308,10 @@ void mipi_dbi_enable_flush(struct mipi_dbi *mipi, }; mipi->enabled = true; - mipi_dbi_fb_dirty(fb, &rect); + if (!dirty) { + dirty = mipi_dbi_fb_dirty; + } + dirty(fb, &rect); backlight_enable(mipi->backlight); } EXPORT_SYMBOL(mipi_dbi_enable_flush); diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c index 50e370ce5a59..d6ffac64822f 100644 --- a/drivers/gpu/drm/tinydrm/st7586.c +++ b/drivers/gpu/drm/tinydrm/st7586.c @@ -234,7 +234,7 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe, mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON); - mipi_dbi_enable_flush(mipi, crtc_state, plane_state); + mipi_dbi_enable_flush(mipi, crtc_state, plane_state, st7586_fb_dirty); } static void st7586_pipe_disable(struct drm_simple_display_pipe *pipe) diff --git a/drivers/gpu/drm/tinydrm/st7735r.c b/drivers/gpu/drm/tinydrm/st7735r.c index 3bab9a9569a6..dee143007048 100644 --- a/drivers/gpu/drm/tinydrm/st7735r.c +++ b/drivers/gpu/drm/tinydrm/st7735r.c @@ -100,7 +100,7 @@ static void jd_t18003_t01_pipe_enable(struct drm_simple_display_pipe *pipe, msleep(20); - mipi_dbi_enable_flush(mipi, crtc_state, plane_state); + mipi_dbi_enable_flush(mipi, crtc_state, plane_state, NULL); } static const struct drm_simple_display_pipe_funcs jd_t18003_t01_pipe_funcs = { diff --git a/include/drm/tinydrm/mipi-dbi.h b/include/drm/tinydrm/mipi-dbi.h index f4ec2834bc22..3d9f003fdcc6 100644 --- a/include/drm/tinydrm/mipi-dbi.h +++ b/include/drm/tinydrm/mipi-dbi.h @@ -72,7 +72,9 @@ void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe, struct drm_plane_state *old_state); void mipi_dbi_enable_flush(struct mipi_dbi *mipi, struct drm_crtc_state *crtc_state, - struct drm_plane_state *plan_state); + struct drm_plane_state *plan_state, + void (*dirty)(struct drm_framebuffer *fb, + struct drm_rect *rect)); void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe); void mipi_dbi_hw_reset(struct mipi_dbi *mipi); bool mipi_dbi_display_is_on(struct mipi_dbi *mipi);
Den 15.01.2019 00.26, skrev David Lechner: > On 1/14/19 4:53 PM, Noralf Trønnes wrote: >> >> >> Den 14.01.2019 23.33, skrev David Lechner: >>> On 1/14/19 3:50 PM, David Lechner wrote: >>>> On 1/14/19 10:13 AM, Noralf Trønnes wrote: >>>>> >>>>> I see that you have this call chain: >>>>> st7586_pipe_enable() -> mipi_dbi_enable_flush() -> >>>>> mipi_dbi_fb_dirty(). >>>>> >>>>> That doesn't look safe. The st7586 driver allocates a tx_buf with >>>>> size: >>>>> size_t bufsize = (mode->vdisplay + 2) / 3 * mode->hdisplay; >>>>> >>>>> whereas mipi_dbi_enable_flush() will trigger a copy to tx_buf with >>>>> len: >>>>> fb->width * fb->height * 2 >>>>> >>>>> It looks like you're writing zeroes way past the end of the buffer. >>>>> >>>>> Noralf. >>>>> >>>> >>>> Thanks! That does indeed seem to be the problem. I'll put together >>>> a patch to fix this. I'm thinking it will be easier to make the >>>> fix before applying this series so that it will be easier to >>>> backport. >>>> >>> >>> Well, now that I am looking into it more, I see that the problem >>> was not preexisting. This patch ("drm/tinydrm: Use damage helper >>> for dirtyfb") also changes mipi_dbi_enable_flush() from calling >>> tdev->fb_dirty() to mipi_dbi_fb_dirty(). >>> >>> Perhaps we should not be dropping tdev->fb_dirty()? >> >> I want to get rid of tinydrm_device, to avoid tinydrm being like a >> mid-layer. My goal is to make tinydrm just a collection of tiny regular >> DRM drivers. >> > > OK. Perhaps we should add a comment to mipi_dbi_enable_flush() to > say that it should not be used by drivers with a custom dirty function. > I've added a note. > Or perhaps we could pass an optional parameter with the custom dirty > function to mipi_dbi_enable_flush() like this: > I prefer to open code the function instead since it's only 2 lines of code. Noralf. > diff --git a/drivers/gpu/drm/tinydrm/hx8357d.c > b/drivers/gpu/drm/tinydrm/hx8357d.c > index 8bbd0beafc6a..4b9981ffe5a4 100644 > --- a/drivers/gpu/drm/tinydrm/hx8357d.c > +++ b/drivers/gpu/drm/tinydrm/hx8357d.c > @@ -170,7 +170,7 @@ static void yx240qv29_enable(struct > drm_simple_display_pipe *pipe, > break; > } > mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode); > - mipi_dbi_enable_flush(mipi, crtc_state, plane_state); > + mipi_dbi_enable_flush(mipi, crtc_state, plane_state, NULL); > } > > static const struct drm_simple_display_pipe_funcs hx8357d_pipe_funcs = { > diff --git a/drivers/gpu/drm/tinydrm/ili9225.c > b/drivers/gpu/drm/tinydrm/ili9225.c > index aa1376a22bda..bebfd0d01340 100644 > --- a/drivers/gpu/drm/tinydrm/ili9225.c > +++ b/drivers/gpu/drm/tinydrm/ili9225.c > @@ -270,7 +270,7 @@ static void ili9225_pipe_enable(struct > drm_simple_display_pipe *pipe, > > ili9225_command(mipi, ILI9225_DISPLAY_CONTROL_1, 0x1017); > > - mipi_dbi_enable_flush(mipi, crtc_state, plane_state); > + mipi_dbi_enable_flush(mipi, crtc_state, plane_state, > ili9225_fb_dirty); > } > > static void ili9225_pipe_disable(struct drm_simple_display_pipe *pipe) > diff --git a/drivers/gpu/drm/tinydrm/ili9341.c > b/drivers/gpu/drm/tinydrm/ili9341.c > index 713bb2dd7e04..ecfb8ff02a40 100644 > --- a/drivers/gpu/drm/tinydrm/ili9341.c > +++ b/drivers/gpu/drm/tinydrm/ili9341.c > @@ -126,7 +126,7 @@ static void yx240qv29_enable(struct > drm_simple_display_pipe *pipe, > } > addr_mode |= ILI9341_MADCTL_BGR; > mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode); > - mipi_dbi_enable_flush(mipi, crtc_state, plane_state); > + mipi_dbi_enable_flush(mipi, crtc_state, plane_state, NULL); > } > > static const struct drm_simple_display_pipe_funcs ili9341_pipe_funcs = { > diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c > b/drivers/gpu/drm/tinydrm/mi0283qt.c > index 82a92ec9ae3c..c81aa1fbc2ad 100644 > --- a/drivers/gpu/drm/tinydrm/mi0283qt.c > +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c > @@ -134,7 +134,7 @@ static void mi0283qt_enable(struct > drm_simple_display_pipe *pipe, > } > addr_mode |= ILI9341_MADCTL_BGR; > mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode); > - mipi_dbi_enable_flush(mipi, crtc_state, plane_state); > + mipi_dbi_enable_flush(mipi, crtc_state, plane_state, NULL); > } > > static const struct drm_simple_display_pipe_funcs mi0283qt_pipe_funcs = { > diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c > b/drivers/gpu/drm/tinydrm/mipi-dbi.c > index 680692a83f94..399c66b3f0d7 100644 > --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c > +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c > @@ -287,6 +287,7 @@ EXPORT_SYMBOL(mipi_dbi_pipe_update); > * @mipi: MIPI DBI structure > * @crtc_state: CRTC state > * @plane_state: Plane state > + * @dirty: Optional custom dirty function > * > * This function sets &mipi_dbi->enabled, flushes the whole framebuffer > and > * enables the backlight. Drivers can use this in their > @@ -294,7 +295,9 @@ EXPORT_SYMBOL(mipi_dbi_pipe_update); > */ > void mipi_dbi_enable_flush(struct mipi_dbi *mipi, > struct drm_crtc_state *crtc_state, > - struct drm_plane_state *plane_state) > + struct drm_plane_state *plane_state, > + void (*dirty)(struct drm_framebuffer *fb, > + struct drm_rect *rect)) > { > struct drm_framebuffer *fb = plane_state->fb; > struct drm_rect rect = { > @@ -305,7 +308,10 @@ void mipi_dbi_enable_flush(struct mipi_dbi *mipi, > }; > > mipi->enabled = true; > - mipi_dbi_fb_dirty(fb, &rect); > + if (!dirty) { > + dirty = mipi_dbi_fb_dirty; > + } > + dirty(fb, &rect); > backlight_enable(mipi->backlight); > } > EXPORT_SYMBOL(mipi_dbi_enable_flush); > diff --git a/drivers/gpu/drm/tinydrm/st7586.c > b/drivers/gpu/drm/tinydrm/st7586.c > index 50e370ce5a59..d6ffac64822f 100644 > --- a/drivers/gpu/drm/tinydrm/st7586.c > +++ b/drivers/gpu/drm/tinydrm/st7586.c > @@ -234,7 +234,7 @@ static void st7586_pipe_enable(struct > drm_simple_display_pipe *pipe, > > mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON); > > - mipi_dbi_enable_flush(mipi, crtc_state, plane_state); > + mipi_dbi_enable_flush(mipi, crtc_state, plane_state, > st7586_fb_dirty); > } > > static void st7586_pipe_disable(struct drm_simple_display_pipe *pipe) > diff --git a/drivers/gpu/drm/tinydrm/st7735r.c > b/drivers/gpu/drm/tinydrm/st7735r.c > index 3bab9a9569a6..dee143007048 100644 > --- a/drivers/gpu/drm/tinydrm/st7735r.c > +++ b/drivers/gpu/drm/tinydrm/st7735r.c > @@ -100,7 +100,7 @@ static void jd_t18003_t01_pipe_enable(struct > drm_simple_display_pipe *pipe, > > msleep(20); > > - mipi_dbi_enable_flush(mipi, crtc_state, plane_state); > + mipi_dbi_enable_flush(mipi, crtc_state, plane_state, NULL); > } > > static const struct drm_simple_display_pipe_funcs > jd_t18003_t01_pipe_funcs = { > diff --git a/include/drm/tinydrm/mipi-dbi.h > b/include/drm/tinydrm/mipi-dbi.h > index f4ec2834bc22..3d9f003fdcc6 100644 > --- a/include/drm/tinydrm/mipi-dbi.h > +++ b/include/drm/tinydrm/mipi-dbi.h > @@ -72,7 +72,9 @@ void mipi_dbi_pipe_update(struct > drm_simple_display_pipe *pipe, > struct drm_plane_state *old_state); > void mipi_dbi_enable_flush(struct mipi_dbi *mipi, > struct drm_crtc_state *crtc_state, > - struct drm_plane_state *plan_state); > + struct drm_plane_state *plan_state, > + void (*dirty)(struct drm_framebuffer *fb, > + struct drm_rect *rect)); > void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe); > void mipi_dbi_hw_reset(struct mipi_dbi *mipi); > bool mipi_dbi_display_is_on(struct mipi_dbi *mipi);
Den 15.01.2019 05.38, skrev Noralf Trønnes: > > > Den 15.01.2019 00.26, skrev David Lechner: >> On 1/14/19 4:53 PM, Noralf Trønnes wrote: >>> >>> >>> Den 14.01.2019 23.33, skrev David Lechner: >>>> On 1/14/19 3:50 PM, David Lechner wrote: >>>>> On 1/14/19 10:13 AM, Noralf Trønnes wrote: >>>>>> >>>>>> I see that you have this call chain: >>>>>> st7586_pipe_enable() -> mipi_dbi_enable_flush() -> >>>>>> mipi_dbi_fb_dirty(). >>>>>> >>>>>> That doesn't look safe. The st7586 driver allocates a tx_buf with >>>>>> size: >>>>>> size_t bufsize = (mode->vdisplay + 2) / 3 * mode->hdisplay; >>>>>> >>>>>> whereas mipi_dbi_enable_flush() will trigger a copy to tx_buf with >>>>>> len: >>>>>> fb->width * fb->height * 2 >>>>>> >>>>>> It looks like you're writing zeroes way past the end of the buffer. >>>>>> >>>>>> Noralf. >>>>>> >>>>> >>>>> Thanks! That does indeed seem to be the problem. I'll put together >>>>> a patch to fix this. I'm thinking it will be easier to make the >>>>> fix before applying this series so that it will be easier to >>>>> backport. >>>>> >>>> >>>> Well, now that I am looking into it more, I see that the problem >>>> was not preexisting. This patch ("drm/tinydrm: Use damage helper >>>> for dirtyfb") also changes mipi_dbi_enable_flush() from calling >>>> tdev->fb_dirty() to mipi_dbi_fb_dirty(). >>>> >>>> Perhaps we should not be dropping tdev->fb_dirty()? >>> >>> I want to get rid of tinydrm_device, to avoid tinydrm being like a >>> mid-layer. My goal is to make tinydrm just a collection of tiny regular >>> DRM drivers. >>> >> >> OK. Perhaps we should add a comment to mipi_dbi_enable_flush() to >> say that it should not be used by drivers with a custom dirty function. >> > > I've added a note. > >> Or perhaps we could pass an optional parameter with the custom dirty >> function to mipi_dbi_enable_flush() like this: >> > > I prefer to open code the function instead since it's only 2 lines of code. '2 lines of code', that sounds like a misleading commercial :-) It's more like 9 including the declarions. But anyways I prefer it. > > Noralf. > >> diff --git a/drivers/gpu/drm/tinydrm/hx8357d.c >> b/drivers/gpu/drm/tinydrm/hx8357d.c >> index 8bbd0beafc6a..4b9981ffe5a4 100644 >> --- a/drivers/gpu/drm/tinydrm/hx8357d.c >> +++ b/drivers/gpu/drm/tinydrm/hx8357d.c >> @@ -170,7 +170,7 @@ static void yx240qv29_enable(struct >> drm_simple_display_pipe *pipe, >> break; >> } >> mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode); >> - mipi_dbi_enable_flush(mipi, crtc_state, plane_state); >> + mipi_dbi_enable_flush(mipi, crtc_state, plane_state, NULL); >> } >> >> static const struct drm_simple_display_pipe_funcs hx8357d_pipe_funcs = { >> diff --git a/drivers/gpu/drm/tinydrm/ili9225.c >> b/drivers/gpu/drm/tinydrm/ili9225.c >> index aa1376a22bda..bebfd0d01340 100644 >> --- a/drivers/gpu/drm/tinydrm/ili9225.c >> +++ b/drivers/gpu/drm/tinydrm/ili9225.c >> @@ -270,7 +270,7 @@ static void ili9225_pipe_enable(struct >> drm_simple_display_pipe *pipe, >> >> ili9225_command(mipi, ILI9225_DISPLAY_CONTROL_1, 0x1017); >> >> - mipi_dbi_enable_flush(mipi, crtc_state, plane_state); >> + mipi_dbi_enable_flush(mipi, crtc_state, plane_state, >> ili9225_fb_dirty); >> } >> >> static void ili9225_pipe_disable(struct drm_simple_display_pipe *pipe) >> diff --git a/drivers/gpu/drm/tinydrm/ili9341.c >> b/drivers/gpu/drm/tinydrm/ili9341.c >> index 713bb2dd7e04..ecfb8ff02a40 100644 >> --- a/drivers/gpu/drm/tinydrm/ili9341.c >> +++ b/drivers/gpu/drm/tinydrm/ili9341.c >> @@ -126,7 +126,7 @@ static void yx240qv29_enable(struct >> drm_simple_display_pipe *pipe, >> } >> addr_mode |= ILI9341_MADCTL_BGR; >> mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode); >> - mipi_dbi_enable_flush(mipi, crtc_state, plane_state); >> + mipi_dbi_enable_flush(mipi, crtc_state, plane_state, NULL); >> } >> >> static const struct drm_simple_display_pipe_funcs ili9341_pipe_funcs = { >> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c >> b/drivers/gpu/drm/tinydrm/mi0283qt.c >> index 82a92ec9ae3c..c81aa1fbc2ad 100644 >> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c >> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c >> @@ -134,7 +134,7 @@ static void mi0283qt_enable(struct >> drm_simple_display_pipe *pipe, >> } >> addr_mode |= ILI9341_MADCTL_BGR; >> mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode); >> - mipi_dbi_enable_flush(mipi, crtc_state, plane_state); >> + mipi_dbi_enable_flush(mipi, crtc_state, plane_state, NULL); >> } >> >> static const struct drm_simple_display_pipe_funcs mi0283qt_pipe_funcs = { >> diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c >> b/drivers/gpu/drm/tinydrm/mipi-dbi.c >> index 680692a83f94..399c66b3f0d7 100644 >> --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c >> +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c >> @@ -287,6 +287,7 @@ EXPORT_SYMBOL(mipi_dbi_pipe_update); >> * @mipi: MIPI DBI structure >> * @crtc_state: CRTC state >> * @plane_state: Plane state >> + * @dirty: Optional custom dirty function >> * >> * This function sets &mipi_dbi->enabled, flushes the whole framebuffer >> and >> * enables the backlight. Drivers can use this in their >> @@ -294,7 +295,9 @@ EXPORT_SYMBOL(mipi_dbi_pipe_update); >> */ >> void mipi_dbi_enable_flush(struct mipi_dbi *mipi, >> struct drm_crtc_state *crtc_state, >> - struct drm_plane_state *plane_state) >> + struct drm_plane_state *plane_state, >> + void (*dirty)(struct drm_framebuffer *fb, >> + struct drm_rect *rect)) >> { >> struct drm_framebuffer *fb = plane_state->fb; >> struct drm_rect rect = { >> @@ -305,7 +308,10 @@ void mipi_dbi_enable_flush(struct mipi_dbi *mipi, >> }; >> >> mipi->enabled = true; >> - mipi_dbi_fb_dirty(fb, &rect); >> + if (!dirty) { >> + dirty = mipi_dbi_fb_dirty; >> + } >> + dirty(fb, &rect); >> backlight_enable(mipi->backlight); >> } >> EXPORT_SYMBOL(mipi_dbi_enable_flush); >> diff --git a/drivers/gpu/drm/tinydrm/st7586.c >> b/drivers/gpu/drm/tinydrm/st7586.c >> index 50e370ce5a59..d6ffac64822f 100644 >> --- a/drivers/gpu/drm/tinydrm/st7586.c >> +++ b/drivers/gpu/drm/tinydrm/st7586.c >> @@ -234,7 +234,7 @@ static void st7586_pipe_enable(struct >> drm_simple_display_pipe *pipe, >> >> mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON); >> >> - mipi_dbi_enable_flush(mipi, crtc_state, plane_state); >> + mipi_dbi_enable_flush(mipi, crtc_state, plane_state, >> st7586_fb_dirty); >> } >> >> static void st7586_pipe_disable(struct drm_simple_display_pipe *pipe) >> diff --git a/drivers/gpu/drm/tinydrm/st7735r.c >> b/drivers/gpu/drm/tinydrm/st7735r.c >> index 3bab9a9569a6..dee143007048 100644 >> --- a/drivers/gpu/drm/tinydrm/st7735r.c >> +++ b/drivers/gpu/drm/tinydrm/st7735r.c >> @@ -100,7 +100,7 @@ static void jd_t18003_t01_pipe_enable(struct >> drm_simple_display_pipe *pipe, >> >> msleep(20); >> >> - mipi_dbi_enable_flush(mipi, crtc_state, plane_state); >> + mipi_dbi_enable_flush(mipi, crtc_state, plane_state, NULL); >> } >> >> static const struct drm_simple_display_pipe_funcs >> jd_t18003_t01_pipe_funcs = { >> diff --git a/include/drm/tinydrm/mipi-dbi.h >> b/include/drm/tinydrm/mipi-dbi.h >> index f4ec2834bc22..3d9f003fdcc6 100644 >> --- a/include/drm/tinydrm/mipi-dbi.h >> +++ b/include/drm/tinydrm/mipi-dbi.h >> @@ -72,7 +72,9 @@ void mipi_dbi_pipe_update(struct >> drm_simple_display_pipe *pipe, >> struct drm_plane_state *old_state); >> void mipi_dbi_enable_flush(struct mipi_dbi *mipi, >> struct drm_crtc_state *crtc_state, >> - struct drm_plane_state *plan_state); >> + struct drm_plane_state *plan_state, >> + void (*dirty)(struct drm_framebuffer *fb, >> + struct drm_rect *rect)); >> void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe); >> void mipi_dbi_hw_reset(struct mipi_dbi *mipi); >> bool mipi_dbi_display_is_on(struct mipi_dbi *mipi); > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c index aeb93eadb047..614f532ea89f 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c @@ -39,31 +39,17 @@ * and registers the DRM device using devm_tinydrm_register(). */ -static struct drm_framebuffer * -tinydrm_fb_create(struct drm_device *drm, struct drm_file *file_priv, - const struct drm_mode_fb_cmd2 *mode_cmd) -{ - struct tinydrm_device *tdev = drm->dev_private; - - return drm_gem_fb_create_with_funcs(drm, file_priv, mode_cmd, - tdev->fb_funcs); -} - static const struct drm_mode_config_funcs tinydrm_mode_config_funcs = { - .fb_create = tinydrm_fb_create, + .fb_create = drm_gem_fb_create_with_dirty, .atomic_check = drm_atomic_helper_check, .atomic_commit = drm_atomic_helper_commit, }; static int tinydrm_init(struct device *parent, struct tinydrm_device *tdev, - const struct drm_framebuffer_funcs *fb_funcs, struct drm_driver *driver) { struct drm_device *drm; - mutex_init(&tdev->dirty_lock); - tdev->fb_funcs = fb_funcs; - /* * We don't embed drm_device, because that prevent us from using * devm_kzalloc() to allocate tinydrm_device in the driver since @@ -86,7 +72,6 @@ static int tinydrm_init(struct device *parent, struct tinydrm_device *tdev, static void tinydrm_fini(struct tinydrm_device *tdev) { drm_mode_config_cleanup(tdev->drm); - mutex_destroy(&tdev->dirty_lock); tdev->drm->dev_private = NULL; drm_dev_put(tdev->drm); } @@ -100,7 +85,6 @@ static void devm_tinydrm_release(void *data) * devm_tinydrm_init - Initialize tinydrm device * @parent: Parent device object * @tdev: tinydrm device - * @fb_funcs: Framebuffer functions * @driver: DRM driver * * This function initializes @tdev, the underlying DRM device and it's @@ -111,12 +95,11 @@ static void devm_tinydrm_release(void *data) * Zero on success, negative error code on failure. */ int devm_tinydrm_init(struct device *parent, struct tinydrm_device *tdev, - const struct drm_framebuffer_funcs *fb_funcs, struct drm_driver *driver) { int ret; - ret = tinydrm_init(parent, tdev, fb_funcs, driver); + ret = tinydrm_init(parent, tdev, driver); if (ret) return ret; diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c index d0ece6ad4a1c..2737b6fdadc8 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c @@ -17,104 +17,15 @@ #include <drm/drm_device.h> #include <drm/drm_drv.h> #include <drm/drm_fourcc.h> +#include <drm/drm_framebuffer.h> #include <drm/drm_print.h> #include <drm/drm_rect.h> -#include <drm/tinydrm/tinydrm.h> #include <drm/tinydrm/tinydrm-helpers.h> -#include <uapi/drm/drm.h> static unsigned int spi_max; module_param(spi_max, uint, 0400); MODULE_PARM_DESC(spi_max, "Set a lower SPI max transfer size"); -/** - * tinydrm_merge_clips - Merge clip rectangles - * @dst: Destination clip rectangle - * @src: Source clip rectangle(s) - * @num_clips: Number of @src clip rectangles - * @flags: Dirty fb ioctl flags - * @max_width: Maximum width of @dst - * @max_height: Maximum height of @dst - * - * This function merges @src clip rectangle(s) into @dst. If @src is NULL, - * @max_width and @min_width is used to set a full @dst clip rectangle. - * - * Returns: - * true if it's a full clip, false otherwise - */ -bool tinydrm_merge_clips(struct drm_rect *dst, - struct drm_clip_rect *src, unsigned int num_clips, - unsigned int flags, u32 max_width, u32 max_height) -{ - unsigned int i; - - if (!src || !num_clips) { - dst->x1 = 0; - dst->x2 = max_width; - dst->y1 = 0; - dst->y2 = max_height; - return true; - } - - dst->x1 = ~0; - dst->y1 = ~0; - dst->x2 = 0; - dst->y2 = 0; - - for (i = 0; i < num_clips; i++) { - if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY) - i++; - dst->x1 = min_t(int, dst->x1, src[i].x1); - dst->x2 = max_t(int, dst->x2, src[i].x2); - dst->y1 = min_t(int, dst->y1, src[i].y1); - dst->y2 = max_t(int, dst->y2, src[i].y2); - } - - if (dst->x2 > max_width || dst->y2 > max_height || - dst->x1 >= dst->x2 || dst->y1 >= dst->y2) { - DRM_DEBUG_KMS("Illegal clip: x1=%u, x2=%u, y1=%u, y2=%u\n", - dst->x1, dst->x2, dst->y1, dst->y2); - dst->x1 = 0; - dst->y1 = 0; - dst->x2 = max_width; - dst->y2 = max_height; - } - - return (dst->x2 - dst->x1) == max_width && - (dst->y2 - dst->y1) == max_height; -} -EXPORT_SYMBOL(tinydrm_merge_clips); - -int tinydrm_fb_dirty(struct drm_framebuffer *fb, - struct drm_file *file_priv, - unsigned int flags, unsigned int color, - struct drm_clip_rect *clips, - unsigned int num_clips) -{ - struct tinydrm_device *tdev = fb->dev->dev_private; - struct drm_plane *plane = &tdev->pipe.plane; - int ret = 0; - - drm_modeset_lock(&plane->mutex, NULL); - - /* fbdev can flush even when we're not interested */ - if (plane->state->fb == fb) { - mutex_lock(&tdev->dirty_lock); - ret = tdev->fb_dirty(fb, file_priv, flags, - color, clips, num_clips); - mutex_unlock(&tdev->dirty_lock); - } - - drm_modeset_unlock(&plane->mutex); - - if (ret) - dev_err_once(fb->dev->dev, - "Failed to update display %d\n", ret); - - return ret; -} -EXPORT_SYMBOL(tinydrm_fb_dirty); - /** * tinydrm_memcpy - Copy clip buffer * @dst: Destination buffer diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c index d4576d6e8ce4..a4796ddb08f0 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c @@ -111,36 +111,6 @@ tinydrm_connector_create(struct drm_device *drm, return connector; } -/** - * tinydrm_display_pipe_update - Display pipe update helper - * @pipe: Simple display pipe - * @old_state: Old plane state - * - * This function does a full framebuffer flush if the plane framebuffer - * has changed. It also handles vblank events. Drivers can use this as their - * &drm_simple_display_pipe_funcs->update callback. - */ -void tinydrm_display_pipe_update(struct drm_simple_display_pipe *pipe, - struct drm_plane_state *old_state) -{ - struct tinydrm_device *tdev = pipe_to_tinydrm(pipe); - struct drm_framebuffer *fb = pipe->plane.state->fb; - struct drm_crtc *crtc = &tdev->pipe.crtc; - - if (fb && (fb != old_state->fb)) { - if (tdev->fb_dirty) - tdev->fb_dirty(fb, NULL, 0, 0, NULL, 0); - } - - if (crtc->state->event) { - spin_lock_irq(&crtc->dev->event_lock); - drm_crtc_send_vblank_event(crtc, crtc->state->event); - spin_unlock_irq(&crtc->dev->event_lock); - crtc->state->event = NULL; - } -} -EXPORT_SYMBOL(tinydrm_display_pipe_update); - static int tinydrm_rotate_mode(struct drm_display_mode *mode, unsigned int rotation) { diff --git a/drivers/gpu/drm/tinydrm/hx8357d.c b/drivers/gpu/drm/tinydrm/hx8357d.c index 3ae11aa4b73b..8bbd0beafc6a 100644 --- a/drivers/gpu/drm/tinydrm/hx8357d.c +++ b/drivers/gpu/drm/tinydrm/hx8357d.c @@ -176,7 +176,7 @@ static void yx240qv29_enable(struct drm_simple_display_pipe *pipe, static const struct drm_simple_display_pipe_funcs hx8357d_pipe_funcs = { .enable = yx240qv29_enable, .disable = mipi_dbi_pipe_disable, - .update = tinydrm_display_pipe_update, + .update = mipi_dbi_pipe_update, .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb, }; diff --git a/drivers/gpu/drm/tinydrm/ili9225.c b/drivers/gpu/drm/tinydrm/ili9225.c index ad644069089f..aa1376a22bda 100644 --- a/drivers/gpu/drm/tinydrm/ili9225.c +++ b/drivers/gpu/drm/tinydrm/ili9225.c @@ -20,6 +20,7 @@ #include <linux/spi/spi.h> #include <video/mipi_display.h> +#include <drm/drm_damage_helper.h> #include <drm/drm_drv.h> #include <drm/drm_fb_cma_helper.h> #include <drm/drm_fourcc.h> @@ -76,17 +77,14 @@ static inline int ili9225_command(struct mipi_dbi *mipi, u8 cmd, u16 data) return mipi_dbi_command_buf(mipi, cmd, par, 2); } -static int ili9225_fb_dirty(struct drm_framebuffer *fb, - struct drm_file *file_priv, unsigned int flags, - unsigned int color, struct drm_clip_rect *clips, - unsigned int num_clips) +static void ili9225_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect) { struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0); struct tinydrm_device *tdev = fb->dev->dev_private; struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev); + unsigned int height = rect->y2 - rect->y1; + unsigned int width = rect->x2 - rect->x1; bool swap = mipi->swap_bytes; - struct drm_rect clip; - struct drm_rect *rect = &clip; u16 x_start, y_start; u16 x1, x2, y1, y2; int ret = 0; @@ -94,10 +92,9 @@ static int ili9225_fb_dirty(struct drm_framebuffer *fb, void *tr; if (!mipi->enabled) - return 0; + return; - full = tinydrm_merge_clips(&clip, clips, num_clips, flags, - fb->width, fb->height); + full = width == fb->width && height == fb->height; DRM_DEBUG_KMS("Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id, DRM_RECT_ARG(rect)); @@ -106,7 +103,7 @@ static int ili9225_fb_dirty(struct drm_framebuffer *fb, tr = mipi->tx_buf; ret = mipi_dbi_buf_copy(mipi->tx_buf, fb, rect, swap); if (ret) - return ret; + goto err_msg; } else { tr = cma_obj->vaddr; } @@ -155,16 +152,29 @@ static int ili9225_fb_dirty(struct drm_framebuffer *fb, ili9225_command(mipi, ILI9225_RAM_ADDRESS_SET_2, y_start); ret = mipi_dbi_command_buf(mipi, ILI9225_WRITE_DATA_TO_GRAM, tr, - (rect->x2 - rect->x1) * (rect->y2 - rect->y1) * 2); - - return ret; + width * height * 2); +err_msg: + if (ret) + dev_err_once(fb->dev->dev, "Failed to update display %d\n", ret); } -static const struct drm_framebuffer_funcs ili9225_fb_funcs = { - .destroy = drm_gem_fb_destroy, - .create_handle = drm_gem_fb_create_handle, - .dirty = tinydrm_fb_dirty, -}; +static void ili9225_pipe_update(struct drm_simple_display_pipe *pipe, + struct drm_plane_state *old_state) +{ + struct drm_plane_state *state = pipe->plane.state; + struct drm_crtc *crtc = &pipe->crtc; + struct drm_rect rect; + + if (drm_atomic_helper_damage_merged(old_state, state, &rect)) + ili9225_fb_dirty(state->fb, &rect); + + if (crtc->state->event) { + spin_lock_irq(&crtc->dev->event_lock); + drm_crtc_send_vblank_event(crtc, crtc->state->event); + spin_unlock_irq(&crtc->dev->event_lock); + crtc->state->event = NULL; + } +} static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe, struct drm_crtc_state *crtc_state, @@ -305,59 +315,10 @@ static int ili9225_dbi_command(struct mipi_dbi *mipi, u8 cmd, u8 *par, return tinydrm_spi_transfer(spi, speed_hz, NULL, bpw, par, num); } -static const u32 ili9225_formats[] = { - DRM_FORMAT_RGB565, - DRM_FORMAT_XRGB8888, -}; - -static int ili9225_init(struct device *dev, struct mipi_dbi *mipi, - const struct drm_simple_display_pipe_funcs *pipe_funcs, - struct drm_driver *driver, - const struct drm_display_mode *mode, - unsigned int rotation) -{ - size_t bufsize = mode->vdisplay * mode->hdisplay * sizeof(u16); - struct tinydrm_device *tdev = &mipi->tinydrm; - int ret; - - if (!mipi->command) - return -EINVAL; - - mutex_init(&mipi->cmdlock); - - mipi->tx_buf = devm_kmalloc(dev, bufsize, GFP_KERNEL); - if (!mipi->tx_buf) - return -ENOMEM; - - ret = devm_tinydrm_init(dev, tdev, &ili9225_fb_funcs, driver); - if (ret) - return ret; - - tdev->fb_dirty = ili9225_fb_dirty; - - ret = tinydrm_display_pipe_init(tdev, pipe_funcs, - DRM_MODE_CONNECTOR_VIRTUAL, - ili9225_formats, - ARRAY_SIZE(ili9225_formats), mode, - rotation); - if (ret) - return ret; - - tdev->drm->mode_config.preferred_depth = 16; - mipi->rotation = rotation; - - drm_mode_config_reset(tdev->drm); - - DRM_DEBUG_KMS("preferred_depth=%u, rotation = %u\n", - tdev->drm->mode_config.preferred_depth, rotation); - - return 0; -} - static const struct drm_simple_display_pipe_funcs ili9225_pipe_funcs = { .enable = ili9225_pipe_enable, .disable = ili9225_pipe_disable, - .update = tinydrm_display_pipe_update, + .update = ili9225_pipe_update, .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb, }; @@ -424,8 +385,8 @@ static int ili9225_probe(struct spi_device *spi) /* override the command function set in mipi_dbi_spi_init() */ mipi->command = ili9225_dbi_command; - ret = ili9225_init(&spi->dev, mipi, &ili9225_pipe_funcs, - &ili9225_driver, &ili9225_mode, rotation); + ret = mipi_dbi_init(&spi->dev, mipi, &ili9225_pipe_funcs, + &ili9225_driver, &ili9225_mode, rotation); if (ret) return ret; diff --git a/drivers/gpu/drm/tinydrm/ili9341.c b/drivers/gpu/drm/tinydrm/ili9341.c index bcdf10906ade..713bb2dd7e04 100644 --- a/drivers/gpu/drm/tinydrm/ili9341.c +++ b/drivers/gpu/drm/tinydrm/ili9341.c @@ -132,7 +132,7 @@ static void yx240qv29_enable(struct drm_simple_display_pipe *pipe, static const struct drm_simple_display_pipe_funcs ili9341_pipe_funcs = { .enable = yx240qv29_enable, .disable = mipi_dbi_pipe_disable, - .update = tinydrm_display_pipe_update, + .update = mipi_dbi_pipe_update, .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb, }; diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c index 97805ca37a04..82a92ec9ae3c 100644 --- a/drivers/gpu/drm/tinydrm/mi0283qt.c +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c @@ -140,7 +140,7 @@ static void mi0283qt_enable(struct drm_simple_display_pipe *pipe, static const struct drm_simple_display_pipe_funcs mi0283qt_pipe_funcs = { .enable = mi0283qt_enable, .disable = mipi_dbi_pipe_disable, - .update = tinydrm_display_pipe_update, + .update = mipi_dbi_pipe_update, .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb, }; diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c index 1e7e8cfc99f0..680692a83f94 100644 --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c @@ -17,6 +17,7 @@ #include <linux/regulator/consumer.h> #include <linux/spi/spi.h> +#include <drm/drm_damage_helper.h> #include <drm/drm_drv.h> #include <drm/drm_fb_cma_helper.h> #include <drm/drm_gem_cma_helper.h> @@ -25,7 +26,6 @@ #include <drm/drm_rect.h> #include <drm/tinydrm/mipi-dbi.h> #include <drm/tinydrm/tinydrm-helpers.h> -#include <uapi/drm/drm.h> #include <video/mipi_display.h> #define MIPI_DBI_MAX_SPI_READ_SPEED 2000000 /* 2MHz */ @@ -212,27 +212,22 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb, } EXPORT_SYMBOL(mipi_dbi_buf_copy); -static int mipi_dbi_fb_dirty(struct drm_framebuffer *fb, - struct drm_file *file_priv, - unsigned int flags, unsigned int color, - struct drm_clip_rect *clips, - unsigned int num_clips) +static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect) { struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0); struct tinydrm_device *tdev = fb->dev->dev_private; struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev); + unsigned int height = rect->y2 - rect->y1; + unsigned int width = rect->x2 - rect->x1; bool swap = mipi->swap_bytes; - struct drm_rect clip; - struct drm_rect *rect = &clip; int ret = 0; bool full; void *tr; if (!mipi->enabled) - return 0; + return; - full = tinydrm_merge_clips(&clip, clips, num_clips, flags, - fb->width, fb->height); + full = width == fb->width && height == fb->height; DRM_DEBUG_KMS("Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id, DRM_RECT_ARG(rect)); @@ -241,7 +236,7 @@ static int mipi_dbi_fb_dirty(struct drm_framebuffer *fb, tr = mipi->tx_buf; ret = mipi_dbi_buf_copy(mipi->tx_buf, fb, rect, swap); if (ret) - return ret; + goto err_msg; } else { tr = cma_obj->vaddr; } @@ -254,16 +249,38 @@ static int mipi_dbi_fb_dirty(struct drm_framebuffer *fb, ((rect->y2 - 1) >> 8) & 0xff, (rect->y2 - 1) & 0xff); ret = mipi_dbi_command_buf(mipi, MIPI_DCS_WRITE_MEMORY_START, tr, - (rect->x2 - rect->x1) * (rect->y2 - rect->y1) * 2); - - return ret; + width * height * 2); +err_msg: + if (ret) + dev_err_once(fb->dev->dev, "Failed to update display %d\n", ret); } -static const struct drm_framebuffer_funcs mipi_dbi_fb_funcs = { - .destroy = drm_gem_fb_destroy, - .create_handle = drm_gem_fb_create_handle, - .dirty = tinydrm_fb_dirty, -}; +/** + * mipi_dbi_pipe_update - Display pipe update helper + * @pipe: Simple display pipe + * @old_state: Old plane state + * + * This function handles framebuffer flushing and vblank events. Drivers can use + * this as their &drm_simple_display_pipe_funcs->update callback. + */ +void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe, + struct drm_plane_state *old_state) +{ + struct drm_plane_state *state = pipe->plane.state; + struct drm_crtc *crtc = &pipe->crtc; + struct drm_rect rect; + + if (drm_atomic_helper_damage_merged(old_state, state, &rect)) + mipi_dbi_fb_dirty(state->fb, &rect); + + if (crtc->state->event) { + spin_lock_irq(&crtc->dev->event_lock); + drm_crtc_send_vblank_event(crtc, crtc->state->event); + spin_unlock_irq(&crtc->dev->event_lock); + crtc->state->event = NULL; + } +} +EXPORT_SYMBOL(mipi_dbi_pipe_update); /** * mipi_dbi_enable_flush - MIPI DBI enable helper @@ -279,13 +296,16 @@ void mipi_dbi_enable_flush(struct mipi_dbi *mipi, struct drm_crtc_state *crtc_state, struct drm_plane_state *plane_state) { - struct tinydrm_device *tdev = &mipi->tinydrm; struct drm_framebuffer *fb = plane_state->fb; + struct drm_rect rect = { + .x1 = 0, + .x2 = fb->width, + .y1 = 0, + .y2 = fb->height, + }; mipi->enabled = true; - if (fb) - tdev->fb_dirty(fb, NULL, 0, 0, NULL, 0); - + mipi_dbi_fb_dirty(fb, &rect); backlight_enable(mipi->backlight); } EXPORT_SYMBOL(mipi_dbi_enable_flush); @@ -377,12 +397,10 @@ int mipi_dbi_init(struct device *dev, struct mipi_dbi *mipi, if (!mipi->tx_buf) return -ENOMEM; - ret = devm_tinydrm_init(dev, tdev, &mipi_dbi_fb_funcs, driver); + ret = devm_tinydrm_init(dev, tdev, driver); if (ret) return ret; - tdev->fb_dirty = mipi_dbi_fb_dirty; - /* TODO: Maybe add DRM_MODE_CONNECTOR_SPI */ ret = tinydrm_display_pipe_init(tdev, pipe_funcs, DRM_MODE_CONNECTOR_VIRTUAL, @@ -392,6 +410,8 @@ int mipi_dbi_init(struct device *dev, struct mipi_dbi *mipi, if (ret) return ret; + drm_plane_enable_fb_damage_clips(&tdev->pipe.plane); + tdev->drm->mode_config.preferred_depth = 16; mipi->rotation = rotation; diff --git a/drivers/gpu/drm/tinydrm/repaper.c b/drivers/gpu/drm/tinydrm/repaper.c index 238515de449e..f9cacf49f16b 100644 --- a/drivers/gpu/drm/tinydrm/repaper.c +++ b/drivers/gpu/drm/tinydrm/repaper.c @@ -26,6 +26,7 @@ #include <linux/spi/spi.h> #include <linux/thermal.h> +#include <drm/drm_damage_helper.h> #include <drm/drm_drv.h> #include <drm/drm_fb_cma_helper.h> #include <drm/drm_gem_cma_helper.h> @@ -523,11 +524,7 @@ static void repaper_gray8_to_mono_reversed(u8 *buf, u32 width, u32 height) } } -static int repaper_fb_dirty(struct drm_framebuffer *fb, - struct drm_file *file_priv, - unsigned int flags, unsigned int color, - struct drm_clip_rect *clips, - unsigned int num_clips) +static int repaper_fb_dirty(struct drm_framebuffer *fb) { struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0); struct dma_buf_attachment *import_attach = cma_obj->base.import_attach; @@ -626,12 +623,6 @@ static int repaper_fb_dirty(struct drm_framebuffer *fb, return ret; } -static const struct drm_framebuffer_funcs repaper_fb_funcs = { - .destroy = drm_gem_fb_destroy, - .create_handle = drm_gem_fb_create_handle, - .dirty = tinydrm_fb_dirty, -}; - static void power_off(struct repaper_epd *epd) { /* Turn off power and all signals */ @@ -795,9 +786,7 @@ static void repaper_pipe_disable(struct drm_simple_display_pipe *pipe) DRM_DEBUG_DRIVER("\n"); - mutex_lock(&tdev->dirty_lock); epd->enabled = false; - mutex_unlock(&tdev->dirty_lock); /* Nothing frame */ for (line = 0; line < epd->height; line++) @@ -840,10 +829,28 @@ static void repaper_pipe_disable(struct drm_simple_display_pipe *pipe) power_off(epd); } +static void repaper_pipe_update(struct drm_simple_display_pipe *pipe, + struct drm_plane_state *old_state) +{ + struct drm_plane_state *state = pipe->plane.state; + struct drm_crtc *crtc = &pipe->crtc; + struct drm_rect rect; + + if (drm_atomic_helper_damage_merged(old_state, state, &rect)) + repaper_fb_dirty(state->fb); + + if (crtc->state->event) { + spin_lock_irq(&crtc->dev->event_lock); + drm_crtc_send_vblank_event(crtc, crtc->state->event); + spin_unlock_irq(&crtc->dev->event_lock); + crtc->state->event = NULL; + } +} + static const struct drm_simple_display_pipe_funcs repaper_pipe_funcs = { .enable = repaper_pipe_enable, .disable = repaper_pipe_disable, - .update = tinydrm_display_pipe_update, + .update = repaper_pipe_update, .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb, }; @@ -1057,12 +1064,10 @@ static int repaper_probe(struct spi_device *spi) tdev = &epd->tinydrm; - ret = devm_tinydrm_init(dev, tdev, &repaper_fb_funcs, &repaper_driver); + ret = devm_tinydrm_init(dev, tdev, &repaper_driver); if (ret) return ret; - tdev->fb_dirty = repaper_fb_dirty; - ret = tinydrm_display_pipe_init(tdev, &repaper_pipe_funcs, DRM_MODE_CONNECTOR_VIRTUAL, repaper_formats, diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c index a52bc3b02606..50e370ce5a59 100644 --- a/drivers/gpu/drm/tinydrm/st7586.c +++ b/drivers/gpu/drm/tinydrm/st7586.c @@ -17,6 +17,7 @@ #include <linux/spi/spi.h> #include <video/mipi_display.h> +#include <drm/drm_damage_helper.h> #include <drm/drm_drv.h> #include <drm/drm_fb_cma_helper.h> #include <drm/drm_gem_cma_helper.h> @@ -112,23 +113,15 @@ static int st7586_buf_copy(void *dst, struct drm_framebuffer *fb, return ret; } -static int st7586_fb_dirty(struct drm_framebuffer *fb, - struct drm_file *file_priv, unsigned int flags, - unsigned int color, struct drm_clip_rect *clips, - unsigned int num_clips) +static void st7586_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect) { struct tinydrm_device *tdev = fb->dev->dev_private; struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev); - struct drm_rect clip; - struct drm_rect *rect = &clip; int start, end; int ret = 0; if (!mipi->enabled) - return 0; - - tinydrm_merge_clips(&clip, clips, num_clips, flags, fb->width, - fb->height); + return; /* 3 pixels per byte, so grow clip to nearest multiple of 3 */ rect->x1 = rounddown(rect->x1, 3); @@ -138,7 +131,7 @@ static int st7586_fb_dirty(struct drm_framebuffer *fb, ret = st7586_buf_copy(mipi->tx_buf, fb, rect); if (ret) - return ret; + goto err_msg; /* Pixels are packed 3 per byte */ start = rect->x1 / 3; @@ -154,15 +147,28 @@ static int st7586_fb_dirty(struct drm_framebuffer *fb, ret = mipi_dbi_command_buf(mipi, MIPI_DCS_WRITE_MEMORY_START, (u8 *)mipi->tx_buf, (end - start) * (rect->y2 - rect->y1)); - - return ret; +err_msg: + if (ret) + dev_err_once(fb->dev->dev, "Failed to update display %d\n", ret); } -static const struct drm_framebuffer_funcs st7586_fb_funcs = { - .destroy = drm_gem_fb_destroy, - .create_handle = drm_gem_fb_create_handle, - .dirty = tinydrm_fb_dirty, -}; +static void st7586_pipe_update(struct drm_simple_display_pipe *pipe, + struct drm_plane_state *old_state) +{ + struct drm_plane_state *state = pipe->plane.state; + struct drm_crtc *crtc = &pipe->crtc; + struct drm_rect rect; + + if (drm_atomic_helper_damage_merged(old_state, state, &rect)) + st7586_fb_dirty(state->fb, &rect); + + if (crtc->state->event) { + spin_lock_irq(&crtc->dev->event_lock); + drm_crtc_send_vblank_event(crtc, crtc->state->event); + spin_unlock_irq(&crtc->dev->event_lock); + crtc->state->event = NULL; + } +} static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe, struct drm_crtc_state *crtc_state, @@ -264,12 +270,10 @@ static int st7586_init(struct device *dev, struct mipi_dbi *mipi, if (!mipi->tx_buf) return -ENOMEM; - ret = devm_tinydrm_init(dev, tdev, &st7586_fb_funcs, driver); + ret = devm_tinydrm_init(dev, tdev, driver); if (ret) return ret; - tdev->fb_dirty = st7586_fb_dirty; - ret = tinydrm_display_pipe_init(tdev, pipe_funcs, DRM_MODE_CONNECTOR_VIRTUAL, st7586_formats, @@ -278,6 +282,8 @@ static int st7586_init(struct device *dev, struct mipi_dbi *mipi, if (ret) return ret; + drm_plane_enable_fb_damage_clips(&tdev->pipe.plane); + tdev->drm->mode_config.preferred_depth = 32; mipi->rotation = rotation; @@ -292,7 +298,7 @@ static int st7586_init(struct device *dev, struct mipi_dbi *mipi, static const struct drm_simple_display_pipe_funcs st7586_pipe_funcs = { .enable = st7586_pipe_enable, .disable = st7586_pipe_disable, - .update = tinydrm_display_pipe_update, + .update = st7586_pipe_update, .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb, }; diff --git a/drivers/gpu/drm/tinydrm/st7735r.c b/drivers/gpu/drm/tinydrm/st7735r.c index 9bc93d5a0401..3bab9a9569a6 100644 --- a/drivers/gpu/drm/tinydrm/st7735r.c +++ b/drivers/gpu/drm/tinydrm/st7735r.c @@ -106,7 +106,7 @@ static void jd_t18003_t01_pipe_enable(struct drm_simple_display_pipe *pipe, static const struct drm_simple_display_pipe_funcs jd_t18003_t01_pipe_funcs = { .enable = jd_t18003_t01_pipe_enable, .disable = mipi_dbi_pipe_disable, - .update = tinydrm_display_pipe_update, + .update = mipi_dbi_pipe_update, .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb, }; diff --git a/include/drm/tinydrm/mipi-dbi.h b/include/drm/tinydrm/mipi-dbi.h index b52f32897f23..f4ec2834bc22 100644 --- a/include/drm/tinydrm/mipi-dbi.h +++ b/include/drm/tinydrm/mipi-dbi.h @@ -68,6 +68,8 @@ int mipi_dbi_init(struct device *dev, struct mipi_dbi *mipi, const struct drm_simple_display_pipe_funcs *pipe_funcs, struct drm_driver *driver, const struct drm_display_mode *mode, unsigned int rotation); +void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe, + struct drm_plane_state *old_state); void mipi_dbi_enable_flush(struct mipi_dbi *mipi, struct drm_crtc_state *crtc_state, struct drm_plane_state *plan_state); diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h index 8edb75df4e31..f0d598789e4d 100644 --- a/include/drm/tinydrm/tinydrm-helpers.h +++ b/include/drm/tinydrm/tinydrm-helpers.h @@ -11,8 +11,7 @@ #define __LINUX_TINYDRM_HELPERS_H struct backlight_device; -struct tinydrm_device; -struct drm_clip_rect; +struct drm_framebuffer; struct drm_rect; struct spi_transfer; struct spi_message; @@ -34,14 +33,6 @@ static inline bool tinydrm_machine_little_endian(void) #endif } -bool tinydrm_merge_clips(struct drm_rect *dst, - struct drm_clip_rect *src, unsigned int num_clips, - unsigned int flags, u32 max_width, u32 max_height); -int tinydrm_fb_dirty(struct drm_framebuffer *fb, - struct drm_file *file_priv, - unsigned int flags, unsigned int color, - struct drm_clip_rect *clips, - unsigned int num_clips); void tinydrm_memcpy(void *dst, void *vaddr, struct drm_framebuffer *fb, struct drm_rect *clip); void tinydrm_swab16(u16 *dst, void *vaddr, struct drm_framebuffer *fb, diff --git a/include/drm/tinydrm/tinydrm.h b/include/drm/tinydrm/tinydrm.h index 448aa5ea4722..5621688edcc0 100644 --- a/include/drm/tinydrm/tinydrm.h +++ b/include/drm/tinydrm/tinydrm.h @@ -10,14 +10,9 @@ #ifndef __LINUX_TINYDRM_H #define __LINUX_TINYDRM_H -#include <linux/mutex.h> #include <drm/drm_simple_kms_helper.h> -struct drm_clip_rect; struct drm_driver; -struct drm_file; -struct drm_framebuffer; -struct drm_framebuffer_funcs; /** * struct tinydrm_device - tinydrm device @@ -32,24 +27,6 @@ struct tinydrm_device { * @pipe: Display pipe structure */ struct drm_simple_display_pipe pipe; - - /** - * @dirty_lock: Serializes framebuffer flushing - */ - struct mutex dirty_lock; - - /** - * @fb_funcs: Framebuffer functions used when creating framebuffers - */ - const struct drm_framebuffer_funcs *fb_funcs; - - /** - * @fb_dirty: Framebuffer dirty callback - */ - int (*fb_dirty)(struct drm_framebuffer *framebuffer, - struct drm_file *file_priv, unsigned flags, - unsigned color, struct drm_clip_rect *clips, - unsigned num_clips); }; static inline struct tinydrm_device * @@ -82,13 +59,10 @@ pipe_to_tinydrm(struct drm_simple_display_pipe *pipe) .clock = 1 /* pass validation */ int devm_tinydrm_init(struct device *parent, struct tinydrm_device *tdev, - const struct drm_framebuffer_funcs *fb_funcs, struct drm_driver *driver); int devm_tinydrm_register(struct tinydrm_device *tdev); void tinydrm_shutdown(struct tinydrm_device *tdev); -void tinydrm_display_pipe_update(struct drm_simple_display_pipe *pipe, - struct drm_plane_state *old_state); int tinydrm_display_pipe_init(struct tinydrm_device *tdev, const struct drm_simple_display_pipe_funcs *funcs,
This switches to drm_atomic_helper_dirtyfb() as the framebuffer dirty handler. All flushing will now happen in the pipe functions. Also enable the damage plane property for all except repaper which can only do full updates. ili9225: This change made ili9225_init() equal to mipi_dbi_init() so use it. v2: Remove fb check in mipi_dbi_enable_flush() it can't be NULL (kbuild test robot) Cc: David Lechner <david@lechnology.com> Cc: Eric Anholt <eric@anholt.net> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> Acked-by: Sam Ravnborg <sam@ravnborg.org> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/tinydrm/core/tinydrm-core.c | 21 +--- .../gpu/drm/tinydrm/core/tinydrm-helpers.c | 91 +--------------- drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c | 30 ------ drivers/gpu/drm/tinydrm/hx8357d.c | 2 +- drivers/gpu/drm/tinydrm/ili9225.c | 101 ++++++------------ drivers/gpu/drm/tinydrm/ili9341.c | 2 +- drivers/gpu/drm/tinydrm/mi0283qt.c | 2 +- drivers/gpu/drm/tinydrm/mipi-dbi.c | 74 ++++++++----- drivers/gpu/drm/tinydrm/repaper.c | 39 ++++--- drivers/gpu/drm/tinydrm/st7586.c | 50 +++++---- drivers/gpu/drm/tinydrm/st7735r.c | 2 +- include/drm/tinydrm/mipi-dbi.h | 2 + include/drm/tinydrm/tinydrm-helpers.h | 11 +- include/drm/tinydrm/tinydrm.h | 26 ----- 14 files changed, 138 insertions(+), 315 deletions(-)