Re: [PATCH 4/8] drm: Move ->old_fb from crtc to plane

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jul 29, 2014 at 04:46:11PM -0700, Matt Roper wrote:
> On Tue, Jul 29, 2014 at 11:32:19PM +0200, Daniel Vetter wrote:
> > Atomic implemenations for legacy ioctls must be able to drop locks.
> > Which doesn't cause havoc since we only do that while constructing
> > the new state, so no driver or hardware state change has happened.
> > 
> > The only troubling bit is the fb refcounting the core does - if
> > someone else has snuck in then it might potentially unref an
> > outdated framebuffer. To fix that move the old_fb temporary storage
> > into struct drm_plane for all ioctls, so that the atomic helpers can
> > update it.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/drm_crtc.c | 40 ++++++++++++++++++++++++----------------
> >  include/drm/drm_crtc.h     |  8 ++++----
> >  2 files changed, 28 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index c09374038f9a..bacf565449d5 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -1200,19 +1200,21 @@ EXPORT_SYMBOL(drm_plane_index);
> >   */
> >  void drm_plane_force_disable(struct drm_plane *plane)
> >  {
> > -	struct drm_framebuffer *old_fb = plane->fb;
> >  	int ret;
> >  
> > -	if (!old_fb)
> > +	if (!plane->fb)
> >  		return;
> >  
> > +	plane->old_fb = plane->fb;
> >  	ret = plane->funcs->disable_plane(plane);
> >  	if (ret) {
> >  		DRM_ERROR("failed to disable plane with busy fb\n");
> > +		plane->old_fb = NULL;
> >  		return;
> >  	}
> >  	/* disconnect the plane from the fb and crtc: */
> > -	__drm_framebuffer_unreference(old_fb);
> > +	__drm_framebuffer_unreference(plane->old_fb);
> > +	plane->old_fb = NULL;
> >  	plane->fb = NULL;
> >  	plane->crtc = NULL;
> >  }
> > @@ -2188,7 +2190,7 @@ static int setplane_internal(struct drm_plane *plane,
> >  			     uint32_t src_w, uint32_t src_h)
> >  {
> >  	struct drm_device *dev = plane->dev;
> > -	struct drm_framebuffer *old_fb = NULL;
> > +	struct drm_framebuffer *old_fb;
> 
> I think there may be cases where old_fb gets unref'd without ever being
> set if we drop the NULL assignment.  E.g., if the possible_crtcs test or
> the format test fail, we jump down to out and then test the value +
> unref which could be garbage.

Oops, totally missed that. And somehow also missed the gcc warning about
unitialized usage of old_fb - that one was the reason why I've dropped the
initializer. Looks like I've failed.

> Would it be simpler to just drm_modeset_lock_all() unconditionally at
> the start of the function and then just unlock after the unrefs at the
> end of the function so that we don't need a local old_fb?

Yeah considered that and since you're suggesting this too I'll do it.
Trying hard to not grab locks for the error case is fairly pointless
optimization.

> 
> >  	int ret = 0;
> >  	unsigned int fb_width, fb_height;
> >  	int i;
> > @@ -2196,14 +2198,16 @@ static int setplane_internal(struct drm_plane *plane,
> >  	/* No fb means shut it down */
> >  	if (!fb) {
> >  		drm_modeset_lock_all(dev);
> > -		old_fb = plane->fb;
> > +		plane->old_fb = plane->fb;
> >  		ret = plane->funcs->disable_plane(plane);
> >  		if (!ret) {
> >  			plane->crtc = NULL;
> >  			plane->fb = NULL;
> >  		} else {
> > -			old_fb = NULL;
> > +			plane->old_fb = NULL;
> >  		}
> > +		old_fb = plane->old_fb;
> > +		plane->old_fb = NULL;
> >  		drm_modeset_unlock_all(dev);
> >  		goto out;
> >  	}
> > @@ -2245,7 +2249,7 @@ static int setplane_internal(struct drm_plane *plane,
> >  	}
> >  
> >  	drm_modeset_lock_all(dev);
> > -	old_fb = plane->fb;
> > +	plane->old_fb = plane->fb;
> >  	ret = plane->funcs->update_plane(plane, crtc, fb,
> >  					 crtc_x, crtc_y, crtc_w, crtc_h,
> >  					 src_x, src_y, src_w, src_h);
> > @@ -2254,8 +2258,10 @@ static int setplane_internal(struct drm_plane *plane,
> >  		plane->fb = fb;
> >  		fb = NULL;
> >  	} else {
> > -		old_fb = NULL;
> > +		plane->old_fb = NULL;
> >  	}
> > +	old_fb = plane->old_fb;
> > +	plane->old_fb = NULL;
> >  	drm_modeset_unlock_all(dev);
> >  
> >  out:
> > @@ -2369,7 +2375,7 @@ int drm_mode_set_config_internal(struct drm_mode_set *set)
> >  	 * crtcs. Atomic modeset will have saner semantics ...
> >  	 */
> >  	list_for_each_entry(tmp, &crtc->dev->mode_config.crtc_list, head)
> > -		tmp->old_fb = tmp->primary->fb;
> > +		tmp->primary->old_fb = tmp->primary->fb;
> >  
> >  	fb = set->fb;
> >  
> > @@ -2382,8 +2388,9 @@ int drm_mode_set_config_internal(struct drm_mode_set *set)
> >  	list_for_each_entry(tmp, &crtc->dev->mode_config.crtc_list, head) {
> >  		if (tmp->primary->fb)
> >  			drm_framebuffer_reference(tmp->primary->fb);
> > -		if (tmp->old_fb)
> > -			drm_framebuffer_unreference(tmp->old_fb);
> > +		if (tmp->primary->old_fb)
> > +			drm_framebuffer_unreference(tmp->primary->old_fb);
> > +		tmp->primary->old_fb = NULL;
> >  	}
> >  
> >  	return ret;
> > @@ -4458,7 +4465,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >  {
> >  	struct drm_mode_crtc_page_flip *page_flip = data;
> >  	struct drm_crtc *crtc;
> > -	struct drm_framebuffer *fb = NULL, *old_fb = NULL;
> > +	struct drm_framebuffer *fb = NULL;
> >  	struct drm_pending_vblank_event *e = NULL;
> >  	unsigned long flags;
> >  	int ret = -EINVAL;
> > @@ -4530,7 +4537,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >  			(void (*) (struct drm_pending_event *)) kfree;
> >  	}
> >  
> > -	old_fb = crtc->primary->fb;
> > +	crtc->primary->old_fb = crtc->primary->fb;
> >  	ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags);
> >  	if (ret) {
> >  		if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> > @@ -4540,7 +4547,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >  			kfree(e);
> >  		}
> >  		/* Keep the old fb, don't unref it. */
> > -		old_fb = NULL;
> > +		crtc->primary->old_fb = NULL;
> >  	} else {
> >  		/*
> >  		 * Warn if the driver hasn't properly updated the crtc->fb
> > @@ -4556,8 +4563,9 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >  out:
> >  	if (fb)
> >  		drm_framebuffer_unreference(fb);
> > -	if (old_fb)
> > -		drm_framebuffer_unreference(old_fb);
> > +	if (crtc->primary->old_fb)
> > +		drm_framebuffer_unreference(crtc->primary->old_fb);
> > +	crtc->primary->old_fb = NULL;
> >  	drm_modeset_unlock_crtc(crtc);
> >  
> >  	return ret;
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index b0e30c5526ce..5fffb5c53ba6 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -341,10 +341,6 @@ struct drm_crtc {
> >  	int cursor_x;
> >  	int cursor_y;
> >  
> > -	/* Temporary tracking of the old fb while a modeset is ongoing. Used
> > -	 * by drm_mode_set_config_internal to implement correct refcounting. */
> > -	struct drm_framebuffer *old_fb;
> > -
> >  	bool enabled;
> >  
> >  	/* Requested mode from modesetting. */
> > @@ -622,6 +618,10 @@ struct drm_plane {
> >  	struct drm_crtc *crtc;
> >  	struct drm_framebuffer *fb;
> >  
> > +	/* Temporary tracking of the old fb while a modeset is ongoing. Used
> > +	 * by drm_mode_set_config_internal to implement correct refcounting. */
> 
> Might want to update the wording of this comment slightly since it isn't
> just for drm_mode_set_config_internal (or modesets) anymore.

Good idea, will augment.
-Daniel

> 
> 
> 
> Matt
> 
> > +	struct drm_framebuffer *old_fb;
> > +
> >  	const struct drm_plane_funcs *funcs;
> >  
> >  	struct drm_object_properties properties;
> > -- 
> > 2.0.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch