| Message ID | 1463490484-19540-19-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 17, 2016 at 03:08:01PM +0200, Maarten Lankhorst wrote: > All of intel_post_plane_update is handled there now, so move it over. > This is run after the hw state checker because it can't handle checking > crtc's separately yet. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Reviewed-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_atomic.c | 11 ++ > drivers/gpu/drm/i915/intel_display.c | 344 ++++++++++++++++++++++------------- > drivers/gpu/drm/i915/intel_drv.h | 5 +- > 3 files changed, 228 insertions(+), 132 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c > index 50ff90aea721..b4927f6bbeac 100644 > --- a/drivers/gpu/drm/i915/intel_atomic.c > +++ b/drivers/gpu/drm/i915/intel_atomic.c > @@ -311,6 +311,17 @@ intel_atomic_state_alloc(struct drm_device *dev) > void intel_atomic_state_clear(struct drm_atomic_state *s) > { > struct intel_atomic_state *state = to_intel_atomic_state(s); > + int i; > + > + for (i = 0; i < ARRAY_SIZE(state->work); i++) { > + struct intel_flip_work *work = state->work[i]; > + > + if (work) > + intel_free_flip_work(work); > + > + state->work[i] = NULL; > + } > + > drm_atomic_state_default_clear(&state->base); > state->dpll_set = state->modeset = false; > } > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 69abc808a2c4..16d8e299994d 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4537,39 +4537,6 @@ intel_pre_disable_primary_noatomic(struct drm_crtc *crtc) > } > } > > -static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) > -{ > - struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc); > - struct drm_atomic_state *old_state = old_crtc_state->base.state; > - struct intel_crtc_state *pipe_config = > - to_intel_crtc_state(crtc->base.state); > - struct drm_device *dev = crtc->base.dev; > - struct drm_plane *primary = crtc->base.primary; > - struct drm_plane_state *old_pri_state = > - drm_atomic_get_existing_plane_state(old_state, primary); > - > - intel_frontbuffer_flip(dev, pipe_config->fb_bits); > - > - crtc->wm.cxsr_allowed = true; > - > - if (pipe_config->update_wm_post && pipe_config->base.active) > - intel_update_watermarks(&crtc->base); > - > - if (old_pri_state) { > - struct intel_plane_state *primary_state = > - to_intel_plane_state(primary->state); > - struct intel_plane_state *old_primary_state = > - to_intel_plane_state(old_pri_state); > - > - intel_fbc_post_update(crtc); > - > - if (primary_state->visible && > - (needs_modeset(&pipe_config->base) || > - !old_primary_state->visible)) > - intel_post_enable_primary(&crtc->base); > - } > -} > - > static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state) > { > struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc); > @@ -5179,18 +5146,21 @@ modeset_get_crtc_power_domains(struct drm_crtc *crtc, > struct drm_i915_private *dev_priv = crtc->dev->dev_private; > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > enum intel_display_power_domain domain; > - unsigned long domains, new_domains, old_domains; > + unsigned long domains, new_domains, old_domains, ms_domain = 0; > > old_domains = intel_crtc->enabled_power_domains; > intel_crtc->enabled_power_domains = new_domains = > get_crtc_power_domains(crtc, crtc_state); > > - domains = new_domains & ~old_domains; > + if (needs_modeset(&crtc_state->base)) > + ms_domain = BIT(POWER_DOMAIN_MODESET); > + > + domains = (new_domains & ~old_domains) | ms_domain; > > for_each_power_domain(domain, domains) > intel_display_power_get(dev_priv, domain); > > - return old_domains & ~new_domains; > + return (old_domains & ~new_domains) | ms_domain; > } > > static void modeset_put_power_domains(struct drm_i915_private *dev_priv, > @@ -6236,6 +6206,12 @@ int intel_display_suspend(struct drm_device *dev) > DRM_ERROR("Suspending crtc's failed with %i\n", ret); > else > dev_priv->modeset_restore_state = state; > + > + /* > + * Make sure all unpin_work completes before returning. > + */ > + flush_workqueue(dev_priv->wq); > + > return ret; > } > > @@ -10776,8 +10752,7 @@ void intel_mark_idle(struct drm_i915_private *dev_priv) > intel_runtime_pm_put(dev_priv); > } > > -static void > -intel_free_flip_work(struct intel_flip_work *work) > +void intel_free_flip_work(struct intel_flip_work *work) > { > kfree(work->old_connector_state); > kfree(work->new_connector_state); > @@ -10852,9 +10827,13 @@ static void intel_unpin_work_fn(struct work_struct *__work) > * that work->new_crtc_state contains valid memory during unpin > * because intel_atomic_commit may free it before this runs. > */ > - if (!work->can_async_unpin) > + if (!work->can_async_unpin) { > intel_crtc_post_flip_update(work, crtc); > > + if (dev_priv->display.optimize_watermarks) > + dev_priv->display.optimize_watermarks(work->new_crtc_state); > + } > + > if (work->fb_bits & to_intel_plane(crtc->primary)->frontbuffer_bit) > intel_fbc_post_update(intel_crtc); > > @@ -10876,6 +10855,8 @@ static void intel_unpin_work_fn(struct work_struct *__work) > work->new_connector_state[i]; > struct drm_connector *con = conn_state->connector; > > + WARN_ON(!con); > + > intel_connector_verify_state(to_intel_connector(con), > conn_state); > } > @@ -10905,6 +10886,10 @@ static void intel_unpin_work_fn(struct work_struct *__work) > spin_unlock_irq(&dev->event_lock); > } > > + /* New crtc_state freed? */ > + if (work->free_new_crtc_state) > + intel_crtc_destroy_state(crtc, &work->new_crtc_state->base); > + > intel_crtc_destroy_state(crtc, &work->old_crtc_state->base); > > for (i = 0; i < work->num_planes; i++) { > @@ -10916,7 +10901,8 @@ static void intel_unpin_work_fn(struct work_struct *__work) > > req = old_plane_state->wait_req; > old_plane_state->wait_req = NULL; > - i915_gem_request_unreference(req); > + if (req) > + i915_gem_request_unreference(req); > > fence_put(old_plane_state->base.fence); > old_plane_state->base.fence = NULL; > @@ -10997,6 +10983,11 @@ static void intel_mmio_flip_work_func(struct work_struct *w) > struct drm_i915_gem_request *req; > int i; > > + if (!needs_modeset(&crtc_state->base) && crtc_state->update_pipe) { > + work->put_power_domains = > + modeset_get_crtc_power_domains(crtc, crtc_state); > + } > + > for (i = 0; i < work->num_planes; i++) { > struct intel_plane_state *old_plane_state = work->old_plane_state[i]; > > @@ -12897,11 +12888,33 @@ static int intel_atomic_check(struct drm_device *dev, > return calc_watermark_data(state); > } > > +static bool needs_work(struct drm_crtc_state *crtc_state) > +{ > + /* hw state checker needs to run */ > + if (needs_modeset(crtc_state)) > + return true; > + > + /* unpin old fb's, possibly vblank update */ > + if (crtc_state->planes_changed) > + return true; > + > + /* pipe parameters need to be updated, and hw state checker */ > + if (to_intel_crtc_state(crtc_state)->update_pipe) > + return true; > + > + /* vblank event requested? */ > + if (crtc_state->event) > + return true; > + > + return false; > +} > + > static int intel_atomic_prepare_commit(struct drm_device *dev, > struct drm_atomic_state *state, > bool nonblock) > { > struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_atomic_state *intel_state = to_intel_atomic_state(state); > struct drm_plane_state *plane_state; > struct drm_crtc_state *crtc_state; > struct drm_plane *plane; > @@ -12914,12 +12927,42 @@ static int intel_atomic_prepare_commit(struct drm_device *dev, > } > > for_each_crtc_in_state(state, crtc, crtc_state, i) { > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + struct intel_flip_work *work; > + > ret = intel_crtc_wait_for_pending_flips(crtc); > if (ret) > return ret; > > - if (atomic_read(&to_intel_crtc(crtc)->unpin_work_count) >= 2) > + if (atomic_read(&intel_crtc->unpin_work_count) >= 2) > flush_workqueue(dev_priv->wq); > + > + /* test if we need to update something */ > + if (!needs_work(crtc_state)) > + continue; > + > + intel_state->work[i] = work = > + kzalloc(sizeof(**intel_state->work), GFP_KERNEL); > + > + if (!work) > + return -ENOMEM; > + > + if (needs_modeset(crtc_state) || > + to_intel_crtc_state(crtc_state)->update_pipe) { > + work->num_old_connectors = hweight32(crtc->state->connector_mask); > + > + work->old_connector_state = kcalloc(work->num_old_connectors, > + sizeof(*work->old_connector_state), > + GFP_KERNEL); > + > + work->num_new_connectors = hweight32(crtc_state->connector_mask); > + work->new_connector_state = kcalloc(work->num_new_connectors, > + sizeof(*work->new_connector_state), > + GFP_KERNEL); > + > + if (!work->old_connector_state || !work->new_connector_state) > + return -ENOMEM; > + } > } > > ret = mutex_lock_interruptible(&dev->struct_mutex); > @@ -12972,69 +13015,126 @@ u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc) > return dev->driver->get_vblank_counter(dev, crtc->pipe); > } > > -static void intel_atomic_wait_for_vblanks(struct drm_device *dev, > - struct drm_i915_private *dev_priv, > - unsigned crtc_mask) > +static void intel_prepare_work(struct drm_crtc *crtc, > + struct intel_flip_work *work, > + struct drm_atomic_state *state, > + struct drm_crtc_state *old_crtc_state) > { > - unsigned last_vblank_count[I915_MAX_PIPES]; > - enum pipe pipe; > - int ret; > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + struct drm_plane_state *old_plane_state; > + struct drm_plane *plane; > + int i, j = 0; > > - if (!crtc_mask) > - return; > + INIT_WORK(&work->unpin_work, intel_unpin_work_fn); > + INIT_WORK(&work->mmio_work, intel_mmio_flip_work_func); > + atomic_inc(&intel_crtc->unpin_work_count); > > - for_each_pipe(dev_priv, pipe) { > - struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; > + for_each_plane_in_state(state, plane, old_plane_state, i) { > + struct intel_plane_state *old_state = to_intel_plane_state(old_plane_state); > + struct intel_plane_state *new_state = to_intel_plane_state(plane->state); > > - if (!((1 << pipe) & crtc_mask)) > + if (old_state->base.crtc != crtc && > + new_state->base.crtc != crtc) > continue; > > - ret = drm_crtc_vblank_get(crtc); > - if (WARN_ON(ret != 0)) { > - crtc_mask &= ~(1 << pipe); > - continue; > + if (plane->type == DRM_PLANE_TYPE_PRIMARY) { > + plane->fb = new_state->base.fb; > + crtc->x = new_state->base.src_x >> 16; > + crtc->y = new_state->base.src_y >> 16; > } > > - last_vblank_count[pipe] = drm_crtc_vblank_count(crtc); > + old_state->wait_req = new_state->wait_req; > + new_state->wait_req = NULL; > + > + old_state->base.fence = new_state->base.fence; > + new_state->base.fence = NULL; > + > + /* remove plane state from the atomic state and move it to work */ > + old_plane_state->state = NULL; > + state->planes[i] = NULL; > + state->plane_states[i] = NULL; > + > + work->old_plane_state[j] = old_state; > + work->new_plane_state[j++] = new_state; > } > > - for_each_pipe(dev_priv, pipe) { > - struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; > - long lret; > + old_crtc_state->state = NULL; > + state->crtcs[drm_crtc_index(crtc)] = NULL; > + state->crtc_states[drm_crtc_index(crtc)] = NULL; > > - if (!((1 << pipe) & crtc_mask)) > - continue; > + work->old_crtc_state = to_intel_crtc_state(old_crtc_state); > + work->new_crtc_state = to_intel_crtc_state(crtc->state); > + work->num_planes = j; > > - lret = wait_event_timeout(dev->vblank[pipe].queue, > - last_vblank_count[pipe] != > - drm_crtc_vblank_count(crtc), > - msecs_to_jiffies(50)); > + work->event = crtc->state->event; > + crtc->state->event = NULL; > > - WARN(!lret, "pipe %c vblank wait timed out\n", pipe_name(pipe)); > + if (needs_modeset(crtc->state) || work->new_crtc_state->update_pipe) { > + struct drm_connector *conn; > + struct drm_connector_state *old_conn_state; > + int k = 0; > > - drm_crtc_vblank_put(crtc); > - } > + j = 0; > + > + /* > + * intel_unpin_work_fn cannot depend on the connector list > + * because it may be freed from underneath it, so add > + * them all to the work struct while we're holding locks. > + */ > + for_each_connector_in_state(state, conn, old_conn_state, i) { > + if (old_conn_state->crtc == crtc) { > + work->old_connector_state[j++] = old_conn_state; > + > + state->connectors[i] = NULL; > + state->connector_states[i] = NULL; > + } > + } > + > + /* If another crtc has stolen the connector from state, > + * then for_each_connector_in_state is no longer reliable, > + * so use drm_for_each_connector here. > + */ > + drm_for_each_connector(conn, state->dev) > + if (conn->state->crtc == crtc) > + work->new_connector_state[k++] = conn->state; > + > + WARN(j != work->num_old_connectors, "j = %i, expected %i\n", j, work->num_old_connectors); > + WARN(k != work->num_new_connectors, "k = %i, expected %i\n", k, work->num_new_connectors); > + } else if (!work->new_crtc_state->update_wm_post) > + work->can_async_unpin = true; > + > + work->fb_bits = work->new_crtc_state->fb_bits; > } > > -static bool needs_vblank_wait(struct intel_crtc_state *crtc_state) > +static void intel_schedule_unpin(struct drm_crtc *crtc, > + struct intel_atomic_state *state, > + struct intel_flip_work *work) > { > - /* fb updated, need to unpin old fb */ > - if (crtc_state->fb_changed) > - return true; > + struct drm_device *dev = crtc->dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > > - /* wm changes, need vblank before final wm's */ > - if (crtc_state->update_wm_post) > - return true; > + to_intel_crtc(crtc)->config = work->new_crtc_state; > > - /* > - * cxsr is re-enabled after vblank. > - * This is already handled by crtc_state->update_wm_post, > - * but added for clarity. > - */ > - if (crtc_state->disable_cxsr) > - return true; > + queue_work(dev_priv->wq, &work->unpin_work); > +} > > - return false; > +static void intel_schedule_update(struct drm_crtc *crtc, > + struct intel_atomic_state *state, > + struct intel_flip_work *work) > +{ > + struct drm_device *dev = crtc->dev; > + > + if (work->can_async_unpin) { > + INIT_LIST_HEAD(&work->head); > + intel_schedule_unpin(crtc, state, work); > + return; > + } > + > + spin_lock_irq(&dev->event_lock); > + list_add_tail(&work->head, &to_intel_crtc(crtc)->flip_work); > + spin_unlock_irq(&dev->event_lock); > + > + intel_schedule_unpin(crtc, state, work); > } > > /** > @@ -13061,11 +13161,7 @@ static int intel_atomic_commit(struct drm_device *dev, > struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_crtc_state *old_crtc_state; > struct drm_crtc *crtc; > - struct intel_crtc_state *intel_cstate; > int ret = 0, i; > - bool hw_check = intel_state->modeset; > - unsigned long put_domains[I915_MAX_PIPES] = {}; > - unsigned crtc_vblank_mask = 0; > > ret = intel_atomic_prepare_commit(dev, state, nonblock); > if (ret) { > @@ -13083,27 +13179,20 @@ static int intel_atomic_commit(struct drm_device *dev, > sizeof(intel_state->min_pixclk)); > dev_priv->active_crtcs = intel_state->active_crtcs; > dev_priv->atomic_cdclk_freq = intel_state->cdclk; > - > - intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET); > } > > for_each_crtc_in_state(state, crtc, old_crtc_state, i) { > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > - if (needs_modeset(crtc->state) || > - to_intel_crtc_state(crtc->state)->update_pipe) { > - hw_check = true; > - > - put_domains[to_intel_crtc(crtc)->pipe] = > - modeset_get_crtc_power_domains(crtc, > - to_intel_crtc_state(crtc->state)); > - } > - > if (!needs_modeset(crtc->state)) > continue; > > intel_pre_plane_update(to_intel_crtc_state(old_crtc_state)); > > + intel_state->work[i]->put_power_domains = > + modeset_get_crtc_power_domains(crtc, > + to_intel_crtc_state(crtc->state)); > + > if (old_crtc_state->active) { > intel_crtc_disable_planes(crtc, old_crtc_state->plane_mask); > dev_priv->display.crtc_disable(crtc); > @@ -13160,46 +13249,37 @@ static int intel_atomic_commit(struct drm_device *dev, > if (crtc->state->active && > (crtc->state->planes_changed || update_pipe)) > drm_atomic_helper_commit_planes_on_crtc(old_crtc_state); > - > - if (pipe_config->base.active && needs_vblank_wait(pipe_config)) > - crtc_vblank_mask |= 1 << i; > } > > /* FIXME: add subpixel order */ > > - if (!state->legacy_cursor_update) > - intel_atomic_wait_for_vblanks(dev, dev_priv, crtc_vblank_mask); > - > - /* > - * Now that the vblank has passed, we can go ahead and program the > - * optimal watermarks on platforms that need two-step watermark > - * programming. > - * > - * TODO: Move this (and other cleanup) to an async worker eventually. > - */ > - for_each_crtc_in_state(state, crtc, old_crtc_state, i) { > - intel_cstate = to_intel_crtc_state(crtc->state); > - > - if (dev_priv->display.optimize_watermarks) > - dev_priv->display.optimize_watermarks(intel_cstate); > - } > - > for_each_crtc_in_state(state, crtc, old_crtc_state, i) { > - intel_post_plane_update(to_intel_crtc_state(old_crtc_state)); > - > - if (put_domains[i]) > - modeset_put_power_domains(dev_priv, put_domains[i]); > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + struct intel_flip_work *work = > + intel_state->work[i]; > + > + if (!work) { > + if (!list_empty_careful(&intel_crtc->flip_work)) { > + spin_lock_irq(&dev->event_lock); > + if (!list_empty(&intel_crtc->flip_work)) > + work = list_last_entry(&intel_crtc->flip_work, > + struct intel_flip_work, head); > + > + if (work && work->new_crtc_state == to_intel_crtc_state(old_crtc_state)) { > + work->free_new_crtc_state = true; > + state->crtc_states[i] = NULL; > + state->crtcs[i] = NULL; > + } > + spin_unlock_irq(&dev->event_lock); > + } > + continue; > + } > > - intel_modeset_verify_crtc(crtc, old_crtc_state, crtc->state); > + intel_state->work[i] = NULL; > + intel_prepare_work(crtc, work, state, old_crtc_state); > + intel_schedule_update(crtc, intel_state, work); > } > > - if (intel_state->modeset) > - intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET); > - > - mutex_lock(&dev->struct_mutex); > - drm_atomic_helper_cleanup_planes(dev, state); > - mutex_unlock(&dev->struct_mutex); > - > drm_atomic_state_free(state); > > /* As one of the primary mmio accessors, KMS has a high likelihood > @@ -13454,6 +13534,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, > to_intel_crtc_state(old_crtc_state); > bool modeset = needs_modeset(crtc->state); > > + intel_frontbuffer_flip_prepare(dev, to_intel_crtc_state(crtc->state)->fb_bits); > + > /* Perform vblank evasion around commit operation */ > intel_pipe_update_start(intel_crtc); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index af1a9ab0cc6c..5688e5949a87 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -304,6 +304,8 @@ struct intel_atomic_state { > unsigned int active_crtcs; > unsigned int min_pixclk[I915_MAX_PIPES]; > > + struct intel_flip_work *work[I915_MAX_PIPES]; > + > struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS]; > > /* > @@ -979,7 +981,7 @@ struct intel_flip_work { > unsigned put_power_domains; > unsigned num_planes; > > - bool can_async_unpin; > + bool can_async_unpin, free_new_crtc_state; > unsigned fb_bits; > > unsigned num_old_connectors, num_new_connectors; > @@ -1147,6 +1149,7 @@ unsigned int intel_rotation_info_size(const struct intel_rotation_info *rot_info > bool intel_has_pending_fb_unpin(struct drm_device *dev); > void intel_mark_busy(struct drm_i915_private *dev_priv); > void intel_mark_idle(struct drm_i915_private *dev_priv); > +void intel_free_flip_work(struct intel_flip_work *work); > void intel_crtc_restore_mode(struct drm_crtc *crtc); > int intel_display_suspend(struct drm_device *dev); > void intel_encoder_destroy(struct drm_encoder *encoder); > -- > 2.5.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, May 17, 2016 at 03:08:01PM +0200, Maarten Lankhorst wrote: > All of intel_post_plane_update is handled there now, so move it over. > This is run after the hw state checker because it can't handle checking > crtc's separately yet. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> First this patch is massive, and imo too big and should have been split up. > --- > drivers/gpu/drm/i915/intel_atomic.c | 11 ++ > drivers/gpu/drm/i915/intel_display.c | 344 ++++++++++++++++++++++------------- > drivers/gpu/drm/i915/intel_drv.h | 5 +- > 3 files changed, 228 insertions(+), 132 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c > index 50ff90aea721..b4927f6bbeac 100644 > --- a/drivers/gpu/drm/i915/intel_atomic.c > +++ b/drivers/gpu/drm/i915/intel_atomic.c > @@ -311,6 +311,17 @@ intel_atomic_state_alloc(struct drm_device *dev) > void intel_atomic_state_clear(struct drm_atomic_state *s) > { > struct intel_atomic_state *state = to_intel_atomic_state(s); > + int i; > + > + for (i = 0; i < ARRAY_SIZE(state->work); i++) { > + struct intel_flip_work *work = state->work[i]; > + > + if (work) > + intel_free_flip_work(work); > + > + state->work[i] = NULL; > + } > + > drm_atomic_state_default_clear(&state->base); > state->dpll_set = state->modeset = false; > } > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 69abc808a2c4..16d8e299994d 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4537,39 +4537,6 @@ intel_pre_disable_primary_noatomic(struct drm_crtc *crtc) > } > } > > -static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) > -{ > - struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc); > - struct drm_atomic_state *old_state = old_crtc_state->base.state; > - struct intel_crtc_state *pipe_config = > - to_intel_crtc_state(crtc->base.state); > - struct drm_device *dev = crtc->base.dev; > - struct drm_plane *primary = crtc->base.primary; > - struct drm_plane_state *old_pri_state = > - drm_atomic_get_existing_plane_state(old_state, primary); > - > - intel_frontbuffer_flip(dev, pipe_config->fb_bits); > - > - crtc->wm.cxsr_allowed = true; > - > - if (pipe_config->update_wm_post && pipe_config->base.active) > - intel_update_watermarks(&crtc->base); > - > - if (old_pri_state) { > - struct intel_plane_state *primary_state = > - to_intel_plane_state(primary->state); > - struct intel_plane_state *old_primary_state = > - to_intel_plane_state(old_pri_state); > - > - intel_fbc_post_update(crtc); > - > - if (primary_state->visible && > - (needs_modeset(&pipe_config->base) || > - !old_primary_state->visible)) > - intel_post_enable_primary(&crtc->base); > - } > -} The above seems to have moved/disappeared entirely, and is definitely not unpin related. Really, this must be split up. I guess another reason to revert this for now. Or at least the commit should have a more accurate summary. But since I can't even find the new callsite of these functions in this diff here something seems fishy, but I didn't look at the entire series. Anyway, with that rant out of the way see below for what I really wanted to comment on. [snip] > +static void intel_prepare_work(struct drm_crtc *crtc, > + struct intel_flip_work *work, > + struct drm_atomic_state *state, > + struct drm_crtc_state *old_crtc_state) > { > - unsigned last_vblank_count[I915_MAX_PIPES]; > - enum pipe pipe; > - int ret; > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + struct drm_plane_state *old_plane_state; > + struct drm_plane *plane; > + int i, j = 0; > > - if (!crtc_mask) > - return; > + INIT_WORK(&work->unpin_work, intel_unpin_work_fn); > + INIT_WORK(&work->mmio_work, intel_mmio_flip_work_func); > + atomic_inc(&intel_crtc->unpin_work_count); > > - for_each_pipe(dev_priv, pipe) { > - struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; > + for_each_plane_in_state(state, plane, old_plane_state, i) { > + struct intel_plane_state *old_state = to_intel_plane_state(old_plane_state); > + struct intel_plane_state *new_state = to_intel_plane_state(plane->state); > > - if (!((1 << pipe) & crtc_mask)) > + if (old_state->base.crtc != crtc && > + new_state->base.crtc != crtc) > continue; > > - ret = drm_crtc_vblank_get(crtc); > - if (WARN_ON(ret != 0)) { > - crtc_mask &= ~(1 << pipe); > - continue; > + if (plane->type == DRM_PLANE_TYPE_PRIMARY) { > + plane->fb = new_state->base.fb; > + crtc->x = new_state->base.src_x >> 16; > + crtc->y = new_state->base.src_y >> 16; > } > > - last_vblank_count[pipe] = drm_crtc_vblank_count(crtc); > + old_state->wait_req = new_state->wait_req; > + new_state->wait_req = NULL; > + > + old_state->base.fence = new_state->base.fence; > + new_state->base.fence = NULL; > + > + /* remove plane state from the atomic state and move it to work */ > + old_plane_state->state = NULL; > + state->planes[i] = NULL; > + state->plane_states[i] = NULL; > + > + work->old_plane_state[j] = old_state; > + work->new_plane_state[j++] = new_state; > } > > - for_each_pipe(dev_priv, pipe) { > - struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; > - long lret; > + old_crtc_state->state = NULL; > + state->crtcs[drm_crtc_index(crtc)] = NULL; > + state->crtc_states[drm_crtc_index(crtc)] = NULL; > > - if (!((1 << pipe) & crtc_mask)) > - continue; > + work->old_crtc_state = to_intel_crtc_state(old_crtc_state); > + work->new_crtc_state = to_intel_crtc_state(crtc->state); > + work->num_planes = j; > > - lret = wait_event_timeout(dev->vblank[pipe].queue, > - last_vblank_count[pipe] != > - drm_crtc_vblank_count(crtc), > - msecs_to_jiffies(50)); > + work->event = crtc->state->event; > + crtc->state->event = NULL; > > - WARN(!lret, "pipe %c vblank wait timed out\n", pipe_name(pipe)); > + if (needs_modeset(crtc->state) || work->new_crtc_state->update_pipe) { > + struct drm_connector *conn; > + struct drm_connector_state *old_conn_state; > + int k = 0; > > - drm_crtc_vblank_put(crtc); > - } > + j = 0; > + > + /* > + * intel_unpin_work_fn cannot depend on the connector list > + * because it may be freed from underneath it, so add > + * them all to the work struct while we're holding locks. > + */ Thanks to connector refcounting no longer true, and as long as you hold onto drm_atomic_state the connectors will no longer disappear. Please revise/update. Thanks, Daniel
Op 24-05-16 om 16:00 schreef Daniel Vetter: > On Tue, May 17, 2016 at 03:08:01PM +0200, Maarten Lankhorst wrote: >> All of intel_post_plane_update is handled there now, so move it over. >> This is run after the hw state checker because it can't handle checking >> crtc's separately yet. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > First this patch is massive, and imo too big and should have been split > up. > >> --- >> drivers/gpu/drm/i915/intel_atomic.c | 11 ++ >> drivers/gpu/drm/i915/intel_display.c | 344 ++++++++++++++++++++++------------- >> drivers/gpu/drm/i915/intel_drv.h | 5 +- >> 3 files changed, 228 insertions(+), 132 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c >> index 50ff90aea721..b4927f6bbeac 100644 >> --- a/drivers/gpu/drm/i915/intel_atomic.c >> +++ b/drivers/gpu/drm/i915/intel_atomic.c >> @@ -311,6 +311,17 @@ intel_atomic_state_alloc(struct drm_device *dev) >> void intel_atomic_state_clear(struct drm_atomic_state *s) >> { >> struct intel_atomic_state *state = to_intel_atomic_state(s); >> + int i; >> + >> + for (i = 0; i < ARRAY_SIZE(state->work); i++) { >> + struct intel_flip_work *work = state->work[i]; >> + >> + if (work) >> + intel_free_flip_work(work); >> + >> + state->work[i] = NULL; >> + } >> + >> drm_atomic_state_default_clear(&state->base); >> state->dpll_set = state->modeset = false; >> } >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 69abc808a2c4..16d8e299994d 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -4537,39 +4537,6 @@ intel_pre_disable_primary_noatomic(struct drm_crtc *crtc) >> } >> } >> >> -static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) >> -{ >> - struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc); >> - struct drm_atomic_state *old_state = old_crtc_state->base.state; >> - struct intel_crtc_state *pipe_config = >> - to_intel_crtc_state(crtc->base.state); >> - struct drm_device *dev = crtc->base.dev; >> - struct drm_plane *primary = crtc->base.primary; >> - struct drm_plane_state *old_pri_state = >> - drm_atomic_get_existing_plane_state(old_state, primary); >> - >> - intel_frontbuffer_flip(dev, pipe_config->fb_bits); >> - >> - crtc->wm.cxsr_allowed = true; >> - >> - if (pipe_config->update_wm_post && pipe_config->base.active) >> - intel_update_watermarks(&crtc->base); >> - >> - if (old_pri_state) { >> - struct intel_plane_state *primary_state = >> - to_intel_plane_state(primary->state); >> - struct intel_plane_state *old_primary_state = >> - to_intel_plane_state(old_pri_state); >> - >> - intel_fbc_post_update(crtc); >> - >> - if (primary_state->visible && >> - (needs_modeset(&pipe_config->base) || >> - !old_primary_state->visible)) >> - intel_post_enable_primary(&crtc->base); >> - } >> -} > The above seems to have moved/disappeared entirely, and is definitely not > unpin related. Really, this must be split up. I guess another reason to > revert this for now. > > Or at least the commit should have a more accurate summary. But since I > can't even find the new callsite of these functions in this diff here > something seems fishy, but I didn't look at the entire series. > > Anyway, with that rant out of the way see below for what I really wanted > to comment on. > > > [snip] > >> +static void intel_prepare_work(struct drm_crtc *crtc, >> + struct intel_flip_work *work, >> + struct drm_atomic_state *state, >> + struct drm_crtc_state *old_crtc_state) >> { >> - unsigned last_vblank_count[I915_MAX_PIPES]; >> - enum pipe pipe; >> - int ret; >> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> + struct drm_plane_state *old_plane_state; >> + struct drm_plane *plane; >> + int i, j = 0; >> >> - if (!crtc_mask) >> - return; >> + INIT_WORK(&work->unpin_work, intel_unpin_work_fn); >> + INIT_WORK(&work->mmio_work, intel_mmio_flip_work_func); >> + atomic_inc(&intel_crtc->unpin_work_count); >> >> - for_each_pipe(dev_priv, pipe) { >> - struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; >> + for_each_plane_in_state(state, plane, old_plane_state, i) { >> + struct intel_plane_state *old_state = to_intel_plane_state(old_plane_state); >> + struct intel_plane_state *new_state = to_intel_plane_state(plane->state); >> >> - if (!((1 << pipe) & crtc_mask)) >> + if (old_state->base.crtc != crtc && >> + new_state->base.crtc != crtc) >> continue; >> >> - ret = drm_crtc_vblank_get(crtc); >> - if (WARN_ON(ret != 0)) { >> - crtc_mask &= ~(1 << pipe); >> - continue; >> + if (plane->type == DRM_PLANE_TYPE_PRIMARY) { >> + plane->fb = new_state->base.fb; >> + crtc->x = new_state->base.src_x >> 16; >> + crtc->y = new_state->base.src_y >> 16; >> } >> >> - last_vblank_count[pipe] = drm_crtc_vblank_count(crtc); >> + old_state->wait_req = new_state->wait_req; >> + new_state->wait_req = NULL; >> + >> + old_state->base.fence = new_state->base.fence; >> + new_state->base.fence = NULL; >> + >> + /* remove plane state from the atomic state and move it to work */ >> + old_plane_state->state = NULL; >> + state->planes[i] = NULL; >> + state->plane_states[i] = NULL; >> + >> + work->old_plane_state[j] = old_state; >> + work->new_plane_state[j++] = new_state; >> } >> >> - for_each_pipe(dev_priv, pipe) { >> - struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; >> - long lret; >> + old_crtc_state->state = NULL; >> + state->crtcs[drm_crtc_index(crtc)] = NULL; >> + state->crtc_states[drm_crtc_index(crtc)] = NULL; >> >> - if (!((1 << pipe) & crtc_mask)) >> - continue; >> + work->old_crtc_state = to_intel_crtc_state(old_crtc_state); >> + work->new_crtc_state = to_intel_crtc_state(crtc->state); >> + work->num_planes = j; >> >> - lret = wait_event_timeout(dev->vblank[pipe].queue, >> - last_vblank_count[pipe] != >> - drm_crtc_vblank_count(crtc), >> - msecs_to_jiffies(50)); >> + work->event = crtc->state->event; >> + crtc->state->event = NULL; >> >> - WARN(!lret, "pipe %c vblank wait timed out\n", pipe_name(pipe)); >> + if (needs_modeset(crtc->state) || work->new_crtc_state->update_pipe) { >> + struct drm_connector *conn; >> + struct drm_connector_state *old_conn_state; >> + int k = 0; >> >> - drm_crtc_vblank_put(crtc); >> - } >> + j = 0; >> + >> + /* >> + * intel_unpin_work_fn cannot depend on the connector list >> + * because it may be freed from underneath it, so add >> + * them all to the work struct while we're holding locks. >> + */ > Thanks to connector refcounting no longer true, and as long as you hold > onto drm_atomic_state the connectors will no longer disappear. Please > revise/update. This comment only became true thanks to connector refcounting. unpin_work cannot take connection_mutex or mode_config->mutex because that would deadlock if flushed from intel_atomic_commit with those locks held, but needs it for the hw state verifier. Before connector refcounting I could perform a flush in intel_dp_destroy_mst_connector so even though unpin_work wouldn't have the locks, it would never see a modified list. With connector refcounting I move the connector_states to the work, and free them there. But it looks like the locking for unreferencing a connector seems to be messed up. drm_atomic_state_free can be called without locking anything, so drm_connector_free needs to handle its own locking when removing the connector from the list. This affects all atomic drivers, and probably means requiring a work item to free all connectors with the right locks.. ~Maarten
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index 50ff90aea721..b4927f6bbeac 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -311,6 +311,17 @@ intel_atomic_state_alloc(struct drm_device *dev) void intel_atomic_state_clear(struct drm_atomic_state *s) { struct intel_atomic_state *state = to_intel_atomic_state(s); + int i; + + for (i = 0; i < ARRAY_SIZE(state->work); i++) { + struct intel_flip_work *work = state->work[i]; + + if (work) + intel_free_flip_work(work); + + state->work[i] = NULL; + } + drm_atomic_state_default_clear(&state->base); state->dpll_set = state->modeset = false; } diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 69abc808a2c4..16d8e299994d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4537,39 +4537,6 @@ intel_pre_disable_primary_noatomic(struct drm_crtc *crtc) } } -static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) -{ - struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc); - struct drm_atomic_state *old_state = old_crtc_state->base.state; - struct intel_crtc_state *pipe_config = - to_intel_crtc_state(crtc->base.state); - struct drm_device *dev = crtc->base.dev; - struct drm_plane *primary = crtc->base.primary; - struct drm_plane_state *old_pri_state = - drm_atomic_get_existing_plane_state(old_state, primary); - - intel_frontbuffer_flip(dev, pipe_config->fb_bits); - - crtc->wm.cxsr_allowed = true; - - if (pipe_config->update_wm_post && pipe_config->base.active) - intel_update_watermarks(&crtc->base); - - if (old_pri_state) { - struct intel_plane_state *primary_state = - to_intel_plane_state(primary->state); - struct intel_plane_state *old_primary_state = - to_intel_plane_state(old_pri_state); - - intel_fbc_post_update(crtc); - - if (primary_state->visible && - (needs_modeset(&pipe_config->base) || - !old_primary_state->visible)) - intel_post_enable_primary(&crtc->base); - } -} - static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state) { struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc); @@ -5179,18 +5146,21 @@ modeset_get_crtc_power_domains(struct drm_crtc *crtc, struct drm_i915_private *dev_priv = crtc->dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); enum intel_display_power_domain domain; - unsigned long domains, new_domains, old_domains; + unsigned long domains, new_domains, old_domains, ms_domain = 0; old_domains = intel_crtc->enabled_power_domains; intel_crtc->enabled_power_domains = new_domains = get_crtc_power_domains(crtc, crtc_state); - domains = new_domains & ~old_domains; + if (needs_modeset(&crtc_state->base)) + ms_domain = BIT(POWER_DOMAIN_MODESET); + + domains = (new_domains & ~old_domains) | ms_domain; for_each_power_domain(domain, domains) intel_display_power_get(dev_priv, domain); - return old_domains & ~new_domains; + return (old_domains & ~new_domains) | ms_domain; } static void modeset_put_power_domains(struct drm_i915_private *dev_priv, @@ -6236,6 +6206,12 @@ int intel_display_suspend(struct drm_device *dev) DRM_ERROR("Suspending crtc's failed with %i\n", ret); else dev_priv->modeset_restore_state = state; + + /* + * Make sure all unpin_work completes before returning. + */ + flush_workqueue(dev_priv->wq); + return ret; } @@ -10776,8 +10752,7 @@ void intel_mark_idle(struct drm_i915_private *dev_priv) intel_runtime_pm_put(dev_priv); } -static void -intel_free_flip_work(struct intel_flip_work *work) +void intel_free_flip_work(struct intel_flip_work *work) { kfree(work->old_connector_state); kfree(work->new_connector_state); @@ -10852,9 +10827,13 @@ static void intel_unpin_work_fn(struct work_struct *__work) * that work->new_crtc_state contains valid memory during unpin * because intel_atomic_commit may free it before this runs. */ - if (!work->can_async_unpin) + if (!work->can_async_unpin) { intel_crtc_post_flip_update(work, crtc); + if (dev_priv->display.optimize_watermarks) + dev_priv->display.optimize_watermarks(work->new_crtc_state); + } + if (work->fb_bits & to_intel_plane(crtc->primary)->frontbuffer_bit) intel_fbc_post_update(intel_crtc); @@ -10876,6 +10855,8 @@ static void intel_unpin_work_fn(struct work_struct *__work) work->new_connector_state[i]; struct drm_connector *con = conn_state->connector; + WARN_ON(!con); + intel_connector_verify_state(to_intel_connector(con), conn_state); } @@ -10905,6 +10886,10 @@ static void intel_unpin_work_fn(struct work_struct *__work) spin_unlock_irq(&dev->event_lock); } + /* New crtc_state freed? */ + if (work->free_new_crtc_state) + intel_crtc_destroy_state(crtc, &work->new_crtc_state->base); + intel_crtc_destroy_state(crtc, &work->old_crtc_state->base); for (i = 0; i < work->num_planes; i++) { @@ -10916,7 +10901,8 @@ static void intel_unpin_work_fn(struct work_struct *__work) req = old_plane_state->wait_req; old_plane_state->wait_req = NULL; - i915_gem_request_unreference(req); + if (req) + i915_gem_request_unreference(req); fence_put(old_plane_state->base.fence); old_plane_state->base.fence = NULL; @@ -10997,6 +10983,11 @@ static void intel_mmio_flip_work_func(struct work_struct *w) struct drm_i915_gem_request *req; int i; + if (!needs_modeset(&crtc_state->base) && crtc_state->update_pipe) { + work->put_power_domains = + modeset_get_crtc_power_domains(crtc, crtc_state); + } + for (i = 0; i < work->num_planes; i++) { struct intel_plane_state *old_plane_state = work->old_plane_state[i]; @@ -12897,11 +12888,33 @@ static int intel_atomic_check(struct drm_device *dev, return calc_watermark_data(state); } +static bool needs_work(struct drm_crtc_state *crtc_state) +{ + /* hw state checker needs to run */ + if (needs_modeset(crtc_state)) + return true; + + /* unpin old fb's, possibly vblank update */ + if (crtc_state->planes_changed) + return true; + + /* pipe parameters need to be updated, and hw state checker */ + if (to_intel_crtc_state(crtc_state)->update_pipe) + return true; + + /* vblank event requested? */ + if (crtc_state->event) + return true; + + return false; +} + static int intel_atomic_prepare_commit(struct drm_device *dev, struct drm_atomic_state *state, bool nonblock) { struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_atomic_state *intel_state = to_intel_atomic_state(state); struct drm_plane_state *plane_state; struct drm_crtc_state *crtc_state; struct drm_plane *plane; @@ -12914,12 +12927,42 @@ static int intel_atomic_prepare_commit(struct drm_device *dev, } for_each_crtc_in_state(state, crtc, crtc_state, i) { + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + struct intel_flip_work *work; + ret = intel_crtc_wait_for_pending_flips(crtc); if (ret) return ret; - if (atomic_read(&to_intel_crtc(crtc)->unpin_work_count) >= 2) + if (atomic_read(&intel_crtc->unpin_work_count) >= 2) flush_workqueue(dev_priv->wq); + + /* test if we need to update something */ + if (!needs_work(crtc_state)) + continue; + + intel_state->work[i] = work = + kzalloc(sizeof(**intel_state->work), GFP_KERNEL); + + if (!work) + return -ENOMEM; + + if (needs_modeset(crtc_state) || + to_intel_crtc_state(crtc_state)->update_pipe) { + work->num_old_connectors = hweight32(crtc->state->connector_mask); + + work->old_connector_state = kcalloc(work->num_old_connectors, + sizeof(*work->old_connector_state), + GFP_KERNEL); + + work->num_new_connectors = hweight32(crtc_state->connector_mask); + work->new_connector_state = kcalloc(work->num_new_connectors, + sizeof(*work->new_connector_state), + GFP_KERNEL); + + if (!work->old_connector_state || !work->new_connector_state) + return -ENOMEM; + } } ret = mutex_lock_interruptible(&dev->struct_mutex); @@ -12972,69 +13015,126 @@ u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc) return dev->driver->get_vblank_counter(dev, crtc->pipe); } -static void intel_atomic_wait_for_vblanks(struct drm_device *dev, - struct drm_i915_private *dev_priv, - unsigned crtc_mask) +static void intel_prepare_work(struct drm_crtc *crtc, + struct intel_flip_work *work, + struct drm_atomic_state *state, + struct drm_crtc_state *old_crtc_state) { - unsigned last_vblank_count[I915_MAX_PIPES]; - enum pipe pipe; - int ret; + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + struct drm_plane_state *old_plane_state; + struct drm_plane *plane; + int i, j = 0; - if (!crtc_mask) - return; + INIT_WORK(&work->unpin_work, intel_unpin_work_fn); + INIT_WORK(&work->mmio_work, intel_mmio_flip_work_func); + atomic_inc(&intel_crtc->unpin_work_count); - for_each_pipe(dev_priv, pipe) { - struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; + for_each_plane_in_state(state, plane, old_plane_state, i) { + struct intel_plane_state *old_state = to_intel_plane_state(old_plane_state); + struct intel_plane_state *new_state = to_intel_plane_state(plane->state); - if (!((1 << pipe) & crtc_mask)) + if (old_state->base.crtc != crtc && + new_state->base.crtc != crtc) continue; - ret = drm_crtc_vblank_get(crtc); - if (WARN_ON(ret != 0)) { - crtc_mask &= ~(1 << pipe); - continue; + if (plane->type == DRM_PLANE_TYPE_PRIMARY) { + plane->fb = new_state->base.fb; + crtc->x = new_state->base.src_x >> 16; + crtc->y = new_state->base.src_y >> 16; } - last_vblank_count[pipe] = drm_crtc_vblank_count(crtc); + old_state->wait_req = new_state->wait_req; + new_state->wait_req = NULL; + + old_state->base.fence = new_state->base.fence; + new_state->base.fence = NULL; + + /* remove plane state from the atomic state and move it to work */ + old_plane_state->state = NULL; + state->planes[i] = NULL; + state->plane_states[i] = NULL; + + work->old_plane_state[j] = old_state; + work->new_plane_state[j++] = new_state; } - for_each_pipe(dev_priv, pipe) { - struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; - long lret; + old_crtc_state->state = NULL; + state->crtcs[drm_crtc_index(crtc)] = NULL; + state->crtc_states[drm_crtc_index(crtc)] = NULL; - if (!((1 << pipe) & crtc_mask)) - continue; + work->old_crtc_state = to_intel_crtc_state(old_crtc_state); + work->new_crtc_state = to_intel_crtc_state(crtc->state); + work->num_planes = j; - lret = wait_event_timeout(dev->vblank[pipe].queue, - last_vblank_count[pipe] != - drm_crtc_vblank_count(crtc), - msecs_to_jiffies(50)); + work->event = crtc->state->event; + crtc->state->event = NULL; - WARN(!lret, "pipe %c vblank wait timed out\n", pipe_name(pipe)); + if (needs_modeset(crtc->state) || work->new_crtc_state->update_pipe) { + struct drm_connector *conn; + struct drm_connector_state *old_conn_state; + int k = 0; - drm_crtc_vblank_put(crtc); - } + j = 0; + + /* + * intel_unpin_work_fn cannot depend on the connector list + * because it may be freed from underneath it, so add + * them all to the work struct while we're holding locks. + */ + for_each_connector_in_state(state, conn, old_conn_state, i) { + if (old_conn_state->crtc == crtc) { + work->old_connector_state[j++] = old_conn_state; + + state->connectors[i] = NULL; + state->connector_states[i] = NULL; + } + } + + /* If another crtc has stolen the connector from state, + * then for_each_connector_in_state is no longer reliable, + * so use drm_for_each_connector here. + */ + drm_for_each_connector(conn, state->dev) + if (conn->state->crtc == crtc) + work->new_connector_state[k++] = conn->state; + + WARN(j != work->num_old_connectors, "j = %i, expected %i\n", j, work->num_old_connectors); + WARN(k != work->num_new_connectors, "k = %i, expected %i\n", k, work->num_new_connectors); + } else if (!work->new_crtc_state->update_wm_post) + work->can_async_unpin = true; + + work->fb_bits = work->new_crtc_state->fb_bits; } -static bool needs_vblank_wait(struct intel_crtc_state *crtc_state) +static void intel_schedule_unpin(struct drm_crtc *crtc, + struct intel_atomic_state *state, + struct intel_flip_work *work) { - /* fb updated, need to unpin old fb */ - if (crtc_state->fb_changed) - return true; + struct drm_device *dev = crtc->dev; + struct drm_i915_private *dev_priv = dev->dev_private; - /* wm changes, need vblank before final wm's */ - if (crtc_state->update_wm_post) - return true; + to_intel_crtc(crtc)->config = work->new_crtc_state; - /* - * cxsr is re-enabled after vblank. - * This is already handled by crtc_state->update_wm_post, - * but added for clarity. - */ - if (crtc_state->disable_cxsr) - return true; + queue_work(dev_priv->wq, &work->unpin_work); +} - return false; +static void intel_schedule_update(struct drm_crtc *crtc, + struct intel_atomic_state *state, + struct intel_flip_work *work) +{ + struct drm_device *dev = crtc->dev; + + if (work->can_async_unpin) { + INIT_LIST_HEAD(&work->head); + intel_schedule_unpin(crtc, state, work); + return; + } + + spin_lock_irq(&dev->event_lock); + list_add_tail(&work->head, &to_intel_crtc(crtc)->flip_work); + spin_unlock_irq(&dev->event_lock); + + intel_schedule_unpin(crtc, state, work); } /** @@ -13061,11 +13161,7 @@ static int intel_atomic_commit(struct drm_device *dev, struct drm_i915_private *dev_priv = dev->dev_private; struct drm_crtc_state *old_crtc_state; struct drm_crtc *crtc; - struct intel_crtc_state *intel_cstate; int ret = 0, i; - bool hw_check = intel_state->modeset; - unsigned long put_domains[I915_MAX_PIPES] = {}; - unsigned crtc_vblank_mask = 0; ret = intel_atomic_prepare_commit(dev, state, nonblock); if (ret) { @@ -13083,27 +13179,20 @@ static int intel_atomic_commit(struct drm_device *dev, sizeof(intel_state->min_pixclk)); dev_priv->active_crtcs = intel_state->active_crtcs; dev_priv->atomic_cdclk_freq = intel_state->cdclk; - - intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET); } for_each_crtc_in_state(state, crtc, old_crtc_state, i) { struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - if (needs_modeset(crtc->state) || - to_intel_crtc_state(crtc->state)->update_pipe) { - hw_check = true; - - put_domains[to_intel_crtc(crtc)->pipe] = - modeset_get_crtc_power_domains(crtc, - to_intel_crtc_state(crtc->state)); - } - if (!needs_modeset(crtc->state)) continue; intel_pre_plane_update(to_intel_crtc_state(old_crtc_state)); + intel_state->work[i]->put_power_domains = + modeset_get_crtc_power_domains(crtc, + to_intel_crtc_state(crtc->state)); + if (old_crtc_state->active) { intel_crtc_disable_planes(crtc, old_crtc_state->plane_mask); dev_priv->display.crtc_disable(crtc); @@ -13160,46 +13249,37 @@ static int intel_atomic_commit(struct drm_device *dev, if (crtc->state->active && (crtc->state->planes_changed || update_pipe)) drm_atomic_helper_commit_planes_on_crtc(old_crtc_state); - - if (pipe_config->base.active && needs_vblank_wait(pipe_config)) - crtc_vblank_mask |= 1 << i; } /* FIXME: add subpixel order */ - if (!state->legacy_cursor_update) - intel_atomic_wait_for_vblanks(dev, dev_priv, crtc_vblank_mask); - - /* - * Now that the vblank has passed, we can go ahead and program the - * optimal watermarks on platforms that need two-step watermark - * programming. - * - * TODO: Move this (and other cleanup) to an async worker eventually. - */ - for_each_crtc_in_state(state, crtc, old_crtc_state, i) { - intel_cstate = to_intel_crtc_state(crtc->state); - - if (dev_priv->display.optimize_watermarks) - dev_priv->display.optimize_watermarks(intel_cstate); - } - for_each_crtc_in_state(state, crtc, old_crtc_state, i) { - intel_post_plane_update(to_intel_crtc_state(old_crtc_state)); - - if (put_domains[i]) - modeset_put_power_domains(dev_priv, put_domains[i]); + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + struct intel_flip_work *work = + intel_state->work[i]; + + if (!work) { + if (!list_empty_careful(&intel_crtc->flip_work)) { + spin_lock_irq(&dev->event_lock); + if (!list_empty(&intel_crtc->flip_work)) + work = list_last_entry(&intel_crtc->flip_work, + struct intel_flip_work, head); + + if (work && work->new_crtc_state == to_intel_crtc_state(old_crtc_state)) { + work->free_new_crtc_state = true; + state->crtc_states[i] = NULL; + state->crtcs[i] = NULL; + } + spin_unlock_irq(&dev->event_lock); + } + continue; + } - intel_modeset_verify_crtc(crtc, old_crtc_state, crtc->state); + intel_state->work[i] = NULL; + intel_prepare_work(crtc, work, state, old_crtc_state); + intel_schedule_update(crtc, intel_state, work); } - if (intel_state->modeset) - intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET); - - mutex_lock(&dev->struct_mutex); - drm_atomic_helper_cleanup_planes(dev, state); - mutex_unlock(&dev->struct_mutex); - drm_atomic_state_free(state); /* As one of the primary mmio accessors, KMS has a high likelihood @@ -13454,6 +13534,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, to_intel_crtc_state(old_crtc_state); bool modeset = needs_modeset(crtc->state); + intel_frontbuffer_flip_prepare(dev, to_intel_crtc_state(crtc->state)->fb_bits); + /* Perform vblank evasion around commit operation */ intel_pipe_update_start(intel_crtc); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index af1a9ab0cc6c..5688e5949a87 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -304,6 +304,8 @@ struct intel_atomic_state { unsigned int active_crtcs; unsigned int min_pixclk[I915_MAX_PIPES]; + struct intel_flip_work *work[I915_MAX_PIPES]; + struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS]; /* @@ -979,7 +981,7 @@ struct intel_flip_work { unsigned put_power_domains; unsigned num_planes; - bool can_async_unpin; + bool can_async_unpin, free_new_crtc_state; unsigned fb_bits; unsigned num_old_connectors, num_new_connectors; @@ -1147,6 +1149,7 @@ unsigned int intel_rotation_info_size(const struct intel_rotation_info *rot_info bool intel_has_pending_fb_unpin(struct drm_device *dev); void intel_mark_busy(struct drm_i915_private *dev_priv); void intel_mark_idle(struct drm_i915_private *dev_priv); +void intel_free_flip_work(struct intel_flip_work *work); void intel_crtc_restore_mode(struct drm_crtc *crtc); int intel_display_suspend(struct drm_device *dev); void intel_encoder_destroy(struct drm_encoder *encoder);
All of intel_post_plane_update is handled there now, so move it over. This is run after the hw state checker because it can't handle checking crtc's separately yet. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_atomic.c | 11 ++ drivers/gpu/drm/i915/intel_display.c | 344 ++++++++++++++++++++++------------- drivers/gpu/drm/i915/intel_drv.h | 5 +- 3 files changed, 228 insertions(+), 132 deletions(-)