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@xxxxxxx> 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@xxxxxxxxxxxxxx> Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>--- 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
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstrasse 146, 90461 Nuernberg, Germany GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB 36809 (AG Nuernberg)
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
