* [PATCH RFC 0/5] Convert planes and crtc state updates to atomic.
@ 2015-04-22 11:24 maarten.lankhorst
2015-04-22 11:24 ` [PATCH RFC 1/5] drm/i915: Get rid of intel_crtc_disable and related code maarten.lankhorst
` (5 more replies)
0 siblings, 6 replies; 14+ messages in thread
From: maarten.lankhorst @ 2015-04-22 11:24 UTC (permalink / raw)
To: intel-gfx; +Cc: Ander Conselvan de Oliveira
From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
This applies on top of the patch series:
"[PATCH 00/35] Make legacy modeset a lot more atomic-like"
after the last patch crtc will no longer be enabled/disabled except through
atomic updates, which should make it a lot easier to rely on the atomic state
in the future.
Maarten Lankhorst (5):
drm/i915: Get rid of intel_crtc_disable and related code.
drm/i915: Only update required power domains.
drm/i915: use intel_crtc_control everywhere
drm/i915: make plane helpers fully atomic
drm/i915: Implement intel_crtc_toggle using atomic state
drivers/gpu/drm/i915/i915_debugfs.c | 18 +-
drivers/gpu/drm/i915/i915_drv.h | 1 -
drivers/gpu/drm/i915/intel_atomic_plane.c | 49 +-
drivers/gpu/drm/i915/intel_display.c | 796 ++++++++++++++++--------------
drivers/gpu/drm/i915/intel_drv.h | 2 -
drivers/gpu/drm/i915/intel_sprite.c | 33 +-
6 files changed, 467 insertions(+), 432 deletions(-)
--
2.1.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH RFC 1/5] drm/i915: Get rid of intel_crtc_disable and related code. 2015-04-22 11:24 [PATCH RFC 0/5] Convert planes and crtc state updates to atomic maarten.lankhorst @ 2015-04-22 11:24 ` maarten.lankhorst 2015-04-24 8:46 ` Ander Conselvan De Oliveira 2015-04-22 11:24 ` [PATCH RFC 2/5] drm/i915: Only update required power domains maarten.lankhorst ` (4 subsequent siblings) 5 siblings, 1 reply; 14+ messages in thread From: maarten.lankhorst @ 2015-04-22 11:24 UTC (permalink / raw) To: intel-gfx; +Cc: Ander Conselvan de Oliveira From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Now that the dpll updates are (mostly) atomic, the .off() code is no longer used, and there are no more callers for intel_put_shared_dpll. Move all the updates done in intel_crtc_disable to intel_modeset_update_state, one less special case to worry about. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 1 - drivers/gpu/drm/i915/intel_display.c | 100 +++++------------------------------ drivers/gpu/drm/i915/intel_drv.h | 1 - 3 files changed, 14 insertions(+), 88 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 61b756bdbaad..9e62926e71f0 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -572,7 +572,6 @@ struct drm_i915_display_funcs { struct intel_crtc_state *crtc_state); void (*crtc_enable)(struct drm_crtc *crtc); void (*crtc_disable)(struct drm_crtc *crtc); - void (*off)(struct drm_crtc *crtc); void (*audio_codec_enable)(struct drm_connector *connector, struct intel_encoder *encoder, struct drm_display_mode *mode); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 35fde239c200..92d54dd30d7e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4123,27 +4123,6 @@ static void lpt_pch_enable(struct drm_crtc *crtc) lpt_enable_pch_transcoder(dev_priv, cpu_transcoder); } -void intel_put_shared_dpll(struct intel_crtc *crtc) -{ - struct intel_shared_dpll *pll = intel_crtc_to_shared_dpll(crtc); - - if (pll == NULL) - return; - - if (!(pll->config.crtc_mask & (1 << crtc->pipe))) { - WARN(1, "bad %s crtc mask\n", pll->name); - return; - } - - pll->config.crtc_mask &= ~(1 << crtc->pipe); - if (pll->config.crtc_mask == 0) { - WARN_ON(pll->on); - WARN_ON(pll->active); - } - - crtc->config->shared_dpll = DPLL_ID_PRIVATE; -} - struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) { @@ -5089,13 +5068,6 @@ static void haswell_crtc_disable(struct drm_crtc *crtc) intel_disable_shared_dpll(intel_crtc); } -static void ironlake_crtc_off(struct drm_crtc *crtc) -{ - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - intel_put_shared_dpll(intel_crtc); -} - - static void i9xx_pfit_enable(struct intel_crtc *crtc) { struct drm_device *dev = crtc->base.dev; @@ -5722,10 +5694,6 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) mutex_unlock(&dev->struct_mutex); } -static void i9xx_crtc_off(struct drm_crtc *crtc) -{ -} - /* Master function to enable/disable CRTC and corresponding power wells */ void intel_crtc_control(struct drm_crtc *crtc, bool enable) { @@ -5775,34 +5743,6 @@ void intel_crtc_update_dpms(struct drm_crtc *crtc) crtc->state->active = enable; } -static void intel_crtc_disable(struct drm_crtc *crtc) -{ - struct drm_device *dev = crtc->dev; - struct drm_connector *connector; - struct drm_i915_private *dev_priv = dev->dev_private; - - /* crtc should still be enabled when we disable it. */ - WARN_ON(!crtc->state->enable); - - intel_crtc_disable_planes(crtc); - dev_priv->display.crtc_disable(crtc); - dev_priv->display.off(crtc); - - drm_plane_helper_disable(crtc->primary); - - /* Update computed state. */ - list_for_each_entry(connector, &dev->mode_config.connector_list, head) { - if (!connector->encoder || !connector->encoder->crtc) - continue; - - if (connector->encoder->crtc != crtc) - continue; - - connector->dpms = DRM_MODE_DPMS_OFF; - to_intel_encoder(connector->encoder)->connectors_active = false; - } -} - void intel_encoder_destroy(struct drm_encoder *encoder) { struct intel_encoder *intel_encoder = to_intel_encoder(encoder); @@ -11219,7 +11159,6 @@ intel_modeset_update_state(struct drm_atomic_state *state) struct drm_crtc *crtc; struct drm_crtc_state *crtc_state; struct drm_connector *connector; - int i; intel_shared_dpll_commit(dev_priv); @@ -11227,15 +11166,12 @@ intel_modeset_update_state(struct drm_atomic_state *state) if (!intel_encoder->base.crtc) continue; - for_each_crtc_in_state(state, crtc, crtc_state, i) - if (crtc == intel_encoder->base.crtc) - break; - - if (crtc != intel_encoder->base.crtc) + crtc = intel_encoder->base.crtc; + crtc_state = state->crtc_states[drm_crtc_index(crtc)]; + if (!crtc_state || !needs_modeset(crtc->state)) continue; - if (crtc_state->enable && needs_modeset(crtc_state)) - intel_encoder->connectors_active = false; + intel_encoder->connectors_active = false; } drm_atomic_helper_swap_state(state->dev, state); @@ -11250,14 +11186,12 @@ intel_modeset_update_state(struct drm_atomic_state *state) if (!connector->encoder || !connector->encoder->crtc) continue; - for_each_crtc_in_state(state, crtc, crtc_state, i) - if (crtc == connector->encoder->crtc) - break; - - if (crtc != connector->encoder->crtc) + crtc = connector->encoder->crtc; + crtc_state = state->crtc_states[drm_crtc_index(crtc)]; + if (!crtc_state || !needs_modeset(crtc->state)) continue; - if (crtc->state->enable && needs_modeset(crtc->state)) { + if (crtc->state->enable) { struct drm_property *dpms_property = dev->mode_config.dpms_property; @@ -11268,7 +11202,8 @@ intel_modeset_update_state(struct drm_atomic_state *state) intel_encoder = to_intel_encoder(connector->encoder); intel_encoder->connectors_active = true; - } + } else + connector->dpms = DRM_MODE_DPMS_OFF; } } @@ -11946,12 +11881,10 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, if (!needs_modeset(crtc_state)) continue; - if (!crtc_state->enable) { - intel_crtc_disable(crtc); - } else if (crtc->state->enable) { - intel_crtc_disable_planes(crtc); - dev_priv->display.crtc_disable(crtc); - } + intel_crtc_disable_planes(crtc); + dev_priv->display.crtc_disable(crtc); + if (!crtc_state->enable) + drm_plane_helper_disable(crtc->primary); } /* crtc->mode is already used by the ->mode_set callbacks, hence we need @@ -13615,7 +13548,6 @@ static void intel_init_display(struct drm_device *dev) haswell_crtc_compute_clock; dev_priv->display.crtc_enable = haswell_crtc_enable; dev_priv->display.crtc_disable = haswell_crtc_disable; - dev_priv->display.off = ironlake_crtc_off; dev_priv->display.update_primary_plane = skylake_update_primary_plane; } else if (HAS_DDI(dev)) { @@ -13626,7 +13558,6 @@ static void intel_init_display(struct drm_device *dev) haswell_crtc_compute_clock; dev_priv->display.crtc_enable = haswell_crtc_enable; dev_priv->display.crtc_disable = haswell_crtc_disable; - dev_priv->display.off = ironlake_crtc_off; dev_priv->display.update_primary_plane = ironlake_update_primary_plane; } else if (HAS_PCH_SPLIT(dev)) { @@ -13637,7 +13568,6 @@ static void intel_init_display(struct drm_device *dev) ironlake_crtc_compute_clock; dev_priv->display.crtc_enable = ironlake_crtc_enable; dev_priv->display.crtc_disable = ironlake_crtc_disable; - dev_priv->display.off = ironlake_crtc_off; dev_priv->display.update_primary_plane = ironlake_update_primary_plane; } else if (IS_VALLEYVIEW(dev)) { @@ -13647,7 +13577,6 @@ static void intel_init_display(struct drm_device *dev) dev_priv->display.crtc_compute_clock = i9xx_crtc_compute_clock; dev_priv->display.crtc_enable = valleyview_crtc_enable; dev_priv->display.crtc_disable = i9xx_crtc_disable; - dev_priv->display.off = i9xx_crtc_off; dev_priv->display.update_primary_plane = i9xx_update_primary_plane; } else { @@ -13657,7 +13586,6 @@ static void intel_init_display(struct drm_device *dev) dev_priv->display.crtc_compute_clock = i9xx_crtc_compute_clock; dev_priv->display.crtc_enable = i9xx_crtc_enable; dev_priv->display.crtc_disable = i9xx_crtc_disable; - dev_priv->display.off = i9xx_crtc_off; dev_priv->display.update_primary_plane = i9xx_update_primary_plane; } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 70b70e9be167..fb89f5f3498c 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1078,7 +1078,6 @@ void assert_shared_dpll(struct drm_i915_private *dev_priv, #define assert_shared_dpll_disabled(d, p) assert_shared_dpll(d, p, false) struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc, struct intel_crtc_state *state); -void intel_put_shared_dpll(struct intel_crtc *crtc); void vlv_force_pll_on(struct drm_device *dev, enum pipe pipe, const struct dpll *dpll); -- 2.1.0 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 1/5] drm/i915: Get rid of intel_crtc_disable and related code. 2015-04-22 11:24 ` [PATCH RFC 1/5] drm/i915: Get rid of intel_crtc_disable and related code maarten.lankhorst @ 2015-04-24 8:46 ` Ander Conselvan De Oliveira 0 siblings, 0 replies; 14+ messages in thread From: Ander Conselvan De Oliveira @ 2015-04-24 8:46 UTC (permalink / raw) To: maarten.lankhorst; +Cc: intel-gfx On Wed, 2015-04-22 at 13:24 +0200, maarten.lankhorst@linux.intel.com wrote: > From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Now that the dpll updates are (mostly) atomic, the .off() code is no longer used, > and there are no more callers for intel_put_shared_dpll. This is a bit confusing since until this patch the .off() code is actually called from intel_crtc_disable, which calls intel_put_shared_dpll(). I think what you intended to say here is that intel_shared_dpll_commit() has the same effect as put_shared_dpll(), so it is safe to remove that call, at which point the .off() hook is not necessary anymore. > Move all the updates > done in intel_crtc_disable to intel_modeset_update_state, one less special case > to worry about. Maybe split this to a separate patch? Ander > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 - > drivers/gpu/drm/i915/intel_display.c | 100 +++++------------------------------ > drivers/gpu/drm/i915/intel_drv.h | 1 - > 3 files changed, 14 insertions(+), 88 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 61b756bdbaad..9e62926e71f0 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -572,7 +572,6 @@ struct drm_i915_display_funcs { > struct intel_crtc_state *crtc_state); > void (*crtc_enable)(struct drm_crtc *crtc); > void (*crtc_disable)(struct drm_crtc *crtc); > - void (*off)(struct drm_crtc *crtc); > void (*audio_codec_enable)(struct drm_connector *connector, > struct intel_encoder *encoder, > struct drm_display_mode *mode); > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 35fde239c200..92d54dd30d7e 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4123,27 +4123,6 @@ static void lpt_pch_enable(struct drm_crtc *crtc) > lpt_enable_pch_transcoder(dev_priv, cpu_transcoder); > } > > -void intel_put_shared_dpll(struct intel_crtc *crtc) > -{ > - struct intel_shared_dpll *pll = intel_crtc_to_shared_dpll(crtc); > - > - if (pll == NULL) > - return; > - > - if (!(pll->config.crtc_mask & (1 << crtc->pipe))) { > - WARN(1, "bad %s crtc mask\n", pll->name); > - return; > - } > - > - pll->config.crtc_mask &= ~(1 << crtc->pipe); > - if (pll->config.crtc_mask == 0) { > - WARN_ON(pll->on); > - WARN_ON(pll->active); > - } > - > - crtc->config->shared_dpll = DPLL_ID_PRIVATE; > -} > - > struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc, > struct intel_crtc_state *crtc_state) > { > @@ -5089,13 +5068,6 @@ static void haswell_crtc_disable(struct drm_crtc *crtc) > intel_disable_shared_dpll(intel_crtc); > } > > -static void ironlake_crtc_off(struct drm_crtc *crtc) > -{ > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > - intel_put_shared_dpll(intel_crtc); > -} > - > - > static void i9xx_pfit_enable(struct intel_crtc *crtc) > { > struct drm_device *dev = crtc->base.dev; > @@ -5722,10 +5694,6 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) > mutex_unlock(&dev->struct_mutex); > } > > -static void i9xx_crtc_off(struct drm_crtc *crtc) > -{ > -} > - > /* Master function to enable/disable CRTC and corresponding power wells */ > void intel_crtc_control(struct drm_crtc *crtc, bool enable) > { > @@ -5775,34 +5743,6 @@ void intel_crtc_update_dpms(struct drm_crtc *crtc) > crtc->state->active = enable; > } > > -static void intel_crtc_disable(struct drm_crtc *crtc) > -{ > - struct drm_device *dev = crtc->dev; > - struct drm_connector *connector; > - struct drm_i915_private *dev_priv = dev->dev_private; > - > - /* crtc should still be enabled when we disable it. */ > - WARN_ON(!crtc->state->enable); > - > - intel_crtc_disable_planes(crtc); > - dev_priv->display.crtc_disable(crtc); > - dev_priv->display.off(crtc); > - > - drm_plane_helper_disable(crtc->primary); > - > - /* Update computed state. */ > - list_for_each_entry(connector, &dev->mode_config.connector_list, head) { > - if (!connector->encoder || !connector->encoder->crtc) > - continue; > - > - if (connector->encoder->crtc != crtc) > - continue; > - > - connector->dpms = DRM_MODE_DPMS_OFF; > - to_intel_encoder(connector->encoder)->connectors_active = false; > - } > -} > - > void intel_encoder_destroy(struct drm_encoder *encoder) > { > struct intel_encoder *intel_encoder = to_intel_encoder(encoder); > @@ -11219,7 +11159,6 @@ intel_modeset_update_state(struct drm_atomic_state *state) > struct drm_crtc *crtc; > struct drm_crtc_state *crtc_state; > struct drm_connector *connector; > - int i; > > intel_shared_dpll_commit(dev_priv); > > @@ -11227,15 +11166,12 @@ intel_modeset_update_state(struct drm_atomic_state *state) > if (!intel_encoder->base.crtc) > continue; > > - for_each_crtc_in_state(state, crtc, crtc_state, i) > - if (crtc == intel_encoder->base.crtc) > - break; > - > - if (crtc != intel_encoder->base.crtc) > + crtc = intel_encoder->base.crtc; > + crtc_state = state->crtc_states[drm_crtc_index(crtc)]; > + if (!crtc_state || !needs_modeset(crtc->state)) > continue; > > - if (crtc_state->enable && needs_modeset(crtc_state)) > - intel_encoder->connectors_active = false; > + intel_encoder->connectors_active = false; > } > > drm_atomic_helper_swap_state(state->dev, state); > @@ -11250,14 +11186,12 @@ intel_modeset_update_state(struct drm_atomic_state *state) > if (!connector->encoder || !connector->encoder->crtc) > continue; > > - for_each_crtc_in_state(state, crtc, crtc_state, i) > - if (crtc == connector->encoder->crtc) > - break; > - > - if (crtc != connector->encoder->crtc) > + crtc = connector->encoder->crtc; > + crtc_state = state->crtc_states[drm_crtc_index(crtc)]; > + if (!crtc_state || !needs_modeset(crtc->state)) > continue; > > - if (crtc->state->enable && needs_modeset(crtc->state)) { > + if (crtc->state->enable) { > struct drm_property *dpms_property = > dev->mode_config.dpms_property; > > @@ -11268,7 +11202,8 @@ intel_modeset_update_state(struct drm_atomic_state *state) > > intel_encoder = to_intel_encoder(connector->encoder); > intel_encoder->connectors_active = true; > - } > + } else > + connector->dpms = DRM_MODE_DPMS_OFF; > } > > } > @@ -11946,12 +11881,10 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, > if (!needs_modeset(crtc_state)) > continue; > > - if (!crtc_state->enable) { > - intel_crtc_disable(crtc); > - } else if (crtc->state->enable) { > - intel_crtc_disable_planes(crtc); > - dev_priv->display.crtc_disable(crtc); > - } > + intel_crtc_disable_planes(crtc); > + dev_priv->display.crtc_disable(crtc); > + if (!crtc_state->enable) > + drm_plane_helper_disable(crtc->primary); > } > > /* crtc->mode is already used by the ->mode_set callbacks, hence we need > @@ -13615,7 +13548,6 @@ static void intel_init_display(struct drm_device *dev) > haswell_crtc_compute_clock; > dev_priv->display.crtc_enable = haswell_crtc_enable; > dev_priv->display.crtc_disable = haswell_crtc_disable; > - dev_priv->display.off = ironlake_crtc_off; > dev_priv->display.update_primary_plane = > skylake_update_primary_plane; > } else if (HAS_DDI(dev)) { > @@ -13626,7 +13558,6 @@ static void intel_init_display(struct drm_device *dev) > haswell_crtc_compute_clock; > dev_priv->display.crtc_enable = haswell_crtc_enable; > dev_priv->display.crtc_disable = haswell_crtc_disable; > - dev_priv->display.off = ironlake_crtc_off; > dev_priv->display.update_primary_plane = > ironlake_update_primary_plane; > } else if (HAS_PCH_SPLIT(dev)) { > @@ -13637,7 +13568,6 @@ static void intel_init_display(struct drm_device *dev) > ironlake_crtc_compute_clock; > dev_priv->display.crtc_enable = ironlake_crtc_enable; > dev_priv->display.crtc_disable = ironlake_crtc_disable; > - dev_priv->display.off = ironlake_crtc_off; > dev_priv->display.update_primary_plane = > ironlake_update_primary_plane; > } else if (IS_VALLEYVIEW(dev)) { > @@ -13647,7 +13577,6 @@ static void intel_init_display(struct drm_device *dev) > dev_priv->display.crtc_compute_clock = i9xx_crtc_compute_clock; > dev_priv->display.crtc_enable = valleyview_crtc_enable; > dev_priv->display.crtc_disable = i9xx_crtc_disable; > - dev_priv->display.off = i9xx_crtc_off; > dev_priv->display.update_primary_plane = > i9xx_update_primary_plane; > } else { > @@ -13657,7 +13586,6 @@ static void intel_init_display(struct drm_device *dev) > dev_priv->display.crtc_compute_clock = i9xx_crtc_compute_clock; > dev_priv->display.crtc_enable = i9xx_crtc_enable; > dev_priv->display.crtc_disable = i9xx_crtc_disable; > - dev_priv->display.off = i9xx_crtc_off; > dev_priv->display.update_primary_plane = > i9xx_update_primary_plane; > } > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 70b70e9be167..fb89f5f3498c 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1078,7 +1078,6 @@ void assert_shared_dpll(struct drm_i915_private *dev_priv, > #define assert_shared_dpll_disabled(d, p) assert_shared_dpll(d, p, false) > struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc, > struct intel_crtc_state *state); > -void intel_put_shared_dpll(struct intel_crtc *crtc); > > void vlv_force_pll_on(struct drm_device *dev, enum pipe pipe, > const struct dpll *dpll); _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH RFC 2/5] drm/i915: Only update required power domains. 2015-04-22 11:24 [PATCH RFC 0/5] Convert planes and crtc state updates to atomic maarten.lankhorst 2015-04-22 11:24 ` [PATCH RFC 1/5] drm/i915: Get rid of intel_crtc_disable and related code maarten.lankhorst @ 2015-04-22 11:24 ` maarten.lankhorst 2015-04-24 8:47 ` Ander Conselvan De Oliveira 2015-04-22 11:24 ` [PATCH RFC 3/5] drm/i915: use intel_crtc_control everywhere maarten.lankhorst ` (3 subsequent siblings) 5 siblings, 1 reply; 14+ messages in thread From: maarten.lankhorst @ 2015-04-22 11:24 UTC (permalink / raw) To: intel-gfx; +Cc: Ander Conselvan de Oliveira From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> This prevents unnecessarily updating power domains, while still enabling all power domains on initial setup. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 52 ++++++++++++++++++++++++++++-------- 1 file changed, 41 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 92d54dd30d7e..438d8e213748 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5163,36 +5163,72 @@ static unsigned long get_crtc_power_domains(struct drm_crtc *crtc) return mask; } +static bool +needs_modeset(struct drm_crtc_state *state) +{ + return state->mode_changed || state->active_changed; +} + static void modeset_update_crtc_power_domains(struct drm_atomic_state *state) { struct drm_device *dev = state->dev; struct drm_i915_private *dev_priv = dev->dev_private; unsigned long pipe_domains[I915_MAX_PIPES] = { 0, }; struct intel_crtc *crtc; + bool init_power = dev_priv->power_domains.init_power_on; + bool any_power = init_power, any_modeset = false; + unsigned long domains; /* * First get all needed power domains, then put all unneeded, to avoid * any unnecessary toggling of the power wells. */ for_each_intel_crtc(dev, crtc) { + int idx = drm_crtc_index(&crtc->base); + struct drm_crtc_state *crtc_state = state->crtc_states[idx]; enum intel_display_power_domain domain; - if (!crtc->base.state->enable) + if (!init_power && !crtc_state) + continue; + + if (needs_modeset(crtc->base.state)) + any_modeset = true; + + if (crtc->base.state->enable) + pipe_domains[crtc->pipe] = + get_crtc_power_domains(&crtc->base); + + if (pipe_domains[crtc->pipe] == crtc->enabled_power_domains) continue; - pipe_domains[crtc->pipe] = get_crtc_power_domains(&crtc->base); + WARN_ON(!init_power && !needs_modeset(crtc->base.state)); + + any_power = true; + domains = pipe_domains[crtc->pipe] & + ~crtc->enabled_power_domains; - for_each_power_domain(domain, pipe_domains[crtc->pipe]) + for_each_power_domain(domain, domains) intel_display_power_get(dev_priv, domain); } - if (dev_priv->display.modeset_global_resources) + if (any_modeset && dev_priv->display.modeset_global_resources) dev_priv->display.modeset_global_resources(state); + if (!any_power) + return; + for_each_intel_crtc(dev, crtc) { + int idx = drm_crtc_index(&crtc->base); + struct drm_crtc_state *crtc_state = state->crtc_states[idx]; enum intel_display_power_domain domain; - for_each_power_domain(domain, crtc->enabled_power_domains) + if (!init_power && !crtc_state) + continue; + + domains = crtc->enabled_power_domains & + ~pipe_domains[crtc->pipe]; + + for_each_power_domain(domain, domains) intel_display_power_put(dev_priv, domain); crtc->enabled_power_domains = pipe_domains[crtc->pipe]; @@ -11144,12 +11180,6 @@ static bool intel_crtc_in_use(struct drm_crtc *crtc) return false; } -static bool -needs_modeset(struct drm_crtc_state *state) -{ - return state->mode_changed || state->active_changed; -} - static void intel_modeset_update_state(struct drm_atomic_state *state) { -- 2.1.0 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 2/5] drm/i915: Only update required power domains. 2015-04-22 11:24 ` [PATCH RFC 2/5] drm/i915: Only update required power domains maarten.lankhorst @ 2015-04-24 8:47 ` Ander Conselvan De Oliveira 0 siblings, 0 replies; 14+ messages in thread From: Ander Conselvan De Oliveira @ 2015-04-24 8:47 UTC (permalink / raw) To: maarten.lankhorst; +Cc: intel-gfx On Wed, 2015-04-22 at 13:24 +0200, maarten.lankhorst@linux.intel.com wrote: > From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > This prevents unnecessarily updating power domains, while still > enabling all power domains on initial setup. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 52 ++++++++++++++++++++++++++++-------- > 1 file changed, 41 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 92d54dd30d7e..438d8e213748 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -5163,36 +5163,72 @@ static unsigned long get_crtc_power_domains(struct drm_crtc *crtc) > return mask; > } > > +static bool > +needs_modeset(struct drm_crtc_state *state) > +{ > + return state->mode_changed || state->active_changed; > +} > + > static void modeset_update_crtc_power_domains(struct drm_atomic_state *state) > { > struct drm_device *dev = state->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > unsigned long pipe_domains[I915_MAX_PIPES] = { 0, }; > struct intel_crtc *crtc; > + bool init_power = dev_priv->power_domains.init_power_on; > + bool any_power = init_power, any_modeset = false; > + unsigned long domains; > > /* > * First get all needed power domains, then put all unneeded, to avoid > * any unnecessary toggling of the power wells. > */ > for_each_intel_crtc(dev, crtc) { > + int idx = drm_crtc_index(&crtc->base); > + struct drm_crtc_state *crtc_state = state->crtc_states[idx]; > enum intel_display_power_domain domain; > > - if (!crtc->base.state->enable) > + if (!init_power && !crtc_state) > + continue; > + > + if (needs_modeset(crtc->base.state)) > + any_modeset = true; > + > + if (crtc->base.state->enable) > + pipe_domains[crtc->pipe] = > + get_crtc_power_domains(&crtc->base); > + > + if (pipe_domains[crtc->pipe] == crtc->enabled_power_domains) > continue; > > - pipe_domains[crtc->pipe] = get_crtc_power_domains(&crtc->base); > + WARN_ON(!init_power && !needs_modeset(crtc->base.state)); > + > + any_power = true; > + domains = pipe_domains[crtc->pipe] & > + ~crtc->enabled_power_domains; > > - for_each_power_domain(domain, pipe_domains[crtc->pipe]) > + for_each_power_domain(domain, domains) > intel_display_power_get(dev_priv, domain); Isn't intel_display_power_get() already a no-op if the power domain is already active, except for the reference counting? Or did I miss something? Ander > } > > - if (dev_priv->display.modeset_global_resources) > + if (any_modeset && dev_priv->display.modeset_global_resources) > dev_priv->display.modeset_global_resources(state); > > + if (!any_power) > + return; > + > for_each_intel_crtc(dev, crtc) { > + int idx = drm_crtc_index(&crtc->base); > + struct drm_crtc_state *crtc_state = state->crtc_states[idx]; > enum intel_display_power_domain domain; > > - for_each_power_domain(domain, crtc->enabled_power_domains) > + if (!init_power && !crtc_state) > + continue; > + > + domains = crtc->enabled_power_domains & > + ~pipe_domains[crtc->pipe]; > + > + for_each_power_domain(domain, domains) > intel_display_power_put(dev_priv, domain); > > crtc->enabled_power_domains = pipe_domains[crtc->pipe]; > @@ -11144,12 +11180,6 @@ static bool intel_crtc_in_use(struct drm_crtc *crtc) > return false; > } > > -static bool > -needs_modeset(struct drm_crtc_state *state) > -{ > - return state->mode_changed || state->active_changed; > -} > - > static void > intel_modeset_update_state(struct drm_atomic_state *state) > { _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH RFC 3/5] drm/i915: use intel_crtc_control everywhere 2015-04-22 11:24 [PATCH RFC 0/5] Convert planes and crtc state updates to atomic maarten.lankhorst 2015-04-22 11:24 ` [PATCH RFC 1/5] drm/i915: Get rid of intel_crtc_disable and related code maarten.lankhorst 2015-04-22 11:24 ` [PATCH RFC 2/5] drm/i915: Only update required power domains maarten.lankhorst @ 2015-04-22 11:24 ` maarten.lankhorst 2015-05-04 13:44 ` Daniel Vetter 2015-04-22 11:24 ` [PATCH RFC 4/5] drm/i915: make plane helpers fully atomic maarten.lankhorst ` (2 subsequent siblings) 5 siblings, 1 reply; 14+ messages in thread From: maarten.lankhorst @ 2015-04-22 11:24 UTC (permalink / raw) To: intel-gfx; +Cc: Ander Conselvan de Oliveira From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 18 ++++++++-- drivers/gpu/drm/i915/intel_display.c | 68 +++++++++++++----------------------- drivers/gpu/drm/i915/intel_drv.h | 1 - 3 files changed, 39 insertions(+), 48 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 60e86f331313..91c9a4fc8b6a 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -3589,12 +3589,18 @@ static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev) */ if (crtc->config->cpu_transcoder == TRANSCODER_EDP && !crtc->config->pch_pfit.enabled) { + bool active = crtc->active; + + if (active) + intel_crtc_control(&crtc->base, false); + crtc->config->pch_pfit.force_thru = true; intel_display_power_get(dev_priv, POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A)); - intel_crtc_reset(&crtc->base); + if (active) + intel_crtc_control(&crtc->base, true); } drm_modeset_unlock_all(dev); } @@ -3613,12 +3619,18 @@ static void hsw_undo_trans_edp_pipe_A_crc_wa(struct drm_device *dev) * routing. */ if (crtc->config->pch_pfit.force_thru) { - crtc->config->pch_pfit.force_thru = false; + bool active = crtc->active; - intel_crtc_reset(&crtc->base); + if (active) + intel_crtc_control(&crtc->base, false); + + crtc->config->pch_pfit.force_thru = false; intel_display_power_put(dev_priv, POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A)); + + if (active) + intel_crtc_control(&crtc->base, true); } drm_modeset_unlock_all(dev); } diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 438d8e213748..2ffacb4c3a12 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3101,22 +3101,8 @@ static void intel_update_primary_planes(struct drm_device *dev) } } -void intel_crtc_reset(struct intel_crtc *crtc) -{ - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); - - if (!crtc->active) - return; - - intel_crtc_disable_planes(&crtc->base); - dev_priv->display.crtc_disable(&crtc->base); - dev_priv->display.crtc_enable(&crtc->base); - intel_crtc_enable_planes(&crtc->base); -} - void intel_prepare_reset(struct drm_device *dev) { - struct drm_i915_private *dev_priv = to_i915(dev); struct intel_crtc *crtc; /* no reset support for gen2 */ @@ -3128,18 +3114,12 @@ void intel_prepare_reset(struct drm_device *dev) return; drm_modeset_lock_all(dev); - /* * Disabling the crtcs gracefully seems nicer. Also the * g33 docs say we should at least disable all the planes. */ - for_each_intel_crtc(dev, crtc) { - if (!crtc->active) - continue; - - intel_crtc_disable_planes(&crtc->base); - dev_priv->display.crtc_disable(&crtc->base); - } + for_each_intel_crtc(dev, crtc) + intel_crtc_control(&crtc->base, false); } void intel_finish_reset(struct drm_device *dev) @@ -5739,26 +5719,29 @@ void intel_crtc_control(struct drm_crtc *crtc, bool enable) enum intel_display_power_domain domain; unsigned long domains; + if (enable == intel_crtc->active) + return; + + if (enable && !crtc->state->enable) + return; + + crtc->state->active = enable; if (enable) { - if (!intel_crtc->active) { - domains = get_crtc_power_domains(crtc); - for_each_power_domain(domain, domains) - intel_display_power_get(dev_priv, domain); - intel_crtc->enabled_power_domains = domains; - - dev_priv->display.crtc_enable(crtc); - intel_crtc_enable_planes(crtc); - } + domains = get_crtc_power_domains(crtc); + for_each_power_domain(domain, domains) + intel_display_power_get(dev_priv, domain); + intel_crtc->enabled_power_domains = domains; + + dev_priv->display.crtc_enable(crtc); + intel_crtc_enable_planes(crtc); } else { - if (intel_crtc->active) { - intel_crtc_disable_planes(crtc); - dev_priv->display.crtc_disable(crtc); - - domains = intel_crtc->enabled_power_domains; - for_each_power_domain(domain, domains) - intel_display_power_put(dev_priv, domain); - intel_crtc->enabled_power_domains = 0; - } + intel_crtc_disable_planes(crtc); + dev_priv->display.crtc_disable(crtc); + + domains = intel_crtc->enabled_power_domains; + for_each_power_domain(domain, domains) + intel_display_power_put(dev_priv, domain); + intel_crtc->enabled_power_domains = 0; } } @@ -5775,8 +5758,6 @@ void intel_crtc_update_dpms(struct drm_crtc *crtc) enable |= intel_encoder->connectors_active; intel_crtc_control(crtc, enable); - - crtc->state->active = enable; } void intel_encoder_destroy(struct drm_encoder *encoder) @@ -14087,8 +14068,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) plane = crtc->plane; to_intel_plane_state(crtc->base.primary->state)->visible = true; crtc->plane = !plane; - intel_crtc_disable_planes(&crtc->base); - dev_priv->display.crtc_disable(&crtc->base); + intel_crtc_control(&crtc->base, false); crtc->plane = plane; /* ... and break all links. */ diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index fb89f5f3498c..9668b17d7e0e 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -987,7 +987,6 @@ void intel_mark_busy(struct drm_device *dev); void intel_mark_idle(struct drm_device *dev); void intel_crtc_restore_mode(struct drm_crtc *crtc); void intel_crtc_control(struct drm_crtc *crtc, bool enable); -void intel_crtc_reset(struct intel_crtc *crtc); void intel_crtc_update_dpms(struct drm_crtc *crtc); void intel_encoder_destroy(struct drm_encoder *encoder); int intel_connector_init(struct intel_connector *); -- 2.1.0 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 3/5] drm/i915: use intel_crtc_control everywhere 2015-04-22 11:24 ` [PATCH RFC 3/5] drm/i915: use intel_crtc_control everywhere maarten.lankhorst @ 2015-05-04 13:44 ` Daniel Vetter 0 siblings, 0 replies; 14+ messages in thread From: Daniel Vetter @ 2015-05-04 13:44 UTC (permalink / raw) To: maarten.lankhorst; +Cc: Ander Conselvan de Oliveira, intel-gfx On Wed, Apr 22, 2015 at 01:24:20PM +0200, maarten.lankhorst@linux.intel.com wrote: > From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> A bit more explanation in the commit message here would be useful, i.e. what changed that we need this now. -Daniel > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 18 ++++++++-- > drivers/gpu/drm/i915/intel_display.c | 68 +++++++++++++----------------------- > drivers/gpu/drm/i915/intel_drv.h | 1 - > 3 files changed, 39 insertions(+), 48 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 60e86f331313..91c9a4fc8b6a 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -3589,12 +3589,18 @@ static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev) > */ > if (crtc->config->cpu_transcoder == TRANSCODER_EDP && > !crtc->config->pch_pfit.enabled) { > + bool active = crtc->active; > + > + if (active) > + intel_crtc_control(&crtc->base, false); > + > crtc->config->pch_pfit.force_thru = true; > > intel_display_power_get(dev_priv, > POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A)); > > - intel_crtc_reset(&crtc->base); > + if (active) > + intel_crtc_control(&crtc->base, true); > } > drm_modeset_unlock_all(dev); > } > @@ -3613,12 +3619,18 @@ static void hsw_undo_trans_edp_pipe_A_crc_wa(struct drm_device *dev) > * routing. > */ > if (crtc->config->pch_pfit.force_thru) { > - crtc->config->pch_pfit.force_thru = false; > + bool active = crtc->active; > > - intel_crtc_reset(&crtc->base); > + if (active) > + intel_crtc_control(&crtc->base, false); > + > + crtc->config->pch_pfit.force_thru = false; > > intel_display_power_put(dev_priv, > POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A)); > + > + if (active) > + intel_crtc_control(&crtc->base, true); > } > drm_modeset_unlock_all(dev); > } > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 438d8e213748..2ffacb4c3a12 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3101,22 +3101,8 @@ static void intel_update_primary_planes(struct drm_device *dev) > } > } > > -void intel_crtc_reset(struct intel_crtc *crtc) > -{ > - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > - > - if (!crtc->active) > - return; > - > - intel_crtc_disable_planes(&crtc->base); > - dev_priv->display.crtc_disable(&crtc->base); > - dev_priv->display.crtc_enable(&crtc->base); > - intel_crtc_enable_planes(&crtc->base); > -} > - > void intel_prepare_reset(struct drm_device *dev) > { > - struct drm_i915_private *dev_priv = to_i915(dev); > struct intel_crtc *crtc; > > /* no reset support for gen2 */ > @@ -3128,18 +3114,12 @@ void intel_prepare_reset(struct drm_device *dev) > return; > > drm_modeset_lock_all(dev); > - > /* > * Disabling the crtcs gracefully seems nicer. Also the > * g33 docs say we should at least disable all the planes. > */ > - for_each_intel_crtc(dev, crtc) { > - if (!crtc->active) > - continue; > - > - intel_crtc_disable_planes(&crtc->base); > - dev_priv->display.crtc_disable(&crtc->base); > - } > + for_each_intel_crtc(dev, crtc) > + intel_crtc_control(&crtc->base, false); > } > > void intel_finish_reset(struct drm_device *dev) > @@ -5739,26 +5719,29 @@ void intel_crtc_control(struct drm_crtc *crtc, bool enable) > enum intel_display_power_domain domain; > unsigned long domains; > > + if (enable == intel_crtc->active) > + return; > + > + if (enable && !crtc->state->enable) > + return; > + > + crtc->state->active = enable; > if (enable) { > - if (!intel_crtc->active) { > - domains = get_crtc_power_domains(crtc); > - for_each_power_domain(domain, domains) > - intel_display_power_get(dev_priv, domain); > - intel_crtc->enabled_power_domains = domains; > - > - dev_priv->display.crtc_enable(crtc); > - intel_crtc_enable_planes(crtc); > - } > + domains = get_crtc_power_domains(crtc); > + for_each_power_domain(domain, domains) > + intel_display_power_get(dev_priv, domain); > + intel_crtc->enabled_power_domains = domains; > + > + dev_priv->display.crtc_enable(crtc); > + intel_crtc_enable_planes(crtc); > } else { > - if (intel_crtc->active) { > - intel_crtc_disable_planes(crtc); > - dev_priv->display.crtc_disable(crtc); > - > - domains = intel_crtc->enabled_power_domains; > - for_each_power_domain(domain, domains) > - intel_display_power_put(dev_priv, domain); > - intel_crtc->enabled_power_domains = 0; > - } > + intel_crtc_disable_planes(crtc); > + dev_priv->display.crtc_disable(crtc); > + > + domains = intel_crtc->enabled_power_domains; > + for_each_power_domain(domain, domains) > + intel_display_power_put(dev_priv, domain); > + intel_crtc->enabled_power_domains = 0; > } > } > > @@ -5775,8 +5758,6 @@ void intel_crtc_update_dpms(struct drm_crtc *crtc) > enable |= intel_encoder->connectors_active; > > intel_crtc_control(crtc, enable); > - > - crtc->state->active = enable; > } > > void intel_encoder_destroy(struct drm_encoder *encoder) > @@ -14087,8 +14068,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) > plane = crtc->plane; > to_intel_plane_state(crtc->base.primary->state)->visible = true; > crtc->plane = !plane; > - intel_crtc_disable_planes(&crtc->base); > - dev_priv->display.crtc_disable(&crtc->base); > + intel_crtc_control(&crtc->base, false); > crtc->plane = plane; > > /* ... and break all links. */ > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index fb89f5f3498c..9668b17d7e0e 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -987,7 +987,6 @@ void intel_mark_busy(struct drm_device *dev); > void intel_mark_idle(struct drm_device *dev); > void intel_crtc_restore_mode(struct drm_crtc *crtc); > void intel_crtc_control(struct drm_crtc *crtc, bool enable); > -void intel_crtc_reset(struct intel_crtc *crtc); > void intel_crtc_update_dpms(struct drm_crtc *crtc); > void intel_encoder_destroy(struct drm_encoder *encoder); > int intel_connector_init(struct intel_connector *); > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH RFC 4/5] drm/i915: make plane helpers fully atomic 2015-04-22 11:24 [PATCH RFC 0/5] Convert planes and crtc state updates to atomic maarten.lankhorst ` (2 preceding siblings ...) 2015-04-22 11:24 ` [PATCH RFC 3/5] drm/i915: use intel_crtc_control everywhere maarten.lankhorst @ 2015-04-22 11:24 ` maarten.lankhorst 2015-04-23 6:19 ` [PATCH v2 " Maarten Lankhorst 2015-04-22 11:24 ` [PATCH RFC 5/5] drm/i915: Implement intel_crtc_toggle using atomic state maarten.lankhorst 2015-04-23 6:29 ` [PATCH v2 RFC 6/5] drm/i915: Update less state during modeset Maarten Lankhorst 5 siblings, 1 reply; 14+ messages in thread From: maarten.lankhorst @ 2015-04-22 11:24 UTC (permalink / raw) To: intel-gfx; +Cc: Ander Conselvan de Oliveira From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_atomic_plane.c | 49 +-- drivers/gpu/drm/i915/intel_display.c | 481 +++++++++++++++++++++--------- drivers/gpu/drm/i915/intel_sprite.c | 33 +- 3 files changed, 353 insertions(+), 210 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c index f7bd3b8fa245..0be720edb971 100644 --- a/drivers/gpu/drm/i915/intel_atomic_plane.c +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c @@ -110,32 +110,31 @@ static int intel_plane_atomic_check(struct drm_plane *plane, struct drm_plane_state *state) { struct drm_crtc *crtc = state->crtc; - struct intel_crtc *intel_crtc; - struct intel_crtc_state *crtc_state; + struct drm_crtc_state *crtc_state; struct intel_plane *intel_plane = to_intel_plane(plane); struct intel_plane_state *intel_state = to_intel_plane_state(state); - crtc = crtc ? crtc : plane->crtc; - intel_crtc = to_intel_crtc(crtc); - + intel_state->visible = false; /* * Both crtc and plane->crtc could be NULL if we're updating a * property while the plane is disabled. We don't actually have * anything driver-specific we need to test in that case, so * just return success. */ - if (!crtc) + if (!crtc) { + DRM_DEBUG_ATOMIC("Invisible: no crtc\n"); + return 0; + } + + crtc_state = state->state->crtc_states[drm_crtc_index(crtc)]; + if (WARN_ON(!crtc_state) || + WARN_ON(!crtc_state->enable)) { return 0; + } - /* FIXME: temporary hack necessary while we still use the plane update - * helper. */ - if (state->state) { - crtc_state = - intel_atomic_get_crtc_state(state->state, intel_crtc); - if (IS_ERR(crtc_state)) - return PTR_ERR(crtc_state); - } else { - crtc_state = intel_crtc->config; + if (!crtc_state->active) { + DRM_DEBUG_ATOMIC("Invisible: dpms off\n"); + return 0; } /* @@ -155,24 +154,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane, /* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */ intel_state->clip.x1 = 0; intel_state->clip.y1 = 0; - intel_state->clip.x2 = - crtc_state->base.active ? crtc_state->pipe_src_w : 0; - intel_state->clip.y2 = - crtc_state->base.active ? crtc_state->pipe_src_h : 0; - - /* - * Disabling a plane is always okay; we just need to update - * fb tracking in a special way since cleanup_fb() won't - * get called by the plane helpers. - */ - if (state->fb == NULL && plane->state->fb != NULL) { - /* - * 'prepare' is never called when plane is being disabled, so - * we need to handle frontbuffer tracking as a special case - */ - intel_crtc->atomic.disabled_planes |= - (1 << drm_plane_index(plane)); - } + intel_state->clip.x2 = to_intel_crtc_state(crtc_state)->pipe_src_w; + intel_state->clip.y2 = to_intel_crtc_state(crtc_state)->pipe_src_h; if (state->fb && intel_rotation_90_or_270(state->rotation)) { if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED || diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2ffacb4c3a12..99a45bee20d8 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -100,12 +100,16 @@ static void vlv_prepare_pll(struct intel_crtc *crtc, const struct intel_crtc_state *pipe_config); static void chv_prepare_pll(struct intel_crtc *crtc, const struct intel_crtc_state *pipe_config); +static int intel_atomic_check_crtc(struct drm_crtc *crtc, + struct drm_crtc_state *crtc_state); static void intel_begin_crtc_commit(struct drm_crtc *crtc); static void intel_finish_crtc_commit(struct drm_crtc *crtc); static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state); static void intel_crtc_enable_planes(struct drm_crtc *crtc); static void intel_crtc_disable_planes(struct drm_crtc *crtc); +static void intel_pre_disable_primary(struct drm_crtc *crtc); +static void intel_post_enable_primary(struct drm_crtc *crtc); static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe) { @@ -3056,11 +3060,20 @@ intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb, { struct drm_device *dev = crtc->dev; struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_plane_state *plane_state = + to_intel_plane_state(crtc->primary->state); + bool was_visible = plane_state->visible; - if (dev_priv->display.disable_fbc) + /* Not supported right now by the helper, but lets be thorough. */ + if (was_visible && !fb) + intel_pre_disable_primary(crtc); + else if (was_visible && dev_priv->display.disable_fbc) dev_priv->display.disable_fbc(dev); + plane_state->visible = !!fb; dev_priv->display.update_primary_plane(crtc, fb, x, y); + if (!was_visible && fb) + intel_post_enable_primary(crtc); return 0; } @@ -3087,16 +3100,17 @@ static void intel_update_primary_planes(struct drm_device *dev) struct intel_crtc *intel_crtc = to_intel_crtc(crtc); drm_modeset_lock(&crtc->mutex, NULL); - /* - * FIXME: Once we have proper support for primary planes (and - * disabling them without disabling the entire crtc) allow again - * a NULL crtc->primary->fb. - */ - if (intel_crtc->active && crtc->primary->fb) + + if (intel_crtc->active) { + const struct intel_plane_state *state = + to_intel_plane_state(crtc->primary->state); + dev_priv->display.update_primary_plane(crtc, - crtc->primary->fb, - crtc->x, - crtc->y); + state->base.fb, + state->src.x1 >> 16, + state->src.y1 >> 16); + } + drm_modeset_unlock(&crtc->mutex); } } @@ -10659,6 +10673,7 @@ static struct drm_crtc_helper_funcs intel_helper_funcs = { .load_lut = intel_crtc_load_lut, .atomic_begin = intel_begin_crtc_commit, .atomic_flush = intel_finish_crtc_commit, + .atomic_check = intel_atomic_check_crtc, }; /** @@ -10713,7 +10728,6 @@ static void intel_modeset_update_connector_atomic_state(struct drm_device *dev) */ static void intel_modeset_fixup_state(struct drm_atomic_state *state) { - struct intel_crtc *crtc; struct intel_encoder *encoder; struct intel_connector *connector; @@ -10736,11 +10750,6 @@ static void intel_modeset_fixup_state(struct drm_atomic_state *state) encoder->base.crtc = NULL; } - for_each_intel_crtc(state->dev, crtc) { - crtc->base.enabled = crtc->base.state->enable; - crtc->config = to_intel_crtc_state(crtc->base.state); - } - /* Copy the new configuration to the staged state, to keep the few * pieces of code that haven't been converted yet happy */ intel_modeset_update_staged_output_state(state->dev); @@ -11170,6 +11179,8 @@ intel_modeset_update_state(struct drm_atomic_state *state) struct drm_crtc *crtc; struct drm_crtc_state *crtc_state; struct drm_connector *connector; + struct drm_connector_state *connector_state; + int i; intel_shared_dpll_commit(dev_priv); @@ -11185,7 +11196,26 @@ intel_modeset_update_state(struct drm_atomic_state *state) intel_encoder->connectors_active = false; } - drm_atomic_helper_swap_state(state->dev, state); + /* + * swap crtc and connector state, plane state is already swapped in + * __intel_set_mode_update_planes. Once .crtc_disable is fixed + * all state should be swapped before disabling crtc's. + */ + for_each_crtc_in_state(state, crtc, crtc_state, i) { + crtc->enabled = crtc_state->enable; + to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc_state); + + crtc->state->state = state; + swap(state->crtc_states[i], crtc->state); + crtc->state->state = NULL; + } + + for_each_connector_in_state(state, connector, connector_state, i) { + connector->state->state = state; + swap(state->connector_states[i], connector->state); + connector->state->state = NULL; + } + intel_modeset_fixup_state(state); /* Double check state. */ @@ -11740,6 +11770,26 @@ static void update_scanline_offset(struct intel_crtc *crtc) crtc->scanline_offset = 1; } +static int intel_modeset_compute_planes(struct drm_atomic_state *state, + struct drm_crtc *crtc) +{ + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + int pipe = intel_crtc->pipe; + struct drm_plane_state *plane_state; + struct drm_plane *plane; + + /* Add all overlay planes */ + drm_for_each_legacy_plane(plane, &crtc->dev->mode_config.plane_list) { + if (to_intel_plane(plane)->pipe == pipe) { + plane_state = drm_atomic_get_plane_state(state, plane); + if (IS_ERR(plane_state)) + return PTR_ERR(plane_state); + } + } + + return 0; +} + static struct intel_crtc_state * intel_modeset_compute_config(struct drm_crtc *crtc, struct drm_atomic_state *state) @@ -11765,8 +11815,14 @@ intel_modeset_compute_config(struct drm_crtc *crtc, if (IS_ERR(pipe_config)) return pipe_config; + if (needs_modeset(&pipe_config->base)) { + ret = intel_modeset_compute_planes(state, crtc); + if (ret) + return ERR_PTR(ret); + } + if (!pipe_config->base.enable) - return pipe_config; + goto done; ret = intel_modeset_pipe_config(crtc, state, pipe_config); if (ret) @@ -11784,8 +11840,8 @@ intel_modeset_compute_config(struct drm_crtc *crtc, * required changes and forcing a mode set. */ - intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,"[modeset]"); - + intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config, "[modeset]"); +done: ret = drm_atomic_helper_check_planes(state->dev, state); if (ret) return ERR_PTR(ret); @@ -11869,6 +11925,113 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state) return 0; } +static void __intel_set_mode_update_planes(struct drm_device *dev, + struct drm_atomic_state *state) +{ + int i; + struct drm_plane_state *old_plane_state; + struct drm_crtc_state *crtc_state; + struct drm_plane *plane; + struct drm_crtc *crtc; + + /* + * For now only swap plane state, should be replaced with a + * call to drm_atomic_helper_swap_state + */ + for_each_plane_in_state(state, plane, old_plane_state, i) { + struct drm_plane *plane = state->planes[i]; + + if (!plane) + continue; + + plane->state->state = state; + swap(state->plane_states[i], plane->state); + plane->state->state = NULL; + } + + for_each_crtc_in_state(state, crtc, crtc_state, i) { + const struct drm_crtc_helper_funcs *funcs; + + funcs = crtc->helper_private; + + if (!funcs || !funcs->atomic_begin) + continue; + + /* XXX: Hack because crtc state is not swapped */ + crtc->state->mode_changed = crtc_state->mode_changed; + crtc->state->active_changed = crtc_state->active_changed; + + DRM_DEBUG_ATOMIC("Calling atomic_begin on crtc %i\n", i); + funcs->atomic_begin(crtc); + } + + for_each_plane_in_state(state, plane, old_plane_state, i) { + bool visible = to_intel_plane_state(plane->state)->visible; + struct intel_plane *intel_plane = to_intel_plane(plane); + const struct drm_plane_helper_funcs *funcs; + + crtc = plane->state->crtc; + funcs = plane->helper_private; + + if (!funcs) + continue; + + DRM_DEBUG_ATOMIC("Plane %i is visible: %i\n", i, visible); + + if (!visible) + funcs->atomic_update(plane, old_plane_state); + else if (crtc->state->mode_changed) + intel_plane->disable_plane(plane, crtc, true); + } +} + +static void __intel_set_mode_cleanup_planes(struct drm_device *dev, + struct drm_atomic_state *old_state) +{ + int nplanes = dev->mode_config.num_total_plane; + int ncrtcs = dev->mode_config.num_crtc; + int i; + + for (i = 0; i < nplanes; i++) { + const struct drm_plane_helper_funcs *funcs; + struct drm_plane *plane = old_state->planes[i]; + struct drm_plane_state *old_plane_state; + + if (!plane) + continue; + + funcs = plane->helper_private; + + if (!funcs) + continue; + + old_plane_state = old_state->plane_states[i]; + + if (to_intel_plane_state(plane->state)->visible) { + DRM_DEBUG_ATOMIC("Plane %i is updated\n", i); + funcs->atomic_update(plane, old_plane_state); + } else + DRM_DEBUG_ATOMIC("Plane %i is left alone\n", i); + } + + for (i = 0; i < ncrtcs; i++) { + const struct drm_crtc_helper_funcs *funcs; + struct drm_crtc *crtc = old_state->crtcs[i]; + + if (!crtc) + continue; + + funcs = crtc->helper_private; + + if (!funcs || !funcs->atomic_flush) + continue; + + DRM_DEBUG_ATOMIC("Calling atomic_flush on crtc %i\n", i); + funcs->atomic_flush(crtc); + } + drm_atomic_helper_cleanup_planes(dev, old_state); +} + static int __intel_set_mode(struct drm_crtc *modeset_crtc, struct intel_crtc_state *pipe_config) { @@ -11877,7 +12040,7 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, struct drm_atomic_state *state = pipe_config->base.state; struct drm_crtc *crtc; struct drm_crtc_state *crtc_state; - int ret = 0; + int ret; int i; ret = __intel_set_mode_checks(state); @@ -11888,14 +12051,14 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, if (ret) return ret; + __intel_set_mode_update_planes(dev, state); + for_each_crtc_in_state(state, crtc, crtc_state, i) { if (!needs_modeset(crtc_state)) continue; - intel_crtc_disable_planes(crtc); + intel_crtc_dpms_overlay_disable(to_intel_crtc(crtc)); dev_priv->display.crtc_disable(crtc); - if (!crtc_state->enable) - drm_plane_helper_disable(crtc->primary); } /* crtc->mode is already used by the ->mode_set callbacks, hence we need @@ -11926,8 +12089,6 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, modeset_update_crtc_power_domains(state); - drm_atomic_helper_commit_planes(dev, state); - /* Now enable the clocks, plane, pipe, and connectors that we set up. */ for_each_crtc_in_state(state, crtc, crtc_state, i) { if (!crtc->state->enable) @@ -11935,13 +12096,13 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, update_scanline_offset(to_intel_crtc(crtc)); - dev_priv->display.crtc_enable(crtc); - intel_crtc_enable_planes(crtc); + if (needs_modeset(crtc->state)) + dev_priv->display.crtc_enable(crtc); } - /* FIXME: add subpixel order */ + __intel_set_mode_cleanup_planes(dev, state); - drm_atomic_helper_cleanup_planes(dev, state); + /* FIXME: add subpixel order */ drm_atomic_state_free(state); @@ -12170,6 +12331,7 @@ intel_modeset_stage_output_state(struct drm_device *dev, return ret; crtc_state->enable = drm_atomic_connectors_for_crtc(state, crtc); + crtc_state->active = crtc_state->enable; } ret = intel_modeset_setup_plane_state(state, set->crtc, set->mode, @@ -12190,20 +12352,11 @@ intel_modeset_stage_output_state(struct drm_device *dev, return 0; } -static bool primary_plane_visible(struct drm_crtc *crtc) -{ - struct intel_plane_state *plane_state = - to_intel_plane_state(crtc->primary->state); - - return plane_state->visible; -} - static int intel_crtc_set_config(struct drm_mode_set *set) { struct drm_device *dev; struct drm_atomic_state *state = NULL; struct intel_crtc_state *pipe_config; - bool primary_plane_was_visible; int ret; BUG_ON(!set); @@ -12242,38 +12395,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set) intel_update_pipe_size(to_intel_crtc(set->crtc)); - primary_plane_was_visible = primary_plane_visible(set->crtc); - ret = intel_set_mode_with_config(set->crtc, pipe_config); - - if (ret == 0 && - pipe_config->base.enable && - pipe_config->base.planes_changed && - !needs_modeset(&pipe_config->base)) { - struct intel_crtc *intel_crtc = to_intel_crtc(set->crtc); - - /* - * We need to make sure the primary plane is re-enabled if it - * has previously been turned off. - */ - if (ret == 0 && !primary_plane_was_visible && - primary_plane_visible(set->crtc)) { - WARN_ON(!intel_crtc->active); - intel_post_enable_primary(set->crtc); - } - - /* - * In the fastboot case this may be our only check of the - * state after boot. It would be better to only do it on - * the first update, but we don't have a nice way of doing that - * (and really, set_config isn't used much for high freq page - * flipping, so increasing its cost here shouldn't be a big - * deal). - */ - if (i915.fastboot && ret == 0) - intel_modeset_check_state(set->crtc->dev); - } - if (ret) { DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n", set->crtc->base.id, ret); @@ -12413,6 +12535,9 @@ bool intel_wm_need_update(struct drm_plane *plane, plane->state->rotation != state->rotation) return true; + if (plane->state->crtc_w != state->crtc_w) + return true; + return false; } @@ -12507,74 +12632,22 @@ intel_check_primary_plane(struct drm_plane *plane, struct intel_plane_state *state) { struct drm_device *dev = plane->dev; - struct drm_i915_private *dev_priv = dev->dev_private; struct drm_crtc *crtc = state->base.crtc; - struct intel_crtc *intel_crtc; struct drm_framebuffer *fb = state->base.fb; struct drm_rect *dest = &state->dst; struct drm_rect *src = &state->src; const struct drm_rect *clip = &state->clip; bool can_position = false; - int ret; - - crtc = crtc ? crtc : plane->crtc; - intel_crtc = to_intel_crtc(crtc); if (INTEL_INFO(dev)->gen >= 9) can_position = true; - ret = drm_plane_helper_check_update(plane, crtc, fb, - src, dest, clip, - DRM_PLANE_HELPER_NO_SCALING, - DRM_PLANE_HELPER_NO_SCALING, - can_position, true, - &state->visible); - if (ret) - return ret; - - if (intel_crtc->active) { - struct intel_plane_state *old_state = - to_intel_plane_state(plane->state); - - intel_crtc->atomic.wait_for_flips = true; - - /* - * FBC does not work on some platforms for rotated - * planes, so disable it when rotation is not 0 and - * update it when rotation is set back to 0. - * - * FIXME: This is redundant with the fbc update done in - * the primary plane enable function except that that - * one is done too late. We eventually need to unify - * this. - */ - if (state->visible && - INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) && - dev_priv->fbc.crtc == intel_crtc && - state->base.rotation != BIT(DRM_ROTATE_0)) { - intel_crtc->atomic.disable_fbc = true; - } - - if (state->visible && !old_state->visible) { - /* - * BDW signals flip done immediately if the plane - * is disabled, even if the plane enable is already - * armed to occur at the next vblank :( - */ - if (IS_BROADWELL(dev)) - intel_crtc->atomic.wait_vblank = true; - } - - intel_crtc->atomic.fb_bits |= - INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe); - - intel_crtc->atomic.update_fbc = true; - - if (intel_wm_need_update(plane, &state->base)) - intel_crtc->atomic.update_wm = true; - } - - return 0; + return drm_plane_helper_check_update(plane, crtc, fb, + src, dest, clip, + DRM_PLANE_HELPER_NO_SCALING, + DRM_PLANE_HELPER_NO_SCALING, + can_position, true, + &state->visible); } static void @@ -12600,8 +12673,10 @@ intel_commit_primary_plane(struct drm_plane *plane, /* FIXME: kill this fastboot hack */ intel_update_pipe_size(intel_crtc); - dev_priv->display.update_primary_plane(crtc, plane->fb, - crtc->x, crtc->y); + dev_priv->display.update_primary_plane(crtc, + state->base.fb, + state->src.x1 >> 16, + state->src.y1 >> 16); } } @@ -12616,6 +12691,123 @@ intel_disable_primary_plane(struct drm_plane *plane, dev_priv->display.update_primary_plane(crtc, NULL, 0, 0); } +/* Transitional checking here, mostly for plane updates */ +static int intel_atomic_check_crtc(struct drm_crtc *crtc, + struct drm_crtc_state *crtc_state) +{ + struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc_state); + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + struct drm_atomic_state *state = crtc_state->state; + struct drm_device *dev = crtc->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + unsigned plane_mask; + int i, nplanes = dev->mode_config.num_total_plane, idx; + bool mode_changed = needs_modeset(crtc_state); + bool is_crtc_enabled = crtc_state->active; + bool was_crtc_enabled = crtc->state->active && intel_crtc->active; + + memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic)); + intel_crtc->atomic.update_wm = mode_changed; + + idx = drm_crtc_index(crtc); + DRM_DEBUG_ATOMIC("Crtc %i was enabled %i now enabled: %i\n", + idx, was_crtc_enabled, is_crtc_enabled); + + plane_mask = crtc_state->plane_mask | crtc->state->plane_mask; + for (i = 0; i < nplanes; i++) { + struct intel_plane_state *plane_state, *old_plane_state; + struct intel_plane *plane = to_intel_plane(state->planes[i]); + bool turn_off, turn_on, visible, was_visible; + struct drm_framebuffer *fb; + + if (!plane) + continue; + + plane_state = to_intel_plane_state(state->plane_states[i]); + old_plane_state = to_intel_plane_state(plane->base.state); + + was_visible = old_plane_state->visible && was_crtc_enabled; + visible = plane_state->visible && is_crtc_enabled; + + turn_off = was_visible && (!visible || mode_changed); + turn_on = visible && (!was_visible || mode_changed); + fb = plane_state->base.fb; + + DRM_DEBUG_ATOMIC("Crtc %i has plane %i with fb %i\n", idx, + drm_plane_index(&plane->base), fb ? fb->base.id : -1); + DRM_DEBUG_ATOMIC("\tvisible %i -> %i, off %i, on %i, ms %i\n", + was_visible, visible, turn_off, turn_on, mode_changed); + + /* plane being turned off as part of modeset or changes? */ + if (intel_wm_need_update(&plane->base, &plane_state->base)) + intel_crtc->atomic.update_wm = true; + + /* + * 'prepare' is never called when plane is being disabled, so + * we need to handle frontbuffer tracking as a special case + */ + if (old_plane_state->base.fb && !plane_state->base.fb) + intel_crtc->atomic.disabled_planes |= + (1 << drm_plane_index(&plane->base)); + + switch (plane->base.type) { + case DRM_PLANE_TYPE_PRIMARY: + intel_crtc->atomic.fb_bits |= + INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe); + + intel_crtc->atomic.wait_for_flips = true; + intel_crtc->atomic.pre_disable_primary = turn_off; + intel_crtc->atomic.post_enable_primary = turn_on; + + if (turn_off) + intel_crtc->atomic.disable_fbc = true; + + /* + * FBC does not work on some platforms for rotated + * planes, so disable it when rotation is not 0 and + * update it when rotation is set back to 0. + * + * FIXME: This is redundant with the fbc update done in + * the primary plane enable function except that that + * one is done too late. We eventually need to unify + * this. + */ + + if (visible && + INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) && + dev_priv->fbc.crtc == intel_crtc && + plane_state->base.rotation != BIT(DRM_ROTATE_0)) + intel_crtc->atomic.disable_fbc = true; + + /* + * BDW signals flip done immediately if the plane + * is disabled, even if the plane enable is already + * armed to occur at the next vblank :( + */ + if (turn_on && IS_BROADWELL(dev)) + intel_crtc->atomic.wait_vblank = true; + + intel_crtc->atomic.update_fbc = true; + break; + case DRM_PLANE_TYPE_CURSOR: + intel_crtc->atomic.fb_bits |= + INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe); + break; + case DRM_PLANE_TYPE_OVERLAY: + intel_crtc->atomic.fb_bits |= + INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe); + + if (turn_off) { + intel_crtc->atomic.wait_vblank = true; + intel_crtc->atomic.update_sprite_watermarks |= + (1 << drm_plane_index(&plane->base)); + } + break; + } + } + return 0; +} + static void intel_begin_crtc_commit(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev; @@ -12664,10 +12856,13 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc) intel_runtime_pm_get(dev_priv); /* Perform vblank evasion around commit operation */ - if (intel_crtc->active) + if (intel_crtc->active && !needs_modeset(crtc->state) && + !dev_priv->power_domains.init_power_on) intel_crtc->atomic.evade = intel_pipe_update_start(intel_crtc, &intel_crtc->atomic.start_vbl_count); + else + intel_crtc->atomic.evade = false; } static void intel_finish_crtc_commit(struct drm_crtc *crtc) @@ -12703,6 +12898,8 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc) false, false); memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic)); + crtc->state->mode_changed = false; + crtc->state->active_changed = false; } /** @@ -12812,13 +13009,9 @@ intel_check_cursor_plane(struct drm_plane *plane, struct drm_rect *src = &state->src; const struct drm_rect *clip = &state->clip; struct drm_i915_gem_object *obj = intel_fb_obj(fb); - struct intel_crtc *intel_crtc; unsigned stride; int ret; - crtc = crtc ? crtc : plane->crtc; - intel_crtc = to_intel_crtc(crtc); - ret = drm_plane_helper_check_update(plane, crtc, fb, src, dest, clip, DRM_PLANE_HELPER_NO_SCALING, @@ -12830,7 +13023,7 @@ intel_check_cursor_plane(struct drm_plane *plane, /* if we want to turn off the cursor ignore width and height */ if (!obj) - goto finish; + return 0; /* Check for which cursor types we support */ if (!cursor_size_ok(dev, state->base.crtc_w, state->base.crtc_h)) { @@ -12847,19 +13040,10 @@ intel_check_cursor_plane(struct drm_plane *plane, if (fb->modifier[0] != DRM_FORMAT_MOD_NONE) { DRM_DEBUG_KMS("cursor cannot be tiled\n"); - ret = -EINVAL; - } - -finish: - if (intel_crtc->active) { - if (plane->state->crtc_w != state->base.crtc_w) - intel_crtc->atomic.update_wm = true; - - intel_crtc->atomic.fb_bits |= - INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe); + return -EINVAL; } - return ret; + return 0; } static void @@ -14089,6 +14273,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) WARN_ON(crtc->active); crtc->base.state->enable = false; + crtc->base.state->active = false; crtc->base.enabled = false; } @@ -14117,6 +14302,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) crtc->active ? "enabled" : "disabled"); crtc->base.state->enable = crtc->active; + crtc->base.state->active = crtc->active; crtc->base.enabled = crtc->active; /* Because we only establish the connector -> encoder -> @@ -14255,6 +14441,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) crtc->config); crtc->base.state->enable = crtc->active; + crtc->base.state->active = crtc->active; crtc->base.enabled = crtc->active; plane_state = to_intel_plane_state(primary->state); diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 7419e04b113f..5a277757ac2d 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -811,12 +811,8 @@ intel_check_sprite_plane(struct drm_plane *plane, int max_scale, min_scale; int pixel_size; - intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc); - - if (!fb) { - state->visible = false; - goto finish; - } + if (!fb) + return 0; /* Don't modify another pipe's plane */ if (intel_plane->pipe != intel_crtc->pipe) { @@ -847,7 +843,7 @@ intel_check_sprite_plane(struct drm_plane *plane, vscale = drm_rect_calc_vscale_relaxed(src, dst, min_scale, max_scale); BUG_ON(vscale < 0); - state->visible = drm_rect_clip_scaled(src, dst, clip, hscale, vscale); + state->visible = drm_rect_clip_scaled(src, dst, clip, hscale, vscale); crtc_x = dst->x1; crtc_y = dst->y1; @@ -952,29 +948,6 @@ intel_check_sprite_plane(struct drm_plane *plane, dst->y1 = crtc_y; dst->y2 = crtc_y + crtc_h; -finish: - /* - * If the sprite is completely covering the primary plane, - * we can disable the primary and save power. - */ - if (intel_crtc->active) { - intel_crtc->atomic.fb_bits |= - INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe); - - if (intel_wm_need_update(plane, &state->base)) - intel_crtc->atomic.update_wm = true; - - if (!state->visible) { - /* - * Avoid underruns when disabling the sprite. - * FIXME remove once watermark updates are done properly. - */ - intel_crtc->atomic.wait_vblank = true; - intel_crtc->atomic.update_sprite_watermarks |= - (1 << drm_plane_index(plane)); - } - } - return 0; } -- 2.1.0 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 RFC 4/5] drm/i915: make plane helpers fully atomic 2015-04-22 11:24 ` [PATCH RFC 4/5] drm/i915: make plane helpers fully atomic maarten.lankhorst @ 2015-04-23 6:19 ` Maarten Lankhorst 2015-04-24 8:52 ` Ander Conselvan De Oliveira 0 siblings, 1 reply; 14+ messages in thread From: Maarten Lankhorst @ 2015-04-23 6:19 UTC (permalink / raw) To: intel-gfx; +Cc: Ander Conselvan de Oliveira This kills off most of the transitional sers and uses atomic plane updates in the modeset path to update everything. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- Changes since v1: - Add atomic and sprite planes during a modeset too so they will be restored. - Drop a WARN_ON(!crtc_state->enabled) in atomic_plane_check for cursor and sprite planes. Keep it for primary as this probably indicates we messed up somewhere. drivers/gpu/drm/i915/intel_atomic_plane.c | 58 ++-- drivers/gpu/drm/i915/intel_display.c | 485 +++++++++++++++++++++--------- drivers/gpu/drm/i915/intel_sprite.c | 33 +- 3 files changed, 366 insertions(+), 210 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c index f7bd3b8fa245..ba4ab392b6b0 100644 --- a/drivers/gpu/drm/i915/intel_atomic_plane.c +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c @@ -110,32 +110,40 @@ static int intel_plane_atomic_check(struct drm_plane *plane, struct drm_plane_state *state) { struct drm_crtc *crtc = state->crtc; - struct intel_crtc *intel_crtc; - struct intel_crtc_state *crtc_state; + struct drm_crtc_state *crtc_state; struct intel_plane *intel_plane = to_intel_plane(plane); struct intel_plane_state *intel_state = to_intel_plane_state(state); - crtc = crtc ? crtc : plane->crtc; - intel_crtc = to_intel_crtc(crtc); - + intel_state->visible = false; /* * Both crtc and plane->crtc could be NULL if we're updating a * property while the plane is disabled. We don't actually have * anything driver-specific we need to test in that case, so * just return success. */ - if (!crtc) + if (!crtc) { + DRM_DEBUG_ATOMIC("Invisible: no crtc\n"); return 0; + } + + crtc_state = state->state->crtc_states[drm_crtc_index(crtc)]; + if (WARN_ON(!crtc_state)) + return 0; + + if (!crtc_state->enable) { + DRM_DEBUG_ATOMIC("Invisible: crtc off\n"); - /* FIXME: temporary hack necessary while we still use the plane update - * helper. */ - if (state->state) { - crtc_state = - intel_atomic_get_crtc_state(state->state, intel_crtc); - if (IS_ERR(crtc_state)) - return PTR_ERR(crtc_state); - } else { - crtc_state = intel_crtc->config; + /* + * Probably allowed after converting to atomic. Right + * now it probably means we have the state confused. + */ + I915_STATE_WARN_ON(plane->type == DRM_PLANE_TYPE_PRIMARY); + return 0; + } + + if (!crtc_state->active) { + DRM_DEBUG_ATOMIC("Invisible: dpms off\n"); + return 0; } /* @@ -155,24 +163,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane, /* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */ intel_state->clip.x1 = 0; intel_state->clip.y1 = 0; - intel_state->clip.x2 = - crtc_state->base.active ? crtc_state->pipe_src_w : 0; - intel_state->clip.y2 = - crtc_state->base.active ? crtc_state->pipe_src_h : 0; - - /* - * Disabling a plane is always okay; we just need to update - * fb tracking in a special way since cleanup_fb() won't - * get called by the plane helpers. - */ - if (state->fb == NULL && plane->state->fb != NULL) { - /* - * 'prepare' is never called when plane is being disabled, so - * we need to handle frontbuffer tracking as a special case - */ - intel_crtc->atomic.disabled_planes |= - (1 << drm_plane_index(plane)); - } + intel_state->clip.x2 = to_intel_crtc_state(crtc_state)->pipe_src_w; + intel_state->clip.y2 = to_intel_crtc_state(crtc_state)->pipe_src_h; if (state->fb && intel_rotation_90_or_270(state->rotation)) { if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED || diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2ffacb4c3a12..acb5c5bea428 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -100,12 +100,16 @@ static void vlv_prepare_pll(struct intel_crtc *crtc, const struct intel_crtc_state *pipe_config); static void chv_prepare_pll(struct intel_crtc *crtc, const struct intel_crtc_state *pipe_config); +static int intel_atomic_check_crtc(struct drm_crtc *crtc, + struct drm_crtc_state *crtc_state); static void intel_begin_crtc_commit(struct drm_crtc *crtc); static void intel_finish_crtc_commit(struct drm_crtc *crtc); static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state); static void intel_crtc_enable_planes(struct drm_crtc *crtc); static void intel_crtc_disable_planes(struct drm_crtc *crtc); +static void intel_pre_disable_primary(struct drm_crtc *crtc); +static void intel_post_enable_primary(struct drm_crtc *crtc); static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe) { @@ -3056,11 +3060,20 @@ intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb, { struct drm_device *dev = crtc->dev; struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_plane_state *plane_state = + to_intel_plane_state(crtc->primary->state); + bool was_visible = plane_state->visible; - if (dev_priv->display.disable_fbc) + /* Not supported right now by the helper, but lets be thorough. */ + if (was_visible && !fb) + intel_pre_disable_primary(crtc); + else if (was_visible && dev_priv->display.disable_fbc) dev_priv->display.disable_fbc(dev); + plane_state->visible = !!fb; dev_priv->display.update_primary_plane(crtc, fb, x, y); + if (!was_visible && fb) + intel_post_enable_primary(crtc); return 0; } @@ -3087,16 +3100,17 @@ static void intel_update_primary_planes(struct drm_device *dev) struct intel_crtc *intel_crtc = to_intel_crtc(crtc); drm_modeset_lock(&crtc->mutex, NULL); - /* - * FIXME: Once we have proper support for primary planes (and - * disabling them without disabling the entire crtc) allow again - * a NULL crtc->primary->fb. - */ - if (intel_crtc->active && crtc->primary->fb) + + if (intel_crtc->active) { + const struct intel_plane_state *state = + to_intel_plane_state(crtc->primary->state); + dev_priv->display.update_primary_plane(crtc, - crtc->primary->fb, - crtc->x, - crtc->y); + state->base.fb, + state->src.x1 >> 16, + state->src.y1 >> 16); + } + drm_modeset_unlock(&crtc->mutex); } } @@ -10659,6 +10673,7 @@ static struct drm_crtc_helper_funcs intel_helper_funcs = { .load_lut = intel_crtc_load_lut, .atomic_begin = intel_begin_crtc_commit, .atomic_flush = intel_finish_crtc_commit, + .atomic_check = intel_atomic_check_crtc, }; /** @@ -10713,7 +10728,6 @@ static void intel_modeset_update_connector_atomic_state(struct drm_device *dev) */ static void intel_modeset_fixup_state(struct drm_atomic_state *state) { - struct intel_crtc *crtc; struct intel_encoder *encoder; struct intel_connector *connector; @@ -10736,11 +10750,6 @@ static void intel_modeset_fixup_state(struct drm_atomic_state *state) encoder->base.crtc = NULL; } - for_each_intel_crtc(state->dev, crtc) { - crtc->base.enabled = crtc->base.state->enable; - crtc->config = to_intel_crtc_state(crtc->base.state); - } - /* Copy the new configuration to the staged state, to keep the few * pieces of code that haven't been converted yet happy */ intel_modeset_update_staged_output_state(state->dev); @@ -11170,6 +11179,8 @@ intel_modeset_update_state(struct drm_atomic_state *state) struct drm_crtc *crtc; struct drm_crtc_state *crtc_state; struct drm_connector *connector; + struct drm_connector_state *connector_state; + int i; intel_shared_dpll_commit(dev_priv); @@ -11185,7 +11196,26 @@ intel_modeset_update_state(struct drm_atomic_state *state) intel_encoder->connectors_active = false; } - drm_atomic_helper_swap_state(state->dev, state); + /* + * swap crtc and connector state, plane state is already swapped in + * __intel_set_mode_update_planes. Once .crtc_disable is fixed + * all state should be swapped before disabling crtc's. + */ + for_each_crtc_in_state(state, crtc, crtc_state, i) { + crtc->enabled = crtc_state->enable; + to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc_state); + + crtc->state->state = state; + swap(state->crtc_states[i], crtc->state); + crtc->state->state = NULL; + } + + for_each_connector_in_state(state, connector, connector_state, i) { + connector->state->state = state; + swap(state->connector_states[i], connector->state); + connector->state->state = NULL; + } + intel_modeset_fixup_state(state); /* Double check state. */ @@ -11740,6 +11770,30 @@ static void update_scanline_offset(struct intel_crtc *crtc) crtc->scanline_offset = 1; } +static int intel_modeset_compute_planes(struct drm_atomic_state *state, + struct drm_crtc *crtc) +{ + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + int pipe = intel_crtc->pipe; + struct drm_plane_state *plane_state; + struct drm_plane *plane; + + plane_state = drm_atomic_get_plane_state(state, crtc->cursor); + if (IS_ERR(plane_state)) + return PTR_ERR(plane_state); + + /* Add all overlay planes */ + drm_for_each_legacy_plane(plane, &crtc->dev->mode_config.plane_list) { + if (to_intel_plane(plane)->pipe == pipe) { + plane_state = drm_atomic_get_plane_state(state, plane); + if (IS_ERR(plane_state)) + return PTR_ERR(plane_state); + } + } + + return 0; +} + static struct intel_crtc_state * intel_modeset_compute_config(struct drm_crtc *crtc, struct drm_atomic_state *state) @@ -11765,8 +11819,14 @@ intel_modeset_compute_config(struct drm_crtc *crtc, if (IS_ERR(pipe_config)) return pipe_config; + if (needs_modeset(&pipe_config->base)) { + ret = intel_modeset_compute_planes(state, crtc); + if (ret) + return ERR_PTR(ret); + } + if (!pipe_config->base.enable) - return pipe_config; + goto done; ret = intel_modeset_pipe_config(crtc, state, pipe_config); if (ret) @@ -11784,8 +11844,8 @@ intel_modeset_compute_config(struct drm_crtc *crtc, * required changes and forcing a mode set. */ - intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,"[modeset]"); - + intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config, "[modeset]"); +done: ret = drm_atomic_helper_check_planes(state->dev, state); if (ret) return ERR_PTR(ret); @@ -11869,6 +11929,113 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state) return 0; } +static void __intel_set_mode_update_planes(struct drm_device *dev, + struct drm_atomic_state *state) +{ + int i; + struct drm_plane_state *old_plane_state; + struct drm_crtc_state *crtc_state; + struct drm_plane *plane; + struct drm_crtc *crtc; + + /* + * For now only swap plane state, should be replaced with a + * call to drm_atomic_helper_swap_state + */ + for_each_plane_in_state(state, plane, old_plane_state, i) { + struct drm_plane *plane = state->planes[i]; + + if (!plane) + continue; + + plane->state->state = state; + swap(state->plane_states[i], plane->state); + plane->state->state = NULL; + } + + for_each_crtc_in_state(state, crtc, crtc_state, i) { + const struct drm_crtc_helper_funcs *funcs; + + funcs = crtc->helper_private; + + if (!funcs || !funcs->atomic_begin) + continue; + + /* XXX: Hack because crtc state is not swapped */ + crtc->state->mode_changed = crtc_state->mode_changed; + crtc->state->active_changed = crtc_state->active_changed; + + DRM_DEBUG_ATOMIC("Calling atomic_begin on crtc %i\n", i); + funcs->atomic_begin(crtc); + } + + for_each_plane_in_state(state, plane, old_plane_state, i) { + bool visible = to_intel_plane_state(plane->state)->visible; + struct intel_plane *intel_plane = to_intel_plane(plane); + const struct drm_plane_helper_funcs *funcs; + + crtc = plane->state->crtc; + funcs = plane->helper_private; + + if (!funcs) + continue; + + DRM_DEBUG_ATOMIC("Plane %i is visible: %i\n", i, visible); + + if (!visible) + funcs->atomic_update(plane, old_plane_state); + else if (crtc->state->mode_changed) + intel_plane->disable_plane(plane, crtc, true); + } +} + +static void __intel_set_mode_cleanup_planes(struct drm_device *dev, + struct drm_atomic_state *old_state) +{ + int nplanes = dev->mode_config.num_total_plane; + int ncrtcs = dev->mode_config.num_crtc; + int i; + + for (i = 0; i < nplanes; i++) { + const struct drm_plane_helper_funcs *funcs; + struct drm_plane *plane = old_state->planes[i]; + struct drm_plane_state *old_plane_state; + + if (!plane) + continue; + + funcs = plane->helper_private; + + if (!funcs) + continue; + + old_plane_state = old_state->plane_states[i]; + + if (to_intel_plane_state(plane->state)->visible) { + DRM_DEBUG_ATOMIC("Plane %i is updated\n", i); + funcs->atomic_update(plane, old_plane_state); + } else + DRM_DEBUG_ATOMIC("Plane %i is left alone\n", i); + } + + for (i = 0; i < ncrtcs; i++) { + const struct drm_crtc_helper_funcs *funcs; + struct drm_crtc *crtc = old_state->crtcs[i]; + + if (!crtc) + continue; + + funcs = crtc->helper_private; + + if (!funcs || !funcs->atomic_flush) + continue; + + DRM_DEBUG_ATOMIC("Calling atomic_flush on crtc %i\n", i); + funcs->atomic_flush(crtc); + } + drm_atomic_helper_cleanup_planes(dev, old_state); +} + static int __intel_set_mode(struct drm_crtc *modeset_crtc, struct intel_crtc_state *pipe_config) { @@ -11877,7 +12044,7 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, struct drm_atomic_state *state = pipe_config->base.state; struct drm_crtc *crtc; struct drm_crtc_state *crtc_state; - int ret = 0; + int ret; int i; ret = __intel_set_mode_checks(state); @@ -11888,14 +12055,14 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, if (ret) return ret; + __intel_set_mode_update_planes(dev, state); + for_each_crtc_in_state(state, crtc, crtc_state, i) { if (!needs_modeset(crtc_state)) continue; - intel_crtc_disable_planes(crtc); + intel_crtc_dpms_overlay_disable(to_intel_crtc(crtc)); dev_priv->display.crtc_disable(crtc); - if (!crtc_state->enable) - drm_plane_helper_disable(crtc->primary); } /* crtc->mode is already used by the ->mode_set callbacks, hence we need @@ -11926,8 +12093,6 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, modeset_update_crtc_power_domains(state); - drm_atomic_helper_commit_planes(dev, state); - /* Now enable the clocks, plane, pipe, and connectors that we set up. */ for_each_crtc_in_state(state, crtc, crtc_state, i) { if (!crtc->state->enable) @@ -11935,13 +12100,13 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, update_scanline_offset(to_intel_crtc(crtc)); - dev_priv->display.crtc_enable(crtc); - intel_crtc_enable_planes(crtc); + if (needs_modeset(crtc->state)) + dev_priv->display.crtc_enable(crtc); } - /* FIXME: add subpixel order */ + __intel_set_mode_cleanup_planes(dev, state); - drm_atomic_helper_cleanup_planes(dev, state); + /* FIXME: add subpixel order */ drm_atomic_state_free(state); @@ -12170,6 +12335,7 @@ intel_modeset_stage_output_state(struct drm_device *dev, return ret; crtc_state->enable = drm_atomic_connectors_for_crtc(state, crtc); + crtc_state->active = crtc_state->enable; } ret = intel_modeset_setup_plane_state(state, set->crtc, set->mode, @@ -12190,20 +12356,11 @@ intel_modeset_stage_output_state(struct drm_device *dev, return 0; } -static bool primary_plane_visible(struct drm_crtc *crtc) -{ - struct intel_plane_state *plane_state = - to_intel_plane_state(crtc->primary->state); - - return plane_state->visible; -} - static int intel_crtc_set_config(struct drm_mode_set *set) { struct drm_device *dev; struct drm_atomic_state *state = NULL; struct intel_crtc_state *pipe_config; - bool primary_plane_was_visible; int ret; BUG_ON(!set); @@ -12242,38 +12399,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set) intel_update_pipe_size(to_intel_crtc(set->crtc)); - primary_plane_was_visible = primary_plane_visible(set->crtc); - ret = intel_set_mode_with_config(set->crtc, pipe_config); - - if (ret == 0 && - pipe_config->base.enable && - pipe_config->base.planes_changed && - !needs_modeset(&pipe_config->base)) { - struct intel_crtc *intel_crtc = to_intel_crtc(set->crtc); - - /* - * We need to make sure the primary plane is re-enabled if it - * has previously been turned off. - */ - if (ret == 0 && !primary_plane_was_visible && - primary_plane_visible(set->crtc)) { - WARN_ON(!intel_crtc->active); - intel_post_enable_primary(set->crtc); - } - - /* - * In the fastboot case this may be our only check of the - * state after boot. It would be better to only do it on - * the first update, but we don't have a nice way of doing that - * (and really, set_config isn't used much for high freq page - * flipping, so increasing its cost here shouldn't be a big - * deal). - */ - if (i915.fastboot && ret == 0) - intel_modeset_check_state(set->crtc->dev); - } - if (ret) { DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n", set->crtc->base.id, ret); @@ -12413,6 +12539,9 @@ bool intel_wm_need_update(struct drm_plane *plane, plane->state->rotation != state->rotation) return true; + if (plane->state->crtc_w != state->crtc_w) + return true; + return false; } @@ -12507,74 +12636,22 @@ intel_check_primary_plane(struct drm_plane *plane, struct intel_plane_state *state) { struct drm_device *dev = plane->dev; - struct drm_i915_private *dev_priv = dev->dev_private; struct drm_crtc *crtc = state->base.crtc; - struct intel_crtc *intel_crtc; struct drm_framebuffer *fb = state->base.fb; struct drm_rect *dest = &state->dst; struct drm_rect *src = &state->src; const struct drm_rect *clip = &state->clip; bool can_position = false; - int ret; - - crtc = crtc ? crtc : plane->crtc; - intel_crtc = to_intel_crtc(crtc); if (INTEL_INFO(dev)->gen >= 9) can_position = true; - ret = drm_plane_helper_check_update(plane, crtc, fb, - src, dest, clip, - DRM_PLANE_HELPER_NO_SCALING, - DRM_PLANE_HELPER_NO_SCALING, - can_position, true, - &state->visible); - if (ret) - return ret; - - if (intel_crtc->active) { - struct intel_plane_state *old_state = - to_intel_plane_state(plane->state); - - intel_crtc->atomic.wait_for_flips = true; - - /* - * FBC does not work on some platforms for rotated - * planes, so disable it when rotation is not 0 and - * update it when rotation is set back to 0. - * - * FIXME: This is redundant with the fbc update done in - * the primary plane enable function except that that - * one is done too late. We eventually need to unify - * this. - */ - if (state->visible && - INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) && - dev_priv->fbc.crtc == intel_crtc && - state->base.rotation != BIT(DRM_ROTATE_0)) { - intel_crtc->atomic.disable_fbc = true; - } - - if (state->visible && !old_state->visible) { - /* - * BDW signals flip done immediately if the plane - * is disabled, even if the plane enable is already - * armed to occur at the next vblank :( - */ - if (IS_BROADWELL(dev)) - intel_crtc->atomic.wait_vblank = true; - } - - intel_crtc->atomic.fb_bits |= - INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe); - - intel_crtc->atomic.update_fbc = true; - - if (intel_wm_need_update(plane, &state->base)) - intel_crtc->atomic.update_wm = true; - } - - return 0; + return drm_plane_helper_check_update(plane, crtc, fb, + src, dest, clip, + DRM_PLANE_HELPER_NO_SCALING, + DRM_PLANE_HELPER_NO_SCALING, + can_position, true, + &state->visible); } static void @@ -12600,8 +12677,10 @@ intel_commit_primary_plane(struct drm_plane *plane, /* FIXME: kill this fastboot hack */ intel_update_pipe_size(intel_crtc); - dev_priv->display.update_primary_plane(crtc, plane->fb, - crtc->x, crtc->y); + dev_priv->display.update_primary_plane(crtc, + state->base.fb, + state->src.x1 >> 16, + state->src.y1 >> 16); } } @@ -12616,6 +12695,123 @@ intel_disable_primary_plane(struct drm_plane *plane, dev_priv->display.update_primary_plane(crtc, NULL, 0, 0); } +/* Transitional checking here, mostly for plane updates */ +static int intel_atomic_check_crtc(struct drm_crtc *crtc, + struct drm_crtc_state *crtc_state) +{ + struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc_state); + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + struct drm_atomic_state *state = crtc_state->state; + struct drm_device *dev = crtc->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + unsigned plane_mask; + int i, nplanes = dev->mode_config.num_total_plane, idx; + bool mode_changed = needs_modeset(crtc_state); + bool is_crtc_enabled = crtc_state->active; + bool was_crtc_enabled = crtc->state->active && intel_crtc->active; + + memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic)); + intel_crtc->atomic.update_wm = mode_changed; + + idx = drm_crtc_index(crtc); + DRM_DEBUG_ATOMIC("Crtc %i was enabled %i now enabled: %i\n", + idx, was_crtc_enabled, is_crtc_enabled); + + plane_mask = crtc_state->plane_mask | crtc->state->plane_mask; + for (i = 0; i < nplanes; i++) { + struct intel_plane_state *plane_state, *old_plane_state; + struct intel_plane *plane = to_intel_plane(state->planes[i]); + bool turn_off, turn_on, visible, was_visible; + struct drm_framebuffer *fb; + + if (!plane) + continue; + + plane_state = to_intel_plane_state(state->plane_states[i]); + old_plane_state = to_intel_plane_state(plane->base.state); + + was_visible = old_plane_state->visible && was_crtc_enabled; + visible = plane_state->visible && is_crtc_enabled; + + turn_off = was_visible && (!visible || mode_changed); + turn_on = visible && (!was_visible || mode_changed); + fb = plane_state->base.fb; + + DRM_DEBUG_ATOMIC("Crtc %i has plane %i with fb %i\n", idx, + drm_plane_index(&plane->base), fb ? fb->base.id : -1); + DRM_DEBUG_ATOMIC("\tvisible %i -> %i, off %i, on %i, ms %i\n", + was_visible, visible, turn_off, turn_on, mode_changed); + + /* plane being turned off as part of modeset or changes? */ + if (intel_wm_need_update(&plane->base, &plane_state->base)) + intel_crtc->atomic.update_wm = true; + + /* + * 'prepare' is never called when plane is being disabled, so + * we need to handle frontbuffer tracking as a special case + */ + if (old_plane_state->base.fb && !plane_state->base.fb) + intel_crtc->atomic.disabled_planes |= + (1 << drm_plane_index(&plane->base)); + + switch (plane->base.type) { + case DRM_PLANE_TYPE_PRIMARY: + intel_crtc->atomic.fb_bits |= + INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe); + + intel_crtc->atomic.wait_for_flips = true; + intel_crtc->atomic.pre_disable_primary = turn_off; + intel_crtc->atomic.post_enable_primary = turn_on; + + if (turn_off) + intel_crtc->atomic.disable_fbc = true; + + /* + * FBC does not work on some platforms for rotated + * planes, so disable it when rotation is not 0 and + * update it when rotation is set back to 0. + * + * FIXME: This is redundant with the fbc update done in + * the primary plane enable function except that that + * one is done too late. We eventually need to unify + * this. + */ + + if (visible && + INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) && + dev_priv->fbc.crtc == intel_crtc && + plane_state->base.rotation != BIT(DRM_ROTATE_0)) + intel_crtc->atomic.disable_fbc = true; + + /* + * BDW signals flip done immediately if the plane + * is disabled, even if the plane enable is already + * armed to occur at the next vblank :( + */ + if (turn_on && IS_BROADWELL(dev)) + intel_crtc->atomic.wait_vblank = true; + + intel_crtc->atomic.update_fbc = true; + break; + case DRM_PLANE_TYPE_CURSOR: + intel_crtc->atomic.fb_bits |= + INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe); + break; + case DRM_PLANE_TYPE_OVERLAY: + intel_crtc->atomic.fb_bits |= + INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe); + + if (turn_off) { + intel_crtc->atomic.wait_vblank = true; + intel_crtc->atomic.update_sprite_watermarks |= + (1 << drm_plane_index(&plane->base)); + } + break; + } + } + return 0; +} + static void intel_begin_crtc_commit(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev; @@ -12664,10 +12860,13 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc) intel_runtime_pm_get(dev_priv); /* Perform vblank evasion around commit operation */ - if (intel_crtc->active) + if (intel_crtc->active && !needs_modeset(crtc->state) && + !dev_priv->power_domains.init_power_on) intel_crtc->atomic.evade = intel_pipe_update_start(intel_crtc, &intel_crtc->atomic.start_vbl_count); + else + intel_crtc->atomic.evade = false; } static void intel_finish_crtc_commit(struct drm_crtc *crtc) @@ -12703,6 +12902,8 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc) false, false); memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic)); + crtc->state->mode_changed = false; + crtc->state->active_changed = false; } /** @@ -12812,13 +13013,9 @@ intel_check_cursor_plane(struct drm_plane *plane, struct drm_rect *src = &state->src; const struct drm_rect *clip = &state->clip; struct drm_i915_gem_object *obj = intel_fb_obj(fb); - struct intel_crtc *intel_crtc; unsigned stride; int ret; - crtc = crtc ? crtc : plane->crtc; - intel_crtc = to_intel_crtc(crtc); - ret = drm_plane_helper_check_update(plane, crtc, fb, src, dest, clip, DRM_PLANE_HELPER_NO_SCALING, @@ -12830,7 +13027,7 @@ intel_check_cursor_plane(struct drm_plane *plane, /* if we want to turn off the cursor ignore width and height */ if (!obj) - goto finish; + return 0; /* Check for which cursor types we support */ if (!cursor_size_ok(dev, state->base.crtc_w, state->base.crtc_h)) { @@ -12847,19 +13044,10 @@ intel_check_cursor_plane(struct drm_plane *plane, if (fb->modifier[0] != DRM_FORMAT_MOD_NONE) { DRM_DEBUG_KMS("cursor cannot be tiled\n"); - ret = -EINVAL; - } - -finish: - if (intel_crtc->active) { - if (plane->state->crtc_w != state->base.crtc_w) - intel_crtc->atomic.update_wm = true; - - intel_crtc->atomic.fb_bits |= - INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe); + return -EINVAL; } - return ret; + return 0; } static void @@ -14089,6 +14277,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) WARN_ON(crtc->active); crtc->base.state->enable = false; + crtc->base.state->active = false; crtc->base.enabled = false; } @@ -14117,6 +14306,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) crtc->active ? "enabled" : "disabled"); crtc->base.state->enable = crtc->active; + crtc->base.state->active = crtc->active; crtc->base.enabled = crtc->active; /* Because we only establish the connector -> encoder -> @@ -14255,6 +14445,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) crtc->config); crtc->base.state->enable = crtc->active; + crtc->base.state->active = crtc->active; crtc->base.enabled = crtc->active; plane_state = to_intel_plane_state(primary->state); diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 7419e04b113f..5a277757ac2d 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -811,12 +811,8 @@ intel_check_sprite_plane(struct drm_plane *plane, int max_scale, min_scale; int pixel_size; - intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc); - - if (!fb) { - state->visible = false; - goto finish; - } + if (!fb) + return 0; /* Don't modify another pipe's plane */ if (intel_plane->pipe != intel_crtc->pipe) { @@ -847,7 +843,7 @@ intel_check_sprite_plane(struct drm_plane *plane, vscale = drm_rect_calc_vscale_relaxed(src, dst, min_scale, max_scale); BUG_ON(vscale < 0); - state->visible = drm_rect_clip_scaled(src, dst, clip, hscale, vscale); + state->visible = drm_rect_clip_scaled(src, dst, clip, hscale, vscale); crtc_x = dst->x1; crtc_y = dst->y1; @@ -952,29 +948,6 @@ intel_check_sprite_plane(struct drm_plane *plane, dst->y1 = crtc_y; dst->y2 = crtc_y + crtc_h; -finish: - /* - * If the sprite is completely covering the primary plane, - * we can disable the primary and save power. - */ - if (intel_crtc->active) { - intel_crtc->atomic.fb_bits |= - INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe); - - if (intel_wm_need_update(plane, &state->base)) - intel_crtc->atomic.update_wm = true; - - if (!state->visible) { - /* - * Avoid underruns when disabling the sprite. - * FIXME remove once watermark updates are done properly. - */ - intel_crtc->atomic.wait_vblank = true; - intel_crtc->atomic.update_sprite_watermarks |= - (1 << drm_plane_index(plane)); - } - } - return 0; } -- 2.1.0 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 RFC 4/5] drm/i915: make plane helpers fully atomic 2015-04-23 6:19 ` [PATCH v2 " Maarten Lankhorst @ 2015-04-24 8:52 ` Ander Conselvan De Oliveira 0 siblings, 0 replies; 14+ messages in thread From: Ander Conselvan De Oliveira @ 2015-04-24 8:52 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx On Thu, 2015-04-23 at 08:19 +0200, Maarten Lankhorst wrote: > This kills off most of the transitional sers and uses atomic plane updates > in the modeset path to update everything. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > Changes since v1: > - Add atomic and sprite planes during a modeset too so they > will be restored. > - Drop a WARN_ON(!crtc_state->enabled) in atomic_plane_check for > cursor and sprite planes. Keep it for primary as this probably > indicates we messed up somewhere. > > drivers/gpu/drm/i915/intel_atomic_plane.c | 58 ++-- > drivers/gpu/drm/i915/intel_display.c | 485 +++++++++++++++++++++--------- > drivers/gpu/drm/i915/intel_sprite.c | 33 +- > 3 files changed, 366 insertions(+), 210 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c > index f7bd3b8fa245..ba4ab392b6b0 100644 > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c > @@ -110,32 +110,40 @@ static int intel_plane_atomic_check(struct drm_plane *plane, > struct drm_plane_state *state) > { > struct drm_crtc *crtc = state->crtc; > - struct intel_crtc *intel_crtc; > - struct intel_crtc_state *crtc_state; > + struct drm_crtc_state *crtc_state; > struct intel_plane *intel_plane = to_intel_plane(plane); > struct intel_plane_state *intel_state = to_intel_plane_state(state); > > - crtc = crtc ? crtc : plane->crtc; > - intel_crtc = to_intel_crtc(crtc); > - > + intel_state->visible = false; > /* > * Both crtc and plane->crtc could be NULL if we're updating a > * property while the plane is disabled. We don't actually have > * anything driver-specific we need to test in that case, so > * just return success. > */ > - if (!crtc) > + if (!crtc) { > + DRM_DEBUG_ATOMIC("Invisible: no crtc\n"); > return 0; > + } > + > + crtc_state = state->state->crtc_states[drm_crtc_index(crtc)]; > + if (WARN_ON(!crtc_state)) > + return 0; > + > + if (!crtc_state->enable) { > + DRM_DEBUG_ATOMIC("Invisible: crtc off\n"); > > - /* FIXME: temporary hack necessary while we still use the plane update > - * helper. */ > - if (state->state) { > - crtc_state = > - intel_atomic_get_crtc_state(state->state, intel_crtc); > - if (IS_ERR(crtc_state)) > - return PTR_ERR(crtc_state); > - } else { > - crtc_state = intel_crtc->config; > + /* > + * Probably allowed after converting to atomic. Right > + * now it probably means we have the state confused. > + */ > + I915_STATE_WARN_ON(plane->type == DRM_PLANE_TYPE_PRIMARY); > + return 0; > + } > + > + if (!crtc_state->active) { > + DRM_DEBUG_ATOMIC("Invisible: dpms off\n"); > + return 0; > } > > /* > @@ -155,24 +163,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane, > /* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */ > intel_state->clip.x1 = 0; > intel_state->clip.y1 = 0; > - intel_state->clip.x2 = > - crtc_state->base.active ? crtc_state->pipe_src_w : 0; > - intel_state->clip.y2 = > - crtc_state->base.active ? crtc_state->pipe_src_h : 0; > - > - /* > - * Disabling a plane is always okay; we just need to update > - * fb tracking in a special way since cleanup_fb() won't > - * get called by the plane helpers. > - */ > - if (state->fb == NULL && plane->state->fb != NULL) { > - /* > - * 'prepare' is never called when plane is being disabled, so > - * we need to handle frontbuffer tracking as a special case > - */ > - intel_crtc->atomic.disabled_planes |= > - (1 << drm_plane_index(plane)); > - } > + intel_state->clip.x2 = to_intel_crtc_state(crtc_state)->pipe_src_w; > + intel_state->clip.y2 = to_intel_crtc_state(crtc_state)->pipe_src_h; > > if (state->fb && intel_rotation_90_or_270(state->rotation)) { > if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED || > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 2ffacb4c3a12..acb5c5bea428 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -100,12 +100,16 @@ static void vlv_prepare_pll(struct intel_crtc *crtc, > const struct intel_crtc_state *pipe_config); > static void chv_prepare_pll(struct intel_crtc *crtc, > const struct intel_crtc_state *pipe_config); > +static int intel_atomic_check_crtc(struct drm_crtc *crtc, > + struct drm_crtc_state *crtc_state); > static void intel_begin_crtc_commit(struct drm_crtc *crtc); > static void intel_finish_crtc_commit(struct drm_crtc *crtc); > static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_crtc, > struct intel_crtc_state *crtc_state); > static void intel_crtc_enable_planes(struct drm_crtc *crtc); > static void intel_crtc_disable_planes(struct drm_crtc *crtc); > +static void intel_pre_disable_primary(struct drm_crtc *crtc); > +static void intel_post_enable_primary(struct drm_crtc *crtc); > > static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe) > { > @@ -3056,11 +3060,20 @@ intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb, > { > struct drm_device *dev = crtc->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_plane_state *plane_state = > + to_intel_plane_state(crtc->primary->state); > + bool was_visible = plane_state->visible; > > - if (dev_priv->display.disable_fbc) > + /* Not supported right now by the helper, but lets be thorough. */ > + if (was_visible && !fb) > + intel_pre_disable_primary(crtc); > + else if (was_visible && dev_priv->display.disable_fbc) > dev_priv->display.disable_fbc(dev); > > + plane_state->visible = !!fb; > dev_priv->display.update_primary_plane(crtc, fb, x, y); > + if (!was_visible && fb) > + intel_post_enable_primary(crtc); > > return 0; > } > @@ -3087,16 +3100,17 @@ static void intel_update_primary_planes(struct drm_device *dev) > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > drm_modeset_lock(&crtc->mutex, NULL); > - /* > - * FIXME: Once we have proper support for primary planes (and > - * disabling them without disabling the entire crtc) allow again > - * a NULL crtc->primary->fb. > - */ > - if (intel_crtc->active && crtc->primary->fb) > + > + if (intel_crtc->active) { > + const struct intel_plane_state *state = > + to_intel_plane_state(crtc->primary->state); > + > dev_priv->display.update_primary_plane(crtc, > - crtc->primary->fb, > - crtc->x, > - crtc->y); > + state->base.fb, > + state->src.x1 >> 16, > + state->src.y1 >> 16); > + } > + > drm_modeset_unlock(&crtc->mutex); > } > } > @@ -10659,6 +10673,7 @@ static struct drm_crtc_helper_funcs intel_helper_funcs = { > .load_lut = intel_crtc_load_lut, > .atomic_begin = intel_begin_crtc_commit, > .atomic_flush = intel_finish_crtc_commit, > + .atomic_check = intel_atomic_check_crtc, > }; > > /** > @@ -10713,7 +10728,6 @@ static void intel_modeset_update_connector_atomic_state(struct drm_device *dev) > */ > static void intel_modeset_fixup_state(struct drm_atomic_state *state) > { > - struct intel_crtc *crtc; > struct intel_encoder *encoder; > struct intel_connector *connector; > > @@ -10736,11 +10750,6 @@ static void intel_modeset_fixup_state(struct drm_atomic_state *state) > encoder->base.crtc = NULL; > } > > - for_each_intel_crtc(state->dev, crtc) { > - crtc->base.enabled = crtc->base.state->enable; > - crtc->config = to_intel_crtc_state(crtc->base.state); > - } > - > /* Copy the new configuration to the staged state, to keep the few > * pieces of code that haven't been converted yet happy */ > intel_modeset_update_staged_output_state(state->dev); > @@ -11170,6 +11179,8 @@ intel_modeset_update_state(struct drm_atomic_state *state) > struct drm_crtc *crtc; > struct drm_crtc_state *crtc_state; > struct drm_connector *connector; > + struct drm_connector_state *connector_state; > + int i; > > intel_shared_dpll_commit(dev_priv); > > @@ -11185,7 +11196,26 @@ intel_modeset_update_state(struct drm_atomic_state *state) > intel_encoder->connectors_active = false; > } > > - drm_atomic_helper_swap_state(state->dev, state); > + /* > + * swap crtc and connector state, plane state is already swapped in > + * __intel_set_mode_update_planes. Once .crtc_disable is fixed > + * all state should be swapped before disabling crtc's. > + */ Can't we just fix .crtc_disable() first? > + for_each_crtc_in_state(state, crtc, crtc_state, i) { > + crtc->enabled = crtc_state->enable; > + to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc_state); > + > + crtc->state->state = state; > + swap(state->crtc_states[i], crtc->state); > + crtc->state->state = NULL; > + } > + > + for_each_connector_in_state(state, connector, connector_state, i) { > + connector->state->state = state; > + swap(state->connector_states[i], connector->state); > + connector->state->state = NULL; > + } > + > intel_modeset_fixup_state(state); > > /* Double check state. */ > @@ -11740,6 +11770,30 @@ static void update_scanline_offset(struct intel_crtc *crtc) > crtc->scanline_offset = 1; > } > > +static int intel_modeset_compute_planes(struct drm_atomic_state *state, > + struct drm_crtc *crtc) > +{ > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + int pipe = intel_crtc->pipe; > + struct drm_plane_state *plane_state; > + struct drm_plane *plane; > + > + plane_state = drm_atomic_get_plane_state(state, crtc->cursor); > + if (IS_ERR(plane_state)) > + return PTR_ERR(plane_state); > + > + /* Add all overlay planes */ > + drm_for_each_legacy_plane(plane, &crtc->dev->mode_config.plane_list) { > + if (to_intel_plane(plane)->pipe == pipe) { > + plane_state = drm_atomic_get_plane_state(state, plane); > + if (IS_ERR(plane_state)) > + return PTR_ERR(plane_state); > + } > + } > + > + return 0; > +} > + > static struct intel_crtc_state * > intel_modeset_compute_config(struct drm_crtc *crtc, > struct drm_atomic_state *state) > @@ -11765,8 +11819,14 @@ intel_modeset_compute_config(struct drm_crtc *crtc, > if (IS_ERR(pipe_config)) > return pipe_config; > > + if (needs_modeset(&pipe_config->base)) { > + ret = intel_modeset_compute_planes(state, crtc); > + if (ret) > + return ERR_PTR(ret); > + } > + > if (!pipe_config->base.enable) > - return pipe_config; > + goto done; > > ret = intel_modeset_pipe_config(crtc, state, pipe_config); > if (ret) > @@ -11784,8 +11844,8 @@ intel_modeset_compute_config(struct drm_crtc *crtc, > * required changes and forcing a mode set. > */ > > - intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,"[modeset]"); > - > + intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config, "[modeset]"); > +done: > ret = drm_atomic_helper_check_planes(state->dev, state); > if (ret) > return ERR_PTR(ret); > @@ -11869,6 +11929,113 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state) > return 0; > } > > +static void __intel_set_mode_update_planes(struct drm_device *dev, > + struct drm_atomic_state *state) > +{ > + int i; > + struct drm_plane_state *old_plane_state; > + struct drm_crtc_state *crtc_state; > + struct drm_plane *plane; > + struct drm_crtc *crtc; > + > + /* > + * For now only swap plane state, should be replaced with a > + * call to drm_atomic_helper_swap_state > + */ > + for_each_plane_in_state(state, plane, old_plane_state, i) { > + struct drm_plane *plane = state->planes[i]; > + > + if (!plane) > + continue; > + > + plane->state->state = state; > + swap(state->plane_states[i], plane->state); > + plane->state->state = NULL; > + } > + > + for_each_crtc_in_state(state, crtc, crtc_state, i) { > + const struct drm_crtc_helper_funcs *funcs; > + > + funcs = crtc->helper_private; > + > + if (!funcs || !funcs->atomic_begin) > + continue; > + > + /* XXX: Hack because crtc state is not swapped */ > + crtc->state->mode_changed = crtc_state->mode_changed; > + crtc->state->active_changed = crtc_state->active_changed; > + > + DRM_DEBUG_ATOMIC("Calling atomic_begin on crtc %i\n", i); > + funcs->atomic_begin(crtc); > + } > + > + for_each_plane_in_state(state, plane, old_plane_state, i) { > + bool visible = to_intel_plane_state(plane->state)->visible; > + struct intel_plane *intel_plane = to_intel_plane(plane); > + const struct drm_plane_helper_funcs *funcs; > + > + crtc = plane->state->crtc; > + funcs = plane->helper_private; > + > + if (!funcs) > + continue; > + > + DRM_DEBUG_ATOMIC("Plane %i is visible: %i\n", i, visible); > + > + if (!visible) > + funcs->atomic_update(plane, old_plane_state); I'm getting a NULL pointer dereference in intel_commit_primary_plane() because of a NULL crtc when an already disabled plane is disabled again. Should we just skip the update here? > + else if (crtc->state->mode_changed) > + intel_plane->disable_plane(plane, crtc, true); > + } > +} > + > +static void __intel_set_mode_cleanup_planes(struct drm_device *dev, > + struct drm_atomic_state *old_state) > +{ > + int nplanes = dev->mode_config.num_total_plane; > + int ncrtcs = dev->mode_config.num_crtc; > + int i; > + > + for (i = 0; i < nplanes; i++) { > + const struct drm_plane_helper_funcs *funcs; > + struct drm_plane *plane = old_state->planes[i]; > + struct drm_plane_state *old_plane_state; > + > + if (!plane) > + continue; > + > + funcs = plane->helper_private; > + > + if (!funcs) > + continue; > + > + old_plane_state = old_state->plane_states[i]; > + > + if (to_intel_plane_state(plane->state)->visible) { > + DRM_DEBUG_ATOMIC("Plane %i is updated\n", i); > + funcs->atomic_update(plane, old_plane_state); > + } else > + DRM_DEBUG_ATOMIC("Plane %i is left alone\n", i); > + } > + > + for (i = 0; i < ncrtcs; i++) { > + const struct drm_crtc_helper_funcs *funcs; > + struct drm_crtc *crtc = old_state->crtcs[i]; > + > + if (!crtc) > + continue; > + > + funcs = crtc->helper_private; > + > + if (!funcs || !funcs->atomic_flush) > + continue; > + > + DRM_DEBUG_ATOMIC("Calling atomic_flush on crtc %i\n", i); > + funcs->atomic_flush(crtc); > + } These loops could use for_each_{crtc,plane}_in_state macros. > + drm_atomic_helper_cleanup_planes(dev, old_state); > +} > + > static int __intel_set_mode(struct drm_crtc *modeset_crtc, > struct intel_crtc_state *pipe_config) > { > @@ -11877,7 +12044,7 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, > struct drm_atomic_state *state = pipe_config->base.state; > struct drm_crtc *crtc; > struct drm_crtc_state *crtc_state; > - int ret = 0; > + int ret; > int i; > > ret = __intel_set_mode_checks(state); > @@ -11888,14 +12055,14 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, > if (ret) > return ret; > > + __intel_set_mode_update_planes(dev, state); > + > for_each_crtc_in_state(state, crtc, crtc_state, i) { > if (!needs_modeset(crtc_state)) > continue; > > - intel_crtc_disable_planes(crtc); > + intel_crtc_dpms_overlay_disable(to_intel_crtc(crtc)); > dev_priv->display.crtc_disable(crtc); > - if (!crtc_state->enable) > - drm_plane_helper_disable(crtc->primary); > } > > /* crtc->mode is already used by the ->mode_set callbacks, hence we need > @@ -11926,8 +12093,6 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, > > modeset_update_crtc_power_domains(state); > > - drm_atomic_helper_commit_planes(dev, state); > - > /* Now enable the clocks, plane, pipe, and connectors that we set up. */ > for_each_crtc_in_state(state, crtc, crtc_state, i) { > if (!crtc->state->enable) > @@ -11935,13 +12100,13 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, > > update_scanline_offset(to_intel_crtc(crtc)); > > - dev_priv->display.crtc_enable(crtc); > - intel_crtc_enable_planes(crtc); > + if (needs_modeset(crtc->state)) > + dev_priv->display.crtc_enable(crtc); In v2 of the patch that removes modeset_pipes and friends, I added a check for needs_modeset() before update_scanline_offset(). > } > > - /* FIXME: add subpixel order */ > + __intel_set_mode_cleanup_planes(dev, state); > > - drm_atomic_helper_cleanup_planes(dev, state); > + /* FIXME: add subpixel order */ > > drm_atomic_state_free(state); > > @@ -12170,6 +12335,7 @@ intel_modeset_stage_output_state(struct drm_device *dev, > return ret; > > crtc_state->enable = drm_atomic_connectors_for_crtc(state, crtc); > + crtc_state->active = crtc_state->enable; Should this be a separate fix? > } > > ret = intel_modeset_setup_plane_state(state, set->crtc, set->mode, > @@ -12190,20 +12356,11 @@ intel_modeset_stage_output_state(struct drm_device *dev, > return 0; > } > > -static bool primary_plane_visible(struct drm_crtc *crtc) > -{ > - struct intel_plane_state *plane_state = > - to_intel_plane_state(crtc->primary->state); > - > - return plane_state->visible; > -} > - > static int intel_crtc_set_config(struct drm_mode_set *set) > { > struct drm_device *dev; > struct drm_atomic_state *state = NULL; > struct intel_crtc_state *pipe_config; > - bool primary_plane_was_visible; > int ret; > > BUG_ON(!set); > @@ -12242,38 +12399,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set) > > intel_update_pipe_size(to_intel_crtc(set->crtc)); > > - primary_plane_was_visible = primary_plane_visible(set->crtc); > - > ret = intel_set_mode_with_config(set->crtc, pipe_config); > - > - if (ret == 0 && > - pipe_config->base.enable && > - pipe_config->base.planes_changed && > - !needs_modeset(&pipe_config->base)) { > - struct intel_crtc *intel_crtc = to_intel_crtc(set->crtc); > - > - /* > - * We need to make sure the primary plane is re-enabled if it > - * has previously been turned off. > - */ > - if (ret == 0 && !primary_plane_was_visible && > - primary_plane_visible(set->crtc)) { > - WARN_ON(!intel_crtc->active); > - intel_post_enable_primary(set->crtc); > - } > - > - /* > - * In the fastboot case this may be our only check of the > - * state after boot. It would be better to only do it on > - * the first update, but we don't have a nice way of doing that > - * (and really, set_config isn't used much for high freq page > - * flipping, so increasing its cost here shouldn't be a big > - * deal). > - */ > - if (i915.fastboot && ret == 0) > - intel_modeset_check_state(set->crtc->dev); > - } > - Maybe this could be a separate patch? > if (ret) { > DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n", > set->crtc->base.id, ret); > @@ -12413,6 +12539,9 @@ bool intel_wm_need_update(struct drm_plane *plane, > plane->state->rotation != state->rotation) > return true; > > + if (plane->state->crtc_w != state->crtc_w) > + return true; > + > return false; > } > > @@ -12507,74 +12636,22 @@ intel_check_primary_plane(struct drm_plane *plane, > struct intel_plane_state *state) > { > struct drm_device *dev = plane->dev; > - struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_crtc *crtc = state->base.crtc; > - struct intel_crtc *intel_crtc; > struct drm_framebuffer *fb = state->base.fb; > struct drm_rect *dest = &state->dst; > struct drm_rect *src = &state->src; > const struct drm_rect *clip = &state->clip; > bool can_position = false; > - int ret; > - > - crtc = crtc ? crtc : plane->crtc; > - intel_crtc = to_intel_crtc(crtc); > > if (INTEL_INFO(dev)->gen >= 9) > can_position = true; > > - ret = drm_plane_helper_check_update(plane, crtc, fb, > - src, dest, clip, > - DRM_PLANE_HELPER_NO_SCALING, > - DRM_PLANE_HELPER_NO_SCALING, > - can_position, true, > - &state->visible); > - if (ret) > - return ret; > - > - if (intel_crtc->active) { > - struct intel_plane_state *old_state = > - to_intel_plane_state(plane->state); > - > - intel_crtc->atomic.wait_for_flips = true; > - > - /* > - * FBC does not work on some platforms for rotated > - * planes, so disable it when rotation is not 0 and > - * update it when rotation is set back to 0. > - * > - * FIXME: This is redundant with the fbc update done in > - * the primary plane enable function except that that > - * one is done too late. We eventually need to unify > - * this. > - */ > - if (state->visible && > - INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) && > - dev_priv->fbc.crtc == intel_crtc && > - state->base.rotation != BIT(DRM_ROTATE_0)) { > - intel_crtc->atomic.disable_fbc = true; > - } > - > - if (state->visible && !old_state->visible) { > - /* > - * BDW signals flip done immediately if the plane > - * is disabled, even if the plane enable is already > - * armed to occur at the next vblank :( > - */ > - if (IS_BROADWELL(dev)) > - intel_crtc->atomic.wait_vblank = true; > - } > - > - intel_crtc->atomic.fb_bits |= > - INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe); > - > - intel_crtc->atomic.update_fbc = true; > - > - if (intel_wm_need_update(plane, &state->base)) > - intel_crtc->atomic.update_wm = true; > - } > - > - return 0; > + return drm_plane_helper_check_update(plane, crtc, fb, > + src, dest, clip, > + DRM_PLANE_HELPER_NO_SCALING, > + DRM_PLANE_HELPER_NO_SCALING, > + can_position, true, > + &state->visible); > } > > static void > @@ -12600,8 +12677,10 @@ intel_commit_primary_plane(struct drm_plane *plane, > /* FIXME: kill this fastboot hack */ > intel_update_pipe_size(intel_crtc); > > - dev_priv->display.update_primary_plane(crtc, plane->fb, > - crtc->x, crtc->y); > + dev_priv->display.update_primary_plane(crtc, > + state->base.fb, > + state->src.x1 >> 16, > + state->src.y1 >> 16); > } > } > > @@ -12616,6 +12695,123 @@ intel_disable_primary_plane(struct drm_plane *plane, > dev_priv->display.update_primary_plane(crtc, NULL, 0, 0); > } > > +/* Transitional checking here, mostly for plane updates */ > +static int intel_atomic_check_crtc(struct drm_crtc *crtc, > + struct drm_crtc_state *crtc_state) > +{ > + struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc_state); > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + struct drm_atomic_state *state = crtc_state->state; > + struct drm_device *dev = crtc->dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + unsigned plane_mask; > + int i, nplanes = dev->mode_config.num_total_plane, idx; > + bool mode_changed = needs_modeset(crtc_state); > + bool is_crtc_enabled = crtc_state->active; > + bool was_crtc_enabled = crtc->state->active && intel_crtc->active; > + > + memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic)); > + intel_crtc->atomic.update_wm = mode_changed; > + > + idx = drm_crtc_index(crtc); > + DRM_DEBUG_ATOMIC("Crtc %i was enabled %i now enabled: %i\n", > + idx, was_crtc_enabled, is_crtc_enabled); > + > + plane_mask = crtc_state->plane_mask | crtc->state->plane_mask; > + for (i = 0; i < nplanes; i++) { > + struct intel_plane_state *plane_state, *old_plane_state; > + struct intel_plane *plane = to_intel_plane(state->planes[i]); > + bool turn_off, turn_on, visible, was_visible; > + struct drm_framebuffer *fb; > + > + if (!plane) > + continue; > + > + plane_state = to_intel_plane_state(state->plane_states[i]); > + old_plane_state = to_intel_plane_state(plane->base.state); > + > + was_visible = old_plane_state->visible && was_crtc_enabled; > + visible = plane_state->visible && is_crtc_enabled; > + > + turn_off = was_visible && (!visible || mode_changed); > + turn_on = visible && (!was_visible || mode_changed); > + fb = plane_state->base.fb; > + > + DRM_DEBUG_ATOMIC("Crtc %i has plane %i with fb %i\n", idx, > + drm_plane_index(&plane->base), fb ? fb->base.id : -1); > + DRM_DEBUG_ATOMIC("\tvisible %i -> %i, off %i, on %i, ms %i\n", > + was_visible, visible, turn_off, turn_on, mode_changed); > + > + /* plane being turned off as part of modeset or changes? */ > + if (intel_wm_need_update(&plane->base, &plane_state->base)) > + intel_crtc->atomic.update_wm = true; > + > + /* > + * 'prepare' is never called when plane is being disabled, so > + * we need to handle frontbuffer tracking as a special case > + */ > + if (old_plane_state->base.fb && !plane_state->base.fb) > + intel_crtc->atomic.disabled_planes |= > + (1 << drm_plane_index(&plane->base)); > + > + switch (plane->base.type) { > + case DRM_PLANE_TYPE_PRIMARY: > + intel_crtc->atomic.fb_bits |= > + INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe); > + > + intel_crtc->atomic.wait_for_flips = true; > + intel_crtc->atomic.pre_disable_primary = turn_off; > + intel_crtc->atomic.post_enable_primary = turn_on; > + > + if (turn_off) > + intel_crtc->atomic.disable_fbc = true; > + > + /* > + * FBC does not work on some platforms for rotated > + * planes, so disable it when rotation is not 0 and > + * update it when rotation is set back to 0. > + * > + * FIXME: This is redundant with the fbc update done in > + * the primary plane enable function except that that > + * one is done too late. We eventually need to unify > + * this. > + */ > + > + if (visible && > + INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) && > + dev_priv->fbc.crtc == intel_crtc && > + plane_state->base.rotation != BIT(DRM_ROTATE_0)) > + intel_crtc->atomic.disable_fbc = true; > + > + /* > + * BDW signals flip done immediately if the plane > + * is disabled, even if the plane enable is already > + * armed to occur at the next vblank :( > + */ > + if (turn_on && IS_BROADWELL(dev)) > + intel_crtc->atomic.wait_vblank = true; > + > + intel_crtc->atomic.update_fbc = true; > + break; > + case DRM_PLANE_TYPE_CURSOR: > + intel_crtc->atomic.fb_bits |= > + INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe); > + break; > + case DRM_PLANE_TYPE_OVERLAY: > + intel_crtc->atomic.fb_bits |= > + INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe); > + > + if (turn_off) { > + intel_crtc->atomic.wait_vblank = true; > + intel_crtc->atomic.update_sprite_watermarks |= > + (1 << drm_plane_index(&plane->base)); > + } > + break; > + } > + } > + return 0; > +} > + Is there a specific reason why this needs to be in check_crtc vs check_plane? IMO, it would make review easier if this move was a separate patch. > static void intel_begin_crtc_commit(struct drm_crtc *crtc) > { > struct drm_device *dev = crtc->dev; > @@ -12664,10 +12860,13 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc) > intel_runtime_pm_get(dev_priv); > > /* Perform vblank evasion around commit operation */ > - if (intel_crtc->active) > + if (intel_crtc->active && !needs_modeset(crtc->state) && > + !dev_priv->power_domains.init_power_on) > intel_crtc->atomic.evade = > intel_pipe_update_start(intel_crtc, > &intel_crtc->atomic.start_vbl_count); > + else > + intel_crtc->atomic.evade = false; > } > > static void intel_finish_crtc_commit(struct drm_crtc *crtc) > @@ -12703,6 +12902,8 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc) > false, false); > > memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic)); > + crtc->state->mode_changed = false; > + crtc->state->active_changed = false; > } > > /** > @@ -12812,13 +13013,9 @@ intel_check_cursor_plane(struct drm_plane *plane, > struct drm_rect *src = &state->src; > const struct drm_rect *clip = &state->clip; > struct drm_i915_gem_object *obj = intel_fb_obj(fb); > - struct intel_crtc *intel_crtc; > unsigned stride; > int ret; > > - crtc = crtc ? crtc : plane->crtc; > - intel_crtc = to_intel_crtc(crtc); > - > ret = drm_plane_helper_check_update(plane, crtc, fb, > src, dest, clip, > DRM_PLANE_HELPER_NO_SCALING, > @@ -12830,7 +13027,7 @@ intel_check_cursor_plane(struct drm_plane *plane, > > /* if we want to turn off the cursor ignore width and height */ > if (!obj) > - goto finish; > + return 0; > > /* Check for which cursor types we support */ > if (!cursor_size_ok(dev, state->base.crtc_w, state->base.crtc_h)) { > @@ -12847,19 +13044,10 @@ intel_check_cursor_plane(struct drm_plane *plane, > > if (fb->modifier[0] != DRM_FORMAT_MOD_NONE) { > DRM_DEBUG_KMS("cursor cannot be tiled\n"); > - ret = -EINVAL; > - } > - > -finish: > - if (intel_crtc->active) { > - if (plane->state->crtc_w != state->base.crtc_w) > - intel_crtc->atomic.update_wm = true; > - > - intel_crtc->atomic.fb_bits |= > - INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe); > + return -EINVAL; > } > > - return ret; > + return 0; > } > > static void > @@ -14089,6 +14277,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) > > WARN_ON(crtc->active); > crtc->base.state->enable = false; > + crtc->base.state->active = false; > crtc->base.enabled = false; > } > > @@ -14117,6 +14306,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) > crtc->active ? "enabled" : "disabled"); > > crtc->base.state->enable = crtc->active; > + crtc->base.state->active = crtc->active; > crtc->base.enabled = crtc->active; > > /* Because we only establish the connector -> encoder -> > @@ -14255,6 +14445,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > crtc->config); > > crtc->base.state->enable = crtc->active; > + crtc->base.state->active = crtc->active; > crtc->base.enabled = crtc->active; > Should this be a separate bug fix? Ander > plane_state = to_intel_plane_state(primary->state); > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index 7419e04b113f..5a277757ac2d 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -811,12 +811,8 @@ intel_check_sprite_plane(struct drm_plane *plane, > int max_scale, min_scale; > int pixel_size; > > - intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc); > - > - if (!fb) { > - state->visible = false; > - goto finish; > - } > + if (!fb) > + return 0; > > /* Don't modify another pipe's plane */ > if (intel_plane->pipe != intel_crtc->pipe) { > @@ -847,7 +843,7 @@ intel_check_sprite_plane(struct drm_plane *plane, > vscale = drm_rect_calc_vscale_relaxed(src, dst, min_scale, max_scale); > BUG_ON(vscale < 0); > > - state->visible = drm_rect_clip_scaled(src, dst, clip, hscale, vscale); > + state->visible = drm_rect_clip_scaled(src, dst, clip, hscale, vscale); > > crtc_x = dst->x1; > crtc_y = dst->y1; > @@ -952,29 +948,6 @@ intel_check_sprite_plane(struct drm_plane *plane, > dst->y1 = crtc_y; > dst->y2 = crtc_y + crtc_h; > > -finish: > - /* > - * If the sprite is completely covering the primary plane, > - * we can disable the primary and save power. > - */ > - if (intel_crtc->active) { > - intel_crtc->atomic.fb_bits |= > - INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe); > - > - if (intel_wm_need_update(plane, &state->base)) > - intel_crtc->atomic.update_wm = true; > - > - if (!state->visible) { > - /* > - * Avoid underruns when disabling the sprite. > - * FIXME remove once watermark updates are done properly. > - */ > - intel_crtc->atomic.wait_vblank = true; > - intel_crtc->atomic.update_sprite_watermarks |= > - (1 << drm_plane_index(plane)); > - } > - } > - > return 0; > } > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH RFC 5/5] drm/i915: Implement intel_crtc_toggle using atomic state 2015-04-22 11:24 [PATCH RFC 0/5] Convert planes and crtc state updates to atomic maarten.lankhorst ` (3 preceding siblings ...) 2015-04-22 11:24 ` [PATCH RFC 4/5] drm/i915: make plane helpers fully atomic maarten.lankhorst @ 2015-04-22 11:24 ` maarten.lankhorst 2015-04-22 14:18 ` Maarten Lankhorst 2015-04-23 6:23 ` [PATCH v2 " Maarten Lankhorst 2015-04-23 6:29 ` [PATCH v2 RFC 6/5] drm/i915: Update less state during modeset Maarten Lankhorst 5 siblings, 2 replies; 14+ messages in thread From: maarten.lankhorst @ 2015-04-22 11:24 UTC (permalink / raw) To: intel-gfx; +Cc: Ander Conselvan de Oliveira From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Assume the function is locked with drm_modeset_lock_all for now. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 131 ++++++++++------------------------- 1 file changed, 38 insertions(+), 93 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 99a45bee20d8..5a43ac02b925 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -106,8 +106,6 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc); static void intel_finish_crtc_commit(struct drm_crtc *crtc); static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state); -static void intel_crtc_enable_planes(struct drm_crtc *crtc); -static void intel_crtc_disable_planes(struct drm_crtc *crtc); static void intel_pre_disable_primary(struct drm_crtc *crtc); static void intel_post_enable_primary(struct drm_crtc *crtc); @@ -2197,28 +2195,6 @@ void intel_flush_primary_plane(struct drm_i915_private *dev_priv, POSTING_READ(reg); } -/** - * intel_enable_primary_hw_plane - enable the primary plane on a given pipe - * @plane: plane to be enabled - * @crtc: crtc for the plane - * - * Enable @plane on @crtc, making sure that the pipe is running first. - */ -static void intel_enable_primary_hw_plane(struct drm_plane *plane, - struct drm_crtc *crtc) -{ - struct drm_device *dev = plane->dev; - struct drm_i915_private *dev_priv = dev->dev_private; - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - - /* If the pipe isn't enabled, we can't pump pixels and may hang */ - assert_pipe_enabled(dev_priv, intel_crtc->pipe); - to_intel_plane_state(plane->state)->visible = true; - - dev_priv->display.update_primary_plane(crtc, plane->fb, - crtc->x, crtc->y); -} - static bool need_vtd_wa(struct drm_device *dev) { #ifdef CONFIG_INTEL_IOMMU @@ -4455,20 +4431,6 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc) } } -static void intel_enable_sprite_planes(struct drm_crtc *crtc) -{ - struct drm_device *dev = crtc->dev; - enum pipe pipe = to_intel_crtc(crtc)->pipe; - struct drm_plane *plane; - struct intel_plane *intel_plane; - - drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) { - intel_plane = to_intel_plane(plane); - if (intel_plane->pipe == pipe) - intel_plane_restore(&intel_plane->base); - } -} - void hsw_enable_ips(struct intel_crtc *crtc) { struct drm_device *dev = crtc->base.dev; @@ -4698,44 +4660,6 @@ intel_pre_disable_primary(struct drm_crtc *crtc) hsw_disable_ips(intel_crtc); } -static void intel_crtc_enable_planes(struct drm_crtc *crtc) -{ - intel_enable_primary_hw_plane(crtc->primary, crtc); - intel_enable_sprite_planes(crtc); - intel_crtc_update_cursor(crtc, true); - - intel_post_enable_primary(crtc); -} - -static void intel_crtc_disable_planes(struct drm_crtc *crtc) -{ - struct drm_device *dev = crtc->dev; - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - struct intel_plane *intel_plane; - int pipe = intel_crtc->pipe; - - intel_crtc_wait_for_pending_flips(crtc); - - intel_pre_disable_primary(crtc); - - intel_crtc_dpms_overlay_disable(intel_crtc); - for_each_intel_plane(dev, intel_plane) { - if (intel_plane->pipe == pipe) { - struct drm_crtc *from = intel_plane->base.crtc; - - intel_plane->disable_plane(&intel_plane->base, - from ?: crtc, true); - } - } - - /* - * FIXME: Once we grow proper nuclear flip support out of this we need - * to compute the mask of flip planes precisely. For the time being - * consider this a flip to a NULL plane. - */ - intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe)); -} - static void ironlake_crtc_enable(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev; @@ -5728,10 +5652,13 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) void intel_crtc_control(struct drm_crtc *crtc, bool enable) { struct drm_device *dev = crtc->dev; - struct drm_i915_private *dev_priv = dev->dev_private; + struct drm_mode_config *config = &dev->mode_config; + struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - enum intel_display_power_domain domain; - unsigned long domains; + struct intel_crtc_state *pipe_config; + struct drm_plane_state *plane_state; + struct drm_atomic_state *state; + int ret; if (enable == intel_crtc->active) return; @@ -5740,23 +5667,41 @@ void intel_crtc_control(struct drm_crtc *crtc, bool enable) return; crtc->state->active = enable; - if (enable) { - domains = get_crtc_power_domains(crtc); - for_each_power_domain(domain, domains) - intel_display_power_get(dev_priv, domain); - intel_crtc->enabled_power_domains = domains; - dev_priv->display.crtc_enable(crtc); - intel_crtc_enable_planes(crtc); - } else { - intel_crtc_disable_planes(crtc); - dev_priv->display.crtc_disable(crtc); + /* this function should be called with drm_modeset_lock_all for now */ + if (WARN_ON(!ctx)) + return; + lockdep_assert_held(&ctx->ww_ctx); - domains = intel_crtc->enabled_power_domains; - for_each_power_domain(domain, domains) - intel_display_power_put(dev_priv, domain); - intel_crtc->enabled_power_domains = 0; + state = drm_atomic_state_alloc(dev); + if (WARN_ON(!state)) + return; + + state->acquire_ctx = ctx; + state->allow_modeset = true; + + pipe_config = intel_atomic_get_crtc_state(state, intel_crtc); + if (IS_ERR(pipe_config)) { + ret = PTR_ERR(pipe_config); + goto err; + } + pipe_config->base.active = enable; + + plane_state = drm_atomic_get_plane_state(state, crtc->primary); + if (IS_ERR(plane_state)) { + ret = PTR_ERR(plane_state); + goto err; } + + ret = intel_set_mode(crtc, state); + if (!ret) + return; + + DRM_ERROR("Failed to toggle crtc!\n"); + +err: + DRM_ERROR("Updating crtc active failed with %i\n", ret); + drm_atomic_state_free(state); } /** -- 2.1.0 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 5/5] drm/i915: Implement intel_crtc_toggle using atomic state 2015-04-22 11:24 ` [PATCH RFC 5/5] drm/i915: Implement intel_crtc_toggle using atomic state maarten.lankhorst @ 2015-04-22 14:18 ` Maarten Lankhorst 2015-04-23 6:23 ` [PATCH v2 " Maarten Lankhorst 1 sibling, 0 replies; 14+ messages in thread From: Maarten Lankhorst @ 2015-04-22 14:18 UTC (permalink / raw) To: intel-gfx; +Cc: Ander Conselvan de Oliveira Op 22-04-15 om 13:24 schreef maarten.lankhorst@linux.intel.com: > From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Assume the function is locked with drm_modeset_lock_all for now. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 131 ++++++++++------------------------- > 1 file changed, 38 insertions(+), 93 deletions(-) > <snip> > static void ironlake_crtc_enable(struct drm_crtc *crtc) > { > struct drm_device *dev = crtc->dev; > @@ -5728,10 +5652,13 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) > void intel_crtc_control(struct drm_crtc *crtc, bool enable) > { > struct drm_device *dev = crtc->dev; > - struct drm_i915_private *dev_priv = dev->dev_private; > + struct drm_mode_config *config = &dev->mode_config; > + struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx; > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > - enum intel_display_power_domain domain; > - unsigned long domains; > + struct intel_crtc_state *pipe_config; > + struct drm_plane_state *plane_state; > + struct drm_atomic_state *state; > + int ret; > > if (enable == intel_crtc->active) > return; > @@ -5740,23 +5667,41 @@ void intel_crtc_control(struct drm_crtc *crtc, bool enable) > return; > > crtc->state->active = enable; ^Oops, this line should be removed now or this patch will glitch badly. And a lot of places that use state->enable should use state->active instead. Still leaving this here as it's a RFC. :-) > - if (enable) { > - domains = get_crtc_power_domains(crtc); > - for_each_power_domain(domain, domains) > - intel_display_power_get(dev_priv, domain); > - intel_crtc->enabled_power_domains = domains; > > - dev_priv->display.crtc_enable(crtc); > - intel_crtc_enable_planes(crtc); > - } else { > - intel_crtc_disable_planes(crtc); > - dev_priv->display.crtc_disable(crtc); > + /* this function should be called with drm_modeset_lock_all for now */ > + if (WARN_ON(!ctx)) > + return; > + lockdep_assert_held(&ctx->ww_ctx); > > - domains = intel_crtc->enabled_power_domains; > - for_each_power_domain(domain, domains) > - intel_display_power_put(dev_priv, domain); > - intel_crtc->enabled_power_domains = 0; > + state = drm_atomic_state_alloc(dev); > + if (WARN_ON(!state)) > + return; > + > + state->acquire_ctx = ctx; > + state->allow_modeset = true; > + > + pipe_config = intel_atomic_get_crtc_state(state, intel_crtc); > + if (IS_ERR(pipe_config)) { > + ret = PTR_ERR(pipe_config); > + goto err; > + } > + pipe_config->base.active = enable; > + > + plane_state = drm_atomic_get_plane_state(state, crtc->primary); > + if (IS_ERR(plane_state)) { > + ret = PTR_ERR(plane_state); > + goto err; > } > + > + ret = intel_set_mode(crtc, state); > + if (!ret) > + return; > + > + DRM_ERROR("Failed to toggle crtc!\n"); > + > +err: > + DRM_ERROR("Updating crtc active failed with %i\n", ret); > + drm_atomic_state_free(state); > } > > /** _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 RFC 5/5] drm/i915: Implement intel_crtc_toggle using atomic state 2015-04-22 11:24 ` [PATCH RFC 5/5] drm/i915: Implement intel_crtc_toggle using atomic state maarten.lankhorst 2015-04-22 14:18 ` Maarten Lankhorst @ 2015-04-23 6:23 ` Maarten Lankhorst 1 sibling, 0 replies; 14+ messages in thread From: Maarten Lankhorst @ 2015-04-23 6:23 UTC (permalink / raw) To: intel-gfx; +Cc: Ander Conselvan de Oliveira Assume the function is locked with drm_modeset_lock_all for now. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- Changes since RFC v1: - Get rid of the crtc->state->active assignment in intel_crtc_control, it caused the whole state to be confused. - Convert some places that use state->enable to state->active. - crtc->state->active should mirror crtc->active now, latter should be removed at some point. drivers/gpu/drm/i915/i915_irq.c | 2 +- drivers/gpu/drm/i915/intel_display.c | 174 +++++++++++++---------------------- 2 files changed, 65 insertions(+), 111 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 9da955e4f355..a6816503a080 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -796,7 +796,7 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe, return -EINVAL; } - if (!crtc->state->enable) { + if (!crtc->state->active) { DRM_DEBUG_KMS("crtc %d is disabled\n", pipe); return -EBUSY; } diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index acb5c5bea428..2d2ada580b36 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -106,8 +106,6 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc); static void intel_finish_crtc_commit(struct drm_crtc *crtc); static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state); -static void intel_crtc_enable_planes(struct drm_crtc *crtc); -static void intel_crtc_disable_planes(struct drm_crtc *crtc); static void intel_pre_disable_primary(struct drm_crtc *crtc); static void intel_post_enable_primary(struct drm_crtc *crtc); @@ -2197,28 +2195,6 @@ void intel_flush_primary_plane(struct drm_i915_private *dev_priv, POSTING_READ(reg); } -/** - * intel_enable_primary_hw_plane - enable the primary plane on a given pipe - * @plane: plane to be enabled - * @crtc: crtc for the plane - * - * Enable @plane on @crtc, making sure that the pipe is running first. - */ -static void intel_enable_primary_hw_plane(struct drm_plane *plane, - struct drm_crtc *crtc) -{ - struct drm_device *dev = plane->dev; - struct drm_i915_private *dev_priv = dev->dev_private; - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - - /* If the pipe isn't enabled, we can't pump pixels and may hang */ - assert_pipe_enabled(dev_priv, intel_crtc->pipe); - to_intel_plane_state(plane->state)->visible = true; - - dev_priv->display.update_primary_plane(crtc, plane->fb, - crtc->x, crtc->y); -} - static bool need_vtd_wa(struct drm_device *dev) { #ifdef CONFIG_INTEL_IOMMU @@ -4455,20 +4431,6 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc) } } -static void intel_enable_sprite_planes(struct drm_crtc *crtc) -{ - struct drm_device *dev = crtc->dev; - enum pipe pipe = to_intel_crtc(crtc)->pipe; - struct drm_plane *plane; - struct intel_plane *intel_plane; - - drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) { - intel_plane = to_intel_plane(plane); - if (intel_plane->pipe == pipe) - intel_plane_restore(&intel_plane->base); - } -} - void hsw_enable_ips(struct intel_crtc *crtc) { struct drm_device *dev = crtc->base.dev; @@ -4539,7 +4501,7 @@ static void intel_crtc_load_lut(struct drm_crtc *crtc) bool reenable_ips = false; /* The clocks have to be on to load the palette. */ - if (!crtc->state->enable || !intel_crtc->active) + if (!crtc->state->active || !intel_crtc->active) return; if (!HAS_PCH_SPLIT(dev_priv->dev)) { @@ -4698,44 +4660,6 @@ intel_pre_disable_primary(struct drm_crtc *crtc) hsw_disable_ips(intel_crtc); } -static void intel_crtc_enable_planes(struct drm_crtc *crtc) -{ - intel_enable_primary_hw_plane(crtc->primary, crtc); - intel_enable_sprite_planes(crtc); - intel_crtc_update_cursor(crtc, true); - - intel_post_enable_primary(crtc); -} - -static void intel_crtc_disable_planes(struct drm_crtc *crtc) -{ - struct drm_device *dev = crtc->dev; - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - struct intel_plane *intel_plane; - int pipe = intel_crtc->pipe; - - intel_crtc_wait_for_pending_flips(crtc); - - intel_pre_disable_primary(crtc); - - intel_crtc_dpms_overlay_disable(intel_crtc); - for_each_intel_plane(dev, intel_plane) { - if (intel_plane->pipe == pipe) { - struct drm_crtc *from = intel_plane->base.crtc; - - intel_plane->disable_plane(&intel_plane->base, - from ?: crtc, true); - } - } - - /* - * FIXME: Once we grow proper nuclear flip support out of this we need - * to compute the mask of flip planes precisely. For the time being - * consider this a flip to a NULL plane. - */ - intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe)); -} - static void ironlake_crtc_enable(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev; @@ -5441,7 +5365,7 @@ static int valleyview_modeset_global_pipes(struct drm_atomic_state *state) /* add all active pipes to the state */ for_each_crtc(state->dev, crtc) { - if (!crtc->state->enable) + if (!crtc->state->active) continue; crtc_state = drm_atomic_get_crtc_state(state, crtc); @@ -5539,7 +5463,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc) int pipe = intel_crtc->pipe; bool is_dsi; - WARN_ON(!crtc->state->enable); + WARN_ON(!crtc->state->active); if (intel_crtc->active) return; @@ -5617,7 +5541,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc) struct intel_encoder *encoder; int pipe = intel_crtc->pipe; - WARN_ON(!crtc->state->enable); + WARN_ON(!crtc->state->active); if (intel_crtc->active) return; @@ -5728,10 +5652,13 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) void intel_crtc_control(struct drm_crtc *crtc, bool enable) { struct drm_device *dev = crtc->dev; - struct drm_i915_private *dev_priv = dev->dev_private; + struct drm_mode_config *config = &dev->mode_config; + struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - enum intel_display_power_domain domain; - unsigned long domains; + struct intel_crtc_state *pipe_config; + struct drm_plane_state *plane_state; + struct drm_atomic_state *state; + int ret; if (enable == intel_crtc->active) return; @@ -5739,24 +5666,40 @@ void intel_crtc_control(struct drm_crtc *crtc, bool enable) if (enable && !crtc->state->enable) return; - crtc->state->active = enable; - if (enable) { - domains = get_crtc_power_domains(crtc); - for_each_power_domain(domain, domains) - intel_display_power_get(dev_priv, domain); - intel_crtc->enabled_power_domains = domains; + /* this function should be called with drm_modeset_lock_all for now */ + if (WARN_ON(!ctx)) + return; + lockdep_assert_held(&ctx->ww_ctx); - dev_priv->display.crtc_enable(crtc); - intel_crtc_enable_planes(crtc); - } else { - intel_crtc_disable_planes(crtc); - dev_priv->display.crtc_disable(crtc); + state = drm_atomic_state_alloc(dev); + if (WARN_ON(!state)) + return; - domains = intel_crtc->enabled_power_domains; - for_each_power_domain(domain, domains) - intel_display_power_put(dev_priv, domain); - intel_crtc->enabled_power_domains = 0; + state->acquire_ctx = ctx; + state->allow_modeset = true; + + pipe_config = intel_atomic_get_crtc_state(state, intel_crtc); + if (IS_ERR(pipe_config)) { + ret = PTR_ERR(pipe_config); + goto err; } + pipe_config->base.active = enable; + + plane_state = drm_atomic_get_plane_state(state, crtc->primary); + if (IS_ERR(plane_state)) { + ret = PTR_ERR(plane_state); + goto err; + } + + ret = intel_set_mode(crtc, state); + if (!ret) + return; + + DRM_ERROR("Failed to toggle crtc!\n"); + +err: + DRM_ERROR("Updating crtc active failed with %i\n", ret); + drm_atomic_state_free(state); } /** @@ -5832,7 +5775,7 @@ static void intel_connector_check_state(struct intel_connector *connector) crtc = encoder->base.crtc; - I915_STATE_WARN(!crtc->state->enable, + I915_STATE_WARN(!crtc->state->active, "crtc not enabled\n"); I915_STATE_WARN(!to_intel_crtc(crtc)->active, "crtc not active\n"); I915_STATE_WARN(pipe != to_intel_crtc(crtc)->pipe, @@ -11232,7 +11175,7 @@ intel_modeset_update_state(struct drm_atomic_state *state) if (!crtc_state || !needs_modeset(crtc->state)) continue; - if (crtc->state->enable) { + if (crtc->state->active) { struct drm_property *dpms_property = dev->mode_config.dpms_property; @@ -11243,8 +11186,15 @@ intel_modeset_update_state(struct drm_atomic_state *state) intel_encoder = to_intel_encoder(connector->encoder); intel_encoder->connectors_active = true; - } else + } else { + struct drm_property *dpms_property = + dev->mode_config.dpms_property; + connector->dpms = DRM_MODE_DPMS_OFF; + drm_object_property_set_value(&connector->base, + dpms_property, + DRM_MODE_DPMS_OFF); + } } } @@ -11692,7 +11642,7 @@ check_shared_dpll_state(struct drm_device *dev) pll->on, active); for_each_intel_crtc(dev, crtc) { - if (crtc->base.state->enable && intel_crtc_to_shared_dpll(crtc) == pll) + if (crtc->base.state->active && intel_crtc_to_shared_dpll(crtc) == pll) enabled_crtcs++; if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll) active_crtcs++; @@ -11825,7 +11775,7 @@ intel_modeset_compute_config(struct drm_crtc *crtc, return ERR_PTR(ret); } - if (!pipe_config->base.enable) + if (!pipe_config->base.active) goto done; ret = intel_modeset_pipe_config(crtc, state, pipe_config); @@ -11885,7 +11835,7 @@ static int __intel_set_mode_setup_plls(struct drm_atomic_state *state) goto done; for_each_crtc_in_state(state, crtc, crtc_state, i) { - if (!needs_modeset(crtc_state) || !crtc_state->enable) + if (!needs_modeset(crtc_state) || !crtc_state->active) continue; intel_crtc = to_intel_crtc(crtc); @@ -12072,7 +12022,7 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, * pipes; here we assume a single modeset_pipe and only track the * single crtc and mode. */ - if (pipe_config->base.enable && needs_modeset(&pipe_config->base)) { + if (pipe_config->base.active && needs_modeset(&pipe_config->base)) { modeset_crtc->mode = pipe_config->base.mode; /* @@ -12095,7 +12045,7 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, /* Now enable the clocks, plane, pipe, and connectors that we set up. */ for_each_crtc_in_state(state, crtc, crtc_state, i) { - if (!crtc->state->enable) + if (!crtc->state->active) continue; update_scanline_offset(to_intel_crtc(crtc)); @@ -12708,12 +12658,16 @@ static int intel_atomic_check_crtc(struct drm_crtc *crtc, int i, nplanes = dev->mode_config.num_total_plane, idx; bool mode_changed = needs_modeset(crtc_state); bool is_crtc_enabled = crtc_state->active; - bool was_crtc_enabled = crtc->state->active && intel_crtc->active; + bool was_crtc_enabled = crtc->state->active; memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic)); intel_crtc->atomic.update_wm = mode_changed; - idx = drm_crtc_index(crtc); + idx = crtc->base.id; + I915_STATE_WARN(crtc->state->active != intel_crtc->active, + "Crtc %i mismatch between state->active(%i) and crtc->active (%i)\n", + idx, crtc->state->active, intel_crtc->active); + DRM_DEBUG_ATOMIC("Crtc %i was enabled %i now enabled: %i\n", idx, was_crtc_enabled, is_crtc_enabled); @@ -12738,7 +12692,7 @@ static int intel_atomic_check_crtc(struct drm_crtc *crtc, fb = plane_state->base.fb; DRM_DEBUG_ATOMIC("Crtc %i has plane %i with fb %i\n", idx, - drm_plane_index(&plane->base), fb ? fb->base.id : -1); + plane->base.base.id, fb ? fb->base.id : -1); DRM_DEBUG_ATOMIC("\tvisible %i -> %i, off %i, on %i, ms %i\n", was_visible, visible, turn_off, turn_on, mode_changed); @@ -14294,7 +14248,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) * have active connectors/encoders. */ intel_crtc_update_dpms(&crtc->base); - if (crtc->active != crtc->base.state->enable) { + if (crtc->active != crtc->base.state->active) { struct intel_encoder *encoder; /* This can happen either due to bugs in the get_hw_state -- 2.1.0 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 RFC 6/5] drm/i915: Update less state during modeset. 2015-04-22 11:24 [PATCH RFC 0/5] Convert planes and crtc state updates to atomic maarten.lankhorst ` (4 preceding siblings ...) 2015-04-22 11:24 ` [PATCH RFC 5/5] drm/i915: Implement intel_crtc_toggle using atomic state maarten.lankhorst @ 2015-04-23 6:29 ` Maarten Lankhorst 5 siblings, 0 replies; 14+ messages in thread From: Maarten Lankhorst @ 2015-04-23 6:29 UTC (permalink / raw) To: intel-gfx; +Cc: Ander Conselvan de Oliveira No need to repeatedly call update_watermarks, or update_fbc. For update_watermarks once should be enough after disabling crtc's and swapping the state. Down to a single call to update_watermarks in .crtc_enable Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- There is no v1, this patch was added later to the series. :-) The order in ironlake_crtc_disable is slightly changed with intel_disable_shared_dpll and ironlake_fdi_pll_disable order changed. I have no idea if this can cause problems, but it works on my system. drivers/gpu/drm/i915/intel_display.c | 79 +++++++++++++----------------------- 1 file changed, 28 insertions(+), 51 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2d2ada580b36..16204c525004 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4588,10 +4588,6 @@ intel_post_enable_primary(struct drm_crtc *crtc) */ hsw_enable_ips(intel_crtc); - mutex_lock(&dev->struct_mutex); - intel_fbc_update(dev); - mutex_unlock(&dev->struct_mutex); - /* * Gen2 reports pipe underruns whenever all planes are disabled. * So don't enable underrun reporting before at least some planes @@ -4646,11 +4642,6 @@ intel_pre_disable_primary(struct drm_crtc *crtc) if (HAS_GMCH_DISPLAY(dev)) intel_set_memory_cxsr(dev_priv, false); - mutex_lock(&dev->struct_mutex); - if (dev_priv->fbc.crtc == intel_crtc) - intel_fbc_disable(dev); - mutex_unlock(&dev->struct_mutex); - /* * FIXME IPS should be fine as long as one plane is * enabled, but in practice it seems to have problems @@ -4876,9 +4867,6 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) int pipe = intel_crtc->pipe; u32 reg, temp; - if (!intel_crtc->active) - return; - for_each_encoder_on_crtc(dev, crtc, encoder) encoder->disable(encoder); @@ -4916,18 +4904,8 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) I915_WRITE(PCH_DPLL_SEL, temp); } - /* disable PCH DPLL */ - intel_disable_shared_dpll(intel_crtc); - ironlake_fdi_pll_disable(intel_crtc); } - - intel_crtc->active = false; - intel_update_watermarks(crtc); - - mutex_lock(&dev->struct_mutex); - intel_fbc_update(dev); - mutex_unlock(&dev->struct_mutex); } static void haswell_crtc_disable(struct drm_crtc *crtc) @@ -4938,9 +4916,6 @@ static void haswell_crtc_disable(struct drm_crtc *crtc) struct intel_encoder *encoder; enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder; - if (!intel_crtc->active) - return; - for_each_encoder_on_crtc(dev, crtc, encoder) { intel_opregion_notify_encoder(encoder, false); encoder->disable(encoder); @@ -4974,16 +4949,6 @@ static void haswell_crtc_disable(struct drm_crtc *crtc) for_each_encoder_on_crtc(dev, crtc, encoder) if (encoder->post_disable) encoder->post_disable(encoder); - - intel_crtc->active = false; - intel_update_watermarks(crtc); - - mutex_lock(&dev->struct_mutex); - intel_fbc_update(dev); - mutex_unlock(&dev->struct_mutex); - - if (intel_crtc_to_shared_dpll(intel_crtc)) - intel_disable_shared_dpll(intel_crtc); } static void i9xx_pfit_enable(struct intel_crtc *crtc) @@ -5603,9 +5568,6 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) struct intel_encoder *encoder; int pipe = intel_crtc->pipe; - if (!intel_crtc->active) - return; - /* * On gen2 planes are double buffered but the pipe isn't, so we must * wait for planes to fully turn off before disabling the pipe. @@ -5639,13 +5601,6 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) if (!IS_GEN2(dev)) intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false); - - intel_crtc->active = false; - intel_update_watermarks(crtc); - - mutex_lock(&dev->struct_mutex); - intel_fbc_update(dev); - mutex_unlock(&dev->struct_mutex); } /* Master function to enable/disable CRTC and corresponding power wells */ @@ -12008,11 +11963,21 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, __intel_set_mode_update_planes(dev, state); for_each_crtc_in_state(state, crtc, crtc_state, i) { + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + if (!needs_modeset(crtc_state)) continue; + if (!crtc->state->active) + continue; + intel_crtc_dpms_overlay_disable(to_intel_crtc(crtc)); dev_priv->display.crtc_disable(crtc); + + intel_crtc->active = false; + + if (intel_crtc_to_shared_dpll(intel_crtc)) + intel_disable_shared_dpll(intel_crtc); } /* crtc->mode is already used by the ->mode_set callbacks, hence we need @@ -12045,8 +12010,11 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, /* Now enable the clocks, plane, pipe, and connectors that we set up. */ for_each_crtc_in_state(state, crtc, crtc_state, i) { - if (!crtc->state->active) + if (!crtc->state->active) { + if (needs_modeset(crtc->state)) + intel_update_watermarks(crtc); continue; + } update_scanline_offset(to_intel_crtc(crtc)); @@ -12662,6 +12630,7 @@ static int intel_atomic_check_crtc(struct drm_crtc *crtc, memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic)); intel_crtc->atomic.update_wm = mode_changed; + intel_crtc->atomic.disable_fbc = mode_changed; idx = crtc->base.id; I915_STATE_WARN(crtc->state->active != intel_crtc->active, @@ -12763,6 +12732,9 @@ static int intel_atomic_check_crtc(struct drm_crtc *crtc, break; } } + + if (mode_changed) + intel_crtc->atomic.update_wm = false; return 0; } @@ -12802,8 +12774,13 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc) if (intel_crtc->atomic.wait_for_flips) intel_crtc_wait_for_pending_flips(crtc); - if (intel_crtc->atomic.disable_fbc) - intel_fbc_disable(dev); + if (intel_crtc->atomic.disable_fbc && + dev_priv->fbc.crtc == intel_crtc) { + mutex_lock(&dev->struct_mutex); + if (dev_priv->fbc.crtc == intel_crtc) + intel_fbc_disable(dev); + mutex_unlock(&dev->struct_mutex); + } if (intel_crtc->atomic.pre_disable_primary) intel_pre_disable_primary(crtc); @@ -12841,15 +12818,15 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc) intel_frontbuffer_flip(dev, intel_crtc->atomic.fb_bits); + if (intel_crtc->atomic.post_enable_primary) + intel_post_enable_primary(crtc); + if (intel_crtc->atomic.update_fbc) { mutex_lock(&dev->struct_mutex); intel_fbc_update(dev); mutex_unlock(&dev->struct_mutex); } - if (intel_crtc->atomic.post_enable_primary) - intel_post_enable_primary(crtc); - drm_for_each_legacy_plane(p, &dev->mode_config.plane_list) if (intel_crtc->atomic.update_sprite_watermarks & drm_plane_index(p)) intel_update_sprite_watermarks(p, crtc, 0, 0, 0, -- 2.1.0 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2015-05-04 13:42 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-04-22 11:24 [PATCH RFC 0/5] Convert planes and crtc state updates to atomic maarten.lankhorst 2015-04-22 11:24 ` [PATCH RFC 1/5] drm/i915: Get rid of intel_crtc_disable and related code maarten.lankhorst 2015-04-24 8:46 ` Ander Conselvan De Oliveira 2015-04-22 11:24 ` [PATCH RFC 2/5] drm/i915: Only update required power domains maarten.lankhorst 2015-04-24 8:47 ` Ander Conselvan De Oliveira 2015-04-22 11:24 ` [PATCH RFC 3/5] drm/i915: use intel_crtc_control everywhere maarten.lankhorst 2015-05-04 13:44 ` Daniel Vetter 2015-04-22 11:24 ` [PATCH RFC 4/5] drm/i915: make plane helpers fully atomic maarten.lankhorst 2015-04-23 6:19 ` [PATCH v2 " Maarten Lankhorst 2015-04-24 8:52 ` Ander Conselvan De Oliveira 2015-04-22 11:24 ` [PATCH RFC 5/5] drm/i915: Implement intel_crtc_toggle using atomic state maarten.lankhorst 2015-04-22 14:18 ` Maarten Lankhorst 2015-04-23 6:23 ` [PATCH v2 " Maarten Lankhorst 2015-04-23 6:29 ` [PATCH v2 RFC 6/5] drm/i915: Update less state during modeset Maarten Lankhorst
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox