[PATCH] drm: Simplify fb refcounting rules around ->update_plane

[Daniel Vetter]

On Wed, Apr 23, 2014 at 3:08 AM, Matt Roper <matthew.d.roper at intel.com> 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