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
