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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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