[PATCH 00/23] Remove depencies on staged config for atomic transition

[PATCH 00/23] Remove depencies on staged config for atomic transition

[Ander Conselvan de Oliveira]

This patch series starts to remove dependencies from the modeset code to
enable the transition to atomic. That is achieved by using an atomic
state struct for the legacy modeset, and changing related functiond to
depend on it.

I wasn't able to test all of the changes, so I'm very interested on
PRTS results for this. In particular, I expect support for 3 pipes and
the load detect pipe to be problematic.

  Ander

Ander Conselvan de Oliveira (23):
  drm/i915: Set crtc backpointer when duplicating crtc state
  drm/i915: Add a for_each_intel_connector macro
  drm/i915: Improve staged config logging
  drm/i915: Add intel_atomic_get_crtc_state() helper function
  drm/i915: Allocate a drm_atomic_state for the legacy modeset code
  drm/i915: Add an optional atomic state argument to intel_set_mode()
  drm/i915: Use an atomic state for the load detect modeset
  drm/i915: Allocate a crtc_state also when the crtc is being disabled
  drm/i915: Update dummy connector atomic state with current config
  drm/i915: Implement connector state duplication
  drm/i915: Copy the staged connector config to the legacy atomic state
  drm/i915: Don't use encoder->new_crtc in intel_modeset_pipe_config()
  drm/i915: Don't use encoder->new_crtc in compute_baseline_pipe_bpp()
  drm/i915: Don't depend on encoder->new_crtc in
    intel_dp_compute_config()
  drm/i915: Don't depend on encoder->new_crtc in
    intel_hdmi_compute_config
  drm/i915: Use atomic state in intel_ddi_crtc_get_new_encoder()
  drm/i915: Don't use staged config in intel_dp_mst_compute_config()
  drm/i915: Don't use encoder->new_crtc in intel_lvds_compute_config()
  drm/i915: Pass an atomic state to modeset_global_resources() functions
  drm/i915: Use atomic state in pipe_has_enabled_pch()
  drm/i915: Convert intel_pipe_will_have_type() to using atomic state
  drm/i915: Don't look at staged config crtc when changing DRRS state
  drm/i915: Remove usage of encoder->new_crtc from clock computations

 drivers/gpu/drm/i915/i915_debugfs.c  |   3 +-
 drivers/gpu/drm/i915/i915_drv.h      |  10 +-
 drivers/gpu/drm/i915/intel_atomic.c  |  12 +-
 drivers/gpu/drm/i915/intel_crt.c     |   3 +-
 drivers/gpu/drm/i915/intel_ddi.c     |  24 +-
 drivers/gpu/drm/i915/intel_display.c | 578 ++++++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_dp.c      |   5 +-
 drivers/gpu/drm/i915/intel_dp_mst.c  |  20 +-
 drivers/gpu/drm/i915/intel_drv.h     |  13 +-
 drivers/gpu/drm/i915/intel_dsi.c     |   1 +
 drivers/gpu/drm/i915/intel_dvo.c     |   1 +
 drivers/gpu/drm/i915/intel_hdmi.c    |  22 +-
 drivers/gpu/drm/i915/intel_lvds.c    |   3 +-
 drivers/gpu/drm/i915/intel_sdvo.c    |   1 +
 drivers/gpu/drm/i915/intel_tv.c      |   3 +-
 15 files changed, 492 insertions(+), 207 deletions(-)

-- 
2.1.0

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

[PATCH 01/23] drm/i915: Set crtc backpointer when duplicating crtc state

[Ander Conselvan de Oliveira]

In the path were there is no state to duplicate, the allocated crtc
state wouldn't have the crtc backpointer initialized.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_atomic.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 011b896..3903b90 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -214,12 +214,18 @@ struct drm_crtc_state *
 intel_crtc_duplicate_state(struct drm_crtc *crtc)
 {
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_crtc_state *crtc_state;
 
 	if (WARN_ON(!intel_crtc->config))
-		return kzalloc(sizeof(*intel_crtc->config), GFP_KERNEL);
+		crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL);
+	else
+		crtc_state = kmemdup(intel_crtc->config,
+				     sizeof(*intel_crtc->config), GFP_KERNEL);
 
-	return kmemdup(intel_crtc->config, sizeof(*intel_crtc->config),
-		       GFP_KERNEL);
+	if (crtc_state)
+		crtc_state->base.crtc = crtc;
+
+	return &crtc_state->base;
 }
 
 /**
-- 
2.1.0

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

Re: [PATCH 01/23] drm/i915: Set crtc backpointer when duplicating crtc state

[Daniel Vetter]

On Tue, Mar 03, 2015 at 03:21:55PM +0200, Ander Conselvan de Oliveira wrote:
> In the path were there is no state to duplicate, the allocated crtc
> state wouldn't have the crtc backpointer initialized.
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 011b896..3903b90 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -214,12 +214,18 @@ struct drm_crtc_state *
>  intel_crtc_duplicate_state(struct drm_crtc *crtc)
>  {
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_crtc_state *crtc_state;
>  
>  	if (WARN_ON(!intel_crtc->config))
> -		return kzalloc(sizeof(*intel_crtc->config), GFP_KERNEL);
> +		crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL);
> +	else
> +		crtc_state = kmemdup(intel_crtc->config,
> +				     sizeof(*intel_crtc->config), GFP_KERNEL);
>  
> -	return kmemdup(intel_crtc->config, sizeof(*intel_crtc->config),
> -		       GFP_KERNEL);
> +	if (crtc_state)
> +		crtc_state->base.crtc = crtc;
> +
> +	return &crtc_state->base;

I think we should eventually extract another set of helpers to share the
core initialization code common with all drivers and the atomic helpers.
We've already screwed up this a few times when additions to the helper
state tracking haven't propagated to all drivers.

Last time I've tried this I got stuck on a good name for this function
though.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 02/23] drm/i915: Add a for_each_intel_connector macro

[Ander Conselvan de Oliveira]

We have similar macros for crtcs and encoders, and the pattern happens
often enough to justify the macro.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |  3 +-
 drivers/gpu/drm/i915/i915_drv.h      |  6 ++++
 drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++------------------------
 drivers/gpu/drm/i915/intel_dp_mst.c  |  4 +--
 4 files changed, 27 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 6717052..f9b5a97 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2308,8 +2308,7 @@ static int i915_sink_crc(struct seq_file *m, void *data)
 	u8 crc[6];
 
 	drm_modeset_lock_all(dev);
-	list_for_each_entry(connector, &dev->mode_config.connector_list,
-			    base.head) {
+	for_each_intel_encoder(dev, connector) {
 
 		if (connector->base.dpms != DRM_MODE_DPMS_ON)
 			continue;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 92f8300..29c6fbc 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -243,6 +243,12 @@ enum hpd_pin {
 			    &(dev)->mode_config.encoder_list,	\
 			    base.head)
 
+#define for_each_intel_connector(dev, intel_connector)		\
+	list_for_each_entry(intel_connector,			\
+			    &dev->mode_config.connector_list,	\
+			    base.head)
+
+
 #define for_each_encoder_on_crtc(dev, __crtc, intel_encoder) \
 	list_for_each_entry((intel_encoder), &(dev)->mode_config.encoder_list, base.head) \
 		if ((intel_encoder)->base.crtc == (__crtc))
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 589addf..3417e17 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10013,8 +10013,7 @@ static void intel_modeset_update_staged_output_state(struct drm_device *dev)
 	struct intel_encoder *encoder;
 	struct intel_connector *connector;
 
-	list_for_each_entry(connector, &dev->mode_config.connector_list,
-			    base.head) {
+	for_each_intel_connector(dev, connector) {
 		connector->new_encoder =
 			to_intel_encoder(connector->base.encoder);
 	}
@@ -10045,8 +10044,7 @@ static void intel_modeset_commit_output_state(struct drm_device *dev)
 	struct intel_encoder *encoder;
 	struct intel_connector *connector;
 
-	list_for_each_entry(connector, &dev->mode_config.connector_list,
-			    base.head) {
+	for_each_intel_connector(dev, connector) {
 		connector->base.encoder = &connector->new_encoder->base;
 	}
 
@@ -10134,8 +10132,7 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc,
 	pipe_config->pipe_bpp = bpp;
 
 	/* Clamp display bpp to EDID value */
-	list_for_each_entry(connector, &dev->mode_config.connector_list,
-			    base.head) {
+	for_each_intel_connector(dev, connector) {
 		if (!connector->new_encoder ||
 		    connector->new_encoder->new_crtc != crtc)
 			continue;
@@ -10262,8 +10259,7 @@ static bool check_digital_port_conflicts(struct drm_device *dev)
 	 * list to detect the problem on ddi platforms
 	 * where there's just one encoder per digital port.
 	 */
-	list_for_each_entry(connector,
-			    &dev->mode_config.connector_list, base.head) {
+	for_each_intel_connector(dev, connector) {
 		struct intel_encoder *encoder = connector->new_encoder;
 
 		if (!encoder)
@@ -10436,8 +10432,7 @@ intel_modeset_affected_pipes(struct drm_crtc *crtc, unsigned *modeset_pipes,
 	 * to be part of the prepare_pipes mask. We don't (yet) support global
 	 * modeset across multiple crtcs, so modeset_pipes will only have one
 	 * bit set at most. */
-	list_for_each_entry(connector, &dev->mode_config.connector_list,
-			    base.head) {
+	for_each_intel_connector(dev, connector) {
 		if (connector->base.encoder == &connector->new_encoder->base)
 			continue;
 
@@ -10840,8 +10835,7 @@ check_connector_state(struct drm_device *dev)
 {
 	struct intel_connector *connector;
 
-	list_for_each_entry(connector, &dev->mode_config.connector_list,
-			    base.head) {
+	for_each_intel_connector(dev, connector) {
 		/* This also checks the encoder/connector hw state with the
 		 * ->get_hw_state callbacks. */
 		intel_connector_check_state(connector);
@@ -10871,8 +10865,7 @@ check_encoder_state(struct drm_device *dev)
 		I915_STATE_WARN(encoder->connectors_active && !encoder->base.crtc,
 		     "encoder's active_connectors set, but no crtc\n");
 
-		list_for_each_entry(connector, &dev->mode_config.connector_list,
-				    base.head) {
+		for_each_intel_connector(dev, connector) {
 			if (connector->base.encoder != &encoder->base)
 				continue;
 			enabled = true;
@@ -11393,7 +11386,7 @@ static void intel_set_config_restore_state(struct drm_device *dev,
 	}
 
 	count = 0;
-	list_for_each_entry(connector, &dev->mode_config.connector_list, base.head) {
+	for_each_intel_connector(dev, connector) {
 		connector->new_encoder =
 			to_intel_encoder(config->save_connector_encoders[count++]);
 	}
@@ -11485,8 +11478,7 @@ intel_modeset_stage_output_state(struct drm_device *dev,
 	WARN_ON(!set->fb && (set->num_connectors != 0));
 	WARN_ON(set->fb && (set->num_connectors == 0));
 
-	list_for_each_entry(connector, &dev->mode_config.connector_list,
-			    base.head) {
+	for_each_intel_connector(dev, connector) {
 		/* Otherwise traverse passed in connector list and get encoders
 		 * for them. */
 		for (ro = 0; ro < set->num_connectors; ro++) {
@@ -11518,8 +11510,7 @@ intel_modeset_stage_output_state(struct drm_device *dev,
 	/* connector->new_encoder is now updated for all connectors. */
 
 	/* Update crtc of enabled connectors. */
-	list_for_each_entry(connector, &dev->mode_config.connector_list,
-			    base.head) {
+	for_each_intel_connector(dev, connector) {
 		struct drm_crtc *new_crtc;
 
 		if (!connector->new_encoder)
@@ -11548,9 +11539,7 @@ intel_modeset_stage_output_state(struct drm_device *dev,
 	/* Check for any encoders that needs to be disabled. */
 	for_each_intel_encoder(dev, encoder) {
 		int num_connectors = 0;
-		list_for_each_entry(connector,
-				    &dev->mode_config.connector_list,
-				    base.head) {
+		for_each_intel_connector(dev, connector) {
 			if (connector->new_encoder == encoder) {
 				WARN_ON(!connector->new_encoder->new_crtc);
 				num_connectors++;
@@ -11570,8 +11559,7 @@ intel_modeset_stage_output_state(struct drm_device *dev,
 		}
 	}
 	/* Now we've also updated encoder->new_crtc for all encoders. */
-	list_for_each_entry(connector, &dev->mode_config.connector_list,
-			    base.head) {
+	for_each_intel_connector(dev, connector) {
 		if (connector->new_encoder)
 			if (connector->new_encoder != connector->encoder)
 				connector->encoder = connector->new_encoder;
@@ -11610,7 +11598,7 @@ static void disable_crtc_nofb(struct intel_crtc *crtc)
 	DRM_DEBUG_KMS("Trying to restore without FB -> disabling pipe %c\n",
 		      pipe_name(crtc->pipe));
 
-	list_for_each_entry(connector, &dev->mode_config.connector_list, base.head) {
+	for_each_intel_connector(dev, connector) {
 		if (connector->new_encoder &&
 		    connector->new_encoder->new_crtc == crtc)
 			connector->new_encoder = NULL;
@@ -13416,9 +13404,7 @@ static void intel_enable_pipe_a(struct drm_device *dev)
 	/* We can't just switch on the pipe A, we need to set things up with a
 	 * proper mode and output configuration. As a gross hack, enable pipe A
 	 * by enabling the load detect pipe once. */
-	list_for_each_entry(connector,
-			    &dev->mode_config.connector_list,
-			    base.head) {
+	for_each_intel_connector(dev, connector) {
 		if (connector->encoder->type == INTEL_OUTPUT_ANALOG) {
 			crt = &connector->base;
 			break;
@@ -13489,8 +13475,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 		crtc->plane = plane;
 
 		/* ... and break all links. */
-		list_for_each_entry(connector, &dev->mode_config.connector_list,
-				    base.head) {
+		for_each_intel_connector(dev, connector) {
 			if (connector->encoder->base.crtc != &crtc->base)
 				continue;
 
@@ -13499,8 +13484,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 		}
 		/* multiple connectors may have the same encoder:
 		 *  handle them and break crtc link separately */
-		list_for_each_entry(connector, &dev->mode_config.connector_list,
-				    base.head)
+		for_each_intel_connector(dev, connector)
 			if (connector->encoder->base.crtc == &crtc->base) {
 				connector->encoder->base.crtc = NULL;
 				connector->encoder->connectors_active = false;
@@ -13604,9 +13588,7 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
 		 * a bug in one of the get_hw_state functions. Or someplace else
 		 * in our code, like the register restore mess on resume. Clamp
 		 * things to off as a safer default. */
-		list_for_each_entry(connector,
-				    &dev->mode_config.connector_list,
-				    base.head) {
+		for_each_intel_connector(dev, connector) {
 			if (connector->encoder != encoder)
 				continue;
 			connector->base.dpms = DRM_MODE_DPMS_OFF;
@@ -13721,8 +13703,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 			      pipe_name(pipe));
 	}
 
-	list_for_each_entry(connector, &dev->mode_config.connector_list,
-			    base.head) {
+	for_each_intel_connector(dev, connector) {
 		if (connector->get_hw_state(connector)) {
 			connector->base.dpms = DRM_MODE_DPMS_ON;
 			connector->encoder->connectors_active = true;
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 9f67a37..be12492 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -58,7 +58,7 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 	pipe_config->pipe_bpp = 24;
 	pipe_config->port_clock = drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
 
-	list_for_each_entry(intel_connector, &dev->mode_config.connector_list, base.head) {
+	for_each_intel_connector(dev, intel_connector) {
 		if (intel_connector->new_encoder == encoder) {
 			found = intel_connector;
 			break;
@@ -140,7 +140,7 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder)
 	struct drm_crtc *crtc = encoder->base.crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
-	list_for_each_entry(intel_connector, &dev->mode_config.connector_list, base.head) {
+	for_each_intel_connector(dev, intel_connector) {
 		if (intel_connector->new_encoder == encoder) {
 			found = intel_connector;
 			break;
-- 
2.1.0

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

[PATCH 03/23] drm/i915: Improve staged config logging

[Ander Conselvan de Oliveira]

When logging that full mode switch is necessary, log which connector,
encoder or crtc has caused it, so it is easier to figure out what is
goind on by just looking at the log.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3417e17..798de7b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11503,7 +11503,9 @@ intel_modeset_stage_output_state(struct drm_device *dev,
 
 
 		if (&connector->new_encoder->base != connector->base.encoder) {
-			DRM_DEBUG_KMS("encoder changed, full mode switch\n");
+			DRM_DEBUG_KMS("[CONNECTOR:%d:%s] encoder changed, full mode switch\n",
+				      connector->base.base.id,
+				      connector->base.name);
 			config->mode_changed = true;
 		}
 	}
@@ -11554,7 +11556,9 @@ intel_modeset_stage_output_state(struct drm_device *dev,
 		/* Only now check for crtc changes so we don't miss encoders
 		 * that will be disabled. */
 		if (&encoder->new_crtc->base != encoder->base.crtc) {
-			DRM_DEBUG_KMS("crtc changed, full mode switch\n");
+			DRM_DEBUG_KMS("[ENCODER:%d:%s] crtc changed, full mode switch\n",
+				      encoder->base.base.id,
+				      encoder->base.name);
 			config->mode_changed = true;
 		}
 	}
@@ -11575,7 +11579,8 @@ intel_modeset_stage_output_state(struct drm_device *dev,
 		}
 
 		if (crtc->new_enabled != crtc->base.state->enable) {
-			DRM_DEBUG_KMS("crtc %sabled, full mode switch\n",
+			DRM_DEBUG_KMS("[CRTC:%d] %sabled, full mode switch\n",
+				      crtc->base.base.id,
 				      crtc->new_enabled ? "en" : "dis");
 			config->mode_changed = true;
 		}
-- 
2.1.0

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

[PATCH 04/23] drm/i915: Add intel_atomic_get_crtc_state() helper function

[Ander Conselvan de Oliveira]

The pattern of getting the crtc state with drm_atomic_get_crtc_state()
and then converting it to intel_crtc_state will repeat quite often in
the following patches, so add a helper function to save some typing.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 632df1c..c1959e0 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -35,6 +35,7 @@
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_dp_mst_helper.h>
 #include <drm/drm_rect.h>
+#include <drm/drm_atomic.h>
 
 #define DIV_ROUND_CLOSEST_ULL(ll, d)	\
 ({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; })
@@ -564,6 +565,7 @@ struct cxsr_latency {
 };
 
 #define to_intel_crtc(x) container_of(x, struct intel_crtc, base)
+#define to_intel_crtc_state(x) container_of(x, struct intel_crtc_state, base)
 #define to_intel_connector(x) container_of(x, struct intel_connector, base)
 #define to_intel_encoder(x) container_of(x, struct intel_encoder, base)
 #define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base)
@@ -1273,6 +1275,14 @@ int intel_connector_atomic_get_property(struct drm_connector *connector,
 struct drm_crtc_state *intel_crtc_duplicate_state(struct drm_crtc *crtc);
 void intel_crtc_destroy_state(struct drm_crtc *crtc,
 			       struct drm_crtc_state *state);
+static inline struct intel_crtc_state *
+intel_atomic_get_crtc_state(struct drm_atomic_state *state,
+			    struct intel_crtc *crtc)
+{
+	struct drm_crtc_state *crtc_state;
+	crtc_state = drm_atomic_get_crtc_state(state, &crtc->base);
+	return to_intel_crtc_state(crtc_state);
+}
 
 /* intel_atomic_plane.c */
 struct intel_plane_state *intel_create_plane_state(struct drm_plane *plane);
-- 
2.1.0

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

Re: [PATCH 04/23] drm/i915: Add intel_atomic_get_crtc_state() helper function

[Daniel Vetter]

On Tue, Mar 03, 2015 at 03:21:58PM +0200, Ander Conselvan de Oliveira wrote:
> The pattern of getting the crtc state with drm_atomic_get_crtc_state()
> and then converting it to intel_crtc_state will repeat quite often in
> the following patches, so add a helper function to save some typing.
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 632df1c..c1959e0 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -35,6 +35,7 @@
>  #include <drm/drm_fb_helper.h>
>  #include <drm/drm_dp_mst_helper.h>
>  #include <drm/drm_rect.h>
> +#include <drm/drm_atomic.h>
>  
>  #define DIV_ROUND_CLOSEST_ULL(ll, d)	\
>  ({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; })
> @@ -564,6 +565,7 @@ struct cxsr_latency {
>  };
>  
>  #define to_intel_crtc(x) container_of(x, struct intel_crtc, base)
> +#define to_intel_crtc_state(x) container_of(x, struct intel_crtc_state, base)
>  #define to_intel_connector(x) container_of(x, struct intel_connector, base)
>  #define to_intel_encoder(x) container_of(x, struct intel_encoder, base)
>  #define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base)
> @@ -1273,6 +1275,14 @@ int intel_connector_atomic_get_property(struct drm_connector *connector,
>  struct drm_crtc_state *intel_crtc_duplicate_state(struct drm_crtc *crtc);
>  void intel_crtc_destroy_state(struct drm_crtc *crtc,
>  			       struct drm_crtc_state *state);
> +static inline struct intel_crtc_state *
> +intel_atomic_get_crtc_state(struct drm_atomic_state *state,
> +			    struct intel_crtc *crtc)
> +{
> +	struct drm_crtc_state *crtc_state;
> +	crtc_state = drm_atomic_get_crtc_state(state, &crtc->base);
> +	return to_intel_crtc_state(crtc_state);

You need to check for error pointers before upcasting. Just to be robust
against someone putting the base structure not as the first thing.

I've merged all previous patches thus far, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 05/23] drm/i915: Allocate a drm_atomic_state for the legacy modeset code

[Ander Conselvan de Oliveira]

For the atomic conversion, the mode set paths need to be changed to rely
on an atomic state instead of using the staged config. By using an
atomic state for the legacy code, we will be able to convert the code
base in small chunks.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 118 +++++++++++++++++++++++++++--------
 1 file changed, 91 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 798de7b..97d4df5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -37,6 +37,7 @@
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 #include "i915_trace.h"
+#include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_dp_helper.h>
 #include <drm/drm_crtc_helper.h>
@@ -10290,10 +10291,22 @@ static bool check_digital_port_conflicts(struct drm_device *dev)
 	return true;
 }
 
-static struct intel_crtc_state *
+static void
+clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
+{
+	struct drm_crtc_state tmp_state;
+
+	/* Clear only the intel specific part of the crtc state */
+	tmp_state = crtc_state->base;
+	memset(crtc_state, 0, sizeof *crtc_state);
+	crtc_state->base = tmp_state;
+}
+
+static int
 intel_modeset_pipe_config(struct drm_crtc *crtc,
 			  struct drm_framebuffer *fb,
-			  struct drm_display_mode *mode)
+			  struct drm_display_mode *mode,
+			  struct drm_atomic_state *state)
 {
 	struct drm_device *dev = crtc->dev;
 	struct intel_encoder *encoder;
@@ -10303,17 +10316,19 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
 
 	if (!check_encoder_cloning(to_intel_crtc(crtc))) {
 		DRM_DEBUG_KMS("rejecting invalid cloning configuration\n");
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 	}
 
 	if (!check_digital_port_conflicts(dev)) {
 		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 	}
 
-	pipe_config = kzalloc(sizeof(*pipe_config), GFP_KERNEL);
-	if (!pipe_config)
-		return ERR_PTR(-ENOMEM);
+	pipe_config = intel_atomic_get_crtc_state(state, to_intel_crtc(crtc));
+	if (IS_ERR(pipe_config))
+		return PTR_ERR(pipe_config);
+
+	clear_intel_crtc_state(pipe_config);
 
 	pipe_config->base.crtc = crtc;
 	drm_mode_copy(&pipe_config->base.adjusted_mode, mode);
@@ -10408,10 +10423,9 @@ encoder_retry:
 	DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n",
 		      plane_bpp, pipe_config->pipe_bpp, pipe_config->dither);
 
-	return pipe_config;
+	return 0;
 fail:
-	kfree(pipe_config);
-	return ERR_PTR(ret);
+	return ret;
 }
 
 /* Computes which crtcs are affected and sets the relevant bits in the mask. For
@@ -11089,17 +11103,19 @@ static struct intel_crtc_state *
 intel_modeset_compute_config(struct drm_crtc *crtc,
 			     struct drm_display_mode *mode,
 			     struct drm_framebuffer *fb,
+			     struct drm_atomic_state *state,
 			     unsigned *modeset_pipes,
 			     unsigned *prepare_pipes,
 			     unsigned *disable_pipes)
 {
 	struct intel_crtc_state *pipe_config = NULL;
+	int ret = 0;
 
 	intel_modeset_affected_pipes(crtc, modeset_pipes,
 				     prepare_pipes, disable_pipes);
 
 	if ((*modeset_pipes) == 0)
-		goto out;
+		return NULL;
 
 	/*
 	 * Note this needs changes when we start tracking multiple modes
@@ -11107,14 +11123,17 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
 	 * (i.e. one pipe_config for each crtc) rather than just the one
 	 * for this crtc.
 	 */
-	pipe_config = intel_modeset_pipe_config(crtc, fb, mode);
-	if (IS_ERR(pipe_config)) {
-		goto out;
-	}
+	ret = intel_modeset_pipe_config(crtc, fb, mode, state);
+	if (ret)
+		return ERR_PTR(ret);
+
+	pipe_config = intel_atomic_get_crtc_state(state, to_intel_crtc(crtc));
+	if (IS_ERR(pipe_config))
+		return pipe_config;
+
 	intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,
 			       "[modeset]");
 
-out:
 	return pipe_config;
 }
 
@@ -11159,6 +11178,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_display_mode *saved_mode;
+	struct intel_crtc_state *crtc_state_copy = NULL;
 	struct intel_crtc *intel_crtc;
 	int ret = 0;
 
@@ -11166,6 +11186,12 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 	if (!saved_mode)
 		return -ENOMEM;
 
+	crtc_state_copy = kmalloc(sizeof(*crtc_state_copy), GFP_KERNEL);
+	if (!crtc_state_copy) {
+		ret = -ENOMEM;
+		goto done;
+	}
+
 	*saved_mode = crtc->mode;
 
 	if (modeset_pipes)
@@ -11252,6 +11278,19 @@ done:
 	if (ret && crtc->state->enable)
 		crtc->mode = *saved_mode;
 
+	if (ret == 0 && pipe_config) {
+		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+		/* The pipe_config will be freed with the atomic state, so
+		 * make a copy. */
+		memcpy(crtc_state_copy, intel_crtc->config,
+		       sizeof *crtc_state_copy);
+		intel_crtc->config = intel_crtc->new_config = crtc_state_copy;
+		intel_crtc->base.state = &crtc_state_copy->base;
+	} else {
+		kfree(crtc_state_copy);
+	}
+
 	kfree(saved_mode);
 	return ret;
 }
@@ -11279,20 +11318,37 @@ static int intel_set_mode(struct drm_crtc *crtc,
 			  struct drm_display_mode *mode,
 			  int x, int y, struct drm_framebuffer *fb)
 {
+	struct drm_device *dev = crtc->dev;
+	struct drm_atomic_state *state;
 	struct intel_crtc_state *pipe_config;
 	unsigned modeset_pipes, prepare_pipes, disable_pipes;
+	int ret = 0;
+
+	state = drm_atomic_state_alloc(dev);
+	if (!state)
+		return -ENOMEM;
+
+	state->acquire_ctx = dev->mode_config.acquire_ctx;
 
-	pipe_config = intel_modeset_compute_config(crtc, mode, fb,
+	pipe_config = intel_modeset_compute_config(crtc, mode, fb, state,
 						   &modeset_pipes,
 						   &prepare_pipes,
 						   &disable_pipes);
 
-	if (IS_ERR(pipe_config))
-		return PTR_ERR(pipe_config);
+	if (IS_ERR(pipe_config)) {
+		ret = PTR_ERR(pipe_config);
+		goto out;
+	}
+
+	ret = intel_set_mode_pipes(crtc, mode, x, y, fb, pipe_config,
+				   modeset_pipes, prepare_pipes,
+				   disable_pipes);
+	if (ret)
+		goto out;
 
-	return intel_set_mode_pipes(crtc, mode, x, y, fb, pipe_config,
-				    modeset_pipes, prepare_pipes,
-				    disable_pipes);
+out:
+	drm_atomic_state_free(state);
+	return ret;
 }
 
 void intel_crtc_restore_mode(struct drm_crtc *crtc)
@@ -11622,6 +11678,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 {
 	struct drm_device *dev;
 	struct drm_mode_set save_set;
+	struct drm_atomic_state *state = NULL;
 	struct intel_set_config *config;
 	struct intel_crtc_state *pipe_config;
 	unsigned modeset_pipes, prepare_pipes, disable_pipes;
@@ -11666,12 +11723,20 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 	 * such cases. */
 	intel_set_config_compute_mode_changes(set, config);
 
+	state = drm_atomic_state_alloc(dev);
+	if (!state) {
+		ret = -ENOMEM;
+		goto out_config;
+	}
+
+	state->acquire_ctx = dev->mode_config.acquire_ctx;
+
 	ret = intel_modeset_stage_output_state(dev, set, config);
 	if (ret)
 		goto fail;
 
 	pipe_config = intel_modeset_compute_config(set->crtc, set->mode,
-						   set->fb,
+						   set->fb, state,
 						   &modeset_pipes,
 						   &prepare_pipes,
 						   &disable_pipes);
@@ -11691,10 +11756,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 		 */
 	}
 
-	/* set_mode will free it in the mode_changed case */
-	if (!config->mode_changed)
-		kfree(pipe_config);
-
 	intel_update_pipe_size(to_intel_crtc(set->crtc));
 
 	if (config->mode_changed) {
@@ -11757,6 +11818,9 @@ fail:
 	}
 
 out_config:
+	if (state)
+		drm_atomic_state_free(state);
+
 	intel_set_config_free(config);
 	return ret;
 }
-- 
2.1.0

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

Re: [PATCH 05/23] drm/i915: Allocate a drm_atomic_state for the legacy modeset code

[Daniel Vetter]

On Tue, Mar 03, 2015 at 03:21:59PM +0200, Ander Conselvan de Oliveira wrote:
> For the atomic conversion, the mode set paths need to be changed to rely
> on an atomic state instead of using the staged config. By using an
> atomic state for the legacy code, we will be able to convert the code
> base in small chunks.
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

Two small comments below.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 118 +++++++++++++++++++++++++++--------
>  1 file changed, 91 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 798de7b..97d4df5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -37,6 +37,7 @@
>  #include <drm/i915_drm.h>
>  #include "i915_drv.h"
>  #include "i915_trace.h"
> +#include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_dp_helper.h>
>  #include <drm/drm_crtc_helper.h>
> @@ -10290,10 +10291,22 @@ static bool check_digital_port_conflicts(struct drm_device *dev)
>  	return true;
>  }
>  
> -static struct intel_crtc_state *
> +static void
> +clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
> +{
> +	struct drm_crtc_state tmp_state;
> +
> +	/* Clear only the intel specific part of the crtc state */
> +	tmp_state = crtc_state->base;
> +	memset(crtc_state, 0, sizeof *crtc_state);
> +	crtc_state->base = tmp_state;
> +}

I guess this is to clear out state which we want to recompute, and our
compute_config code assumes that it's always kzalloc'ed a new config?

I think this should be part of the crtc duplicate_state callback to make
sure we're doing this consistently.

> +
> +static int
>  intel_modeset_pipe_config(struct drm_crtc *crtc,
>  			  struct drm_framebuffer *fb,
> -			  struct drm_display_mode *mode)
> +			  struct drm_display_mode *mode,
> +			  struct drm_atomic_state *state)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct intel_encoder *encoder;
> @@ -10303,17 +10316,19 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>  
>  	if (!check_encoder_cloning(to_intel_crtc(crtc))) {
>  		DRM_DEBUG_KMS("rejecting invalid cloning configuration\n");
> -		return ERR_PTR(-EINVAL);
> +		return -EINVAL;
>  	}
>  
>  	if (!check_digital_port_conflicts(dev)) {
>  		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
> -		return ERR_PTR(-EINVAL);
> +		return -EINVAL;
>  	}
>  
> -	pipe_config = kzalloc(sizeof(*pipe_config), GFP_KERNEL);
> -	if (!pipe_config)
> -		return ERR_PTR(-ENOMEM);
> +	pipe_config = intel_atomic_get_crtc_state(state, to_intel_crtc(crtc));
> +	if (IS_ERR(pipe_config))
> +		return PTR_ERR(pipe_config);
> +
> +	clear_intel_crtc_state(pipe_config);
>  
>  	pipe_config->base.crtc = crtc;
>  	drm_mode_copy(&pipe_config->base.adjusted_mode, mode);
> @@ -10408,10 +10423,9 @@ encoder_retry:
>  	DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n",
>  		      plane_bpp, pipe_config->pipe_bpp, pipe_config->dither);
>  
> -	return pipe_config;
> +	return 0;
>  fail:
> -	kfree(pipe_config);
> -	return ERR_PTR(ret);
> +	return ret;
>  }
>  
>  /* Computes which crtcs are affected and sets the relevant bits in the mask. For
> @@ -11089,17 +11103,19 @@ static struct intel_crtc_state *
>  intel_modeset_compute_config(struct drm_crtc *crtc,
>  			     struct drm_display_mode *mode,
>  			     struct drm_framebuffer *fb,
> +			     struct drm_atomic_state *state,
>  			     unsigned *modeset_pipes,
>  			     unsigned *prepare_pipes,
>  			     unsigned *disable_pipes)
>  {
>  	struct intel_crtc_state *pipe_config = NULL;
> +	int ret = 0;
>  
>  	intel_modeset_affected_pipes(crtc, modeset_pipes,
>  				     prepare_pipes, disable_pipes);
>  
>  	if ((*modeset_pipes) == 0)
> -		goto out;
> +		return NULL;
>  
>  	/*
>  	 * Note this needs changes when we start tracking multiple modes
> @@ -11107,14 +11123,17 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
>  	 * (i.e. one pipe_config for each crtc) rather than just the one
>  	 * for this crtc.
>  	 */
> -	pipe_config = intel_modeset_pipe_config(crtc, fb, mode);
> -	if (IS_ERR(pipe_config)) {
> -		goto out;
> -	}
> +	ret = intel_modeset_pipe_config(crtc, fb, mode, state);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	pipe_config = intel_atomic_get_crtc_state(state, to_intel_crtc(crtc));
> +	if (IS_ERR(pipe_config))
> +		return pipe_config;
> +
>  	intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,
>  			       "[modeset]");
>  
> -out:
>  	return pipe_config;
>  }
>  
> @@ -11159,6 +11178,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_display_mode *saved_mode;
> +	struct intel_crtc_state *crtc_state_copy = NULL;
>  	struct intel_crtc *intel_crtc;
>  	int ret = 0;
>  
> @@ -11166,6 +11186,12 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  	if (!saved_mode)
>  		return -ENOMEM;
>  
> +	crtc_state_copy = kmalloc(sizeof(*crtc_state_copy), GFP_KERNEL);
> +	if (!crtc_state_copy) {
> +		ret = -ENOMEM;
> +		goto done;
> +	}
> +
>  	*saved_mode = crtc->mode;
>  
>  	if (modeset_pipes)
> @@ -11252,6 +11278,19 @@ done:
>  	if (ret && crtc->state->enable)
>  		crtc->mode = *saved_mode;
>  
> +	if (ret == 0 && pipe_config) {
> +		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +
> +		/* The pipe_config will be freed with the atomic state, so
> +		 * make a copy. */
> +		memcpy(crtc_state_copy, intel_crtc->config,
> +		       sizeof *crtc_state_copy);
> +		intel_crtc->config = intel_crtc->new_config = crtc_state_copy;
> +		intel_crtc->base.state = &crtc_state_copy->base;
> +	} else {
> +		kfree(crtc_state_copy);
> +	}
> +
>  	kfree(saved_mode);
>  	return ret;
>  }
> @@ -11279,20 +11318,37 @@ static int intel_set_mode(struct drm_crtc *crtc,
>  			  struct drm_display_mode *mode,
>  			  int x, int y, struct drm_framebuffer *fb)
>  {
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_atomic_state *state;
>  	struct intel_crtc_state *pipe_config;
>  	unsigned modeset_pipes, prepare_pipes, disable_pipes;
> +	int ret = 0;
> +
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state)
> +		return -ENOMEM;
> +
> +	state->acquire_ctx = dev->mode_config.acquire_ctx;
>  
> -	pipe_config = intel_modeset_compute_config(crtc, mode, fb,
> +	pipe_config = intel_modeset_compute_config(crtc, mode, fb, state,
>  						   &modeset_pipes,
>  						   &prepare_pipes,
>  						   &disable_pipes);
>  
> -	if (IS_ERR(pipe_config))
> -		return PTR_ERR(pipe_config);
> +	if (IS_ERR(pipe_config)) {
> +		ret = PTR_ERR(pipe_config);
> +		goto out;
> +	}
> +
> +	ret = intel_set_mode_pipes(crtc, mode, x, y, fb, pipe_config,
> +				   modeset_pipes, prepare_pipes,
> +				   disable_pipes);
> +	if (ret)
> +		goto out;
>  
> -	return intel_set_mode_pipes(crtc, mode, x, y, fb, pipe_config,
> -				    modeset_pipes, prepare_pipes,
> -				    disable_pipes);
> +out:
> +	drm_atomic_state_free(state);
> +	return ret;
>  }
>  
>  void intel_crtc_restore_mode(struct drm_crtc *crtc)
> @@ -11622,6 +11678,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  {
>  	struct drm_device *dev;
>  	struct drm_mode_set save_set;
> +	struct drm_atomic_state *state = NULL;
>  	struct intel_set_config *config;
>  	struct intel_crtc_state *pipe_config;
>  	unsigned modeset_pipes, prepare_pipes, disable_pipes;
> @@ -11666,12 +11723,20 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  	 * such cases. */
>  	intel_set_config_compute_mode_changes(set, config);
>  
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state) {
> +		ret = -ENOMEM;
> +		goto out_config;
> +	}
> +
> +	state->acquire_ctx = dev->mode_config.acquire_ctx;
> +
>  	ret = intel_modeset_stage_output_state(dev, set, config);
>  	if (ret)
>  		goto fail;
>  
>  	pipe_config = intel_modeset_compute_config(set->crtc, set->mode,
> -						   set->fb,
> +						   set->fb, state,
>  						   &modeset_pipes,
>  						   &prepare_pipes,
>  						   &disable_pipes);
> @@ -11691,10 +11756,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  		 */
>  	}
>  
> -	/* set_mode will free it in the mode_changed case */
> -	if (!config->mode_changed)
> -		kfree(pipe_config);
> -
>  	intel_update_pipe_size(to_intel_crtc(set->crtc));
>  
>  	if (config->mode_changed) {
> @@ -11757,6 +11818,9 @@ fail:
>  	}
>  
>  out_config:
> +	if (state)
> +		drm_atomic_state_free(state);

Right above this we also call set_mode again, which also grabs a global
state. Nesting seems tricky, so I thnk you should free up the atomic state
before we try the failure code paths to restore the old state.

> +
>  	intel_set_config_free(config);
>  	return ret;
>  }
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/23] drm/i915: Allocate a drm_atomic_state for the legacy modeset code

[Matt Roper]

On Wed, Mar 04, 2015 at 04:33:17PM +0100, Daniel Vetter wrote:
> On Tue, Mar 03, 2015 at 03:21:59PM +0200, Ander Conselvan de Oliveira wrote:
> > For the atomic conversion, the mode set paths need to be changed to rely
> > on an atomic state instead of using the staged config. By using an
> > atomic state for the legacy code, we will be able to convert the code
> > base in small chunks.
> > 
> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> 
> Two small comments below.
> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 118 +++++++++++++++++++++++++++--------
> >  1 file changed, 91 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 798de7b..97d4df5 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -37,6 +37,7 @@
> >  #include <drm/i915_drm.h>
> >  #include "i915_drv.h"
> >  #include "i915_trace.h"
> > +#include <drm/drm_atomic.h>
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_dp_helper.h>
> >  #include <drm/drm_crtc_helper.h>
> > @@ -10290,10 +10291,22 @@ static bool check_digital_port_conflicts(struct drm_device *dev)
> >  	return true;
> >  }
> >  
> > -static struct intel_crtc_state *
> > +static void
> > +clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
> > +{
> > +	struct drm_crtc_state tmp_state;
> > +
> > +	/* Clear only the intel specific part of the crtc state */
> > +	tmp_state = crtc_state->base;
> > +	memset(crtc_state, 0, sizeof *crtc_state);
> > +	crtc_state->base = tmp_state;
> > +}
> 
> I guess this is to clear out state which we want to recompute, and our
> compute_config code assumes that it's always kzalloc'ed a new config?
> 
> I think this should be part of the crtc duplicate_state callback to make
> sure we're doing this consistently.

If we zero out the config in duplicate_state(), then we're not really
"duplicating" anymore are we?  Ultimately we want to be able to
duplicate the existing state, tweak a couple things, and then pass that
state through the atomic pipeline, so it feels like doing a clear in the
duplicate function is the wrong move, even if it would work for the
frankenstein-style codebase we have at the moment.


Matt

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/23] drm/i915: Allocate a drm_atomic_state for the legacy modeset code

[Ander Conselvan De Oliveira]

On Mon, 2015-03-09 at 16:19 -0700, Matt Roper wrote:
> On Wed, Mar 04, 2015 at 04:33:17PM +0100, Daniel Vetter wrote:
> > On Tue, Mar 03, 2015 at 03:21:59PM +0200, Ander Conselvan de Oliveira wrote:
> > > For the atomic conversion, the mode set paths need to be changed to rely
> > > on an atomic state instead of using the staged config. By using an
> > > atomic state for the legacy code, we will be able to convert the code
> > > base in small chunks.
> > > 
> > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> > 
> > Two small comments below.
> > -Daniel
> > 
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 118 +++++++++++++++++++++++++++--------
> > >  1 file changed, 91 insertions(+), 27 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 798de7b..97d4df5 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -37,6 +37,7 @@
> > >  #include <drm/i915_drm.h>
> > >  #include "i915_drv.h"
> > >  #include "i915_trace.h"
> > > +#include <drm/drm_atomic.h>
> > >  #include <drm/drm_atomic_helper.h>
> > >  #include <drm/drm_dp_helper.h>
> > >  #include <drm/drm_crtc_helper.h>
> > > @@ -10290,10 +10291,22 @@ static bool check_digital_port_conflicts(struct drm_device *dev)
> > >  	return true;
> > >  }
> > >  
> > > -static struct intel_crtc_state *
> > > +static void
> > > +clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
> > > +{
> > > +	struct drm_crtc_state tmp_state;
> > > +
> > > +	/* Clear only the intel specific part of the crtc state */
> > > +	tmp_state = crtc_state->base;
> > > +	memset(crtc_state, 0, sizeof *crtc_state);
> > > +	crtc_state->base = tmp_state;
> > > +}
> > 
> > I guess this is to clear out state which we want to recompute, and our
> > compute_config code assumes that it's always kzalloc'ed a new config?
> > 
> > I think this should be part of the crtc duplicate_state callback to make
> > sure we're doing this consistently.
> 
> If we zero out the config in duplicate_state(), then we're not really
> "duplicating" anymore are we?  Ultimately we want to be able to
> duplicate the existing state, tweak a couple things, and then pass that
> state through the atomic pipeline, so it feels like doing a clear in the
> duplicate function is the wrong move, even if it would work for the
> frankenstein-style codebase we have at the moment.

Yeah, I guess I can't run away from inspecting the code and fixing
whatever expects the crtc_state to be zeroed.

Ander

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

[PATCH 06/23] drm/i915: Add an optional atomic state argument to intel_set_mode()

[Ander Conselvan de Oliveira]

In the set config modeset path, the atomic state is updated when
changing the staged config in intel_modeset_stage_output_config(). The
load detect code also causes a modeset, but it changes the staged config
before calling intel_set_mode(). A follow up patch will change that
function to also update a drm_atomic_state, and it will need to be able
to pass that to intel_set_mode().

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 97d4df5..3c3b5b2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -83,7 +83,8 @@ static void ironlake_pch_clock_get(struct intel_crtc *crtc,
 				   struct intel_crtc_state *pipe_config);
 
 static int intel_set_mode(struct drm_crtc *crtc, struct drm_display_mode *mode,
-			  int x, int y, struct drm_framebuffer *old_fb);
+			  int x, int y, struct drm_framebuffer *old_fb,
+			  struct drm_atomic_state *state);
 static int intel_framebuffer_init(struct drm_device *dev,
 				  struct intel_framebuffer *ifb,
 				  struct drm_mode_fb_cmd2 *mode_cmd,
@@ -8854,7 +8855,7 @@ retry:
 		goto fail;
 	}
 
-	if (intel_set_mode(crtc, mode, 0, 0, fb)) {
+	if (intel_set_mode(crtc, mode, 0, 0, fb, NULL)) {
 		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
 		if (old->release_fb)
 			old->release_fb->funcs->destroy(old->release_fb);
@@ -8898,7 +8899,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
 		intel_encoder->new_crtc = NULL;
 		intel_crtc->new_enabled = false;
 		intel_crtc->new_config = NULL;
-		intel_set_mode(crtc, NULL, 0, 0, NULL);
+		intel_set_mode(crtc, NULL, 0, 0, NULL, NULL);
 
 		if (old->release_fb) {
 			drm_framebuffer_unregister_private(old->release_fb);
@@ -11316,19 +11317,21 @@ static int intel_set_mode_pipes(struct drm_crtc *crtc,
 
 static int intel_set_mode(struct drm_crtc *crtc,
 			  struct drm_display_mode *mode,
-			  int x, int y, struct drm_framebuffer *fb)
+			  int x, int y, struct drm_framebuffer *fb,
+			  struct drm_atomic_state *state)
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_atomic_state *state;
 	struct intel_crtc_state *pipe_config;
 	unsigned modeset_pipes, prepare_pipes, disable_pipes;
 	int ret = 0;
 
-	state = drm_atomic_state_alloc(dev);
-	if (!state)
-		return -ENOMEM;
+	if (!state) {
+		state = drm_atomic_state_alloc(dev);
+		if (!state)
+			return -ENOMEM;
 
-	state->acquire_ctx = dev->mode_config.acquire_ctx;
+		state->acquire_ctx = dev->mode_config.acquire_ctx;
+	}
 
 	pipe_config = intel_modeset_compute_config(crtc, mode, fb, state,
 						   &modeset_pipes,
@@ -11353,7 +11356,8 @@ out:
 
 void intel_crtc_restore_mode(struct drm_crtc *crtc)
 {
-	intel_set_mode(crtc, &crtc->mode, crtc->x, crtc->y, crtc->primary->fb);
+	intel_set_mode(crtc, &crtc->mode, crtc->x, crtc->y, crtc->primary->fb,
+		       NULL);
 }
 
 #undef for_each_intel_crtc_masked
@@ -11813,7 +11817,8 @@ fail:
 		/* Try to restore the config */
 		if (config->mode_changed &&
 		    intel_set_mode(save_set.crtc, save_set.mode,
-				   save_set.x, save_set.y, save_set.fb))
+				   save_set.x, save_set.y, save_set.fb,
+				   NULL))
 			DRM_ERROR("failed to restore config after modeset failure\n");
 	}
 
@@ -13857,7 +13862,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 				dev_priv->pipe_to_crtc_mapping[pipe];
 
 			intel_set_mode(crtc, &crtc->mode, crtc->x, crtc->y,
-				       crtc->primary->fb);
+				       crtc->primary->fb, NULL);
 		}
 	} else {
 		intel_modeset_update_staged_output_state(dev);
-- 
2.1.0

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

Re: [PATCH 06/23] drm/i915: Add an optional atomic state argument to intel_set_mode()

[Daniel Vetter]

On Tue, Mar 03, 2015 at 03:22:00PM +0200, Ander Conselvan de Oliveira wrote:
> In the set config modeset path, the atomic state is updated when
> changing the staged config in intel_modeset_stage_output_config(). The
> load detect code also causes a modeset, but it changes the staged config
> before calling intel_set_mode(). A follow up patch will change that
> function to also update a drm_atomic_state, and it will need to be able
> to pass that to intel_set_mode().
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

Hm given this patch and the nesting in the previous one maybe we should
just add drm_atomic_state scaffolding to all callers of intel_set_mode?

Long term we need to switch over the restore_mode function over to atomic
anyway (I have ideas for generic helpers already). And the load detect
code needs to switch to atomic too anyway.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 97d4df5..3c3b5b2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -83,7 +83,8 @@ static void ironlake_pch_clock_get(struct intel_crtc *crtc,
>  				   struct intel_crtc_state *pipe_config);
>  
>  static int intel_set_mode(struct drm_crtc *crtc, struct drm_display_mode *mode,
> -			  int x, int y, struct drm_framebuffer *old_fb);
> +			  int x, int y, struct drm_framebuffer *old_fb,
> +			  struct drm_atomic_state *state);
>  static int intel_framebuffer_init(struct drm_device *dev,
>  				  struct intel_framebuffer *ifb,
>  				  struct drm_mode_fb_cmd2 *mode_cmd,
> @@ -8854,7 +8855,7 @@ retry:
>  		goto fail;
>  	}
>  
> -	if (intel_set_mode(crtc, mode, 0, 0, fb)) {
> +	if (intel_set_mode(crtc, mode, 0, 0, fb, NULL)) {
>  		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
>  		if (old->release_fb)
>  			old->release_fb->funcs->destroy(old->release_fb);
> @@ -8898,7 +8899,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
>  		intel_encoder->new_crtc = NULL;
>  		intel_crtc->new_enabled = false;
>  		intel_crtc->new_config = NULL;
> -		intel_set_mode(crtc, NULL, 0, 0, NULL);
> +		intel_set_mode(crtc, NULL, 0, 0, NULL, NULL);
>  
>  		if (old->release_fb) {
>  			drm_framebuffer_unregister_private(old->release_fb);
> @@ -11316,19 +11317,21 @@ static int intel_set_mode_pipes(struct drm_crtc *crtc,
>  
>  static int intel_set_mode(struct drm_crtc *crtc,
>  			  struct drm_display_mode *mode,
> -			  int x, int y, struct drm_framebuffer *fb)
> +			  int x, int y, struct drm_framebuffer *fb,
> +			  struct drm_atomic_state *state)
>  {
>  	struct drm_device *dev = crtc->dev;
> -	struct drm_atomic_state *state;
>  	struct intel_crtc_state *pipe_config;
>  	unsigned modeset_pipes, prepare_pipes, disable_pipes;
>  	int ret = 0;
>  
> -	state = drm_atomic_state_alloc(dev);
> -	if (!state)
> -		return -ENOMEM;
> +	if (!state) {
> +		state = drm_atomic_state_alloc(dev);
> +		if (!state)
> +			return -ENOMEM;
>  
> -	state->acquire_ctx = dev->mode_config.acquire_ctx;
> +		state->acquire_ctx = dev->mode_config.acquire_ctx;
> +	}
>  
>  	pipe_config = intel_modeset_compute_config(crtc, mode, fb, state,
>  						   &modeset_pipes,
> @@ -11353,7 +11356,8 @@ out:
>  
>  void intel_crtc_restore_mode(struct drm_crtc *crtc)
>  {
> -	intel_set_mode(crtc, &crtc->mode, crtc->x, crtc->y, crtc->primary->fb);
> +	intel_set_mode(crtc, &crtc->mode, crtc->x, crtc->y, crtc->primary->fb,
> +		       NULL);
>  }
>  
>  #undef for_each_intel_crtc_masked
> @@ -11813,7 +11817,8 @@ fail:
>  		/* Try to restore the config */
>  		if (config->mode_changed &&
>  		    intel_set_mode(save_set.crtc, save_set.mode,
> -				   save_set.x, save_set.y, save_set.fb))
> +				   save_set.x, save_set.y, save_set.fb,
> +				   NULL))
>  			DRM_ERROR("failed to restore config after modeset failure\n");
>  	}
>  
> @@ -13857,7 +13862,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  				dev_priv->pipe_to_crtc_mapping[pipe];
>  
>  			intel_set_mode(crtc, &crtc->mode, crtc->x, crtc->y,
> -				       crtc->primary->fb);
> +				       crtc->primary->fb, NULL);
>  		}
>  	} else {
>  		intel_modeset_update_staged_output_state(dev);
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 07/23] drm/i915: Use an atomic state for the load detect modeset

[Ander Conselvan de Oliveira]

Create an atomic state and initialize it for the load detect pipe
modesets, so it doesn't break once the rest of the mode set code starts
using atomic states instead of the staged config.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_crt.c     |  2 +-
 drivers/gpu/drm/i915/intel_display.c | 52 +++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_drv.h     |  3 ++-
 drivers/gpu/drm/i915/intel_tv.c      |  2 +-
 4 files changed, 52 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index e66e17a..974534e 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -708,7 +708,7 @@ intel_crt_detect(struct drm_connector *connector, bool force)
 			status = connector_status_connected;
 		else
 			status = intel_crt_load_detect(crt);
-		intel_release_load_detect_pipe(connector, &tmp);
+		intel_release_load_detect_pipe(connector, &tmp, &ctx);
 	} else
 		status = connector_status_unknown;
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3c3b5b2..080a197 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8752,6 +8752,8 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
 	struct drm_device *dev = encoder->dev;
 	struct drm_framebuffer *fb;
 	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_atomic_state *state = NULL;
+	struct drm_connector_state *connector_state;
 	int ret, i = -1;
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
@@ -8833,6 +8835,21 @@ retry:
 	old->load_detect_temp = true;
 	old->release_fb = NULL;
 
+	state = drm_atomic_state_alloc(dev);
+	if (!state)
+		return false;
+
+	state->acquire_ctx = ctx;
+
+	connector_state = drm_atomic_get_connector_state(state, connector);
+	if (IS_ERR(connector_state)) {
+		ret = PTR_ERR(connector_state);
+		goto fail;
+	}
+
+	connector_state->crtc = crtc;
+	connector_state->best_encoder = &intel_encoder->base;
+
 	if (!mode)
 		mode = &load_detect_mode;
 
@@ -8855,7 +8872,7 @@ retry:
 		goto fail;
 	}
 
-	if (intel_set_mode(crtc, mode, 0, 0, fb, NULL)) {
+	if (intel_set_mode(crtc, mode, 0, 0, fb, state)) {
 		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
 		if (old->release_fb)
 			old->release_fb->funcs->destroy(old->release_fb);
@@ -8873,6 +8890,11 @@ retry:
 	else
 		intel_crtc->new_config = NULL;
 fail_unlock:
+	if (state) {
+		drm_atomic_state_free(state);
+		state = NULL;
+	}
+
 	if (ret == -EDEADLK) {
 		drm_modeset_backoff(ctx);
 		goto retry;
@@ -8882,24 +8904,41 @@ fail_unlock:
 }
 
 void intel_release_load_detect_pipe(struct drm_connector *connector,
-				    struct intel_load_detect_pipe *old)
+				    struct intel_load_detect_pipe *old,
+				    struct drm_modeset_acquire_ctx *ctx)
 {
 	struct intel_encoder *intel_encoder =
 		intel_attached_encoder(connector);
 	struct drm_encoder *encoder = &intel_encoder->base;
 	struct drm_crtc *crtc = encoder->crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_atomic_state *state;
+	struct drm_connector_state *connector_state;
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
 		      connector->base.id, connector->name,
 		      encoder->base.id, encoder->name);
 
 	if (old->load_detect_temp) {
+		state = drm_atomic_state_alloc(connector->dev);
+		if (!state)
+			goto fail;
+
+		state->acquire_ctx = ctx;
+
+		connector_state = drm_atomic_get_connector_state(state, connector);
+		if (IS_ERR(connector_state))
+			goto fail;
+
 		to_intel_connector(connector)->new_encoder = NULL;
 		intel_encoder->new_crtc = NULL;
 		intel_crtc->new_enabled = false;
 		intel_crtc->new_config = NULL;
-		intel_set_mode(crtc, NULL, 0, 0, NULL, NULL);
+
+		connector_state->best_encoder = NULL;
+		connector_state->crtc = NULL;
+
+		intel_set_mode(crtc, NULL, 0, 0, NULL, state);
 
 		if (old->release_fb) {
 			drm_framebuffer_unregister_private(old->release_fb);
@@ -8912,6 +8951,11 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
 	/* Switch crtc and encoder back off if necessary */
 	if (old->dpms_mode != DRM_MODE_DPMS_ON)
 		connector->funcs->dpms(connector, old->dpms_mode);
+
+	return;
+fail:
+	DRM_DEBUG_KMS("Couldn't release load detect pipe.\n");
+	drm_atomic_state_free(state);
 }
 
 static int i9xx_pll_refclk(struct drm_device *dev,
@@ -13489,7 +13533,7 @@ static void intel_enable_pipe_a(struct drm_device *dev)
 		return;
 
 	if (intel_get_load_detect_pipe(crt, NULL, &load_detect_temp, ctx))
-		intel_release_load_detect_pipe(crt, &load_detect_temp);
+		intel_release_load_detect_pipe(crt, &load_detect_temp, ctx);
 }
 
 static bool
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c1959e0..78563d7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -954,7 +954,8 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
 				struct intel_load_detect_pipe *old,
 				struct drm_modeset_acquire_ctx *ctx);
 void intel_release_load_detect_pipe(struct drm_connector *connector,
-				    struct intel_load_detect_pipe *old);
+				    struct intel_load_detect_pipe *old,
+				    struct drm_modeset_acquire_ctx *ctx);
 int intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 			       struct drm_framebuffer *fb,
 			       struct intel_engine_cs *pipelined);
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 892d23c..4e6684e 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1332,7 +1332,7 @@ intel_tv_detect(struct drm_connector *connector, bool force)
 
 		if (intel_get_load_detect_pipe(connector, &mode, &tmp, &ctx)) {
 			type = intel_tv_detect_type(intel_tv, connector);
-			intel_release_load_detect_pipe(connector, &tmp);
+			intel_release_load_detect_pipe(connector, &tmp, &ctx);
 			status = type < 0 ?
 				connector_status_disconnected :
 				connector_status_connected;
-- 
2.1.0

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

[PATCH 08/23] drm/i915: Allocate a crtc_state also when the crtc is being disabled

[Ander Conselvan de Oliveira]

For consistency, allocate a new crtc_state for a crtc that is being
disabled. Previously only the enabled value of the current state would
change.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 080a197..f020ee5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11153,14 +11153,21 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
 			     unsigned *prepare_pipes,
 			     unsigned *disable_pipes)
 {
+	struct drm_device *dev = crtc->dev;
 	struct intel_crtc_state *pipe_config = NULL;
+	struct intel_crtc *intel_crtc;
 	int ret = 0;
 
 	intel_modeset_affected_pipes(crtc, modeset_pipes,
 				     prepare_pipes, disable_pipes);
 
-	if ((*modeset_pipes) == 0)
-		return NULL;
+	for_each_intel_crtc_masked(dev, *disable_pipes, intel_crtc) {
+		pipe_config = intel_atomic_get_crtc_state(state, intel_crtc);
+		if (IS_ERR(pipe_config))
+			return pipe_config;
+
+		pipe_config->base.enable = false;
+	}
 
 	/*
 	 * Note this needs changes when we start tracking multiple modes
@@ -11168,18 +11175,25 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
 	 * (i.e. one pipe_config for each crtc) rather than just the one
 	 * for this crtc.
 	 */
-	ret = intel_modeset_pipe_config(crtc, fb, mode, state);
-	if (ret)
-		return ERR_PTR(ret);
+	for_each_intel_crtc_masked(dev, *modeset_pipes, intel_crtc) {
+		/* FIXME: For now we still expect modeset_pipes has at most
+		 * one bit set. */
+		if (WARN_ON(&intel_crtc->base != crtc))
+			continue;
 
-	pipe_config = intel_atomic_get_crtc_state(state, to_intel_crtc(crtc));
-	if (IS_ERR(pipe_config))
-		return pipe_config;
+		ret = intel_modeset_pipe_config(crtc, fb, mode, state);
+		if (ret)
+			return ERR_PTR(ret);
+
+		pipe_config = intel_atomic_get_crtc_state(state, intel_crtc);
+		if (IS_ERR(pipe_config))
+			return pipe_config;
 
-	intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,
-			       "[modeset]");
+		intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,
+				       "[modeset]");
+	}
 
-	return pipe_config;
+	return intel_atomic_get_crtc_state(state, to_intel_crtc(crtc));
 }
 
 static int __intel_set_mode_setup_plls(struct drm_device *dev,
-- 
2.1.0

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

[PATCH 09/23] drm/i915: Update dummy connector atomic state with current config

[Ander Conselvan de Oliveira]

Keep that state updated so that we can write code that depends on it on
the follow up patches.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 41 ++++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f020ee5..108d3d2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10079,6 +10079,27 @@ static void intel_modeset_update_staged_output_state(struct drm_device *dev)
 	}
 }
 
+/* Transitional helper to copy current connector/encoder state to
+ * connector->state. This is needed so that code that is partially
+ * converted to atomic does the right thing.
+ */
+static void intel_modeset_update_connector_atomic_state(struct drm_device *dev)
+{
+	struct intel_encoder *encoder;
+	struct intel_connector *connector;
+
+	for_each_intel_encoder(dev, encoder) {
+		if (encoder->base.crtc &&
+		    to_intel_crtc(encoder->base.crtc)->active)
+
+		for_each_intel_connector(dev, connector) {
+			if (connector->base.encoder != &encoder->base)
+				continue;
+
+			connector->base.state->crtc = encoder->base.crtc;
+		}
+	}
+}
 /**
  * intel_modeset_commit_output_state
  *
@@ -10102,6 +10123,8 @@ static void intel_modeset_commit_output_state(struct drm_device *dev)
 		crtc->base.state->enable = crtc->new_enabled;
 		crtc->base.enabled = crtc->new_enabled;
 	}
+
+	intel_modeset_update_connector_atomic_state(dev);
 }
 
 static void
@@ -12828,15 +12851,13 @@ static void intel_setup_outputs(struct drm_device *dev)
 	 * be removed since we'll be setting up real connector state, which
 	 * will contain Intel-specific properties.
 	 */
-	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
-		list_for_each_entry(connector,
-				    &dev->mode_config.connector_list,
-				    head) {
-			if (!WARN_ON(connector->state)) {
-				connector->state =
-					kzalloc(sizeof(*connector->state),
-						GFP_KERNEL);
-			}
+	/* FIXME: need to update the comment above. */
+	list_for_each_entry(connector,
+			    &dev->mode_config.connector_list,
+			    head) {
+		if (!WARN_ON(connector->state)) {
+			connector->state = kzalloc(sizeof(*connector->state),
+						   GFP_KERNEL);
 		}
 	}
 
@@ -13891,6 +13912,8 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 				       "[setup_hw_state]");
 	}
 
+	intel_modeset_update_connector_atomic_state(dev);
+
 	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
 		struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i];
 
-- 
2.1.0

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

[PATCH 10/23] drm/i915: Implement connector state duplication

[Ander Conselvan de Oliveira]

So that we can add connector states to the drm_atomic_state used in the
legacy modeset.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_crt.c    | 1 +
 drivers/gpu/drm/i915/intel_dp.c     | 1 +
 drivers/gpu/drm/i915/intel_dp_mst.c | 1 +
 drivers/gpu/drm/i915/intel_dsi.c    | 1 +
 drivers/gpu/drm/i915/intel_dvo.c    | 1 +
 drivers/gpu/drm/i915/intel_hdmi.c   | 1 +
 drivers/gpu/drm/i915/intel_lvds.c   | 1 +
 drivers/gpu/drm/i915/intel_sdvo.c   | 1 +
 drivers/gpu/drm/i915/intel_tv.c     | 1 +
 9 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 974534e..573aaff 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -794,6 +794,7 @@ static const struct drm_connector_funcs intel_crt_connector_funcs = {
 	.destroy = intel_crt_destroy,
 	.set_property = intel_crt_set_property,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 	.atomic_get_property = intel_connector_atomic_get_property,
 };
 
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index d1141d3..5accf3f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4398,6 +4398,7 @@ static const struct drm_connector_funcs intel_dp_connector_funcs = {
 	.atomic_get_property = intel_connector_atomic_get_property,
 	.destroy = intel_dp_connector_destroy,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 };
 
 static const struct drm_connector_helper_funcs intel_dp_connector_helper_funcs = {
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index be12492..5c06a06 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -317,6 +317,7 @@ static const struct drm_connector_funcs intel_dp_mst_connector_funcs = {
 	.atomic_get_property = intel_connector_atomic_get_property,
 	.destroy = intel_dp_mst_connector_destroy,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 };
 
 static int intel_dp_mst_get_modes(struct drm_connector *connector)
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index c8c8b24..572251e 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -975,6 +975,7 @@ static const struct drm_connector_funcs intel_dsi_connector_funcs = {
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.atomic_get_property = intel_connector_atomic_get_property,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 };
 
 void intel_dsi_init(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
index d857951..4ccd6c3 100644
--- a/drivers/gpu/drm/i915/intel_dvo.c
+++ b/drivers/gpu/drm/i915/intel_dvo.c
@@ -393,6 +393,7 @@ static const struct drm_connector_funcs intel_dvo_connector_funcs = {
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.atomic_get_property = intel_connector_atomic_get_property,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 };
 
 static const struct drm_connector_helper_funcs intel_dvo_connector_helper_funcs = {
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 995c5b2..b13a8be 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1618,6 +1618,7 @@ static const struct drm_connector_funcs intel_hdmi_connector_funcs = {
 	.atomic_get_property = intel_connector_atomic_get_property,
 	.destroy = intel_hdmi_destroy,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 };
 
 static const struct drm_connector_helper_funcs intel_hdmi_connector_helper_funcs = {
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 24e8730..2b008b02 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -535,6 +535,7 @@ static const struct drm_connector_funcs intel_lvds_connector_funcs = {
 	.atomic_get_property = intel_connector_atomic_get_property,
 	.destroy = intel_lvds_destroy,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 };
 
 static const struct drm_encoder_funcs intel_lvds_enc_funcs = {
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 9e554c2..f5b7e1e 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2194,6 +2194,7 @@ static const struct drm_connector_funcs intel_sdvo_connector_funcs = {
 	.atomic_get_property = intel_connector_atomic_get_property,
 	.destroy = intel_sdvo_destroy,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 };
 
 static const struct drm_connector_helper_funcs intel_sdvo_connector_helper_funcs = {
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 4e6684e..bc1d9d7 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1516,6 +1516,7 @@ static const struct drm_connector_funcs intel_tv_connector_funcs = {
 	.atomic_get_property = intel_connector_atomic_get_property,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 };
 
 static const struct drm_connector_helper_funcs intel_tv_connector_helper_funcs = {
-- 
2.1.0

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

[PATCH 11/23] drm/i915: Copy the staged connector config to the legacy atomic state

[Ander Conselvan de Oliveira]

With this in place, we can start converting pieces of the modeset code
to look at the connector atomic state instead of the staged config.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 108d3d2..4e90cb4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11607,9 +11607,11 @@ intel_set_config_compute_mode_changes(struct drm_mode_set *set,
 static int
 intel_modeset_stage_output_state(struct drm_device *dev,
 				 struct drm_mode_set *set,
-				 struct intel_set_config *config)
+				 struct intel_set_config *config,
+				 struct drm_atomic_state *state)
 {
 	struct intel_connector *connector;
+	struct drm_connector_state *connector_state;
 	struct intel_encoder *encoder;
 	struct intel_crtc *crtc;
 	int ro;
@@ -11673,6 +11675,14 @@ intel_modeset_stage_output_state(struct drm_device *dev,
 		}
 		connector->new_encoder->new_crtc = to_intel_crtc(new_crtc);
 
+		connector_state =
+			drm_atomic_get_connector_state(state, &connector->base);
+		if (IS_ERR(connector_state))
+			return PTR_ERR(connector_state);
+
+		connector_state->crtc = new_crtc;
+		connector_state->best_encoder = &connector->new_encoder->base;
+
 		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] to [CRTC:%d]\n",
 			connector->base.base.id,
 			connector->base.name,
@@ -11705,9 +11715,16 @@ intel_modeset_stage_output_state(struct drm_device *dev,
 	}
 	/* Now we've also updated encoder->new_crtc for all encoders. */
 	for_each_intel_connector(dev, connector) {
-		if (connector->new_encoder)
+		connector_state =
+			drm_atomic_get_connector_state(state, &connector->base);
+
+		if (connector->new_encoder) {
 			if (connector->new_encoder != connector->encoder)
 				connector->encoder = connector->new_encoder;
+		} else {
+			connector_state->crtc = NULL;
+		}
+
 	}
 	for_each_intel_crtc(dev, crtc) {
 		crtc->new_enabled = false;
@@ -11816,7 +11833,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 
 	state->acquire_ctx = dev->mode_config.acquire_ctx;
 
-	ret = intel_modeset_stage_output_state(dev, set, config);
+	ret = intel_modeset_stage_output_state(dev, set, config, state);
 	if (ret)
 		goto fail;
 
-- 
2.1.0

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

Re: [PATCH 11/23] drm/i915: Copy the staged connector config to the legacy atomic state

[Daniel Vetter]

On Tue, Mar 03, 2015 at 03:22:05PM +0200, Ander Conselvan de Oliveira wrote:
> With this in place, we can start converting pieces of the modeset code
> to look at the connector atomic state instead of the staged config.
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 108d3d2..4e90cb4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11607,9 +11607,11 @@ intel_set_config_compute_mode_changes(struct drm_mode_set *set,
>  static int
>  intel_modeset_stage_output_state(struct drm_device *dev,
>  				 struct drm_mode_set *set,
> -				 struct intel_set_config *config)
> +				 struct intel_set_config *config,
> +				 struct drm_atomic_state *state)
>  {
>  	struct intel_connector *connector;
> +	struct drm_connector_state *connector_state;
>  	struct intel_encoder *encoder;
>  	struct intel_crtc *crtc;
>  	int ro;
> @@ -11673,6 +11675,14 @@ intel_modeset_stage_output_state(struct drm_device *dev,
>  		}
>  		connector->new_encoder->new_crtc = to_intel_crtc(new_crtc);
>  
> +		connector_state =
> +			drm_atomic_get_connector_state(state, &connector->base);
> +		if (IS_ERR(connector_state))
> +			return PTR_ERR(connector_state);
> +
> +		connector_state->crtc = new_crtc;
> +		connector_state->best_encoder = &connector->new_encoder->base;
> +
>  		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] to [CRTC:%d]\n",
>  			connector->base.base.id,
>  			connector->base.name,
> @@ -11705,9 +11715,16 @@ intel_modeset_stage_output_state(struct drm_device *dev,
>  	}
>  	/* Now we've also updated encoder->new_crtc for all encoders. */
>  	for_each_intel_connector(dev, connector) {
> -		if (connector->new_encoder)
> +		connector_state =
> +			drm_atomic_get_connector_state(state, &connector->base);
> +
> +		if (connector->new_encoder) {
>  			if (connector->new_encoder != connector->encoder)
>  				connector->encoder = connector->new_encoder;
> +		} else {
> +			connector_state->crtc = NULL;
> +		}
> +

Unecessary line. Of course you've put that in there to check that I
actually read your patches ;-)

Cheers, Daniel

>  	}
>  	for_each_intel_crtc(dev, crtc) {
>  		crtc->new_enabled = false;
> @@ -11816,7 +11833,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  
>  	state->acquire_ctx = dev->mode_config.acquire_ctx;
>  
> -	ret = intel_modeset_stage_output_state(dev, set, config);
> +	ret = intel_modeset_stage_output_state(dev, set, config, state);
>  	if (ret)
>  		goto fail;
>  
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/23] drm/i915: Copy the staged connector config to the legacy atomic state

[Conselvan De Oliveira, Ander]

On Wed, 2015-03-04 at 16:46 +0100, Daniel Vetter wrote:
> On Tue, Mar 03, 2015 at 03:22:05PM +0200, Ander Conselvan de Oliveira wrote:
> > With this in place, we can start converting pieces of the modeset code
> > to look at the connector atomic state instead of the staged config.
> > 
> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 23 ++++++++++++++++++++---
> >  1 file changed, 20 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 108d3d2..4e90cb4 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11607,9 +11607,11 @@ intel_set_config_compute_mode_changes(struct drm_mode_set *set,
> >  static int
> >  intel_modeset_stage_output_state(struct drm_device *dev,
> >  				 struct drm_mode_set *set,
> > -				 struct intel_set_config *config)
> > +				 struct intel_set_config *config,
> > +				 struct drm_atomic_state *state)
> >  {
> >  	struct intel_connector *connector;
> > +	struct drm_connector_state *connector_state;
> >  	struct intel_encoder *encoder;
> >  	struct intel_crtc *crtc;
> >  	int ro;
> > @@ -11673,6 +11675,14 @@ intel_modeset_stage_output_state(struct drm_device *dev,
> >  		}
> >  		connector->new_encoder->new_crtc = to_intel_crtc(new_crtc);
> >  
> > +		connector_state =
> > +			drm_atomic_get_connector_state(state, &connector->base);
> > +		if (IS_ERR(connector_state))
> > +			return PTR_ERR(connector_state);
> > +
> > +		connector_state->crtc = new_crtc;
> > +		connector_state->best_encoder = &connector->new_encoder->base;
> > +
> >  		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] to [CRTC:%d]\n",
> >  			connector->base.base.id,
> >  			connector->base.name,
> > @@ -11705,9 +11715,16 @@ intel_modeset_stage_output_state(struct drm_device *dev,
> >  	}
> >  	/* Now we've also updated encoder->new_crtc for all encoders. */
> >  	for_each_intel_connector(dev, connector) {
> > -		if (connector->new_encoder)
> > +		connector_state =
> > +			drm_atomic_get_connector_state(state, &connector->base);
> > +
> > +		if (connector->new_encoder) {
> >  			if (connector->new_encoder != connector->encoder)
> >  				connector->encoder = connector->new_encoder;
> > +		} else {
> > +			connector_state->crtc = NULL;
> > +		}
> > +
> 
> Unecessary line. Of course you've put that in there to check that I
> actually read your patches ;-)

It actually is needed, since the other hunk we only update the
connector's crtc when it has an enabled connector. It makes sense for
the staged config, since the new_crtc field is in the encoder, which
would be NULL in that case. The loop just above this one sets new_crtc
to NULL for these encoders, but for the connectors it was just more
convenient to set it here.

Ander

> 
> Cheers, Daniel
> 
> >  	}
> >  	for_each_intel_crtc(dev, crtc) {
> >  		crtc->new_enabled = false;
> > @@ -11816,7 +11833,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
> >  
> >  	state->acquire_ctx = dev->mode_config.acquire_ctx;
> >  
> > -	ret = intel_modeset_stage_output_state(dev, set, config);
> > +	ret = intel_modeset_stage_output_state(dev, set, config, state);
> >  	if (ret)
> >  		goto fail;
> >  
> > -- 
> > 2.1.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/23] drm/i915: Copy the staged connector config to the legacy atomic state

[Daniel Vetter]

On Wed, Mar 4, 2015 at 5:58 PM, Conselvan De Oliveira, Ander
<ander.conselvan.de.oliveira@intel.com> wrote:
>> > +

I meant this empty line between 2 closing braces ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 12/23] drm/i915: Don't use encoder->new_crtc in intel_modeset_pipe_config()

[Ander Conselvan de Oliveira]

Move towards atomic by using the legacy modeset's drm_atomic_state
instead.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4e90cb4..0dbd20f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10378,8 +10378,11 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
 {
 	struct drm_device *dev = crtc->dev;
 	struct intel_encoder *encoder;
+	struct intel_connector *connector;
+	struct drm_connector_state *connector_state;
 	struct intel_crtc_state *pipe_config;
 	int plane_bpp, ret = -EINVAL;
+	int i;
 	bool retry = true;
 
 	if (!check_encoder_cloning(to_intel_crtc(crtc))) {
@@ -10449,15 +10452,27 @@ encoder_retry:
 	drm_mode_set_crtcinfo(&pipe_config->base.adjusted_mode,
 			      CRTC_STEREO_DOUBLE);
 
+	ret = drm_atomic_add_affected_connectors(state, crtc);
+	if (ret) {
+		DRM_DEBUG_KMS("Failed to add affected connectors\n");
+		goto fail;
+	}
+
 	/* Pass our mode to the connectors and the CRTC to give them a chance to
 	 * adjust it according to limitations or connector properties, and also
 	 * a chance to reject the mode entirely.
 	 */
-	for_each_intel_encoder(dev, encoder) {
+	for (i = 0; i < state->num_connector; i++) {
+		connector = to_intel_connector(state->connectors[i]);
+		if (!connector)
+			continue;
 
-		if (&encoder->new_crtc->base != crtc)
+		connector_state = state->connector_states[i];
+		if (connector_state->crtc != crtc)
 			continue;
 
+		encoder = to_intel_encoder(connector_state->best_encoder);
+
 		if (!(encoder->compute_config(encoder, pipe_config))) {
 			DRM_DEBUG_KMS("Encoder config failure\n");
 			goto fail;
-- 
2.1.0

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

[PATCH 13/23] drm/i915: Don't use encoder->new_crtc in compute_baseline_pipe_bpp()

[Ander Conselvan de Oliveira]

Move towards atomic by using the legacy modeset's drm_atomic_state
instead.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0dbd20f..9905200 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10159,8 +10159,9 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc,
 			  struct intel_crtc_state *pipe_config)
 {
 	struct drm_device *dev = crtc->base.dev;
+	struct drm_atomic_state *state;
 	struct intel_connector *connector;
-	int bpp;
+	int bpp, i;
 
 	switch (fb->pixel_format) {
 	case DRM_FORMAT_C8:
@@ -10200,10 +10201,13 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc,
 
 	pipe_config->pipe_bpp = bpp;
 
+	state = pipe_config->base.state;
+
 	/* Clamp display bpp to EDID value */
-	for_each_intel_connector(dev, connector) {
-		if (!connector->new_encoder ||
-		    connector->new_encoder->new_crtc != crtc)
+	for (i = 0; i < state->num_connector; i++) {
+		connector = to_intel_connector(state->connectors[i]);
+		if (!connector ||
+		    state->connector_states[i]->crtc != &crtc->base)
 			continue;
 
 		connected_sink_compute_bpp(connector, pipe_config);
-- 
2.1.0

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

[PATCH 14/23] drm/i915: Don't depend on encoder->new_crtc in intel_dp_compute_config()

[Ander Conselvan de Oliveira]

Move towards atomic by using the legacy modeset's drm_atomic_state
instead.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 5accf3f..a477e07 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1159,7 +1159,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
 	enum port port = dp_to_dig_port(intel_dp)->port;
-	struct intel_crtc *intel_crtc = encoder->new_crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc);
 	struct intel_connector *intel_connector = intel_dp->attached_connector;
 	int lane_count, clock;
 	int min_lane_count = 1;
-- 
2.1.0

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

[PATCH 15/23] drm/i915: Don't depend on encoder->new_crtc in intel_hdmi_compute_config

[Ander Conselvan de Oliveira]

Move towards atomic by using the legacy modeset's drm_atomic_state
instead.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index b13a8be..cacbafd 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -951,19 +951,30 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
 	return MODE_OK;
 }
 
-static bool hdmi_12bpc_possible(struct intel_crtc *crtc)
+static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state)
 {
-	struct drm_device *dev = crtc->base.dev;
+	struct drm_device *dev = crtc_state->base.crtc->dev;
+	struct drm_atomic_state *state;
 	struct intel_encoder *encoder;
+	struct drm_connector_state *connector_state;
 	int count = 0, count_hdmi = 0;
+	int i;
 
 	if (HAS_GMCH_DISPLAY(dev))
 		return false;
 
-	for_each_intel_encoder(dev, encoder) {
-		if (encoder->new_crtc != crtc)
+	state = crtc_state->base.state;
+
+	for (i = 0; i < state->num_connector; i++) {
+		if (!state->connectors[i])
 			continue;
 
+		connector_state = state->connector_states[i];
+		if (connector_state->crtc != crtc_state->base.crtc)
+			continue;
+
+		encoder = to_intel_encoder(connector_state->best_encoder);
+
 		count_hdmi += encoder->type == INTEL_OUTPUT_HDMI;
 		count++;
 	}
@@ -1020,7 +1031,7 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
 	 */
 	if (pipe_config->pipe_bpp > 8*3 && pipe_config->has_hdmi_sink &&
 	    clock_12bpc <= portclock_limit &&
-	    hdmi_12bpc_possible(encoder->new_crtc)) {
+	    hdmi_12bpc_possible(pipe_config)) {
 		DRM_DEBUG_KMS("picking bpc to 12 for HDMI output\n");
 		desired_bpp = 12*3;
 
-- 
2.1.0

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

[PATCH 16/23] drm/i915: Use atomic state in intel_ddi_crtc_get_new_encoder()

[Ander Conselvan de Oliveira]

Instead of using connector->new_encoder, get the same information from
the pipe_config, thus making the function ready for the atomic
conversion.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 985d531..91e7cb1 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -494,17 +494,23 @@ intel_ddi_get_crtc_encoder(struct drm_crtc *crtc)
 }
 
 static struct intel_encoder *
-intel_ddi_get_crtc_new_encoder(struct intel_crtc *crtc)
+intel_ddi_get_crtc_new_encoder(struct intel_crtc_state *crtc_state)
 {
-	struct drm_device *dev = crtc->base.dev;
-	struct intel_encoder *intel_encoder, *ret = NULL;
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	struct intel_encoder *ret = NULL;
+	struct drm_atomic_state *state;
 	int num_encoders = 0;
+	int i;
 
-	for_each_intel_encoder(dev, intel_encoder) {
-		if (intel_encoder->new_crtc == crtc) {
-			ret = intel_encoder;
-			num_encoders++;
-		}
+	state = crtc_state->base.state;
+
+	for (i = 0; i < state->num_connector; i++) {
+		if (!state->connectors[i] ||
+		    state->connector_states[i]->crtc != crtc_state->base.crtc)
+			continue;
+
+		ret = to_intel_encoder(state->connector_states[i]->best_encoder);
+		num_encoders++;
 	}
 
 	WARN(num_encoders != 1, "%d encoders on crtc for pipe %c\n", num_encoders,
@@ -1209,7 +1215,7 @@ bool intel_ddi_pll_select(struct intel_crtc *intel_crtc,
 {
 	struct drm_device *dev = intel_crtc->base.dev;
 	struct intel_encoder *intel_encoder =
-		intel_ddi_get_crtc_new_encoder(intel_crtc);
+		intel_ddi_get_crtc_new_encoder(crtc_state);
 	int clock = crtc_state->port_clock;
 
 	if (IS_SKYLAKE(dev))
-- 
2.1.0

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

[PATCH 17/23] drm/i915: Don't use staged config in intel_dp_mst_compute_config()

[Ander Conselvan de Oliveira]

Move towards atomic by using the legacy modeset's drm_atomic_state
instead.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 5c06a06..b132fe6 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -36,11 +36,11 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 	struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder->base);
 	struct intel_digital_port *intel_dig_port = intel_mst->primary;
 	struct intel_dp *intel_dp = &intel_dig_port->dp;
-	struct drm_device *dev = encoder->base.dev;
-	int bpp;
+	struct drm_atomic_state *state;
+	int bpp, i;
 	int lane_count, slots;
 	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
-	struct intel_connector *found = NULL, *intel_connector;
+	struct intel_connector *found = NULL;
 	int mst_pbn;
 
 	pipe_config->dp_encoder_is_mst = true;
@@ -58,9 +58,14 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 	pipe_config->pipe_bpp = 24;
 	pipe_config->port_clock = drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
 
-	for_each_intel_connector(dev, intel_connector) {
-		if (intel_connector->new_encoder == encoder) {
-			found = intel_connector;
+	state = pipe_config->base.state;
+
+	for (i = 0; i < state->num_connector; i++) {
+		if (!state->connectors[i])
+			continue;
+
+		if (state->connector_states[i]->best_encoder == &encoder->base) {
+			found = to_intel_connector(state->connectors[i]);
 			break;
 		}
 	}
-- 
2.1.0

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

[PATCH 18/23] drm/i915: Don't use encoder->new_crtc in intel_lvds_compute_config()

[Ander Conselvan de Oliveira]

Move towards atomic by using the legacy modeset's drm_atomic_state
instead.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_lvds.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 2b008b02..06d2da3 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -286,7 +286,7 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
 	struct intel_connector *intel_connector =
 		&lvds_encoder->attached_connector->base;
 	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
-	struct intel_crtc *intel_crtc = lvds_encoder->base.new_crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc);
 	unsigned int lvds_bpp;
 
 	/* Should never happen!! */
-- 
2.1.0

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

[PATCH 19/23] drm/i915: Pass an atomic state to modeset_global_resources() functions

[Ander Conselvan de Oliveira]

Follow up patches will convert some functions called from there to use
the atomic state, instead of directly accessing the new or current
config. This patch just changes the parameters, but shouldn't have any
functional changes.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  2 +-
 drivers/gpu/drm/i915/intel_display.c | 13 ++++++++-----
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 29c6fbc..0c6ba2d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -550,7 +550,7 @@ struct drm_i915_display_funcs {
 				 struct drm_crtc *crtc,
 				 uint32_t sprite_width, uint32_t sprite_height,
 				 int pixel_size, bool enable, bool scaled);
-	void (*modeset_global_resources)(struct drm_device *dev);
+	void (*modeset_global_resources)(struct drm_atomic_state *state);
 	/* Returns the active state of the crtc, and if the crtc is active,
 	 * fills out the pipe-config with the hw state. */
 	bool (*get_pipe_config)(struct intel_crtc *,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9905200..64751b6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3156,8 +3156,9 @@ static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
 		crtc->config->has_pch_encoder;
 }
 
-static void ivb_modeset_global_resources(struct drm_device *dev)
+static void ivb_modeset_global_resources(struct drm_atomic_state *state)
 {
+	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *pipe_B_crtc =
 		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
@@ -4845,8 +4846,9 @@ static unsigned long get_crtc_power_domains(struct drm_crtc *crtc)
 	return mask;
 }
 
-static void modeset_update_crtc_power_domains(struct drm_device *dev)
+static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
 {
+	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long pipe_domains[I915_MAX_PIPES] = { 0, };
 	struct intel_crtc *crtc;
@@ -4868,7 +4870,7 @@ static void modeset_update_crtc_power_domains(struct drm_device *dev)
 	}
 
 	if (dev_priv->display.modeset_global_resources)
-		dev_priv->display.modeset_global_resources(dev);
+		dev_priv->display.modeset_global_resources(state);
 
 	for_each_intel_crtc(dev, crtc) {
 		enum intel_display_power_domain domain;
@@ -5083,8 +5085,9 @@ static void valleyview_modeset_global_pipes(struct drm_device *dev,
 			*prepare_pipes |= (1 << intel_crtc->pipe);
 }
 
-static void valleyview_modeset_global_resources(struct drm_device *dev)
+static void valleyview_modeset_global_resources(struct drm_atomic_state *state)
 {
+	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int max_pixclk = intel_mode_max_pixclk(dev_priv);
 	int req_cdclk = valleyview_calc_cdclk(dev_priv, max_pixclk);
@@ -11350,7 +11353,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 	 * update the the output configuration. */
 	intel_modeset_update_state(dev, prepare_pipes);
 
-	modeset_update_crtc_power_domains(dev);
+	modeset_update_crtc_power_domains(pipe_config->base.state);
 
 	/* Set up the DPLL and any encoders state that needs to adjust or depend
 	 * on the DPLL.
-- 
2.1.0

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

[PATCH 20/23] drm/i915: Use atomic state in pipe_has_enabled_pch()

[Ander Conselvan de Oliveira]

This function is called indirectly by intel_crtc_compute_config(),
which needs to be converted to work only with an atomic state.

---

I'm not sure what are the implications of ignoring intel_crtc->active in
pipe_has_enabled_pch(). If we allow a config because the third pipe is
enabled but not active, wouldn't we run into trouble when we tried to
activate the crtc?

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 64751b6..518903e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3150,10 +3150,9 @@ static void intel_fdi_normal_train(struct drm_crtc *crtc)
 			   FDI_FE_ERRC_ENABLE);
 }
 
-static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
+static bool pipe_has_enabled_pch(struct intel_crtc_state *crtc_state)
 {
-	return crtc->base.state->enable && crtc->active &&
-		crtc->config->has_pch_encoder;
+	return crtc_state->base.enable && crtc_state->has_pch_encoder;
 }
 
 static void ivb_modeset_global_resources(struct drm_atomic_state *state)
@@ -3164,15 +3163,21 @@ static void ivb_modeset_global_resources(struct drm_atomic_state *state)
 		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
 	struct intel_crtc *pipe_C_crtc =
 		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_C]);
+	struct intel_crtc_state *pipe_B_crtc_state, *pipe_C_crtc_state;
 	uint32_t temp;
 
+	pipe_B_crtc_state = intel_atomic_get_crtc_state(state, pipe_B_crtc);
+	pipe_C_crtc_state = intel_atomic_get_crtc_state(state, pipe_C_crtc);
+	if (WARN_ON(IS_ERR(pipe_B_crtc_state) || IS_ERR(pipe_C_crtc_state)))
+		return;
+
 	/*
 	 * When everything is off disable fdi C so that we could enable fdi B
 	 * with all lanes. Note that we don't care about enabled pipes without
 	 * an enabled pch encoder.
 	 */
-	if (!pipe_has_enabled_pch(pipe_B_crtc) &&
-	    !pipe_has_enabled_pch(pipe_C_crtc)) {
+	if (!pipe_has_enabled_pch(pipe_B_crtc_state) &&
+	    !pipe_has_enabled_pch(pipe_C_crtc_state)) {
 		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
 		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
 
@@ -5528,6 +5533,9 @@ static bool ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *pipe_B_crtc =
 		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
+	struct intel_crtc_state *pipe_B_crtc_state =
+		intel_atomic_get_crtc_state(pipe_config->base.state,
+					    pipe_B_crtc);
 
 	DRM_DEBUG_KMS("checking fdi config on pipe %c, lanes %i\n",
 		      pipe_name(pipe), pipe_config->fdi_lanes);
@@ -5563,8 +5571,8 @@ static bool ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe,
 		}
 		return true;
 	case PIPE_C:
-		if (!pipe_has_enabled_pch(pipe_B_crtc) ||
-		    pipe_B_crtc->config->fdi_lanes <= 2) {
+		if (!pipe_has_enabled_pch(pipe_B_crtc_state) ||
+		    pipe_B_crtc_state->fdi_lanes <= 2) {
 			if (pipe_config->fdi_lanes > 2) {
 				DRM_DEBUG_KMS("invalid shared fdi lane config on pipe %c: %i lanes\n",
 					      pipe_name(pipe), pipe_config->fdi_lanes);
-- 
2.1.0

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

Re: [PATCH 20/23] drm/i915: Use atomic state in pipe_has_enabled_pch()

[Daniel Vetter]

On Tue, Mar 03, 2015 at 03:22:14PM +0200, Ander Conselvan de Oliveira wrote:
> This function is called indirectly by intel_crtc_compute_config(),
> which needs to be converted to work only with an atomic state.
> 
> ---
> 
> I'm not sure what are the implications of ignoring intel_crtc->active in
> pipe_has_enabled_pch(). If we allow a config because the third pipe is
> enabled but not active, wouldn't we run into trouble when we tried to
> activate the crtc?

Once we have more of atomic ready and switched to the helpers (or
something equivalent) for enabling/disabling display pipes we need to
remove our usage of intel_crtc->active. Maybe keep it as a debug check.

The new rules are:
- crtc_state->enable controls resource allocation, i.e. this is the bit we
  need to look at for checking shared resources and stuff like that.
- crtc_state->active (and also intel_crtc->active) only control/track the
  physical active state of the pipe (controlled by dpms or the new atomic
  active property). Changing crtc_state->active may never fail.

I think your change actually fixes a bug with the fdi config check code:
If the other pipe is in dpms off we might steal the fdi lanes and then the
subsequent dpms on could fail.

Perhaps split out the removal of intel_crtc->active from the
has_enabled_pch as a bugfix? Would at least help with review.

It should be possible to reproduce this on an ivb with the following
sequence:

1. Enable big mode (uses more than 2 lanes) on pipe B.
2. dpms off pipe B
3. Enable big mode (uses more than 2 lanes) on pipe C.
4. dpms on pipe B should go boom now on one of the FDI checks.

Maybe we need another modeset on pipe C to hit them though or something
like that, we don't have full checks in the dpms patch.

Thanks, Daniel

> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 64751b6..518903e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3150,10 +3150,9 @@ static void intel_fdi_normal_train(struct drm_crtc *crtc)
>  			   FDI_FE_ERRC_ENABLE);
>  }
>  
> -static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
> +static bool pipe_has_enabled_pch(struct intel_crtc_state *crtc_state)
>  {
> -	return crtc->base.state->enable && crtc->active &&
> -		crtc->config->has_pch_encoder;
> +	return crtc_state->base.enable && crtc_state->has_pch_encoder;
>  }
>  
>  static void ivb_modeset_global_resources(struct drm_atomic_state *state)
> @@ -3164,15 +3163,21 @@ static void ivb_modeset_global_resources(struct drm_atomic_state *state)
>  		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
>  	struct intel_crtc *pipe_C_crtc =
>  		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_C]);
> +	struct intel_crtc_state *pipe_B_crtc_state, *pipe_C_crtc_state;
>  	uint32_t temp;
>  
> +	pipe_B_crtc_state = intel_atomic_get_crtc_state(state, pipe_B_crtc);
> +	pipe_C_crtc_state = intel_atomic_get_crtc_state(state, pipe_C_crtc);
> +	if (WARN_ON(IS_ERR(pipe_B_crtc_state) || IS_ERR(pipe_C_crtc_state)))
> +		return;
> +
>  	/*
>  	 * When everything is off disable fdi C so that we could enable fdi B
>  	 * with all lanes. Note that we don't care about enabled pipes without
>  	 * an enabled pch encoder.
>  	 */
> -	if (!pipe_has_enabled_pch(pipe_B_crtc) &&
> -	    !pipe_has_enabled_pch(pipe_C_crtc)) {
> +	if (!pipe_has_enabled_pch(pipe_B_crtc_state) &&
> +	    !pipe_has_enabled_pch(pipe_C_crtc_state)) {
>  		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
>  		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
>  
> @@ -5528,6 +5533,9 @@ static bool ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *pipe_B_crtc =
>  		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
> +	struct intel_crtc_state *pipe_B_crtc_state =
> +		intel_atomic_get_crtc_state(pipe_config->base.state,
> +					    pipe_B_crtc);
>  
>  	DRM_DEBUG_KMS("checking fdi config on pipe %c, lanes %i\n",
>  		      pipe_name(pipe), pipe_config->fdi_lanes);
> @@ -5563,8 +5571,8 @@ static bool ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe,
>  		}
>  		return true;
>  	case PIPE_C:
> -		if (!pipe_has_enabled_pch(pipe_B_crtc) ||
> -		    pipe_B_crtc->config->fdi_lanes <= 2) {
> +		if (!pipe_has_enabled_pch(pipe_B_crtc_state) ||
> +		    pipe_B_crtc_state->fdi_lanes <= 2) {
>  			if (pipe_config->fdi_lanes > 2) {
>  				DRM_DEBUG_KMS("invalid shared fdi lane config on pipe %c: %i lanes\n",
>  					      pipe_name(pipe), pipe_config->fdi_lanes);
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 21/23] drm/i915: Convert intel_pipe_will_have_type() to using atomic state

[Ander Conselvan de Oliveira]

Pass a crtc_state to it and find whether the pipe has an encoder of a
given type by looking at the drm_atomic_state the crtc_state points to.

Note that is possible to reach i9xx_get_refclk() without a proper atomic
state, since in the function vlv_force_pll_on() a minimally initialized
crtc_state is allocated in the stack. With the current code, it is not
possible to end up in a call to intel_pipe_will_have_type() with that
bogus atomic state. To avoid future problems, a comment is added to warn
people changing that code.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |   2 +-
 drivers/gpu/drm/i915/intel_display.c | 134 +++++++++++++++++++++--------------
 2 files changed, 83 insertions(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0c6ba2d..aab4421 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -541,7 +541,7 @@ struct drm_i915_display_funcs {
 	 * Returns true on success, false on failure.
 	 */
 	bool (*find_dpll)(const struct intel_limit *limit,
-			  struct intel_crtc *crtc,
+			  struct intel_crtc_state *crtc_state,
 			  int target, int refclk,
 			  struct dpll *match_clock,
 			  struct dpll *best_clock);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 518903e..f3652f9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -431,25 +431,37 @@ bool intel_pipe_has_type(struct intel_crtc *crtc, enum intel_output_type type)
  * intel_pipe_has_type() but looking at encoder->new_crtc instead of
  * encoder->crtc.
  */
-static bool intel_pipe_will_have_type(struct intel_crtc *crtc, int type)
+static bool intel_pipe_will_have_type(const struct intel_crtc_state *crtc_state,
+				      int type)
 {
-	struct drm_device *dev = crtc->base.dev;
+	struct drm_atomic_state *state = crtc_state->base.state;
+	struct drm_connector_state *connector_state;
 	struct intel_encoder *encoder;
+	int i;
+
+	for (i = 0; i < state->num_connector; i++) {
+		if (!state->connectors[i])
+			continue;
+
+		connector_state = state->connector_states[i];
+		if (connector_state->crtc != crtc_state->base.crtc)
+			continue;
 
-	for_each_intel_encoder(dev, encoder)
-		if (encoder->new_crtc == crtc && encoder->type == type)
+		encoder = to_intel_encoder(connector_state->best_encoder);
+		if (encoder->type == type)
 			return true;
+	}
 
 	return false;
 }
 
-static const intel_limit_t *intel_ironlake_limit(struct intel_crtc *crtc,
-						int refclk)
+static const intel_limit_t *
+intel_ironlake_limit(struct intel_crtc_state *crtc_state, int refclk)
 {
-	struct drm_device *dev = crtc->base.dev;
+	struct drm_device *dev = crtc_state->base.crtc->dev;
 	const intel_limit_t *limit;
 
-	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
+	if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) {
 		if (intel_is_dual_link_lvds(dev)) {
 			if (refclk == 100000)
 				limit = &intel_limits_ironlake_dual_lvds_100m;
@@ -467,20 +479,21 @@ static const intel_limit_t *intel_ironlake_limit(struct intel_crtc *crtc,
 	return limit;
 }
 
-static const intel_limit_t *intel_g4x_limit(struct intel_crtc *crtc)
+static const intel_limit_t *
+intel_g4x_limit(struct intel_crtc_state *crtc_state)
 {
-	struct drm_device *dev = crtc->base.dev;
+	struct drm_device *dev = crtc_state->base.crtc->dev;
 	const intel_limit_t *limit;
 
-	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
+	if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) {
 		if (intel_is_dual_link_lvds(dev))
 			limit = &intel_limits_g4x_dual_channel_lvds;
 		else
 			limit = &intel_limits_g4x_single_channel_lvds;
-	} else if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_HDMI) ||
-		   intel_pipe_will_have_type(crtc, INTEL_OUTPUT_ANALOG)) {
+	} else if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_HDMI) ||
+		   intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_ANALOG)) {
 		limit = &intel_limits_g4x_hdmi;
-	} else if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_SDVO)) {
+	} else if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_SDVO)) {
 		limit = &intel_limits_g4x_sdvo;
 	} else /* The option is for other outputs */
 		limit = &intel_limits_i9xx_sdvo;
@@ -488,17 +501,18 @@ static const intel_limit_t *intel_g4x_limit(struct intel_crtc *crtc)
 	return limit;
 }
 
-static const intel_limit_t *intel_limit(struct intel_crtc *crtc, int refclk)
+static const intel_limit_t *
+intel_limit(struct intel_crtc_state *crtc_state, int refclk)
 {
-	struct drm_device *dev = crtc->base.dev;
+	struct drm_device *dev = crtc_state->base.crtc->dev;
 	const intel_limit_t *limit;
 
 	if (HAS_PCH_SPLIT(dev))
-		limit = intel_ironlake_limit(crtc, refclk);
+		limit = intel_ironlake_limit(crtc_state, refclk);
 	else if (IS_G4X(dev)) {
-		limit = intel_g4x_limit(crtc);
+		limit = intel_g4x_limit(crtc_state);
 	} else if (IS_PINEVIEW(dev)) {
-		if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS))
+		if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS))
 			limit = &intel_limits_pineview_lvds;
 		else
 			limit = &intel_limits_pineview_sdvo;
@@ -507,14 +521,14 @@ static const intel_limit_t *intel_limit(struct intel_crtc *crtc, int refclk)
 	} else if (IS_VALLEYVIEW(dev)) {
 		limit = &intel_limits_vlv;
 	} else if (!IS_GEN2(dev)) {
-		if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS))
+		if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS))
 			limit = &intel_limits_i9xx_lvds;
 		else
 			limit = &intel_limits_i9xx_sdvo;
 	} else {
-		if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS))
+		if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS))
 			limit = &intel_limits_i8xx_lvds;
-		else if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_DVO))
+		else if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_DVO))
 			limit = &intel_limits_i8xx_dvo;
 		else
 			limit = &intel_limits_i8xx_dac;
@@ -601,15 +615,17 @@ static bool intel_PLL_is_valid(struct drm_device *dev,
 }
 
 static bool
-i9xx_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
+i9xx_find_best_dpll(const intel_limit_t *limit,
+		    struct intel_crtc_state *crtc_state,
 		    int target, int refclk, intel_clock_t *match_clock,
 		    intel_clock_t *best_clock)
 {
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
 	struct drm_device *dev = crtc->base.dev;
 	intel_clock_t clock;
 	int err = target;
 
-	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
+	if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) {
 		/*
 		 * For LVDS just rely on its current settings for dual-channel.
 		 * We haven't figured out how to reliably set up different
@@ -662,15 +678,17 @@ i9xx_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
 }
 
 static bool
-pnv_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
+pnv_find_best_dpll(const intel_limit_t *limit,
+		   struct intel_crtc_state *crtc_state,
 		   int target, int refclk, intel_clock_t *match_clock,
 		   intel_clock_t *best_clock)
 {
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
 	struct drm_device *dev = crtc->base.dev;
 	intel_clock_t clock;
 	int err = target;
 
-	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
+	if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) {
 		/*
 		 * For LVDS just rely on its current settings for dual-channel.
 		 * We haven't figured out how to reliably set up different
@@ -721,10 +739,12 @@ pnv_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
 }
 
 static bool
-g4x_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
+g4x_find_best_dpll(const intel_limit_t *limit,
+		   struct intel_crtc_state *crtc_state,
 		   int target, int refclk, intel_clock_t *match_clock,
 		   intel_clock_t *best_clock)
 {
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
 	struct drm_device *dev = crtc->base.dev;
 	intel_clock_t clock;
 	int max_n;
@@ -733,7 +753,7 @@ g4x_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
 	int err_most = (target >> 8) + (target >> 9);
 	found = false;
 
-	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
+	if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) {
 		if (intel_is_dual_link_lvds(dev))
 			clock.p2 = limit->p2.p2_fast;
 		else
@@ -778,10 +798,12 @@ g4x_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
 }
 
 static bool
-vlv_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
+vlv_find_best_dpll(const intel_limit_t *limit,
+		   struct intel_crtc_state *crtc_state,
 		   int target, int refclk, intel_clock_t *match_clock,
 		   intel_clock_t *best_clock)
 {
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
 	struct drm_device *dev = crtc->base.dev;
 	intel_clock_t clock;
 	unsigned int bestppm = 1000000;
@@ -835,10 +857,12 @@ vlv_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
 }
 
 static bool
-chv_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
+chv_find_best_dpll(const intel_limit_t *limit,
+		   struct intel_crtc_state *crtc_state,
 		   int target, int refclk, intel_clock_t *match_clock,
 		   intel_clock_t *best_clock)
 {
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
 	struct drm_device *dev = crtc->base.dev;
 	intel_clock_t clock;
 	uint64_t m2;
@@ -5678,7 +5702,7 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
 	 * - LVDS dual channel mode
 	 * - Double wide pipe
 	 */
-	if ((intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS) &&
+	if ((intel_pipe_will_have_type(pipe_config, INTEL_OUTPUT_LVDS) &&
 	     intel_is_dual_link_lvds(dev)) || pipe_config->double_wide)
 		pipe_config->pipe_src_w &= ~1;
 
@@ -5861,15 +5885,21 @@ static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
 		&& !(dev_priv->quirks & QUIRK_LVDS_SSC_DISABLE);
 }
 
-static int i9xx_get_refclk(struct intel_crtc *crtc, int num_connectors)
+static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state,
+			   int num_connectors)
 {
-	struct drm_device *dev = crtc->base.dev;
+	struct drm_device *dev = crtc_state->base.crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int refclk;
 
 	if (IS_VALLEYVIEW(dev)) {
+		/* Note that this path may be reached with a crtc_state that
+		 * is not part of an atomic state, due to vlv_force_pll_on().
+		 * That means we can't use intel_pipe_will_have_type() or
+		 * anything that depends on the whole atomic state here.
+		 */
 		refclk = 100000;
-	} else if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS) &&
+	} else if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS) &&
 	    intel_panel_use_ssc(dev_priv) && num_connectors < 2) {
 		refclk = dev_priv->vbt.lvds_ssc_freq;
 		DRM_DEBUG_KMS("using SSC reference clock of %d kHz\n", refclk);
@@ -5912,7 +5942,7 @@ static void i9xx_update_pll_dividers(struct intel_crtc *crtc,
 	crtc_state->dpll_hw_state.fp0 = fp;
 
 	crtc->lowfreq_avail = false;
-	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS) &&
+	if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS) &&
 	    reduced_clock && i915.powersave) {
 		crtc_state->dpll_hw_state.fp1 = fp2;
 		crtc->lowfreq_avail = true;
@@ -6200,7 +6230,7 @@ static void chv_prepare_pll(struct intel_crtc *crtc,
 		       (2 << DPIO_CHV_FEEDFWD_GAIN_SHIFT));
 
 	/* Loop filter */
-	refclk = i9xx_get_refclk(crtc, 0);
+	refclk = i9xx_get_refclk(pipe_config, 0);
 	loopfilter = 5 << DPIO_CHV_PROP_COEFF_SHIFT |
 		2 << DPIO_CHV_GAIN_CTRL_SHIFT;
 	if (refclk == 100000)
@@ -6236,6 +6266,7 @@ void vlv_force_pll_on(struct drm_device *dev, enum pipe pipe,
 	struct intel_crtc *crtc =
 		to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
 	struct intel_crtc_state pipe_config = {
+		.base.crtc = &crtc->base,
 		.pixel_multiplier = 1,
 		.dpll = *dpll,
 	};
@@ -6280,12 +6311,12 @@ static void i9xx_update_pll(struct intel_crtc *crtc,
 
 	i9xx_update_pll_dividers(crtc, crtc_state, reduced_clock);
 
-	is_sdvo = intel_pipe_will_have_type(crtc, INTEL_OUTPUT_SDVO) ||
-		intel_pipe_will_have_type(crtc, INTEL_OUTPUT_HDMI);
+	is_sdvo = intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_SDVO) ||
+		intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_HDMI);
 
 	dpll = DPLL_VGA_MODE_DIS;
 
-	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS))
+	if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS))
 		dpll |= DPLLB_MODE_LVDS;
 	else
 		dpll |= DPLLB_MODE_DAC_SERIAL;
@@ -6328,7 +6359,7 @@ static void i9xx_update_pll(struct intel_crtc *crtc,
 
 	if (crtc_state->sdvo_tv_clock)
 		dpll |= PLL_REF_INPUT_TVCLKINBC;
-	else if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS) &&
+	else if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS) &&
 		 intel_panel_use_ssc(dev_priv) && num_connectors < 2)
 		dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN;
 	else
@@ -6358,7 +6389,7 @@ static void i8xx_update_pll(struct intel_crtc *crtc,
 
 	dpll = DPLL_VGA_MODE_DIS;
 
-	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
+	if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) {
 		dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
 	} else {
 		if (clock->p1 == 2)
@@ -6369,10 +6400,10 @@ static void i8xx_update_pll(struct intel_crtc *crtc,
 			dpll |= PLL_P2_DIVIDE_BY_4;
 	}
 
-	if (!IS_I830(dev) && intel_pipe_will_have_type(crtc, INTEL_OUTPUT_DVO))
+	if (!IS_I830(dev) && intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_DVO))
 		dpll |= DPLL_DVO_2X_MODE;
 
-	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS) &&
+	if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS) &&
 		 intel_panel_use_ssc(dev_priv) && num_connectors < 2)
 		dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN;
 	else
@@ -6609,7 +6640,7 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc,
 		return 0;
 
 	if (!crtc_state->clock_set) {
-		refclk = i9xx_get_refclk(crtc, num_connectors);
+		refclk = i9xx_get_refclk(crtc_state, num_connectors);
 
 		/*
 		 * Returns a set of divisors for the desired target clock with
@@ -6617,8 +6648,8 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc,
 		 * the clock equation: reflck * (5 * (m1 + 2) + (m2 + 2)) / (n +
 		 * 2) / p1 / p2.
 		 */
-		limit = intel_limit(crtc, refclk);
-		ok = dev_priv->display.find_dpll(limit, crtc,
+		limit = intel_limit(crtc_state, refclk);
+		ok = dev_priv->display.find_dpll(limit, crtc_state,
 						 crtc_state->port_clock,
 						 refclk, NULL, &clock);
 		if (!ok) {
@@ -6634,7 +6665,7 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc,
 			 * we will disable the LVDS downclock feature.
 			 */
 			has_reduced_clock =
-				dev_priv->display.find_dpll(limit, crtc,
+				dev_priv->display.find_dpll(limit, crtc_state,
 							    dev_priv->lvds_downclock,
 							    refclk, &clock,
 							    &reduced_clock);
@@ -7462,12 +7493,11 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int refclk;
 	const intel_limit_t *limit;
 	bool ret, is_lvds = false;
 
-	is_lvds = intel_pipe_will_have_type(intel_crtc, INTEL_OUTPUT_LVDS);
+	is_lvds = intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS);
 
 	refclk = ironlake_get_refclk(crtc);
 
@@ -7476,8 +7506,8 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
 	 * refclk, or FALSE.  The returned values represent the clock equation:
 	 * reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
 	 */
-	limit = intel_limit(intel_crtc, refclk);
-	ret = dev_priv->display.find_dpll(limit, intel_crtc,
+	limit = intel_limit(crtc_state, refclk);
+	ret = dev_priv->display.find_dpll(limit, crtc_state,
 					  crtc_state->port_clock,
 					  refclk, NULL, clock);
 	if (!ret)
@@ -7491,7 +7521,7 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
 		 * downclock feature.
 		*/
 		*has_reduced_clock =
-			dev_priv->display.find_dpll(limit, intel_crtc,
+			dev_priv->display.find_dpll(limit, crtc_state,
 						    dev_priv->lvds_downclock,
 						    refclk, clock,
 						    reduced_clock);
-- 
2.1.0

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

Re: [PATCH 21/23] drm/i915: Convert intel_pipe_will_have_type() to using atomic state

[Daniel Vetter]

On Tue, Mar 03, 2015 at 03:22:15PM +0200, Ander Conselvan de Oliveira wrote:
> Pass a crtc_state to it and find whether the pipe has an encoder of a
> given type by looking at the drm_atomic_state the crtc_state points to.
> 
> Note that is possible to reach i9xx_get_refclk() without a proper atomic
> state, since in the function vlv_force_pll_on() a minimally initialized
> crtc_state is allocated in the stack. With the current code, it is not
> possible to end up in a call to intel_pipe_will_have_type() with that
> bogus atomic state. To avoid future problems, a comment is added to warn
> people changing that code.
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |   2 +-
>  drivers/gpu/drm/i915/intel_display.c | 134 +++++++++++++++++++++--------------
>  2 files changed, 83 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0c6ba2d..aab4421 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -541,7 +541,7 @@ struct drm_i915_display_funcs {
>  	 * Returns true on success, false on failure.
>  	 */
>  	bool (*find_dpll)(const struct intel_limit *limit,
> -			  struct intel_crtc *crtc,
> +			  struct intel_crtc_state *crtc_state,
>  			  int target, int refclk,
>  			  struct dpll *match_clock,
>  			  struct dpll *best_clock);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 518903e..f3652f9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -431,25 +431,37 @@ bool intel_pipe_has_type(struct intel_crtc *crtc, enum intel_output_type type)
>   * intel_pipe_has_type() but looking at encoder->new_crtc instead of
>   * encoder->crtc.
>   */
> -static bool intel_pipe_will_have_type(struct intel_crtc *crtc, int type)
> +static bool intel_pipe_will_have_type(const struct intel_crtc_state *crtc_state,
> +				      int type)
>  {
> -	struct drm_device *dev = crtc->base.dev;
> +	struct drm_atomic_state *state = crtc_state->base.state;
> +	struct drm_connector_state *connector_state;
>  	struct intel_encoder *encoder;
> +	int i;
> +
> +	for (i = 0; i < state->num_connector; i++) {
> +		if (!state->connectors[i])
> +			continue;
> +
> +		connector_state = state->connector_states[i];
> +		if (connector_state->crtc != crtc_state->base.crtc)
> +			continue;
>  
> -	for_each_intel_encoder(dev, encoder)
> -		if (encoder->new_crtc == crtc && encoder->type == type)
> +		encoder = to_intel_encoder(connector_state->best_encoder);
> +		if (encoder->type == type)
>  			return true;
> +	}
>  
>  	return false;
>  }

The tricky bit here is that we must have all the connectors added to the
drm_atomic_sate for the given crtc. Otherwise there might be no connector
at all and we'd return a bogus answer. drm_atomic_add_affected_connectors
is the function which does this for you, and I think we need to call it
somewhere in the top-level compute_config code. And I haven't spotted that
call anywhere in your series.

Otherwise the implementation of the new will_have_type looks correct.
-Daniel

>  
> -static const intel_limit_t *intel_ironlake_limit(struct intel_crtc *crtc,
> -						int refclk)
> +static const intel_limit_t *
> +intel_ironlake_limit(struct intel_crtc_state *crtc_state, int refclk)
>  {
> -	struct drm_device *dev = crtc->base.dev;
> +	struct drm_device *dev = crtc_state->base.crtc->dev;
>  	const intel_limit_t *limit;
>  
> -	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
> +	if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) {
>  		if (intel_is_dual_link_lvds(dev)) {
>  			if (refclk == 100000)
>  				limit = &intel_limits_ironlake_dual_lvds_100m;
> @@ -467,20 +479,21 @@ static const intel_limit_t *intel_ironlake_limit(struct intel_crtc *crtc,
>  	return limit;
>  }
>  
> -static const intel_limit_t *intel_g4x_limit(struct intel_crtc *crtc)
> +static const intel_limit_t *
> +intel_g4x_limit(struct intel_crtc_state *crtc_state)
>  {
> -	struct drm_device *dev = crtc->base.dev;
> +	struct drm_device *dev = crtc_state->base.crtc->dev;
>  	const intel_limit_t *limit;
>  
> -	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
> +	if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) {
>  		if (intel_is_dual_link_lvds(dev))
>  			limit = &intel_limits_g4x_dual_channel_lvds;
>  		else
>  			limit = &intel_limits_g4x_single_channel_lvds;
> -	} else if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_HDMI) ||
> -		   intel_pipe_will_have_type(crtc, INTEL_OUTPUT_ANALOG)) {
> +	} else if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_HDMI) ||
> +		   intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_ANALOG)) {
>  		limit = &intel_limits_g4x_hdmi;
> -	} else if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_SDVO)) {
> +	} else if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_SDVO)) {
>  		limit = &intel_limits_g4x_sdvo;
>  	} else /* The option is for other outputs */
>  		limit = &intel_limits_i9xx_sdvo;
> @@ -488,17 +501,18 @@ static const intel_limit_t *intel_g4x_limit(struct intel_crtc *crtc)
>  	return limit;
>  }
>  
> -static const intel_limit_t *intel_limit(struct intel_crtc *crtc, int refclk)
> +static const intel_limit_t *
> +intel_limit(struct intel_crtc_state *crtc_state, int refclk)
>  {
> -	struct drm_device *dev = crtc->base.dev;
> +	struct drm_device *dev = crtc_state->base.crtc->dev;
>  	const intel_limit_t *limit;
>  
>  	if (HAS_PCH_SPLIT(dev))
> -		limit = intel_ironlake_limit(crtc, refclk);
> +		limit = intel_ironlake_limit(crtc_state, refclk);
>  	else if (IS_G4X(dev)) {
> -		limit = intel_g4x_limit(crtc);
> +		limit = intel_g4x_limit(crtc_state);
>  	} else if (IS_PINEVIEW(dev)) {
> -		if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS))
> +		if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS))
>  			limit = &intel_limits_pineview_lvds;
>  		else
>  			limit = &intel_limits_pineview_sdvo;
> @@ -507,14 +521,14 @@ static const intel_limit_t *intel_limit(struct intel_crtc *crtc, int refclk)
>  	} else if (IS_VALLEYVIEW(dev)) {
>  		limit = &intel_limits_vlv;
>  	} else if (!IS_GEN2(dev)) {
> -		if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS))
> +		if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS))
>  			limit = &intel_limits_i9xx_lvds;
>  		else
>  			limit = &intel_limits_i9xx_sdvo;
>  	} else {
> -		if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS))
> +		if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS))
>  			limit = &intel_limits_i8xx_lvds;
> -		else if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_DVO))
> +		else if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_DVO))
>  			limit = &intel_limits_i8xx_dvo;
>  		else
>  			limit = &intel_limits_i8xx_dac;
> @@ -601,15 +615,17 @@ static bool intel_PLL_is_valid(struct drm_device *dev,
>  }
>  
>  static bool
> -i9xx_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
> +i9xx_find_best_dpll(const intel_limit_t *limit,
> +		    struct intel_crtc_state *crtc_state,
>  		    int target, int refclk, intel_clock_t *match_clock,
>  		    intel_clock_t *best_clock)
>  {
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>  	struct drm_device *dev = crtc->base.dev;
>  	intel_clock_t clock;
>  	int err = target;
>  
> -	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
> +	if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) {
>  		/*
>  		 * For LVDS just rely on its current settings for dual-channel.
>  		 * We haven't figured out how to reliably set up different
> @@ -662,15 +678,17 @@ i9xx_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
>  }
>  
>  static bool
> -pnv_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
> +pnv_find_best_dpll(const intel_limit_t *limit,
> +		   struct intel_crtc_state *crtc_state,
>  		   int target, int refclk, intel_clock_t *match_clock,
>  		   intel_clock_t *best_clock)
>  {
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>  	struct drm_device *dev = crtc->base.dev;
>  	intel_clock_t clock;
>  	int err = target;
>  
> -	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
> +	if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) {
>  		/*
>  		 * For LVDS just rely on its current settings for dual-channel.
>  		 * We haven't figured out how to reliably set up different
> @@ -721,10 +739,12 @@ pnv_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
>  }
>  
>  static bool
> -g4x_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
> +g4x_find_best_dpll(const intel_limit_t *limit,
> +		   struct intel_crtc_state *crtc_state,
>  		   int target, int refclk, intel_clock_t *match_clock,
>  		   intel_clock_t *best_clock)
>  {
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>  	struct drm_device *dev = crtc->base.dev;
>  	intel_clock_t clock;
>  	int max_n;
> @@ -733,7 +753,7 @@ g4x_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
>  	int err_most = (target >> 8) + (target >> 9);
>  	found = false;
>  
> -	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
> +	if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) {
>  		if (intel_is_dual_link_lvds(dev))
>  			clock.p2 = limit->p2.p2_fast;
>  		else
> @@ -778,10 +798,12 @@ g4x_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
>  }
>  
>  static bool
> -vlv_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
> +vlv_find_best_dpll(const intel_limit_t *limit,
> +		   struct intel_crtc_state *crtc_state,
>  		   int target, int refclk, intel_clock_t *match_clock,
>  		   intel_clock_t *best_clock)
>  {
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>  	struct drm_device *dev = crtc->base.dev;
>  	intel_clock_t clock;
>  	unsigned int bestppm = 1000000;
> @@ -835,10 +857,12 @@ vlv_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
>  }
>  
>  static bool
> -chv_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
> +chv_find_best_dpll(const intel_limit_t *limit,
> +		   struct intel_crtc_state *crtc_state,
>  		   int target, int refclk, intel_clock_t *match_clock,
>  		   intel_clock_t *best_clock)
>  {
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>  	struct drm_device *dev = crtc->base.dev;
>  	intel_clock_t clock;
>  	uint64_t m2;
> @@ -5678,7 +5702,7 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>  	 * - LVDS dual channel mode
>  	 * - Double wide pipe
>  	 */
> -	if ((intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS) &&
> +	if ((intel_pipe_will_have_type(pipe_config, INTEL_OUTPUT_LVDS) &&
>  	     intel_is_dual_link_lvds(dev)) || pipe_config->double_wide)
>  		pipe_config->pipe_src_w &= ~1;
>  
> @@ -5861,15 +5885,21 @@ static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
>  		&& !(dev_priv->quirks & QUIRK_LVDS_SSC_DISABLE);
>  }
>  
> -static int i9xx_get_refclk(struct intel_crtc *crtc, int num_connectors)
> +static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state,
> +			   int num_connectors)
>  {
> -	struct drm_device *dev = crtc->base.dev;
> +	struct drm_device *dev = crtc_state->base.crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int refclk;
>  
>  	if (IS_VALLEYVIEW(dev)) {
> +		/* Note that this path may be reached with a crtc_state that
> +		 * is not part of an atomic state, due to vlv_force_pll_on().
> +		 * That means we can't use intel_pipe_will_have_type() or
> +		 * anything that depends on the whole atomic state here.
> +		 */
>  		refclk = 100000;
> -	} else if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS) &&
> +	} else if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS) &&
>  	    intel_panel_use_ssc(dev_priv) && num_connectors < 2) {
>  		refclk = dev_priv->vbt.lvds_ssc_freq;
>  		DRM_DEBUG_KMS("using SSC reference clock of %d kHz\n", refclk);
> @@ -5912,7 +5942,7 @@ static void i9xx_update_pll_dividers(struct intel_crtc *crtc,
>  	crtc_state->dpll_hw_state.fp0 = fp;
>  
>  	crtc->lowfreq_avail = false;
> -	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS) &&
> +	if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS) &&
>  	    reduced_clock && i915.powersave) {
>  		crtc_state->dpll_hw_state.fp1 = fp2;
>  		crtc->lowfreq_avail = true;
> @@ -6200,7 +6230,7 @@ static void chv_prepare_pll(struct intel_crtc *crtc,
>  		       (2 << DPIO_CHV_FEEDFWD_GAIN_SHIFT));
>  
>  	/* Loop filter */
> -	refclk = i9xx_get_refclk(crtc, 0);
> +	refclk = i9xx_get_refclk(pipe_config, 0);
>  	loopfilter = 5 << DPIO_CHV_PROP_COEFF_SHIFT |
>  		2 << DPIO_CHV_GAIN_CTRL_SHIFT;
>  	if (refclk == 100000)
> @@ -6236,6 +6266,7 @@ void vlv_force_pll_on(struct drm_device *dev, enum pipe pipe,
>  	struct intel_crtc *crtc =
>  		to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
>  	struct intel_crtc_state pipe_config = {
> +		.base.crtc = &crtc->base,
>  		.pixel_multiplier = 1,
>  		.dpll = *dpll,
>  	};
> @@ -6280,12 +6311,12 @@ static void i9xx_update_pll(struct intel_crtc *crtc,
>  
>  	i9xx_update_pll_dividers(crtc, crtc_state, reduced_clock);
>  
> -	is_sdvo = intel_pipe_will_have_type(crtc, INTEL_OUTPUT_SDVO) ||
> -		intel_pipe_will_have_type(crtc, INTEL_OUTPUT_HDMI);
> +	is_sdvo = intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_SDVO) ||
> +		intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_HDMI);
>  
>  	dpll = DPLL_VGA_MODE_DIS;
>  
> -	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS))
> +	if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS))
>  		dpll |= DPLLB_MODE_LVDS;
>  	else
>  		dpll |= DPLLB_MODE_DAC_SERIAL;
> @@ -6328,7 +6359,7 @@ static void i9xx_update_pll(struct intel_crtc *crtc,
>  
>  	if (crtc_state->sdvo_tv_clock)
>  		dpll |= PLL_REF_INPUT_TVCLKINBC;
> -	else if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS) &&
> +	else if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS) &&
>  		 intel_panel_use_ssc(dev_priv) && num_connectors < 2)
>  		dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN;
>  	else
> @@ -6358,7 +6389,7 @@ static void i8xx_update_pll(struct intel_crtc *crtc,
>  
>  	dpll = DPLL_VGA_MODE_DIS;
>  
> -	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
> +	if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) {
>  		dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
>  	} else {
>  		if (clock->p1 == 2)
> @@ -6369,10 +6400,10 @@ static void i8xx_update_pll(struct intel_crtc *crtc,
>  			dpll |= PLL_P2_DIVIDE_BY_4;
>  	}
>  
> -	if (!IS_I830(dev) && intel_pipe_will_have_type(crtc, INTEL_OUTPUT_DVO))
> +	if (!IS_I830(dev) && intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_DVO))
>  		dpll |= DPLL_DVO_2X_MODE;
>  
> -	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS) &&
> +	if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS) &&
>  		 intel_panel_use_ssc(dev_priv) && num_connectors < 2)
>  		dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN;
>  	else
> @@ -6609,7 +6640,7 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc,
>  		return 0;
>  
>  	if (!crtc_state->clock_set) {
> -		refclk = i9xx_get_refclk(crtc, num_connectors);
> +		refclk = i9xx_get_refclk(crtc_state, num_connectors);
>  
>  		/*
>  		 * Returns a set of divisors for the desired target clock with
> @@ -6617,8 +6648,8 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc,
>  		 * the clock equation: reflck * (5 * (m1 + 2) + (m2 + 2)) / (n +
>  		 * 2) / p1 / p2.
>  		 */
> -		limit = intel_limit(crtc, refclk);
> -		ok = dev_priv->display.find_dpll(limit, crtc,
> +		limit = intel_limit(crtc_state, refclk);
> +		ok = dev_priv->display.find_dpll(limit, crtc_state,
>  						 crtc_state->port_clock,
>  						 refclk, NULL, &clock);
>  		if (!ok) {
> @@ -6634,7 +6665,7 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc,
>  			 * we will disable the LVDS downclock feature.
>  			 */
>  			has_reduced_clock =
> -				dev_priv->display.find_dpll(limit, crtc,
> +				dev_priv->display.find_dpll(limit, crtc_state,
>  							    dev_priv->lvds_downclock,
>  							    refclk, &clock,
>  							    &reduced_clock);
> @@ -7462,12 +7493,11 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int refclk;
>  	const intel_limit_t *limit;
>  	bool ret, is_lvds = false;
>  
> -	is_lvds = intel_pipe_will_have_type(intel_crtc, INTEL_OUTPUT_LVDS);
> +	is_lvds = intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS);
>  
>  	refclk = ironlake_get_refclk(crtc);
>  
> @@ -7476,8 +7506,8 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
>  	 * refclk, or FALSE.  The returned values represent the clock equation:
>  	 * reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
>  	 */
> -	limit = intel_limit(intel_crtc, refclk);
> -	ret = dev_priv->display.find_dpll(limit, intel_crtc,
> +	limit = intel_limit(crtc_state, refclk);
> +	ret = dev_priv->display.find_dpll(limit, crtc_state,
>  					  crtc_state->port_clock,
>  					  refclk, NULL, clock);
>  	if (!ret)
> @@ -7491,7 +7521,7 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
>  		 * downclock feature.
>  		*/
>  		*has_reduced_clock =
> -			dev_priv->display.find_dpll(limit, intel_crtc,
> +			dev_priv->display.find_dpll(limit, crtc_state,
>  						    dev_priv->lvds_downclock,
>  						    refclk, clock,
>  						    reduced_clock);
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 21/23] drm/i915: Convert intel_pipe_will_have_type() to using atomic state

[Conselvan De Oliveira, Ander]

On Wed, 2015-03-04 at 17:03 +0100, Daniel Vetter wrote:
> On Tue, Mar 03, 2015 at 03:22:15PM +0200, Ander Conselvan de Oliveira wrote:
> > Pass a crtc_state to it and find whether the pipe has an encoder of a
> > given type by looking at the drm_atomic_state the crtc_state points to.
> > 
> > Note that is possible to reach i9xx_get_refclk() without a proper atomic
> > state, since in the function vlv_force_pll_on() a minimally initialized
> > crtc_state is allocated in the stack. With the current code, it is not
> > possible to end up in a call to intel_pipe_will_have_type() with that
> > bogus atomic state. To avoid future problems, a comment is added to warn
> > people changing that code.
> > 
> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |   2 +-
> >  drivers/gpu/drm/i915/intel_display.c | 134 +++++++++++++++++++++--------------
> >  2 files changed, 83 insertions(+), 53 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 0c6ba2d..aab4421 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -541,7 +541,7 @@ struct drm_i915_display_funcs {
> >  	 * Returns true on success, false on failure.
> >  	 */
> >  	bool (*find_dpll)(const struct intel_limit *limit,
> > -			  struct intel_crtc *crtc,
> > +			  struct intel_crtc_state *crtc_state,
> >  			  int target, int refclk,
> >  			  struct dpll *match_clock,
> >  			  struct dpll *best_clock);
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 518903e..f3652f9 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -431,25 +431,37 @@ bool intel_pipe_has_type(struct intel_crtc *crtc, enum intel_output_type type)
> >   * intel_pipe_has_type() but looking at encoder->new_crtc instead of
> >   * encoder->crtc.
> >   */
> > -static bool intel_pipe_will_have_type(struct intel_crtc *crtc, int type)
> > +static bool intel_pipe_will_have_type(const struct intel_crtc_state *crtc_state,
> > +				      int type)
> >  {
> > -	struct drm_device *dev = crtc->base.dev;
> > +	struct drm_atomic_state *state = crtc_state->base.state;
> > +	struct drm_connector_state *connector_state;
> >  	struct intel_encoder *encoder;
> > +	int i;
> > +
> > +	for (i = 0; i < state->num_connector; i++) {
> > +		if (!state->connectors[i])
> > +			continue;
> > +
> > +		connector_state = state->connector_states[i];
> > +		if (connector_state->crtc != crtc_state->base.crtc)
> > +			continue;
> >  
> > -	for_each_intel_encoder(dev, encoder)
> > -		if (encoder->new_crtc == crtc && encoder->type == type)
> > +		encoder = to_intel_encoder(connector_state->best_encoder);
> > +		if (encoder->type == type)
> >  			return true;
> > +	}
> >  
> >  	return false;
> >  }
> 
> The tricky bit here is that we must have all the connectors added to the
> drm_atomic_sate for the given crtc. Otherwise there might be no connector
> at all and we'd return a bogus answer. drm_atomic_add_affected_connectors
> is the function which does this for you, and I think we need to call it
> somewhere in the top-level compute_config code. And I haven't spotted that
> call anywhere in your series.

I did add it in patch 12, but now I realize it won't be called for the
disable case, since the call is in intel_modeset_pipe_config(). I'll
move that to intel_modeset_compute_config().

Ander

> Otherwise the implementation of the new will_have_type looks correct.
> -Daniel
> 
> >  
> > -static const intel_limit_t *intel_ironlake_limit(struct intel_crtc *crtc,
> > -						int refclk)
> > +static const intel_limit_t *
> > +intel_ironlake_limit(struct intel_crtc_state *crtc_state, int refclk)
> >  {
> > -	struct drm_device *dev = crtc->base.dev;
> > +	struct drm_device *dev = crtc_state->base.crtc->dev;
> >  	const intel_limit_t *limit;
> >  
> > -	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
> > +	if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) {
> >  		if (intel_is_dual_link_lvds(dev)) {
> >  			if (refclk == 100000)
> >  				limit = &intel_limits_ironlake_dual_lvds_100m;
> > @@ -467,20 +479,21 @@ static const intel_limit_t *intel_ironlake_limit(struct intel_crtc *crtc,
> >  	return limit;
> >  }
> >  
> > -static const intel_limit_t *intel_g4x_limit(struct intel_crtc *crtc)
> > +static const intel_limit_t *
> > +intel_g4x_limit(struct intel_crtc_state *crtc_state)
> >  {
> > -	struct drm_device *dev = crtc->base.dev;
> > +	struct drm_device *dev = crtc_state->base.crtc->dev;
> >  	const intel_limit_t *limit;
> >  
> > -	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
> > +	if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) {
> >  		if (intel_is_dual_link_lvds(dev))
> >  			limit = &intel_limits_g4x_dual_channel_lvds;
> >  		else
> >  			limit = &intel_limits_g4x_single_channel_lvds;
> > -	} else if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_HDMI) ||
> > -		   intel_pipe_will_have_type(crtc, INTEL_OUTPUT_ANALOG)) {
> > +	} else if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_HDMI) ||
> > +		   intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_ANALOG)) {
> >  		limit = &intel_limits_g4x_hdmi;
> > -	} else if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_SDVO)) {
> > +	} else if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_SDVO)) {
> >  		limit = &intel_limits_g4x_sdvo;
> >  	} else /* The option is for other outputs */
> >  		limit = &intel_limits_i9xx_sdvo;
> > @@ -488,17 +501,18 @@ static const intel_limit_t *intel_g4x_limit(struct intel_crtc *crtc)
> >  	return limit;
> >  }
> >  
> > -static const intel_limit_t *intel_limit(struct intel_crtc *crtc, int refclk)
> > +static const intel_limit_t *
> > +intel_limit(struct intel_crtc_state *crtc_state, int refclk)
> >  {
> > -	struct drm_device *dev = crtc->base.dev;
> > +	struct drm_device *dev = crtc_state->base.crtc->dev;
> >  	const intel_limit_t *limit;
> >  
> >  	if (HAS_PCH_SPLIT(dev))
> > -		limit = intel_ironlake_limit(crtc, refclk);
> > +		limit = intel_ironlake_limit(crtc_state, refclk);
> >  	else if (IS_G4X(dev)) {
> > -		limit = intel_g4x_limit(crtc);
> > +		limit = intel_g4x_limit(crtc_state);
> >  	} else if (IS_PINEVIEW(dev)) {
> > -		if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS))
> > +		if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS))
> >  			limit = &intel_limits_pineview_lvds;
> >  		else
> >  			limit = &intel_limits_pineview_sdvo;
> > @@ -507,14 +521,14 @@ static const intel_limit_t *intel_limit(struct intel_crtc *crtc, int refclk)
> >  	} else if (IS_VALLEYVIEW(dev)) {
> >  		limit = &intel_limits_vlv;
> >  	} else if (!IS_GEN2(dev)) {
> > -		if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS))
> > +		if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS))
> >  			limit = &intel_limits_i9xx_lvds;
> >  		else
> >  			limit = &intel_limits_i9xx_sdvo;
> >  	} else {
> > -		if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS))
> > +		if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS))
> >  			limit = &intel_limits_i8xx_lvds;
> > -		else if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_DVO))
> > +		else if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_DVO))
> >  			limit = &intel_limits_i8xx_dvo;
> >  		else
> >  			limit = &intel_limits_i8xx_dac;
> > @@ -601,15 +615,17 @@ static bool intel_PLL_is_valid(struct drm_device *dev,
> >  }
> >  
> >  static bool
> > -i9xx_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
> > +i9xx_find_best_dpll(const intel_limit_t *limit,
> > +		    struct intel_crtc_state *crtc_state,
> >  		    int target, int refclk, intel_clock_t *match_clock,
> >  		    intel_clock_t *best_clock)
> >  {
> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> >  	struct drm_device *dev = crtc->base.dev;
> >  	intel_clock_t clock;
> >  	int err = target;
> >  
> > -	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
> > +	if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) {
> >  		/*
> >  		 * For LVDS just rely on its current settings for dual-channel.
> >  		 * We haven't figured out how to reliably set up different
> > @@ -662,15 +678,17 @@ i9xx_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
> >  }
> >  
> >  static bool
> > -pnv_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
> > +pnv_find_best_dpll(const intel_limit_t *limit,
> > +		   struct intel_crtc_state *crtc_state,
> >  		   int target, int refclk, intel_clock_t *match_clock,
> >  		   intel_clock_t *best_clock)
> >  {
> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> >  	struct drm_device *dev = crtc->base.dev;
> >  	intel_clock_t clock;
> >  	int err = target;
> >  
> > -	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
> > +	if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) {
> >  		/*
> >  		 * For LVDS just rely on its current settings for dual-channel.
> >  		 * We haven't figured out how to reliably set up different
> > @@ -721,10 +739,12 @@ pnv_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
> >  }
> >  
> >  static bool
> > -g4x_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
> > +g4x_find_best_dpll(const intel_limit_t *limit,
> > +		   struct intel_crtc_state *crtc_state,
> >  		   int target, int refclk, intel_clock_t *match_clock,
> >  		   intel_clock_t *best_clock)
> >  {
> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> >  	struct drm_device *dev = crtc->base.dev;
> >  	intel_clock_t clock;
> >  	int max_n;
> > @@ -733,7 +753,7 @@ g4x_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
> >  	int err_most = (target >> 8) + (target >> 9);
> >  	found = false;
> >  
> > -	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
> > +	if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) {
> >  		if (intel_is_dual_link_lvds(dev))
> >  			clock.p2 = limit->p2.p2_fast;
> >  		else
> > @@ -778,10 +798,12 @@ g4x_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
> >  }
> >  
> >  static bool
> > -vlv_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
> > +vlv_find_best_dpll(const intel_limit_t *limit,
> > +		   struct intel_crtc_state *crtc_state,
> >  		   int target, int refclk, intel_clock_t *match_clock,
> >  		   intel_clock_t *best_clock)
> >  {
> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> >  	struct drm_device *dev = crtc->base.dev;
> >  	intel_clock_t clock;
> >  	unsigned int bestppm = 1000000;
> > @@ -835,10 +857,12 @@ vlv_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
> >  }
> >  
> >  static bool
> > -chv_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
> > +chv_find_best_dpll(const intel_limit_t *limit,
> > +		   struct intel_crtc_state *crtc_state,
> >  		   int target, int refclk, intel_clock_t *match_clock,
> >  		   intel_clock_t *best_clock)
> >  {
> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> >  	struct drm_device *dev = crtc->base.dev;
> >  	intel_clock_t clock;
> >  	uint64_t m2;
> > @@ -5678,7 +5702,7 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
> >  	 * - LVDS dual channel mode
> >  	 * - Double wide pipe
> >  	 */
> > -	if ((intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS) &&
> > +	if ((intel_pipe_will_have_type(pipe_config, INTEL_OUTPUT_LVDS) &&
> >  	     intel_is_dual_link_lvds(dev)) || pipe_config->double_wide)
> >  		pipe_config->pipe_src_w &= ~1;
> >  
> > @@ -5861,15 +5885,21 @@ static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
> >  		&& !(dev_priv->quirks & QUIRK_LVDS_SSC_DISABLE);
> >  }
> >  
> > -static int i9xx_get_refclk(struct intel_crtc *crtc, int num_connectors)
> > +static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state,
> > +			   int num_connectors)
> >  {
> > -	struct drm_device *dev = crtc->base.dev;
> > +	struct drm_device *dev = crtc_state->base.crtc->dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	int refclk;
> >  
> >  	if (IS_VALLEYVIEW(dev)) {
> > +		/* Note that this path may be reached with a crtc_state that
> > +		 * is not part of an atomic state, due to vlv_force_pll_on().
> > +		 * That means we can't use intel_pipe_will_have_type() or
> > +		 * anything that depends on the whole atomic state here.
> > +		 */
> >  		refclk = 100000;
> > -	} else if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS) &&
> > +	} else if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS) &&
> >  	    intel_panel_use_ssc(dev_priv) && num_connectors < 2) {
> >  		refclk = dev_priv->vbt.lvds_ssc_freq;
> >  		DRM_DEBUG_KMS("using SSC reference clock of %d kHz\n", refclk);
> > @@ -5912,7 +5942,7 @@ static void i9xx_update_pll_dividers(struct intel_crtc *crtc,
> >  	crtc_state->dpll_hw_state.fp0 = fp;
> >  
> >  	crtc->lowfreq_avail = false;
> > -	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS) &&
> > +	if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS) &&
> >  	    reduced_clock && i915.powersave) {
> >  		crtc_state->dpll_hw_state.fp1 = fp2;
> >  		crtc->lowfreq_avail = true;
> > @@ -6200,7 +6230,7 @@ static void chv_prepare_pll(struct intel_crtc *crtc,
> >  		       (2 << DPIO_CHV_FEEDFWD_GAIN_SHIFT));
> >  
> >  	/* Loop filter */
> > -	refclk = i9xx_get_refclk(crtc, 0);
> > +	refclk = i9xx_get_refclk(pipe_config, 0);
> >  	loopfilter = 5 << DPIO_CHV_PROP_COEFF_SHIFT |
> >  		2 << DPIO_CHV_GAIN_CTRL_SHIFT;
> >  	if (refclk == 100000)
> > @@ -6236,6 +6266,7 @@ void vlv_force_pll_on(struct drm_device *dev, enum pipe pipe,
> >  	struct intel_crtc *crtc =
> >  		to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
> >  	struct intel_crtc_state pipe_config = {
> > +		.base.crtc = &crtc->base,
> >  		.pixel_multiplier = 1,
> >  		.dpll = *dpll,
> >  	};
> > @@ -6280,12 +6311,12 @@ static void i9xx_update_pll(struct intel_crtc *crtc,
> >  
> >  	i9xx_update_pll_dividers(crtc, crtc_state, reduced_clock);
> >  
> > -	is_sdvo = intel_pipe_will_have_type(crtc, INTEL_OUTPUT_SDVO) ||
> > -		intel_pipe_will_have_type(crtc, INTEL_OUTPUT_HDMI);
> > +	is_sdvo = intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_SDVO) ||
> > +		intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_HDMI);
> >  
> >  	dpll = DPLL_VGA_MODE_DIS;
> >  
> > -	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS))
> > +	if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS))
> >  		dpll |= DPLLB_MODE_LVDS;
> >  	else
> >  		dpll |= DPLLB_MODE_DAC_SERIAL;
> > @@ -6328,7 +6359,7 @@ static void i9xx_update_pll(struct intel_crtc *crtc,
> >  
> >  	if (crtc_state->sdvo_tv_clock)
> >  		dpll |= PLL_REF_INPUT_TVCLKINBC;
> > -	else if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS) &&
> > +	else if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS) &&
> >  		 intel_panel_use_ssc(dev_priv) && num_connectors < 2)
> >  		dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN;
> >  	else
> > @@ -6358,7 +6389,7 @@ static void i8xx_update_pll(struct intel_crtc *crtc,
> >  
> >  	dpll = DPLL_VGA_MODE_DIS;
> >  
> > -	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
> > +	if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) {
> >  		dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
> >  	} else {
> >  		if (clock->p1 == 2)
> > @@ -6369,10 +6400,10 @@ static void i8xx_update_pll(struct intel_crtc *crtc,
> >  			dpll |= PLL_P2_DIVIDE_BY_4;
> >  	}
> >  
> > -	if (!IS_I830(dev) && intel_pipe_will_have_type(crtc, INTEL_OUTPUT_DVO))
> > +	if (!IS_I830(dev) && intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_DVO))
> >  		dpll |= DPLL_DVO_2X_MODE;
> >  
> > -	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS) &&
> > +	if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS) &&
> >  		 intel_panel_use_ssc(dev_priv) && num_connectors < 2)
> >  		dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN;
> >  	else
> > @@ -6609,7 +6640,7 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc,
> >  		return 0;
> >  
> >  	if (!crtc_state->clock_set) {
> > -		refclk = i9xx_get_refclk(crtc, num_connectors);
> > +		refclk = i9xx_get_refclk(crtc_state, num_connectors);
> >  
> >  		/*
> >  		 * Returns a set of divisors for the desired target clock with
> > @@ -6617,8 +6648,8 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc,
> >  		 * the clock equation: reflck * (5 * (m1 + 2) + (m2 + 2)) / (n +
> >  		 * 2) / p1 / p2.
> >  		 */
> > -		limit = intel_limit(crtc, refclk);
> > -		ok = dev_priv->display.find_dpll(limit, crtc,
> > +		limit = intel_limit(crtc_state, refclk);
> > +		ok = dev_priv->display.find_dpll(limit, crtc_state,
> >  						 crtc_state->port_clock,
> >  						 refclk, NULL, &clock);
> >  		if (!ok) {
> > @@ -6634,7 +6665,7 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc,
> >  			 * we will disable the LVDS downclock feature.
> >  			 */
> >  			has_reduced_clock =
> > -				dev_priv->display.find_dpll(limit, crtc,
> > +				dev_priv->display.find_dpll(limit, crtc_state,
> >  							    dev_priv->lvds_downclock,
> >  							    refclk, &clock,
> >  							    &reduced_clock);
> > @@ -7462,12 +7493,11 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
> >  {
> >  	struct drm_device *dev = crtc->dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	int refclk;
> >  	const intel_limit_t *limit;
> >  	bool ret, is_lvds = false;
> >  
> > -	is_lvds = intel_pipe_will_have_type(intel_crtc, INTEL_OUTPUT_LVDS);
> > +	is_lvds = intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS);
> >  
> >  	refclk = ironlake_get_refclk(crtc);
> >  
> > @@ -7476,8 +7506,8 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
> >  	 * refclk, or FALSE.  The returned values represent the clock equation:
> >  	 * reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
> >  	 */
> > -	limit = intel_limit(intel_crtc, refclk);
> > -	ret = dev_priv->display.find_dpll(limit, intel_crtc,
> > +	limit = intel_limit(crtc_state, refclk);
> > +	ret = dev_priv->display.find_dpll(limit, crtc_state,
> >  					  crtc_state->port_clock,
> >  					  refclk, NULL, clock);
> >  	if (!ret)
> > @@ -7491,7 +7521,7 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
> >  		 * downclock feature.
> >  		*/
> >  		*has_reduced_clock =
> > -			dev_priv->display.find_dpll(limit, intel_crtc,
> > +			dev_priv->display.find_dpll(limit, crtc_state,
> >  						    dev_priv->lvds_downclock,
> >  						    refclk, clock,
> >  						    reduced_clock);
> > -- 
> > 2.1.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 21/23] drm/i915: Convert intel_pipe_will_have_type() to using atomic state

[Daniel Vetter]

On Wed, Mar 4, 2015 at 5:51 PM, Conselvan De Oliveira, Ander
<ander.conselvan.de.oliveira@intel.com> wrote:
>> The tricky bit here is that we must have all the connectors added to the
>> drm_atomic_sate for the given crtc. Otherwise there might be no connector
>> at all and we'd return a bogus answer. drm_atomic_add_affected_connectors
>> is the function which does this for you, and I think we need to call it
>> somewhere in the top-level compute_config code. And I haven't spotted that
>> call anywhere in your series.
>
> I did add it in patch 12, but now I realize it won't be called for the
> disable case, since the call is in intel_modeset_pipe_config(). I'll
> move that to intel_modeset_compute_config().

Indeed. I've used my mail client to search for it, no idea why that
didn't show up ... For safety maybe we should have a WARN_ON if there
are no connectors for a given crtc? Normally there's no cloning going
on hence should catch all such bugs.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 22/23] drm/i915: Don't look at staged config crtc when changing DRRS state

[Ander Conselvan de Oliveira]

The function intel_dp_set_drrs_state() would decide which pipe to
downclock based on the staged config for the given connector. However,
the result of that function is immediate, and it uses input values from
crtc->config, so it should be looking at the current crtc instead.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a477e07..6cd4c56 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4785,7 +4785,7 @@ static void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
 
 	dig_port = dp_to_dig_port(intel_dp);
 	encoder = &dig_port->base;
-	intel_crtc = encoder->new_crtc;
+	intel_crtc = to_intel_crtc(encoder->base.crtc);
 
 	if (!intel_crtc) {
 		DRM_DEBUG_KMS("DRRS: intel_crtc not initialized\n");
-- 
2.1.0

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

[PATCH 23/23] drm/i915: Remove usage of encoder->new_crtc from clock computations

[Ander Conselvan de Oliveira]

Some of the crtc_compute_clock() still depended on encoder->new_crtc
since they didn't use intel_pipe_will_have_type() and used an open
coded version of that function instead. This patch replaces those with
the appropriate code that checks the atomic state intead.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 45 +++++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f3652f9..8864484 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6617,11 +6617,18 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc,
 	bool is_lvds = false, is_dsi = false;
 	struct intel_encoder *encoder;
 	const intel_limit_t *limit;
+	struct drm_atomic_state *state = crtc_state->base.state;
+	struct drm_connector_state *connector_state;
+	int i;
 
-	for_each_intel_encoder(dev, encoder) {
-		if (encoder->new_crtc != crtc)
+	for (i = 0; i < state->num_connector; i++) {
+		connector_state = state->connector_states[i];
+		if (!state->connectors[i] ||
+		    connector_state->crtc != &crtc->base)
 			continue;
 
+		encoder = to_intel_encoder(connector_state->best_encoder);
+
 		switch (encoder->type) {
 		case INTEL_OUTPUT_LVDS:
 			is_lvds = true;
@@ -7295,18 +7302,24 @@ void intel_init_pch_refclk(struct drm_device *dev)
 		lpt_init_pch_refclk(dev);
 }
 
-static int ironlake_get_refclk(struct drm_crtc *crtc)
+static int ironlake_get_refclk(struct intel_crtc_state *crtc_state)
 {
-	struct drm_device *dev = crtc->dev;
+	struct drm_device *dev = crtc_state->base.crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_atomic_state *state = crtc_state->base.state;
+	struct drm_connector_state *connector_state;
 	struct intel_encoder *encoder;
-	int num_connectors = 0;
+	int num_connectors = 0, i;
 	bool is_lvds = false;
 
-	for_each_intel_encoder(dev, encoder) {
-		if (encoder->new_crtc != to_intel_crtc(crtc))
+	for (i = 0; i < state->num_connector; i++) {
+		connector_state = state->connector_states[i];
+		if (!state->connectors[i] ||
+		    connector_state->crtc != crtc_state->base.crtc)
 			continue;
 
+		encoder = to_intel_encoder(connector_state->best_encoder);
+
 		switch (encoder->type) {
 		case INTEL_OUTPUT_LVDS:
 			is_lvds = true;
@@ -7499,7 +7512,7 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
 
 	is_lvds = intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS);
 
-	refclk = ironlake_get_refclk(crtc);
+	refclk = ironlake_get_refclk(crtc_state);
 
 	/*
 	 * Returns a set of divisors for the desired target clock with the given
@@ -7554,16 +7567,22 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
 	struct drm_crtc *crtc = &intel_crtc->base;
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_encoder *intel_encoder;
+	struct drm_atomic_state *state = crtc_state->base.state;
+	struct drm_connector_state *connector_state;
+	struct intel_encoder *encoder;
 	uint32_t dpll;
-	int factor, num_connectors = 0;
+	int factor, num_connectors = 0, i;
 	bool is_lvds = false, is_sdvo = false;
 
-	for_each_intel_encoder(dev, intel_encoder) {
-		if (intel_encoder->new_crtc != to_intel_crtc(crtc))
+	for (i = 0; i < state->num_connector; i++) {
+		connector_state = state->connector_states[i];
+		if (!state->connectors[i] ||
+		    connector_state->crtc != crtc_state->base.crtc)
 			continue;
 
-		switch (intel_encoder->type) {
+		encoder = to_intel_encoder(connector_state->best_encoder);
+
+		switch (encoder->type) {
 		case INTEL_OUTPUT_LVDS:
 			is_lvds = true;
 			break;
-- 
2.1.0

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

Re: [PATCH 23/23] drm/i915: Remove usage of encoder->new_crtc from clock computations

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5876
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -8              278/278              270/278
ILK                 -5              307/307              302/307
SNB                 -1              284/284              283/284
IVB                                  380/380              380/380
BYT                                  294/294              294/294
HSW                                  387/387              387/387
BDW                 -1              316/316              315/316
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*PNV  igt_gem_fence_thrash_bo-write-verify-none      NRUN(1)PASS(6)      FAIL(1)PASS(1)
*PNV  igt_gem_fence_thrash_bo-write-verify-x      PASS(7)      FAIL(1)PASS(1)
*PNV  igt_gem_fence_thrash_bo-write-verify-y      NO_RESULT(1)PASS(6)      FAIL(1)PASS(1)
 PNV  igt_gem_userptr_blits_coherency-sync      NO_RESULT(1)CRASH(7)NRUN(1)PASS(7)      CRASH(2)
 PNV  igt_gem_userptr_blits_minor-sync-interruptible      DMESG_WARN(1)PASS(2)      DMESG_WARN(2)
 PNV  igt_gen3_render_linear_blits      FAIL(5)NRUN(1)DMESG_WARN(1)PASS(9)      FAIL(2)
 PNV  igt_gen3_render_mixed_blits      FAIL(8)PASS(9)      FAIL(2)
 PNV  igt_gem_fence_thrash_bo-write-verify-threaded-none      FAIL(2)CRASH(4)PASS(6)      CRASH(2)
*ILK  igt_drv_suspend_debugfs-reader      PASS(2)      NO_RESULT(1)
*ILK  igt_drv_suspend_fence-restore-tiled2untiled      PASS(2)      NO_RESULT(1)
*ILK  igt_drv_suspend_fence-restore-untiled      PASS(2)      NO_RESULT(1)
*ILK  igt_drv_suspend_forcewake      PASS(2)      NO_RESULT(1)
*ILK  igt_gem_workarounds_suspend-resume      PASS(2)      NO_RESULT(1)
*SNB  igt_gem_fenced_exec_thrash_no-spare-fences-interruptible      PASS(2)      DMESG_WARN(1)PASS(1)
*BDW  igt_gem_gtt_hog      PASS(19)      DMESG_WARN(1)PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 00/23] Remove depencies on staged config for atomic transition

[Daniel Vetter]

On Tue, Mar 03, 2015 at 03:21:54PM +0200, Ander Conselvan de Oliveira wrote:
> This patch series starts to remove dependencies from the modeset code to
> enable the transition to atomic. That is achieved by using an atomic
> state struct for the legacy modeset, and changing related functiond to
> depend on it.
> 
> I wasn't able to test all of the changes, so I'm very interested on
> PRTS results for this. In particular, I expect support for 3 pipes and
> the load detect pipe to be problematic.

Matt has promised to create an igt using my load-detect test code. With
that you can test load-detect on any modern-ish platform that still
supports vga (i.e. up to hsw).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx