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@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
