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