Re: [PATCH v4] drm/ssd130x: Allocate buffers in the plane's .atomic_check callback

[Javier Martinez Canillas]

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