| Message ID | 20180322152313.6561-16-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 22, 2018 at 05:23:05PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Stop playing around with plane->crtc/fb/old_fb with atomic > drivers. Make life a lot simpler when we don't have to do the > magic old_fb vs. fb dance around plane updates. That way we > can't risk plane->fb getting out of sync with plane->state->fb > and we're less likely to leak any refcounts as well. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/drm_atomic.c | 55 ++++--------------------------------- > drivers/gpu/drm/drm_atomic_helper.c | 15 +--------- > drivers/gpu/drm/drm_crtc.c | 8 ++++-- > drivers/gpu/drm/drm_fb_helper.c | 7 ----- > drivers/gpu/drm/drm_framebuffer.c | 5 ---- > drivers/gpu/drm/drm_plane.c | 14 ++++++---- > drivers/gpu/drm/drm_plane_helper.c | 4 ++- > include/drm/drm_atomic.h | 3 -- > 8 files changed, 24 insertions(+), 87 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 7d25c42f22db..b16cc37e2adf 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -692,6 +692,11 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state, > > WARN_ON(!state->acquire_ctx); > > + /* the legacy pointers should never be set */ > + WARN_ON(plane->fb); > + WARN_ON(plane->old_fb); > + WARN_ON(plane->crtc); I did already review all the users for plane->crtc and found: armada + shmob: not atomic, should be fine But there's also exynos, msm/mdp5, sti and vc4 doing various silly things with setting plane->crtc. I think before you can add this WARN_ON we need to clean up that cruft (it looks like 100% cargo culting, so should be quit). Going to think about the other patches tomorrow. -Daniel > + > plane_state = drm_atomic_get_existing_plane_state(state, plane); > if (plane_state) > return plane_state; > @@ -2021,45 +2026,6 @@ int drm_atomic_set_property(struct drm_atomic_state *state, > return ret; > } > > -/** > - * drm_atomic_clean_old_fb -- Unset old_fb pointers and set plane->fb pointers. > - * > - * @dev: drm device to check. > - * @plane_mask: plane mask for planes that were updated. > - * @ret: return value, can be -EDEADLK for a retry. > - * > - * Before doing an update &drm_plane.old_fb is set to &drm_plane.fb, but before > - * dropping the locks old_fb needs to be set to NULL and plane->fb updated. This > - * is a common operation for each atomic update, so this call is split off as a > - * helper. > - */ > -void drm_atomic_clean_old_fb(struct drm_device *dev, > - unsigned plane_mask, > - int ret) > -{ > - struct drm_plane *plane; > - > - /* if succeeded, fixup legacy plane crtc/fb ptrs before dropping > - * locks (ie. while it is still safe to deref plane->state). We > - * need to do this here because the driver entry points cannot > - * distinguish between legacy and atomic ioctls. > - */ > - drm_for_each_plane_mask(plane, dev, plane_mask) { > - if (ret == 0) { > - struct drm_framebuffer *new_fb = plane->state->fb; > - if (new_fb) > - drm_framebuffer_get(new_fb); > - plane->fb = new_fb; > - plane->crtc = plane->state->crtc; > - > - if (plane->old_fb) > - drm_framebuffer_put(plane->old_fb); > - } > - plane->old_fb = NULL; > - } > -} > -EXPORT_SYMBOL(drm_atomic_clean_old_fb); > - > /** > * DOC: explicit fencing properties > * > @@ -2280,9 +2246,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, > unsigned int copied_objs, copied_props; > struct drm_atomic_state *state; > struct drm_modeset_acquire_ctx ctx; > - struct drm_plane *plane; > struct drm_out_fence_state *fence_state; > - unsigned plane_mask; > int ret = 0; > unsigned int i, j, num_fences; > > @@ -2322,7 +2286,6 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, > state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET); > > retry: > - plane_mask = 0; > copied_objs = 0; > copied_props = 0; > fence_state = NULL; > @@ -2393,12 +2356,6 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, > copied_props++; > } > > - if (obj->type == DRM_MODE_OBJECT_PLANE && count_props && > - !(arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)) { > - plane = obj_to_plane(obj); > - plane_mask |= (1 << drm_plane_index(plane)); > - plane->old_fb = plane->fb; > - } > drm_mode_object_put(obj); > } > > @@ -2419,8 +2376,6 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, > } > > out: > - drm_atomic_clean_old_fb(dev, plane_mask, ret); > - > complete_crtc_signaling(dev, state, fence_state, num_fences, !ret); > > if (ret == -EDEADLK) { > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 0e806f070d00..d42d88b97396 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -2892,7 +2892,6 @@ static 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 int plane_mask = 0; > int ret, i; > > state = drm_atomic_state_alloc(dev); > @@ -2935,17 +2934,10 @@ static int __drm_atomic_helper_disable_all(struct drm_device *dev, > goto free; > > drm_atomic_set_fb_for_plane(plane_state, NULL); > - > - if (clean_old_fbs) { > - plane->old_fb = plane->fb; > - plane_mask |= BIT(drm_plane_index(plane)); > - } > } > > ret = drm_atomic_commit(state); > free: > - drm_atomic_clean_old_fb(dev, plane_mask, ret); > - > drm_atomic_state_put(state); > return ret; > } > @@ -3106,13 +3098,8 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, > > state->acquire_ctx = ctx; > > - for_each_new_plane_in_state(state, plane, new_plane_state, i) { > - WARN_ON(plane->crtc != new_plane_state->crtc); > - WARN_ON(plane->fb != new_plane_state->fb); > - WARN_ON(plane->old_fb); > - > + for_each_new_plane_in_state(state, plane, new_plane_state, i) > 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; > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index a231dd5dce16..4e3c1a8d118a 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -474,8 +474,12 @@ static int __drm_mode_set_config_internal(struct drm_mode_set *set, > > ret = crtc->funcs->set_config(set, ctx); > if (ret == 0) { > - crtc->primary->crtc = fb ? crtc : NULL; > - crtc->primary->fb = fb; > + struct drm_plane *plane = crtc->primary; > + > + if (!plane->state) { > + plane->crtc = fb ? crtc : NULL; > + plane->fb = fb; > + } > } > > drm_for_each_crtc(tmp, crtc->dev) { > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 0646b108030b..5639e804a0cd 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -368,7 +368,6 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ > struct drm_plane *plane; > struct drm_atomic_state *state; > int i, ret; > - unsigned int plane_mask; > struct drm_modeset_acquire_ctx ctx; > > drm_modeset_acquire_init(&ctx, 0); > @@ -381,7 +380,6 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ > > state->acquire_ctx = &ctx; > retry: > - plane_mask = 0; > drm_for_each_plane(plane, dev) { > plane_state = drm_atomic_get_plane_state(state, plane); > if (IS_ERR(plane_state)) { > @@ -391,9 +389,6 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ > > plane_state->rotation = DRM_MODE_ROTATE_0; > > - plane->old_fb = plane->fb; > - plane_mask |= 1 << drm_plane_index(plane); > - > /* disable non-primary: */ > if (plane->type == DRM_PLANE_TYPE_PRIMARY) > continue; > @@ -430,8 +425,6 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ > ret = drm_atomic_commit(state); > > out_state: > - drm_atomic_clean_old_fb(dev, plane_mask, ret); > - > if (ret == -EDEADLK) > goto backoff; > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > index ad67203de715..421a77c2a4ac 100644 > --- a/drivers/gpu/drm/drm_framebuffer.c > +++ b/drivers/gpu/drm/drm_framebuffer.c > @@ -835,8 +835,6 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) > goto unlock; > > plane_mask |= BIT(drm_plane_index(plane)); > - > - plane->old_fb = plane->fb; > } > > /* This list is only filled when disable_crtcs is set. */ > @@ -851,9 +849,6 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) > ret = drm_atomic_commit(state); > > unlock: > - if (plane_mask) > - drm_atomic_clean_old_fb(dev, plane_mask, ret); > - > if (ret == -EDEADLK) { > drm_atomic_state_clear(state); > drm_modeset_backoff(&ctx); > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > index 035054455301..143041666096 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -650,9 +650,11 @@ static int __setplane_internal(struct drm_plane *plane, > crtc_x, crtc_y, crtc_w, crtc_h, > src_x, src_y, src_w, src_h, ctx); > if (!ret) { > - plane->crtc = crtc; > - plane->fb = fb; > - drm_framebuffer_get(plane->fb); > + if (!plane->state) { > + plane->crtc = crtc; > + plane->fb = fb; > + drm_framebuffer_get(plane->fb); > + } > } else { > plane->old_fb = NULL; > } > @@ -1092,8 +1094,10 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, > /* Keep the old fb, don't unref it. */ > plane->old_fb = NULL; > } else { > - plane->fb = fb; > - drm_framebuffer_get(fb); > + if (!plane->state) { > + plane->fb = fb; > + drm_framebuffer_get(fb); > + } > } > > out: > diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c > index f88f68161519..2010794943bc 100644 > --- a/drivers/gpu/drm/drm_plane_helper.c > +++ b/drivers/gpu/drm/drm_plane_helper.c > @@ -502,6 +502,7 @@ EXPORT_SYMBOL(drm_plane_helper_update); > int drm_plane_helper_disable(struct drm_plane *plane) > { > struct drm_plane_state *plane_state; > + struct drm_framebuffer *old_fb; > > /* crtc helpers love to call disable functions for already disabled hw > * functions. So cope with that. */ > @@ -521,8 +522,9 @@ int drm_plane_helper_disable(struct drm_plane *plane) > plane_state->plane = plane; > > plane_state->crtc = NULL; > + old_fb = plane_state->fb; > drm_atomic_set_fb_for_plane(plane_state, NULL); > > - return drm_plane_helper_commit(plane, plane_state, plane->fb); > + return drm_plane_helper_commit(plane, plane_state, old_fb); > } > EXPORT_SYMBOL(drm_plane_helper_disable); > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > index a57a8aa90ffb..ca461b6cf71f 100644 > --- a/include/drm/drm_atomic.h > +++ b/include/drm/drm_atomic.h > @@ -601,9 +601,6 @@ int __must_check > drm_atomic_add_affected_planes(struct drm_atomic_state *state, > struct drm_crtc *crtc); > > -void > -drm_atomic_clean_old_fb(struct drm_device *dev, unsigned plane_mask, int ret); > - > int __must_check drm_atomic_check_only(struct drm_atomic_state *state); > int __must_check drm_atomic_commit(struct drm_atomic_state *state); > int __must_check drm_atomic_nonblocking_commit(struct drm_atomic_state *state); > -- > 2.16.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Mar 26, 2018 at 10:52:58PM +0200, Daniel Vetter wrote: > On Thu, Mar 22, 2018 at 05:23:05PM +0200, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Stop playing around with plane->crtc/fb/old_fb with atomic > > drivers. Make life a lot simpler when we don't have to do the > > magic old_fb vs. fb dance around plane updates. That way we > > can't risk plane->fb getting out of sync with plane->state->fb > > and we're less likely to leak any refcounts as well. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/drm_atomic.c | 55 ++++--------------------------------- > > drivers/gpu/drm/drm_atomic_helper.c | 15 +--------- > > drivers/gpu/drm/drm_crtc.c | 8 ++++-- > > drivers/gpu/drm/drm_fb_helper.c | 7 ----- > > drivers/gpu/drm/drm_framebuffer.c | 5 ---- > > drivers/gpu/drm/drm_plane.c | 14 ++++++---- > > drivers/gpu/drm/drm_plane_helper.c | 4 ++- > > include/drm/drm_atomic.h | 3 -- > > 8 files changed, 24 insertions(+), 87 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index 7d25c42f22db..b16cc37e2adf 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -692,6 +692,11 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state, > > > > WARN_ON(!state->acquire_ctx); > > > > + /* the legacy pointers should never be set */ > > + WARN_ON(plane->fb); > > + WARN_ON(plane->old_fb); > > + WARN_ON(plane->crtc); > > I did already review all the users for plane->crtc and found: > > armada + shmob: not atomic, should be fine > > But there's also exynos, msm/mdp5, sti and vc4 doing various silly things > with setting plane->crtc. I think before you can add this WARN_ON we need > to clean up that cruft (it looks like 100% cargo culting, so should be > quit). Ah, follow-up patches take care of most of this. But note that msm sets plane->crtc in its _init function, so will trip over your WARN_ON here. And you seem to have missed sti, which looks at plane->crtc instead of plane->state->crtc (and appropriate locking) in its debugfs code. -Daniel > > Going to think about the other patches tomorrow. > -Daniel > > > + > > plane_state = drm_atomic_get_existing_plane_state(state, plane); > > if (plane_state) > > return plane_state; > > @@ -2021,45 +2026,6 @@ int drm_atomic_set_property(struct drm_atomic_state *state, > > return ret; > > } > > > > -/** > > - * drm_atomic_clean_old_fb -- Unset old_fb pointers and set plane->fb pointers. > > - * > > - * @dev: drm device to check. > > - * @plane_mask: plane mask for planes that were updated. > > - * @ret: return value, can be -EDEADLK for a retry. > > - * > > - * Before doing an update &drm_plane.old_fb is set to &drm_plane.fb, but before > > - * dropping the locks old_fb needs to be set to NULL and plane->fb updated. This > > - * is a common operation for each atomic update, so this call is split off as a > > - * helper. > > - */ > > -void drm_atomic_clean_old_fb(struct drm_device *dev, > > - unsigned plane_mask, > > - int ret) > > -{ > > - struct drm_plane *plane; > > - > > - /* if succeeded, fixup legacy plane crtc/fb ptrs before dropping > > - * locks (ie. while it is still safe to deref plane->state). We > > - * need to do this here because the driver entry points cannot > > - * distinguish between legacy and atomic ioctls. > > - */ > > - drm_for_each_plane_mask(plane, dev, plane_mask) { > > - if (ret == 0) { > > - struct drm_framebuffer *new_fb = plane->state->fb; > > - if (new_fb) > > - drm_framebuffer_get(new_fb); > > - plane->fb = new_fb; > > - plane->crtc = plane->state->crtc; > > - > > - if (plane->old_fb) > > - drm_framebuffer_put(plane->old_fb); > > - } > > - plane->old_fb = NULL; > > - } > > -} > > -EXPORT_SYMBOL(drm_atomic_clean_old_fb); > > - > > /** > > * DOC: explicit fencing properties > > * > > @@ -2280,9 +2246,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, > > unsigned int copied_objs, copied_props; > > struct drm_atomic_state *state; > > struct drm_modeset_acquire_ctx ctx; > > - struct drm_plane *plane; > > struct drm_out_fence_state *fence_state; > > - unsigned plane_mask; > > int ret = 0; > > unsigned int i, j, num_fences; > > > > @@ -2322,7 +2286,6 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, > > state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET); > > > > retry: > > - plane_mask = 0; > > copied_objs = 0; > > copied_props = 0; > > fence_state = NULL; > > @@ -2393,12 +2356,6 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, > > copied_props++; > > } > > > > - if (obj->type == DRM_MODE_OBJECT_PLANE && count_props && > > - !(arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)) { > > - plane = obj_to_plane(obj); > > - plane_mask |= (1 << drm_plane_index(plane)); > > - plane->old_fb = plane->fb; > > - } > > drm_mode_object_put(obj); > > } > > > > @@ -2419,8 +2376,6 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, > > } > > > > out: > > - drm_atomic_clean_old_fb(dev, plane_mask, ret); > > - > > complete_crtc_signaling(dev, state, fence_state, num_fences, !ret); > > > > if (ret == -EDEADLK) { > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > index 0e806f070d00..d42d88b97396 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -2892,7 +2892,6 @@ static 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 int plane_mask = 0; > > int ret, i; > > > > state = drm_atomic_state_alloc(dev); > > @@ -2935,17 +2934,10 @@ static int __drm_atomic_helper_disable_all(struct drm_device *dev, > > goto free; > > > > drm_atomic_set_fb_for_plane(plane_state, NULL); > > - > > - if (clean_old_fbs) { > > - plane->old_fb = plane->fb; > > - plane_mask |= BIT(drm_plane_index(plane)); > > - } > > } > > > > ret = drm_atomic_commit(state); > > free: > > - drm_atomic_clean_old_fb(dev, plane_mask, ret); > > - > > drm_atomic_state_put(state); > > return ret; > > } > > @@ -3106,13 +3098,8 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, > > > > state->acquire_ctx = ctx; > > > > - for_each_new_plane_in_state(state, plane, new_plane_state, i) { > > - WARN_ON(plane->crtc != new_plane_state->crtc); > > - WARN_ON(plane->fb != new_plane_state->fb); > > - WARN_ON(plane->old_fb); > > - > > + for_each_new_plane_in_state(state, plane, new_plane_state, i) > > 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; > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > index a231dd5dce16..4e3c1a8d118a 100644 > > --- a/drivers/gpu/drm/drm_crtc.c > > +++ b/drivers/gpu/drm/drm_crtc.c > > @@ -474,8 +474,12 @@ static int __drm_mode_set_config_internal(struct drm_mode_set *set, > > > > ret = crtc->funcs->set_config(set, ctx); > > if (ret == 0) { > > - crtc->primary->crtc = fb ? crtc : NULL; > > - crtc->primary->fb = fb; > > + struct drm_plane *plane = crtc->primary; > > + > > + if (!plane->state) { > > + plane->crtc = fb ? crtc : NULL; > > + plane->fb = fb; > > + } > > } > > > > drm_for_each_crtc(tmp, crtc->dev) { > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > > index 0646b108030b..5639e804a0cd 100644 > > --- a/drivers/gpu/drm/drm_fb_helper.c > > +++ b/drivers/gpu/drm/drm_fb_helper.c > > @@ -368,7 +368,6 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ > > struct drm_plane *plane; > > struct drm_atomic_state *state; > > int i, ret; > > - unsigned int plane_mask; > > struct drm_modeset_acquire_ctx ctx; > > > > drm_modeset_acquire_init(&ctx, 0); > > @@ -381,7 +380,6 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ > > > > state->acquire_ctx = &ctx; > > retry: > > - plane_mask = 0; > > drm_for_each_plane(plane, dev) { > > plane_state = drm_atomic_get_plane_state(state, plane); > > if (IS_ERR(plane_state)) { > > @@ -391,9 +389,6 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ > > > > plane_state->rotation = DRM_MODE_ROTATE_0; > > > > - plane->old_fb = plane->fb; > > - plane_mask |= 1 << drm_plane_index(plane); > > - > > /* disable non-primary: */ > > if (plane->type == DRM_PLANE_TYPE_PRIMARY) > > continue; > > @@ -430,8 +425,6 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ > > ret = drm_atomic_commit(state); > > > > out_state: > > - drm_atomic_clean_old_fb(dev, plane_mask, ret); > > - > > if (ret == -EDEADLK) > > goto backoff; > > > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > > index ad67203de715..421a77c2a4ac 100644 > > --- a/drivers/gpu/drm/drm_framebuffer.c > > +++ b/drivers/gpu/drm/drm_framebuffer.c > > @@ -835,8 +835,6 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) > > goto unlock; > > > > plane_mask |= BIT(drm_plane_index(plane)); > > - > > - plane->old_fb = plane->fb; > > } > > > > /* This list is only filled when disable_crtcs is set. */ > > @@ -851,9 +849,6 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) > > ret = drm_atomic_commit(state); > > > > unlock: > > - if (plane_mask) > > - drm_atomic_clean_old_fb(dev, plane_mask, ret); > > - > > if (ret == -EDEADLK) { > > drm_atomic_state_clear(state); > > drm_modeset_backoff(&ctx); > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > > index 035054455301..143041666096 100644 > > --- a/drivers/gpu/drm/drm_plane.c > > +++ b/drivers/gpu/drm/drm_plane.c > > @@ -650,9 +650,11 @@ static int __setplane_internal(struct drm_plane *plane, > > crtc_x, crtc_y, crtc_w, crtc_h, > > src_x, src_y, src_w, src_h, ctx); > > if (!ret) { > > - plane->crtc = crtc; > > - plane->fb = fb; > > - drm_framebuffer_get(plane->fb); > > + if (!plane->state) { > > + plane->crtc = crtc; > > + plane->fb = fb; > > + drm_framebuffer_get(plane->fb); > > + } > > } else { > > plane->old_fb = NULL; > > } > > @@ -1092,8 +1094,10 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, > > /* Keep the old fb, don't unref it. */ > > plane->old_fb = NULL; > > } else { > > - plane->fb = fb; > > - drm_framebuffer_get(fb); > > + if (!plane->state) { > > + plane->fb = fb; > > + drm_framebuffer_get(fb); > > + } > > } > > > > out: > > diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c > > index f88f68161519..2010794943bc 100644 > > --- a/drivers/gpu/drm/drm_plane_helper.c > > +++ b/drivers/gpu/drm/drm_plane_helper.c > > @@ -502,6 +502,7 @@ EXPORT_SYMBOL(drm_plane_helper_update); > > int drm_plane_helper_disable(struct drm_plane *plane) > > { > > struct drm_plane_state *plane_state; > > + struct drm_framebuffer *old_fb; > > > > /* crtc helpers love to call disable functions for already disabled hw > > * functions. So cope with that. */ > > @@ -521,8 +522,9 @@ int drm_plane_helper_disable(struct drm_plane *plane) > > plane_state->plane = plane; > > > > plane_state->crtc = NULL; > > + old_fb = plane_state->fb; > > drm_atomic_set_fb_for_plane(plane_state, NULL); > > > > - return drm_plane_helper_commit(plane, plane_state, plane->fb); > > + return drm_plane_helper_commit(plane, plane_state, old_fb); > > } > > EXPORT_SYMBOL(drm_plane_helper_disable); > > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > > index a57a8aa90ffb..ca461b6cf71f 100644 > > --- a/include/drm/drm_atomic.h > > +++ b/include/drm/drm_atomic.h > > @@ -601,9 +601,6 @@ int __must_check > > drm_atomic_add_affected_planes(struct drm_atomic_state *state, > > struct drm_crtc *crtc); > > > > -void > > -drm_atomic_clean_old_fb(struct drm_device *dev, unsigned plane_mask, int ret); > > - > > int __must_check drm_atomic_check_only(struct drm_atomic_state *state); > > int __must_check drm_atomic_commit(struct drm_atomic_state *state); > > int __must_check drm_atomic_nonblocking_commit(struct drm_atomic_state *state); > > -- > > 2.16.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Thu, Mar 22, 2018 at 05:23:05PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Stop playing around with plane->crtc/fb/old_fb with atomic > drivers. Make life a lot simpler when we don't have to do the > magic old_fb vs. fb dance around plane updates. That way we > can't risk plane->fb getting out of sync with plane->state->fb > and we're less likely to leak any refcounts as well. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> What's the reason for this patch being in the middle of the patch series? I figured it's savest if we put this at the end? If you need parts of this here, we definitely need to split out the WARN_ON hunk, since you haven't fixed up everything yet at this point here. -Daniel > --- > drivers/gpu/drm/drm_atomic.c | 55 ++++--------------------------------- > drivers/gpu/drm/drm_atomic_helper.c | 15 +--------- > drivers/gpu/drm/drm_crtc.c | 8 ++++-- > drivers/gpu/drm/drm_fb_helper.c | 7 ----- > drivers/gpu/drm/drm_framebuffer.c | 5 ---- > drivers/gpu/drm/drm_plane.c | 14 ++++++---- > drivers/gpu/drm/drm_plane_helper.c | 4 ++- > include/drm/drm_atomic.h | 3 -- > 8 files changed, 24 insertions(+), 87 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 7d25c42f22db..b16cc37e2adf 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -692,6 +692,11 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state, > > WARN_ON(!state->acquire_ctx); > > + /* the legacy pointers should never be set */ > + WARN_ON(plane->fb); > + WARN_ON(plane->old_fb); > + WARN_ON(plane->crtc); > + > plane_state = drm_atomic_get_existing_plane_state(state, plane); > if (plane_state) > return plane_state; > @@ -2021,45 +2026,6 @@ int drm_atomic_set_property(struct drm_atomic_state *state, > return ret; > } > > -/** > - * drm_atomic_clean_old_fb -- Unset old_fb pointers and set plane->fb pointers. > - * > - * @dev: drm device to check. > - * @plane_mask: plane mask for planes that were updated. > - * @ret: return value, can be -EDEADLK for a retry. > - * > - * Before doing an update &drm_plane.old_fb is set to &drm_plane.fb, but before > - * dropping the locks old_fb needs to be set to NULL and plane->fb updated. This > - * is a common operation for each atomic update, so this call is split off as a > - * helper. > - */ > -void drm_atomic_clean_old_fb(struct drm_device *dev, > - unsigned plane_mask, > - int ret) > -{ > - struct drm_plane *plane; > - > - /* if succeeded, fixup legacy plane crtc/fb ptrs before dropping > - * locks (ie. while it is still safe to deref plane->state). We > - * need to do this here because the driver entry points cannot > - * distinguish between legacy and atomic ioctls. > - */ > - drm_for_each_plane_mask(plane, dev, plane_mask) { > - if (ret == 0) { > - struct drm_framebuffer *new_fb = plane->state->fb; > - if (new_fb) > - drm_framebuffer_get(new_fb); > - plane->fb = new_fb; > - plane->crtc = plane->state->crtc; > - > - if (plane->old_fb) > - drm_framebuffer_put(plane->old_fb); > - } > - plane->old_fb = NULL; > - } > -} > -EXPORT_SYMBOL(drm_atomic_clean_old_fb); > - > /** > * DOC: explicit fencing properties > * > @@ -2280,9 +2246,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, > unsigned int copied_objs, copied_props; > struct drm_atomic_state *state; > struct drm_modeset_acquire_ctx ctx; > - struct drm_plane *plane; > struct drm_out_fence_state *fence_state; > - unsigned plane_mask; > int ret = 0; > unsigned int i, j, num_fences; > > @@ -2322,7 +2286,6 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, > state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET); > > retry: > - plane_mask = 0; > copied_objs = 0; > copied_props = 0; > fence_state = NULL; > @@ -2393,12 +2356,6 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, > copied_props++; > } > > - if (obj->type == DRM_MODE_OBJECT_PLANE && count_props && > - !(arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)) { > - plane = obj_to_plane(obj); > - plane_mask |= (1 << drm_plane_index(plane)); > - plane->old_fb = plane->fb; > - } > drm_mode_object_put(obj); > } > > @@ -2419,8 +2376,6 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, > } > > out: > - drm_atomic_clean_old_fb(dev, plane_mask, ret); > - > complete_crtc_signaling(dev, state, fence_state, num_fences, !ret); > > if (ret == -EDEADLK) { > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 0e806f070d00..d42d88b97396 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -2892,7 +2892,6 @@ static 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 int plane_mask = 0; > int ret, i; > > state = drm_atomic_state_alloc(dev); > @@ -2935,17 +2934,10 @@ static int __drm_atomic_helper_disable_all(struct drm_device *dev, > goto free; > > drm_atomic_set_fb_for_plane(plane_state, NULL); > - > - if (clean_old_fbs) { > - plane->old_fb = plane->fb; > - plane_mask |= BIT(drm_plane_index(plane)); > - } > } > > ret = drm_atomic_commit(state); > free: > - drm_atomic_clean_old_fb(dev, plane_mask, ret); > - > drm_atomic_state_put(state); > return ret; > } > @@ -3106,13 +3098,8 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, > > state->acquire_ctx = ctx; > > - for_each_new_plane_in_state(state, plane, new_plane_state, i) { > - WARN_ON(plane->crtc != new_plane_state->crtc); > - WARN_ON(plane->fb != new_plane_state->fb); > - WARN_ON(plane->old_fb); > - > + for_each_new_plane_in_state(state, plane, new_plane_state, i) > 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; > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index a231dd5dce16..4e3c1a8d118a 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -474,8 +474,12 @@ static int __drm_mode_set_config_internal(struct drm_mode_set *set, > > ret = crtc->funcs->set_config(set, ctx); > if (ret == 0) { > - crtc->primary->crtc = fb ? crtc : NULL; > - crtc->primary->fb = fb; > + struct drm_plane *plane = crtc->primary; > + > + if (!plane->state) { > + plane->crtc = fb ? crtc : NULL; > + plane->fb = fb; > + } > } > > drm_for_each_crtc(tmp, crtc->dev) { > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 0646b108030b..5639e804a0cd 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -368,7 +368,6 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ > struct drm_plane *plane; > struct drm_atomic_state *state; > int i, ret; > - unsigned int plane_mask; > struct drm_modeset_acquire_ctx ctx; > > drm_modeset_acquire_init(&ctx, 0); > @@ -381,7 +380,6 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ > > state->acquire_ctx = &ctx; > retry: > - plane_mask = 0; > drm_for_each_plane(plane, dev) { > plane_state = drm_atomic_get_plane_state(state, plane); > if (IS_ERR(plane_state)) { > @@ -391,9 +389,6 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ > > plane_state->rotation = DRM_MODE_ROTATE_0; > > - plane->old_fb = plane->fb; > - plane_mask |= 1 << drm_plane_index(plane); > - > /* disable non-primary: */ > if (plane->type == DRM_PLANE_TYPE_PRIMARY) > continue; > @@ -430,8 +425,6 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ > ret = drm_atomic_commit(state); > > out_state: > - drm_atomic_clean_old_fb(dev, plane_mask, ret); > - > if (ret == -EDEADLK) > goto backoff; > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > index ad67203de715..421a77c2a4ac 100644 > --- a/drivers/gpu/drm/drm_framebuffer.c > +++ b/drivers/gpu/drm/drm_framebuffer.c > @@ -835,8 +835,6 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) > goto unlock; > > plane_mask |= BIT(drm_plane_index(plane)); > - > - plane->old_fb = plane->fb; > } > > /* This list is only filled when disable_crtcs is set. */ > @@ -851,9 +849,6 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) > ret = drm_atomic_commit(state); > > unlock: > - if (plane_mask) > - drm_atomic_clean_old_fb(dev, plane_mask, ret); > - > if (ret == -EDEADLK) { > drm_atomic_state_clear(state); > drm_modeset_backoff(&ctx); > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > index 035054455301..143041666096 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -650,9 +650,11 @@ static int __setplane_internal(struct drm_plane *plane, > crtc_x, crtc_y, crtc_w, crtc_h, > src_x, src_y, src_w, src_h, ctx); > if (!ret) { > - plane->crtc = crtc; > - plane->fb = fb; > - drm_framebuffer_get(plane->fb); > + if (!plane->state) { > + plane->crtc = crtc; > + plane->fb = fb; > + drm_framebuffer_get(plane->fb); > + } > } else { > plane->old_fb = NULL; > } > @@ -1092,8 +1094,10 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, > /* Keep the old fb, don't unref it. */ > plane->old_fb = NULL; > } else { > - plane->fb = fb; > - drm_framebuffer_get(fb); > + if (!plane->state) { > + plane->fb = fb; > + drm_framebuffer_get(fb); > + } > } > > out: > diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c > index f88f68161519..2010794943bc 100644 > --- a/drivers/gpu/drm/drm_plane_helper.c > +++ b/drivers/gpu/drm/drm_plane_helper.c > @@ -502,6 +502,7 @@ EXPORT_SYMBOL(drm_plane_helper_update); > int drm_plane_helper_disable(struct drm_plane *plane) > { > struct drm_plane_state *plane_state; > + struct drm_framebuffer *old_fb; > > /* crtc helpers love to call disable functions for already disabled hw > * functions. So cope with that. */ > @@ -521,8 +522,9 @@ int drm_plane_helper_disable(struct drm_plane *plane) > plane_state->plane = plane; > > plane_state->crtc = NULL; > + old_fb = plane_state->fb; > drm_atomic_set_fb_for_plane(plane_state, NULL); > > - return drm_plane_helper_commit(plane, plane_state, plane->fb); > + return drm_plane_helper_commit(plane, plane_state, old_fb); > } > EXPORT_SYMBOL(drm_plane_helper_disable); > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > index a57a8aa90ffb..ca461b6cf71f 100644 > --- a/include/drm/drm_atomic.h > +++ b/include/drm/drm_atomic.h > @@ -601,9 +601,6 @@ int __must_check > drm_atomic_add_affected_planes(struct drm_atomic_state *state, > struct drm_crtc *crtc); > > -void > -drm_atomic_clean_old_fb(struct drm_device *dev, unsigned plane_mask, int ret); > - > int __must_check drm_atomic_check_only(struct drm_atomic_state *state); > int __must_check drm_atomic_commit(struct drm_atomic_state *state); > int __must_check drm_atomic_nonblocking_commit(struct drm_atomic_state *state); > -- > 2.16.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Mar 27, 2018 at 09:57:41AM +0200, Daniel Vetter wrote: > On Thu, Mar 22, 2018 at 05:23:05PM +0200, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Stop playing around with plane->crtc/fb/old_fb with atomic > > drivers. Make life a lot simpler when we don't have to do the > > magic old_fb vs. fb dance around plane updates. That way we > > can't risk plane->fb getting out of sync with plane->state->fb > > and we're less likely to leak any refcounts as well. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > What's the reason for this patch being in the middle of the patch series? > I figured it's savest if we put this at the end? If you need parts of this > here, we definitely need to split out the WARN_ON hunk, since you haven't > fixed up everything yet at this point here. Yeah, the ordering is probably not great. I think I had some idea why I had to do "cleanup drivers a bit, do core/helper stuff, cleanup some more drivers, do other core/helper stuff". But I can't even recall that reason now. Most likely I had just managed to confuse myself by that time. > -Daniel > > > --- > > drivers/gpu/drm/drm_atomic.c | 55 ++++--------------------------------- > > drivers/gpu/drm/drm_atomic_helper.c | 15 +--------- > > drivers/gpu/drm/drm_crtc.c | 8 ++++-- > > drivers/gpu/drm/drm_fb_helper.c | 7 ----- > > drivers/gpu/drm/drm_framebuffer.c | 5 ---- > > drivers/gpu/drm/drm_plane.c | 14 ++++++---- > > drivers/gpu/drm/drm_plane_helper.c | 4 ++- > > include/drm/drm_atomic.h | 3 -- > > 8 files changed, 24 insertions(+), 87 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index 7d25c42f22db..b16cc37e2adf 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -692,6 +692,11 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state, > > > > WARN_ON(!state->acquire_ctx); > > > > + /* the legacy pointers should never be set */ > > + WARN_ON(plane->fb); > > + WARN_ON(plane->old_fb); > > + WARN_ON(plane->crtc); > > + > > plane_state = drm_atomic_get_existing_plane_state(state, plane); > > if (plane_state) > > return plane_state; > > @@ -2021,45 +2026,6 @@ int drm_atomic_set_property(struct drm_atomic_state *state, > > return ret; > > } > > > > -/** > > - * drm_atomic_clean_old_fb -- Unset old_fb pointers and set plane->fb pointers. > > - * > > - * @dev: drm device to check. > > - * @plane_mask: plane mask for planes that were updated. > > - * @ret: return value, can be -EDEADLK for a retry. > > - * > > - * Before doing an update &drm_plane.old_fb is set to &drm_plane.fb, but before > > - * dropping the locks old_fb needs to be set to NULL and plane->fb updated. This > > - * is a common operation for each atomic update, so this call is split off as a > > - * helper. > > - */ > > -void drm_atomic_clean_old_fb(struct drm_device *dev, > > - unsigned plane_mask, > > - int ret) > > -{ > > - struct drm_plane *plane; > > - > > - /* if succeeded, fixup legacy plane crtc/fb ptrs before dropping > > - * locks (ie. while it is still safe to deref plane->state). We > > - * need to do this here because the driver entry points cannot > > - * distinguish between legacy and atomic ioctls. > > - */ > > - drm_for_each_plane_mask(plane, dev, plane_mask) { > > - if (ret == 0) { > > - struct drm_framebuffer *new_fb = plane->state->fb; > > - if (new_fb) > > - drm_framebuffer_get(new_fb); > > - plane->fb = new_fb; > > - plane->crtc = plane->state->crtc; > > - > > - if (plane->old_fb) > > - drm_framebuffer_put(plane->old_fb); > > - } > > - plane->old_fb = NULL; > > - } > > -} > > -EXPORT_SYMBOL(drm_atomic_clean_old_fb); > > - > > /** > > * DOC: explicit fencing properties > > * > > @@ -2280,9 +2246,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, > > unsigned int copied_objs, copied_props; > > struct drm_atomic_state *state; > > struct drm_modeset_acquire_ctx ctx; > > - struct drm_plane *plane; > > struct drm_out_fence_state *fence_state; > > - unsigned plane_mask; > > int ret = 0; > > unsigned int i, j, num_fences; > > > > @@ -2322,7 +2286,6 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, > > state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET); > > > > retry: > > - plane_mask = 0; > > copied_objs = 0; > > copied_props = 0; > > fence_state = NULL; > > @@ -2393,12 +2356,6 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, > > copied_props++; > > } > > > > - if (obj->type == DRM_MODE_OBJECT_PLANE && count_props && > > - !(arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)) { > > - plane = obj_to_plane(obj); > > - plane_mask |= (1 << drm_plane_index(plane)); > > - plane->old_fb = plane->fb; > > - } > > drm_mode_object_put(obj); > > } > > > > @@ -2419,8 +2376,6 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, > > } > > > > out: > > - drm_atomic_clean_old_fb(dev, plane_mask, ret); > > - > > complete_crtc_signaling(dev, state, fence_state, num_fences, !ret); > > > > if (ret == -EDEADLK) { > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > index 0e806f070d00..d42d88b97396 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -2892,7 +2892,6 @@ static 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 int plane_mask = 0; > > int ret, i; > > > > state = drm_atomic_state_alloc(dev); > > @@ -2935,17 +2934,10 @@ static int __drm_atomic_helper_disable_all(struct drm_device *dev, > > goto free; > > > > drm_atomic_set_fb_for_plane(plane_state, NULL); > > - > > - if (clean_old_fbs) { > > - plane->old_fb = plane->fb; > > - plane_mask |= BIT(drm_plane_index(plane)); > > - } > > } > > > > ret = drm_atomic_commit(state); > > free: > > - drm_atomic_clean_old_fb(dev, plane_mask, ret); > > - > > drm_atomic_state_put(state); > > return ret; > > } > > @@ -3106,13 +3098,8 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, > > > > state->acquire_ctx = ctx; > > > > - for_each_new_plane_in_state(state, plane, new_plane_state, i) { > > - WARN_ON(plane->crtc != new_plane_state->crtc); > > - WARN_ON(plane->fb != new_plane_state->fb); > > - WARN_ON(plane->old_fb); > > - > > + for_each_new_plane_in_state(state, plane, new_plane_state, i) > > 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; > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > index a231dd5dce16..4e3c1a8d118a 100644 > > --- a/drivers/gpu/drm/drm_crtc.c > > +++ b/drivers/gpu/drm/drm_crtc.c > > @@ -474,8 +474,12 @@ static int __drm_mode_set_config_internal(struct drm_mode_set *set, > > > > ret = crtc->funcs->set_config(set, ctx); > > if (ret == 0) { > > - crtc->primary->crtc = fb ? crtc : NULL; > > - crtc->primary->fb = fb; > > + struct drm_plane *plane = crtc->primary; > > + > > + if (!plane->state) { > > + plane->crtc = fb ? crtc : NULL; > > + plane->fb = fb; > > + } > > } > > > > drm_for_each_crtc(tmp, crtc->dev) { > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > > index 0646b108030b..5639e804a0cd 100644 > > --- a/drivers/gpu/drm/drm_fb_helper.c > > +++ b/drivers/gpu/drm/drm_fb_helper.c > > @@ -368,7 +368,6 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ > > struct drm_plane *plane; > > struct drm_atomic_state *state; > > int i, ret; > > - unsigned int plane_mask; > > struct drm_modeset_acquire_ctx ctx; > > > > drm_modeset_acquire_init(&ctx, 0); > > @@ -381,7 +380,6 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ > > > > state->acquire_ctx = &ctx; > > retry: > > - plane_mask = 0; > > drm_for_each_plane(plane, dev) { > > plane_state = drm_atomic_get_plane_state(state, plane); > > if (IS_ERR(plane_state)) { > > @@ -391,9 +389,6 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ > > > > plane_state->rotation = DRM_MODE_ROTATE_0; > > > > - plane->old_fb = plane->fb; > > - plane_mask |= 1 << drm_plane_index(plane); > > - > > /* disable non-primary: */ > > if (plane->type == DRM_PLANE_TYPE_PRIMARY) > > continue; > > @@ -430,8 +425,6 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ > > ret = drm_atomic_commit(state); > > > > out_state: > > - drm_atomic_clean_old_fb(dev, plane_mask, ret); > > - > > if (ret == -EDEADLK) > > goto backoff; > > > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > > index ad67203de715..421a77c2a4ac 100644 > > --- a/drivers/gpu/drm/drm_framebuffer.c > > +++ b/drivers/gpu/drm/drm_framebuffer.c > > @@ -835,8 +835,6 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) > > goto unlock; > > > > plane_mask |= BIT(drm_plane_index(plane)); > > - > > - plane->old_fb = plane->fb; > > } > > > > /* This list is only filled when disable_crtcs is set. */ > > @@ -851,9 +849,6 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) > > ret = drm_atomic_commit(state); > > > > unlock: > > - if (plane_mask) > > - drm_atomic_clean_old_fb(dev, plane_mask, ret); > > - > > if (ret == -EDEADLK) { > > drm_atomic_state_clear(state); > > drm_modeset_backoff(&ctx); > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > > index 035054455301..143041666096 100644 > > --- a/drivers/gpu/drm/drm_plane.c > > +++ b/drivers/gpu/drm/drm_plane.c > > @@ -650,9 +650,11 @@ static int __setplane_internal(struct drm_plane *plane, > > crtc_x, crtc_y, crtc_w, crtc_h, > > src_x, src_y, src_w, src_h, ctx); > > if (!ret) { > > - plane->crtc = crtc; > > - plane->fb = fb; > > - drm_framebuffer_get(plane->fb); > > + if (!plane->state) { > > + plane->crtc = crtc; > > + plane->fb = fb; > > + drm_framebuffer_get(plane->fb); > > + } > > } else { > > plane->old_fb = NULL; > > } > > @@ -1092,8 +1094,10 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, > > /* Keep the old fb, don't unref it. */ > > plane->old_fb = NULL; > > } else { > > - plane->fb = fb; > > - drm_framebuffer_get(fb); > > + if (!plane->state) { > > + plane->fb = fb; > > + drm_framebuffer_get(fb); > > + } > > } > > > > out: > > diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c > > index f88f68161519..2010794943bc 100644 > > --- a/drivers/gpu/drm/drm_plane_helper.c > > +++ b/drivers/gpu/drm/drm_plane_helper.c > > @@ -502,6 +502,7 @@ EXPORT_SYMBOL(drm_plane_helper_update); > > int drm_plane_helper_disable(struct drm_plane *plane) > > { > > struct drm_plane_state *plane_state; > > + struct drm_framebuffer *old_fb; > > > > /* crtc helpers love to call disable functions for already disabled hw > > * functions. So cope with that. */ > > @@ -521,8 +522,9 @@ int drm_plane_helper_disable(struct drm_plane *plane) > > plane_state->plane = plane; > > > > plane_state->crtc = NULL; > > + old_fb = plane_state->fb; > > drm_atomic_set_fb_for_plane(plane_state, NULL); > > > > - return drm_plane_helper_commit(plane, plane_state, plane->fb); > > + return drm_plane_helper_commit(plane, plane_state, old_fb); > > } > > EXPORT_SYMBOL(drm_plane_helper_disable); > > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > > index a57a8aa90ffb..ca461b6cf71f 100644 > > --- a/include/drm/drm_atomic.h > > +++ b/include/drm/drm_atomic.h > > @@ -601,9 +601,6 @@ int __must_check > > drm_atomic_add_affected_planes(struct drm_atomic_state *state, > > struct drm_crtc *crtc); > > > > -void > > -drm_atomic_clean_old_fb(struct drm_device *dev, unsigned plane_mask, int ret); > > - > > int __must_check drm_atomic_check_only(struct drm_atomic_state *state); > > int __must_check drm_atomic_commit(struct drm_atomic_state *state); > > int __must_check drm_atomic_nonblocking_commit(struct drm_atomic_state *state); > > -- > > 2.16.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 7d25c42f22db..b16cc37e2adf 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -692,6 +692,11 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state, WARN_ON(!state->acquire_ctx); + /* the legacy pointers should never be set */ + WARN_ON(plane->fb); + WARN_ON(plane->old_fb); + WARN_ON(plane->crtc); + plane_state = drm_atomic_get_existing_plane_state(state, plane); if (plane_state) return plane_state; @@ -2021,45 +2026,6 @@ int drm_atomic_set_property(struct drm_atomic_state *state, return ret; } -/** - * drm_atomic_clean_old_fb -- Unset old_fb pointers and set plane->fb pointers. - * - * @dev: drm device to check. - * @plane_mask: plane mask for planes that were updated. - * @ret: return value, can be -EDEADLK for a retry. - * - * Before doing an update &drm_plane.old_fb is set to &drm_plane.fb, but before - * dropping the locks old_fb needs to be set to NULL and plane->fb updated. This - * is a common operation for each atomic update, so this call is split off as a - * helper. - */ -void drm_atomic_clean_old_fb(struct drm_device *dev, - unsigned plane_mask, - int ret) -{ - struct drm_plane *plane; - - /* if succeeded, fixup legacy plane crtc/fb ptrs before dropping - * locks (ie. while it is still safe to deref plane->state). We - * need to do this here because the driver entry points cannot - * distinguish between legacy and atomic ioctls. - */ - drm_for_each_plane_mask(plane, dev, plane_mask) { - if (ret == 0) { - struct drm_framebuffer *new_fb = plane->state->fb; - if (new_fb) - drm_framebuffer_get(new_fb); - plane->fb = new_fb; - plane->crtc = plane->state->crtc; - - if (plane->old_fb) - drm_framebuffer_put(plane->old_fb); - } - plane->old_fb = NULL; - } -} -EXPORT_SYMBOL(drm_atomic_clean_old_fb); - /** * DOC: explicit fencing properties * @@ -2280,9 +2246,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, unsigned int copied_objs, copied_props; struct drm_atomic_state *state; struct drm_modeset_acquire_ctx ctx; - struct drm_plane *plane; struct drm_out_fence_state *fence_state; - unsigned plane_mask; int ret = 0; unsigned int i, j, num_fences; @@ -2322,7 +2286,6 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET); retry: - plane_mask = 0; copied_objs = 0; copied_props = 0; fence_state = NULL; @@ -2393,12 +2356,6 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, copied_props++; } - if (obj->type == DRM_MODE_OBJECT_PLANE && count_props && - !(arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)) { - plane = obj_to_plane(obj); - plane_mask |= (1 << drm_plane_index(plane)); - plane->old_fb = plane->fb; - } drm_mode_object_put(obj); } @@ -2419,8 +2376,6 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, } out: - drm_atomic_clean_old_fb(dev, plane_mask, ret); - complete_crtc_signaling(dev, state, fence_state, num_fences, !ret); if (ret == -EDEADLK) { diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 0e806f070d00..d42d88b97396 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2892,7 +2892,6 @@ static 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 int plane_mask = 0; int ret, i; state = drm_atomic_state_alloc(dev); @@ -2935,17 +2934,10 @@ static int __drm_atomic_helper_disable_all(struct drm_device *dev, goto free; drm_atomic_set_fb_for_plane(plane_state, NULL); - - if (clean_old_fbs) { - plane->old_fb = plane->fb; - plane_mask |= BIT(drm_plane_index(plane)); - } } ret = drm_atomic_commit(state); free: - drm_atomic_clean_old_fb(dev, plane_mask, ret); - drm_atomic_state_put(state); return ret; } @@ -3106,13 +3098,8 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, state->acquire_ctx = ctx; - for_each_new_plane_in_state(state, plane, new_plane_state, i) { - WARN_ON(plane->crtc != new_plane_state->crtc); - WARN_ON(plane->fb != new_plane_state->fb); - WARN_ON(plane->old_fb); - + for_each_new_plane_in_state(state, plane, new_plane_state, i) 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; diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index a231dd5dce16..4e3c1a8d118a 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -474,8 +474,12 @@ static int __drm_mode_set_config_internal(struct drm_mode_set *set, ret = crtc->funcs->set_config(set, ctx); if (ret == 0) { - crtc->primary->crtc = fb ? crtc : NULL; - crtc->primary->fb = fb; + struct drm_plane *plane = crtc->primary; + + if (!plane->state) { + plane->crtc = fb ? crtc : NULL; + plane->fb = fb; + } } drm_for_each_crtc(tmp, crtc->dev) { diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 0646b108030b..5639e804a0cd 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -368,7 +368,6 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ struct drm_plane *plane; struct drm_atomic_state *state; int i, ret; - unsigned int plane_mask; struct drm_modeset_acquire_ctx ctx; drm_modeset_acquire_init(&ctx, 0); @@ -381,7 +380,6 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ state->acquire_ctx = &ctx; retry: - plane_mask = 0; drm_for_each_plane(plane, dev) { plane_state = drm_atomic_get_plane_state(state, plane); if (IS_ERR(plane_state)) { @@ -391,9 +389,6 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ plane_state->rotation = DRM_MODE_ROTATE_0; - plane->old_fb = plane->fb; - plane_mask |= 1 << drm_plane_index(plane); - /* disable non-primary: */ if (plane->type == DRM_PLANE_TYPE_PRIMARY) continue; @@ -430,8 +425,6 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ ret = drm_atomic_commit(state); out_state: - drm_atomic_clean_old_fb(dev, plane_mask, ret); - if (ret == -EDEADLK) goto backoff; diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index ad67203de715..421a77c2a4ac 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -835,8 +835,6 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) goto unlock; plane_mask |= BIT(drm_plane_index(plane)); - - plane->old_fb = plane->fb; } /* This list is only filled when disable_crtcs is set. */ @@ -851,9 +849,6 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) ret = drm_atomic_commit(state); unlock: - if (plane_mask) - drm_atomic_clean_old_fb(dev, plane_mask, ret); - if (ret == -EDEADLK) { drm_atomic_state_clear(state); drm_modeset_backoff(&ctx); diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 035054455301..143041666096 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -650,9 +650,11 @@ static int __setplane_internal(struct drm_plane *plane, crtc_x, crtc_y, crtc_w, crtc_h, src_x, src_y, src_w, src_h, ctx); if (!ret) { - plane->crtc = crtc; - plane->fb = fb; - drm_framebuffer_get(plane->fb); + if (!plane->state) { + plane->crtc = crtc; + plane->fb = fb; + drm_framebuffer_get(plane->fb); + } } else { plane->old_fb = NULL; } @@ -1092,8 +1094,10 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, /* Keep the old fb, don't unref it. */ plane->old_fb = NULL; } else { - plane->fb = fb; - drm_framebuffer_get(fb); + if (!plane->state) { + plane->fb = fb; + drm_framebuffer_get(fb); + } } out: diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c index f88f68161519..2010794943bc 100644 --- a/drivers/gpu/drm/drm_plane_helper.c +++ b/drivers/gpu/drm/drm_plane_helper.c @@ -502,6 +502,7 @@ EXPORT_SYMBOL(drm_plane_helper_update); int drm_plane_helper_disable(struct drm_plane *plane) { struct drm_plane_state *plane_state; + struct drm_framebuffer *old_fb; /* crtc helpers love to call disable functions for already disabled hw * functions. So cope with that. */ @@ -521,8 +522,9 @@ int drm_plane_helper_disable(struct drm_plane *plane) plane_state->plane = plane; plane_state->crtc = NULL; + old_fb = plane_state->fb; drm_atomic_set_fb_for_plane(plane_state, NULL); - return drm_plane_helper_commit(plane, plane_state, plane->fb); + return drm_plane_helper_commit(plane, plane_state, old_fb); } EXPORT_SYMBOL(drm_plane_helper_disable); diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index a57a8aa90ffb..ca461b6cf71f 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -601,9 +601,6 @@ int __must_check drm_atomic_add_affected_planes(struct drm_atomic_state *state, struct drm_crtc *crtc); -void -drm_atomic_clean_old_fb(struct drm_device *dev, unsigned plane_mask, int ret); - int __must_check drm_atomic_check_only(struct drm_atomic_state *state); int __must_check drm_atomic_commit(struct drm_atomic_state *state); int __must_check drm_atomic_nonblocking_commit(struct drm_atomic_state *state);