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

[Thomas Zimmermann]

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 at suse.de> 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 at linux-m68k.org>
>>> Signed-off-by: Javier Martinez Canillas <javierm at redhat.com>
> 
> 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)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230830/3c08df38/attachment.sig>