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

[Geert Uytterhoeven]

Hi Javier,

On Wed, Jul 26, 2023 at 2:22 PM Javier Martinez Canillas
<javierm@xxxxxxxxxx> wrote:
> Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> writes:
> > On Wed, Jul 26, 2023 at 12:00 PM Maxime Ripard <mripard@xxxxxxxxxx> wrote:
> >> On Tue, Jul 25, 2023 at 09:21:52PM +0200, Geert Uytterhoeven wrote:
> >> > > --- a/drivers/gpu/drm/solomon/ssd130x.c
> >> > > +++ b/drivers/gpu/drm/solomon/ssd130x.c
> >> > > @@ -141,12 +141,26 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = {
> >> > >  };
> >> > >  EXPORT_SYMBOL_NS_GPL(ssd130x_variants, DRM_SSD130X);
> >> > >
> >> > > +struct ssd130x_plane_state {
> >> > > +       struct drm_plane_state base;
> >> > > +       /* Intermediate buffer to convert pixels from XRGB8888 to R1 */
> >> > > +       u8 *buffer;
> >> > > +       /* Buffer that contains R1 pixels to be written to the panel */
> >> > > +       u8 *data_array;
> >> >
> >> > The second buffer actually contains pixels in hardware format.
> >> > For now that is a transposed buffer of R1 pixels, but that will change
> >> > when you will add support for greyscale displays.
> >> >
> >> > So I'd write "hardware format" instead of R1 for both.
> >> >
> >> > BTW, I still think data_array should be allocated during probing,
> >> > as it is related to the hardware, not to a plane.
> >>
> >> I somewhat disagree.
> >>
> >> If I understood right during our discussion with Javier, the buffer size
> >> derives from the mode size (height and width).
> >>
> >> In KMS, the mode is tied to the KMS state, and thus you can expect the
> >> mode to change every state commit. So the more logical thing to do is to
> >> tie the buffer size (and thus the buffer pointer) to the state since
> >> it's only valid for that particular state for all we know.
> >>
> >> Of course, our case is allows use to simplify things since it's a fixed
> >> mode, but one of Javier's goal with this driver was to provide a good
> >> example we can refer people to, so I think it's worth keeping.
> >
> > The second buffer (containing the hardware format) has a size that
> > depends on the full screen size, not the current mode (I believe that's
> > also the size of the frame buffer backing the plane?).  So its size is
> > fixed.
> >
>
> Yes, is fixed. But Maxime's point is that this is a characteristic of this
> particular device and even when the display resolution can't be changed,
> the correct thing to do is to keep all state related to the mode (even the
> buffer used to store the hardware pixels that are written to the display)
>
> > Given the allocations are now done based on plane state, I think the
> > first buffer should be sized according to the frame buffer backing
> > the plane? Currently it uses the full screen size, too (cfr. below).
> >
>
> But can the mode even be changed if ssd130x_connector_helper_get_modes()
> just adds a single display mode with mode->hdisplay == ssd130x->width and
> mode->vdisplay == ssd130x->height.

No, the mode cannot be changed.

At first, I thought you could still create a smaller frame buffer,
and attach that to the (single, thus primary) plane, but it turns out
I was wrong[*], so you can ignore that.

[*] ssd130x_primary_plane_helper_atomic_check() calls
    drm_plane_helper_atomic_check(), which calls
    drm_atomic_helper_check_plane_state() with can_position = false.
    As the position of planes is actually a software thing on ssd130x,
    positioning support could be added later, though...

> >> 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().

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