[PATCH 1/6] drm/i915: Use drm_connector_list_iter in debugfs

[PATCH 1/6] drm/i915: Use drm_connector_list_iter in debugfs

[Daniel Vetter]

While at it also try to reduce the locking a bit to what's really just
needed instead of everything that we could possibly lock.

Added a new for_each_intel_connector_iter which includes the cast to
intel_connector.

Otherwise just plain transformation with nothing special going on.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 62 ++++++++++++++++++++++++-------------
 drivers/gpu/drm/i915/i915_drv.h     |  3 ++
 2 files changed, 44 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 15deb2bc568b..f7633e8474b2 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2617,12 +2617,15 @@ static int i915_sink_crc(struct seq_file *m, void *data)
 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
 	struct drm_device *dev = &dev_priv->drm;
 	struct intel_connector *connector;
+	struct drm_connector_list_iter conn_iter;
 	struct intel_dp *intel_dp = NULL;
 	int ret;
 	u8 crc[6];
 
-	drm_modeset_lock_all(dev);
-	for_each_intel_connector(dev, connector) {
+	/* connection mutex also gives us a read lock on the crtc */
+	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+	drm_connector_list_iter_get(dev, &conn_iter);
+	for_each_intel_connector_iter(connector, &conn_iter) {
 		struct drm_crtc *crtc;
 
 		if (!connector->base.state->best_encoder)
@@ -2648,7 +2651,8 @@ static int i915_sink_crc(struct seq_file *m, void *data)
 	}
 	ret = -ENODEV;
 out:
-	drm_modeset_unlock_all(dev);
+	drm_connector_list_iter_put(&conn_iter);
+	drm_modeset_unlock(&dev->mode_config.connection_mutex);
 	return ret;
 }
 
@@ -3089,9 +3093,9 @@ static int i915_display_info(struct seq_file *m, void *unused)
 	struct drm_device *dev = &dev_priv->drm;
 	struct intel_crtc *crtc;
 	struct drm_connector *connector;
+	struct drm_connector_list_iter conn_iter;
 
 	intel_runtime_pm_get(dev_priv);
-	drm_modeset_lock_all(dev);
 	seq_printf(m, "CRTC info\n");
 	seq_printf(m, "---------\n");
 	for_each_intel_crtc(dev, crtc) {
@@ -3099,6 +3103,7 @@ static int i915_display_info(struct seq_file *m, void *unused)
 		struct intel_crtc_state *pipe_config;
 		int x, y;
 
+		drm_modeset_lock(&crtc->base.mutex, NULL);
 		pipe_config = to_intel_crtc_state(crtc->base.state);
 
 		seq_printf(m, "CRTC %d: pipe: %c, active=%s, (size=%dx%d), dither=%s, bpp=%d\n",
@@ -3123,15 +3128,19 @@ static int i915_display_info(struct seq_file *m, void *unused)
 		seq_printf(m, "\tunderrun reporting: cpu=%s pch=%s \n",
 			   yesno(!crtc->cpu_fifo_underrun_disabled),
 			   yesno(!crtc->pch_fifo_underrun_disabled));
+		drm_modeset_unlock(&crtc->base.mutex);
 	}
 
 	seq_printf(m, "\n");
 	seq_printf(m, "Connector info\n");
 	seq_printf(m, "--------------\n");
-	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
+	mutex_lock(&dev->mode_config.mutex);
+	drm_connector_list_iter_get(dev, &conn_iter);
+	drm_for_each_connector_iter(connector, &conn_iter)
 		intel_connector_info(m, connector);
-	}
-	drm_modeset_unlock_all(dev);
+	drm_connector_list_iter_put(&conn_iter);
+	mutex_unlock(&dev->mode_config.mutex);
+
 	intel_runtime_pm_put(dev_priv);
 
 	return 0;
@@ -3452,13 +3461,16 @@ static void drrs_status_per_crtc(struct seq_file *m,
 	struct i915_drrs *drrs = &dev_priv->drrs;
 	int vrefresh = 0;
 	struct drm_connector *connector;
+	struct drm_connector_list_iter conn_iter;
 
-	drm_for_each_connector(connector, dev) {
+	drm_connector_list_iter_get(dev, &conn_iter);
+	drm_for_each_connector_iter(connector, &conn_iter) {
 		if (connector->state->crtc != &intel_crtc->base)
 			continue;
 
 		seq_printf(m, "%s:\n", connector->name);
 	}
+	drm_connector_list_iter_put(&conn_iter);
 
 	if (dev_priv->vbt.drrs_type == STATIC_DRRS_SUPPORT)
 		seq_puts(m, "\tVBT: DRRS_type: Static");
@@ -3544,9 +3556,10 @@ static int i915_dp_mst_info(struct seq_file *m, void *unused)
 	struct intel_encoder *intel_encoder;
 	struct intel_digital_port *intel_dig_port;
 	struct drm_connector *connector;
+	struct drm_connector_list_iter conn_iter;
 
-	drm_modeset_lock_all(dev);
-	drm_for_each_connector(connector, dev) {
+	drm_connector_list_iter_get(dev, &conn_iter);
+	drm_for_each_connector_iter(connector, &conn_iter) {
 		if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort)
 			continue;
 
@@ -3562,7 +3575,8 @@ static int i915_dp_mst_info(struct seq_file *m, void *unused)
 			   port_name(intel_dig_port->port));
 		drm_dp_mst_dump_topology(m, &intel_dig_port->dp.mst_mgr);
 	}
-	drm_modeset_unlock_all(dev);
+	drm_connector_list_iter_put(&conn_iter);
+
 	return 0;
 }
 
@@ -3574,14 +3588,12 @@ static ssize_t i915_displayport_test_active_write(struct file *file,
 	int status = 0;
 	struct drm_device *dev;
 	struct drm_connector *connector;
-	struct list_head *connector_list;
+	struct drm_connector_list_iter conn_iter;
 	struct intel_dp *intel_dp;
 	int val = 0;
 
 	dev = ((struct seq_file *)file->private_data)->private;
 
-	connector_list = &dev->mode_config.connector_list;
-
 	if (len == 0)
 		return 0;
 
@@ -3597,7 +3609,8 @@ static ssize_t i915_displayport_test_active_write(struct file *file,
 	input_buffer[len] = '\0';
 	DRM_DEBUG_DRIVER("Copied %d bytes from user\n", (unsigned int)len);
 
-	list_for_each_entry(connector, connector_list, head) {
+	drm_connector_list_iter_get(dev, &conn_iter);
+	drm_for_each_connector_iter(connector, &conn_iter) {
 		if (connector->connector_type !=
 		    DRM_MODE_CONNECTOR_DisplayPort)
 			continue;
@@ -3618,6 +3631,7 @@ static ssize_t i915_displayport_test_active_write(struct file *file,
 				intel_dp->compliance.test_active = 0;
 		}
 	}
+	drm_connector_list_iter_put(&conn_iter);
 out:
 	kfree(input_buffer);
 	if (status < 0)
@@ -3631,10 +3645,11 @@ static int i915_displayport_test_active_show(struct seq_file *m, void *data)
 {
 	struct drm_device *dev = m->private;
 	struct drm_connector *connector;
-	struct list_head *connector_list = &dev->mode_config.connector_list;
+	struct drm_connector_list_iter conn_iter;
 	struct intel_dp *intel_dp;
 
-	list_for_each_entry(connector, connector_list, head) {
+	drm_connector_list_iter_get(dev, &conn_iter);
+	drm_for_each_connector_iter(connector, &conn_iter) {
 		if (connector->connector_type !=
 		    DRM_MODE_CONNECTOR_DisplayPort)
 			continue;
@@ -3649,6 +3664,7 @@ static int i915_displayport_test_active_show(struct seq_file *m, void *data)
 		} else
 			seq_puts(m, "0");
 	}
+	drm_connector_list_iter_put(&conn_iter);
 
 	return 0;
 }
@@ -3675,10 +3691,11 @@ static int i915_displayport_test_data_show(struct seq_file *m, void *data)
 {
 	struct drm_device *dev = m->private;
 	struct drm_connector *connector;
-	struct list_head *connector_list = &dev->mode_config.connector_list;
+	struct drm_connector_list_iter conn_iter;
 	struct intel_dp *intel_dp;
 
-	list_for_each_entry(connector, connector_list, head) {
+	drm_connector_list_iter_get(dev, &conn_iter);
+	drm_for_each_connector_iter(connector, &conn_iter) {
 		if (connector->connector_type !=
 		    DRM_MODE_CONNECTOR_DisplayPort)
 			continue;
@@ -3690,6 +3707,7 @@ static int i915_displayport_test_data_show(struct seq_file *m, void *data)
 		} else
 			seq_puts(m, "0");
 	}
+	drm_connector_list_iter_put(&conn_iter);
 
 	return 0;
 }
@@ -3714,10 +3732,11 @@ static int i915_displayport_test_type_show(struct seq_file *m, void *data)
 {
 	struct drm_device *dev = m->private;
 	struct drm_connector *connector;
-	struct list_head *connector_list = &dev->mode_config.connector_list;
+	struct drm_connector_list_iter conn_iter;
 	struct intel_dp *intel_dp;
 
-	list_for_each_entry(connector, connector_list, head) {
+	drm_connector_list_iter_get(dev, &conn_iter);
+	drm_for_each_connector_iter(connector, &conn_iter) {
 		if (connector->connector_type !=
 		    DRM_MODE_CONNECTOR_DisplayPort)
 			continue;
@@ -3729,6 +3748,7 @@ static int i915_displayport_test_type_show(struct seq_file *m, void *data)
 		} else
 			seq_puts(m, "0");
 	}
+	drm_connector_list_iter_put(&conn_iter);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6217f01d3c11..57914f008ed8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -493,6 +493,9 @@ struct i915_hotplug {
 			    &(dev)->mode_config.connector_list,	\
 			    base.head)
 
+#define for_each_intel_connector_iter(intel_connector, iter) \
+	while ((intel_connector = to_intel_connector(drm_connector_list_iter_next(iter))))
+
 #define for_each_encoder_on_crtc(dev, __crtc, intel_encoder) \
 	list_for_each_entry((intel_encoder), &(dev)->mode_config.encoder_list, base.head) \
 		for_each_if ((intel_encoder)->base.crtc == (__crtc))
-- 
2.11.0

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

[PATCH 2/6] drm/i915: use drm_connector_list_iter in intel_hotplug.c

[Daniel Vetter]

Nothing special, just rote conversion.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_hotplug.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
index 3d546c019de0..2ddc9e5842ec 100644
--- a/drivers/gpu/drm/i915/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/intel_hotplug.c
@@ -145,16 +145,17 @@ static bool intel_hpd_irq_storm_detect(struct drm_i915_private *dev_priv,
 static void intel_hpd_irq_storm_disable(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = &dev_priv->drm;
-	struct drm_mode_config *mode_config = &dev->mode_config;
 	struct intel_connector *intel_connector;
 	struct intel_encoder *intel_encoder;
 	struct drm_connector *connector;
+	struct drm_connector_list_iter conn_iter;
 	enum hpd_pin pin;
 	bool hpd_disabled = false;
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	list_for_each_entry(connector, &mode_config->connector_list, head) {
+	drm_connector_list_iter_get(dev, &conn_iter);
+	drm_for_each_connector_iter(connector, &conn_iter) {
 		if (connector->polled != DRM_CONNECTOR_POLL_HPD)
 			continue;
 
@@ -177,6 +178,7 @@ static void intel_hpd_irq_storm_disable(struct drm_i915_private *dev_priv)
 			| DRM_CONNECTOR_POLL_DISCONNECT;
 		hpd_disabled = true;
 	}
+	drm_connector_list_iter_put(&conn_iter);
 
 	/* Enable polling and queue hotplug re-enabling. */
 	if (hpd_disabled) {
@@ -192,7 +194,6 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work)
 		container_of(work, typeof(*dev_priv),
 			     hotplug.reenable_work.work);
 	struct drm_device *dev = &dev_priv->drm;
-	struct drm_mode_config *mode_config = &dev->mode_config;
 	int i;
 
 	intel_runtime_pm_get(dev_priv);
@@ -200,13 +201,15 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work)
 	spin_lock_irq(&dev_priv->irq_lock);
 	for_each_hpd_pin(i) {
 		struct drm_connector *connector;
+		struct drm_connector_list_iter conn_iter;
 
 		if (dev_priv->hotplug.stats[i].state != HPD_DISABLED)
 			continue;
 
 		dev_priv->hotplug.stats[i].state = HPD_ENABLED;
 
-		list_for_each_entry(connector, &mode_config->connector_list, head) {
+		drm_connector_list_iter_get(dev, &conn_iter);
+		drm_for_each_connector_iter(connector, &conn_iter) {
 			struct intel_connector *intel_connector = to_intel_connector(connector);
 
 			if (intel_connector->encoder->hpd_pin == i) {
@@ -218,6 +221,7 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work)
 					connector->polled = DRM_CONNECTOR_POLL_HPD;
 			}
 		}
+		drm_connector_list_iter_put(&conn_iter);
 	}
 	if (dev_priv->display.hpd_irq_setup)
 		dev_priv->display.hpd_irq_setup(dev_priv);
@@ -303,14 +307,14 @@ static void i915_hotplug_work_func(struct work_struct *work)
 	struct drm_i915_private *dev_priv =
 		container_of(work, struct drm_i915_private, hotplug.hotplug_work);
 	struct drm_device *dev = &dev_priv->drm;
-	struct drm_mode_config *mode_config = &dev->mode_config;
 	struct intel_connector *intel_connector;
 	struct intel_encoder *intel_encoder;
 	struct drm_connector *connector;
+	struct drm_connector_list_iter conn_iter;
 	bool changed = false;
 	u32 hpd_event_bits;
 
-	mutex_lock(&mode_config->mutex);
+	mutex_lock(&dev->mode_config.mutex);
 	DRM_DEBUG_KMS("running encoder hotplug functions\n");
 
 	spin_lock_irq(&dev_priv->irq_lock);
@@ -323,7 +327,8 @@ static void i915_hotplug_work_func(struct work_struct *work)
 
 	spin_unlock_irq(&dev_priv->irq_lock);
 
-	list_for_each_entry(connector, &mode_config->connector_list, head) {
+	drm_connector_list_iter_get(dev, &conn_iter);
+	drm_for_each_connector_iter(connector, &conn_iter) {
 		intel_connector = to_intel_connector(connector);
 		if (!intel_connector->encoder)
 			continue;
@@ -337,7 +342,8 @@ static void i915_hotplug_work_func(struct work_struct *work)
 				changed = true;
 		}
 	}
-	mutex_unlock(&mode_config->mutex);
+	drm_connector_list_iter_put(&conn_iter);
+	mutex_unlock(&dev->mode_config.mutex);
 
 	if (changed)
 		drm_kms_helper_hotplug_event(dev);
@@ -483,15 +489,16 @@ static void i915_hpd_poll_init_work(struct work_struct *work)
 		container_of(work, struct drm_i915_private,
 			     hotplug.poll_init_work);
 	struct drm_device *dev = &dev_priv->drm;
-	struct drm_mode_config *mode_config = &dev->mode_config;
 	struct drm_connector *connector;
+	struct drm_connector_list_iter conn_iter;
 	bool enabled;
 
 	mutex_lock(&dev->mode_config.mutex);
 
 	enabled = READ_ONCE(dev_priv->hotplug.poll_enabled);
 
-	list_for_each_entry(connector, &mode_config->connector_list, head) {
+	drm_connector_list_iter_get(dev, &conn_iter);
+	drm_for_each_connector_iter(connector, &conn_iter) {
 		struct intel_connector *intel_connector =
 			to_intel_connector(connector);
 		connector->polled = intel_connector->polled;
@@ -509,6 +516,7 @@ static void i915_hpd_poll_init_work(struct work_struct *work)
 				DRM_CONNECTOR_POLL_HPD;
 		}
 	}
+	drm_connector_list_iter_put(&conn_iter);
 
 	if (enabled)
 		drm_kms_helper_poll_enable_locked(dev);
-- 
2.11.0

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

[PATCH 3/6] drm/i915: use drm_connector_list_iter in intel_opregion.c

[Daniel Vetter]

One case where I nuked a now unecessary locking, otherwise all just
boring stuff.

Cc: Jani Nikula <jani.nikula@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_opregion.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index f4429f67a4e3..d586ff9a66ce 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -434,6 +434,7 @@ int intel_opregion_notify_adapter(struct drm_i915_private *dev_priv,
 static u32 asle_set_backlight(struct drm_i915_private *dev_priv, u32 bclp)
 {
 	struct intel_connector *connector;
+	struct drm_connector_list_iter conn_iter;
 	struct opregion_asle *asle = dev_priv->opregion.asle;
 	struct drm_device *dev = &dev_priv->drm;
 
@@ -458,8 +459,10 @@ static u32 asle_set_backlight(struct drm_i915_private *dev_priv, u32 bclp)
 	 * only one).
 	 */
 	DRM_DEBUG_KMS("updating opregion backlight %d/255\n", bclp);
-	for_each_intel_connector(dev, connector)
+	drm_connector_list_iter_get(dev, &conn_iter);
+	for_each_intel_connector_iter(connector, &conn_iter)
 		intel_panel_set_backlight_acpi(connector, bclp, 255);
+	drm_connector_list_iter_put(&conn_iter);
 	asle->cblv = DIV_ROUND_UP(bclp * 100, 255) | ASLE_CBLV_VALID;
 
 	drm_modeset_unlock(&dev->mode_config.connection_mutex);
@@ -701,6 +704,7 @@ static void intel_didl_outputs(struct drm_i915_private *dev_priv)
 {
 	struct intel_opregion *opregion = &dev_priv->opregion;
 	struct intel_connector *connector;
+	struct drm_connector_list_iter conn_iter;
 	int i = 0, max_outputs;
 	int display_index[16] = {};
 
@@ -714,7 +718,8 @@ static void intel_didl_outputs(struct drm_i915_private *dev_priv)
 	max_outputs = ARRAY_SIZE(opregion->acpi->didl) +
 		ARRAY_SIZE(opregion->acpi->did2);
 
-	for_each_intel_connector(&dev_priv->drm, connector) {
+	drm_connector_list_iter_get(&dev_priv->drm, &conn_iter);
+	for_each_intel_connector_iter(connector, &conn_iter) {
 		u32 device_id, type;
 
 		device_id = acpi_display_type(connector);
@@ -729,6 +734,7 @@ static void intel_didl_outputs(struct drm_i915_private *dev_priv)
 			set_did(opregion, i, device_id);
 		i++;
 	}
+	drm_connector_list_iter_put(&conn_iter);
 
 	DRM_DEBUG_KMS("%d outputs detected\n", i);
 
@@ -745,6 +751,7 @@ static void intel_setup_cadls(struct drm_i915_private *dev_priv)
 {
 	struct intel_opregion *opregion = &dev_priv->opregion;
 	struct intel_connector *connector;
+	struct drm_connector_list_iter conn_iter;
 	int i = 0;
 
 	/*
@@ -757,11 +764,13 @@ static void intel_setup_cadls(struct drm_i915_private *dev_priv)
 	 * Note that internal panels should be at the front of the connector
 	 * list already, ensuring they're not left out.
 	 */
-	for_each_intel_connector(&dev_priv->drm, connector) {
+	drm_connector_list_iter_get(&dev_priv->drm, &conn_iter);
+	for_each_intel_connector_iter(connector, &conn_iter) {
 		if (i >= ARRAY_SIZE(opregion->acpi->cadl))
 			break;
 		opregion->acpi->cadl[i++] = connector->acpi_device_id;
 	}
+	drm_connector_list_iter_put(&conn_iter);
 
 	/* If fewer than 8 active devices, the list must be null terminated */
 	if (i < ARRAY_SIZE(opregion->acpi->cadl))
-- 
2.11.0

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

[PATCH 4/6] drm/i915: Make intel_get_pipe_from_connector atomic

[Daniel Vetter]

Drive-by fixup while looking at all the connector_list walkers -
holding connection_mutex does actually _not_ give you locking to look
at the legacy drm_connector->encoder->crtc pointer chain. That one is
solely owned by the atomic commit workers. Instead we must inspect the
atomic state.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0b0d7e8be630..438d27f93aca 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15427,15 +15427,14 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
 
 enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
 {
-	struct drm_encoder *encoder = connector->base.encoder;
 	struct drm_device *dev = connector->base.dev;
 
 	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
-	if (!encoder || WARN_ON(!encoder->crtc))
+	if (!connector->base.state->crtc)
 		return INVALID_PIPE;
 
-	return to_intel_crtc(encoder->crtc)->pipe;
+	return to_intel_crtc(connector->base.state->crtc)->pipe;
 }
 
 int intel_get_pipe_from_crtc_id(struct drm_device *dev, void *data,
-- 
2.11.0

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

[PATCH 5/6] drm/i915: use for_each_intel_connector_iter in intel_display.c

[Daniel Vetter]

This gets rid of the last users of for_each_intel_connector(), remove
that too.

At first I wasn't sure whether the 2 loops in the modeset state
checker should instead only loop over the connectors in the atomic
commit. But we never add connectors to an atomic update if they don't
(or won't have) a CRTC assigned, which means there'd be a gap in check
coverage. Hence loop over everything on those too.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  5 -----
 drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++++++++----
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 57914f008ed8..fe2b37fe0b91 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -488,11 +488,6 @@ struct i915_hotplug {
 			    &(dev)->mode_config.encoder_list,	\
 			    base.head)
 
-#define for_each_intel_connector(dev, intel_connector)		\
-	list_for_each_entry(intel_connector,			\
-			    &(dev)->mode_config.connector_list,	\
-			    base.head)
-
 #define for_each_intel_connector_iter(intel_connector, iter) \
 	while ((intel_connector = to_intel_connector(drm_connector_list_iter_next(iter))))
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 438d27f93aca..e029dba96bca 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12661,8 +12661,10 @@ static const struct drm_crtc_helper_funcs intel_helper_funcs = {
 static void intel_modeset_update_connector_atomic_state(struct drm_device *dev)
 {
 	struct intel_connector *connector;
+	struct drm_connector_list_iter conn_iter;
 
-	for_each_intel_connector(dev, connector) {
+	drm_connector_list_iter_get(dev, &conn_iter);
+	for_each_intel_connector_iter(connector, &conn_iter) {
 		if (connector->base.state->crtc)
 			drm_connector_unreference(&connector->base);
 
@@ -12678,6 +12680,7 @@ static void intel_modeset_update_connector_atomic_state(struct drm_device *dev)
 			connector->base.state->crtc = NULL;
 		}
 	}
+	drm_connector_list_iter_put(&conn_iter);
 }
 
 static void
@@ -13610,6 +13613,7 @@ verify_encoder_state(struct drm_device *dev)
 {
 	struct intel_encoder *encoder;
 	struct intel_connector *connector;
+	struct drm_connector_list_iter conn_iter;
 
 	for_each_intel_encoder(dev, encoder) {
 		bool enabled = false;
@@ -13619,7 +13623,8 @@ verify_encoder_state(struct drm_device *dev)
 			      encoder->base.base.id,
 			      encoder->base.name);
 
-		for_each_intel_connector(dev, connector) {
+		drm_connector_list_iter_get(dev, &conn_iter);
+		for_each_intel_connector_iter(connector, &conn_iter) {
 			if (connector->base.state->best_encoder != &encoder->base)
 				continue;
 			enabled = true;
@@ -13628,6 +13633,7 @@ verify_encoder_state(struct drm_device *dev)
 					encoder->base.crtc,
 			     "connector's crtc doesn't match encoder crtc\n");
 		}
+		drm_connector_list_iter_put(&conn_iter);
 
 		I915_STATE_WARN(!!encoder->base.crtc != enabled,
 		     "encoder's enabled state mismatch "
@@ -16638,6 +16644,7 @@ int intel_modeset_init(struct drm_device *dev)
 static void intel_enable_pipe_a(struct drm_device *dev)
 {
 	struct intel_connector *connector;
+	struct drm_connector_list_iter conn_iter;
 	struct drm_connector *crt = NULL;
 	struct intel_load_detect_pipe load_detect_temp;
 	struct drm_modeset_acquire_ctx *ctx = dev->mode_config.acquire_ctx;
@@ -16645,12 +16652,14 @@ static void intel_enable_pipe_a(struct drm_device *dev)
 	/* We can't just switch on the pipe A, we need to set things up with a
 	 * proper mode and output configuration. As a gross hack, enable pipe A
 	 * by enabling the load detect pipe once. */
-	for_each_intel_connector(dev, connector) {
+	drm_connector_list_iter_get(dev, &conn_iter);
+	for_each_intel_connector_iter(connector, &conn_iter) {
 		if (connector->encoder->type == INTEL_OUTPUT_ANALOG) {
 			crt = &connector->base;
 			break;
 		}
 	}
+	drm_connector_list_iter_put(&conn_iter);
 
 	if (!crt)
 		return;
@@ -16896,6 +16905,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 	struct intel_crtc *crtc;
 	struct intel_encoder *encoder;
 	struct intel_connector *connector;
+	struct drm_connector_list_iter conn_iter;
 	int i;
 
 	dev_priv->active_crtcs = 0;
@@ -16973,7 +16983,8 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 			      pipe_name(pipe));
 	}
 
-	for_each_intel_connector(dev, connector) {
+	drm_connector_list_iter_get(dev, &conn_iter);
+	for_each_intel_connector_iter(connector, &conn_iter) {
 		if (connector->get_hw_state(connector)) {
 			connector->base.dpms = DRM_MODE_DPMS_ON;
 
@@ -17001,6 +17012,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 			      connector->base.base.id, connector->base.name,
 			      enableddisabled(connector->base.encoder));
 	}
+	drm_connector_list_iter_put(&conn_iter);
 
 	for_each_intel_crtc(dev, crtc) {
 		crtc->base.hwmode = crtc->config->base.adjusted_mode;
-- 
2.11.0

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

[PATCH 6/6] drm/i915: Update comment that sets I915_MODE_FLAG_INHERITED

[Daniel Vetter]

The code was moved, but the comment not updated. It confused me.

Fixes: 7f4c62840cc4 ("drm/i915: Assign hwmode after encoder state readout")
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e029dba96bca..c7d775f01b23 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -17028,17 +17028,9 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 			 * the atomic core happy. It wants a valid mode if the
 			 * crtc's enabled, so we do the above call.
 			 *
-			 * At this point some state updated by the connectors
-			 * in their ->detect() callback has not run yet, so
-			 * no recalculation can be done yet.
-			 *
-			 * Even if we could do a recalculation and modeset
-			 * right now it would cause a double modeset if
-			 * fbdev or userspace chooses a different initial mode.
-			 *
-			 * If that happens, someone indicated they wanted a
-			 * mode change, which means it's safe to do a full
-			 * recalculation.
+			 * But we don't set all the derived state fully, hence
+			 * set a flag to indicate that a full recalculation is
+			 * needed on the next commit.
 			 */
 			crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED;
 
-- 
2.11.0

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

✓ Fi.CI.BAT: success for series starting with [1/6] drm/i915: Use drm_connector_list_iter in debugfs

[Patchwork]

== Series Details ==

Series: series starting with [1/6] drm/i915: Use drm_connector_list_iter in debugfs
URL   : https://patchwork.freedesktop.org/series/16979/
State : success

== Summary ==

Series 16979v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/16979/revisions/1/mbox/

Test drv_module_reload:
        Subgroup basic-reload-inject:
                dmesg-warn -> PASS       (fi-ilk-650)

fi-bdw-5557u     total:247  pass:233  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:247  pass:208  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:247  pass:222  dwarn:0   dfail:0   fail:0   skip:25 
fi-byt-j1900     total:247  pass:220  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:247  pass:216  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:247  pass:228  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:247  pass:228  dwarn:0   dfail:0   fail:0   skip:19 
fi-ilk-650       total:247  pass:195  dwarn:0   dfail:0   fail:0   skip:52 
fi-ivb-3520m     total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6260u     total:247  pass:234  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:247  pass:227  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:247  pass:224  dwarn:3   dfail:0   fail:0   skip:20 
fi-skl-6770hq    total:247  pass:234  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:247  pass:216  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:247  pass:215  dwarn:0   dfail:0   fail:0   skip:32 

2a932d085375c80a1bbb332799db3df9738e8eba drm-tip: 2016y-12m-18d-16h-31m-05s UTC integration manifest
3c99bf8 drm/i915: Update comment that sets I915_MODE_FLAG_INHERITED
ccedbbc drm/i915: use for_each_intel_connector_iter in intel_display.c
854dab8 drm/i915: Make intel_get_pipe_from_connector atomic
e1e40a0 drm/i915: use drm_connector_list_iter in intel_opregion.c
fed9d3d drm/i915: use drm_connector_list_iter in intel_hotplug.c
6f92481 drm/i915: Use drm_connector_list_iter in debugfs

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3320/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: use for_each_intel_connector_iter in intel_display.c

[Daniel Vetter]

This gets rid of the last users of for_each_intel_connector(), remove
that too.

The one exception is the loop in verify_encoder_state - that needs to
switch over to for_each_connector_in_state.

v2: Maarten corrected me that in the state verifier we indeed must
only walk connectors in the atomic commit, not all of them!

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  5 -----
 drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++++---------
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 57914f008ed8..fe2b37fe0b91 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -488,11 +488,6 @@ struct i915_hotplug {
 			    &(dev)->mode_config.encoder_list,	\
 			    base.head)
 
-#define for_each_intel_connector(dev, intel_connector)		\
-	list_for_each_entry(intel_connector,			\
-			    &(dev)->mode_config.connector_list,	\
-			    base.head)
-
 #define for_each_intel_connector_iter(intel_connector, iter) \
 	while ((intel_connector = to_intel_connector(drm_connector_list_iter_next(iter))))
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 438d27f93aca..9715c64eeb08 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12661,8 +12661,10 @@ static const struct drm_crtc_helper_funcs intel_helper_funcs = {
 static void intel_modeset_update_connector_atomic_state(struct drm_device *dev)
 {
 	struct intel_connector *connector;
+	struct drm_connector_list_iter conn_iter;
 
-	for_each_intel_connector(dev, connector) {
+	drm_connector_list_iter_get(dev, &conn_iter);
+	for_each_intel_connector_iter(connector, &conn_iter) {
 		if (connector->base.state->crtc)
 			drm_connector_unreference(&connector->base);
 
@@ -12678,6 +12680,7 @@ static void intel_modeset_update_connector_atomic_state(struct drm_device *dev)
 			connector->base.state->crtc = NULL;
 		}
 	}
+	drm_connector_list_iter_put(&conn_iter);
 }
 
 static void
@@ -13606,10 +13609,12 @@ verify_connector_state(struct drm_device *dev,
 }
 
 static void
-verify_encoder_state(struct drm_device *dev)
+verify_encoder_state(struct drm_device *dev, struct drm_atomic_state *state)
 {
 	struct intel_encoder *encoder;
-	struct intel_connector *connector;
+	struct drm_connector *connector;
+	struct drm_connector_state *old_conn_state;
+	int i;
 
 	for_each_intel_encoder(dev, encoder) {
 		bool enabled = false;
@@ -13619,12 +13624,12 @@ verify_encoder_state(struct drm_device *dev)
 			      encoder->base.base.id,
 			      encoder->base.name);
 
-		for_each_intel_connector(dev, connector) {
-			if (connector->base.state->best_encoder != &encoder->base)
+		for_each_connector_in_state(state, connector, old_conn_state, i) {
+			if (connector->state->best_encoder != &encoder->base)
 				continue;
 			enabled = true;
 
-			I915_STATE_WARN(connector->base.state->crtc !=
+			I915_STATE_WARN(connector->state->crtc !=
 					encoder->base.crtc,
 			     "connector's crtc doesn't match encoder crtc\n");
 		}
@@ -13827,7 +13832,7 @@ static void
 intel_modeset_verify_disabled(struct drm_device *dev,
 			      struct drm_atomic_state *state)
 {
-	verify_encoder_state(dev);
+	verify_encoder_state(dev, state);
 	verify_connector_state(dev, state, NULL);
 	verify_disabled_dpll_state(dev);
 }
@@ -16638,6 +16643,7 @@ int intel_modeset_init(struct drm_device *dev)
 static void intel_enable_pipe_a(struct drm_device *dev)
 {
 	struct intel_connector *connector;
+	struct drm_connector_list_iter conn_iter;
 	struct drm_connector *crt = NULL;
 	struct intel_load_detect_pipe load_detect_temp;
 	struct drm_modeset_acquire_ctx *ctx = dev->mode_config.acquire_ctx;
@@ -16645,12 +16651,14 @@ static void intel_enable_pipe_a(struct drm_device *dev)
 	/* We can't just switch on the pipe A, we need to set things up with a
 	 * proper mode and output configuration. As a gross hack, enable pipe A
 	 * by enabling the load detect pipe once. */
-	for_each_intel_connector(dev, connector) {
+	drm_connector_list_iter_get(dev, &conn_iter);
+	for_each_intel_connector_iter(connector, &conn_iter) {
 		if (connector->encoder->type == INTEL_OUTPUT_ANALOG) {
 			crt = &connector->base;
 			break;
 		}
 	}
+	drm_connector_list_iter_put(&conn_iter);
 
 	if (!crt)
 		return;
@@ -16896,6 +16904,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 	struct intel_crtc *crtc;
 	struct intel_encoder *encoder;
 	struct intel_connector *connector;
+	struct drm_connector_list_iter conn_iter;
 	int i;
 
 	dev_priv->active_crtcs = 0;
@@ -16973,7 +16982,8 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 			      pipe_name(pipe));
 	}
 
-	for_each_intel_connector(dev, connector) {
+	drm_connector_list_iter_get(dev, &conn_iter);
+	for_each_intel_connector_iter(connector, &conn_iter) {
 		if (connector->get_hw_state(connector)) {
 			connector->base.dpms = DRM_MODE_DPMS_ON;
 
@@ -17001,6 +17011,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 			      connector->base.base.id, connector->base.name,
 			      enableddisabled(connector->base.encoder));
 	}
+	drm_connector_list_iter_put(&conn_iter);
 
 	for_each_intel_crtc(dev, crtc) {
 		crtc->base.hwmode = crtc->config->base.adjusted_mode;
-- 
2.11.0

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

✗ Fi.CI.BAT: warning for series starting with [1/6] drm/i915: Use drm_connector_list_iter in debugfs (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/6] drm/i915: Use drm_connector_list_iter in debugfs (rev2)
URL   : https://patchwork.freedesktop.org/series/16979/
State : warning

== Summary ==

Series 16979v2 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/16979/revisions/2/mbox/

Test drv_module_reload:
        Subgroup basic-reload:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-bxt-j4205)
        Subgroup basic-reload-inject:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-bxt-j4205)
Test kms_busy:
        Subgroup basic-flip-default-a:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-skl-6700k)
                pass       -> DMESG-WARN (fi-bxt-j4205)
        Subgroup basic-flip-default-b:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-skl-6700k)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-bxt-j4205)
        Subgroup basic-flip-default-c:
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-skl-6700k)
                pass       -> DMESG-WARN (fi-bxt-j4205)
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-legacy:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-skl-6700k)
                pass       -> DMESG-WARN (fi-bxt-j4205)
        Subgroup basic-busy-flip-before-cursor-varying-size:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-skl-6700k)
                pass       -> DMESG-WARN (fi-bxt-j4205)
        Subgroup basic-flip-after-cursor-legacy:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-skl-6700k)
                pass       -> DMESG-WARN (fi-bxt-j4205)
        Subgroup basic-flip-after-cursor-varying-size:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-skl-6700k)
                pass       -> DMESG-WARN (fi-bxt-j4205)
        Subgroup basic-flip-before-cursor-legacy:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-skl-6700k)
                pass       -> DMESG-WARN (fi-bxt-j4205)
        Subgroup basic-flip-before-cursor-varying-size:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-skl-6700k)
                pass       -> DMESG-WARN (fi-bxt-j4205)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-skl-6700k)
        Subgroup basic-flip-vs-modeset:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-skl-6700k)
                pass       -> DMESG-WARN (fi-skl-6770hq)
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-skl-6700k)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-bxt-j4205)
        Subgroup basic-plain-flip:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-skl-6700k)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-bxt-j4205)
Test kms_force_connector_basic:
        Subgroup force-connector-state:
                pass       -> DMESG-WARN (fi-snb-2520m)
        Subgroup force-load-detect:
                pass       -> DMESG-WARN (fi-snb-2520m)
Test kms_frontbuffer_tracking:
        Subgroup basic:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-skl-6700k)
                pass       -> DMESG-WARN (fi-bxt-j4205)
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-a:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-skl-6700k)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-bxt-j4205)
WARNING: Long output truncated

2a932d085375c80a1bbb332799db3df9738e8eba drm-tip: 2016y-12m-18d-16h-31m-05s UTC integration manifest
a71a34c drm/i915: Update comment that sets I915_MODE_FLAG_INHERITED
4d05b68 drm/i915: use for_each_intel_connector_iter in intel_display.c
6b4d44d drm/i915: Make intel_get_pipe_from_connector atomic
159abab drm/i915: use drm_connector_list_iter in intel_opregion.c
79bb10f drm/i915: use drm_connector_list_iter in intel_hotplug.c
8543b83 drm/i915: Use drm_connector_list_iter in debugfs

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3321/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: use for_each_intel_connector_iter in intel_display.c

[Daniel Vetter]

On Mon, Dec 19, 2016 at 09:58:28AM +0100, Daniel Vetter wrote:
> This gets rid of the last users of for_each_intel_connector(), remove
> that too.
> 
> The one exception is the loop in verify_encoder_state - that needs to
> switch over to for_each_connector_in_state.
> 
> v2: Maarten corrected me that in the state verifier we indeed must
> only walk connectors in the atomic commit, not all of them!

Ok, CI just told me this was a stupid idea. Since we don't walk all
connectors, but still walk all encoders there's not plenty of state
mismatches (e.g. if you have 2 crtc with different connectors enabled and
you're doing a modeset on just one).

Not exactly sure how to best fix this, since replacing the encoder
walking with a connnector walking and then dereferencing
connector->state->best_encoder to get at only the encoders relevant for us
defeats the point of the cross check.
-Daniel

> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  5 -----
>  drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++++---------
>  2 files changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 57914f008ed8..fe2b37fe0b91 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -488,11 +488,6 @@ struct i915_hotplug {
>  			    &(dev)->mode_config.encoder_list,	\
>  			    base.head)
>  
> -#define for_each_intel_connector(dev, intel_connector)		\
> -	list_for_each_entry(intel_connector,			\
> -			    &(dev)->mode_config.connector_list,	\
> -			    base.head)
> -
>  #define for_each_intel_connector_iter(intel_connector, iter) \
>  	while ((intel_connector = to_intel_connector(drm_connector_list_iter_next(iter))))
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 438d27f93aca..9715c64eeb08 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12661,8 +12661,10 @@ static const struct drm_crtc_helper_funcs intel_helper_funcs = {
>  static void intel_modeset_update_connector_atomic_state(struct drm_device *dev)
>  {
>  	struct intel_connector *connector;
> +	struct drm_connector_list_iter conn_iter;
>  
> -	for_each_intel_connector(dev, connector) {
> +	drm_connector_list_iter_get(dev, &conn_iter);
> +	for_each_intel_connector_iter(connector, &conn_iter) {
>  		if (connector->base.state->crtc)
>  			drm_connector_unreference(&connector->base);
>  
> @@ -12678,6 +12680,7 @@ static void intel_modeset_update_connector_atomic_state(struct drm_device *dev)
>  			connector->base.state->crtc = NULL;
>  		}
>  	}
> +	drm_connector_list_iter_put(&conn_iter);
>  }
>  
>  static void
> @@ -13606,10 +13609,12 @@ verify_connector_state(struct drm_device *dev,
>  }
>  
>  static void
> -verify_encoder_state(struct drm_device *dev)
> +verify_encoder_state(struct drm_device *dev, struct drm_atomic_state *state)
>  {
>  	struct intel_encoder *encoder;
> -	struct intel_connector *connector;
> +	struct drm_connector *connector;
> +	struct drm_connector_state *old_conn_state;
> +	int i;
>  
>  	for_each_intel_encoder(dev, encoder) {
>  		bool enabled = false;
> @@ -13619,12 +13624,12 @@ verify_encoder_state(struct drm_device *dev)
>  			      encoder->base.base.id,
>  			      encoder->base.name);
>  
> -		for_each_intel_connector(dev, connector) {
> -			if (connector->base.state->best_encoder != &encoder->base)
> +		for_each_connector_in_state(state, connector, old_conn_state, i) {
> +			if (connector->state->best_encoder != &encoder->base)
>  				continue;
>  			enabled = true;
>  
> -			I915_STATE_WARN(connector->base.state->crtc !=
> +			I915_STATE_WARN(connector->state->crtc !=
>  					encoder->base.crtc,
>  			     "connector's crtc doesn't match encoder crtc\n");
>  		}
> @@ -13827,7 +13832,7 @@ static void
>  intel_modeset_verify_disabled(struct drm_device *dev,
>  			      struct drm_atomic_state *state)
>  {
> -	verify_encoder_state(dev);
> +	verify_encoder_state(dev, state);
>  	verify_connector_state(dev, state, NULL);
>  	verify_disabled_dpll_state(dev);
>  }
> @@ -16638,6 +16643,7 @@ int intel_modeset_init(struct drm_device *dev)
>  static void intel_enable_pipe_a(struct drm_device *dev)
>  {
>  	struct intel_connector *connector;
> +	struct drm_connector_list_iter conn_iter;
>  	struct drm_connector *crt = NULL;
>  	struct intel_load_detect_pipe load_detect_temp;
>  	struct drm_modeset_acquire_ctx *ctx = dev->mode_config.acquire_ctx;
> @@ -16645,12 +16651,14 @@ static void intel_enable_pipe_a(struct drm_device *dev)
>  	/* We can't just switch on the pipe A, we need to set things up with a
>  	 * proper mode and output configuration. As a gross hack, enable pipe A
>  	 * by enabling the load detect pipe once. */
> -	for_each_intel_connector(dev, connector) {
> +	drm_connector_list_iter_get(dev, &conn_iter);
> +	for_each_intel_connector_iter(connector, &conn_iter) {
>  		if (connector->encoder->type == INTEL_OUTPUT_ANALOG) {
>  			crt = &connector->base;
>  			break;
>  		}
>  	}
> +	drm_connector_list_iter_put(&conn_iter);
>  
>  	if (!crt)
>  		return;
> @@ -16896,6 +16904,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  	struct intel_crtc *crtc;
>  	struct intel_encoder *encoder;
>  	struct intel_connector *connector;
> +	struct drm_connector_list_iter conn_iter;
>  	int i;
>  
>  	dev_priv->active_crtcs = 0;
> @@ -16973,7 +16982,8 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  			      pipe_name(pipe));
>  	}
>  
> -	for_each_intel_connector(dev, connector) {
> +	drm_connector_list_iter_get(dev, &conn_iter);
> +	for_each_intel_connector_iter(connector, &conn_iter) {
>  		if (connector->get_hw_state(connector)) {
>  			connector->base.dpms = DRM_MODE_DPMS_ON;
>  
> @@ -17001,6 +17011,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  			      connector->base.base.id, connector->base.name,
>  			      enableddisabled(connector->base.encoder));
>  	}
> +	drm_connector_list_iter_put(&conn_iter);
>  
>  	for_each_intel_crtc(dev, crtc) {
>  		crtc->base.hwmode = crtc->config->base.adjusted_mode;
> -- 
> 2.11.0
> 

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

Re: [PATCH] drm/i915: use for_each_intel_connector_iter in intel_display.c

[Maarten Lankhorst]

Op 19-12-16 om 10:22 schreef Daniel Vetter:
> On Mon, Dec 19, 2016 at 09:58:28AM +0100, Daniel Vetter wrote:
>> This gets rid of the last users of for_each_intel_connector(), remove
>> that too.
>>
>> The one exception is the loop in verify_encoder_state - that needs to
>> switch over to for_each_connector_in_state.
>>
>> v2: Maarten corrected me that in the state verifier we indeed must
>> only walk connectors in the atomic commit, not all of them!
> Ok, CI just told me this was a stupid idea. Since we don't walk all
> connectors, but still walk all encoders there's not plenty of state
> mismatches (e.g. if you have 2 crtc with different connectors enabled and
> you're doing a modeset on just one).
>
> Not exactly sure how to best fix this, since replacing the encoder
> walking with a connnector walking and then dereferencing
> connector->state->best_encoder to get at only the encoders relevant for us
> defeats the point of the cross check.
Assuming we trust the atomic state, we could fix this by iterating either state and ORing (old,new)_crtc_state->encoder_mask.
This way we'll have all encoders that we care about.

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

Re: [PATCH 1/6] drm/i915: Use drm_connector_list_iter in debugfs

[Maarten Lankhorst]

Op 19-12-16 om 09:24 schreef Daniel Vetter:
> While at it also try to reduce the locking a bit to what's really just
> needed instead of everything that we could possibly lock.
>
> Added a new for_each_intel_connector_iter which includes the cast to
> intel_connector.
>
> Otherwise just plain transformation with nothing special going on.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 62 ++++++++++++++++++++++++-------------
>  drivers/gpu/drm/i915/i915_drv.h     |  3 ++
>  2 files changed, 44 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 15deb2bc568b..f7633e8474b2 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2617,12 +2617,15 @@ static int i915_sink_crc(struct seq_file *m, void *data)
>  	struct drm_i915_private *dev_priv = node_to_i915(m->private);
>  	struct drm_device *dev = &dev_priv->drm;
>  	struct intel_connector *connector;
> +	struct drm_connector_list_iter conn_iter;
>  	struct intel_dp *intel_dp = NULL;
>  	int ret;
>  	u8 crc[6];
>  
> -	drm_modeset_lock_all(dev);
> -	for_each_intel_connector(dev, connector) {
> +	/* connection mutex also gives us a read lock on the crtc */
> +	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> +	drm_connector_list_iter_get(dev, &conn_iter);
> +	for_each_intel_connector_iter(connector, &conn_iter) {
>  		struct drm_crtc *crtc;
>  
>  		if (!connector->base.state->best_encoder)
> @@ -2648,7 +2651,8 @@ static int i915_sink_crc(struct seq_file *m, void *data)
Wrong, it's using crtc->state, which definitely requires the crtc mutex. crtc_state can update without connection_mutex.

Not the other way around though, acquiring connector_state also acquires the crtc_mutex, so if you have only a crtc lock you can iterate over crtc_state->connector_mask and dereference its state. In practice we still require the connection_mutex lock. :)
>  	}
>  	ret = -ENODEV;
>  out:
> -	drm_modeset_unlock_all(dev);
> +	drm_connector_list_iter_put(&conn_iter);
> +	drm_modeset_unlock(&dev->mode_config.connection_mutex);
>  	return ret;
>  }
>  
> @@ -3089,9 +3093,9 @@ static int i915_display_info(struct seq_file *m, void *unused)
>  	struct drm_device *dev = &dev_priv->drm;
>  	struct intel_crtc *crtc;
>  	struct drm_connector *connector;
> +	struct drm_connector_list_iter conn_iter;
>  
>  	intel_runtime_pm_get(dev_priv);
> -	drm_modeset_lock_all(dev);
>  	seq_printf(m, "CRTC info\n");
>  	seq_printf(m, "---------\n");
>  	for_each_intel_crtc(dev, crtc) {
> @@ -3099,6 +3103,7 @@ static int i915_display_info(struct seq_file *m, void *unused)
>  		struct intel_crtc_state *pipe_config;
>  		int x, y;
>  
> +		drm_modeset_lock(&crtc->base.mutex, NULL);
>  		pipe_config = to_intel_crtc_state(crtc->base.state);
>  
>  		seq_printf(m, "CRTC %d: pipe: %c, active=%s, (size=%dx%d), dither=%s, bpp=%d\n",
> @@ -3123,15 +3128,19 @@ static int i915_display_info(struct seq_file *m, void *unused)
>  		seq_printf(m, "\tunderrun reporting: cpu=%s pch=%s \n",
>  			   yesno(!crtc->cpu_fifo_underrun_disabled),
>  			   yesno(!crtc->pch_fifo_underrun_disabled));
> +		drm_modeset_unlock(&crtc->base.mutex);
>  	}
>  
>  	seq_printf(m, "\n");
>  	seq_printf(m, "Connector info\n");
>  	seq_printf(m, "--------------\n");
> -	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> +	mutex_lock(&dev->mode_config.mutex);
> +	drm_connector_list_iter_get(dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter)
>  		intel_connector_info(m, connector);
> -	}
> -	drm_modeset_unlock_all(dev);
> +	drm_connector_list_iter_put(&conn_iter);
> +	mutex_unlock(&dev->mode_config.mutex);
> +
>  	intel_runtime_pm_put(dev_priv);
>  
>  	return 0;
> @@ -3452,13 +3461,16 @@ static void drrs_status_per_crtc(struct seq_file *m,
>  	struct i915_drrs *drrs = &dev_priv->drrs;
>  	int vrefresh = 0;
>  	struct drm_connector *connector;
> +	struct drm_connector_list_iter conn_iter;
>  
> -	drm_for_each_connector(connector, dev) {
> +	drm_connector_list_iter_get(dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
>  		if (connector->state->crtc != &intel_crtc->base)
>  			continue;
>  
>  		seq_printf(m, "%s:\n", connector->name);
>  	}
> +	drm_connector_list_iter_put(&conn_iter);
>  
>  	if (dev_priv->vbt.drrs_type == STATIC_DRRS_SUPPORT)
>  		seq_puts(m, "\tVBT: DRRS_type: Static");
> @@ -3544,9 +3556,10 @@ static int i915_dp_mst_info(struct seq_file *m, void *unused)
>  	struct intel_encoder *intel_encoder;
>  	struct intel_digital_port *intel_dig_port;
>  	struct drm_connector *connector;
> +	struct drm_connector_list_iter conn_iter;
>  
> -	drm_modeset_lock_all(dev);
> -	drm_for_each_connector(connector, dev) {
> +	drm_connector_list_iter_get(dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
>  		if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort)
>  			continue;
>  
> @@ -3562,7 +3575,8 @@ static int i915_dp_mst_info(struct seq_file *m, void *unused)
>  			   port_name(intel_dig_port->port));
>  		drm_dp_mst_dump_topology(m, &intel_dig_port->dp.mst_mgr);
>  	}
> -	drm_modeset_unlock_all(dev);
> +	drm_connector_list_iter_put(&conn_iter);
> +
>  	return 0;
>  }
>  
> @@ -3574,14 +3588,12 @@ static ssize_t i915_displayport_test_active_write(struct file *file,
>  	int status = 0;
>  	struct drm_device *dev;
>  	struct drm_connector *connector;
> -	struct list_head *connector_list;
> +	struct drm_connector_list_iter conn_iter;
>  	struct intel_dp *intel_dp;
>  	int val = 0;
>  
>  	dev = ((struct seq_file *)file->private_data)->private;
>  
> -	connector_list = &dev->mode_config.connector_list;
> -
>  	if (len == 0)
>  		return 0;
>  
> @@ -3597,7 +3609,8 @@ static ssize_t i915_displayport_test_active_write(struct file *file,
>  	input_buffer[len] = '\0';
>  	DRM_DEBUG_DRIVER("Copied %d bytes from user\n", (unsigned int)len);
>  
> -	list_for_each_entry(connector, connector_list, head) {
> +	drm_connector_list_iter_get(dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
>  		if (connector->connector_type !=
>  		    DRM_MODE_CONNECTOR_DisplayPort)
>  			continue;
> @@ -3618,6 +3631,7 @@ static ssize_t i915_displayport_test_active_write(struct file *file,
>  				intel_dp->compliance.test_active = 0;
>  		}
>  	}
> +	drm_connector_list_iter_put(&conn_iter);
There's a goto out in this loop that should be converted to a break.
>  out:
>  	kfree(input_buffer);
>  	if (status < 0)
> @@ -3631,10 +3645,11 @@ static int i915_displayport_test_active_show(struct seq_file *m, void *data)
>  {
>  	struct drm_device *dev = m->private;
>  	struct drm_connector *connector;
> -	struct list_head *connector_list = &dev->mode_config.connector_list;
> +	struct drm_connector_list_iter conn_iter;
>  	struct intel_dp *intel_dp;
>  
> -	list_for_each_entry(connector, connector_list, head) {
> +	drm_connector_list_iter_get(dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
>  		if (connector->connector_type !=
>  		    DRM_MODE_CONNECTOR_DisplayPort)
>  			continue;
> @@ -3649,6 +3664,7 @@ static int i915_displayport_test_active_show(struct seq_file *m, void *data)
>  		} else
>  			seq_puts(m, "0");
>  	}
> +	drm_connector_list_iter_put(&conn_iter);
>  
>  	return 0;
>  }
> @@ -3675,10 +3691,11 @@ static int i915_displayport_test_data_show(struct seq_file *m, void *data)
>  {
>  	struct drm_device *dev = m->private;
>  	struct drm_connector *connector;
> -	struct list_head *connector_list = &dev->mode_config.connector_list;
> +	struct drm_connector_list_iter conn_iter;
>  	struct intel_dp *intel_dp;
>  
> -	list_for_each_entry(connector, connector_list, head) {
> +	drm_connector_list_iter_get(dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
>  		if (connector->connector_type !=
>  		    DRM_MODE_CONNECTOR_DisplayPort)
>  			continue;
> @@ -3690,6 +3707,7 @@ static int i915_displayport_test_data_show(struct seq_file *m, void *data)
>  		} else
>  			seq_puts(m, "0");
>  	}
> +	drm_connector_list_iter_put(&conn_iter);
>  
>  	return 0;
>  }
> @@ -3714,10 +3732,11 @@ static int i915_displayport_test_type_show(struct seq_file *m, void *data)
>  {
>  	struct drm_device *dev = m->private;
>  	struct drm_connector *connector;
> -	struct list_head *connector_list = &dev->mode_config.connector_list;
> +	struct drm_connector_list_iter conn_iter;
>  	struct intel_dp *intel_dp;
>  
> -	list_for_each_entry(connector, connector_list, head) {
> +	drm_connector_list_iter_get(dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
>  		if (connector->connector_type !=
>  		    DRM_MODE_CONNECTOR_DisplayPort)
>  			continue;
> @@ -3729,6 +3748,7 @@ static int i915_displayport_test_type_show(struct seq_file *m, void *data)
>  		} else
>  			seq_puts(m, "0");
>  	}
> +	drm_connector_list_iter_put(&conn_iter);
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6217f01d3c11..57914f008ed8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -493,6 +493,9 @@ struct i915_hotplug {
>  			    &(dev)->mode_config.connector_list,	\
>  			    base.head)
>  
> +#define for_each_intel_connector_iter(intel_connector, iter) \
> +	while ((intel_connector = to_intel_connector(drm_connector_list_iter_next(iter))))
> +
>  #define for_each_encoder_on_crtc(dev, __crtc, intel_encoder) \
>  	list_for_each_entry((intel_encoder), &(dev)->mode_config.encoder_list, base.head) \
>  		for_each_if ((intel_encoder)->base.crtc == (__crtc))

With those fixed

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

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

Re: [PATCH 2/6] drm/i915: use drm_connector_list_iter in intel_hotplug.c

[Maarten Lankhorst]

Op 19-12-16 om 09:24 schreef Daniel Vetter:
> Nothing special, just rote conversion.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_hotplug.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
> index 3d546c019de0..2ddc9e5842ec 100644
> --- a/drivers/gpu/drm/i915/intel_hotplug.c
> +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> @@ -145,16 +145,17 @@ static bool intel_hpd_irq_storm_detect(struct drm_i915_private *dev_priv,
>  static void intel_hpd_irq_storm_disable(struct drm_i915_private *dev_priv)
>  {
>  	struct drm_device *dev = &dev_priv->drm;
> -	struct drm_mode_config *mode_config = &dev->mode_config;
>  	struct intel_connector *intel_connector;
>  	struct intel_encoder *intel_encoder;
>  	struct drm_connector *connector;
> +	struct drm_connector_list_iter conn_iter;
>  	enum hpd_pin pin;
>  	bool hpd_disabled = false;
>  
>  	assert_spin_locked(&dev_priv->irq_lock);
>  
> -	list_for_each_entry(connector, &mode_config->connector_list, head) {
> +	drm_connector_list_iter_get(dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
>  		if (connector->polled != DRM_CONNECTOR_POLL_HPD)
>  			continue;
This is called with irqs disabled, will lockdep complain about inconsistent use of this lock with and without irqs?

If not,

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> @@ -177,6 +178,7 @@ static void intel_hpd_irq_storm_disable(struct drm_i915_private *dev_priv)
>  			| DRM_CONNECTOR_POLL_DISCONNECT;
>  		hpd_disabled = true;
>  	}
> +	drm_connector_list_iter_put(&conn_iter);
>  
>  	/* Enable polling and queue hotplug re-enabling. */
>  	if (hpd_disabled) {
> @@ -192,7 +194,6 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work)
>  		container_of(work, typeof(*dev_priv),
>  			     hotplug.reenable_work.work);
>  	struct drm_device *dev = &dev_priv->drm;
> -	struct drm_mode_config *mode_config = &dev->mode_config;
>  	int i;
>  
>  	intel_runtime_pm_get(dev_priv);
> @@ -200,13 +201,15 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work)
>  	spin_lock_irq(&dev_priv->irq_lock);
>  	for_each_hpd_pin(i) {
>  		struct drm_connector *connector;
> +		struct drm_connector_list_iter conn_iter;
>  
>  		if (dev_priv->hotplug.stats[i].state != HPD_DISABLED)
>  			continue;
>  
>  		dev_priv->hotplug.stats[i].state = HPD_ENABLED;
>  
> -		list_for_each_entry(connector, &mode_config->connector_list, head) {
> +		drm_connector_list_iter_get(dev, &conn_iter);
> +		drm_for_each_connector_iter(connector, &conn_iter) {
>  			struct intel_connector *intel_connector = to_intel_connector(connector);
>  
>  			if (intel_connector->encoder->hpd_pin == i) {
> @@ -218,6 +221,7 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work)
>  					connector->polled = DRM_CONNECTOR_POLL_HPD;
>  			}
>  		}
> +		drm_connector_list_iter_put(&conn_iter);
>  	}
>  	if (dev_priv->display.hpd_irq_setup)
>  		dev_priv->display.hpd_irq_setup(dev_priv);
> @@ -303,14 +307,14 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  	struct drm_i915_private *dev_priv =
>  		container_of(work, struct drm_i915_private, hotplug.hotplug_work);
>  	struct drm_device *dev = &dev_priv->drm;
> -	struct drm_mode_config *mode_config = &dev->mode_config;
>  	struct intel_connector *intel_connector;
>  	struct intel_encoder *intel_encoder;
>  	struct drm_connector *connector;
> +	struct drm_connector_list_iter conn_iter;
>  	bool changed = false;
>  	u32 hpd_event_bits;
>  
> -	mutex_lock(&mode_config->mutex);
> +	mutex_lock(&dev->mode_config.mutex);
>  	DRM_DEBUG_KMS("running encoder hotplug functions\n");
>  
>  	spin_lock_irq(&dev_priv->irq_lock);
> @@ -323,7 +327,8 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  
>  	spin_unlock_irq(&dev_priv->irq_lock);
>  
> -	list_for_each_entry(connector, &mode_config->connector_list, head) {
> +	drm_connector_list_iter_get(dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
>  		intel_connector = to_intel_connector(connector);
>  		if (!intel_connector->encoder)
>  			continue;
> @@ -337,7 +342,8 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  				changed = true;
>  		}
>  	}
> -	mutex_unlock(&mode_config->mutex);
> +	drm_connector_list_iter_put(&conn_iter);
> +	mutex_unlock(&dev->mode_config.mutex);
>  
>  	if (changed)
>  		drm_kms_helper_hotplug_event(dev);
> @@ -483,15 +489,16 @@ static void i915_hpd_poll_init_work(struct work_struct *work)
>  		container_of(work, struct drm_i915_private,
>  			     hotplug.poll_init_work);
>  	struct drm_device *dev = &dev_priv->drm;
> -	struct drm_mode_config *mode_config = &dev->mode_config;
>  	struct drm_connector *connector;
> +	struct drm_connector_list_iter conn_iter;
>  	bool enabled;
>  
>  	mutex_lock(&dev->mode_config.mutex);
>  
>  	enabled = READ_ONCE(dev_priv->hotplug.poll_enabled);
>  
> -	list_for_each_entry(connector, &mode_config->connector_list, head) {
> +	drm_connector_list_iter_get(dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
>  		struct intel_connector *intel_connector =
>  			to_intel_connector(connector);
>  		connector->polled = intel_connector->polled;
> @@ -509,6 +516,7 @@ static void i915_hpd_poll_init_work(struct work_struct *work)
>  				DRM_CONNECTOR_POLL_HPD;
>  		}
>  	}
> +	drm_connector_list_iter_put(&conn_iter);
>  
>  	if (enabled)
>  		drm_kms_helper_poll_enable_locked(dev);


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

Re: [PATCH 4/6] drm/i915: Make intel_get_pipe_from_connector atomic

[Maarten Lankhorst]

Op 19-12-16 om 09:24 schreef Daniel Vetter:
> Drive-by fixup while looking at all the connector_list walkers -
> holding connection_mutex does actually _not_ give you locking to look
> at the legacy drm_connector->encoder->crtc pointer chain. That one is
> solely owned by the atomic commit workers. Instead we must inspect the
> atomic state.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0b0d7e8be630..438d27f93aca 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15427,15 +15427,14 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
>  
>  enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
>  {
> -	struct drm_encoder *encoder = connector->base.encoder;
>  	struct drm_device *dev = connector->base.dev;
>  
>  	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>  
> -	if (!encoder || WARN_ON(!encoder->crtc))
> +	if (!connector->base.state->crtc)
>  		return INVALID_PIPE;
>  
> -	return to_intel_crtc(encoder->crtc)->pipe;
> +	return to_intel_crtc(connector->base.state->crtc)->pipe;
>  }
>  
>  int intel_get_pipe_from_crtc_id(struct drm_device *dev, void *data,

This patch clashes with the previous patch. intel_panel_set_backlight_acpi is called
from asle_set_backlight. You should probably keep connection_mutex alive for it.

~Maarten

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

Re: [PATCH 6/6] drm/i915: Update comment that sets I915_MODE_FLAG_INHERITED

[Maarten Lankhorst]

Op 19-12-16 om 09:24 schreef Daniel Vetter:
> The code was moved, but the comment not updated. It confused me.
>
> Fixes: 7f4c62840cc4 ("drm/i915: Assign hwmode after encoder state readout")
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e029dba96bca..c7d775f01b23 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -17028,17 +17028,9 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  			 * the atomic core happy. It wants a valid mode if the
>  			 * crtc's enabled, so we do the above call.
>  			 *
> -			 * At this point some state updated by the connectors
> -			 * in their ->detect() callback has not run yet, so
> -			 * no recalculation can be done yet.
> -			 *
> -			 * Even if we could do a recalculation and modeset
> -			 * right now it would cause a double modeset if
> -			 * fbdev or userspace chooses a different initial mode.
> -			 *
> -			 * If that happens, someone indicated they wanted a
> -			 * mode change, which means it's safe to do a full
> -			 * recalculation.
> +			 * But we don't set all the derived state fully, hence
> +			 * set a flag to indicate that a full recalculation is
> +			 * needed on the next commit.
>  			 */
>  			crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED;
>  

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

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

Re: [PATCH 4/6] drm/i915: Make intel_get_pipe_from_connector atomic

[Daniel Vetter]

On Tue, Dec 20, 2016 at 02:35:40PM +0100, Maarten Lankhorst wrote:
> Op 19-12-16 om 09:24 schreef Daniel Vetter:
> > Drive-by fixup while looking at all the connector_list walkers -
> > holding connection_mutex does actually _not_ give you locking to look
> > at the legacy drm_connector->encoder->crtc pointer chain. That one is
> > solely owned by the atomic commit workers. Instead we must inspect the
> > atomic state.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 0b0d7e8be630..438d27f93aca 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -15427,15 +15427,14 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
> >  
> >  enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
> >  {
> > -	struct drm_encoder *encoder = connector->base.encoder;
> >  	struct drm_device *dev = connector->base.dev;
> >  
> >  	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> >  
> > -	if (!encoder || WARN_ON(!encoder->crtc))
> > +	if (!connector->base.state->crtc)
> >  		return INVALID_PIPE;
> >  
> > -	return to_intel_crtc(encoder->crtc)->pipe;
> > +	return to_intel_crtc(connector->base.state->crtc)->pipe;
> >  }
> >  
> >  int intel_get_pipe_from_crtc_id(struct drm_device *dev, void *data,
> 
> This patch clashes with the previous patch. intel_panel_set_backlight_acpi is called
> from asle_set_backlight. You should probably keep connection_mutex alive for it.

Oh, I've forgotten to update the commit message for the previous patch - I
ended up _not_ nuking the locking because it did blow up here ;-)

r-b on both if I fix up the previous commit message that we can't nuke the
locking because the asle_set_backlight thing needs it for the backlight
callbacks?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/6] drm/i915: use drm_connector_list_iter in intel_opregion.c

[Daniel Vetter]

On Mon, Dec 19, 2016 at 09:24:20AM +0100, Daniel Vetter wrote:
> One case where I nuked a now unecessary locking, otherwise all just
> boring stuff.

Forgot to update the commit message:

"Note that we can't drop the connection_mutex in asle_set_backlight
because the backlight functions need that (they look at connector->state
to figure out the pipe). Otherwise all boring stuff."

I fixed the patch, but not the original commit message ;-)
-Daniel

> 
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_opregion.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index f4429f67a4e3..d586ff9a66ce 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -434,6 +434,7 @@ int intel_opregion_notify_adapter(struct drm_i915_private *dev_priv,
>  static u32 asle_set_backlight(struct drm_i915_private *dev_priv, u32 bclp)
>  {
>  	struct intel_connector *connector;
> +	struct drm_connector_list_iter conn_iter;
>  	struct opregion_asle *asle = dev_priv->opregion.asle;
>  	struct drm_device *dev = &dev_priv->drm;
>  
> @@ -458,8 +459,10 @@ static u32 asle_set_backlight(struct drm_i915_private *dev_priv, u32 bclp)
>  	 * only one).
>  	 */
>  	DRM_DEBUG_KMS("updating opregion backlight %d/255\n", bclp);
> -	for_each_intel_connector(dev, connector)
> +	drm_connector_list_iter_get(dev, &conn_iter);
> +	for_each_intel_connector_iter(connector, &conn_iter)
>  		intel_panel_set_backlight_acpi(connector, bclp, 255);
> +	drm_connector_list_iter_put(&conn_iter);
>  	asle->cblv = DIV_ROUND_UP(bclp * 100, 255) | ASLE_CBLV_VALID;
>  
>  	drm_modeset_unlock(&dev->mode_config.connection_mutex);
> @@ -701,6 +704,7 @@ static void intel_didl_outputs(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_opregion *opregion = &dev_priv->opregion;
>  	struct intel_connector *connector;
> +	struct drm_connector_list_iter conn_iter;
>  	int i = 0, max_outputs;
>  	int display_index[16] = {};
>  
> @@ -714,7 +718,8 @@ static void intel_didl_outputs(struct drm_i915_private *dev_priv)
>  	max_outputs = ARRAY_SIZE(opregion->acpi->didl) +
>  		ARRAY_SIZE(opregion->acpi->did2);
>  
> -	for_each_intel_connector(&dev_priv->drm, connector) {
> +	drm_connector_list_iter_get(&dev_priv->drm, &conn_iter);
> +	for_each_intel_connector_iter(connector, &conn_iter) {
>  		u32 device_id, type;
>  
>  		device_id = acpi_display_type(connector);
> @@ -729,6 +734,7 @@ static void intel_didl_outputs(struct drm_i915_private *dev_priv)
>  			set_did(opregion, i, device_id);
>  		i++;
>  	}
> +	drm_connector_list_iter_put(&conn_iter);
>  
>  	DRM_DEBUG_KMS("%d outputs detected\n", i);
>  
> @@ -745,6 +751,7 @@ static void intel_setup_cadls(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_opregion *opregion = &dev_priv->opregion;
>  	struct intel_connector *connector;
> +	struct drm_connector_list_iter conn_iter;
>  	int i = 0;
>  
>  	/*
> @@ -757,11 +764,13 @@ static void intel_setup_cadls(struct drm_i915_private *dev_priv)
>  	 * Note that internal panels should be at the front of the connector
>  	 * list already, ensuring they're not left out.
>  	 */
> -	for_each_intel_connector(&dev_priv->drm, connector) {
> +	drm_connector_list_iter_get(&dev_priv->drm, &conn_iter);
> +	for_each_intel_connector_iter(connector, &conn_iter) {
>  		if (i >= ARRAY_SIZE(opregion->acpi->cadl))
>  			break;
>  		opregion->acpi->cadl[i++] = connector->acpi_device_id;
>  	}
> +	drm_connector_list_iter_put(&conn_iter);
>  
>  	/* If fewer than 8 active devices, the list must be null terminated */
>  	if (i < ARRAY_SIZE(opregion->acpi->cadl))
> -- 
> 2.11.0
> 

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

[PATCH] drm/i915: Use drm_connector_list_iter in debugfs

[Daniel Vetter]

While at it also try to reduce the locking a bit to what's really just
needed instead of everything that we could possibly lock.

Added a new for_each_intel_connector_iter which includes the cast to
intel_connector.

Otherwise just plain transformation with nothing special going on.

v2: Review from Maarten:
- Stick with modeset_lock_all in sink_crc, it looks at crtc->state.
- Fix up early loop exit in i915_displayport_test_active_write.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 59 ++++++++++++++++++++++++-------------
 drivers/gpu/drm/i915/i915_drv.h     |  3 ++
 2 files changed, 42 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index a5552a1ca01f..800504fd617a 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2617,12 +2617,14 @@ static int i915_sink_crc(struct seq_file *m, void *data)
 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
 	struct drm_device *dev = &dev_priv->drm;
 	struct intel_connector *connector;
+	struct drm_connector_list_iter conn_iter;
 	struct intel_dp *intel_dp = NULL;
 	int ret;
 	u8 crc[6];
 
 	drm_modeset_lock_all(dev);
-	for_each_intel_connector(dev, connector) {
+	drm_connector_list_iter_get(dev, &conn_iter);
+	for_each_intel_connector_iter(connector, &conn_iter) {
 		struct drm_crtc *crtc;
 
 		if (!connector->base.state->best_encoder)
@@ -2648,6 +2650,7 @@ static int i915_sink_crc(struct seq_file *m, void *data)
 	}
 	ret = -ENODEV;
 out:
+	drm_connector_list_iter_put(&conn_iter);
 	drm_modeset_unlock_all(dev);
 	return ret;
 }
@@ -3089,9 +3092,9 @@ static int i915_display_info(struct seq_file *m, void *unused)
 	struct drm_device *dev = &dev_priv->drm;
 	struct intel_crtc *crtc;
 	struct drm_connector *connector;
+	struct drm_connector_list_iter conn_iter;
 
 	intel_runtime_pm_get(dev_priv);
-	drm_modeset_lock_all(dev);
 	seq_printf(m, "CRTC info\n");
 	seq_printf(m, "---------\n");
 	for_each_intel_crtc(dev, crtc) {
@@ -3099,6 +3102,7 @@ static int i915_display_info(struct seq_file *m, void *unused)
 		struct intel_crtc_state *pipe_config;
 		int x, y;
 
+		drm_modeset_lock(&crtc->base.mutex, NULL);
 		pipe_config = to_intel_crtc_state(crtc->base.state);
 
 		seq_printf(m, "CRTC %d: pipe: %c, active=%s, (size=%dx%d), dither=%s, bpp=%d\n",
@@ -3123,15 +3127,19 @@ static int i915_display_info(struct seq_file *m, void *unused)
 		seq_printf(m, "\tunderrun reporting: cpu=%s pch=%s \n",
 			   yesno(!crtc->cpu_fifo_underrun_disabled),
 			   yesno(!crtc->pch_fifo_underrun_disabled));
+		drm_modeset_unlock(&crtc->base.mutex);
 	}
 
 	seq_printf(m, "\n");
 	seq_printf(m, "Connector info\n");
 	seq_printf(m, "--------------\n");
-	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
+	mutex_lock(&dev->mode_config.mutex);
+	drm_connector_list_iter_get(dev, &conn_iter);
+	drm_for_each_connector_iter(connector, &conn_iter)
 		intel_connector_info(m, connector);
-	}
-	drm_modeset_unlock_all(dev);
+	drm_connector_list_iter_put(&conn_iter);
+	mutex_unlock(&dev->mode_config.mutex);
+
 	intel_runtime_pm_put(dev_priv);
 
 	return 0;
@@ -3452,13 +3460,16 @@ static void drrs_status_per_crtc(struct seq_file *m,
 	struct i915_drrs *drrs = &dev_priv->drrs;
 	int vrefresh = 0;
 	struct drm_connector *connector;
+	struct drm_connector_list_iter conn_iter;
 
-	drm_for_each_connector(connector, dev) {
+	drm_connector_list_iter_get(dev, &conn_iter);
+	drm_for_each_connector_iter(connector, &conn_iter) {
 		if (connector->state->crtc != &intel_crtc->base)
 			continue;
 
 		seq_printf(m, "%s:\n", connector->name);
 	}
+	drm_connector_list_iter_put(&conn_iter);
 
 	if (dev_priv->vbt.drrs_type == STATIC_DRRS_SUPPORT)
 		seq_puts(m, "\tVBT: DRRS_type: Static");
@@ -3544,9 +3555,10 @@ static int i915_dp_mst_info(struct seq_file *m, void *unused)
 	struct intel_encoder *intel_encoder;
 	struct intel_digital_port *intel_dig_port;
 	struct drm_connector *connector;
+	struct drm_connector_list_iter conn_iter;
 
-	drm_modeset_lock_all(dev);
-	drm_for_each_connector(connector, dev) {
+	drm_connector_list_iter_get(dev, &conn_iter);
+	drm_for_each_connector_iter(connector, &conn_iter) {
 		if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort)
 			continue;
 
@@ -3562,7 +3574,8 @@ static int i915_dp_mst_info(struct seq_file *m, void *unused)
 			   port_name(intel_dig_port->port));
 		drm_dp_mst_dump_topology(m, &intel_dig_port->dp.mst_mgr);
 	}
-	drm_modeset_unlock_all(dev);
+	drm_connector_list_iter_put(&conn_iter);
+
 	return 0;
 }
 
@@ -3574,14 +3587,12 @@ static ssize_t i915_displayport_test_active_write(struct file *file,
 	int status = 0;
 	struct drm_device *dev;
 	struct drm_connector *connector;
-	struct list_head *connector_list;
+	struct drm_connector_list_iter conn_iter;
 	struct intel_dp *intel_dp;
 	int val = 0;
 
 	dev = ((struct seq_file *)file->private_data)->private;
 
-	connector_list = &dev->mode_config.connector_list;
-
 	if (len == 0)
 		return 0;
 
@@ -3597,7 +3608,8 @@ static ssize_t i915_displayport_test_active_write(struct file *file,
 	input_buffer[len] = '\0';
 	DRM_DEBUG_DRIVER("Copied %d bytes from user\n", (unsigned int)len);
 
-	list_for_each_entry(connector, connector_list, head) {
+	drm_connector_list_iter_get(dev, &conn_iter);
+	drm_for_each_connector_iter(connector, &conn_iter) {
 		if (connector->connector_type !=
 		    DRM_MODE_CONNECTOR_DisplayPort)
 			continue;
@@ -3607,7 +3619,7 @@ static ssize_t i915_displayport_test_active_write(struct file *file,
 			intel_dp = enc_to_intel_dp(connector->encoder);
 			status = kstrtoint(input_buffer, 10, &val);
 			if (status < 0)
-				goto out;
+				break;
 			DRM_DEBUG_DRIVER("Got %d for test active\n", val);
 			/* To prevent erroneous activation of the compliance
 			 * testing code, only accept an actual value of 1 here
@@ -3618,6 +3630,7 @@ static ssize_t i915_displayport_test_active_write(struct file *file,
 				intel_dp->compliance.test_active = 0;
 		}
 	}
+	drm_connector_list_iter_put(&conn_iter);
 out:
 	kfree(input_buffer);
 	if (status < 0)
@@ -3631,10 +3644,11 @@ static int i915_displayport_test_active_show(struct seq_file *m, void *data)
 {
 	struct drm_device *dev = m->private;
 	struct drm_connector *connector;
-	struct list_head *connector_list = &dev->mode_config.connector_list;
+	struct drm_connector_list_iter conn_iter;
 	struct intel_dp *intel_dp;
 
-	list_for_each_entry(connector, connector_list, head) {
+	drm_connector_list_iter_get(dev, &conn_iter);
+	drm_for_each_connector_iter(connector, &conn_iter) {
 		if (connector->connector_type !=
 		    DRM_MODE_CONNECTOR_DisplayPort)
 			continue;
@@ -3649,6 +3663,7 @@ static int i915_displayport_test_active_show(struct seq_file *m, void *data)
 		} else
 			seq_puts(m, "0");
 	}
+	drm_connector_list_iter_put(&conn_iter);
 
 	return 0;
 }
@@ -3675,10 +3690,11 @@ static int i915_displayport_test_data_show(struct seq_file *m, void *data)
 {
 	struct drm_device *dev = m->private;
 	struct drm_connector *connector;
-	struct list_head *connector_list = &dev->mode_config.connector_list;
+	struct drm_connector_list_iter conn_iter;
 	struct intel_dp *intel_dp;
 
-	list_for_each_entry(connector, connector_list, head) {
+	drm_connector_list_iter_get(dev, &conn_iter);
+	drm_for_each_connector_iter(connector, &conn_iter) {
 		if (connector->connector_type !=
 		    DRM_MODE_CONNECTOR_DisplayPort)
 			continue;
@@ -3690,6 +3706,7 @@ static int i915_displayport_test_data_show(struct seq_file *m, void *data)
 		} else
 			seq_puts(m, "0");
 	}
+	drm_connector_list_iter_put(&conn_iter);
 
 	return 0;
 }
@@ -3714,10 +3731,11 @@ static int i915_displayport_test_type_show(struct seq_file *m, void *data)
 {
 	struct drm_device *dev = m->private;
 	struct drm_connector *connector;
-	struct list_head *connector_list = &dev->mode_config.connector_list;
+	struct drm_connector_list_iter conn_iter;
 	struct intel_dp *intel_dp;
 
-	list_for_each_entry(connector, connector_list, head) {
+	drm_connector_list_iter_get(dev, &conn_iter);
+	drm_for_each_connector_iter(connector, &conn_iter) {
 		if (connector->connector_type !=
 		    DRM_MODE_CONNECTOR_DisplayPort)
 			continue;
@@ -3729,6 +3747,7 @@ static int i915_displayport_test_type_show(struct seq_file *m, void *data)
 		} else
 			seq_puts(m, "0");
 	}
+	drm_connector_list_iter_put(&conn_iter);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 77d7a079c51b..db4121679d65 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -493,6 +493,9 @@ struct i915_hotplug {
 			    &(dev)->mode_config.connector_list,	\
 			    base.head)
 
+#define for_each_intel_connector_iter(intel_connector, iter) \
+	while ((intel_connector = to_intel_connector(drm_connector_list_iter_next(iter))))
+
 #define for_each_encoder_on_crtc(dev, __crtc, intel_encoder) \
 	list_for_each_entry((intel_encoder), &(dev)->mode_config.encoder_list, base.head) \
 		for_each_if ((intel_encoder)->base.crtc == (__crtc))
-- 
2.11.0

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

✗ Fi.CI.BAT: warning for series starting with drm/i915: Use drm_connector_list_iter in debugfs (rev3)

[Patchwork]

== Series Details ==

Series: series starting with drm/i915: Use drm_connector_list_iter in debugfs (rev3)
URL   : https://patchwork.freedesktop.org/series/16979/
State : warning

== Summary ==

Series 16979v3 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/16979/revisions/3/mbox/

Test drv_module_reload:
        Subgroup basic-reload:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-ilk-650)
        Subgroup basic-reload-inject:
                pass       -> DMESG-WARN (fi-snb-2520m)
                dmesg-warn -> PASS       (fi-skl-6260u)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-ilk-650)
Test kms_busy:
        Subgroup basic-flip-default-a:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-skl-6700k)
        Subgroup basic-flip-default-b:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-skl-6700k)
        Subgroup basic-flip-default-c:
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-skl-6700k)
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-legacy:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-skl-6700k)
        Subgroup basic-busy-flip-before-cursor-varying-size:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-skl-6700k)
        Subgroup basic-flip-after-cursor-legacy:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-skl-6700k)
        Subgroup basic-flip-after-cursor-varying-size:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-skl-6700k)
        Subgroup basic-flip-before-cursor-legacy:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-skl-6700k)
        Subgroup basic-flip-before-cursor-varying-size:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-skl-6700k)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-skl-6700k)
        Subgroup basic-flip-vs-modeset:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-ivb-3520m)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-skl-6700k)
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-skl-6700k)
        Subgroup basic-plain-flip:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-skl-6700k)
Test kms_force_connector_basic:
        Subgroup force-connector-state:
                pass       -> DMESG-WARN (fi-snb-2520m)
        Subgroup force-load-detect:
                pass       -> DMESG-WARN (fi-snb-2520m)
Test kms_frontbuffer_tracking:
        Subgroup basic:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-skl-6700k)
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-a:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-skl-6700k)
        Subgroup hang-read-crc-pipe-b:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-skl-6700k)
        Subgroup hang-read-crc-pipe-c:
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-skl-6700k)
        Subgroup nonblocking-crc-pipe-a:
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-ilk-650)
WARNING: Long output truncated
fi-bxt-j4205 failed to connect after reboot

7285204135ffcf095bea64196db6ef900d47e048 drm-tip: 2016y-12m-20d-14h-50m-18s UTC integration manifest
ac8557f drm/i915: Update comment that sets I915_MODE_FLAG_INHERITED
672a3b2 drm/i915: use for_each_intel_connector_iter in intel_display.c
4afa415 drm/i915: Make intel_get_pipe_from_connector atomic
e0f5f9e drm/i915: use drm_connector_list_iter in intel_opregion.c
1ae5c85 drm/i915: use drm_connector_list_iter in intel_hotplug.c
8e7c486 drm/i915: Use drm_connector_list_iter in debugfs

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3343/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: use for_each_intel_connector_iter in intel_display.c

[Daniel Vetter]

On Mon, Dec 19, 2016 at 10:47:25AM +0100, Maarten Lankhorst wrote:
> Op 19-12-16 om 10:22 schreef Daniel Vetter:
> > On Mon, Dec 19, 2016 at 09:58:28AM +0100, Daniel Vetter wrote:
> >> This gets rid of the last users of for_each_intel_connector(), remove
> >> that too.
> >>
> >> The one exception is the loop in verify_encoder_state - that needs to
> >> switch over to for_each_connector_in_state.
> >>
> >> v2: Maarten corrected me that in the state verifier we indeed must
> >> only walk connectors in the atomic commit, not all of them!
> > Ok, CI just told me this was a stupid idea. Since we don't walk all
> > connectors, but still walk all encoders there's not plenty of state
> > mismatches (e.g. if you have 2 crtc with different connectors enabled and
> > you're doing a modeset on just one).
> >
> > Not exactly sure how to best fix this, since replacing the encoder
> > walking with a connnector walking and then dereferencing
> > connector->state->best_encoder to get at only the encoders relevant for us
> > defeats the point of the cross check.
> Assuming we trust the atomic state, we could fix this by iterating either state and ORing (old,new)_crtc_state->encoder_mask.
> This way we'll have all encoders that we care about.

Yeah, I think we have to rely on the correctness of the atomic states. For
merging I think it's better to get v1 in to correct the connector_list
enumeration, then fix up the encoder checking in a separate patch on top.
Does that sound good?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: use for_each_intel_connector_iter in intel_display.c

[Maarten Lankhorst]

Op 20-12-16 om 19:29 schreef Daniel Vetter:
> On Mon, Dec 19, 2016 at 10:47:25AM +0100, Maarten Lankhorst wrote:
>> Op 19-12-16 om 10:22 schreef Daniel Vetter:
>>> On Mon, Dec 19, 2016 at 09:58:28AM +0100, Daniel Vetter wrote:
>>>> This gets rid of the last users of for_each_intel_connector(), remove
>>>> that too.
>>>>
>>>> The one exception is the loop in verify_encoder_state - that needs to
>>>> switch over to for_each_connector_in_state.
>>>>
>>>> v2: Maarten corrected me that in the state verifier we indeed must
>>>> only walk connectors in the atomic commit, not all of them!
>>> Ok, CI just told me this was a stupid idea. Since we don't walk all
>>> connectors, but still walk all encoders there's not plenty of state
>>> mismatches (e.g. if you have 2 crtc with different connectors enabled and
>>> you're doing a modeset on just one).
>>>
>>> Not exactly sure how to best fix this, since replacing the encoder
>>> walking with a connnector walking and then dereferencing
>>> connector->state->best_encoder to get at only the encoders relevant for us
>>> defeats the point of the cross check.
>> Assuming we trust the atomic state, we could fix this by iterating either state and ORing (old,new)_crtc_state->encoder_mask.
>> This way we'll have all encoders that we care about.
> Yeah, I think we have to rely on the correctness of the atomic states. For
> merging I think it's better to get v1 in to correct the connector_list
> enumeration, then fix up the encoder checking in a separate patch on top.
> Does that sound good?
Yes sounds good.

~Maarten


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

Re: [PATCH 6/6] drm/i915: Update comment that sets I915_MODE_FLAG_INHERITED

[Daniel Vetter]

On Tue, Dec 20, 2016 at 02:38:55PM +0100, Maarten Lankhorst wrote:
> Op 19-12-16 om 09:24 schreef Daniel Vetter:
> > The code was moved, but the comment not updated. It confused me.
> >
> > Fixes: 7f4c62840cc4 ("drm/i915: Assign hwmode after encoder state readout")
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 14 +++-----------
> >  1 file changed, 3 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index e029dba96bca..c7d775f01b23 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -17028,17 +17028,9 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> >  			 * the atomic core happy. It wants a valid mode if the
> >  			 * crtc's enabled, so we do the above call.
> >  			 *
> > -			 * At this point some state updated by the connectors
> > -			 * in their ->detect() callback has not run yet, so
> > -			 * no recalculation can be done yet.
> > -			 *
> > -			 * Even if we could do a recalculation and modeset
> > -			 * right now it would cause a double modeset if
> > -			 * fbdev or userspace chooses a different initial mode.
> > -			 *
> > -			 * If that happens, someone indicated they wanted a
> > -			 * mode change, which means it's safe to do a full
> > -			 * recalculation.
> > +			 * But we don't set all the derived state fully, hence
> > +			 * set a flag to indicate that a full recalculation is
> > +			 * needed on the next commit.
> >  			 */
> >  			crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED;
> >  
> 
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Applied this one to dinq. Ping about patches 3-5 where there's still
questions from you open, but I think I answered them all.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/6] drm/i915: Make intel_get_pipe_from_connector atomic

[Maarten Lankhorst]

Op 20-12-16 om 14:49 schreef Daniel Vetter:
> On Tue, Dec 20, 2016 at 02:35:40PM +0100, Maarten Lankhorst wrote:
>> Op 19-12-16 om 09:24 schreef Daniel Vetter:
>>> Drive-by fixup while looking at all the connector_list walkers -
>>> holding connection_mutex does actually _not_ give you locking to look
>>> at the legacy drm_connector->encoder->crtc pointer chain. That one is
>>> solely owned by the atomic commit workers. Instead we must inspect the
>>> atomic state.
>>>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_display.c | 5 ++---
>>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index 0b0d7e8be630..438d27f93aca 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -15427,15 +15427,14 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
>>>  
>>>  enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
>>>  {
>>> -	struct drm_encoder *encoder = connector->base.encoder;
>>>  	struct drm_device *dev = connector->base.dev;
>>>  
>>>  	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>>>  
>>> -	if (!encoder || WARN_ON(!encoder->crtc))
>>> +	if (!connector->base.state->crtc)
>>>  		return INVALID_PIPE;
>>>  
>>> -	return to_intel_crtc(encoder->crtc)->pipe;
>>> +	return to_intel_crtc(connector->base.state->crtc)->pipe;
>>>  }
>>>  
>>>  int intel_get_pipe_from_crtc_id(struct drm_device *dev, void *data,
>> This patch clashes with the previous patch. intel_panel_set_backlight_acpi is called
>> from asle_set_backlight. You should probably keep connection_mutex alive for it.
> Oh, I've forgotten to update the commit message for the previous patch - I
> ended up _not_ nuking the locking because it did blow up here ;-)
>
> r-b on both if I fix up the previous commit message that we can't nuke the
> locking because the asle_set_backlight thing needs it for the backlight
> callbacks?
> -Daniel

Yeah ok. I'd rather we pass the state, but this will do for now.


Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

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

[PATCH 4/6] drm/i915: Make intel_get_pipe_from_connector atomic

[Daniel Vetter]

Drive-by fixup while looking at all the connector_list walkers -
holding connection_mutex does actually _not_ give you locking to look
at the legacy drm_connector->encoder->crtc pointer chain. That one is
solely owned by the atomic commit workers. Instead we must inspect the
atomic state.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7673d5d9e74a..0f30362b4ad8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13875,15 +13875,14 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
 
 enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
 {
-	struct drm_encoder *encoder = connector->base.encoder;
 	struct drm_device *dev = connector->base.dev;
 
 	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
-	if (!encoder || WARN_ON(!encoder->crtc))
+	if (!connector->base.state->crtc)
 		return INVALID_PIPE;
 
-	return to_intel_crtc(encoder->crtc)->pipe;
+	return to_intel_crtc(connector->base.state->crtc)->pipe;
 }
 
 int intel_get_pipe_from_crtc_id(struct drm_device *dev, void *data,
-- 
2.11.0

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

[PATCH 4/6] drm/i915: Make intel_get_pipe_from_connector atomic

[Daniel Vetter]

Drive-by fixup while looking at all the connector_list walkers -
holding connection_mutex does actually _not_ give you locking to look
at the legacy drm_connector->encoder->crtc pointer chain. That one is
solely owned by the atomic commit workers. Instead we must inspect the
atomic state.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7673d5d9e74a..0f30362b4ad8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13875,15 +13875,14 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
 
 enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
 {
-	struct drm_encoder *encoder = connector->base.encoder;
 	struct drm_device *dev = connector->base.dev;
 
 	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
-	if (!encoder || WARN_ON(!encoder->crtc))
+	if (!connector->base.state->crtc)
 		return INVALID_PIPE;
 
-	return to_intel_crtc(encoder->crtc)->pipe;
+	return to_intel_crtc(connector->base.state->crtc)->pipe;
 }
 
 int intel_get_pipe_from_crtc_id(struct drm_device *dev, void *data,
-- 
2.11.0

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