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
