On Fri, Jun 07, 2019 at 04:23:26PM -0700, Vivek Kasireddy wrote:
> This patch adds support for DPLL4 on EHL that include the
> following restrictions:
>
> - DPLL4 cannot be used with DDIA (combo port A internal eDP usage).
> DPLL4 can be used with other DDIs, including DDID
> (combo port A external usage).
>
> - DPLL4 cannot be enabled when DC5 or DC6 are enabled.
>
> - The DPLL4 enable, lock, power enabled, and power state are connected
> to the MGPLL1_ENABLE register.
>
> v2: (suggestions from Bob Paauwe)
> - Rework ehl_get_dpll() function to call intel_find_shared_dpll() and
> iterate twice: once for Combo plls and once for MG plls.
>
> - Use MG pll funcs for DPLL4 instead of creating new ones and modify
> mg_pll_enable to include the restrictions for EHL.
>
> v3: Fix compilation error
>
> v4: (suggestions from Lucas and Ville)
> - Treat DPLL4 as a combo phy PLL and not as MG PLL
> - Disable DC states when this DPLL is being enabled
> - Reuse icl_get_dpll instead of creating a separate one for EHL
>
> v5: (suggestion from Ville)
> - Refcount the DC OFF power domains during the enabling and disabling
> of this DPLL.
>
> v6: rebase
>
> v7: (suggestion from Imre)
> - Add a new power domain instead of iterating over the domains
> assoicated with DC OFF power well.
>
> Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> Cc: José Roberto de Souza <jose.souza@xxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_display_power.c | 3 ++
> drivers/gpu/drm/i915/intel_display_power.h | 1 +
> drivers/gpu/drm/i915/intel_dpll_mgr.c | 44 ++++++++++++++++++++--
> drivers/gpu/drm/i915/intel_dpll_mgr.h | 6 +++
> 4 files changed, 51 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display_power.c b/drivers/gpu/drm/i915/intel_display_power.c
> index 278a7edc94f5..2134d8b43f58 100644
> --- a/drivers/gpu/drm/i915/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/intel_display_power.c
> @@ -116,6 +116,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
> return "MODESET";
> case POWER_DOMAIN_GT_IRQ:
> return "GT_IRQ";
> + case POWER_DOMAIN_DPLL4:
> + return "DPLL4";
> default:
> MISSING_CASE(domain);
> return "?";
> @@ -2357,6 +2359,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> ICL_PW_2_POWER_DOMAINS | \
> BIT_ULL(POWER_DOMAIN_MODESET) | \
> BIT_ULL(POWER_DOMAIN_AUX_A) | \
> + BIT_ULL(POWER_DOMAIN_DPLL4) | \
> BIT_ULL(POWER_DOMAIN_INIT))
>
> #define ICL_DDI_IO_A_POWER_DOMAINS ( \
> diff --git a/drivers/gpu/drm/i915/intel_display_power.h b/drivers/gpu/drm/i915/intel_display_power.h
> index ff57b0a7fe59..cccc47266279 100644
> --- a/drivers/gpu/drm/i915/intel_display_power.h
> +++ b/drivers/gpu/drm/i915/intel_display_power.h
> @@ -59,6 +59,7 @@ enum intel_display_power_domain {
> POWER_DOMAIN_GMBUS,
> POWER_DOMAIN_MODESET,
> POWER_DOMAIN_GT_IRQ,
> + POWER_DOMAIN_DPLL4,
Probably should give this a bit more abstract name. _DPLL_NO_DC ?
Or should we just make it a generic _NO_DC power domain?
Though it's a bit annoying that we leak the DC yes vs. no into power
domains which are supposed to be pretty abstract.
Hmm. Or we could just abuse the MODESET domain here as that's
pretty much the "make sure my PLLs aren't shut down" domain?
> POWER_DOMAIN_INIT,
>
> POWER_DOMAIN_NUM,
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index 69787f259677..3d712f54dc56 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -2806,6 +2806,12 @@ icl_get_dpll(struct intel_crtc_state *crtc_state,
> if (intel_port_is_combophy(dev_priv, port)) {
> min = DPLL_ID_ICL_DPLL0;
> max = DPLL_ID_ICL_DPLL1;
> +
> + if (IS_ELKHARTLAKE(dev_priv)) {
> + if (encoder->type != INTEL_OUTPUT_EDP)
> + max = DPLL_ID_EHL_DPLL4;
I think we want
IS_EHL && port != PORT_A
> + }
> +
> ret = icl_calc_dpll_state(crtc_state, encoder);
> } else if (intel_port_is_tc(dev_priv, port)) {
> if (encoder->type == INTEL_OUTPUT_DP_MST) {
> @@ -2945,8 +2951,14 @@ static bool combo_pll_get_hw_state(struct drm_i915_private *dev_priv,
> struct intel_shared_dpll *pll,
> struct intel_dpll_hw_state *hw_state)
> {
> - return icl_pll_get_hw_state(dev_priv, pll, hw_state,
> - CNL_DPLL_ENABLE(pll->info->id));
> + i915_reg_t enable_reg = CNL_DPLL_ENABLE(pll->info->id);
> +
> + if (IS_ELKHARTLAKE(dev_priv) &&
> + pll->info->id == DPLL_ID_EHL_DPLL4) {
> + enable_reg = MG_PLL_ENABLE(0);
> + }
> +
> + return icl_pll_get_hw_state(dev_priv, pll, hw_state, enable_reg);
> }
>
> static bool tbt_pll_get_hw_state(struct drm_i915_private *dev_priv,
> @@ -3057,6 +3069,19 @@ static void combo_pll_enable(struct drm_i915_private *dev_priv,
> {
> i915_reg_t enable_reg = CNL_DPLL_ENABLE(pll->info->id);
>
> + if (IS_ELKHARTLAKE(dev_priv) &&
> + pll->info->id == DPLL_ID_EHL_DPLL4) {
> + enable_reg = MG_PLL_ENABLE(0);
> +
> + /*
> + * We need to disable DC states when this DPLL is enabled.
> + * This can be done by taking a reference on DPLL4 power
> + * domain.
> + */
> + pll->wakeref = intel_display_power_get(dev_priv,
> + POWER_DOMAIN_DPLL4);
> + }
> +
> icl_pll_power_enable(dev_priv, pll, enable_reg);
>
> icl_dpll_write(dev_priv, pll);
> @@ -3152,7 +3177,19 @@ static void icl_pll_disable(struct drm_i915_private *dev_priv,
> static void combo_pll_disable(struct drm_i915_private *dev_priv,
> struct intel_shared_dpll *pll)
> {
> - icl_pll_disable(dev_priv, pll, CNL_DPLL_ENABLE(pll->info->id));
> + i915_reg_t enable_reg = CNL_DPLL_ENABLE(pll->info->id);
> +
> + if (IS_ELKHARTLAKE(dev_priv) &&
> + pll->info->id == DPLL_ID_EHL_DPLL4) {
> + enable_reg = MG_PLL_ENABLE(0);
> + icl_pll_disable(dev_priv, pll, enable_reg);
> +
> + intel_display_power_put(dev_priv, POWER_DOMAIN_DPLL4,
> + pll->wakeref);
What happens if we didn't turn on the DPLL ourself?
Ie. the BIOS did it?
> + return;
> + }
> +
> + icl_pll_disable(dev_priv, pll, enable_reg);
> }
>
> static void tbt_pll_disable(struct drm_i915_private *dev_priv,
> @@ -3230,6 +3267,7 @@ static const struct intel_dpll_mgr icl_pll_mgr = {
> static const struct dpll_info ehl_plls[] = {
> { "DPLL 0", &combo_pll_funcs, DPLL_ID_ICL_DPLL0, 0 },
> { "DPLL 1", &combo_pll_funcs, DPLL_ID_ICL_DPLL1, 0 },
> + { "DPLL 4", &combo_pll_funcs, DPLL_ID_EHL_DPLL4, 0 },
> { },
> };
>
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> index b5dd9c7ad772..7358b1a525e1 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> @@ -28,6 +28,7 @@
> #include <linux/types.h>
>
> #include "intel_display.h"
> +#include "intel_wakeref.h"
>
> /*FIXME: Move this to a more appropriate place. */
> #define abs_diff(a, b) ({ \
> @@ -117,6 +118,10 @@ enum intel_dpll_id {
> * @DPLL_ID_ICL_DPLL1: ICL combo PHY DPLL1
> */
> DPLL_ID_ICL_DPLL1 = 1,
> + /**
> + * @DPLL_ID_EHL_DPLL4: EHL combo PHY DPLL4
> + */
> + DPLL_ID_EHL_DPLL4 = 2,
> /**
> * @DPLL_ID_ICL_TBTPLL: ICL TBT PLL
> */
> @@ -312,6 +317,7 @@ struct intel_shared_dpll {
> * @info: platform specific info
> */
> const struct dpll_info *info;
> + intel_wakeref_t wakeref;
> };
>
> #define SKL_DPLL0 0
> --
> 2.21.0
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
