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