[PATCH 00/11] Small modeset refactoring

[PATCH 00/11] Small modeset refactoring

[Ander Conselvan de Oliveira]

Hi,

This patch series simplifies the legacy modeset a bit. I'm not sure
the *_pipes flags removal is done properly, so I only signed off
up to patch 4. My hope here is to get some comments and see how badly
does PRTS screams.

Thanks,
Ander

Ander Conselvan de Oliveira (11):
  drm/i915: Allocate connector state together with the connectors
  drm/i915: Remove stale comment from __intel_set_mode()
  drm/i915: Reset changed flags when duplicating crtc state
  drm/i915: Add for_each_connector_in_state helper macro
  drm/i915: Extract mode_changed computation out of
    stage_output_config()
  drm/i915: Add crtc states before calling compute_config()
  drm/i915: Remove all *_pipes flags from modeset
  drm/i915: Remove saved_mode from __intel_set_mode()
  drm/i915: Move compute part of __intel_set_mode() to separate function
  drm/i915: Simplify error handling in __intel_set_mode()
  drm/i915: Don't modeset with old mode when set_crtc fails

 drivers/gpu/drm/i915/i915_drv.h      |  16 +
 drivers/gpu/drm/i915/intel_atomic.c  |   6 +-
 drivers/gpu/drm/i915/intel_crt.c     |   2 +-
 drivers/gpu/drm/i915/intel_ddi.c     |  13 +-
 drivers/gpu/drm/i915/intel_display.c | 653 +++++++++++++++--------------------
 drivers/gpu/drm/i915/intel_dp.c      |   2 +-
 drivers/gpu/drm/i915/intel_dp_mst.c  |  14 +-
 drivers/gpu/drm/i915/intel_drv.h     |   2 +
 drivers/gpu/drm/i915/intel_dsi.c     |   2 +-
 drivers/gpu/drm/i915/intel_dvo.c     |   2 +-
 drivers/gpu/drm/i915/intel_hdmi.c    |   9 +-
 drivers/gpu/drm/i915/intel_lvds.c    |   6 +
 drivers/gpu/drm/i915/intel_sdvo.c    |  22 +-
 drivers/gpu/drm/i915/intel_tv.c      |   2 +-
 14 files changed, 343 insertions(+), 408 deletions(-)

-- 
2.1.0

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

[PATCH 01/11] drm/i915: Allocate connector state together with the connectors

[Ander Conselvan de Oliveira]

Connector states were being allocated in intel_setup_outputs() in loop
over all connectors. That meant hot-added connectors would have a NULL
state. Since the change to use a struct drm_atomic_state for the legacy
modeset, connector states are necessary for the i915 driver to function
properly, so that would lead to oopses.

v2: Fix test for intel_connector_init() success in lvds and sdvo (PRTS)

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_ddi.c     |  4 +--
 drivers/gpu/drm/i915/intel_display.c | 62 ++++++++++++++++--------------------
 drivers/gpu/drm/i915/intel_dp.c      |  2 +-
 drivers/gpu/drm/i915/intel_dp_mst.c  |  2 +-
 drivers/gpu/drm/i915/intel_drv.h     |  2 ++
 drivers/gpu/drm/i915/intel_dsi.c     |  2 +-
 drivers/gpu/drm/i915/intel_dvo.c     |  2 +-
 drivers/gpu/drm/i915/intel_hdmi.c    |  2 +-
 drivers/gpu/drm/i915/intel_lvds.c    |  6 ++++
 drivers/gpu/drm/i915/intel_sdvo.c    | 22 +++++++++++--
 drivers/gpu/drm/i915/intel_tv.c      |  2 +-
 12 files changed, 64 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index fa5699c..93bb515 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -851,7 +851,7 @@ void intel_crt_init(struct drm_device *dev)
 	if (!crt)
 		return;
 
-	intel_connector = kzalloc(sizeof(*intel_connector), GFP_KERNEL);
+	intel_connector = intel_connector_alloc();
 	if (!intel_connector) {
 		kfree(crt);
 		return;
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 8c692d8..486f6fa 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2101,7 +2101,7 @@ intel_ddi_init_dp_connector(struct intel_digital_port *intel_dig_port)
 	struct intel_connector *connector;
 	enum port port = intel_dig_port->port;
 
-	connector = kzalloc(sizeof(*connector), GFP_KERNEL);
+	connector = intel_connector_alloc();
 	if (!connector)
 		return NULL;
 
@@ -2120,7 +2120,7 @@ intel_ddi_init_hdmi_connector(struct intel_digital_port *intel_dig_port)
 	struct intel_connector *connector;
 	enum port port = intel_dig_port->port;
 
-	connector = kzalloc(sizeof(*connector), GFP_KERNEL);
+	connector = intel_connector_alloc();
 	if (!connector)
 		return NULL;
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bc8e221..2263a71 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5658,6 +5658,34 @@ static void intel_connector_check_state(struct intel_connector *connector)
 	}
 }
 
+int intel_connector_init(struct intel_connector *connector)
+{
+	struct drm_connector_state *connector_state;
+
+	connector_state = kzalloc(sizeof *connector_state, GFP_KERNEL);
+	if (!connector_state)
+		return -ENOMEM;
+
+	connector->base.state = connector_state;
+	return 0;
+}
+
+struct intel_connector *intel_connector_alloc(void)
+{
+	struct intel_connector *connector;
+
+	connector = kzalloc(sizeof *connector, GFP_KERNEL);
+	if (!connector)
+		return NULL;
+
+	if (intel_connector_init(connector) < 0) {
+		kfree(connector);
+		return NULL;
+	}
+
+	return connector;
+}
+
 /* Even simpler default implementation, if there's really no special case to
  * consider. */
 void intel_connector_dpms(struct drm_connector *connector, int mode)
@@ -13135,7 +13163,6 @@ static void intel_setup_outputs(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_encoder *encoder;
-	struct drm_connector *connector;
 	bool dpd_is_edp = false;
 
 	intel_lvds_init(dev);
@@ -13271,39 +13298,6 @@ static void intel_setup_outputs(struct drm_device *dev)
 	if (SUPPORTS_TV(dev))
 		intel_tv_init(dev);
 
-	/*
-	 * FIXME:  We don't have full atomic support yet, but we want to be
-	 * able to enable/test plane updates via the atomic interface in the
-	 * meantime.  However as soon as we flip DRIVER_ATOMIC on, the DRM core
-	 * will take some atomic codepaths to lookup properties during
-	 * drmModeGetConnector() that unconditionally dereference
-	 * connector->state.
-	 *
-	 * We create a dummy connector state here for each connector to ensure
-	 * the DRM core doesn't try to dereference a NULL connector->state.
-	 * The actual connector properties will never be updated or contain
-	 * useful information, but since we're doing this specifically for
-	 * testing/debug of the plane operations (and only when a specific
-	 * kernel module option is given), that shouldn't really matter.
-	 *
-	 * We are also relying on these states to convert the legacy mode set
-	 * to use a drm_atomic_state struct. The states are kept consistent
-	 * with actual state, so that it is safe to rely on that instead of
-	 * the staged config.
-	 *
-	 * Once atomic support for crtc's + connectors lands, this loop should
-	 * be removed since we'll be setting up real connector state, which
-	 * will contain Intel-specific properties.
-	 */
-	list_for_each_entry(connector,
-			    &dev->mode_config.connector_list,
-			    head) {
-		if (!WARN_ON(connector->state)) {
-			connector->state = kzalloc(sizeof(*connector->state),
-						   GFP_KERNEL);
-		}
-	}
-
 	intel_psr_init(dev);
 
 	for_each_intel_encoder(dev, encoder) {
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1b87969..589cd92 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5602,7 +5602,7 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
 	if (!intel_dig_port)
 		return;
 
-	intel_connector = kzalloc(sizeof(*intel_connector), GFP_KERNEL);
+	intel_connector = intel_connector_alloc();
 	if (!intel_connector) {
 		kfree(intel_dig_port);
 		return;
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index adcc5e6..7335089 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -415,7 +415,7 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 	struct drm_connector *connector;
 	int i;
 
-	intel_connector = kzalloc(sizeof(*intel_connector), GFP_KERNEL);
+	intel_connector = intel_connector_alloc();
 	if (!intel_connector)
 		return NULL;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 686014b..e07dc4a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -928,6 +928,8 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc);
 void intel_crtc_control(struct drm_crtc *crtc, bool enable);
 void intel_crtc_update_dpms(struct drm_crtc *crtc);
 void intel_encoder_destroy(struct drm_encoder *encoder);
+int intel_connector_init(struct intel_connector *);
+struct intel_connector *intel_connector_alloc(void);
 void intel_connector_dpms(struct drm_connector *, int mode);
 bool intel_connector_get_hw_state(struct intel_connector *connector);
 void intel_modeset_check_state(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 572251e..5196642 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -1007,7 +1007,7 @@ void intel_dsi_init(struct drm_device *dev)
 	if (!intel_dsi)
 		return;
 
-	intel_connector = kzalloc(sizeof(*intel_connector), GFP_KERNEL);
+	intel_connector = intel_connector_alloc();
 	if (!intel_connector) {
 		kfree(intel_dsi);
 		return;
diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
index 9a27ec7..7c9f852 100644
--- a/drivers/gpu/drm/i915/intel_dvo.c
+++ b/drivers/gpu/drm/i915/intel_dvo.c
@@ -469,7 +469,7 @@ void intel_dvo_init(struct drm_device *dev)
 	if (!intel_dvo)
 		return;
 
-	intel_connector = kzalloc(sizeof(*intel_connector), GFP_KERNEL);
+	intel_connector = intel_connector_alloc();
 	if (!intel_connector) {
 		kfree(intel_dvo);
 		return;
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 26222e6..02252d9 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1755,7 +1755,7 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
 	if (!intel_dig_port)
 		return;
 
-	intel_connector = kzalloc(sizeof(*intel_connector), GFP_KERNEL);
+	intel_connector = intel_connector_alloc();
 	if (!intel_connector) {
 		kfree(intel_dig_port);
 		return;
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 314a5d5..9a9df0f 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -946,6 +946,12 @@ void intel_lvds_init(struct drm_device *dev)
 		return;
 	}
 
+	if (intel_connector_init(&lvds_connector->base) < 0) {
+		kfree(lvds_connector);
+		kfree(lvds_encoder);
+		return;
+	}
+
 	lvds_encoder->attached_connector = lvds_connector;
 
 	intel_encoder = &lvds_encoder->base;
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index b121796..10cd332 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2427,6 +2427,22 @@ intel_sdvo_add_hdmi_properties(struct intel_sdvo *intel_sdvo,
 	}
 }
 
+static struct intel_sdvo_connector *intel_sdvo_connector_alloc(void)
+{
+	struct intel_sdvo_connector *sdvo_connector;
+
+	sdvo_connector = kzalloc(sizeof(*sdvo_connector), GFP_KERNEL);
+	if (!sdvo_connector)
+		return NULL;
+
+	if (intel_connector_init(&sdvo_connector->base) < 0) {
+		kfree(sdvo_connector);
+		return NULL;
+	}
+
+	return sdvo_connector;
+}
+
 static bool
 intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
 {
@@ -2438,7 +2454,7 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
 
 	DRM_DEBUG_KMS("initialising DVI device %d\n", device);
 
-	intel_sdvo_connector = kzalloc(sizeof(*intel_sdvo_connector), GFP_KERNEL);
+	intel_sdvo_connector = intel_sdvo_connector_alloc();
 	if (!intel_sdvo_connector)
 		return false;
 
@@ -2492,7 +2508,7 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type)
 
 	DRM_DEBUG_KMS("initialising TV type %d\n", type);
 
-	intel_sdvo_connector = kzalloc(sizeof(*intel_sdvo_connector), GFP_KERNEL);
+	intel_sdvo_connector = intel_sdvo_connector_alloc();
 	if (!intel_sdvo_connector)
 		return false;
 
@@ -2571,7 +2587,7 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
 
 	DRM_DEBUG_KMS("initialising LVDS device %d\n", device);
 
-	intel_sdvo_connector = kzalloc(sizeof(*intel_sdvo_connector), GFP_KERNEL);
+	intel_sdvo_connector = intel_sdvo_connector_alloc();
 	if (!intel_sdvo_connector)
 		return false;
 
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index bc1d9d7..8b9d325 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1621,7 +1621,7 @@ intel_tv_init(struct drm_device *dev)
 		return;
 	}
 
-	intel_connector = kzalloc(sizeof(*intel_connector), GFP_KERNEL);
+	intel_connector = intel_connector_alloc();
 	if (!intel_connector) {
 		kfree(intel_tv);
 		return;
-- 
2.1.0

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

[PATCH 02/11] drm/i915: Remove stale comment from __intel_set_mode()

[Ander Conselvan de Oliveira]

Since the following commit, the PLL calculations are done earlier, so
the code following the comment doesn't do anything PLL or encoder
related. It only updates the primary plane now.

commit f3019a4d92f08b2dd92443a4b567a066a51c6ec0
Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Date:   Wed Oct 29 11:32:37 2014 +0200

    drm/i915: Remove crtc_mode_set() hook

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2263a71..894788d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11737,9 +11737,6 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 
 	modeset_update_crtc_power_domains(state);
 
-	/* Set up the DPLL and any encoders state that needs to adjust or depend
-	 * on the DPLL.
-	 */
 	for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
 		struct drm_plane *primary = intel_crtc->base.primary;
 		int vdisplay, hdisplay;
-- 
2.1.0

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

Re: [PATCH 02/11] drm/i915: Remove stale comment from __intel_set_mode()

[Daniel Vetter]

On Fri, Apr 10, 2015 at 11:38:31AM +0300, Ander Conselvan de Oliveira wrote:
> Since the following commit, the PLL calculations are done earlier, so
> the code following the comment doesn't do anything PLL or encoder
> related. It only updates the primary plane now.
> 
> commit f3019a4d92f08b2dd92443a4b567a066a51c6ec0
> Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> Date:   Wed Oct 29 11:32:37 2014 +0200
> 
>     drm/i915: Remove crtc_mode_set() hook
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

Queued for -next, thanks for the patch.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2263a71..894788d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11737,9 +11737,6 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  
>  	modeset_update_crtc_power_domains(state);
>  
> -	/* Set up the DPLL and any encoders state that needs to adjust or depend
> -	 * on the DPLL.
> -	 */
>  	for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
>  		struct drm_plane *primary = intel_crtc->base.primary;
>  		int vdisplay, hdisplay;
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/11] drm/i915: Remove stale comment from __intel_set_mode()

[Daniel Vetter]

On Fri, Apr 10, 2015 at 11:38:31AM +0300, Ander Conselvan de Oliveira wrote:
> Since the following commit, the PLL calculations are done earlier, so
> the code following the comment doesn't do anything PLL or encoder
> related. It only updates the primary plane now.
> 
> commit f3019a4d92f08b2dd92443a4b567a066a51c6ec0
> Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> Date:   Wed Oct 29 11:32:37 2014 +0200
> 
>     drm/i915: Remove crtc_mode_set() hook
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

Queued for -next, thanks for the patch.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2263a71..894788d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11737,9 +11737,6 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  
>  	modeset_update_crtc_power_domains(state);
>  
> -	/* Set up the DPLL and any encoders state that needs to adjust or depend
> -	 * on the DPLL.
> -	 */
>  	for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
>  		struct drm_plane *primary = intel_crtc->base.primary;
>  		int vdisplay, hdisplay;
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 03/11] drm/i915: Reset changed flags when duplicating crtc state

[Ander Conselvan de Oliveira]

Otherwise, once we start doing atomic updates, the values from a
previous update might be used, leading to unnecessary changes.

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

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 3903b90..f951782 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -222,8 +222,12 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
 		crtc_state = kmemdup(intel_crtc->config,
 				     sizeof(*intel_crtc->config), GFP_KERNEL);
 
-	if (crtc_state)
+	if (crtc_state) {
 		crtc_state->base.crtc = crtc;
+		crtc_state->base.mode_changed = 0;
+		crtc_state->base.active_changed = 0;
+		crtc_state->base.planes_changed = 0;
+	}
 
 	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 03/11] drm/i915: Reset changed flags when duplicating crtc state

[Daniel Vetter]

On Fri, Apr 10, 2015 at 11:38:32AM +0300, Ander Conselvan de Oliveira wrote:
> Otherwise, once we start doing atomic updates, the values from a
> previous update might be used, leading to unnecessary changes.
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

Imo better to use the new helpers added in

commit f5e7840b0c4368f8cdbb055188c2a0eef50c3052
Author: Thierry Reding <treding@nvidia.com>
Date:   Wed Jan 28 14:54:32 2015 +0100

    drm/atomic: Add helpers for state-subclassing drivers

Probably best to roll them out for all our state objects.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_atomic.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 3903b90..f951782 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -222,8 +222,12 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
>  		crtc_state = kmemdup(intel_crtc->config,
>  				     sizeof(*intel_crtc->config), GFP_KERNEL);
>  
> -	if (crtc_state)
> +	if (crtc_state) {
>  		crtc_state->base.crtc = crtc;
> +		crtc_state->base.mode_changed = 0;
> +		crtc_state->base.active_changed = 0;
> +		crtc_state->base.planes_changed = 0;
> +	}
>  
>  	return &crtc_state->base;
>  }
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/11] drm/i915: Reset changed flags when duplicating crtc state

[Jani Nikula]

On Fri, 10 Apr 2015, Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> wrote:
> Otherwise, once we start doing atomic updates, the values from a
> previous update might be used, leading to unnecessary changes.
>
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 3903b90..f951782 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -222,8 +222,12 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
>  		crtc_state = kmemdup(intel_crtc->config,
>  				     sizeof(*intel_crtc->config), GFP_KERNEL);
>  
> -	if (crtc_state)
> +	if (crtc_state) {
>  		crtc_state->base.crtc = crtc;
> +		crtc_state->base.mode_changed = 0;
> +		crtc_state->base.active_changed = 0;
> +		crtc_state->base.planes_changed = 0;

They are bools, please use false not 0.

BR,
Jani.

> +	}
>  
>  	return &crtc_state->base;
>  }
> -- 
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 04/11] drm/i915: Add for_each_connector_in_state helper macro

[Ander Conselvan de Oliveira]

This saves some typing whenever a iteration over all the connector
states in the atomic state is written, which happens quite often.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  8 +++++
 drivers/gpu/drm/i915/intel_ddi.c     |  9 ++---
 drivers/gpu/drm/i915/intel_display.c | 65 ++++++++++--------------------------
 drivers/gpu/drm/i915/intel_dp_mst.c  | 12 +++----
 drivers/gpu/drm/i915/intel_hdmi.c    |  7 ++--
 5 files changed, 38 insertions(+), 63 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 78f31cb..e12bc5a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -264,6 +264,14 @@ enum hpd_pin {
 	for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++)	\
 		if ((1 << (domain)) & (mask))
 
+#define for_each_connector_in_state(state, connector, connector_state, __i) \
+	for ((__i) = 0;							\
+	     (connector) = to_intel_connector((state)->connectors[__i]), \
+	     (connector_state) = (state)->connector_states[__i],	\
+	     (__i) < (state)->num_connector;				\
+	     (__i)++)							\
+		if (connector)
+
 struct drm_i915_private;
 struct i915_mm_struct;
 struct i915_mmu_object;
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 486f6fa..2e005e3 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -496,18 +496,19 @@ intel_ddi_get_crtc_new_encoder(struct intel_crtc_state *crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
 	struct intel_encoder *ret = NULL;
+	struct intel_connector *connector;
 	struct drm_atomic_state *state;
+	struct drm_connector_state *connector_state;
 	int num_encoders = 0;
 	int i;
 
 	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)
+	for_each_connector_in_state(state, connector, connector_state, i) {
+		if (connector_state->crtc != crtc_state->base.crtc)
 			continue;
 
-		ret = to_intel_encoder(state->connector_states[i]->best_encoder);
+		ret = to_intel_encoder(connector_state->best_encoder);
 		num_encoders++;
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 894788d..3035a3d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -437,13 +437,10 @@ static bool intel_pipe_will_have_type(const struct intel_crtc_state *crtc_state,
 	struct drm_atomic_state *state = crtc_state->base.state;
 	struct drm_connector_state *connector_state;
 	struct intel_encoder *encoder;
+	struct intel_connector *connector;
 	int i, num_connectors = 0;
 
-	for (i = 0; i < state->num_connector; i++) {
-		if (!state->connectors[i])
-			continue;
-
-		connector_state = state->connector_states[i];
+	for_each_connector_in_state(state, connector, connector_state, i) {
 		if (connector_state->crtc != crtc_state->base.crtc)
 			continue;
 
@@ -6919,16 +6916,13 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc,
 	bool ok, has_reduced_clock = false;
 	bool is_lvds = false, is_dsi = false;
 	struct intel_encoder *encoder;
+	struct intel_connector *connector;
 	const intel_limit_t *limit;
 	struct drm_atomic_state *state = crtc_state->base.state;
 	struct drm_connector_state *connector_state;
 	int i;
 
-	for (i = 0; i < state->num_connector; i++) {
-		if (!state->connectors[i])
-			continue;
-
-		connector_state = state->connector_states[i];
+	for_each_connector_in_state(state, connector, connector_state, i) {
 		if (connector_state->crtc != &crtc->base)
 			continue;
 
@@ -7614,14 +7608,11 @@ static int ironlake_get_refclk(struct intel_crtc_state *crtc_state)
 	struct drm_atomic_state *state = crtc_state->base.state;
 	struct drm_connector_state *connector_state;
 	struct intel_encoder *encoder;
+	struct intel_connector *connector;
 	int num_connectors = 0, i;
 	bool is_lvds = false;
 
-	for (i = 0; i < state->num_connector; i++) {
-		if (!state->connectors[i])
-			continue;
-
-		connector_state = state->connector_states[i];
+	for_each_connector_in_state(state, connector, connector_state, i) {
 		if (connector_state->crtc != crtc_state->base.crtc)
 			continue;
 
@@ -7877,15 +7868,12 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
 	struct drm_atomic_state *state = crtc_state->base.state;
 	struct drm_connector_state *connector_state;
 	struct intel_encoder *encoder;
+	struct intel_connector *connector;
 	uint32_t dpll;
 	int factor, num_connectors = 0, i;
 	bool is_lvds = false, is_sdvo = false;
 
-	for (i = 0; i < state->num_connector; i++) {
-		if (!state->connectors[i])
-			continue;
-
-		connector_state = state->connector_states[i];
+	for_each_connector_in_state(state, connector, connector_state, i) {
 		if (connector_state->crtc != crtc_state->base.crtc)
 			continue;
 
@@ -10522,6 +10510,7 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc,
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_atomic_state *state;
+	struct drm_connector_state *connector_state;
 	struct intel_connector *connector;
 	int bpp, i;
 
@@ -10566,12 +10555,8 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc,
 	state = pipe_config->base.state;
 
 	/* Clamp display bpp to EDID value */
-	for (i = 0; i < state->num_connector; i++) {
-		if (!state->connectors[i])
-			continue;
-
-		connector = to_intel_connector(state->connectors[i]);
-		if (state->connector_states[i]->crtc != &crtc->base)
+	for_each_connector_in_state(state, connector, connector_state, i) {
+		if (connector_state->crtc != &crtc->base)
 			continue;
 
 		connected_sink_compute_bpp(connector, pipe_config);
@@ -10658,14 +10643,11 @@ static bool check_single_encoder_cloning(struct drm_atomic_state *state,
 					 struct intel_encoder *encoder)
 {
 	struct intel_encoder *source_encoder;
+	struct intel_connector *connector;
 	struct drm_connector_state *connector_state;
 	int i;
 
-	for (i = 0; i < state->num_connector; i++) {
-		if (!state->connectors[i])
-			continue;
-
-		connector_state = state->connector_states[i];
+	for_each_connector_in_state(state, connector, connector_state, i) {
 		if (connector_state->crtc != &crtc->base)
 			continue;
 
@@ -10682,14 +10664,11 @@ static bool check_encoder_cloning(struct drm_atomic_state *state,
 				  struct intel_crtc *crtc)
 {
 	struct intel_encoder *encoder;
+	struct intel_connector *connector;
 	struct drm_connector_state *connector_state;
 	int i;
 
-	for (i = 0; i < state->num_connector; i++) {
-		if (!state->connectors[i])
-			continue;
-
-		connector_state = state->connector_states[i];
+	for_each_connector_in_state(state, connector, connector_state, i) {
 		if (connector_state->crtc != &crtc->base)
 			continue;
 
@@ -10705,6 +10684,7 @@ static bool check_digital_port_conflicts(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
 	struct intel_encoder *encoder;
+	struct intel_connector *connector;
 	struct drm_connector_state *connector_state;
 	unsigned int used_ports = 0;
 	int i;
@@ -10714,11 +10694,7 @@ static bool check_digital_port_conflicts(struct drm_atomic_state *state)
 	 * list to detect the problem on ddi platforms
 	 * where there's just one encoder per digital port.
 	 */
-	for (i = 0; i < state->num_connector; i++) {
-		if (!state->connectors[i])
-			continue;
-
-		connector_state = state->connector_states[i];
+	for_each_connector_in_state(state, connector, connector_state, i) {
 		if (!connector_state->best_encoder)
 			continue;
 
@@ -10845,12 +10821,7 @@ encoder_retry:
 	 * adjust it according to limitations or connector properties, and also
 	 * a chance to reject the mode entirely.
 	 */
-	for (i = 0; i < state->num_connector; i++) {
-		connector = to_intel_connector(state->connectors[i]);
-		if (!connector)
-			continue;
-
-		connector_state = state->connector_states[i];
+	for_each_connector_in_state(state, connector, connector_state, i) {
 		if (connector_state->crtc != crtc)
 			continue;
 
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 7335089..5561eca 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -40,7 +40,8 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 	int bpp, i;
 	int lane_count, slots, rate;
 	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
-	struct intel_connector *found = NULL;
+	struct intel_connector *connector, *found = NULL;
+	struct drm_connector_state *connector_state;
 	int mst_pbn;
 
 	pipe_config->dp_encoder_is_mst = true;
@@ -70,12 +71,9 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 
 	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]);
+	for_each_connector_in_state(state, connector, connector_state, i) {
+		if (connector_state->best_encoder == &encoder->base) {
+			found = connector;
 			break;
 		}
 	}
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 02252d9..9d02746 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -956,6 +956,7 @@ static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state)
 	struct drm_device *dev = crtc_state->base.crtc->dev;
 	struct drm_atomic_state *state;
 	struct intel_encoder *encoder;
+	struct intel_connector *connector;
 	struct drm_connector_state *connector_state;
 	int count = 0, count_hdmi = 0;
 	int i;
@@ -965,11 +966,7 @@ static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state)
 
 	state = crtc_state->base.state;
 
-	for (i = 0; i < state->num_connector; i++) {
-		if (!state->connectors[i])
-			continue;
-
-		connector_state = state->connector_states[i];
+	for_each_connector_in_state(state, connector, connector_state, i) {
 		if (connector_state->crtc != crtc_state->base.crtc)
 			continue;
 
-- 
2.1.0

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

Re: [PATCH 04/11] drm/i915: Add for_each_connector_in_state helper macro

[Daniel Vetter]

On Fri, Apr 10, 2015 at 11:38:33AM +0300, Ander Conselvan de Oliveira wrote:
> This saves some typing whenever a iteration over all the connector
> states in the atomic state is written, which happens quite often.
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  8 +++++
>  drivers/gpu/drm/i915/intel_ddi.c     |  9 ++---
>  drivers/gpu/drm/i915/intel_display.c | 65 ++++++++++--------------------------
>  drivers/gpu/drm/i915/intel_dp_mst.c  | 12 +++----
>  drivers/gpu/drm/i915/intel_hdmi.c    |  7 ++--
>  5 files changed, 38 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 78f31cb..e12bc5a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -264,6 +264,14 @@ enum hpd_pin {
>  	for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++)	\
>  		if ((1 << (domain)) & (mask))
>  
> +#define for_each_connector_in_state(state, connector, connector_state, __i) \

Imo this should be a drm_atomic.h thing. At least there's a lot of loops
with exactly this pattern in drm_atomic_helper.c and drm_atomic.c

Maybe even do helpers for plane/crtc looping too.
-Daniel


> +	for ((__i) = 0;							\
> +	     (connector) = to_intel_connector((state)->connectors[__i]), \
> +	     (connector_state) = (state)->connector_states[__i],	\
> +	     (__i) < (state)->num_connector;				\
> +	     (__i)++)							\
> +		if (connector)
> +
>  struct drm_i915_private;
>  struct i915_mm_struct;
>  struct i915_mmu_object;
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 486f6fa..2e005e3 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -496,18 +496,19 @@ intel_ddi_get_crtc_new_encoder(struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>  	struct intel_encoder *ret = NULL;
> +	struct intel_connector *connector;
>  	struct drm_atomic_state *state;
> +	struct drm_connector_state *connector_state;
>  	int num_encoders = 0;
>  	int i;
>  
>  	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)
> +	for_each_connector_in_state(state, connector, connector_state, i) {
> +		if (connector_state->crtc != crtc_state->base.crtc)
>  			continue;
>  
> -		ret = to_intel_encoder(state->connector_states[i]->best_encoder);
> +		ret = to_intel_encoder(connector_state->best_encoder);
>  		num_encoders++;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 894788d..3035a3d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -437,13 +437,10 @@ static bool intel_pipe_will_have_type(const struct intel_crtc_state *crtc_state,
>  	struct drm_atomic_state *state = crtc_state->base.state;
>  	struct drm_connector_state *connector_state;
>  	struct intel_encoder *encoder;
> +	struct intel_connector *connector;
>  	int i, num_connectors = 0;
>  
> -	for (i = 0; i < state->num_connector; i++) {
> -		if (!state->connectors[i])
> -			continue;
> -
> -		connector_state = state->connector_states[i];
> +	for_each_connector_in_state(state, connector, connector_state, i) {
>  		if (connector_state->crtc != crtc_state->base.crtc)
>  			continue;
>  
> @@ -6919,16 +6916,13 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc,
>  	bool ok, has_reduced_clock = false;
>  	bool is_lvds = false, is_dsi = false;
>  	struct intel_encoder *encoder;
> +	struct intel_connector *connector;
>  	const intel_limit_t *limit;
>  	struct drm_atomic_state *state = crtc_state->base.state;
>  	struct drm_connector_state *connector_state;
>  	int i;
>  
> -	for (i = 0; i < state->num_connector; i++) {
> -		if (!state->connectors[i])
> -			continue;
> -
> -		connector_state = state->connector_states[i];
> +	for_each_connector_in_state(state, connector, connector_state, i) {
>  		if (connector_state->crtc != &crtc->base)
>  			continue;
>  
> @@ -7614,14 +7608,11 @@ static int ironlake_get_refclk(struct intel_crtc_state *crtc_state)
>  	struct drm_atomic_state *state = crtc_state->base.state;
>  	struct drm_connector_state *connector_state;
>  	struct intel_encoder *encoder;
> +	struct intel_connector *connector;
>  	int num_connectors = 0, i;
>  	bool is_lvds = false;
>  
> -	for (i = 0; i < state->num_connector; i++) {
> -		if (!state->connectors[i])
> -			continue;
> -
> -		connector_state = state->connector_states[i];
> +	for_each_connector_in_state(state, connector, connector_state, i) {
>  		if (connector_state->crtc != crtc_state->base.crtc)
>  			continue;
>  
> @@ -7877,15 +7868,12 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
>  	struct drm_atomic_state *state = crtc_state->base.state;
>  	struct drm_connector_state *connector_state;
>  	struct intel_encoder *encoder;
> +	struct intel_connector *connector;
>  	uint32_t dpll;
>  	int factor, num_connectors = 0, i;
>  	bool is_lvds = false, is_sdvo = false;
>  
> -	for (i = 0; i < state->num_connector; i++) {
> -		if (!state->connectors[i])
> -			continue;
> -
> -		connector_state = state->connector_states[i];
> +	for_each_connector_in_state(state, connector, connector_state, i) {
>  		if (connector_state->crtc != crtc_state->base.crtc)
>  			continue;
>  
> @@ -10522,6 +10510,7 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc,
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_atomic_state *state;
> +	struct drm_connector_state *connector_state;
>  	struct intel_connector *connector;
>  	int bpp, i;
>  
> @@ -10566,12 +10555,8 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc,
>  	state = pipe_config->base.state;
>  
>  	/* Clamp display bpp to EDID value */
> -	for (i = 0; i < state->num_connector; i++) {
> -		if (!state->connectors[i])
> -			continue;
> -
> -		connector = to_intel_connector(state->connectors[i]);
> -		if (state->connector_states[i]->crtc != &crtc->base)
> +	for_each_connector_in_state(state, connector, connector_state, i) {
> +		if (connector_state->crtc != &crtc->base)
>  			continue;
>  
>  		connected_sink_compute_bpp(connector, pipe_config);
> @@ -10658,14 +10643,11 @@ static bool check_single_encoder_cloning(struct drm_atomic_state *state,
>  					 struct intel_encoder *encoder)
>  {
>  	struct intel_encoder *source_encoder;
> +	struct intel_connector *connector;
>  	struct drm_connector_state *connector_state;
>  	int i;
>  
> -	for (i = 0; i < state->num_connector; i++) {
> -		if (!state->connectors[i])
> -			continue;
> -
> -		connector_state = state->connector_states[i];
> +	for_each_connector_in_state(state, connector, connector_state, i) {
>  		if (connector_state->crtc != &crtc->base)
>  			continue;
>  
> @@ -10682,14 +10664,11 @@ static bool check_encoder_cloning(struct drm_atomic_state *state,
>  				  struct intel_crtc *crtc)
>  {
>  	struct intel_encoder *encoder;
> +	struct intel_connector *connector;
>  	struct drm_connector_state *connector_state;
>  	int i;
>  
> -	for (i = 0; i < state->num_connector; i++) {
> -		if (!state->connectors[i])
> -			continue;
> -
> -		connector_state = state->connector_states[i];
> +	for_each_connector_in_state(state, connector, connector_state, i) {
>  		if (connector_state->crtc != &crtc->base)
>  			continue;
>  
> @@ -10705,6 +10684,7 @@ static bool check_digital_port_conflicts(struct drm_atomic_state *state)
>  {
>  	struct drm_device *dev = state->dev;
>  	struct intel_encoder *encoder;
> +	struct intel_connector *connector;
>  	struct drm_connector_state *connector_state;
>  	unsigned int used_ports = 0;
>  	int i;
> @@ -10714,11 +10694,7 @@ static bool check_digital_port_conflicts(struct drm_atomic_state *state)
>  	 * list to detect the problem on ddi platforms
>  	 * where there's just one encoder per digital port.
>  	 */
> -	for (i = 0; i < state->num_connector; i++) {
> -		if (!state->connectors[i])
> -			continue;
> -
> -		connector_state = state->connector_states[i];
> +	for_each_connector_in_state(state, connector, connector_state, i) {
>  		if (!connector_state->best_encoder)
>  			continue;
>  
> @@ -10845,12 +10821,7 @@ encoder_retry:
>  	 * adjust it according to limitations or connector properties, and also
>  	 * a chance to reject the mode entirely.
>  	 */
> -	for (i = 0; i < state->num_connector; i++) {
> -		connector = to_intel_connector(state->connectors[i]);
> -		if (!connector)
> -			continue;
> -
> -		connector_state = state->connector_states[i];
> +	for_each_connector_in_state(state, connector, connector_state, i) {
>  		if (connector_state->crtc != crtc)
>  			continue;
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 7335089..5561eca 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -40,7 +40,8 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  	int bpp, i;
>  	int lane_count, slots, rate;
>  	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> -	struct intel_connector *found = NULL;
> +	struct intel_connector *connector, *found = NULL;
> +	struct drm_connector_state *connector_state;
>  	int mst_pbn;
>  
>  	pipe_config->dp_encoder_is_mst = true;
> @@ -70,12 +71,9 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  
>  	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]);
> +	for_each_connector_in_state(state, connector, connector_state, i) {
> +		if (connector_state->best_encoder == &encoder->base) {
> +			found = connector;
>  			break;
>  		}
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 02252d9..9d02746 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -956,6 +956,7 @@ static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state)
>  	struct drm_device *dev = crtc_state->base.crtc->dev;
>  	struct drm_atomic_state *state;
>  	struct intel_encoder *encoder;
> +	struct intel_connector *connector;
>  	struct drm_connector_state *connector_state;
>  	int count = 0, count_hdmi = 0;
>  	int i;
> @@ -965,11 +966,7 @@ static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state)
>  
>  	state = crtc_state->base.state;
>  
> -	for (i = 0; i < state->num_connector; i++) {
> -		if (!state->connectors[i])
> -			continue;
> -
> -		connector_state = state->connector_states[i];
> +	for_each_connector_in_state(state, connector, connector_state, i) {
>  		if (connector_state->crtc != crtc_state->base.crtc)
>  			continue;
>  
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/11] drm/i915: Add for_each_connector_in_state helper macro

[Ander Conselvan De Oliveira]

On Fri, 2015-04-10 at 11:42 +0200, Daniel Vetter wrote:
> On Fri, Apr 10, 2015 at 11:38:33AM +0300, Ander Conselvan de Oliveira wrote:
> > This saves some typing whenever a iteration over all the connector
> > states in the atomic state is written, which happens quite often.
> > 
> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  8 +++++
> >  drivers/gpu/drm/i915/intel_ddi.c     |  9 ++---
> >  drivers/gpu/drm/i915/intel_display.c | 65 ++++++++++--------------------------
> >  drivers/gpu/drm/i915/intel_dp_mst.c  | 12 +++----
> >  drivers/gpu/drm/i915/intel_hdmi.c    |  7 ++--
> >  5 files changed, 38 insertions(+), 63 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 78f31cb..e12bc5a 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -264,6 +264,14 @@ enum hpd_pin {
> >  	for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++)	\
> >  		if ((1 << (domain)) & (mask))
> >  
> > +#define for_each_connector_in_state(state, connector, connector_state, __i) \
> 
> Imo this should be a drm_atomic.h thing. At least there's a lot of loops
> with exactly this pattern in drm_atomic_helper.c and drm_atomic.c
> 
> Maybe even do helpers for plane/crtc looping too.

We could do them, but usually we are interested in intel_crtc_state or
intel_plane_state, and we can't hide the casting in the macro if it is
drm_atomic.h as I did in patch 7.

Ander

> -Daniel
> 
> 
> > +	for ((__i) = 0;							\
> > +	     (connector) = to_intel_connector((state)->connectors[__i]), \
> > +	     (connector_state) = (state)->connector_states[__i],	\
> > +	     (__i) < (state)->num_connector;				\
> > +	     (__i)++)							\
> > +		if (connector)
> > +
> >  struct drm_i915_private;
> >  struct i915_mm_struct;
> >  struct i915_mmu_object;
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 486f6fa..2e005e3 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -496,18 +496,19 @@ intel_ddi_get_crtc_new_encoder(struct intel_crtc_state *crtc_state)
> >  {
> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> >  	struct intel_encoder *ret = NULL;
> > +	struct intel_connector *connector;
> >  	struct drm_atomic_state *state;
> > +	struct drm_connector_state *connector_state;
> >  	int num_encoders = 0;
> >  	int i;
> >  
> >  	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)
> > +	for_each_connector_in_state(state, connector, connector_state, i) {
> > +		if (connector_state->crtc != crtc_state->base.crtc)
> >  			continue;
> >  
> > -		ret = to_intel_encoder(state->connector_states[i]->best_encoder);
> > +		ret = to_intel_encoder(connector_state->best_encoder);
> >  		num_encoders++;
> >  	}
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 894788d..3035a3d 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -437,13 +437,10 @@ static bool intel_pipe_will_have_type(const struct intel_crtc_state *crtc_state,
> >  	struct drm_atomic_state *state = crtc_state->base.state;
> >  	struct drm_connector_state *connector_state;
> >  	struct intel_encoder *encoder;
> > +	struct intel_connector *connector;
> >  	int i, num_connectors = 0;
> >  
> > -	for (i = 0; i < state->num_connector; i++) {
> > -		if (!state->connectors[i])
> > -			continue;
> > -
> > -		connector_state = state->connector_states[i];
> > +	for_each_connector_in_state(state, connector, connector_state, i) {
> >  		if (connector_state->crtc != crtc_state->base.crtc)
> >  			continue;
> >  
> > @@ -6919,16 +6916,13 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc,
> >  	bool ok, has_reduced_clock = false;
> >  	bool is_lvds = false, is_dsi = false;
> >  	struct intel_encoder *encoder;
> > +	struct intel_connector *connector;
> >  	const intel_limit_t *limit;
> >  	struct drm_atomic_state *state = crtc_state->base.state;
> >  	struct drm_connector_state *connector_state;
> >  	int i;
> >  
> > -	for (i = 0; i < state->num_connector; i++) {
> > -		if (!state->connectors[i])
> > -			continue;
> > -
> > -		connector_state = state->connector_states[i];
> > +	for_each_connector_in_state(state, connector, connector_state, i) {
> >  		if (connector_state->crtc != &crtc->base)
> >  			continue;
> >  
> > @@ -7614,14 +7608,11 @@ static int ironlake_get_refclk(struct intel_crtc_state *crtc_state)
> >  	struct drm_atomic_state *state = crtc_state->base.state;
> >  	struct drm_connector_state *connector_state;
> >  	struct intel_encoder *encoder;
> > +	struct intel_connector *connector;
> >  	int num_connectors = 0, i;
> >  	bool is_lvds = false;
> >  
> > -	for (i = 0; i < state->num_connector; i++) {
> > -		if (!state->connectors[i])
> > -			continue;
> > -
> > -		connector_state = state->connector_states[i];
> > +	for_each_connector_in_state(state, connector, connector_state, i) {
> >  		if (connector_state->crtc != crtc_state->base.crtc)
> >  			continue;
> >  
> > @@ -7877,15 +7868,12 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
> >  	struct drm_atomic_state *state = crtc_state->base.state;
> >  	struct drm_connector_state *connector_state;
> >  	struct intel_encoder *encoder;
> > +	struct intel_connector *connector;
> >  	uint32_t dpll;
> >  	int factor, num_connectors = 0, i;
> >  	bool is_lvds = false, is_sdvo = false;
> >  
> > -	for (i = 0; i < state->num_connector; i++) {
> > -		if (!state->connectors[i])
> > -			continue;
> > -
> > -		connector_state = state->connector_states[i];
> > +	for_each_connector_in_state(state, connector, connector_state, i) {
> >  		if (connector_state->crtc != crtc_state->base.crtc)
> >  			continue;
> >  
> > @@ -10522,6 +10510,7 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc,
> >  {
> >  	struct drm_device *dev = crtc->base.dev;
> >  	struct drm_atomic_state *state;
> > +	struct drm_connector_state *connector_state;
> >  	struct intel_connector *connector;
> >  	int bpp, i;
> >  
> > @@ -10566,12 +10555,8 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc,
> >  	state = pipe_config->base.state;
> >  
> >  	/* Clamp display bpp to EDID value */
> > -	for (i = 0; i < state->num_connector; i++) {
> > -		if (!state->connectors[i])
> > -			continue;
> > -
> > -		connector = to_intel_connector(state->connectors[i]);
> > -		if (state->connector_states[i]->crtc != &crtc->base)
> > +	for_each_connector_in_state(state, connector, connector_state, i) {
> > +		if (connector_state->crtc != &crtc->base)
> >  			continue;
> >  
> >  		connected_sink_compute_bpp(connector, pipe_config);
> > @@ -10658,14 +10643,11 @@ static bool check_single_encoder_cloning(struct drm_atomic_state *state,
> >  					 struct intel_encoder *encoder)
> >  {
> >  	struct intel_encoder *source_encoder;
> > +	struct intel_connector *connector;
> >  	struct drm_connector_state *connector_state;
> >  	int i;
> >  
> > -	for (i = 0; i < state->num_connector; i++) {
> > -		if (!state->connectors[i])
> > -			continue;
> > -
> > -		connector_state = state->connector_states[i];
> > +	for_each_connector_in_state(state, connector, connector_state, i) {
> >  		if (connector_state->crtc != &crtc->base)
> >  			continue;
> >  
> > @@ -10682,14 +10664,11 @@ static bool check_encoder_cloning(struct drm_atomic_state *state,
> >  				  struct intel_crtc *crtc)
> >  {
> >  	struct intel_encoder *encoder;
> > +	struct intel_connector *connector;
> >  	struct drm_connector_state *connector_state;
> >  	int i;
> >  
> > -	for (i = 0; i < state->num_connector; i++) {
> > -		if (!state->connectors[i])
> > -			continue;
> > -
> > -		connector_state = state->connector_states[i];
> > +	for_each_connector_in_state(state, connector, connector_state, i) {
> >  		if (connector_state->crtc != &crtc->base)
> >  			continue;
> >  
> > @@ -10705,6 +10684,7 @@ static bool check_digital_port_conflicts(struct drm_atomic_state *state)
> >  {
> >  	struct drm_device *dev = state->dev;
> >  	struct intel_encoder *encoder;
> > +	struct intel_connector *connector;
> >  	struct drm_connector_state *connector_state;
> >  	unsigned int used_ports = 0;
> >  	int i;
> > @@ -10714,11 +10694,7 @@ static bool check_digital_port_conflicts(struct drm_atomic_state *state)
> >  	 * list to detect the problem on ddi platforms
> >  	 * where there's just one encoder per digital port.
> >  	 */
> > -	for (i = 0; i < state->num_connector; i++) {
> > -		if (!state->connectors[i])
> > -			continue;
> > -
> > -		connector_state = state->connector_states[i];
> > +	for_each_connector_in_state(state, connector, connector_state, i) {
> >  		if (!connector_state->best_encoder)
> >  			continue;
> >  
> > @@ -10845,12 +10821,7 @@ encoder_retry:
> >  	 * adjust it according to limitations or connector properties, and also
> >  	 * a chance to reject the mode entirely.
> >  	 */
> > -	for (i = 0; i < state->num_connector; i++) {
> > -		connector = to_intel_connector(state->connectors[i]);
> > -		if (!connector)
> > -			continue;
> > -
> > -		connector_state = state->connector_states[i];
> > +	for_each_connector_in_state(state, connector, connector_state, i) {
> >  		if (connector_state->crtc != crtc)
> >  			continue;
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index 7335089..5561eca 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -40,7 +40,8 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
> >  	int bpp, i;
> >  	int lane_count, slots, rate;
> >  	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> > -	struct intel_connector *found = NULL;
> > +	struct intel_connector *connector, *found = NULL;
> > +	struct drm_connector_state *connector_state;
> >  	int mst_pbn;
> >  
> >  	pipe_config->dp_encoder_is_mst = true;
> > @@ -70,12 +71,9 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
> >  
> >  	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]);
> > +	for_each_connector_in_state(state, connector, connector_state, i) {
> > +		if (connector_state->best_encoder == &encoder->base) {
> > +			found = connector;
> >  			break;
> >  		}
> >  	}
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 02252d9..9d02746 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -956,6 +956,7 @@ static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state)
> >  	struct drm_device *dev = crtc_state->base.crtc->dev;
> >  	struct drm_atomic_state *state;
> >  	struct intel_encoder *encoder;
> > +	struct intel_connector *connector;
> >  	struct drm_connector_state *connector_state;
> >  	int count = 0, count_hdmi = 0;
> >  	int i;
> > @@ -965,11 +966,7 @@ static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state)
> >  
> >  	state = crtc_state->base.state;
> >  
> > -	for (i = 0; i < state->num_connector; i++) {
> > -		if (!state->connectors[i])
> > -			continue;
> > -
> > -		connector_state = state->connector_states[i];
> > +	for_each_connector_in_state(state, connector, connector_state, i) {
> >  		if (connector_state->crtc != crtc_state->base.crtc)
> >  			continue;
> >  
> > -- 
> > 2.1.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 


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

[PATCH 05/11] drm/i915: Extract mode_changed computation out of stage_output_config()

[Ander Conselvan de Oliveira]

This should make the conversion to atomic easier, by splitting the
initialization of the atomic state from the logic that decides if a
modeset is needed.
---
 drivers/gpu/drm/i915/intel_display.c | 73 ++++++++++++++++++++----------------
 1 file changed, 41 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3035a3d..72ceb6d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11958,6 +11958,10 @@ static void
 intel_set_config_compute_mode_changes(struct drm_mode_set *set,
 				      struct intel_set_config *config)
 {
+	struct drm_device *dev = set->crtc->dev;
+	struct intel_connector *connector;
+	struct intel_encoder *encoder;
+	struct intel_crtc *crtc;
 
 	/* We should be able to check here if the fb has the same properties
 	 * and then just flip_or_move it */
@@ -12001,6 +12005,36 @@ intel_set_config_compute_mode_changes(struct drm_mode_set *set,
 		config->mode_changed = true;
 	}
 
+	for_each_intel_connector(dev, connector) {
+		if (&connector->new_encoder->base == connector->base.encoder)
+			continue;
+
+		config->mode_changed = true;
+		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] encoder changed, full mode switch\n",
+			      connector->base.base.id,
+			      connector->base.name);
+	}
+
+	for_each_intel_encoder(dev, encoder) {
+		if (&encoder->new_crtc->base == encoder->base.crtc)
+			continue;
+
+		DRM_DEBUG_KMS("[ENCODER:%d:%s] crtc changed, full mode switch\n",
+			      encoder->base.base.id,
+			      encoder->base.name);
+		config->mode_changed = true;
+	}
+
+	for_each_intel_crtc(dev, crtc) {
+		if (crtc->new_enabled == crtc->base.state->enable)
+			continue;
+
+		DRM_DEBUG_KMS("[CRTC:%d] %sabled, full mode switch\n",
+			      crtc->base.base.id,
+			      crtc->new_enabled ? "en" : "dis");
+		config->mode_changed = true;
+	}
+
 	DRM_DEBUG_KMS("computed changes for [CRTC:%d], mode_changed=%d, fb_changed=%d\n",
 			set->crtc->base.id, config->mode_changed, config->fb_changed);
 }
@@ -12008,7 +12042,6 @@ 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 drm_atomic_state *state)
 {
 	struct intel_connector *connector;
@@ -12044,14 +12077,6 @@ intel_modeset_stage_output_state(struct drm_device *dev,
 				connector->base.base.id,
 				connector->base.name);
 		}
-
-
-		if (&connector->new_encoder->base != connector->base.encoder) {
-			DRM_DEBUG_KMS("[CONNECTOR:%d:%s] encoder changed, full mode switch\n",
-				      connector->base.base.id,
-				      connector->base.name);
-			config->mode_changed = true;
-		}
 	}
 	/* connector->new_encoder is now updated for all connectors. */
 
@@ -12104,15 +12129,6 @@ intel_modeset_stage_output_state(struct drm_device *dev,
 			encoder->new_crtc = NULL;
 		else if (num_connectors > 1)
 			return -EINVAL;
-
-		/* 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("[ENCODER:%d:%s] crtc changed, full mode switch\n",
-				      encoder->base.base.id,
-				      encoder->base.name);
-			config->mode_changed = true;
-		}
 	}
 	/* Now we've also updated encoder->new_crtc for all encoders. */
 	for_each_intel_connector(dev, connector) {
@@ -12138,13 +12154,6 @@ intel_modeset_stage_output_state(struct drm_device *dev,
 				break;
 			}
 		}
-
-		if (crtc->new_enabled != crtc->base.state->enable) {
-			DRM_DEBUG_KMS("[CRTC:%d] %sabled, full mode switch\n",
-				      crtc->base.base.id,
-				      crtc->new_enabled ? "en" : "dis");
-			config->mode_changed = true;
-		}
 	}
 
 	return 0;
@@ -12216,12 +12225,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 	save_set.y = set->crtc->y;
 	save_set.fb = set->crtc->primary->fb;
 
-	/* Compute whether we need a full modeset, only an fb base update or no
-	 * change at all. In the future we might also check whether only the
-	 * mode changed, e.g. for LVDS where we only change the panel fitter in
-	 * such cases. */
-	intel_set_config_compute_mode_changes(set, config);
-
 	state = drm_atomic_state_alloc(dev);
 	if (!state) {
 		ret = -ENOMEM;
@@ -12230,10 +12233,16 @@ 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, state);
+	ret = intel_modeset_stage_output_state(dev, set, state);
 	if (ret)
 		goto fail;
 
+	/* Compute whether we need a full modeset, only an fb base update or no
+	 * change at all. In the future we might also check whether only the
+	 * mode changed, e.g. for LVDS where we only change the panel fitter in
+	 * such cases. */
+	intel_set_config_compute_mode_changes(set, config);
+
 	pipe_config = intel_modeset_compute_config(set->crtc, set->mode,
 						   set->fb, state,
 						   &modeset_pipes,
-- 
2.1.0

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

Re: [PATCH 05/11] drm/i915: Extract mode_changed computation out of stage_output_config()

[Daniel Vetter]

On Fri, Apr 10, 2015 at 11:38:34AM +0300, Ander Conselvan de Oliveira wrote:
> This should make the conversion to atomic easier, by splitting the
> initialization of the atomic state from the logic that decides if a
> modeset is needed.
> ---
>  drivers/gpu/drm/i915/intel_display.c | 73 ++++++++++++++++++++----------------
>  1 file changed, 41 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3035a3d..72ceb6d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11958,6 +11958,10 @@ static void
>  intel_set_config_compute_mode_changes(struct drm_mode_set *set,
>  				      struct intel_set_config *config)
>  {
> +	struct drm_device *dev = set->crtc->dev;
> +	struct intel_connector *connector;
> +	struct intel_encoder *encoder;
> +	struct intel_crtc *crtc;
>  
>  	/* We should be able to check here if the fb has the same properties
>  	 * and then just flip_or_move it */
> @@ -12001,6 +12005,36 @@ intel_set_config_compute_mode_changes(struct drm_mode_set *set,
>  		config->mode_changed = true;
>  	}
>  
> +	for_each_intel_connector(dev, connector) {
> +		if (&connector->new_encoder->base == connector->base.encoder)
> +			continue;
> +
> +		config->mode_changed = true;
> +		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] encoder changed, full mode switch\n",
> +			      connector->base.base.id,
> +			      connector->base.name);
> +	}
> +
> +	for_each_intel_encoder(dev, encoder) {
> +		if (&encoder->new_crtc->base == encoder->base.crtc)
> +			continue;
> +
> +		DRM_DEBUG_KMS("[ENCODER:%d:%s] crtc changed, full mode switch\n",
> +			      encoder->base.base.id,
> +			      encoder->base.name);
> +		config->mode_changed = true;
> +	}
> +
> +	for_each_intel_crtc(dev, crtc) {
> +		if (crtc->new_enabled == crtc->base.state->enable)
> +			continue;
> +
> +		DRM_DEBUG_KMS("[CRTC:%d] %sabled, full mode switch\n",
> +			      crtc->base.base.id,
> +			      crtc->new_enabled ? "en" : "dis");
> +		config->mode_changed = true;
> +	}
> +
>  	DRM_DEBUG_KMS("computed changes for [CRTC:%d], mode_changed=%d, fb_changed=%d\n",
>  			set->crtc->base.id, config->mode_changed, config->fb_changed);
>  }

Any chance we could reuse some parts of the logic in drm_atomic_helper.c?
Copypasting that to have something which uses state objects instead of our
new_ pointers feels lame.

> @@ -12008,7 +12042,6 @@ 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 drm_atomic_state *state)
>  {
>  	struct intel_connector *connector;
> @@ -12044,14 +12077,6 @@ intel_modeset_stage_output_state(struct drm_device *dev,
>  				connector->base.base.id,
>  				connector->base.name);
>  		}
> -
> -
> -		if (&connector->new_encoder->base != connector->base.encoder) {
> -			DRM_DEBUG_KMS("[CONNECTOR:%d:%s] encoder changed, full mode switch\n",
> -				      connector->base.base.id,
> -				      connector->base.name);
> -			config->mode_changed = true;
> -		}
>  	}
>  	/* connector->new_encoder is now updated for all connectors. */
>  
> @@ -12104,15 +12129,6 @@ intel_modeset_stage_output_state(struct drm_device *dev,
>  			encoder->new_crtc = NULL;
>  		else if (num_connectors > 1)
>  			return -EINVAL;
> -
> -		/* 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("[ENCODER:%d:%s] crtc changed, full mode switch\n",
> -				      encoder->base.base.id,
> -				      encoder->base.name);
> -			config->mode_changed = true;
> -		}
>  	}
>  	/* Now we've also updated encoder->new_crtc for all encoders. */
>  	for_each_intel_connector(dev, connector) {
> @@ -12138,13 +12154,6 @@ intel_modeset_stage_output_state(struct drm_device *dev,
>  				break;
>  			}
>  		}
> -
> -		if (crtc->new_enabled != crtc->base.state->enable) {
> -			DRM_DEBUG_KMS("[CRTC:%d] %sabled, full mode switch\n",
> -				      crtc->base.base.id,
> -				      crtc->new_enabled ? "en" : "dis");
> -			config->mode_changed = true;
> -		}
>  	}
>  
>  	return 0;
> @@ -12216,12 +12225,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  	save_set.y = set->crtc->y;
>  	save_set.fb = set->crtc->primary->fb;
>  
> -	/* Compute whether we need a full modeset, only an fb base update or no
> -	 * change at all. In the future we might also check whether only the
> -	 * mode changed, e.g. for LVDS where we only change the panel fitter in
> -	 * such cases. */
> -	intel_set_config_compute_mode_changes(set, config);
> -
>  	state = drm_atomic_state_alloc(dev);
>  	if (!state) {
>  		ret = -ENOMEM;
> @@ -12230,10 +12233,16 @@ 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, state);
> +	ret = intel_modeset_stage_output_state(dev, set, state);
>  	if (ret)
>  		goto fail;
>  
> +	/* Compute whether we need a full modeset, only an fb base update or no
> +	 * change at all. In the future we might also check whether only the
> +	 * mode changed, e.g. for LVDS where we only change the panel fitter in
> +	 * such cases. */
> +	intel_set_config_compute_mode_changes(set, config);

Imo rename this to intel_compute_mode_changes or similar, the set_config
is because of the ->set_config hook.
-Daniel

> +
>  	pipe_config = intel_modeset_compute_config(set->crtc, set->mode,
>  						   set->fb, state,
>  						   &modeset_pipes,
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 06/11] drm/i915: Add crtc states before calling compute_config()

[Ander Conselvan de Oliveira]

The function intel_modeset_compute_config() needs to eventually become
part of atomic_check(). At that point, all the affected crtcs need to be
in the atomic state with the new values. So move the logic of adding
crtc states out of that function.
---
 drivers/gpu/drm/i915/intel_display.c | 48 ++++++++++++++++++++++++++++--------
 1 file changed, 38 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 72ceb6d..232ef56 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9111,6 +9111,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
 	struct drm_mode_config *config = &dev->mode_config;
 	struct drm_atomic_state *state = NULL;
 	struct drm_connector_state *connector_state;
+	struct intel_crtc_state *crtc_state;
 	int ret, i = -1;
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
@@ -9206,6 +9207,14 @@ retry:
 	connector_state->crtc = crtc;
 	connector_state->best_encoder = &intel_encoder->base;
 
+	crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
+	if (IS_ERR(crtc_state)) {
+		ret = PTR_ERR(crtc_state);
+		goto fail;
+	}
+
+	crtc_state->base.enable = true;
+
 	if (!mode)
 		mode = &load_detect_mode;
 
@@ -9268,6 +9277,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_atomic_state *state;
 	struct drm_connector_state *connector_state;
+	struct intel_crtc_state *crtc_state;
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
 		      connector->base.id, connector->name,
@@ -9284,6 +9294,10 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
 		if (IS_ERR(connector_state))
 			goto fail;
 
+		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
+		if (IS_ERR(crtc_state))
+			goto fail;
+
 		to_intel_connector(connector)->new_encoder = NULL;
 		intel_encoder->new_crtc = NULL;
 		intel_crtc->new_enabled = false;
@@ -11554,14 +11568,6 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
 	intel_modeset_affected_pipes(crtc, modeset_pipes,
 				     prepare_pipes, disable_pipes);
 
-	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
 	 * and crtcs.  At that point we'll need to compute the whole config
@@ -11578,8 +11584,6 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
 		if (IS_ERR(pipe_config))
 			return pipe_config;
 
-		pipe_config->base.enable = true;
-
 		intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,
 				       "[modeset]");
 	}
@@ -11801,9 +11805,11 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_atomic_state *state;
+	struct intel_crtc *intel_crtc;
 	struct intel_encoder *encoder;
 	struct intel_connector *connector;
 	struct drm_connector_state *connector_state;
+	struct intel_crtc_state *crtc_state;
 
 	state = drm_atomic_state_alloc(dev);
 	if (!state) {
@@ -11841,6 +11847,19 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
 		}
 	}
 
+	for_each_intel_crtc(dev, intel_crtc) {
+		if (intel_crtc->new_enabled == intel_crtc->base.enabled)
+			continue;
+
+		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
+		if (IS_ERR(crtc_state)) {
+			DRM_DEBUG_KMS("Failed to add [CRTC:%d] to state: %ld\n",
+				      intel_crtc->base.base.id,
+				      PTR_ERR(crtc_state));
+			continue;
+		}
+	}
+
 	intel_set_mode(crtc, &crtc->mode, crtc->x, crtc->y, crtc->primary->fb,
 		       state);
 
@@ -12048,6 +12067,7 @@ intel_modeset_stage_output_state(struct drm_device *dev,
 	struct drm_connector_state *connector_state;
 	struct intel_encoder *encoder;
 	struct intel_crtc *crtc;
+	struct intel_crtc_state *crtc_state;
 	int ro;
 
 	/* The upper layers ensure that we either disable a crtc or have a list
@@ -12154,6 +12174,14 @@ intel_modeset_stage_output_state(struct drm_device *dev,
 				break;
 			}
 		}
+
+		if (crtc->new_enabled != crtc->base.state->enable) {
+			crtc_state = intel_atomic_get_crtc_state(state, crtc);
+			if (IS_ERR(crtc_state))
+				return PTR_ERR(crtc_state);
+
+			crtc_state->base.enable = crtc->new_enabled;
+		}
 	}
 
 	return 0;
-- 
2.1.0

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

[PATCH 07/11] drm/i915: Remove all *_pipes flags from modeset

[Ander Conselvan de Oliveira]

Set the mode_changed field on the crtc_states and use that instead.

Note that even though this patch doesn't completely replace the logic in
intel_modeset_affected_pipes(), that logic was never fully used to its
full extent. Since the commit mentioned below, modeset_pipes and
prepare_pipes would only contain at most the pipe for which the set_crtc
ioctl was called. We can grow back that logic when the time comes.

commit b6c5164d7bf624f3e1b750787ddb983150c5117c
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Fri Apr 12 18:48:43 2013 +0200

    drm/i915: Fixup Oops in the pipe config computation
---
 drivers/gpu/drm/i915/i915_drv.h      |   8 +
 drivers/gpu/drm/i915/intel_display.c | 287 ++++++++++++++---------------------
 2 files changed, 126 insertions(+), 169 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e12bc5a..08ea6d0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -272,6 +272,14 @@ enum hpd_pin {
 	     (__i)++)							\
 		if (connector)
 
+#define for_each_intel_crtc_in_state(state, crtc, crtc_state, __i)	\
+	for ((__i) = 0;							\
+	     (crtc) = to_intel_crtc((state)->crtcs[__i]),		\
+	     (crtc_state) = to_intel_crtc_state((state)->crtc_states[__i]), \
+	     (__i) < (state)->dev->mode_config.num_crtc;		\
+	     (__i)++)							\
+		if (crtc_state)
+
 struct drm_i915_private;
 struct i915_mm_struct;
 struct i915_mmu_object;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 232ef56..a255b24 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5179,12 +5179,13 @@ static int intel_mode_max_pixclk(struct drm_atomic_state *state)
 	return max_pixclk;
 }
 
-static int valleyview_modeset_global_pipes(struct drm_atomic_state *state,
-					    unsigned *prepare_pipes)
+static int valleyview_modeset_global_pipes(struct drm_atomic_state *state)
 {
 	struct drm_i915_private *dev_priv = to_i915(state->dev);
 	struct intel_crtc *intel_crtc;
+	struct intel_crtc_state *crtc_state;
 	int max_pixclk = intel_mode_max_pixclk(state);
+	int i;
 
 	if (max_pixclk < 0)
 		return max_pixclk;
@@ -5193,10 +5194,20 @@ static int valleyview_modeset_global_pipes(struct drm_atomic_state *state,
 	    dev_priv->vlv_cdclk_freq)
 		return 0;
 
+	/* add all active pipes to the state */
+	for_each_intel_crtc(state->dev, intel_crtc) {
+		if (!intel_crtc->base.state->enable)
+			continue;
+
+		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
+		if (IS_ERR(crtc_state))
+			return PTR_ERR(crtc_state);
+	}
+
 	/* disable/enable all currently active pipes while we change cdclk */
-	for_each_intel_crtc(state->dev, intel_crtc)
-		if (intel_crtc->base.state->enable)
-			*prepare_pipes |= (1 << intel_crtc->pipe);
+	for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i)
+		if (crtc_state->base.enable)
+			crtc_state->base.mode_changed = true;
 
 	return 0;
 }
@@ -10879,94 +10890,6 @@ fail:
 	return ERR_PTR(ret);
 }
 
-/* Computes which crtcs are affected and sets the relevant bits in the mask. For
- * simplicity we use the crtc's pipe number (because it's easier to obtain). */
-static void
-intel_modeset_affected_pipes(struct drm_crtc *crtc, unsigned *modeset_pipes,
-			     unsigned *prepare_pipes, unsigned *disable_pipes)
-{
-	struct intel_crtc *intel_crtc;
-	struct drm_device *dev = crtc->dev;
-	struct intel_encoder *encoder;
-	struct intel_connector *connector;
-	struct drm_crtc *tmp_crtc;
-
-	*disable_pipes = *modeset_pipes = *prepare_pipes = 0;
-
-	/* Check which crtcs have changed outputs connected to them, these need
-	 * 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. */
-	for_each_intel_connector(dev, connector) {
-		if (connector->base.encoder == &connector->new_encoder->base)
-			continue;
-
-		if (connector->base.encoder) {
-			tmp_crtc = connector->base.encoder->crtc;
-
-			*prepare_pipes |= 1 << to_intel_crtc(tmp_crtc)->pipe;
-		}
-
-		if (connector->new_encoder)
-			*prepare_pipes |=
-				1 << connector->new_encoder->new_crtc->pipe;
-	}
-
-	for_each_intel_encoder(dev, encoder) {
-		if (encoder->base.crtc == &encoder->new_crtc->base)
-			continue;
-
-		if (encoder->base.crtc) {
-			tmp_crtc = encoder->base.crtc;
-
-			*prepare_pipes |= 1 << to_intel_crtc(tmp_crtc)->pipe;
-		}
-
-		if (encoder->new_crtc)
-			*prepare_pipes |= 1 << encoder->new_crtc->pipe;
-	}
-
-	/* Check for pipes that will be enabled/disabled ... */
-	for_each_intel_crtc(dev, intel_crtc) {
-		if (intel_crtc->base.state->enable == intel_crtc->new_enabled)
-			continue;
-
-		if (!intel_crtc->new_enabled)
-			*disable_pipes |= 1 << intel_crtc->pipe;
-		else
-			*prepare_pipes |= 1 << intel_crtc->pipe;
-	}
-
-
-	/* set_mode is also used to update properties on life display pipes. */
-	intel_crtc = to_intel_crtc(crtc);
-	if (intel_crtc->new_enabled)
-		*prepare_pipes |= 1 << intel_crtc->pipe;
-
-	/*
-	 * For simplicity do a full modeset on any pipe where the output routing
-	 * changed. We could be more clever, but that would require us to be
-	 * more careful with calling the relevant encoder->mode_set functions.
-	 */
-	if (*prepare_pipes)
-		*modeset_pipes = *prepare_pipes;
-
-	/* ... and mask these out. */
-	*modeset_pipes &= ~(*disable_pipes);
-	*prepare_pipes &= ~(*disable_pipes);
-
-	/*
-	 * HACK: We don't (yet) fully support global modesets. intel_set_config
-	 * obies this rule, but the modeset restore mode of
-	 * intel_modeset_setup_hw_state does not.
-	 */
-	*modeset_pipes &= 1 << intel_crtc->pipe;
-	*prepare_pipes &= 1 << intel_crtc->pipe;
-
-	DRM_DEBUG_KMS("set mode pipe masks: modeset: %x, prepare: %x, disable: %x\n",
-		      *modeset_pipes, *prepare_pipes, *disable_pipes);
-}
-
 static bool intel_crtc_in_use(struct drm_crtc *crtc)
 {
 	struct drm_encoder *encoder;
@@ -10979,13 +10902,22 @@ static bool intel_crtc_in_use(struct drm_crtc *crtc)
 	return false;
 }
 
+static bool
+needs_modeset(struct intel_crtc_state *state)
+{
+	return state->base.mode_changed || state->base.active_changed;
+}
+
 static void
-intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
+intel_modeset_update_state(struct drm_atomic_state *state)
 {
+	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_encoder *intel_encoder;
 	struct intel_crtc *intel_crtc;
+	struct intel_crtc_state *crtc_state;
 	struct drm_connector *connector;
+	int i;
 
 	intel_shared_dpll_commit(dev_priv);
 
@@ -10993,9 +10925,14 @@ intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
 		if (!intel_encoder->base.crtc)
 			continue;
 
-		intel_crtc = to_intel_crtc(intel_encoder->base.crtc);
+		for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i)
+			if (&intel_crtc->base == intel_encoder->base.crtc)
+				break;
+
+		if (&intel_crtc->base != intel_encoder->base.crtc)
+			continue;
 
-		if (prepare_pipes & (1 << intel_crtc->pipe))
+		if (crtc_state->base.enable && needs_modeset(crtc_state))
 			intel_encoder->connectors_active = false;
 	}
 
@@ -11010,9 +10947,14 @@ intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
 		if (!connector->encoder || !connector->encoder->crtc)
 			continue;
 
-		intel_crtc = to_intel_crtc(connector->encoder->crtc);
+		for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i)
+			if (&intel_crtc->base == connector->encoder->crtc)
+				break;
+
+		if (&intel_crtc->base != connector->encoder->crtc)
+			continue;
 
-		if (prepare_pipes & (1 << intel_crtc->pipe)) {
+		if (crtc_state->base.enable && needs_modeset(crtc_state)) {
 			struct drm_property *dpms_property =
 				dev->mode_config.dpms_property;
 
@@ -11547,26 +11489,41 @@ static void update_scanline_offset(struct intel_crtc *crtc)
 		crtc->scanline_offset = 1;
 }
 
+static void
+intel_atomic_modeset_compute_changed_flags(struct drm_atomic_state *state,
+					   struct drm_crtc *modeset_crtc)
+{
+	struct intel_crtc_state *crtc_state;
+	struct intel_crtc *crtc;
+	int i;
+
+	for_each_intel_crtc_in_state(state, crtc, crtc_state, i) {
+		if (crtc_state->base.enable != crtc->base.state->enable)
+			crtc_state->base.mode_changed = true;
+
+		/* FIXME: move comment about changing properties in live pipes
+		 * from affectec_pipes() here */
+		if (&crtc->base == modeset_crtc && crtc_state->base.enable)
+			crtc_state->base.mode_changed = true;
+	}
+}
+
 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 drm_atomic_state *state)
 {
-	struct drm_device *dev = crtc->dev;
-	struct intel_crtc_state *pipe_config = NULL;
+	struct intel_crtc_state *crtc_state;
 	struct intel_crtc *intel_crtc;
 	int ret = 0;
+	int i;
 
 	ret = drm_atomic_add_affected_connectors(state, crtc);
 	if (ret)
 		return ERR_PTR(ret);
 
-	intel_modeset_affected_pipes(crtc, modeset_pipes,
-				     prepare_pipes, disable_pipes);
+	intel_atomic_modeset_compute_changed_flags(state, crtc);
 
 	/*
 	 * Note this needs changes when we start tracking multiple modes
@@ -11574,46 +11531,49 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
 	 * (i.e. one pipe_config for each crtc) rather than just the one
 	 * for this crtc.
 	 */
-	for_each_intel_crtc_masked(dev, *modeset_pipes, intel_crtc) {
-		/* FIXME: For now we still expect modeset_pipes has at most
-		 * one bit set. */
+	for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i) {
+		if (!crtc_state->base.enable || !needs_modeset(crtc_state))
+			continue;
+
+		/* FIXME: For now we still expect modeset on only one pipe. */
 		if (WARN_ON(&intel_crtc->base != crtc))
 			continue;
 
-		pipe_config = intel_modeset_pipe_config(crtc, fb, mode, state);
-		if (IS_ERR(pipe_config))
-			return pipe_config;
+		crtc_state = intel_modeset_pipe_config(crtc, fb, mode, state);
+		if (IS_ERR(crtc_state))
+			return crtc_state;
 
-		intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,
+		intel_dump_pipe_config(to_intel_crtc(crtc), crtc_state,
 				       "[modeset]");
 	}
 
 	return intel_atomic_get_crtc_state(state, to_intel_crtc(crtc));;
 }
 
-static int __intel_set_mode_setup_plls(struct drm_atomic_state *state,
-				       unsigned modeset_pipes,
-				       unsigned disable_pipes)
+static int __intel_set_mode_setup_plls(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	unsigned clear_pipes = modeset_pipes | disable_pipes;
+	unsigned clear_pipes = 0;
 	struct intel_crtc *intel_crtc;
+	struct intel_crtc_state *crtc_state;
 	int ret = 0;
+	int i;
 
 	if (!dev_priv->display.crtc_compute_clock)
 		return 0;
 
+	for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i) {
+		if (needs_modeset(crtc_state))
+			clear_pipes |= 1 << intel_crtc->pipe;
+	}
+
 	ret = intel_shared_dpll_start_config(dev_priv, clear_pipes);
 	if (ret)
 		goto done;
 
-	for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
-		struct intel_crtc_state *crtc_state =
-			intel_atomic_get_crtc_state(state, intel_crtc);
-
-		/* Modeset pipes should have a new state by now */
-		if (WARN_ON(IS_ERR(crtc_state)))
+	for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i) {
+		if (!crtc_state->base.enable)
 			continue;
 
 		ret = dev_priv->display.crtc_compute_clock(intel_crtc,
@@ -11631,10 +11591,7 @@ done:
 static int __intel_set_mode(struct drm_crtc *crtc,
 			    struct drm_display_mode *mode,
 			    int x, int y, struct drm_framebuffer *fb,
-			    struct intel_crtc_state *pipe_config,
-			    unsigned modeset_pipes,
-			    unsigned prepare_pipes,
-			    unsigned disable_pipes)
+			    struct intel_crtc_state *pipe_config)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -11642,7 +11599,9 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 	struct drm_atomic_state *state = pipe_config->base.state;
 	struct intel_crtc_state *crtc_state_copy = NULL;
 	struct intel_crtc *intel_crtc;
+	struct intel_crtc_state *crtc_state;
 	int ret = 0;
+	int i;
 
 	saved_mode = kmalloc(sizeof(*saved_mode), GFP_KERNEL);
 	if (!saved_mode)
@@ -11664,23 +11623,22 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 	 * adjusted_mode bits in the crtc directly.
 	 */
 	if (IS_VALLEYVIEW(dev)) {
-		ret = valleyview_modeset_global_pipes(state, &prepare_pipes);
+		ret = valleyview_modeset_global_pipes(state);
 		if (ret)
 			goto done;
-
-		/* may have added more to prepare_pipes than we should */
-		prepare_pipes &= ~disable_pipes;
 	}
 
-	ret = __intel_set_mode_setup_plls(state, modeset_pipes, disable_pipes);
+	ret = __intel_set_mode_setup_plls(state);
 	if (ret)
 		goto done;
 
-	for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc)
-		intel_crtc_disable(&intel_crtc->base);
+	for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i) {
+		if (!needs_modeset(crtc_state))
+			continue;
 
-	for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
-		if (intel_crtc->base.state->enable)
+		if (!crtc_state->base.enable)
+			intel_crtc_disable(&intel_crtc->base);
+		else if (intel_crtc->base.state->enable)
 			dev_priv->display.crtc_disable(&intel_crtc->base);
 	}
 
@@ -11691,7 +11649,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 	 * pipes; here we assume a single modeset_pipe and only track the
 	 * single crtc and mode.
 	 */
-	if (modeset_pipes) {
+	if (pipe_config->base.enable && needs_modeset(pipe_config)) {
 		crtc->mode = *mode;
 		/* mode_set/enable/disable functions rely on a correct pipe
 		 * config. */
@@ -11708,16 +11666,20 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 
 	/* Only after disabling all output pipelines that will be changed can we
 	 * update the the output configuration. */
-	intel_modeset_update_state(dev, prepare_pipes);
+	intel_modeset_update_state(state);
 
 	modeset_update_crtc_power_domains(state);
 
-	for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
-		struct drm_plane *primary = intel_crtc->base.primary;
+	if (pipe_config->base.enable && needs_modeset(pipe_config)) {
+		struct drm_plane *primary;
 		int vdisplay, hdisplay;
 
+		intel_crtc = to_intel_crtc(crtc);
+		primary = intel_crtc->base.primary;
+
 		drm_crtc_get_hv_timing(mode, &hdisplay, &vdisplay);
-		ret = primary->funcs->update_plane(primary, &intel_crtc->base,
+
+		ret = primary->funcs->update_plane(primary, crtc,
 						   fb, 0, 0,
 						   hdisplay, vdisplay,
 						   x << 16, y << 16,
@@ -11725,7 +11687,10 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 	}
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
-	for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
+	for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i) {
+		if (!crtc_state->base.enable)
+			continue;
+
 		update_scanline_offset(intel_crtc);
 
 		dev_priv->display.crtc_enable(&intel_crtc->base);
@@ -11753,18 +11718,14 @@ done:
 	return ret;
 }
 
-static int intel_set_mode_pipes(struct drm_crtc *crtc,
-				struct drm_display_mode *mode,
-				int x, int y, struct drm_framebuffer *fb,
-				struct intel_crtc_state *pipe_config,
-				unsigned modeset_pipes,
-				unsigned prepare_pipes,
-				unsigned disable_pipes)
+static int intel_set_mode_with_config(struct drm_crtc *crtc,
+				      struct drm_display_mode *mode,
+				      int x, int y, struct drm_framebuffer *fb,
+				      struct intel_crtc_state *pipe_config)
 {
 	int ret;
 
-	ret = __intel_set_mode(crtc, mode, x, y, fb, pipe_config, modeset_pipes,
-			       prepare_pipes, disable_pipes);
+	ret = __intel_set_mode(crtc, mode, x, y, fb, pipe_config);
 
 	if (ret == 0)
 		intel_modeset_check_state(crtc->dev);
@@ -11778,22 +11739,15 @@ static int intel_set_mode(struct drm_crtc *crtc,
 			  struct drm_atomic_state *state)
 {
 	struct intel_crtc_state *pipe_config;
-	unsigned modeset_pipes, prepare_pipes, disable_pipes;
 	int ret = 0;
 
-	pipe_config = intel_modeset_compute_config(crtc, mode, fb, state,
-						   &modeset_pipes,
-						   &prepare_pipes,
-						   &disable_pipes);
-
+	pipe_config = intel_modeset_compute_config(crtc, mode, fb, state);
 	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);
+	ret = intel_set_mode_with_config(crtc, mode, x, y, fb, pipe_config);
 	if (ret)
 		goto out;
 
@@ -12217,7 +12171,6 @@ static int intel_crtc_set_config(struct drm_mode_set *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;
 	int ret;
 
 	BUG_ON(!set);
@@ -12272,10 +12225,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 	intel_set_config_compute_mode_changes(set, config);
 
 	pipe_config = intel_modeset_compute_config(set->crtc, set->mode,
-						   set->fb, state,
-						   &modeset_pipes,
-						   &prepare_pipes,
-						   &disable_pipes);
+						   set->fb, state);
 	if (IS_ERR(pipe_config)) {
 		ret = PTR_ERR(pipe_config);
 		goto fail;
@@ -12295,10 +12245,9 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 	intel_update_pipe_size(to_intel_crtc(set->crtc));
 
 	if (config->mode_changed) {
-		ret = intel_set_mode_pipes(set->crtc, set->mode,
-					   set->x, set->y, set->fb, pipe_config,
-					   modeset_pipes, prepare_pipes,
-					   disable_pipes);
+		ret = intel_set_mode_with_config(set->crtc, set->mode,
+						 set->x, set->y, set->fb,
+						 pipe_config);
 	} else if (config->fb_changed) {
 		struct intel_crtc *intel_crtc = to_intel_crtc(set->crtc);
 		struct drm_plane *primary = set->crtc->primary;
-- 
2.1.0

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

Re: [PATCH 07/11] drm/i915: Remove all *_pipes flags from modeset

[Daniel Vetter]

On Fri, Apr 10, 2015 at 11:38:36AM +0300, Ander Conselvan de Oliveira wrote:
> Set the mode_changed field on the crtc_states and use that instead.
> 
> Note that even though this patch doesn't completely replace the logic in
> intel_modeset_affected_pipes(), that logic was never fully used to its
> full extent. Since the commit mentioned below, modeset_pipes and
> prepare_pipes would only contain at most the pipe for which the set_crtc
> ioctl was called. We can grow back that logic when the time comes.
> 
> commit b6c5164d7bf624f3e1b750787ddb983150c5117c
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Fri Apr 12 18:48:43 2013 +0200
> 
>     drm/i915: Fixup Oops in the pipe config computation

Bikeshed: Instead of switching to pipe_config I'd just switch to the
overall state for these functions. Or is there something preventing this?
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h      |   8 +
>  drivers/gpu/drm/i915/intel_display.c | 287 ++++++++++++++---------------------
>  2 files changed, 126 insertions(+), 169 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e12bc5a..08ea6d0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -272,6 +272,14 @@ enum hpd_pin {
>  	     (__i)++)							\
>  		if (connector)
>  
> +#define for_each_intel_crtc_in_state(state, crtc, crtc_state, __i)	\
> +	for ((__i) = 0;							\
> +	     (crtc) = to_intel_crtc((state)->crtcs[__i]),		\
> +	     (crtc_state) = to_intel_crtc_state((state)->crtc_states[__i]), \
> +	     (__i) < (state)->dev->mode_config.num_crtc;		\
> +	     (__i)++)							\
> +		if (crtc_state)
> +
>  struct drm_i915_private;
>  struct i915_mm_struct;
>  struct i915_mmu_object;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 232ef56..a255b24 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5179,12 +5179,13 @@ static int intel_mode_max_pixclk(struct drm_atomic_state *state)
>  	return max_pixclk;
>  }
>  
> -static int valleyview_modeset_global_pipes(struct drm_atomic_state *state,
> -					    unsigned *prepare_pipes)
> +static int valleyview_modeset_global_pipes(struct drm_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
>  	struct intel_crtc *intel_crtc;
> +	struct intel_crtc_state *crtc_state;
>  	int max_pixclk = intel_mode_max_pixclk(state);
> +	int i;
>  
>  	if (max_pixclk < 0)
>  		return max_pixclk;
> @@ -5193,10 +5194,20 @@ static int valleyview_modeset_global_pipes(struct drm_atomic_state *state,
>  	    dev_priv->vlv_cdclk_freq)
>  		return 0;
>  
> +	/* add all active pipes to the state */
> +	for_each_intel_crtc(state->dev, intel_crtc) {
> +		if (!intel_crtc->base.state->enable)
> +			continue;
> +
> +		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
> +		if (IS_ERR(crtc_state))
> +			return PTR_ERR(crtc_state);
> +	}
> +
>  	/* disable/enable all currently active pipes while we change cdclk */
> -	for_each_intel_crtc(state->dev, intel_crtc)
> -		if (intel_crtc->base.state->enable)
> -			*prepare_pipes |= (1 << intel_crtc->pipe);
> +	for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i)
> +		if (crtc_state->base.enable)
> +			crtc_state->base.mode_changed = true;
>  
>  	return 0;
>  }
> @@ -10879,94 +10890,6 @@ fail:
>  	return ERR_PTR(ret);
>  }
>  
> -/* Computes which crtcs are affected and sets the relevant bits in the mask. For
> - * simplicity we use the crtc's pipe number (because it's easier to obtain). */
> -static void
> -intel_modeset_affected_pipes(struct drm_crtc *crtc, unsigned *modeset_pipes,
> -			     unsigned *prepare_pipes, unsigned *disable_pipes)
> -{
> -	struct intel_crtc *intel_crtc;
> -	struct drm_device *dev = crtc->dev;
> -	struct intel_encoder *encoder;
> -	struct intel_connector *connector;
> -	struct drm_crtc *tmp_crtc;
> -
> -	*disable_pipes = *modeset_pipes = *prepare_pipes = 0;
> -
> -	/* Check which crtcs have changed outputs connected to them, these need
> -	 * 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. */
> -	for_each_intel_connector(dev, connector) {
> -		if (connector->base.encoder == &connector->new_encoder->base)
> -			continue;
> -
> -		if (connector->base.encoder) {
> -			tmp_crtc = connector->base.encoder->crtc;
> -
> -			*prepare_pipes |= 1 << to_intel_crtc(tmp_crtc)->pipe;
> -		}
> -
> -		if (connector->new_encoder)
> -			*prepare_pipes |=
> -				1 << connector->new_encoder->new_crtc->pipe;
> -	}
> -
> -	for_each_intel_encoder(dev, encoder) {
> -		if (encoder->base.crtc == &encoder->new_crtc->base)
> -			continue;
> -
> -		if (encoder->base.crtc) {
> -			tmp_crtc = encoder->base.crtc;
> -
> -			*prepare_pipes |= 1 << to_intel_crtc(tmp_crtc)->pipe;
> -		}
> -
> -		if (encoder->new_crtc)
> -			*prepare_pipes |= 1 << encoder->new_crtc->pipe;
> -	}
> -
> -	/* Check for pipes that will be enabled/disabled ... */
> -	for_each_intel_crtc(dev, intel_crtc) {
> -		if (intel_crtc->base.state->enable == intel_crtc->new_enabled)
> -			continue;
> -
> -		if (!intel_crtc->new_enabled)
> -			*disable_pipes |= 1 << intel_crtc->pipe;
> -		else
> -			*prepare_pipes |= 1 << intel_crtc->pipe;
> -	}
> -
> -
> -	/* set_mode is also used to update properties on life display pipes. */
> -	intel_crtc = to_intel_crtc(crtc);
> -	if (intel_crtc->new_enabled)
> -		*prepare_pipes |= 1 << intel_crtc->pipe;
> -
> -	/*
> -	 * For simplicity do a full modeset on any pipe where the output routing
> -	 * changed. We could be more clever, but that would require us to be
> -	 * more careful with calling the relevant encoder->mode_set functions.
> -	 */
> -	if (*prepare_pipes)
> -		*modeset_pipes = *prepare_pipes;
> -
> -	/* ... and mask these out. */
> -	*modeset_pipes &= ~(*disable_pipes);
> -	*prepare_pipes &= ~(*disable_pipes);
> -
> -	/*
> -	 * HACK: We don't (yet) fully support global modesets. intel_set_config
> -	 * obies this rule, but the modeset restore mode of
> -	 * intel_modeset_setup_hw_state does not.
> -	 */
> -	*modeset_pipes &= 1 << intel_crtc->pipe;
> -	*prepare_pipes &= 1 << intel_crtc->pipe;
> -
> -	DRM_DEBUG_KMS("set mode pipe masks: modeset: %x, prepare: %x, disable: %x\n",
> -		      *modeset_pipes, *prepare_pipes, *disable_pipes);
> -}
> -
>  static bool intel_crtc_in_use(struct drm_crtc *crtc)
>  {
>  	struct drm_encoder *encoder;
> @@ -10979,13 +10902,22 @@ static bool intel_crtc_in_use(struct drm_crtc *crtc)
>  	return false;
>  }
>  
> +static bool
> +needs_modeset(struct intel_crtc_state *state)
> +{
> +	return state->base.mode_changed || state->base.active_changed;
> +}
> +
>  static void
> -intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
> +intel_modeset_update_state(struct drm_atomic_state *state)
>  {
> +	struct drm_device *dev = state->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_encoder *intel_encoder;
>  	struct intel_crtc *intel_crtc;
> +	struct intel_crtc_state *crtc_state;
>  	struct drm_connector *connector;
> +	int i;
>  
>  	intel_shared_dpll_commit(dev_priv);
>  
> @@ -10993,9 +10925,14 @@ intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
>  		if (!intel_encoder->base.crtc)
>  			continue;
>  
> -		intel_crtc = to_intel_crtc(intel_encoder->base.crtc);
> +		for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i)
> +			if (&intel_crtc->base == intel_encoder->base.crtc)
> +				break;
> +
> +		if (&intel_crtc->base != intel_encoder->base.crtc)
> +			continue;
>  
> -		if (prepare_pipes & (1 << intel_crtc->pipe))
> +		if (crtc_state->base.enable && needs_modeset(crtc_state))
>  			intel_encoder->connectors_active = false;
>  	}
>  
> @@ -11010,9 +10947,14 @@ intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
>  		if (!connector->encoder || !connector->encoder->crtc)
>  			continue;
>  
> -		intel_crtc = to_intel_crtc(connector->encoder->crtc);
> +		for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i)
> +			if (&intel_crtc->base == connector->encoder->crtc)
> +				break;
> +
> +		if (&intel_crtc->base != connector->encoder->crtc)
> +			continue;
>  
> -		if (prepare_pipes & (1 << intel_crtc->pipe)) {
> +		if (crtc_state->base.enable && needs_modeset(crtc_state)) {
>  			struct drm_property *dpms_property =
>  				dev->mode_config.dpms_property;
>  
> @@ -11547,26 +11489,41 @@ static void update_scanline_offset(struct intel_crtc *crtc)
>  		crtc->scanline_offset = 1;
>  }
>  
> +static void
> +intel_atomic_modeset_compute_changed_flags(struct drm_atomic_state *state,
> +					   struct drm_crtc *modeset_crtc)
> +{
> +	struct intel_crtc_state *crtc_state;
> +	struct intel_crtc *crtc;
> +	int i;
> +
> +	for_each_intel_crtc_in_state(state, crtc, crtc_state, i) {
> +		if (crtc_state->base.enable != crtc->base.state->enable)
> +			crtc_state->base.mode_changed = true;
> +
> +		/* FIXME: move comment about changing properties in live pipes
> +		 * from affectec_pipes() here */
> +		if (&crtc->base == modeset_crtc && crtc_state->base.enable)
> +			crtc_state->base.mode_changed = true;
> +	}
> +}
> +
>  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 drm_atomic_state *state)
>  {
> -	struct drm_device *dev = crtc->dev;
> -	struct intel_crtc_state *pipe_config = NULL;
> +	struct intel_crtc_state *crtc_state;
>  	struct intel_crtc *intel_crtc;
>  	int ret = 0;
> +	int i;
>  
>  	ret = drm_atomic_add_affected_connectors(state, crtc);
>  	if (ret)
>  		return ERR_PTR(ret);
>  
> -	intel_modeset_affected_pipes(crtc, modeset_pipes,
> -				     prepare_pipes, disable_pipes);
> +	intel_atomic_modeset_compute_changed_flags(state, crtc);
>  
>  	/*
>  	 * Note this needs changes when we start tracking multiple modes
> @@ -11574,46 +11531,49 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
>  	 * (i.e. one pipe_config for each crtc) rather than just the one
>  	 * for this crtc.
>  	 */
> -	for_each_intel_crtc_masked(dev, *modeset_pipes, intel_crtc) {
> -		/* FIXME: For now we still expect modeset_pipes has at most
> -		 * one bit set. */
> +	for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i) {
> +		if (!crtc_state->base.enable || !needs_modeset(crtc_state))
> +			continue;
> +
> +		/* FIXME: For now we still expect modeset on only one pipe. */
>  		if (WARN_ON(&intel_crtc->base != crtc))
>  			continue;
>  
> -		pipe_config = intel_modeset_pipe_config(crtc, fb, mode, state);
> -		if (IS_ERR(pipe_config))
> -			return pipe_config;
> +		crtc_state = intel_modeset_pipe_config(crtc, fb, mode, state);
> +		if (IS_ERR(crtc_state))
> +			return crtc_state;
>  
> -		intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,
> +		intel_dump_pipe_config(to_intel_crtc(crtc), crtc_state,
>  				       "[modeset]");
>  	}
>  
>  	return intel_atomic_get_crtc_state(state, to_intel_crtc(crtc));;
>  }
>  
> -static int __intel_set_mode_setup_plls(struct drm_atomic_state *state,
> -				       unsigned modeset_pipes,
> -				       unsigned disable_pipes)
> +static int __intel_set_mode_setup_plls(struct drm_atomic_state *state)
>  {
>  	struct drm_device *dev = state->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	unsigned clear_pipes = modeset_pipes | disable_pipes;
> +	unsigned clear_pipes = 0;
>  	struct intel_crtc *intel_crtc;
> +	struct intel_crtc_state *crtc_state;
>  	int ret = 0;
> +	int i;
>  
>  	if (!dev_priv->display.crtc_compute_clock)
>  		return 0;
>  
> +	for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i) {
> +		if (needs_modeset(crtc_state))
> +			clear_pipes |= 1 << intel_crtc->pipe;
> +	}
> +
>  	ret = intel_shared_dpll_start_config(dev_priv, clear_pipes);
>  	if (ret)
>  		goto done;
>  
> -	for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
> -		struct intel_crtc_state *crtc_state =
> -			intel_atomic_get_crtc_state(state, intel_crtc);
> -
> -		/* Modeset pipes should have a new state by now */
> -		if (WARN_ON(IS_ERR(crtc_state)))
> +	for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i) {
> +		if (!crtc_state->base.enable)
>  			continue;
>  
>  		ret = dev_priv->display.crtc_compute_clock(intel_crtc,
> @@ -11631,10 +11591,7 @@ done:
>  static int __intel_set_mode(struct drm_crtc *crtc,
>  			    struct drm_display_mode *mode,
>  			    int x, int y, struct drm_framebuffer *fb,
> -			    struct intel_crtc_state *pipe_config,
> -			    unsigned modeset_pipes,
> -			    unsigned prepare_pipes,
> -			    unsigned disable_pipes)
> +			    struct intel_crtc_state *pipe_config)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -11642,7 +11599,9 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  	struct drm_atomic_state *state = pipe_config->base.state;
>  	struct intel_crtc_state *crtc_state_copy = NULL;
>  	struct intel_crtc *intel_crtc;
> +	struct intel_crtc_state *crtc_state;
>  	int ret = 0;
> +	int i;
>  
>  	saved_mode = kmalloc(sizeof(*saved_mode), GFP_KERNEL);
>  	if (!saved_mode)
> @@ -11664,23 +11623,22 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  	 * adjusted_mode bits in the crtc directly.
>  	 */
>  	if (IS_VALLEYVIEW(dev)) {
> -		ret = valleyview_modeset_global_pipes(state, &prepare_pipes);
> +		ret = valleyview_modeset_global_pipes(state);
>  		if (ret)
>  			goto done;
> -
> -		/* may have added more to prepare_pipes than we should */
> -		prepare_pipes &= ~disable_pipes;
>  	}
>  
> -	ret = __intel_set_mode_setup_plls(state, modeset_pipes, disable_pipes);
> +	ret = __intel_set_mode_setup_plls(state);
>  	if (ret)
>  		goto done;
>  
> -	for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc)
> -		intel_crtc_disable(&intel_crtc->base);
> +	for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i) {
> +		if (!needs_modeset(crtc_state))
> +			continue;
>  
> -	for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
> -		if (intel_crtc->base.state->enable)
> +		if (!crtc_state->base.enable)
> +			intel_crtc_disable(&intel_crtc->base);
> +		else if (intel_crtc->base.state->enable)
>  			dev_priv->display.crtc_disable(&intel_crtc->base);
>  	}
>  
> @@ -11691,7 +11649,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  	 * pipes; here we assume a single modeset_pipe and only track the
>  	 * single crtc and mode.
>  	 */
> -	if (modeset_pipes) {
> +	if (pipe_config->base.enable && needs_modeset(pipe_config)) {
>  		crtc->mode = *mode;
>  		/* mode_set/enable/disable functions rely on a correct pipe
>  		 * config. */
> @@ -11708,16 +11666,20 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  
>  	/* Only after disabling all output pipelines that will be changed can we
>  	 * update the the output configuration. */
> -	intel_modeset_update_state(dev, prepare_pipes);
> +	intel_modeset_update_state(state);
>  
>  	modeset_update_crtc_power_domains(state);
>  
> -	for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
> -		struct drm_plane *primary = intel_crtc->base.primary;
> +	if (pipe_config->base.enable && needs_modeset(pipe_config)) {
> +		struct drm_plane *primary;
>  		int vdisplay, hdisplay;
>  
> +		intel_crtc = to_intel_crtc(crtc);
> +		primary = intel_crtc->base.primary;
> +
>  		drm_crtc_get_hv_timing(mode, &hdisplay, &vdisplay);
> -		ret = primary->funcs->update_plane(primary, &intel_crtc->base,
> +
> +		ret = primary->funcs->update_plane(primary, crtc,
>  						   fb, 0, 0,
>  						   hdisplay, vdisplay,
>  						   x << 16, y << 16,
> @@ -11725,7 +11687,10 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  	}
>  
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
> -	for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
> +	for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i) {
> +		if (!crtc_state->base.enable)
> +			continue;
> +
>  		update_scanline_offset(intel_crtc);
>  
>  		dev_priv->display.crtc_enable(&intel_crtc->base);
> @@ -11753,18 +11718,14 @@ done:
>  	return ret;
>  }
>  
> -static int intel_set_mode_pipes(struct drm_crtc *crtc,
> -				struct drm_display_mode *mode,
> -				int x, int y, struct drm_framebuffer *fb,
> -				struct intel_crtc_state *pipe_config,
> -				unsigned modeset_pipes,
> -				unsigned prepare_pipes,
> -				unsigned disable_pipes)
> +static int intel_set_mode_with_config(struct drm_crtc *crtc,
> +				      struct drm_display_mode *mode,
> +				      int x, int y, struct drm_framebuffer *fb,
> +				      struct intel_crtc_state *pipe_config)
>  {
>  	int ret;
>  
> -	ret = __intel_set_mode(crtc, mode, x, y, fb, pipe_config, modeset_pipes,
> -			       prepare_pipes, disable_pipes);
> +	ret = __intel_set_mode(crtc, mode, x, y, fb, pipe_config);
>  
>  	if (ret == 0)
>  		intel_modeset_check_state(crtc->dev);
> @@ -11778,22 +11739,15 @@ static int intel_set_mode(struct drm_crtc *crtc,
>  			  struct drm_atomic_state *state)
>  {
>  	struct intel_crtc_state *pipe_config;
> -	unsigned modeset_pipes, prepare_pipes, disable_pipes;
>  	int ret = 0;
>  
> -	pipe_config = intel_modeset_compute_config(crtc, mode, fb, state,
> -						   &modeset_pipes,
> -						   &prepare_pipes,
> -						   &disable_pipes);
> -
> +	pipe_config = intel_modeset_compute_config(crtc, mode, fb, state);
>  	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);
> +	ret = intel_set_mode_with_config(crtc, mode, x, y, fb, pipe_config);
>  	if (ret)
>  		goto out;
>  
> @@ -12217,7 +12171,6 @@ static int intel_crtc_set_config(struct drm_mode_set *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;
>  	int ret;
>  
>  	BUG_ON(!set);
> @@ -12272,10 +12225,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  	intel_set_config_compute_mode_changes(set, config);
>  
>  	pipe_config = intel_modeset_compute_config(set->crtc, set->mode,
> -						   set->fb, state,
> -						   &modeset_pipes,
> -						   &prepare_pipes,
> -						   &disable_pipes);
> +						   set->fb, state);
>  	if (IS_ERR(pipe_config)) {
>  		ret = PTR_ERR(pipe_config);
>  		goto fail;
> @@ -12295,10 +12245,9 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  	intel_update_pipe_size(to_intel_crtc(set->crtc));
>  
>  	if (config->mode_changed) {
> -		ret = intel_set_mode_pipes(set->crtc, set->mode,
> -					   set->x, set->y, set->fb, pipe_config,
> -					   modeset_pipes, prepare_pipes,
> -					   disable_pipes);
> +		ret = intel_set_mode_with_config(set->crtc, set->mode,
> +						 set->x, set->y, set->fb,
> +						 pipe_config);
>  	} else if (config->fb_changed) {
>  		struct intel_crtc *intel_crtc = to_intel_crtc(set->crtc);
>  		struct drm_plane *primary = set->crtc->primary;
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 08/11] drm/i915: Remove saved_mode from __intel_set_mode()

[Ander Conselvan de Oliveira]

There's no way that function can fail after it sets crtc->mode anymore,
so there's no need to save the old mode for the failure case.
---
 drivers/gpu/drm/i915/intel_display.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a255b24..af3439a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11595,7 +11595,6 @@ 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 drm_atomic_state *state = pipe_config->base.state;
 	struct intel_crtc_state *crtc_state_copy = NULL;
 	struct intel_crtc *intel_crtc;
@@ -11603,18 +11602,12 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 	int ret = 0;
 	int i;
 
-	saved_mode = kmalloc(sizeof(*saved_mode), GFP_KERNEL);
-	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;
-
 	/*
 	 * See if the config requires any additional preparation, e.g.
 	 * to adjust global state with pipes off.  We need to do this
@@ -11698,9 +11691,6 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 
 	/* FIXME: add subpixel order */
 done:
-	if (ret && crtc->state->enable)
-		crtc->mode = *saved_mode;
-
 	if (ret == 0 && pipe_config) {
 		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
@@ -11714,7 +11704,6 @@ done:
 		kfree(crtc_state_copy);
 	}
 
-	kfree(saved_mode);
 	return ret;
 }
 
-- 
2.1.0

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

[PATCH 09/11] drm/i915: Move compute part of __intel_set_mode() to separate function

[Ander Conselvan de Oliveira]

The first function calls done in that function can still cause changes
to the atomic state and may fail. This should eventually be part of our
atomic check function, while the rest of the code in __intel_set_mode()
is the commit hook. So this makes the legacy mode set more atomic-y.
---
 drivers/gpu/drm/i915/intel_display.c | 47 +++++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index af3439a..b249133 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11588,6 +11588,32 @@ done:
 	return ret;
 }
 
+/* Code that should eventually be part of atomic_check() */
+static int __intel_set_mode_checks(struct drm_atomic_state *state)
+{
+	struct drm_device *dev = state->dev;
+	int ret;
+
+	/*
+	 * See if the config requires any additional preparation, e.g.
+	 * to adjust global state with pipes off.  We need to do this
+	 * here so we can get the modeset_pipe updated config for the new
+	 * mode set on this crtc.  For other crtcs we need to use the
+	 * adjusted_mode bits in the crtc directly.
+	 */
+	if (IS_VALLEYVIEW(dev)) {
+		ret = valleyview_modeset_global_pipes(state);
+		if (ret)
+			return ret;
+	}
+
+	ret = __intel_set_mode_setup_plls(state);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int __intel_set_mode(struct drm_crtc *crtc,
 			    struct drm_display_mode *mode,
 			    int x, int y, struct drm_framebuffer *fb,
@@ -11602,29 +11628,16 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 	int ret = 0;
 	int i;
 
+	ret = __intel_set_mode_checks(state);
+	if (ret < 0)
+		return ret;
+
 	crtc_state_copy = kmalloc(sizeof(*crtc_state_copy), GFP_KERNEL);
 	if (!crtc_state_copy) {
 		ret = -ENOMEM;
 		goto done;
 	}
 
-	/*
-	 * See if the config requires any additional preparation, e.g.
-	 * to adjust global state with pipes off.  We need to do this
-	 * here so we can get the modeset_pipe updated config for the new
-	 * mode set on this crtc.  For other crtcs we need to use the
-	 * adjusted_mode bits in the crtc directly.
-	 */
-	if (IS_VALLEYVIEW(dev)) {
-		ret = valleyview_modeset_global_pipes(state);
-		if (ret)
-			goto done;
-	}
-
-	ret = __intel_set_mode_setup_plls(state);
-	if (ret)
-		goto done;
-
 	for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i) {
 		if (!needs_modeset(crtc_state))
 			continue;
-- 
2.1.0

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

[PATCH 10/11] drm/i915: Simplify error handling in __intel_set_mode()

[Ander Conselvan de Oliveira]

The remaining parts of the failure path could only be reached if the
allocation of crtc_state_copy would fail. In that case, there is nothing
to undo, so just get rid of the label for error handling and return an
error code immediately.

We also always allocate a pipe_config, even if the pipe is being
disabled, so the remaining part of what was the error/done case can be
simplified a little too.
---
 drivers/gpu/drm/i915/intel_display.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b249133..06f3b83 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11633,10 +11633,8 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 		return ret;
 
 	crtc_state_copy = kmalloc(sizeof(*crtc_state_copy), GFP_KERNEL);
-	if (!crtc_state_copy) {
-		ret = -ENOMEM;
-		goto done;
-	}
+	if (!crtc_state_copy)
+		return -ENOMEM;
 
 	for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i) {
 		if (!needs_modeset(crtc_state))
@@ -11703,19 +11701,14 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 	}
 
 	/* FIXME: add subpixel order */
-done:
-	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 = crtc_state_copy;
-		intel_crtc->base.state = &crtc_state_copy->base;
-	} else {
-		kfree(crtc_state_copy);
-	}
+	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 = crtc_state_copy;
+	intel_crtc->base.state = &crtc_state_copy->base;
 
 	return ret;
 }
-- 
2.1.0

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

[PATCH 11/11] drm/i915: Don't modeset with old mode when set_crtc fails

[Ander Conselvan de Oliveira]

The modeset code is now properly divided in two phases, so that it only
changes hardware state if it succeeds, so there's no ill-effect that
needs to be undone on failure anymore.
---
 drivers/gpu/drm/i915/intel_display.c | 48 ------------------------------------
 1 file changed, 48 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 06f3b83..84efd7a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12136,33 +12136,9 @@ intel_modeset_stage_output_state(struct drm_device *dev,
 	return 0;
 }
 
-static void disable_crtc_nofb(struct intel_crtc *crtc)
-{
-	struct drm_device *dev = crtc->base.dev;
-	struct intel_encoder *encoder;
-	struct intel_connector *connector;
-
-	DRM_DEBUG_KMS("Trying to restore without FB -> disabling pipe %c\n",
-		      pipe_name(crtc->pipe));
-
-	for_each_intel_connector(dev, connector) {
-		if (connector->new_encoder &&
-		    connector->new_encoder->new_crtc == crtc)
-			connector->new_encoder = NULL;
-	}
-
-	for_each_intel_encoder(dev, encoder) {
-		if (encoder->new_crtc == crtc)
-			encoder->new_crtc = NULL;
-	}
-
-	crtc->new_enabled = false;
-}
-
 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;
@@ -12195,12 +12171,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 	if (ret)
 		goto out_config;
 
-	save_set.crtc = set->crtc;
-	save_set.mode = &set->crtc->mode;
-	save_set.x = set->crtc->x;
-	save_set.y = set->crtc->y;
-	save_set.fb = set->crtc->primary->fb;
-
 	state = drm_atomic_state_alloc(dev);
 	if (!state) {
 		ret = -ENOMEM;
@@ -12280,24 +12250,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 			      set->crtc->base.id, ret);
 fail:
 		intel_set_config_restore_state(dev, config);
-
-		drm_atomic_state_clear(state);
-
-		/*
-		 * HACK: if the pipe was on, but we didn't have a framebuffer,
-		 * force the pipe off to avoid oopsing in the modeset code
-		 * due to fb==NULL. This should only happen during boot since
-		 * we don't yet reconstruct the FB from the hardware state.
-		 */
-		if (to_intel_crtc(save_set.crtc)->new_enabled && !save_set.fb)
-			disable_crtc_nofb(to_intel_crtc(save_set.crtc));
-
-		/* 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,
-				   state))
-			DRM_ERROR("failed to restore config after modeset failure\n");
 	}
 
 out_config:
-- 
2.1.0

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

Re: [PATCH 11/11] drm/i915: Don't modeset with old mode when set_crtc fails

[Daniel Vetter]

On Fri, Apr 10, 2015 at 11:38:40AM +0300, Ander Conselvan de Oliveira wrote:
> The modeset code is now properly divided in two phases, so that it only
> changes hardware state if it succeeds, so there's no ill-effect that
> needs to be undone on failure anymore.
> ---
>  drivers/gpu/drm/i915/intel_display.c | 48 ------------------------------------
>  1 file changed, 48 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 06f3b83..84efd7a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12136,33 +12136,9 @@ intel_modeset_stage_output_state(struct drm_device *dev,
>  	return 0;
>  }
>  
> -static void disable_crtc_nofb(struct intel_crtc *crtc)
> -{
> -	struct drm_device *dev = crtc->base.dev;
> -	struct intel_encoder *encoder;
> -	struct intel_connector *connector;
> -
> -	DRM_DEBUG_KMS("Trying to restore without FB -> disabling pipe %c\n",
> -		      pipe_name(crtc->pipe));
> -
> -	for_each_intel_connector(dev, connector) {
> -		if (connector->new_encoder &&
> -		    connector->new_encoder->new_crtc == crtc)
> -			connector->new_encoder = NULL;
> -	}
> -
> -	for_each_intel_encoder(dev, encoder) {
> -		if (encoder->new_crtc == crtc)
> -			encoder->new_crtc = NULL;
> -	}
> -
> -	crtc->new_enabled = false;
> -}
> -
>  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;
> @@ -12195,12 +12171,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  	if (ret)
>  		goto out_config;
>  
> -	save_set.crtc = set->crtc;
> -	save_set.mode = &set->crtc->mode;
> -	save_set.x = set->crtc->x;
> -	save_set.y = set->crtc->y;
> -	save_set.fb = set->crtc->primary->fb;
> -
>  	state = drm_atomic_state_alloc(dev);
>  	if (!state) {
>  		ret = -ENOMEM;
> @@ -12280,24 +12250,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  			      set->crtc->base.id, ret);
>  fail:
>  		intel_set_config_restore_state(dev, config);
> -
> -		drm_atomic_state_clear(state);
> -
> -		/*
> -		 * HACK: if the pipe was on, but we didn't have a framebuffer,
> -		 * force the pipe off to avoid oopsing in the modeset code
> -		 * due to fb==NULL. This should only happen during boot since
> -		 * we don't yet reconstruct the FB from the hardware state.
> -		 */
> -		if (to_intel_crtc(save_set.crtc)->new_enabled && !save_set.fb)
> -			disable_crtc_nofb(to_intel_crtc(save_set.crtc));
> -
> -		/* 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,
> -				   state))
> -			DRM_ERROR("failed to restore config after modeset failure\n");

Hm removing this reminds me that we still have troubles with always
assuming that there is a primary fb around. Getting rid of all the fb
pointers in our modeset code looks like a sizeable task too.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/11] drm/i915: Don't modeset with old mode when set_crtc fails

[Ander Conselvan De Oliveira]

On Fri, 2015-04-10 at 12:12 +0200, Daniel Vetter wrote:
> On Fri, Apr 10, 2015 at 11:38:40AM +0300, Ander Conselvan de Oliveira wrote:
> > The modeset code is now properly divided in two phases, so that it only
> > changes hardware state if it succeeds, so there's no ill-effect that
> > needs to be undone on failure anymore.
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 48 ------------------------------------
> >  1 file changed, 48 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 06f3b83..84efd7a 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12136,33 +12136,9 @@ intel_modeset_stage_output_state(struct drm_device *dev,
> >  	return 0;
> >  }
> >  
> > -static void disable_crtc_nofb(struct intel_crtc *crtc)
> > -{
> > -	struct drm_device *dev = crtc->base.dev;
> > -	struct intel_encoder *encoder;
> > -	struct intel_connector *connector;
> > -
> > -	DRM_DEBUG_KMS("Trying to restore without FB -> disabling pipe %c\n",
> > -		      pipe_name(crtc->pipe));
> > -
> > -	for_each_intel_connector(dev, connector) {
> > -		if (connector->new_encoder &&
> > -		    connector->new_encoder->new_crtc == crtc)
> > -			connector->new_encoder = NULL;
> > -	}
> > -
> > -	for_each_intel_encoder(dev, encoder) {
> > -		if (encoder->new_crtc == crtc)
> > -			encoder->new_crtc = NULL;
> > -	}
> > -
> > -	crtc->new_enabled = false;
> > -}
> > -
> >  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;
> > @@ -12195,12 +12171,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
> >  	if (ret)
> >  		goto out_config;
> >  
> > -	save_set.crtc = set->crtc;
> > -	save_set.mode = &set->crtc->mode;
> > -	save_set.x = set->crtc->x;
> > -	save_set.y = set->crtc->y;
> > -	save_set.fb = set->crtc->primary->fb;
> > -
> >  	state = drm_atomic_state_alloc(dev);
> >  	if (!state) {
> >  		ret = -ENOMEM;
> > @@ -12280,24 +12250,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
> >  			      set->crtc->base.id, ret);
> >  fail:
> >  		intel_set_config_restore_state(dev, config);
> > -
> > -		drm_atomic_state_clear(state);
> > -
> > -		/*
> > -		 * HACK: if the pipe was on, but we didn't have a framebuffer,
> > -		 * force the pipe off to avoid oopsing in the modeset code
> > -		 * due to fb==NULL. This should only happen during boot since
> > -		 * we don't yet reconstruct the FB from the hardware state.
> > -		 */
> > -		if (to_intel_crtc(save_set.crtc)->new_enabled && !save_set.fb)
> > -			disable_crtc_nofb(to_intel_crtc(save_set.crtc));
> > -
> > -		/* 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,
> > -				   state))
> > -			DRM_ERROR("failed to restore config after modeset failure\n");
> 
> Hm removing this reminds me that we still have troubles with always
> assuming that there is a primary fb around. Getting rid of all the fb
> pointers in our modeset code looks like a sizeable task too.

I only see the fb argument being used in two places now: the part where
we update the primary plane in __intel_set_mode() and in
compute_baseline_pipe_bpp() called from intel_modeset_pipe_config(). I
think those are easy to solve by adding the plane state. Or are we
talking about something else here?

Ander

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

Re: [PATCH 11/11] drm/i915: Don't modeset with old mode when set_crtc fails

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6169
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -5              276/276              271/276
ILK                                  301/301              301/301
SNB                 -1              316/316              315/316
IVB                 -1              328/328              327/328
BYT                                  285/285              285/285
HSW                                  394/394              394/394
BDW                                  321/321              321/321
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 PNV  igt@gem_userptr_blits@coherency-unsync      CRASH(1)PASS(2)      CRASH(2)
 PNV  igt@gen3_render_linear_blits      FAIL(2)PASS(1)      FAIL(2)
 PNV  igt@gen3_render_mixed_blits      FAIL(2)PASS(1)      FAIL(2)
 PNV  igt@gen3_render_tiledx_blits      FAIL(2)PASS(1)      FAIL(2)
 PNV  igt@gen3_render_tiledy_blits      FAIL(2)PASS(1)      FAIL(2)
 SNB  igt@kms_flip@modeset-vs-vblank-race      DMESG_WARN(1)PASS(3)      DMESG_WARN(1)PASS(1)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*failed_to_enable_link_training@failed to enable link training
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_start_channel_equalization@failed to start channel equalization
drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 IVB  igt@gem_pwrite_pread@uncached-copy-performance      DMESG_WARN(1)PASS(1)      DMESG_WARN(2)
(dmesg patch applied)drm:i915_hangcheck_elapsed[i915]]*ERROR*Hangcheck_timer_elapsed...blitter_ring_idle@Hangcheck timer elapsed... blitter ring idle
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