Op 15-07-17 om 11:31 schreef Daniel Vetter: > The legacy plane->fb pointer is refcounted by calling > drm_atomic_clean_old_fb(). > > In practice this isn't a real problem because: > - The caller in the i915 gpu reset code restores the original state > again, which means the plane->fb pointer won't change, hence can't > leak. > - Drivers using drm_atomic_helper_shutdown call the fbdev cleanup > first, and that usually cleans up the fb through > drm_remove_framebuffer, which does this correctly. > - Without fbdev the only framebuffers are from userspace, and those > get cleaned up (again using drm_remove_framebuffer) befor the driver > can even be unloaded. > > But in i915 I've switched the cleanup sequence around so that the > _shutdown() calls happens after the drm_remove_framebuffer(), which is > how I discovered this issue. > > v2: My analysis why the current code was ok for gpu reset and > suspend/resume was correct, but then I totally failed to realize that > we better keep this symmetric. Thanksfully CI noticed that for > balance, a refcounting bug must exist at 2 places if previously there > was no issue ... > > v3: Don't be lazy and compute the plane_mask in > commit_duplicated_state properly too, instead of just using ~0U. > > Cc: [email protected] > Cc: [email protected] > Acked-by: Dave Airlie <[email protected]> (v1) > Signed-off-by: Daniel Vetter <[email protected]> > --- > drivers/gpu/drm/drm_atomic_helper.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > b/drivers/gpu/drm/drm_atomic_helper.c > index b07fc30372d3..545328a9a769 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -2726,6 +2726,7 @@ int drm_atomic_helper_disable_all(struct drm_device > *dev, > struct drm_plane *plane; > struct drm_crtc_state *crtc_state; > struct drm_crtc *crtc; > + unsigned plane_mask = 0; > int ret, i; > > state = drm_atomic_state_alloc(dev); > @@ -2768,10 +2769,14 @@ int drm_atomic_helper_disable_all(struct drm_device > *dev, > goto free; > > drm_atomic_set_fb_for_plane(plane_state, NULL); > + plane_mask |= BIT(drm_plane_index(plane)); > + plane->old_fb = plane->fb; > } > > ret = drm_atomic_commit(state); > free: > + if (plane_mask) > + drm_atomic_clean_old_fb(dev, plane_mask, ret); > drm_atomic_state_put(state); > return ret; > } > @@ -2902,11 +2907,16 @@ int drm_atomic_helper_commit_duplicated_state(struct > drm_atomic_state *state, > struct drm_connector_state *new_conn_state; > struct drm_crtc *crtc; > struct drm_crtc_state *new_crtc_state; > + unsigned plane_mask = 0; > + struct drm_device *dev = state->dev; > + int ret; > > state->acquire_ctx = ctx; > > - for_each_new_plane_in_state(state, plane, new_plane_state, i) > + for_each_new_plane_in_state(state, plane, new_plane_state, i) { > + plane_mask |= BIT(drm_plane_index(plane)); We should really add a drm_plane_mask/drm_connector_mask at some point, to complement drm_crtc_mask. > state->planes[i].old_state = plane->state; > + } > > for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) > state->crtcs[i].old_state = crtc->state; > @@ -2914,7 +2924,11 @@ int drm_atomic_helper_commit_duplicated_state(struct > drm_atomic_state *state, > for_each_new_connector_in_state(state, connector, new_conn_state, i) > state->connectors[i].old_state = connector->state; > > - return drm_atomic_commit(state); > + ret = drm_atomic_commit(state); > + if (plane_mask) > + drm_atomic_clean_old_fb(dev, plane_mask, ret); Kill the if () part, and make it unconditional? On second thought, I should have done the same in drm_framebuffer.c > + return ret; > } > EXPORT_SYMBOL(drm_atomic_helper_commit_duplicated_state); >
_______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
