Hi,
On 09/18/2014 01:41 AM, Daniel Drake wrote:
On Wed, Sep 17, 2014 at 7:45 AM, Daniel Vetter daniel@ffwll.ch wrote:
I think fb refcounting in exynos is just plain busted. If you look at other drivers the only place the refcount framebuffers or backing storage objects is for pageflips to make sure the memory doesn't go away while the hw is still scanning out the old framebuffer. If you refcount anywhere else you either do something really crazy or your driver is broken.
With my patch actually the behaviour is much more similar to omapdrm,
Your patch will occur fb reference count problem when setplane.
which also doesn't quite match your description of "other drivers". See omap_plane.c.
There is a fb reference taken for "pinning" in update_pin() which presumably is what you describe - avoid destroying the fb while it is being scanned out. (Maybe exynos should have something equivalent too, but thats a separate issue)
However there is *another* fb reference taken in omap_plane_mode_set(). And my patch is modelled to do the same in exynos-drm.
I believe this is necessary under the current model. At least, when drm_mode_rmfb() is running for the last user of the active framebuffer, it expects to drop 3 references from the framebuffer before dropping the 4th causes the object to be destroyed, as follows:
- drm_mode_rmfb explicitly drops a reference - it calls
__drm_framebuffer_unregister which then calls __drm_framebuffer_unreference /* Mark fb as reaped, we still have a ref from fpriv->fbs. */ __drm_framebuffer_unregister(dev, fb);
- drm_mode_rmfb then calls drm_framebuffer_remove, which calls
drm_mode_set_config_internal() in order to turn off the CRTC, dropping another reference in the process. if (tmp->old_fb) drm_framebuffer_unreference(tmp->old_fb);
- drm_framebuffer_remove calls drm_plane_force_disable() which drops
another reference: /* disconnect the plane from the fb and crtc: */ __drm_framebuffer_unreference(old_fb);
This call is new path, before universal planes merged, private plane of exynos crtc wasn't included in dev->mode_config.plane_list because private plane wasn't exposed to userspace so this path wasn't called.
- drm_framebuffer drops the final reference itself, to cause freeing
of the object: drm_framebuffer_unreference(fb);
So ordinarily, after a fb is created by drm core (with refcnt at 1), there would have to be 3 references added to it by the time it is the primary fb so that when we do rmfb, it has a refcnt of 4, and gets freed correctly. (The second bug I was seeing with pageflips was that refcnt was 3, which means that the final reference was dropped in (3) above, but __drm_framebuffer_unreference doesn't like that at all - it calls drm_framebuffer_free_bug)
Not being overly familiar with DRM internals I tried to go backwards to find out where these 3 references would be created during normal operation. 2 are clear:
drm_framebuffer_init() explicitly grabs one: /* Grab the idr reference. */ drm_framebuffer_reference(fb)
drm_mode_set_config_internal() takes one: if (tmp->primary->fb) drm_framebuffer_reference(tmp->primary->fb);
Where should the 3rd one be created? I don't know, but looking at previous exynos commit 25c8b5c304 and omapdrm, I assumed that the drm driver should take one, both on crtc mode set and crtc page flip.
So Andrzej added fb reference count increasing in crtc modeset path, but i think we can take away this workaround if remove private plane for exynos crtc.
Thanks.
However, I'll be happy if universal planes means the driver does not have to care about this any more. Andrej, please go ahead if you are interested, I'll be happy to test your results.
universal planes will fix up the mess with 2 drm plane objects (primary plane + exonys internal primary). So should help to untangle this not, but it will not magically fix the refcounting bugs itself.
So even when we move to universal planes (fixing 1 of the issues), its good that we're having this refcount discussion (which we need to understand to confidently solve the 2nd issue). Thanks for your input!
Daniel