[PATCH 0/8] Remaining ICL display patches

[PATCH 0/8] Remaining ICL display patches

[Paulo Zanoni]

These are the remaining patches from the series called "[PATCH 00/24]
More ICL display patches". Patches 1, 3 and 4 still need reviews.

Animesh Manna (1):
  drm/i915/icl: Update FIA supported lane count for hpd.

Paulo Zanoni (7):
  drm/i915/icl: compute the TBT PLL registers
  drm/i915/icl: implement icl_digital_port_connected()
  drm/i915/icl: store the port type for TC ports
  drm/i915/icl: implement the tc/legacy HPD {dis,}connect flow for DP
  drm/i915/icl: implement the legacy HPD {dis,}connect flow for HDMI
  drm/i915/icl: program MG_DP_MODE
  drm/i915/icl: toggle PHY clock gating around link training

 drivers/gpu/drm/i915/i915_reg.h       |  51 ++++++
 drivers/gpu/drm/i915/intel_ddi.c      |   5 +
 drivers/gpu/drm/i915/intel_display.h  |   7 +
 drivers/gpu/drm/i915/intel_dp.c       | 311 +++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_dpll_mgr.c |  22 ++-
 drivers/gpu/drm/i915/intel_drv.h      |   4 +
 drivers/gpu/drm/i915/intel_hdmi.c     |  11 +-
 7 files changed, 406 insertions(+), 5 deletions(-)

-- 
2.14.4

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

[PATCH 1/8] drm/i915/icl: compute the TBT PLL registers

[Paulo Zanoni]

Use the hardcoded tables provided by our spec.

v2:
  - SSC stays disabled.
  - Use intel_port_is_tc().

Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index b51ad2917dbe..7e5e6eb5dfe2 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -2452,6 +2452,16 @@ static const struct skl_wrpll_params icl_dp_combo_pll_19_2MHz_values[] = {
 	  .pdiv = 0x1 /* 2 */, .kdiv = 1, .qdiv_mode = 0, .qdiv_ratio = 0},
 };
 
+static const struct skl_wrpll_params icl_tbt_pll_24MHz_values = {
+	.dco_integer = 0x151, .dco_fraction = 0x4000,
+	.pdiv = 0x4 /* 5 */, .kdiv = 1, .qdiv_mode = 0, .qdiv_ratio = 0,
+};
+
+static const struct skl_wrpll_params icl_tbt_pll_19_2MHz_values = {
+	.dco_integer = 0x1A5, .dco_fraction = 0x7000,
+	.pdiv = 0x4 /* 5 */, .kdiv = 1, .qdiv_mode = 0, .qdiv_ratio = 0,
+};
+
 static bool icl_calc_dp_combo_pll(struct drm_i915_private *dev_priv, int clock,
 				  struct skl_wrpll_params *pll_params)
 {
@@ -2494,6 +2504,14 @@ static bool icl_calc_dp_combo_pll(struct drm_i915_private *dev_priv, int clock,
 	return true;
 }
 
+static bool icl_calc_tbt_pll(struct drm_i915_private *dev_priv, int clock,
+			     struct skl_wrpll_params *pll_params)
+{
+	*pll_params = dev_priv->cdclk.hw.ref == 24000 ?
+			icl_tbt_pll_24MHz_values : icl_tbt_pll_19_2MHz_values;
+	return true;
+}
+
 static bool icl_calc_dpll_state(struct intel_crtc_state *crtc_state,
 				struct intel_encoder *encoder, int clock,
 				struct intel_dpll_hw_state *pll_state)
@@ -2503,7 +2521,9 @@ static bool icl_calc_dpll_state(struct intel_crtc_state *crtc_state,
 	struct skl_wrpll_params pll_params = { 0 };
 	bool ret;
 
-	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
+	if (intel_port_is_tc(dev_priv, encoder->port))
+		ret = icl_calc_tbt_pll(dev_priv, clock, &pll_params);
+	else if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
 		ret = cnl_ddi_calculate_wrpll(clock, dev_priv, &pll_params);
 	else
 		ret = icl_calc_dp_combo_pll(dev_priv, clock, &pll_params);
-- 
2.14.4

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

Re: [PATCH 1/8] drm/i915/icl: compute the TBT PLL registers

[Rodrigo Vivi]

On Wed, Jul 11, 2018 at 02:59:02PM -0700, Paulo Zanoni wrote:
> Use the hardcoded tables provided by our spec.
> 
> v2:
>   - SSC stays disabled.
>   - Use intel_port_is_tc().
> 
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index b51ad2917dbe..7e5e6eb5dfe2 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -2452,6 +2452,16 @@ static const struct skl_wrpll_params icl_dp_combo_pll_19_2MHz_values[] = {
>  	  .pdiv = 0x1 /* 2 */, .kdiv = 1, .qdiv_mode = 0, .qdiv_ratio = 0},
>  };
>  
> +static const struct skl_wrpll_params icl_tbt_pll_24MHz_values = {
> +	.dco_integer = 0x151, .dco_fraction = 0x4000,

I see 0x151 is the most common for 24MHz, but not the only one.
I also see 0x168 and 0x195, depending on the link rate.

So, what am I missing?

> +	.pdiv = 0x4 /* 5 */, .kdiv = 1, .qdiv_mode = 0, .qdiv_ratio = 0,
> +};
> +
> +static const struct skl_wrpll_params icl_tbt_pll_19_2MHz_values = {
> +	.dco_integer = 0x1A5, .dco_fraction = 0x7000,
> +	.pdiv = 0x4 /* 5 */, .kdiv = 1, .qdiv_mode = 0, .qdiv_ratio = 0,
> +};
> +
>  static bool icl_calc_dp_combo_pll(struct drm_i915_private *dev_priv, int clock,
>  				  struct skl_wrpll_params *pll_params)
>  {
> @@ -2494,6 +2504,14 @@ static bool icl_calc_dp_combo_pll(struct drm_i915_private *dev_priv, int clock,
>  	return true;
>  }
>  
> +static bool icl_calc_tbt_pll(struct drm_i915_private *dev_priv, int clock,
> +			     struct skl_wrpll_params *pll_params)
> +{
> +	*pll_params = dev_priv->cdclk.hw.ref == 24000 ?
> +			icl_tbt_pll_24MHz_values : icl_tbt_pll_19_2MHz_values;
> +	return true;
> +}
> +
>  static bool icl_calc_dpll_state(struct intel_crtc_state *crtc_state,
>  				struct intel_encoder *encoder, int clock,
>  				struct intel_dpll_hw_state *pll_state)
> @@ -2503,7 +2521,9 @@ static bool icl_calc_dpll_state(struct intel_crtc_state *crtc_state,
>  	struct skl_wrpll_params pll_params = { 0 };
>  	bool ret;
>  
> -	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
> +	if (intel_port_is_tc(dev_priv, encoder->port))
> +		ret = icl_calc_tbt_pll(dev_priv, clock, &pll_params);
> +	else if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
>  		ret = cnl_ddi_calculate_wrpll(clock, dev_priv, &pll_params);
>  	else
>  		ret = icl_calc_dp_combo_pll(dev_priv, clock, &pll_params);
> -- 
> 2.14.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/8] drm/i915/icl: compute the TBT PLL registers

[Paulo Zanoni]

Em Qui, 2018-07-12 às 17:16 -0700, Rodrigo Vivi escreveu:
> On Wed, Jul 11, 2018 at 02:59:02PM -0700, Paulo Zanoni wrote:
> > Use the hardcoded tables provided by our spec.
> > 
> > v2:
> >   - SSC stays disabled.
> >   - Use intel_port_is_tc().
> > 
> > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 22 +++++++++++++++++++++-
> >  1 file changed, 21 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > index b51ad2917dbe..7e5e6eb5dfe2 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > @@ -2452,6 +2452,16 @@ static const struct skl_wrpll_params
> > icl_dp_combo_pll_19_2MHz_values[] = {
> >  	  .pdiv = 0x1 /* 2 */, .kdiv = 1, .qdiv_mode = 0,
> > .qdiv_ratio = 0},
> >  };
> >  
> > +static const struct skl_wrpll_params icl_tbt_pll_24MHz_values = {
> > +	.dco_integer = 0x151, .dco_fraction = 0x4000,
> 
> I see 0x151 is the most common for 24MHz, but not the only one.
> I also see 0x168 and 0x195, depending on the link rate.
> 
> So, what am I missing?

Are you looking at the table inside the Combo PLL section instead of
the TBT PLL section? On that same page you are, scroll down by a lot
until the "TBT PLL Divider Values" table inside the TBT section.


> 
> > +	.pdiv = 0x4 /* 5 */, .kdiv = 1, .qdiv_mode = 0,
> > .qdiv_ratio = 0,
> > +};
> > +
> > +static const struct skl_wrpll_params icl_tbt_pll_19_2MHz_values =
> > {
> > +	.dco_integer = 0x1A5, .dco_fraction = 0x7000,
> > +	.pdiv = 0x4 /* 5 */, .kdiv = 1, .qdiv_mode = 0,
> > .qdiv_ratio = 0,
> > +};
> > +
> >  static bool icl_calc_dp_combo_pll(struct drm_i915_private
> > *dev_priv, int clock,
> >  				  struct skl_wrpll_params
> > *pll_params)
> >  {
> > @@ -2494,6 +2504,14 @@ static bool icl_calc_dp_combo_pll(struct
> > drm_i915_private *dev_priv, int clock,
> >  	return true;
> >  }
> >  
> > +static bool icl_calc_tbt_pll(struct drm_i915_private *dev_priv,
> > int clock,
> > +			     struct skl_wrpll_params *pll_params)
> > +{
> > +	*pll_params = dev_priv->cdclk.hw.ref == 24000 ?
> > +			icl_tbt_pll_24MHz_values :
> > icl_tbt_pll_19_2MHz_values;
> > +	return true;
> > +}
> > +
> >  static bool icl_calc_dpll_state(struct intel_crtc_state
> > *crtc_state,
> >  				struct intel_encoder *encoder, int
> > clock,
> >  				struct intel_dpll_hw_state
> > *pll_state)
> > @@ -2503,7 +2521,9 @@ static bool icl_calc_dpll_state(struct
> > intel_crtc_state *crtc_state,
> >  	struct skl_wrpll_params pll_params = { 0 };
> >  	bool ret;
> >  
> > -	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
> > +	if (intel_port_is_tc(dev_priv, encoder->port))
> > +		ret = icl_calc_tbt_pll(dev_priv, clock,
> > &pll_params);
> > +	else if (intel_crtc_has_type(crtc_state,
> > INTEL_OUTPUT_HDMI))
> >  		ret = cnl_ddi_calculate_wrpll(clock, dev_priv,
> > &pll_params);
> >  	else
> >  		ret = icl_calc_dp_combo_pll(dev_priv, clock,
> > &pll_params);
> > -- 
> > 2.14.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/8] drm/i915/icl: compute the TBT PLL registers

[Rodrigo Vivi]

On Fri, Jul 13, 2018 at 10:20:27AM -0700, Paulo Zanoni wrote:
> Em Qui, 2018-07-12 às 17:16 -0700, Rodrigo Vivi escreveu:
> > On Wed, Jul 11, 2018 at 02:59:02PM -0700, Paulo Zanoni wrote:
> > > Use the hardcoded tables provided by our spec.
> > > 
> > > v2:
> > >   - SSC stays disabled.
> > >   - Use intel_port_is_tc().
> > > 
> > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 22 +++++++++++++++++++++-
> > >  1 file changed, 21 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > index b51ad2917dbe..7e5e6eb5dfe2 100644
> > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > @@ -2452,6 +2452,16 @@ static const struct skl_wrpll_params
> > > icl_dp_combo_pll_19_2MHz_values[] = {
> > >  	  .pdiv = 0x1 /* 2 */, .kdiv = 1, .qdiv_mode = 0,
> > > .qdiv_ratio = 0},
> > >  };
> > >  
> > > +static const struct skl_wrpll_params icl_tbt_pll_24MHz_values = {
> > > +	.dco_integer = 0x151, .dco_fraction = 0x4000,
> > 
> > I see 0x151 is the most common for 24MHz, but not the only one.
> > I also see 0x168 and 0x195, depending on the link rate.
> > 
> > So, what am I missing?
> 
> Are you looking at the table inside the Combo PLL section instead of
> the TBT PLL section? On that same page you are, scroll down by a lot
> until the "TBT PLL Divider Values" table inside the TBT section.

Ha! I knew I was missing something.

Could the order be inverted to match the spec order?
first line 19.2
second 24

> 
> 
> > 
> > > +	.pdiv = 0x4 /* 5 */, .kdiv = 1, .qdiv_mode = 0,
> > > .qdiv_ratio = 0,

Also, I see pdiv = 5 and and qdiv = 1...

but I'm sure that I'm missing something else again...
probably some mathmagic somewhere... where?

anyways, if that is the case could we change to this table
here to match spec one?

Thanks,
Rodrigo.

> > > +};
> > > +
> > > +static const struct skl_wrpll_params icl_tbt_pll_19_2MHz_values =
> > > {
> > > +	.dco_integer = 0x1A5, .dco_fraction = 0x7000,
> > > +	.pdiv = 0x4 /* 5 */, .kdiv = 1, .qdiv_mode = 0,
> > > .qdiv_ratio = 0,
> > > +};
> > > +
> > >  static bool icl_calc_dp_combo_pll(struct drm_i915_private
> > > *dev_priv, int clock,
> > >  				  struct skl_wrpll_params
> > > *pll_params)
> > >  {
> > > @@ -2494,6 +2504,14 @@ static bool icl_calc_dp_combo_pll(struct
> > > drm_i915_private *dev_priv, int clock,
> > >  	return true;
> > >  }
> > >  
> > > +static bool icl_calc_tbt_pll(struct drm_i915_private *dev_priv,
> > > int clock,
> > > +			     struct skl_wrpll_params *pll_params)
> > > +{
> > > +	*pll_params = dev_priv->cdclk.hw.ref == 24000 ?
> > > +			icl_tbt_pll_24MHz_values :
> > > icl_tbt_pll_19_2MHz_values;
> > > +	return true;
> > > +}
> > > +
> > >  static bool icl_calc_dpll_state(struct intel_crtc_state
> > > *crtc_state,
> > >  				struct intel_encoder *encoder, int
> > > clock,
> > >  				struct intel_dpll_hw_state
> > > *pll_state)
> > > @@ -2503,7 +2521,9 @@ static bool icl_calc_dpll_state(struct
> > > intel_crtc_state *crtc_state,
> > >  	struct skl_wrpll_params pll_params = { 0 };
> > >  	bool ret;
> > >  
> > > -	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
> > > +	if (intel_port_is_tc(dev_priv, encoder->port))
> > > +		ret = icl_calc_tbt_pll(dev_priv, clock,
> > > &pll_params);
> > > +	else if (intel_crtc_has_type(crtc_state,
> > > INTEL_OUTPUT_HDMI))
> > >  		ret = cnl_ddi_calculate_wrpll(clock, dev_priv,
> > > &pll_params);
> > >  	else
> > >  		ret = icl_calc_dp_combo_pll(dev_priv, clock,
> > > &pll_params);
> > > -- 
> > > 2.14.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/8] drm/i915/icl: compute the TBT PLL registers

[Paulo Zanoni]

Em Sex, 2018-07-13 às 11:04 -0700, Rodrigo Vivi escreveu:
> On Fri, Jul 13, 2018 at 10:20:27AM -0700, Paulo Zanoni wrote:
> > Em Qui, 2018-07-12 às 17:16 -0700, Rodrigo Vivi escreveu:
> > > On Wed, Jul 11, 2018 at 02:59:02PM -0700, Paulo Zanoni wrote:
> > > > Use the hardcoded tables provided by our spec.
> > > > 
> > > > v2:
> > > >   - SSC stays disabled.
> > > >   - Use intel_port_is_tc().
> > > > 
> > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 22
> > > > +++++++++++++++++++++-
> > > >  1 file changed, 21 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > index b51ad2917dbe..7e5e6eb5dfe2 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > @@ -2452,6 +2452,16 @@ static const struct skl_wrpll_params
> > > > icl_dp_combo_pll_19_2MHz_values[] = {
> > > >  	  .pdiv = 0x1 /* 2 */, .kdiv = 1, .qdiv_mode = 0,
> > > > .qdiv_ratio = 0},
> > > >  };
> > > >  
> > > > +static const struct skl_wrpll_params icl_tbt_pll_24MHz_values
> > > > = {
> > > > +	.dco_integer = 0x151, .dco_fraction = 0x4000,
> > > 
> > > I see 0x151 is the most common for 24MHz, but not the only one.
> > > I also see 0x168 and 0x195, depending on the link rate.
> > > 
> > > So, what am I missing?
> > 
> > Are you looking at the table inside the Combo PLL section instead
> > of
> > the TBT PLL section? On that same page you are, scroll down by a
> > lot
> > until the "TBT PLL Divider Values" table inside the TBT section.
> 
> Ha! I knew I was missing something.
> 
> Could the order be inverted to match the spec order?
> first line 19.2
> second 24

The combo table has 24 first (both in BSpec and in the code), so this
patch matches the order that's already in our code. Also the code
checks usually put 24000 first (since "else" fits 19200 and 38400), so
I kept the "24000 first" pattern. But I can paint it with the other
color if you insist :).


> 
> > 
> > 
> > > 
> > > > +	.pdiv = 0x4 /* 5 */, .kdiv = 1, .qdiv_mode = 0,
> > > > .qdiv_ratio = 0,
> 
> Also, I see pdiv = 5 and and qdiv = 1...
> 
> but I'm sure that I'm missing something else again...
> probably some mathmagic somewhere... where?

Look at the DPLL_CFGCR1 definition (page 15725).

Bit 9 value 0 means Q divider = 1.

Bits 8:6 value 100b means 3.

Bits 5:2 value 0100b means 5, 1000b means 7.

Amazing, huh?

See the combo pll table: it includes both the bit numbers and the
"original" numbers. The TBT one doesn't. That's very confusing, I know.
Our table is supposed to have the bits to add to the registers, with
the "real numbers" in comments. This also matches the combo tables
defined just above.

> 
> anyways, if that is the case could we change to this table
> here to match spec one?

I would prefer to keep as-is since otherwise we'd have to add code to
translate from the table numbers to register bit numbers.

> 
> Thanks,
> Rodrigo.
> 
> > > > +};
> > > > +
> > > > +static const struct skl_wrpll_params
> > > > icl_tbt_pll_19_2MHz_values =
> > > > {
> > > > +	.dco_integer = 0x1A5, .dco_fraction = 0x7000,
> > > > +	.pdiv = 0x4 /* 5 */, .kdiv = 1, .qdiv_mode = 0,
> > > > .qdiv_ratio = 0,
> > > > +};
> > > > +
> > > >  static bool icl_calc_dp_combo_pll(struct drm_i915_private
> > > > *dev_priv, int clock,
> > > >  				  struct skl_wrpll_params
> > > > *pll_params)
> > > >  {
> > > > @@ -2494,6 +2504,14 @@ static bool icl_calc_dp_combo_pll(struct
> > > > drm_i915_private *dev_priv, int clock,
> > > >  	return true;
> > > >  }
> > > >  
> > > > +static bool icl_calc_tbt_pll(struct drm_i915_private
> > > > *dev_priv,
> > > > int clock,
> > > > +			     struct skl_wrpll_params
> > > > *pll_params)
> > > > +{
> > > > +	*pll_params = dev_priv->cdclk.hw.ref == 24000 ?
> > > > +			icl_tbt_pll_24MHz_values :
> > > > icl_tbt_pll_19_2MHz_values;
> > > > +	return true;
> > > > +}
> > > > +
> > > >  static bool icl_calc_dpll_state(struct intel_crtc_state
> > > > *crtc_state,
> > > >  				struct intel_encoder *encoder,
> > > > int
> > > > clock,
> > > >  				struct intel_dpll_hw_state
> > > > *pll_state)
> > > > @@ -2503,7 +2521,9 @@ static bool icl_calc_dpll_state(struct
> > > > intel_crtc_state *crtc_state,
> > > >  	struct skl_wrpll_params pll_params = { 0 };
> > > >  	bool ret;
> > > >  
> > > > -	if (intel_crtc_has_type(crtc_state,
> > > > INTEL_OUTPUT_HDMI))
> > > > +	if (intel_port_is_tc(dev_priv, encoder->port))
> > > > +		ret = icl_calc_tbt_pll(dev_priv, clock,
> > > > &pll_params);
> > > > +	else if (intel_crtc_has_type(crtc_state,
> > > > INTEL_OUTPUT_HDMI))
> > > >  		ret = cnl_ddi_calculate_wrpll(clock, dev_priv,
> > > > &pll_params);
> > > >  	else
> > > >  		ret = icl_calc_dp_combo_pll(dev_priv, clock,
> > > > &pll_params);
> > > > -- 
> > > > 2.14.4
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/8] drm/i915/icl: compute the TBT PLL registers

[Rodrigo Vivi]

On Fri, Jul 13, 2018 at 11:43:47AM -0700, Paulo Zanoni wrote:
> Em Sex, 2018-07-13 às 11:04 -0700, Rodrigo Vivi escreveu:
> > On Fri, Jul 13, 2018 at 10:20:27AM -0700, Paulo Zanoni wrote:
> > > Em Qui, 2018-07-12 às 17:16 -0700, Rodrigo Vivi escreveu:
> > > > On Wed, Jul 11, 2018 at 02:59:02PM -0700, Paulo Zanoni wrote:
> > > > > Use the hardcoded tables provided by our spec.
> > > > > 
> > > > > v2:
> > > > >   - SSC stays disabled.
> > > > >   - Use intel_port_is_tc().
> > > > > 
> > > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 22
> > > > > +++++++++++++++++++++-
> > > > >  1 file changed, 21 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > index b51ad2917dbe..7e5e6eb5dfe2 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > @@ -2452,6 +2452,16 @@ static const struct skl_wrpll_params
> > > > > icl_dp_combo_pll_19_2MHz_values[] = {
> > > > >  	  .pdiv = 0x1 /* 2 */, .kdiv = 1, .qdiv_mode = 0,
> > > > > .qdiv_ratio = 0},
> > > > >  };
> > > > >  
> > > > > +static const struct skl_wrpll_params icl_tbt_pll_24MHz_values
> > > > > = {
> > > > > +	.dco_integer = 0x151, .dco_fraction = 0x4000,
> > > > 
> > > > I see 0x151 is the most common for 24MHz, but not the only one.
> > > > I also see 0x168 and 0x195, depending on the link rate.
> > > > 
> > > > So, what am I missing?
> > > 
> > > Are you looking at the table inside the Combo PLL section instead
> > > of
> > > the TBT PLL section? On that same page you are, scroll down by a
> > > lot
> > > until the "TBT PLL Divider Values" table inside the TBT section.
> > 
> > Ha! I knew I was missing something.
> > 
> > Could the order be inverted to match the spec order?
> > first line 19.2
> > second 24
> 
> The combo table has 24 first (both in BSpec and in the code), so this
> patch matches the order that's already in our code. Also the code
> checks usually put 24000 first (since "else" fits 19200 and 38400), so
> I kept the "24000 first" pattern. But I can paint it with the other
> color if you insist :).

Nope, I don't insist.

> 
> 
> > 
> > > 
> > > 
> > > > 
> > > > > +	.pdiv = 0x4 /* 5 */, .kdiv = 1, .qdiv_mode = 0,
> > > > > .qdiv_ratio = 0,
> > 
> > Also, I see pdiv = 5 and and qdiv = 1...
> > 
> > but I'm sure that I'm missing something else again...
> > probably some mathmagic somewhere... where?
> 
> Look at the DPLL_CFGCR1 definition (page 15725).
> 
> Bit 9 value 0 means Q divider = 1.

I'm afraid there is a confusion here between qdiv ratio
and qdiv mode here.

Bit 9 you mention should be qdiv_mode which should be 0 for TBT indeed.

But qdiv ratio 1 should be qdiv = 1

> 
> Bits 8:6 value 100b means 3.
> 
> Bits 5:2 value 0100b means 5, 1000b means 7.

for these ones you are right.

> 
> Amazing, huh?
> 
> See the combo pll table: it includes both the bit numbers and the
> "original" numbers. The TBT one doesn't. That's very confusing, I know.
> Our table is supposed to have the bits to add to the registers, with
> the "real numbers" in comments. This also matches the combo tables
> defined just above.

I agree... no point... specially because I can't see an standard of the
math there, so better to do .pdiv = 0x4 /* 5 */ like everywhere else
is doing already anyways.

> 
> > 
> > anyways, if that is the case could we change to this table
> > here to match spec one?
> 
> I would prefer to keep as-is since otherwise we'd have to add code to
> translate from the table numbers to register bit numbers.
> 
> > 
> > Thanks,
> > Rodrigo.
> > 
> > > > > +};
> > > > > +
> > > > > +static const struct skl_wrpll_params
> > > > > icl_tbt_pll_19_2MHz_values =
> > > > > {
> > > > > +	.dco_integer = 0x1A5, .dco_fraction = 0x7000,
> > > > > +	.pdiv = 0x4 /* 5 */, .kdiv = 1, .qdiv_mode = 0,
> > > > > .qdiv_ratio = 0,
> > > > > +};
> > > > > +
> > > > >  static bool icl_calc_dp_combo_pll(struct drm_i915_private
> > > > > *dev_priv, int clock,
> > > > >  				  struct skl_wrpll_params
> > > > > *pll_params)
> > > > >  {
> > > > > @@ -2494,6 +2504,14 @@ static bool icl_calc_dp_combo_pll(struct
> > > > > drm_i915_private *dev_priv, int clock,
> > > > >  	return true;
> > > > >  }
> > > > >  
> > > > > +static bool icl_calc_tbt_pll(struct drm_i915_private
> > > > > *dev_priv,
> > > > > int clock,
> > > > > +			     struct skl_wrpll_params
> > > > > *pll_params)
> > > > > +{
> > > > > +	*pll_params = dev_priv->cdclk.hw.ref == 24000 ?
> > > > > +			icl_tbt_pll_24MHz_values :
> > > > > icl_tbt_pll_19_2MHz_values;
> > > > > +	return true;
> > > > > +}
> > > > > +
> > > > >  static bool icl_calc_dpll_state(struct intel_crtc_state
> > > > > *crtc_state,
> > > > >  				struct intel_encoder *encoder,
> > > > > int
> > > > > clock,
> > > > >  				struct intel_dpll_hw_state
> > > > > *pll_state)
> > > > > @@ -2503,7 +2521,9 @@ static bool icl_calc_dpll_state(struct
> > > > > intel_crtc_state *crtc_state,
> > > > >  	struct skl_wrpll_params pll_params = { 0 };
> > > > >  	bool ret;
> > > > >  
> > > > > -	if (intel_crtc_has_type(crtc_state,
> > > > > INTEL_OUTPUT_HDMI))
> > > > > +	if (intel_port_is_tc(dev_priv, encoder->port))
> > > > > +		ret = icl_calc_tbt_pll(dev_priv, clock,
> > > > > &pll_params);
> > > > > +	else if (intel_crtc_has_type(crtc_state,
> > > > > INTEL_OUTPUT_HDMI))
> > > > >  		ret = cnl_ddi_calculate_wrpll(clock, dev_priv,
> > > > > &pll_params);
> > > > >  	else
> > > > >  		ret = icl_calc_dp_combo_pll(dev_priv, clock,
> > > > > &pll_params);
> > > > > -- 
> > > > > 2.14.4
> > > > > 
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/8] drm/i915/icl: compute the TBT PLL registers

[Rodrigo Vivi]

On Wed, Jul 11, 2018 at 02:59:02PM -0700, Paulo Zanoni wrote:
> Use the hardcoded tables provided by our spec.
> 
> v2:
>   - SSC stays disabled.
>   - Use intel_port_is_tc().
> 
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index b51ad2917dbe..7e5e6eb5dfe2 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -2452,6 +2452,16 @@ static const struct skl_wrpll_params icl_dp_combo_pll_19_2MHz_values[] = {
>  	  .pdiv = 0x1 /* 2 */, .kdiv = 1, .qdiv_mode = 0, .qdiv_ratio = 0},
>  };
>  
> +static const struct skl_wrpll_params icl_tbt_pll_24MHz_values = {
> +	.dco_integer = 0x151, .dco_fraction = 0x4000,
> +	.pdiv = 0x4 /* 5 */, .kdiv = 1, .qdiv_mode = 0, .qdiv_ratio = 0,
> +};
> +
> +static const struct skl_wrpll_params icl_tbt_pll_19_2MHz_values = {
> +	.dco_integer = 0x1A5, .dco_fraction = 0x7000,
> +	.pdiv = 0x4 /* 5 */, .kdiv = 1, .qdiv_mode = 0, .qdiv_ratio = 0,

in other words, in a cleaner view:

s/qdiv_ratio = 0/qdiv_ratio = 1/g #both tables

with that:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> +};
> +
>  static bool icl_calc_dp_combo_pll(struct drm_i915_private *dev_priv, int clock,
>  				  struct skl_wrpll_params *pll_params)
>  {
> @@ -2494,6 +2504,14 @@ static bool icl_calc_dp_combo_pll(struct drm_i915_private *dev_priv, int clock,
>  	return true;
>  }
>  
> +static bool icl_calc_tbt_pll(struct drm_i915_private *dev_priv, int clock,
> +			     struct skl_wrpll_params *pll_params)
> +{
> +	*pll_params = dev_priv->cdclk.hw.ref == 24000 ?
> +			icl_tbt_pll_24MHz_values : icl_tbt_pll_19_2MHz_values;
> +	return true;
> +}
> +
>  static bool icl_calc_dpll_state(struct intel_crtc_state *crtc_state,
>  				struct intel_encoder *encoder, int clock,
>  				struct intel_dpll_hw_state *pll_state)
> @@ -2503,7 +2521,9 @@ static bool icl_calc_dpll_state(struct intel_crtc_state *crtc_state,
>  	struct skl_wrpll_params pll_params = { 0 };
>  	bool ret;
>  
> -	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
> +	if (intel_port_is_tc(dev_priv, encoder->port))
> +		ret = icl_calc_tbt_pll(dev_priv, clock, &pll_params);
> +	else if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
>  		ret = cnl_ddi_calculate_wrpll(clock, dev_priv, &pll_params);
>  	else
>  		ret = icl_calc_dp_combo_pll(dev_priv, clock, &pll_params);
> -- 
> 2.14.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/8] drm/i915/icl: compute the TBT PLL registers

[Paulo Zanoni]

Em Sex, 2018-07-13 às 14:08 -0700, Rodrigo Vivi escreveu:
> On Wed, Jul 11, 2018 at 02:59:02PM -0700, Paulo Zanoni wrote:
> > Use the hardcoded tables provided by our spec.
> > 
> > v2:
> >   - SSC stays disabled.
> >   - Use intel_port_is_tc().
> > 
> > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 22 +++++++++++++++++++++-
> >  1 file changed, 21 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > index b51ad2917dbe..7e5e6eb5dfe2 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > @@ -2452,6 +2452,16 @@ static const struct skl_wrpll_params
> > icl_dp_combo_pll_19_2MHz_values[] = {
> >  	  .pdiv = 0x1 /* 2 */, .kdiv = 1, .qdiv_mode = 0,
> > .qdiv_ratio = 0},
> >  };
> >  
> > +static const struct skl_wrpll_params icl_tbt_pll_24MHz_values = {
> > +	.dco_integer = 0x151, .dco_fraction = 0x4000,
> > +	.pdiv = 0x4 /* 5 */, .kdiv = 1, .qdiv_mode = 0,
> > .qdiv_ratio = 0,
> > +};
> > +
> > +static const struct skl_wrpll_params icl_tbt_pll_19_2MHz_values =
> > {
> > +	.dco_integer = 0x1A5, .dco_fraction = 0x7000,
> > +	.pdiv = 0x4 /* 5 */, .kdiv = 1, .qdiv_mode = 0,
> > .qdiv_ratio = 0,
> 
> in other words, in a cleaner view:
> 
> s/qdiv_ratio = 0/qdiv_ratio = 1/g #both tables

From the qdiv ratio bit:

"This field specifies the Q divider ratio. This field is only used when
Qdiv Mode is set to Enable to get a divider value other than 1.".

So having 0 or 1 shouldn't matter since qdiv_mode is zero.

On the other hand, if you look at the Combo PLL table, when qdiv_mode=0
it explicitly tells us to use qdiv_ratio=0, opposite to what you're
asking.

Then the example below and also the DSI example later both keep saying
to use qdiv_mode=0 + qdiv_ratio=0.

Now the TBT PLL example later asks for qdiv_mode=0 qdiv_ratio=1, which
is what you're asking here, but it's the only the only thing that asks
for that pattern in that way, which makes me believe that either it's
wrong (unlikely) or it simply doesn't matter (as written in the bit
definition).

> 
> with that:
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> > +};
> > +
> >  static bool icl_calc_dp_combo_pll(struct drm_i915_private
> > *dev_priv, int clock,
> >  				  struct skl_wrpll_params
> > *pll_params)
> >  {
> > @@ -2494,6 +2504,14 @@ static bool icl_calc_dp_combo_pll(struct
> > drm_i915_private *dev_priv, int clock,
> >  	return true;
> >  }
> >  
> > +static bool icl_calc_tbt_pll(struct drm_i915_private *dev_priv,
> > int clock,
> > +			     struct skl_wrpll_params *pll_params)
> > +{
> > +	*pll_params = dev_priv->cdclk.hw.ref == 24000 ?
> > +			icl_tbt_pll_24MHz_values :
> > icl_tbt_pll_19_2MHz_values;
> > +	return true;
> > +}
> > +
> >  static bool icl_calc_dpll_state(struct intel_crtc_state
> > *crtc_state,
> >  				struct intel_encoder *encoder, int
> > clock,
> >  				struct intel_dpll_hw_state
> > *pll_state)
> > @@ -2503,7 +2521,9 @@ static bool icl_calc_dpll_state(struct
> > intel_crtc_state *crtc_state,
> >  	struct skl_wrpll_params pll_params = { 0 };
> >  	bool ret;
> >  
> > -	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
> > +	if (intel_port_is_tc(dev_priv, encoder->port))
> > +		ret = icl_calc_tbt_pll(dev_priv, clock,
> > &pll_params);
> > +	else if (intel_crtc_has_type(crtc_state,
> > INTEL_OUTPUT_HDMI))
> >  		ret = cnl_ddi_calculate_wrpll(clock, dev_priv,
> > &pll_params);
> >  	else
> >  		ret = icl_calc_dp_combo_pll(dev_priv, clock,
> > &pll_params);
> > -- 
> > 2.14.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/8] drm/i915/icl: compute the TBT PLL registers

[Rodrigo Vivi]

On Fri, Jul 13, 2018 at 03:57:45PM -0700, Paulo Zanoni wrote:
> Em Sex, 2018-07-13 às 14:08 -0700, Rodrigo Vivi escreveu:
> > On Wed, Jul 11, 2018 at 02:59:02PM -0700, Paulo Zanoni wrote:
> > > Use the hardcoded tables provided by our spec.
> > > 
> > > v2:
> > >   - SSC stays disabled.
> > >   - Use intel_port_is_tc().
> > > 
> > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 22 +++++++++++++++++++++-
> > >  1 file changed, 21 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > index b51ad2917dbe..7e5e6eb5dfe2 100644
> > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > @@ -2452,6 +2452,16 @@ static const struct skl_wrpll_params
> > > icl_dp_combo_pll_19_2MHz_values[] = {
> > >  	  .pdiv = 0x1 /* 2 */, .kdiv = 1, .qdiv_mode = 0,
> > > .qdiv_ratio = 0},
> > >  };
> > >  
> > > +static const struct skl_wrpll_params icl_tbt_pll_24MHz_values = {
> > > +	.dco_integer = 0x151, .dco_fraction = 0x4000,
> > > +	.pdiv = 0x4 /* 5 */, .kdiv = 1, .qdiv_mode = 0,
> > > .qdiv_ratio = 0,
> > > +};
> > > +
> > > +static const struct skl_wrpll_params icl_tbt_pll_19_2MHz_values =
> > > {
> > > +	.dco_integer = 0x1A5, .dco_fraction = 0x7000,
> > > +	.pdiv = 0x4 /* 5 */, .kdiv = 1, .qdiv_mode = 0,
> > > .qdiv_ratio = 0,
> > 
> > in other words, in a cleaner view:
> > 
> > s/qdiv_ratio = 0/qdiv_ratio = 1/g #both tables
> 
> From the qdiv ratio bit:
> 
> "This field specifies the Q divider ratio. This field is only used when
> Qdiv Mode is set to Enable to get a divider value other than 1.".
> 
> So having 0 or 1 shouldn't matter since qdiv_mode is zero.
> 
> On the other hand, if you look at the Combo PLL table, when qdiv_mode=0
> it explicitly tells us to use qdiv_ratio=0, opposite to what you're
> asking.
> 
> Then the example below and also the DSI example later both keep saying
> to use qdiv_mode=0 + qdiv_ratio=0.
> 
> Now the TBT PLL example later asks for qdiv_mode=0 qdiv_ratio=1, which
> is what you're asking here, but it's the only the only thing that asks
> for that pattern in that way, which makes me believe that either it's
> wrong (unlikely) or it simply doesn't matter (as written in the bit
> definition).

(just adding a note about what we already discussed in pvt last friday)

On the TBL section there are 3 places they add qdiv = 1, so spec is clearly
non-sense because at same time it "indicates" that for qmode=0 qdiv is always 1
it asks driver to set, because qmode for TBL is always 0 anyways.

So in my view or bspec is just completely wrong or it is stating that it is
driver's responsibility to set it to 1.

Anyways since you already filed the bug against the bspec we could just move
to either way for now... and fix later...

However one thing still bugs me... the mismatch of code and spec table. Even
if spec tells it doesn't matter but they don't update the value there.

I can see more devs in the future getting confused because qdiv=1 is part of
spec's table and code's table mismatch setting it to 0.

> 
> > 
> > with that:
> > 
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > 
> > > +};
> > > +
> > >  static bool icl_calc_dp_combo_pll(struct drm_i915_private
> > > *dev_priv, int clock,
> > >  				  struct skl_wrpll_params
> > > *pll_params)
> > >  {
> > > @@ -2494,6 +2504,14 @@ static bool icl_calc_dp_combo_pll(struct
> > > drm_i915_private *dev_priv, int clock,
> > >  	return true;
> > >  }
> > >  
> > > +static bool icl_calc_tbt_pll(struct drm_i915_private *dev_priv,
> > > int clock,
> > > +			     struct skl_wrpll_params *pll_params)
> > > +{
> > > +	*pll_params = dev_priv->cdclk.hw.ref == 24000 ?
> > > +			icl_tbt_pll_24MHz_values :
> > > icl_tbt_pll_19_2MHz_values;
> > > +	return true;
> > > +}
> > > +
> > >  static bool icl_calc_dpll_state(struct intel_crtc_state
> > > *crtc_state,
> > >  				struct intel_encoder *encoder, int
> > > clock,
> > >  				struct intel_dpll_hw_state
> > > *pll_state)
> > > @@ -2503,7 +2521,9 @@ static bool icl_calc_dpll_state(struct
> > > intel_crtc_state *crtc_state,
> > >  	struct skl_wrpll_params pll_params = { 0 };
> > >  	bool ret;
> > >  
> > > -	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
> > > +	if (intel_port_is_tc(dev_priv, encoder->port))
> > > +		ret = icl_calc_tbt_pll(dev_priv, clock,
> > > &pll_params);
> > > +	else if (intel_crtc_has_type(crtc_state,
> > > INTEL_OUTPUT_HDMI))
> > >  		ret = cnl_ddi_calculate_wrpll(clock, dev_priv,
> > > &pll_params);
> > >  	else
> > >  		ret = icl_calc_dp_combo_pll(dev_priv, clock,
> > > &pll_params);
> > > -- 
> > > 2.14.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/8] drm/i915/icl: compute the TBT PLL registers

[Paulo Zanoni]

Em Seg, 2018-07-16 às 15:47 -0700, Rodrigo Vivi escreveu:
> On Fri, Jul 13, 2018 at 03:57:45PM -0700, Paulo Zanoni wrote:
> > Em Sex, 2018-07-13 às 14:08 -0700, Rodrigo Vivi escreveu:
> > > On Wed, Jul 11, 2018 at 02:59:02PM -0700, Paulo Zanoni wrote:
> > > > Use the hardcoded tables provided by our spec.
> > > > 
> > > > v2:
> > > >   - SSC stays disabled.
> > > >   - Use intel_port_is_tc().
> > > > 
> > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 22
> > > > +++++++++++++++++++++-
> > > >  1 file changed, 21 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > index b51ad2917dbe..7e5e6eb5dfe2 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > @@ -2452,6 +2452,16 @@ static const struct skl_wrpll_params
> > > > icl_dp_combo_pll_19_2MHz_values[] = {
> > > >  	  .pdiv = 0x1 /* 2 */, .kdiv = 1, .qdiv_mode = 0,
> > > > .qdiv_ratio = 0},
> > > >  };
> > > >  
> > > > +static const struct skl_wrpll_params icl_tbt_pll_24MHz_values
> > > > = {
> > > > +	.dco_integer = 0x151, .dco_fraction = 0x4000,
> > > > +	.pdiv = 0x4 /* 5 */, .kdiv = 1, .qdiv_mode = 0,
> > > > .qdiv_ratio = 0,
> > > > +};
> > > > +
> > > > +static const struct skl_wrpll_params
> > > > icl_tbt_pll_19_2MHz_values =
> > > > {
> > > > +	.dco_integer = 0x1A5, .dco_fraction = 0x7000,
> > > > +	.pdiv = 0x4 /* 5 */, .kdiv = 1, .qdiv_mode = 0,
> > > > .qdiv_ratio = 0,
> > > 
> > > in other words, in a cleaner view:
> > > 
> > > s/qdiv_ratio = 0/qdiv_ratio = 1/g #both tables
> > 
> > From the qdiv ratio bit:
> > 
> > "This field specifies the Q divider ratio. This field is only used
> > when
> > Qdiv Mode is set to Enable to get a divider value other than 1.".
> > 
> > So having 0 or 1 shouldn't matter since qdiv_mode is zero.
> > 
> > On the other hand, if you look at the Combo PLL table, when
> > qdiv_mode=0
> > it explicitly tells us to use qdiv_ratio=0, opposite to what you're
> > asking.
> > 
> > Then the example below and also the DSI example later both keep
> > saying
> > to use qdiv_mode=0 + qdiv_ratio=0.
> > 
> > Now the TBT PLL example later asks for qdiv_mode=0 qdiv_ratio=1,
> > which
> > is what you're asking here, but it's the only the only thing that
> > asks
> > for that pattern in that way, which makes me believe that either
> > it's
> > wrong (unlikely) or it simply doesn't matter (as written in the bit
> > definition).
> 
> (just adding a note about what we already discussed in pvt last
> friday)
> 
> On the TBL section there are 3 places they add qdiv = 1, so spec is
> clearly
> non-sense because at same time it "indicates" that for qmode=0 qdiv
> is always 1
> it asks driver to set, because qmode for TBL is always 0 anyways.
> 
> So in my view or bspec is just completely wrong or it is stating that
> it is
> driver's responsibility to set it to 1.
> 
> Anyways since you already filed the bug against the bspec we could
> just move
> to either way for now... and fix later...
> 
> However one thing still bugs me... the mismatch of code and spec
> table. Even
> if spec tells it doesn't matter but they don't update the value 
> there.

As I explained in person, I don't think there is a mismatch of code and
table. When the spec says "qdiv" it means the high-level qdiv value,
which is set through the combination of qdiv_mode and qdiv_ratio.

So the TBT table says "qdiv=1", which you can achieve by setting
qdiv_mode=0 qdiv_ratio=whatever. 

The Combo table is more explicit that it lists both qdiv_mode and
qdiv_ratio to be set, and it also lists the real "qdiv" as a number in
parens on the qdiv_ratio column.

The only thing that really "mismatches" is that in the examples, when
qdiv is 1, in three places they say we need qdiv_mode=0 qdiv_ratio=0
and in the TBT example they say to use qdiv_mode=0 and qdiv_ratio=1.
But that perceived "mismatch" shouldn't matter since when qdiv_mode=0
qdiv_ratio can be anything. Still inconsistent, but IMHO irrelevant.

> 
> I can see more devs in the future getting confused because qdiv=1 is
> part of
> spec's table and code's table mismatch setting it to 0.

I think your confusion is that you don't see qdiv as a thing that is
achieved through qdiv_mode and qdiv_ratio.

> 
> > 
> > > 
> > > with that:
> > > 
> > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > 
> > > > +};
> > > > +
> > > >  static bool icl_calc_dp_combo_pll(struct drm_i915_private
> > > > *dev_priv, int clock,
> > > >  				  struct skl_wrpll_params
> > > > *pll_params)
> > > >  {
> > > > @@ -2494,6 +2504,14 @@ static bool icl_calc_dp_combo_pll(struct
> > > > drm_i915_private *dev_priv, int clock,
> > > >  	return true;
> > > >  }
> > > >  
> > > > +static bool icl_calc_tbt_pll(struct drm_i915_private
> > > > *dev_priv,
> > > > int clock,
> > > > +			     struct skl_wrpll_params
> > > > *pll_params)
> > > > +{
> > > > +	*pll_params = dev_priv->cdclk.hw.ref == 24000 ?
> > > > +			icl_tbt_pll_24MHz_values :
> > > > icl_tbt_pll_19_2MHz_values;
> > > > +	return true;
> > > > +}
> > > > +
> > > >  static bool icl_calc_dpll_state(struct intel_crtc_state
> > > > *crtc_state,
> > > >  				struct intel_encoder *encoder,
> > > > int
> > > > clock,
> > > >  				struct intel_dpll_hw_state
> > > > *pll_state)
> > > > @@ -2503,7 +2521,9 @@ static bool icl_calc_dpll_state(struct
> > > > intel_crtc_state *crtc_state,
> > > >  	struct skl_wrpll_params pll_params = { 0 };
> > > >  	bool ret;
> > > >  
> > > > -	if (intel_crtc_has_type(crtc_state,
> > > > INTEL_OUTPUT_HDMI))
> > > > +	if (intel_port_is_tc(dev_priv, encoder->port))
> > > > +		ret = icl_calc_tbt_pll(dev_priv, clock,
> > > > &pll_params);
> > > > +	else if (intel_crtc_has_type(crtc_state,
> > > > INTEL_OUTPUT_HDMI))
> > > >  		ret = cnl_ddi_calculate_wrpll(clock, dev_priv,
> > > > &pll_params);
> > > >  	else
> > > >  		ret = icl_calc_dp_combo_pll(dev_priv, clock,
> > > > &pll_params);
> > > > -- 
> > > > 2.14.4
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/8] drm/i915/icl: compute the TBT PLL registers

[Rodrigo Vivi]

On Mon, Jul 16, 2018 at 04:05:52PM -0700, Paulo Zanoni wrote:
> Em Seg, 2018-07-16 às 15:47 -0700, Rodrigo Vivi escreveu:
> > On Fri, Jul 13, 2018 at 03:57:45PM -0700, Paulo Zanoni wrote:
> > > Em Sex, 2018-07-13 às 14:08 -0700, Rodrigo Vivi escreveu:
> > > > On Wed, Jul 11, 2018 at 02:59:02PM -0700, Paulo Zanoni wrote:
> > > > > Use the hardcoded tables provided by our spec.
> > > > > 
> > > > > v2:
> > > > >   - SSC stays disabled.
> > > > >   - Use intel_port_is_tc().
> > > > > 
> > > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 22
> > > > > +++++++++++++++++++++-
> > > > >  1 file changed, 21 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > index b51ad2917dbe..7e5e6eb5dfe2 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > @@ -2452,6 +2452,16 @@ static const struct skl_wrpll_params
> > > > > icl_dp_combo_pll_19_2MHz_values[] = {
> > > > >  	  .pdiv = 0x1 /* 2 */, .kdiv = 1, .qdiv_mode = 0,
> > > > > .qdiv_ratio = 0},
> > > > >  };
> > > > >  
> > > > > +static const struct skl_wrpll_params icl_tbt_pll_24MHz_values
> > > > > = {
> > > > > +	.dco_integer = 0x151, .dco_fraction = 0x4000,
> > > > > +	.pdiv = 0x4 /* 5 */, .kdiv = 1, .qdiv_mode = 0,
> > > > > .qdiv_ratio = 0,
> > > > > +};
> > > > > +
> > > > > +static const struct skl_wrpll_params
> > > > > icl_tbt_pll_19_2MHz_values =
> > > > > {
> > > > > +	.dco_integer = 0x1A5, .dco_fraction = 0x7000,
> > > > > +	.pdiv = 0x4 /* 5 */, .kdiv = 1, .qdiv_mode = 0,
> > > > > .qdiv_ratio = 0,
> > > > 
> > > > in other words, in a cleaner view:
> > > > 
> > > > s/qdiv_ratio = 0/qdiv_ratio = 1/g #both tables
> > > 
> > > From the qdiv ratio bit:
> > > 
> > > "This field specifies the Q divider ratio. This field is only used
> > > when
> > > Qdiv Mode is set to Enable to get a divider value other than 1.".
> > > 
> > > So having 0 or 1 shouldn't matter since qdiv_mode is zero.
> > > 
> > > On the other hand, if you look at the Combo PLL table, when
> > > qdiv_mode=0
> > > it explicitly tells us to use qdiv_ratio=0, opposite to what you're
> > > asking.
> > > 
> > > Then the example below and also the DSI example later both keep
> > > saying
> > > to use qdiv_mode=0 + qdiv_ratio=0.
> > > 
> > > Now the TBT PLL example later asks for qdiv_mode=0 qdiv_ratio=1,
> > > which
> > > is what you're asking here, but it's the only the only thing that
> > > asks
> > > for that pattern in that way, which makes me believe that either
> > > it's
> > > wrong (unlikely) or it simply doesn't matter (as written in the bit
> > > definition).
> > 
> > (just adding a note about what we already discussed in pvt last
> > friday)
> > 
> > On the TBL section there are 3 places they add qdiv = 1, so spec is
> > clearly
> > non-sense because at same time it "indicates" that for qmode=0 qdiv
> > is always 1
> > it asks driver to set, because qmode for TBL is always 0 anyways.
> > 
> > So in my view or bspec is just completely wrong or it is stating that
> > it is
> > driver's responsibility to set it to 1.
> > 
> > Anyways since you already filed the bug against the bspec we could
> > just move
> > to either way for now... and fix later...
> > 
> > However one thing still bugs me... the mismatch of code and spec
> > table. Even
> > if spec tells it doesn't matter but they don't update the value 
> > there.
> 
> As I explained in person, I don't think there is a mismatch of code and
> table.

ok, so this is the part that we disagree mostly ;)
I look to spec and there is qdiv = 1 I look to the code and I see qdiv = 0
for my average developer brain this is a mismatch.

> When the spec says "qdiv" it means the high-level qdiv value,
> which is set through the combination of qdiv_mode and qdiv_ratio.
> 
> So the TBT table says "qdiv=1", which you can achieve by setting
> qdiv_mode=0 qdiv_ratio=whatever. 
> 
> The Combo table is more explicit that it lists both qdiv_mode and
> qdiv_ratio to be set, and it also lists the real "qdiv" as a number in
> parens on the qdiv_ratio column.

I know, but I shouldn't have to look to other table than the tbl one to
check tbt code and arrive to conclusions...  Actually you pointed me that
table at first hand because I was looking to the Combo one instead of the TBT :P

> 
> The only thing that really "mismatches" is that in the examples, when
> qdiv is 1, in three places they say we need qdiv_mode=0 qdiv_ratio=0

on Combo section... on TBT section itself they all mention qdiv=1.

> and in the TBT example they say to use qdiv_mode=0 and qdiv_ratio=1.
> But that perceived "mismatch" shouldn't matter since when qdiv_mode=0
> qdiv_ratio can be anything. Still inconsistent, but IMHO irrelevant.
> 
> > 
> > I can see more devs in the future getting confused because qdiv=1 is
> > part of
> > spec's table and code's table mismatch setting it to 0.
> 
> I think your confusion is that you don't see qdiv as a thing that is
> achieved through qdiv_mode and qdiv_ratio.
> 
> > 
> > > 
> > > > 
> > > > with that:
> > > > 
> > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > 
> > > > > +};
> > > > > +
> > > > >  static bool icl_calc_dp_combo_pll(struct drm_i915_private
> > > > > *dev_priv, int clock,
> > > > >  				  struct skl_wrpll_params
> > > > > *pll_params)
> > > > >  {
> > > > > @@ -2494,6 +2504,14 @@ static bool icl_calc_dp_combo_pll(struct
> > > > > drm_i915_private *dev_priv, int clock,
> > > > >  	return true;
> > > > >  }
> > > > >  
> > > > > +static bool icl_calc_tbt_pll(struct drm_i915_private
> > > > > *dev_priv,
> > > > > int clock,
> > > > > +			     struct skl_wrpll_params
> > > > > *pll_params)
> > > > > +{
> > > > > +	*pll_params = dev_priv->cdclk.hw.ref == 24000 ?
> > > > > +			icl_tbt_pll_24MHz_values :
> > > > > icl_tbt_pll_19_2MHz_values;
> > > > > +	return true;
> > > > > +}
> > > > > +
> > > > >  static bool icl_calc_dpll_state(struct intel_crtc_state
> > > > > *crtc_state,
> > > > >  				struct intel_encoder *encoder,
> > > > > int
> > > > > clock,
> > > > >  				struct intel_dpll_hw_state
> > > > > *pll_state)
> > > > > @@ -2503,7 +2521,9 @@ static bool icl_calc_dpll_state(struct
> > > > > intel_crtc_state *crtc_state,
> > > > >  	struct skl_wrpll_params pll_params = { 0 };
> > > > >  	bool ret;
> > > > >  
> > > > > -	if (intel_crtc_has_type(crtc_state,
> > > > > INTEL_OUTPUT_HDMI))
> > > > > +	if (intel_port_is_tc(dev_priv, encoder->port))
> > > > > +		ret = icl_calc_tbt_pll(dev_priv, clock,
> > > > > &pll_params);
> > > > > +	else if (intel_crtc_has_type(crtc_state,
> > > > > INTEL_OUTPUT_HDMI))
> > > > >  		ret = cnl_ddi_calculate_wrpll(clock, dev_priv,
> > > > > &pll_params);
> > > > >  	else
> > > > >  		ret = icl_calc_dp_combo_pll(dev_priv, clock,
> > > > > &pll_params);
> > > > > -- 
> > > > > 2.14.4
> > > > > 
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/8] drm/i915/icl: compute the TBT PLL registers

[Rodrigo Vivi]

On Mon, Jul 16, 2018 at 04:22:15PM -0700, Rodrigo Vivi wrote:
> On Mon, Jul 16, 2018 at 04:05:52PM -0700, Paulo Zanoni wrote:
> > Em Seg, 2018-07-16 às 15:47 -0700, Rodrigo Vivi escreveu:
> > > On Fri, Jul 13, 2018 at 03:57:45PM -0700, Paulo Zanoni wrote:
> > > > Em Sex, 2018-07-13 às 14:08 -0700, Rodrigo Vivi escreveu:
> > > > > On Wed, Jul 11, 2018 at 02:59:02PM -0700, Paulo Zanoni wrote:
> > > > > > Use the hardcoded tables provided by our spec.
> > > > > > 
> > > > > > v2:
> > > > > >   - SSC stays disabled.
> > > > > >   - Use intel_port_is_tc().
> > > > > > 
> > > > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 22
> > > > > > +++++++++++++++++++++-
> > > > > >  1 file changed, 21 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > > index b51ad2917dbe..7e5e6eb5dfe2 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > > @@ -2452,6 +2452,16 @@ static const struct skl_wrpll_params
> > > > > > icl_dp_combo_pll_19_2MHz_values[] = {
> > > > > >  	  .pdiv = 0x1 /* 2 */, .kdiv = 1, .qdiv_mode = 0,
> > > > > > .qdiv_ratio = 0},
> > > > > >  };
> > > > > >  
> > > > > > +static const struct skl_wrpll_params icl_tbt_pll_24MHz_values
> > > > > > = {
> > > > > > +	.dco_integer = 0x151, .dco_fraction = 0x4000,
> > > > > > +	.pdiv = 0x4 /* 5 */, .kdiv = 1, .qdiv_mode = 0,
> > > > > > .qdiv_ratio = 0,
> > > > > > +};
> > > > > > +
> > > > > > +static const struct skl_wrpll_params
> > > > > > icl_tbt_pll_19_2MHz_values =
> > > > > > {
> > > > > > +	.dco_integer = 0x1A5, .dco_fraction = 0x7000,
> > > > > > +	.pdiv = 0x4 /* 5 */, .kdiv = 1, .qdiv_mode = 0,
> > > > > > .qdiv_ratio = 0,
> > > > > 
> > > > > in other words, in a cleaner view:
> > > > > 
> > > > > s/qdiv_ratio = 0/qdiv_ratio = 1/g #both tables
> > > > 
> > > > From the qdiv ratio bit:
> > > > 
> > > > "This field specifies the Q divider ratio. This field is only used
> > > > when
> > > > Qdiv Mode is set to Enable to get a divider value other than 1.".
> > > > 
> > > > So having 0 or 1 shouldn't matter since qdiv_mode is zero.
> > > > 
> > > > On the other hand, if you look at the Combo PLL table, when
> > > > qdiv_mode=0
> > > > it explicitly tells us to use qdiv_ratio=0, opposite to what you're
> > > > asking.
> > > > 
> > > > Then the example below and also the DSI example later both keep
> > > > saying
> > > > to use qdiv_mode=0 + qdiv_ratio=0.
> > > > 
> > > > Now the TBT PLL example later asks for qdiv_mode=0 qdiv_ratio=1,
> > > > which
> > > > is what you're asking here, but it's the only the only thing that
> > > > asks
> > > > for that pattern in that way, which makes me believe that either
> > > > it's
> > > > wrong (unlikely) or it simply doesn't matter (as written in the bit
> > > > definition).
> > > 
> > > (just adding a note about what we already discussed in pvt last
> > > friday)
> > > 
> > > On the TBL section there are 3 places they add qdiv = 1, so spec is
> > > clearly
> > > non-sense because at same time it "indicates" that for qmode=0 qdiv
> > > is always 1
> > > it asks driver to set, because qmode for TBL is always 0 anyways.
> > > 
> > > So in my view or bspec is just completely wrong or it is stating that
> > > it is
> > > driver's responsibility to set it to 1.
> > > 
> > > Anyways since you already filed the bug against the bspec we could
> > > just move
> > > to either way for now... and fix later...
> > > 
> > > However one thing still bugs me... the mismatch of code and spec
> > > table. Even
> > > if spec tells it doesn't matter but they don't update the value 
> > > there.
> > 
> > As I explained in person, I don't think there is a mismatch of code and
> > table.
> 
> ok, so this is the part that we disagree mostly ;)
> I look to spec and there is qdiv = 1 I look to the code and I see qdiv = 0
> for my average developer brain this is a mismatch.

No disagreements anymore. Spec was updated so code matches that now.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Thanks for filing the spec bug.

> 
> > When the spec says "qdiv" it means the high-level qdiv value,
> > which is set through the combination of qdiv_mode and qdiv_ratio.
> > 
> > So the TBT table says "qdiv=1", which you can achieve by setting
> > qdiv_mode=0 qdiv_ratio=whatever. 
> > 
> > The Combo table is more explicit that it lists both qdiv_mode and
> > qdiv_ratio to be set, and it also lists the real "qdiv" as a number in
> > parens on the qdiv_ratio column.
> 
> I know, but I shouldn't have to look to other table than the tbl one to
> check tbt code and arrive to conclusions...  Actually you pointed me that
> table at first hand because I was looking to the Combo one instead of the TBT :P
> 
> > 
> > The only thing that really "mismatches" is that in the examples, when
> > qdiv is 1, in three places they say we need qdiv_mode=0 qdiv_ratio=0
> 
> on Combo section... on TBT section itself they all mention qdiv=1.
> 
> > and in the TBT example they say to use qdiv_mode=0 and qdiv_ratio=1.
> > But that perceived "mismatch" shouldn't matter since when qdiv_mode=0
> > qdiv_ratio can be anything. Still inconsistent, but IMHO irrelevant.
> > 
> > > 
> > > I can see more devs in the future getting confused because qdiv=1 is
> > > part of
> > > spec's table and code's table mismatch setting it to 0.
> > 
> > I think your confusion is that you don't see qdiv as a thing that is
> > achieved through qdiv_mode and qdiv_ratio.
> > 
> > > 
> > > > 
> > > > > 
> > > > > with that:
> > > > > 
> > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > 
> > > > > > +};
> > > > > > +
> > > > > >  static bool icl_calc_dp_combo_pll(struct drm_i915_private
> > > > > > *dev_priv, int clock,
> > > > > >  				  struct skl_wrpll_params
> > > > > > *pll_params)
> > > > > >  {
> > > > > > @@ -2494,6 +2504,14 @@ static bool icl_calc_dp_combo_pll(struct
> > > > > > drm_i915_private *dev_priv, int clock,
> > > > > >  	return true;
> > > > > >  }
> > > > > >  
> > > > > > +static bool icl_calc_tbt_pll(struct drm_i915_private
> > > > > > *dev_priv,
> > > > > > int clock,
> > > > > > +			     struct skl_wrpll_params
> > > > > > *pll_params)
> > > > > > +{
> > > > > > +	*pll_params = dev_priv->cdclk.hw.ref == 24000 ?
> > > > > > +			icl_tbt_pll_24MHz_values :
> > > > > > icl_tbt_pll_19_2MHz_values;
> > > > > > +	return true;
> > > > > > +}
> > > > > > +
> > > > > >  static bool icl_calc_dpll_state(struct intel_crtc_state
> > > > > > *crtc_state,
> > > > > >  				struct intel_encoder *encoder,
> > > > > > int
> > > > > > clock,
> > > > > >  				struct intel_dpll_hw_state
> > > > > > *pll_state)
> > > > > > @@ -2503,7 +2521,9 @@ static bool icl_calc_dpll_state(struct
> > > > > > intel_crtc_state *crtc_state,
> > > > > >  	struct skl_wrpll_params pll_params = { 0 };
> > > > > >  	bool ret;
> > > > > >  
> > > > > > -	if (intel_crtc_has_type(crtc_state,
> > > > > > INTEL_OUTPUT_HDMI))
> > > > > > +	if (intel_port_is_tc(dev_priv, encoder->port))
> > > > > > +		ret = icl_calc_tbt_pll(dev_priv, clock,
> > > > > > &pll_params);
> > > > > > +	else if (intel_crtc_has_type(crtc_state,
> > > > > > INTEL_OUTPUT_HDMI))
> > > > > >  		ret = cnl_ddi_calculate_wrpll(clock, dev_priv,
> > > > > > &pll_params);
> > > > > >  	else
> > > > > >  		ret = icl_calc_dp_combo_pll(dev_priv, clock,
> > > > > > &pll_params);
> > > > > > -- 
> > > > > > 2.14.4
> > > > > > 
> > > > > > _______________________________________________
> > > > > > Intel-gfx mailing list
> > > > > > Intel-gfx@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > > 
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/8] drm/i915/icl: implement icl_digital_port_connected()

[Paulo Zanoni]

Do like the other functions and check for the ISR bits. We have plans
to add a few more checks in this code in the next patches, that's why
it's a little more verbose than it could be.

v2: Rebase.

Cc: Animesh Manna <animesh.manna@intel.com>
Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> (v1)
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h |  4 +++
 drivers/gpu/drm/i915/intel_dp.c | 57 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index b95bab7a3d24..c1f350469ff6 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7198,6 +7198,7 @@ enum {
 #define  GEN11_TC3_HOTPLUG			(1 << 18)
 #define  GEN11_TC2_HOTPLUG			(1 << 17)
 #define  GEN11_TC1_HOTPLUG			(1 << 16)
+#define  GEN11_TC_HOTPLUG(tc_port)		(1 << ((tc_port) + 16))
 #define  GEN11_DE_TC_HOTPLUG_MASK		(GEN11_TC4_HOTPLUG | \
 						 GEN11_TC3_HOTPLUG | \
 						 GEN11_TC2_HOTPLUG | \
@@ -7206,6 +7207,7 @@ enum {
 #define  GEN11_TBT3_HOTPLUG			(1 << 2)
 #define  GEN11_TBT2_HOTPLUG			(1 << 1)
 #define  GEN11_TBT1_HOTPLUG			(1 << 0)
+#define  GEN11_TBT_HOTPLUG(tc_port)		(1 << (tc_port))
 #define  GEN11_DE_TBT_HOTPLUG_MASK		(GEN11_TBT4_HOTPLUG | \
 						 GEN11_TBT3_HOTPLUG | \
 						 GEN11_TBT2_HOTPLUG | \
@@ -7578,6 +7580,8 @@ enum {
 #define SDE_GMBUS_ICP			(1 << 23)
 #define SDE_DDIB_HOTPLUG_ICP		(1 << 17)
 #define SDE_DDIA_HOTPLUG_ICP		(1 << 16)
+#define SDE_TC_HOTPLUG_ICP(tc_port)	(1 << ((tc_port) + 24))
+#define SDE_DDI_HOTPLUG_ICP(port)	(1 << ((port) + 16))
 #define SDE_DDI_MASK_ICP		(SDE_DDIB_HOTPLUG_ICP |	\
 					 SDE_DDIA_HOTPLUG_ICP)
 #define SDE_TC_MASK_ICP			(SDE_TC4_HOTPLUG_ICP |	\
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 5be07e1d816d..f2197dea02d0 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4744,6 +4744,61 @@ static bool bxt_digital_port_connected(struct intel_encoder *encoder)
 	return I915_READ(GEN8_DE_PORT_ISR) & bit;
 }
 
+static bool icl_combo_port_connected(struct drm_i915_private *dev_priv,
+				     struct intel_digital_port *intel_dig_port)
+{
+	enum port port = intel_dig_port->base.port;
+
+	return I915_READ(SDEISR) & SDE_DDI_HOTPLUG_ICP(port);
+}
+
+static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
+				  struct intel_digital_port *intel_dig_port)
+{
+	enum port port = intel_dig_port->base.port;
+	enum tc_port tc_port = intel_port_to_tc(dev_priv, port);
+	u32 legacy_bit = SDE_TC_HOTPLUG_ICP(tc_port);
+	u32 typec_bit = GEN11_TC_HOTPLUG(tc_port);
+	u32 tbt_bit = GEN11_TBT_HOTPLUG(tc_port);
+	bool is_legacy = false, is_typec = false, is_tbt = false;
+	u32 cpu_isr;
+
+	if (I915_READ(SDEISR) & legacy_bit)
+		is_legacy = true;
+
+	cpu_isr = I915_READ(GEN11_DE_HPD_ISR);
+	if (cpu_isr & typec_bit)
+		is_typec = true;
+	if (cpu_isr & tbt_bit)
+		is_tbt = true;
+
+	WARN_ON(is_legacy + is_typec + is_tbt > 1);
+	if (!is_legacy && !is_typec && !is_tbt)
+		return false;
+
+	return true;
+}
+
+static bool icl_digital_port_connected(struct intel_encoder *encoder)
+{
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
+
+	switch (encoder->hpd_pin) {
+	case HPD_PORT_A:
+	case HPD_PORT_B:
+		return icl_combo_port_connected(dev_priv, dig_port);
+	case HPD_PORT_C:
+	case HPD_PORT_D:
+	case HPD_PORT_E:
+	case HPD_PORT_F:
+		return icl_tc_port_connected(dev_priv, dig_port);
+	default:
+		MISSING_CASE(encoder->hpd_pin);
+		return false;
+	}
+}
+
 /*
  * intel_digital_port_connected - is the specified port connected?
  * @encoder: intel_encoder
@@ -4771,6 +4826,8 @@ bool intel_digital_port_connected(struct intel_encoder *encoder)
 		return bdw_digital_port_connected(encoder);
 	else if (IS_GEN9_LP(dev_priv))
 		return bxt_digital_port_connected(encoder);
+	else if (IS_ICELAKE(dev_priv))
+		return icl_digital_port_connected(encoder);
 	else
 		return spt_digital_port_connected(encoder);
 }
-- 
2.14.4

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

Re: [PATCH 2/8] drm/i915/icl: implement icl_digital_port_connected()

[Rodrigo Vivi]

On Wed, Jul 11, 2018 at 02:59:03PM -0700, Paulo Zanoni wrote:
> Do like the other functions and check for the ISR bits. We have plans
> to add a few more checks in this code in the next patches, that's why
> it's a little more verbose than it could be.
> 
> v2: Rebase.
> 
> Cc: Animesh Manna <animesh.manna@intel.com>
> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> (v1)
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h |  4 +++
>  drivers/gpu/drm/i915/intel_dp.c | 57 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 61 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b95bab7a3d24..c1f350469ff6 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7198,6 +7198,7 @@ enum {
>  #define  GEN11_TC3_HOTPLUG			(1 << 18)
>  #define  GEN11_TC2_HOTPLUG			(1 << 17)
>  #define  GEN11_TC1_HOTPLUG			(1 << 16)
> +#define  GEN11_TC_HOTPLUG(tc_port)		(1 << ((tc_port) + 16))
>  #define  GEN11_DE_TC_HOTPLUG_MASK		(GEN11_TC4_HOTPLUG | \
>  						 GEN11_TC3_HOTPLUG | \
>  						 GEN11_TC2_HOTPLUG | \
> @@ -7206,6 +7207,7 @@ enum {
>  #define  GEN11_TBT3_HOTPLUG			(1 << 2)
>  #define  GEN11_TBT2_HOTPLUG			(1 << 1)
>  #define  GEN11_TBT1_HOTPLUG			(1 << 0)
> +#define  GEN11_TBT_HOTPLUG(tc_port)		(1 << (tc_port))
>  #define  GEN11_DE_TBT_HOTPLUG_MASK		(GEN11_TBT4_HOTPLUG | \
>  						 GEN11_TBT3_HOTPLUG | \
>  						 GEN11_TBT2_HOTPLUG | \
> @@ -7578,6 +7580,8 @@ enum {
>  #define SDE_GMBUS_ICP			(1 << 23)
>  #define SDE_DDIB_HOTPLUG_ICP		(1 << 17)
>  #define SDE_DDIA_HOTPLUG_ICP		(1 << 16)
> +#define SDE_TC_HOTPLUG_ICP(tc_port)	(1 << ((tc_port) + 24))
> +#define SDE_DDI_HOTPLUG_ICP(port)	(1 << ((port) + 16))
>  #define SDE_DDI_MASK_ICP		(SDE_DDIB_HOTPLUG_ICP |	\
>  					 SDE_DDIA_HOTPLUG_ICP)
>  #define SDE_TC_MASK_ICP			(SDE_TC4_HOTPLUG_ICP |	\
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 5be07e1d816d..f2197dea02d0 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4744,6 +4744,61 @@ static bool bxt_digital_port_connected(struct intel_encoder *encoder)
>  	return I915_READ(GEN8_DE_PORT_ISR) & bit;
>  }
>  
> +static bool icl_combo_port_connected(struct drm_i915_private *dev_priv,
> +				     struct intel_digital_port *intel_dig_port)
> +{
> +	enum port port = intel_dig_port->base.port;
> +
> +	return I915_READ(SDEISR) & SDE_DDI_HOTPLUG_ICP(port);
> +}
> +
> +static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
> +				  struct intel_digital_port *intel_dig_port)
> +{
> +	enum port port = intel_dig_port->base.port;
> +	enum tc_port tc_port = intel_port_to_tc(dev_priv, port);
> +	u32 legacy_bit = SDE_TC_HOTPLUG_ICP(tc_port);
> +	u32 typec_bit = GEN11_TC_HOTPLUG(tc_port);
> +	u32 tbt_bit = GEN11_TBT_HOTPLUG(tc_port);
> +	bool is_legacy = false, is_typec = false, is_tbt = false;
> +	u32 cpu_isr;
> +
> +	if (I915_READ(SDEISR) & legacy_bit)
> +		is_legacy = true;
> +
> +	cpu_isr = I915_READ(GEN11_DE_HPD_ISR);
> +	if (cpu_isr & typec_bit)
> +		is_typec = true;
> +	if (cpu_isr & tbt_bit)
> +		is_tbt = true;
> +
> +	WARN_ON(is_legacy + is_typec + is_tbt > 1);
> +	if (!is_legacy && !is_typec && !is_tbt)
> +		return false;

what about something like below?

+{
+	enum port port = intel_dig_port->base.port;
+	enum tc_port tc_port = intel_port_to_tc(dev_priv, port);
+	bool is_legacy = false, is_typec = false, is_tbt = false;
+	u32 cpu_isr;
+
+	is_legacy = I915_READ(SDEISR) & legacy_bit;
+
+	cpu_isr = I915_READ(GEN11_DE_HPD_ISR);
+	is_typec = cpu_isr & typec_bit;
+	is_tbt = cpu_isr & tbt_bit;
+
+	WARN_ON(is_legacy && (is_typec || is_tbt));
+	return (is_legacy || is_typec || is_tbt);
+}

> +
> +static bool icl_digital_port_connected(struct intel_encoder *encoder)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
> +
> +	switch (encoder->hpd_pin) {
> +	case HPD_PORT_A:
> +	case HPD_PORT_B:
> +		return icl_combo_port_connected(dev_priv, dig_port);
> +	case HPD_PORT_C:
> +	case HPD_PORT_D:
> +	case HPD_PORT_E:
> +	case HPD_PORT_F:
> +		return icl_tc_port_connected(dev_priv, dig_port);
> +	default:
> +		MISSING_CASE(encoder->hpd_pin);
> +		return false;
> +	}
> +}
> +
>  /*
>   * intel_digital_port_connected - is the specified port connected?
>   * @encoder: intel_encoder
> @@ -4771,6 +4826,8 @@ bool intel_digital_port_connected(struct intel_encoder *encoder)
>  		return bdw_digital_port_connected(encoder);
>  	else if (IS_GEN9_LP(dev_priv))
>  		return bxt_digital_port_connected(encoder);
> +	else if (IS_ICELAKE(dev_priv))
> +		return icl_digital_port_connected(encoder);

this is messing the order and getting hard to read.
probably better to invert the order in a separated patch and then
put ICL on top.

>  	else
>  		return spt_digital_port_connected(encoder);
>  }
> -- 
> 2.14.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/8] drm/i915/icl: store the port type for TC ports

[Paulo Zanoni]

The type is detected based on the interrupt ISR bit. Once detected,
it's not supposed to be changed, so we have some sanity checks for
that.

Cc: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_display.h |  7 +++++++
 drivers/gpu/drm/i915/intel_dp.c      | 36 +++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 3 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
index ca5a10f3400d..18ed9835335c 100644
--- a/drivers/gpu/drm/i915/intel_display.h
+++ b/drivers/gpu/drm/i915/intel_display.h
@@ -137,6 +137,13 @@ enum tc_port {
 	I915_MAX_TC_PORTS
 };
 
+enum tc_port_type {
+	TC_PORT_UNKNOWN = 0,
+	TC_PORT_TYPEC,
+	TC_PORT_TBT,
+	TC_PORT_LEGACY,
+};
+
 enum dpio_channel {
 	DPIO_CH0,
 	DPIO_CH1
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f2197dea02d0..486b879cdef7 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4752,6 +4752,38 @@ static bool icl_combo_port_connected(struct drm_i915_private *dev_priv,
 	return I915_READ(SDEISR) & SDE_DDI_HOTPLUG_ICP(port);
 }
 
+static void icl_update_tc_port_type(struct drm_i915_private *dev_priv,
+				    struct intel_digital_port *intel_dig_port,
+				    bool is_legacy, bool is_typec, bool is_tbt)
+{
+	enum port port = intel_dig_port->base.port;
+	enum tc_port_type old_type = intel_dig_port->tc_type;
+	const char *type_str;
+
+	WARN_ON(is_legacy + is_typec + is_tbt != 1);
+
+	if (is_legacy) {
+		intel_dig_port->tc_type = TC_PORT_LEGACY;
+		type_str = "legacy";
+	} else if (is_typec) {
+		intel_dig_port->tc_type = TC_PORT_TYPEC;
+		type_str = "typec";
+	} else if (is_tbt) {
+		intel_dig_port->tc_type = TC_PORT_TBT;
+		type_str = "tbt";
+	} else {
+		return;
+	}
+
+	/* Types are not supposed to be changed at runtime. */
+	WARN_ON(old_type != TC_PORT_UNKNOWN &&
+		old_type != intel_dig_port->tc_type);
+
+	if (old_type != intel_dig_port->tc_type)
+		DRM_DEBUG_KMS("Port %c has TC type %s\n", port_name(port),
+			      type_str);
+}
+
 static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
 				  struct intel_digital_port *intel_dig_port)
 {
@@ -4772,10 +4804,12 @@ static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
 	if (cpu_isr & tbt_bit)
 		is_tbt = true;
 
-	WARN_ON(is_legacy + is_typec + is_tbt > 1);
 	if (!is_legacy && !is_typec && !is_tbt)
 		return false;
 
+	icl_update_tc_port_type(dev_priv, intel_dig_port, is_legacy, is_typec,
+				is_tbt);
+
 	return true;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 61e715ddd0d5..8b3c3dc76810 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1162,6 +1162,7 @@ struct intel_digital_port {
 	bool release_cl2_override;
 	uint8_t max_lanes;
 	enum intel_display_power_domain ddi_io_power_domain;
+	enum tc_port_type tc_type;
 
 	void (*write_infoframe)(struct drm_encoder *encoder,
 				const struct intel_crtc_state *crtc_state,
-- 
2.14.4

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

Re: [PATCH 3/8] drm/i915/icl: store the port type for TC ports

[Rodrigo Vivi]

On Wed, Jul 11, 2018 at 02:59:04PM -0700, Paulo Zanoni wrote:
> The type is detected based on the interrupt ISR bit. Once detected,
> it's not supposed to be changed, so we have some sanity checks for
> that.
> 
> Cc: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.h |  7 +++++++
>  drivers/gpu/drm/i915/intel_dp.c      | 36 +++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  3 files changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> index ca5a10f3400d..18ed9835335c 100644
> --- a/drivers/gpu/drm/i915/intel_display.h
> +++ b/drivers/gpu/drm/i915/intel_display.h
> @@ -137,6 +137,13 @@ enum tc_port {
>  	I915_MAX_TC_PORTS
>  };
>  
> +enum tc_port_type {
> +	TC_PORT_UNKNOWN = 0,
> +	TC_PORT_TYPEC,
> +	TC_PORT_TBT,
> +	TC_PORT_LEGACY,
> +};
> +
>  enum dpio_channel {
>  	DPIO_CH0,
>  	DPIO_CH1
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f2197dea02d0..486b879cdef7 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4752,6 +4752,38 @@ static bool icl_combo_port_connected(struct drm_i915_private *dev_priv,
>  	return I915_READ(SDEISR) & SDE_DDI_HOTPLUG_ICP(port);
>  }
>  
> +static void icl_update_tc_port_type(struct drm_i915_private *dev_priv,
> +				    struct intel_digital_port *intel_dig_port,
> +				    bool is_legacy, bool is_typec, bool is_tbt)
> +{
> +	enum port port = intel_dig_port->base.port;
> +	enum tc_port_type old_type = intel_dig_port->tc_type;
> +	const char *type_str;
> +
> +	WARN_ON(is_legacy + is_typec + is_tbt != 1);
> +
> +	if (is_legacy) {
> +		intel_dig_port->tc_type = TC_PORT_LEGACY;
> +		type_str = "legacy";
> +	} else if (is_typec) {
> +		intel_dig_port->tc_type = TC_PORT_TYPEC;
> +		type_str = "typec";
> +	} else if (is_tbt) {
> +		intel_dig_port->tc_type = TC_PORT_TBT;
> +		type_str = "tbt";
> +	} else {
> +		return;
> +	}
> +
> +	/* Types are not supposed to be changed at runtime. */
> +	WARN_ON(old_type != TC_PORT_UNKNOWN &&
> +		old_type != intel_dig_port->tc_type);
> +
> +	if (old_type != intel_dig_port->tc_type)
> +		DRM_DEBUG_KMS("Port %c has TC type %s\n", port_name(port),
> +			      type_str);
> +}
> +
>  static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
>  				  struct intel_digital_port *intel_dig_port)
>  {
> @@ -4772,10 +4804,12 @@ static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
>  	if (cpu_isr & tbt_bit)
>  		is_tbt = true;
>  
> -	WARN_ON(is_legacy + is_typec + is_tbt > 1);

oh, I know see that I got it wrong on my previous suggestion, but since
we are removing maybe it is better to not add at all at first place?!

>  	if (!is_legacy && !is_typec && !is_tbt)
>  		return false;
>  
> +	icl_update_tc_port_type(dev_priv, intel_dig_port, is_legacy, is_typec,
> +				is_tbt);

for a while I thougth this was the start of the chain that I didn't like,
but I was wrong, the type I believe it can/need to be here indeed.

but for everything else I believe that you need to handle on long_pulse.

if it is connected and tc_type != known than you do all the rest
of the opperation instead of making a huge chain from this point.

(for the lspcon wa path I don't believe we need that at all, but if we
need we should also handle there)

For this patch:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> +
>  	return true;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 61e715ddd0d5..8b3c3dc76810 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1162,6 +1162,7 @@ struct intel_digital_port {
>  	bool release_cl2_override;
>  	uint8_t max_lanes;
>  	enum intel_display_power_domain ddi_io_power_domain;
> +	enum tc_port_type tc_type;
>  
>  	void (*write_infoframe)(struct drm_encoder *encoder,
>  				const struct intel_crtc_state *crtc_state,
> -- 
> 2.14.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/8] drm/i915/icl: store the port type for TC ports

[Paulo Zanoni]

Em Qui, 2018-07-12 às 23:14 -0700, Rodrigo Vivi escreveu:
> On Wed, Jul 11, 2018 at 02:59:04PM -0700, Paulo Zanoni wrote:
> > The type is detected based on the interrupt ISR bit. Once detected,
> > it's not supposed to be changed, so we have some sanity checks for
> > that.
> > 
> > Cc: Animesh Manna <animesh.manna@intel.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.h |  7 +++++++
> >  drivers/gpu/drm/i915/intel_dp.c      | 36
> > +++++++++++++++++++++++++++++++++++-
> >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> >  3 files changed, 43 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.h
> > b/drivers/gpu/drm/i915/intel_display.h
> > index ca5a10f3400d..18ed9835335c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.h
> > +++ b/drivers/gpu/drm/i915/intel_display.h
> > @@ -137,6 +137,13 @@ enum tc_port {
> >  	I915_MAX_TC_PORTS
> >  };
> >  
> > +enum tc_port_type {
> > +	TC_PORT_UNKNOWN = 0,
> > +	TC_PORT_TYPEC,
> > +	TC_PORT_TBT,
> > +	TC_PORT_LEGACY,
> > +};
> > +
> >  enum dpio_channel {
> >  	DPIO_CH0,
> >  	DPIO_CH1
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index f2197dea02d0..486b879cdef7 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4752,6 +4752,38 @@ static bool icl_combo_port_connected(struct
> > drm_i915_private *dev_priv,
> >  	return I915_READ(SDEISR) & SDE_DDI_HOTPLUG_ICP(port);
> >  }
> >  
> > +static void icl_update_tc_port_type(struct drm_i915_private
> > *dev_priv,
> > +				    struct intel_digital_port
> > *intel_dig_port,
> > +				    bool is_legacy, bool is_typec,
> > bool is_tbt)
> > +{
> > +	enum port port = intel_dig_port->base.port;
> > +	enum tc_port_type old_type = intel_dig_port->tc_type;
> > +	const char *type_str;
> > +
> > +	WARN_ON(is_legacy + is_typec + is_tbt != 1);
> > +
> > +	if (is_legacy) {
> > +		intel_dig_port->tc_type = TC_PORT_LEGACY;
> > +		type_str = "legacy";
> > +	} else if (is_typec) {
> > +		intel_dig_port->tc_type = TC_PORT_TYPEC;
> > +		type_str = "typec";
> > +	} else if (is_tbt) {
> > +		intel_dig_port->tc_type = TC_PORT_TBT;
> > +		type_str = "tbt";
> > +	} else {
> > +		return;
> > +	}
> > +
> > +	/* Types are not supposed to be changed at runtime. */
> > +	WARN_ON(old_type != TC_PORT_UNKNOWN &&
> > +		old_type != intel_dig_port->tc_type);
> > +
> > +	if (old_type != intel_dig_port->tc_type)
> > +		DRM_DEBUG_KMS("Port %c has TC type %s\n",
> > port_name(port),
> > +			      type_str);
> > +}
> > +
> >  static bool icl_tc_port_connected(struct drm_i915_private
> > *dev_priv,
> >  				  struct intel_digital_port
> > *intel_dig_port)
> >  {
> > @@ -4772,10 +4804,12 @@ static bool icl_tc_port_connected(struct
> > drm_i915_private *dev_priv,
> >  	if (cpu_isr & tbt_bit)
> >  		is_tbt = true;
> >  
> > -	WARN_ON(is_legacy + is_typec + is_tbt > 1);
> 
> oh, I know see that I got it wrong on my previous suggestion, but
> since
> we are removing maybe it is better to not add at all at first place?!

We are moving it. It was in the correct place in the last patch, it is
in the correct place now.

> 
> >  	if (!is_legacy && !is_typec && !is_tbt)
> >  		return false;
> >  
> > +	icl_update_tc_port_type(dev_priv, intel_dig_port,
> > is_legacy, is_typec,
> > +				is_tbt);
> 
> for a while I thougth this was the start of the chain that I didn't
> like,
> but I was wrong, the type I believe it can/need to be here indeed.
> 
> but for everything else I believe that you need to handle on
> long_pulse.

I really need better explanations instead of "I don't like" and "I feel
this is wrong". I can't even start to think on how to implement an
acceptable solution if you don't try to elaborate a few more problems
on what exactly is wrong with the series, in technical explanations
with reasons instead of feelings.

The way I imagine a solution with code moved out is that every call to
intel_digital_port_connected() will need to have a following call to
the function you're telling me to extract, which doesn't make sense to
me. Or maybe we could wrap the all the call pairs under
intel_digital_port_really_connected() :).

> 
> if it is connected and tc_type != known than you do all the rest
> of the opperation instead of making a huge chain from this point.

What exactly is "huge" about it?

> 
> (for the lspcon wa path I don't believe we need that at all, but if
> we
> need we should also handle there)

Please explain with technical terms the "believe" part.


> 
> For this patch:
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> > +
> >  	return true;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 61e715ddd0d5..8b3c3dc76810 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1162,6 +1162,7 @@ struct intel_digital_port {
> >  	bool release_cl2_override;
> >  	uint8_t max_lanes;
> >  	enum intel_display_power_domain ddi_io_power_domain;
> > +	enum tc_port_type tc_type;
> >  
> >  	void (*write_infoframe)(struct drm_encoder *encoder,
> >  				const struct intel_crtc_state
> > *crtc_state,
> > -- 
> > 2.14.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/8] drm/i915/icl: store the port type for TC ports

[Rodrigo Vivi]

On Mon, Jul 16, 2018 at 03:04:01PM -0700, Paulo Zanoni wrote:
> Em Qui, 2018-07-12 às 23:14 -0700, Rodrigo Vivi escreveu:
> > On Wed, Jul 11, 2018 at 02:59:04PM -0700, Paulo Zanoni wrote:
> > > The type is detected based on the interrupt ISR bit. Once detected,
> > > it's not supposed to be changed, so we have some sanity checks for
> > > that.
> > > 
> > > Cc: Animesh Manna <animesh.manna@intel.com>
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.h |  7 +++++++
> > >  drivers/gpu/drm/i915/intel_dp.c      | 36
> > > +++++++++++++++++++++++++++++++++++-
> > >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> > >  3 files changed, 43 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.h
> > > b/drivers/gpu/drm/i915/intel_display.h
> > > index ca5a10f3400d..18ed9835335c 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.h
> > > +++ b/drivers/gpu/drm/i915/intel_display.h
> > > @@ -137,6 +137,13 @@ enum tc_port {
> > >  	I915_MAX_TC_PORTS
> > >  };
> > >  
> > > +enum tc_port_type {
> > > +	TC_PORT_UNKNOWN = 0,
> > > +	TC_PORT_TYPEC,
> > > +	TC_PORT_TBT,
> > > +	TC_PORT_LEGACY,
> > > +};
> > > +
> > >  enum dpio_channel {
> > >  	DPIO_CH0,
> > >  	DPIO_CH1
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index f2197dea02d0..486b879cdef7 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -4752,6 +4752,38 @@ static bool icl_combo_port_connected(struct
> > > drm_i915_private *dev_priv,
> > >  	return I915_READ(SDEISR) & SDE_DDI_HOTPLUG_ICP(port);
> > >  }
> > >  
> > > +static void icl_update_tc_port_type(struct drm_i915_private
> > > *dev_priv,
> > > +				    struct intel_digital_port
> > > *intel_dig_port,
> > > +				    bool is_legacy, bool is_typec,
> > > bool is_tbt)
> > > +{
> > > +	enum port port = intel_dig_port->base.port;
> > > +	enum tc_port_type old_type = intel_dig_port->tc_type;
> > > +	const char *type_str;
> > > +
> > > +	WARN_ON(is_legacy + is_typec + is_tbt != 1);
> > > +
> > > +	if (is_legacy) {
> > > +		intel_dig_port->tc_type = TC_PORT_LEGACY;
> > > +		type_str = "legacy";
> > > +	} else if (is_typec) {
> > > +		intel_dig_port->tc_type = TC_PORT_TYPEC;
> > > +		type_str = "typec";
> > > +	} else if (is_tbt) {
> > > +		intel_dig_port->tc_type = TC_PORT_TBT;
> > > +		type_str = "tbt";
> > > +	} else {
> > > +		return;
> > > +	}
> > > +
> > > +	/* Types are not supposed to be changed at runtime. */
> > > +	WARN_ON(old_type != TC_PORT_UNKNOWN &&
> > > +		old_type != intel_dig_port->tc_type);
> > > +
> > > +	if (old_type != intel_dig_port->tc_type)
> > > +		DRM_DEBUG_KMS("Port %c has TC type %s\n",
> > > port_name(port),
> > > +			      type_str);
> > > +}
> > > +
> > >  static bool icl_tc_port_connected(struct drm_i915_private
> > > *dev_priv,
> > >  				  struct intel_digital_port
> > > *intel_dig_port)
> > >  {
> > > @@ -4772,10 +4804,12 @@ static bool icl_tc_port_connected(struct
> > > drm_i915_private *dev_priv,
> > >  	if (cpu_isr & tbt_bit)
> > >  		is_tbt = true;
> > >  
> > > -	WARN_ON(is_legacy + is_typec + is_tbt > 1);
> > 
> > oh, I know see that I got it wrong on my previous suggestion, but
> > since
> > we are removing maybe it is better to not add at all at first place?!
> 
> We are moving it. It was in the correct place in the last patch, it is
> in the correct place now.
> 
> > 
> > >  	if (!is_legacy && !is_typec && !is_tbt)
> > >  		return false;
> > >  
> > > +	icl_update_tc_port_type(dev_priv, intel_dig_port,
> > > is_legacy, is_typec,
> > > +				is_tbt);
> > 
> > for a while I thougth this was the start of the chain that I didn't
> > like,
> > but I was wrong, the type I believe it can/need to be here indeed.
> > 
> > but for everything else I believe that you need to handle on
> > long_pulse.
> 
> I really need better explanations instead of "I don't like" and "I feel
> this is wrong". I can't even start to think on how to implement an
> acceptable solution if you don't try to elaborate a few more problems
> on what exactly is wrong with the series, in technical explanations
> with reasons instead of feelings.
> 
> The way I imagine a solution with code moved out is that every call to
> intel_digital_port_connected() will need to have a following call to
> the function you're telling me to extract, which doesn't make sense to
> me. Or maybe we could wrap the all the call pairs under
> intel_digital_port_really_connected() :).
> 
> > 
> > if it is connected and tc_type != known than you do all the rest
> > of the opperation instead of making a huge chain from this point.
> 
> What exactly is "huge" about it?

ok... huge was just my wrong impression... it is not huge indeed and
not such a chain because status function is not called inside the tc_port_type...

> 
> > 
> > (for the lspcon wa path I don't believe we need that at all, but if
> > we
> > need we should also handle there)
> 
> Please explain with technical terms the "believe" part.

But it is not just a matter of belief and I thought I had already explained
my concerns on that very first email where you asked for options, anyways:

According to our current documentation

"
* intel_digital_port_connected - is the specified port connected?
* Return %true if port is connected, %false otherwise.
"

while behavior of intel_digital_port_connected for TC on ICL
after these patches is something like this:

If port is connected, set the TC type, set the safe bit status and
return true if all of these succeeded, false otherwise.

(now I'm avoiding the word "believe") in my honest view the architecture
here could be better in a way that we keep intel_digital_port_connect with
its only meaning and move the safe bit setup anywhere else.

After all spec also indicates that we need to set it if using the
main link or aux. imho the hotplug part is not the best path to
determine that... maybe somewhere around the modeset or power-well
or in the worst case, right after intel_digital_port_connected call,
but outside of intel_digital_port_connect itself...

> 
> 
> > 
> > For this patch:
> > 
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > 
> > > +
> > >  	return true;
> > >  }
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 61e715ddd0d5..8b3c3dc76810 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1162,6 +1162,7 @@ struct intel_digital_port {
> > >  	bool release_cl2_override;
> > >  	uint8_t max_lanes;
> > >  	enum intel_display_power_domain ddi_io_power_domain;
> > > +	enum tc_port_type tc_type;
> > >  
> > >  	void (*write_infoframe)(struct drm_encoder *encoder,
> > >  				const struct intel_crtc_state
> > > *crtc_state,
> > > -- 
> > > 2.14.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/8] drm/i915/icl: implement the tc/legacy HPD {dis, }connect flow for DP

[Paulo Zanoni]

Implement the DFLEXDPPMS/DFLEXDPCSSS dance for DisplayPort. These
functions need to be called during HPD assert/deassert, but due to how
our driver works it's much simpler if we always call them when
icl_digital_port_connected() is called, which means we won't call them
exactly once per HPD event. This should also cover the connected boot
case, whatever the BIOS does.

We're still missing the HDMI case, which should be implemented in the
next patch.

Also notice that, today, the BSpec pages for the DFLEXDPPMS and
DFLEXDPCSSS registers are wrong, so you should only trust the flows
described by the "Gen11 TypeC Programming" page in our spec.

Cc: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h |  6 +++++
 drivers/gpu/drm/i915/intel_dp.c | 57 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index c1f350469ff6..96f590a22b26 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -10223,4 +10223,10 @@ enum skl_power_gate {
 						 _ICL_PHY_MISC_B)
 #define  ICL_PHY_MISC_DE_IO_COMP_PWR_DOWN	(1 << 23)
 
+#define PORT_TX_DFLEXDPPMS				_MMIO(0x163890)
+#define   DP_PHY_MODE_STATUS_COMPLETED(tc_port)		(1 << (tc_port))
+
+#define PORT_TX_DFLEXDPCSSS				_MMIO(0x163894)
+#define   DP_PHY_MODE_STATUS_NOT_SAFE(tc_port)		(1 << (tc_port))
+
 #endif /* _I915_REG_H_ */
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 486b879cdef7..0ebce7e4c538 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4784,6 +4784,56 @@ static void icl_update_tc_port_type(struct drm_i915_private *dev_priv,
 			      type_str);
 }
 
+static bool icl_tc_phy_mode_status_connect(struct drm_i915_private *dev_priv,
+					   struct intel_digital_port *dig_port)
+{
+	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
+	u32 val;
+
+	if (dig_port->tc_type != TC_PORT_LEGACY &&
+	    dig_port->tc_type != TC_PORT_TYPEC)
+		return true;
+
+	val = I915_READ(PORT_TX_DFLEXDPPMS);
+	if (!(val & DP_PHY_MODE_STATUS_COMPLETED(tc_port))) {
+		DRM_ERROR("DP PHY for TC port %d not ready\n", tc_port);
+		return false;
+	}
+
+	/*
+	 * This function may be called many times in a row without an HPD event
+	 * in between, so try to avoid the write when we can.
+	 */
+	val = I915_READ(PORT_TX_DFLEXDPCSSS);
+	if (!(val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port))) {
+		val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
+		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
+	}
+
+	return true;
+}
+
+static void icl_tc_phy_mode_status_disconnect(struct drm_i915_private *dev_priv,
+					      struct intel_digital_port *dig_port)
+{
+	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
+	u32 val;
+
+	if (dig_port->tc_type != TC_PORT_LEGACY &&
+	    dig_port->tc_type != TC_PORT_TYPEC)
+		return;
+
+	/*
+	 * This function may be called many times in a row without an HPD event
+	 * in between, so try to avoid the write when we can.
+	 */
+	val = I915_READ(PORT_TX_DFLEXDPCSSS);
+	if (val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port)) {
+		val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
+		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
+	}
+}
+
 static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
 				  struct intel_digital_port *intel_dig_port)
 {
@@ -4804,12 +4854,17 @@ static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
 	if (cpu_isr & tbt_bit)
 		is_tbt = true;
 
-	if (!is_legacy && !is_typec && !is_tbt)
+	if (!is_legacy && !is_typec && !is_tbt) {
+		icl_tc_phy_mode_status_disconnect(dev_priv, intel_dig_port);
 		return false;
+	}
 
 	icl_update_tc_port_type(dev_priv, intel_dig_port, is_legacy, is_typec,
 				is_tbt);
 
+	if (!icl_tc_phy_mode_status_connect(dev_priv, intel_dig_port))
+		return false;
+
 	return true;
 }
 
-- 
2.14.4

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

Re: [PATCH 4/8] drm/i915/icl: implement the tc/legacy HPD {dis, }connect flow for DP

[Srivatsa, Anusha]

>-----Original Message-----
>From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
>Paulo Zanoni
>Sent: Wednesday, July 11, 2018 2:59 PM
>To: intel-gfx@lists.freedesktop.org
>Cc: Zanoni, Paulo R <paulo.r.zanoni@intel.com>
>Subject: [Intel-gfx] [PATCH 4/8] drm/i915/icl: implement the tc/legacy HPD {dis,
>}connect flow for DP
>
>Implement the DFLEXDPPMS/DFLEXDPCSSS dance for DisplayPort. These
>functions need to be called during HPD assert/deassert, but due to how our driver
>works it's much simpler if we always call them when
>icl_digital_port_connected() is called, which means we won't call them exactly
>once per HPD event. This should also cover the connected boot case, whatever
>the BIOS does.
>
>We're still missing the HDMI case, which should be implemented in the next
>patch.
>
>Also notice that, today, the BSpec pages for the DFLEXDPPMS and DFLEXDPCSSS
>registers are wrong, so you should only trust the flows described by the "Gen11
>TypeC Programming" page in our spec.
>
>Cc: Animesh Manna <animesh.manna@intel.com>
>Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com>

>---
> drivers/gpu/drm/i915/i915_reg.h |  6 +++++  drivers/gpu/drm/i915/intel_dp.c |
>57 ++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 62 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>index c1f350469ff6..96f590a22b26 100644
>--- a/drivers/gpu/drm/i915/i915_reg.h
>+++ b/drivers/gpu/drm/i915/i915_reg.h
>@@ -10223,4 +10223,10 @@ enum skl_power_gate {
> 						 _ICL_PHY_MISC_B)
> #define  ICL_PHY_MISC_DE_IO_COMP_PWR_DOWN	(1 << 23)
>
>+#define PORT_TX_DFLEXDPPMS
>	_MMIO(0x163890)
>+#define   DP_PHY_MODE_STATUS_COMPLETED(tc_port)		(1 <<
>(tc_port))
>+
>+#define PORT_TX_DFLEXDPCSSS
>	_MMIO(0x163894)
>+#define   DP_PHY_MODE_STATUS_NOT_SAFE(tc_port)		(1 << (tc_port))
>+
> #endif /* _I915_REG_H_ */
>diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>index 486b879cdef7..0ebce7e4c538 100644
>--- a/drivers/gpu/drm/i915/intel_dp.c
>+++ b/drivers/gpu/drm/i915/intel_dp.c
>@@ -4784,6 +4784,56 @@ static void icl_update_tc_port_type(struct
>drm_i915_private *dev_priv,
> 			      type_str);
> }
>
>+static bool icl_tc_phy_mode_status_connect(struct drm_i915_private *dev_priv,
>+					   struct intel_digital_port *dig_port) {
>+	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
>+	u32 val;
>+
>+	if (dig_port->tc_type != TC_PORT_LEGACY &&
>+	    dig_port->tc_type != TC_PORT_TYPEC)
>+		return true;
>+
>+	val = I915_READ(PORT_TX_DFLEXDPPMS);
>+	if (!(val & DP_PHY_MODE_STATUS_COMPLETED(tc_port))) {
>+		DRM_ERROR("DP PHY for TC port %d not ready\n", tc_port);
>+		return false;
>+	}
>+
>+	/*
>+	 * This function may be called many times in a row without an HPD event
>+	 * in between, so try to avoid the write when we can.
>+	 */
>+	val = I915_READ(PORT_TX_DFLEXDPCSSS);
>+	if (!(val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port))) {
>+		val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
>+		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
>+	}
>+
>+	return true;
>+}
>+
>+static void icl_tc_phy_mode_status_disconnect(struct drm_i915_private
>*dev_priv,
>+					      struct intel_digital_port *dig_port) {
>+	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
>+	u32 val;
>+
>+	if (dig_port->tc_type != TC_PORT_LEGACY &&
>+	    dig_port->tc_type != TC_PORT_TYPEC)
>+		return;
>+
>+	/*
>+	 * This function may be called many times in a row without an HPD event
>+	 * in between, so try to avoid the write when we can.
>+	 */
>+	val = I915_READ(PORT_TX_DFLEXDPCSSS);
>+	if (val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port)) {
>+		val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
>+		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
>+	}
>+}
>+
> static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
> 				  struct intel_digital_port *intel_dig_port)  { @@
>-4804,12 +4854,17 @@ static bool icl_tc_port_connected(struct
>drm_i915_private *dev_priv,
> 	if (cpu_isr & tbt_bit)
> 		is_tbt = true;
>
>-	if (!is_legacy && !is_typec && !is_tbt)
>+	if (!is_legacy && !is_typec && !is_tbt) {
>+		icl_tc_phy_mode_status_disconnect(dev_priv, intel_dig_port);
> 		return false;
>+	}
>
> 	icl_update_tc_port_type(dev_priv, intel_dig_port, is_legacy, is_typec,
> 				is_tbt);
>
>+	if (!icl_tc_phy_mode_status_connect(dev_priv, intel_dig_port))
>+		return false;
>+
> 	return true;
> }
>
>--
>2.14.4
>
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/8] drm/i915/icl: implement the legacy HPD {dis, }connect flow for HDMI

[Paulo Zanoni]

Just like DP, HDMI needs to implement these flows. The side effect is
that HDMI is now going to rely on the ISR bits, just like DP.

Reviewed-by: Mika Kahola <mika.kahola@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
[Rodrigo: non-trivial rebase.]
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 8363fbd18ee8..548199e59b6c 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1905,21 +1905,26 @@ intel_hdmi_set_edid(struct drm_connector *connector)
 static enum drm_connector_status
 intel_hdmi_detect(struct drm_connector *connector, bool force)
 {
-	enum drm_connector_status status;
+	enum drm_connector_status status = connector_status_disconnected;
 	struct drm_i915_private *dev_priv = to_i915(connector->dev);
+	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
+	struct intel_encoder *encoder = &hdmi_to_dig_port(intel_hdmi)->base;
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
 		      connector->base.id, connector->name);
 
 	intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
 
+	if (IS_ICELAKE(dev_priv) &&
+	    !intel_digital_port_connected(encoder))
+		goto out;
+
 	intel_hdmi_unset_edid(connector);
 
 	if (intel_hdmi_set_edid(connector))
 		status = connector_status_connected;
-	else
-		status = connector_status_disconnected;
 
+out:
 	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
 
 	return status;
-- 
2.14.4

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

[PATCH 6/8] drm/i915/icl: Update FIA supported lane count for hpd.

[Paulo Zanoni]

From: Animesh Manna <animesh.manna@intel.com>

In ICL, Flexible IO Adapter (FIA) muxes data and clocks of USB 3.1,
tbt and display controller. In DP alt mode FIA configure the
number of lanes and will be used apart from DPCD read to calculate max
available lanes for DP enablement.

Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
[Paulo: significant rewrite of the patch.]
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h |  5 +++++
 drivers/gpu/drm/i915/intel_dp.c | 33 ++++++++++++++++++++++++++++++++-
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 96f590a22b26..b6e98da87452 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -10229,4 +10229,9 @@ enum skl_power_gate {
 #define PORT_TX_DFLEXDPCSSS				_MMIO(0x163894)
 #define   DP_PHY_MODE_STATUS_NOT_SAFE(tc_port)		(1 << (tc_port))
 
+#define PORT_TX_DFLEXDPSP			_MMIO(0x1638A0)
+#define   DP_LANE_ASSIGNMENT_SHIFT(tc_port)	((tc_port) * 8)
+#define   DP_LANE_ASSIGNMENT_MASK(tc_port)	(0xf << ((tc_port) * 8))
+#define   DP_LANE_ASSIGNMENT(tc_port, x)	((x) << ((tc_port) * 8))
+
 #endif /* _I915_REG_H_ */
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 0ebce7e4c538..9b37f8cec7c3 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -176,14 +176,45 @@ static int intel_dp_max_common_rate(struct intel_dp *intel_dp)
 	return intel_dp->common_rates[intel_dp->num_common_rates - 1];
 }
 
+static int intel_dp_get_fia_supported_lane_count(struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
+	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
+	u32 lane_info;
+
+	if (tc_port == PORT_TC_NONE || dig_port->tc_type != TC_PORT_TYPEC)
+		return 4;
+
+	lane_info = (I915_READ(PORT_TX_DFLEXDPSP) &
+		     DP_LANE_ASSIGNMENT_MASK(tc_port)) >>
+		    DP_LANE_ASSIGNMENT_SHIFT(tc_port);
+
+	switch (lane_info) {
+	default:
+		MISSING_CASE(lane_info);
+	case 1:
+	case 2:
+	case 4:
+	case 8:
+		return 1;
+	case 3:
+	case 12:
+		return 2;
+	case 15:
+		return 4;
+	}
+}
+
 /* Theoretical max between source and sink */
 static int intel_dp_max_common_lane_count(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	int source_max = intel_dig_port->max_lanes;
 	int sink_max = drm_dp_max_lane_count(intel_dp->dpcd);
+	int fia_max = intel_dp_get_fia_supported_lane_count(intel_dp);
 
-	return min(source_max, sink_max);
+	return min3(source_max, sink_max, fia_max);
 }
 
 int intel_dp_max_lane_count(struct intel_dp *intel_dp)
-- 
2.14.4

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

[PATCH 7/8] drm/i915/icl: program MG_DP_MODE

[Paulo Zanoni]

Programming this register is part of the Enable Sequence for
DisplayPort on ICL. Do as the spec says.

Cc: Animesh Manna <animesh.manna@intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  | 15 +++++++++
 drivers/gpu/drm/i915/intel_ddi.c |  2 ++
 drivers/gpu/drm/i915/intel_dp.c  | 66 ++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 4 files changed, 84 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index b6e98da87452..72220c47e4dc 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2048,6 +2048,21 @@ enum i915_power_well_id {
 #define CRI_TXDEEMPH_OVERRIDE_5_0(x)			((x) << 16)
 #define CRI_TXDEEMPH_OVERRIDE_5_0_MASK			(0x3F << 16)
 
+#define _MG_DP_MODE_LN0_ACU_PORT1			0x1683A0
+#define _MG_DP_MODE_LN1_ACU_PORT1			0x1687A0
+#define _MG_DP_MODE_LN0_ACU_PORT2			0x1693A0
+#define _MG_DP_MODE_LN1_ACU_PORT2			0x1697A0
+#define _MG_DP_MODE_LN0_ACU_PORT3			0x16A3A0
+#define _MG_DP_MODE_LN1_ACU_PORT3			0x16A7A0
+#define _MG_DP_MODE_LN0_ACU_PORT4			0x16B3A0
+#define _MG_DP_MODE_LN1_ACU_PORT4			0x16B7A0
+#define MG_DP_MODE(port, ln)	\
+	_ICL_MG_PHY_PORT_LN(port, ln, _MG_DP_MODE_LN0_ACU_PORT1, \
+				      _MG_DP_MODE_LN0_ACU_PORT2, \
+				      _MG_DP_MODE_LN1_ACU_PORT1)
+#define   MG_DP_MODE_CFG_DP_X2_MODE			(1 << 7)
+#define   MG_DP_MODE_CFG_DP_X1_MODE			(1 << 6)
+
 /* The spec defines this only for BXT PHY0, but lets assume that this
  * would exist for PHY1 too if it had a second channel.
  */
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 32838ed89ee7..0e50d4c3160a 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2679,6 +2679,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
 
 	intel_display_power_get(dev_priv, dig_port->ddi_io_power_domain);
 
+	icl_program_mg_dp_mode(intel_dp);
+
 	if (IS_ICELAKE(dev_priv))
 		icl_ddi_vswing_sequence(encoder, level, encoder->type);
 	else if (IS_CANNONLAKE(dev_priv))
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 9b37f8cec7c3..88074e011333 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -229,6 +229,72 @@ intel_dp_link_required(int pixel_clock, int bpp)
 	return DIV_ROUND_UP(pixel_clock * bpp, 8);
 }
 
+void icl_program_mg_dp_mode(struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
+	enum port port = intel_dig_port->base.port;
+	enum tc_port tc_port = intel_port_to_tc(dev_priv, port);
+	u32 ln0, ln1, lane_info;
+
+	if (tc_port == PORT_TC_NONE || intel_dig_port->tc_type == TC_PORT_TBT)
+		return;
+
+	ln0 = I915_READ(MG_DP_MODE(port, 0));
+	ln1 = I915_READ(MG_DP_MODE(port, 1));
+
+	switch (intel_dig_port->tc_type) {
+	case TC_PORT_TYPEC:
+		ln0 &= ~(MG_DP_MODE_CFG_DP_X1_MODE | MG_DP_MODE_CFG_DP_X2_MODE);
+		ln1 &= ~(MG_DP_MODE_CFG_DP_X1_MODE | MG_DP_MODE_CFG_DP_X2_MODE);
+
+		lane_info = (I915_READ(PORT_TX_DFLEXDPSP) &
+			     DP_LANE_ASSIGNMENT_MASK(tc_port)) >>
+			    DP_LANE_ASSIGNMENT_SHIFT(tc_port);
+
+		switch (lane_info) {
+		case 0x1:
+		case 0x4:
+			break;
+		case 0x2:
+			ln0 |= MG_DP_MODE_CFG_DP_X1_MODE;
+			break;
+		case 0x3:
+			ln0 |= MG_DP_MODE_CFG_DP_X1_MODE |
+			       MG_DP_MODE_CFG_DP_X2_MODE;
+			break;
+		case 0x8:
+			ln1 |= MG_DP_MODE_CFG_DP_X1_MODE;
+			break;
+		case 0xC:
+			ln1 |= MG_DP_MODE_CFG_DP_X1_MODE |
+			       MG_DP_MODE_CFG_DP_X2_MODE;
+			break;
+		case 0xF:
+			ln0 |= MG_DP_MODE_CFG_DP_X1_MODE |
+			       MG_DP_MODE_CFG_DP_X2_MODE;
+			ln1 |= MG_DP_MODE_CFG_DP_X1_MODE |
+			       MG_DP_MODE_CFG_DP_X2_MODE;
+			break;
+		default:
+			MISSING_CASE(lane_info);
+		}
+		break;
+
+	case TC_PORT_LEGACY:
+		ln0 |= MG_DP_MODE_CFG_DP_X1_MODE | MG_DP_MODE_CFG_DP_X2_MODE;
+		ln1 |= MG_DP_MODE_CFG_DP_X1_MODE | MG_DP_MODE_CFG_DP_X2_MODE;
+		break;
+
+	default:
+		MISSING_CASE(intel_dig_port->tc_type);
+		return;
+	}
+
+	I915_WRITE(MG_DP_MODE(port, 0), ln0);
+	I915_WRITE(MG_DP_MODE(port, 1), ln1);
+}
+
 int
 intel_dp_max_data_rate(int max_link_clock, int max_lanes)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 8b3c3dc76810..7946d41e78a3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1698,6 +1698,7 @@ void intel_edp_drrs_invalidate(struct drm_i915_private *dev_priv,
 			       unsigned int frontbuffer_bits);
 void intel_edp_drrs_flush(struct drm_i915_private *dev_priv,
 			  unsigned int frontbuffer_bits);
+void icl_program_mg_dp_mode(struct intel_dp *intel_dp);
 
 void
 intel_dp_program_link_training_pattern(struct intel_dp *intel_dp,
-- 
2.14.4

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

[PATCH 8/8] drm/i915/icl: toggle PHY clock gating around link training

[Paulo Zanoni]

The Gen11 TypeC PHY DDI Buffer chapter, PHY Clock Gating Programming
section says that PHY clock gating should be disabled before starting
voltage swing programming, then enabled after any link training is
complete.

Cc: Animesh Manna <animesh.manna@intel.com>
Cc: Manasi Navare <manasi.d.navare@intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  | 21 +++++++++++++
 drivers/gpu/drm/i915/intel_ddi.c |  3 ++
 drivers/gpu/drm/i915/intel_dp.c  | 66 ++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  2 ++
 4 files changed, 92 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 72220c47e4dc..c4e470676717 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2062,6 +2062,27 @@ enum i915_power_well_id {
 				      _MG_DP_MODE_LN1_ACU_PORT1)
 #define   MG_DP_MODE_CFG_DP_X2_MODE			(1 << 7)
 #define   MG_DP_MODE_CFG_DP_X1_MODE			(1 << 6)
+#define   MG_DP_MODE_CFG_TR2PWR_GATING			(1 << 5)
+#define   MG_DP_MODE_CFG_TRPWR_GATING			(1 << 4)
+#define   MG_DP_MODE_CFG_CLNPWR_GATING			(1 << 3)
+#define   MG_DP_MODE_CFG_DIGPWR_GATING			(1 << 2)
+#define   MG_DP_MODE_CFG_GAONPWR_GATING			(1 << 1)
+
+#define _MG_MISC_SUS0_PORT1				0x168814
+#define _MG_MISC_SUS0_PORT2				0x169814
+#define _MG_MISC_SUS0_PORT3				0x16A814
+#define _MG_MISC_SUS0_PORT4				0x16B814
+#define MG_MISC_SUS0(tc_port) \
+	_MMIO(_PORT(tc_port, _MG_MISC_SUS0_PORT1, _MG_MISC_SUS0_PORT2))
+#define   MG_MISC_SUS0_SUSCLK_DYNCLKGATE_MODE_MASK	(3 << 14)
+#define   MG_MISC_SUS0_SUSCLK_DYNCLKGATE_MODE(x)	((x) << 14)
+#define   MG_MISC_SUS0_CFG_TR2PWR_GATING		(1 << 12)
+#define   MG_MISC_SUS0_CFG_CL2PWR_GATING		(1 << 11)
+#define   MG_MISC_SUS0_CFG_GAONPWR_GATING		(1 << 10)
+#define   MG_MISC_SUS0_CFG_TRPWR_GATING			(1 << 7)
+#define   MG_MISC_SUS0_CFG_CL1PWR_GATING		(1 << 6)
+#define   MG_MISC_SUS0_CFG_DGPWR_GATING			(1 << 5)
+
 
 /* The spec defines this only for BXT PHY0, but lets assume that this
  * would exist for PHY1 too if it had a second channel.
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 0e50d4c3160a..3308f4439e4d 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2680,6 +2680,7 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
 	intel_display_power_get(dev_priv, dig_port->ddi_io_power_domain);
 
 	icl_program_mg_dp_mode(intel_dp);
+	icl_disable_phy_clock_gating(dig_port);
 
 	if (IS_ICELAKE(dev_priv))
 		icl_ddi_vswing_sequence(encoder, level, encoder->type);
@@ -2697,6 +2698,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
 	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
 		intel_dp_stop_link_train(intel_dp);
 
+	icl_enable_phy_clock_gating(dig_port);
+
 	intel_ddi_enable_pipe_clock(crtc_state);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 88074e011333..fc3fc7d7115e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -295,6 +295,72 @@ void icl_program_mg_dp_mode(struct intel_dp *intel_dp)
 	I915_WRITE(MG_DP_MODE(port, 1), ln1);
 }
 
+void icl_enable_phy_clock_gating(struct intel_digital_port *dig_port)
+{
+	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
+	enum port port = dig_port->base.port;
+	enum tc_port tc_port = intel_port_to_tc(dev_priv, port);
+	i915_reg_t mg_regs[2] = { MG_DP_MODE(port, 0), MG_DP_MODE(port, 1) };
+	u32 val;
+	int i;
+
+	if (tc_port == PORT_TC_NONE)
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(mg_regs); i++) {
+		val = I915_READ(mg_regs[i]);
+		val |= MG_DP_MODE_CFG_TR2PWR_GATING |
+		       MG_DP_MODE_CFG_TRPWR_GATING |
+		       MG_DP_MODE_CFG_CLNPWR_GATING |
+		       MG_DP_MODE_CFG_DIGPWR_GATING |
+		       MG_DP_MODE_CFG_GAONPWR_GATING;
+		I915_WRITE(mg_regs[i], val);
+	}
+
+	val = I915_READ(MG_MISC_SUS0(tc_port));
+	val |= MG_MISC_SUS0_SUSCLK_DYNCLKGATE_MODE(3) |
+	       MG_MISC_SUS0_CFG_TR2PWR_GATING |
+	       MG_MISC_SUS0_CFG_CL2PWR_GATING |
+	       MG_MISC_SUS0_CFG_GAONPWR_GATING |
+	       MG_MISC_SUS0_CFG_TRPWR_GATING |
+	       MG_MISC_SUS0_CFG_CL1PWR_GATING |
+	       MG_MISC_SUS0_CFG_DGPWR_GATING;
+	I915_WRITE(MG_MISC_SUS0(tc_port), val);
+}
+
+void icl_disable_phy_clock_gating(struct intel_digital_port *dig_port)
+{
+	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
+	enum port port = dig_port->base.port;
+	enum tc_port tc_port = intel_port_to_tc(dev_priv, port);
+	i915_reg_t mg_regs[2] = { MG_DP_MODE(port, 0), MG_DP_MODE(port, 1) };
+	u32 val;
+	int i;
+
+	if (tc_port == PORT_TC_NONE)
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(mg_regs); i++) {
+		val = I915_READ(mg_regs[i]);
+		val &= ~(MG_DP_MODE_CFG_TR2PWR_GATING |
+			 MG_DP_MODE_CFG_TRPWR_GATING |
+			 MG_DP_MODE_CFG_CLNPWR_GATING |
+			 MG_DP_MODE_CFG_DIGPWR_GATING |
+			 MG_DP_MODE_CFG_GAONPWR_GATING);
+		I915_WRITE(mg_regs[i], val);
+	}
+
+	val = I915_READ(MG_MISC_SUS0(tc_port));
+	val &= ~(MG_MISC_SUS0_SUSCLK_DYNCLKGATE_MODE_MASK |
+		 MG_MISC_SUS0_CFG_TR2PWR_GATING |
+		 MG_MISC_SUS0_CFG_CL2PWR_GATING |
+		 MG_MISC_SUS0_CFG_GAONPWR_GATING |
+		 MG_MISC_SUS0_CFG_TRPWR_GATING |
+		 MG_MISC_SUS0_CFG_CL1PWR_GATING |
+		 MG_MISC_SUS0_CFG_DGPWR_GATING);
+	I915_WRITE(MG_MISC_SUS0(tc_port), val);
+}
+
 int
 intel_dp_max_data_rate(int max_link_clock, int max_lanes)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 7946d41e78a3..ef709d201dcb 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1699,6 +1699,8 @@ void intel_edp_drrs_invalidate(struct drm_i915_private *dev_priv,
 void intel_edp_drrs_flush(struct drm_i915_private *dev_priv,
 			  unsigned int frontbuffer_bits);
 void icl_program_mg_dp_mode(struct intel_dp *intel_dp);
+void icl_enable_phy_clock_gating(struct intel_digital_port *dig_port);
+void icl_disable_phy_clock_gating(struct intel_digital_port *dig_port);
 
 void
 intel_dp_program_link_training_pattern(struct intel_dp *intel_dp,
-- 
2.14.4

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

✗ Fi.CI.CHECKPATCH: warning for Remaining ICL display patches

[Patchwork]

== Series Details ==

Series: Remaining ICL display patches
URL   : https://patchwork.freedesktop.org/series/46338/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
0b47f5ccf84a drm/i915/icl: compute the TBT PLL registers
-:23: CHECK:CAMELCASE: Avoid CamelCase: <icl_tbt_pll_24MHz_values>
#23: FILE: drivers/gpu/drm/i915/intel_dpll_mgr.c:2455:
+static const struct skl_wrpll_params icl_tbt_pll_24MHz_values = {

-:28: CHECK:CAMELCASE: Avoid CamelCase: <icl_tbt_pll_19_2MHz_values>
#28: FILE: drivers/gpu/drm/i915/intel_dpll_mgr.c:2460:
+static const struct skl_wrpll_params icl_tbt_pll_19_2MHz_values = {

total: 0 errors, 0 warnings, 2 checks, 40 lines checked
c0139766f2a4 drm/i915/icl: implement icl_digital_port_connected()
4b17c6d840cc drm/i915/icl: store the port type for TC ports
b2100d0ada7f drm/i915/icl: implement the tc/legacy HPD {dis, }connect flow for DP
a48797927962 drm/i915/icl: implement the legacy HPD {dis, }connect flow for HDMI
43efb13b9e7f drm/i915/icl: Update FIA supported lane count for hpd.
980aba720404 drm/i915/icl: program MG_DP_MODE
71c95254a556 drm/i915/icl: toggle PHY clock gating around link training

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

✗ Fi.CI.SPARSE: warning for Remaining ICL display patches

[Patchwork]

== Series Details ==

Series: Remaining ICL display patches
URL   : https://patchwork.freedesktop.org/series/46338/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm/i915/icl: compute the TBT PLL registers
Okay!

Commit: drm/i915/icl: implement icl_digital_port_connected()
Okay!

Commit: drm/i915/icl: store the port type for TC ports
Okay!

Commit: drm/i915/icl: implement the tc/legacy HPD {dis, }connect flow for DP
Okay!

Commit: drm/i915/icl: implement the legacy HPD {dis, }connect flow for HDMI
Okay!

Commit: drm/i915/icl: Update FIA supported lane count for hpd.
-O:drivers/gpu/drm/i915/intel_dp.c:186:16: warning: expression using sizeof(void)
-O:drivers/gpu/drm/i915/intel_dp.c:186:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_dp.c:217:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_dp.c:217:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_dp.c:217:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_dp.c:217:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_dp.c:217:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_dp.c:217:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_dp.c:217:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_dp.c:217:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_dp.c:217:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_dp.c:217:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_dp.c:217:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_dp.c:217:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_dp.c:217:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_dp.c:217:16: warning: expression using sizeof(void)

Commit: drm/i915/icl: program MG_DP_MODE
Okay!

Commit: drm/i915/icl: toggle PHY clock gating around link training
Okay!

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

✓ Fi.CI.BAT: success for Remaining ICL display patches

[Patchwork]

== Series Details ==

Series: Remaining ICL display patches
URL   : https://patchwork.freedesktop.org/series/46338/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4473 -> Patchwork_9618 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/46338/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_9618 that come from known issues:

  === IGT changes ===

    ==== Possible fixes ====

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-skl-6700k2:      FAIL (fdo#103191) -> PASS

    
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191


== Participating hosts (46 -> 42) ==

  Additional (1): fi-skl-guc 
  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4473 -> Patchwork_9618

  CI_DRM_4473: 17c95b123220c6896ac9cf99c78be4709da704d4 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4549: e19fd5549e9cf603251704117fc64f4068be5016 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9618: 71c95254a55617b1179aad69f5c53c8d843a8565 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

71c95254a556 drm/i915/icl: toggle PHY clock gating around link training
980aba720404 drm/i915/icl: program MG_DP_MODE
43efb13b9e7f drm/i915/icl: Update FIA supported lane count for hpd.
a48797927962 drm/i915/icl: implement the legacy HPD {dis, }connect flow for HDMI
b2100d0ada7f drm/i915/icl: implement the tc/legacy HPD {dis, }connect flow for DP
4b17c6d840cc drm/i915/icl: store the port type for TC ports
c0139766f2a4 drm/i915/icl: implement icl_digital_port_connected()
0b47f5ccf84a drm/i915/icl: compute the TBT PLL registers

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9618/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx