Re: [PATCH v2 4/5] drm/ssd130x: Don't allocate buffers on each plane update

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> writes:

Hello Geert,

> Hi Javier,
>
> On Fri, Jun 9, 2023 at 7:09 PM Javier Martinez Canillas
> <javierm@xxxxxxxxxx> wrote:
>> The resolutions for these panels are fixed and defined in the Device Tree,
>> so there's no point to allocate the buffers on each plane update and that
>> can just be done once.
>>
>> Let's do the allocation and free on the encoder enable and disable helpers
>> since that's where others initialization and teardown operations are done.
>>
>> Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
>> Reviewed-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
>> ---
>>
>> (no changes since v1)
>
> Thanks for your patch, which is now commit 49d7d581ceaf4cf8
> ("drm/ssd130x: Don't allocate buffers on each plane update") in
> drm-misc/for-linux-next.
>
>> --- a/drivers/gpu/drm/solomon/ssd130x.c
>> +++ b/drivers/gpu/drm/solomon/ssd130x.c
>> @@ -701,14 +709,22 @@ static void ssd130x_encoder_helper_atomic_enable(struct drm_encoder *encoder,
>>                 return;
>>
>>         ret = ssd130x_init(ssd130x);
>> -       if (ret) {
>> -               ssd130x_power_off(ssd130x);
>> -               return;
>> -       }
>> +       if (ret)
>> +               goto power_off;
>> +
>> +       ret = ssd130x_buf_alloc(ssd130x);
>
> This appears to be too late, causing a NULL pointer dereference:
>

Thanks for reporting this issue.

> [   59.302761] [<c0303d90>] ssd130x_update_rect.isra.0+0x13c/0x340
> [   59.304231] [<c0304200>]
> ssd130x_primary_plane_helper_atomic_update+0x26c/0x284
> [   59.305716] [<c02f8d54>] drm_atomic_helper_commit_planes+0xfc/0x27c
>

I wonder how this could be too late. I thought that the encoder
.atomic_enable callback would be called before any plane .atomic_update.

> Bailing out from ssd130x_update_rect() when data_array is still NULL
> fixes that.
>

Maybe we can add that with a drm_WARN() ? I still want to understand how
a plane update can happen before an encoder enable.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat