diff mbox

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

Message ID 1409830075-11139-76-git-send-email-damien.lespiau@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Lespiau, Damien Sept. 4, 2014, 11:27 a.m. UTC 
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(-)

Comments

Paulo Zanoni Sept. 23, 2014, 8:50 p.m. UTC | #1 
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
Lespiau, Damien Sept. 24, 2014, 10:44 a.m. UTC | #2 
Hi Jesse,

Mind looking at those review comments?
Jesse Barnes Sept. 25, 2014, 2:48 p.m. UTC | #3 
On Tue, 23 Sep 2014 17:50:29 -0300
Paulo Zanoni <przanoni@gmail.com> 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.

Yeah that sounds fine.

> 
> 
> > +#define _PSA_WIN_SZ            0x68174
> > +#define _PSB_WIN_SZ            0x68974
> > +#define _PSA_WIN_POS           0x68178
> 
> 0x68170
> 
> > +#define _PSB_WIN_POS           0x68978
> 
> 0x68970

Ooh thanks.

> 
> > +
> > +#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.

The checker will only if the fitter is enabled, but yeah it looks like
it would be safer to zero it.

Damien, did you want to make these changes as part of your re-post or
should I send an updated patch to replace this one?

Thanks,
Lespiau, Damien Sept. 25, 2014, 2:55 p.m. UTC | #4 
On Thu, Sep 25, 2014 at 07:48:34AM -0700, Jesse Barnes wrote:
> Damien, did you want to make these changes as part of your re-post or
> should I send an updated patch to replace this one?

I wasn't planning to go through this one but let the author works for
his commit :)
diff mbox

Patch

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)
+#define _PSA_WIN_SZ		0x68174
+#define _PSB_WIN_SZ		0x68974
+#define _PSA_WIN_POS		0x68178
+#define _PSB_WIN_POS		0x68978
+
+#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);
+	}
+}
+
 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) &&