On Fri, Jun 14, 2019 at 10:22:05PM +0300, Ville Syrjälä wrote:
> 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 ?
I think DPLL_NO_DC or DPLL_DC_OFF is ok, we could rename it to be more
generic later if a new user similar to this one comes up.
>
> 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?
I think it's better to keep it for the use of modeset alone, for
debugging purposes.
>
> > 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?
Hm, yea, we'd need to get a reference for those cases from
intel_modeset_setup_hw_state().
>
> > + 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
