From 0fe27f063fcef920a0be93ad5e89d9c8ef5c5858 Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Wed, 23 Apr 2014 17:34:06 +0200 Subject: drm: Simplify fb refcounting rules around ->update_plane MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The introduction of primary planes has apparently caused a bit of fb refcounting fun for people. That makes it a good time to clean up the arcane rules and slight differences between ->update_plane and ->set_config. The new rules are: - The core holds a reference for both the new and the old fb (if they're non-NULL of course) while calling into the driver through either ->update_plane or ->set_config. - Drivers may not clobber plane->fb if their callback fails. If they do that, they need to store a pointer to the old fb in it again. When calling into the driver plane->fb still points at the current (old) framebuffer. - The core will update the plane->fb pointer on success. Drivers can do that themselves too, but aren't required to any more for the primary plane. - The core will update fb refcounts for the plane->fb pointer, presuming the drivers hold up their end of the bargain. v2: Remove now unused tmpfb (Thierry) v3: Drop broken changes from drm_mode_setplane (Ville). Also polish the commit message a bit. v4: Also fix up the handling of ->disable_plane in drm_plane_force_disable. The issue was that we didn't save plane->fb over the ->disable_plane call. Just paranoia, nothing relies on this. v5: Keep still useful comments about directly calling ->set_config, which I should have done for v4 already. Requested by Matt. Cc: Thierry Reding Cc: Ville Syrjälä Cc: Matt Roper Reviewed-by: Matt Roper Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_crtc.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) (limited to 'drivers/gpu/drm/drm_crtc.c') diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index d8b7099abec..f6633cb927b 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1145,16 +1145,17 @@ EXPORT_SYMBOL(drm_plane_cleanup); */ void drm_plane_force_disable(struct drm_plane *plane) { + struct drm_framebuffer *old_fb = plane->fb; int ret; - if (!plane->fb) + if (!old_fb) return; ret = plane->funcs->disable_plane(plane); if (ret) DRM_ERROR("failed to disable plane with busy fb\n"); /* disconnect the plane from the fb and crtc: */ - __drm_framebuffer_unreference(plane->fb); + __drm_framebuffer_unreference(old_fb); plane->fb = NULL; plane->crtc = NULL; } @@ -2187,16 +2188,18 @@ int drm_mode_setplane(struct drm_device *dev, void *data, } drm_modeset_lock_all(dev); + old_fb = plane->fb; ret = plane->funcs->update_plane(plane, crtc, fb, plane_req->crtc_x, plane_req->crtc_y, plane_req->crtc_w, plane_req->crtc_h, plane_req->src_x, plane_req->src_y, plane_req->src_w, plane_req->src_h); if (!ret) { - old_fb = plane->fb; plane->crtc = crtc; plane->fb = fb; fb = NULL; + } else { + old_fb = NULL; } drm_modeset_unlock_all(dev); @@ -2239,9 +2242,7 @@ int drm_mode_set_config_internal(struct drm_mode_set *set) ret = crtc->funcs->set_config(set); if (ret == 0) { crtc->primary->crtc = crtc; - - /* crtc->fb must be updated by ->set_config, enforces this. */ - WARN_ON(fb != crtc->primary->fb); + crtc->primary->fb = fb; } list_for_each_entry(tmp, &crtc->dev->mode_config.crtc_list, head) { -- cgit v1.2.3-70-g09d2 From 731cce487ab0485c6607e44a0759d92bef1c144e Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Wed, 23 Apr 2014 10:24:11 +0200 Subject: drm: Handle ->disable_plane failures correctly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ->disable_plane hook always had a return value, but only since the introduction of primary planes was there any implementation that actually failed. So handle such failures correctly. Note that drm_plane_force_disable is special: In the modeset cleanup case we first disable all crtc, so primary planes should all be freed already. And in the fb helper we only reset non-primary planes. Still better be paranoid and add an early return. I don't see how this could happen, but it might fix the fb refcount underrun Thierry is seeing. Matt Roper spotted this issue. Cc: Thierry Reding Cc: Ville Syrjälä Cc: Matt Roper Reviewed-by: Matt Roper Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_crtc.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) (limited to 'drivers/gpu/drm/drm_crtc.c') diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index f6633cb927b..461d19bd14e 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1152,8 +1152,10 @@ void drm_plane_force_disable(struct drm_plane *plane) return; ret = plane->funcs->disable_plane(plane); - if (ret) + if (ret) { DRM_ERROR("failed to disable plane with busy fb\n"); + return; + } /* disconnect the plane from the fb and crtc: */ __drm_framebuffer_unreference(old_fb); plane->fb = NULL; @@ -2117,9 +2119,13 @@ int drm_mode_setplane(struct drm_device *dev, void *data, if (!plane_req->fb_id) { drm_modeset_lock_all(dev); old_fb = plane->fb; - plane->funcs->disable_plane(plane); - plane->crtc = NULL; - plane->fb = NULL; + ret = plane->funcs->disable_plane(plane); + if (!ret) { + plane->crtc = NULL; + plane->fb = NULL; + } else { + old_fb = NULL; + } drm_modeset_unlock_all(dev); goto out; } -- cgit v1.2.3-70-g09d2