| Message ID | 20230721070955.1170974-1-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4] drm/ssd130x: Allocate buffers in the plane's .atomic_check callback | expand |
On Fri, 21 Jul 2023 09:09:50 +0200, Javier Martinez Canillas 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 > > [ ... ] Acked-by: Maxime Ripard <mripard@kernel.org> Thanks! Maxime
Hi Javier, Thanks for your patch! On Fri, Jul 21, 2023 at 9:10 AM 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. Upon further investigation, the NULL pointer dereference does not happen with the current and accepted code (only with my patches to add support for R1), because of the "superfluous" NULL buffer check in ssd130x_fb_blit_rect(): https://cgit.freedesktop.org/drm-misc/tree/drivers/gpu/drm/solomon/ssd130x.c#n592 So you probably want to drop the crash report... > Fix this by having custom helpers to allocate, duplicate and destroy the > plane state, that will take care of allocating and freeing these buffers. > > Instead of doing it in the encoder atomic enabled and disabled callbacks, > since allocations must not be done in an .atomic_enable handler. Because > drivers are not allowed to fail after drm_atomic_helper_swap_state() has > been called and the new atomic state is stored into the current sw state. > > Fixes: 49d7d581ceaf ("drm/ssd130x: Don't allocate buffers on each plane update") ... and the Fixes tag. > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > Suggested-by: Maxime Ripard <mripard@kernel.org> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> Regardless, avoiding calls to ssd130x_fb_blit_rect() when the buffer is not yet allocated is worthwhile. And this patch achieves that. Tested-by: Geert Uytterhoeven <geert@linux-m68k.org> Still, some comments below. > --- a/drivers/gpu/drm/solomon/ssd130x.c > +++ b/drivers/gpu/drm/solomon/ssd130x.c > @@ -141,12 +141,26 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = { > }; > EXPORT_SYMBOL_NS_GPL(ssd130x_variants, DRM_SSD130X); > > +struct ssd130x_plane_state { > + struct drm_plane_state base; > + /* Intermediate buffer to convert pixels from XRGB8888 to R1 */ > + u8 *buffer; > + /* Buffer that contains R1 pixels to be written to the panel */ > + u8 *data_array; The second buffer actually contains pixels in hardware format. For now that is a transposed buffer of R1 pixels, but that will change when you will add support for greyscale displays. So I'd write "hardware format" instead of R1 for both. BTW, I still think data_array should be allocated during probing, as it is related to the hardware, not to a plane. > +}; > @@ -159,23 +173,23 @@ static int ssd130x_buf_alloc(struct ssd130x_device *ssd130x) > > pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width); > > - ssd130x->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL); > - if (!ssd130x->buffer) > + ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL); > + if (!ssd130x_state->buffer) > return -ENOMEM; > > - ssd130x->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL); > - if (!ssd130x->data_array) { > - kfree(ssd130x->buffer); > + ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL); > + if (!ssd130x_state->data_array) { > + kfree(ssd130x_state->buffer); Should ssd130x_state->buffer be reset to NULL here? I.e. if .atomic_check() fails, will .atomic_destroy_state() be called, leading to a double free? > return -ENOMEM; > } > > return 0; > } > @@ -576,18 +593,21 @@ static void ssd130x_clear_screen(struct ssd130x_device *ssd130x) > .y2 = ssd130x->height, > }; > > - ssd130x_update_rect(ssd130x, &fullscreen); > + ssd130x_update_rect(ssd130x, ssd130x_state, &fullscreen); > } > > -static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_map *vmap, > +static int ssd130x_fb_blit_rect(struct drm_plane_state *state, > + const struct iosys_map *vmap, > struct drm_rect *rect) > { > + struct drm_framebuffer *fb = state->fb; > struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev); > + struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(state); > unsigned int page_height = ssd130x->device_info->page_height; > struct iosys_map dst; > unsigned int dst_pitch; > int ret = 0; > - u8 *buf = ssd130x->buffer; > + u8 *buf = ssd130x_state->buffer; > > if (!buf) This check should no longer be needed (and prevented you from seeing the issue before). > return 0; > @@ -607,11 +627,27 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m > > drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE); > > - ssd130x_update_rect(ssd130x, rect); > + ssd130x_update_rect(ssd130x, ssd130x_state, rect); > > return ret; > } > > +static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane, > + struct drm_atomic_state *state) > +{ > + struct drm_device *drm = plane->dev; > + struct ssd130x_device *ssd130x = drm_to_ssd130x(drm); > + struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane); > + struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state); > + int ret; > + > + ret = drm_plane_helper_atomic_check(plane, state); > + if (ret) > + return ret; > + > + return ssd130x_buf_alloc(ssd130x, ssd130x_state); I think the code would become easier to read by inlining ssd130x_buf_alloc() here. > +} > + > +static struct drm_plane_state *ssd130x_primary_plane_duplicate_state(struct drm_plane *plane) > +{ > + struct ssd130x_plane_state *old_ssd130x_state; > + struct ssd130x_plane_state *ssd130x_state; > + > + if (WARN_ON(!plane->state)) > + return NULL; > + > + old_ssd130x_state = to_ssd130x_plane_state(plane->state); > + ssd130x_state = kmemdup(old_ssd130x_state, sizeof(*ssd130x_state), GFP_KERNEL); I know this is the standard pattern, but the "dup" part is a bit silly here: - The ssd130x-specific parts in ssd130x_plane_state are zeroed below, - The base part is copied again in __drm_atomic_helper_plane_duplicate_state). So (for now) you might as well just call kzalloc() ;-) > + if (!ssd130x_state) > + return NULL; > + > + /* The buffers are not duplicated and are allocated in .atomic_check */ > + ssd130x_state->buffer = NULL; > + ssd130x_state->data_array = NULL; > + > + __drm_atomic_helper_plane_duplicate_state(plane, &ssd130x_state->base); > + > + return &ssd130x_state->base; > +} > + > +static void ssd130x_primary_plane_destroy_state(struct drm_plane *plane, > + struct drm_plane_state *state) > +{ > + struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(state); > + > + ssd130x_buf_free(ssd130x_state); I think the code would become easier to read by inlining ssd130x_buf_free() here. > + > + __drm_atomic_helper_plane_destroy_state(&ssd130x_state->base); > + > + kfree(ssd130x_state); > +} Gr{oetje,eeting}s, Geert
Geert Uytterhoeven <geert@linux-m68k.org> writes: Hello Geert, > Hi Javier, > > Thanks for your patch! > Thanks a lot for your feedabck. > On Fri, Jul 21, 2023 at 9:10 AM Javier Martinez Canillas > <javierm@redhat.com> wrote: >> Geert reports that the following NULL pointer dereference happens for him [...] > >> 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. > > Upon further investigation, the NULL pointer dereference does not > happen with the current and accepted code (only with my patches to > add support for R1), because of the "superfluous" NULL buffer check > in ssd130x_fb_blit_rect(): > https://cgit.freedesktop.org/drm-misc/tree/drivers/gpu/drm/solomon/ssd130x.c#n592 > > So you probably want to drop the crash report... > >> Fix this by having custom helpers to allocate, duplicate and destroy the >> plane state, that will take care of allocating and freeing these buffers. >> >> Instead of doing it in the encoder atomic enabled and disabled callbacks, >> since allocations must not be done in an .atomic_enable handler. Because >> drivers are not allowed to fail after drm_atomic_helper_swap_state() has >> been called and the new atomic state is stored into the current sw state. >> >> Fixes: 49d7d581ceaf ("drm/ssd130x: Don't allocate buffers on each plane update") > > ... and the Fixes tag. > Ah, I see. Thanks a lot for that information. >> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> Will drop your Reported-by tag too then. >> Suggested-by: Maxime Ripard <mripard@kernel.org> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > > Regardless, avoiding calls to ssd130x_fb_blit_rect() when the buffer > is not yet allocated is worthwhile. And this patch achieves that. > Agreed, and as Maxime mentioned is the correct thing to do. > Tested-by: Geert Uytterhoeven <geert@linux-m68k.org> > Thanks for testing! > Still, some comments below. > >> --- a/drivers/gpu/drm/solomon/ssd130x.c >> +++ b/drivers/gpu/drm/solomon/ssd130x.c >> @@ -141,12 +141,26 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = { >> }; >> EXPORT_SYMBOL_NS_GPL(ssd130x_variants, DRM_SSD130X); >> >> +struct ssd130x_plane_state { >> + struct drm_plane_state base; >> + /* Intermediate buffer to convert pixels from XRGB8888 to R1 */ >> + u8 *buffer; >> + /* Buffer that contains R1 pixels to be written to the panel */ >> + u8 *data_array; > > The second buffer actually contains pixels in hardware format. > For now that is a transposed buffer of R1 pixels, but that will change > when you will add support for greyscale displays. > > So I'd write "hardware format" instead of R1 for both. > Agreed. > BTW, I still think data_array should be allocated during probing, > as it is related to the hardware, not to a plane. > Ack. I'll do that as a separate patch on v5. >> +}; > >> @@ -159,23 +173,23 @@ static int ssd130x_buf_alloc(struct ssd130x_device *ssd130x) >> >> pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width); >> >> - ssd130x->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL); >> - if (!ssd130x->buffer) >> + ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL); >> + if (!ssd130x_state->buffer) >> return -ENOMEM; >> >> - ssd130x->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL); >> - if (!ssd130x->data_array) { >> - kfree(ssd130x->buffer); >> + ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL); >> + if (!ssd130x_state->data_array) { >> + kfree(ssd130x_state->buffer); > > Should ssd130x_state->buffer be reset to NULL here? > I.e. if .atomic_check() fails, will .atomic_destroy_state() be called, > leading to a double free? > Yeah. I'll add it. >> return -ENOMEM; >> } >> >> return 0; >> } > >> @@ -576,18 +593,21 @@ static void ssd130x_clear_screen(struct ssd130x_device *ssd130x) >> .y2 = ssd130x->height, >> }; >> >> - ssd130x_update_rect(ssd130x, &fullscreen); >> + ssd130x_update_rect(ssd130x, ssd130x_state, &fullscreen); >> } >> >> -static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_map *vmap, >> +static int ssd130x_fb_blit_rect(struct drm_plane_state *state, >> + const struct iosys_map *vmap, >> struct drm_rect *rect) >> { >> + struct drm_framebuffer *fb = state->fb; >> struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev); >> + struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(state); >> unsigned int page_height = ssd130x->device_info->page_height; >> struct iosys_map dst; >> unsigned int dst_pitch; >> int ret = 0; >> - u8 *buf = ssd130x->buffer; >> + u8 *buf = ssd130x_state->buffer; >> >> if (!buf) > > This check should no longer be needed (and prevented you from seeing > the issue before). > Ack. >> return 0; [...] >> +static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane, >> + struct drm_atomic_state *state) >> +{ >> + struct drm_device *drm = plane->dev; >> + struct ssd130x_device *ssd130x = drm_to_ssd130x(drm); >> + struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane); >> + struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state); >> + int ret; >> + >> + ret = drm_plane_helper_atomic_check(plane, state); >> + if (ret) >> + return ret; >> + >> + return ssd130x_buf_alloc(ssd130x, ssd130x_state); > > I think the code would become easier to read by inlining > ssd130x_buf_alloc() here. > Agreed. I'll do that. >> +} >> + > >> +static struct drm_plane_state *ssd130x_primary_plane_duplicate_state(struct drm_plane *plane) >> +{ >> + struct ssd130x_plane_state *old_ssd130x_state; >> + struct ssd130x_plane_state *ssd130x_state; >> + >> + if (WARN_ON(!plane->state)) >> + return NULL; >> + >> + old_ssd130x_state = to_ssd130x_plane_state(plane->state); >> + ssd130x_state = kmemdup(old_ssd130x_state, sizeof(*ssd130x_state), GFP_KERNEL); > > I know this is the standard pattern, but the "dup" part is a bit > silly here: > - The ssd130x-specific parts in ssd130x_plane_state are zeroed below, > - The base part is copied again in > __drm_atomic_helper_plane_duplicate_state). > So (for now) you might as well just call kzalloc() ;-) > Indeed. You are correct. >> + if (!ssd130x_state) >> + return NULL; >> + >> + /* The buffers are not duplicated and are allocated in .atomic_check */ >> + ssd130x_state->buffer = NULL; >> + ssd130x_state->data_array = NULL; >> + >> + __drm_atomic_helper_plane_duplicate_state(plane, &ssd130x_state->base); >> + >> + return &ssd130x_state->base; >> +} >> + >> +static void ssd130x_primary_plane_destroy_state(struct drm_plane *plane, >> + struct drm_plane_state *state) >> +{ >> + struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(state); >> + >> + ssd130x_buf_free(ssd130x_state); > > I think the code would become easier to read by inlining > ssd130x_buf_free() here. > Ack.
On Tue, Jul 25, 2023 at 09:21:52PM +0200, Geert Uytterhoeven wrote: > > --- a/drivers/gpu/drm/solomon/ssd130x.c > > +++ b/drivers/gpu/drm/solomon/ssd130x.c > > @@ -141,12 +141,26 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = { > > }; > > EXPORT_SYMBOL_NS_GPL(ssd130x_variants, DRM_SSD130X); > > > > +struct ssd130x_plane_state { > > + struct drm_plane_state base; > > + /* Intermediate buffer to convert pixels from XRGB8888 to R1 */ > > + u8 *buffer; > > + /* Buffer that contains R1 pixels to be written to the panel */ > > + u8 *data_array; > > The second buffer actually contains pixels in hardware format. > For now that is a transposed buffer of R1 pixels, but that will change > when you will add support for greyscale displays. > > So I'd write "hardware format" instead of R1 for both. > > BTW, I still think data_array should be allocated during probing, > as it is related to the hardware, not to a plane. I somewhat disagree. If I understood right during our discussion with Javier, the buffer size derives from the mode size (height and width). In KMS, the mode is tied to the KMS state, and thus you can expect the mode to change every state commit. So the more logical thing to do is to tie the buffer size (and thus the buffer pointer) to the state since it's only valid for that particular state for all we know. Of course, our case is allows use to simplify things since it's a fixed mode, but one of Javier's goal with this driver was to provide a good example we can refer people to, so I think it's worth keeping. > > @@ -159,23 +173,23 @@ static int ssd130x_buf_alloc(struct ssd130x_device *ssd130x) > > > > pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width); > > > > - ssd130x->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL); > > - if (!ssd130x->buffer) > > + ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL); > > + if (!ssd130x_state->buffer) > > return -ENOMEM; > > > > - ssd130x->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL); > > - if (!ssd130x->data_array) { > > - kfree(ssd130x->buffer); > > + ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL); > > + if (!ssd130x_state->data_array) { > > + kfree(ssd130x_state->buffer); > > Should ssd130x_state->buffer be reset to NULL here? > I.e. if .atomic_check() fails, will .atomic_destroy_state() be called, > leading to a double free? That's a good question. I never really thought of that, but yeah, AFAIK atomic_destroy_state() will be called when atomic_check() fails. Which means that it's probably broken in a lot of drivers. Also, Javier pointed me to a discussion you had on IRC about using devm allocation here. We can't really do that. KMS devices are only freed once the last userspace application closes its fd to the device file, so you have an unbounded window during which the driver is still callable by userspace (and thus can still trigger an atomic commit) but the buffer would have been freed for a while. The driver could use a bit more work on that area (by adding drm_dev_enter/drm_dev_exit) but it still creates unnecessary risks to use devm there. > > +static struct drm_plane_state *ssd130x_primary_plane_duplicate_state(struct drm_plane *plane) > > +{ > > + struct ssd130x_plane_state *old_ssd130x_state; > > + struct ssd130x_plane_state *ssd130x_state; > > + > > + if (WARN_ON(!plane->state)) > > + return NULL; > > + > > + old_ssd130x_state = to_ssd130x_plane_state(plane->state); > > + ssd130x_state = kmemdup(old_ssd130x_state, sizeof(*ssd130x_state), GFP_KERNEL); > > I know this is the standard pattern, but the "dup" part is a bit > silly here: > - The ssd130x-specific parts in ssd130x_plane_state are zeroed below, > - The base part is copied again in > __drm_atomic_helper_plane_duplicate_state). > So (for now) you might as well just call kzalloc() ;-) Still if we ever add a field in the state structure, it will be surprising that it's not copied over. The code is there and looks good to me, so I'd rather keep it.
Maxime Ripard <mripard@kernel.org> writes: Hello Maxime, > On Tue, Jul 25, 2023 at 09:21:52PM +0200, Geert Uytterhoeven wrote: >> > --- a/drivers/gpu/drm/solomon/ssd130x.c >> > +++ b/drivers/gpu/drm/solomon/ssd130x.c >> > @@ -141,12 +141,26 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = { >> > }; >> > EXPORT_SYMBOL_NS_GPL(ssd130x_variants, DRM_SSD130X); >> > >> > +struct ssd130x_plane_state { >> > + struct drm_plane_state base; >> > + /* Intermediate buffer to convert pixels from XRGB8888 to R1 */ >> > + u8 *buffer; >> > + /* Buffer that contains R1 pixels to be written to the panel */ >> > + u8 *data_array; >> >> The second buffer actually contains pixels in hardware format. >> For now that is a transposed buffer of R1 pixels, but that will change >> when you will add support for greyscale displays. >> >> So I'd write "hardware format" instead of R1 for both. >> >> BTW, I still think data_array should be allocated during probing, >> as it is related to the hardware, not to a plane. > > I somewhat disagree. > > If I understood right during our discussion with Javier, the buffer size > derives from the mode size (height and width). > > In KMS, the mode is tied to the KMS state, and thus you can expect the > mode to change every state commit. So the more logical thing to do is to > tie the buffer size (and thus the buffer pointer) to the state since > it's only valid for that particular state for all we know. > > Of course, our case is allows use to simplify things since it's a fixed > mode, but one of Javier's goal with this driver was to provide a good > example we can refer people to, so I think it's worth keeping. > Yes, that's certainly one of my goals. So I'll just keep it as-is then. >> > @@ -159,23 +173,23 @@ static int ssd130x_buf_alloc(struct ssd130x_device *ssd130x) >> > >> > pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width); >> > >> > - ssd130x->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL); >> > - if (!ssd130x->buffer) >> > + ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL); >> > + if (!ssd130x_state->buffer) >> > return -ENOMEM; >> > >> > - ssd130x->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL); >> > - if (!ssd130x->data_array) { >> > - kfree(ssd130x->buffer); >> > + ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL); >> > + if (!ssd130x_state->data_array) { >> > + kfree(ssd130x_state->buffer); >> >> Should ssd130x_state->buffer be reset to NULL here? >> I.e. if .atomic_check() fails, will .atomic_destroy_state() be called, >> leading to a double free? > > That's a good question. > > I never really thought of that, but yeah, AFAIK atomic_destroy_state() > will be called when atomic_check() fails. > Interesting. I didn't know that. I'll set to NULL and add a comment. > Which means that it's probably broken in a lot of drivers. > > Also, Javier pointed me to a discussion you had on IRC about using devm > allocation here. We can't really do that. KMS devices are only freed > once the last userspace application closes its fd to the device file, so > you have an unbounded window during which the driver is still callable > by userspace (and thus can still trigger an atomic commit) but the > buffer would have been freed for a while. > > The driver could use a bit more work on that area (by adding > drm_dev_enter/drm_dev_exit) but it still creates unnecessary risks to > use devm there. > Yes, but that discussion is not relevant anymore anyways if we keep the .data_array allocatioin the plane's .atomic_check handler. >> > +static struct drm_plane_state *ssd130x_primary_plane_duplicate_state(struct drm_plane *plane) >> > +{ >> > + struct ssd130x_plane_state *old_ssd130x_state; >> > + struct ssd130x_plane_state *ssd130x_state; >> > + >> > + if (WARN_ON(!plane->state)) >> > + return NULL; >> > + >> > + old_ssd130x_state = to_ssd130x_plane_state(plane->state); >> > + ssd130x_state = kmemdup(old_ssd130x_state, sizeof(*ssd130x_state), GFP_KERNEL); >> >> I know this is the standard pattern, but the "dup" part is a bit >> silly here: >> - The ssd130x-specific parts in ssd130x_plane_state are zeroed below, >> - The base part is copied again in >> __drm_atomic_helper_plane_duplicate_state). >> So (for now) you might as well just call kzalloc() ;-) > > Still if we ever add a field in the state structure, it will be > surprising that it's not copied over. > > The code is there and looks good to me, so I'd rather keep it. Yes, it's unlikely that this state structure will get other fields but still since is the standard patter, we can keep it just for others to use it as a reference.
Hi Maxime, On Wed, Jul 26, 2023 at 12:00 PM Maxime Ripard <mripard@kernel.org> wrote: > On Tue, Jul 25, 2023 at 09:21:52PM +0200, Geert Uytterhoeven wrote: > > > --- a/drivers/gpu/drm/solomon/ssd130x.c > > > +++ b/drivers/gpu/drm/solomon/ssd130x.c > > > @@ -141,12 +141,26 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = { > > > }; > > > EXPORT_SYMBOL_NS_GPL(ssd130x_variants, DRM_SSD130X); > > > > > > +struct ssd130x_plane_state { > > > + struct drm_plane_state base; > > > + /* Intermediate buffer to convert pixels from XRGB8888 to R1 */ > > > + u8 *buffer; > > > + /* Buffer that contains R1 pixels to be written to the panel */ > > > + u8 *data_array; > > > > The second buffer actually contains pixels in hardware format. > > For now that is a transposed buffer of R1 pixels, but that will change > > when you will add support for greyscale displays. > > > > So I'd write "hardware format" instead of R1 for both. > > > > BTW, I still think data_array should be allocated during probing, > > as it is related to the hardware, not to a plane. > > I somewhat disagree. > > If I understood right during our discussion with Javier, the buffer size > derives from the mode size (height and width). > > In KMS, the mode is tied to the KMS state, and thus you can expect the > mode to change every state commit. So the more logical thing to do is to > tie the buffer size (and thus the buffer pointer) to the state since > it's only valid for that particular state for all we know. > > Of course, our case is allows use to simplify things since it's a fixed > mode, but one of Javier's goal with this driver was to provide a good > example we can refer people to, so I think it's worth keeping. The second buffer (containing the hardware format) has a size that depends on the full screen size, not the current mode (I believe that's also the size of the frame buffer backing the plane?). So its size is fixed. Given the allocations are now done based on plane state, I think the first buffer should be sized according to the frame buffer backing the plane? Currently it uses the full screen size, too (cfr. below). > > > @@ -159,23 +173,23 @@ static int ssd130x_buf_alloc(struct ssd130x_device *ssd130x) > > > > > > pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width); > > > > > > - ssd130x->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL); > > > - if (!ssd130x->buffer) > > > + ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL); Based on full screen width and height. > > > + if (!ssd130x_state->buffer) > > > return -ENOMEM; > > > > > > - ssd130x->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL); > > > - if (!ssd130x->data_array) { > > > - kfree(ssd130x->buffer); > > > + ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL); Based on full screen width and height (hardware page size). > > > + if (!ssd130x_state->data_array) { > > > + kfree(ssd130x_state->buffer); > > > > Should ssd130x_state->buffer be reset to NULL here? > > I.e. if .atomic_check() fails, will .atomic_destroy_state() be called, > > leading to a double free? > > That's a good question. > > I never really thought of that, but yeah, AFAIK atomic_destroy_state() > will be called when atomic_check() fails. > > Which means that it's probably broken in a lot of drivers. > > Also, Javier pointed me to a discussion you had on IRC about using devm > allocation here. We can't really do that. KMS devices are only freed > once the last userspace application closes its fd to the device file, so > you have an unbounded window during which the driver is still callable > by userspace (and thus can still trigger an atomic commit) but the > buffer would have been freed for a while. It should still be safe for (at least) the data_array buffer. That buffer is only used to store pixels in hardware format, and immediately send them to the hardware. If this can be called that late, it will fail horribly, as you can no longer talk to the hardware at that point (as ssd130x is an i2c driver, it might actually work; but a DRM driver that calls devm_platform_ioremap_resource() will crash when writing to its MMIO registers)?!? > > > +static struct drm_plane_state *ssd130x_primary_plane_duplicate_state(struct drm_plane *plane) > > > +{ > > > + struct ssd130x_plane_state *old_ssd130x_state; > > > + struct ssd130x_plane_state *ssd130x_state; > > > + > > > + if (WARN_ON(!plane->state)) > > > + return NULL; > > > + > > > + old_ssd130x_state = to_ssd130x_plane_state(plane->state); > > > + ssd130x_state = kmemdup(old_ssd130x_state, sizeof(*ssd130x_state), GFP_KERNEL); > > > > I know this is the standard pattern, but the "dup" part is a bit > > silly here: > > - The ssd130x-specific parts in ssd130x_plane_state are zeroed below, > > - The base part is copied again in > > __drm_atomic_helper_plane_duplicate_state). > > So (for now) you might as well just call kzalloc() ;-) > > Still if we ever add a field in the state structure, it will be > surprising that it's not copied over. > > The code is there and looks good to me, so I'd rather keep it. Fair enough, let's keep it. Gr{oetje,eeting}s, Geert
Geert Uytterhoeven <geert@linux-m68k.org> writes: Hello Geert, > Hi Maxime, > > On Wed, Jul 26, 2023 at 12:00 PM Maxime Ripard <mripard@kernel.org> wrote: >> On Tue, Jul 25, 2023 at 09:21:52PM +0200, Geert Uytterhoeven wrote: >> > > --- a/drivers/gpu/drm/solomon/ssd130x.c >> > > +++ b/drivers/gpu/drm/solomon/ssd130x.c >> > > @@ -141,12 +141,26 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = { >> > > }; >> > > EXPORT_SYMBOL_NS_GPL(ssd130x_variants, DRM_SSD130X); >> > > >> > > +struct ssd130x_plane_state { >> > > + struct drm_plane_state base; >> > > + /* Intermediate buffer to convert pixels from XRGB8888 to R1 */ >> > > + u8 *buffer; >> > > + /* Buffer that contains R1 pixels to be written to the panel */ >> > > + u8 *data_array; >> > >> > The second buffer actually contains pixels in hardware format. >> > For now that is a transposed buffer of R1 pixels, but that will change >> > when you will add support for greyscale displays. >> > >> > So I'd write "hardware format" instead of R1 for both. >> > >> > BTW, I still think data_array should be allocated during probing, >> > as it is related to the hardware, not to a plane. >> >> I somewhat disagree. >> >> If I understood right during our discussion with Javier, the buffer size >> derives from the mode size (height and width). >> >> In KMS, the mode is tied to the KMS state, and thus you can expect the >> mode to change every state commit. So the more logical thing to do is to >> tie the buffer size (and thus the buffer pointer) to the state since >> it's only valid for that particular state for all we know. >> >> Of course, our case is allows use to simplify things since it's a fixed >> mode, but one of Javier's goal with this driver was to provide a good >> example we can refer people to, so I think it's worth keeping. > > The second buffer (containing the hardware format) has a size that > depends on the full screen size, not the current mode (I believe that's > also the size of the frame buffer backing the plane?). So its size is > fixed. > Yes, is fixed. But Maxime's point is that this is a characteristic of this particular device and even when the display resolution can't be changed, the correct thing to do is to keep all state related to the mode (even the buffer used to store the hardware pixels that are written to the display) > Given the allocations are now done based on plane state, I think the > first buffer should be sized according to the frame buffer backing > the plane? Currently it uses the full screen size, too (cfr. below). > But can the mode even be changed if ssd130x_connector_helper_get_modes() just adds a single display mode with mode->hdisplay == ssd130x->width and mode->vdisplay == ssd130x->height. >> > > @@ -159,23 +173,23 @@ static int ssd130x_buf_alloc(struct ssd130x_device *ssd130x) >> > > >> > > pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width); >> > > >> > > - ssd130x->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL); >> > > - if (!ssd130x->buffer) >> > > + ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL); > > Based on full screen width and height. > You think that using ssd130x->mode->vdisplay instead will be more clear here? >> > > + if (!ssd130x_state->buffer) >> > > return -ENOMEM; >> > > >> > > - ssd130x->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL); >> > > - if (!ssd130x->data_array) { >> > > - kfree(ssd130x->buffer); >> > > + ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL); > > Based on full screen width and height (hardware page size). > Yes, this depends on the panel attached to the OLED controller, and that resolution is fixed and taken from the Device Tree (or ACPI table). >> > > + if (!ssd130x_state->data_array) { >> > > + kfree(ssd130x_state->buffer); >> > >> > Should ssd130x_state->buffer be reset to NULL here? >> > I.e. if .atomic_check() fails, will .atomic_destroy_state() be called, >> > leading to a double free? >> >> That's a good question. >> >> I never really thought of that, but yeah, AFAIK atomic_destroy_state() >> will be called when atomic_check() fails. >> >> Which means that it's probably broken in a lot of drivers. >> >> Also, Javier pointed me to a discussion you had on IRC about using devm >> allocation here. We can't really do that. KMS devices are only freed >> once the last userspace application closes its fd to the device file, so >> you have an unbounded window during which the driver is still callable >> by userspace (and thus can still trigger an atomic commit) but the >> buffer would have been freed for a while. > > It should still be safe for (at least) the data_array buffer. That > buffer is only used to store pixels in hardware format, and immediately > send them to the hardware. If this can be called that late, it will > fail horribly, as you can no longer talk to the hardware at that point > (as ssd130x is an i2c driver, it might actually work; but a DRM driver > that calls devm_platform_ioremap_resource() will crash when writing > to its MMIO registers)?!? > At the very least the SPI driver will fail since the GPIO that is used to toggle the D/C pin is allocated with devm_gpiod_get_optional(), but also the regulator, backlight device, etc. But in any case, as mentioned it is only relevant if the data_array buffer is allocated at probe time, and from Maxime's explanation is more correct to do it in the .atomic_check handler. >> > > +static struct drm_plane_state *ssd130x_primary_plane_duplicate_state(struct drm_plane *plane) >> > > +{ >> > > + struct ssd130x_plane_state *old_ssd130x_state; >> > > + struct ssd130x_plane_state *ssd130x_state; >> > > + >> > > + if (WARN_ON(!plane->state)) >> > > + return NULL; >> > > + >> > > + old_ssd130x_state = to_ssd130x_plane_state(plane->state); >> > > + ssd130x_state = kmemdup(old_ssd130x_state, sizeof(*ssd130x_state), GFP_KERNEL); >> > >> > I know this is the standard pattern, but the "dup" part is a bit >> > silly here: >> > - The ssd130x-specific parts in ssd130x_plane_state are zeroed below, >> > - The base part is copied again in >> > __drm_atomic_helper_plane_duplicate_state). >> > So (for now) you might as well just call kzalloc() ;-) >> >> Still if we ever add a field in the state structure, it will be >> surprising that it's not copied over. >> >> The code is there and looks good to me, so I'd rather keep it. > > Fair enough, let's keep it. > Yeah. At the very least helps since will be consistent with other drivers. > Gr{oetje,eeting}s, > > Geert
Hi Javier, On Wed, Jul 26, 2023 at 2:22 PM Javier Martinez Canillas <javierm@redhat.com> wrote: > Geert Uytterhoeven <geert@linux-m68k.org> writes: > > On Wed, Jul 26, 2023 at 12:00 PM Maxime Ripard <mripard@kernel.org> wrote: > >> On Tue, Jul 25, 2023 at 09:21:52PM +0200, Geert Uytterhoeven wrote: > >> > > --- a/drivers/gpu/drm/solomon/ssd130x.c > >> > > +++ b/drivers/gpu/drm/solomon/ssd130x.c > >> > > @@ -141,12 +141,26 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = { > >> > > }; > >> > > EXPORT_SYMBOL_NS_GPL(ssd130x_variants, DRM_SSD130X); > >> > > > >> > > +struct ssd130x_plane_state { > >> > > + struct drm_plane_state base; > >> > > + /* Intermediate buffer to convert pixels from XRGB8888 to R1 */ > >> > > + u8 *buffer; > >> > > + /* Buffer that contains R1 pixels to be written to the panel */ > >> > > + u8 *data_array; > >> > > >> > The second buffer actually contains pixels in hardware format. > >> > For now that is a transposed buffer of R1 pixels, but that will change > >> > when you will add support for greyscale displays. > >> > > >> > So I'd write "hardware format" instead of R1 for both. > >> > > >> > BTW, I still think data_array should be allocated during probing, > >> > as it is related to the hardware, not to a plane. > >> > >> I somewhat disagree. > >> > >> If I understood right during our discussion with Javier, the buffer size > >> derives from the mode size (height and width). > >> > >> In KMS, the mode is tied to the KMS state, and thus you can expect the > >> mode to change every state commit. So the more logical thing to do is to > >> tie the buffer size (and thus the buffer pointer) to the state since > >> it's only valid for that particular state for all we know. > >> > >> Of course, our case is allows use to simplify things since it's a fixed > >> mode, but one of Javier's goal with this driver was to provide a good > >> example we can refer people to, so I think it's worth keeping. > > > > The second buffer (containing the hardware format) has a size that > > depends on the full screen size, not the current mode (I believe that's > > also the size of the frame buffer backing the plane?). So its size is > > fixed. > > > > Yes, is fixed. But Maxime's point is that this is a characteristic of this > particular device and even when the display resolution can't be changed, > the correct thing to do is to keep all state related to the mode (even the > buffer used to store the hardware pixels that are written to the display) > > > Given the allocations are now done based on plane state, I think the > > first buffer should be sized according to the frame buffer backing > > the plane? Currently it uses the full screen size, too (cfr. below). > > > > But can the mode even be changed if ssd130x_connector_helper_get_modes() > just adds a single display mode with mode->hdisplay == ssd130x->width and > mode->vdisplay == ssd130x->height. No, the mode cannot be changed. At first, I thought you could still create a smaller frame buffer, and attach that to the (single, thus primary) plane, but it turns out I was wrong[*], so you can ignore that. [*] ssd130x_primary_plane_helper_atomic_check() calls drm_plane_helper_atomic_check(), which calls drm_atomic_helper_check_plane_state() with can_position = false. As the position of planes is actually a software thing on ssd130x, positioning support could be added later, though... > >> Also, Javier pointed me to a discussion you had on IRC about using devm > >> allocation here. We can't really do that. KMS devices are only freed > >> once the last userspace application closes its fd to the device file, so > >> you have an unbounded window during which the driver is still callable > >> by userspace (and thus can still trigger an atomic commit) but the > >> buffer would have been freed for a while. > > > > It should still be safe for (at least) the data_array buffer. That > > buffer is only used to store pixels in hardware format, and immediately > > send them to the hardware. If this can be called that late, it will > > fail horribly, as you can no longer talk to the hardware at that point > > (as ssd130x is an i2c driver, it might actually work; but a DRM driver > > that calls devm_platform_ioremap_resource() will crash when writing > > to its MMIO registers)?!? > > At the very least the SPI driver will fail since the GPIO that is used to > toggle the D/C pin is allocated with devm_gpiod_get_optional(), but also > the regulator, backlight device, etc. > > But in any case, as mentioned it is only relevant if the data_array buffer > is allocated at probe time, and from Maxime's explanation is more correct > to do it in the .atomic_check handler. You need (at least) data_array for clear_screen, too, which is called from .atomic_disable(). Gr{oetje,eeting}s, Geert
Hi, On Wed, Jul 26, 2023 at 01:52:37PM +0200, Geert Uytterhoeven wrote: > On Wed, Jul 26, 2023 at 12:00 PM Maxime Ripard <mripard@kernel.org> wrote: > > On Tue, Jul 25, 2023 at 09:21:52PM +0200, Geert Uytterhoeven wrote: > > > > --- a/drivers/gpu/drm/solomon/ssd130x.c > > > > +++ b/drivers/gpu/drm/solomon/ssd130x.c > > > > @@ -141,12 +141,26 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = { > > > > }; > > > > EXPORT_SYMBOL_NS_GPL(ssd130x_variants, DRM_SSD130X); > > > > > > > > +struct ssd130x_plane_state { > > > > + struct drm_plane_state base; > > > > + /* Intermediate buffer to convert pixels from XRGB8888 to R1 */ > > > > + u8 *buffer; > > > > + /* Buffer that contains R1 pixels to be written to the panel */ > > > > + u8 *data_array; > > > > > > The second buffer actually contains pixels in hardware format. > > > For now that is a transposed buffer of R1 pixels, but that will change > > > when you will add support for greyscale displays. > > > > > > So I'd write "hardware format" instead of R1 for both. > > > > > > BTW, I still think data_array should be allocated during probing, > > > as it is related to the hardware, not to a plane. > > > > I somewhat disagree. > > > > If I understood right during our discussion with Javier, the buffer size > > derives from the mode size (height and width). > > > > In KMS, the mode is tied to the KMS state, and thus you can expect the > > mode to change every state commit. So the more logical thing to do is to > > tie the buffer size (and thus the buffer pointer) to the state since > > it's only valid for that particular state for all we know. > > > > Of course, our case is allows use to simplify things since it's a fixed > > mode, but one of Javier's goal with this driver was to provide a good > > example we can refer people to, so I think it's worth keeping. > > The second buffer (containing the hardware format) has a size that > depends on the full screen size, not the current mode (I believe that's > also the size of the frame buffer backing the plane?). So its size is > fixed. In KMS in general, no. For that particular case, yes. And about the framebuffer size == full screen size, there's multiple sizes involved. I guess what you call full screen size is the CRTC size. You can't make the assumption in KMS that CRTC size == (primary) plane size == framebuffer size. If you're using scaling for example, you will have a framebuffer size smaller or larger than it plane size. If you're using cropping, then the plane size or framebuffer size will be different from the CRTC size. Some ways to work around overscan is also to have a smaller plane size than the CRTC size. You don't have to have the CRTC size == primary plane size, and then you don't have to have plane size == framebuffer size. you can't really make that assumption in the general case either: scaling or cropping will have a different framebuffer size than the CRTC primary plane size (which doesn't have to be "full screen" either). > Given the allocations are now done based on plane state, I think the > first buffer should be sized according to the frame buffer backing > the plane? Currently it uses the full screen size, too (cfr. below). Yeah, probably. > > > > @@ -159,23 +173,23 @@ static int ssd130x_buf_alloc(struct ssd130x_device *ssd130x) > > > > > > > > pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width); > > > > > > > > - ssd130x->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL); > > > > - if (!ssd130x->buffer) > > > > + ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL); > > Based on full screen width and height. > > > > > + if (!ssd130x_state->buffer) > > > > return -ENOMEM; > > > > > > > > - ssd130x->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL); > > > > - if (!ssd130x->data_array) { > > > > - kfree(ssd130x->buffer); > > > > + ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL); > > Based on full screen width and height (hardware page size). > > > > > + if (!ssd130x_state->data_array) { > > > > + kfree(ssd130x_state->buffer); > > > > > > Should ssd130x_state->buffer be reset to NULL here? > > > I.e. if .atomic_check() fails, will .atomic_destroy_state() be called, > > > leading to a double free? > > > > That's a good question. > > > > I never really thought of that, but yeah, AFAIK atomic_destroy_state() > > will be called when atomic_check() fails. > > > > Which means that it's probably broken in a lot of drivers. > > > > Also, Javier pointed me to a discussion you had on IRC about using devm > > allocation here. We can't really do that. KMS devices are only freed > > once the last userspace application closes its fd to the device file, so > > you have an unbounded window during which the driver is still callable > > by userspace (and thus can still trigger an atomic commit) but the > > buffer would have been freed for a while. > > It should still be safe for (at least) the data_array buffer. That > buffer is only used to store pixels in hardware format, and immediately > send them to the hardware. If this can be called that late, it will > fail horribly, as you can no longer talk to the hardware at that point > (as ssd130x is an i2c driver, it might actually work; but a DRM driver > that calls devm_platform_ioremap_resource() will crash when writing > to its MMIO registers)?!? Yep, that's exactly why we have drm_dev_enter/drm_dev_exit :) Not a lot of drivers are using it but all should. vc4 does exactly what you are describing for example. Maxime
On Wed, Jul 26, 2023 at 02:33:06PM +0200, Geert Uytterhoeven wrote: > > >> Also, Javier pointed me to a discussion you had on IRC about using devm > > >> allocation here. We can't really do that. KMS devices are only freed > > >> once the last userspace application closes its fd to the device file, so > > >> you have an unbounded window during which the driver is still callable > > >> by userspace (and thus can still trigger an atomic commit) but the > > >> buffer would have been freed for a while. > > > > > > It should still be safe for (at least) the data_array buffer. That > > > buffer is only used to store pixels in hardware format, and immediately > > > send them to the hardware. If this can be called that late, it will > > > fail horribly, as you can no longer talk to the hardware at that point > > > (as ssd130x is an i2c driver, it might actually work; but a DRM driver > > > that calls devm_platform_ioremap_resource() will crash when writing > > > to its MMIO registers)?!? > > > > At the very least the SPI driver will fail since the GPIO that is used to > > toggle the D/C pin is allocated with devm_gpiod_get_optional(), but also > > the regulator, backlight device, etc. > > > > But in any case, as mentioned it is only relevant if the data_array buffer > > is allocated at probe time, and from Maxime's explanation is more correct > > to do it in the .atomic_check handler. > > You need (at least) data_array for clear_screen, too, which is called > from .atomic_disable(). I'm not sure I get what your concern is? Even if we entirely disable the plane, the state will not have been destroyed yet so you still have at least access to the data_array from the old state. Maxime
Hi Maxime, On Wed, Jul 26, 2023 at 3:54 PM Maxime Ripard <mripard@kernel.org> wrote: > On Wed, Jul 26, 2023 at 02:33:06PM +0200, Geert Uytterhoeven wrote: > > > >> Also, Javier pointed me to a discussion you had on IRC about using devm > > > >> allocation here. We can't really do that. KMS devices are only freed > > > >> once the last userspace application closes its fd to the device file, so > > > >> you have an unbounded window during which the driver is still callable > > > >> by userspace (and thus can still trigger an atomic commit) but the > > > >> buffer would have been freed for a while. > > > > > > > > It should still be safe for (at least) the data_array buffer. That > > > > buffer is only used to store pixels in hardware format, and immediately > > > > send them to the hardware. If this can be called that late, it will > > > > fail horribly, as you can no longer talk to the hardware at that point > > > > (as ssd130x is an i2c driver, it might actually work; but a DRM driver > > > > that calls devm_platform_ioremap_resource() will crash when writing > > > > to its MMIO registers)?!? > > > > > > At the very least the SPI driver will fail since the GPIO that is used to > > > toggle the D/C pin is allocated with devm_gpiod_get_optional(), but also > > > the regulator, backlight device, etc. > > > > > > But in any case, as mentioned it is only relevant if the data_array buffer > > > is allocated at probe time, and from Maxime's explanation is more correct > > > to do it in the .atomic_check handler. > > > > You need (at least) data_array for clear_screen, too, which is called > > from .atomic_disable(). > > I'm not sure I get what your concern is? > > Even if we entirely disable the plane, the state will not have been > destroyed yet so you still have at least access to the data_array from > the old state. Currently, clearing the screen is done from the plane's .atomic_disable() callback, so the plane's state should be fine. But IIUIC, DRM allows the user to enable/disable the display without creating any plane first, so clearing should be handled in the CRTC's .atomic_disable() callback instead? Then, would we still have access to valid plane state? Thanks! Gr{oetje,eeting}s, Geert
Maxime Ripard <mripard@kernel.org> writes: Hello Maxime, > Hi, > > On Wed, Jul 26, 2023 at 01:52:37PM +0200, Geert Uytterhoeven wrote: >> On Wed, Jul 26, 2023 at 12:00 PM Maxime Ripard <mripard@kernel.org> wrote: >> > On Tue, Jul 25, 2023 at 09:21:52PM +0200, Geert Uytterhoeven wrote: [...] >> The second buffer (containing the hardware format) has a size that >> depends on the full screen size, not the current mode (I believe that's >> also the size of the frame buffer backing the plane?). So its size is >> fixed. > > In KMS in general, no. For that particular case, yes. > > And about the framebuffer size == full screen size, there's multiple > sizes involved. I guess what you call full screen size is the CRTC size. > > You can't make the assumption in KMS that CRTC size == (primary) plane > size == framebuffer size. > > If you're using scaling for example, you will have a framebuffer size > smaller or larger than it plane size. If you're using cropping, then the > plane size or framebuffer size will be different from the CRTC size. > Some ways to work around overscan is also to have a smaller plane size > than the CRTC size. > > You don't have to have the CRTC size == primary plane size, and then you > don't have to have plane size == framebuffer size. > > you can't really make that assumption in the general case either: > scaling or cropping will have a different framebuffer size than the CRTC > primary plane size (which doesn't have to be "full screen" either). > Yes, this particular driver is using the drm_plane_helper_atomic_check() as the primary plane .atomic_check and this function helper calls to: drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state, DRM_PLANE_NO_SCALING, DRM_PLANE_NO_SCALING, false, false); so it does not support scaling nor positioning. >> Given the allocations are now done based on plane state, I think the >> first buffer should be sized according to the frame buffer backing >> the plane? Currently it uses the full screen size, too (cfr. below). > > Yeah, probably. > Right, that could be fixed as another patch if anything to make it more reable since it won't have any functional change. >> > > > @@ -159,23 +173,23 @@ static int ssd130x_buf_alloc(struct ssd130x_device *ssd130x) >> > > > >> > > > pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width); >> > > > >> > > > - ssd130x->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL); >> > > > - if (!ssd130x->buffer) >> > > > + ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL); >> >> Based on full screen width and height. >> >> > > > + if (!ssd130x_state->buffer) >> > > > return -ENOMEM; >> > > > >> > > > - ssd130x->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL); >> > > > - if (!ssd130x->data_array) { >> > > > - kfree(ssd130x->buffer); >> > > > + ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL); >> >> Based on full screen width and height (hardware page size). >> >> > > > + if (!ssd130x_state->data_array) { >> > > > + kfree(ssd130x_state->buffer); >> > > >> > > Should ssd130x_state->buffer be reset to NULL here? >> > > I.e. if .atomic_check() fails, will .atomic_destroy_state() be called, >> > > leading to a double free? >> > >> > That's a good question. >> > >> > I never really thought of that, but yeah, AFAIK atomic_destroy_state() >> > will be called when atomic_check() fails. >> > >> > Which means that it's probably broken in a lot of drivers. >> > >> > Also, Javier pointed me to a discussion you had on IRC about using devm >> > allocation here. We can't really do that. KMS devices are only freed >> > once the last userspace application closes its fd to the device file, so >> > you have an unbounded window during which the driver is still callable >> > by userspace (and thus can still trigger an atomic commit) but the >> > buffer would have been freed for a while. >> >> It should still be safe for (at least) the data_array buffer. That >> buffer is only used to store pixels in hardware format, and immediately >> send them to the hardware. If this can be called that late, it will >> fail horribly, as you can no longer talk to the hardware at that point >> (as ssd130x is an i2c driver, it might actually work; but a DRM driver >> that calls devm_platform_ioremap_resource() will crash when writing >> to its MMIO registers)?!? > > Yep, that's exactly why we have drm_dev_enter/drm_dev_exit :) > Thanks. As a follow-up I can also do that in another patch.
Geert Uytterhoeven <geert@linux-m68k.org> writes: Hello Geert, > Hi Maxime, > > On Wed, Jul 26, 2023 at 3:54 PM Maxime Ripard <mripard@kernel.org> wrote: >> On Wed, Jul 26, 2023 at 02:33:06PM +0200, Geert Uytterhoeven wrote: >> > > >> Also, Javier pointed me to a discussion you had on IRC about using devm >> > > >> allocation here. We can't really do that. KMS devices are only freed >> > > >> once the last userspace application closes its fd to the device file, so >> > > >> you have an unbounded window during which the driver is still callable >> > > >> by userspace (and thus can still trigger an atomic commit) but the >> > > >> buffer would have been freed for a while. >> > > > >> > > > It should still be safe for (at least) the data_array buffer. That >> > > > buffer is only used to store pixels in hardware format, and immediately >> > > > send them to the hardware. If this can be called that late, it will >> > > > fail horribly, as you can no longer talk to the hardware at that point >> > > > (as ssd130x is an i2c driver, it might actually work; but a DRM driver >> > > > that calls devm_platform_ioremap_resource() will crash when writing >> > > > to its MMIO registers)?!? >> > > >> > > At the very least the SPI driver will fail since the GPIO that is used to >> > > toggle the D/C pin is allocated with devm_gpiod_get_optional(), but also >> > > the regulator, backlight device, etc. >> > > >> > > But in any case, as mentioned it is only relevant if the data_array buffer >> > > is allocated at probe time, and from Maxime's explanation is more correct >> > > to do it in the .atomic_check handler. >> > >> > You need (at least) data_array for clear_screen, too, which is called >> > from .atomic_disable(). >> >> I'm not sure I get what your concern is? >> >> Even if we entirely disable the plane, the state will not have been >> destroyed yet so you still have at least access to the data_array from >> the old state. > > Currently, clearing the screen is done from the plane's .atomic_disable() > callback, so the plane's state should be fine. > > But IIUIC, DRM allows the user to enable/disable the display without > creating any plane first, so clearing should be handled in the CRTC's But it's needed to be clared in this case? Currently the power on/off is done in the encoder's .atomic_{en,dis}able() but the actual panel clear is only done for the plane disable as you mentioned. > .atomic_disable() callback instead? Then, would we still have access > to valid plane state? > If the clear has to be moved to the CRTC atomic enable/disable, then the driver should be reworked to not use the data_array and instead just allocate a zero'ed buffer and pass that as you proposed. But again that's something that could be done later as another patch. > Thanks! > > Gr{oetje,eeting}s, >
Hi Javier, this patch is completely broken. It's easy to fix though. Am 21.07.23 um 09:09 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 having custom helpers to allocate, duplicate and destroy the > plane state, that will take care of allocating and freeing these buffers. > > Instead of doing it in the encoder atomic enabled and disabled callbacks, > since allocations must not be done in an .atomic_enable handler. Because > drivers are not allowed to fail after drm_atomic_helper_swap_state() has > been called and the new atomic state is stored into the current sw state. > > Fixes: 49d7d581ceaf ("drm/ssd130x: Don't allocate buffers on each plane update") > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > Suggested-by: Maxime Ripard <mripard@kernel.org> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > --- > > Changes in v4: > - Move buffers allocation/free to plane .reset/.destroy helpers (Maxime Ripard). > > 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 | 134 ++++++++++++++++++++++++------ > drivers/gpu/drm/solomon/ssd130x.h | 3 - > 2 files changed, 108 insertions(+), 29 deletions(-) > > diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c > index afb08a8aa9fc..21b2afe40b13 100644 > --- a/drivers/gpu/drm/solomon/ssd130x.c > +++ b/drivers/gpu/drm/solomon/ssd130x.c > @@ -141,12 +141,26 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = { > }; > EXPORT_SYMBOL_NS_GPL(ssd130x_variants, DRM_SSD130X); > > +struct ssd130x_plane_state { > + struct drm_plane_state base; You need to use a struct drm_shadow_plane_state here. > + /* Intermediate buffer to convert pixels from XRGB8888 to R1 */ > + u8 *buffer; > + /* Buffer that contains R1 pixels to be written to the panel */ > + u8 *data_array; > +}; > + > +static inline struct ssd130x_plane_state *to_ssd130x_plane_state(struct drm_plane_state *state) > +{ > + return container_of(state, struct ssd130x_plane_state, base); > +} > + > 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, > + struct ssd130x_plane_state *ssd130x_state) > { > unsigned int page_height = ssd130x->device_info->page_height; > unsigned int pages = DIV_ROUND_UP(ssd130x->height, page_height); > @@ -159,23 +173,23 @@ static int ssd130x_buf_alloc(struct ssd130x_device *ssd130x) > > pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width); > > - ssd130x->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL); > - if (!ssd130x->buffer) > + ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL); > + if (!ssd130x_state->buffer) > return -ENOMEM; > > - ssd130x->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL); > - if (!ssd130x->data_array) { > - kfree(ssd130x->buffer); > + ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL); > + if (!ssd130x_state->data_array) { > + kfree(ssd130x_state->buffer); > return -ENOMEM; > } > > return 0; > } > > -static void ssd130x_buf_free(struct ssd130x_device *ssd130x) > +static void ssd130x_buf_free(struct ssd130x_plane_state *ssd130x_state) > { > - kfree(ssd130x->data_array); > - kfree(ssd130x->buffer); > + kfree(ssd130x_state->data_array); > + kfree(ssd130x_state->buffer); > } > > /* > @@ -466,12 +480,14 @@ static int ssd130x_init(struct ssd130x_device *ssd130x) > SSD130X_SET_ADDRESS_MODE_HORIZONTAL); > } > > -static int ssd130x_update_rect(struct ssd130x_device *ssd130x, struct drm_rect *rect) > +static int ssd130x_update_rect(struct ssd130x_device *ssd130x, > + struct ssd130x_plane_state *ssd130x_state, > + struct drm_rect *rect) > { > unsigned int x = rect->x1; > unsigned int y = rect->y1; > - u8 *buf = ssd130x->buffer; > - u8 *data_array = ssd130x->data_array; > + u8 *buf = ssd130x_state->buffer; > + u8 *data_array = ssd130x_state->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); > @@ -567,7 +583,8 @@ static int ssd130x_update_rect(struct ssd130x_device *ssd130x, struct drm_rect * > return ret; > } > > -static void ssd130x_clear_screen(struct ssd130x_device *ssd130x) > +static void ssd130x_clear_screen(struct ssd130x_device *ssd130x, > + struct ssd130x_plane_state *ssd130x_state) > { > struct drm_rect fullscreen = { > .x1 = 0, > @@ -576,18 +593,21 @@ static void ssd130x_clear_screen(struct ssd130x_device *ssd130x) > .y2 = ssd130x->height, > }; > > - ssd130x_update_rect(ssd130x, &fullscreen); > + ssd130x_update_rect(ssd130x, ssd130x_state, &fullscreen); > } > > -static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_map *vmap, > +static int ssd130x_fb_blit_rect(struct drm_plane_state *state, > + const struct iosys_map *vmap, > struct drm_rect *rect) > { > + struct drm_framebuffer *fb = state->fb; > struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev); > + struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(state); > unsigned int page_height = ssd130x->device_info->page_height; > struct iosys_map dst; > unsigned int dst_pitch; > int ret = 0; > - u8 *buf = ssd130x->buffer; > + u8 *buf = ssd130x_state->buffer; > > if (!buf) > return 0; > @@ -607,11 +627,27 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m > > drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE); > > - ssd130x_update_rect(ssd130x, rect); > + ssd130x_update_rect(ssd130x, ssd130x_state, rect); > > return ret; > } > > +static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane, > + struct drm_atomic_state *state) > +{ > + struct drm_device *drm = plane->dev; > + struct ssd130x_device *ssd130x = drm_to_ssd130x(drm); > + struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane); > + struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state); > + int ret; > + > + ret = drm_plane_helper_atomic_check(plane, state); > + if (ret) > + return ret; > + > + return ssd130x_buf_alloc(ssd130x, ssd130x_state); > +} > + > static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane, > struct drm_atomic_state *state) > { > @@ -634,7 +670,7 @@ static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane, > if (!drm_rect_intersect(&dst_clip, &damage)) > continue; > > - ssd130x_fb_blit_rect(plane_state->fb, &shadow_plane_state->data[0], &dst_clip); > + ssd130x_fb_blit_rect(plane_state, &shadow_plane_state->data[0], &dst_clip); > } > > drm_dev_exit(idx); > @@ -645,19 +681,68 @@ static void ssd130x_primary_plane_helper_atomic_disable(struct drm_plane *plane, > { > struct drm_device *drm = plane->dev; > struct ssd130x_device *ssd130x = drm_to_ssd130x(drm); > + struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane->state); > int idx; > > if (!drm_dev_enter(drm, &idx)) > return; > > - ssd130x_clear_screen(ssd130x); > + ssd130x_clear_screen(ssd130x, ssd130x_state); > > drm_dev_exit(idx); > } > > +/* Called during init to allocate the plane's atomic state. */ > +static void ssd130x_primary_plane_reset(struct drm_plane *plane) > +{ > + struct ssd130x_plane_state *ssd130x_state; > + > + WARN_ON(plane->state); > + > + ssd130x_state = kzalloc(sizeof(*ssd130x_state), GFP_KERNEL); > + if (!ssd130x_state) > + return; > + > + __drm_atomic_helper_plane_reset(plane, &ssd130x_state->base); Use __drm_gem_reset_shadow_plane() here > +} > + > +static struct drm_plane_state *ssd130x_primary_plane_duplicate_state(struct drm_plane *plane) > +{ > + struct ssd130x_plane_state *old_ssd130x_state; > + struct ssd130x_plane_state *ssd130x_state; > + > + if (WARN_ON(!plane->state)) > + return NULL; > + > + old_ssd130x_state = to_ssd130x_plane_state(plane->state); > + ssd130x_state = kmemdup(old_ssd130x_state, sizeof(*ssd130x_state), GFP_KERNEL); > + if (!ssd130x_state) > + return NULL; > + > + /* The buffers are not duplicated and are allocated in .atomic_check */ > + ssd130x_state->buffer = NULL; > + ssd130x_state->data_array = NULL; > + > + __drm_atomic_helper_plane_duplicate_state(plane, &ssd130x_state->base); Use __drm_gem_duplicate_shadow_plane_state() here. > + > + return &ssd130x_state->base; > +} > + > +static void ssd130x_primary_plane_destroy_state(struct drm_plane *plane, > + struct drm_plane_state *state) > +{ > + struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(state); > + > + ssd130x_buf_free(ssd130x_state); > + > + __drm_atomic_helper_plane_destroy_state(&ssd130x_state->base); Use __drm_gem_destroy_shadow_plane_state() here. > + > + kfree(ssd130x_state); > +} > + > static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = { > DRM_GEM_SHADOW_PLANE_HELPER_FUNCS, This macro sets the callbacks that vmap/vunmap the GEM buffer during the display update. They expect to upcast from drm_plane_state to drm_shadow_plane_state. And hence, your driver's plane state needs to inherit from drm_shadow_plane_state. > - .atomic_check = drm_plane_helper_atomic_check, > + .atomic_check = ssd130x_primary_plane_helper_atomic_check, > .atomic_update = ssd130x_primary_plane_helper_atomic_update, > .atomic_disable = ssd130x_primary_plane_helper_atomic_disable, > }; > @@ -665,6 +750,9 @@ static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = > static const struct drm_plane_funcs ssd130x_primary_plane_funcs = { > .update_plane = drm_atomic_helper_update_plane, > .disable_plane = drm_atomic_helper_disable_plane, > + .reset = ssd130x_primary_plane_reset, > + .atomic_duplicate_state = ssd130x_primary_plane_duplicate_state, > + .atomic_destroy_state = ssd130x_primary_plane_destroy_state, > .destroy = drm_plane_cleanup, > DRM_GEM_SHADOW_PLANE_FUNCS, Arnd already mentioned that this macro now needs to go away. Best regards Thomas > }; > @@ -719,10 +807,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); > @@ -744,8 +828,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.h b/drivers/gpu/drm/solomon/ssd130x.h > index 161588b1cc4d..87968b3e7fb8 100644 > --- a/drivers/gpu/drm/solomon/ssd130x.h > +++ b/drivers/gpu/drm/solomon/ssd130x.h > @@ -89,9 +89,6 @@ struct ssd130x_device { > u8 col_end; > u8 page_start; > u8 page_end; > - > - u8 *buffer; > - u8 *data_array; > }; > > extern const struct ssd130x_deviceinfo ssd130x_variants[];
Thomas Zimmermann <tzimmermann@suse.de> writes: Hello Thomas, Thanks a lot for the feedback! > Hi Javier, > > this patch is completely broken. It's easy to fix though. > > Am 21.07.23 um 09:09 schrieb Javier Martinez Canillas: [...] >> +struct ssd130x_plane_state { >> + struct drm_plane_state base; > > You need to use a struct drm_shadow_plane_state here. > Yes, I already noticed this when testing Arnd's patch to drop DRM_GEM_SHADOW_PLANE_FUNCS. I already have a patch ready. [...] + >> static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = { >> DRM_GEM_SHADOW_PLANE_HELPER_FUNCS, > > This macro sets the callbacks that vmap/vunmap the GEM buffer during the > display update. They expect to upcast from drm_plane_state to > drm_shadow_plane_state. And hence, your driver's plane state needs to > inherit from drm_shadow_plane_state. > Yup. I missed that. I'm now testing my patch and will post it.
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c index afb08a8aa9fc..21b2afe40b13 100644 --- a/drivers/gpu/drm/solomon/ssd130x.c +++ b/drivers/gpu/drm/solomon/ssd130x.c @@ -141,12 +141,26 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = { }; EXPORT_SYMBOL_NS_GPL(ssd130x_variants, DRM_SSD130X); +struct ssd130x_plane_state { + struct drm_plane_state base; + /* Intermediate buffer to convert pixels from XRGB8888 to R1 */ + u8 *buffer; + /* Buffer that contains R1 pixels to be written to the panel */ + u8 *data_array; +}; + +static inline struct ssd130x_plane_state *to_ssd130x_plane_state(struct drm_plane_state *state) +{ + return container_of(state, struct ssd130x_plane_state, base); +} + 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, + struct ssd130x_plane_state *ssd130x_state) { unsigned int page_height = ssd130x->device_info->page_height; unsigned int pages = DIV_ROUND_UP(ssd130x->height, page_height); @@ -159,23 +173,23 @@ static int ssd130x_buf_alloc(struct ssd130x_device *ssd130x) pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width); - ssd130x->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL); - if (!ssd130x->buffer) + ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL); + if (!ssd130x_state->buffer) return -ENOMEM; - ssd130x->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL); - if (!ssd130x->data_array) { - kfree(ssd130x->buffer); + ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL); + if (!ssd130x_state->data_array) { + kfree(ssd130x_state->buffer); return -ENOMEM; } return 0; } -static void ssd130x_buf_free(struct ssd130x_device *ssd130x) +static void ssd130x_buf_free(struct ssd130x_plane_state *ssd130x_state) { - kfree(ssd130x->data_array); - kfree(ssd130x->buffer); + kfree(ssd130x_state->data_array); + kfree(ssd130x_state->buffer); } /* @@ -466,12 +480,14 @@ static int ssd130x_init(struct ssd130x_device *ssd130x) SSD130X_SET_ADDRESS_MODE_HORIZONTAL); } -static int ssd130x_update_rect(struct ssd130x_device *ssd130x, struct drm_rect *rect) +static int ssd130x_update_rect(struct ssd130x_device *ssd130x, + struct ssd130x_plane_state *ssd130x_state, + struct drm_rect *rect) { unsigned int x = rect->x1; unsigned int y = rect->y1; - u8 *buf = ssd130x->buffer; - u8 *data_array = ssd130x->data_array; + u8 *buf = ssd130x_state->buffer; + u8 *data_array = ssd130x_state->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); @@ -567,7 +583,8 @@ static int ssd130x_update_rect(struct ssd130x_device *ssd130x, struct drm_rect * return ret; } -static void ssd130x_clear_screen(struct ssd130x_device *ssd130x) +static void ssd130x_clear_screen(struct ssd130x_device *ssd130x, + struct ssd130x_plane_state *ssd130x_state) { struct drm_rect fullscreen = { .x1 = 0, @@ -576,18 +593,21 @@ static void ssd130x_clear_screen(struct ssd130x_device *ssd130x) .y2 = ssd130x->height, }; - ssd130x_update_rect(ssd130x, &fullscreen); + ssd130x_update_rect(ssd130x, ssd130x_state, &fullscreen); } -static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_map *vmap, +static int ssd130x_fb_blit_rect(struct drm_plane_state *state, + const struct iosys_map *vmap, struct drm_rect *rect) { + struct drm_framebuffer *fb = state->fb; struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev); + struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(state); unsigned int page_height = ssd130x->device_info->page_height; struct iosys_map dst; unsigned int dst_pitch; int ret = 0; - u8 *buf = ssd130x->buffer; + u8 *buf = ssd130x_state->buffer; if (!buf) return 0; @@ -607,11 +627,27 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE); - ssd130x_update_rect(ssd130x, rect); + ssd130x_update_rect(ssd130x, ssd130x_state, rect); return ret; } +static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane, + struct drm_atomic_state *state) +{ + struct drm_device *drm = plane->dev; + struct ssd130x_device *ssd130x = drm_to_ssd130x(drm); + struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane); + struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state); + int ret; + + ret = drm_plane_helper_atomic_check(plane, state); + if (ret) + return ret; + + return ssd130x_buf_alloc(ssd130x, ssd130x_state); +} + static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane, struct drm_atomic_state *state) { @@ -634,7 +670,7 @@ static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane, if (!drm_rect_intersect(&dst_clip, &damage)) continue; - ssd130x_fb_blit_rect(plane_state->fb, &shadow_plane_state->data[0], &dst_clip); + ssd130x_fb_blit_rect(plane_state, &shadow_plane_state->data[0], &dst_clip); } drm_dev_exit(idx); @@ -645,19 +681,68 @@ static void ssd130x_primary_plane_helper_atomic_disable(struct drm_plane *plane, { struct drm_device *drm = plane->dev; struct ssd130x_device *ssd130x = drm_to_ssd130x(drm); + struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane->state); int idx; if (!drm_dev_enter(drm, &idx)) return; - ssd130x_clear_screen(ssd130x); + ssd130x_clear_screen(ssd130x, ssd130x_state); drm_dev_exit(idx); } +/* Called during init to allocate the plane's atomic state. */ +static void ssd130x_primary_plane_reset(struct drm_plane *plane) +{ + struct ssd130x_plane_state *ssd130x_state; + + WARN_ON(plane->state); + + ssd130x_state = kzalloc(sizeof(*ssd130x_state), GFP_KERNEL); + if (!ssd130x_state) + return; + + __drm_atomic_helper_plane_reset(plane, &ssd130x_state->base); +} + +static struct drm_plane_state *ssd130x_primary_plane_duplicate_state(struct drm_plane *plane) +{ + struct ssd130x_plane_state *old_ssd130x_state; + struct ssd130x_plane_state *ssd130x_state; + + if (WARN_ON(!plane->state)) + return NULL; + + old_ssd130x_state = to_ssd130x_plane_state(plane->state); + ssd130x_state = kmemdup(old_ssd130x_state, sizeof(*ssd130x_state), GFP_KERNEL); + if (!ssd130x_state) + return NULL; + + /* The buffers are not duplicated and are allocated in .atomic_check */ + ssd130x_state->buffer = NULL; + ssd130x_state->data_array = NULL; + + __drm_atomic_helper_plane_duplicate_state(plane, &ssd130x_state->base); + + return &ssd130x_state->base; +} + +static void ssd130x_primary_plane_destroy_state(struct drm_plane *plane, + struct drm_plane_state *state) +{ + struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(state); + + ssd130x_buf_free(ssd130x_state); + + __drm_atomic_helper_plane_destroy_state(&ssd130x_state->base); + + kfree(ssd130x_state); +} + static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = { DRM_GEM_SHADOW_PLANE_HELPER_FUNCS, - .atomic_check = drm_plane_helper_atomic_check, + .atomic_check = ssd130x_primary_plane_helper_atomic_check, .atomic_update = ssd130x_primary_plane_helper_atomic_update, .atomic_disable = ssd130x_primary_plane_helper_atomic_disable, }; @@ -665,6 +750,9 @@ static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = static const struct drm_plane_funcs ssd130x_primary_plane_funcs = { .update_plane = drm_atomic_helper_update_plane, .disable_plane = drm_atomic_helper_disable_plane, + .reset = ssd130x_primary_plane_reset, + .atomic_duplicate_state = ssd130x_primary_plane_duplicate_state, + .atomic_destroy_state = ssd130x_primary_plane_destroy_state, .destroy = drm_plane_cleanup, DRM_GEM_SHADOW_PLANE_FUNCS, }; @@ -719,10 +807,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); @@ -744,8 +828,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.h b/drivers/gpu/drm/solomon/ssd130x.h index 161588b1cc4d..87968b3e7fb8 100644 --- a/drivers/gpu/drm/solomon/ssd130x.h +++ b/drivers/gpu/drm/solomon/ssd130x.h @@ -89,9 +89,6 @@ struct ssd130x_device { u8 col_end; u8 page_start; u8 page_end; - - u8 *buffer; - u8 *data_array; }; extern const struct ssd130x_deviceinfo ssd130x_variants[];
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 having custom helpers to allocate, duplicate and destroy the plane state, that will take care of allocating and freeing these buffers. Instead of doing it in the encoder atomic enabled and disabled callbacks, since allocations must not be done in an .atomic_enable handler. Because drivers are not allowed to fail after drm_atomic_helper_swap_state() has been called and the new atomic state is stored into the current sw state. Fixes: 49d7d581ceaf ("drm/ssd130x: Don't allocate buffers on each plane update") Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> Suggested-by: Maxime Ripard <mripard@kernel.org> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> --- Changes in v4: - Move buffers allocation/free to plane .reset/.destroy helpers (Maxime Ripard). 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 | 134 ++++++++++++++++++++++++------ drivers/gpu/drm/solomon/ssd130x.h | 3 - 2 files changed, 108 insertions(+), 29 deletions(-)