Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Javier Martinez Canillas <javierm@redhat.com>
To:     Thomas Zimmermann <tzimmermann@suse.de>,
        linux-kernel@vger.kernel.org
Cc:     Geert Uytterhoeven <geert@linux-m68k.org>,
        dri-devel@lists.freedesktop.org, Maxime Ripard <mripard@kernel.org>
Subject: Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's
 .atomic_check() callback
In-Reply-To: <a6eca431-7464-09da-333d-a40318422077@suse.de>
References: <20230830062546.720679-1-javierm@redhat.com>
 <6654778d-1f40-1775-c32c-ebf9728bc9a9@suse.de>
 <87ledqbah2.fsf@minerva.mail-host-address-is-not-set>
 <a6eca431-7464-09da-333d-a40318422077@suse.de>
Date:   Fri, 01 Sep 2023 13:50:20 +0200
Message-ID: <87cyz25czn.fsf@minerva.mail-host-address-is-not-set>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Thomas Zimmermann <tzimmermann@suse.de> writes:

Hello Thomas,

> Hi
>
> Am 01.09.23 um 09:48 schrieb Javier Martinez Canillas:
>> Thomas Zimmermann <tzimmermann@suse.de> writes:
>> 
>>> Hi Javier,
>>>
>>> another idea about this patch: why not just keep the allocation in the
>>> plane's atomic check, but store the temporary buffers in a plane struct.
>>> You'd only grow the arrays length in atomic_check and later fetch the
>>> pointers in atomic_update. It needs some locking, but nothing complicated.
>>>
>> 
>> Yes, that would work too. Another option is to just move the buffers to
>> struct ssd130x_device as it was before commit 45b58669e532 ("drm/ssd130x:
>
> Adding something like a struct ssd130x_plane that holds the temporary 
> memory has the advantage of making a clear connection between the memory 
> and the plane. If nothing else, to the next programmer reading the code.
>

Ok, I'm confused now. The current version of the driver already has a
struct ssd130x_plane_state that stores the buffers:

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/solomon/ssd130x.c#n144

But what you are saying is that instead of setting those pointers to NULL
in the ssd130x_primary_plane_duplicate_state() function, it can just be
allocated once and the pointers copied when duplicating the new state?

So you want me to add some locking when accesing those since will be
shared between the old and new state?

In that case another option is to just kref the buffers, I think that
should be enough?

>> Allocate buffer in the plane's .atomic_check() callback") but just make
>> them fixed arrays with the size of the biggest format.
>
> What is the size of the biggest format? I haven't read the driver code, 
> but a shadow plane can be up to 4096 pixels wide. It's 16 KiB for 
> XRGB888. Not too much, but not nothing either.
>

The shadow plane yes, but I was talking about the buffers in:

struct ssd130x_plane_state {
	struct drm_shadow_plane_state base;
	/* Intermediate buffer to convert pixels from XRGB8888 to HW format */
	u8 *buffer;
	/* Buffer to store pixels in HW format and written to the panel */
	u8 *data_array;
};

These are not the size of the shadow buffer but the of the displayed area,
that depends on the panel fixed resolution (defined in the Device Tree).

The biggest resolution for ssd130x panels is 132x64 (SSD1305 and SH1106),
so that means the biggest buffer will be 132 * 64 * R1 pitch = 1056 bytes.

> To reduce allocation and/or locking overhead, you could try to update 
> the pointers in the plane struct with RCU semantics. Plane updates would 
> use whatever pointer they saw, while the plane's atomic_check could grow 
> the memory buffers as necessary.
>

I'm still unsure the added complexity is worth it. As mentioned, the fbdev
driver also allocats the buffers on each display update and at least the
intermediate .buffer that stores the XRGB8888 -> R1 conversion is tied to
the plane, the .data_array that sends the actual data to the controller
can be argued to be tied to the CRTC.

But given that a format change doesn't trigger a CRTC mode set (as Maxime
pointed out to me) then at least the .buffer allocation can't be done in
the CRTC .atomic_check() handler.

I could move the .data_array allocatoin there or even do it at probe time,
but since the .buffer allocation would be done in the plane
.atomic_check() anyways, I would rather keep as is and have both in the
same function.

> Best regards
> Thomas
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat