[v2,4/8] drm/i915: Flatten a bunch of the pfit functions
diff mbox series

Message ID 20200212161738.28141-5-ville.syrjala@linux.intel.com
State New
Headers show
Series
  • drm/i915: pfit/scaler rework prep stuff
Related show

Commit Message

Ville Syrjälä Feb. 12, 2020, 4:17 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Most of the pfit functions are of the form:

func()
{
	if (pfit_enabled) {
		...
	}
}

Flip the pfit_enabled check around to flatten the functions.

And while we're touching all this let's do the usual
s/pipe_config/crtc_state/ replacement.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 233 +++++++++----------
 1 file changed, 115 insertions(+), 118 deletions(-)

Comments

Manasi Navare April 1, 2020, 11:53 p.m. UTC | #1
On Wed, Feb 12, 2020 at 06:17:34PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Most of the pfit functions are of the form:
> 
> func()
> {
> 	if (pfit_enabled) {
> 		...
> 	}
> }
> 
> Flip the pfit_enabled check around to flatten the functions.
> 
> And while we're touching all this let's do the usual
> s/pipe_config/crtc_state/ replacement.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 233 +++++++++----------
>  1 file changed, 115 insertions(+), 118 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index becc6322b7dc..796e27c4aece 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -6233,42 +6233,42 @@ static void skl_pfit_enable(const struct intel_crtc_state *crtc_state)
>  	enum pipe pipe = crtc->pipe;
>  	const struct intel_crtc_scaler_state *scaler_state =
>  		&crtc_state->scaler_state;
> +	u16 uv_rgb_hphase, uv_rgb_vphase;
> +	int pfit_w, pfit_h, hscale, vscale;
> +	unsigned long irqflags;
> +	int id;
>  
> -	if (crtc_state->pch_pfit.enabled) {
> -		u16 uv_rgb_hphase, uv_rgb_vphase;
> -		int pfit_w, pfit_h, hscale, vscale;
> -		unsigned long irqflags;
> -		int id;
> +	if (!crtc_state->pch_pfit.enabled)
> +		return;
>  
> -		if (WARN_ON(crtc_state->scaler_state.scaler_id < 0))
> -			return;
> +	if (WARN_ON(crtc_state->scaler_state.scaler_id < 0))
> +		return;
>  
> -		pfit_w = (crtc_state->pch_pfit.size >> 16) & 0xFFFF;
> -		pfit_h = crtc_state->pch_pfit.size & 0xFFFF;
> +	pfit_w = (crtc_state->pch_pfit.size >> 16) & 0xFFFF;
> +	pfit_h = crtc_state->pch_pfit.size & 0xFFFF;
>  
> -		hscale = (crtc_state->pipe_src_w << 16) / pfit_w;
> -		vscale = (crtc_state->pipe_src_h << 16) / pfit_h;
> +	hscale = (crtc_state->pipe_src_w << 16) / pfit_w;
> +	vscale = (crtc_state->pipe_src_h << 16) / pfit_h;
>  
> -		uv_rgb_hphase = skl_scaler_calc_phase(1, hscale, false);
> -		uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false);
> +	uv_rgb_hphase = skl_scaler_calc_phase(1, hscale, false);
> +	uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false);
>  
> -		id = scaler_state->scaler_id;
> +	id = scaler_state->scaler_id;
>  
> -		spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> +	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>  
> -		intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id), PS_SCALER_EN |
> -				  PS_FILTER_MEDIUM | scaler_state->scalers[id].mode);
> -		intel_de_write_fw(dev_priv, SKL_PS_VPHASE(pipe, id),
> -				  PS_Y_PHASE(0) | PS_UV_RGB_PHASE(uv_rgb_vphase));
> -		intel_de_write_fw(dev_priv, SKL_PS_HPHASE(pipe, id),
> -				  PS_Y_PHASE(0) | PS_UV_RGB_PHASE(uv_rgb_hphase));
> -		intel_de_write_fw(dev_priv, SKL_PS_WIN_POS(pipe, id),
> -				  crtc_state->pch_pfit.pos);
> -		intel_de_write_fw(dev_priv, SKL_PS_WIN_SZ(pipe, id),
> -				  crtc_state->pch_pfit.size);
> +	intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id), PS_SCALER_EN |
> +			  PS_FILTER_MEDIUM | scaler_state->scalers[id].mode);
> +	intel_de_write_fw(dev_priv, SKL_PS_VPHASE(pipe, id),
> +			  PS_Y_PHASE(0) | PS_UV_RGB_PHASE(uv_rgb_vphase));
> +	intel_de_write_fw(dev_priv, SKL_PS_HPHASE(pipe, id),
> +			  PS_Y_PHASE(0) | PS_UV_RGB_PHASE(uv_rgb_hphase));
> +	intel_de_write_fw(dev_priv, SKL_PS_WIN_POS(pipe, id),
> +			  crtc_state->pch_pfit.pos);
> +	intel_de_write_fw(dev_priv, SKL_PS_WIN_SZ(pipe, id),
> +			  crtc_state->pch_pfit.size);
>  
> -		spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> -	}
> +	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  }
>  
>  static void ilk_pfit_enable(const struct intel_crtc_state *crtc_state)
> @@ -6277,22 +6277,23 @@ static void ilk_pfit_enable(const struct intel_crtc_state *crtc_state)
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	enum pipe pipe = crtc->pipe;
>  
> -	if (crtc_state->pch_pfit.enabled) {
> -		/* Force use of hard-coded filter coefficients
> -		 * as some pre-programmed values are broken,
> -		 * e.g. x201.
> -		 */
> -		if (IS_IVYBRIDGE(dev_priv) || IS_HASWELL(dev_priv))
> -			intel_de_write(dev_priv, PF_CTL(pipe),
> -				       PF_ENABLE | PF_FILTER_MED_3x3 | PF_PIPE_SEL_IVB(pipe));
> -		else
> -			intel_de_write(dev_priv, PF_CTL(pipe),
> -				       PF_ENABLE | PF_FILTER_MED_3x3);
> -		intel_de_write(dev_priv, PF_WIN_POS(pipe),
> -			       crtc_state->pch_pfit.pos);
> -		intel_de_write(dev_priv, PF_WIN_SZ(pipe),
> -			       crtc_state->pch_pfit.size);

Why dont we use the intel_de_write_fw() everywhere?

Manasi

> -	}
> +	if (!crtc_state->pch_pfit.enabled)
> +		return;
> +
> +	/* Force use of hard-coded filter coefficients
> +	 * as some pre-programmed values are broken,
> +	 * e.g. x201.
> +	 */
> +	if (IS_IVYBRIDGE(dev_priv) || IS_HASWELL(dev_priv))
> +		intel_de_write(dev_priv, PF_CTL(pipe), PF_ENABLE |
> +			       PF_FILTER_MED_3x3 | PF_PIPE_SEL_IVB(pipe));
> +	else
> +		intel_de_write(dev_priv, PF_CTL(pipe), PF_ENABLE |
> +			       PF_FILTER_MED_3x3);
> +	intel_de_write(dev_priv, PF_WIN_POS(pipe),
> +		       crtc_state->pch_pfit.pos);
> +	intel_de_write(dev_priv, PF_WIN_SZ(pipe),
> +		       crtc_state->pch_pfit.size);
>  }
>  
>  void hsw_enable_ips(const struct intel_crtc_state *crtc_state)
> @@ -7107,11 +7108,12 @@ void ilk_pfit_disable(const struct intel_crtc_state *old_crtc_state)
>  
>  	/* To avoid upsetting the power well on haswell only disable the pfit if
>  	 * it's in use. The hw state code will make sure we get this right. */
> -	if (old_crtc_state->pch_pfit.enabled) {
> -		intel_de_write(dev_priv, PF_CTL(pipe), 0);
> -		intel_de_write(dev_priv, PF_WIN_POS(pipe), 0);
> -		intel_de_write(dev_priv, PF_WIN_SZ(pipe), 0);
> -	}
> +	if (!old_crtc_state->pch_pfit.enabled)
> +		return;
> +
> +	intel_de_write(dev_priv, PF_CTL(pipe), 0);
> +	intel_de_write(dev_priv, PF_WIN_POS(pipe), 0);
> +	intel_de_write(dev_priv, PF_WIN_SZ(pipe), 0);
>  }
>  
>  static void ilk_crtc_disable(struct intel_atomic_state *state,
> @@ -7927,39 +7929,35 @@ static bool intel_crtc_supports_double_wide(const struct intel_crtc *crtc)
>  		(crtc->pipe == PIPE_A || IS_I915G(dev_priv));
>  }
>  
> -static u32 ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config)
> +static u32 ilk_pipe_pixel_rate(const struct intel_crtc_state *crtc_state)
>  {
> -	u32 pixel_rate;
> -
> -	pixel_rate = pipe_config->hw.adjusted_mode.crtc_clock;
> +	u32 pixel_rate = crtc_state->hw.adjusted_mode.crtc_clock;
> +	u32 pfit_size = crtc_state->pch_pfit.size;
> +	u64 pipe_w, pipe_h, pfit_w, pfit_h;
>  
>  	/*
>  	 * We only use IF-ID interlacing. If we ever use
>  	 * PF-ID we'll need to adjust the pixel_rate here.
>  	 */
>  
> -	if (pipe_config->pch_pfit.enabled) {
> -		u64 pipe_w, pipe_h, pfit_w, pfit_h;
> -		u32 pfit_size = pipe_config->pch_pfit.size;
> -
> -		pipe_w = pipe_config->pipe_src_w;
> -		pipe_h = pipe_config->pipe_src_h;
> +	if (!crtc_state->pch_pfit.enabled)
> +		return pixel_rate;
>  
> -		pfit_w = (pfit_size >> 16) & 0xFFFF;
> -		pfit_h = pfit_size & 0xFFFF;
> -		if (pipe_w < pfit_w)
> -			pipe_w = pfit_w;
> -		if (pipe_h < pfit_h)
> -			pipe_h = pfit_h;
> +	pipe_w = crtc_state->pipe_src_w;
> +	pipe_h = crtc_state->pipe_src_h;
>  
> -		if (WARN_ON(!pfit_w || !pfit_h))
> -			return pixel_rate;
> +	pfit_w = (pfit_size >> 16) & 0xFFFF;
> +	pfit_h = pfit_size & 0xFFFF;
> +	if (pipe_w < pfit_w)
> +		pipe_w = pfit_w;
> +	if (pipe_h < pfit_h)
> +		pipe_h = pfit_h;
>  
> -		pixel_rate = div_u64(mul_u32_u32(pixel_rate, pipe_w * pipe_h),
> -				     pfit_w * pfit_h);
> -	}
> +	if (WARN_ON(!pfit_w || !pfit_h))
> +		return pixel_rate;
>  
> -	return pixel_rate;
> +	return div_u64(mul_u32_u32(pixel_rate, pipe_w * pipe_h),
> +		       pfit_w * pfit_h);
>  }
>  
>  static void intel_crtc_compute_pixel_rate(struct intel_crtc_state *crtc_state)
> @@ -9153,9 +9151,9 @@ static bool i9xx_has_pfit(struct drm_i915_private *dev_priv)
>  		IS_PINEVIEW(dev_priv) || IS_MOBILE(dev_priv);
>  }
>  
> -static void i9xx_get_pfit_config(struct intel_crtc *crtc,
> -				 struct intel_crtc_state *pipe_config)
> +static void i9xx_get_pfit_config(struct intel_crtc_state *crtc_state)
>  {
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	u32 tmp;
>  
> @@ -9175,9 +9173,9 @@ static void i9xx_get_pfit_config(struct intel_crtc *crtc,
>  			return;
>  	}
>  
> -	pipe_config->gmch_pfit.control = tmp;
> -	pipe_config->gmch_pfit.pgm_ratios = intel_de_read(dev_priv,
> -							  PFIT_PGM_RATIOS);
> +	crtc_state->gmch_pfit.control = tmp;
> +	crtc_state->gmch_pfit.pgm_ratios =
> +		intel_de_read(dev_priv, PFIT_PGM_RATIOS);
>  }
>  
>  static void vlv_crtc_clock_get(struct intel_crtc *crtc,
> @@ -9427,7 +9425,7 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>  	intel_get_pipe_timings(crtc, pipe_config);
>  	intel_get_pipe_src_size(crtc, pipe_config);
>  
> -	i9xx_get_pfit_config(crtc, pipe_config);
> +	i9xx_get_pfit_config(pipe_config);
>  
>  	if (INTEL_GEN(dev_priv) >= 4) {
>  		/* No way to read it out on pipes B and C */
> @@ -10393,37 +10391,37 @@ static void ilk_get_fdi_m_n_config(struct intel_crtc *crtc,
>  				     &pipe_config->fdi_m_n, NULL);
>  }
>  
> -static void skl_get_pfit_config(struct intel_crtc *crtc,
> -				struct intel_crtc_state *pipe_config)
> +static void skl_get_pfit_config(struct intel_crtc_state *crtc_state)
>  {
> -	struct drm_device *dev = crtc->base.dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct intel_crtc_scaler_state *scaler_state = &pipe_config->scaler_state;
> -	u32 ps_ctrl = 0;
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	struct intel_crtc_scaler_state *scaler_state = &crtc_state->scaler_state;
>  	int id = -1;
>  	int i;
>  
>  	/* find scaler attached to this pipe */
>  	for (i = 0; i < crtc->num_scalers; i++) {
> -		ps_ctrl = intel_de_read(dev_priv, SKL_PS_CTRL(crtc->pipe, i));
> -		if (ps_ctrl & PS_SCALER_EN && !(ps_ctrl & PS_PLANE_SEL_MASK)) {
> -			id = i;
> -			pipe_config->pch_pfit.enabled = true;
> -			pipe_config->pch_pfit.pos = intel_de_read(dev_priv,
> -								  SKL_PS_WIN_POS(crtc->pipe, i));
> -			pipe_config->pch_pfit.size = intel_de_read(dev_priv,
> -								   SKL_PS_WIN_SZ(crtc->pipe, i));
> -			scaler_state->scalers[i].in_use = true;
> -			break;
> -		}
> +		u32 tmp;
> +
> +		tmp = intel_de_read(dev_priv, SKL_PS_CTRL(crtc->pipe, i));
> +		if ((tmp & (PS_SCALER_EN | PS_PLANE_SEL_MASK)) != PS_SCALER_EN)
> +			continue;
> +
> +		id = i;
> +		crtc_state->pch_pfit.enabled = true;
> +		crtc_state->pch_pfit.pos =
> +			intel_de_read(dev_priv, SKL_PS_WIN_POS(crtc->pipe, i));
> +		crtc_state->pch_pfit.size =
> +			intel_de_read(dev_priv, SKL_PS_WIN_SZ(crtc->pipe, i));
> +		scaler_state->scalers[i].in_use = true;
> +		break;
>  	}
>  
>  	scaler_state->scaler_id = id;
> -	if (id >= 0) {
> +	if (id >= 0)
>  		scaler_state->scaler_users |= (1 << SKL_CRTC_INDEX);
> -	} else {
> +	else
>  		scaler_state->scaler_users &= ~(1 << SKL_CRTC_INDEX);
> -	}
>  }
>  
>  static void
> @@ -10559,30 +10557,29 @@ skl_get_initial_plane_config(struct intel_crtc *crtc,
>  	kfree(intel_fb);
>  }
>  
> -static void ilk_get_pfit_config(struct intel_crtc *crtc,
> -				struct intel_crtc_state *pipe_config)
> +static void ilk_get_pfit_config(struct intel_crtc_state *crtc_state)
>  {
> -	struct drm_device *dev = crtc->base.dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	u32 tmp;
>  
>  	tmp = intel_de_read(dev_priv, PF_CTL(crtc->pipe));
> +	if ((tmp & PF_ENABLE) == 0)
> +		return;
>  
> -	if (tmp & PF_ENABLE) {
> -		pipe_config->pch_pfit.enabled = true;
> -		pipe_config->pch_pfit.pos = intel_de_read(dev_priv,
> -							  PF_WIN_POS(crtc->pipe));
> -		pipe_config->pch_pfit.size = intel_de_read(dev_priv,
> -							   PF_WIN_SZ(crtc->pipe));
> -
> -		/* We currently do not free assignements of panel fitters on
> -		 * ivb/hsw (since we don't use the higher upscaling modes which
> -		 * differentiates them) so just WARN about this case for now. */
> -		if (IS_GEN(dev_priv, 7)) {
> -			WARN_ON((tmp & PF_PIPE_SEL_MASK_IVB) !=
> -				PF_PIPE_SEL_IVB(crtc->pipe));
> -		}
> -	}
> +	crtc_state->pch_pfit.enabled = true;
> +	crtc_state->pch_pfit.pos =
> +		intel_de_read(dev_priv, PF_WIN_POS(crtc->pipe));
> +	crtc_state->pch_pfit.size =
> +		intel_de_read(dev_priv, PF_WIN_SZ(crtc->pipe));
> +
> +	/*
> +	 * We currently do not free assignements of panel fitters on
> +	 * ivb/hsw (since we don't use the higher upscaling modes which
> +	 * differentiates them) so just WARN about this case for now.
> +	 */
> +	WARN_ON(IS_GEN(dev_priv, 7) &&
> +		(tmp & PF_PIPE_SEL_MASK_IVB) != PF_PIPE_SEL_IVB(crtc->pipe));
>  }
>  
>  static bool ilk_get_pipe_config(struct intel_crtc *crtc,
> @@ -10694,7 +10691,7 @@ static bool ilk_get_pipe_config(struct intel_crtc *crtc,
>  	intel_get_pipe_timings(crtc, pipe_config);
>  	intel_get_pipe_src_size(crtc, pipe_config);
>  
> -	ilk_get_pfit_config(crtc, pipe_config);
> +	ilk_get_pfit_config(pipe_config);
>  
>  	ret = true;
>  
> @@ -11219,9 +11216,9 @@ static bool hsw_get_pipe_config(struct intel_crtc *crtc,
>  		power_domain_mask |= BIT_ULL(power_domain);
>  
>  		if (INTEL_GEN(dev_priv) >= 9)
> -			skl_get_pfit_config(crtc, pipe_config);
> +			skl_get_pfit_config(pipe_config);
>  		else
> -			ilk_get_pfit_config(crtc, pipe_config);
> +			ilk_get_pfit_config(pipe_config);
>  	}
>  
>  	if (hsw_crtc_supports_ips(crtc)) {
> -- 
> 2.24.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä April 2, 2020, 1:55 p.m. UTC | #2
On Wed, Apr 01, 2020 at 04:53:23PM -0700, Manasi Navare wrote:
> On Wed, Feb 12, 2020 at 06:17:34PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Most of the pfit functions are of the form:
> > 
> > func()
> > {
> > 	if (pfit_enabled) {
> > 		...
> > 	}
> > }
> > 
> > Flip the pfit_enabled check around to flatten the functions.
> > 
> > And while we're touching all this let's do the usual
> > s/pipe_config/crtc_state/ replacement.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 233 +++++++++----------
> >  1 file changed, 115 insertions(+), 118 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index becc6322b7dc..796e27c4aece 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -6233,42 +6233,42 @@ static void skl_pfit_enable(const struct intel_crtc_state *crtc_state)
> >  	enum pipe pipe = crtc->pipe;
> >  	const struct intel_crtc_scaler_state *scaler_state =
> >  		&crtc_state->scaler_state;
> > +	u16 uv_rgb_hphase, uv_rgb_vphase;
> > +	int pfit_w, pfit_h, hscale, vscale;
> > +	unsigned long irqflags;
> > +	int id;
> >  
> > -	if (crtc_state->pch_pfit.enabled) {
> > -		u16 uv_rgb_hphase, uv_rgb_vphase;
> > -		int pfit_w, pfit_h, hscale, vscale;
> > -		unsigned long irqflags;
> > -		int id;
> > +	if (!crtc_state->pch_pfit.enabled)
> > +		return;
> >  
> > -		if (WARN_ON(crtc_state->scaler_state.scaler_id < 0))
> > -			return;
> > +	if (WARN_ON(crtc_state->scaler_state.scaler_id < 0))
> > +		return;
> >  
> > -		pfit_w = (crtc_state->pch_pfit.size >> 16) & 0xFFFF;
> > -		pfit_h = crtc_state->pch_pfit.size & 0xFFFF;
> > +	pfit_w = (crtc_state->pch_pfit.size >> 16) & 0xFFFF;
> > +	pfit_h = crtc_state->pch_pfit.size & 0xFFFF;
> >  
> > -		hscale = (crtc_state->pipe_src_w << 16) / pfit_w;
> > -		vscale = (crtc_state->pipe_src_h << 16) / pfit_h;
> > +	hscale = (crtc_state->pipe_src_w << 16) / pfit_w;
> > +	vscale = (crtc_state->pipe_src_h << 16) / pfit_h;
> >  
> > -		uv_rgb_hphase = skl_scaler_calc_phase(1, hscale, false);
> > -		uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false);
> > +	uv_rgb_hphase = skl_scaler_calc_phase(1, hscale, false);
> > +	uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false);
> >  
> > -		id = scaler_state->scaler_id;
> > +	id = scaler_state->scaler_id;
> >  
> > -		spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> > +	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> >  
> > -		intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id), PS_SCALER_EN |
> > -				  PS_FILTER_MEDIUM | scaler_state->scalers[id].mode);
> > -		intel_de_write_fw(dev_priv, SKL_PS_VPHASE(pipe, id),
> > -				  PS_Y_PHASE(0) | PS_UV_RGB_PHASE(uv_rgb_vphase));
> > -		intel_de_write_fw(dev_priv, SKL_PS_HPHASE(pipe, id),
> > -				  PS_Y_PHASE(0) | PS_UV_RGB_PHASE(uv_rgb_hphase));
> > -		intel_de_write_fw(dev_priv, SKL_PS_WIN_POS(pipe, id),
> > -				  crtc_state->pch_pfit.pos);
> > -		intel_de_write_fw(dev_priv, SKL_PS_WIN_SZ(pipe, id),
> > -				  crtc_state->pch_pfit.size);
> > +	intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id), PS_SCALER_EN |
> > +			  PS_FILTER_MEDIUM | scaler_state->scalers[id].mode);
> > +	intel_de_write_fw(dev_priv, SKL_PS_VPHASE(pipe, id),
> > +			  PS_Y_PHASE(0) | PS_UV_RGB_PHASE(uv_rgb_vphase));
> > +	intel_de_write_fw(dev_priv, SKL_PS_HPHASE(pipe, id),
> > +			  PS_Y_PHASE(0) | PS_UV_RGB_PHASE(uv_rgb_hphase));
> > +	intel_de_write_fw(dev_priv, SKL_PS_WIN_POS(pipe, id),
> > +			  crtc_state->pch_pfit.pos);
> > +	intel_de_write_fw(dev_priv, SKL_PS_WIN_SZ(pipe, id),
> > +			  crtc_state->pch_pfit.size);
> >  
> > -		spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> > -	}
> > +	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> >  }
> >  
> >  static void ilk_pfit_enable(const struct intel_crtc_state *crtc_state)
> > @@ -6277,22 +6277,23 @@ static void ilk_pfit_enable(const struct intel_crtc_state *crtc_state)
> >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >  	enum pipe pipe = crtc->pipe;
> >  
> > -	if (crtc_state->pch_pfit.enabled) {
> > -		/* Force use of hard-coded filter coefficients
> > -		 * as some pre-programmed values are broken,
> > -		 * e.g. x201.
> > -		 */
> > -		if (IS_IVYBRIDGE(dev_priv) || IS_HASWELL(dev_priv))
> > -			intel_de_write(dev_priv, PF_CTL(pipe),
> > -				       PF_ENABLE | PF_FILTER_MED_3x3 | PF_PIPE_SEL_IVB(pipe));
> > -		else
> > -			intel_de_write(dev_priv, PF_CTL(pipe),
> > -				       PF_ENABLE | PF_FILTER_MED_3x3);
> > -		intel_de_write(dev_priv, PF_WIN_POS(pipe),
> > -			       crtc_state->pch_pfit.pos);
> > -		intel_de_write(dev_priv, PF_WIN_SZ(pipe),
> > -			       crtc_state->pch_pfit.size);
> 
> Why dont we use the intel_de_write_fw() everywhere?

Because no one thought it mattered much. It only really becomes
significant (mainly when lockdep is enabled) when a large number
of registers are written.

> 
> Manasi
> 
> > -	}
> > +	if (!crtc_state->pch_pfit.enabled)
> > +		return;
> > +
> > +	/* Force use of hard-coded filter coefficients
> > +	 * as some pre-programmed values are broken,
> > +	 * e.g. x201.
> > +	 */
> > +	if (IS_IVYBRIDGE(dev_priv) || IS_HASWELL(dev_priv))
> > +		intel_de_write(dev_priv, PF_CTL(pipe), PF_ENABLE |
> > +			       PF_FILTER_MED_3x3 | PF_PIPE_SEL_IVB(pipe));
> > +	else
> > +		intel_de_write(dev_priv, PF_CTL(pipe), PF_ENABLE |
> > +			       PF_FILTER_MED_3x3);
> > +	intel_de_write(dev_priv, PF_WIN_POS(pipe),
> > +		       crtc_state->pch_pfit.pos);
> > +	intel_de_write(dev_priv, PF_WIN_SZ(pipe),
> > +		       crtc_state->pch_pfit.size);
> >  }
> >  
> >  void hsw_enable_ips(const struct intel_crtc_state *crtc_state)
> > @@ -7107,11 +7108,12 @@ void ilk_pfit_disable(const struct intel_crtc_state *old_crtc_state)
> >  
> >  	/* To avoid upsetting the power well on haswell only disable the pfit if
> >  	 * it's in use. The hw state code will make sure we get this right. */
> > -	if (old_crtc_state->pch_pfit.enabled) {
> > -		intel_de_write(dev_priv, PF_CTL(pipe), 0);
> > -		intel_de_write(dev_priv, PF_WIN_POS(pipe), 0);
> > -		intel_de_write(dev_priv, PF_WIN_SZ(pipe), 0);
> > -	}
> > +	if (!old_crtc_state->pch_pfit.enabled)
> > +		return;
> > +
> > +	intel_de_write(dev_priv, PF_CTL(pipe), 0);
> > +	intel_de_write(dev_priv, PF_WIN_POS(pipe), 0);
> > +	intel_de_write(dev_priv, PF_WIN_SZ(pipe), 0);
> >  }
> >  
> >  static void ilk_crtc_disable(struct intel_atomic_state *state,
> > @@ -7927,39 +7929,35 @@ static bool intel_crtc_supports_double_wide(const struct intel_crtc *crtc)
> >  		(crtc->pipe == PIPE_A || IS_I915G(dev_priv));
> >  }
> >  
> > -static u32 ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config)
> > +static u32 ilk_pipe_pixel_rate(const struct intel_crtc_state *crtc_state)
> >  {
> > -	u32 pixel_rate;
> > -
> > -	pixel_rate = pipe_config->hw.adjusted_mode.crtc_clock;
> > +	u32 pixel_rate = crtc_state->hw.adjusted_mode.crtc_clock;
> > +	u32 pfit_size = crtc_state->pch_pfit.size;
> > +	u64 pipe_w, pipe_h, pfit_w, pfit_h;
> >  
> >  	/*
> >  	 * We only use IF-ID interlacing. If we ever use
> >  	 * PF-ID we'll need to adjust the pixel_rate here.
> >  	 */
> >  
> > -	if (pipe_config->pch_pfit.enabled) {
> > -		u64 pipe_w, pipe_h, pfit_w, pfit_h;
> > -		u32 pfit_size = pipe_config->pch_pfit.size;
> > -
> > -		pipe_w = pipe_config->pipe_src_w;
> > -		pipe_h = pipe_config->pipe_src_h;
> > +	if (!crtc_state->pch_pfit.enabled)
> > +		return pixel_rate;
> >  
> > -		pfit_w = (pfit_size >> 16) & 0xFFFF;
> > -		pfit_h = pfit_size & 0xFFFF;
> > -		if (pipe_w < pfit_w)
> > -			pipe_w = pfit_w;
> > -		if (pipe_h < pfit_h)
> > -			pipe_h = pfit_h;
> > +	pipe_w = crtc_state->pipe_src_w;
> > +	pipe_h = crtc_state->pipe_src_h;
> >  
> > -		if (WARN_ON(!pfit_w || !pfit_h))
> > -			return pixel_rate;
> > +	pfit_w = (pfit_size >> 16) & 0xFFFF;
> > +	pfit_h = pfit_size & 0xFFFF;
> > +	if (pipe_w < pfit_w)
> > +		pipe_w = pfit_w;
> > +	if (pipe_h < pfit_h)
> > +		pipe_h = pfit_h;
> >  
> > -		pixel_rate = div_u64(mul_u32_u32(pixel_rate, pipe_w * pipe_h),
> > -				     pfit_w * pfit_h);
> > -	}
> > +	if (WARN_ON(!pfit_w || !pfit_h))
> > +		return pixel_rate;
> >  
> > -	return pixel_rate;
> > +	return div_u64(mul_u32_u32(pixel_rate, pipe_w * pipe_h),
> > +		       pfit_w * pfit_h);
> >  }
> >  
> >  static void intel_crtc_compute_pixel_rate(struct intel_crtc_state *crtc_state)
> > @@ -9153,9 +9151,9 @@ static bool i9xx_has_pfit(struct drm_i915_private *dev_priv)
> >  		IS_PINEVIEW(dev_priv) || IS_MOBILE(dev_priv);
> >  }
> >  
> > -static void i9xx_get_pfit_config(struct intel_crtc *crtc,
> > -				 struct intel_crtc_state *pipe_config)
> > +static void i9xx_get_pfit_config(struct intel_crtc_state *crtc_state)
> >  {
> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >  	u32 tmp;
> >  
> > @@ -9175,9 +9173,9 @@ static void i9xx_get_pfit_config(struct intel_crtc *crtc,
> >  			return;
> >  	}
> >  
> > -	pipe_config->gmch_pfit.control = tmp;
> > -	pipe_config->gmch_pfit.pgm_ratios = intel_de_read(dev_priv,
> > -							  PFIT_PGM_RATIOS);
> > +	crtc_state->gmch_pfit.control = tmp;
> > +	crtc_state->gmch_pfit.pgm_ratios =
> > +		intel_de_read(dev_priv, PFIT_PGM_RATIOS);
> >  }
> >  
> >  static void vlv_crtc_clock_get(struct intel_crtc *crtc,
> > @@ -9427,7 +9425,7 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
> >  	intel_get_pipe_timings(crtc, pipe_config);
> >  	intel_get_pipe_src_size(crtc, pipe_config);
> >  
> > -	i9xx_get_pfit_config(crtc, pipe_config);
> > +	i9xx_get_pfit_config(pipe_config);
> >  
> >  	if (INTEL_GEN(dev_priv) >= 4) {
> >  		/* No way to read it out on pipes B and C */
> > @@ -10393,37 +10391,37 @@ static void ilk_get_fdi_m_n_config(struct intel_crtc *crtc,
> >  				     &pipe_config->fdi_m_n, NULL);
> >  }
> >  
> > -static void skl_get_pfit_config(struct intel_crtc *crtc,
> > -				struct intel_crtc_state *pipe_config)
> > +static void skl_get_pfit_config(struct intel_crtc_state *crtc_state)
> >  {
> > -	struct drm_device *dev = crtc->base.dev;
> > -	struct drm_i915_private *dev_priv = to_i915(dev);
> > -	struct intel_crtc_scaler_state *scaler_state = &pipe_config->scaler_state;
> > -	u32 ps_ctrl = 0;
> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > +	struct intel_crtc_scaler_state *scaler_state = &crtc_state->scaler_state;
> >  	int id = -1;
> >  	int i;
> >  
> >  	/* find scaler attached to this pipe */
> >  	for (i = 0; i < crtc->num_scalers; i++) {
> > -		ps_ctrl = intel_de_read(dev_priv, SKL_PS_CTRL(crtc->pipe, i));
> > -		if (ps_ctrl & PS_SCALER_EN && !(ps_ctrl & PS_PLANE_SEL_MASK)) {
> > -			id = i;
> > -			pipe_config->pch_pfit.enabled = true;
> > -			pipe_config->pch_pfit.pos = intel_de_read(dev_priv,
> > -								  SKL_PS_WIN_POS(crtc->pipe, i));
> > -			pipe_config->pch_pfit.size = intel_de_read(dev_priv,
> > -								   SKL_PS_WIN_SZ(crtc->pipe, i));
> > -			scaler_state->scalers[i].in_use = true;
> > -			break;
> > -		}
> > +		u32 tmp;
> > +
> > +		tmp = intel_de_read(dev_priv, SKL_PS_CTRL(crtc->pipe, i));
> > +		if ((tmp & (PS_SCALER_EN | PS_PLANE_SEL_MASK)) != PS_SCALER_EN)
> > +			continue;
> > +
> > +		id = i;
> > +		crtc_state->pch_pfit.enabled = true;
> > +		crtc_state->pch_pfit.pos =
> > +			intel_de_read(dev_priv, SKL_PS_WIN_POS(crtc->pipe, i));
> > +		crtc_state->pch_pfit.size =
> > +			intel_de_read(dev_priv, SKL_PS_WIN_SZ(crtc->pipe, i));
> > +		scaler_state->scalers[i].in_use = true;
> > +		break;
> >  	}
> >  
> >  	scaler_state->scaler_id = id;
> > -	if (id >= 0) {
> > +	if (id >= 0)
> >  		scaler_state->scaler_users |= (1 << SKL_CRTC_INDEX);
> > -	} else {
> > +	else
> >  		scaler_state->scaler_users &= ~(1 << SKL_CRTC_INDEX);
> > -	}
> >  }
> >  
> >  static void
> > @@ -10559,30 +10557,29 @@ skl_get_initial_plane_config(struct intel_crtc *crtc,
> >  	kfree(intel_fb);
> >  }
> >  
> > -static void ilk_get_pfit_config(struct intel_crtc *crtc,
> > -				struct intel_crtc_state *pipe_config)
> > +static void ilk_get_pfit_config(struct intel_crtc_state *crtc_state)
> >  {
> > -	struct drm_device *dev = crtc->base.dev;
> > -	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >  	u32 tmp;
> >  
> >  	tmp = intel_de_read(dev_priv, PF_CTL(crtc->pipe));
> > +	if ((tmp & PF_ENABLE) == 0)
> > +		return;
> >  
> > -	if (tmp & PF_ENABLE) {
> > -		pipe_config->pch_pfit.enabled = true;
> > -		pipe_config->pch_pfit.pos = intel_de_read(dev_priv,
> > -							  PF_WIN_POS(crtc->pipe));
> > -		pipe_config->pch_pfit.size = intel_de_read(dev_priv,
> > -							   PF_WIN_SZ(crtc->pipe));
> > -
> > -		/* We currently do not free assignements of panel fitters on
> > -		 * ivb/hsw (since we don't use the higher upscaling modes which
> > -		 * differentiates them) so just WARN about this case for now. */
> > -		if (IS_GEN(dev_priv, 7)) {
> > -			WARN_ON((tmp & PF_PIPE_SEL_MASK_IVB) !=
> > -				PF_PIPE_SEL_IVB(crtc->pipe));
> > -		}
> > -	}
> > +	crtc_state->pch_pfit.enabled = true;
> > +	crtc_state->pch_pfit.pos =
> > +		intel_de_read(dev_priv, PF_WIN_POS(crtc->pipe));
> > +	crtc_state->pch_pfit.size =
> > +		intel_de_read(dev_priv, PF_WIN_SZ(crtc->pipe));
> > +
> > +	/*
> > +	 * We currently do not free assignements of panel fitters on
> > +	 * ivb/hsw (since we don't use the higher upscaling modes which
> > +	 * differentiates them) so just WARN about this case for now.
> > +	 */
> > +	WARN_ON(IS_GEN(dev_priv, 7) &&
> > +		(tmp & PF_PIPE_SEL_MASK_IVB) != PF_PIPE_SEL_IVB(crtc->pipe));
> >  }
> >  
> >  static bool ilk_get_pipe_config(struct intel_crtc *crtc,
> > @@ -10694,7 +10691,7 @@ static bool ilk_get_pipe_config(struct intel_crtc *crtc,
> >  	intel_get_pipe_timings(crtc, pipe_config);
> >  	intel_get_pipe_src_size(crtc, pipe_config);
> >  
> > -	ilk_get_pfit_config(crtc, pipe_config);
> > +	ilk_get_pfit_config(pipe_config);
> >  
> >  	ret = true;
> >  
> > @@ -11219,9 +11216,9 @@ static bool hsw_get_pipe_config(struct intel_crtc *crtc,
> >  		power_domain_mask |= BIT_ULL(power_domain);
> >  
> >  		if (INTEL_GEN(dev_priv) >= 9)
> > -			skl_get_pfit_config(crtc, pipe_config);
> > +			skl_get_pfit_config(pipe_config);
> >  		else
> > -			ilk_get_pfit_config(crtc, pipe_config);
> > +			ilk_get_pfit_config(pipe_config);
> >  	}
> >  
> >  	if (hsw_crtc_supports_ips(crtc)) {
> > -- 
> > 2.24.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Manasi Navare April 21, 2020, 11:32 p.m. UTC | #3
On Thu, Apr 02, 2020 at 04:55:06PM +0300, Ville Syrjälä wrote:
> On Wed, Apr 01, 2020 at 04:53:23PM -0700, Manasi Navare wrote:
> > On Wed, Feb 12, 2020 at 06:17:34PM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Most of the pfit functions are of the form:
> > > 
> > > func()
> > > {
> > > 	if (pfit_enabled) {
> > > 		...
> > > 	}
> > > }
> > > 
> > > Flip the pfit_enabled check around to flatten the functions.
> > > 
> > > And while we're touching all this let's do the usual
> > > s/pipe_config/crtc_state/ replacement.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 233 +++++++++----------
> > >  1 file changed, 115 insertions(+), 118 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index becc6322b7dc..796e27c4aece 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -6233,42 +6233,42 @@ static void skl_pfit_enable(const struct intel_crtc_state *crtc_state)
> > >  	enum pipe pipe = crtc->pipe;
> > >  	const struct intel_crtc_scaler_state *scaler_state =
> > >  		&crtc_state->scaler_state;
> > > +	u16 uv_rgb_hphase, uv_rgb_vphase;
> > > +	int pfit_w, pfit_h, hscale, vscale;
> > > +	unsigned long irqflags;
> > > +	int id;
> > >  
> > > -	if (crtc_state->pch_pfit.enabled) {
> > > -		u16 uv_rgb_hphase, uv_rgb_vphase;
> > > -		int pfit_w, pfit_h, hscale, vscale;
> > > -		unsigned long irqflags;
> > > -		int id;
> > > +	if (!crtc_state->pch_pfit.enabled)
> > > +		return;
> > >  
> > > -		if (WARN_ON(crtc_state->scaler_state.scaler_id < 0))
> > > -			return;
> > > +	if (WARN_ON(crtc_state->scaler_state.scaler_id < 0))
> > > +		return;
> > >  
> > > -		pfit_w = (crtc_state->pch_pfit.size >> 16) & 0xFFFF;
> > > -		pfit_h = crtc_state->pch_pfit.size & 0xFFFF;
> > > +	pfit_w = (crtc_state->pch_pfit.size >> 16) & 0xFFFF;
> > > +	pfit_h = crtc_state->pch_pfit.size & 0xFFFF;
> > >  
> > > -		hscale = (crtc_state->pipe_src_w << 16) / pfit_w;
> > > -		vscale = (crtc_state->pipe_src_h << 16) / pfit_h;
> > > +	hscale = (crtc_state->pipe_src_w << 16) / pfit_w;
> > > +	vscale = (crtc_state->pipe_src_h << 16) / pfit_h;
> > >  
> > > -		uv_rgb_hphase = skl_scaler_calc_phase(1, hscale, false);
> > > -		uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false);
> > > +	uv_rgb_hphase = skl_scaler_calc_phase(1, hscale, false);
> > > +	uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false);
> > >  
> > > -		id = scaler_state->scaler_id;
> > > +	id = scaler_state->scaler_id;
> > >  
> > > -		spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> > > +	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> > >  
> > > -		intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id), PS_SCALER_EN |
> > > -				  PS_FILTER_MEDIUM | scaler_state->scalers[id].mode);
> > > -		intel_de_write_fw(dev_priv, SKL_PS_VPHASE(pipe, id),
> > > -				  PS_Y_PHASE(0) | PS_UV_RGB_PHASE(uv_rgb_vphase));
> > > -		intel_de_write_fw(dev_priv, SKL_PS_HPHASE(pipe, id),
> > > -				  PS_Y_PHASE(0) | PS_UV_RGB_PHASE(uv_rgb_hphase));
> > > -		intel_de_write_fw(dev_priv, SKL_PS_WIN_POS(pipe, id),
> > > -				  crtc_state->pch_pfit.pos);
> > > -		intel_de_write_fw(dev_priv, SKL_PS_WIN_SZ(pipe, id),
> > > -				  crtc_state->pch_pfit.size);
> > > +	intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id), PS_SCALER_EN |
> > > +			  PS_FILTER_MEDIUM | scaler_state->scalers[id].mode);
> > > +	intel_de_write_fw(dev_priv, SKL_PS_VPHASE(pipe, id),
> > > +			  PS_Y_PHASE(0) | PS_UV_RGB_PHASE(uv_rgb_vphase));
> > > +	intel_de_write_fw(dev_priv, SKL_PS_HPHASE(pipe, id),
> > > +			  PS_Y_PHASE(0) | PS_UV_RGB_PHASE(uv_rgb_hphase));
> > > +	intel_de_write_fw(dev_priv, SKL_PS_WIN_POS(pipe, id),
> > > +			  crtc_state->pch_pfit.pos);
> > > +	intel_de_write_fw(dev_priv, SKL_PS_WIN_SZ(pipe, id),
> > > +			  crtc_state->pch_pfit.size);
> > >  
> > > -		spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> > > -	}
> > > +	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> > >  }
> > >  
> > >  static void ilk_pfit_enable(const struct intel_crtc_state *crtc_state)
> > > @@ -6277,22 +6277,23 @@ static void ilk_pfit_enable(const struct intel_crtc_state *crtc_state)
> > >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > >  	enum pipe pipe = crtc->pipe;
> > >  
> > > -	if (crtc_state->pch_pfit.enabled) {
> > > -		/* Force use of hard-coded filter coefficients
> > > -		 * as some pre-programmed values are broken,
> > > -		 * e.g. x201.
> > > -		 */
> > > -		if (IS_IVYBRIDGE(dev_priv) || IS_HASWELL(dev_priv))
> > > -			intel_de_write(dev_priv, PF_CTL(pipe),
> > > -				       PF_ENABLE | PF_FILTER_MED_3x3 | PF_PIPE_SEL_IVB(pipe));
> > > -		else
> > > -			intel_de_write(dev_priv, PF_CTL(pipe),
> > > -				       PF_ENABLE | PF_FILTER_MED_3x3);
> > > -		intel_de_write(dev_priv, PF_WIN_POS(pipe),
> > > -			       crtc_state->pch_pfit.pos);
> > > -		intel_de_write(dev_priv, PF_WIN_SZ(pipe),
> > > -			       crtc_state->pch_pfit.size);
> > 
> > Why dont we use the intel_de_write_fw() everywhere?
> 
> Because no one thought it mattered much. It only really becomes
> significant (mainly when lockdep is enabled) when a large number
> of registers are written.

Okay everything else flattening things in the functions make sense for readability

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

Manasi
> 
> > 
> > Manasi
> > 
> > > -	}
> > > +	if (!crtc_state->pch_pfit.enabled)
> > > +		return;
> > > +
> > > +	/* Force use of hard-coded filter coefficients
> > > +	 * as some pre-programmed values are broken,
> > > +	 * e.g. x201.
> > > +	 */
> > > +	if (IS_IVYBRIDGE(dev_priv) || IS_HASWELL(dev_priv))
> > > +		intel_de_write(dev_priv, PF_CTL(pipe), PF_ENABLE |
> > > +			       PF_FILTER_MED_3x3 | PF_PIPE_SEL_IVB(pipe));
> > > +	else
> > > +		intel_de_write(dev_priv, PF_CTL(pipe), PF_ENABLE |
> > > +			       PF_FILTER_MED_3x3);
> > > +	intel_de_write(dev_priv, PF_WIN_POS(pipe),
> > > +		       crtc_state->pch_pfit.pos);
> > > +	intel_de_write(dev_priv, PF_WIN_SZ(pipe),
> > > +		       crtc_state->pch_pfit.size);
> > >  }
> > >  
> > >  void hsw_enable_ips(const struct intel_crtc_state *crtc_state)
> > > @@ -7107,11 +7108,12 @@ void ilk_pfit_disable(const struct intel_crtc_state *old_crtc_state)
> > >  
> > >  	/* To avoid upsetting the power well on haswell only disable the pfit if
> > >  	 * it's in use. The hw state code will make sure we get this right. */
> > > -	if (old_crtc_state->pch_pfit.enabled) {
> > > -		intel_de_write(dev_priv, PF_CTL(pipe), 0);
> > > -		intel_de_write(dev_priv, PF_WIN_POS(pipe), 0);
> > > -		intel_de_write(dev_priv, PF_WIN_SZ(pipe), 0);
> > > -	}
> > > +	if (!old_crtc_state->pch_pfit.enabled)
> > > +		return;
> > > +
> > > +	intel_de_write(dev_priv, PF_CTL(pipe), 0);
> > > +	intel_de_write(dev_priv, PF_WIN_POS(pipe), 0);
> > > +	intel_de_write(dev_priv, PF_WIN_SZ(pipe), 0);
> > >  }
> > >  
> > >  static void ilk_crtc_disable(struct intel_atomic_state *state,
> > > @@ -7927,39 +7929,35 @@ static bool intel_crtc_supports_double_wide(const struct intel_crtc *crtc)
> > >  		(crtc->pipe == PIPE_A || IS_I915G(dev_priv));
> > >  }
> > >  
> > > -static u32 ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config)
> > > +static u32 ilk_pipe_pixel_rate(const struct intel_crtc_state *crtc_state)
> > >  {
> > > -	u32 pixel_rate;
> > > -
> > > -	pixel_rate = pipe_config->hw.adjusted_mode.crtc_clock;
> > > +	u32 pixel_rate = crtc_state->hw.adjusted_mode.crtc_clock;
> > > +	u32 pfit_size = crtc_state->pch_pfit.size;
> > > +	u64 pipe_w, pipe_h, pfit_w, pfit_h;
> > >  
> > >  	/*
> > >  	 * We only use IF-ID interlacing. If we ever use
> > >  	 * PF-ID we'll need to adjust the pixel_rate here.
> > >  	 */
> > >  
> > > -	if (pipe_config->pch_pfit.enabled) {
> > > -		u64 pipe_w, pipe_h, pfit_w, pfit_h;
> > > -		u32 pfit_size = pipe_config->pch_pfit.size;
> > > -
> > > -		pipe_w = pipe_config->pipe_src_w;
> > > -		pipe_h = pipe_config->pipe_src_h;
> > > +	if (!crtc_state->pch_pfit.enabled)
> > > +		return pixel_rate;
> > >  
> > > -		pfit_w = (pfit_size >> 16) & 0xFFFF;
> > > -		pfit_h = pfit_size & 0xFFFF;
> > > -		if (pipe_w < pfit_w)
> > > -			pipe_w = pfit_w;
> > > -		if (pipe_h < pfit_h)
> > > -			pipe_h = pfit_h;
> > > +	pipe_w = crtc_state->pipe_src_w;
> > > +	pipe_h = crtc_state->pipe_src_h;
> > >  
> > > -		if (WARN_ON(!pfit_w || !pfit_h))
> > > -			return pixel_rate;
> > > +	pfit_w = (pfit_size >> 16) & 0xFFFF;
> > > +	pfit_h = pfit_size & 0xFFFF;
> > > +	if (pipe_w < pfit_w)
> > > +		pipe_w = pfit_w;
> > > +	if (pipe_h < pfit_h)
> > > +		pipe_h = pfit_h;
> > >  
> > > -		pixel_rate = div_u64(mul_u32_u32(pixel_rate, pipe_w * pipe_h),
> > > -				     pfit_w * pfit_h);
> > > -	}
> > > +	if (WARN_ON(!pfit_w || !pfit_h))
> > > +		return pixel_rate;
> > >  
> > > -	return pixel_rate;
> > > +	return div_u64(mul_u32_u32(pixel_rate, pipe_w * pipe_h),
> > > +		       pfit_w * pfit_h);
> > >  }
> > >  
> > >  static void intel_crtc_compute_pixel_rate(struct intel_crtc_state *crtc_state)
> > > @@ -9153,9 +9151,9 @@ static bool i9xx_has_pfit(struct drm_i915_private *dev_priv)
> > >  		IS_PINEVIEW(dev_priv) || IS_MOBILE(dev_priv);
> > >  }
> > >  
> > > -static void i9xx_get_pfit_config(struct intel_crtc *crtc,
> > > -				 struct intel_crtc_state *pipe_config)
> > > +static void i9xx_get_pfit_config(struct intel_crtc_state *crtc_state)
> > >  {
> > > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > >  	u32 tmp;
> > >  
> > > @@ -9175,9 +9173,9 @@ static void i9xx_get_pfit_config(struct intel_crtc *crtc,
> > >  			return;
> > >  	}
> > >  
> > > -	pipe_config->gmch_pfit.control = tmp;
> > > -	pipe_config->gmch_pfit.pgm_ratios = intel_de_read(dev_priv,
> > > -							  PFIT_PGM_RATIOS);
> > > +	crtc_state->gmch_pfit.control = tmp;
> > > +	crtc_state->gmch_pfit.pgm_ratios =
> > > +		intel_de_read(dev_priv, PFIT_PGM_RATIOS);
> > >  }
> > >  
> > >  static void vlv_crtc_clock_get(struct intel_crtc *crtc,
> > > @@ -9427,7 +9425,7 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
> > >  	intel_get_pipe_timings(crtc, pipe_config);
> > >  	intel_get_pipe_src_size(crtc, pipe_config);
> > >  
> > > -	i9xx_get_pfit_config(crtc, pipe_config);
> > > +	i9xx_get_pfit_config(pipe_config);
> > >  
> > >  	if (INTEL_GEN(dev_priv) >= 4) {
> > >  		/* No way to read it out on pipes B and C */
> > > @@ -10393,37 +10391,37 @@ static void ilk_get_fdi_m_n_config(struct intel_crtc *crtc,
> > >  				     &pipe_config->fdi_m_n, NULL);
> > >  }
> > >  
> > > -static void skl_get_pfit_config(struct intel_crtc *crtc,
> > > -				struct intel_crtc_state *pipe_config)
> > > +static void skl_get_pfit_config(struct intel_crtc_state *crtc_state)
> > >  {
> > > -	struct drm_device *dev = crtc->base.dev;
> > > -	struct drm_i915_private *dev_priv = to_i915(dev);
> > > -	struct intel_crtc_scaler_state *scaler_state = &pipe_config->scaler_state;
> > > -	u32 ps_ctrl = 0;
> > > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > +	struct intel_crtc_scaler_state *scaler_state = &crtc_state->scaler_state;
> > >  	int id = -1;
> > >  	int i;
> > >  
> > >  	/* find scaler attached to this pipe */
> > >  	for (i = 0; i < crtc->num_scalers; i++) {
> > > -		ps_ctrl = intel_de_read(dev_priv, SKL_PS_CTRL(crtc->pipe, i));
> > > -		if (ps_ctrl & PS_SCALER_EN && !(ps_ctrl & PS_PLANE_SEL_MASK)) {
> > > -			id = i;
> > > -			pipe_config->pch_pfit.enabled = true;
> > > -			pipe_config->pch_pfit.pos = intel_de_read(dev_priv,
> > > -								  SKL_PS_WIN_POS(crtc->pipe, i));
> > > -			pipe_config->pch_pfit.size = intel_de_read(dev_priv,
> > > -								   SKL_PS_WIN_SZ(crtc->pipe, i));
> > > -			scaler_state->scalers[i].in_use = true;
> > > -			break;
> > > -		}
> > > +		u32 tmp;
> > > +
> > > +		tmp = intel_de_read(dev_priv, SKL_PS_CTRL(crtc->pipe, i));
> > > +		if ((tmp & (PS_SCALER_EN | PS_PLANE_SEL_MASK)) != PS_SCALER_EN)
> > > +			continue;
> > > +
> > > +		id = i;
> > > +		crtc_state->pch_pfit.enabled = true;
> > > +		crtc_state->pch_pfit.pos =
> > > +			intel_de_read(dev_priv, SKL_PS_WIN_POS(crtc->pipe, i));
> > > +		crtc_state->pch_pfit.size =
> > > +			intel_de_read(dev_priv, SKL_PS_WIN_SZ(crtc->pipe, i));
> > > +		scaler_state->scalers[i].in_use = true;
> > > +		break;
> > >  	}
> > >  
> > >  	scaler_state->scaler_id = id;
> > > -	if (id >= 0) {
> > > +	if (id >= 0)
> > >  		scaler_state->scaler_users |= (1 << SKL_CRTC_INDEX);
> > > -	} else {
> > > +	else
> > >  		scaler_state->scaler_users &= ~(1 << SKL_CRTC_INDEX);
> > > -	}
> > >  }
> > >  
> > >  static void
> > > @@ -10559,30 +10557,29 @@ skl_get_initial_plane_config(struct intel_crtc *crtc,
> > >  	kfree(intel_fb);
> > >  }
> > >  
> > > -static void ilk_get_pfit_config(struct intel_crtc *crtc,
> > > -				struct intel_crtc_state *pipe_config)
> > > +static void ilk_get_pfit_config(struct intel_crtc_state *crtc_state)
> > >  {
> > > -	struct drm_device *dev = crtc->base.dev;
> > > -	struct drm_i915_private *dev_priv = to_i915(dev);
> > > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > >  	u32 tmp;
> > >  
> > >  	tmp = intel_de_read(dev_priv, PF_CTL(crtc->pipe));
> > > +	if ((tmp & PF_ENABLE) == 0)
> > > +		return;
> > >  
> > > -	if (tmp & PF_ENABLE) {
> > > -		pipe_config->pch_pfit.enabled = true;
> > > -		pipe_config->pch_pfit.pos = intel_de_read(dev_priv,
> > > -							  PF_WIN_POS(crtc->pipe));
> > > -		pipe_config->pch_pfit.size = intel_de_read(dev_priv,
> > > -							   PF_WIN_SZ(crtc->pipe));
> > > -
> > > -		/* We currently do not free assignements of panel fitters on
> > > -		 * ivb/hsw (since we don't use the higher upscaling modes which
> > > -		 * differentiates them) so just WARN about this case for now. */
> > > -		if (IS_GEN(dev_priv, 7)) {
> > > -			WARN_ON((tmp & PF_PIPE_SEL_MASK_IVB) !=
> > > -				PF_PIPE_SEL_IVB(crtc->pipe));
> > > -		}
> > > -	}
> > > +	crtc_state->pch_pfit.enabled = true;
> > > +	crtc_state->pch_pfit.pos =
> > > +		intel_de_read(dev_priv, PF_WIN_POS(crtc->pipe));
> > > +	crtc_state->pch_pfit.size =
> > > +		intel_de_read(dev_priv, PF_WIN_SZ(crtc->pipe));
> > > +
> > > +	/*
> > > +	 * We currently do not free assignements of panel fitters on
> > > +	 * ivb/hsw (since we don't use the higher upscaling modes which
> > > +	 * differentiates them) so just WARN about this case for now.
> > > +	 */
> > > +	WARN_ON(IS_GEN(dev_priv, 7) &&
> > > +		(tmp & PF_PIPE_SEL_MASK_IVB) != PF_PIPE_SEL_IVB(crtc->pipe));
> > >  }
> > >  
> > >  static bool ilk_get_pipe_config(struct intel_crtc *crtc,
> > > @@ -10694,7 +10691,7 @@ static bool ilk_get_pipe_config(struct intel_crtc *crtc,
> > >  	intel_get_pipe_timings(crtc, pipe_config);
> > >  	intel_get_pipe_src_size(crtc, pipe_config);
> > >  
> > > -	ilk_get_pfit_config(crtc, pipe_config);
> > > +	ilk_get_pfit_config(pipe_config);
> > >  
> > >  	ret = true;
> > >  
> > > @@ -11219,9 +11216,9 @@ static bool hsw_get_pipe_config(struct intel_crtc *crtc,
> > >  		power_domain_mask |= BIT_ULL(power_domain);
> > >  
> > >  		if (INTEL_GEN(dev_priv) >= 9)
> > > -			skl_get_pfit_config(crtc, pipe_config);
> > > +			skl_get_pfit_config(pipe_config);
> > >  		else
> > > -			ilk_get_pfit_config(crtc, pipe_config);
> > > +			ilk_get_pfit_config(pipe_config);
> > >  	}
> > >  
> > >  	if (hsw_crtc_supports_ips(crtc)) {
> > > -- 
> > > 2.24.1
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index becc6322b7dc..796e27c4aece 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -6233,42 +6233,42 @@  static void skl_pfit_enable(const struct intel_crtc_state *crtc_state)
 	enum pipe pipe = crtc->pipe;
 	const struct intel_crtc_scaler_state *scaler_state =
 		&crtc_state->scaler_state;
+	u16 uv_rgb_hphase, uv_rgb_vphase;
+	int pfit_w, pfit_h, hscale, vscale;
+	unsigned long irqflags;
+	int id;
 
-	if (crtc_state->pch_pfit.enabled) {
-		u16 uv_rgb_hphase, uv_rgb_vphase;
-		int pfit_w, pfit_h, hscale, vscale;
-		unsigned long irqflags;
-		int id;
+	if (!crtc_state->pch_pfit.enabled)
+		return;
 
-		if (WARN_ON(crtc_state->scaler_state.scaler_id < 0))
-			return;
+	if (WARN_ON(crtc_state->scaler_state.scaler_id < 0))
+		return;
 
-		pfit_w = (crtc_state->pch_pfit.size >> 16) & 0xFFFF;
-		pfit_h = crtc_state->pch_pfit.size & 0xFFFF;
+	pfit_w = (crtc_state->pch_pfit.size >> 16) & 0xFFFF;
+	pfit_h = crtc_state->pch_pfit.size & 0xFFFF;
 
-		hscale = (crtc_state->pipe_src_w << 16) / pfit_w;
-		vscale = (crtc_state->pipe_src_h << 16) / pfit_h;
+	hscale = (crtc_state->pipe_src_w << 16) / pfit_w;
+	vscale = (crtc_state->pipe_src_h << 16) / pfit_h;
 
-		uv_rgb_hphase = skl_scaler_calc_phase(1, hscale, false);
-		uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false);
+	uv_rgb_hphase = skl_scaler_calc_phase(1, hscale, false);
+	uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false);
 
-		id = scaler_state->scaler_id;
+	id = scaler_state->scaler_id;
 
-		spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
+	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 
-		intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id), PS_SCALER_EN |
-				  PS_FILTER_MEDIUM | scaler_state->scalers[id].mode);
-		intel_de_write_fw(dev_priv, SKL_PS_VPHASE(pipe, id),
-				  PS_Y_PHASE(0) | PS_UV_RGB_PHASE(uv_rgb_vphase));
-		intel_de_write_fw(dev_priv, SKL_PS_HPHASE(pipe, id),
-				  PS_Y_PHASE(0) | PS_UV_RGB_PHASE(uv_rgb_hphase));
-		intel_de_write_fw(dev_priv, SKL_PS_WIN_POS(pipe, id),
-				  crtc_state->pch_pfit.pos);
-		intel_de_write_fw(dev_priv, SKL_PS_WIN_SZ(pipe, id),
-				  crtc_state->pch_pfit.size);
+	intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id), PS_SCALER_EN |
+			  PS_FILTER_MEDIUM | scaler_state->scalers[id].mode);
+	intel_de_write_fw(dev_priv, SKL_PS_VPHASE(pipe, id),
+			  PS_Y_PHASE(0) | PS_UV_RGB_PHASE(uv_rgb_vphase));
+	intel_de_write_fw(dev_priv, SKL_PS_HPHASE(pipe, id),
+			  PS_Y_PHASE(0) | PS_UV_RGB_PHASE(uv_rgb_hphase));
+	intel_de_write_fw(dev_priv, SKL_PS_WIN_POS(pipe, id),
+			  crtc_state->pch_pfit.pos);
+	intel_de_write_fw(dev_priv, SKL_PS_WIN_SZ(pipe, id),
+			  crtc_state->pch_pfit.size);
 
-		spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
-	}
+	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
 static void ilk_pfit_enable(const struct intel_crtc_state *crtc_state)
@@ -6277,22 +6277,23 @@  static void ilk_pfit_enable(const struct intel_crtc_state *crtc_state)
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	enum pipe pipe = crtc->pipe;
 
-	if (crtc_state->pch_pfit.enabled) {
-		/* Force use of hard-coded filter coefficients
-		 * as some pre-programmed values are broken,
-		 * e.g. x201.
-		 */
-		if (IS_IVYBRIDGE(dev_priv) || IS_HASWELL(dev_priv))
-			intel_de_write(dev_priv, PF_CTL(pipe),
-				       PF_ENABLE | PF_FILTER_MED_3x3 | PF_PIPE_SEL_IVB(pipe));
-		else
-			intel_de_write(dev_priv, PF_CTL(pipe),
-				       PF_ENABLE | PF_FILTER_MED_3x3);
-		intel_de_write(dev_priv, PF_WIN_POS(pipe),
-			       crtc_state->pch_pfit.pos);
-		intel_de_write(dev_priv, PF_WIN_SZ(pipe),
-			       crtc_state->pch_pfit.size);
-	}
+	if (!crtc_state->pch_pfit.enabled)
+		return;
+
+	/* Force use of hard-coded filter coefficients
+	 * as some pre-programmed values are broken,
+	 * e.g. x201.
+	 */
+	if (IS_IVYBRIDGE(dev_priv) || IS_HASWELL(dev_priv))
+		intel_de_write(dev_priv, PF_CTL(pipe), PF_ENABLE |
+			       PF_FILTER_MED_3x3 | PF_PIPE_SEL_IVB(pipe));
+	else
+		intel_de_write(dev_priv, PF_CTL(pipe), PF_ENABLE |
+			       PF_FILTER_MED_3x3);
+	intel_de_write(dev_priv, PF_WIN_POS(pipe),
+		       crtc_state->pch_pfit.pos);
+	intel_de_write(dev_priv, PF_WIN_SZ(pipe),
+		       crtc_state->pch_pfit.size);
 }
 
 void hsw_enable_ips(const struct intel_crtc_state *crtc_state)
@@ -7107,11 +7108,12 @@  void ilk_pfit_disable(const struct intel_crtc_state *old_crtc_state)
 
 	/* To avoid upsetting the power well on haswell only disable the pfit if
 	 * it's in use. The hw state code will make sure we get this right. */
-	if (old_crtc_state->pch_pfit.enabled) {
-		intel_de_write(dev_priv, PF_CTL(pipe), 0);
-		intel_de_write(dev_priv, PF_WIN_POS(pipe), 0);
-		intel_de_write(dev_priv, PF_WIN_SZ(pipe), 0);
-	}
+	if (!old_crtc_state->pch_pfit.enabled)
+		return;
+
+	intel_de_write(dev_priv, PF_CTL(pipe), 0);
+	intel_de_write(dev_priv, PF_WIN_POS(pipe), 0);
+	intel_de_write(dev_priv, PF_WIN_SZ(pipe), 0);
 }
 
 static void ilk_crtc_disable(struct intel_atomic_state *state,
@@ -7927,39 +7929,35 @@  static bool intel_crtc_supports_double_wide(const struct intel_crtc *crtc)
 		(crtc->pipe == PIPE_A || IS_I915G(dev_priv));
 }
 
-static u32 ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config)
+static u32 ilk_pipe_pixel_rate(const struct intel_crtc_state *crtc_state)
 {
-	u32 pixel_rate;
-
-	pixel_rate = pipe_config->hw.adjusted_mode.crtc_clock;
+	u32 pixel_rate = crtc_state->hw.adjusted_mode.crtc_clock;
+	u32 pfit_size = crtc_state->pch_pfit.size;
+	u64 pipe_w, pipe_h, pfit_w, pfit_h;
 
 	/*
 	 * We only use IF-ID interlacing. If we ever use
 	 * PF-ID we'll need to adjust the pixel_rate here.
 	 */
 
-	if (pipe_config->pch_pfit.enabled) {
-		u64 pipe_w, pipe_h, pfit_w, pfit_h;
-		u32 pfit_size = pipe_config->pch_pfit.size;
-
-		pipe_w = pipe_config->pipe_src_w;
-		pipe_h = pipe_config->pipe_src_h;
+	if (!crtc_state->pch_pfit.enabled)
+		return pixel_rate;
 
-		pfit_w = (pfit_size >> 16) & 0xFFFF;
-		pfit_h = pfit_size & 0xFFFF;
-		if (pipe_w < pfit_w)
-			pipe_w = pfit_w;
-		if (pipe_h < pfit_h)
-			pipe_h = pfit_h;
+	pipe_w = crtc_state->pipe_src_w;
+	pipe_h = crtc_state->pipe_src_h;
 
-		if (WARN_ON(!pfit_w || !pfit_h))
-			return pixel_rate;
+	pfit_w = (pfit_size >> 16) & 0xFFFF;
+	pfit_h = pfit_size & 0xFFFF;
+	if (pipe_w < pfit_w)
+		pipe_w = pfit_w;
+	if (pipe_h < pfit_h)
+		pipe_h = pfit_h;
 
-		pixel_rate = div_u64(mul_u32_u32(pixel_rate, pipe_w * pipe_h),
-				     pfit_w * pfit_h);
-	}
+	if (WARN_ON(!pfit_w || !pfit_h))
+		return pixel_rate;
 
-	return pixel_rate;
+	return div_u64(mul_u32_u32(pixel_rate, pipe_w * pipe_h),
+		       pfit_w * pfit_h);
 }
 
 static void intel_crtc_compute_pixel_rate(struct intel_crtc_state *crtc_state)
@@ -9153,9 +9151,9 @@  static bool i9xx_has_pfit(struct drm_i915_private *dev_priv)
 		IS_PINEVIEW(dev_priv) || IS_MOBILE(dev_priv);
 }
 
-static void i9xx_get_pfit_config(struct intel_crtc *crtc,
-				 struct intel_crtc_state *pipe_config)
+static void i9xx_get_pfit_config(struct intel_crtc_state *crtc_state)
 {
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	u32 tmp;
 
@@ -9175,9 +9173,9 @@  static void i9xx_get_pfit_config(struct intel_crtc *crtc,
 			return;
 	}
 
-	pipe_config->gmch_pfit.control = tmp;
-	pipe_config->gmch_pfit.pgm_ratios = intel_de_read(dev_priv,
-							  PFIT_PGM_RATIOS);
+	crtc_state->gmch_pfit.control = tmp;
+	crtc_state->gmch_pfit.pgm_ratios =
+		intel_de_read(dev_priv, PFIT_PGM_RATIOS);
 }
 
 static void vlv_crtc_clock_get(struct intel_crtc *crtc,
@@ -9427,7 +9425,7 @@  static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
 	intel_get_pipe_timings(crtc, pipe_config);
 	intel_get_pipe_src_size(crtc, pipe_config);
 
-	i9xx_get_pfit_config(crtc, pipe_config);
+	i9xx_get_pfit_config(pipe_config);
 
 	if (INTEL_GEN(dev_priv) >= 4) {
 		/* No way to read it out on pipes B and C */
@@ -10393,37 +10391,37 @@  static void ilk_get_fdi_m_n_config(struct intel_crtc *crtc,
 				     &pipe_config->fdi_m_n, NULL);
 }
 
-static void skl_get_pfit_config(struct intel_crtc *crtc,
-				struct intel_crtc_state *pipe_config)
+static void skl_get_pfit_config(struct intel_crtc_state *crtc_state)
 {
-	struct drm_device *dev = crtc->base.dev;
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_crtc_scaler_state *scaler_state = &pipe_config->scaler_state;
-	u32 ps_ctrl = 0;
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	struct intel_crtc_scaler_state *scaler_state = &crtc_state->scaler_state;
 	int id = -1;
 	int i;
 
 	/* find scaler attached to this pipe */
 	for (i = 0; i < crtc->num_scalers; i++) {
-		ps_ctrl = intel_de_read(dev_priv, SKL_PS_CTRL(crtc->pipe, i));
-		if (ps_ctrl & PS_SCALER_EN && !(ps_ctrl & PS_PLANE_SEL_MASK)) {
-			id = i;
-			pipe_config->pch_pfit.enabled = true;
-			pipe_config->pch_pfit.pos = intel_de_read(dev_priv,
-								  SKL_PS_WIN_POS(crtc->pipe, i));
-			pipe_config->pch_pfit.size = intel_de_read(dev_priv,
-								   SKL_PS_WIN_SZ(crtc->pipe, i));
-			scaler_state->scalers[i].in_use = true;
-			break;
-		}
+		u32 tmp;
+
+		tmp = intel_de_read(dev_priv, SKL_PS_CTRL(crtc->pipe, i));
+		if ((tmp & (PS_SCALER_EN | PS_PLANE_SEL_MASK)) != PS_SCALER_EN)
+			continue;
+
+		id = i;
+		crtc_state->pch_pfit.enabled = true;
+		crtc_state->pch_pfit.pos =
+			intel_de_read(dev_priv, SKL_PS_WIN_POS(crtc->pipe, i));
+		crtc_state->pch_pfit.size =
+			intel_de_read(dev_priv, SKL_PS_WIN_SZ(crtc->pipe, i));
+		scaler_state->scalers[i].in_use = true;
+		break;
 	}
 
 	scaler_state->scaler_id = id;
-	if (id >= 0) {
+	if (id >= 0)
 		scaler_state->scaler_users |= (1 << SKL_CRTC_INDEX);
-	} else {
+	else
 		scaler_state->scaler_users &= ~(1 << SKL_CRTC_INDEX);
-	}
 }
 
 static void
@@ -10559,30 +10557,29 @@  skl_get_initial_plane_config(struct intel_crtc *crtc,
 	kfree(intel_fb);
 }
 
-static void ilk_get_pfit_config(struct intel_crtc *crtc,
-				struct intel_crtc_state *pipe_config)
+static void ilk_get_pfit_config(struct intel_crtc_state *crtc_state)
 {
-	struct drm_device *dev = crtc->base.dev;
-	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	u32 tmp;
 
 	tmp = intel_de_read(dev_priv, PF_CTL(crtc->pipe));
+	if ((tmp & PF_ENABLE) == 0)
+		return;
 
-	if (tmp & PF_ENABLE) {
-		pipe_config->pch_pfit.enabled = true;
-		pipe_config->pch_pfit.pos = intel_de_read(dev_priv,
-							  PF_WIN_POS(crtc->pipe));
-		pipe_config->pch_pfit.size = intel_de_read(dev_priv,
-							   PF_WIN_SZ(crtc->pipe));
-
-		/* We currently do not free assignements of panel fitters on
-		 * ivb/hsw (since we don't use the higher upscaling modes which
-		 * differentiates them) so just WARN about this case for now. */
-		if (IS_GEN(dev_priv, 7)) {
-			WARN_ON((tmp & PF_PIPE_SEL_MASK_IVB) !=
-				PF_PIPE_SEL_IVB(crtc->pipe));
-		}
-	}
+	crtc_state->pch_pfit.enabled = true;
+	crtc_state->pch_pfit.pos =
+		intel_de_read(dev_priv, PF_WIN_POS(crtc->pipe));
+	crtc_state->pch_pfit.size =
+		intel_de_read(dev_priv, PF_WIN_SZ(crtc->pipe));
+
+	/*
+	 * We currently do not free assignements of panel fitters on
+	 * ivb/hsw (since we don't use the higher upscaling modes which
+	 * differentiates them) so just WARN about this case for now.
+	 */
+	WARN_ON(IS_GEN(dev_priv, 7) &&
+		(tmp & PF_PIPE_SEL_MASK_IVB) != PF_PIPE_SEL_IVB(crtc->pipe));
 }
 
 static bool ilk_get_pipe_config(struct intel_crtc *crtc,
@@ -10694,7 +10691,7 @@  static bool ilk_get_pipe_config(struct intel_crtc *crtc,
 	intel_get_pipe_timings(crtc, pipe_config);
 	intel_get_pipe_src_size(crtc, pipe_config);
 
-	ilk_get_pfit_config(crtc, pipe_config);
+	ilk_get_pfit_config(pipe_config);
 
 	ret = true;
 
@@ -11219,9 +11216,9 @@  static bool hsw_get_pipe_config(struct intel_crtc *crtc,
 		power_domain_mask |= BIT_ULL(power_domain);
 
 		if (INTEL_GEN(dev_priv) >= 9)
-			skl_get_pfit_config(crtc, pipe_config);
+			skl_get_pfit_config(pipe_config);
 		else
-			ilk_get_pfit_config(crtc, pipe_config);
+			ilk_get_pfit_config(pipe_config);
 	}
 
 	if (hsw_crtc_supports_ips(crtc)) {