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
