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

Daniel Vetter daniel at ffwll.ch
Mon Dec 19 09:22:19 UTC 2016


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 at linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter at 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


More information about the Intel-gfx mailing list