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

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

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.

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?

>  	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.



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