[PATCH 0/5] drm/i915: FDI port_clock fix for DDI and other FDI changes

[PATCH 0/5] drm/i915: FDI port_clock fix for DDI and other FDI changes

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

I just set out to fix [1], but I ended up massaging the FDI and DDI
code a bit more than I initially intended.

The SPLL sharing part is untested due to the PLL sharing and selection
being hardcoded to the output type. So I would've had to massage the code
even more to get it to use SPLL for non FDI outputs. It would be nice to
make the code eventually more like the PCH PLL sharing code so it isn't
so strictly tied in with the output type. I was thinking each output
type should just have some kind of priority list of possible PLLs, and
the PLL sharing code itself should be entirely oblivious about the
output type. But that's something for the future...

[1] https://bugs.freedesktop.org/show_bug.cgi?id=74955

Ville Syrjälä (5):
  drm/i915: Fix DDI port_clock for VGA output
  drm/i915: Change intel_fdi_link_freq() to 10kHz
  drm/i915: Use DIV_ROUND_UP() when calculating number of required FDI
    lanes
  drm/i915: Use port_clock for FDI frequency on DDI
  drm/i915: Add SPLL sharing support for DDI

 drivers/gpu/drm/i915/intel_crt.c     |  4 ++++
 drivers/gpu/drm/i915/intel_ddi.c     | 45 +++++++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++------------
 3 files changed, 57 insertions(+), 21 deletions(-)

-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/5] drm/i915: Fix DDI port_clock for VGA output

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On DDI there's no PLL as such to generate the pixel clock for VGA.
Instead we derive the pixel clock from the FDI link frequency. So
to make .compute_config match what .get_config does, we need to
set the port_clock based on the FDI link frequency.

Note that we don't even check the port_clock when selecting the
PLL for VGA output. We just assume SPLL at 1.35GHz is what we want,
and that does match with the asumption of FDI frequency of 2.7Ghz
we have in intel_fdi_link_freq().

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=74955
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_crt.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 9864aa1..02e2b02 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -262,6 +262,10 @@ static bool intel_crt_compute_config(struct intel_encoder *encoder,
 	if (HAS_PCH_LPT(dev))
 		pipe_config->pipe_bpp = 24;
 
+	/* FDI must always be 2.7 GHz */
+	if (HAS_DDI(dev))
+		pipe_config->port_clock = 135000 * 2;
+
 	return true;
 }
 
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/5] drm/i915: Change intel_fdi_link_freq() to 10kHz

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We normally use 10Khz units when describing DP link frequency.
Have intel_fdi_link_freq() return the same units. I always get confused
when the units start to be totally different.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f19e6ea..a366e91 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -81,14 +81,14 @@ intel_pch_rawclk(struct drm_device *dev)
 	return I915_READ(PCH_RAWCLK_FREQ) & RAWCLK_FREQ_MASK;
 }
 
