Possible fb ref count issue with drm_plane_force_disable()

Commit Message

Tomi Valkeinen April 15, 2014, 1:30 p.m. UTC

On 15/04/14 15:24, Rob Clark wrote:

> probably split out omap_plane_mode_set_internal(), call that directly
> from update_plane() for plane operations.  And then do the refcnt
> dance in the new omap_plane_mode_set() which calls _internal()..

We don't actually need that. This did the trick:


With quick tests, works fine so far.

Still I have to say that the ref counting doesn't feel quite right (or
I'm missing something).

As far as I understand, the drm_mode_setplane() gets a ref to the fb,
and stores it in plane->fb. Why do we take a new ref in
omap_plane_update (or with the patch above, in omap_plane_mode_set), and
also store it in plane->fb there? Feels like the same task is done in
two places.

It looks to me like drm_mode_setplane() doesn't really presume that the
update_plane would set plane->fb or plane->crtc, as it does that itself
(and only if update_plane has succeeded).

 Tomi

Comments

Daniel Vetter April 15, 2014, 10:01 p.m. UTC | #1

On Tue, Apr 15, 2014 at 3:30 PM, Tomi Valkeinen <[e-mail]> wrote:
>
> It looks to me like drm_mode_setplane() doesn't really presume that the
> update_plane would set plane->fb or plane->crtc, as it does that itself
> (and only if update_plane has succeeded).

Yeah that's a bit of historical hilarity. ->set_config should update
crtc->primary->fb, but when calling ->update_plane the drm core will
do this for you. In both cases the plane->fb is the old fb when your
driver hook gets called.

I guess now that we have primary planes we should unify these
semantics a bit since they're fairly pointless.
-Daniel

Patch

diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c
b/drivers/gpu/drm/omapdrm/omap_plane.c
index df1725247cca..3cf31ee59aac 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -225,6 +225,11 @@  int omap_plane_mode_set(struct drm_plane *plane,
                omap_plane->apply_done_cb.arg = arg;
        }

+       if (plane->fb)
+               drm_framebuffer_unreference(plane->fb);
+
+       drm_framebuffer_reference(fb);
+
        plane->fb = fb;
        plane->crtc = crtc;

@@ -241,11 +246,6 @@  static int omap_plane_update(struct drm_plane *plane,
        struct omap_plane *omap_plane = to_omap_plane(plane);
        omap_plane->enabled = true;

-       if (plane->fb)
-               drm_framebuffer_unreference(plane->fb);
-
-       drm_framebuffer_reference(fb);
-
        /* omap_plane_mode_set() takes adjusted src */
        switch (omap_plane->win.rotation & 0xf) {
        case BIT(DRM_ROTATE_90):