Re: [PATCH 4/4] drm: Resurrect atomic rmfb code, v2

[Matt Roper <matthew.d.roper@intel.com>]

On Wed, Jan 11, 2017 at 05:15:47PM +0100, Daniel Vetter wrote:
> On Thu, Dec 15, 2016 at 03:29:45PM +0100, Maarten Lankhorst wrote:
> > From: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > This was somehow lost between v3 and the merged version in Maarten's
> > patch merged as:
> > 
> > commit f2d580b9a8149735cbc4b59c4a8df60173658140
> > Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Date:   Wed May 4 14:38:26 2016 +0200
> > 
> >     drm/core: Do not preserve framebuffer on rmfb, v4.
> > 
> > Actual code copied from Maarten's patch, but with the slight change to
> > just use dev->mode_config.funcs->atomic_commit to decide whether to
> > use the atomic path or not.
> > 
> > v2:
> > - Remove plane->fb assignment, done by drm_atomic_clean_old_fb.
> > - Add WARN_ON when atomic_remove_fb fails.
> > - Always call drm_atomic_state_put.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> Would be great if someone else could r-b this, I've proven pretty well
> that I don't understand the complexity here :(
> -Daniel

It looks like this will change the behavior slightly in that rmfb will
cause primary planes to be disabled, but no longer cause the entire CRTC
to be turned off.  You'll probably want to note that in the commit
message, along with the justification on why this is okay ABI-wise.

I know that 13803132818c ("drm/core: Preserve the framebuffer after
removing it.") was initially trying to not only leave the CRTC on, but
also preserve the framebuffer and leave the planes on; that wound up
causing some kind of regression for vmwgfx, but I'm unclear on the
details there.  I'd suggest getting an Ack from one of the vmware guys
to ensure that the less drastic change in behavior here won't cause them
any problems.

Your code looks correct to me, so with an expanded commit message,

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

General observation; we specifically test for the presence of
mode_config->funcs.atomic_commit here rather than DRIVER_ATOMIC because
we care about a driver's internal atomic plumbing rather than whether it
exposes atomic to userspace or not.  I can see that causing confusion in
the future, so I wonder if adding macros like
DRM_SUPPORTS_ATOMIC_INTERNAL(dev) vs DRM_SUPPORTS_ATOMIC_USERSPACE(dev)
might help overclarify exactly what/why we're testing in various places
in the driver.


Matt

> 
> > ---
> >  drivers/gpu/drm/drm_atomic.c        | 64 +++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/drm_crtc_internal.h |  1 +
> >  drivers/gpu/drm/drm_framebuffer.c   |  7 ++++
> >  3 files changed, 72 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index d1d252261bf1..23a3845542e1 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -2059,6 +2059,70 @@ static void complete_crtc_signaling(struct drm_device *dev,
> >  	kfree(fence_state);
> >  }
> >  
> > +int drm_atomic_remove_fb(struct drm_framebuffer *fb)
> > +{
> > +	struct drm_modeset_acquire_ctx ctx;
> > +	struct drm_device *dev = fb->dev;
> > +	struct drm_atomic_state *state;
> > +	struct drm_plane *plane;
> > +	int ret = 0;
> > +	unsigned plane_mask;
> > +
> > +	state = drm_atomic_state_alloc(dev);
> > +	if (!state)
> > +		return -ENOMEM;
> > +
> > +	drm_modeset_acquire_init(&ctx, 0);
> > +	state->acquire_ctx = &ctx;
> > +
> > +retry:
> > +	plane_mask = 0;
> > +	ret = drm_modeset_lock_all_ctx(dev, &ctx);
> > +	if (ret)
> > +		goto unlock;
> > +
> > +	drm_for_each_plane(plane, dev) {
> > +		struct drm_plane_state *plane_state;
> > +
> > +		if (plane->state->fb != fb)
> > +			continue;
> > +
> > +		plane_state = drm_atomic_get_plane_state(state, plane);
> > +		if (IS_ERR(plane_state)) {
> > +			ret = PTR_ERR(plane_state);
> > +			goto unlock;
> > +		}
> > +
> > +		drm_atomic_set_fb_for_plane(plane_state, NULL);
> > +		ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
> > +		if (ret)
> > +			goto unlock;
> > +
> > +		plane_mask |= BIT(drm_plane_index(plane));
> > +
> > +		plane->old_fb = plane->fb;
> > +	}
> > +
> > +	if (plane_mask)
> > +		ret = drm_atomic_commit(state);
> > +
> > +unlock:
> > +	if (plane_mask)
> > +		drm_atomic_clean_old_fb(dev, plane_mask, ret);
> > +
> > +	if (ret == -EDEADLK) {
> > +		drm_modeset_backoff(&ctx);
> > +		goto retry;
> > +	}
> > +
> > +	drm_atomic_state_put(state);
> > +
> > +	drm_modeset_drop_locks(&ctx);
> > +	drm_modeset_acquire_fini(&ctx);
> > +
> > +	return ret;
> > +}
> > +
> >  int drm_mode_atomic_ioctl(struct drm_device *dev,
> >  			  void *data, struct drm_file *file_priv)
> >  {
> > diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> > index cdf6860c9d22..121e250853d2 100644
> > --- a/drivers/gpu/drm/drm_crtc_internal.h
> > +++ b/drivers/gpu/drm/drm_crtc_internal.h
> > @@ -178,6 +178,7 @@ int drm_atomic_get_property(struct drm_mode_object *obj,
> >  			    struct drm_property *property, uint64_t *val);
> >  int drm_mode_atomic_ioctl(struct drm_device *dev,
> >  			  void *data, struct drm_file *file_priv);
> > +int drm_atomic_remove_fb(struct drm_framebuffer *fb);
> >  
> >  
> >  /* drm_plane.c */
> > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> > index cbf0c893f426..c358bf8280a8 100644
> > --- a/drivers/gpu/drm/drm_framebuffer.c
> > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > @@ -770,6 +770,12 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
> >  	 * in this manner.
> >  	 */
> >  	if (drm_framebuffer_read_refcount(fb) > 1) {
> > +		if (dev->mode_config.funcs->atomic_commit) {
> > +			int ret = drm_atomic_remove_fb(fb);
> > +			WARN(ret, "atomic remove_fb failed with %i\n", ret);
> > +			goto out;
> > +		}
> > +
> >  		drm_modeset_lock_all(dev);
> >  		/* remove from any CRTC */
> >  		drm_for_each_crtc(crtc, dev) {
> > @@ -787,6 +793,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
> >  		drm_modeset_unlock_all(dev);
> >  	}
> >  
> > +out:
> >  	drm_framebuffer_unreference(fb);
> >  }
> >  EXPORT_SYMBOL(drm_framebuffer_remove);
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel