Hi, On 09/18/2014 01:41 AM, Daniel Drake wrote: > On Wed, Sep 17, 2014 at 7:45 AM, Daniel Vetter <daniel@xxxxxxxx> 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: > > 1. 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); > > 2. 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); > > 3. 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. > > 4. 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: > > 1. drm_framebuffer_init() explicitly grabs one: > /* Grab the idr reference. */ > drm_framebuffer_reference(fb) > > 2. 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |