[RESEND PATCH 0/6] drm/i915: Enable support for nonblocking modeset.

[RESEND PATCH 0/6] drm/i915: Enable support for nonblocking modeset.

[Maarten Lankhorst]

This is a rebased version of the series. Patch 4 fails to apply because
of drm_atomic_state refcounting, but otherwise unchanged.

This series is tested on my BDW and SKL. Skylake requires
Lyude's fixes + "[PATCH 0/8] drm/i915/gen9+: Atomic wm fixes."

Maarten Lankhorst (6):
  drm/i915: Convert intel_hdmi to use atomic state
  drm/i915: Pass atomic state to intel_audio_codec_enable
  drm/edid: Remove drm_select_eld
  drm/i915: Update atomic modeset state synchronously
  drm/i915: Pass atomic state to verify_connector_state
  drm/i915: Enable support for nonblocking modeset

 drivers/gpu/drm/drm_edid.c           | 26 -------------------
 drivers/gpu/drm/i915/intel_audio.c   | 17 +++++++-----
 drivers/gpu/drm/i915/intel_ddi.c     |  2 +-
 drivers/gpu/drm/i915/intel_display.c | 48 +++++++++++++++++-----------------
 drivers/gpu/drm/i915/intel_dp.c      | 11 ++++----
 drivers/gpu/drm/i915/intel_drv.h     |  4 ++-
 drivers/gpu/drm/i915/intel_hdmi.c    | 50 ++++++++++++++++--------------------
 include/drm/drm_edid.h               |  1 -
 8 files changed, 65 insertions(+), 94 deletions(-)

-- 
2.7.4

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

[RESEND PATCH 1/6] drm/i915: Convert intel_hdmi to use atomic state

[Maarten Lankhorst]

This is the last connector still looking at crtc->config. Fix this.

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

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 501334242d38..ef43244a884f 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -975,7 +975,9 @@ static void intel_hdmi_get_config(struct intel_encoder *encoder,
 	pipe_config->lane_count = 4;
 }
 
-static void intel_enable_hdmi_audio(struct intel_encoder *encoder)
+static void intel_enable_hdmi_audio(struct intel_encoder *encoder,
+				    struct intel_crtc_state *pipe_config,
+				    struct drm_connector_state *conn_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
 
@@ -991,21 +993,20 @@ static void g4x_enable_hdmi(struct intel_encoder *encoder,
 {
 	struct drm_device *dev = encoder->base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
 	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
 	u32 temp;
 
 	temp = I915_READ(intel_hdmi->hdmi_reg);
 
 	temp |= SDVO_ENABLE;
-	if (crtc->config->has_audio)
+	if (pipe_config->has_audio)
 		temp |= SDVO_AUDIO_ENABLE;
 
 	I915_WRITE(intel_hdmi->hdmi_reg, temp);
 	POSTING_READ(intel_hdmi->hdmi_reg);
 
-	if (crtc->config->has_audio)
-		intel_enable_hdmi_audio(encoder);
+	if (pipe_config->has_audio)
+		intel_enable_hdmi_audio(encoder, pipe_config, conn_state);
 }
 
 static void ibx_enable_hdmi(struct intel_encoder *encoder,
@@ -1040,8 +1041,8 @@ static void ibx_enable_hdmi(struct intel_encoder *encoder,
 	 * FIXME: BSpec says this should be done at the end of
 	 * of the modeset sequence, so not sure if this isn't too soon.
 	 */
-	if (crtc->config->pipe_bpp > 24 &&
-	    crtc->config->pixel_multiplier > 1) {
+	if (pipe_config->pipe_bpp > 24 &&
+	    pipe_config->pixel_multiplier > 1) {
 		I915_WRITE(intel_hdmi->hdmi_reg, temp & ~SDVO_ENABLE);
 		POSTING_READ(intel_hdmi->hdmi_reg);
 
@@ -1055,8 +1056,8 @@ static void ibx_enable_hdmi(struct intel_encoder *encoder,
 		POSTING_READ(intel_hdmi->hdmi_reg);
 	}
 
-	if (crtc->config->has_audio)
-		intel_enable_hdmi_audio(encoder);
+	if (pipe_config->has_audio)
+		intel_enable_hdmi_audio(encoder, pipe_config, conn_state);
 }
 
 static void cpt_enable_hdmi(struct intel_encoder *encoder,
@@ -1073,7 +1074,7 @@ static void cpt_enable_hdmi(struct intel_encoder *encoder,
 	temp = I915_READ(intel_hdmi->hdmi_reg);
 
 	temp |= SDVO_ENABLE;
-	if (crtc->config->has_audio)
+	if (pipe_config->has_audio)
 		temp |= SDVO_AUDIO_ENABLE;
 
 	/*
@@ -1086,7 +1087,7 @@ static void cpt_enable_hdmi(struct intel_encoder *encoder,
 	 * 4. enable HDMI clock gating
 	 */
 
-	if (crtc->config->pipe_bpp > 24) {
+	if (pipe_config->pipe_bpp > 24) {
 		I915_WRITE(TRANS_CHICKEN1(pipe),
 			   I915_READ(TRANS_CHICKEN1(pipe)) |
 			   TRANS_CHICKEN1_HDMIUNIT_GC_DISABLE);
@@ -1098,7 +1099,7 @@ static void cpt_enable_hdmi(struct intel_encoder *encoder,
 	I915_WRITE(intel_hdmi->hdmi_reg, temp);
 	POSTING_READ(intel_hdmi->hdmi_reg);
 
-	if (crtc->config->pipe_bpp > 24) {
+	if (pipe_config->pipe_bpp > 24) {
 		temp &= ~SDVO_COLOR_FORMAT_MASK;
 		temp |= HDMI_COLOR_FORMAT_12bpc;
 
@@ -1110,8 +1111,8 @@ static void cpt_enable_hdmi(struct intel_encoder *encoder,
 			   ~TRANS_CHICKEN1_HDMIUNIT_GC_DISABLE);
 	}
 
-	if (crtc->config->has_audio)
-		intel_enable_hdmi_audio(encoder);
+	if (pipe_config->has_audio)
+		intel_enable_hdmi_audio(encoder, pipe_config, conn_state);
 }
 
 static void vlv_enable_hdmi(struct intel_encoder *encoder,
@@ -1178,9 +1179,7 @@ static void g4x_disable_hdmi(struct intel_encoder *encoder,
 			     struct intel_crtc_state *old_crtc_state,
 			     struct drm_connector_state *old_conn_state)
 {
-	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
-
-	if (crtc->config->has_audio)
+	if (old_crtc_state->has_audio)
 		intel_audio_codec_disable(encoder);
 
 	intel_disable_hdmi(encoder, old_crtc_state, old_conn_state);
@@ -1190,9 +1189,7 @@ static void pch_disable_hdmi(struct intel_encoder *encoder,
 			     struct intel_crtc_state *old_crtc_state,
 			     struct drm_connector_state *old_conn_state)
 {
-	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
-
-	if (crtc->config->has_audio)
+	if (old_crtc_state->has_audio)
 		intel_audio_codec_disable(encoder);
 }
 
@@ -1645,13 +1642,12 @@ static void intel_hdmi_pre_enable(struct intel_encoder *encoder,
 				  struct drm_connector_state *conn_state)
 {
 	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
-	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
-	const struct drm_display_mode *adjusted_mode = &intel_crtc->config->base.adjusted_mode;
+	const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
 
 	intel_hdmi_prepare(encoder);
 
 	intel_hdmi->set_infoframes(&encoder->base,
-				   intel_crtc->config->has_hdmi_sink,
+				   pipe_config->has_hdmi_sink,
 				   adjusted_mode);
 }
 
@@ -1663,9 +1659,7 @@ static void vlv_hdmi_pre_enable(struct intel_encoder *encoder,
 	struct intel_hdmi *intel_hdmi = &dport->hdmi;
 	struct drm_device *dev = encoder->base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_crtc *intel_crtc =
-		to_intel_crtc(encoder->base.crtc);
-	const struct drm_display_mode *adjusted_mode = &intel_crtc->config->base.adjusted_mode;
+	const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
 
 	vlv_phy_pre_encoder_enable(encoder);
 
@@ -1674,7 +1668,7 @@ static void vlv_hdmi_pre_enable(struct intel_encoder *encoder,
 				 0x2b247878);
 
 	intel_hdmi->set_infoframes(&encoder->base,
-				   intel_crtc->config->has_hdmi_sink,
+				   pipe_config->has_hdmi_sink,
 				   adjusted_mode);
 
 	g4x_enable_hdmi(encoder, pipe_config, conn_state);
-- 
2.7.4

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

[RESEND PATCH 2/6] drm/i915: Pass atomic state to intel_audio_codec_enable

[Maarten Lankhorst]

drm_select_eld requires mode_config.mutex and connection_mutex
because it looks at the connector list and at the legacy encoders.

This is not required, because when we call audio_codec_enable we know
which connector it was called for, so pass the state.

This also removes having to look at crtc->config.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_audio.c | 17 ++++++++++-------
 drivers/gpu/drm/i915/intel_ddi.c   |  2 +-
 drivers/gpu/drm/i915/intel_dp.c    | 11 ++++++-----
 drivers/gpu/drm/i915/intel_drv.h   |  4 +++-
 drivers/gpu/drm/i915/intel_hdmi.c  |  2 +-
 5 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 7093cfbb62b1..63ef25893c7e 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -485,23 +485,26 @@ static void ilk_audio_codec_enable(struct drm_connector *connector,
 /**
  * intel_audio_codec_enable - Enable the audio codec for HD audio
  * @intel_encoder: encoder on which to enable audio
+ * @crtc_state: pointer to the current crtc state.
+ * @conn_state: pointer to the current connector state.
  *
  * The enable sequences may only be performed after enabling the transcoder and
  * port, and after completed link training.
  */
-void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
+void intel_audio_codec_enable(struct intel_encoder *intel_encoder,
+			      const struct intel_crtc_state *crtc_state,
+			      const struct drm_connector_state *conn_state)
 {
 	struct drm_encoder *encoder = &intel_encoder->base;
-	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
-	const struct drm_display_mode *adjusted_mode = &crtc->config->base.adjusted_mode;
+	const struct drm_display_mode *adjusted_mode = &crtc_state->base.adjusted_mode;
 	struct drm_connector *connector;
 	struct drm_i915_private *dev_priv = to_i915(encoder->dev);
 	struct i915_audio_component *acomp = dev_priv->audio_component;
 	enum port port = intel_encoder->port;
-	enum pipe pipe = crtc->pipe;
+	enum pipe pipe = drm_crtc_index(crtc_state->base.crtc);
 
-	connector = drm_select_eld(encoder);
-	if (!connector)
+	connector = conn_state->connector;
+	if (!connector || !connector->eld[0])
 		return;
 
 	DRM_DEBUG_DRIVER("ELD on [CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
@@ -512,7 +515,7 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
 
 	/* ELD Conn_Type */
 	connector->eld[5] &= ~(3 << 2);
-	if (intel_crtc_has_dp_encoder(crtc->config))
+	if (intel_crtc_has_dp_encoder(crtc_state))
 		connector->eld[5] |= (1 << 2);
 
 	connector->eld[6] = drm_av_sync_delay(connector, adjusted_mode) / 2;
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 7f7741c1406d..6748279310a4 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1824,7 +1824,7 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder,
 
 	if (intel_crtc->config->has_audio) {
 		intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
-		intel_audio_codec_enable(intel_encoder);
+		intel_audio_codec_enable(intel_encoder, pipe_config, conn_state);
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 88f3b745a326..6046f0f54cb3 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2734,7 +2734,8 @@ static void intel_dp_enable_port(struct intel_dp *intel_dp,
 }
 
 static void intel_enable_dp(struct intel_encoder *encoder,
-			    struct intel_crtc_state *pipe_config)
+			    struct intel_crtc_state *pipe_config,
+			    struct drm_connector_state *conn_state)
 {
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
 	struct drm_device *dev = encoder->base.dev;
@@ -2776,7 +2777,7 @@ static void intel_enable_dp(struct intel_encoder *encoder,
 	if (pipe_config->has_audio) {
 		DRM_DEBUG_DRIVER("Enabling DP audio on pipe %c\n",
 				 pipe_name(pipe));
-		intel_audio_codec_enable(encoder);
+		intel_audio_codec_enable(encoder, pipe_config, conn_state);
 	}
 }
 
@@ -2786,7 +2787,7 @@ static void g4x_enable_dp(struct intel_encoder *encoder,
 {
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
 
-	intel_enable_dp(encoder, pipe_config);
+	intel_enable_dp(encoder, pipe_config, conn_state);
 	intel_edp_backlight_on(intel_dp);
 }
 
@@ -2923,7 +2924,7 @@ static void vlv_pre_enable_dp(struct intel_encoder *encoder,
 {
 	vlv_phy_pre_encoder_enable(encoder);
 
-	intel_enable_dp(encoder, pipe_config);
+	intel_enable_dp(encoder, pipe_config, conn_state);
 }
 
 static void vlv_dp_pre_pll_enable(struct intel_encoder *encoder,
@@ -2941,7 +2942,7 @@ static void chv_pre_enable_dp(struct intel_encoder *encoder,
 {
 	chv_phy_pre_encoder_enable(encoder);
 
-	intel_enable_dp(encoder, pipe_config);
+	intel_enable_dp(encoder, pipe_config, conn_state);
 
 	/* Second common lane will stay alive on its own now */
 	chv_phy_release_cl2_override(encoder);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5760420ace61..e3c426563589 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1192,7 +1192,9 @@ u32 intel_fb_stride_alignment(const struct drm_i915_private *dev_priv,
 
 /* intel_audio.c */
 void intel_init_audio_hooks(struct drm_i915_private *dev_priv);
-void intel_audio_codec_enable(struct intel_encoder *encoder);
+void intel_audio_codec_enable(struct intel_encoder *encoder,
+			      const struct intel_crtc_state *crtc_state,
+			      const struct drm_connector_state *conn_state);
 void intel_audio_codec_disable(struct intel_encoder *encoder);
 void i915_audio_component_init(struct drm_i915_private *dev_priv);
 void i915_audio_component_cleanup(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index ef43244a884f..fd6439a2882b 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -984,7 +984,7 @@ static void intel_enable_hdmi_audio(struct intel_encoder *encoder,
 	WARN_ON(!crtc->config->has_hdmi_sink);
 	DRM_DEBUG_DRIVER("Enabling HDMI audio on pipe %c\n",
 			 pipe_name(crtc->pipe));
-	intel_audio_codec_enable(encoder);
+	intel_audio_codec_enable(encoder, pipe_config, conn_state);
 }
 
 static void g4x_enable_hdmi(struct intel_encoder *encoder,
-- 
2.7.4

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

[RESEND PATCH 3/6] drm/edid: Remove drm_select_eld

[Maarten Lankhorst]

The only user was i915, which is now gone.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_edid.c | 26 --------------------------
 include/drm/drm_edid.h     |  1 -
 2 files changed, 27 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 95de47ba1e77..fc81e0eb6abf 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3578,32 +3578,6 @@ int drm_av_sync_delay(struct drm_connector *connector,
 EXPORT_SYMBOL(drm_av_sync_delay);
 
 /**
- * drm_select_eld - select one ELD from multiple HDMI/DP sinks
- * @encoder: the encoder just changed display mode
- *
- * It's possible for one encoder to be associated with multiple HDMI/DP sinks.
- * The policy is now hard coded to simply use the first HDMI/DP sink's ELD.
- *
- * Return: The connector associated with the first HDMI/DP sink that has ELD
- * attached to it.
- */
-struct drm_connector *drm_select_eld(struct drm_encoder *encoder)
-{
-	struct drm_connector *connector;
-	struct drm_device *dev = encoder->dev;
-
-	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
-	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
-
-	drm_for_each_connector(connector, dev)
-		if (connector->encoder == encoder && connector->eld[0])
-			return connector;
-
-	return NULL;
-}
-EXPORT_SYMBOL(drm_select_eld);
-
-/**
  * drm_detect_hdmi_monitor - detect whether monitor is HDMI
  * @edid: monitor EDID information
  *
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index c3a7d440bc11..38eabf65f19d 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -330,7 +330,6 @@ int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads);
 int drm_edid_to_speaker_allocation(struct edid *edid, u8 **sadb);
 int drm_av_sync_delay(struct drm_connector *connector,
 		      const struct drm_display_mode *mode);
-struct drm_connector *drm_select_eld(struct drm_encoder *encoder);
 
 #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
 int drm_load_edid_firmware(struct drm_connector *connector);
-- 
2.7.4

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

[RESEND PATCH 4/6] drm/i915: Update atomic modeset state synchronously

[Maarten Lankhorst]

All of this state should be updated as soon as possible. It shouldn't be
done later because then future updates may not depend on it.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 69b9e91f071e..ba7f7be3aa4f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14341,14 +14341,8 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 
 	drm_atomic_helper_wait_for_dependencies(state);
 
-	if (intel_state->modeset) {
-		memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
-		       sizeof(intel_state->min_pixclk));
-		dev_priv->active_crtcs = intel_state->active_crtcs;
-		dev_priv->atomic_cdclk_freq = intel_state->cdclk;
-
+	if (intel_state->modeset)
 		intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
-	}
 
 	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
 		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -14558,6 +14552,13 @@ static int intel_atomic_commit(struct drm_device *dev,
 	intel_atomic_track_fbs(state);
 
 	drm_atomic_state_get(state);
+	if (intel_state->modeset) {
+		memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
+		       sizeof(intel_state->min_pixclk));
+		dev_priv->active_crtcs = intel_state->active_crtcs;
+		dev_priv->atomic_cdclk_freq = intel_state->cdclk;
+	}
+
 	if (nonblock)
 		queue_work(system_unbound_wq, &state->commit_work);
 	else
-- 
2.7.4

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

[RESEND PATCH 5/6] drm/i915: Pass atomic state to verify_connector_state

[Maarten Lankhorst]

This gets rid of a warning that the connectors are used without locking
when doing a nonblocking modeset.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ba7f7be3aa4f..54a4b2704179 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13501,11 +13501,15 @@ static void verify_wm_state(struct drm_crtc *crtc,
 }
 
 static void
-verify_connector_state(struct drm_device *dev, struct drm_crtc *crtc)
+verify_connector_state(struct drm_device *dev,
+		       struct drm_atomic_state *state,
+		       struct drm_crtc *crtc)
 {
 	struct drm_connector *connector;
+	struct drm_connector_state *old_conn_state;
+	int i;
 
-	drm_for_each_connector(connector, dev) {
+	for_each_connector_in_state(state, connector, old_conn_state, i) {
 		struct drm_encoder *encoder = connector->encoder;
 		struct drm_connector_state *state = connector->state;
 
@@ -13713,15 +13717,16 @@ verify_shared_dpll_state(struct drm_device *dev, struct drm_crtc *crtc,
 
 static void
 intel_modeset_verify_crtc(struct drm_crtc *crtc,
-			 struct drm_crtc_state *old_state,
-			 struct drm_crtc_state *new_state)
+			  struct drm_atomic_state *state,
+			  struct drm_crtc_state *old_state,
+			  struct drm_crtc_state *new_state)
 {
 	if (!needs_modeset(new_state) &&
 	    !to_intel_crtc_state(new_state)->update_pipe)
 		return;
 
 	verify_wm_state(crtc, new_state);
-	verify_connector_state(crtc->dev, crtc);
+	verify_connector_state(crtc->dev, state, crtc);
 	verify_crtc_state(crtc, old_state, new_state);
 	verify_shared_dpll_state(crtc->dev, crtc, old_state, new_state);
 }
@@ -13737,10 +13742,11 @@ verify_disabled_dpll_state(struct drm_device *dev)
 }
 
 static void
-intel_modeset_verify_disabled(struct drm_device *dev)
+intel_modeset_verify_disabled(struct drm_device *dev,
+			      struct drm_atomic_state *state)
 {
 	verify_encoder_state(dev);
-	verify_connector_state(dev, NULL);
+	verify_connector_state(dev, state, NULL);
 	verify_disabled_dpll_state(dev);
 }
 
@@ -14399,7 +14405,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		if (!intel_can_enable_sagv(state))
 			intel_disable_sagv(dev_priv);
 
-		intel_modeset_verify_disabled(dev);
+		intel_modeset_verify_disabled(dev, state);
 	}
 
 	/* Complete the events for pipes that have now been disabled */
@@ -14451,7 +14457,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		if (put_domains[i])
 			modeset_put_power_domains(dev_priv, put_domains[i]);
 
-		intel_modeset_verify_crtc(crtc, old_crtc_state, crtc->state);
+		intel_modeset_verify_crtc(crtc, state, old_crtc_state, crtc->state);
 	}
 
 	if (intel_state->modeset && intel_can_enable_sagv(state))
-- 
2.7.4

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

[RESEND PATCH 6/6] drm/i915: Enable support for nonblocking modeset

[Maarten Lankhorst]

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 54a4b2704179..523bb97c3bca 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14519,10 +14519,6 @@ static void intel_atomic_track_fbs(struct drm_atomic_state *state)
  * This function commits a top-level state object that has been validated
  * with drm_atomic_helper_check().
  *
- * FIXME:  Atomic modeset support for i915 is not yet complete.  At the moment
- * nonblocking commits are only safe for pure plane updates. Everything else
- * should work though.
- *
  * RETURNS
  * Zero for success or -errno.
  */
@@ -14534,11 +14530,6 @@ static int intel_atomic_commit(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	int ret = 0;
 
-	if (intel_state->modeset && nonblock) {
-		DRM_DEBUG_KMS("nonblocking commit for modeset not yet implemented.\n");
-		return -EINVAL;
-	}
-
 	ret = drm_atomic_helper_setup_commit(state, nonblock);
 	if (ret)
 		return ret;
-- 
2.7.4

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

✗ Fi.CI.BAT: failure for drm/i915: Enable support for nonblocking modeset.

[Patchwork]

== Series Details ==

Series: drm/i915: Enable support for nonblocking modeset.
URL   : https://patchwork.freedesktop.org/series/14026/
State : failure

== Summary ==

Series 14026v1 drm/i915: Enable support for nonblocking modeset.
https://patchwork.freedesktop.org/api/1.0/series/14026/revisions/1/mbox/

Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-legacy:
                pass       -> DMESG-WARN (fi-ilk-650)
Test kms_force_connector_basic:
        Subgroup force-load-detect:
                incomplete -> PASS       (fi-byt-j1900)
                pass       -> INCOMPLETE (fi-snb-2600)
Test kms_pipe_crc_basic:
        Subgroup bad-source:
                pass       -> DMESG-WARN (fi-ivb-3770)

fi-bdw-5557u     total:246  pass:231  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:246  pass:203  dwarn:1   dfail:0   fail:0   skip:42 
fi-bxt-t5700     total:246  pass:216  dwarn:0   dfail:0   fail:0   skip:30 
fi-byt-j1900     total:246  pass:213  dwarn:1   dfail:0   fail:1   skip:31 
fi-byt-n2820     total:246  pass:209  dwarn:1   dfail:0   fail:1   skip:35 
fi-hsw-4770      total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-hsw-4770r     total:246  pass:223  dwarn:1   dfail:0   fail:0   skip:22 
fi-ilk-650       total:246  pass:159  dwarn:25  dfail:0   fail:2   skip:60 
fi-ivb-3520m     total:246  pass:221  dwarn:0   dfail:0   fail:0   skip:25 
fi-ivb-3770      total:246  pass:220  dwarn:1   dfail:0   fail:0   skip:25 
fi-skl-6260u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:246  pass:219  dwarn:4   dfail:0   fail:0   skip:23 
fi-skl-6700k     total:246  pass:221  dwarn:1   dfail:0   fail:0   skip:24 
fi-skl-6770hq    total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:246  pass:210  dwarn:0   dfail:0   fail:0   skip:36 
fi-snb-2600      total:186  pass:158  dwarn:0   dfail:0   fail:0   skip:27 
fi-kbl-7200u failed to collect. IGT log at Patchwork_2763/fi-kbl-7200u/igt.log

Results at /archive/results/CI_IGT_test/Patchwork_2763/

21787266bd182df4c0d2067cf1b5c2379f61c24d drm-intel-nightly: 2016y-10m-19d-15h-39m-22s UTC integration manifest
c7a17a0 drm/i915: Enable support for nonblocking modeset
e2737d1 drm/i915: Pass atomic state to verify_connector_state
7755c7a drm/i915: Update atomic modeset state synchronously
0244ff6 drm/edid: Remove drm_select_eld
58fffa7 drm/i915: Pass atomic state to intel_audio_codec_enable
cdd07ea drm/i915: Convert intel_hdmi to use atomic state

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

Re: [RESEND PATCH 2/6] drm/i915: Pass atomic state to intel_audio_codec_enable

[Ville Syrjälä]

On Wed, Oct 19, 2016 at 03:55:35PM +0200, Maarten Lankhorst wrote:
> drm_select_eld requires mode_config.mutex and connection_mutex
> because it looks at the connector list and at the legacy encoders.
> 
> This is not required, because when we call audio_codec_enable we know
> which connector it was called for, so pass the state.
> 
> This also removes having to look at crtc->config.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_audio.c | 17 ++++++++++-------
>  drivers/gpu/drm/i915/intel_ddi.c   |  2 +-
>  drivers/gpu/drm/i915/intel_dp.c    | 11 ++++++-----
>  drivers/gpu/drm/i915/intel_drv.h   |  4 +++-
>  drivers/gpu/drm/i915/intel_hdmi.c  |  2 +-
>  5 files changed, 21 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 7093cfbb62b1..63ef25893c7e 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -485,23 +485,26 @@ static void ilk_audio_codec_enable(struct drm_connector *connector,
>  /**
>   * intel_audio_codec_enable - Enable the audio codec for HD audio
>   * @intel_encoder: encoder on which to enable audio
> + * @crtc_state: pointer to the current crtc state.
> + * @conn_state: pointer to the current connector state.
>   *
>   * The enable sequences may only be performed after enabling the transcoder and
>   * port, and after completed link training.
>   */
> -void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
> +void intel_audio_codec_enable(struct intel_encoder *intel_encoder,
> +			      const struct intel_crtc_state *crtc_state,
> +			      const struct drm_connector_state *conn_state)
>  {
>  	struct drm_encoder *encoder = &intel_encoder->base;
> -	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> -	const struct drm_display_mode *adjusted_mode = &crtc->config->base.adjusted_mode;
> +	const struct drm_display_mode *adjusted_mode = &crtc_state->base.adjusted_mode;
>  	struct drm_connector *connector;
>  	struct drm_i915_private *dev_priv = to_i915(encoder->dev);
>  	struct i915_audio_component *acomp = dev_priv->audio_component;
>  	enum port port = intel_encoder->port;
> -	enum pipe pipe = crtc->pipe;
> +	enum pipe pipe = drm_crtc_index(crtc_state->base.crtc);

While we may require that to be true, I'm not sure I like this use.

>  
> -	connector = drm_select_eld(encoder);
> -	if (!connector)
> +	connector = conn_state->connector;
> +	if (!connector || !connector->eld[0])
>  		return;
>  
>  	DRM_DEBUG_DRIVER("ELD on [CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
> @@ -512,7 +515,7 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
>  
>  	/* ELD Conn_Type */
>  	connector->eld[5] &= ~(3 << 2);
> -	if (intel_crtc_has_dp_encoder(crtc->config))
> +	if (intel_crtc_has_dp_encoder(crtc_state))
>  		connector->eld[5] |= (1 << 2);
>  
>  	connector->eld[6] = drm_av_sync_delay(connector, adjusted_mode) / 2;
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 7f7741c1406d..6748279310a4 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1824,7 +1824,7 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder,
>  
>  	if (intel_crtc->config->has_audio) {
>  		intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
> -		intel_audio_codec_enable(intel_encoder);
> +		intel_audio_codec_enable(intel_encoder, pipe_config, conn_state);
>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 88f3b745a326..6046f0f54cb3 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2734,7 +2734,8 @@ static void intel_dp_enable_port(struct intel_dp *intel_dp,
>  }
>  
>  static void intel_enable_dp(struct intel_encoder *encoder,
> -			    struct intel_crtc_state *pipe_config)
> +			    struct intel_crtc_state *pipe_config,
> +			    struct drm_connector_state *conn_state)
>  {
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
>  	struct drm_device *dev = encoder->base.dev;
> @@ -2776,7 +2777,7 @@ static void intel_enable_dp(struct intel_encoder *encoder,
>  	if (pipe_config->has_audio) {
>  		DRM_DEBUG_DRIVER("Enabling DP audio on pipe %c\n",
>  				 pipe_name(pipe));
> -		intel_audio_codec_enable(encoder);
> +		intel_audio_codec_enable(encoder, pipe_config, conn_state);
>  	}
>  }
>  
> @@ -2786,7 +2787,7 @@ static void g4x_enable_dp(struct intel_encoder *encoder,
>  {
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
>  
> -	intel_enable_dp(encoder, pipe_config);
> +	intel_enable_dp(encoder, pipe_config, conn_state);
>  	intel_edp_backlight_on(intel_dp);
>  }
>  
> @@ -2923,7 +2924,7 @@ static void vlv_pre_enable_dp(struct intel_encoder *encoder,
>  {
>  	vlv_phy_pre_encoder_enable(encoder);
>  
> -	intel_enable_dp(encoder, pipe_config);
> +	intel_enable_dp(encoder, pipe_config, conn_state);
>  }
>  
>  static void vlv_dp_pre_pll_enable(struct intel_encoder *encoder,
> @@ -2941,7 +2942,7 @@ static void chv_pre_enable_dp(struct intel_encoder *encoder,
>  {
>  	chv_phy_pre_encoder_enable(encoder);
>  
> -	intel_enable_dp(encoder, pipe_config);
> +	intel_enable_dp(encoder, pipe_config, conn_state);
>  
>  	/* Second common lane will stay alive on its own now */
>  	chv_phy_release_cl2_override(encoder);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 5760420ace61..e3c426563589 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1192,7 +1192,9 @@ u32 intel_fb_stride_alignment(const struct drm_i915_private *dev_priv,
>  
>  /* intel_audio.c */
>  void intel_init_audio_hooks(struct drm_i915_private *dev_priv);
> -void intel_audio_codec_enable(struct intel_encoder *encoder);
> +void intel_audio_codec_enable(struct intel_encoder *encoder,
> +			      const struct intel_crtc_state *crtc_state,
> +			      const struct drm_connector_state *conn_state);
>  void intel_audio_codec_disable(struct intel_encoder *encoder);
>  void i915_audio_component_init(struct drm_i915_private *dev_priv);
>  void i915_audio_component_cleanup(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index ef43244a884f..fd6439a2882b 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -984,7 +984,7 @@ static void intel_enable_hdmi_audio(struct intel_encoder *encoder,
>  	WARN_ON(!crtc->config->has_hdmi_sink);
>  	DRM_DEBUG_DRIVER("Enabling HDMI audio on pipe %c\n",
>  			 pipe_name(crtc->pipe));
> -	intel_audio_codec_enable(encoder);
> +	intel_audio_codec_enable(encoder, pipe_config, conn_state);
>  }
>  
>  static void g4x_enable_hdmi(struct intel_encoder *encoder,
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RESEND PATCH 3/6] drm/edid: Remove drm_select_eld

[Ville Syrjälä]

On Wed, Oct 19, 2016 at 03:55:36PM +0200, Maarten Lankhorst wrote:
> The only user was i915, which is now gone.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

cc: dri-devel missing

> ---
>  drivers/gpu/drm/drm_edid.c | 26 --------------------------
>  include/drm/drm_edid.h     |  1 -
>  2 files changed, 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 95de47ba1e77..fc81e0eb6abf 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3578,32 +3578,6 @@ int drm_av_sync_delay(struct drm_connector *connector,
>  EXPORT_SYMBOL(drm_av_sync_delay);
>  
>  /**
> - * drm_select_eld - select one ELD from multiple HDMI/DP sinks
> - * @encoder: the encoder just changed display mode
> - *
> - * It's possible for one encoder to be associated with multiple HDMI/DP sinks.
> - * The policy is now hard coded to simply use the first HDMI/DP sink's ELD.
> - *
> - * Return: The connector associated with the first HDMI/DP sink that has ELD
> - * attached to it.
> - */
> -struct drm_connector *drm_select_eld(struct drm_encoder *encoder)
> -{
> -	struct drm_connector *connector;
> -	struct drm_device *dev = encoder->dev;
> -
> -	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
> -	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> -
> -	drm_for_each_connector(connector, dev)
> -		if (connector->encoder == encoder && connector->eld[0])
> -			return connector;
> -
> -	return NULL;
> -}
> -EXPORT_SYMBOL(drm_select_eld);
> -
> -/**
>   * drm_detect_hdmi_monitor - detect whether monitor is HDMI
>   * @edid: monitor EDID information
>   *
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index c3a7d440bc11..38eabf65f19d 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -330,7 +330,6 @@ int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads);
>  int drm_edid_to_speaker_allocation(struct edid *edid, u8 **sadb);
>  int drm_av_sync_delay(struct drm_connector *connector,
>  		      const struct drm_display_mode *mode);
> -struct drm_connector *drm_select_eld(struct drm_encoder *encoder);
>  
>  #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
>  int drm_load_edid_firmware(struct drm_connector *connector);
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RESEND PATCH 4/6] drm/i915: Update atomic modeset state synchronously

[Ville Syrjälä]

On Wed, Oct 19, 2016 at 03:55:37PM +0200, Maarten Lankhorst wrote:
> All of this state should be updated as soon as possible. It shouldn't be
> done later because then future updates may not depend on it.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 69b9e91f071e..ba7f7be3aa4f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14341,14 +14341,8 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  
>  	drm_atomic_helper_wait_for_dependencies(state);
>  
> -	if (intel_state->modeset) {
> -		memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
> -		       sizeof(intel_state->min_pixclk));
> -		dev_priv->active_crtcs = intel_state->active_crtcs;
> -		dev_priv->atomic_cdclk_freq = intel_state->cdclk;
> -
> +	if (intel_state->modeset)
>  		intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
> -	}
>  
>  	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -14558,6 +14552,13 @@ static int intel_atomic_commit(struct drm_device *dev,
>  	intel_atomic_track_fbs(state);
>  
>  	drm_atomic_state_get(state);
> +	if (intel_state->modeset) {
> +		memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
> +		       sizeof(intel_state->min_pixclk));
> +		dev_priv->active_crtcs = intel_state->active_crtcs;
> +		dev_priv->atomic_cdclk_freq = intel_state->cdclk;
> +	}
> +

I'm not very happy about this whole intel_atomic_state <-> dev_priv
mess. I think what might be nicer is to have an intel_atomic_device_state
or something, which would be part of the top level atomic state just
like the other states, and we would just a pointer to the current
device state under dev_priv I suppose. This way the top level atomic
state would be more like an atomic transaction thing.

>  	if (nonblock)
>  		queue_work(system_unbound_wq, &state->commit_work);
>  	else
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RESEND PATCH 1/6] drm/i915: Convert intel_hdmi to use atomic state

[Ville Syrjälä]

On Wed, Oct 19, 2016 at 03:55:34PM +0200, Maarten Lankhorst wrote:
> This is the last connector still looking at crtc->config. Fix this.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

looks all right

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_hdmi.c | 48 +++++++++++++++++----------------------
>  1 file changed, 21 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 501334242d38..ef43244a884f 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -975,7 +975,9 @@ static void intel_hdmi_get_config(struct intel_encoder *encoder,
>  	pipe_config->lane_count = 4;
>  }
>  
> -static void intel_enable_hdmi_audio(struct intel_encoder *encoder)
> +static void intel_enable_hdmi_audio(struct intel_encoder *encoder,
> +				    struct intel_crtc_state *pipe_config,
> +				    struct drm_connector_state *conn_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
>  
> @@ -991,21 +993,20 @@ static void g4x_enable_hdmi(struct intel_encoder *encoder,
>  {
>  	struct drm_device *dev = encoder->base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
>  	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
>  	u32 temp;
>  
>  	temp = I915_READ(intel_hdmi->hdmi_reg);
>  
>  	temp |= SDVO_ENABLE;
> -	if (crtc->config->has_audio)
> +	if (pipe_config->has_audio)
>  		temp |= SDVO_AUDIO_ENABLE;
>  
>  	I915_WRITE(intel_hdmi->hdmi_reg, temp);
>  	POSTING_READ(intel_hdmi->hdmi_reg);
>  
> -	if (crtc->config->has_audio)
> -		intel_enable_hdmi_audio(encoder);
> +	if (pipe_config->has_audio)
> +		intel_enable_hdmi_audio(encoder, pipe_config, conn_state);
>  }
>  
>  static void ibx_enable_hdmi(struct intel_encoder *encoder,
> @@ -1040,8 +1041,8 @@ static void ibx_enable_hdmi(struct intel_encoder *encoder,
>  	 * FIXME: BSpec says this should be done at the end of
>  	 * of the modeset sequence, so not sure if this isn't too soon.
>  	 */
> -	if (crtc->config->pipe_bpp > 24 &&
> -	    crtc->config->pixel_multiplier > 1) {
> +	if (pipe_config->pipe_bpp > 24 &&
> +	    pipe_config->pixel_multiplier > 1) {
>  		I915_WRITE(intel_hdmi->hdmi_reg, temp & ~SDVO_ENABLE);
>  		POSTING_READ(intel_hdmi->hdmi_reg);
>  
> @@ -1055,8 +1056,8 @@ static void ibx_enable_hdmi(struct intel_encoder *encoder,
>  		POSTING_READ(intel_hdmi->hdmi_reg);
>  	}
>  
> -	if (crtc->config->has_audio)
> -		intel_enable_hdmi_audio(encoder);
> +	if (pipe_config->has_audio)
> +		intel_enable_hdmi_audio(encoder, pipe_config, conn_state);
>  }
>  
>  static void cpt_enable_hdmi(struct intel_encoder *encoder,
> @@ -1073,7 +1074,7 @@ static void cpt_enable_hdmi(struct intel_encoder *encoder,
>  	temp = I915_READ(intel_hdmi->hdmi_reg);
>  
>  	temp |= SDVO_ENABLE;
> -	if (crtc->config->has_audio)
> +	if (pipe_config->has_audio)
>  		temp |= SDVO_AUDIO_ENABLE;
>  
>  	/*
> @@ -1086,7 +1087,7 @@ static void cpt_enable_hdmi(struct intel_encoder *encoder,
>  	 * 4. enable HDMI clock gating
>  	 */
>  
> -	if (crtc->config->pipe_bpp > 24) {
> +	if (pipe_config->pipe_bpp > 24) {
>  		I915_WRITE(TRANS_CHICKEN1(pipe),
>  			   I915_READ(TRANS_CHICKEN1(pipe)) |
>  			   TRANS_CHICKEN1_HDMIUNIT_GC_DISABLE);
> @@ -1098,7 +1099,7 @@ static void cpt_enable_hdmi(struct intel_encoder *encoder,
>  	I915_WRITE(intel_hdmi->hdmi_reg, temp);
>  	POSTING_READ(intel_hdmi->hdmi_reg);
>  
> -	if (crtc->config->pipe_bpp > 24) {
> +	if (pipe_config->pipe_bpp > 24) {
>  		temp &= ~SDVO_COLOR_FORMAT_MASK;
>  		temp |= HDMI_COLOR_FORMAT_12bpc;
>  
> @@ -1110,8 +1111,8 @@ static void cpt_enable_hdmi(struct intel_encoder *encoder,
>  			   ~TRANS_CHICKEN1_HDMIUNIT_GC_DISABLE);
>  	}
>  
> -	if (crtc->config->has_audio)
> -		intel_enable_hdmi_audio(encoder);
> +	if (pipe_config->has_audio)
> +		intel_enable_hdmi_audio(encoder, pipe_config, conn_state);
>  }
>  
>  static void vlv_enable_hdmi(struct intel_encoder *encoder,
> @@ -1178,9 +1179,7 @@ static void g4x_disable_hdmi(struct intel_encoder *encoder,
>  			     struct intel_crtc_state *old_crtc_state,
>  			     struct drm_connector_state *old_conn_state)
>  {
> -	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
> -
> -	if (crtc->config->has_audio)
> +	if (old_crtc_state->has_audio)
>  		intel_audio_codec_disable(encoder);
>  
>  	intel_disable_hdmi(encoder, old_crtc_state, old_conn_state);
> @@ -1190,9 +1189,7 @@ static void pch_disable_hdmi(struct intel_encoder *encoder,
>  			     struct intel_crtc_state *old_crtc_state,
>  			     struct drm_connector_state *old_conn_state)
>  {
> -	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
> -
> -	if (crtc->config->has_audio)
> +	if (old_crtc_state->has_audio)
>  		intel_audio_codec_disable(encoder);
>  }
>  
> @@ -1645,13 +1642,12 @@ static void intel_hdmi_pre_enable(struct intel_encoder *encoder,
>  				  struct drm_connector_state *conn_state)
>  {
>  	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
> -	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> -	const struct drm_display_mode *adjusted_mode = &intel_crtc->config->base.adjusted_mode;
> +	const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
>  
>  	intel_hdmi_prepare(encoder);
>  
>  	intel_hdmi->set_infoframes(&encoder->base,
> -				   intel_crtc->config->has_hdmi_sink,
> +				   pipe_config->has_hdmi_sink,
>  				   adjusted_mode);
>  }
>  
> @@ -1663,9 +1659,7 @@ static void vlv_hdmi_pre_enable(struct intel_encoder *encoder,
>  	struct intel_hdmi *intel_hdmi = &dport->hdmi;
>  	struct drm_device *dev = encoder->base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct intel_crtc *intel_crtc =
> -		to_intel_crtc(encoder->base.crtc);
> -	const struct drm_display_mode *adjusted_mode = &intel_crtc->config->base.adjusted_mode;
> +	const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
>  
>  	vlv_phy_pre_encoder_enable(encoder);
>  
> @@ -1674,7 +1668,7 @@ static void vlv_hdmi_pre_enable(struct intel_encoder *encoder,
>  				 0x2b247878);
>  
>  	intel_hdmi->set_infoframes(&encoder->base,
> -				   intel_crtc->config->has_hdmi_sink,
> +				   pipe_config->has_hdmi_sink,
>  				   adjusted_mode);
>  
>  	g4x_enable_hdmi(encoder, pipe_config, conn_state);
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RESEND PATCH 2/6] drm/i915: Pass atomic state to intel_audio_codec_enable

[Ville Syrjälä]

On Fri, Oct 21, 2016 at 05:04:46PM +0300, Ville Syrjälä wrote:
> On Wed, Oct 19, 2016 at 03:55:35PM +0200, Maarten Lankhorst wrote:
> > drm_select_eld requires mode_config.mutex and connection_mutex
> > because it looks at the connector list and at the legacy encoders.
> > 
> > This is not required, because when we call audio_codec_enable we know
> > which connector it was called for, so pass the state.
> > 
> > This also removes having to look at crtc->config.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_audio.c | 17 ++++++++++-------
> >  drivers/gpu/drm/i915/intel_ddi.c   |  2 +-
> >  drivers/gpu/drm/i915/intel_dp.c    | 11 ++++++-----
> >  drivers/gpu/drm/i915/intel_drv.h   |  4 +++-
> >  drivers/gpu/drm/i915/intel_hdmi.c  |  2 +-
> >  5 files changed, 21 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > index 7093cfbb62b1..63ef25893c7e 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -485,23 +485,26 @@ static void ilk_audio_codec_enable(struct drm_connector *connector,
> >  /**
> >   * intel_audio_codec_enable - Enable the audio codec for HD audio
> >   * @intel_encoder: encoder on which to enable audio
> > + * @crtc_state: pointer to the current crtc state.
> > + * @conn_state: pointer to the current connector state.
> >   *
> >   * The enable sequences may only be performed after enabling the transcoder and
> >   * port, and after completed link training.
> >   */
> > -void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
> > +void intel_audio_codec_enable(struct intel_encoder *intel_encoder,
> > +			      const struct intel_crtc_state *crtc_state,
> > +			      const struct drm_connector_state *conn_state)
> >  {
> >  	struct drm_encoder *encoder = &intel_encoder->base;
> > -	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> > -	const struct drm_display_mode *adjusted_mode = &crtc->config->base.adjusted_mode;
> > +	const struct drm_display_mode *adjusted_mode = &crtc_state->base.adjusted_mode;
> >  	struct drm_connector *connector;
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->dev);
> >  	struct i915_audio_component *acomp = dev_priv->audio_component;
> >  	enum port port = intel_encoder->port;
> > -	enum pipe pipe = crtc->pipe;
> > +	enum pipe pipe = drm_crtc_index(crtc_state->base.crtc);
> 
> While we may require that to be true, I'm not sure I like this use.

I should say that otherwise I like this.

> 
> >  
> > -	connector = drm_select_eld(encoder);
> > -	if (!connector)
> > +	connector = conn_state->connector;
> > +	if (!connector || !connector->eld[0])
> >  		return;
> >  
> >  	DRM_DEBUG_DRIVER("ELD on [CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
> > @@ -512,7 +515,7 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
> >  
> >  	/* ELD Conn_Type */
> >  	connector->eld[5] &= ~(3 << 2);
> > -	if (intel_crtc_has_dp_encoder(crtc->config))
> > +	if (intel_crtc_has_dp_encoder(crtc_state))
> >  		connector->eld[5] |= (1 << 2);
> >  
> >  	connector->eld[6] = drm_av_sync_delay(connector, adjusted_mode) / 2;
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 7f7741c1406d..6748279310a4 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1824,7 +1824,7 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder,
> >  
> >  	if (intel_crtc->config->has_audio) {
> >  		intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
> > -		intel_audio_codec_enable(intel_encoder);
> > +		intel_audio_codec_enable(intel_encoder, pipe_config, conn_state);
> >  	}
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 88f3b745a326..6046f0f54cb3 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -2734,7 +2734,8 @@ static void intel_dp_enable_port(struct intel_dp *intel_dp,
> >  }
> >  
> >  static void intel_enable_dp(struct intel_encoder *encoder,
> > -			    struct intel_crtc_state *pipe_config)
> > +			    struct intel_crtc_state *pipe_config,
> > +			    struct drm_connector_state *conn_state)
> >  {
> >  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> >  	struct drm_device *dev = encoder->base.dev;
> > @@ -2776,7 +2777,7 @@ static void intel_enable_dp(struct intel_encoder *encoder,
> >  	if (pipe_config->has_audio) {
> >  		DRM_DEBUG_DRIVER("Enabling DP audio on pipe %c\n",
> >  				 pipe_name(pipe));
> > -		intel_audio_codec_enable(encoder);
> > +		intel_audio_codec_enable(encoder, pipe_config, conn_state);
> >  	}
> >  }
> >  
> > @@ -2786,7 +2787,7 @@ static void g4x_enable_dp(struct intel_encoder *encoder,
> >  {
> >  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> >  
> > -	intel_enable_dp(encoder, pipe_config);
> > +	intel_enable_dp(encoder, pipe_config, conn_state);
> >  	intel_edp_backlight_on(intel_dp);
> >  }
> >  
> > @@ -2923,7 +2924,7 @@ static void vlv_pre_enable_dp(struct intel_encoder *encoder,
> >  {
> >  	vlv_phy_pre_encoder_enable(encoder);
> >  
> > -	intel_enable_dp(encoder, pipe_config);
> > +	intel_enable_dp(encoder, pipe_config, conn_state);
> >  }
> >  
> >  static void vlv_dp_pre_pll_enable(struct intel_encoder *encoder,
> > @@ -2941,7 +2942,7 @@ static void chv_pre_enable_dp(struct intel_encoder *encoder,
> >  {
> >  	chv_phy_pre_encoder_enable(encoder);
> >  
> > -	intel_enable_dp(encoder, pipe_config);
> > +	intel_enable_dp(encoder, pipe_config, conn_state);
> >  
> >  	/* Second common lane will stay alive on its own now */
> >  	chv_phy_release_cl2_override(encoder);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 5760420ace61..e3c426563589 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1192,7 +1192,9 @@ u32 intel_fb_stride_alignment(const struct drm_i915_private *dev_priv,
> >  
> >  /* intel_audio.c */
> >  void intel_init_audio_hooks(struct drm_i915_private *dev_priv);
> > -void intel_audio_codec_enable(struct intel_encoder *encoder);
> > +void intel_audio_codec_enable(struct intel_encoder *encoder,
> > +			      const struct intel_crtc_state *crtc_state,
> > +			      const struct drm_connector_state *conn_state);
> >  void intel_audio_codec_disable(struct intel_encoder *encoder);
> >  void i915_audio_component_init(struct drm_i915_private *dev_priv);
> >  void i915_audio_component_cleanup(struct drm_i915_private *dev_priv);
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index ef43244a884f..fd6439a2882b 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -984,7 +984,7 @@ static void intel_enable_hdmi_audio(struct intel_encoder *encoder,
> >  	WARN_ON(!crtc->config->has_hdmi_sink);
> >  	DRM_DEBUG_DRIVER("Enabling HDMI audio on pipe %c\n",
> >  			 pipe_name(crtc->pipe));
> > -	intel_audio_codec_enable(encoder);
> > +	intel_audio_codec_enable(encoder, pipe_config, conn_state);
> >  }
> >  
> >  static void g4x_enable_hdmi(struct intel_encoder *encoder,
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RESEND PATCH 3/6] drm/edid: Remove drm_select_eld

[Ville Syrjälä]

On Fri, Oct 21, 2016 at 05:05:16PM +0300, Ville Syrjälä wrote:
> On Wed, Oct 19, 2016 at 03:55:36PM +0200, Maarten Lankhorst wrote:
> > The only user was i915, which is now gone.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> cc: dri-devel missing

Also
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> > ---
> >  drivers/gpu/drm/drm_edid.c | 26 --------------------------
> >  include/drm/drm_edid.h     |  1 -
> >  2 files changed, 27 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 95de47ba1e77..fc81e0eb6abf 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -3578,32 +3578,6 @@ int drm_av_sync_delay(struct drm_connector *connector,
> >  EXPORT_SYMBOL(drm_av_sync_delay);
> >  
> >  /**
> > - * drm_select_eld - select one ELD from multiple HDMI/DP sinks
> > - * @encoder: the encoder just changed display mode
> > - *
> > - * It's possible for one encoder to be associated with multiple HDMI/DP sinks.
> > - * The policy is now hard coded to simply use the first HDMI/DP sink's ELD.
> > - *
> > - * Return: The connector associated with the first HDMI/DP sink that has ELD
> > - * attached to it.
> > - */
> > -struct drm_connector *drm_select_eld(struct drm_encoder *encoder)
> > -{
> > -	struct drm_connector *connector;
> > -	struct drm_device *dev = encoder->dev;
> > -
> > -	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
> > -	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> > -
> > -	drm_for_each_connector(connector, dev)
> > -		if (connector->encoder == encoder && connector->eld[0])
> > -			return connector;
> > -
> > -	return NULL;
> > -}
> > -EXPORT_SYMBOL(drm_select_eld);
> > -
> > -/**
> >   * drm_detect_hdmi_monitor - detect whether monitor is HDMI
> >   * @edid: monitor EDID information
> >   *
> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> > index c3a7d440bc11..38eabf65f19d 100644
> > --- a/include/drm/drm_edid.h
> > +++ b/include/drm/drm_edid.h
> > @@ -330,7 +330,6 @@ int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads);
> >  int drm_edid_to_speaker_allocation(struct edid *edid, u8 **sadb);
> >  int drm_av_sync_delay(struct drm_connector *connector,
> >  		      const struct drm_display_mode *mode);
> > -struct drm_connector *drm_select_eld(struct drm_encoder *encoder);
> >  
> >  #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
> >  int drm_load_edid_firmware(struct drm_connector *connector);
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RESEND PATCH 4/6] drm/i915: Update atomic modeset state synchronously

[Ville Syrjälä]

On Wed, Oct 19, 2016 at 03:55:37PM +0200, Maarten Lankhorst wrote:
> All of this state should be updated as soon as possible. It shouldn't be
> done later because then future updates may not depend on it.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 69b9e91f071e..ba7f7be3aa4f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14341,14 +14341,8 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  
>  	drm_atomic_helper_wait_for_dependencies(state);
>  
> -	if (intel_state->modeset) {
> -		memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
> -		       sizeof(intel_state->min_pixclk));
> -		dev_priv->active_crtcs = intel_state->active_crtcs;
> -		dev_priv->atomic_cdclk_freq = intel_state->cdclk;
> -
> +	if (intel_state->modeset)
>  		intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
> -	}
>  
>  	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -14558,6 +14552,13 @@ static int intel_atomic_commit(struct drm_device *dev,
>  	intel_atomic_track_fbs(state);
>  
>  	drm_atomic_state_get(state);
> +	if (intel_state->modeset) {
> +		memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
> +		       sizeof(intel_state->min_pixclk));
> +		dev_priv->active_crtcs = intel_state->active_crtcs;
> +		dev_priv->atomic_cdclk_freq = intel_state->cdclk;
> +	}

The placement after the _get() looks a bit weird. The other parts of the
state were flipped over before the _get(), so putting this next to the
other would make it look less magicy.

> +
>  	if (nonblock)
>  		queue_work(system_unbound_wq, &state->commit_work);
>  	else
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RESEND PATCH 3/6] drm/edid: Remove drm_select_eld

[Maarten Lankhorst]

Op 21-10-16 om 16:17 schreef Ville Syrjälä:
> On Fri, Oct 21, 2016 at 05:05:16PM +0300, Ville Syrjälä wrote:
>> On Wed, Oct 19, 2016 at 03:55:36PM +0200, Maarten Lankhorst wrote:
>>> The only user was i915, which is now gone.
>>>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> cc: dri-devel missing
> Also
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Indeed, I'll ask for a ack from a drm maintainer.
>>> ---
>>>  drivers/gpu/drm/drm_edid.c | 26 --------------------------
>>>  include/drm/drm_edid.h     |  1 -
>>>  2 files changed, 27 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>> index 95de47ba1e77..fc81e0eb6abf 100644
>>> --- a/drivers/gpu/drm/drm_edid.c
>>> +++ b/drivers/gpu/drm/drm_edid.c
>>> @@ -3578,32 +3578,6 @@ int drm_av_sync_delay(struct drm_connector *connector,
>>>  EXPORT_SYMBOL(drm_av_sync_delay);
>>>  
>>>  /**
>>> - * drm_select_eld - select one ELD from multiple HDMI/DP sinks
>>> - * @encoder: the encoder just changed display mode
>>> - *
>>> - * It's possible for one encoder to be associated with multiple HDMI/DP sinks.
>>> - * The policy is now hard coded to simply use the first HDMI/DP sink's ELD.
>>> - *
>>> - * Return: The connector associated with the first HDMI/DP sink that has ELD
>>> - * attached to it.
>>> - */
>>> -struct drm_connector *drm_select_eld(struct drm_encoder *encoder)
>>> -{
>>> -	struct drm_connector *connector;
>>> -	struct drm_device *dev = encoder->dev;
>>> -
>>> -	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
>>> -	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>>> -
>>> -	drm_for_each_connector(connector, dev)
>>> -		if (connector->encoder == encoder && connector->eld[0])
>>> -			return connector;
>>> -
>>> -	return NULL;
>>> -}
>>> -EXPORT_SYMBOL(drm_select_eld);
>>> -
>>> -/**
>>>   * drm_detect_hdmi_monitor - detect whether monitor is HDMI
>>>   * @edid: monitor EDID information
>>>   *
>>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>>> index c3a7d440bc11..38eabf65f19d 100644
>>> --- a/include/drm/drm_edid.h
>>> +++ b/include/drm/drm_edid.h
>>> @@ -330,7 +330,6 @@ int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads);
>>>  int drm_edid_to_speaker_allocation(struct edid *edid, u8 **sadb);
>>>  int drm_av_sync_delay(struct drm_connector *connector,
>>>  		      const struct drm_display_mode *mode);
>>> -struct drm_connector *drm_select_eld(struct drm_encoder *encoder);
>>>  
>>>  #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
>>>  int drm_load_edid_firmware(struct drm_connector *connector);
>>> -- 
>>> 2.7.4
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> -- 
>> Ville Syrjälä
>> Intel OTC
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RESEND PATCH 2/6] drm/i915: Pass atomic state to intel_audio_codec_enable

[Maarten Lankhorst]

Op 21-10-16 om 16:16 schreef Ville Syrjälä:
> On Fri, Oct 21, 2016 at 05:04:46PM +0300, Ville Syrjälä wrote:
>> On Wed, Oct 19, 2016 at 03:55:35PM +0200, Maarten Lankhorst wrote:
>>> drm_select_eld requires mode_config.mutex and connection_mutex
>>> because it looks at the connector list and at the legacy encoders.
>>>
>>> This is not required, because when we call audio_codec_enable we know
>>> which connector it was called for, so pass the state.
>>>
>>> This also removes having to look at crtc->config.
>>>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_audio.c | 17 ++++++++++-------
>>>  drivers/gpu/drm/i915/intel_ddi.c   |  2 +-
>>>  drivers/gpu/drm/i915/intel_dp.c    | 11 ++++++-----
>>>  drivers/gpu/drm/i915/intel_drv.h   |  4 +++-
>>>  drivers/gpu/drm/i915/intel_hdmi.c  |  2 +-
>>>  5 files changed, 21 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
>>> index 7093cfbb62b1..63ef25893c7e 100644
>>> --- a/drivers/gpu/drm/i915/intel_audio.c
>>> +++ b/drivers/gpu/drm/i915/intel_audio.c
>>> @@ -485,23 +485,26 @@ static void ilk_audio_codec_enable(struct drm_connector *connector,
>>>  /**
>>>   * intel_audio_codec_enable - Enable the audio codec for HD audio
>>>   * @intel_encoder: encoder on which to enable audio
>>> + * @crtc_state: pointer to the current crtc state.
>>> + * @conn_state: pointer to the current connector state.
>>>   *
>>>   * The enable sequences may only be performed after enabling the transcoder and
>>>   * port, and after completed link training.
>>>   */
>>> -void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
>>> +void intel_audio_codec_enable(struct intel_encoder *intel_encoder,
>>> +			      const struct intel_crtc_state *crtc_state,
>>> +			      const struct drm_connector_state *conn_state)
>>>  {
>>>  	struct drm_encoder *encoder = &intel_encoder->base;
>>> -	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
>>> -	const struct drm_display_mode *adjusted_mode = &crtc->config->base.adjusted_mode;
>>> +	const struct drm_display_mode *adjusted_mode = &crtc_state->base.adjusted_mode;
>>>  	struct drm_connector *connector;
>>>  	struct drm_i915_private *dev_priv = to_i915(encoder->dev);
>>>  	struct i915_audio_component *acomp = dev_priv->audio_component;
>>>  	enum port port = intel_encoder->port;
>>> -	enum pipe pipe = crtc->pipe;
>>> +	enum pipe pipe = drm_crtc_index(crtc_state->base.crtc);
>> While we may require that to be true, I'm not sure I like this use.
> I should say that otherwise I like this.
What do you mean with this comment?

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

Re: [RESEND PATCH 4/6] drm/i915: Update atomic modeset state synchronously

[Maarten Lankhorst]

Op 21-10-16 om 16:21 schreef Ville Syrjälä:
> On Wed, Oct 19, 2016 at 03:55:37PM +0200, Maarten Lankhorst wrote:
>> All of this state should be updated as soon as possible. It shouldn't be
>> done later because then future updates may not depend on it.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 15 ++++++++-------
>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 69b9e91f071e..ba7f7be3aa4f 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -14341,14 +14341,8 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>>  
>>  	drm_atomic_helper_wait_for_dependencies(state);
>>  
>> -	if (intel_state->modeset) {
>> -		memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
>> -		       sizeof(intel_state->min_pixclk));
>> -		dev_priv->active_crtcs = intel_state->active_crtcs;
>> -		dev_priv->atomic_cdclk_freq = intel_state->cdclk;
>> -
>> +	if (intel_state->modeset)
>>  		intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
>> -	}
>>  
>>  	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
>>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> @@ -14558,6 +14552,13 @@ static int intel_atomic_commit(struct drm_device *dev,
>>  	intel_atomic_track_fbs(state);
>>  
>>  	drm_atomic_state_get(state);
>> +	if (intel_state->modeset) {
>> +		memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
>> +		       sizeof(intel_state->min_pixclk));
>> +		dev_priv->active_crtcs = intel_state->active_crtcs;
>> +		dev_priv->atomic_cdclk_freq = intel_state->cdclk;
>> +	}
> The placement after the _get() looks a bit weird. The other parts of the
> state were flipped over before the _get(), so putting this next to the
> other would make it look less magicy.
>
That's fine, this patch was rebased after thje drm_atomic_state_get was added, which explains the placement.

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

Re: [RESEND PATCH 2/6] drm/i915: Pass atomic state to intel_audio_codec_enable

[Ville Syrjälä]

On Mon, Oct 24, 2016 at 10:45:36AM +0200, Maarten Lankhorst wrote:
> Op 21-10-16 om 16:16 schreef Ville Syrjälä:
> > On Fri, Oct 21, 2016 at 05:04:46PM +0300, Ville Syrjälä wrote:
> >> On Wed, Oct 19, 2016 at 03:55:35PM +0200, Maarten Lankhorst wrote:
> >>> drm_select_eld requires mode_config.mutex and connection_mutex
> >>> because it looks at the connector list and at the legacy encoders.
> >>>
> >>> This is not required, because when we call audio_codec_enable we know
> >>> which connector it was called for, so pass the state.
> >>>
> >>> This also removes having to look at crtc->config.
> >>>
> >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>> ---
> >>>  drivers/gpu/drm/i915/intel_audio.c | 17 ++++++++++-------
> >>>  drivers/gpu/drm/i915/intel_ddi.c   |  2 +-
> >>>  drivers/gpu/drm/i915/intel_dp.c    | 11 ++++++-----
> >>>  drivers/gpu/drm/i915/intel_drv.h   |  4 +++-
> >>>  drivers/gpu/drm/i915/intel_hdmi.c  |  2 +-
> >>>  5 files changed, 21 insertions(+), 15 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> >>> index 7093cfbb62b1..63ef25893c7e 100644
> >>> --- a/drivers/gpu/drm/i915/intel_audio.c
> >>> +++ b/drivers/gpu/drm/i915/intel_audio.c
> >>> @@ -485,23 +485,26 @@ static void ilk_audio_codec_enable(struct drm_connector *connector,
> >>>  /**
> >>>   * intel_audio_codec_enable - Enable the audio codec for HD audio
> >>>   * @intel_encoder: encoder on which to enable audio
> >>> + * @crtc_state: pointer to the current crtc state.
> >>> + * @conn_state: pointer to the current connector state.
> >>>   *
> >>>   * The enable sequences may only be performed after enabling the transcoder and
> >>>   * port, and after completed link training.
> >>>   */
> >>> -void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
> >>> +void intel_audio_codec_enable(struct intel_encoder *intel_encoder,
> >>> +			      const struct intel_crtc_state *crtc_state,
> >>> +			      const struct drm_connector_state *conn_state)
> >>>  {
> >>>  	struct drm_encoder *encoder = &intel_encoder->base;
> >>> -	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> >>> -	const struct drm_display_mode *adjusted_mode = &crtc->config->base.adjusted_mode;
> >>> +	const struct drm_display_mode *adjusted_mode = &crtc_state->base.adjusted_mode;
> >>>  	struct drm_connector *connector;
> >>>  	struct drm_i915_private *dev_priv = to_i915(encoder->dev);
> >>>  	struct i915_audio_component *acomp = dev_priv->audio_component;
> >>>  	enum port port = intel_encoder->port;
> >>> -	enum pipe pipe = crtc->pipe;
> >>> +	enum pipe pipe = drm_crtc_index(crtc_state->base.crtc);
> >> While we may require that to be true, I'm not sure I like this use.
> > I should say that otherwise I like this.
> What do you mean with this comment?

That the rest of the patch makes sense.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RESEND PATCH 4/6] drm/i915: Update atomic modeset state synchronously

[Maarten Lankhorst]

Op 21-10-16 om 16:08 schreef Ville Syrjälä:
> On Wed, Oct 19, 2016 at 03:55:37PM +0200, Maarten Lankhorst wrote:
>> All of this state should be updated as soon as possible. It shouldn't be
>> done later because then future updates may not depend on it.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 15 ++++++++-------
>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 69b9e91f071e..ba7f7be3aa4f 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -14341,14 +14341,8 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>>  
>>  	drm_atomic_helper_wait_for_dependencies(state);
>>  
>> -	if (intel_state->modeset) {
>> -		memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
>> -		       sizeof(intel_state->min_pixclk));
>> -		dev_priv->active_crtcs = intel_state->active_crtcs;
>> -		dev_priv->atomic_cdclk_freq = intel_state->cdclk;
>> -
>> +	if (intel_state->modeset)
>>  		intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
>> -	}
>>  
>>  	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
>>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> @@ -14558,6 +14552,13 @@ static int intel_atomic_commit(struct drm_device *dev,
>>  	intel_atomic_track_fbs(state);
>>  
>>  	drm_atomic_state_get(state);
>> +	if (intel_state->modeset) {
>> +		memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
>> +		       sizeof(intel_state->min_pixclk));
>> +		dev_priv->active_crtcs = intel_state->active_crtcs;
>> +		dev_priv->atomic_cdclk_freq = intel_state->cdclk;
>> +	}
>> +
> I'm not very happy about this whole intel_atomic_state <-> dev_priv
> mess. I think what might be nicer is to have an intel_atomic_device_state
> or something, which would be part of the top level atomic state just
> like the other states, and we would just a pointer to the current
> device state under dev_priv I suppose. This way the top level atomic
> state would be more like an atomic transaction thing.
>
Neither, but I don't see a way to separate it cleanly. The updated members are too random to be put together in a struct.
And some are used read-only when !modeset, and others are not meant to be used at all in that case. Might even depend
on the platform.

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

Re: [RESEND PATCH 2/6] drm/i915: Pass atomic state to intel_audio_codec_enable

[Maarten Lankhorst]

Op 24-10-16 om 12:04 schreef Ville Syrjälä:
> On Mon, Oct 24, 2016 at 10:45:36AM +0200, Maarten Lankhorst wrote:
>> Op 21-10-16 om 16:16 schreef Ville Syrjälä:
>>> On Fri, Oct 21, 2016 at 05:04:46PM +0300, Ville Syrjälä wrote:
>>>> On Wed, Oct 19, 2016 at 03:55:35PM +0200, Maarten Lankhorst wrote:
>>>>> drm_select_eld requires mode_config.mutex and connection_mutex
>>>>> because it looks at the connector list and at the legacy encoders.
>>>>>
>>>>> This is not required, because when we call audio_codec_enable we know
>>>>> which connector it was called for, so pass the state.
>>>>>
>>>>> This also removes having to look at crtc->config.
>>>>>
>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>> ---
>>>>>  drivers/gpu/drm/i915/intel_audio.c | 17 ++++++++++-------
>>>>>  drivers/gpu/drm/i915/intel_ddi.c   |  2 +-
>>>>>  drivers/gpu/drm/i915/intel_dp.c    | 11 ++++++-----
>>>>>  drivers/gpu/drm/i915/intel_drv.h   |  4 +++-
>>>>>  drivers/gpu/drm/i915/intel_hdmi.c  |  2 +-
>>>>>  5 files changed, 21 insertions(+), 15 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
>>>>> index 7093cfbb62b1..63ef25893c7e 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_audio.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_audio.c
>>>>> @@ -485,23 +485,26 @@ static void ilk_audio_codec_enable(struct drm_connector *connector,
>>>>>  /**
>>>>>   * intel_audio_codec_enable - Enable the audio codec for HD audio
>>>>>   * @intel_encoder: encoder on which to enable audio
>>>>> + * @crtc_state: pointer to the current crtc state.
>>>>> + * @conn_state: pointer to the current connector state.
>>>>>   *
>>>>>   * The enable sequences may only be performed after enabling the transcoder and
>>>>>   * port, and after completed link training.
>>>>>   */
>>>>> -void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
>>>>> +void intel_audio_codec_enable(struct intel_encoder *intel_encoder,
>>>>> +			      const struct intel_crtc_state *crtc_state,
>>>>> +			      const struct drm_connector_state *conn_state)
>>>>>  {
>>>>>  	struct drm_encoder *encoder = &intel_encoder->base;
>>>>> -	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
>>>>> -	const struct drm_display_mode *adjusted_mode = &crtc->config->base.adjusted_mode;
>>>>> +	const struct drm_display_mode *adjusted_mode = &crtc_state->base.adjusted_mode;
>>>>>  	struct drm_connector *connector;
>>>>>  	struct drm_i915_private *dev_priv = to_i915(encoder->dev);
>>>>>  	struct i915_audio_component *acomp = dev_priv->audio_component;
>>>>>  	enum port port = intel_encoder->port;
>>>>> -	enum pipe pipe = crtc->pipe;
>>>>> +	enum pipe pipe = drm_crtc_index(crtc_state->base.crtc);
>>>> While we may require that to be true, I'm not sure I like this use.
>>> I should say that otherwise I like this.
>> What do you mean with this comment?
> That the rest of the patch makes sense.
>
Sorry, I meant the first comment you wrote.


~Maarten

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

Re: [RESEND PATCH 2/6] drm/i915: Pass atomic state to intel_audio_codec_enable

[Ville Syrjälä]

On Mon, Oct 24, 2016 at 12:12:59PM +0200, Maarten Lankhorst wrote:
> Op 24-10-16 om 12:04 schreef Ville Syrjälä:
> > On Mon, Oct 24, 2016 at 10:45:36AM +0200, Maarten Lankhorst wrote:
> >> Op 21-10-16 om 16:16 schreef Ville Syrjälä:
> >>> On Fri, Oct 21, 2016 at 05:04:46PM +0300, Ville Syrjälä wrote:
> >>>> On Wed, Oct 19, 2016 at 03:55:35PM +0200, Maarten Lankhorst wrote:
> >>>>> drm_select_eld requires mode_config.mutex and connection_mutex
> >>>>> because it looks at the connector list and at the legacy encoders.
> >>>>>
> >>>>> This is not required, because when we call audio_codec_enable we know
> >>>>> which connector it was called for, so pass the state.
> >>>>>
> >>>>> This also removes having to look at crtc->config.
> >>>>>
> >>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>>> ---
> >>>>>  drivers/gpu/drm/i915/intel_audio.c | 17 ++++++++++-------
> >>>>>  drivers/gpu/drm/i915/intel_ddi.c   |  2 +-
> >>>>>  drivers/gpu/drm/i915/intel_dp.c    | 11 ++++++-----
> >>>>>  drivers/gpu/drm/i915/intel_drv.h   |  4 +++-
> >>>>>  drivers/gpu/drm/i915/intel_hdmi.c  |  2 +-
> >>>>>  5 files changed, 21 insertions(+), 15 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> >>>>> index 7093cfbb62b1..63ef25893c7e 100644
> >>>>> --- a/drivers/gpu/drm/i915/intel_audio.c
> >>>>> +++ b/drivers/gpu/drm/i915/intel_audio.c
> >>>>> @@ -485,23 +485,26 @@ static void ilk_audio_codec_enable(struct drm_connector *connector,
> >>>>>  /**
> >>>>>   * intel_audio_codec_enable - Enable the audio codec for HD audio
> >>>>>   * @intel_encoder: encoder on which to enable audio
> >>>>> + * @crtc_state: pointer to the current crtc state.
> >>>>> + * @conn_state: pointer to the current connector state.
> >>>>>   *
> >>>>>   * The enable sequences may only be performed after enabling the transcoder and
> >>>>>   * port, and after completed link training.
> >>>>>   */
> >>>>> -void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
> >>>>> +void intel_audio_codec_enable(struct intel_encoder *intel_encoder,
> >>>>> +			      const struct intel_crtc_state *crtc_state,
> >>>>> +			      const struct drm_connector_state *conn_state)
> >>>>>  {
> >>>>>  	struct drm_encoder *encoder = &intel_encoder->base;
> >>>>> -	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> >>>>> -	const struct drm_display_mode *adjusted_mode = &crtc->config->base.adjusted_mode;
> >>>>> +	const struct drm_display_mode *adjusted_mode = &crtc_state->base.adjusted_mode;
> >>>>>  	struct drm_connector *connector;
> >>>>>  	struct drm_i915_private *dev_priv = to_i915(encoder->dev);
> >>>>>  	struct i915_audio_component *acomp = dev_priv->audio_component;
> >>>>>  	enum port port = intel_encoder->port;
> >>>>> -	enum pipe pipe = crtc->pipe;
> >>>>> +	enum pipe pipe = drm_crtc_index(crtc_state->base.crtc);
> >>>> While we may require that to be true, I'm not sure I like this use.
> >>> I should say that otherwise I like this.
> >> What do you mean with this comment?
> > That the rest of the patch makes sense.
> >
> Sorry, I meant the first comment you wrote.

I mean that 'enum pipe pipe = drm_crtc_index(crtc_state->base.crtc);'
is not something that's done anywhere else. So it looks totally foreign.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RESEND PATCH 4/6] drm/i915: Update atomic modeset state synchronously

[Ville Syrjälä]

On Mon, Oct 24, 2016 at 12:07:30PM +0200, Maarten Lankhorst wrote:
> Op 21-10-16 om 16:08 schreef Ville Syrjälä:
> > On Wed, Oct 19, 2016 at 03:55:37PM +0200, Maarten Lankhorst wrote:
> >> All of this state should be updated as soon as possible. It shouldn't be
> >> done later because then future updates may not depend on it.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_display.c | 15 ++++++++-------
> >>  1 file changed, 8 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 69b9e91f071e..ba7f7be3aa4f 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -14341,14 +14341,8 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> >>  
> >>  	drm_atomic_helper_wait_for_dependencies(state);
> >>  
> >> -	if (intel_state->modeset) {
> >> -		memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
> >> -		       sizeof(intel_state->min_pixclk));
> >> -		dev_priv->active_crtcs = intel_state->active_crtcs;
> >> -		dev_priv->atomic_cdclk_freq = intel_state->cdclk;
> >> -
> >> +	if (intel_state->modeset)
> >>  		intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
> >> -	}
> >>  
> >>  	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
> >>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >> @@ -14558,6 +14552,13 @@ static int intel_atomic_commit(struct drm_device *dev,
> >>  	intel_atomic_track_fbs(state);
> >>  
> >>  	drm_atomic_state_get(state);
> >> +	if (intel_state->modeset) {
> >> +		memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
> >> +		       sizeof(intel_state->min_pixclk));
> >> +		dev_priv->active_crtcs = intel_state->active_crtcs;
> >> +		dev_priv->atomic_cdclk_freq = intel_state->cdclk;
> >> +	}
> >> +
> > I'm not very happy about this whole intel_atomic_state <-> dev_priv
> > mess. I think what might be nicer is to have an intel_atomic_device_state
> > or something, which would be part of the top level atomic state just
> > like the other states, and we would just a pointer to the current
> > device state under dev_priv I suppose. This way the top level atomic
> > state would be more like an atomic transaction thing.
> >
> Neither, but I don't see a way to separate it cleanly. The updated members are too random to be put together in a struct.

What do you mean random? Just not related to each other. That doesn't
prohibit from collecting them up in a struct. They're already there
in intel_atomic_state after all.

> And some are used read-only when !modeset, and others are not meant to be used at all in that case. Might even depend
> on the platform.

How is one supposed to tell when they're to be used or not?

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RESEND PATCH 4/6] drm/i915: Update atomic modeset state synchronously

[Maarten Lankhorst]

Op 24-10-16 om 12:19 schreef Ville Syrjälä:
> On Mon, Oct 24, 2016 at 12:07:30PM +0200, Maarten Lankhorst wrote:
>> Op 21-10-16 om 16:08 schreef Ville Syrjälä:
>>> On Wed, Oct 19, 2016 at 03:55:37PM +0200, Maarten Lankhorst wrote:
>>>> All of this state should be updated as soon as possible. It shouldn't be
>>>> done later because then future updates may not depend on it.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_display.c | 15 ++++++++-------
>>>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index 69b9e91f071e..ba7f7be3aa4f 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -14341,14 +14341,8 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>>>>  
>>>>  	drm_atomic_helper_wait_for_dependencies(state);
>>>>  
>>>> -	if (intel_state->modeset) {
>>>> -		memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
>>>> -		       sizeof(intel_state->min_pixclk));
>>>> -		dev_priv->active_crtcs = intel_state->active_crtcs;
>>>> -		dev_priv->atomic_cdclk_freq = intel_state->cdclk;
>>>> -
>>>> +	if (intel_state->modeset)
>>>>  		intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
>>>> -	}
>>>>  
>>>>  	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
>>>>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>>> @@ -14558,6 +14552,13 @@ static int intel_atomic_commit(struct drm_device *dev,
>>>>  	intel_atomic_track_fbs(state);
>>>>  
>>>>  	drm_atomic_state_get(state);
>>>> +	if (intel_state->modeset) {
>>>> +		memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
>>>> +		       sizeof(intel_state->min_pixclk));
>>>> +		dev_priv->active_crtcs = intel_state->active_crtcs;
>>>> +		dev_priv->atomic_cdclk_freq = intel_state->cdclk;
>>>> +	}
>>>> +
>>> I'm not very happy about this whole intel_atomic_state <-> dev_priv
>>> mess. I think what might be nicer is to have an intel_atomic_device_state
>>> or something, which would be part of the top level atomic state just
>>> like the other states, and we would just a pointer to the current
>>> device state under dev_priv I suppose. This way the top level atomic
>>> state would be more like an atomic transaction thing.
>>>
>> Neither, but I don't see a way to separate it cleanly. The updated members are too random to be put together in a struct.
> What do you mean random? Just not related to each other. That doesn't
> prohibit from collecting them up in a struct. They're already there
> in intel_atomic_state after all.
>
>> And some are used read-only when !modeset, and others are not meant to be used at all in that case. Might even depend
>> on the platform.
> How is one supposed to tell when they're to be used or not?
Outside of modeset I think only intel_atomic_state->cdclk is valid. But only used by skl watermark sanitization. It uses a trick
in which intel_state->active_crtcs is valid too. I think that requirement can be removed though with some small changes. It
looks wrong anyway..

Some more looking shows that the wrong cdclk is used for scaler calculations with !modeset, only matters in the dpms off case.
But that needs fixing too..

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

Re: [RESEND PATCH 2/6] drm/i915: Pass atomic state to intel_audio_codec_enable

[Maarten Lankhorst]

Op 24-10-16 om 12:17 schreef Ville Syrjälä:
> On Mon, Oct 24, 2016 at 12:12:59PM +0200, Maarten Lankhorst wrote:
>> Op 24-10-16 om 12:04 schreef Ville Syrjälä:
>>> On Mon, Oct 24, 2016 at 10:45:36AM +0200, Maarten Lankhorst wrote:
>>>> Op 21-10-16 om 16:16 schreef Ville Syrjälä:
>>>>> On Fri, Oct 21, 2016 at 05:04:46PM +0300, Ville Syrjälä wrote:
>>>>>> On Wed, Oct 19, 2016 at 03:55:35PM +0200, Maarten Lankhorst wrote:
>>>>>>> drm_select_eld requires mode_config.mutex and connection_mutex
>>>>>>> because it looks at the connector list and at the legacy encoders.
>>>>>>>
>>>>>>> This is not required, because when we call audio_codec_enable we know
>>>>>>> which connector it was called for, so pass the state.
>>>>>>>
>>>>>>> This also removes having to look at crtc->config.
>>>>>>>
>>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>>> ---
>>>>>>>  drivers/gpu/drm/i915/intel_audio.c | 17 ++++++++++-------
>>>>>>>  drivers/gpu/drm/i915/intel_ddi.c   |  2 +-
>>>>>>>  drivers/gpu/drm/i915/intel_dp.c    | 11 ++++++-----
>>>>>>>  drivers/gpu/drm/i915/intel_drv.h   |  4 +++-
>>>>>>>  drivers/gpu/drm/i915/intel_hdmi.c  |  2 +-
>>>>>>>  5 files changed, 21 insertions(+), 15 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
>>>>>>> index 7093cfbb62b1..63ef25893c7e 100644
>>>>>>> --- a/drivers/gpu/drm/i915/intel_audio.c
>>>>>>> +++ b/drivers/gpu/drm/i915/intel_audio.c
>>>>>>> @@ -485,23 +485,26 @@ static void ilk_audio_codec_enable(struct drm_connector *connector,
>>>>>>>  /**
>>>>>>>   * intel_audio_codec_enable - Enable the audio codec for HD audio
>>>>>>>   * @intel_encoder: encoder on which to enable audio
>>>>>>> + * @crtc_state: pointer to the current crtc state.
>>>>>>> + * @conn_state: pointer to the current connector state.
>>>>>>>   *
>>>>>>>   * The enable sequences may only be performed after enabling the transcoder and
>>>>>>>   * port, and after completed link training.
>>>>>>>   */
>>>>>>> -void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
>>>>>>> +void intel_audio_codec_enable(struct intel_encoder *intel_encoder,
>>>>>>> +			      const struct intel_crtc_state *crtc_state,
>>>>>>> +			      const struct drm_connector_state *conn_state)
>>>>>>>  {
>>>>>>>  	struct drm_encoder *encoder = &intel_encoder->base;
>>>>>>> -	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
>>>>>>> -	const struct drm_display_mode *adjusted_mode = &crtc->config->base.adjusted_mode;
>>>>>>> +	const struct drm_display_mode *adjusted_mode = &crtc_state->base.adjusted_mode;
>>>>>>>  	struct drm_connector *connector;
>>>>>>>  	struct drm_i915_private *dev_priv = to_i915(encoder->dev);
>>>>>>>  	struct i915_audio_component *acomp = dev_priv->audio_component;
>>>>>>>  	enum port port = intel_encoder->port;
>>>>>>> -	enum pipe pipe = crtc->pipe;
>>>>>>> +	enum pipe pipe = drm_crtc_index(crtc_state->base.crtc);
>>>>>> While we may require that to be true, I'm not sure I like this use.
>>>>> I should say that otherwise I like this.
>>>> What do you mean with this comment?
>>> That the rest of the patch makes sense.
>>>
>> Sorry, I meant the first comment you wrote.
> I mean that 'enum pipe pipe = drm_crtc_index(crtc_state->base.crtc);'
> is not something that's done anywhere else. So it looks totally foreign.
>
Not directly I guess. Some places already assume that drm_crtc_index == pipe.
But it's ok when I change it to to_intel_crtc(crtc)->pipe?

~Maarten

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

Re: [RESEND PATCH 2/6] drm/i915: Pass atomic state to intel_audio_codec_enable

[Ville Syrjälä]

On Mon, Oct 24, 2016 at 12:47:21PM +0200, Maarten Lankhorst wrote:
> Op 24-10-16 om 12:17 schreef Ville Syrjälä:
> > On Mon, Oct 24, 2016 at 12:12:59PM +0200, Maarten Lankhorst wrote:
> >> Op 24-10-16 om 12:04 schreef Ville Syrjälä:
> >>> On Mon, Oct 24, 2016 at 10:45:36AM +0200, Maarten Lankhorst wrote:
> >>>> Op 21-10-16 om 16:16 schreef Ville Syrjälä:
> >>>>> On Fri, Oct 21, 2016 at 05:04:46PM +0300, Ville Syrjälä wrote:
> >>>>>> On Wed, Oct 19, 2016 at 03:55:35PM +0200, Maarten Lankhorst wrote:
> >>>>>>> drm_select_eld requires mode_config.mutex and connection_mutex
> >>>>>>> because it looks at the connector list and at the legacy encoders.
> >>>>>>>
> >>>>>>> This is not required, because when we call audio_codec_enable we know
> >>>>>>> which connector it was called for, so pass the state.
> >>>>>>>
> >>>>>>> This also removes having to look at crtc->config.
> >>>>>>>
> >>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>>>>> ---
> >>>>>>>  drivers/gpu/drm/i915/intel_audio.c | 17 ++++++++++-------
> >>>>>>>  drivers/gpu/drm/i915/intel_ddi.c   |  2 +-
> >>>>>>>  drivers/gpu/drm/i915/intel_dp.c    | 11 ++++++-----
> >>>>>>>  drivers/gpu/drm/i915/intel_drv.h   |  4 +++-
> >>>>>>>  drivers/gpu/drm/i915/intel_hdmi.c  |  2 +-
> >>>>>>>  5 files changed, 21 insertions(+), 15 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> >>>>>>> index 7093cfbb62b1..63ef25893c7e 100644
> >>>>>>> --- a/drivers/gpu/drm/i915/intel_audio.c
> >>>>>>> +++ b/drivers/gpu/drm/i915/intel_audio.c
> >>>>>>> @@ -485,23 +485,26 @@ static void ilk_audio_codec_enable(struct drm_connector *connector,
> >>>>>>>  /**
> >>>>>>>   * intel_audio_codec_enable - Enable the audio codec for HD audio
> >>>>>>>   * @intel_encoder: encoder on which to enable audio
> >>>>>>> + * @crtc_state: pointer to the current crtc state.
> >>>>>>> + * @conn_state: pointer to the current connector state.
> >>>>>>>   *
> >>>>>>>   * The enable sequences may only be performed after enabling the transcoder and
> >>>>>>>   * port, and after completed link training.
> >>>>>>>   */
> >>>>>>> -void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
> >>>>>>> +void intel_audio_codec_enable(struct intel_encoder *intel_encoder,
> >>>>>>> +			      const struct intel_crtc_state *crtc_state,
> >>>>>>> +			      const struct drm_connector_state *conn_state)
> >>>>>>>  {
> >>>>>>>  	struct drm_encoder *encoder = &intel_encoder->base;
> >>>>>>> -	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> >>>>>>> -	const struct drm_display_mode *adjusted_mode = &crtc->config->base.adjusted_mode;
> >>>>>>> +	const struct drm_display_mode *adjusted_mode = &crtc_state->base.adjusted_mode;
> >>>>>>>  	struct drm_connector *connector;
> >>>>>>>  	struct drm_i915_private *dev_priv = to_i915(encoder->dev);
> >>>>>>>  	struct i915_audio_component *acomp = dev_priv->audio_component;
> >>>>>>>  	enum port port = intel_encoder->port;
> >>>>>>> -	enum pipe pipe = crtc->pipe;
> >>>>>>> +	enum pipe pipe = drm_crtc_index(crtc_state->base.crtc);
> >>>>>> While we may require that to be true, I'm not sure I like this use.
> >>>>> I should say that otherwise I like this.
> >>>> What do you mean with this comment?
> >>> That the rest of the patch makes sense.
> >>>
> >> Sorry, I meant the first comment you wrote.
> > I mean that 'enum pipe pipe = drm_crtc_index(crtc_state->base.crtc);'
> > is not something that's done anywhere else. So it looks totally foreign.
> >
> Not directly I guess. Some places already assume that drm_crtc_index == pipe.
> But it's ok when I change it to to_intel_crtc(crtc)->pipe?

Yes.

If we wanted to, I guess we could just do

static inline enum pipe intel_crtc_pipe(crtc)
{
	return drm_crtc_index(&crtc->base);
}

and just nuke crtc->pipe entirely.

And then we get to hope that the hw people aren't going to allow fusing
off pipes in some random order (eg. just fuse off pipe B on a three pipe
platform). That would obviously break this scheme.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RESEND PATCH 2/6] drm/i915: Pass atomic state to intel_audio_codec_enable

[Maarten Lankhorst]

Op 24-10-16 om 13:41 schreef Ville Syrjälä:
> On Mon, Oct 24, 2016 at 12:47:21PM +0200, Maarten Lankhorst wrote:
>> Op 24-10-16 om 12:17 schreef Ville Syrjälä:
>>> On Mon, Oct 24, 2016 at 12:12:59PM +0200, Maarten Lankhorst wrote:
>>>> Op 24-10-16 om 12:04 schreef Ville Syrjälä:
>>>>> On Mon, Oct 24, 2016 at 10:45:36AM +0200, Maarten Lankhorst wrote:
>>>>>> Op 21-10-16 om 16:16 schreef Ville Syrjälä:
>>>>>>> On Fri, Oct 21, 2016 at 05:04:46PM +0300, Ville Syrjälä wrote:
>>>>>>>> On Wed, Oct 19, 2016 at 03:55:35PM +0200, Maarten Lankhorst wrote:
>>>>>>>>> drm_select_eld requires mode_config.mutex and connection_mutex
>>>>>>>>> because it looks at the connector list and at the legacy encoders.
>>>>>>>>>
>>>>>>>>> This is not required, because when we call audio_codec_enable we know
>>>>>>>>> which connector it was called for, so pass the state.
>>>>>>>>>
>>>>>>>>> This also removes having to look at crtc->config.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>>>>> ---
>>>>>>>>>  drivers/gpu/drm/i915/intel_audio.c | 17 ++++++++++-------
>>>>>>>>>  drivers/gpu/drm/i915/intel_ddi.c   |  2 +-
>>>>>>>>>  drivers/gpu/drm/i915/intel_dp.c    | 11 ++++++-----
>>>>>>>>>  drivers/gpu/drm/i915/intel_drv.h   |  4 +++-
>>>>>>>>>  drivers/gpu/drm/i915/intel_hdmi.c  |  2 +-
>>>>>>>>>  5 files changed, 21 insertions(+), 15 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
>>>>>>>>> index 7093cfbb62b1..63ef25893c7e 100644
>>>>>>>>> --- a/drivers/gpu/drm/i915/intel_audio.c
>>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_audio.c
>>>>>>>>> @@ -485,23 +485,26 @@ static void ilk_audio_codec_enable(struct drm_connector *connector,
>>>>>>>>>  /**
>>>>>>>>>   * intel_audio_codec_enable - Enable the audio codec for HD audio
>>>>>>>>>   * @intel_encoder: encoder on which to enable audio
>>>>>>>>> + * @crtc_state: pointer to the current crtc state.
>>>>>>>>> + * @conn_state: pointer to the current connector state.
>>>>>>>>>   *
>>>>>>>>>   * The enable sequences may only be performed after enabling the transcoder and
>>>>>>>>>   * port, and after completed link training.
>>>>>>>>>   */
>>>>>>>>> -void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
>>>>>>>>> +void intel_audio_codec_enable(struct intel_encoder *intel_encoder,
>>>>>>>>> +			      const struct intel_crtc_state *crtc_state,
>>>>>>>>> +			      const struct drm_connector_state *conn_state)
>>>>>>>>>  {
>>>>>>>>>  	struct drm_encoder *encoder = &intel_encoder->base;
>>>>>>>>> -	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
>>>>>>>>> -	const struct drm_display_mode *adjusted_mode = &crtc->config->base.adjusted_mode;
>>>>>>>>> +	const struct drm_display_mode *adjusted_mode = &crtc_state->base.adjusted_mode;
>>>>>>>>>  	struct drm_connector *connector;
>>>>>>>>>  	struct drm_i915_private *dev_priv = to_i915(encoder->dev);
>>>>>>>>>  	struct i915_audio_component *acomp = dev_priv->audio_component;
>>>>>>>>>  	enum port port = intel_encoder->port;
>>>>>>>>> -	enum pipe pipe = crtc->pipe;
>>>>>>>>> +	enum pipe pipe = drm_crtc_index(crtc_state->base.crtc);
>>>>>>>> While we may require that to be true, I'm not sure I like this use.
>>>>>>> I should say that otherwise I like this.
>>>>>> What do you mean with this comment?
>>>>> That the rest of the patch makes sense.
>>>>>
>>>> Sorry, I meant the first comment you wrote.
>>> I mean that 'enum pipe pipe = drm_crtc_index(crtc_state->base.crtc);'
>>> is not something that's done anywhere else. So it looks totally foreign.
>>>
>> Not directly I guess. Some places already assume that drm_crtc_index == pipe.
>> But it's ok when I change it to to_intel_crtc(crtc)->pipe?
> Yes.
>
> If we wanted to, I guess we could just do
>
> static inline enum pipe intel_crtc_pipe(crtc)
> {
> 	return drm_crtc_index(&crtc->base);
> }
>
> and just nuke crtc->pipe entirely.
>
> And then we get to hope that the hw people aren't going to allow fusing
> off pipes in some random order (eg. just fuse off pipe B on a three pipe
> platform). That would obviously break this scheme.
>
That would already cause subtle bugs right now. Lets hope it never happens. :)

~Maarten

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

Re: [RESEND PATCH 5/6] drm/i915: Pass atomic state to verify_connector_state

[Ville Syrjälä]

On Wed, Oct 19, 2016 at 03:55:38PM +0200, Maarten Lankhorst wrote:
> This gets rid of a warning that the connectors are used without locking
> when doing a nonblocking modeset.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ba7f7be3aa4f..54a4b2704179 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13501,11 +13501,15 @@ static void verify_wm_state(struct drm_crtc *crtc,
>  }
>  
>  static void
> -verify_connector_state(struct drm_device *dev, struct drm_crtc *crtc)
> +verify_connector_state(struct drm_device *dev,
> +		       struct drm_atomic_state *state,
> +		       struct drm_crtc *crtc)
>  {
>  	struct drm_connector *connector;
> +	struct drm_connector_state *old_conn_state;
> +	int i;
>  
> -	drm_for_each_connector(connector, dev) {
> +	for_each_connector_in_state(state, connector, old_conn_state, i) {
>  		struct drm_encoder *encoder = connector->encoder;
>  		struct drm_connector_state *state = connector->state;
>  
> @@ -13713,15 +13717,16 @@ verify_shared_dpll_state(struct drm_device *dev, struct drm_crtc *crtc,
>  
>  static void
>  intel_modeset_verify_crtc(struct drm_crtc *crtc,
> -			 struct drm_crtc_state *old_state,
> -			 struct drm_crtc_state *new_state)
> +			  struct drm_atomic_state *state,
> +			  struct drm_crtc_state *old_state,
> +			  struct drm_crtc_state *new_state)
>  {
>  	if (!needs_modeset(new_state) &&
>  	    !to_intel_crtc_state(new_state)->update_pipe)
>  		return;
>  
>  	verify_wm_state(crtc, new_state);
> -	verify_connector_state(crtc->dev, crtc);
> +	verify_connector_state(crtc->dev, state, crtc);
>  	verify_crtc_state(crtc, old_state, new_state);
>  	verify_shared_dpll_state(crtc->dev, crtc, old_state, new_state);
>  }
> @@ -13737,10 +13742,11 @@ verify_disabled_dpll_state(struct drm_device *dev)
>  }
>  
>  static void
> -intel_modeset_verify_disabled(struct drm_device *dev)
> +intel_modeset_verify_disabled(struct drm_device *dev,
> +			      struct drm_atomic_state *state)
>  {
>  	verify_encoder_state(dev);
> -	verify_connector_state(dev, NULL);
> +	verify_connector_state(dev, state, NULL);
>  	verify_disabled_dpll_state(dev);
>  }
>  
> @@ -14399,7 +14405,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  		if (!intel_can_enable_sagv(state))
>  			intel_disable_sagv(dev_priv);
>  
> -		intel_modeset_verify_disabled(dev);
> +		intel_modeset_verify_disabled(dev, state);
>  	}
>  
>  	/* Complete the events for pipes that have now been disabled */
> @@ -14451,7 +14457,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  		if (put_domains[i])
>  			modeset_put_power_domains(dev_priv, put_domains[i]);
>  
> -		intel_modeset_verify_crtc(crtc, old_crtc_state, crtc->state);
> +		intel_modeset_verify_crtc(crtc, state, old_crtc_state, crtc->state);
>  	}
>  
>  	if (intel_state->modeset && intel_can_enable_sagv(state))
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx