[PATCH v3 00/13] DPMS updates and atomic state checking.

[PATCH v3 00/13] DPMS updates and atomic state checking.

[Maarten Lankhorst]

Mixed bag of fixes for -next now that the first merge happened.
Patch series deals with getting rid of intel DPMS handling and
making the state checker atomic.

The state checker is now atomic and only checks the affected
crtc's, encoders and connectors.

This is just a resend with all the fixes done after v2.

Maarten Lankhorst (13):
  drm/i915: Make the force_thru workaround atomic, v2.
  drm/i915: Validate the state after an atomic modeset only, and pass the state.
  drm/i915: Update atomic state when removing mst connector.
  drm/i915: Convert connector checking to atomic, v2.
  drm/i915: Remove some unneeded checks from check_crtc_state.
  drm/i915: Remove connectors_active from state checking.
  drm/i915: Make crtc checking use the atomic state, v2.
  drm/i915: Get rid of dpms handling.
  drm/i915: Remove connectors_active from sanitization, v2.
  drm/i915: Remove connectors_active from intel_dp.c, v2.
  drm/i915: Remove connectors_active.
  drm/i915: Only update mode related state if a modeset happened.
  drm/i915: Handle return value in intel_pin_and_fence_fb_obj, v2.

 drivers/gpu/drm/i915/i915_debugfs.c  |  82 ++-----
 drivers/gpu/drm/i915/intel_crt.c     |  51 +----
 drivers/gpu/drm/i915/intel_display.c | 426 +++++++++++------------------------
 drivers/gpu/drm/i915/intel_dp.c      |   9 +-
 drivers/gpu/drm/i915/intel_dp_mst.c  |  47 +++-
 drivers/gpu/drm/i915/intel_drv.h     |   5 -
 drivers/gpu/drm/i915/intel_dsi.c     |   2 +-
 drivers/gpu/drm/i915/intel_dvo.c     |  48 +---
 drivers/gpu/drm/i915/intel_hdmi.c    |   2 +-
 drivers/gpu/drm/i915/intel_lvds.c    |   2 +-
 drivers/gpu/drm/i915/intel_sdvo.c    |  49 +---
 drivers/gpu/drm/i915/intel_tv.c      |   2 +-
 12 files changed, 213 insertions(+), 512 deletions(-)

-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 01/13] drm/i915: Make the force_thru workaround atomic, v2.

[Maarten Lankhorst]

Set connectors_changed to force a modeset if the panel fitter's force
enabled on eDP.

Changes since v1:
- Use connectors_changed instead of active_changed because it's a
  routing update.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  | 82 +++++++++++-------------------------
 drivers/gpu/drm/i915/intel_display.c |  3 ++
 2 files changed, 27 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 23a69307e12e..d1c643a82267 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3645,74 +3645,40 @@ static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
 	return 0;
 }
 
-static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
+static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev, bool enable)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *crtc =
 		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_A]);
 	struct intel_crtc_state *pipe_config;
+	struct drm_atomic_state *state;
+	int ret = 0;
 
 	drm_modeset_lock_all(dev);
-	pipe_config = to_intel_crtc_state(crtc->base.state);
-
-	/*
-	 * If we use the eDP transcoder we need to make sure that we don't
-	 * bypass the pfit, since otherwise the pipe CRC source won't work. Only
-	 * relevant on hsw with pipe A when using the always-on power well
-	 * routing.
-	 */
-	if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
-	    !pipe_config->pch_pfit.enabled) {
-		bool active = pipe_config->base.active;
-
-		if (active) {
-			intel_crtc_control(&crtc->base, false);
-			pipe_config = to_intel_crtc_state(crtc->base.state);
-		}
-
-		pipe_config->pch_pfit.force_thru = true;
-
-		intel_display_power_get(dev_priv,
-					POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
-
-		if (active)
-			intel_crtc_control(&crtc->base, true);
+	state = drm_atomic_state_alloc(dev);
+	if (!state) {
+		ret = -ENOMEM;
+		goto out;
 	}
-	drm_modeset_unlock_all(dev);
-}
 
-static void hsw_undo_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *crtc =
-		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_A]);
-	struct intel_crtc_state *pipe_config;
-
-	drm_modeset_lock_all(dev);
-	/*
-	 * If we use the eDP transcoder we need to make sure that we don't
-	 * bypass the pfit, since otherwise the pipe CRC source won't work. Only
-	 * relevant on hsw with pipe A when using the always-on power well
-	 * routing.
-	 */
-	pipe_config = to_intel_crtc_state(crtc->base.state);
-	if (pipe_config->pch_pfit.force_thru) {
-		bool active = pipe_config->base.active;
-
-		if (active) {
-			intel_crtc_control(&crtc->base, false);
-			pipe_config = to_intel_crtc_state(crtc->base.state);
-		}
-
-		pipe_config->pch_pfit.force_thru = false;
+	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(&crtc->base);
+	pipe_config = intel_atomic_get_crtc_state(state, crtc);
+	if (IS_ERR(pipe_config)) {
+		ret = PTR_ERR(pipe_config);
+		goto out;
+	}
 
-		intel_display_power_put(dev_priv,
-					POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
+	pipe_config->pch_pfit.force_thru = enable;
+	if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
+	    pipe_config->pch_pfit.enabled != enable)
+		pipe_config->base.connectors_changed = true;
 
-		if (active)
-			intel_crtc_control(&crtc->base, true);
-	}
+	ret = drm_atomic_commit(state);
+out:
 	drm_modeset_unlock_all(dev);
+	WARN(ret, "Toggling workaround to %i returns %i\n", enable, ret);
+	if (ret)
+		drm_atomic_state_free(state);
 }
 
 static int ivb_pipe_crc_ctl_reg(struct drm_device *dev,
@@ -3732,7 +3698,7 @@ static int ivb_pipe_crc_ctl_reg(struct drm_device *dev,
 		break;
 	case INTEL_PIPE_CRC_SOURCE_PF:
 		if (IS_HASWELL(dev) && pipe == PIPE_A)
-			hsw_trans_edp_pipe_A_crc_wa(dev);
+			hsw_trans_edp_pipe_A_crc_wa(dev, true);
 
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
 		break;
@@ -3844,7 +3810,7 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 		else if (IS_VALLEYVIEW(dev))
 			vlv_undo_pipe_scramble_reset(dev, pipe);
 		else if (IS_HASWELL(dev) && pipe == PIPE_A)
-			hsw_undo_trans_edp_pipe_A_crc_wa(dev);
+			hsw_trans_edp_pipe_A_crc_wa(dev, false);
 
 		hsw_enable_ips(crtc);
 	}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 43b0f17ad1fa..7747520bf9f6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12160,6 +12160,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	struct intel_dpll_hw_state dpll_hw_state;
 	enum intel_dpll_id shared_dpll;
 	uint32_t ddi_pll_sel;
+	bool force_thru;
 
 	/* FIXME: before the switch to atomic started, a new pipe_config was
 	 * kzalloc'd. Code that depends on any field being zero should be
@@ -12171,6 +12172,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	shared_dpll = crtc_state->shared_dpll;
 	dpll_hw_state = crtc_state->dpll_hw_state;
 	ddi_pll_sel = crtc_state->ddi_pll_sel;
+	force_thru = crtc_state->pch_pfit.force_thru;
 
 	memset(crtc_state, 0, sizeof *crtc_state);
 
@@ -12179,6 +12181,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	crtc_state->shared_dpll = shared_dpll;
 	crtc_state->dpll_hw_state = dpll_hw_state;
 	crtc_state->ddi_pll_sel = ddi_pll_sel;
+	crtc_state->pch_pfit.force_thru = force_thru;
 }
 
 static int
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 01/13] drm/i915: Make the force_thru workaround atomic, v2.

[Daniel Vetter]

On Wed, Aug 05, 2015 at 12:36:59PM +0200, Maarten Lankhorst wrote:
> Set connectors_changed to force a modeset if the panel fitter's force
> enabled on eDP.
> 
> Changes since v1:
> - Use connectors_changed instead of active_changed because it's a
>   routing update.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Queued for -next, thanks for the patch. I can't merge more since I need to
backmerge topic/drm-misc first and that still suffers from arm driver
breakage.
-Daniel
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  | 82 +++++++++++-------------------------
>  drivers/gpu/drm/i915/intel_display.c |  3 ++
>  2 files changed, 27 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 23a69307e12e..d1c643a82267 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3645,74 +3645,40 @@ static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
>  	return 0;
>  }
>  
> -static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
> +static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev, bool enable)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *crtc =
>  		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_A]);
>  	struct intel_crtc_state *pipe_config;
> +	struct drm_atomic_state *state;
> +	int ret = 0;
>  
>  	drm_modeset_lock_all(dev);
> -	pipe_config = to_intel_crtc_state(crtc->base.state);
> -
> -	/*
> -	 * If we use the eDP transcoder we need to make sure that we don't
> -	 * bypass the pfit, since otherwise the pipe CRC source won't work. Only
> -	 * relevant on hsw with pipe A when using the always-on power well
> -	 * routing.
> -	 */
> -	if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> -	    !pipe_config->pch_pfit.enabled) {
> -		bool active = pipe_config->base.active;
> -
> -		if (active) {
> -			intel_crtc_control(&crtc->base, false);
> -			pipe_config = to_intel_crtc_state(crtc->base.state);
> -		}
> -
> -		pipe_config->pch_pfit.force_thru = true;
> -
> -		intel_display_power_get(dev_priv,
> -					POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
> -
> -		if (active)
> -			intel_crtc_control(&crtc->base, true);
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state) {
> +		ret = -ENOMEM;
> +		goto out;
>  	}
> -	drm_modeset_unlock_all(dev);
> -}
>  
> -static void hsw_undo_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
> -{
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crtc *crtc =
> -		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_A]);
> -	struct intel_crtc_state *pipe_config;
> -
> -	drm_modeset_lock_all(dev);
> -	/*
> -	 * If we use the eDP transcoder we need to make sure that we don't
> -	 * bypass the pfit, since otherwise the pipe CRC source won't work. Only
> -	 * relevant on hsw with pipe A when using the always-on power well
> -	 * routing.
> -	 */
> -	pipe_config = to_intel_crtc_state(crtc->base.state);
> -	if (pipe_config->pch_pfit.force_thru) {
> -		bool active = pipe_config->base.active;
> -
> -		if (active) {
> -			intel_crtc_control(&crtc->base, false);
> -			pipe_config = to_intel_crtc_state(crtc->base.state);
> -		}
> -
> -		pipe_config->pch_pfit.force_thru = false;
> +	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(&crtc->base);
> +	pipe_config = intel_atomic_get_crtc_state(state, crtc);
> +	if (IS_ERR(pipe_config)) {
> +		ret = PTR_ERR(pipe_config);
> +		goto out;
> +	}
>  
> -		intel_display_power_put(dev_priv,
> -					POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
> +	pipe_config->pch_pfit.force_thru = enable;
> +	if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> +	    pipe_config->pch_pfit.enabled != enable)
> +		pipe_config->base.connectors_changed = true;
>  
> -		if (active)
> -			intel_crtc_control(&crtc->base, true);
> -	}
> +	ret = drm_atomic_commit(state);
> +out:
>  	drm_modeset_unlock_all(dev);
> +	WARN(ret, "Toggling workaround to %i returns %i\n", enable, ret);
> +	if (ret)
> +		drm_atomic_state_free(state);
>  }
>  
>  static int ivb_pipe_crc_ctl_reg(struct drm_device *dev,
> @@ -3732,7 +3698,7 @@ static int ivb_pipe_crc_ctl_reg(struct drm_device *dev,
>  		break;
>  	case INTEL_PIPE_CRC_SOURCE_PF:
>  		if (IS_HASWELL(dev) && pipe == PIPE_A)
> -			hsw_trans_edp_pipe_A_crc_wa(dev);
> +			hsw_trans_edp_pipe_A_crc_wa(dev, true);
>  
>  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
>  		break;
> @@ -3844,7 +3810,7 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
>  		else if (IS_VALLEYVIEW(dev))
>  			vlv_undo_pipe_scramble_reset(dev, pipe);
>  		else if (IS_HASWELL(dev) && pipe == PIPE_A)
> -			hsw_undo_trans_edp_pipe_A_crc_wa(dev);
> +			hsw_trans_edp_pipe_A_crc_wa(dev, false);
>  
>  		hsw_enable_ips(crtc);
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 43b0f17ad1fa..7747520bf9f6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12160,6 +12160,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>  	struct intel_dpll_hw_state dpll_hw_state;
>  	enum intel_dpll_id shared_dpll;
>  	uint32_t ddi_pll_sel;
> +	bool force_thru;
>  
>  	/* FIXME: before the switch to atomic started, a new pipe_config was
>  	 * kzalloc'd. Code that depends on any field being zero should be
> @@ -12171,6 +12172,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>  	shared_dpll = crtc_state->shared_dpll;
>  	dpll_hw_state = crtc_state->dpll_hw_state;
>  	ddi_pll_sel = crtc_state->ddi_pll_sel;
> +	force_thru = crtc_state->pch_pfit.force_thru;
>  
>  	memset(crtc_state, 0, sizeof *crtc_state);
>  
> @@ -12179,6 +12181,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>  	crtc_state->shared_dpll = shared_dpll;
>  	crtc_state->dpll_hw_state = dpll_hw_state;
>  	crtc_state->ddi_pll_sel = ddi_pll_sel;
> +	crtc_state->pch_pfit.force_thru = force_thru;
>  }
>  
>  static int
> -- 
> 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

[PATCH v3 02/13] drm/i915: Validate the state after an atomic modeset only, and pass the state.

[Maarten Lankhorst]

First step in removing dpms and validating atomic state.

There can still be a mismatch in the connector state because the dpms
callbacks are still used, but this can not happen immediately after a modeset.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
---
 drivers/gpu/drm/i915/intel_crt.c     |  2 --
 drivers/gpu/drm/i915/intel_display.c | 12 ++++++------
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 drivers/gpu/drm/i915/intel_dvo.c     |  2 --
 drivers/gpu/drm/i915/intel_sdvo.c    |  2 --
 5 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 5d78c1feec81..9eba3dd5b434 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -280,8 +280,6 @@ static int intel_crt_dpms(struct drm_connector *connector, int mode)
 		intel_crtc_update_dpms(crtc);
 	}
 
-	intel_modeset_check_state(connector->dev);
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index dfe4407b0390..77b4da7e698c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6440,8 +6440,6 @@ int intel_connector_dpms(struct drm_connector *connector, int mode)
 	if (connector->encoder)
 		intel_encoder_dpms(to_intel_encoder(connector->encoder), mode);
 
-	intel_modeset_check_state(connector->dev);
-
 	return 0;
 }
 
@@ -12900,8 +12898,9 @@ check_shared_dpll_state(struct drm_device *dev)
 	}
 }
 
-void
-intel_modeset_check_state(struct drm_device *dev)
+static void
+intel_modeset_check_state(struct drm_device *dev,
+			  struct drm_atomic_state *old_state)
 {
 	check_wm_state(dev);
 	check_connector_state(dev);
@@ -13290,10 +13289,11 @@ static int intel_atomic_commit(struct drm_device *dev,
 
 	drm_atomic_helper_wait_for_vblanks(dev, state);
 	drm_atomic_helper_cleanup_planes(dev, state);
-	drm_atomic_state_free(state);
 
 	if (any_ms)
-		intel_modeset_check_state(dev);
+		intel_modeset_check_state(dev, state);
+
+	drm_atomic_state_free(state);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 320c9e6bd848..0da4236dc85a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -999,7 +999,6 @@ int intel_connector_init(struct intel_connector *);
 struct intel_connector *intel_connector_alloc(void);
 int intel_connector_dpms(struct drm_connector *, int mode);
 bool intel_connector_get_hw_state(struct intel_connector *connector);
-void intel_modeset_check_state(struct drm_device *dev);
 bool ibx_digital_port_connected(struct drm_i915_private *dev_priv,
 				struct intel_digital_port *port);
 void intel_connector_attach_encoder(struct intel_connector *connector,
diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
index fd5e522abebb..600f7fb855d8 100644
--- a/drivers/gpu/drm/i915/intel_dvo.c
+++ b/drivers/gpu/drm/i915/intel_dvo.c
@@ -237,8 +237,6 @@ static int intel_dvo_dpms(struct drm_connector *connector, int mode)
 		intel_crtc_update_dpms(crtc);
 	}
 
-	intel_modeset_check_state(connector->dev);
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 2c435a79d4da..8911e0e417ee 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1550,8 +1550,6 @@ static int intel_sdvo_dpms(struct drm_connector *connector, int mode)
 		intel_sdvo_set_active_outputs(intel_sdvo, intel_sdvo->attached_output);
 	}
 
-	intel_modeset_check_state(connector->dev);
-
 	return 0;
 }
 
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 03/13] drm/i915: Update atomic state when removing mst connector.

[Maarten Lankhorst]

Fully remove the MST connector from the atomic state, and remove the
early returns in check_*_state for MST connectors.

With atomic the state can be made consistent all the time.

Changes since v1:
- Remove the MST check in intel_connector_check_state too.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
---
 drivers/gpu/drm/i915/intel_display.c | 11 ---------
 drivers/gpu/drm/i915/intel_dp_mst.c  | 45 +++++++++++++++++++++++++++++++++++-
 2 files changed, 44 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7747520bf9f6..dfe4407b0390 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6370,10 +6370,6 @@ static void intel_connector_check_state(struct intel_connector *connector)
 			      connector->base.base.id,
 			      connector->base.name);
 
-		/* there is no real hw state for MST connectors */
-		if (connector->mst_port)
-			return;
-
 		I915_STATE_WARN(connector->base.dpms == DRM_MODE_DPMS_OFF,
 		     "wrong connector dpms state\n");
 		I915_STATE_WARN(connector->base.encoder != &encoder->base,
@@ -12751,13 +12747,6 @@ check_encoder_state(struct drm_device *dev)
 					encoder->base.crtc,
 			     "connector's crtc doesn't match encoder crtc\n");
 		}
-		/*
-		 * for MST connectors if we unplug the connector is gone
-		 * away but the encoder is still connected to a crtc
-		 * until a modeset happens in response to the hotplug.
-		 */
-		if (!enabled && encoder->base.encoder_type == DRM_MODE_ENCODER_DPMST)
-			continue;
 
 		I915_STATE_WARN(!!encoder->base.crtc != enabled,
 		     "encoder's enabled state mismatch "
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index e3a5864160fa..36bd9394f789 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -459,6 +459,49 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 	return connector;
 }
 
+static void
+intel_dp_unbind_mst_connector(struct drm_device *dev,
+			      struct drm_connector *connector)
+{
+	struct drm_atomic_state *state;
+	struct drm_connector_state *conn_state;
+	struct drm_crtc *crtc = connector->state->crtc;
+	int ret;
+
+	if (!crtc)
+		return;
+
+	state = drm_atomic_state_alloc(dev);
+	if (!state)
+		return;
+
+	state->acquire_ctx = dev->mode_config.acquire_ctx;
+
+	conn_state = drm_atomic_get_connector_state(state, connector);
+	ret = PTR_ERR_OR_ZERO(conn_state);
+	if (!ret)
+		ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
+
+	if (!ret)
+		ret = drm_atomic_add_affected_connectors(state, crtc);
+
+	if (!ret && !drm_atomic_connectors_for_crtc(state, crtc)) {
+		struct drm_crtc_state *crtc_state =
+			drm_atomic_get_existing_crtc_state(state, crtc);
+
+		crtc_state->active = false;
+		ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
+	}
+
+	if (!ret)
+		ret = drm_atomic_commit(state);
+
+	if (ret)
+		drm_atomic_state_free(state);
+
+	I915_STATE_WARN_ON(ret);
+}
+
 static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 					   struct drm_connector *connector)
 {
@@ -466,7 +509,7 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 	struct drm_device *dev = connector->dev;
 	/* need to nuke the connector */
 	drm_modeset_lock_all(dev);
-	intel_connector_dpms(connector, DRM_MODE_DPMS_OFF);
+	intel_dp_unbind_mst_connector(dev, connector);
 	drm_modeset_unlock_all(dev);
 
 	intel_connector->unregister(intel_connector);
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3.1 1/3] drm/i915: Fix broken mst get_hw_state.

[Maarten Lankhorst]

This function always returned false because intel_connector->encoder
is always NULL. Instead use the attached encoder from atomic.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index e3a5864160fa..ff01569158ea 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -395,9 +395,12 @@ static const struct drm_encoder_funcs intel_dp_mst_enc_funcs = {
 
 static bool intel_dp_mst_get_hw_state(struct intel_connector *connector)
 {
-	if (connector->encoder) {
+	struct intel_encoder *encoder;
+
+	encoder = to_intel_encoder(connector->base.state->best_encoder);
+	if (encoder) {
 		enum pipe pipe;
-		if (!connector->encoder->get_hw_state(connector->encoder, &pipe))
+		if (!encoder->get_hw_state(encoder, &pipe))
 			return false;
 		return true;
 	}
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3.1 2/3] drm/i915: Update atomic state when removing mst connector, v3.

[Maarten Lankhorst]

Fully remove the MST connector from the atomic state, and remove the
early returns in check_*_state for MST connectors.

With atomic the state can be made consistent all the time.

Thanks to Sivakumar Thulasimani for the idea of using
drm_atomic_helper_set_config.

Changes since v1:
- Remove the MST check in intel_connector_check_state too.
Changes since v2:
- Use drm_atomic_helper_set_config.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 11 -----------
 drivers/gpu/drm/i915/intel_dp_mst.c  | 13 ++++++++++++-
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5e40b7e7013a..77b4da7e698c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6370,10 +6370,6 @@ static void intel_connector_check_state(struct intel_connector *connector)
 			      connector->base.base.id,
 			      connector->base.name);
 
-		/* there is no real hw state for MST connectors */
-		if (connector->mst_port)
-			return;
-
 		I915_STATE_WARN(connector->base.dpms == DRM_MODE_DPMS_OFF,
 		     "wrong connector dpms state\n");
 		I915_STATE_WARN(connector->base.encoder != &encoder->base,
@@ -12749,13 +12745,6 @@ check_encoder_state(struct drm_device *dev)
 					encoder->base.crtc,
 			     "connector's crtc doesn't match encoder crtc\n");
 		}
-		/*
-		 * for MST connectors if we unplug the connector is gone
-		 * away but the encoder is still connected to a crtc
-		 * until a modeset happens in response to the hotplug.
-		 */
-		if (!enabled && encoder->base.encoder_type == DRM_MODE_ENCODER_DPMST)
-			continue;
 
 		I915_STATE_WARN(!!encoder->base.crtc != enabled,
 		     "encoder's enabled state mismatch "
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index ff01569158ea..91ad17110c2f 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -467,9 +467,20 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 {
 	struct intel_connector *intel_connector = to_intel_connector(connector);
 	struct drm_device *dev = connector->dev;
+
 	/* need to nuke the connector */
 	drm_modeset_lock_all(dev);
-	intel_connector_dpms(connector, DRM_MODE_DPMS_OFF);
+	if (connector->state->crtc) {
+		struct drm_mode_set set;
+		int ret;
+
+		memset(&set, 0, sizeof(set));
+		set.crtc = connector->state->crtc,
+
+		ret = drm_atomic_helper_set_config(&set);
+
+		WARN(ret, "Disabling mst crtc failed with %i\n", ret);
+	}
 	drm_modeset_unlock_all(dev);
 
 	intel_connector->unregister(intel_connector);
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3.1 2/3] drm/i915: Update atomic state when removing mst connector, v3.

[Sivakumar Thulasimani]

[-- Attachment #1.1: Type: text/plain, Size: 3098 bytes --]

thanks for the change :)


Reviewed-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>


On 8/6/2015 5:17 PM, Maarten Lankhorst wrote:
> Fully remove the MST connector from the atomic state, and remove the
> early returns in check_*_state for MST connectors.
>
> With atomic the state can be made consistent all the time.
>
> Thanks to Sivakumar Thulasimani for the idea of using
> drm_atomic_helper_set_config.
>
> Changes since v1:
> - Remove the MST check in intel_connector_check_state too.
> Changes since v2:
> - Use drm_atomic_helper_set_config.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 11 -----------
>   drivers/gpu/drm/i915/intel_dp_mst.c  | 13 ++++++++++++-
>   2 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5e40b7e7013a..77b4da7e698c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6370,10 +6370,6 @@ static void intel_connector_check_state(struct intel_connector *connector)
>   			      connector->base.base.id,
>   			      connector->base.name);
>   
> -		/* there is no real hw state for MST connectors */
> -		if (connector->mst_port)
> -			return;
> -
>   		I915_STATE_WARN(connector->base.dpms == DRM_MODE_DPMS_OFF,
>   		     "wrong connector dpms state\n");
>   		I915_STATE_WARN(connector->base.encoder != &encoder->base,
> @@ -12749,13 +12745,6 @@ check_encoder_state(struct drm_device *dev)
>   					encoder->base.crtc,
>   			     "connector's crtc doesn't match encoder crtc\n");
>   		}
> -		/*
> -		 * for MST connectors if we unplug the connector is gone
> -		 * away but the encoder is still connected to a crtc
> -		 * until a modeset happens in response to the hotplug.
> -		 */
> -		if (!enabled && encoder->base.encoder_type == DRM_MODE_ENCODER_DPMST)
> -			continue;
>   
>   		I915_STATE_WARN(!!encoder->base.crtc != enabled,
>   		     "encoder's enabled state mismatch "
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index ff01569158ea..91ad17110c2f 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -467,9 +467,20 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
>   {
>   	struct intel_connector *intel_connector = to_intel_connector(connector);
>   	struct drm_device *dev = connector->dev;
> +
>   	/* need to nuke the connector */
>   	drm_modeset_lock_all(dev);
> -	intel_connector_dpms(connector, DRM_MODE_DPMS_OFF);
> +	if (connector->state->crtc) {
> +		struct drm_mode_set set;
> +		int ret;
> +
> +		memset(&set, 0, sizeof(set));
> +		set.crtc = connector->state->crtc,
> +
> +		ret = drm_atomic_helper_set_config(&set);
> +
> +		WARN(ret, "Disabling mst crtc failed with %i\n", ret);
> +	}
>   	drm_modeset_unlock_all(dev);
>   
>   	intel_connector->unregister(intel_connector);

-- 
regards,
Sivakumar


[-- Attachment #1.2: Type: text/html, Size: 4065 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3.1 3/3] drm/i915: Don't try to remove MST cleanly when force removed.

[Maarten Lankhorst]

Physically disconnecting a DP connector with an active MST stream
can lead to a kernel panic in intel_mst_disable_dp when calling
drm_dp_update_payload_part1. Examining the code it seems that the
port is freed while work to remove the connector is scheduled.

This probably means it's fine to skip call all the mst helper calls
and only attempt to disable the real encoder.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 91ad17110c2f..19c9e49d74e0 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -110,6 +110,9 @@ static void intel_mst_disable_dp(struct intel_encoder *encoder)
 
 	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
 
+	if (!intel_mst->port)
+		return;
+
 	drm_dp_mst_reset_vcpi_slots(&intel_dp->mst_mgr, intel_mst->port);
 
 	ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr);
@@ -126,12 +129,14 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder)
 
 	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
 
-	/* this can fail */
-	drm_dp_check_act_status(&intel_dp->mst_mgr);
-	/* and this can also fail */
-	drm_dp_update_payload_part2(&intel_dp->mst_mgr);
+	if (intel_mst->port) {
+		/* this can fail */
+		drm_dp_check_act_status(&intel_dp->mst_mgr);
+		/* and this can also fail */
+		drm_dp_update_payload_part2(&intel_dp->mst_mgr);
 
-	drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, intel_mst->port);
+		drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, intel_mst->port);
+	}
 
 	intel_dp->active_mst_links--;
 	intel_mst->port = NULL;
@@ -471,9 +476,13 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 	/* need to nuke the connector */
 	drm_modeset_lock_all(dev);
 	if (connector->state->crtc) {
+		struct drm_encoder *encoder = connector->state->best_encoder;
+		struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder);
 		struct drm_mode_set set;
 		int ret;
 
+		intel_mst->port = NULL;
+
 		memset(&set, 0, sizeof(set));
 		set.crtc = connector->state->crtc,
 
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3.1 3/3] drm/i915: Don't try to remove MST cleanly when force removed.

[Daniel Vetter]

On Thu, Aug 06, 2015 at 01:47:37PM +0200, Maarten Lankhorst wrote:
> Physically disconnecting a DP connector with an active MST stream
> can lead to a kernel panic in intel_mst_disable_dp when calling
> drm_dp_update_payload_part1. Examining the code it seems that the
> port is freed while work to remove the connector is scheduled.
> 
> This probably means it's fine to skip call all the mst helper calls
> and only attempt to disable the real encoder.

I think this is a refcounting bug in the dp mst helpers, the port
shouldn't just go poof when we still hold a reference to it. Can you
please try to figure out what's really broken here, and if that's too
tricky add a big FIXME comment?

Thanks, Daniel

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 91ad17110c2f..19c9e49d74e0 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -110,6 +110,9 @@ static void intel_mst_disable_dp(struct intel_encoder *encoder)
>  
>  	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
>  
> +	if (!intel_mst->port)
> +		return;
> +
>  	drm_dp_mst_reset_vcpi_slots(&intel_dp->mst_mgr, intel_mst->port);
>  
>  	ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr);
> @@ -126,12 +129,14 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder)
>  
>  	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
>  
> -	/* this can fail */
> -	drm_dp_check_act_status(&intel_dp->mst_mgr);
> -	/* and this can also fail */
> -	drm_dp_update_payload_part2(&intel_dp->mst_mgr);
> +	if (intel_mst->port) {
> +		/* this can fail */
> +		drm_dp_check_act_status(&intel_dp->mst_mgr);
> +		/* and this can also fail */
> +		drm_dp_update_payload_part2(&intel_dp->mst_mgr);
>  
> -	drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, intel_mst->port);
> +		drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, intel_mst->port);
> +	}
>  
>  	intel_dp->active_mst_links--;
>  	intel_mst->port = NULL;
> @@ -471,9 +476,13 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
>  	/* need to nuke the connector */
>  	drm_modeset_lock_all(dev);
>  	if (connector->state->crtc) {
> +		struct drm_encoder *encoder = connector->state->best_encoder;
> +		struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder);
>  		struct drm_mode_set set;
>  		int ret;
>  
> +		intel_mst->port = NULL;
> +
>  		memset(&set, 0, sizeof(set));
>  		set.crtc = connector->state->crtc,
>  
> -- 
> 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

Re: [PATCH v3.1 3/3] drm/i915: Don't try to remove MST cleanly when force removed.

[Maarten Lankhorst]

Hey,

Op 06-08-15 om 15:01 schreef Daniel Vetter:
> On Thu, Aug 06, 2015 at 01:47:37PM +0200, Maarten Lankhorst wrote:
>> Physically disconnecting a DP connector with an active MST stream
>> can lead to a kernel panic in intel_mst_disable_dp when calling
>> drm_dp_update_payload_part1. Examining the code it seems that the
>> port is freed while work to remove the connector is scheduled.
>>
>> This probably means it's fine to skip call all the mst helper calls
>> and only attempt to disable the real encoder.
> I think this is a refcounting bug in the dp mst helpers, the port
> shouldn't just go poof when we still hold a reference to it. Can you
> please try to figure out what's really broken here, and if that's too
> tricky add a big FIXME comment?
Look at drm_dp_destroy_port, it calls schedule_work(&mgr->destroy_connector_work), and also calls kfree(port).

Doesn't look like a refcounting bug to me, more like something intentional.

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3.1 3/3] drm/i915: Don't try to remove MST cleanly when force removed.

[Daniel Vetter]

On Thu, Aug 06, 2015 at 03:51:31PM +0200, Maarten Lankhorst wrote:
> Hey,
> 
> Op 06-08-15 om 15:01 schreef Daniel Vetter:
> > On Thu, Aug 06, 2015 at 01:47:37PM +0200, Maarten Lankhorst wrote:
> >> Physically disconnecting a DP connector with an active MST stream
> >> can lead to a kernel panic in intel_mst_disable_dp when calling
> >> drm_dp_update_payload_part1. Examining the code it seems that the
> >> port is freed while work to remove the connector is scheduled.
> >>
> >> This probably means it's fine to skip call all the mst helper calls
> >> and only attempt to disable the real encoder.
> > I think this is a refcounting bug in the dp mst helpers, the port
> > shouldn't just go poof when we still hold a reference to it. Can you
> > please try to figure out what's really broken here, and if that's too
> > tricky add a big FIXME comment?
> Look at drm_dp_destroy_port, it calls schedule_work(&mgr->destroy_connector_work), and also calls kfree(port).
> 
> Doesn't look like a refcounting bug to me, more like something intentional.

Intentionally buggy is still buggy ;-)

And yeah we probably need to separate the port cleanup due to unplug (i.e.
the driver callback) from freeing the port. And the intel_connector should
hold a reference onto the underlying port I think. Of course it would be
best to do the get/put on the port in the mst helper itself (so that we
don't need to change i915/radeon).
-Daniel
-- 
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

Re: [PATCH v3.1 1/3] drm/i915: Fix broken mst get_hw_state.

[Daniel Vetter]

On Thu, Aug 06, 2015 at 01:47:35PM +0200, Maarten Lankhorst wrote:
> This function always returned false because intel_connector->encoder
> is always NULL. Instead use the attached encoder from atomic.

Note that you've broken this since you removed the updating of
intel_connector->encoder somewhere in the 4.3 atomic series. So this is a
regression fix. Can you please digg out the right commit?

> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index e3a5864160fa..ff01569158ea 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -395,9 +395,12 @@ static const struct drm_encoder_funcs intel_dp_mst_enc_funcs = {
>  
>  static bool intel_dp_mst_get_hw_state(struct intel_connector *connector)
>  {
> -	if (connector->encoder) {
> +	struct intel_encoder *encoder;
> +
> +	encoder = to_intel_encoder(connector->base.state->best_encoder);

Strictly speaking this won't work for hw state readout since what we
really need to do is ask the mst connector which mst stream it accepts and
then compare that with all the mst encoders ...

But that's been broken since forever. Only side-effect here is that
fastboot won't work with mst and that's imo totally ok. Please also add
this to your commit message.
-Daniel
> +	if (encoder) {
>  		enum pipe pipe;
> -		if (!connector->encoder->get_hw_state(connector->encoder, &pipe))
> +		if (!encoder->get_hw_state(encoder, &pipe))
>  			return false;
>  		return true;
>  	}
> -- 
> 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

Re: [PATCH v3.1 1/3] drm/i915: Fix broken mst get_hw_state.

[Maarten Lankhorst]

Op 06-08-15 om 14:59 schreef Daniel Vetter:
> On Thu, Aug 06, 2015 at 01:47:35PM +0200, Maarten Lankhorst wrote:
>> This function always returned false because intel_connector->encoder
>> is always NULL. Instead use the attached encoder from atomic.
> Note that you've broken this since you removed the updating of
> intel_connector->encoder somewhere in the 4.3 atomic series. So this is a
> regression fix. Can you please digg out the right commit?

Looks like 'Use full atomic modeset' broke it. intel_connector->encoder was updated in its set_config function..

Would below amend for this commit be good?
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index ee2a77144373..671715ba8538 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -173,6 +173,11 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder)
 		return;
 	}
 
+	/* MST encoders are bound to a crtc, not to a connector,
+	 * force the mapping here for get_hw_state.
+	 */
+	found->encoder = encoder;
+
 	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
 	intel_mst->port = found->port;
 

And corresponding amendment to the convert connector to atomic..

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a6617283b1bd..90caa62cf2c3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6313,7 +6313,7 @@ static void intel_connector_check_state(struct intel_connector *connector)
 		I915_STATE_WARN(!crtc->state->active,
 		      "connector is active, but attached crtc isn't\n");
 
-		if (!encoder)
+		if (!encoder || encoder->type == INTEL_OUTPUT_DP_MST)
 			return;
 
 		I915_STATE_WARN(conn_state->best_encoder != encoder,

>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dp_mst.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
>> index e3a5864160fa..ff01569158ea 100644
>> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
>> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
>> @@ -395,9 +395,12 @@ static const struct drm_encoder_funcs intel_dp_mst_enc_funcs = {
>>  
>>  static bool intel_dp_mst_get_hw_state(struct intel_connector *connector)
>>  {
>> -	if (connector->encoder) {
>> +	struct intel_encoder *encoder;
>> +
>> +	encoder = to_intel_encoder(connector->base.state->best_encoder);
> Strictly speaking this won't work for hw state readout since what we
> really need to do is ask the mst connector which mst stream it accepts and
> then compare that with all the mst encoders ...
It's good enough to make sure the encoder callbacks did run.
> But that's been broken since forever. Only side-effect here is that
> fastboot won't work with mst and that's imo totally ok. Please also add
> this to your commit message.
HW readout runs before mst probing, so you wouldn't get the mst connectors anyway with fastboot. Not sure it needs a fixme. :-)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3.1 1/3] drm/i915: Fix broken mst get_hw_state.

[Daniel Vetter]

On Thu, Aug 06, 2015 at 03:37:08PM +0200, Maarten Lankhorst wrote:
> Op 06-08-15 om 14:59 schreef Daniel Vetter:
> > On Thu, Aug 06, 2015 at 01:47:35PM +0200, Maarten Lankhorst wrote:
> >> This function always returned false because intel_connector->encoder
> >> is always NULL. Instead use the attached encoder from atomic.
> > Note that you've broken this since you removed the updating of
> > intel_connector->encoder somewhere in the 4.3 atomic series. So this is a
> > regression fix. Can you please digg out the right commit?
> 
> Looks like 'Use full atomic modeset' broke it. intel_connector->encoder was updated in its set_config function..
> 
> Would below amend for this commit be good?
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index ee2a77144373..671715ba8538 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -173,6 +173,11 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder)
>  		return;
>  	}
>  
> +	/* MST encoders are bound to a crtc, not to a connector,
> +	 * force the mapping here for get_hw_state.
> +	 */
> +	found->encoder = encoder;
> +
>  	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
>  	intel_mst->port = found->port;
>  
> 
> And corresponding amendment to the convert connector to atomic..
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a6617283b1bd..90caa62cf2c3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6313,7 +6313,7 @@ static void intel_connector_check_state(struct intel_connector *connector)
>  		I915_STATE_WARN(!crtc->state->active,
>  		      "connector is active, but attached crtc isn't\n");
>  
> -		if (!encoder)
> +		if (!encoder || encoder->type == INTEL_OUTPUT_DP_MST)
>  			return;

Nah, I don't want to keep using the legacy stuff, your patch is good imo.
What I wanted is sha1 of the patches which broke the old
intel_connector->encoder trick.

And of course we get to chase around the code for all the other users of
intel_connector->encoder.
-Daniel

>  
>  		I915_STATE_WARN(conn_state->best_encoder != encoder,
> 
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_dp_mst.c | 7 +++++--
> >>  1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> >> index e3a5864160fa..ff01569158ea 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> >> @@ -395,9 +395,12 @@ static const struct drm_encoder_funcs intel_dp_mst_enc_funcs = {
> >>  
> >>  static bool intel_dp_mst_get_hw_state(struct intel_connector *connector)
> >>  {
> >> -	if (connector->encoder) {
> >> +	struct intel_encoder *encoder;
> >> +
> >> +	encoder = to_intel_encoder(connector->base.state->best_encoder);
> > Strictly speaking this won't work for hw state readout since what we
> > really need to do is ask the mst connector which mst stream it accepts and
> > then compare that with all the mst encoders ...
> It's good enough to make sure the encoder callbacks did run.
> > But that's been broken since forever. Only side-effect here is that
> > fastboot won't work with mst and that's imo totally ok. Please also add
> > this to your commit message.
> HW readout runs before mst probing, so you wouldn't get the mst connectors anyway with fastboot. Not sure it needs a fixme. :-)

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

[PATCH v3 04/13] drm/i915: Convert connector checking to atomic, v2.

[Maarten Lankhorst]

Right now dpms callbacks can still fiddle with the connector state,
but it can only turn connectors off.

This is remediated by only checking crtc->state->active when the
connector is active, and ignore crtc->state->active when the
connector is off.

connectors_active is no longer checked, and will be removed later
in this series together with dpms.

Another check for !encoder->crtc is performed by check_encoder_state
too, so it can be removed.

Changes since v1:
- Add commit message.
- rename state to old_state.
- Move deletion of mst_port check to mst patch.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
---
 drivers/gpu/drm/i915/intel_display.c | 66 +++++++++++++++++-------------------
 1 file changed, 32 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 77b4da7e698c..876071ad4681 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6360,38 +6360,33 @@ static void intel_encoder_dpms(struct intel_encoder *encoder, int mode)
  * internal consistency). */
 static void intel_connector_check_state(struct intel_connector *connector)
 {
-	if (connector->get_hw_state(connector)) {
-		struct intel_encoder *encoder = connector->encoder;
-		struct drm_crtc *crtc;
-		bool encoder_enabled;
-		enum pipe pipe;
+	struct drm_crtc *crtc = connector->base.state->crtc;
 
-		DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
-			      connector->base.base.id,
-			      connector->base.name);
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
+		      connector->base.base.id,
+		      connector->base.name);
 
-		I915_STATE_WARN(connector->base.dpms == DRM_MODE_DPMS_OFF,
-		     "wrong connector dpms state\n");
-		I915_STATE_WARN(connector->base.encoder != &encoder->base,
-		     "active connector not linked to encoder\n");
+	if (connector->get_hw_state(connector)) {
+		struct drm_encoder *encoder = &connector->encoder->base;
+		struct drm_connector_state *conn_state = connector->base.state;
 
-		if (encoder) {
-			I915_STATE_WARN(!encoder->connectors_active,
-			     "encoder->connectors_active not set\n");
+		I915_STATE_WARN(!crtc,
+			 "connector enabled without attached crtc\n");
 
-			encoder_enabled = encoder->get_hw_state(encoder, &pipe);
-			I915_STATE_WARN(!encoder_enabled, "encoder not enabled\n");
-			if (I915_STATE_WARN_ON(!encoder->base.crtc))
-				return;
+		if (!crtc)
+			return;
 
-			crtc = encoder->base.crtc;
+		I915_STATE_WARN(!crtc->state->active,
+		      "connector is active, but attached crtc isn't\n");
 
-			I915_STATE_WARN(!crtc->state->enable,
-					"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,
-			     "encoder active on the wrong pipe\n");
-		}
+		I915_STATE_WARN(conn_state->best_encoder != encoder,
+			"atomic encoder doesn't match attached encoder\n");
+
+		I915_STATE_WARN(conn_state->crtc != encoder->crtc,
+			"attached encoder crtc differs from connector crtc\n");
+	} else {
+		I915_STATE_WARN(!crtc && connector->base.state->best_encoder,
+			"best encoder set without crtc!\n");
 	}
 }
 
@@ -12699,20 +12694,23 @@ static void check_wm_state(struct drm_device *dev)
 }
 
 static void
-check_connector_state(struct drm_device *dev)
+check_connector_state(struct drm_device *dev,
+		      struct drm_atomic_state *old_state)
 {
-	struct intel_connector *connector;
+	struct drm_connector_state *old_conn_state;
+	struct drm_connector *connector;
+	int i;
 
-	for_each_intel_connector(dev, connector) {
-		struct drm_encoder *encoder = connector->base.encoder;
-		struct drm_connector_state *state = connector->base.state;
+	for_each_connector_in_state(old_state, connector, old_conn_state, i) {
+		struct drm_encoder *encoder = connector->encoder;
+		struct drm_connector_state *state = connector->state;
 
 		/* This also checks the encoder/connector hw state with the
 		 * ->get_hw_state callbacks. */
-		intel_connector_check_state(connector);
+		intel_connector_check_state(to_intel_connector(connector));
 
 		I915_STATE_WARN(state->best_encoder != encoder,
-		     "connector's staged encoder doesn't match current encoder\n");
+		     "connector's atomic encoder doesn't match legacy encoder\n");
 	}
 }
 
@@ -12903,7 +12901,7 @@ intel_modeset_check_state(struct drm_device *dev,
 			  struct drm_atomic_state *old_state)
 {
 	check_wm_state(dev);
-	check_connector_state(dev);
+	check_connector_state(dev, old_state);
 	check_encoder_state(dev);
 	check_crtc_state(dev);
 	check_shared_dpll_state(dev);
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3.1 04/13] drm/i915: Convert connector checking to atomic, v3.

[Maarten Lankhorst]

Right now dpms callbacks can still fiddle with the connector state,
but it can only turn connectors off.

This is remediated by only checking crtc->state->active when the
connector is active, and ignore crtc->state->active when the
connector is off.

connectors_active is no longer checked, and will be removed later
in this series together with dpms.

Another check for !encoder->crtc is performed by check_encoder_state
too, so it can be removed.

Changes since v1:
- Add commit message.
- rename state to old_state.
- Move deletion of mst_port check to mst patch.
Changes since v2:
- Fix a null pointer dereference on MST now hw readout is fixed.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
---
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 77b4da7e698c..9baddc16b285 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6360,38 +6360,36 @@ static void intel_encoder_dpms(struct intel_encoder *encoder, int mode)
  * internal consistency). */
 static void intel_connector_check_state(struct intel_connector *connector)
 {
+	struct drm_crtc *crtc = connector->base.state->crtc;
+
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
+		      connector->base.base.id,
+		      connector->base.name);
+
 	if (connector->get_hw_state(connector)) {
-		struct intel_encoder *encoder = connector->encoder;
-		struct drm_crtc *crtc;
-		bool encoder_enabled;
-		enum pipe pipe;
+		struct drm_encoder *encoder = &connector->encoder->base;
+		struct drm_connector_state *conn_state = connector->base.state;
 
-		DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
-			      connector->base.base.id,
-			      connector->base.name);
+		I915_STATE_WARN(!crtc,
+			 "connector enabled without attached crtc\n");
 
-		I915_STATE_WARN(connector->base.dpms == DRM_MODE_DPMS_OFF,
-		     "wrong connector dpms state\n");
-		I915_STATE_WARN(connector->base.encoder != &encoder->base,
-		     "active connector not linked to encoder\n");
+		if (!crtc)
+			return;
 
-		if (encoder) {
-			I915_STATE_WARN(!encoder->connectors_active,
-			     "encoder->connectors_active not set\n");
+		I915_STATE_WARN(!crtc->state->active,
+		      "connector is active, but attached crtc isn't\n");
 
-			encoder_enabled = encoder->get_hw_state(encoder, &pipe);
-			I915_STATE_WARN(!encoder_enabled, "encoder not enabled\n");
-			if (I915_STATE_WARN_ON(!encoder->base.crtc))
-				return;
+		if (!encoder)
+			return;
 
-			crtc = encoder->base.crtc;
+		I915_STATE_WARN(conn_state->best_encoder != encoder,
+			"atomic encoder doesn't match attached encoder\n");
 
-			I915_STATE_WARN(!crtc->state->enable,
-					"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,
-			     "encoder active on the wrong pipe\n");
-		}
+		I915_STATE_WARN(conn_state->crtc != encoder->crtc,
+			"attached encoder crtc differs from connector crtc\n");
+	} else {
+		I915_STATE_WARN(!crtc && connector->base.state->best_encoder,
+			"best encoder set without crtc!\n");
 	}
 }
 
@@ -12699,20 +12697,23 @@ static void check_wm_state(struct drm_device *dev)
 }
 
 static void
-check_connector_state(struct drm_device *dev)
+check_connector_state(struct drm_device *dev,
+		      struct drm_atomic_state *old_state)
 {
-	struct intel_connector *connector;
+	struct drm_connector_state *old_conn_state;
+	struct drm_connector *connector;
+	int i;
 
-	for_each_intel_connector(dev, connector) {
-		struct drm_encoder *encoder = connector->base.encoder;
-		struct drm_connector_state *state = connector->base.state;
+	for_each_connector_in_state(old_state, connector, old_conn_state, i) {
+		struct drm_encoder *encoder = connector->encoder;
+		struct drm_connector_state *state = connector->state;
 
 		/* This also checks the encoder/connector hw state with the
 		 * ->get_hw_state callbacks. */
-		intel_connector_check_state(connector);
+		intel_connector_check_state(to_intel_connector(connector));
 
 		I915_STATE_WARN(state->best_encoder != encoder,
-		     "connector's staged encoder doesn't match current encoder\n");
+		     "connector's atomic encoder doesn't match legacy encoder\n");
 	}
 }
 
@@ -12903,7 +12904,7 @@ intel_modeset_check_state(struct drm_device *dev,
 			  struct drm_atomic_state *old_state)
 {
 	check_wm_state(dev);
-	check_connector_state(dev);
+	check_connector_state(dev, old_state);
 	check_encoder_state(dev);
 	check_crtc_state(dev);
 	check_shared_dpll_state(dev);

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 05/13] drm/i915: Remove some unneeded checks from check_crtc_state.

[Maarten Lankhorst]

This is handled by the atomic core now, no need to check this for ourself.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
---
 drivers/gpu/drm/i915/intel_display.c | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 876071ad4681..7f4c8df86555 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12782,8 +12782,7 @@ check_crtc_state(struct drm_device *dev)
 	struct intel_crtc_state pipe_config;
 
 	for_each_intel_crtc(dev, crtc) {
-		bool enabled = false;
-		bool active = false;
+		bool active;
 
 		memset(&pipe_config, 0, sizeof(pipe_config));
 
@@ -12793,22 +12792,6 @@ check_crtc_state(struct drm_device *dev)
 		I915_STATE_WARN(crtc->active && !crtc->base.state->enable,
 		     "active crtc, but not enabled in sw tracking\n");
 
-		for_each_intel_encoder(dev, encoder) {
-			if (encoder->base.crtc != &crtc->base)
-				continue;
-			enabled = true;
-			if (encoder->connectors_active)
-				active = true;
-		}
-
-		I915_STATE_WARN(active != crtc->active,
-		     "crtc's computed active state doesn't match tracked active state "
-		     "(expected %i, found %i)\n", active, crtc->active);
-		I915_STATE_WARN(enabled != crtc->base.state->enable,
-		     "crtc's computed enabled state doesn't match tracked enabled state "
-		     "(expected %i, found %i)\n", enabled,
-				crtc->base.state->enable);
-
 		active = dev_priv->display.get_pipe_config(crtc,
 							   &pipe_config);
 
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 06/13] drm/i915: Remove connectors_active from state checking.

[Maarten Lankhorst]

Connectors are updated atomically now, so the only interaction
with the encoder is through base.crtc.

If it's NULL the encoder's not part of any crtc, and if it's
not NULL then active should be equal to crtc_state->active.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
---
 drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7f4c8df86555..129931f3013e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12729,9 +12729,6 @@ check_encoder_state(struct drm_device *dev)
 			      encoder->base.base.id,
 			      encoder->base.name);
 
-		I915_STATE_WARN(encoder->connectors_active && !encoder->base.crtc,
-		     "encoder's active_connectors set, but no crtc\n");
-
 		for_each_intel_connector(dev, connector) {
 			if (connector->base.encoder != &encoder->base)
 				continue;
@@ -12751,18 +12748,20 @@ check_encoder_state(struct drm_device *dev)
 		I915_STATE_WARN(active && !encoder->base.crtc,
 		     "active encoder with no crtc\n");
 
-		I915_STATE_WARN(encoder->connectors_active != active,
-		     "encoder's computed active state doesn't match tracked active state "
-		     "(expected %i, found %i)\n", active, encoder->connectors_active);
-
 		active = encoder->get_hw_state(encoder, &pipe);
-		I915_STATE_WARN(active != encoder->connectors_active,
+
+		if (!encoder->base.crtc) {
+			I915_STATE_WARN(active,
+			     "encoder detached but not turned off.\n");
+
+			continue;
+		}
+
+		I915_STATE_WARN(active != encoder->base.crtc->state->active,
 		     "encoder's hw state doesn't match sw tracking "
 		     "(expected %i, found %i)\n",
-		     encoder->connectors_active, active);
+		     encoder->base.crtc->state->active, active);
 
-		if (!encoder->base.crtc)
-			continue;
 
 		tracked_pipe = to_intel_crtc(encoder->base.crtc)->pipe;
 		I915_STATE_WARN(active && pipe != tracked_pipe,
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 07/13] drm/i915: Make crtc checking use the atomic state, v2.

[Maarten Lankhorst]

Instead of allocating pipe_config on the stack use the old
crtc_state, it's only going to freed from this point on.

All crtc' are now only checked once during modeset,
because false positives can happen with encoders after
dpms changes and to limit the amount of errors for 1 failure.

Changes since v1:
- crtc_state -> old_crtc_state
- state -> old_state

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
---
 drivers/gpu/drm/i915/intel_display.c | 118 +++++++++++++++++------------------
 1 file changed, 56 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 129931f3013e..4babecd1c99f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11829,7 +11829,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 	struct intel_crtc_state *pipe_config =
 		to_intel_crtc_state(crtc_state);
 	struct drm_atomic_state *state = crtc_state->state;
-	int ret, idx = crtc->base.id;
+	int ret;
 	bool mode_changed = needs_modeset(crtc_state);
 
 	if (mode_changed && !check_encoder_cloning(state, intel_crtc)) {
@@ -11837,10 +11837,6 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 		return -EINVAL;
 	}
 
-	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);
-
 	if (mode_changed && !crtc_state->active)
 		intel_crtc->atomic.update_wm_post = true;
 
@@ -12722,19 +12718,16 @@ check_encoder_state(struct drm_device *dev)
 
 	for_each_intel_encoder(dev, encoder) {
 		bool enabled = false;
-		bool active = false;
-		enum pipe pipe, tracked_pipe;
+		enum pipe pipe;
 
 		DRM_DEBUG_KMS("[ENCODER:%d:%s]\n",
 			      encoder->base.base.id,
 			      encoder->base.name);
 
 		for_each_intel_connector(dev, connector) {
-			if (connector->base.encoder != &encoder->base)
+			if (connector->base.state->best_encoder != &encoder->base)
 				continue;
 			enabled = true;
-			if (connector->base.dpms != DRM_MODE_DPMS_OFF)
-				active = true;
 
 			I915_STATE_WARN(connector->base.state->crtc !=
 					encoder->base.crtc,
@@ -12745,85 +12738,86 @@ check_encoder_state(struct drm_device *dev)
 		     "encoder's enabled state mismatch "
 		     "(expected %i, found %i)\n",
 		     !!encoder->base.crtc, enabled);
-		I915_STATE_WARN(active && !encoder->base.crtc,
-		     "active encoder with no crtc\n");
-
-		active = encoder->get_hw_state(encoder, &pipe);
 
 		if (!encoder->base.crtc) {
-			I915_STATE_WARN(active,
-			     "encoder detached but not turned off.\n");
+			bool active;
 
-			continue;
+			active = encoder->get_hw_state(encoder, &pipe);
+			I915_STATE_WARN(active,
+			     "encoder detached but still enabled on pipe %c.\n",
+			     pipe_name(pipe));
 		}
-
-		I915_STATE_WARN(active != encoder->base.crtc->state->active,
-		     "encoder's hw state doesn't match sw tracking "
-		     "(expected %i, found %i)\n",
-		     encoder->base.crtc->state->active, active);
-
-
-		tracked_pipe = to_intel_crtc(encoder->base.crtc)->pipe;
-		I915_STATE_WARN(active && pipe != tracked_pipe,
-		     "active encoder's pipe doesn't match"
-		     "(expected %i, found %i)\n",
-		     tracked_pipe, pipe);
-
 	}
 }
 
 static void
-check_crtc_state(struct drm_device *dev)
+check_crtc_state(struct drm_device *dev, struct drm_atomic_state *old_state)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *crtc;
 	struct intel_encoder *encoder;
-	struct intel_crtc_state pipe_config;
+	struct drm_crtc_state *old_crtc_state;
+	struct drm_crtc *crtc;
+	int i;
 
