| Message ID | 20230713163213.1028952-1-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/ssd130x: Fix an oops when attempting to update a disabled plane | expand |
Hi Javier, On Thu, Jul 13, 2023 at 6:32 PM Javier Martinez Canillas <javierm@redhat.com> wrote: > Geert reports that the following NULL pointer dereference happens for him > after commit 49d7d581ceaf ("drm/ssd130x: Don't allocate buffers on each > plane update"): > > [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 > ... > 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 > ... > > The problem is that fbcon calls fbcon_init() which triggers a DRM modeset > and this leads to drm_atomic_helper_commit_planes() attempting to commit > the atomic state for all planes, even the ones whose CRTC is not enabled. > > Since the primary plane buffer is allocated in the encoder .atomic_enable > callback, this happens after that initial modeset commit and leads to the > mentioned NULL pointer dereference. > > Fix this by not using the default drm_atomic_helper_commit_tail() helper, > but instead the drm_atomic_helper_commit_tail_rpm() function that doesn't > attempt to commit the atomic state for planes related to inactive CRTCs. > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> Tested-by: Geert Uytterhoeven <geert@linux-m68k.org> > --- 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, The docs say this is intended for drivers that support runtime_pm or need the CRTC to be enabled to perform a commit. Might be worthwhile to add basic Runtime PM, so the I2C controller can go to sleep when the display is not used. > +}; > + > static const uint32_t ssd130x_formats[] = { > DRM_FORMAT_XRGB8888, > }; Gr{oetje,eeting}s, Geert
Geert Uytterhoeven <geert@linux-m68k.org> writes: Hello Geert, > Hi Javier, > > On Thu, Jul 13, 2023 at 6:32 PM Javier Martinez Canillas > <javierm@redhat.com> wrote: >> Geert reports that the following NULL pointer dereference happens for him >> after commit 49d7d581ceaf ("drm/ssd130x: Don't allocate buffers on each >> plane update"): >> >> [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 >> ... >> 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 >> ... >> >> The problem is that fbcon calls fbcon_init() which triggers a DRM modeset >> and this leads to drm_atomic_helper_commit_planes() attempting to commit >> the atomic state for all planes, even the ones whose CRTC is not enabled. >> >> Since the primary plane buffer is allocated in the encoder .atomic_enable >> callback, this happens after that initial modeset commit and leads to the >> mentioned NULL pointer dereference. >> >> Fix this by not using the default drm_atomic_helper_commit_tail() helper, >> but instead the drm_atomic_helper_commit_tail_rpm() function that doesn't >> attempt to commit the atomic state for planes related to inactive CRTCs. >> >> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > > Tested-by: Geert Uytterhoeven <geert@linux-m68k.org> > Thanks for reporting the issue in the first place and for the testing! >> --- 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, > > The docs say this is intended for drivers that support runtime_pm or > need the CRTC to be enabled to perform a commit. Might be worthwhile > to add basic Runtime PM, so the I2C controller can go to sleep when > the display is not used. > Indeed, I thought the same. But I believe we can do that as a follow-up patch.
Hi Am 13.07.23 um 18:32 schrieb Javier Martinez Canillas: > Geert reports that the following NULL pointer dereference happens for him > after commit 49d7d581ceaf ("drm/ssd130x: Don't allocate buffers on each > plane update"): > > [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 > ... > 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 > ... > > The problem is that fbcon calls fbcon_init() which triggers a DRM modeset > and this leads to drm_atomic_helper_commit_planes() attempting to commit > the atomic state for all planes, even the ones whose CRTC is not enabled. > > Since the primary plane buffer is allocated in the encoder .atomic_enable > callback, this happens after that initial modeset commit and leads to the > mentioned NULL pointer dereference. > > Fix this by not using the default drm_atomic_helper_commit_tail() helper, > but instead the drm_atomic_helper_commit_tail_rpm() function that doesn't > attempt to commit the atomic state for planes related to inactive CRTCs. > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > --- > > drivers/gpu/drm/solomon/ssd130x.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c > index afb08a8aa9fc..12820d16b15b 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, > +}; > + After some discussion on IRC, I'd suggest to allocate the buffer somewhere within probe. So it will always be there when the plane code runs. A full fix would be to allocate the buffer memory as part of the plane state and/or the plane's atomic_check. That's a bit more complicated if you want to shared the buffer memory across plane updates. Best regards Thomas > 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 */ >
Thomas Zimmermann <tzimmermann@suse.de> writes: Hello Thomas, > Hi > > Am 13.07.23 um 18:32 schrieb Javier Martinez Canillas: [...] >> >> +static const struct drm_mode_config_helper_funcs ssd130x_mode_config_helpers = { >> + .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm, >> +}; >> + > > After some discussion on IRC, I'd suggest to allocate the buffer > somewhere within probe. So it will always be there when the plane code runs. > Yes, that's also what Geert suggested so I'll just do that. And also make it a dev managed resource. > A full fix would be to allocate the buffer memory as part of the plane > state and/or the plane's atomic_check. That's a bit more complicated if > you want to shared the buffer memory across plane updates. > I don't think is worth the complexity, allocating it on probe and released when the device is unbound from the driver should be enough as Geert said. > Best regards > Thomas > >
Hi Thomas, On Mon, Jul 17, 2023 at 10:48 AM Thomas Zimmermann <tzimmermann@suse.de> wrote: > Am 13.07.23 um 18:32 schrieb Javier Martinez Canillas: > > Geert reports that the following NULL pointer dereference happens for him > > after commit 49d7d581ceaf ("drm/ssd130x: Don't allocate buffers on each > > plane update"): > > > > [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 > > ... > > 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 > > ... > > > > The problem is that fbcon calls fbcon_init() which triggers a DRM modeset > > and this leads to drm_atomic_helper_commit_planes() attempting to commit > > the atomic state for all planes, even the ones whose CRTC is not enabled. > > > > Since the primary plane buffer is allocated in the encoder .atomic_enable > > callback, this happens after that initial modeset commit and leads to the > > mentioned NULL pointer dereference. > > > > Fix this by not using the default drm_atomic_helper_commit_tail() helper, > > but instead the drm_atomic_helper_commit_tail_rpm() function that doesn't > > attempt to commit the atomic state for planes related to inactive CRTCs. > > > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > > --- 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, > > +}; > > + > > After some discussion on IRC, I'd suggest to allocate the buffer > somewhere within probe. So it will always be there when the plane code runs. > > A full fix would be to allocate the buffer memory as part of the plane > state and/or the plane's atomic_check. That's a bit more complicated if > you want to shared the buffer memory across plane updates. Note that actually two buffers are involved: data_array (monochrome, needed for each update), and buffer (R8, only needed when converting from XR24 to R1). For the former, I agree, as it's always needed. For the latter, I'm afraid it would set a bad example: while allocating a possibly-unused buffer doesn't hurt for small displays, it would mean wasting 1 MiB in e.g. the repaper driver (once it has gained support for R1 ;^). Gr{oetje,eeting}s, Geert
Geert Uytterhoeven <geert@linux-m68k.org> writes: Hello Geert and Thomas, > Hi Thomas, > > On Mon, Jul 17, 2023 at 10:48 AM Thomas Zimmermann <tzimmermann@suse.de> wrote: [...] >> >> After some discussion on IRC, I'd suggest to allocate the buffer >> somewhere within probe. So it will always be there when the plane code runs. >> >> A full fix would be to allocate the buffer memory as part of the plane >> state and/or the plane's atomic_check. That's a bit more complicated if >> you want to shared the buffer memory across plane updates. > > Note that actually two buffers are involved: data_array (monochrome, > needed for each update), and buffer (R8, only needed when converting > from XR24 to R1). > > For the former, I agree, as it's always needed. > For the latter, I'm afraid it would set a bad example: while allocating > a possibly-unused buffer doesn't hurt for small displays, it would > mean wasting 1 MiB in e.g. the repaper driver (once it has gained > support for R1 ;^). > Maybe another option is to allocate on the struct drm_mode_config_funcs .fb_create callback? That way, we can get the mode_cmd->pixel_format and determine if only "data_array" buffer must be allocated or also "buffer". > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-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 >
Javier Martinez Canillas <javierm@redhat.com> writes: > Geert Uytterhoeven <geert@linux-m68k.org> writes: > > Hello Geert and Thomas, > >> Hi Thomas, >> >> On Mon, Jul 17, 2023 at 10:48 AM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > [...] > >>> >>> After some discussion on IRC, I'd suggest to allocate the buffer >>> somewhere within probe. So it will always be there when the plane code runs. >>> >>> A full fix would be to allocate the buffer memory as part of the plane >>> state and/or the plane's atomic_check. That's a bit more complicated if >>> you want to shared the buffer memory across plane updates. >> >> Note that actually two buffers are involved: data_array (monochrome, >> needed for each update), and buffer (R8, only needed when converting >> from XR24 to R1). >> >> For the former, I agree, as it's always needed. >> For the latter, I'm afraid it would set a bad example: while allocating >> a possibly-unused buffer doesn't hurt for small displays, it would >> mean wasting 1 MiB in e.g. the repaper driver (once it has gained >> support for R1 ;^). >> > > Maybe another option is to allocate on the struct drm_mode_config_funcs > .fb_create callback? That way, we can get the mode_cmd->pixel_format and > determine if only "data_array" buffer must be allocated or also "buffer". > Something like the following patch: From 307bf062c9a999693a4a68c6740ec55b81f796b8 Mon Sep 17 00:00:00 2001 From: Javier Martinez Canillas <javierm@redhat.com> Date: Mon, 17 Jul 2023 12:30:35 +0200 Subject: [PATCH v2] drm/ssd130x: Fix an oops when attempting to update a disabled plane Geert reports that the following NULL pointer dereference happens for him after commit 49d7d581ceaf ("drm/ssd130x: Don't allocate buffers on each plane update"): [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 ... 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 ... The problem is that fbcon calls fbcon_init() which triggers a DRM modeset and this leads to drm_atomic_helper_commit_planes() attempting to commit the atomic state for all planes, even the ones whose CRTC is not enabled. Since the primary plane buffer is allocated in the encoder .atomic_enable callback, this happens after that initial modeset commit and leads to the mentioned NULL pointer dereference. Fix this by allocating the buffers in the struct drm_mode_config_funcs .fb_create, instead of doing it when the encoder is enabled. Also, make a couple of improvements to the ssd130x_buf_alloc() function: * Make the allocation smarter to only allocate the intermediate buffer if the native R1 format is not used. Otherwise is not needed, since there is no XRGB8888 to R1 format conversion during plane updates. * Allocate the buffers as device managed resources, this is enough and simplifies the logic since there is no need to explicitly free them. Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> --- Changes in v2: - Move the buffers allocation to .fb_create instead of preventing atomic updates for disable planes. - Don't allocate the intermediate buffer when using the native R1 format. - Allocate buffers as device managed resources. drivers/gpu/drm/solomon/ssd130x.c | 54 +++++++++++++++++-------------- 1 file changed, 29 insertions(+), 25 deletions(-) diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c index b8c90d507a35..615805a066de 100644 --- a/drivers/gpu/drm/solomon/ssd130x.c +++ b/drivers/gpu/drm/solomon/ssd130x.c @@ -146,38 +146,35 @@ static inline struct ssd130x_device *drm_to_ssd130x(struct drm_device *drm) return container_of(drm, struct ssd130x_device, drm); } -static int ssd130x_buf_alloc(struct ssd130x_device *ssd130x) +static int ssd130x_buf_alloc(struct ssd130x_device *ssd130x, __u32 pixel_format) { unsigned int page_height = ssd130x->device_info->page_height; unsigned int pages = DIV_ROUND_UP(ssd130x->height, page_height); const struct drm_format_info *fi; unsigned int pitch; - fi = drm_format_info(DRM_FORMAT_R1); - if (!fi) - return -EINVAL; + /* If the native format is not used an intermediate buffer is needed */ + if (pixel_format != DRM_FORMAT_R1) { + fi = drm_format_info(DRM_FORMAT_R1); + if (!fi) + return -EINVAL; - pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width); + pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width); - ssd130x->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL); - if (!ssd130x->buffer) - return -ENOMEM; + ssd130x->buffer = devm_kcalloc(ssd130x->dev, pitch, ssd130x->height, + GFP_KERNEL); + if (!ssd130x->buffer) + return -ENOMEM; + } - ssd130x->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL); - if (!ssd130x->data_array) { - kfree(ssd130x->buffer); + ssd130x->data_array = devm_kcalloc(ssd130x->dev, ssd130x->width, pages, + GFP_KERNEL); + if (!ssd130x->data_array) return -ENOMEM; - } return 0; } -static void ssd130x_buf_free(struct ssd130x_device *ssd130x) -{ - kfree(ssd130x->data_array); - kfree(ssd130x->buffer); -} - /* * Helper to write data (SSD130X_DATA) to the device. */ @@ -741,10 +738,6 @@ static void ssd130x_encoder_helper_atomic_enable(struct drm_encoder *encoder, if (ret) goto power_off; - ret = ssd130x_buf_alloc(ssd130x); - if (ret) - goto power_off; - ssd130x_write_cmd(ssd130x, 1, SSD130X_DISPLAY_ON); backlight_enable(ssd130x->bl_dev); @@ -766,8 +759,6 @@ static void ssd130x_encoder_helper_atomic_disable(struct drm_encoder *encoder, ssd130x_write_cmd(ssd130x, 1, SSD130X_DISPLAY_OFF); - ssd130x_buf_free(ssd130x); - ssd130x_power_off(ssd130x); } @@ -811,8 +802,21 @@ static const struct drm_connector_funcs ssd130x_connector_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, }; +static struct drm_framebuffer *ssd130x_fb_create(struct drm_device *dev, struct drm_file *file, + const struct drm_mode_fb_cmd2 *mode_cmd) +{ + struct ssd130x_device *ssd130x = drm_to_ssd130x(dev); + int ret; + + ret = ssd130x_buf_alloc(ssd130x, mode_cmd->pixel_format); + if (ret) + return ERR_PTR(ret); + + return drm_gem_fb_create_with_dirty(dev, file, mode_cmd); +} + static const struct drm_mode_config_funcs ssd130x_mode_config_funcs = { - .fb_create = drm_gem_fb_create_with_dirty, + .fb_create = ssd130x_fb_create, .atomic_check = drm_atomic_helper_check, .atomic_commit = drm_atomic_helper_commit, };
Hi Am 17.07.23 um 11:04 schrieb Geert Uytterhoeven: > Hi Thomas, > > On Mon, Jul 17, 2023 at 10:48 AM Thomas Zimmermann <tzimmermann@suse.de> wrote: >> Am 13.07.23 um 18:32 schrieb Javier Martinez Canillas: >>> Geert reports that the following NULL pointer dereference happens for him >>> after commit 49d7d581ceaf ("drm/ssd130x: Don't allocate buffers on each >>> plane update"): >>> >>> [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 >>> ... >>> 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 >>> ... >>> >>> The problem is that fbcon calls fbcon_init() which triggers a DRM modeset >>> and this leads to drm_atomic_helper_commit_planes() attempting to commit >>> the atomic state for all planes, even the ones whose CRTC is not enabled. >>> >>> Since the primary plane buffer is allocated in the encoder .atomic_enable >>> callback, this happens after that initial modeset commit and leads to the >>> mentioned NULL pointer dereference. >>> >>> Fix this by not using the default drm_atomic_helper_commit_tail() helper, >>> but instead the drm_atomic_helper_commit_tail_rpm() function that doesn't >>> attempt to commit the atomic state for planes related to inactive CRTCs. >>> >>> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> >>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > >>> --- 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, >>> +}; >>> + >> >> After some discussion on IRC, I'd suggest to allocate the buffer >> somewhere within probe. So it will always be there when the plane code runs. >> >> A full fix would be to allocate the buffer memory as part of the plane >> state and/or the plane's atomic_check. That's a bit more complicated if >> you want to shared the buffer memory across plane updates. > > Note that actually two buffers are involved: data_array (monochrome, > needed for each update), and buffer (R8, only needed when converting > from XR24 to R1). > > For the former, I agree, as it's always needed. > For the latter, I'm afraid it would set a bad example: while allocating > a possibly-unused buffer doesn't hurt for small displays, it would > mean wasting 1 MiB in e.g. the repaper driver (once it has gained > support for R1 ;^). Let me explain: a real DRM-ideomatic solution would allocate the required buffers at the correct size in the plane's atomic check. The pointer would be stored in the plane state and later be free'd as part of releasing that plane_state. But as this is temporary memory for the plane update, so it can be shared among plane states. Keeping track of the references requires a bit of work though. Repaper appears to allocate buffer memory on each update, so anything is an improvement there. Best regards Thomsa > > Gr{oetje,eeting}s, > > Geert >
Hi Javier, On Mon, Jul 17, 2023 at 12:37 PM Javier Martinez Canillas <javierm@redhat.com> wrote: > Javier Martinez Canillas <javierm@redhat.com> writes: > >> On Mon, Jul 17, 2023 at 10:48 AM Thomas Zimmermann <tzimmermann@suse.de> wrote: > >>> After some discussion on IRC, I'd suggest to allocate the buffer > >>> somewhere within probe. So it will always be there when the plane code runs. > >>> > >>> A full fix would be to allocate the buffer memory as part of the plane > >>> state and/or the plane's atomic_check. That's a bit more complicated if > >>> you want to shared the buffer memory across plane updates. > >> > >> Note that actually two buffers are involved: data_array (monochrome, > >> needed for each update), and buffer (R8, only needed when converting > >> from XR24 to R1). > >> > >> For the former, I agree, as it's always needed. > >> For the latter, I'm afraid it would set a bad example: while allocating > >> a possibly-unused buffer doesn't hurt for small displays, it would > >> mean wasting 1 MiB in e.g. the repaper driver (once it has gained > >> support for R1 ;^). > >> > > > > Maybe another option is to allocate on the struct drm_mode_config_funcs > > .fb_create callback? That way, we can get the mode_cmd->pixel_format and > > determine if only "data_array" buffer must be allocated or also "buffer". > > > > Something like the following patch: > > From 307bf062c9a999693a4a68c6740ec55b81f796b8 Mon Sep 17 00:00:00 2001 > From: Javier Martinez Canillas <javierm@redhat.com> > Date: Mon, 17 Jul 2023 12:30:35 +0200 > Subject: [PATCH v2] drm/ssd130x: Fix an oops when attempting to update a > disabled plane > > Geert reports that the following NULL pointer dereference happens for him > after commit 49d7d581ceaf ("drm/ssd130x: Don't allocate buffers on each > plane update"): > > [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 > ... > 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 > ... > > The problem is that fbcon calls fbcon_init() which triggers a DRM modeset > and this leads to drm_atomic_helper_commit_planes() attempting to commit > the atomic state for all planes, even the ones whose CRTC is not enabled. > > Since the primary plane buffer is allocated in the encoder .atomic_enable > callback, this happens after that initial modeset commit and leads to the > mentioned NULL pointer dereference. > > Fix this by allocating the buffers in the struct drm_mode_config_funcs > .fb_create, instead of doing it when the encoder is enabled. > > Also, make a couple of improvements to the ssd130x_buf_alloc() function: > > * Make the allocation smarter to only allocate the intermediate buffer > if the native R1 format is not used. Otherwise is not needed, since > there is no XRGB8888 to R1 format conversion during plane updates. > > * Allocate the buffers as device managed resources, this is enough and > simplifies the logic since there is no need to explicitly free them. > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > --- > > Changes in v2: > - Move the buffers allocation to .fb_create instead of preventing atomic > updates for disable planes. > - Don't allocate the intermediate buffer when using the native R1 format. > - Allocate buffers as device managed resources. Thanks for the update! > --- a/drivers/gpu/drm/solomon/ssd130x.c > +++ b/drivers/gpu/drm/solomon/ssd130x.c > @@ -146,38 +146,35 @@ static inline struct ssd130x_device *drm_to_ssd130x(struct drm_device *drm) > return container_of(drm, struct ssd130x_device, drm); > } > > -static int ssd130x_buf_alloc(struct ssd130x_device *ssd130x) > +static int ssd130x_buf_alloc(struct ssd130x_device *ssd130x, __u32 pixel_format) > { > unsigned int page_height = ssd130x->device_info->page_height; > unsigned int pages = DIV_ROUND_UP(ssd130x->height, page_height); > const struct drm_format_info *fi; > unsigned int pitch; > > - fi = drm_format_info(DRM_FORMAT_R1); > - if (!fi) > - return -EINVAL; > + /* If the native format is not used an intermediate buffer is needed */ > + if (pixel_format != DRM_FORMAT_R1) { > + fi = drm_format_info(DRM_FORMAT_R1); > + if (!fi) > + return -EINVAL; > > - pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width); > + pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width); > > - ssd130x->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL); > - if (!ssd130x->buffer) > - return -ENOMEM; > + ssd130x->buffer = devm_kcalloc(ssd130x->dev, pitch, ssd130x->height, > + GFP_KERNEL); This should check if the buffer was allocated before. Else an application creating two or more frame buffers will keep on allocating new buffers. The same is true for fbdev emulation vs. a userspace application. > + if (!ssd130x->buffer) > + return -ENOMEM; > + } > > - ssd130x->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL); > - if (!ssd130x->data_array) { > - kfree(ssd130x->buffer); > + ssd130x->data_array = devm_kcalloc(ssd130x->dev, ssd130x->width, pages, > + GFP_KERNEL); Same here. > + if (!ssd130x->data_array) > return -ENOMEM; > - } And you still need the data_array buffer for clearing the screen, which is not tied to the creation of a frame buffer, I think? > > return 0; > } > > -static void ssd130x_buf_free(struct ssd130x_device *ssd130x) > -{ > - kfree(ssd130x->data_array); > - kfree(ssd130x->buffer); > -} > - > /* > * Helper to write data (SSD130X_DATA) to the device. > */ > @@ -741,10 +738,6 @@ static void ssd130x_encoder_helper_atomic_enable(struct drm_encoder *encoder, > if (ret) > goto power_off; > > - ret = ssd130x_buf_alloc(ssd130x); > - if (ret) > - goto power_off; > - > ssd130x_write_cmd(ssd130x, 1, SSD130X_DISPLAY_ON); > > backlight_enable(ssd130x->bl_dev); > @@ -766,8 +759,6 @@ static void ssd130x_encoder_helper_atomic_disable(struct drm_encoder *encoder, > > ssd130x_write_cmd(ssd130x, 1, SSD130X_DISPLAY_OFF); > > - ssd130x_buf_free(ssd130x); > - > ssd130x_power_off(ssd130x); > } > > @@ -811,8 +802,21 @@ static const struct drm_connector_funcs ssd130x_connector_funcs = { > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > }; > > +static struct drm_framebuffer *ssd130x_fb_create(struct drm_device *dev, struct drm_file *file, > + const struct drm_mode_fb_cmd2 *mode_cmd) > +{ > + struct ssd130x_device *ssd130x = drm_to_ssd130x(dev); > + int ret; > + > + ret = ssd130x_buf_alloc(ssd130x, mode_cmd->pixel_format); > + if (ret) > + return ERR_PTR(ret); > + > + return drm_gem_fb_create_with_dirty(dev, file, mode_cmd); > +} > + > static const struct drm_mode_config_funcs ssd130x_mode_config_funcs = { > - .fb_create = drm_gem_fb_create_with_dirty, > + .fb_create = ssd130x_fb_create, > .atomic_check = drm_atomic_helper_check, > .atomic_commit = drm_atomic_helper_commit, > }; Gr{oetje,eeting}s, Geert
Geert Uytterhoeven <geert@linux-m68k.org> writes: Hello Geert, Thanks for your review! > Hi Javier, [...] >> - ssd130x->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL); >> - if (!ssd130x->buffer) >> - return -ENOMEM; >> + ssd130x->buffer = devm_kcalloc(ssd130x->dev, pitch, ssd130x->height, >> + GFP_KERNEL); > > This should check if the buffer was allocated before. > Else an application creating two or more frame buffers will keep > on allocating new buffers. The same is true for fbdev emulation vs. > a userspace application. > Yes, you are correct. >> + if (!ssd130x->buffer) >> + return -ENOMEM; >> + } >> >> - ssd130x->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL); >> - if (!ssd130x->data_array) { >> - kfree(ssd130x->buffer); >> + ssd130x->data_array = devm_kcalloc(ssd130x->dev, ssd130x->width, pages, >> + GFP_KERNEL); > > Same here. > >> + if (!ssd130x->data_array) >> return -ENOMEM; >> - } > > And you still need the data_array buffer for clearing the screen, > which is not tied to the creation of a frame buffer, I think? > It is indeed. I forgot about that. So what I would do for now is just to allocate the two unconditionally and then we can optimize as a follow-up. But also, this v2 is not correct because as I mentioned the device and drm_device lifecycles are not the same and the kernel crashes on poweroff: [ 568.351783] ssd130x_update_rect.isra.0+0xe0/0x308 [ 568.356881] ssd130x_primary_plane_helper_atomic_disable+0x54/0x78 [ 568.363475] drm_atomic_helper_commit_planes+0x1ec/0x288 [ 568.369128] drm_atomic_helper_commit_tail+0x5c/0xb0 [ 568.374422] commit_tail+0x168/0x1a0 [ 568.378240] drm_atomic_helper_commit+0x1c8/0x1e8 [ 568.383265] drm_atomic_commit+0xa0/0xc8 [ 568.387439] drm_atomic_helper_disable_all+0x204/0x220 [ 568.392914] drm_atomic_helper_shutdown+0x98/0x138 [ 568.398033] ssd130x_shutdown+0x18/0x30 [ 568.402117] ssd130x_i2c_shutdown+0x1c/0x30 [ 568.406558] i2c_device_shutdown+0x48/0x78 [ 568.410913] device_shutdown+0x130/0x238 [ 568.415087] kernel_restart+0x48/0xd0 [ 568.418996] __do_sys_reboot+0x14c/0x230 [ 568.423167] __arm64_sys_reboot+0x2c/0x40 [ 568.427426] invoke_syscall+0x78/0x100 [ 568.431420] el0_svc_common.constprop.0+0x68/0x130 [ 568.436531] do_el0_svc+0x34/0x50 [ 568.440080] el0_svc+0x48/0x150 [ 568.443455] el0t_64_sync_handler+0x114/0x120 [ 568.448072] el0t_64_sync+0x194/0x198 [ 568.451985] Code: 12000949 52800001 52800002 0b830f63 (38634b20) [ 568.458502] ---[ end trace 0000000000000000 ]--- Sima also suggested to make the allocation in the plane .prepare_fb, I think that the better place is .begin_fb_access for allocation and the .end_fb_access for free. Updated patch below and this is the one that I'm more comfortable now as a solution: From b4d078dd65a04ab61dd9264e982b37321f7a75d9 Mon Sep 17 00:00:00 2001 From: Javier Martinez Canillas <javierm@redhat.com> Date: Mon, 17 Jul 2023 12:30:35 +0200 Subject: [PATCH v3] drm/ssd130x: Fix an oops when attempting to update a disabled plane Geert reports that the following NULL pointer dereference happens for him after commit 49d7d581ceaf ("drm/ssd130x: Don't allocate buffers on each plane update"): [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 ... 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 ... The problem is that fbcon calls fbcon_init() which triggers a DRM modeset and this leads to drm_atomic_helper_commit_planes() attempting to commit the atomic state for all planes, even the ones whose CRTC is not enabled. Since the primary plane buffer is allocated in the encoder .atomic_enable callback, this happens after that initial modeset commit and leads to the mentioned NULL pointer dereference. Fix this by allocating the buffers in the struct drm_plane_helper_funcs .begin_fb_access callback and free them in to .end_fb_access callback, instead of doing it when the encoder is enabled. Fixes: commit 49d7d581ceaf ("drm/ssd130x: Don't allocate buffers on each plane update") Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> --- Changes in v3: - Move the buffers allocation to the plane helper funcs .begin_fb_access and the free to .end_fb_access callbacks. - Always allocate intermediate buffer because is use in ssd130x_clear_screen(). - Don't allocate the buffers as device managed resources. Changes in v2: - Move the buffers allocation to .fb_create instead of preventing atomic updates for disable planes. - Don't allocate the intermediate buffer when using the native R1 format. - Allocate buffers as device managed resources. drivers/gpu/drm/solomon/ssd130x.c | 33 ++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c index b8c90d507a35..549d78e9985d 100644 --- a/drivers/gpu/drm/solomon/ssd130x.c +++ b/drivers/gpu/drm/solomon/ssd130x.c @@ -634,6 +634,30 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m return ret; } +static int ssd130x_primary_plane_helper_begin_fb_access(struct drm_plane *plane, + struct drm_plane_state *state) +{ + struct drm_device *drm = plane->dev; + struct ssd130x_device *ssd130x = drm_to_ssd130x(drm); + int ret = ssd130x_buf_alloc(ssd130x); + + if (ret) + return ret; + + return drm_gem_begin_shadow_fb_access(plane, state); +} + +static void ssd130x_primary_plane_helper_end_fb_access(struct drm_plane *plane, + struct drm_plane_state *state) +{ + struct drm_device *drm = plane->dev; + struct ssd130x_device *ssd130x = drm_to_ssd130x(drm); + + ssd130x_buf_free(ssd130x); + + return drm_gem_end_shadow_fb_access(plane, state); +} + static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane, struct drm_atomic_state *state) { @@ -678,7 +702,8 @@ static void ssd130x_primary_plane_helper_atomic_disable(struct drm_plane *plane, } static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = { - DRM_GEM_SHADOW_PLANE_HELPER_FUNCS, + .begin_fb_access = ssd130x_primary_plane_helper_begin_fb_access, + .end_fb_access = ssd130x_primary_plane_helper_end_fb_access, .atomic_check = drm_plane_helper_atomic_check, .atomic_update = ssd130x_primary_plane_helper_atomic_update, .atomic_disable = ssd130x_primary_plane_helper_atomic_disable, @@ -741,10 +766,6 @@ static void ssd130x_encoder_helper_atomic_enable(struct drm_encoder *encoder, if (ret) goto power_off; - ret = ssd130x_buf_alloc(ssd130x); - if (ret) - goto power_off; - ssd130x_write_cmd(ssd130x, 1, SSD130X_DISPLAY_ON); backlight_enable(ssd130x->bl_dev); @@ -766,8 +787,6 @@ static void ssd130x_encoder_helper_atomic_disable(struct drm_encoder *encoder, ssd130x_write_cmd(ssd130x, 1, SSD130X_DISPLAY_OFF); - ssd130x_buf_free(ssd130x); - ssd130x_power_off(ssd130x); }
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c index afb08a8aa9fc..12820d16b15b 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 */
Geert reports that the following NULL pointer dereference happens for him after commit 49d7d581ceaf ("drm/ssd130x: Don't allocate buffers on each plane update"): [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 ... 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 ... The problem is that fbcon calls fbcon_init() which triggers a DRM modeset and this leads to drm_atomic_helper_commit_planes() attempting to commit the atomic state for all planes, even the ones whose CRTC is not enabled. Since the primary plane buffer is allocated in the encoder .atomic_enable callback, this happens after that initial modeset commit and leads to the mentioned NULL pointer dereference. Fix this by not using the default drm_atomic_helper_commit_tail() helper, but instead the drm_atomic_helper_commit_tail_rpm() function that doesn't attempt to commit the atomic state for planes related to inactive CRTCs. Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> --- drivers/gpu/drm/solomon/ssd130x.c | 5 +++++ 1 file changed, 5 insertions(+)