Re: [PATCH 75/89] drm/i915/skl: fetch, enable/disable pfit as needed

[Damien Lespiau <damien.lespiau@intel.com>]

Hi Jesse,

Mind looking at those review comments?

-- 
Damien

On Tue, Sep 23, 2014 at 05:50:29PM -0300, Paulo Zanoni wrote:
> 2014-09-04 8:27 GMT-03:00 Damien Lespiau <damien.lespiau@intel.com>:
> > From: Jesse Barnes <jbarnes@virtuousgeek.org>
> >
> > This moved around on SKL, so we need to make sure we read/write the
> > correct regs.
> >
> > Signed-off-by: Jesse Barnes <jbarnes@virtuougseek.org>
> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h      | 13 ++++++++
> >  drivers/gpu/drm/i915/intel_display.c | 61 +++++++++++++++++++++++++++++++++---
> >  2 files changed, 70 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 176e84e..35c0759 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4833,6 +4833,19 @@ enum skl_disp_power_wells {
> >  #define PF_VSCALE(pipe)                _PIPE(pipe, _PFA_VSCALE, _PFB_VSCALE)
> >  #define PF_HSCALE(pipe)                _PIPE(pipe, _PFA_HSCALE, _PFB_HSCALE)
> >
> > +#define _PSA_CTL               0x68180
> > +#define _PSB_CTL               0x68980
> > +#define PS_ENABLE              (1<<31)
> > +#define PS_PIPE_SEL(pipe)      ((pipe)<<27)
> 
> The PS_PIPE_SEL define is wrong, but it is also not used in the code
> you wrote, so just remove this line.
> 
> 
> > +#define _PSA_WIN_SZ            0x68174
> > +#define _PSB_WIN_SZ            0x68974
> > +#define _PSA_WIN_POS           0x68178
> 
> 0x68170
> 
> > +#define _PSB_WIN_POS           0x68978
> 
> 0x68970
> 
> > +
> > +#define PS_CTL(pipe)           _PIPE(pipe, _PSA_CTL, _PSB_CTL)
> > +#define PS_WIN_SZ(pipe)                _PIPE(pipe, _PSA_WIN_SZ, _PSB_WIN_SZ)
> > +#define PS_WIN_POS(pipe)       _PIPE(pipe, _PSA_WIN_POS, _PSB_WIN_POS)
> > +
> >  /* legacy palette */
> >  #define _LGC_PALETTE_A           0x4a000
> >  #define _LGC_PALETTE_B           0x4a800
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 393bd19..9b31342 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3882,6 +3882,19 @@ static void cpt_verify_modeset(struct drm_device *dev, int pipe)
> >         }
> >  }
> >
> > +static void skylake_pfit_enable(struct intel_crtc *crtc)
> > +{
> > +       struct drm_device *dev = crtc->base.dev;
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +       int pipe = crtc->pipe;
> > +
> > +       if (crtc->config.pch_pfit.enabled) {
> > +               I915_WRITE(PS_CTL(pipe), PS_ENABLE);
> > +               I915_WRITE(PS_WIN_POS(pipe), crtc->config.pch_pfit.pos);
> > +               I915_WRITE(PS_WIN_SZ(pipe), crtc->config.pch_pfit.size);
> > +       }
> > +}
> > +
> >  static void ironlake_pfit_enable(struct intel_crtc *crtc)
> >  {
> >         struct drm_device *dev = crtc->base.dev;
> > @@ -4264,7 +4277,10 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
> >
> >         intel_ddi_enable_pipe_clock(intel_crtc);
> >
> > -       ironlake_pfit_enable(intel_crtc);
> > +       if (IS_SKYLAKE(dev))
> > +               skylake_pfit_enable(intel_crtc);
> > +       else
> > +               ironlake_pfit_enable(intel_crtc);
> >
> >         /*
> >          * On ILK+ LUT must be loaded before the pipe is running but with
> > @@ -4295,6 +4311,20 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
> >         intel_crtc_enable_planes(crtc);
> >  }
> >
> > +static void skylake_pfit_disable(struct intel_crtc *crtc)
> > +{
> > +       struct drm_device *dev = crtc->base.dev;
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +       int pipe = crtc->pipe;
> > +
> > +       /* 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 (crtc->config.pch_pfit.enabled) {
> > +               I915_WRITE(PS_CTL(pipe), 0);
> > +               I915_WRITE(PS_WIN_SZ(pipe), 0);
> 
> Why not also zero _POS just like HSW? Can this affect the HW state
> checker output?
> 
> Everything else looks correct.
> 
> > +       }
> > +}
> > +
> >  static void ironlake_pfit_disable(struct intel_crtc *crtc)
> >  {
> >         struct drm_device *dev = crtc->base.dev;
> > @@ -4399,7 +4429,10 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
> >
> >         intel_ddi_disable_transcoder_func(dev_priv, cpu_transcoder);
> >
> > -       ironlake_pfit_disable(intel_crtc);
> > +       if (IS_SKYLAKE(dev))
> > +               skylake_pfit_disable(intel_crtc);
> > +       else
> > +               ironlake_pfit_disable(intel_crtc);
> >
> >         intel_ddi_disable_pipe_clock(intel_crtc);
> >
> > @@ -7382,6 +7415,22 @@ static void ironlake_get_fdi_m_n_config(struct intel_crtc *crtc,
> >                                      &pipe_config->fdi_m_n, NULL);
> >  }
> >
> > +static void skylake_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(PS_CTL(crtc->pipe));
> > +
> > +       if (tmp & PS_ENABLE) {
> > +               pipe_config->pch_pfit.enabled = true;
> > +               pipe_config->pch_pfit.pos = I915_READ(PS_WIN_POS(crtc->pipe));
> > +               pipe_config->pch_pfit.size = I915_READ(PS_WIN_SZ(crtc->pipe));
> > +       }
> > +}
> > +
> >  static void ironlake_get_pfit_config(struct intel_crtc *crtc,
> >                                      struct intel_crtc_config *pipe_config)
> >  {
> > @@ -7940,8 +7989,12 @@ 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_priv, pfit_domain))
> > -               ironlake_get_pfit_config(crtc, pipe_config);
> > +       if (intel_display_power_enabled(dev_priv, pfit_domain)) {
> > +               if (IS_SKYLAKE(dev))
> > +                       skylake_get_pfit_config(crtc, pipe_config);
> > +               else
> > +                       ironlake_get_pfit_config(crtc, pipe_config);
> > +       }
> >
> >         if (IS_HASWELL(dev))
> >                 pipe_config->ips_enabled = hsw_crtc_supports_ips(crtc) &&
> > --
> > 1.8.3.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni