Hi Javier, On Thu, Jul 13, 2023 at 4:13 PM Javier Martinez Canillas <javierm@xxxxxxxxxx> wrote: > Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> writes: > > 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. [...] > 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 */ > Thanks, that works! 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