-static inline u32 /* units of 100MHz */
+static inline int /* units of 10kHz */
 intel_fdi_link_freq(struct drm_device *dev)
 {
 	if (IS_GEN5(dev)) {
 		struct drm_i915_private *dev_priv = dev->dev_private;
-		return (I915_READ(FDI_PLL_BIOS_0) & FDI_PLL_FB_CLOCK_MASK) + 2;
+		return ((I915_READ(FDI_PLL_BIOS_0) & FDI_PLL_FB_CLOCK_MASK) + 2) * 10000;
 	} else
-		return 27;
+		return 270000;
 }
 
 static const intel_limit_t intel_limits_i8xx_dac = {
@@ -4521,14 +4521,7 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
 	bool setup_ok, needs_recompute = false;
 
 retry:
-	/* FDI is a binary signal running at ~2.7GHz, encoding
-	 * each output octet as 10 bits. The actual frequency
-	 * is stored as a divider into a 100MHz clock, and the
-	 * mode pixel clock is stored in units of 1KHz.
-	 * Hence the bw of each lane in terms of the mode signal
-	 * is:
-	 */
-	link_bw = intel_fdi_link_freq(dev) * MHz(100)/KHz(1)/10;
+	link_bw = intel_fdi_link_freq(dev);
 
 	fdi_dotclock = adjusted_mode->crtc_clock;
 
@@ -8069,7 +8062,7 @@ static void ironlake_pch_clock_get(struct intel_crtc *crtc,
 	 * get_config() function.
 	 */
 	pipe_config->adjusted_mode.crtc_clock =
-		intel_dotclock_calculate(intel_fdi_link_freq(dev) * 10000,
+		intel_dotclock_calculate(intel_fdi_link_freq(dev),
 					 &pipe_config->fdi_m_n);
 }
 
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/5] drm/i915: Use DIV_ROUND_UP() when calculating number of required FDI lanes

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

If we need precisely N lanes to satisfy the FDI bandwidth requirement,
the code would still claim that we need N+1 lanes. Use DIV_ROUND_UP()
to get a more accurate answer.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a366e91..4702858 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6149,7 +6149,7 @@ int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp)
 	 * is 2.5%; use 5% for safety's sake.
 	 */
 	u32 bps = target_clock * bpp * 21 / 20;
-	return bps / (link_bw * 8) + 1;
+	return DIV_ROUND_UP(bps, link_bw * 8);
 }
 
 static bool ironlake_needs_fb_cb_tune(struct dpll *dpll, int factor)
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/5] drm/i915: Use port_clock for FDI frequency on DDI

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Use the port_clock that compute_config filled out in
intel_fdi_link_freq() on DDI platforms. This means we're going to
compute the FDI M/N based on the actual clock we're going to use,
and we'll also check the link bandwidth against that clock.
This eliminates the hidden assumptions about the actaul FDI
frequency.

However as FDI is specified to operate at 2.7GHz always, we should
complain if we've misconfigured things. So add a WARN if
port_clock isn't what it should be.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4702858..fb04fe9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -82,9 +82,15 @@ intel_pch_rawclk(struct drm_device *dev)
 }
 
 static inline int /* units of 10kHz */
-intel_fdi_link_freq(struct drm_device *dev)
+intel_fdi_link_freq(struct drm_device *dev,
+		    const struct intel_crtc_config *pipe_config)
 {
-	if (IS_GEN5(dev)) {
+	if (HAS_DDI(dev)) {
+		WARN(pipe_config->port_clock != 270000,
+		     "Invalid FDI link frequency %d\n",
+		     pipe_config->port_clock);
+		return pipe_config->port_clock;
+	} else if (IS_GEN5(dev)) {
 		struct drm_i915_private *dev_priv = dev->dev_private;
 		return ((I915_READ(FDI_PLL_BIOS_0) & FDI_PLL_FB_CLOCK_MASK) + 2) * 10000;
 	} else
@@ -4521,7 +4527,7 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
 	bool setup_ok, needs_recompute = false;
 
 retry:
-	link_bw = intel_fdi_link_freq(dev);
+	link_bw = intel_fdi_link_freq(dev, pipe_config);
 
 	fdi_dotclock = adjusted_mode->crtc_clock;
 
@@ -8062,7 +8068,7 @@ static void ironlake_pch_clock_get(struct intel_crtc *crtc,
 	 * get_config() function.
 	 */
 	pipe_config->adjusted_mode.crtc_clock =
-		intel_dotclock_calculate(intel_fdi_link_freq(dev),
+		intel_dotclock_calculate(intel_fdi_link_freq(dev, pipe_config),
 					 &pipe_config->fdi_m_n);
 }
 
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/5] drm/i915: Add SPLL sharing support for DDI

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Write some theoretical SPLL sharing support for DDI. Currently that will
never happens since we never use SPLL for anything but FDI. But having
the code there makes it easier if we ever want to do it, and it might
excercise the PLL sharing code a bit more.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 45 ++++++++++++++++++++++++++++++++++------
 1 file changed, 39 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 2643d3b..1dbe3e4 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -870,16 +870,35 @@ bool intel_ddi_pll_select(struct intel_crtc *intel_crtc)
 		}
 
 	} else if (type == INTEL_OUTPUT_ANALOG) {
-		if (plls->spll_refcount == 0) {
+		uint32_t val = SPLL_PLL_ENABLE | SPLL_PLL_SSC;
+
+		switch (clock / 2) {
+		case 81000:
+			val |= SPLL_PLL_FREQ_810MHz;
+			break;
+		case 135000:
+			val |= SPLL_PLL_FREQ_1350MHz;
+			break;
+		case 270000:
+			val |= SPLL_PLL_FREQ_2700MHz;
+			break;
+		default:
+			DRM_ERROR("SPLL frequency %d kHz unsupported\n", clock / 2);
+			return false;
+		}
+
+		if (val == I915_READ(SPLL_CTL)) {
+			DRM_DEBUG_KMS("Reusing SPLL on pipe %c\n",
+				      pipe_name(pipe));
+		} else if (plls->spll_refcount == 0) {
 			DRM_DEBUG_KMS("Using SPLL on pipe %c\n",
 				      pipe_name(pipe));
-			plls->spll_refcount++;
-			intel_crtc->ddi_pll_sel = PORT_CLK_SEL_SPLL;
 		} else {
 			DRM_ERROR("SPLL already in use\n");
 			return false;
 		}
-
+		plls->spll_refcount++;
+		intel_crtc->ddi_pll_sel = PORT_CLK_SEL_SPLL;
 	} else {
 		WARN(1, "Invalid DDI encoder type %d\n", type);
 		return false;
@@ -921,8 +940,22 @@ void intel_ddi_pll_enable(struct intel_crtc *crtc)
 		pll_name = "SPLL";
 		reg = SPLL_CTL;
 		refcount = plls->spll_refcount;
-		new_val = SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz |
-			  SPLL_PLL_SSC;
+		new_val = SPLL_PLL_ENABLE | SPLL_PLL_SSC;
+
+		switch (clock / 2) {
+		case 81000:
+			new_val |= SPLL_PLL_FREQ_810MHz;
+			break;
+		case 135000:
+			new_val |= SPLL_PLL_FREQ_1350MHz;
+			break;
+		case 270000:
+			new_val |= SPLL_PLL_FREQ_2700MHz;
+			break;
+		default:
+			WARN(1, "Bad SPLL frequency %d\n", clock / 2);
+			return;
+		}
 		break;
 
 	case PORT_CLK_SEL_WRPLL1:
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Add SPLL sharing support for DDI

[Paulo Zanoni]

Hi

2014-02-27 9:23 GMT-03:00  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Write some theoretical SPLL sharing support for DDI. Currently that will
> never happens since we never use SPLL for anything but FDI. But having
> the code there makes it easier if we ever want to do it, and it might
> excercise the PLL sharing code a bit more.

One thing is that if we ever want to use SSC on eDP/DP/HDMI, we will
want to use either SPLL or WRPLL instead of LCPLL. I always wanted to
implement this, and recently I saw a bug that made me think about it
again.

So instead of this patch, why don't you write support to use SSC when
requested? We could do this through some i915.use_ssc command line
option that would just try to force SSC on everything, or maybe a
"debug connector property", or debugfs, or anything you might want.
With this, you would be able to actually test your patches, and we'd
have a useful feature.

If you disagree, please just say "no" and then I'll go back to review
this patch :)

Thanks,
Paulo

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 45 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 39 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 2643d3b..1dbe3e4 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -870,16 +870,35 @@ bool intel_ddi_pll_select(struct intel_crtc *intel_crtc)
>                 }
>
>         } else if (type == INTEL_OUTPUT_ANALOG) {
> -               if (plls->spll_refcount == 0) {
> +               uint32_t val = SPLL_PLL_ENABLE | SPLL_PLL_SSC;
> +
> +               switch (clock / 2) {
> +               case 81000:
> +                       val |= SPLL_PLL_FREQ_810MHz;
> +                       break;
> +               case 135000:
> +                       val |= SPLL_PLL_FREQ_1350MHz;
> +                       break;
> +               case 270000:
> +                       val |= SPLL_PLL_FREQ_2700MHz;
> +                       break;
> +               default:
> +                       DRM_ERROR("SPLL frequency %d kHz unsupported\n", clock / 2);
> +                       return false;
> +               }
> +
> +               if (val == I915_READ(SPLL_CTL)) {
> +                       DRM_DEBUG_KMS("Reusing SPLL on pipe %c\n",
> +                                     pipe_name(pipe));
> +               } else if (plls->spll_refcount == 0) {
>                         DRM_DEBUG_KMS("Using SPLL on pipe %c\n",
>                                       pipe_name(pipe));
> -                       plls->spll_refcount++;
> -                       intel_crtc->ddi_pll_sel = PORT_CLK_SEL_SPLL;
>                 } else {
>                         DRM_ERROR("SPLL already in use\n");
>                         return false;
>                 }
> -
> +               plls->spll_refcount++;
> +               intel_crtc->ddi_pll_sel = PORT_CLK_SEL_SPLL;
>         } else {
>                 WARN(1, "Invalid DDI encoder type %d\n", type);
>                 return false;
> @@ -921,8 +940,22 @@ void intel_ddi_pll_enable(struct intel_crtc *crtc)
>                 pll_name = "SPLL";
>                 reg = SPLL_CTL;
>                 refcount = plls->spll_refcount;
> -               new_val = SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz |
> -                         SPLL_PLL_SSC;
> +               new_val = SPLL_PLL_ENABLE | SPLL_PLL_SSC;
> +
> +               switch (clock / 2) {
> +               case 81000:
> +                       new_val |= SPLL_PLL_FREQ_810MHz;
> +                       break;
> +               case 135000:
> +                       new_val |= SPLL_PLL_FREQ_1350MHz;
> +                       break;
> +               case 270000:
> +                       new_val |= SPLL_PLL_FREQ_2700MHz;
> +                       break;
> +               default:
> +                       WARN(1, "Bad SPLL frequency %d\n", clock / 2);
> +                       return;
> +               }
>                 break;
>
>         case PORT_CLK_SEL_WRPLL1:
> --
> 1.8.3.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 1/5] drm/i915: Fix DDI port_clock for VGA output

[Paulo Zanoni]

2014-02-27 9:23 GMT-03:00  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> On DDI there's no PLL as such to generate the pixel clock for VGA.
> Instead we derive the pixel clock from the FDI link frequency. So
> to make .compute_config match what .get_config does, we need to
> set the port_clock based on the FDI link frequency.
>
> Note that we don't even check the port_clock when selecting the
> PLL for VGA output. We just assume SPLL at 1.35GHz is what we want,
> and that does match with the asumption of FDI frequency of 2.7Ghz
> we have in intel_fdi_link_freq().
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=74955
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_crt.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index 9864aa1..02e2b02 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -262,6 +262,10 @@ static bool intel_crt_compute_config(struct intel_encoder *encoder,
>         if (HAS_PCH_LPT(dev))
>                 pipe_config->pipe_bpp = 24;
>
> +       /* FDI must always be 2.7 GHz */
> +       if (HAS_DDI(dev))
> +               pipe_config->port_clock = 135000 * 2;
> +

<rant>
Whenever I have to review patches touching the HW state
readout/compute I get completely confused. We have a bunch of
compute_config/get_config/get_hw_state functions, with no real
documentation of which fields exactly each function should get. Our
hardware also does a bunch of *2 multiplications on PLLs and clocks,
and this gets things even more confusing. Also, we have similar
information (like all these "clocks", from mode, adjusted_mode, port,
pipe, hw, etc) with slightly different meanings. I really wish we had
Kernel documentation describing *exactly* what is expected to be
computed from each function, and what are the differences between all
those similar things with slightly different meanings. Maybe we could
also organize struct intel_crtc_config, adding small sub-structs that
better define the domain of each field?
</rant>

Still, I have reviewed your patch, and despite all my own confusions,
it looks correct, so:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>         return true;
>  }
>
> --
> 1.8.3.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 2/5] drm/i915: Change intel_fdi_link_freq() to 10kHz

[Paulo Zanoni]

2014-02-27 9:23 GMT-03:00  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> We normally use 10Khz units when describing DP link frequency.
> Have intel_fdi_link_freq() return the same units. I always get confused
> when the units start to be totally different.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

A nice simplification!

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f19e6ea..a366e91 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -81,14 +81,14 @@ intel_pch_rawclk(struct drm_device *dev)
>         return I915_READ(PCH_RAWCLK_FREQ) & RAWCLK_FREQ_MASK;
>  }
>
> -static inline u32 /* units of 100MHz */
> +static inline int /* units of 10kHz */
>  intel_fdi_link_freq(struct drm_device *dev)
>  {
>         if (IS_GEN5(dev)) {
>                 struct drm_i915_private *dev_priv = dev->dev_private;
> -               return (I915_READ(FDI_PLL_BIOS_0) & FDI_PLL_FB_CLOCK_MASK) + 2;
> +               return ((I915_READ(FDI_PLL_BIOS_0) & FDI_PLL_FB_CLOCK_MASK) + 2) * 10000;
>         } else
> -               return 27;
> +               return 270000;
>  }
>
>  static const intel_limit_t intel_limits_i8xx_dac = {
> @@ -4521,14 +4521,7 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
>         bool setup_ok, needs_recompute = false;
>
>  retry:
> -       /* FDI is a binary signal running at ~2.7GHz, encoding
> -        * each output octet as 10 bits. The actual frequency
> -        * is stored as a divider into a 100MHz clock, and the
> -        * mode pixel clock is stored in units of 1KHz.
> -        * Hence the bw of each lane in terms of the mode signal
> -        * is:
> -        */
> -       link_bw = intel_fdi_link_freq(dev) * MHz(100)/KHz(1)/10;
> +       link_bw = intel_fdi_link_freq(dev);
>
>         fdi_dotclock = adjusted_mode->crtc_clock;
>
> @@ -8069,7 +8062,7 @@ static void ironlake_pch_clock_get(struct intel_crtc *crtc,
>          * get_config() function.
>          */
>         pipe_config->adjusted_mode.crtc_clock =
> -               intel_dotclock_calculate(intel_fdi_link_freq(dev) * 10000,
> +               intel_dotclock_calculate(intel_fdi_link_freq(dev),
>                                          &pipe_config->fdi_m_n);
>  }
>
> --
> 1.8.3.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 3/5] drm/i915: Use DIV_ROUND_UP() when calculating number of required FDI lanes

[Paulo Zanoni]

2014-02-27 9:23 GMT-03:00  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> If we need precisely N lanes to satisfy the FDI bandwidth requirement,
> the code would still claim that we need N+1 lanes. Use DIV_ROUND_UP()
> to get a more accurate answer.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a366e91..4702858 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6149,7 +6149,7 @@ int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp)
>          * is 2.5%; use 5% for safety's sake.
>          */
>         u32 bps = target_clock * bpp * 21 / 20;
> -       return bps / (link_bw * 8) + 1;
> +       return DIV_ROUND_UP(bps, link_bw * 8);
>  }
>
>  static bool ironlake_needs_fb_cb_tune(struct dpll *dpll, int factor)
> --
> 1.8.3.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 2/5] drm/i915: Change intel_fdi_link_freq() to 10kHz

[Chris Wilson]

On Fri, Feb 28, 2014 at 07:28:41PM -0300, Paulo Zanoni wrote:
> 2014-02-27 9:23 GMT-03:00  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > We normally use 10Khz units when describing DP link frequency.
> > Have intel_fdi_link_freq() return the same units. I always get confused
> > when the units start to be totally different.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> A nice simplification!

An oversimplification. link_bw != link_freq.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 5/5] drm/i915: Add SPLL sharing support for DDI

[Ville Syrjälä]

On Fri, Feb 28, 2014 at 06:40:22PM -0300, Paulo Zanoni wrote:
> Hi
> 
> 2014-02-27 9:23 GMT-03:00  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Write some theoretical SPLL sharing support for DDI. Currently that will
> > never happens since we never use SPLL for anything but FDI. But having
> > the code there makes it easier if we ever want to do it, and it might
> > excercise the PLL sharing code a bit more.
> 
> One thing is that if we ever want to use SSC on eDP/DP/HDMI, we will
> want to use either SPLL or WRPLL instead of LCPLL. I always wanted to
> implement this, and recently I saw a bug that made me think about it
> again.
> 
> So instead of this patch, why don't you write support to use SSC when
> requested? We could do this through some i915.use_ssc command line
> option that would just try to force SSC on everything, or maybe a
> "debug connector property", or debugfs, or anything you might want.
> With this, you would be able to actually test your patches, and we'd
> have a useful feature.
> 
> If you disagree, please just say "no" and then I'll go back to review
> this patch :)

I think improving the PLL selection code should be a separate patch.
IIRC I even said as much in the cover letter.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/5] drm/i915: Fix DDI port_clock for VGA output

[Ville Syrjälä]

On Fri, Feb 28, 2014 at 07:09:51PM -0300, Paulo Zanoni wrote:
> 2014-02-27 9:23 GMT-03:00  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > On DDI there's no PLL as such to generate the pixel clock for VGA.
> > Instead we derive the pixel clock from the FDI link frequency. So
> > to make .compute_config match what .get_config does, we need to
> > set the port_clock based on the FDI link frequency.
> >
> > Note that we don't even check the port_clock when selecting the
> > PLL for VGA output. We just assume SPLL at 1.35GHz is what we want,
> > and that does match with the asumption of FDI frequency of 2.7Ghz
> > we have in intel_fdi_link_freq().
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=74955
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_crt.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> > index 9864aa1..02e2b02 100644
> > --- a/drivers/gpu/drm/i915/intel_crt.c
> > +++ b/drivers/gpu/drm/i915/intel_crt.c
> > @@ -262,6 +262,10 @@ static bool intel_crt_compute_config(struct intel_encoder *encoder,
> >         if (HAS_PCH_LPT(dev))
> >                 pipe_config->pipe_bpp = 24;
> >
> > +       /* FDI must always be 2.7 GHz */
> > +       if (HAS_DDI(dev))
> > +               pipe_config->port_clock = 135000 * 2;
> > +
> 
> <rant>
> Whenever I have to review patches touching the HW state
> readout/compute I get completely confused. We have a bunch of
> compute_config/get_config/get_hw_state functions, with no real
> documentation of which fields exactly each function should get.

I have no objections to improving the docs.

> Our
> hardware also does a bunch of *2 multiplications on PLLs and clocks,
> and this gets things even more confusing.

The 2x is for PLL output->bitclock, and the /5 would be for
PLL output->symbol clock. So maybe it would be nicer to use
<real PLL frequency>/5 in the DDI get_config. It would match
how we compute things on VLV at least.

> Also, we have similar
> information (like all these "clocks", from mode, adjusted_mode, port,
> pipe, hw, etc) with slightly different meanings. I really wish we had
> Kernel documentation describing *exactly* what is expected to be
> computed from each function, and what are the differences between all
> those similar things with slightly different meanings. Maybe we could
> also organize struct intel_crtc_config, adding small sub-structs that
> better define the domain of each field?
> </rant>
> 
> Still, I have reviewed your patch, and despite all my own confusions,
> it looks correct, so:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> >         return true;
> >  }
> >
> > --
> > 1.8.3.2
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/5] drm/i915: Change intel_fdi_link_freq() to 10kHz

[Daniel Vetter]

On Sat, Mar 01, 2014 at 07:34:12AM +0000, Chris Wilson wrote:
> On Fri, Feb 28, 2014 at 07:28:41PM -0300, Paulo Zanoni wrote:
> > 2014-02-27 9:23 GMT-03:00  <ville.syrjala@linux.intel.com>:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > We normally use 10Khz units when describing DP link frequency.
> > > Have intel_fdi_link_freq() return the same units. I always get confused
> > > when the units start to be totally different.
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > A nice simplification!
> 
> An oversimplification. link_bw != link_freq.

Yeah, I think we need a s/link_freq/link_bw now ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 5/5] drm/i915: Add SPLL sharing support for DDI

[Daniel Vetter]

On Mon, Mar 03, 2014 at 11:56:08AM +0200, Ville Syrjälä wrote:
> On Fri, Feb 28, 2014 at 06:40:22PM -0300, Paulo Zanoni wrote:
> > Hi
> > 
> > 2014-02-27 9:23 GMT-03:00  <ville.syrjala@linux.intel.com>:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Write some theoretical SPLL sharing support for DDI. Currently that will
> > > never happens since we never use SPLL for anything but FDI. But having
> > > the code there makes it easier if we ever want to do it, and it might
> > > excercise the PLL sharing code a bit more.
> > 
> > One thing is that if we ever want to use SSC on eDP/DP/HDMI, we will
> > want to use either SPLL or WRPLL instead of LCPLL. I always wanted to
> > implement this, and recently I saw a bug that made me think about it
> > again.
> > 
> > So instead of this patch, why don't you write support to use SSC when
> > requested? We could do this through some i915.use_ssc command line
> > option that would just try to force SSC on everything, or maybe a
> > "debug connector property", or debugfs, or anything you might want.
> > With this, you would be able to actually test your patches, and we'd
> > have a useful feature.
> > 
> > If you disagree, please just say "no" and then I'll go back to review
> > this patch :)
> 
> I think improving the PLL selection code should be a separate patch.
> IIRC I even said as much in the cover letter.

wrt improved pll selection logic I'd welcome if we could port the ddi code
and refcounting to use the somewhat generic stuff I've created for pch
plls. There's not much logic in there really, but the one thing which is
abstracted is the refcount reconstruction for the hw state readout and
cross-checking.

That part is devilishly tricky to get right apparently (it took us a
pile of iterations to get there for both the pch and ddi pll code), so imo
the right thing to do before making this more complicated.

I've merged the two fixes from this series and punted on the others for
now.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/5] drm/i915: Change intel_fdi_link_freq() to 10kHz

[Daniel Vetter]

On Wed, Mar 5, 2014 at 4:04 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Sat, Mar 01, 2014 at 07:34:12AM +0000, Chris Wilson wrote:
>> On Fri, Feb 28, 2014 at 07:28:41PM -0300, Paulo Zanoni wrote:
>> > 2014-02-27 9:23 GMT-03:00  <ville.syrjala@linux.intel.com>:
>> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > >
>> > > We normally use 10Khz units when describing DP link frequency.
>> > > Have intel_fdi_link_freq() return the same units. I always get confused
>> > > when the units start to be totally different.
>> > >
>> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > A nice simplification!
>>
>> An oversimplification. link_bw != link_freq.
>
> Yeah, I think we need a s/link_freq/link_bw now ...

To clarify: In dp we have piles of different ways to call essentially
the same thing:
- link bw code from dpcd
- symbol rate encoded at 8b/10b
- link clock

All have different units and denominators, and historically we've had
countless bugs in here. So if you want to do this please crawl through
the entire dp/fdi code and make sure to update comments and that
everything is still mostly consistent. Otherwise we don't really have
progress in this area.

I hope this helps to clarify my concern and what I guess is Chris' concern.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch