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

[Maxime Ripard]

Hi,

On Wed, Jul 26, 2023 at 01:52:37PM +0200, Geert Uytterhoeven wrote:
> 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.

In KMS in general, no. For that particular case, yes.

And about the framebuffer size == full screen size, there's multiple
sizes involved. I guess what you call full screen size is the CRTC size.

You can't make the assumption in KMS that CRTC size == (primary) plane
size == framebuffer size.

If you're using scaling for example, you will have a framebuffer size
smaller or larger than it plane size. If you're using cropping, then the
plane size or framebuffer size will be different from the CRTC size.
Some ways to work around overscan is also to have a smaller plane size
than the CRTC size.

You don't have to have the CRTC size == primary plane size, and then you
don't have to have plane size == framebuffer size.

you can't really make that assumption in the general case either:
scaling or cropping will have a different framebuffer size than the CRTC
primary plane size (which doesn't have to be "full screen" either).

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

Yeah, probably.

> > > > @@ -159,23 +173,23 @@ static int ssd130x_buf_alloc(struct ssd130x_device *ssd130x)
> > > >
> > > >         pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
> > > >
> > > > -       ssd130x->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
> > > > -       if (!ssd130x->buffer)
> > > > +       ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
> 
> Based on full screen width and height.
> 
> > > > +       if (!ssd130x_state->buffer)
> > > >                 return -ENOMEM;
> > > >
> > > > -       ssd130x->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL);
> > > > -       if (!ssd130x->data_array) {
> > > > -               kfree(ssd130x->buffer);
> > > > +       ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL);
> 
> Based on full screen width and height (hardware page size).
> 
> > > > +       if (!ssd130x_state->data_array) {
> > > > +               kfree(ssd130x_state->buffer);
> > >
> > > Should ssd130x_state->buffer be reset to NULL here?
> > > I.e. if .atomic_check() fails, will .atomic_destroy_state() be called,
> > > leading to a double free?
> >
> > That's a good question.
> >
> > I never really thought of that, but yeah, AFAIK atomic_destroy_state()
> > will be called when atomic_check() fails.
> >
> > Which means that it's probably broken in a lot of drivers.
> >
> > 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)?!?

Yep, that's exactly why we have drm_dev_enter/drm_dev_exit :)

Not a lot of drivers are using it but all should. vc4 does exactly what
you are describing for example.

Maxime

Attachment: signature.asc
Description: PGP signature