Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxxxxx> Will drop your Reported-by tag too then. >> Suggested-by: Maxime Ripard <mripard@xxxxxxxxxx> >> Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx> > > 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@xxxxxxxxxxxxxx> > 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. -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
