On Thu, Jul 07, 2016 at 01:55:46PM +0200, Maarten Lankhorst wrote:
> This cleans up another possible use of the connector list,
> encoder->crtc is legacy state and should not be used.
>
> With the atomic state as argument it's easy to find the encoder from
> the connector it belongs to.
>
> intel_opregion_notify_encoder is a noop for !HAS_DDI, so it's harmless
> to unconditionally include it in encoder enable/disable.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_display.c | 190 ++++++++++++++++++++++++-----------
> 1 file changed, 134 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8e7d453db48c..19e5379000b8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4704,6 +4704,123 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask
> intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe));
> }
>
> +static void intel_encoders_pre_pll_enable(struct drm_crtc *crtc,
> + struct drm_atomic_state *old_state)
This whole old_state thing is very annoying. We're enabling things, so
intuitively one would assume that we want to look at the new state.
It's not quite as bad here as it's in the cdclk code where old_state
actually contain partially old and partially new state.
Would be nice if someone came up with some kind of sane solution for
this, that would avoid having to crosswire one's brain. Would be nice
to have explicit old and new states I think.
And the magic disable phase is my other main annoyance with atomic. If
we had an explicit new state, then splitting the disable+(re)enable
sequence into three states should be much easier, so that we'd have a
nice transition like so: old_state->stuff_disabled_state->stuff_enabled_state.
But of course this migh still be a little hard to achieve on account of
atomic unwisely mixing up the user visible state and internal state in
the same structures.
> +{
> + struct drm_connector_state *old_conn_state;
> + struct drm_connector *conn;
> + int i;
> +
> + for_each_connector_in_state(old_state, conn, old_conn_state, i) {
> + struct drm_connector_state *conn_state = conn->state;
> + struct intel_encoder *encoder =
> + to_intel_encoder(conn_state->best_encoder);
> +
> + if (conn_state->crtc != crtc)
> + continue;
> +
> + if (encoder->pre_pll_enable)
> + encoder->pre_pll_enable(encoder);
> + }
> +}
> +
> +static void intel_encoders_pre_enable(struct drm_crtc *crtc,
> + struct drm_atomic_state *old_state)
> +{
> + struct drm_connector_state *old_conn_state;
> + struct drm_connector *conn;
> + int i;
> +
> + for_each_connector_in_state(old_state, conn, old_conn_state, i) {
> + struct drm_connector_state *conn_state = conn->state;
> + struct intel_encoder *encoder =
> + to_intel_encoder(conn_state->best_encoder);
> +
> + if (conn_state->crtc != crtc)
> + continue;
> +
> + if (encoder->pre_enable)
> + encoder->pre_enable(encoder);
> + }
> +}
> +
> +static void intel_encoders_enable(struct drm_crtc *crtc,
> + struct drm_atomic_state *old_state)
> +{
> + struct drm_connector_state *old_conn_state;
> + struct drm_connector *conn;
> + int i;
> +
> + for_each_connector_in_state(old_state, conn, old_conn_state, i) {
> + struct drm_connector_state *conn_state = conn->state;
> + struct intel_encoder *encoder =
> + to_intel_encoder(conn_state->best_encoder);
> +
> + if (conn_state->crtc != crtc)
> + continue;
> +
> + encoder->enable(encoder);
> + intel_opregion_notify_encoder(encoder, true);
> + }
> +}
> +
> +static void intel_encoders_disable(struct drm_crtc *crtc,
> + struct drm_atomic_state *old_state)
> +{
> + struct drm_connector_state *old_conn_state;
> + struct drm_connector *conn;
> + int i;
> +
> + for_each_connector_in_state(old_state, conn, old_conn_state, i) {
> + struct intel_encoder *encoder =
> + to_intel_encoder(old_conn_state->best_encoder);
> +
> + if (old_conn_state->crtc != crtc)
> + continue;
> +
> + intel_opregion_notify_encoder(encoder, false);
> + encoder->disable(encoder);
> + }
> +}
> +
> +static void intel_encoders_post_disable(struct drm_crtc *crtc,
> + struct drm_atomic_state *old_state)
> +{
> + struct drm_connector_state *old_conn_state;
> + struct drm_connector *conn;
> + int i;
> +
> + for_each_connector_in_state(old_state, conn, old_conn_state, i) {
> + struct intel_encoder *encoder =
> + to_intel_encoder(old_conn_state->best_encoder);
> +
> + if (old_conn_state->crtc != crtc)
> + continue;
> +
> + if (encoder->post_disable)
> + encoder->post_disable(encoder);
> + }
> +}
> +
> +static void intel_encoders_post_pll_disable(struct drm_crtc *crtc,
> + struct drm_atomic_state *old_state)
> +{
> + struct drm_connector_state *old_conn_state;
> + struct drm_connector *conn;
> + int i;
> +
> + for_each_connector_in_state(old_state, conn, old_conn_state, i) {
> + struct intel_encoder *encoder =
> + to_intel_encoder(old_conn_state->best_encoder);
> +
> + if (old_conn_state->crtc != crtc)
> + continue;
> +
> + if (encoder->post_pll_disable)
> + encoder->post_pll_disable(encoder);
> + }
> +}
> +
> static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config,
> struct drm_atomic_state *old_state)
> {
> @@ -4711,7 +4828,6 @@ static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config,
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - struct intel_encoder *encoder;
> int pipe = intel_crtc->pipe;
>
> if (WARN_ON(intel_crtc->active))
> @@ -4750,9 +4866,7 @@ static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config,
>
> intel_crtc->active = true;
>
> - for_each_encoder_on_crtc(dev, crtc, encoder)
> - if (encoder->pre_enable)
> - encoder->pre_enable(encoder);
> + intel_encoders_pre_enable(crtc, old_state);
>
> if (intel_crtc->config->has_pch_encoder) {
> /* Note: FDI PLL enabling _must_ be done before we enable the
> @@ -4782,8 +4896,7 @@ static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config,
> assert_vblank_disabled(crtc);
> drm_crtc_vblank_on(crtc);
>
> - for_each_encoder_on_crtc(dev, crtc, encoder)
> - encoder->enable(encoder);
> + intel_encoders_enable(crtc, old_state);
>
> if (HAS_PCH_CPT(dev))
> cpt_verify_modeset(dev, intel_crtc->pipe);
> @@ -4808,7 +4921,6 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - struct intel_encoder *encoder;
> int pipe = intel_crtc->pipe, hsw_workaround_pipe;
> enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
>
> @@ -4819,9 +4931,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
> intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
> false);
>
> - for_each_encoder_on_crtc(dev, crtc, encoder)
> - if (encoder->pre_pll_enable)
> - encoder->pre_pll_enable(encoder);
> + intel_encoders_pre_pll_enable(crtc, old_state);
>
> if (intel_crtc->config->shared_dpll)
> intel_enable_shared_dpll(intel_crtc);
> @@ -4859,10 +4969,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
> else
> intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
>
> - for_each_encoder_on_crtc(dev, crtc, encoder) {
> - if (encoder->pre_enable)
> - encoder->pre_enable(encoder);
> - }
> + intel_encoders_pre_enable(crtc, old_state);
>
> if (intel_crtc->config->has_pch_encoder)
> dev_priv->display.fdi_link_train(crtc);
> @@ -4903,10 +5010,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
> assert_vblank_disabled(crtc);
> drm_crtc_vblank_on(crtc);
>
> - for_each_encoder_on_crtc(dev, crtc, encoder) {
> - encoder->enable(encoder);
> - intel_opregion_notify_encoder(encoder, true);
> - }
> + intel_encoders_enable(crtc, old_state);
>
> if (intel_crtc->config->has_pch_encoder) {
> intel_wait_for_vblank(dev, pipe);
> @@ -4947,7 +5051,6 @@ static void ironlake_crtc_disable(struct intel_crtc_state *old_crtc_state,
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - struct intel_encoder *encoder;
> int pipe = intel_crtc->pipe;
>
> /*
> @@ -4960,8 +5063,7 @@ static void ironlake_crtc_disable(struct intel_crtc_state *old_crtc_state,
> intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
> }
>
> - for_each_encoder_on_crtc(dev, crtc, encoder)
> - encoder->disable(encoder);
> + intel_encoders_disable(crtc, old_state);
>
> drm_crtc_vblank_off(crtc);
> assert_vblank_disabled(crtc);
> @@ -4973,9 +5075,7 @@ static void ironlake_crtc_disable(struct intel_crtc_state *old_crtc_state,
> if (intel_crtc->config->has_pch_encoder)
> ironlake_fdi_disable(crtc);
>
> - for_each_encoder_on_crtc(dev, crtc, encoder)
> - if (encoder->post_disable)
> - encoder->post_disable(encoder);
> + intel_encoders_post_disable(crtc, old_state);
>
> if (intel_crtc->config->has_pch_encoder) {
> ironlake_disable_pch_transcoder(dev_priv, pipe);
> @@ -5012,17 +5112,13 @@ static void haswell_crtc_disable(struct intel_crtc_state *old_crtc_state,
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - struct intel_encoder *encoder;
> enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
>
> if (intel_crtc->config->has_pch_encoder)
> intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
> false);
>
> - for_each_encoder_on_crtc(dev, crtc, encoder) {
> - intel_opregion_notify_encoder(encoder, false);
> - encoder->disable(encoder);
> - }
> + intel_encoders_disable(crtc, old_state);
>
> drm_crtc_vblank_off(crtc);
> assert_vblank_disabled(crtc);
> @@ -5045,9 +5141,7 @@ static void haswell_crtc_disable(struct intel_crtc_state *old_crtc_state,
> if (!transcoder_is_dsi(cpu_transcoder))
> intel_ddi_disable_pipe_clock(intel_crtc);
>
> - for_each_encoder_on_crtc(dev, crtc, encoder)
> - if (encoder->post_disable)
> - encoder->post_disable(encoder);
> + intel_encoders_post_disable(crtc, old_state);
>
> if (intel_crtc->config->has_pch_encoder) {
> lpt_disable_pch_transcoder(dev_priv);
> @@ -6127,7 +6221,6 @@ static void valleyview_crtc_enable(struct intel_crtc_state *pipe_config,
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - struct intel_encoder *encoder;
> int pipe = intel_crtc->pipe;
>
> if (WARN_ON(intel_crtc->active))
> @@ -6152,9 +6245,7 @@ static void valleyview_crtc_enable(struct intel_crtc_state *pipe_config,
>
> intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
>
> - for_each_encoder_on_crtc(dev, crtc, encoder)
> - if (encoder->pre_pll_enable)
> - encoder->pre_pll_enable(encoder);
> + intel_encoders_pre_pll_enable(crtc, old_state);
>
> if (IS_CHERRYVIEW(dev)) {
> chv_prepare_pll(intel_crtc, intel_crtc->config);
> @@ -6164,9 +6255,7 @@ static void valleyview_crtc_enable(struct intel_crtc_state *pipe_config,
> vlv_enable_pll(intel_crtc, intel_crtc->config);
> }
>
> - for_each_encoder_on_crtc(dev, crtc, encoder)
> - if (encoder->pre_enable)
> - encoder->pre_enable(encoder);
> + intel_encoders_pre_enable(crtc, old_state);
>
> i9xx_pfit_enable(intel_crtc);
>
> @@ -6178,8 +6267,7 @@ static void valleyview_crtc_enable(struct intel_crtc_state *pipe_config,
> assert_vblank_disabled(crtc);
> drm_crtc_vblank_on(crtc);
>
> - for_each_encoder_on_crtc(dev, crtc, encoder)
> - encoder->enable(encoder);
> + intel_encoders_enable(crtc, old_state);
> }
>
> static void i9xx_set_pll_dividers(struct intel_crtc *crtc)
> @@ -6198,7 +6286,6 @@ static void i9xx_crtc_enable(struct intel_crtc_state *pipe_config,
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - struct intel_encoder *encoder;
> enum pipe pipe = intel_crtc->pipe;
>
> if (WARN_ON(intel_crtc->active))
> @@ -6219,9 +6306,7 @@ static void i9xx_crtc_enable(struct intel_crtc_state *pipe_config,
> if (!IS_GEN2(dev))
> intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
>
> - for_each_encoder_on_crtc(dev, crtc, encoder)
> - if (encoder->pre_enable)
> - encoder->pre_enable(encoder);
> + intel_encoders_pre_enable(crtc, old_state);
>
> i9xx_enable_pll(intel_crtc);
>
> @@ -6235,8 +6320,7 @@ static void i9xx_crtc_enable(struct intel_crtc_state *pipe_config,
> assert_vblank_disabled(crtc);
> drm_crtc_vblank_on(crtc);
>
> - for_each_encoder_on_crtc(dev, crtc, encoder)
> - encoder->enable(encoder);
> + intel_encoders_enable(crtc, old_state);
> }
>
> static void i9xx_pfit_disable(struct intel_crtc *crtc)
> @@ -6261,7 +6345,6 @@ static void i9xx_crtc_disable(struct intel_crtc_state *old_crtc_state,
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - struct intel_encoder *encoder;
> int pipe = intel_crtc->pipe;
>
> /*
> @@ -6271,8 +6354,7 @@ static void i9xx_crtc_disable(struct intel_crtc_state *old_crtc_state,
> if (IS_GEN2(dev))
> intel_wait_for_vblank(dev, pipe);
>
> - for_each_encoder_on_crtc(dev, crtc, encoder)
> - encoder->disable(encoder);
> + intel_encoders_disable(crtc, old_state);
>
> drm_crtc_vblank_off(crtc);
> assert_vblank_disabled(crtc);
> @@ -6281,9 +6363,7 @@ static void i9xx_crtc_disable(struct intel_crtc_state *old_crtc_state,
>
> i9xx_pfit_disable(intel_crtc);
>
> - for_each_encoder_on_crtc(dev, crtc, encoder)
> - if (encoder->post_disable)
> - encoder->post_disable(encoder);
> + intel_encoders_post_disable(crtc, old_state);
>
> if (!intel_crtc_has_type(intel_crtc->config, INTEL_OUTPUT_DSI)) {
> if (IS_CHERRYVIEW(dev))
> @@ -6294,9 +6374,7 @@ static void i9xx_crtc_disable(struct intel_crtc_state *old_crtc_state,
> i9xx_disable_pll(intel_crtc);
> }
>
> - for_each_encoder_on_crtc(dev, crtc, encoder)
> - if (encoder->post_pll_disable)
> - encoder->post_pll_disable(encoder);
> + intel_encoders_post_pll_disable(crtc, old_state);
>
> if (!IS_GEN2(dev))
> intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
> --
> 2.5.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
