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 <ch...@chris-wilson.co.uk>
> ---
> 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-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
