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

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> writes:

Hello Geert,

> 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

Thanks for the log, so I think the problem is that the default struct
drm_mode_config_helper_funcs .atomic_commit_tail is
drm_atomic_helper_commit_tail():

https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic_helper.c#L1710

That helper calls drm_atomic_helper_commit_planes() and attempts to commit
the state for all planes even for CRTC that are not enabled. I see that
there is a drm_atomic_helper_commit_tail_rpm() helper that instead calls:

drm_atomic_helper_commit_planes(dev, old_state, DRM_PLANE_COMMIT_ACTIVE_ONLY),
which I thought that was the default behaviour.

Can you please try the following change [0] ? If that works then I can
propose as a proper patch.

[0]:
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index afb08a8aa9fc..c543caa3ceee 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -795,6 +795,10 @@ static const struct drm_mode_config_funcs ssd130x_mode_config_funcs = {
        .atomic_commit = drm_atomic_helper_commit,
 };
 
+static const struct drm_mode_config_helper_funcs ssd130x_mode_config_helpers = {
+        .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
+};
+
 static const uint32_t ssd130x_formats[] = {
        DRM_FORMAT_XRGB8888,
 };
@@ -923,6 +927,7 @@ static int ssd130x_init_modeset(struct ssd130x_device *ssd130x)
        drm->mode_config.max_height = max_height;
        drm->mode_config.preferred_depth = 24;
        drm->mode_config.funcs = &ssd130x_mode_config_funcs;
+       drm->mode_config.helper_private = &ssd130x_mode_config_helpers;
 
        /* Primary plane */
 
-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat