| Message ID | 20230609170941.1150941-5-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | drm/ssd130x: A few enhancements and cleanups | expand |
Hi Javier, On Fri, Jun 9, 2023 at 7:09 PM Javier Martinez Canillas <javierm@redhat.com> 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@redhat.com> > Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > > (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: [ 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 Bailing out from ssd130x_update_rect() when data_array is still NULL fixes that. Gr{oetje,eeting}s, Geert
Geert Uytterhoeven <geert@linux-m68k.org> writes: Hello Geert, > Hi Javier, > > On Fri, Jun 9, 2023 at 7:09 PM Javier Martinez Canillas > <javierm@redhat.com> 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@redhat.com> >> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> >> (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.
Hi Javier, On Thu, Jul 13, 2023 at 3:21 PM Javier Martinez Canillas <javierm@redhat.com> wrote: > Geert Uytterhoeven <geert@linux-m68k.org> writes: > > On Fri, Jun 9, 2023 at 7:09 PM Javier Martinez Canillas > > <javierm@redhat.com> 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@redhat.com> > >> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> > >> --- > >> > >> (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 [<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 [<c02af188>] visual_init+0xac/0x114 [<c02b1834>] do_bind_con_driver.isra.0+0x1bc/0x39c [<c02b2fcc>] do_take_over_console+0x128/0x1b4 [<c027ad24>] do_fbcon_takeover+0x74/0xfc [<c027e704>] fbcon_fb_registered+0x168/0x1b4 [<c0275c84>] register_framebuffer+0x180/0x238 [<c03017a4>] __drm_fb_helper_initial_config_and_unlock+0x328/0x538 [<c0303844>] drm_fb_helper_initial_config+0x40/0x54 [<c0300818>] drm_fbdev_generic_client_hotplug+0x98/0xdc [<c0300c5c>] drm_fbdev_generic_setup+0x9c/0x178 [<c0304fa0>] ssd130x_probe+0x5e0/0x788 [<c030522c>] ssd130x_i2c_probe+0x4c/0x70 [<c0358464>] i2c_device_probe+0x120/0x1f0 [<c030e5a4>] really_probe+0xb8/0x30c [<c030e974>] __driver_probe_device+0x17c/0x1c8 [<c030ea0c>] driver_probe_device+0x4c/0x140 [<c030ebe4>] __device_attach_driver+0xe4/0x150 [<c030ccc4>] bus_for_each_drv+0x8c/0x100 [<c030f034>] __device_attach+0x12c/0x1ac [<c030f3e0>] device_initial_probe+0x18/0x28 [<c030cf14>] bus_probe_device+0xcc/0xd0 [<c030b274>] device_add+0x5d8/0x7b4 [<c030b474>] device_register+0x24/0x38 [<c0358f24>] i2c_new_client_device+0x1a8/0x2b8 [<c035b960>] of_i2c_register_devices+0xdc/0x164 [<c0359750>] i2c_register_adapter+0x1b8/0x56c [<c0359eb0>] i2c_add_adapter+0x94/0x100 [<c035d47c>] __i2c_bit_add_bus+0xc0/0x460 [<c035d838>] i2c_bit_add_bus+0x1c/0x2c [<c035da20>] litex_i2c_probe+0x108/0x164 [<c0310de4>] platform_probe+0x54/0xb0 [<c030e5a4>] really_probe+0xb8/0x30c [<c030e974>] __driver_probe_device+0x17c/0x1c8 [<c030ea0c>] driver_probe_device+0x4c/0x140 [<c030ed74>] __driver_attach+0x124/0x1f4 [<c030c880>] bus_for_each_dev+0x84/0xf4 [<c030f4f8>] driver_attach+0x28/0x38 [<c030d174>] bus_add_driver+0x120/0x214 [<c030fc90>] driver_register+0x70/0x15c [<c0311d98>] __platform_driver_register+0x28/0x38 [<c0520958>] litex_i2c_driver_init+0x24/0x34 [<c0507fe8>] do_one_initcall+0x80/0x238 [<c05083d4>] kernel_init_freeable+0x1b4/0x238 [<c04fdd9c>] kernel_init+0x24/0x144 [<c00022d8>] ret_from_fork+0x10/0x24 Gr{oetje,eeting}s, Geert
Geert Uytterhoeven <geert@linux-m68k.org> writes: Hello Geert, > Hi Javier, > > On Thu, Jul 13, 2023 at 3:21 PM Javier Martinez Canillas > <javierm@redhat.com> wrote: >> Geert Uytterhoeven <geert@linux-m68k.org> writes: >> > On Fri, Jun 9, 2023 at 7:09 PM Javier Martinez Canillas >> > <javierm@redhat.com> 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@redhat.com> >> >> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> >> >> --- >> >> >> >> (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 */
Hi Javier, On Thu, Jul 13, 2023 at 4:13 PM Javier Martinez Canillas <javierm@redhat.com> wrote: > Geert Uytterhoeven <geert@linux-m68k.org> writes: > > On Thu, Jul 13, 2023 at 3:21 PM Javier Martinez Canillas > > <javierm@redhat.com> wrote: > >> Geert Uytterhoeven <geert@linux-m68k.org> writes: > >> > On Fri, Jun 9, 2023 at 7:09 PM Javier Martinez Canillas > >> > <javierm@redhat.com> 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@redhat.com> > >> >> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> > >> >> --- > >> >> > >> >> (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 <geert@linux-m68k.org> writes: Hello Geert, > Hi Javier, [...] >> +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! > Thanks for testing! Proper patch sent: https://lists.freedesktop.org/archives/dri-devel/2023-July/413630.html
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c index 5cac1149e34e..0be3b476dc60 100644 --- a/drivers/gpu/drm/solomon/ssd130x.c +++ b/drivers/gpu/drm/solomon/ssd130x.c @@ -146,6 +146,31 @@ 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) +{ + unsigned int page_height = ssd130x->device_info->page_height; + unsigned int pages = DIV_ROUND_UP(ssd130x->height, page_height); + + ssd130x->buffer = kcalloc(DIV_ROUND_UP(ssd130x->width, 8), + 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); + 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. */ @@ -434,11 +459,12 @@ static int ssd130x_init(struct ssd130x_device *ssd130x) SSD130X_SET_ADDRESS_MODE_HORIZONTAL); } -static int ssd130x_update_rect(struct ssd130x_device *ssd130x, u8 *buf, - struct drm_rect *rect) +static int ssd130x_update_rect(struct ssd130x_device *ssd130x, struct drm_rect *rect) { unsigned int x = rect->x1; unsigned int y = rect->y1; + u8 *buf = ssd130x->buffer; + u8 *data_array = ssd130x->data_array; unsigned int width = drm_rect_width(rect); unsigned int height = drm_rect_height(rect); unsigned int line_length = DIV_ROUND_UP(width, 8); @@ -447,14 +473,9 @@ static int ssd130x_update_rect(struct ssd130x_device *ssd130x, u8 *buf, struct drm_device *drm = &ssd130x->drm; u32 array_idx = 0; int ret, i, j, k; - u8 *data_array = NULL; drm_WARN_ONCE(drm, y % 8 != 0, "y must be aligned to screen page\n"); - data_array = kcalloc(width, pages, GFP_KERNEL); - if (!data_array) - return -ENOMEM; - /* * The screen is divided in pages, each having a height of 8 * pixels, and the width of the screen. When sending a byte of @@ -488,11 +509,11 @@ static int ssd130x_update_rect(struct ssd130x_device *ssd130x, u8 *buf, /* Set address range for horizontal addressing mode */ ret = ssd130x_set_col_range(ssd130x, ssd130x->col_offset + x, width); if (ret < 0) - goto out_free; + return ret; ret = ssd130x_set_page_range(ssd130x, ssd130x->page_offset + y / 8, pages); if (ret < 0) - goto out_free; + return ret; } for (i = 0; i < pages; i++) { @@ -522,11 +543,11 @@ static int ssd130x_update_rect(struct ssd130x_device *ssd130x, u8 *buf, ssd130x->page_offset + i, ssd130x->col_offset + x); if (ret < 0) - goto out_free; + return ret; ret = ssd130x_write_data(ssd130x, data_array, width); if (ret < 0) - goto out_free; + return ret; array_idx = 0; } @@ -536,14 +557,11 @@ static int ssd130x_update_rect(struct ssd130x_device *ssd130x, u8 *buf, if (!ssd130x->page_address_mode) ret = ssd130x_write_data(ssd130x, data_array, width * pages); -out_free: - kfree(data_array); return ret; } static void ssd130x_clear_screen(struct ssd130x_device *ssd130x) { - u8 *buf = NULL; struct drm_rect fullscreen = { .x1 = 0, .x2 = ssd130x->width, @@ -551,14 +569,7 @@ static void ssd130x_clear_screen(struct ssd130x_device *ssd130x) .y2 = ssd130x->height, }; - buf = kcalloc(DIV_ROUND_UP(ssd130x->width, 8), ssd130x->height, - GFP_KERNEL); - if (!buf) - return; - - ssd130x_update_rect(ssd130x, buf, &fullscreen); - - kfree(buf); + ssd130x_update_rect(ssd130x, &fullscreen); } static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_map *vmap, @@ -569,30 +580,27 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m struct iosys_map dst; unsigned int dst_pitch; int ret = 0; - u8 *buf = NULL; + u8 *buf = ssd130x->buffer; + + if (!buf) + return 0; /* Align y to display page boundaries */ rect->y1 = round_down(rect->y1, page_height); rect->y2 = min_t(unsigned int, round_up(rect->y2, page_height), ssd130x->height); dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), page_height); - buf = kcalloc(dst_pitch, drm_rect_height(rect), GFP_KERNEL); - if (!buf) - return -ENOMEM; ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE); if (ret) - goto out_free; + return ret; iosys_map_set_vaddr(&dst, buf); drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect); drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE); - ssd130x_update_rect(ssd130x, buf, rect); - -out_free: - kfree(buf); + ssd130x_update_rect(ssd130x, rect); return ret; } @@ -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); + if (ret) + goto power_off; ssd130x_write_cmd(ssd130x, 1, SSD130X_DISPLAY_ON); backlight_enable(ssd130x->bl_dev); + + return; + +power_off: + ssd130x_power_off(ssd130x); + return; } static void ssd130x_encoder_helper_atomic_disable(struct drm_encoder *encoder, @@ -721,6 +737,8 @@ 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.h b/drivers/gpu/drm/solomon/ssd130x.h index 87968b3e7fb8..161588b1cc4d 100644 --- a/drivers/gpu/drm/solomon/ssd130x.h +++ b/drivers/gpu/drm/solomon/ssd130x.h @@ -89,6 +89,9 @@ struct ssd130x_device { u8 col_end; u8 page_start; u8 page_end; + + u8 *buffer; + u8 *data_array; }; extern const struct ssd130x_deviceinfo ssd130x_variants[];