Re: [PATCH 34/42] drm/i915: get rid of crtc->config in intel_display.c, part 1

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

Op 12-05-15 om 12:11 schreef Daniel Vetter:
> On Mon, May 11, 2015 at 04:25:10PM +0200, Maarten Lankhorst wrote:
>> Removed some occurences, roughly based on where the errors of
>> removing crtc->config occured. Because it's used a lot in this
>> file the changes are done in passes.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 205 ++++++++++++++++++-----------------
>>  drivers/gpu/drm/i915/intel_drv.h     |   4 +-
>>  drivers/gpu/drm/i915/intel_lvds.c    |   2 +-
>>  3 files changed, 105 insertions(+), 106 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 004067bd0b6c..fb2ecb65aaaa 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -1141,29 +1141,20 @@ static void assert_dsi_pll(struct drm_i915_private *dev_priv, bool state)
>>  #define assert_dsi_pll_enabled(d) assert_dsi_pll(d, true)
>>  #define assert_dsi_pll_disabled(d) assert_dsi_pll(d, false)
>>  
>> -struct intel_shared_dpll *
>> -intel_crtc_to_shared_dpll(struct intel_crtc *crtc)
>> -{
>> -	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>> -
>> -	if (crtc->config->shared_dpll < 0)
>> -		return NULL;
>> -
>> -	return &dev_priv->shared_dplls[crtc->config->shared_dpll];
>> -}
>> -
>>  /* For ILK+ */
>>  void assert_shared_dpll(struct drm_i915_private *dev_priv,
>> -			struct intel_shared_dpll *pll,
>> +			enum intel_dpll_id shared_dpll,
>>  			bool state)
>>  {
>> -	bool cur_state;
>>  	struct intel_dpll_hw_state hw_state;
>> +	struct intel_shared_dpll *pll;
>> +	bool cur_state;
>>  
>> -	if (WARN (!pll,
>> +	if (WARN(shared_dpll < 0,
>>  		  "asserting DPLL %s with no DPLL\n", state_string(state)))
>>  		return;
>>  
>> +	pll = &dev_priv->shared_dplls[shared_dpll];
>>  	cur_state = pll->get_hw_state(dev_priv, pll, &hw_state);
>>  	I915_STATE_WARN(cur_state != state,
>>  	     "%s assertion failure (expected %s, current %s)\n",
>> @@ -1691,7 +1682,7 @@ static void i9xx_enable_pll(struct intel_crtc *crtc, struct intel_crtc_state *pi
>>  	struct drm_device *dev = crtc->base.dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	int reg = DPLL(crtc->pipe);
>> -	u32 dpll = crtc->config->dpll_hw_state.dpll;
>> +	u32 dpll = pipe_config->dpll_hw_state.dpll;
> Lots of your patches are sprinkled with unrelated crtc->config ->
> pipe_config conversions. Or just plain rolling out of a local variable
> instead of accessing some other pointer. That makes it fairly hard to
> review them for a given topic (like shared dpll here) since it's never
> clear whether it really includes everything, and what's the reason for all
> the other hunks.
>
>>  
>>  	assert_pipe_disabled(dev_priv, pipe_config->cpu_transcoder, crtc->pipe);
>>  
>> @@ -1721,7 +1712,7 @@ static void i9xx_enable_pll(struct intel_crtc *crtc, struct intel_crtc_state *pi
>>  
>>  	if (INTEL_INFO(dev)->gen >= 4) {
>>  		I915_WRITE(DPLL_MD(crtc->pipe),
>> -			   crtc->config->dpll_hw_state.dpll_md);
>> +			   pipe_config->dpll_hw_state.dpll_md);
>>  	} else {
>>  		/* The pixel multiplier can only be updated once the
>>  		 * DPLL is enabled and the clocks are stable.
>> @@ -1859,20 +1850,19 @@ void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
>>  		     port_name(dport->port), I915_READ(dpll_reg) & port_mask, expected_mask);
>>  }
>>  
>> -static void intel_prepare_shared_dpll(struct intel_crtc *crtc)
>> +static void intel_prepare_shared_dpll(struct drm_i915_private *dev_priv,
>> +				      enum intel_dpll_id shared_dpll)
>>  {
>> -	struct drm_device *dev = crtc->base.dev;
>> -	struct drm_i915_private *dev_priv = dev->dev_private;
>> -	struct intel_shared_dpll *pll = intel_crtc_to_shared_dpll(crtc);
>> +	struct intel_shared_dpll *pll = &dev_priv->shared_dplls[shared_dpll];
> Why change anything here? This is called from enable hooks, so can keep on
> accessing intel_crtc->state ... or not?
I got rid of intel_crtc_to_shared_dpll, because it only required pipe_config->shared_dpll I felt it could be passed as argument instead.

>>  
>> -	if (WARN_ON(pll == NULL))
>> +	if (WARN_ON(shared_dpll < 0))
>>  		return;
>>  
>>  	WARN_ON(!pll->config.crtc_mask);
>>  	if (pll->active == 0) {
>>  		DRM_DEBUG_DRIVER("setting up %s\n", pll->name);
>>  		WARN_ON(pll->on);
>> -		assert_shared_dpll_disabled(dev_priv, pll);
>> +		assert_shared_dpll_disabled(dev_priv, shared_dpll);
>>  
>>  		pll->mode_set(dev_priv, pll);
>>  	}
>> @@ -1886,25 +1876,23 @@ static void intel_prepare_shared_dpll(struct intel_crtc *crtc)
>>   * The PCH PLL needs to be enabled before the PCH transcoder, since it
>>   * drives the transcoder clock.
>>   */
>> -static void intel_enable_shared_dpll(struct intel_crtc *crtc)
>> +static void intel_enable_shared_dpll(struct drm_i915_private *dev_priv,
>> +				     enum intel_dpll_id shared_dpll)
>>  {
>> -	struct drm_device *dev = crtc->base.dev;
>> -	struct drm_i915_private *dev_priv = dev->dev_private;
>> -	struct intel_shared_dpll *pll = intel_crtc_to_shared_dpll(crtc);
>> +	struct intel_shared_dpll *pll = &dev_priv->shared_dplls[shared_dpll];
>>  
>> -	if (WARN_ON(pll == NULL))
>> +	if (WARN_ON(shared_dpll < 0))
>>  		return;
>>  
>>  	if (WARN_ON(pll->config.crtc_mask == 0))
>>  		return;
>>  
>> -	DRM_DEBUG_KMS("enable %s (active %d, on? %d) for crtc %d\n",
>> -		      pll->name, pll->active, pll->on,
>> -		      crtc->base.base.id);
>> +	DRM_DEBUG_KMS("enable %s (active %d, on? %d)\n",
>> +		      pll->name, pll->active, pll->on);
>>  
>>  	if (pll->active++) {
>>  		WARN_ON(!pll->on);
>> -		assert_shared_dpll_enabled(dev_priv, pll);
>> +		assert_shared_dpll_enabled(dev_priv, shared_dpll);
>>  		return;
>>  	}
>>  	WARN_ON(pll->on);
>> @@ -1916,30 +1904,29 @@ static void intel_enable_shared_dpll(struct intel_crtc *crtc)
>>  	pll->on = true;
>>  }
>>  
>> -static void intel_disable_shared_dpll(struct intel_crtc *crtc)
>> +static void intel_disable_shared_dpll(struct drm_i915_private *dev_priv,
>> +				      enum intel_dpll_id shared_dpll)
>>  {
>> -	struct drm_device *dev = crtc->base.dev;
>> -	struct drm_i915_private *dev_priv = dev->dev_private;
>> -	struct intel_shared_dpll *pll = intel_crtc_to_shared_dpll(crtc);
>> +	struct intel_shared_dpll *pll = &dev_priv->shared_dplls[shared_dpll];
>> +
>> +	if (shared_dpll < 0)
>> +		return;
>>  
>>  	/* PCH only available on ILK+ */
>> -	BUG_ON(INTEL_INFO(dev)->gen < 5);
>> -	if (WARN_ON(pll == NULL))
>> -	       return;
>> +	BUG_ON(dev_priv->info.gen < 5);
>>  
>>  	if (WARN_ON(pll->config.crtc_mask == 0))
>>  		return;
>>  
>> -	DRM_DEBUG_KMS("disable %s (active %d, on? %d) for crtc %d\n",
>> -		      pll->name, pll->active, pll->on,
>> -		      crtc->base.base.id);
>> +	DRM_DEBUG_KMS("disable %s (active %d, on? %d)\n",
>> +		      pll->name, pll->active, pll->on);
>>  
>>  	if (WARN_ON(pll->active == 0)) {
>> -		assert_shared_dpll_disabled(dev_priv, pll);
>> +		assert_shared_dpll_disabled(dev_priv, shared_dpll);
>>  		return;
>>  	}
>>  
>> -	assert_shared_dpll_enabled(dev_priv, pll);
>> +	assert_shared_dpll_enabled(dev_priv, shared_dpll);
>>  	WARN_ON(!pll->on);
>>  	if (--pll->active)
>>  		return;
>> @@ -1964,8 +1951,7 @@ static void ironlake_enable_pch_transcoder(struct drm_i915_private *dev_priv,
>>  	BUG_ON(!HAS_PCH_SPLIT(dev));
>>  
>>  	/* Make sure PCH DPLL is enabled */
>> -	assert_shared_dpll_enabled(dev_priv,
>> -				   intel_crtc_to_shared_dpll(intel_crtc));
>> +	assert_shared_dpll_enabled(dev_priv, pipe_config->shared_dpll);
>>  
>>  	/* FDI must be feeding us bits for PCH ports */
>>  	assert_fdi_tx_enabled(dev_priv, pipe_config->cpu_transcoder, pipe);
>> @@ -2126,7 +2112,7 @@ static void intel_enable_pipe(struct intel_crtc *crtc)
>>  		else
>>  			assert_pll_enabled(dev_priv, pipe);
>>  	else {
>> -		if (crtc->config->has_pch_encoder) {
>> +		if (pipe_config->has_pch_encoder) {
>>  			/* if driving the PCH, we need FDI enabled */
>>  			assert_fdi_rx_pll_enabled(dev_priv, pch_transcoder);
>>  			assert_fdi_tx_pll_enabled(dev_priv,
>> @@ -2622,6 +2608,8 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>>  	bool visible = to_intel_plane_state(primary->state)->visible;
>>  	struct drm_i915_gem_object *obj;
>>  	int plane = intel_crtc->plane;
>> +	struct intel_crtc_state *pipe_config =
>> +		to_intel_crtc_state(crtc->state);
>>  	unsigned long linear_offset;
>>  	u32 dspcntr;
>>  	u32 reg = DSPCNTR(plane);
>> @@ -2655,13 +2643,13 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>>  		 * which should always be the user's requested size.
>>  		 */
>>  		I915_WRITE(DSPSIZE(plane),
>> -			   ((intel_crtc->config->pipe_src_h - 1) << 16) |
>> -			   (intel_crtc->config->pipe_src_w - 1));
>> +			   ((pipe_config->pipe_src_h - 1) << 16) |
>> +			   (pipe_config->pipe_src_w - 1));
>>  		I915_WRITE(DSPPOS(plane), 0);
>>  	} else if (IS_CHERRYVIEW(dev) && plane == PLANE_B) {
>>  		I915_WRITE(PRIMSIZE(plane),
>> -			   ((intel_crtc->config->pipe_src_h - 1) << 16) |
>> -			   (intel_crtc->config->pipe_src_w - 1));
>> +			   ((pipe_config->pipe_src_h - 1) << 16) |
>> +			   (pipe_config->pipe_src_w - 1));
>>  		I915_WRITE(PRIMPOS(plane), 0);
>>  		I915_WRITE(PRIMCNSTALPHA(plane), 0);
>>  	}
>> @@ -2719,14 +2707,14 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>>  	if (crtc->primary->state->rotation == BIT(DRM_ROTATE_180)) {
>>  		dspcntr |= DISPPLANE_ROTATE_180;
>>  
>> -		x += (intel_crtc->config->pipe_src_w - 1);
>> -		y += (intel_crtc->config->pipe_src_h - 1);
>> +		x += (pipe_config->pipe_src_w - 1);
>> +		y += (pipe_config->pipe_src_h - 1);
>>  
>>  		/* Finding the last pixel of the last line of the display
>>  		data and adding to linear_offset*/
>>  		linear_offset +=
>> -			(intel_crtc->config->pipe_src_h - 1) * fb->pitches[0] +
>> -			(intel_crtc->config->pipe_src_w - 1) * pixel_size;
>> +			(pipe_config->pipe_src_h - 1) * fb->pitches[0] +
>> +			(pipe_config->pipe_src_w - 1) * pixel_size;
> Unrelated changes above, or do I miss something?
What's unrelated? But yeah I mostly split it up by fixing things up until it started compiling again. I should split the display stuff up further.

>>  	}
>>  
>>  	I915_WRITE(reg, dspcntr);
>> @@ -2818,17 +2806,20 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>>  					       fb->pitches[0]);
>>  	linear_offset -= intel_crtc->dspaddr_offset;
>>  	if (crtc->primary->state->rotation == BIT(DRM_ROTATE_180)) {
>> +		struct intel_crtc_state *pipe_config =
>> +			to_intel_crtc_state(crtc->state);
>> +
>>  		dspcntr |= DISPPLANE_ROTATE_180;
>>  
>>  		if (!IS_HASWELL(dev) && !IS_BROADWELL(dev)) {
>> -			x += (intel_crtc->config->pipe_src_w - 1);
>> -			y += (intel_crtc->config->pipe_src_h - 1);
>> +			x += (pipe_config->pipe_src_w - 1);
>> +			y += (pipe_config->pipe_src_h - 1);
>>  
>>  			/* Finding the last pixel of the last line of the display
>>  			data and adding to linear_offset*/
>>  			linear_offset +=
>> -				(intel_crtc->config->pipe_src_h - 1) * fb->pitches[0] +
>> -				(intel_crtc->config->pipe_src_w - 1) * pixel_size;
>> +				(pipe_config->pipe_src_h - 1) * fb->pitches[0] +
>> +				(pipe_config->pipe_src_w - 1) * pixel_size;
>>  		}
>>  	}
>>  
>> @@ -2901,12 +2892,13 @@ void skl_detach_scalers(struct intel_crtc *intel_crtc)
>>  	struct intel_crtc_scaler_state *scaler_state;
>>  	int i;
>>  
>> -	if (!intel_crtc || !intel_crtc->config)
>> +	if (!intel_crtc)
>>  		return;
>>  
>>  	dev = intel_crtc->base.dev;
>>  	dev_priv = dev->dev_private;
>> -	scaler_state = &intel_crtc->config->scaler_state;
>> +	scaler_state =
>> +		&to_intel_crtc_state(intel_crtc->base.state)->scaler_state;
>>  
>>  	/* loop through and disable scalers that aren't in use */
>>  	for (i = 0; i < intel_crtc->num_scalers; i++) {
>> @@ -3298,6 +3290,8 @@ static void intel_update_pipe_size(struct intel_crtc *crtc)
>>  	struct drm_device *dev = crtc->base.dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	const struct drm_display_mode *adjusted_mode;
>> +	struct intel_crtc_state *pipe_config =
>> +		to_intel_crtc_state(crtc->base.state);
>>  
>>  	if (!i915.fastboot)
>>  		return;
>> @@ -3316,20 +3310,20 @@ static void intel_update_pipe_size(struct intel_crtc *crtc)
>>  	 * then update the pipesrc and pfit state, even on the flip path.
>>  	 */
>>  
>> -	adjusted_mode = &crtc->config->base.adjusted_mode;
>> +	adjusted_mode = &pipe_config->base.adjusted_mode;
>>  
>>  	I915_WRITE(PIPESRC(crtc->pipe),
>>  		   ((adjusted_mode->crtc_hdisplay - 1) << 16) |
>>  		   (adjusted_mode->crtc_vdisplay - 1));
>> -	if (!crtc->config->pch_pfit.enabled &&
>> +	if (!pipe_config->pch_pfit.enabled &&
>>  	    (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) ||
>>  	     intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) {
>>  		I915_WRITE(PF_CTL(crtc->pipe), 0);
>>  		I915_WRITE(PF_WIN_POS(crtc->pipe), 0);
>>  		I915_WRITE(PF_WIN_SZ(crtc->pipe), 0);
>>  	}
>> -	crtc->config->pipe_src_w = adjusted_mode->crtc_hdisplay;
>> -	crtc->config->pipe_src_h = adjusted_mode->crtc_vdisplay;
>> +	pipe_config->pipe_src_w = adjusted_mode->crtc_hdisplay;
>> +	pipe_config->pipe_src_h = adjusted_mode->crtc_vdisplay;
>>  }
>>  
>>  static void intel_fdi_normal_train(struct drm_crtc *crtc)
>> @@ -3379,6 +3373,8 @@ static void ironlake_fdi_link_train(struct drm_crtc *crtc)
>>  	struct drm_device *dev = crtc->dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> +	struct intel_crtc_state *pipe_config =
>> +		to_intel_crtc_state(crtc->state);
>>  	int pipe = intel_crtc->pipe;
>>  	u32 reg, temp, tries;
>>  
>> @@ -3396,7 +3392,7 @@ static void ironlake_fdi_link_train(struct drm_crtc *crtc)
>>  	reg = FDI_TX_CTL(pipe);
>>  	temp = I915_READ(reg);
>>  	temp &= ~FDI_DP_PORT_WIDTH_MASK;
>> -	temp |= FDI_DP_PORT_WIDTH(intel_crtc->config->fdi_lanes);
>> +	temp |= FDI_DP_PORT_WIDTH(pipe_config->fdi_lanes);
>>  	temp &= ~FDI_LINK_TRAIN_NONE;
>>  	temp |= FDI_LINK_TRAIN_PATTERN_1;
>>  	I915_WRITE(reg, temp | FDI_TX_ENABLE);
>> @@ -3476,6 +3472,8 @@ static void gen6_fdi_link_train(struct drm_crtc *crtc)
>>  	struct drm_device *dev = crtc->dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> +	struct intel_crtc_state *pipe_config =
>> +		to_intel_crtc_state(crtc->state);
>>  	int pipe = intel_crtc->pipe;
>>  	u32 reg, temp, i, retry;
>>  
>> @@ -3494,7 +3492,7 @@ static void gen6_fdi_link_train(struct drm_crtc *crtc)
>>  	reg = FDI_TX_CTL(pipe);
>>  	temp = I915_READ(reg);
>>  	temp &= ~FDI_DP_PORT_WIDTH_MASK;
>> -	temp |= FDI_DP_PORT_WIDTH(intel_crtc->config->fdi_lanes);
>> +	temp |= FDI_DP_PORT_WIDTH(pipe_config->fdi_lanes);
>>  	temp &= ~FDI_LINK_TRAIN_NONE;
>>  	temp |= FDI_LINK_TRAIN_PATTERN_1;
>>  	temp &= ~FDI_LINK_TRAIN_VOL_EMP_MASK;
>> @@ -3608,6 +3606,8 @@ static void ivb_manual_fdi_link_train(struct drm_crtc *crtc)
>>  	struct drm_device *dev = crtc->dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> +	struct intel_crtc_state *pipe_config =
>> +		to_intel_crtc_state(crtc->state);
>>  	int pipe = intel_crtc->pipe;
>>  	u32 reg, temp, i, j;
>>  
>> @@ -3645,7 +3645,7 @@ static void ivb_manual_fdi_link_train(struct drm_crtc *crtc)
>>  		reg = FDI_TX_CTL(pipe);
>>  		temp = I915_READ(reg);
>>  		temp &= ~FDI_DP_PORT_WIDTH_MASK;
>> -		temp |= FDI_DP_PORT_WIDTH(intel_crtc->config->fdi_lanes);
>> +		temp |= FDI_DP_PORT_WIDTH(pipe_config->fdi_lanes);
>>  		temp |= FDI_LINK_TRAIN_PATTERN_1_IVB;
>>  		temp &= ~FDI_LINK_TRAIN_VOL_EMP_MASK;
>>  		temp |= snb_b_fdi_train_param[j/2];
>> @@ -3914,11 +3914,10 @@ void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
>>  }
>>  
>>  /* Program iCLKIP clock to the desired frequency */
>> -static void lpt_program_iclkip(struct drm_crtc *crtc)
>> +static void lpt_program_iclkip(struct drm_i915_private *dev_priv,
>> +			       struct intel_crtc_state *pipe_config)
>>  {
>> -	struct drm_device *dev = crtc->dev;
>> -	struct drm_i915_private *dev_priv = dev->dev_private;
>> -	int clock = to_intel_crtc(crtc)->config->base.adjusted_mode.crtc_clock;
>> +	int clock = pipe_config->base.adjusted_mode.crtc_clock;
>>  	u32 divsel, phaseinc, auxdiv, phasedir = 0;
>>  	u32 temp;
>>  
>> @@ -4002,13 +4001,10 @@ static void lpt_program_iclkip(struct drm_crtc *crtc)
>>  	mutex_unlock(&dev_priv->dpio_lock);
>>  }
>>  
>> -static void ironlake_pch_transcoder_set_timings(struct intel_crtc *crtc,
>> +static void ironlake_pch_transcoder_set_timings(struct drm_i915_private *dev_priv,
>> +						enum transcoder cpu_transcoder,
>>  						enum pipe pch_transcoder)
>>  {
>> -	struct drm_device *dev = crtc->base.dev;
>> -	struct drm_i915_private *dev_priv = dev->dev_private;
>> -	enum transcoder cpu_transcoder = crtc->config->cpu_transcoder;
>> -
>>  	I915_WRITE(PCH_TRANS_HTOTAL(pch_transcoder),
>>  		   I915_READ(HTOTAL(cpu_transcoder)));
>>  	I915_WRITE(PCH_TRANS_HBLANK(pch_transcoder),
>> @@ -4026,7 +4022,8 @@ static void ironlake_pch_transcoder_set_timings(struct intel_crtc *crtc,
>>  		   I915_READ(VSYNCSHIFT(cpu_transcoder)));
>>  }
>>  
>> -static void cpt_set_fdi_bc_bifurcation(struct drm_device *dev, bool enable)
>> +static void cpt_set_fdi_bc_bifurcation(struct drm_device *dev,
>> +				       bool enable)
>>  {
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	uint32_t temp;
>> @@ -4047,15 +4044,15 @@ static void cpt_set_fdi_bc_bifurcation(struct drm_device *dev, bool enable)
>>  	POSTING_READ(SOUTH_CHICKEN1);
>>  }
>>  
>> -static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc)
>> +static void ivybridge_update_fdi_bc_bifurcation(struct drm_device *dev,
>> +					struct intel_crtc *intel_crtc,
>> +					struct intel_crtc_state *pipe_config)
>>  {
>> -	struct drm_device *dev = intel_crtc->base.dev;
>> -
>>  	switch (intel_crtc->pipe) {
>>  	case PIPE_A:
>>  		break;
>>  	case PIPE_B:
>> -		if (intel_crtc->config->fdi_lanes > 2)
>> +		if (pipe_config->fdi_lanes > 2)
>>  			cpt_set_fdi_bc_bifurcation(dev, false);
>>  		else
>>  			cpt_set_fdi_bc_bifurcation(dev, true);
>> @@ -4078,7 +4075,8 @@ static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc)
>>   *   - DP transcoding bits
>>   *   - transcoder
>>   */
>> -static void ironlake_pch_enable(struct drm_crtc *crtc, struct intel_crtc_state *pipe_config)
>> +static void ironlake_pch_enable(struct drm_crtc *crtc,
>> +				struct intel_crtc_state *pipe_config)
>>  {
>>  	struct drm_device *dev = crtc->dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>> @@ -4089,7 +4087,8 @@ static void ironlake_pch_enable(struct drm_crtc *crtc, struct intel_crtc_state *
>>  	assert_pch_transcoder_disabled(dev_priv, pipe);
>>  
>>  	if (IS_IVYBRIDGE(dev))
>> -		ivybridge_update_fdi_bc_bifurcation(intel_crtc);
>> +		ivybridge_update_fdi_bc_bifurcation(dev, intel_crtc,
>> +						    pipe_config);
>>  
>>  	/* Write the TU size bits before fdi link training, so that error
>>  	 * detection works. */
>> @@ -4121,11 +4120,12 @@ static void ironlake_pch_enable(struct drm_crtc *crtc, struct intel_crtc_state *
>>  	 * Note that enable_shared_dpll tries to do the right thing, but
>>  	 * get_shared_dpll unconditionally resets the pll - we need that to have
>>  	 * the right LVDS enable sequence. */
>> -	intel_enable_shared_dpll(intel_crtc);
>> +	intel_enable_shared_dpll(dev_priv, pipe_config->shared_dpll);
>>  
>>  	/* set transcoder timing, panel must allow it */
>>  	assert_panel_unlocked(dev_priv, pipe);
>> -	ironlake_pch_transcoder_set_timings(intel_crtc, pipe);
>> +	ironlake_pch_transcoder_set_timings(dev_priv,
>> +					    pipe_config->cpu_transcoder, pipe);
>>  
>>  	intel_fdi_normal_train(crtc);
>>  
>> @@ -4166,19 +4166,17 @@ static void ironlake_pch_enable(struct drm_crtc *crtc, struct intel_crtc_state *
>>  	ironlake_enable_pch_transcoder(dev_priv, pipe, pipe_config);
>>  }
>>  
>> -static void lpt_pch_enable(struct drm_crtc *crtc)
>> +static void lpt_pch_enable(struct drm_i915_private *dev_priv,
>> +			   struct intel_crtc_state *pipe_config)
>>  {
>> -	struct drm_device *dev = crtc->dev;
>> -	struct drm_i915_private *dev_priv = dev->dev_private;
>> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> -	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
>> +	enum transcoder cpu_transcoder = pipe_config->cpu_transcoder;
>>  
>>  	assert_pch_transcoder_disabled(dev_priv, TRANSCODER_A);
>>  
>> -	lpt_program_iclkip(crtc);
>> +	lpt_program_iclkip(dev_priv, pipe_config);
>>  
>>  	/* Set transcoder timing. */
>> -	ironlake_pch_transcoder_set_timings(intel_crtc, PIPE_A);
>> +	ironlake_pch_transcoder_set_timings(dev_priv, cpu_transcoder, PIPE_A);
>>  
>>  	lpt_enable_pch_transcoder(dev_priv, cpu_transcoder);
>>  }
>> @@ -4781,7 +4779,8 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>>  	pipe_config = to_intel_crtc_state(crtc->state);
>>  
>>  	if (pipe_config->has_pch_encoder)
>> -		intel_prepare_shared_dpll(intel_crtc);
>> +		intel_prepare_shared_dpll(dev_priv,
>> +					  pipe_config->shared_dpll);
>>  
>>  	if (pipe_config->has_dp_encoder)
>>  		intel_dp_set_m_n(intel_crtc, M1_N1);
>> @@ -4854,8 +4853,8 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>>  	WARN_ON(!crtc->state->enable);
>>  
>>  	pipe_config = to_intel_crtc_state(crtc->state);
>> -	if (intel_crtc_to_shared_dpll(intel_crtc))
>> -		intel_enable_shared_dpll(intel_crtc);
>> +	if (pipe_config->shared_dpll >= 0)
>> +		intel_enable_shared_dpll(dev_priv, pipe_config->shared_dpll);
>>  
>>  	if (pipe_config->has_dp_encoder)
>>  		intel_dp_set_m_n(intel_crtc, M1_N1);
>> @@ -4910,7 +4909,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>>  	intel_enable_pipe(intel_crtc);
>>  
>>  	if (intel_crtc->config->has_pch_encoder)
>> -		lpt_pch_enable(crtc);
>> +		lpt_pch_enable(dev_priv, pipe_config);
>>  
>>  	if (intel_crtc->config->dp_encoder_is_mst)
>>  		intel_ddi_set_vc_payload_alloc(crtc,
>> @@ -11929,7 +11928,8 @@ check_shared_dpll_state(struct drm_device *dev)
>>  		     pll->on, active);
>>  
>>  		for_each_intel_crtc(dev, crtc) {
>> -			if (crtc->base.state->active && intel_crtc_to_shared_dpll(crtc) == pll)
>> +			if (crtc->base.state->active &&
>> +			    to_intel_crtc_state(crtc->base.state)->shared_dpll == i)
>>  				active_crtcs++;
>>  		}
>>  		I915_STATE_WARN(pll->active != active_crtcs,
>> @@ -12311,7 +12311,6 @@ static int __intel_set_mode(struct drm_atomic_state *state)
>>  	__intel_set_mode_update_planes(dev, state);
>>  
>>  	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
>> -		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>  		struct intel_crtc_state *pipe_config;
>>  
>>  		if (!needs_modeset(crtc->state))
>> @@ -12324,8 +12323,7 @@ static int __intel_set_mode(struct drm_atomic_state *state)
>>  		intel_crtc_dpms_overlay_disable(to_intel_crtc(crtc));
>>  		dev_priv->display.crtc_disable(crtc, pipe_config);
>>  
>> -		if (intel_crtc_to_shared_dpll(intel_crtc))
>> -			intel_disable_shared_dpll(intel_crtc);
>> +		intel_disable_shared_dpll(dev_priv, pipe_config->shared_dpll);
>>  	}
>>  
>>  	/* Only after disabling all output pipelines that will be changed can we
>> @@ -12719,7 +12717,9 @@ static void ibx_pch_dpll_disable(struct drm_i915_private *dev_priv,
>>  
>>  	/* Make sure no transcoder isn't still depending on us. */
>>  	for_each_intel_crtc(dev, crtc) {
>> -		if (intel_crtc_to_shared_dpll(crtc) == pll)
>> +		int i = pll - dev_priv->shared_dplls;
>> +
>> +		if (to_intel_crtc_state(crtc->base.state)->shared_dpll == i)
>>  			assert_pch_transcoder_disabled(dev_priv, crtc->pipe);
>>  	}
> This is called with the with the new config already put in place, but it
> should check whether any of the _old_ pipe configs that used the dpll are
> really all shut down. This won't misfire since if we shut it down to use
> it in some other configuration then those pipes should be ofc off too.
> Otoh we do check before enabling the pipe that the dpll is running, hence
> I think this is redundant and maybe we could remove
> it right away?
Removing sounds fine.

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx