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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
