diff options
author | Daniel Vetter <daniel.vetter@ffwll.ch> | 2014-11-04 21:57:27 (GMT) |
---|---|---|
committer | Daniel Vetter <daniel.vetter@ffwll.ch> | 2014-11-06 20:08:37 (GMT) |
commit | 321ebf04dc7ab5c54d658f93db0ffe35277664ab (patch) | |
tree | 91fcbe2b49f701015e1fbbc8fb98800b1a297fa1 | |
parent | 3150c7d0c686ffee9e414ccce705c34ebcbf4e30 (diff) | |
download | linux-321ebf04dc7ab5c54d658f93db0ffe35277664ab.tar.xz |
drm/atomic: Refcounting for plane_state->fb
So my original plan was that the drm core refcounts framebuffers like
with the legacy ioctls. But that doesn't work for a bunch of reasons:
- State objects might live longer than until the next fb change
happens for a plane. For example delayed cleanup work only happens
_after_ the pageflip ioctl has completed. So this definitely doesn't
work without the plane state holding its own references.
- The other issue is transition from legacy to atomic implementations,
where the driver works under a mix of both worlds. Which means
legacy paths might not properly update the ->fb pointer under
plane->state->fb. Which is a bit a problem when then someone comes
around and _does_ try to clean it up when it's long gone.
The second issue is just a bit a transition bug, since drivers should
update plane->state->fb in all the paths that aren't converted yet.
But a bit more robustness for the transition can't hurt - we pull
similar tricks with cleaning up the old fb in the transitional helpers
already.
The pattern for drivers that transition is
if (plane->state)
drm_atomic_set_fb_for_plane(plane->state, plane->fb);
inserted after the fb update has logically completed at the end of
->set_config (or ->set_base/mode_set if using the crtc helpers),
->page_flip, ->update_plane or any other entry point which updates
plane->fb.
v2: Update kerneldoc - copypasta fail.
v3: Fix spelling in the commit message (Sean).
Reviewed-by: Sean Paul <seanpaul@chromium.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
-rw-r--r-- | drivers/gpu/drm/drm_atomic.c | 28 | ||||
-rw-r--r-- | drivers/gpu/drm/drm_atomic_helper.c | 25 | ||||
-rw-r--r-- | drivers/gpu/drm/drm_crtc_helper.c | 7 | ||||
-rw-r--r-- | drivers/gpu/drm/drm_plane_helper.c | 14 | ||||
-rw-r--r-- | include/drm/drm_atomic.h | 2 |
5 files changed, 60 insertions, 16 deletions
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index ad15a88..ed991ba 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -368,6 +368,34 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state, EXPORT_SYMBOL(drm_atomic_set_crtc_for_plane); /** + * drm_atomic_set_fb_for_plane - set crtc for plane + * @plane_state: atomic state object for the plane + * @fb: fb to use for the plane + * + * Changing the assigned framebuffer for a plane requires us to grab a reference + * to the new fb and drop the reference to the old fb, if there is one. This + * function takes care of all these details besides updating the pointer in the + * state object itself. + */ +void +drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state, + struct drm_framebuffer *fb) +{ + if (plane_state->fb) + drm_framebuffer_unreference(plane_state->fb); + if (fb) + drm_framebuffer_reference(fb); + plane_state->fb = fb; + + if (fb) + DRM_DEBUG_KMS("Set [FB:%d] for plane state %p\n", + fb->base.id, plane_state); + else + DRM_DEBUG_KMS("Set [NOFB] for plane state %p\n", plane_state); +} +EXPORT_SYMBOL(drm_atomic_set_fb_for_plane); + +/** * drm_atomic_set_crtc_for_connector - set crtc for connector * @conn_state: atomic state object for the connector * @crtc: crtc to use for the connector diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 2b1db0c..ca839bd 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1182,7 +1182,7 @@ retry: ret = drm_atomic_set_crtc_for_plane(plane_state, crtc); if (ret != 0) goto fail; - plane_state->fb = fb; + drm_atomic_set_fb_for_plane(plane_state, fb); plane_state->crtc_x = crtc_x; plane_state->crtc_y = crtc_y; plane_state->crtc_h = crtc_h; @@ -1250,7 +1250,7 @@ retry: ret = drm_atomic_set_crtc_for_plane(plane_state, NULL); if (ret != 0) goto fail; - plane_state->fb = NULL; + drm_atomic_set_fb_for_plane(plane_state, NULL); plane_state->crtc_x = 0; plane_state->crtc_y = 0; plane_state->crtc_h = 0; @@ -1422,7 +1422,7 @@ retry: ret = drm_atomic_set_crtc_for_plane(primary_state, crtc); if (ret != 0) goto fail; - primary_state->fb = set->fb; + drm_atomic_set_fb_for_plane(primary_state, set->fb); primary_state->crtc_x = 0; primary_state->crtc_y = 0; primary_state->crtc_h = set->mode->vdisplay; @@ -1694,7 +1694,7 @@ retry: ret = drm_atomic_set_crtc_for_plane(plane_state, crtc); if (ret != 0) goto fail; - plane_state->fb = fb; + drm_atomic_set_fb_for_plane(plane_state, fb); ret = drm_atomic_async_commit(state); if (ret != 0) @@ -1808,6 +1808,9 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_destroy_state); */ void drm_atomic_helper_plane_reset(struct drm_plane *plane) { + if (plane->state && plane->state->fb) + drm_framebuffer_unreference(plane->state->fb); + kfree(plane->state); plane->state = kzalloc(sizeof(*plane->state), GFP_KERNEL); } @@ -1823,10 +1826,17 @@ EXPORT_SYMBOL(drm_atomic_helper_plane_reset); struct drm_plane_state * drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane) { + struct drm_plane_state *state; + if (WARN_ON(!plane->state)) return NULL; - return kmemdup(plane->state, sizeof(*plane->state), GFP_KERNEL); + state = kmemdup(plane->state, sizeof(*plane->state), GFP_KERNEL); + + if (state && state->fb) + drm_framebuffer_reference(state->fb); + + return state; } EXPORT_SYMBOL(drm_atomic_helper_plane_duplicate_state); @@ -1839,8 +1849,11 @@ EXPORT_SYMBOL(drm_atomic_helper_plane_duplicate_state); * subclassed plane state structure. */ void drm_atomic_helper_plane_destroy_state(struct drm_plane *plane, - struct drm_plane_state *state) + struct drm_plane_state *state) { + if (state->fb) + drm_framebuffer_unreference(state->fb); + kfree(state); } EXPORT_SYMBOL(drm_atomic_helper_plane_destroy_state); diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 33195e9..d552708 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -34,11 +34,13 @@ #include <linux/moduleparam.h> #include <drm/drmP.h> +#include <drm/drm_atomic.h> #include <drm/drm_crtc.h> #include <drm/drm_fourcc.h> #include <drm/drm_crtc_helper.h> #include <drm/drm_fb_helper.h> #include <drm/drm_plane_helper.h> +#include <drm/drm_atomic_helper.h> #include <drm/drm_edid.h> /** @@ -998,15 +1000,14 @@ int drm_helper_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, if (plane->funcs->atomic_duplicate_state) plane_state = plane->funcs->atomic_duplicate_state(plane); else if (plane->state) - plane_state = kmemdup(plane->state, sizeof(*plane_state), - GFP_KERNEL); + plane_state = drm_atomic_helper_plane_duplicate_state(plane); else plane_state = kzalloc(sizeof(*plane_state), GFP_KERNEL); if (!plane_state) return -ENOMEM; plane_state->crtc = crtc; - plane_state->fb = crtc->primary->fb; + drm_atomic_set_fb_for_plane(plane_state, crtc->primary->fb); plane_state->crtc_x = 0; plane_state->crtc_y = 0; plane_state->crtc_h = crtc->mode.vdisplay; diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c index fa56bb5..d99c452 100644 --- a/drivers/gpu/drm/drm_plane_helper.c +++ b/drivers/gpu/drm/drm_plane_helper.c @@ -27,7 +27,9 @@ #include <drm/drmP.h> #include <drm/drm_plane_helper.h> #include <drm/drm_rect.h> +#include <drm/drm_atomic.h> #include <drm/drm_crtc_helper.h> +#include <drm/drm_atomic_helper.h> #define SUBPIXEL_MASK 0xffff @@ -464,7 +466,7 @@ out: if (plane->funcs->atomic_destroy_state) plane->funcs->atomic_destroy_state(plane, plane_state); else - kfree(plane_state); + drm_atomic_helper_plane_destroy_state(plane, plane_state); } return ret; @@ -505,15 +507,14 @@ int drm_plane_helper_update(struct drm_plane *plane, struct drm_crtc *crtc, if (plane->funcs->atomic_duplicate_state) plane_state = plane->funcs->atomic_duplicate_state(plane); else if (plane->state) - plane_state = kmemdup(plane->state, sizeof(*plane_state), - GFP_KERNEL); + plane_state = drm_atomic_helper_plane_duplicate_state(plane); else plane_state = kzalloc(sizeof(*plane_state), GFP_KERNEL); if (!plane_state) return -ENOMEM; plane_state->crtc = crtc; - plane_state->fb = fb; + drm_atomic_set_fb_for_plane(plane_state, fb); plane_state->crtc_x = crtc_x; plane_state->crtc_y = crtc_y; plane_state->crtc_h = crtc_h; @@ -552,15 +553,14 @@ int drm_plane_helper_disable(struct drm_plane *plane) if (plane->funcs->atomic_duplicate_state) plane_state = plane->funcs->atomic_duplicate_state(plane); else if (plane->state) - plane_state = kmemdup(plane->state, sizeof(*plane_state), - GFP_KERNEL); + plane_state = drm_atomic_helper_plane_duplicate_state(plane); else plane_state = kzalloc(sizeof(*plane_state), GFP_KERNEL); if (!plane_state) return -ENOMEM; plane_state->crtc = NULL; - plane_state->fb = NULL; + drm_atomic_set_fb_for_plane(plane_state, NULL); return drm_plane_helper_commit(plane, plane_state, plane->fb); } diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 5bb15f5..9d91916 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -46,6 +46,8 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state, int __must_check drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state, struct drm_crtc *crtc); +void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state, + struct drm_framebuffer *fb); int __must_check drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state, struct drm_crtc *crtc); |