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 theDouble '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
