linux-kernel - Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMuHMdVpC6YajGOf7XzkFfs618-WyeMNdpmpDjGrSpG7egSW6Q@mail.gmail.com>
Date:   Wed, 30 Aug 2023 09:40:32 +0200
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Thomas Zimmermann <tzimmermann@...e.de>
Cc:     Javier Martinez Canillas <javierm@...hat.com>,
        linux-kernel@...r.kernel.org, Maxime Ripard <mripard@...nel.org>,
        dri-devel@...ts.freedesktop.org
Subject: Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's
 .atomic_check() callback

Hi Thomas,

On Wed, Aug 30, 2023 at 9:08 AM Thomas Zimmermann <tzimmermann@...e.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'
>
> > 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@...ux-m68k.org>
> > Signed-off-by: Javier Martinez Canillas <javierm@...hat.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.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds