Re: [PATCH v4] drm/ssd130x: Allocate buffers in the plane's .atomic_check callback

[Geert Uytterhoeven]

Hi Maxime,

On Wed, Jul 26, 2023 at 3:54 PM Maxime Ripard <mripard@xxxxxxxxxx> wrote:
> On Wed, Jul 26, 2023 at 02:33:06PM +0200, Geert Uytterhoeven wrote:
> > > >> Also, Javier pointed me to a discussion you had on IRC about using devm
> > > >> allocation here. We can't really do that. KMS devices are only freed
> > > >> once the last userspace application closes its fd to the device file, so
> > > >> you have an unbounded window during which the driver is still callable
> > > >> by userspace (and thus can still trigger an atomic commit) but the
> > > >> buffer would have been freed for a while.
> > > >
> > > > It should still be safe for (at least) the data_array buffer. That
> > > > buffer is only used to store pixels in hardware format, and immediately
> > > > send them to the hardware.  If this can be called that late, it will
> > > > fail horribly, as you can no longer talk to the hardware at that point
> > > > (as ssd130x is an i2c driver, it might actually work; but a DRM driver
> > > >  that calls devm_platform_ioremap_resource() will crash when writing
> > > >  to its MMIO registers)?!?
> > >
> > > At the very least the SPI driver will fail since the GPIO that is used to
> > > toggle the D/C pin is allocated with devm_gpiod_get_optional(), but also
> > > the regulator, backlight device, etc.
> > >
> > > But in any case, as mentioned it is only relevant if the data_array buffer
> > > is allocated at probe time, and from Maxime's explanation is more correct
> > > to do it in the .atomic_check handler.
> >
> > You need (at least) data_array for clear_screen, too, which is called
> > from .atomic_disable().
>
> I'm not sure I get what your concern is?
>
> Even if we entirely disable the plane, the state will not have been
> destroyed yet so you still have at least access to the data_array from
> the old state.

Currently, clearing the screen is done from the plane's .atomic_disable()
callback, so the plane's state should be fine.

But IIUIC, DRM allows the user to enable/disable the display without
creating any plane first, so clearing should be handled in the CRTC's
.atomic_disable() callback instead? Then, would we still have access
to valid plane state?

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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