[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87fs5sgdfh.fsf@minerva.mail-host-address-is-not-set>
Date: Thu, 13 Jul 2023 15:15:46 +0200
From: Javier Martinez Canillas <javierm@...hat.com>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
Cc: linux-kernel@...r.kernel.org,
Thomas Zimmermann <tzimmermann@...e.de>,
Maxime Ripard <mripard@...nel.org>,
Daniel Vetter <daniel@...ll.ch>,
David Airlie <airlied@...il.com>,
dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v2 4/5] drm/ssd130x: Don't allocate buffers on each
plane update
Geert Uytterhoeven <geert@...ux-m68k.org> writes:
Hello Geert,
> Hi Javier,
>
> On Fri, Jun 9, 2023 at 7:09 PM Javier Martinez Canillas
> <javierm@...hat.com> 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@...hat.com>
>> Reviewed-by: Thomas Zimmermann <tzimmermann@...e.de>
>> ---
>>
>> (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
Powered by blists - more mailing lists