Message-ID: <87bkgaken1.fsf@minerva.mail-host-address-is-not-set>
Date:   Mon, 17 Jul 2023 12:37:22 +0200
From:   Javier Martinez Canillas <javierm@...hat.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>,
        Thomas Zimmermann <tzimmermann@...e.de>
Cc:     linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH] drm/ssd130x: Fix an oops when attempting to update a
 disabled plane

Javier Martinez Canillas <javierm@...hat.com> writes:

> Geert Uytterhoeven <geert@...ux-m68k.org> writes:
>
> Hello Geert and Thomas,
>
>> Hi Thomas,
>>
>> On Mon, Jul 17, 2023 at 10:48 AM Thomas Zimmermann <tzimmermann@...e.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@...hat.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@...ux-m68k.org>
Signed-off-by: Javier Martinez Canillas <javierm@...hat.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,
 };
-- 
2.41.0

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Powered by blists - more mailing lists