Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

From: Thomas Zimmermann
Date: Wed Aug 30 2023 - 14:38:16 EST


Hi Geert

Am 30.08.23 um 09:40 schrieb Geert Uytterhoeven:
Hi Thomas,

On Wed, Aug 30, 2023 at 9:08 AM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
Am 30.08.23 um 08:25 schrieb Javier Martinez Canillas:
The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
.atomic_check() callback") moved the allocation of the intermediate and
HW buffers from the encoder's .atomic_enable callback to primary plane's
.atomic_check callback.

This was suggested by Maxime Ripard because drivers aren't allowed to fail
after drm_atomic_helper_swap_state() has been called, and the encoder's
.atomic_enable happens after the new atomic state has been swapped.

But that change caused a performance regression in very slow platforms,
since now the allocation happens for every plane's atomic state commit.
For example, Geert Uytterhoeven reports that is the case on a VexRiscV
softcore (RISC-V CPU implementation on an FPGA).

To prevent that, move the move the buffers' allocation and free to the

Double 'move the'


And maybe buffer's rather than buffers'

Scratch that remark.


CRTC's .atomic_check and .atomic_destroy_state callbacks, so that only
happens on a modeset. Since the intermediate buffer is only needed when
not using the controller native format (R1), doing the buffer allocation
at that CRTC's .atomic_check time would be enough.

Fixes: 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's .atomic_check() callback")
Suggested-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>

Javier: thanks for your patch!

Besides the pointers, the CRTC state can also store the primary plane
format, which you update from the plane's atomic check. By doing so, you
wont need to refer to the plane state from the CRTC's atomic_check. The
plane's atomic_check runs before the CRTC's atomic_check. [1]

I haven't tested Javier's patch yet, but does this mean that his patch
won't help?

The problem I saw was that these buffers were allocated and freed
over and over again on each flash of the cursor of the text console
on top of the emulated frame buffer device.

Javier's current patch should resolve this problem. The temporary buffers are now only allocated on display-mode/format changes, but not on each single screen update. My review concerns only the implementation.

Best regards
Thomas


Gr{oetje,eeting}s,

Geert


--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Attachment: OpenPGP_signature
Description: OpenPGP digital signature