[v3,07/14] drm/i915/dp: Add a standalone function to obtain shared dpll for HSW/BDW/SKL/BXT

Commit Message

Navare, Manasi Sept. 1, 2016, 10:08 p.m. UTC

From: Jim Bride <[e-mail]>

Add the PLL selection code for HSW/BDW/BXT/SKL into a stand-alone function
in order to allow for the implementation of a platform neutral upfront
link training function.

v3:
* Add Hooks for all DDI platforms into this standalone function

v2:
* Change the macro to use dev_priv instead of dev (David Weinehall)

Reviewed-by: Durgadoss R <[e-mail]>
Signed-off-by: Manasi Navare <[e-mail]>
Signed-off-by: Jim Bride <[e-mail]>
---
 drivers/gpu/drm/i915/intel_ddi.c      | 38 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 38 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dpll_mgr.h |  2 ++
 drivers/gpu/drm/i915/intel_drv.h      |  3 ++-
 4 files changed, 80 insertions(+), 1 deletion(-)

Comments

Dhinakaran Pandiyan Sept. 2, 2016, 8:06 p.m. UTC | #1

On Thu, 2016-09-01 at 15:08 -0700, Manasi Navare wrote:
> From: Jim Bride <[e-mail]>

> 

> Add the PLL selection code for HSW/BDW/BXT/SKL into a stand-alone function

> in order to allow for the implementation of a platform neutral upfront

> link training function.

> 

> v3:

> * Add Hooks for all DDI platforms into this standalone function

> 

> v2:

> * Change the macro to use dev_priv instead of dev (David Weinehall)

> 

> Reviewed-by: Durgadoss R <[e-mail]>

> Signed-off-by: Manasi Navare <[e-mail]>

> Signed-off-by: Jim Bride <[e-mail]>

> ---

>  drivers/gpu/drm/i915/intel_ddi.c      | 38 +++++++++++++++++++++++++++++++++++

>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 38 +++++++++++++++++++++++++++++++++++

>  drivers/gpu/drm/i915/intel_dpll_mgr.h |  2 ++

>  drivers/gpu/drm/i915/intel_drv.h      |  3 ++-

>  4 files changed, 80 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c

> index e4b875e..67a6a0b 100644

> --- a/drivers/gpu/drm/i915/intel_ddi.c

> +++ b/drivers/gpu/drm/i915/intel_ddi.c

> @@ -2393,6 +2393,44 @@ intel_ddi_init_hdmi_connector(struct intel_digital_port *intel_dig_port)

>  	return connector;

>  }

>  

> +struct intel_shared_dpll *

> +intel_ddi_get_link_dpll(struct intel_dp *intel_dp, int clock)

> +{

> +	struct intel_connector *connector = intel_dp->attached_connector;

> +	struct intel_encoder *encoder = connector->encoder;

> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);

> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);

> +	struct intel_shared_dpll *pll = NULL;

> +	struct intel_shared_dpll_config tmp_pll_config;

> +	enum intel_dpll_id dpll_id;

> +

> +	if (IS_BROXTON(dev_priv)) {

> +		dpll_id =  (enum intel_dpll_id)dig_port->port;

> +		/*

> +		 * Select the required PLL. This works for platforms where

> +		 * there is no shared DPLL.

> +		 */

> +		pll = &dev_priv->shared_dplls[dpll_id];

> +		if (WARN_ON(pll->active_mask)) {

> +

> +			DRM_ERROR("Shared DPLL in use. active_mask:%x\n",

> +				  pll->active_mask);

> +			pll = NULL;

> +		}

> +		tmp_pll_config = pll->config;


NULL dereference when pll is in use? 

> +		if (!bxt_ddi_dp_set_dpll_hw_state(clock,

> +						  &pll->config.hw_state)) {

> +			DRM_ERROR("Could not setup DPLL\n");

> +			pll->config = tmp_pll_config;

> +		}

> +	} else if (IS_SKYLAKE(dev_priv)) {

> +		pll = skl_find_link_pll(dev_priv, clock);

> +	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {

> +		pll = hsw_ddi_dp_get_dpll(encoder, clock);

> +	}

> +	return pll;

> +}

> +

>  void intel_ddi_init(struct drm_device *dev, enum port port)

>  {

>  	struct drm_i915_private *dev_priv = to_i915(dev);

> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c

> index 9a1da98..4b067ac 100644

> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c

> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c

> @@ -24,6 +24,44 @@

>  #include "intel_drv.h"

>  

>  struct intel_shared_dpll *

> +skl_find_link_pll(struct drm_i915_private *dev_priv, int clock)

> +{

> +	struct intel_shared_dpll *pll = NULL;

> +	struct intel_dpll_hw_state dpll_hw_state;

> +	enum intel_dpll_id i;

> +	bool found = false;

> +

> +	if (!skl_ddi_dp_set_dpll_hw_state(clock, &dpll_hw_state))

> +		return pll;

> +

> +	for (i = DPLL_ID_SKL_DPLL1; i <= DPLL_ID_SKL_DPLL3; i++) {

> +		pll = &dev_priv->shared_dplls[i];

> +

> +		/* Only want to check enabled timings first */

> +		if (pll->config.crtc_mask == 0)

> +			continue;

> +

> +		if (memcmp(&dpll_hw_state, &pll->config.hw_state,

> +			   sizeof(pll->config.hw_state)) == 0) {

> +			found = true;

> +			break;

> +		}

> +	}

> +

> +	/* Ok no matching timings, maybe there's a free one? */

> +	for (i = DPLL_ID_SKL_DPLL1;

> +	     ((found == false) && (i <= DPLL_ID_SKL_DPLL3)); i++) {

> +		pll = &dev_priv->shared_dplls[i];

> +		if (pll->config.crtc_mask == 0) {

> +			pll->config.hw_state = dpll_hw_state;

> +			break;

> +		}

> +	}

> +

> +	return pll;

> +}

> +

> +struct intel_shared_dpll *

>  intel_get_shared_dpll_by_id(struct drm_i915_private *dev_priv,

>  			    enum intel_dpll_id id)

>  {

> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h

> index aed7408..f438535 100644

> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h

> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h

> @@ -168,6 +168,8 @@ bool bxt_ddi_dp_set_dpll_hw_state(int clock,

>  /* SKL dpll related functions */

>  bool skl_ddi_dp_set_dpll_hw_state(int clock,

>  				  struct intel_dpll_hw_state *dpll_hw_state);

> +struct intel_shared_dpll *skl_find_link_pll(struct drm_i915_private *dev_priv,

> +					    int clock);

>  

> 

>  /* HSW dpll related functions */

> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h

> index 529fa7b..efcd80b 100644

> --- a/drivers/gpu/drm/i915/intel_drv.h

> +++ b/drivers/gpu/drm/i915/intel_drv.h

> @@ -1159,7 +1159,8 @@ void intel_ddi_clock_get(struct intel_encoder *encoder,

>  			 struct intel_crtc_state *pipe_config);

>  void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state);

>  uint32_t ddi_signal_levels(struct intel_dp *intel_dp);

> -

> +struct intel_shared_dpll *intel_ddi_get_link_dpll(struct intel_dp *intel_dp,

> +						  int clock);

>  unsigned int intel_fb_align_height(struct drm_device *dev,

>  				   unsigned int height,

>  				   uint32_t pixel_format,

Navare, Manasi Sept. 7, 2016, 10:08 p.m. UTC | #2

On Fri, Sep 02, 2016 at 01:06:32PM -0700, Pandiyan, Dhinakaran wrote:
> On Thu, 2016-09-01 at 15:08 -0700, Manasi Navare wrote:
> > From: Jim Bride <[e-mail]>
> > 
> > Add the PLL selection code for HSW/BDW/BXT/SKL into a stand-alone function
> > in order to allow for the implementation of a platform neutral upfront
> > link training function.
> > 
> > v3:
> > * Add Hooks for all DDI platforms into this standalone function
> > 
> > v2:
> > * Change the macro to use dev_priv instead of dev (David Weinehall)
> > 
> > Reviewed-by: Durgadoss R <[e-mail]>
> > Signed-off-by: Manasi Navare <[e-mail]>
> > Signed-off-by: Jim Bride <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c      | 38 +++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 38 +++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_dpll_mgr.h |  2 ++
> >  drivers/gpu/drm/i915/intel_drv.h      |  3 ++-
> >  4 files changed, 80 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index e4b875e..67a6a0b 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -2393,6 +2393,44 @@ intel_ddi_init_hdmi_connector(struct intel_digital_port *intel_dig_port)
> >  	return connector;
> >  }
> >  
> > +struct intel_shared_dpll *
> > +intel_ddi_get_link_dpll(struct intel_dp *intel_dp, int clock)
> > +{
> > +	struct intel_connector *connector = intel_dp->attached_connector;
> > +	struct intel_encoder *encoder = connector->encoder;
> > +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > +	struct intel_shared_dpll *pll = NULL;
> > +	struct intel_shared_dpll_config tmp_pll_config;
> > +	enum intel_dpll_id dpll_id;
> > +
> > +	if (IS_BROXTON(dev_priv)) {
> > +		dpll_id =  (enum intel_dpll_id)dig_port->port;
> > +		/*
> > +		 * Select the required PLL. This works for platforms where
> > +		 * there is no shared DPLL.
> > +		 */
> > +		pll = &dev_priv->shared_dplls[dpll_id];
> > +		if (WARN_ON(pll->active_mask)) {
> > +
> > +			DRM_ERROR("Shared DPLL in use. active_mask:%x\n",
> > +				  pll->active_mask);
> > +			pll = NULL;
> > +		}
> > +		tmp_pll_config = pll->config;
> 
> NULL dereference when pll is in use? 

Yes I had given this comment in the internal review. So we moved this assignment out 
to the calling function, but left this here as well. I will remove this and resubmit.

Manasi
> 
> > +		if (!bxt_ddi_dp_set_dpll_hw_state(clock,
> > +						  &pll->config.hw_state)) {
> > +			DRM_ERROR("Could not setup DPLL\n");
> > +			pll->config = tmp_pll_config;
> > +		}
> > +	} else if (IS_SKYLAKE(dev_priv)) {
> > +		pll = skl_find_link_pll(dev_priv, clock);
> > +	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> > +		pll = hsw_ddi_dp_get_dpll(encoder, clock);
> > +	}
> > +	return pll;
> > +}
> > +
> >  void intel_ddi_init(struct drm_device *dev, enum port port)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > index 9a1da98..4b067ac 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > @@ -24,6 +24,44 @@
> >  #include "intel_drv.h"
> >  
> >  struct intel_shared_dpll *
> > +skl_find_link_pll(struct drm_i915_private *dev_priv, int clock)
> > +{
> > +	struct intel_shared_dpll *pll = NULL;
> > +	struct intel_dpll_hw_state dpll_hw_state;
> > +	enum intel_dpll_id i;
> > +	bool found = false;
> > +
> > +	if (!skl_ddi_dp_set_dpll_hw_state(clock, &dpll_hw_state))
> > +		return pll;
> > +
> > +	for (i = DPLL_ID_SKL_DPLL1; i <= DPLL_ID_SKL_DPLL3; i++) {
> > +		pll = &dev_priv->shared_dplls[i];
> > +
> > +		/* Only want to check enabled timings first */
> > +		if (pll->config.crtc_mask == 0)
> > +			continue;
> > +
> > +		if (memcmp(&dpll_hw_state, &pll->config.hw_state,
> > +			   sizeof(pll->config.hw_state)) == 0) {
> > +			found = true;
> > +			break;
> > +		}
> > +	}
> > +
> > +	/* Ok no matching timings, maybe there's a free one? */
> > +	for (i = DPLL_ID_SKL_DPLL1;
> > +	     ((found == false) && (i <= DPLL_ID_SKL_DPLL3)); i++) {
> > +		pll = &dev_priv->shared_dplls[i];
> > +		if (pll->config.crtc_mask == 0) {
> > +			pll->config.hw_state = dpll_hw_state;
> > +			break;
> > +		}
> > +	}
> > +
> > +	return pll;
> > +}
> > +
> > +struct intel_shared_dpll *
> >  intel_get_shared_dpll_by_id(struct drm_i915_private *dev_priv,
> >  			    enum intel_dpll_id id)
> >  {
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > index aed7408..f438535 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > @@ -168,6 +168,8 @@ bool bxt_ddi_dp_set_dpll_hw_state(int clock,
> >  /* SKL dpll related functions */
> >  bool skl_ddi_dp_set_dpll_hw_state(int clock,
> >  				  struct intel_dpll_hw_state *dpll_hw_state);
> > +struct intel_shared_dpll *skl_find_link_pll(struct drm_i915_private *dev_priv,
> > +					    int clock);
> >  
> > 
> >  /* HSW dpll related functions */
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 529fa7b..efcd80b 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1159,7 +1159,8 @@ void intel_ddi_clock_get(struct intel_encoder *encoder,
> >  			 struct intel_crtc_state *pipe_config);
> >  void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state);
> >  uint32_t ddi_signal_levels(struct intel_dp *intel_dp);
> > -
> > +struct intel_shared_dpll *intel_ddi_get_link_dpll(struct intel_dp *intel_dp,
> > +						  int clock);
> >  unsigned int intel_fb_align_height(struct drm_device *dev,
> >  				   unsigned int height,
> >  				   uint32_t pixel_format,
>

Patch

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index e4b875e..67a6a0b 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2393,6 +2393,44 @@  intel_ddi_init_hdmi_connector(struct intel_digital_port *intel_dig_port)
 	return connector;
 }
 
+struct intel_shared_dpll *
+intel_ddi_get_link_dpll(struct intel_dp *intel_dp, int clock)
+{
+	struct intel_connector *connector = intel_dp->attached_connector;
+	struct intel_encoder *encoder = connector->encoder;
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct intel_shared_dpll *pll = NULL;
+	struct intel_shared_dpll_config tmp_pll_config;
+	enum intel_dpll_id dpll_id;
+
+	if (IS_BROXTON(dev_priv)) {
+		dpll_id =  (enum intel_dpll_id)dig_port->port;
+		/*
+		 * Select the required PLL. This works for platforms where
+		 * there is no shared DPLL.
+		 */
+		pll = &dev_priv->shared_dplls[dpll_id];
+		if (WARN_ON(pll->active_mask)) {
+
+			DRM_ERROR("Shared DPLL in use. active_mask:%x\n",
+				  pll->active_mask);
+			pll = NULL;
+		}
+		tmp_pll_config = pll->config;
+		if (!bxt_ddi_dp_set_dpll_hw_state(clock,
+						  &pll->config.hw_state)) {
+			DRM_ERROR("Could not setup DPLL\n");
+			pll->config = tmp_pll_config;
+		}
+	} else if (IS_SKYLAKE(dev_priv)) {
+		pll = skl_find_link_pll(dev_priv, clock);
+	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
+		pll = hsw_ddi_dp_get_dpll(encoder, clock);
+	}
+	return pll;
+}
+
 void intel_ddi_init(struct drm_device *dev, enum port port)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index 9a1da98..4b067ac 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -24,6 +24,44 @@ 
 #include "intel_drv.h"
 
 struct intel_shared_dpll *
+skl_find_link_pll(struct drm_i915_private *dev_priv, int clock)
+{
+	struct intel_shared_dpll *pll = NULL;
+	struct intel_dpll_hw_state dpll_hw_state;
+	enum intel_dpll_id i;
+	bool found = false;
+
+	if (!skl_ddi_dp_set_dpll_hw_state(clock, &dpll_hw_state))
+		return pll;
+
+	for (i = DPLL_ID_SKL_DPLL1; i <= DPLL_ID_SKL_DPLL3; i++) {
+		pll = &dev_priv->shared_dplls[i];
+
+		/* Only want to check enabled timings first */
+		if (pll->config.crtc_mask == 0)
+			continue;
+
+		if (memcmp(&dpll_hw_state, &pll->config.hw_state,
+			   sizeof(pll->config.hw_state)) == 0) {
+			found = true;
+			break;
+		}
+	}
+
+	/* Ok no matching timings, maybe there's a free one? */
+	for (i = DPLL_ID_SKL_DPLL1;
+	     ((found == false) && (i <= DPLL_ID_SKL_DPLL3)); i++) {
+		pll = &dev_priv->shared_dplls[i];
+		if (pll->config.crtc_mask == 0) {
+			pll->config.hw_state = dpll_hw_state;
+			break;
+		}
+	}
+
+	return pll;
+}
+
+struct intel_shared_dpll *
 intel_get_shared_dpll_by_id(struct drm_i915_private *dev_priv,
 			    enum intel_dpll_id id)
 {
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
index aed7408..f438535 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
@@ -168,6 +168,8 @@  bool bxt_ddi_dp_set_dpll_hw_state(int clock,
 /* SKL dpll related functions */
 bool skl_ddi_dp_set_dpll_hw_state(int clock,
 				  struct intel_dpll_hw_state *dpll_hw_state);
+struct intel_shared_dpll *skl_find_link_pll(struct drm_i915_private *dev_priv,
+					    int clock);
 
 
 /* HSW dpll related functions */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 529fa7b..efcd80b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1159,7 +1159,8 @@  void intel_ddi_clock_get(struct intel_encoder *encoder,
 			 struct intel_crtc_state *pipe_config);
 void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state);
 uint32_t ddi_signal_levels(struct intel_dp *intel_dp);
-
+struct intel_shared_dpll *intel_ddi_get_link_dpll(struct intel_dp *intel_dp,
+						  int clock);
 unsigned int intel_fb_align_height(struct drm_device *dev,
 				   unsigned int height,
 				   uint32_t pixel_format,