Re: [PATCH v2 4/5] drm/ssd130x: Don't allocate buffers on each plane update

From: Geert Uytterhoeven
Date: Thu Jul 13 2023 - 09:27:12 EST

Hi Javier,

On Thu, Jul 13, 2023 at 3:21 PM Javier Martinez Canillas
<javierm@xxxxxxxxxx> wrote:
> Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> writes:
> > On Fri, Jun 9, 2023 at 7:09 PM Javier Martinez Canillas
> > <javierm@xxxxxxxxxx> wrote:
> >> The resolutions for these panels are fixed and defined in the Device Tree,
> >> so there's no point to allocate the buffers on each plane update and that
> >> can just be done once.
> >>
> >> Let's do the allocation and free on the encoder enable and disable helpers
> >> since that's where others initialization and teardown operations are done.
> >>
> >> Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
> >> Reviewed-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> >> ---
> >>
> >> (no changes since v1)
> >
> > Thanks for your patch, which is now commit 49d7d581ceaf4cf8
> > ("drm/ssd130x: Don't allocate buffers on each plane update") in
> > drm-misc/for-linux-next.
> >
> >> --- a/drivers/gpu/drm/solomon/ssd130x.c
> >> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> >> @@ -701,14 +709,22 @@ static void ssd130x_encoder_helper_atomic_enable(struct drm_encoder *encoder,
> >> return;
> >>
> >> ret = ssd130x_init(ssd130x);
> >> - if (ret) {
> >> - ssd130x_power_off(ssd130x);
> >> - return;
> >> - }
> >> + if (ret)
> >> + goto power_off;
> >> +
> >> + ret = ssd130x_buf_alloc(ssd130x);
> >
> > This appears to be too late, causing a NULL pointer dereference:
> >
>
> Thanks for reporting this issue.
>
> > [ 59.302761] [<c0303d90>] ssd130x_update_rect.isra.0+0x13c/0x340
> > [ 59.304231] [<c0304200>]
> > ssd130x_primary_plane_helper_atomic_update+0x26c/0x284
> > [ 59.305716] [<c02f8d54>] drm_atomic_helper_commit_planes+0xfc/0x27c
> >
>
> I wonder how this could be too late. I thought that the encoder
> .atomic_enable callback would be called before any plane .atomic_update.
>
> > Bailing out from ssd130x_update_rect() when data_array is still NULL
> > fixes that.
> >
>
> Maybe we can add that with a drm_WARN() ? I still want to understand how
> a plane update can happen before an encoder enable.

Full log is:

ssd130x-i2c 0-003c: supply vcc not found, using dummy regulator
[drm] Initialized ssd130x 1.0.0 20220131 for 0-003c on minor 0
ssd130x-i2c 0-003c: [drm] surface width(128), height(32), bpp(1)
and format(R1 little-endian (0x20203152))
Unable to handle kernel NULL pointer dereference at virtual address 00000000
Oops [#1]
CPU: 0 PID: 1 Comm: swapper Not tainted
6.5.0-rc1-orangecrab-02219-g0a529a1e4bf4 #565
epc : ssd130x_update_rect.isra.0+0x13c/0x340
ra : ssd130x_update_rect.isra.0+0x2bc/0x340
epc : c0303d90 ra : c0303f10 sp : c182b5b0
gp : c06d37f0 tp : c1828000 t0 : 00000064
t1 : 00000000 t2 : 00000000 s0 : c182b600
s1 : c2044000 a0 : 00000000 a1 : 00000000
a2 : 00000008 a3 : a040f080 a4 : 00000000
a5 : 00000000 a6 : 00001000 a7 : 00000008
s2 : 00000004 s3 : 00000080 s4 : c2045000
s5 : 00000010 s6 : 00000080 s7 : 00000000
s8 : 00000000 s9 : a040f000 s10: 00000008
s11: 00000000 t3 : 00000153 t4 : c2050ef4
t5 : c20447a0 t6 : 00000080
status: 00000120 badaddr: 00000000 cause: 0000000f
[<c0303d90>] ssd130x_update_rect.isra.0+0x13c/0x340
[<c0304200>] ssd130x_primary_plane_helper_atomic_update+0x26c/0x284
[<c02f8d54>] drm_atomic_helper_commit_planes+0xfc/0x27c
[<c02f9314>] drm_atomic_helper_commit_tail+0x5c/0xb4
[<c02f94fc>] commit_tail+0x190/0x1b8
[<c02f99fc>] drm_atomic_helper_commit+0x194/0x1c0
[<c02c5d00>] drm_atomic_commit+0xa4/0xe4
[<c02cce40>] drm_client_modeset_commit_atomic+0x244/0x278
[<c02ccef0>] drm_client_modeset_commit_locked+0x7c/0x1bc
[<c02cd064>] drm_client_modeset_commit+0x34/0x64
[<c0301a78>] __drm_fb_helper_restore_fbdev_mode_unlocked+0xc4/0xe8
[<c0303424>] drm_fb_helper_set_par+0x38/0x58
[<c027c410>] fbcon_init+0x294/0x534
[<c02af188>] visual_init+0xac/0x114
[<c02b1834>] do_bind_con_driver.isra.0+0x1bc/0x39c
[<c02b2fcc>] do_take_over_console+0x128/0x1b4
[<c027ad24>] do_fbcon_takeover+0x74/0xfc
[<c027e704>] fbcon_fb_registered+0x168/0x1b4
[<c0275c84>] register_framebuffer+0x180/0x238
[<c03017a4>] __drm_fb_helper_initial_config_and_unlock+0x328/0x538
[<c0303844>] drm_fb_helper_initial_config+0x40/0x54
[<c0300818>] drm_fbdev_generic_client_hotplug+0x98/0xdc
[<c0300c5c>] drm_fbdev_generic_setup+0x9c/0x178
[<c0304fa0>] ssd130x_probe+0x5e0/0x788
[<c030522c>] ssd130x_i2c_probe+0x4c/0x70
[<c0358464>] i2c_device_probe+0x120/0x1f0
[<c030e5a4>] really_probe+0xb8/0x30c
[<c030e974>] __driver_probe_device+0x17c/0x1c8
[<c030ea0c>] driver_probe_device+0x4c/0x140
[<c030ebe4>] __device_attach_driver+0xe4/0x150
[<c030ccc4>] bus_for_each_drv+0x8c/0x100
[<c030f034>] __device_attach+0x12c/0x1ac
[<c030f3e0>] device_initial_probe+0x18/0x28
[<c030cf14>] bus_probe_device+0xcc/0xd0
[<c030b274>] device_add+0x5d8/0x7b4
[<c030b474>] device_register+0x24/0x38
[<c0358f24>] i2c_new_client_device+0x1a8/0x2b8
[<c035b960>] of_i2c_register_devices+0xdc/0x164
[<c0359750>] i2c_register_adapter+0x1b8/0x56c
[<c0359eb0>] i2c_add_adapter+0x94/0x100
[<c035d47c>] __i2c_bit_add_bus+0xc0/0x460
[<c035d838>] i2c_bit_add_bus+0x1c/0x2c
[<c035da20>] litex_i2c_probe+0x108/0x164
[<c0310de4>] platform_probe+0x54/0xb0
[<c030e5a4>] really_probe+0xb8/0x30c
[<c030e974>] __driver_probe_device+0x17c/0x1c8
[<c030ea0c>] driver_probe_device+0x4c/0x140
[<c030ed74>] __driver_attach+0x124/0x1f4
[<c030c880>] bus_for_each_dev+0x84/0xf4
[<c030f4f8>] driver_attach+0x28/0x38
[<c030d174>] bus_add_driver+0x120/0x214
[<c030fc90>] driver_register+0x70/0x15c
[<c0311d98>] __platform_driver_register+0x28/0x38
[<c0520958>] litex_i2c_driver_init+0x24/0x34
[<c0507fe8>] do_one_initcall+0x80/0x238
[<c05083d4>] kernel_init_freeable+0x1b4/0x238
[<c04fdd9c>] kernel_init+0x24/0x144
[<c00022d8>] ret_from_fork+0x10/0x24

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds