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

[Javier Martinez Canillas]

Maxime Ripard <mripard@xxxxxxxxxx> writes:

Hello Maxime,

> 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:

[...]

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

Yes, this particular driver is using the drm_plane_helper_atomic_check()
as the primary plane .atomic_check and this function helper calls to:

drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
						   DRM_PLANE_NO_SCALING,
						   DRM_PLANE_NO_SCALING,
						   false, false);

so it does not support scaling nor positioning.

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

Right, that could be fixed as another patch if anything to make it more
reable since it won't have any functional change.

>> > > > @@ -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 :)
>

Thanks. As a follow-up I can also do that in another patch.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat