diff mbox

[10/24] drm/i915/icl: add icelake_get_ddi_pll()

Message ID 20180522002558.29262-11-paulo.r.zanoni@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Zanoni, Paulo R May 22, 2018, 12:25 a.m. UTC 
Implement the hardware state readout code.

Thanks to Animesh Manna for spotting this problem.

Cc: Animesh Manna <animesh.manna@intel.com>
Credits-to: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 42 +++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

Comments

Lucas De Marchi June 13, 2018, 11:15 p.m. UTC | #1 
On Mon, May 21, 2018 at 05:25:44PM -0700, Paulo Zanoni wrote:
> Implement the hardware state readout code.
> 
> Thanks to Animesh Manna for spotting this problem.
> 
> Cc: Animesh Manna <animesh.manna@intel.com>
> Credits-to: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 42 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 64593b0fbebd..d5a19c1b3b20 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9146,6 +9146,44 @@ static void cannonlake_get_ddi_pll(struct drm_i915_private *dev_priv,
>  	pipe_config->shared_dpll = intel_get_shared_dpll_by_id(dev_priv, id);
>  }
>  
> +static void icelake_get_ddi_pll(struct drm_i915_private *dev_priv,
> +				enum port port,
> +				struct intel_crtc_state *pipe_config)
> +{
> +	enum intel_dpll_id id;
> +	u32 temp;
> +
> +	/* TODO: TBT pll not implemented. */
> +	switch (port) {
> +	case PORT_A:
> +	case PORT_B:
> +		temp = I915_READ(DPCLKA_CFGCR0_ICL) &
> +		       DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port);
> +		id = temp >> DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(port);

This could be simpler:

	temp = I915_READ(DPCLKA_CFGCR0_ICL) >> DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(port);
	id = temp & DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(0);

But this ship has sailed, aka MASK above requires the port and
hardcoding 0 doesn't make it better IMO.


Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>


Lucas De Marchi

> +
> +		if (WARN_ON(id != DPLL_ID_ICL_DPLL0 && id != DPLL_ID_ICL_DPLL1))
> +			return;
> +		break;
> +	case PORT_C:
> +		id = DPLL_ID_ICL_MGPLL1;
> +		break;
> +	case PORT_D:
> +		id = DPLL_ID_ICL_MGPLL2;
> +		break;
> +	case PORT_E:
> +		id = DPLL_ID_ICL_MGPLL3;
> +		break;
> +	case PORT_F:
> +		id = DPLL_ID_ICL_MGPLL4;
> +		break;
> +	default:
> +		MISSING_CASE(port);
> +		return;
> +	}
> +
> +	pipe_config->shared_dpll = intel_get_shared_dpll_by_id(dev_priv, id);
> +}
> +
>  static void bxt_get_ddi_pll(struct drm_i915_private *dev_priv,
>  				enum port port,
>  				struct intel_crtc_state *pipe_config)
> @@ -9333,7 +9371,9 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
>  
>  	port = (tmp & TRANS_DDI_PORT_MASK) >> TRANS_DDI_PORT_SHIFT;
>  
> -	if (IS_CANNONLAKE(dev_priv))
> +	if (IS_ICELAKE(dev_priv))
> +		icelake_get_ddi_pll(dev_priv, port, pipe_config);
> +	else if (IS_CANNONLAKE(dev_priv))
>  		cannonlake_get_ddi_pll(dev_priv, port, pipe_config);
>  	else if (IS_GEN9_BC(dev_priv))
>  		skylake_get_ddi_pll(dev_priv, port, pipe_config);
> -- 
> 2.14.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Zanoni, Paulo R June 13, 2018, 11:51 p.m. UTC | #2 
Em Qua, 2018-06-13 às 16:15 -0700, Lucas De Marchi escreveu:
> On Mon, May 21, 2018 at 05:25:44PM -0700, Paulo Zanoni wrote:
> > Implement the hardware state readout code.
> > 
> > Thanks to Animesh Manna for spotting this problem.
> > 
> > Cc: Animesh Manna <animesh.manna@intel.com>
> > Credits-to: Animesh Manna <animesh.manna@intel.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 42
> > +++++++++++++++++++++++++++++++++++-
> >  1 file changed, 41 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 64593b0fbebd..d5a19c1b3b20 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9146,6 +9146,44 @@ static void cannonlake_get_ddi_pll(struct
> > drm_i915_private *dev_priv,
> >  	pipe_config->shared_dpll =
> > intel_get_shared_dpll_by_id(dev_priv, id);
> >  }
> >  
> > +static void icelake_get_ddi_pll(struct drm_i915_private *dev_priv,
> > +				enum port port,
> > +				struct intel_crtc_state
> > *pipe_config)
> > +{
> > +	enum intel_dpll_id id;
> > +	u32 temp;
> > +
> > +	/* TODO: TBT pll not implemented. */
> > +	switch (port) {
> > +	case PORT_A:
> > +	case PORT_B:
> > +		temp = I915_READ(DPCLKA_CFGCR0_ICL) &
> > +		       DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port);
> > +		id = temp >>
> > DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(port);
> 
> This could be simpler:
> 
> 	temp = I915_READ(DPCLKA_CFGCR0_ICL) >>
> DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(port);
> 	id = temp & DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(0);

I fail to understand why this is simpler... The same operations, just
on a different order.


> 
> But this ship has sailed, aka MASK above requires the port and
> hardcoding 0 doesn't make it better IMO.
> 
> 
> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
> 

Thanks!

> 
> Lucas De Marchi
> 
> > +
> > +		if (WARN_ON(id != DPLL_ID_ICL_DPLL0 && id !=
> > DPLL_ID_ICL_DPLL1))
> > +			return;
> > +		break;
> > +	case PORT_C:
> > +		id = DPLL_ID_ICL_MGPLL1;
> > +		break;
> > +	case PORT_D:
> > +		id = DPLL_ID_ICL_MGPLL2;
> > +		break;
> > +	case PORT_E:
> > +		id = DPLL_ID_ICL_MGPLL3;
> > +		break;
> > +	case PORT_F:
> > +		id = DPLL_ID_ICL_MGPLL4;
> > +		break;
> > +	default:
> > +		MISSING_CASE(port);
> > +		return;
> > +	}
> > +
> > +	pipe_config->shared_dpll =
> > intel_get_shared_dpll_by_id(dev_priv, id);
> > +}
> > +
> >  static void bxt_get_ddi_pll(struct drm_i915_private *dev_priv,
> >  				enum port port,
> >  				struct intel_crtc_state
> > *pipe_config)
> > @@ -9333,7 +9371,9 @@ static void haswell_get_ddi_port_state(struct
> > intel_crtc *crtc,
> >  
> >  	port = (tmp & TRANS_DDI_PORT_MASK) >>
> > TRANS_DDI_PORT_SHIFT;
> >  
> > -	if (IS_CANNONLAKE(dev_priv))
> > +	if (IS_ICELAKE(dev_priv))
> > +		icelake_get_ddi_pll(dev_priv, port, pipe_config);
> > +	else if (IS_CANNONLAKE(dev_priv))
> >  		cannonlake_get_ddi_pll(dev_priv, port,
> > pipe_config);
> >  	else if (IS_GEN9_BC(dev_priv))
> >  		skylake_get_ddi_pll(dev_priv, port, pipe_config);
> > -- 
> > 2.14.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Lucas De Marchi June 13, 2018, 11:55 p.m. UTC | #3 
On Wed, Jun 13, 2018 at 04:51:57PM -0700, Paulo Zanoni wrote:
> Em Qua, 2018-06-13 às 16:15 -0700, Lucas De Marchi escreveu:
> > On Mon, May 21, 2018 at 05:25:44PM -0700, Paulo Zanoni wrote:
> > > Implement the hardware state readout code.
> > > 
> > > Thanks to Animesh Manna for spotting this problem.
> > > 
> > > Cc: Animesh Manna <animesh.manna@intel.com>
> > > Credits-to: Animesh Manna <animesh.manna@intel.com>
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 42
> > > +++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 41 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 64593b0fbebd..d5a19c1b3b20 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -9146,6 +9146,44 @@ static void cannonlake_get_ddi_pll(struct
> > > drm_i915_private *dev_priv,
> > >  	pipe_config->shared_dpll =
> > > intel_get_shared_dpll_by_id(dev_priv, id);
> > >  }
> > >  
> > > +static void icelake_get_ddi_pll(struct drm_i915_private *dev_priv,
> > > +				enum port port,
> > > +				struct intel_crtc_state
> > > *pipe_config)
> > > +{
> > > +	enum intel_dpll_id id;
> > > +	u32 temp;
> > > +
> > > +	/* TODO: TBT pll not implemented. */
> > > +	switch (port) {
> > > +	case PORT_A:
> > > +	case PORT_B:
> > > +		temp = I915_READ(DPCLKA_CFGCR0_ICL) &
> > > +		       DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port);
> > > +		id = temp >>
> > > DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(port);
> > 
> > This could be simpler:
> > 
> > 	temp = I915_READ(DPCLKA_CFGCR0_ICL) >>
> > DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(port);
> > 	id = temp & DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(0);
> 
> I fail to understand why this is simpler... The same operations, just
> on a different order.

If you open up the macros you will see there are more operations on what
we are doing right now... and we wouldn't need to depend on the port for
this if we had done this way. However we already depend on having the
port as an argument in other places, so there's nothing to do
differently on this particular patch.

Lucas De Marchi

> 
> 
> > 
> > But this ship has sailed, aka MASK above requires the port and
> > hardcoding 0 doesn't make it better IMO.
> > 
> > 
> > Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > 
> 
> Thanks!
> 
> > 
> > Lucas De Marchi
> > 
> > > +
> > > +		if (WARN_ON(id != DPLL_ID_ICL_DPLL0 && id !=
> > > DPLL_ID_ICL_DPLL1))
> > > +			return;
> > > +		break;
> > > +	case PORT_C:
> > > +		id = DPLL_ID_ICL_MGPLL1;
> > > +		break;
> > > +	case PORT_D:
> > > +		id = DPLL_ID_ICL_MGPLL2;
> > > +		break;
> > > +	case PORT_E:
> > > +		id = DPLL_ID_ICL_MGPLL3;
> > > +		break;
> > > +	case PORT_F:
> > > +		id = DPLL_ID_ICL_MGPLL4;
> > > +		break;
> > > +	default:
> > > +		MISSING_CASE(port);
> > > +		return;
> > > +	}
> > > +
> > > +	pipe_config->shared_dpll =
> > > intel_get_shared_dpll_by_id(dev_priv, id);
> > > +}
> > > +
> > >  static void bxt_get_ddi_pll(struct drm_i915_private *dev_priv,
> > >  				enum port port,
> > >  				struct intel_crtc_state
> > > *pipe_config)
> > > @@ -9333,7 +9371,9 @@ static void haswell_get_ddi_port_state(struct
> > > intel_crtc *crtc,
> > >  
> > >  	port = (tmp & TRANS_DDI_PORT_MASK) >>
> > > TRANS_DDI_PORT_SHIFT;
> > >  
> > > -	if (IS_CANNONLAKE(dev_priv))
> > > +	if (IS_ICELAKE(dev_priv))
> > > +		icelake_get_ddi_pll(dev_priv, port, pipe_config);
> > > +	else if (IS_CANNONLAKE(dev_priv))
> > >  		cannonlake_get_ddi_pll(dev_priv, port,
> > > pipe_config);
> > >  	else if (IS_GEN9_BC(dev_priv))
> > >  		skylake_get_ddi_pll(dev_priv, port, pipe_config);
> > > -- 
> > > 2.14.3
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 64593b0fbebd..d5a19c1b3b20 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9146,6 +9146,44 @@  static void cannonlake_get_ddi_pll(struct drm_i915_private *dev_priv,
 	pipe_config->shared_dpll = intel_get_shared_dpll_by_id(dev_priv, id);
 }
 
+static void icelake_get_ddi_pll(struct drm_i915_private *dev_priv,
+				enum port port,
+				struct intel_crtc_state *pipe_config)
+{
+	enum intel_dpll_id id;
+	u32 temp;
+
+	/* TODO: TBT pll not implemented. */
+	switch (port) {
+	case PORT_A:
+	case PORT_B:
+		temp = I915_READ(DPCLKA_CFGCR0_ICL) &
+		       DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port);
+		id = temp >> DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(port);
+
+		if (WARN_ON(id != DPLL_ID_ICL_DPLL0 && id != DPLL_ID_ICL_DPLL1))
+			return;
+		break;
+	case PORT_C:
+		id = DPLL_ID_ICL_MGPLL1;
+		break;
+	case PORT_D:
+		id = DPLL_ID_ICL_MGPLL2;
+		break;
+	case PORT_E:
+		id = DPLL_ID_ICL_MGPLL3;
+		break;
+	case PORT_F:
+		id = DPLL_ID_ICL_MGPLL4;
+		break;
+	default:
+		MISSING_CASE(port);
+		return;
+	}
+
+	pipe_config->shared_dpll = intel_get_shared_dpll_by_id(dev_priv, id);
+}
+
 static void bxt_get_ddi_pll(struct drm_i915_private *dev_priv,
 				enum port port,
 				struct intel_crtc_state *pipe_config)
@@ -9333,7 +9371,9 @@  static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
 
 	port = (tmp & TRANS_DDI_PORT_MASK) >> TRANS_DDI_PORT_SHIFT;
 
-	if (IS_CANNONLAKE(dev_priv))
+	if (IS_ICELAKE(dev_priv))
+		icelake_get_ddi_pll(dev_priv, port, pipe_config);
+	else if (IS_CANNONLAKE(dev_priv))
 		cannonlake_get_ddi_pll(dev_priv, port, pipe_config);
 	else if (IS_GEN9_BC(dev_priv))
 		skylake_get_ddi_pll(dev_priv, port, pipe_config);