-	for_each_intel_crtc(dev, crtc) {
+	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
+		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+		struct intel_crtc_state *pipe_config, *sw_config;
 		bool active;
 
-		memset(&pipe_config, 0, sizeof(pipe_config));
+		if (!needs_modeset(crtc->state))
+			continue;
 
-		DRM_DEBUG_KMS("[CRTC:%d]\n",
-			      crtc->base.base.id);
+		__drm_atomic_helper_crtc_destroy_state(crtc, old_crtc_state);
+		pipe_config = to_intel_crtc_state(old_crtc_state);
+		memset(pipe_config, 0, sizeof(*pipe_config));
+		pipe_config->base.crtc = crtc;
+		pipe_config->base.state = old_state;
 
-		I915_STATE_WARN(crtc->active && !crtc->base.state->enable,
-		     "active crtc, but not enabled in sw tracking\n");
+		DRM_DEBUG_KMS("[CRTC:%d]\n",
+			      crtc->base.id);
 
-		active = dev_priv->display.get_pipe_config(crtc,
-							   &pipe_config);
+		active = dev_priv->display.get_pipe_config(intel_crtc,
+							   pipe_config);
 
 		/* hw state is inconsistent with the pipe quirk */
-		if ((crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) ||
-		    (crtc->pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE))
-			active = crtc->active;
-
-		for_each_intel_encoder(dev, encoder) {
-			enum pipe pipe;
-			if (encoder->base.crtc != &crtc->base)
-				continue;
-			if (encoder->get_hw_state(encoder, &pipe))
-				encoder->get_config(encoder, &pipe_config);
-		}
+		if ((intel_crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) ||
+		    (intel_crtc->pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE))
+			active = crtc->state->active;
 
-		I915_STATE_WARN(crtc->active != active,
+		I915_STATE_WARN(crtc->state->active != active,
 		     "crtc active state doesn't match with hw state "
-		     "(expected %i, found %i)\n", crtc->active, active);
+		     "(expected %i, found %i)\n", crtc->state->active, active);
 
-		I915_STATE_WARN(crtc->active != crtc->base.state->active,
+		I915_STATE_WARN(intel_crtc->active != crtc->state->active,
 		     "transitional active state does not match atomic hw state "
-		     "(expected %i, found %i)\n", crtc->base.state->active, crtc->active);
+		     "(expected %i, found %i)\n", crtc->state->active, intel_crtc->active);
+
+		for_each_encoder_on_crtc(dev, crtc, encoder) {
+			enum pipe pipe;
+
+			active = encoder->get_hw_state(encoder, &pipe);
+			I915_STATE_WARN(active != crtc->state->active,
+				"[ENCODER:%i] active %i with crtc active %i\n",
+				encoder->base.base.id, active, crtc->state->active);
+
+			I915_STATE_WARN(active && intel_crtc->pipe != pipe,
+					"Encoder connected to wrong pipe %c\n",
+					pipe_name(pipe));
+
+			if (active)
+				encoder->get_config(encoder, pipe_config);
+		}
 
-		if (!active)
+		if (!crtc->state->active)
 			continue;
 
-		if (!intel_pipe_config_compare(dev, crtc->config,
-					       &pipe_config, false)) {
+		sw_config = to_intel_crtc_state(crtc->state);
+		if (!intel_pipe_config_compare(dev, sw_config,
+					       pipe_config, false)) {
 			I915_STATE_WARN(1, "pipe state doesn't match!\n");
-			intel_dump_pipe_config(crtc, &pipe_config,
+			intel_dump_pipe_config(intel_crtc, pipe_config,
 					       "[hw state]");
-			intel_dump_pipe_config(crtc, crtc->config,
+			intel_dump_pipe_config(intel_crtc, sw_config,
 					       "[sw state]");
 		}
 	}
@@ -12885,7 +12879,7 @@ intel_modeset_check_state(struct drm_device *dev,
 	check_wm_state(dev);
 	check_connector_state(dev, old_state);
 	check_encoder_state(dev);
-	check_crtc_state(dev);
+	check_crtc_state(dev, old_state);
 	check_shared_dpll_state(dev);
 }
 
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 08/13] drm/i915: Get rid of dpms handling.

[Maarten Lankhorst]

This is now done completely atomically.
Keep connectors_active for now, but make it mirror crtc_state->active.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
---
 drivers/gpu/drm/i915/intel_crt.c     | 49 +-----------------
 drivers/gpu/drm/i915/intel_display.c | 99 +-----------------------------------
 drivers/gpu/drm/i915/intel_dp.c      |  2 +-
 drivers/gpu/drm/i915/intel_dp_mst.c  |  2 +-
 drivers/gpu/drm/i915/intel_drv.h     |  3 --
 drivers/gpu/drm/i915/intel_dsi.c     |  2 +-
 drivers/gpu/drm/i915/intel_dvo.c     | 46 +----------------
 drivers/gpu/drm/i915/intel_hdmi.c    |  2 +-
 drivers/gpu/drm/i915/intel_lvds.c    |  2 +-
 drivers/gpu/drm/i915/intel_sdvo.c    | 47 +----------------
 drivers/gpu/drm/i915/intel_tv.c      |  2 +-
 11 files changed, 11 insertions(+), 245 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 9eba3dd5b434..af5e43bef4a4 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -236,53 +236,6 @@ static void intel_enable_crt(struct intel_encoder *encoder)
 	intel_crt_set_dpms(encoder, crt->connector->base.dpms);
 }
 
-/* Special dpms function to support cloning between dvo/sdvo/crt. */
-static int intel_crt_dpms(struct drm_connector *connector, int mode)
-{
-	struct drm_device *dev = connector->dev;
-	struct intel_encoder *encoder = intel_attached_encoder(connector);
-	struct drm_crtc *crtc;
-	int old_dpms;
-
-	/* PCH platforms and VLV only support on/off. */
-	if (INTEL_INFO(dev)->gen >= 5 && mode != DRM_MODE_DPMS_ON)
-		mode = DRM_MODE_DPMS_OFF;
-
-	if (mode == connector->dpms)
-		return 0;
-
-	old_dpms = connector->dpms;
-	connector->dpms = mode;
-
-	/* Only need to change hw state when actually enabled */
-	crtc = encoder->base.crtc;
-	if (!crtc) {
-		encoder->connectors_active = false;
-		return 0;
-	}
-
-	/* We need the pipe to run for anything but OFF. */
-	if (mode == DRM_MODE_DPMS_OFF)
-		encoder->connectors_active = false;
-	else
-		encoder->connectors_active = true;
-
-	/* We call connector dpms manually below in case pipe dpms doesn't
-	 * change due to cloning. */
-	if (mode < old_dpms) {
-		/* From off to on, enable the pipe first. */
-		intel_crtc_update_dpms(crtc);
-
-		intel_crt_set_dpms(encoder, mode);
-	} else {
-		intel_crt_set_dpms(encoder, mode);
-
-		intel_crtc_update_dpms(crtc);
-	}
-
-	return 0;
-}
-
 static enum drm_mode_status
 intel_crt_mode_valid(struct drm_connector *connector,
 		     struct drm_display_mode *mode)
@@ -798,7 +751,7 @@ static void intel_crt_reset(struct drm_connector *connector)
 
 static const struct drm_connector_funcs intel_crt_connector_funcs = {
 	.reset = intel_crt_reset,
-	.dpms = intel_crt_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_crt_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.destroy = intel_crt_destroy,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4babecd1c99f..4837d14a5f8e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6271,67 +6271,6 @@ free:
 	return ret;
 }
 
-/* Master function to enable/disable CRTC and corresponding power wells */
-int intel_crtc_control(struct drm_crtc *crtc, bool enable)
-{
-	struct drm_device *dev = crtc->dev;
-	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);
-	struct intel_crtc_state *pipe_config;
-	struct drm_atomic_state *state;
-	int ret;
-
-	if (enable == intel_crtc->active)
-		return 0;
-
-	if (enable && !crtc->state->enable)
-		return 0;
-
-	/* this function should be called with drm_modeset_lock_all for now */
-	if (WARN_ON(!ctx))
-		return -EIO;
-	lockdep_assert_held(&ctx->ww_ctx);
-
-	state = drm_atomic_state_alloc(dev);
-	if (WARN_ON(!state))
-		return -ENOMEM;
-
-	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;
-
-	ret = drm_atomic_commit(state);
-	if (!ret)
-		return ret;
-
-err:
-	DRM_ERROR("Updating crtc active failed with %i\n", ret);
-	drm_atomic_state_free(state);
-	return ret;
-}
-
-/**
- * Sets the power management mode of the pipe and plane.
- */
-void intel_crtc_update_dpms(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	struct intel_encoder *intel_encoder;
-	bool enable = false;
-
-	for_each_encoder_on_crtc(dev, crtc, intel_encoder)
-		enable |= intel_encoder->connectors_active;
-
-	intel_crtc_control(crtc, enable);
-}
-
 void intel_encoder_destroy(struct drm_encoder *encoder)
 {
 	struct intel_encoder *intel_encoder = to_intel_encoder(encoder);
@@ -6340,22 +6279,6 @@ void intel_encoder_destroy(struct drm_encoder *encoder)
 	kfree(intel_encoder);
 }
 
-/* Simple dpms helper for encoders with just one connector, no cloning and only
- * one kind of off state. It clamps all !ON modes to fully OFF and changes the
- * state of the entire output pipe. */
-static void intel_encoder_dpms(struct intel_encoder *encoder, int mode)
-{
-	if (mode == DRM_MODE_DPMS_ON) {
-		encoder->connectors_active = true;
-
-		intel_crtc_update_dpms(encoder->base.crtc);
-	} else {
-		encoder->connectors_active = false;
-
-		intel_crtc_update_dpms(encoder->base.crtc);
-	}
-}
-
 /* Cross check the actual hw state with our own modeset state tracking (and it's
  * internal consistency). */
 static void intel_connector_check_state(struct intel_connector *connector)
@@ -6385,6 +6308,8 @@ static void intel_connector_check_state(struct intel_connector *connector)
 		I915_STATE_WARN(conn_state->crtc != encoder->crtc,
 			"attached encoder crtc differs from connector crtc\n");
 	} else {
+		I915_STATE_WARN(crtc && crtc->state->active,
+			"attached crtc is active, but connector isn't\n");
 		I915_STATE_WARN(!crtc && connector->base.state->best_encoder,
 			"best encoder set without crtc!\n");
 	}
@@ -6418,26 +6343,6 @@ struct intel_connector *intel_connector_alloc(void)
 	return connector;
 }
 
-/* Even simpler default implementation, if there's really no special case to
- * consider. */
-int intel_connector_dpms(struct drm_connector *connector, int mode)
-{
-	/* All the simple cases only support two dpms states. */
-	if (mode != DRM_MODE_DPMS_ON)
-		mode = DRM_MODE_DPMS_OFF;
-
-	if (mode == connector->dpms)
-		return 0;
-
-	connector->dpms = mode;
-
-	/* Only need to change hw state when actually enabled */
-	if (connector->encoder)
-		intel_encoder_dpms(to_intel_encoder(connector->encoder), mode);
-
-	return 0;
-}
-
 /* Simple connector->get_hw_state implementation for encoders that support only
  * one connector and no cloning and hence the encoder state determines the state
  * of the connector. */
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 44f8a32e4d1e..3c243dee72c2 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4825,7 +4825,7 @@ static void intel_dp_encoder_reset(struct drm_encoder *encoder)
 }
 
 static const struct drm_connector_funcs intel_dp_connector_funcs = {
-	.dpms = intel_connector_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_dp_detect,
 	.force = intel_dp_force,
 	.fill_modes = drm_helper_probe_single_connector_modes,
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 36bd9394f789..fa7033094c95 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -328,7 +328,7 @@ intel_dp_mst_connector_destroy(struct drm_connector *connector)
 }
 
 static const struct drm_connector_funcs intel_dp_mst_connector_funcs = {
-	.dpms = intel_connector_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_dp_mst_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.set_property = intel_dp_mst_set_property,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0da4236dc85a..62073d2f83fa 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -992,12 +992,9 @@ 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);
 int intel_display_suspend(struct drm_device *dev);
-int intel_crtc_control(struct drm_crtc *crtc, bool enable);
-void intel_crtc_update_dpms(struct drm_crtc *crtc);
 void intel_encoder_destroy(struct drm_encoder *encoder);
 int intel_connector_init(struct intel_connector *);
 struct intel_connector *intel_connector_alloc(void);
-int intel_connector_dpms(struct drm_connector *, int mode);
 bool intel_connector_get_hw_state(struct intel_connector *connector);
 bool ibx_digital_port_connected(struct drm_i915_private *dev_priv,
 				struct intel_digital_port *port);
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 18dd7d7360a0..4a601cf90f16 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -982,7 +982,7 @@ static const struct drm_connector_helper_funcs intel_dsi_connector_helper_funcs
 };
 
 static const struct drm_connector_funcs intel_dsi_connector_funcs = {
-	.dpms = intel_connector_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_dsi_detect,
 	.destroy = intel_dsi_connector_destroy,
 	.fill_modes = drm_helper_probe_single_connector_modes,
diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
index 600f7fb855d8..dc532bb61d22 100644
--- a/drivers/gpu/drm/i915/intel_dvo.c
+++ b/drivers/gpu/drm/i915/intel_dvo.c
@@ -196,50 +196,6 @@ static void intel_enable_dvo(struct intel_encoder *encoder)
 	intel_dvo->dev.dev_ops->dpms(&intel_dvo->dev, true);
 }
 
-/* Special dpms function to support cloning between dvo/sdvo/crt. */
-static int intel_dvo_dpms(struct drm_connector *connector, int mode)
-{
-	struct intel_dvo *intel_dvo = intel_attached_dvo(connector);
-	struct drm_crtc *crtc;
-	struct intel_crtc_state *config;
-
-	/* dvo supports only 2 dpms states. */
-	if (mode != DRM_MODE_DPMS_ON)
-		mode = DRM_MODE_DPMS_OFF;
-
-	if (mode == connector->dpms)
-		return 0;
-
-	connector->dpms = mode;
-
-	/* Only need to change hw state when actually enabled */
-	crtc = intel_dvo->base.base.crtc;
-	if (!crtc) {
-		intel_dvo->base.connectors_active = false;
-		return 0;
-	}
-
-	/* We call connector dpms manually below in case pipe dpms doesn't
-	 * change due to cloning. */
-	if (mode == DRM_MODE_DPMS_ON) {
-		config = to_intel_crtc(crtc)->config;
-
-		intel_dvo->base.connectors_active = true;
-
-		intel_crtc_update_dpms(crtc);
-
-		intel_dvo->dev.dev_ops->dpms(&intel_dvo->dev, true);
-	} else {
-		intel_dvo->dev.dev_ops->dpms(&intel_dvo->dev, false);
-
-		intel_dvo->base.connectors_active = false;
-
-		intel_crtc_update_dpms(crtc);
-	}
-
-	return 0;
-}
-
 static enum drm_mode_status
 intel_dvo_mode_valid(struct drm_connector *connector,
 		     struct drm_display_mode *mode)
@@ -387,7 +343,7 @@ static void intel_dvo_destroy(struct drm_connector *connector)
 }
 
 static const struct drm_connector_funcs intel_dvo_connector_funcs = {
-	.dpms = intel_dvo_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_dvo_detect,
 	.destroy = intel_dvo_destroy,
 	.fill_modes = drm_helper_probe_single_connector_modes,
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 70bad5bf1d48..51cbea8247fe 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1909,7 +1909,7 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
 }
 
 static const struct drm_connector_funcs intel_hdmi_connector_funcs = {
-	.dpms = intel_connector_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_hdmi_detect,
 	.force = intel_hdmi_force,
 	.fill_modes = drm_helper_probe_single_connector_modes,
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index cb634f48e7d9..881b5d13592e 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -549,7 +549,7 @@ static const struct drm_connector_helper_funcs intel_lvds_connector_helper_funcs
 };
 
 static const struct drm_connector_funcs intel_lvds_connector_funcs = {
-	.dpms = intel_connector_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_lvds_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.set_property = intel_lvds_set_property,
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 8911e0e417ee..c98098e884cc 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1508,51 +1508,6 @@ static void intel_enable_sdvo(struct intel_encoder *encoder)
 	intel_sdvo_set_active_outputs(intel_sdvo, intel_sdvo->attached_output);
 }
 
-/* Special dpms function to support cloning between dvo/sdvo/crt. */
-static int intel_sdvo_dpms(struct drm_connector *connector, int mode)
-{
-	struct drm_crtc *crtc;
-	struct intel_sdvo *intel_sdvo = intel_attached_sdvo(connector);
-
-	/* dvo supports only 2 dpms states. */
-	if (mode != DRM_MODE_DPMS_ON)
-		mode = DRM_MODE_DPMS_OFF;
-
-	if (mode == connector->dpms)
-		return 0;
-
-	connector->dpms = mode;
-
-	/* Only need to change hw state when actually enabled */
-	crtc = intel_sdvo->base.base.crtc;
-	if (!crtc) {
-		intel_sdvo->base.connectors_active = false;
-		return 0;
-	}
-
-	/* We set active outputs manually below in case pipe dpms doesn't change
-	 * due to cloning. */
-	if (mode != DRM_MODE_DPMS_ON) {
-		intel_sdvo_set_active_outputs(intel_sdvo, 0);
-		if (0)
-			intel_sdvo_set_encoder_power_state(intel_sdvo, mode);
-
-		intel_sdvo->base.connectors_active = false;
-
-		intel_crtc_update_dpms(crtc);
-	} else {
-		intel_sdvo->base.connectors_active = true;
-
-		intel_crtc_update_dpms(crtc);
-
-		if (0)
-			intel_sdvo_set_encoder_power_state(intel_sdvo, mode);
-		intel_sdvo_set_active_outputs(intel_sdvo, intel_sdvo->attached_output);
-	}
-
-	return 0;
-}
-
 static enum drm_mode_status
 intel_sdvo_mode_valid(struct drm_connector *connector,
 		      struct drm_display_mode *mode)
@@ -2190,7 +2145,7 @@ done:
 }
 
 static const struct drm_connector_funcs intel_sdvo_connector_funcs = {
-	.dpms = intel_sdvo_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_sdvo_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.set_property = intel_sdvo_set_property,
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 8b9d325bda3c..0568ae6ec9dd 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1509,7 +1509,7 @@ out:
 }
 
 static const struct drm_connector_funcs intel_tv_connector_funcs = {
-	.dpms = intel_connector_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_tv_detect,
 	.destroy = intel_tv_destroy,
 	.set_property = intel_tv_set_property,
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 09/13] drm/i915: Remove connectors_active from sanitization, v2.

[Maarten Lankhorst]

connectors_active will be removed, so just calculate this instead.

Changes since v1:
- Look for the right pointer in intel_sanitize_encoder.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
---
 drivers/gpu/drm/i915/intel_display.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4837d14a5f8e..ff0a22f223e6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14936,8 +14936,10 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 	/* Adjust the state of the output pipe according to whether we
 	 * have active connectors/encoders. */
 	enable = false;
-	for_each_encoder_on_crtc(dev, &crtc->base, encoder)
-		enable |= encoder->connectors_active;
+	for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
+		enable = true;
+		break;
+	}
 
 	if (!enable)
 		intel_crtc_disable_noatomic(&crtc->base);
@@ -14993,6 +14995,7 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
 {
 	struct intel_connector *connector;
 	struct drm_device *dev = encoder->base.dev;
+	bool active = false;
 
 	/* We need to check both for a crtc link (meaning that the
 	 * encoder is active and trying to read from a pipe) and the
@@ -15000,7 +15003,15 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
 	bool has_active_crtc = encoder->base.crtc &&
 		to_intel_crtc(encoder->base.crtc)->active;
 
-	if (encoder->connectors_active && !has_active_crtc) {
+	for_each_intel_connector(dev, connector) {
+		if (connector->base.encoder != &encoder->base)
+			continue;
+
+		active = true;
+		break;
+	}
+
+	if (active && !has_active_crtc) {
 		DRM_DEBUG_KMS("[ENCODER:%d:%s] has active connectors but no active pipe!\n",
 			      encoder->base.base.id,
 			      encoder->base.name);
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 10/13] drm/i915: Remove connectors_active from intel_dp.c, v2.

[Maarten Lankhorst]

Now that everything's atomic, checking encoder->base.crtc is enough.
This function doesn't have the locks to dereference crtc->state, but
stealing an encoder bound to any crtc is probably enough reason to warn.

Changes since v1:
- Commit message.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 3c243dee72c2..da347a8996a6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2624,7 +2624,7 @@ static void vlv_steal_power_sequencer(struct drm_device *dev,
 		DRM_DEBUG_KMS("stealing pipe %c power sequencer from port %c\n",
 			      pipe_name(pipe), port_name(port));
 
-		WARN(encoder->connectors_active,
+		WARN(encoder->base.crtc,
 		     "stealing pipe %c power sequencer from active eDP port %c\n",
 		     pipe_name(pipe), port_name(port));
 
@@ -4241,10 +4241,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 
 	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
-	if (!intel_encoder->connectors_active)
-		return;
-
-	if (WARN_ON(!intel_encoder->base.crtc))
+	if (!intel_encoder->base.crtc)
 		return;
 
 	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 11/13] drm/i915: Remove connectors_active.

[Maarten Lankhorst]

There are no more users, byebye!

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
---
 drivers/gpu/drm/i915/intel_display.c | 37 +-----------------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 2 files changed, 1 insertion(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ff0a22f223e6..1fd8b7dec7da 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12196,27 +12196,12 @@ static bool intel_crtc_in_use(struct drm_crtc *crtc)
 static void
 intel_modeset_update_state(struct drm_atomic_state *state)
 {
-	struct drm_device *dev = state->dev;
-	struct intel_encoder *intel_encoder;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
-	struct drm_connector *connector;
 	int i;
 
 	intel_shared_dpll_commit(state);
 
-	for_each_intel_encoder(dev, intel_encoder) {
-		if (!intel_encoder->base.crtc)
-			continue;
-
-		crtc = intel_encoder->base.crtc;
-		crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
-		if (!crtc_state || !needs_modeset(crtc->state))
-			continue;
-
-		intel_encoder->connectors_active = false;
-	}
-
 	drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
 
 	/* Double check state. */
@@ -12231,21 +12216,6 @@ intel_modeset_update_state(struct drm_atomic_state *state)
 		else
 			crtc->hwmode.crtc_clock = 0;
 	}
-
-	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
-		if (!connector->encoder || !connector->encoder->crtc)
-			continue;
-
-		crtc = connector->encoder->crtc;
-		crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
-		if (!crtc_state || !needs_modeset(crtc->state))
-			continue;
-
-		if (crtc->state->active) {
-			intel_encoder = to_intel_encoder(connector->encoder);
-			intel_encoder->connectors_active = true;
-		}
-	}
 }
 
 static bool intel_fuzzy_clock_check(int clock1, int clock2)
@@ -14966,10 +14936,8 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 		 *  actually up, hence no need to break them. */
 		WARN_ON(crtc->active);
 
-		for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
-			WARN_ON(encoder->connectors_active);
+		for_each_encoder_on_crtc(dev, &crtc->base, encoder)
 			encoder->base.crtc = NULL;
-		}
 	}
 
 	if (crtc->active || HAS_GMCH_DISPLAY(dev)) {
@@ -15028,7 +14996,6 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
 				encoder->post_disable(encoder);
 		}
 		encoder->base.crtc = NULL;
-		encoder->connectors_active = false;
 
 		/* Inconsistent output/port/pipe state happens presumably due to
 		 * a bug in one of the get_hw_state functions. Or someplace else
@@ -15190,7 +15157,6 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 			encoder->base.crtc = NULL;
 		}
 
-		encoder->connectors_active = false;
 		DRM_DEBUG_KMS("[ENCODER:%d:%s] hw state readout: %s, pipe %c\n",
 			      encoder->base.base.id,
 			      encoder->base.name,
@@ -15201,7 +15167,6 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 	for_each_intel_connector(dev, connector) {
 		if (connector->get_hw_state(connector)) {
 			connector->base.dpms = DRM_MODE_DPMS_ON;
-			connector->encoder->connectors_active = true;
 			connector->base.encoder = &connector->encoder->base;
 		} else {
 			connector->base.dpms = DRM_MODE_DPMS_OFF;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 62073d2f83fa..c30bbb22381b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -133,7 +133,6 @@ struct intel_encoder {
 
 	enum intel_output_type type;
 	unsigned int cloneable;
-	bool connectors_active;
 	void (*hot_plug)(struct intel_encoder *);
 	bool (*compute_config)(struct intel_encoder *,
 			       struct intel_crtc_state *);
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 12/13] drm/i915: Only update mode related state if a modeset happened.

[Maarten Lankhorst]

The rest will be a noop anyway, since without modeset there will be
no updated dplls and no modeset state to update.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 30 +++++++-----------------------
 1 file changed, 7 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1fd8b7dec7da..aa444cbc2262 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12181,33 +12181,15 @@ fail:
 	return ret;
 }
 
-static bool intel_crtc_in_use(struct drm_crtc *crtc)
-{
-	struct drm_encoder *encoder;
-	struct drm_device *dev = crtc->dev;
-
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
-		if (encoder->crtc == crtc)
-			return true;
-
-	return false;
-}
-
 static void
-intel_modeset_update_state(struct drm_atomic_state *state)
+intel_modeset_update_crtc_state(struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
 	int i;
 
-	intel_shared_dpll_commit(state);
-
-	drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
-
 	/* Double check state. */
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		WARN_ON(crtc->state->enable != intel_crtc_in_use(crtc));
-
 		to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state);
 
 		/* Update hwmode for vblank functions */
@@ -13110,12 +13092,14 @@ static int intel_atomic_commit(struct drm_device *dev,
 
 	/* Only after disabling all output pipelines that will be changed can we
 	 * update the the output configuration. */
-	intel_modeset_update_state(state);
+	intel_modeset_update_crtc_state(state);
 
-	/* The state has been swaped above, so state actually contains the
-	 * old state now. */
-	if (any_ms)
+	if (any_ms) {
+		intel_shared_dpll_commit(state);
+
+		drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
 		modeset_update_crtc_power_domains(state);
+	}
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 12/13] drm/i915: Only update mode related state if a modeset happened.

[Daniel Vetter]

On Wed, Aug 05, 2015 at 12:37:10PM +0200, Maarten Lankhorst wrote:
> The rest will be a noop anyway, since without modeset there will be
> no updated dplls and no modeset state to update.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 30 +++++++-----------------------
>  1 file changed, 7 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1fd8b7dec7da..aa444cbc2262 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12181,33 +12181,15 @@ fail:
>  	return ret;
>  }
>  
> -static bool intel_crtc_in_use(struct drm_crtc *crtc)
> -{
> -	struct drm_encoder *encoder;
> -	struct drm_device *dev = crtc->dev;
> -
> -	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
> -		if (encoder->crtc == crtc)
> -			return true;
> -
> -	return false;
> -}
> -
>  static void
> -intel_modeset_update_state(struct drm_atomic_state *state)
> +intel_modeset_update_crtc_state(struct drm_atomic_state *state)
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
>  	int i;
>  
> -	intel_shared_dpll_commit(state);

This should be right next to swap_state, and we should probably rename it
to be consistent.

> -
> -	drm_atomic_helper_update_legacy_modeset_state(state->dev, state);

This helper should always work, why try to optimize things?

> -
>  	/* Double check state. */
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		WARN_ON(crtc->state->enable != intel_crtc_in_use(crtc));

I guess this WARN_ON should be moved into the state checker? Or entirely
removed if redundant.

> -
>  		to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state);
>  
>  		/* Update hwmode for vblank functions */
> @@ -13110,12 +13092,14 @@ static int intel_atomic_commit(struct drm_device *dev,
>  
>  	/* Only after disabling all output pipelines that will be changed can we
>  	 * update the the output configuration. */
> -	intel_modeset_update_state(state);
> +	intel_modeset_update_crtc_state(state);
>  
> -	/* The state has been swaped above, so state actually contains the
> -	 * old state now. */
> -	if (any_ms)
> +	if (any_ms) {
> +		intel_shared_dpll_commit(state);
> +
> +		drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
>  		modeset_update_crtc_power_domains(state);
> +	}
>  
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -- 
> 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

Re: [PATCH v3 12/13] drm/i915: Only update mode related state if a modeset happened.

[Maarten Lankhorst]

Op 06-08-15 om 15:12 schreef Daniel Vetter:
> On Wed, Aug 05, 2015 at 12:37:10PM +0200, Maarten Lankhorst wrote:
>> The rest will be a noop anyway, since without modeset there will be
>> no updated dplls and no modeset state to update.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 30 +++++++-----------------------
>>  1 file changed, 7 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 1fd8b7dec7da..aa444cbc2262 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12181,33 +12181,15 @@ fail:
>>  	return ret;
>>  }
>>  
>> -static bool intel_crtc_in_use(struct drm_crtc *crtc)
>> -{
>> -	struct drm_encoder *encoder;
>> -	struct drm_device *dev = crtc->dev;
>> -
>> -	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
>> -		if (encoder->crtc == crtc)
>> -			return true;
>> -
>> -	return false;
>> -}
>> -
>>  static void
>> -intel_modeset_update_state(struct drm_atomic_state *state)
>> +intel_modeset_update_crtc_state(struct drm_atomic_state *state)
>>  {
>>  	struct drm_crtc *crtc;
>>  	struct drm_crtc_state *crtc_state;
>>  	int i;
>>  
>> -	intel_shared_dpll_commit(state);
> This should be right next to swap_state, and we should probably rename it
> to be consistent.
After some digging I think this could work if we no longer check pll->config.crtc_mask
in intel_disable_shared_dpll.

If we check crtc_mask in intel_enable_shared_dpll we can remove the one from disable
and prepare, and let the hw checker deal with it.
>> -
>> -	drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
> This helper should always work, why try to optimize things?
I didn't see the point of going through the connector state,
but shrug I guess the extra loops probably don't matter.
>> -
>>  	/* Double check state. */
>>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> -		WARN_ON(crtc->state->enable != intel_crtc_in_use(crtc));
> I guess this WARN_ON should be moved into the state checker? Or entirely
> removed if redundant.
It's removed because the atomic core does this for us.
>> -
>>  		to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state);
>>  
>>  		/* Update hwmode for vblank functions */
>> @@ -13110,12 +13092,14 @@ static int intel_atomic_commit(struct drm_device *dev,
>>  
>>  	/* Only after disabling all output pipelines that will be changed can we
>>  	 * update the the output configuration. */
>> -	intel_modeset_update_state(state);
>> +	intel_modeset_update_crtc_state(state);
>>  
>> -	/* The state has been swaped above, so state actually contains the
>> -	 * old state now. */
>> -	if (any_ms)
>> +	if (any_ms) {
>> +		intel_shared_dpll_commit(state);
>> +
>> +		drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
>>  		modeset_update_crtc_power_domains(state);
>> +	}
>>  
>>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
>>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> -- 
>> 2.1.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 12/13] drm/i915: Only update mode related state if a modeset happened.

[Daniel Vetter]

On Thu, Aug 06, 2015 at 04:06:58PM +0200, Maarten Lankhorst wrote:
> Op 06-08-15 om 15:12 schreef Daniel Vetter:
> > On Wed, Aug 05, 2015 at 12:37:10PM +0200, Maarten Lankhorst wrote:
> >> The rest will be a noop anyway, since without modeset there will be
> >> no updated dplls and no modeset state to update.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_display.c | 30 +++++++-----------------------
> >>  1 file changed, 7 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 1fd8b7dec7da..aa444cbc2262 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -12181,33 +12181,15 @@ fail:
> >>  	return ret;
> >>  }
> >>  
> >> -static bool intel_crtc_in_use(struct drm_crtc *crtc)
> >> -{
> >> -	struct drm_encoder *encoder;
> >> -	struct drm_device *dev = crtc->dev;
> >> -
> >> -	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
> >> -		if (encoder->crtc == crtc)
> >> -			return true;
> >> -
> >> -	return false;
> >> -}
> >> -
> >>  static void
> >> -intel_modeset_update_state(struct drm_atomic_state *state)
> >> +intel_modeset_update_crtc_state(struct drm_atomic_state *state)
> >>  {
> >>  	struct drm_crtc *crtc;
> >>  	struct drm_crtc_state *crtc_state;
> >>  	int i;
> >>  
> >> -	intel_shared_dpll_commit(state);
> > This should be right next to swap_state, and we should probably rename it
> > to be consistent.
> After some digging I think this could work if we no longer check pll->config.crtc_mask
> in intel_disable_shared_dpll.
> 
> If we check crtc_mask in intel_enable_shared_dpll we can remove the one from disable
> and prepare, and let the hw checker deal with it.

Yeah I think moving all the state checks into the checker would be useful
- my comment was really a high-level "this is how it should be" comment,
without looking into the details ;-9

> >> -
> >> -	drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
> > This helper should always work, why try to optimize things?
> I didn't see the point of going through the connector state,
> but shrug I guess the extra loops probably don't matter.

modeset is expensive, a few more loops won't hurt I think.

> >> -
> >>  	/* Double check state. */
> >>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> >> -		WARN_ON(crtc->state->enable != intel_crtc_in_use(crtc));
> > I guess this WARN_ON should be moved into the state checker? Or entirely
> > removed if redundant.
> It's removed because the atomic core does this for us.

Separate patch and should be explained in the commit message ;-)
-Daniel
-- 
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

[PATCH v3 13/13] drm/i915: Handle return value in intel_pin_and_fence_fb_obj, v2.

[Maarten Lankhorst]

-EDEADLK has special meaning in atomic, but get_fence may call
i915_find_fence_reg which can return -EDEADLK.

This has special meaning in the atomic world, so convert the error
to -EBUSY for this case.

Changes since v1:
- Add comment in the code.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index aa444cbc2262..df237ad4a780 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2395,7 +2395,18 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 	 * a fence as the cost is not that onerous.
 	 */
 	ret = i915_gem_object_get_fence(obj);
-	if (ret)
+	if (ret == -EDEADLK) {
+		/*
+		 * -EDEADLK means there are no free fences
+		 * no pending flips.
+		 *
+		 * This is propagated to atomic, but it uses
+		 * -EDEADLK to force a locking recovery, so
+		 * change the returned error to -EBUSY.
+		 */
+		ret = -EBUSY;
+		goto err_unpin;
+	} else if (ret)
 		goto err_unpin;
 
 	i915_gem_object_pin_fence(obj);
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 13/13] drm/i915: Handle return value in intel_pin_and_fence_fb_obj, v2.

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 7059
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                                  302/302              302/302
SNB                                  315/315              315/315
IVB                                  336/336              336/336
BYT                 -1              283/283              282/283
HSW                                  378/378              378/378
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*BYT  igt@gem_partial_pwrite_pread@reads-uncached      PASS(1)      FAIL(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 00/13] DPMS updates and atomic state checking.

[Daniel Vetter]

On Wed, Aug 05, 2015 at 12:36:58PM +0200, Maarten Lankhorst wrote:
> Mixed bag of fixes for -next now that the first merge happened.
> Patch series deals with getting rid of intel DPMS handling and
> making the state checker atomic.
> 
> The state checker is now atomic and only checks the affected
> crtc's, encoders and connectors.
> 
> This is just a resend with all the fixes done after v2.

Ok finally done the backmerge and merged everything I didn't comment on.

Thanks, Daniel

> 
> Maarten Lankhorst (13):
>   drm/i915: Make the force_thru workaround atomic, v2.
>   drm/i915: Validate the state after an atomic modeset only, and pass the state.
>   drm/i915: Update atomic state when removing mst connector.
>   drm/i915: Convert connector checking to atomic, v2.
>   drm/i915: Remove some unneeded checks from check_crtc_state.
>   drm/i915: Remove connectors_active from state checking.
>   drm/i915: Make crtc checking use the atomic state, v2.
>   drm/i915: Get rid of dpms handling.
>   drm/i915: Remove connectors_active from sanitization, v2.
>   drm/i915: Remove connectors_active from intel_dp.c, v2.
>   drm/i915: Remove connectors_active.
>   drm/i915: Only update mode related state if a modeset happened.
>   drm/i915: Handle return value in intel_pin_and_fence_fb_obj, v2.
> 
>  drivers/gpu/drm/i915/i915_debugfs.c  |  82 ++-----
>  drivers/gpu/drm/i915/intel_crt.c     |  51 +----
>  drivers/gpu/drm/i915/intel_display.c | 426 +++++++++++------------------------
>  drivers/gpu/drm/i915/intel_dp.c      |   9 +-
>  drivers/gpu/drm/i915/intel_dp_mst.c  |  47 +++-
>  drivers/gpu/drm/i915/intel_drv.h     |   5 -
>  drivers/gpu/drm/i915/intel_dsi.c     |   2 +-
>  drivers/gpu/drm/i915/intel_dvo.c     |  48 +---
>  drivers/gpu/drm/i915/intel_hdmi.c    |   2 +-
>  drivers/gpu/drm/i915/intel_lvds.c    |   2 +-
>  drivers/gpu/drm/i915/intel_sdvo.c    |  49 +---
>  drivers/gpu/drm/i915/intel_tv.c      |   2 +-
>  12 files changed, 213 insertions(+), 512 deletions(-)
> 
> -- 
> 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