Message-ID: <CAMuHMdWmJjGX+s=z0bBMDz0=zqZJHrGhkzWyRkmstS0eW9aiTg@mail.gmail.com>
Date:   Thu, 13 Jul 2023 15:25:49 +0200
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Javier Martinez Canillas <javierm@...hat.com>
Cc:     linux-kernel@...r.kernel.org,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Maxime Ripard <mripard@...nel.org>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...il.com>,
        dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v2 4/5] drm/ssd130x: Don't allocate buffers on each plane update

Hi Javier,

On Thu, Jul 13, 2023 at 3:21 PM Javier Martinez Canillas
<javierm@...hat.com> wrote:
> Geert Uytterhoeven <geert@...ux-m68k.org> writes:
> > On Fri, Jun 9, 2023 at 7:09 PM Javier Martinez Canillas
> > <javierm@...hat.com> 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@...hat.com>
> >> Reviewed-by: Thomas Zimmermann <tzimmermann@...e.de>
> >> ---
> >>
> >> (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@...ux-m68k.org

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

Powered by blists - more mailing lists