[PATCH] drm/exynos: fix plane-framebuffer linkage

[Daniel Drake]

On Wed, Sep 17, 2014 at 7:45 AM, Daniel Vetter <daniel at 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,
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);

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.

>> 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