Re: [PATCH 3/3] drm: Track framebuffer references at the point of assignment

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Nov 25, 2016 at 03:32:31PM +0000, Chris Wilson wrote:
> We can simplify the code greatly if both legacy and atomic paths updated
> the references as they assigned the framebuffer to the planes. Long
> before framebuffer reference counting was added, the code had to keep
> the old_fb around until after the operation was completed - but now we
> can simply leave it to the reference handling to prevent invalid use.
> 
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c |  2 +-
>  drivers/gpu/drm/armada/armada_crtc.c        |  9 +----
>  drivers/gpu/drm/bochs/bochs_kms.c           |  2 +-
>  drivers/gpu/drm/drm_atomic.c                | 44 +-------------------
>  drivers/gpu/drm/drm_atomic_helper.c         | 35 ++--------------
>  drivers/gpu/drm/drm_crtc.c                  | 18 +--------
>  drivers/gpu/drm/drm_crtc_helper.c           | 18 +++++----
>  drivers/gpu/drm/drm_fb_helper.c             | 23 +++++------
>  drivers/gpu/drm/drm_plane.c                 | 62 ++++++++++-------------------
>  drivers/gpu/drm/i915/intel_display.c        | 17 ++++----
>  drivers/gpu/drm/mgag200/mgag200_mode.c      |  2 +-
>  drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c   |  2 +-
>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c   |  2 +-
>  drivers/gpu/drm/nouveau/nouveau_display.c   |  2 +-
>  drivers/gpu/drm/nouveau/nv50_display.c      |  7 ----
>  drivers/gpu/drm/qxl/qxl_display.c           |  4 +-
>  drivers/gpu/drm/radeon/radeon_display.c     |  3 +-
>  drivers/gpu/drm/shmobile/shmob_drm_crtc.c   |  2 +-
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c        |  4 +-
>  drivers/gpu/drm/udl/udl_modeset.c           |  2 +-
>  drivers/gpu/drm/vc4/vc4_crtc.c              |  2 +-
>  drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c         |  4 +-
>  drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c        | 10 ++---
>  drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c        |  4 +-
>  include/drm/drm_atomic.h                    |  3 --
>  include/drm/drm_plane.h                     | 27 ++++++++++---
>  26 files changed, 100 insertions(+), 210 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index 741144fcc7bc..2aa4e707be1b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -225,7 +225,7 @@ int amdgpu_crtc_page_flip_target(struct drm_crtc *crtc,
>  	DRM_DEBUG_DRIVER("crtc:%d[%p], pflip_stat:AMDGPU_FLIP_PENDING, work: %p,\n",
>  					 amdgpu_crtc->crtc_id, amdgpu_crtc, work);
>  	/* update crtc fb */
> -	crtc->primary->fb = fb;
> +	drm_plane_set_fb(crtc->primary, fb);
>  	spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>  	amdgpu_flip_work_func(&work->flip_work.work);
>  	return 0;
> diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c
> index 95cb3966b2ca..1b8cc4dbe5a5 100644
> --- a/drivers/gpu/drm/armada/armada_crtc.c
> +++ b/drivers/gpu/drm/armada/armada_crtc.c
> @@ -1065,13 +1065,7 @@ static int armada_drm_crtc_page_flip(struct drm_crtc *crtc,
>  		return ret;
>  	}
>  
> -	/*
> -	 * Don't take a reference on the new framebuffer;
> -	 * drm_mode_page_flip_ioctl() has already grabbed a reference and
> -	 * will _not_ drop that reference on successful return from this
> -	 * function.  Simply mark this new framebuffer as the current one.
> -	 */
> -	dcrtc->crtc.primary->fb = fb;
> +	drm_plane_set_fb(dcrtc->crtc.primary, fb);
>  
>  	/*
>  	 * Finally, if the display is blanked, we won't receive an
> @@ -1080,6 +1074,7 @@ static int armada_drm_crtc_page_flip(struct drm_crtc *crtc,
>  	if (dpms_blanked(dcrtc->dpms))
>  		armada_drm_plane_work_run(dcrtc, dcrtc->crtc.primary);
>  
> +	drm_framebuffer_unreference(fb);
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c
> index 0b4e5d117043..afe3bd2cd79e 100644
> --- a/drivers/gpu/drm/bochs/bochs_kms.c
> +++ b/drivers/gpu/drm/bochs/bochs_kms.c
> @@ -103,7 +103,7 @@ static int bochs_crtc_page_flip(struct drm_crtc *crtc,
>  	struct drm_framebuffer *old_fb = crtc->primary->fb;
>  	unsigned long irqflags;
>  
> -	crtc->primary->fb = fb;
> +	drm_plane_set_fb(crtc->primary, fb);
>  	bochs_crtc_mode_set_base(crtc, 0, 0, old_fb);
>  	if (event) {
>  		spin_lock_irqsave(&bochs->dev->event_lock, irqflags);
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 19d7bcb88217..63dd5d5becdf 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1253,7 +1253,7 @@ drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
>  		DRM_DEBUG_ATOMIC("Set [NOFB] for plane state %p\n",
>  				 plane_state);
>  
> -	drm_framebuffer_assign(&plane_state->fb, fb);
> +	drm_framebuffer_assign(__mkwrite_drm_framebuffer(&plane_state->fb), fb);

Feels like const gone the wrong way round? Or I'm not entirely clear on
what this is supposed to achieve. Just dropping the const would still be a
nice improvement.
-Daniel

>  }
>  EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);
>  
> @@ -1774,45 +1774,6 @@ static int atomic_set_prop(struct drm_atomic_state *state,
>  }
>  
>  /**
> - * 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 plane->old_fb is set to 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_reference(new_fb);
> -			plane->fb = new_fb;
> -			plane->crtc = plane->state->crtc;
> -
> -			if (plane->old_fb)
> -				drm_framebuffer_unreference(plane->old_fb);
> -		}
> -		plane->old_fb = NULL;
> -	}
> -}
> -EXPORT_SYMBOL(drm_atomic_clean_old_fb);
> -
> -/**
>   * DOC: explicit fencing properties
>   *
>   * Explicit fencing allows userspace to control the buffer synchronization
> @@ -2148,7 +2109,6 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  		    !(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_unreference(obj);
>  	}
> @@ -2174,8 +2134,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 494680c9056e..05c7bbf2745e 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2052,6 +2052,9 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  	}
>  
>  	for_each_plane_in_state(state, plane, plane_state, i) {
> +		drm_plane_set_fb(plane, plane_state->fb);
> +		plane->crtc = plane_state->crtc;
> +
>  		plane->state->state = state;
>  		swap(state->planes[i].state, plane->state);
>  		plane->state->state = NULL;
> @@ -2129,14 +2132,6 @@ int drm_atomic_helper_update_plane(struct drm_plane *plane,
>  backoff:
>  	drm_atomic_state_clear(state);
>  	drm_atomic_legacy_backoff(state);
> -
> -	/*
> -	 * Someone might have exchanged the framebuffer while we dropped locks
> -	 * in the backoff code. We need to fix up the fb refcount tracking the
> -	 * core does for us.
> -	 */
> -	plane->old_fb = plane->fb;
> -
>  	goto retry;
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_update_plane);
> @@ -2197,14 +2192,6 @@ int drm_atomic_helper_disable_plane(struct drm_plane *plane)
>  backoff:
>  	drm_atomic_state_clear(state);
>  	drm_atomic_legacy_backoff(state);
> -
> -	/*
> -	 * Someone might have exchanged the framebuffer while we dropped locks
> -	 * in the backoff code. We need to fix up the fb refcount tracking the
> -	 * core does for us.
> -	 */
> -	plane->old_fb = plane->fb;
> -
>  	goto retry;
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_disable_plane);
> @@ -2332,14 +2319,6 @@ int drm_atomic_helper_set_config(struct drm_mode_set *set)
>  backoff:
>  	drm_atomic_state_clear(state);
>  	drm_atomic_legacy_backoff(state);
> -
> -	/*
> -	 * Someone might have exchanged the framebuffer while we dropped locks
> -	 * in the backoff code. We need to fix up the fb refcount tracking the
> -	 * core does for us.
> -	 */
> -	crtc->primary->old_fb = crtc->primary->fb;
> -
>  	goto retry;
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_set_config);
> @@ -2810,14 +2789,6 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
>  backoff:
>  	drm_atomic_state_clear(state);
>  	drm_atomic_legacy_backoff(state);
> -
> -	/*
> -	 * Someone might have exchanged the framebuffer while we dropped locks
> -	 * in the backoff code. We need to fix up the fb refcount tracking the
> -	 * core does for us.
> -	 */
> -	plane->old_fb = plane->fb;
> -
>  	goto retry;
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_page_flip);
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 90931e039731..f94550a40ec9 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -385,8 +385,6 @@ int drm_mode_getcrtc(struct drm_device *dev,
>  int drm_mode_set_config_internal(struct drm_mode_set *set)
>  {
>  	struct drm_crtc *crtc = set->crtc;
> -	struct drm_framebuffer *fb;
> -	struct drm_crtc *tmp;
>  	int ret;
>  
>  	/*
> @@ -394,24 +392,10 @@ int drm_mode_set_config_internal(struct drm_mode_set *set)
>  	 * connectors from it), hence we need to refcount the fbs across all
>  	 * crtcs. Atomic modeset will have saner semantics ...
>  	 */
> -	drm_for_each_crtc(tmp, crtc->dev)
> -		tmp->primary->old_fb = tmp->primary->fb;
> -
> -	fb = set->fb;
>  
>  	ret = crtc->funcs->set_config(set);
> -	if (ret == 0) {
> +	if (ret == 0)
>  		crtc->primary->crtc = crtc;
> -		crtc->primary->fb = fb;
> -	}
> -
> -	drm_for_each_crtc(tmp, crtc->dev) {
> -		if (tmp->primary->fb)
> -			drm_framebuffer_reference(tmp->primary->fb);
> -		if (tmp->primary->old_fb)
> -			drm_framebuffer_unreference(tmp->primary->old_fb);
> -		tmp->primary->old_fb = NULL;
> -	}
>  
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index 5d2cb138eba6..9818fd8c5988 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -177,7 +177,7 @@ static void __drm_helper_disable_unused_functions(struct drm_device *dev)
>  				(*crtc_funcs->disable)(crtc);
>  			else
>  				(*crtc_funcs->dpms)(crtc, DRM_MODE_DPMS_OFF);
> -			crtc->primary->fb = NULL;
> +			drm_plane_set_fb(crtc->primary, NULL);
>  		}
>  	}
>  }
> @@ -579,7 +579,7 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
>  	save_set.mode = &set->crtc->mode;
>  	save_set.x = set->crtc->x;
>  	save_set.y = set->crtc->y;
> -	save_set.fb = set->crtc->primary->fb;
> +	save_set.fb = drm_plane_get_fb(set->crtc->primary);
>  
>  	/* We should be able to check here if the fb has the same properties
>  	 * and then just flip_or_move it */
> @@ -700,13 +700,14 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
>  			DRM_DEBUG_KMS("attempting to set mode from"
>  					" userspace\n");
>  			drm_mode_debug_printmodeline(set->mode);
> -			set->crtc->primary->fb = set->fb;
> +			drm_plane_set_fb(set->crtc->primary, set->fb);
>  			if (!drm_crtc_helper_set_mode(set->crtc, set->mode,
>  						      set->x, set->y,
>  						      save_set.fb)) {
>  				DRM_ERROR("failed to set mode on [CRTC:%d:%s]\n",
>  					  set->crtc->base.id, set->crtc->name);
> -				set->crtc->primary->fb = save_set.fb;
> +				drm_plane_set_fb(set->crtc->primary,
> +						 save_set.fb);
>  				ret = -EINVAL;
>  				goto fail;
>  			}
> @@ -721,17 +722,18 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
>  	} else if (fb_changed) {
>  		set->crtc->x = set->x;
>  		set->crtc->y = set->y;
> -		set->crtc->primary->fb = set->fb;
> +		drm_plane_set_fb(set->crtc->primary, set->fb);
>  		ret = crtc_funcs->mode_set_base(set->crtc,
>  						set->x, set->y, save_set.fb);
>  		if (ret != 0) {
>  			set->crtc->x = save_set.x;
>  			set->crtc->y = save_set.y;
> -			set->crtc->primary->fb = save_set.fb;
> +			drm_plane_set_fb(set->crtc->primary, save_set.fb);
>  			goto fail;
>  		}
>  	}
>  
> +	drm_framebuffer_unreference(save_set.fb);
>  	kfree(save_connector_encoders);
>  	kfree(save_encoder_crtcs);
>  	return 0;
> @@ -763,6 +765,7 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
>  				      save_set.y, save_set.fb))
>  		DRM_ERROR("failed to restore config after modeset failure\n");
>  
> +	drm_framebuffer_unreference(save_set.fb);
>  	kfree(save_connector_encoders);
>  	kfree(save_encoder_crtcs);
>  	return ret;
> @@ -924,7 +927,8 @@ void drm_helper_resume_force_mode(struct drm_device *dev)
>  			continue;
>  
>  		ret = drm_crtc_helper_set_mode(crtc, &crtc->mode,
> -					       crtc->x, crtc->y, crtc->primary->fb);
> +					       crtc->x, crtc->y,
> +					       crtc->primary->fb);
>  
>  		/* Restoring the old config should never fail! */
>  		if (ret == false)
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 62641d59a2d7..851a7e30444b 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -285,7 +285,7 @@ static struct drm_framebuffer *drm_mode_config_fb(struct drm_crtc *crtc)
>  
>  	drm_for_each_crtc(c, dev) {
>  		if (crtc->base.id == c->base.id)
> -			return c->primary->fb;
> +			return drm_plane_get_fb(c->primary);
>  	}
>  
>  	return NULL;
> @@ -305,24 +305,27 @@ int drm_fb_helper_debug_leave(struct fb_info *info)
>  
>  	for (i = 0; i < helper->crtc_count; i++) {
>  		struct drm_mode_set *mode_set = &helper->crtc_info[i].mode_set;
> -		crtc = mode_set->crtc;
> -		funcs = crtc->helper_private;
> -		fb = drm_mode_config_fb(crtc);
>  
> +		crtc = mode_set->crtc;
>  		if (!crtc->enabled)
>  			continue;
>  
> +		funcs = crtc->helper_private;
> +		if (funcs->mode_set_base_atomic == NULL)
> +			continue;
> +
> +		fb = drm_mode_config_fb(crtc);
>  		if (!fb) {
>  			DRM_ERROR("no fb to restore??\n");
>  			continue;
>  		}
>  
> -		if (funcs->mode_set_base_atomic == NULL)
> -			continue;
> -
>  		drm_fb_helper_restore_lut_atomic(mode_set->crtc);
> +
>  		funcs->mode_set_base_atomic(mode_set->crtc, fb, crtc->x,
>  					    crtc->y, LEAVE_ATOMIC_MODE_SET);
> +
> +		drm_framebuffer_unreference(fb);
>  	}
>  
>  	return 0;
> @@ -355,7 +358,6 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper)
>  
>  		plane_state->rotation = DRM_ROTATE_0;
>  
> -		plane->old_fb = plane->fb;
>  		plane_mask |= 1 << drm_plane_index(plane);
>  
>  		/* disable non-primary: */
> @@ -378,8 +380,6 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper)
>  	ret = drm_atomic_commit(state);
>  
>  fail:
> -	drm_atomic_clean_old_fb(dev, plane_mask, ret);
> -
>  	if (ret == -EDEADLK)
>  		goto backoff;
>  
> @@ -1391,7 +1391,6 @@ static int pan_display_atomic(struct fb_var_screeninfo *var,
>  
>  		plane = mode_set->crtc->primary;
>  		plane_mask |= (1 << drm_plane_index(plane));
> -		plane->old_fb = plane->fb;
>  	}
>  
>  	ret = drm_atomic_commit(state);
> @@ -1402,8 +1401,6 @@ static int pan_display_atomic(struct fb_var_screeninfo *var,
>  	info->var.yoffset = var->yoffset;
>  
>  fail:
> -	drm_atomic_clean_old_fb(dev, plane_mask, ret);
> -
>  	if (ret == -EDEADLK)
>  		goto backoff;
>  
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 419ac313c36f..b424a5b40f5e 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -285,17 +285,13 @@ void drm_plane_force_disable(struct drm_plane *plane)
>  	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(plane->old_fb);
> -	plane->old_fb = NULL;
> -	plane->fb = NULL;
> +	drm_plane_set_fb(plane, NULL);
>  	plane->crtc = NULL;
>  }
>  EXPORT_SYMBOL(drm_plane_force_disable);
> @@ -459,13 +455,10 @@ static int __setplane_internal(struct drm_plane *plane,
>  
>  	/* No fb means shut it down */
>  	if (!fb) {
> -		plane->old_fb = plane->fb;
>  		ret = plane->funcs->disable_plane(plane);
>  		if (!ret) {
> +			drm_plane_set_fb(plane, NULL);
>  			plane->crtc = NULL;
> -			plane->fb = NULL;
> -		} else {
> -			plane->old_fb = NULL;
>  		}
>  		goto out;
>  	}
> @@ -502,25 +495,15 @@ static int __setplane_internal(struct drm_plane *plane,
>  	if (ret)
>  		goto out;
>  
> -	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);
>  	if (!ret) {
>  		plane->crtc = crtc;
> -		plane->fb = fb;
> -		fb = NULL;
> -	} else {
> -		plane->old_fb = NULL;
> +		drm_plane_set_fb(plane, fb);
>  	}
>  
>  out:
> -	if (fb)
> -		drm_framebuffer_unreference(fb);
> -	if (plane->old_fb)
> -		drm_framebuffer_unreference(plane->old_fb);
> -	plane->old_fb = NULL;
> -
>  	return ret;
>  }
>  
> @@ -551,6 +534,7 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>  	struct drm_plane *plane;
>  	struct drm_crtc *crtc = NULL;
>  	struct drm_framebuffer *fb = NULL;
> +	int err;
>  
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EINVAL;
> @@ -586,11 +570,16 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>  	 * setplane_internal will take care of deref'ing either the old or new
>  	 * framebuffer depending on success.
>  	 */
> -	return setplane_internal(plane, crtc, fb,
> -				 plane_req->crtc_x, plane_req->crtc_y,
> -				 plane_req->crtc_w, plane_req->crtc_h,
> -				 plane_req->src_x, plane_req->src_y,
> -				 plane_req->src_w, plane_req->src_h);
> +	err = setplane_internal(plane, crtc, fb,
> +				plane_req->crtc_x, plane_req->crtc_y,
> +				plane_req->crtc_w, plane_req->crtc_h,
> +				plane_req->src_x, plane_req->src_y,
> +				plane_req->src_w, plane_req->src_h);
> +
> +	if (fb)
> +		drm_framebuffer_unreference(fb);
> +
> +	return err;
>  }
>  
>  static int drm_mode_cursor_universal(struct drm_crtc *crtc,
> @@ -852,19 +841,19 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  		ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode, fb);
>  	}
>  	if (ret)
> -		goto out;
> +		goto out_fb;
>  
>  	if (crtc->primary->fb->pixel_format != fb->pixel_format) {
>  		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
>  		ret = -EINVAL;
> -		goto out;
> +		goto out_fb;
>  	}
>  
>  	if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) {
>  		e = kzalloc(sizeof *e, GFP_KERNEL);
>  		if (!e) {
>  			ret = -ENOMEM;
> -			goto out;
> +			goto out_fb;
>  		}
>  		e->event.base.type = DRM_EVENT_FLIP_COMPLETE;
>  		e->event.base.length = sizeof(e->event);
> @@ -872,11 +861,10 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  		ret = drm_event_reserve_init(dev, file_priv, &e->base, &e->event.base);
>  		if (ret) {
>  			kfree(e);
> -			goto out;
> +			goto out_fb;
>  		}
>  	}
>  
> -	crtc->primary->old_fb = crtc->primary->fb;
>  	if (crtc->funcs->page_flip_target)
>  		ret = crtc->funcs->page_flip_target(crtc, fb, e,
>  						    page_flip->flags,
> @@ -886,23 +874,13 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	if (ret) {
>  		if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT)
>  			drm_event_cancel_free(dev, &e->base);
> -		/* Keep the old fb, don't unref it. */
> -		crtc->primary->old_fb = NULL;
> -	} else {
> -		crtc->primary->fb = fb;
> -		/* Unref only the old framebuffer. */
> -		fb = NULL;
>  	}
>  
> +out_fb:
> +	drm_framebuffer_unreference(fb);
>  out:
>  	if (ret && crtc->funcs->page_flip_target)
>  		drm_crtc_vblank_put(crtc);
> -	if (fb)
> -		drm_framebuffer_unreference(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/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8630de472f9a..b887b7caf1d1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2818,12 +2818,15 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
>  	if (i915_gem_object_is_tiled(obj))
>  		dev_priv->preserve_bios_swizzle = true;
>  
> -	drm_framebuffer_reference(fb);
> -	primary->fb = primary->state->fb = fb;
> +	drm_plane_set_fb(primary, fb);
> +	update_state_fb(primary);
> +
>  	primary->crtc = primary->state->crtc = &intel_crtc->base;
>  	intel_crtc->base.state->plane_mask |= (1 << drm_plane_index(primary));
>  	atomic_or(to_intel_plane(primary)->frontbuffer_bit,
>  		  &obj->frontbuffer_bits);
> +
> +	drm_framebuffer_unreference(fb);
>  }
>  
>  static int skl_max_plane_width(const struct drm_framebuffer *fb, int plane,
> @@ -12191,7 +12194,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	/* Reference the objects for the scheduled work. */
>  	drm_framebuffer_reference(work->old_fb);
>  
> -	crtc->primary->fb = fb;
> +	drm_plane_set_fb(crtc->primary, fb);
>  	update_state_fb(crtc->primary);
>  
>  	work->pending_flip_obj = i915_gem_object_get(obj);
> @@ -12293,7 +12296,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	atomic_dec(&intel_crtc->unpin_work_count);
>  	mutex_unlock(&dev->struct_mutex);
>  cleanup:
> -	crtc->primary->fb = old_fb;
> +	drm_plane_set_fb(crtc->primary, old_fb);
>  	update_state_fb(crtc->primary);
>  
>  	i915_gem_object_put(obj);
> @@ -13080,7 +13083,6 @@ intel_modeset_update_crtc_state(struct drm_atomic_state *state)
>  		if (drm_atomic_get_existing_plane_state(state, crtc->primary)) {
>  			struct drm_plane_state *plane_state = crtc->primary->state;
>  
> -			crtc->primary->fb = plane_state->fb;
>  			crtc->x = plane_state->src_x >> 16;
>  			crtc->y = plane_state->src_y >> 16;
>  		}
> @@ -17093,10 +17095,9 @@ void intel_modeset_gem_init(struct drm_device *dev)
>  		if (IS_ERR(vma)) {
>  			DRM_ERROR("failed to pin boot fb on pipe %d\n",
>  				  to_intel_crtc(c)->pipe);
> -			drm_framebuffer_unreference(c->primary->fb);
> -			c->primary->fb = NULL;
> -			c->primary->crtc = c->primary->state->crtc = NULL;
> +			drm_plane_set_fb(c->primary, NULL);
>  			update_state_fb(c->primary);
> +			c->primary->crtc = c->primary->state->crtc = NULL;
>  			c->state->plane_mask &= ~(1 << drm_plane_index(c->primary));
>  		}
>  	}
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index 6b21cb27e1cc..a2d3ba237f45 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -1392,7 +1392,7 @@ static void mga_crtc_disable(struct drm_crtc *crtc)
>  		mgag200_bo_push_sysram(bo);
>  		mgag200_bo_unreserve(bo);
>  	}
> -	crtc->primary->fb = NULL;
> +	drm_plane_set_fb(crtc->primary, NULL);
>  }
>  
>  /* These provide the minimum set of functions required to handle a CRTC */
> diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
> index 911e4690d36a..5c902561c715 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
> @@ -180,7 +180,7 @@ static void mdp4_plane_set_scanout(struct drm_plane *plane,
>  	mdp4_write(mdp4_kms, REG_MDP4_PIPE_SRCP3_BASE(pipe),
>  			msm_framebuffer_iova(fb, mdp4_kms->id, 3));
>  
> -	plane->fb = fb;
> +	drm_plane_set_fb(plane, fb);
>  }
>  
>  static void mdp4_write_csc_config(struct mdp4_kms *mdp4_kms,
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> index 81c0562ab489..0191ecbcae0b 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> @@ -425,7 +425,7 @@ static void set_scanout_locked(struct drm_plane *plane,
>  	mdp5_write(mdp5_kms, REG_MDP5_PIPE_SRC3_ADDR(pipe),
>  			msm_framebuffer_iova(fb, mdp5_kms->id, 3));
>  
> -	plane->fb = fb;
> +	drm_plane_set_fb(plane, fb);
>  }
>  
>  /* Note: mdp5_plane->pipe_lock must be locked */
> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
> index bd37ae127582..7979617f5709 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> @@ -977,7 +977,7 @@ nouveau_crtc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb,
>  	mutex_unlock(&cli->mutex);
>  
>  	/* Update the crtc struct and cleanup */
> -	crtc->primary->fb = fb;
> +	drm_plane_set_fb(crtc->primary, fb);
>  
>  	nouveau_bo_fence(old_bo, fence, false);
>  	ttm_bo_unreserve(&old_bo->bo);
> diff --git a/drivers/gpu/drm/nouveau/nv50_display.c b/drivers/gpu/drm/nouveau/nv50_display.c
> index 22a8b70a4d1e..b098b6d04595 100644
> --- a/drivers/gpu/drm/nouveau/nv50_display.c
> +++ b/drivers/gpu/drm/nouveau/nv50_display.c
> @@ -2277,13 +2277,6 @@ nv50_head_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb,
>  	drm_atomic_state_clear(state);
>  	drm_atomic_legacy_backoff(state);
>  
> -	/*
> -	 * Someone might have exchanged the framebuffer while we dropped locks
> -	 * in the backoff code. We need to fix up the fb refcount tracking the
> -	 * core does for us.
> -	 */
> -	plane->old_fb = plane->fb;
> -
>  	goto retry;
>  }
>  
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
> index a61c0d460ec2..2e01c6e5d905 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -237,7 +237,6 @@ static int qxl_crtc_page_flip(struct drm_crtc *crtc,
>  	int one_clip_rect = 1;
>  	int ret = 0;
>  
> -	crtc->primary->fb = fb;
>  	bo_old->is_primary = false;
>  	bo->is_primary = true;
>  
> @@ -249,6 +248,7 @@ static int qxl_crtc_page_flip(struct drm_crtc *crtc,
>  	if (ret)
>  		return ret;
>  
> +	drm_plane_set_fb(crtc->primary, fb);
>  	qxl_draw_dirty_fb(qdev, qfb_src, bo, 0, 0,
>  			  &norect, one_clip_rect, inc);
>  
> @@ -755,7 +755,7 @@ static void qxl_crtc_disable(struct drm_crtc *crtc)
>  		ret = qxl_bo_reserve(bo, false);
>  		qxl_bo_unpin(bo);
>  		qxl_bo_unreserve(bo);
> -		crtc->primary->fb = NULL;
> +		drm_plane_set_fb(crtc->primary, NULL);
>  	}
>  
>  	qxl_monitors_config_set(qdev, qcrtc->index, 0, 0, 0, 0, 0);
> diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
> index e7409e8a9f87..1eb004dc4cd6 100644
> --- a/drivers/gpu/drm/radeon/radeon_display.c
> +++ b/drivers/gpu/drm/radeon/radeon_display.c
> @@ -598,8 +598,7 @@ static int radeon_crtc_page_flip_target(struct drm_crtc *crtc,
>  	radeon_crtc->flip_work = work;
>  
>  	/* update crtc fb */
> -	crtc->primary->fb = fb;
> -
> +	drm_plane_set_fb(crtc->primary, fb);
>  	spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>  
>  	queue_work(radeon_crtc->flip_queue, &work->flip_work);
> diff --git a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
> index 6547b1db460a..fc5df59976cd 100644
> --- a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
> +++ b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
> @@ -462,7 +462,7 @@ static int shmob_drm_crtc_page_flip(struct drm_crtc *crtc,
>  	}
>  	spin_unlock_irqrestore(&dev->event_lock, flags);
>  
> -	crtc->primary->fb = fb;
> +	drm_plane_set_fb(crtc->primary, fb);
>  	shmob_drm_crtc_update_base(scrtc);
>  
>  	if (event) {
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 822531ebd4b0..f57154b69e9d 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -260,9 +260,7 @@ int tilcdc_crtc_update_fb(struct drm_crtc *crtc,
>  		return -EBUSY;
>  	}
>  
> -	drm_framebuffer_reference(fb);
> -
> -	crtc->primary->fb = fb;
> +	drm_plane_set_fb(crtc->primary, fb);
>  
>  	spin_lock_irqsave(&tilcdc_crtc->irq_lock, flags);
>  
> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> index f2b2481cad52..05133ef8942c 100644
> --- a/drivers/gpu/drm/udl/udl_modeset.c
> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> @@ -380,7 +380,7 @@ static int udl_crtc_page_flip(struct drm_crtc *crtc,
>  	if (event)
>  		drm_crtc_send_vblank_event(crtc, event);
>  	spin_unlock_irqrestore(&dev->event_lock, flags);
> -	crtc->primary->fb = fb;
> +	drm_plane_set_fb(crtc->primary, fb);
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index 7f08d681a74b..b13597b3f699 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -785,7 +785,7 @@ static int vc4_async_page_flip(struct drm_crtc *crtc,
>  	 * is released.
>  	 */
>  	drm_atomic_set_fb_for_plane(plane->state, fb);
> -	plane->fb = fb;
> +	drm_plane_set_fb(plane, fb);
>  
>  	vc4_queue_seqno_cb(dev, &flip_state->cb, bo->seqno,
>  			   vc4_async_page_flip_complete);
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
> index 23ec673d5e16..61a3cce08dfe 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
> @@ -260,7 +260,7 @@ static int vmw_ldu_crtc_set_config(struct drm_mode_set *set)
>  
>  		connector->encoder = NULL;
>  		encoder->crtc = NULL;
> -		crtc->primary->fb = NULL;
> +		drm_plane_set_fb(crtc->primary, NULL);
>  		crtc->enabled = false;
>  
>  		vmw_ldu_del_active(dev_priv, ldu);
> @@ -281,7 +281,7 @@ static int vmw_ldu_crtc_set_config(struct drm_mode_set *set)
>  
>  	vmw_svga_enable(dev_priv);
>  
> -	crtc->primary->fb = fb;
> +	drm_plane_set_fb(crtc->primary, fb);
>  	encoder->crtc = crtc;
>  	connector->encoder = encoder;
>  	crtc->x = set->x;
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> index f42359084adc..573c7407c32c 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> @@ -310,7 +310,7 @@ static int vmw_sou_crtc_set_config(struct drm_mode_set *set)
>  
>  		connector->encoder = NULL;
>  		encoder->crtc = NULL;
> -		crtc->primary->fb = NULL;
> +		drm_plane_set_fb(crtc->primary, NULL);
>  		crtc->x = 0;
>  		crtc->y = 0;
>  		crtc->enabled = false;
> @@ -371,7 +371,7 @@ static int vmw_sou_crtc_set_config(struct drm_mode_set *set)
>  
>  		connector->encoder = NULL;
>  		encoder->crtc = NULL;
> -		crtc->primary->fb = NULL;
> +		drm_plane_set_fb(crtc->primary, NULL);
>  		crtc->x = 0;
>  		crtc->y = 0;
>  		crtc->enabled = false;
> @@ -384,7 +384,7 @@ static int vmw_sou_crtc_set_config(struct drm_mode_set *set)
>  	connector->encoder = encoder;
>  	encoder->crtc = crtc;
>  	crtc->mode = *mode;
> -	crtc->primary->fb = fb;
> +	drm_plane_set_fb(crtc->primary, fb);
>  	crtc->x = set->x;
>  	crtc->y = set->y;
>  	crtc->enabled = true;
> @@ -407,7 +407,7 @@ static int vmw_sou_crtc_page_flip(struct drm_crtc *crtc,
>  	if (!vmw_kms_crtc_flippable(dev_priv, crtc))
>  		return -EINVAL;
>  
> -	crtc->primary->fb = fb;
> +	drm_plane_set_fb(crtc->primary, fb);
>  
>  	/* do a full screen dirty update */
>  	vclips.x = crtc->x;
> @@ -454,7 +454,7 @@ static int vmw_sou_crtc_page_flip(struct drm_crtc *crtc,
>  	return ret;
>  
>  out_no_fence:
> -	crtc->primary->fb = old_fb;
> +	drm_plane_set_fb(crtc->primary, old_fb);
>  	return ret;
>  }
>  
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> index 94ad8d2acf9a..01c4d574fa78 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> @@ -486,7 +486,7 @@ static int vmw_stdu_bind_fb(struct vmw_private *dev_priv,
>  		new_display_srf = NULL;
>  	}
>  
> -	crtc->primary->fb = new_fb;
> +	drm_plane_set_fb(crtc->primary, new_fb);
>  	stdu->content_fb_type = new_content_type;
>  	return 0;
>  
> @@ -580,7 +580,7 @@ static int vmw_stdu_crtc_set_config(struct drm_mode_set *set)
>  		if (ret)
>  			return ret;
>  
> -		crtc->primary->fb = NULL;
> +		drm_plane_set_fb(crtc->primary, NULL);
>  		crtc->enabled = false;
>  		encoder->crtc = NULL;
>  		connector->encoder = NULL;
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 5d5f85db43f0..fb616039a2bf 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -360,9 +360,6 @@ drm_atomic_add_affected_planes(struct drm_atomic_state *state,
>  
>  void drm_atomic_legacy_backoff(struct drm_atomic_state *state);
>  
> -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);
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 5b38eb94783b..eacb1124616b 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -72,7 +72,7 @@ struct drm_plane_state {
>  	 * Currently bound framebuffer. Do not write this directly, use
>  	 * drm_atomic_set_fb_for_plane()
>  	 */
> -	struct drm_framebuffer *fb;
> +	struct drm_framebuffer * const fb;
>  
>  	/**
>  	 * @fence:
> @@ -451,8 +451,6 @@ enum drm_plane_type {
>   * @format_default: driver hasn't supplied supported formats for the plane
>   * @crtc: currently bound CRTC
>   * @fb: currently bound fb
> - * @old_fb: Temporary tracking of the old fb while a modeset is ongoing. Used by
> - * 	drm_mode_set_config_internal() to implement correct refcounting.
>   * @funcs: helper functions
>   * @properties: property tracking for this plane
>   * @type: type of plane (overlay, primary, cursor)
> @@ -484,9 +482,7 @@ struct drm_plane {
>  	bool format_default;
>  
>  	struct drm_crtc *crtc;
> -	struct drm_framebuffer *fb;
> -
> -	struct drm_framebuffer *old_fb;
> +	struct drm_framebuffer * const fb;
>  
>  	const struct drm_plane_funcs *funcs;
>  
> @@ -596,5 +592,24 @@ static inline struct drm_plane *drm_plane_find(struct drm_device *dev,
>  #define drm_for_each_plane(plane, dev) \
>  	list_for_each_entry(plane, &(dev)->mode_config.plane_list, head)
>  
> +#define __mkwrite_drm_framebuffer(fb__) (struct drm_framebuffer **)(fb__)
> +
> +static inline void drm_plane_set_fb(struct drm_plane *plane,
> +				    struct drm_framebuffer *fb)
> +{
> +	if (plane->fb == fb)
> +		return;
> +
> +	drm_framebuffer_assign(__mkwrite_drm_framebuffer(&plane->fb), fb);
> +}
> +
> +static inline struct drm_framebuffer *
> +drm_plane_get_fb(struct drm_plane *plane)
> +{
> +	struct drm_framebuffer *fb = plane->fb;
> +	if (fb)
> +		drm_framebuffer_reference(fb);
> +	return fb;
> +}
>  
>  #endif
> -- 
> 2.10.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux