[Intel-gfx] [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.

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.

v3: Rebase onto the iter_get/put->iter_begin/end rename.

Cc: Thierry Reding 
Cc: Maarten Lankhorst 
Reviewed-by: Maarten Lankhorst  (v2)
Signed-off-by: Daniel Vetter 
---
 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 1a28b5279bec..7449458131aa 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2730,12 +2730,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 = _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_begin(dev, _iter);
+   for_each_intel_connector_iter(connector, _iter) {
struct drm_crtc *crtc;
 
if (!connector->base.state->best_encoder)
@@ -2761,6 +2763,7 @@ static int i915_sink_crc(struct seq_file *m, void *data)
}
ret = -ENODEV;
 out:
+   drm_connector_list_iter_end(_iter);
drm_modeset_unlock_all(dev);
return ret;
 }
@@ -3197,9 +3200,9 @@ static int i915_display_info(struct seq_file *m, void 
*unused)
struct drm_device *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) {
@@ -3207,6 +3210,7 @@ static int i915_display_info(struct seq_file *m, void 
*unused)
struct intel_crtc_state *pipe_config;
int x, y;
 
+   drm_modeset_lock(>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",
@@ -3231,15 +3235,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(>base.mutex);
}
 
seq_printf(m, "\n");
seq_printf(m, "Connector info\n");
seq_printf(m, "--\n");
-   list_for_each_entry(connector, >mode_config.connector_list, head) {
+   mutex_lock(>mode_config.mutex);
+   drm_connector_list_iter_begin(dev, _iter);
+   drm_for_each_connector_iter(connector, _iter)
intel_connector_info(m, connector);
-   }
-   drm_modeset_unlock_all(dev);
+   drm_connector_list_iter_end(_iter);
+   mutex_unlock(>mode_config.mutex);
+
intel_runtime_pm_put(dev_priv);
 
return 0;
@@ -3566,13 +3574,16 @@ static void drrs_status_per_crtc(struct seq_file *m,
struct i915_drrs *drrs = _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_begin(dev, _iter);
+   drm_for_each_connector_iter(connector, _iter) {
if (connector->state->crtc != _crtc->base)
continue;
 
seq_printf(m, "%s:\n", connector->name);
}
+   drm_connector_list_iter_end(_iter);
 
if (dev_priv->vbt.drrs_type == STATIC_DRRS_SUPPORT)
seq_puts(m, "\tVBT: DRRS_type: Static");
@@ -3658,9 +3669,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_begin(dev, _iter);
+   drm_for_each_connector_iter(connector, _iter) {

[Intel-gfx] [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.

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 
Reviewed-by: Maarten Lankhorst 
Signed-off-by: Daniel Vetter 
---
 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 1a28b5279bec..4ae30d6de036 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2730,12 +2730,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 = _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, _iter);
+   for_each_intel_connector_iter(connector, _iter) {
struct drm_crtc *crtc;
 
if (!connector->base.state->best_encoder)
@@ -2761,6 +2763,7 @@ static int i915_sink_crc(struct seq_file *m, void *data)
}
ret = -ENODEV;
 out:
+   drm_connector_list_iter_put(_iter);
drm_modeset_unlock_all(dev);
return ret;
 }
@@ -3197,9 +3200,9 @@ static int i915_display_info(struct seq_file *m, void 
*unused)
struct drm_device *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) {
@@ -3207,6 +3210,7 @@ static int i915_display_info(struct seq_file *m, void 
*unused)
struct intel_crtc_state *pipe_config;
int x, y;
 
+   drm_modeset_lock(>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",
@@ -3231,15 +3235,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(>base.mutex);
}
 
seq_printf(m, "\n");
seq_printf(m, "Connector info\n");
seq_printf(m, "--\n");
-   list_for_each_entry(connector, >mode_config.connector_list, head) {
+   mutex_lock(>mode_config.mutex);
+   drm_connector_list_iter_get(dev, _iter);
+   drm_for_each_connector_iter(connector, _iter)
intel_connector_info(m, connector);
-   }
-   drm_modeset_unlock_all(dev);
+   drm_connector_list_iter_put(_iter);
+   mutex_unlock(>mode_config.mutex);
+
intel_runtime_pm_put(dev_priv);
 
return 0;
@@ -3566,13 +3574,16 @@ static void drrs_status_per_crtc(struct seq_file *m,
struct i915_drrs *drrs = _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, _iter);
+   drm_for_each_connector_iter(connector, _iter) {
if (connector->state->crtc != _crtc->base)
continue;
 
seq_printf(m, "%s:\n", connector->name);
}
+   drm_connector_list_iter_put(_iter);
 
if (dev_priv->vbt.drrs_type == STATIC_DRRS_SUPPORT)
seq_puts(m, "\tVBT: DRRS_type: Static");
@@ -3658,9 +3669,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, _iter);
+   drm_for_each_connector_iter(connector, _iter) {
if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort)
continue;
 
@@ -3676,7 

Re: [Intel-gfx] [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 
> ---
>  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 = _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(>mode_config.connection_mutex, NULL);
> + drm_connector_list_iter_get(dev, _iter);
> + for_each_intel_connector_iter(connector, _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(_iter);
> + drm_modeset_unlock(>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 = _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(>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(>base.mutex);
>   }
>  
>   seq_printf(m, "\n");
>   seq_printf(m, "Connector info\n");
>   seq_printf(m, "--\n");
> - list_for_each_entry(connector, >mode_config.connector_list, head) {
> + mutex_lock(>mode_config.mutex);
> + drm_connector_list_iter_get(dev, _iter);
> + drm_for_each_connector_iter(connector, _iter)
>   intel_connector_info(m, connector);
> - }
> - drm_modeset_unlock_all(dev);
> + drm_connector_list_iter_put(_iter);
> + mutex_unlock(>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 = _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, _iter);
> + drm_for_each_connector_iter(connector, _iter) {
>   if (connector->state->crtc != _crtc->base)
>   continue;
>  
>   seq_printf(m, "%s:\n", connector->name);
>   }
> + drm_connector_list_iter_put(_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-gfx] [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 
---
 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 = _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(>mode_config.connection_mutex, NULL);
+   drm_connector_list_iter_get(dev, _iter);
+   for_each_intel_connector_iter(connector, _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(_iter);
+   drm_modeset_unlock(>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 = _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(>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(>base.mutex);
}
 
seq_printf(m, "\n");
seq_printf(m, "Connector info\n");
seq_printf(m, "--\n");
-   list_for_each_entry(connector, >mode_config.connector_list, head) {
+   mutex_lock(>mode_config.mutex);
+   drm_connector_list_iter_get(dev, _iter);
+   drm_for_each_connector_iter(connector, _iter)
intel_connector_info(m, connector);
-   }
-   drm_modeset_unlock_all(dev);
+   drm_connector_list_iter_put(_iter);
+   mutex_unlock(>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 = _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, _iter);
+   drm_for_each_connector_iter(connector, _iter) {
if (connector->state->crtc != _crtc->base)
continue;
 
seq_printf(m, "%s:\n", connector->name);
}
+   drm_connector_list_iter_put(_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, _iter);
+   drm_for_each_connector_iter(connector, _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)