On Wed, Apr 23, 2014 at 3:08 AM, Matt Roper <matthew.d.roper@xxxxxxxxx> wrote: > On Tue, Apr 22, 2014 at 04:19:45PM +0200, Daniel Vetter wrote: >> The introduction of primary planes has apparently caused a bit of fb >> refcounting fun for people. That makes it a good time to clean up the >> arcane rules and slight differences between ->update_plane and >> ->set_config. The new rules are: >> >> - The core holds a reference for both the new and the old fb (if >> they're non-NULL of course) while calling into the driver through >> either ->update_plane or ->set_config. >> >> - Drivers may not clobber plane->fb if their callback fails. If they >> do that, they need to store a pointer to the old fb in it again. >> When calling into the driver plane->fb still points at the current >> (old) framebuffer. >> >> - The core will update the plane->fb pointer on success. Drivers can >> do that themselves too, but aren't required to any more for the >> primary plane. >> >> - The core will update fb refcounts for the plane->fb pointer, >> presuming the drivers hold up their end of the bargain. >> >> v2: Remove now unused tmpfb (Thierry) >> >> v3: Drop broken changes from drm_mode_setplane (Ville). Also polish >> the commit message a bit. > > It looks like we might have some problems around setplane with fbid=0. > It looks like we're assuming that disabling a plane always succeeds > (which is no longer true for helper-based primary planes --- they just > return -EINVAL on disable now), so we wind up setting old_fb to the > currently scanned out framebuffer and then unref it at the end of the > function if I'm reading things correctly. We also clobber the > plane->crtc and plane->fb pointers too when this happens. > > I think the real problem here was introduced on b6ccd7b9 and I gave you > an r-b tag on that email, so that's my bad for not catching it before. > :-( Hm, that's indeed a bit fishy. Otoh I don't understand how this blows up with existing userspace since we should only hit this if userspace does an explicit disable call on the primary plane through the ioctl. It's a bug for sure though. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
