[PATCH] drm/i915: panel fitter hw state readout&check support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Daniel Vetter <daniel at ffwll.ch> writes:

> On Fri, May 10, 2013 at 02:37:59PM +0300, Mika Kuoppala wrote:
>> 
>> Hi Daniel,
>> 
>> Daniel Vetter <daniel.vetter at ffwll.ch> writes:
>> 
>> > Pfit state readout is a bit ugly on gen2/3 due to the intermingling
>> > with the lvds state, but alas.
>> >
>> > Also note that since state is always cleared to zero we can
>> > unconditonally compare all the state and completely neglect the actual
>> > platform we're running on.
>> >
>> > v2: Properly check for the pfit power domain on haswell.
>> >
>> > v3: Don't check pgm_ratios on gen4+, they're auto-computed by the hw.
>> >
>> > v4: Properly clear the lvds border bits, upset the state checker a
>> > bit.
>> >
>> > v5: Unconditionally read out panel dither settings on gen2/3.
>> >
>> > Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>> > ---
>> >  drivers/gpu/drm/i915/intel_display.c | 67 ++++++++++++++++++++++++++++++++++--
>> >  drivers/gpu/drm/i915/intel_lvds.c    |  1 +
>> >  2 files changed, 66 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > index bfbb484..58395e3 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -4967,6 +4967,36 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>> >  	return ret;
>> >  }
>> >  
>> > +static void i9xx_get_pfit_config(struct intel_crtc *crtc,
>> > +				 struct intel_crtc_config *pipe_config)
>> > +{
>> > +	struct drm_device *dev = crtc->base.dev;
>> > +	struct drm_i915_private *dev_priv = dev->dev_private;
>> > +	uint32_t tmp;
>> > +
>> > +	tmp = I915_READ(PFIT_CONTROL);
>> 
>> You could do
>> const uint32_t ctl = I915_READ(PFIT_CONTROL);
>> and...
>> 
>> > +	if (INTEL_INFO(dev)->gen < 4) {
>> > +		if (crtc->pipe != PIPE_B)
>> > +			return;
>> > +
>> > +		/* gen2/3 store dither state in pfit control, needs to match */
>> > +		pipe_config->gmch_pfit.control = tmp & PANEL_8TO6_DITHER_ENABLE;
>> > +	} else {
>> > +		if ((tmp & PFIT_PIPE_MASK) != (crtc->pipe << PFIT_PIPE_SHIFT))
>> > +			return;
>> > +	}
>> > +
>> > +	if (!(tmp & PFIT_ENABLE))
>> > +		return;
>> > +
>> > +	pipe_config->gmch_pfit.control = I915_READ(PFIT_CONTROL);
>> 
>> and use ctl in here to avoid extra read.
>
> Originally I've had that, but then thought it'd look more symmetric if I
> read out all the registers anew once we're sure that we're looking at the
> right panel fitter. I can do the cse again if you want.

Matter of taste bikeshedding it was. No need to change.

>> > +	pipe_config->gmch_pfit.pgm_ratios = I915_READ(PFIT_PGM_RATIOS);
>> > +	if (INTEL_INFO(dev)->gen < 5)
>> > +		pipe_config->gmch_pfit.lvds_border_bits =
>> > +			I915_READ(LVDS) & LVDS_BORDER_ENABLE;
>> > +}
>> > +
>> >  static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>> >  				 struct intel_crtc_config *pipe_config)
>> >  {
>> > @@ -4980,6 +5010,8 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>> >  
>> >  	intel_get_pipe_timings(crtc, pipe_config);
>> >  
>> > +	i9xx_get_pfit_config(crtc, pipe_config);
>> > +
>> >  	return true;
>> >  }
>> >  
>> > @@ -5815,6 +5847,21 @@ static void ironlake_get_fdi_m_n_config(struct intel_crtc *crtc,
>> >  				   & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1;
>> >  }
>> >  
>> > +static void ironlake_get_pfit_config(struct intel_crtc *crtc,
>> > +				     struct intel_crtc_config *pipe_config)
>> > +{
>> > +	struct drm_device *dev = crtc->base.dev;
>> > +	struct drm_i915_private *dev_priv = dev->dev_private;
>> > +	uint32_t tmp;
>> > +
>> > +	tmp = I915_READ(PF_CTL(crtc->pipe));
>> > +
>> > +	if (tmp & PF_ENABLE) {
>> > +		pipe_config->pch_pfit.pos = I915_READ(PF_WIN_POS(crtc->pipe));
>> > +		pipe_config->pch_pfit.size = I915_READ(PF_WIN_SZ(crtc->pipe));
>> > +	}
>> > +}
>> > +
>> >  static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>> >  				     struct intel_crtc_config *pipe_config)
>> >  {
>> > @@ -5838,6 +5885,8 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>> >  
>> >  	intel_get_pipe_timings(crtc, pipe_config);
>> >  
>> > +	ironlake_get_pfit_config(crtc, pipe_config);
>> > +
>> >  	return true;
>> >  }
>> >  
>> > @@ -5957,6 +6006,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>> >  	struct drm_device *dev = crtc->base.dev;
>> >  	struct drm_i915_private *dev_priv = dev->dev_private;
>> >  	enum transcoder cpu_transcoder = crtc->config.cpu_transcoder;
>> > +	enum intel_display_power_domain pfit_domain;
>> >  	uint32_t tmp;
>> >  
>> >  	if (!intel_display_power_enabled(dev,
>> > @@ -5986,6 +6036,10 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>> >  
>> >  	intel_get_pipe_timings(crtc, pipe_config);
>> >  
>> > +	pfit_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
>> > +	if (intel_display_power_enabled(dev, pfit_domain))
>> > +		ironlake_get_pfit_config(crtc, pipe_config);
>> > +
>> >  	return true;
>> >  }
>> >  
>> > @@ -7961,7 +8015,8 @@ intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
>> >  		if (mask & (1 <<(intel_crtc)->pipe))
>> >  
>> >  static bool
>> > -intel_pipe_config_compare(struct intel_crtc_config *current_config,
>> > +intel_pipe_config_compare(struct drm_device *dev,
>> > +			  struct intel_crtc_config *current_config,
>> >  			  struct intel_crtc_config *pipe_config)
>> >  {
>> >  #define PIPE_CONF_CHECK_I(name)	\
>> > @@ -8010,6 +8065,14 @@ intel_pipe_config_compare(struct intel_crtc_config *current_config,
>> >  	PIPE_CONF_CHECK_I(requested_mode.hdisplay);
>> >  	PIPE_CONF_CHECK_I(requested_mode.vdisplay);
>> >  
>> > +	PIPE_CONF_CHECK_I(gmch_pfit.control);
>> > +	/* pfit ratios are autocomputed by the hw on gen4+ */
>> > +	if (INTEL_INFO(dev)->gen < 4)
>> > +		PIPE_CONF_CHECK_I(gmch_pfit.pgm_ratios);
>> > +	PIPE_CONF_CHECK_I(gmch_pfit.lvds_border_bits);
>> > +	PIPE_CONF_CHECK_I(pch_pfit.pos);
>> > +	PIPE_CONF_CHECK_I(pch_pfit.size);
>> > +
>> 
>> For ironlake+ we don't get conf checking for the control register part.
>> Should we?
>
> ilk pfit control state isn't precomputed (we just use size != 0 as a
> signal to set it to a specific value). It's also always the same value, so
> I don't think we need to jump through loops here.
>
> That will change of course once we bother to enable the higher upscaling
> modes (if we ever do that), since those have global sharing constraints.
> But for now I don't see a need.

As discussed in irc, we might come across a case where panel fitter
is not aligned to the same pipe in ilk+. Perhaps warn if we
encounter such a weird setup?

With or without warning added,

Reviewed-by: Mika Kuoppala <mika.kuoppala at intel.com>

> -Daniel
>
>> 
>> >  #undef PIPE_CONF_CHECK_I
>> >  #undef PIPE_CONF_CHECK_FLAGS
>> >  
>> > @@ -8121,7 +8184,7 @@ intel_modeset_check_state(struct drm_device *dev)
>> >  		     "(expected %i, found %i)\n", crtc->active, active);
>> >  
>> >  		WARN(active &&
>> > -		     !intel_pipe_config_compare(&crtc->config, &pipe_config),
>> > +		     !intel_pipe_config_compare(dev, &crtc->config, &pipe_config),
>> >  		     "pipe state doesn't match!\n");
>> >  	}
>> >  }
>> > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
>> > index d256fe4..3294efd 100644
>> > --- a/drivers/gpu/drm/i915/intel_lvds.c
>> > +++ b/drivers/gpu/drm/i915/intel_lvds.c
>> > @@ -116,6 +116,7 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder)
>> >  	}
>> >  
>> >  	/* set the corresponsding LVDS_BORDER bit */
>> > +	temp &= ~LVDS_BORDER_ENABLE;
>> >  	temp |= intel_crtc->config.gmch_pfit.lvds_border_bits;
>> >  	/* Set the B0-B3 data pairs corresponding to whether we're going to
>> >  	 * set the DPLLs for dual-channel mode or not.
>> > -- 
>> > 1.7.11.7
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx at lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch


[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux