Re: [Intel-gfx] [PATCH 15/23] drm: Stop updating plane->crtc/fb/old_fb on atomic drivers

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel