Re: [PATCH 1/2] drm/i915: Remove crtc->config usage from intel_modeset_readout_hw_state()

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Dec 22, 2016 at 04:04:41PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> crtc->config is on its way out. Let's reduce our dependence on it a
> little bit by removing it from intel_modeset_readout_hw_state().
> 
> Also replace crtc->acttive checks with crtc_state->base.active checks.
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Suggested-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Idling musing: I wonder whether we should convert the readout code to use
an entirely free-standing atomic static tracked in drm_atomic_commit. And
then apply it with a directly call to swap_state. That way we could reuse
all the neat iterators.

Not sure that's a good idea though.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 38 +++++++++++++++++++++++-------------
>  1 file changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d8effd4da034..091c192c6c5c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -17032,7 +17032,8 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  	dev_priv->active_crtcs = 0;
>  
>  	for_each_intel_crtc(dev, crtc) {
> -		struct intel_crtc_state *crtc_state = crtc->config;
> +		struct intel_crtc_state *crtc_state =
> +			to_intel_crtc_state(crtc->base.state);
>  
>  		__drm_atomic_helper_crtc_destroy_state(&crtc_state->base);
>  		memset(crtc_state, 0, sizeof(*crtc_state));
> @@ -17051,7 +17052,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  
>  		DRM_DEBUG_KMS("[CRTC:%d:%s] hw state readout: %s\n",
>  			      crtc->base.base.id, crtc->base.name,
> -			      enableddisabled(crtc->active));
> +			      enableddisabled(crtc_state->base.active));
>  	}
>  
>  	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
> @@ -17061,7 +17062,11 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  						  &pll->config.hw_state);
>  		pll->config.crtc_mask = 0;
>  		for_each_intel_crtc(dev, crtc) {
> -			if (crtc->active && crtc->config->shared_dpll == pll)
> +			struct intel_crtc_state *crtc_state =
> +				to_intel_crtc_state(crtc->base.state);
> +
> +			if (crtc_state->base.active &&
> +			    crtc_state->shared_dpll == pll)
>  				pll->config.crtc_mask |= 1 << crtc->pipe;
>  		}
>  		pll->active_mask = pll->config.crtc_mask;
> @@ -17074,11 +17079,14 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  		pipe = 0;
>  
>  		if (encoder->get_hw_state(encoder, &pipe)) {
> +			struct intel_crtc_state *crtc_state;
> +
>  			crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> +			crtc_state = to_intel_crtc_state(crtc->base.state);
>  
>  			encoder->base.crtc = &crtc->base;
> -			crtc->config->output_types |= 1 << encoder->type;
> -			encoder->get_config(encoder, crtc->config);
> +			crtc_state->output_types |= 1 << encoder->type;
> +			encoder->get_config(encoder, crtc_state);
>  		} else {
>  			encoder->base.crtc = NULL;
>  		}
> @@ -17119,14 +17127,16 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  	}
>  
>  	for_each_intel_crtc(dev, crtc) {
> +		struct intel_crtc_state *crtc_state =
> +			to_intel_crtc_state(crtc->base.state);
>  		int pixclk = 0;
>  
> -		crtc->base.hwmode = crtc->config->base.adjusted_mode;
> +		crtc->base.hwmode = crtc_state->base.adjusted_mode;
>  
>  		memset(&crtc->base.mode, 0, sizeof(crtc->base.mode));
> -		if (crtc->base.state->active) {
> -			intel_mode_from_pipe_config(&crtc->base.mode, crtc->config);
> -			intel_mode_from_pipe_config(&crtc->base.state->adjusted_mode, crtc->config);
> +		if (crtc_state->base.active) {
> +			intel_mode_from_pipe_config(&crtc->base.mode, crtc_state);
> +			intel_mode_from_pipe_config(&crtc_state->base.adjusted_mode, crtc_state);
>  			WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, &crtc->base.mode));
>  
>  			/*
> @@ -17146,17 +17156,17 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  			 * mode change, which means it's safe to do a full
>  			 * recalculation.
>  			 */
> -			crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED;
> +			crtc_state->base.mode.private_flags = I915_MODE_FLAG_INHERITED;
>  
>  			if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> -				pixclk = ilk_pipe_pixel_rate(crtc->config);
> +				pixclk = ilk_pipe_pixel_rate(crtc_state);
>  			else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> -				pixclk = crtc->config->base.adjusted_mode.crtc_clock;
> +				pixclk = crtc_state->base.adjusted_mode.crtc_clock;
>  			else
>  				WARN_ON(dev_priv->display.modeset_calc_cdclk);
>  
>  			/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> -			if (IS_BROADWELL(dev_priv) && crtc->config->ips_enabled)
> +			if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
>  				pixclk = DIV_ROUND_UP(pixclk * 100, 95);
>  
>  			drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
> @@ -17165,7 +17175,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  
>  		dev_priv->min_pixclk[crtc->pipe] = pixclk;
>  
> -		intel_pipe_config_sanity_check(dev_priv, crtc->config);
> +		intel_pipe_config_sanity_check(dev_priv, crtc_state);
>  	}
>  }
>  
> -- 
> 2.10.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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