Re: [PATCH 08/13] drm/i915: Refactor platform specifics out of intel_get_shared_dpll()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Op 04-03-16 om 07:49 schreef Ander Conselvan De Oliveira:
> On Thu, 2016-03-03 at 15:08 +0100, Maarten Lankhorst wrote:
>> Op 26-02-16 om 14:54 schreef Ander Conselvan de Oliveira:
>>> The function intel_get_shared_dpll() had a more or less generic
>>> implementation with some platform specific checks to handle smaller
>>> differences between platforms. However, the minimalist approach forces
>>> bigger differences between platforms to be implemented outside of the
>>> shared dpll code (see the *_ddi_pll_select() functions in intel_ddi.c,
>>> for instance).
>>>
>>> This patch changes the implementation of intel_get_share_dpll() so that
>>> a completely platform specific version can be used, providing helpers to
>>> reduce code duplication. This should allow the code from the ddi pll
>>> select functions to be moved, and also make room for making more dplls
>>> managed by the shared dpll infrastructure.
>>>
>>> Signed-off-by: Ander Conselvan de Oliveira <
>>> ander.conselvan.de.oliveira@xxxxxxxxx>
>>> ---
>>>  drivers/gpu/drm/i915/i915_drv.h       |   1 +
>>>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 226 +++++++++++++++++++++----------
>>> ---
>>>  drivers/gpu/drm/i915/intel_dpll_mgr.h |   2 +
>>>  3 files changed, 145 insertions(+), 84 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>> b/drivers/gpu/drm/i915/i915_drv.h
>>> index 6de93dc..b858801 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -1802,6 +1802,7 @@ struct drm_i915_private {
>>>  	/* dpll and cdclk state is protected by connection_mutex */
>>>  	int num_shared_dpll;
>>>  	struct intel_shared_dpll shared_dplls[I915_NUM_PLLS];
>>> +	const struct intel_dpll_mgr *dpll_mgr;
>>>  
>>>  	unsigned int active_crtcs;
>>>  	unsigned int min_pixclk[I915_MAX_PIPES];
>>> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
>>> b/drivers/gpu/drm/i915/intel_dpll_mgr.c
>>> index e88dc46..3553324 100644
>>> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
>>> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
>>> @@ -174,66 +174,20 @@ void intel_disable_shared_dpll(struct intel_crtc
>>> *crtc)
>>>  	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
>>>  }
>>>  
>>> -static enum intel_dpll_id
>>> -ibx_get_fixed_dpll(struct intel_crtc *crtc,
>>> -		   struct intel_crtc_state *crtc_state)
>>> -{
>>> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>> -	struct intel_shared_dpll *pll;
>>> -	enum intel_dpll_id i;
>>> -
>>> -	/* Ironlake PCH has a fixed PLL->PCH pipe mapping. */
>>> -	i = (enum intel_dpll_id) crtc->pipe;
>>> -	pll = &dev_priv->shared_dplls[i];
>>> -
>>> -	DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n",
>>> -		      crtc->base.base.id, pll->name);
>>> -
>>> -	return i;
>>> -}
>>> -
>>> -static enum intel_dpll_id
>>> -bxt_get_fixed_dpll(struct intel_crtc *crtc,
>>> -		   struct intel_crtc_state *crtc_state)
>>> -{
>>> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>> -	struct intel_encoder *encoder;
>>> -	struct intel_digital_port *intel_dig_port;
>>> -	struct intel_shared_dpll *pll;
>>> -	enum intel_dpll_id i;
>>> -
>>> -	/* PLL is attached to port in bxt */
>>> -	encoder = intel_ddi_get_crtc_new_encoder(crtc_state);
>>> -	if (WARN_ON(!encoder))
>>> -		return DPLL_ID_PRIVATE;
>>> -
>>> -	intel_dig_port = enc_to_dig_port(&encoder->base);
>>> -	/* 1:1 mapping between ports and PLLs */
>>> -	i = (enum intel_dpll_id)intel_dig_port->port;
>>> -	pll = &dev_priv->shared_dplls[i];
>>> -	DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n",
>>> -		crtc->base.base.id, pll->name);
>>> -
>>> -	return i;
>>> -}
>>> -
>>> -static enum intel_dpll_id
>>> +static struct intel_shared_dpll *
>>>  intel_find_shared_dpll(struct intel_crtc *crtc,
>>> -		       struct intel_crtc_state *crtc_state)
>>> +		       struct intel_crtc_state *crtc_state,
>>> +		       enum intel_dpll_id range_min,
>>> +		       enum intel_dpll_id range_max)
>>>  {
>>>  	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>>>  	struct intel_shared_dpll *pll;
>>>  	struct intel_shared_dpll_config *shared_dpll;
>>>  	enum intel_dpll_id i;
>>> -	int max = dev_priv->num_shared_dpll;
>>> -
>>> -	if (INTEL_INFO(dev_priv)->gen < 9 && HAS_DDI(dev_priv))
>>> -		/* Do not consider SPLL */
>>> -		max = 2;
>>>  
>>>  	shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state
>>> ->base.state);
>>>  
>>> -	for (i = 0; i < max; i++) {
>>> +	for (i = range_min; i <= range_max; i++) {
>>>  		pll = &dev_priv->shared_dplls[i];
>>>  
>>>  		/* Only want to check enabled timings first */
>>> @@ -247,49 +201,33 @@ intel_find_shared_dpll(struct intel_crtc *crtc,
>>>  				      crtc->base.base.id, pll->name,
>>>  				      shared_dpll[i].crtc_mask,
>>>  				      pll->active);
>>> -			return i;
>>> +			return pll;
>>>  		}
>>>  	}
>>>  
>>>  	/* Ok no matching timings, maybe there's a free one? */
>>> -	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>>> +	for (i = range_min; i <= range_max; i++) {
>>>  		pll = &dev_priv->shared_dplls[i];
>>>  		if (shared_dpll[i].crtc_mask == 0) {
>>>  			DRM_DEBUG_KMS("CRTC:%d allocated %s\n",
>>>  				      crtc->base.base.id, pll->name);
>>> -			return i;
>>> +			return pll;
>>>  		}
>>>  	}
>>>  
>>> -	return DPLL_ID_PRIVATE;
>>> +	return NULL;
>>>  }
>>>  
>>> -struct intel_shared_dpll *
>>> -intel_get_shared_dpll(struct intel_crtc *crtc,
>>> -		      struct intel_crtc_state *crtc_state)
>>> +static void
>>> +intel_reference_shared_dpll(struct intel_shared_dpll *pll,
>>> +			    struct intel_crtc_state *crtc_state)
>>>  {
>>> -	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>>> -	struct intel_shared_dpll *pll;
>>>  	struct intel_shared_dpll_config *shared_dpll;
>>> -	enum intel_dpll_id i;
>>> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>>> +	enum intel_dpll_id i = pll->id;
>>>  
>>>  	shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state
>>> ->base.state);
>>>  
>>> -	if (HAS_PCH_IBX(dev_priv->dev)) {
>>> -		i = ibx_get_fixed_dpll(crtc, crtc_state);
>>> -		WARN_ON(shared_dpll[i].crtc_mask);
>>> -	} else if (IS_BROXTON(dev_priv->dev)) {
>>> -		i = bxt_get_fixed_dpll(crtc, crtc_state);
>>> -		WARN_ON(shared_dpll[i].crtc_mask);
>>> -	} else {
>>> -		i = intel_find_shared_dpll(crtc, crtc_state);
>>> -	}
>>> -
>>> -	if (i < 0)
>>> -		return NULL;
>>> -
>>> -	pll = &dev_priv->shared_dplls[i];
>>> -
>>>  	if (shared_dpll[i].crtc_mask == 0)
>>>  		shared_dpll[i].hw_state =
>>>  			crtc_state->dpll_hw_state;
>>> @@ -299,8 +237,6 @@ intel_get_shared_dpll(struct intel_crtc *crtc,
>>>  			 pipe_name(crtc->pipe));
>>>  
>>>  	intel_shared_dpll_config_get(shared_dpll, pll, crtc);
>>> -
>>> -	return pll;
>>>  }
>>>  
>>>  void intel_shared_dpll_commit(struct drm_atomic_state *state)
>>> @@ -398,6 +334,32 @@ static void ibx_pch_dpll_disable(struct
>>> drm_i915_private *dev_priv,
>>>  	udelay(200);
>>>  }
>>>  
>>> +static struct intel_shared_dpll *
>>> +ibx_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state)
>>> +{
>>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>> +	struct intel_shared_dpll *pll;
>>> +	enum intel_dpll_id i;
>>> +
>>> +	if (HAS_PCH_IBX(dev_priv)) {
>>> +		/* Ironlake PCH has a fixed PLL->PCH pipe mapping. */
>>> +		i = (enum intel_dpll_id) crtc->pipe;
>>> +		pll = &dev_priv->shared_dplls[i];
>>> +
>>> +		DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n",
>>> +			      crtc->base.base.id, pll->name);
>>> +	} else {
>>> +		pll = intel_find_shared_dpll(crtc, crtc_state,
>>> +					     DPLL_ID_PCH_PLL_A,
>>> +					     DPLL_ID_PCH_PLL_B);
>>> +	}
>>> +
>>> +	/* reference the pll */
>>> +	intel_reference_shared_dpll(pll, crtc_state);
>>> +
>>> +	return pll;
>>> +}
>>> +
>>>  static const struct intel_shared_dpll_funcs ibx_pch_dpll_funcs = {
>>>  	.mode_set = ibx_pch_dpll_mode_set,
>>>  	.enable = ibx_pch_dpll_enable,
>>> @@ -475,6 +437,19 @@ static bool hsw_ddi_spll_get_hw_state(struct
>>> drm_i915_private *dev_priv,
>>>  	return val & SPLL_PLL_ENABLE;
>>>  }
>>>  
>>> +static struct intel_shared_dpll *
>>> +hsw_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state)
>>> +{
>>> +	struct intel_shared_dpll *pll;
>>> +
>>> +	pll = intel_find_shared_dpll(crtc, crtc_state,
>>> +				     DPLL_ID_WRPLL1, DPLL_ID_WRPLL2);
>>> +	if (pll)
>>> +		intel_reference_shared_dpll(pll, crtc_state);
>>> +
>>> +	return pll;
>>> +}
>>> +
>>>  
>>>  static const struct intel_shared_dpll_funcs hsw_ddi_wrpll_funcs = {
>>>  	.enable = hsw_ddi_wrpll_enable,
>>> @@ -594,6 +569,19 @@ out:
>>>  	return ret;
>>>  }
>>>  
>>> +static struct intel_shared_dpll *
>>> +skl_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state)
>>> +{
>>> +	struct intel_shared_dpll *pll;
>>> +
>>> +	pll = intel_find_shared_dpll(crtc, crtc_state,
>>> +				     DPLL_ID_SKL_DPLL1, DPLL_ID_SKL_DPLL3);
>>> +	if (pll)
>>> +		intel_reference_shared_dpll(pll, crtc_state);
>>> +
>>> +	return pll;
>>> +}
>>> +
>>>  static const struct intel_shared_dpll_funcs skl_ddi_pll_funcs = {
>>>  	.enable = skl_ddi_pll_enable,
>>>  	.disable = skl_ddi_pll_disable,
>>> @@ -782,6 +770,32 @@ out:
>>>  	return ret;
>>>  }
>>>  
>>> +static struct intel_shared_dpll *
>>> +bxt_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state)
>>> +{
>>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>> +	struct intel_encoder *encoder;
>>> +	struct intel_digital_port *intel_dig_port;
>>> +	struct intel_shared_dpll *pll;
>>> +	enum intel_dpll_id i;
>>> +
>>> +	/* PLL is attached to port in bxt */
>>> +	encoder = intel_ddi_get_crtc_new_encoder(crtc_state);
>>> +	if (WARN_ON(!encoder))
>>> +		return NULL;
>>> +
>>> +	intel_dig_port = enc_to_dig_port(&encoder->base);
>>> +	/* 1:1 mapping between ports and PLLs */
>>> +	i = (enum intel_dpll_id)intel_dig_port->port;
>>> +	pll = &dev_priv->shared_dplls[i];
>>> +	DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n",
>>> +		crtc->base.base.id, pll->name);
>>> +
>>> +	intel_reference_shared_dpll(pll, crtc_state);
>>> +
>>> +	return pll;
>>> +}
>>> +
>>>  static const struct intel_shared_dpll_funcs bxt_ddi_pll_funcs = {
>>>  	.enable = bxt_ddi_pll_enable,
>>>  	.disable = bxt_ddi_pll_disable,
>>> @@ -826,12 +840,24 @@ struct dpll_info {
>>>  	const struct intel_shared_dpll_funcs *funcs;
>>>  };
>>>  
>>> +struct intel_dpll_mgr {
>>> +	const struct dpll_info *dpll_info;
>>> +
>>> +	struct intel_shared_dpll *(*get_dpll)(struct intel_crtc *crtc,
>>> +					      struct intel_crtc_state
>>> *crtc_state);
>>> +};
>>> +
>>>  static const struct dpll_info pch_plls[] = {
>>>  	{ "PCH DPLL A", DPLL_ID_PCH_PLL_A, &ibx_pch_dpll_funcs },
>>>  	{ "PCH DPLL B", DPLL_ID_PCH_PLL_B, &ibx_pch_dpll_funcs },
>>>  	{ NULL, -1, NULL },
>>>  };
>>>  
>>> +static const struct intel_dpll_mgr pch_pll_mgr = {
>>> +	.dpll_info = pch_plls,
>>> +	.get_dpll = ibx_get_dpll,
>>> +};
>>> +
>>>  static const struct dpll_info hsw_plls[] = {
>>>  	{ "WRPLL 1", DPLL_ID_WRPLL1, &hsw_ddi_wrpll_funcs },
>>>  	{ "WRPLL 2", DPLL_ID_WRPLL2, &hsw_ddi_wrpll_funcs },
>>> @@ -839,6 +865,11 @@ static const struct dpll_info hsw_plls[] = {
>>>  	{ NULL, -1, NULL, },
>>>  };
>>>  
>>> +static const struct intel_dpll_mgr hsw_pll_mgr = {
>>> +	.dpll_info = hsw_plls,
>>> +	.get_dpll = hsw_get_dpll,
>>> +};
>>> +
>>>  static const struct dpll_info skl_plls[] = {
>>>  	{ "DPPL 1", DPLL_ID_SKL_DPLL1, &skl_ddi_pll_funcs },
>>>  	{ "DPPL 2", DPLL_ID_SKL_DPLL2, &skl_ddi_pll_funcs },
>>> @@ -846,6 +877,11 @@ static const struct dpll_info skl_plls[] = {
>>>  	{ NULL, -1, NULL, },
>>>  };
>>>  
>>> +static const struct intel_dpll_mgr skl_pll_mgr = {
>>> +	.dpll_info = skl_plls,
>>> +	.get_dpll = skl_get_dpll,
>>> +};
>>> +
>>>  static const struct dpll_info bxt_plls[] = {
>>>  	{ "PORT PLL A", 0, &bxt_ddi_pll_funcs },
>>>  	{ "PORT PLL B", 1, &bxt_ddi_pll_funcs },
>>> @@ -853,26 +889,34 @@ static const struct dpll_info bxt_plls[] = {
>>>  	{ NULL, -1, NULL, },
>>>  };
>>>  
>>> +static const struct intel_dpll_mgr bxt_pll_mgr = {
>>> +	.dpll_info = bxt_plls,
>>> +	.get_dpll = bxt_get_dpll,
>>> +};
>>> +
>>>  void intel_shared_dpll_init(struct drm_device *dev)
>>>  {
>>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>> -	const struct dpll_info *dpll_info = NULL;
>>> +	const struct intel_dpll_mgr *dpll_mgr = NULL;
>>> +	const struct dpll_info *dpll_info;
>>>  	int i;
>>>  
>>>  	if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev))
>>> -		dpll_info = skl_plls;
>>> +		dpll_mgr = &skl_pll_mgr;
>>>  	else if IS_BROXTON(dev)
>>> -		dpll_info = bxt_plls;
>>> +		dpll_mgr = &bxt_pll_mgr;
>>>  	else if (HAS_DDI(dev))
>>> -		dpll_info = hsw_plls;
>>> +		dpll_mgr = &hsw_pll_mgr;
>>>  	else if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
>>> -		dpll_info = pch_plls;
>>> +		dpll_mgr = &pch_pll_mgr;
>>>  
>>> -	if (!dpll_info) {
>>> +	if (!dpll_mgr) {
>>>  		dev_priv->num_shared_dpll = 0;
>>>  		return;
>>>  	}
>>>  
>>> +	dpll_info = dpll_mgr->dpll_info;
>>> +
>>>  	for (i = 0; dpll_info[i].id >= 0; i++) {
>>>  		WARN_ON(i != dpll_info[i].id);
>>>  
>>> @@ -881,6 +925,7 @@ void intel_shared_dpll_init(struct drm_device *dev)
>>>  		dev_priv->shared_dplls[i].funcs = *dpll_info[i].funcs;
>>>  	}
>>>  
>>> +	dev_priv->dpll_mgr = dpll_mgr;
>>>  	dev_priv->num_shared_dpll = i;
>>>  
>>>  	BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS);
>>> @@ -889,3 +934,16 @@ void intel_shared_dpll_init(struct drm_device *dev)
>>>  	if (HAS_DDI(dev))
>>>  		intel_ddi_pll_init(dev);
>>>  }
>>> +
>>> +struct intel_shared_dpll *
>>> +intel_get_shared_dpll(struct intel_crtc *crtc,
>>> +		      struct intel_crtc_state *crtc_state)
>>> +{
>>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>> +	const struct intel_dpll_mgr *dpll_mgr = dev_priv->dpll_mgr;
>>> +
>>> +	if (dpll_mgr)
>>> +		return dpll_mgr->get_dpll(crtc, crtc_state);
>>> +	else
>>> +		return NULL;
>>> +}
>> should there be a WARN_ON here?
>>
>> Rest looks good, so for patch 8...11/13:
> Did you have comments on patch 12? I seem to have missed it.
>
No, I didn't get around to looking yet.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux