| Message ID | 20190405175953.7206-1-vivek.kasireddy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/ehl: Add support for DPLL4 (v3) | expand |
On Fri, Apr 05, 2019 at 10:59:53AM -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 > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> > Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com> > --- > drivers/gpu/drm/i915/intel_dpll_mgr.c | 60 ++++++++++++++++++++++++++++++++++- > 1 file changed, 59 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c > index e01c057ce50b..c3f0b9720c54 100644 > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c > @@ -2870,6 +2870,56 @@ icl_get_dpll(struct intel_crtc_state *crtc_state, > return pll; > } > > +static struct intel_shared_dpll * > +ehl_get_dpll(struct intel_crtc_state *crtc_state, > + struct intel_encoder *encoder) > +{ > + struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); > + struct intel_shared_dpll *pll; > + enum port port = encoder->port; > + enum intel_dpll_id min, max; > + bool ret; > + > + if (!intel_port_is_combophy(dev_priv, port)) { > + MISSING_CASE(port); > + return NULL; > + } > + > + min = DPLL_ID_ICL_DPLL0; > + max = DPLL_ID_ICL_DPLL1; > + ret = icl_calc_dpll_state(crtc_state, encoder); > + if (ret) { > + pll = intel_find_shared_dpll(crtc_state, min, max); > + if (pll) { > + intel_reference_shared_dpll(pll, crtc_state); > + return pll; > + } > + } else { > + DRM_DEBUG_KMS("Could not calculate PLL state.\n"); > + } > + > + if (encoder->type == INTEL_OUTPUT_EDP) { > + DRM_DEBUG_KMS("Cannot use DPLL4 with EDP.\n"); > + return NULL; > + } > + > + min = max = DPLL_ID_ICL_MGPLL1; > + ret = icl_calc_mg_pll_state(crtc_state); > + if (!ret) { > + DRM_DEBUG_KMS("Could not calculate PLL state.\n"); > + return NULL; > + } > + > + pll = intel_find_shared_dpll(crtc_state, min, max); > + if (!pll) { > + DRM_DEBUG_KMS("No PLL selected\n"); > + return NULL; > + } > + > + intel_reference_shared_dpll(pll, crtc_state); > + return pll; > +} > + > static bool mg_pll_get_hw_state(struct drm_i915_private *dev_priv, > struct intel_shared_dpll *pll, > struct intel_dpll_hw_state *hw_state) > @@ -3115,6 +3165,13 @@ static void mg_pll_enable(struct drm_i915_private *dev_priv, > i915_reg_t enable_reg = > MG_PLL_ENABLE(icl_pll_id_to_tc_port(pll->info->id)); > > + if (IS_ELKHARTLAKE(dev_priv) && > + (I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5 || > + I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6)) { > + DRM_ERROR("Cant enable DPLL4 when DC5 or DC6 are enabled\n"); > + return; > + } This looks like dead code, or we messed up earlier already (in which case it should just be some kind of assert IMO). Also what is this EHL thing anyway? I can't even find it in the spec. > + > icl_pll_power_enable(dev_priv, pll, enable_reg); > > icl_mg_pll_write(dev_priv, pll); > @@ -3249,12 +3306,13 @@ 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", &mg_pll_funcs, DPLL_ID_ICL_MGPLL1,0 }, > { }, > }; > > static const struct intel_dpll_mgr ehl_pll_mgr = { > .dpll_info = ehl_plls, > - .get_dpll = icl_get_dpll, > + .get_dpll = ehl_get_dpll, > .dump_hw_state = icl_dump_hw_state, > }; > > -- > 2.14.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Apr 05, 2019 at 09:33:56PM +0300, Ville Syrjälä wrote: > On Fri, Apr 05, 2019 at 10:59:53AM -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 > > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> > > Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dpll_mgr.c | 60 ++++++++++++++++++++++++++++++++++- > > 1 file changed, 59 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c > > index e01c057ce50b..c3f0b9720c54 100644 > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c > > @@ -2870,6 +2870,56 @@ icl_get_dpll(struct intel_crtc_state *crtc_state, > > return pll; > > } > > > > +static struct intel_shared_dpll * > > +ehl_get_dpll(struct intel_crtc_state *crtc_state, > > + struct intel_encoder *encoder) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); > > + struct intel_shared_dpll *pll; > > + enum port port = encoder->port; > > + enum intel_dpll_id min, max; > > + bool ret; > > + > > + if (!intel_port_is_combophy(dev_priv, port)) { > > + MISSING_CASE(port); > > + return NULL; > > + } > > + > > + min = DPLL_ID_ICL_DPLL0; > > + max = DPLL_ID_ICL_DPLL1; > > + ret = icl_calc_dpll_state(crtc_state, encoder); > > + if (ret) { > > + pll = intel_find_shared_dpll(crtc_state, min, max); > > + if (pll) { > > + intel_reference_shared_dpll(pll, crtc_state); > > + return pll; > > + } > > + } else { > > + DRM_DEBUG_KMS("Could not calculate PLL state.\n"); > > + } > > + > > + if (encoder->type == INTEL_OUTPUT_EDP) { > > + DRM_DEBUG_KMS("Cannot use DPLL4 with EDP.\n"); > > + return NULL; > > + } > > + > > + min = max = DPLL_ID_ICL_MGPLL1; > > + ret = icl_calc_mg_pll_state(crtc_state); > > + if (!ret) { > > + DRM_DEBUG_KMS("Could not calculate PLL state.\n"); > > + return NULL; > > + } > > + > > + pll = intel_find_shared_dpll(crtc_state, min, max); > > + if (!pll) { > > + DRM_DEBUG_KMS("No PLL selected\n"); > > + return NULL; > > + } > > + > > + intel_reference_shared_dpll(pll, crtc_state); > > + return pll; > > +} > > + > > static bool mg_pll_get_hw_state(struct drm_i915_private *dev_priv, > > struct intel_shared_dpll *pll, > > struct intel_dpll_hw_state *hw_state) > > @@ -3115,6 +3165,13 @@ static void mg_pll_enable(struct drm_i915_private *dev_priv, > > i915_reg_t enable_reg = > > MG_PLL_ENABLE(icl_pll_id_to_tc_port(pll->info->id)); > > > > + if (IS_ELKHARTLAKE(dev_priv) && > > + (I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5 || > > + I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6)) { > > + DRM_ERROR("Cant enable DPLL4 when DC5 or DC6 are enabled\n"); > > + return; > > + } > > This looks like dead code, or we messed up earlier already (in which > case it should just be some kind of assert IMO). Does DMC handle other MG PLLs? I would have thought not, so if we keep the assert then it should perhaps be unconditional. Hmm. But aren't we still hodling the modeset power domain when this gets called? If so this is definitely dead code. > > Also what is this EHL thing anyway? I can't even find it in the spec. > > > + > > icl_pll_power_enable(dev_priv, pll, enable_reg); > > > > icl_mg_pll_write(dev_priv, pll); > > @@ -3249,12 +3306,13 @@ 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", &mg_pll_funcs, DPLL_ID_ICL_MGPLL1,0 }, > > { }, > > }; > > > > static const struct intel_dpll_mgr ehl_pll_mgr = { > > .dpll_info = ehl_plls, > > - .get_dpll = icl_get_dpll, > > + .get_dpll = ehl_get_dpll, > > .dump_hw_state = icl_dump_hw_state, > > }; > > > > -- > > 2.14.5 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel
On Fri, 5 Apr 2019 21:39:11 +0300 Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: Hi Ville, > On Fri, Apr 05, 2019 at 09:33:56PM +0300, Ville Syrjälä wrote: > > On Fri, Apr 05, 2019 at 10:59:53AM -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 > > > > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> > > > Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_dpll_mgr.c | 60 > > > ++++++++++++++++++++++++++++++++++- 1 file changed, 59 > > > insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c > > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c index > > > e01c057ce50b..c3f0b9720c54 100644 --- > > > a/drivers/gpu/drm/i915/intel_dpll_mgr.c +++ > > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c @@ -2870,6 +2870,56 @@ > > > icl_get_dpll(struct intel_crtc_state *crtc_state, return pll; > > > } > > > > > > +static struct intel_shared_dpll * > > > +ehl_get_dpll(struct intel_crtc_state *crtc_state, > > > + struct intel_encoder *encoder) > > > +{ > > > + struct drm_i915_private *dev_priv = > > > to_i915(crtc_state->base.crtc->dev); > > > + struct intel_shared_dpll *pll; > > > + enum port port = encoder->port; > > > + enum intel_dpll_id min, max; > > > + bool ret; > > > + > > > + if (!intel_port_is_combophy(dev_priv, port)) { > > > + MISSING_CASE(port); > > > + return NULL; > > > + } > > > + > > > + min = DPLL_ID_ICL_DPLL0; > > > + max = DPLL_ID_ICL_DPLL1; > > > + ret = icl_calc_dpll_state(crtc_state, encoder); > > > + if (ret) { > > > + pll = intel_find_shared_dpll(crtc_state, min, > > > max); > > > + if (pll) { > > > + intel_reference_shared_dpll(pll, > > > crtc_state); > > > + return pll; > > > + } > > > + } else { > > > + DRM_DEBUG_KMS("Could not calculate PLL > > > state.\n"); > > > + } > > > + > > > + if (encoder->type == INTEL_OUTPUT_EDP) { > > > + DRM_DEBUG_KMS("Cannot use DPLL4 with EDP.\n"); > > > + return NULL; > > > + } > > > + > > > + min = max = DPLL_ID_ICL_MGPLL1; > > > + ret = icl_calc_mg_pll_state(crtc_state); > > > + if (!ret) { > > > + DRM_DEBUG_KMS("Could not calculate PLL > > > state.\n"); > > > + return NULL; > > > + } > > > + > > > + pll = intel_find_shared_dpll(crtc_state, min, max); > > > + if (!pll) { > > > + DRM_DEBUG_KMS("No PLL selected\n"); > > > + return NULL; > > > + } > > > + > > > + intel_reference_shared_dpll(pll, crtc_state); > > > + return pll; > > > +} > > > + > > > static bool mg_pll_get_hw_state(struct drm_i915_private > > > *dev_priv, struct intel_shared_dpll *pll, > > > struct intel_dpll_hw_state > > > *hw_state) @@ -3115,6 +3165,13 @@ static void > > > mg_pll_enable(struct drm_i915_private *dev_priv, i915_reg_t > > > enable_reg = MG_PLL_ENABLE(icl_pll_id_to_tc_port(pll->info->id)); > > > > > > + if (IS_ELKHARTLAKE(dev_priv) && > > > + (I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5 || > > > + I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6)) { > > > + DRM_ERROR("Cant enable DPLL4 when DC5 or DC6 are > > > enabled\n"); > > > + return; > > > + } > > > > This looks like dead code, or we messed up earlier already (in which > > case it should just be some kind of assert IMO). Are you suggesting that I put this in a ->prepare() hook or much earlier in the atomic check/prepare phase? FYI, I am just adding a restriction mentioned in the spec concerning this DPLL. > > Does DMC handle other MG PLLs? I would have thought not, so if we keep > the assert then it should perhaps be unconditional. Hmm. But aren't we > still hodling the modeset power domain when this gets called? If so > this is definitely dead code. I am not sure whether DMC handles other MG PLLs but the spec explicitly says that DMC will not handle this PLL. Where do you suggest is an appropriate place to put this assert? > > > > > Also what is this EHL thing anyway? I can't even find it in the > > spec. EHL = Elkhart Lake, a new Gen 11 platform. Thanks, Vivek > > > + > > > icl_pll_power_enable(dev_priv, pll, enable_reg); > > > > > > icl_mg_pll_write(dev_priv, pll); > > > @@ -3249,12 +3306,13 @@ 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", &mg_pll_funcs, DPLL_ID_ICL_MGPLL1,0 }, > > > { }, > > > }; > > > > > > static const struct intel_dpll_mgr ehl_pll_mgr = { > > > .dpll_info = ehl_plls, > > > - .get_dpll = icl_get_dpll, > > > + .get_dpll = ehl_get_dpll, > > > .dump_hw_state = icl_dump_hw_state, > > > }; > > > > > > -- > > > 2.14.5 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrjälä > > Intel >
On Fri, Apr 05, 2019 at 10:59:53AM -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. ok > >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. these 2 don't match spec. "3rd PLL for use with combo PHY (DPLL4) and 3rd combo PHY DDI clocks (DDIC clock)" This is a combophy pll, not a mg phy pll. The only thing that is hooked to mg registers is the enable. So my understanding is that what you need: - use the dpll calculations - make sure intel_find_shared_dpll doesn't this if it's for eDP - setup the enable/disable to use MG_ENABLE register > >v3: Fix compilation error > >Cc: Lucas De Marchi <lucas.demarchi@intel.com> >Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> >Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com> >--- > drivers/gpu/drm/i915/intel_dpll_mgr.c | 60 ++++++++++++++++++++++++++++++++++- > 1 file changed, 59 insertions(+), 1 deletion(-) > >diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c >index e01c057ce50b..c3f0b9720c54 100644 >--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c >+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c >@@ -2870,6 +2870,56 @@ icl_get_dpll(struct intel_crtc_state *crtc_state, > return pll; > } > >+static struct intel_shared_dpll * >+ehl_get_dpll(struct intel_crtc_state *crtc_state, >+ struct intel_encoder *encoder) >+{ >+ struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); >+ struct intel_shared_dpll *pll; >+ enum port port = encoder->port; >+ enum intel_dpll_id min, max; >+ bool ret; >+ >+ if (!intel_port_is_combophy(dev_priv, port)) { >+ MISSING_CASE(port); >+ return NULL; >+ } >+ >+ min = DPLL_ID_ICL_DPLL0; >+ max = DPLL_ID_ICL_DPLL1; >+ ret = icl_calc_dpll_state(crtc_state, encoder); >+ if (ret) { >+ pll = intel_find_shared_dpll(crtc_state, min, max); >+ if (pll) { >+ intel_reference_shared_dpll(pll, crtc_state); >+ return pll; >+ } >+ } else { >+ DRM_DEBUG_KMS("Could not calculate PLL state.\n"); the check for ret is swapped and you are missing a return here. But given the comments above, I think it would be better to reuse icl_get_dpll() rather than what you are doing here. >+ } >+ >+ if (encoder->type == INTEL_OUTPUT_EDP) { >+ DRM_DEBUG_KMS("Cannot use DPLL4 with EDP.\n"); >+ return NULL; >+ } this is already too late >+ >+ min = max = DPLL_ID_ICL_MGPLL1; >+ ret = icl_calc_mg_pll_state(crtc_state); >+ if (!ret) { >+ DRM_DEBUG_KMS("Could not calculate PLL state.\n"); >+ return NULL; again... ret == 0 is success, not otherwise Lucas De Marchi >+ } >+ >+ pll = intel_find_shared_dpll(crtc_state, min, max); >+ if (!pll) { >+ DRM_DEBUG_KMS("No PLL selected\n"); >+ return NULL; >+ } >+ >+ intel_reference_shared_dpll(pll, crtc_state); >+ return pll; >+} >+ > static bool mg_pll_get_hw_state(struct drm_i915_private *dev_priv, > struct intel_shared_dpll *pll, > struct intel_dpll_hw_state *hw_state) >@@ -3115,6 +3165,13 @@ static void mg_pll_enable(struct drm_i915_private *dev_priv, > i915_reg_t enable_reg = > MG_PLL_ENABLE(icl_pll_id_to_tc_port(pll->info->id)); > >+ if (IS_ELKHARTLAKE(dev_priv) && >+ (I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5 || >+ I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6)) { >+ DRM_ERROR("Cant enable DPLL4 when DC5 or DC6 are enabled\n"); >+ return; >+ } >+ > icl_pll_power_enable(dev_priv, pll, enable_reg); > > icl_mg_pll_write(dev_priv, pll); >@@ -3249,12 +3306,13 @@ 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", &mg_pll_funcs, DPLL_ID_ICL_MGPLL1,0 }, > { }, > }; > > static const struct intel_dpll_mgr ehl_pll_mgr = { > .dpll_info = ehl_plls, >- .get_dpll = icl_get_dpll, >+ .get_dpll = ehl_get_dpll, > .dump_hw_state = icl_dump_hw_state, > }; > >-- >2.14.5 >
On Fri, Apr 05, 2019 at 04:33:30PM -0700, Vivek Kasireddy wrote: > On Fri, 5 Apr 2019 21:39:11 +0300 > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > Hi Ville, > > > On Fri, Apr 05, 2019 at 09:33:56PM +0300, Ville Syrjälä wrote: > > > On Fri, Apr 05, 2019 at 10:59:53AM -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 > > > > > > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> > > > > Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_dpll_mgr.c | 60 > > > > ++++++++++++++++++++++++++++++++++- 1 file changed, 59 > > > > insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c > > > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c index > > > > e01c057ce50b..c3f0b9720c54 100644 --- > > > > a/drivers/gpu/drm/i915/intel_dpll_mgr.c +++ > > > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c @@ -2870,6 +2870,56 @@ > > > > icl_get_dpll(struct intel_crtc_state *crtc_state, return pll; > > > > } > > > > > > > > +static struct intel_shared_dpll * > > > > +ehl_get_dpll(struct intel_crtc_state *crtc_state, > > > > + struct intel_encoder *encoder) > > > > +{ > > > > + struct drm_i915_private *dev_priv = > > > > to_i915(crtc_state->base.crtc->dev); > > > > + struct intel_shared_dpll *pll; > > > > + enum port port = encoder->port; > > > > + enum intel_dpll_id min, max; > > > > + bool ret; > > > > + > > > > + if (!intel_port_is_combophy(dev_priv, port)) { > > > > + MISSING_CASE(port); > > > > + return NULL; > > > > + } > > > > + > > > > + min = DPLL_ID_ICL_DPLL0; > > > > + max = DPLL_ID_ICL_DPLL1; > > > > + ret = icl_calc_dpll_state(crtc_state, encoder); > > > > + if (ret) { > > > > + pll = intel_find_shared_dpll(crtc_state, min, > > > > max); > > > > + if (pll) { > > > > + intel_reference_shared_dpll(pll, > > > > crtc_state); > > > > + return pll; > > > > + } > > > > + } else { > > > > + DRM_DEBUG_KMS("Could not calculate PLL > > > > state.\n"); > > > > + } > > > > + > > > > + if (encoder->type == INTEL_OUTPUT_EDP) { > > > > + DRM_DEBUG_KMS("Cannot use DPLL4 with EDP.\n"); > > > > + return NULL; > > > > + } > > > > + > > > > + min = max = DPLL_ID_ICL_MGPLL1; > > > > + ret = icl_calc_mg_pll_state(crtc_state); > > > > + if (!ret) { > > > > + DRM_DEBUG_KMS("Could not calculate PLL > > > > state.\n"); > > > > + return NULL; > > > > + } > > > > + > > > > + pll = intel_find_shared_dpll(crtc_state, min, max); > > > > + if (!pll) { > > > > + DRM_DEBUG_KMS("No PLL selected\n"); > > > > + return NULL; > > > > + } > > > > + > > > > + intel_reference_shared_dpll(pll, crtc_state); > > > > + return pll; > > > > +} > > > > + > > > > static bool mg_pll_get_hw_state(struct drm_i915_private > > > > *dev_priv, struct intel_shared_dpll *pll, > > > > struct intel_dpll_hw_state > > > > *hw_state) @@ -3115,6 +3165,13 @@ static void > > > > mg_pll_enable(struct drm_i915_private *dev_priv, i915_reg_t > > > > enable_reg = MG_PLL_ENABLE(icl_pll_id_to_tc_port(pll->info->id)); > > > > > > > > + if (IS_ELKHARTLAKE(dev_priv) && > > > > + (I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5 || > > > > + I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6)) { > > > > + DRM_ERROR("Cant enable DPLL4 when DC5 or DC6 are > > > > enabled\n"); > > > > + return; > > > > + } > > > > > > This looks like dead code, or we messed up earlier already (in which > > > case it should just be some kind of assert IMO). > Are you suggesting that I put this in a ->prepare() hook or much earlier > in the atomic check/prepare phase? FYI, I am just adding a restriction > mentioned in the spec concerning this DPLL. > > > > > Does DMC handle other MG PLLs? I would have thought not, so if we keep > > the assert then it should perhaps be unconditional. Hmm. But aren't we > > still hodling the modeset power domain when this gets called? If so > > this is definitely dead code. > I am not sure whether DMC handles other MG PLLs but the spec explicitly > says that DMC will not handle this PLL. Where do you suggest is an > appropriate place to put this assert? DC5/6 enable probably is the best, if we even want the assert. And I'm not convinced that we do since we don't have it for any other PLL either. > > > > > > > > > Also what is this EHL thing anyway? I can't even find it in the > > > spec. > EHL = Elkhart Lake, a new Gen 11 platform. > > Thanks, > Vivek > > > > > + > > > > icl_pll_power_enable(dev_priv, pll, enable_reg); > > > > > > > > icl_mg_pll_write(dev_priv, pll); > > > > @@ -3249,12 +3306,13 @@ 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", &mg_pll_funcs, DPLL_ID_ICL_MGPLL1,0 }, > > > > { }, > > > > }; > > > > > > > > static const struct intel_dpll_mgr ehl_pll_mgr = { > > > > .dpll_info = ehl_plls, > > > > - .get_dpll = icl_get_dpll, > > > > + .get_dpll = ehl_get_dpll, > > > > .dump_hw_state = icl_dump_hw_state, > > > > }; > > > > > > > > -- > > > > 2.14.5 > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > -- > > > Ville Syrjälä > > > Intel > >
On Fri, 5 Apr 2019 17:46:38 -0700 Lucas De Marchi <lucas.demarchi@intel.com> wrote: Hi, > On Fri, Apr 05, 2019 at 10:59:53AM -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. > > ok > > > > >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. > > these 2 don't match spec. > > "3rd PLL for use with combo PHY (DPLL4) and 3rd combo PHY DDI clocks > (DDIC clock)" > > This is a combophy pll, not a mg phy pll. The only thing that is > hooked to mg registers is the enable. So my understanding is that > what you need: > > - use the dpll calculations > - make sure intel_find_shared_dpll doesn't this if it's for eDP > - setup the enable/disable to use MG_ENABLE register Looks like my interpretation of the spec is different from yours but your comments make sense. Should I create a new ID for this DPLL or juse re-use DPLL_ID_ICL_MGPLL1? > > > > >v3: Fix compilation error > > > >Cc: Lucas De Marchi <lucas.demarchi@intel.com> > >Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> > >Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com> > >--- > > drivers/gpu/drm/i915/intel_dpll_mgr.c | 60 > > ++++++++++++++++++++++++++++++++++- 1 file changed, 59 > > insertions(+), 1 deletion(-) > > > >diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c > >b/drivers/gpu/drm/i915/intel_dpll_mgr.c index > >e01c057ce50b..c3f0b9720c54 100644 --- > >a/drivers/gpu/drm/i915/intel_dpll_mgr.c +++ > >b/drivers/gpu/drm/i915/intel_dpll_mgr.c @@ -2870,6 +2870,56 @@ > >icl_get_dpll(struct intel_crtc_state *crtc_state, > > return pll; > > } > > > >+static struct intel_shared_dpll * > >+ehl_get_dpll(struct intel_crtc_state *crtc_state, > >+ struct intel_encoder *encoder) > >+{ > >+ struct drm_i915_private *dev_priv = > >to_i915(crtc_state->base.crtc->dev); > >+ struct intel_shared_dpll *pll; > >+ enum port port = encoder->port; > >+ enum intel_dpll_id min, max; > >+ bool ret; > >+ > >+ if (!intel_port_is_combophy(dev_priv, port)) { > >+ MISSING_CASE(port); > >+ return NULL; > >+ } > >+ > >+ min = DPLL_ID_ICL_DPLL0; > >+ max = DPLL_ID_ICL_DPLL1; > >+ ret = icl_calc_dpll_state(crtc_state, encoder); > >+ if (ret) { > >+ pll = intel_find_shared_dpll(crtc_state, min, max); > >+ if (pll) { > >+ intel_reference_shared_dpll(pll, > >crtc_state); > >+ return pll; > >+ } > >+ } else { > >+ DRM_DEBUG_KMS("Could not calculate PLL state.\n"); > > the check for ret is swapped and you are missing a return here. Unless I am reading it utterly wrong, icl_get_dpll has this: if (!ret) { DRM_DEBUG_KMS("Could not calculate PLL state.\n"); return NULL; > > But given the comments above, I think it would be better to reuse > icl_get_dpll() rather than what you are doing here. I could have used icl_get_dpll() but thought it would be much cleaner to have a separate function for EHL; otherwise, I guess I need to sprinkle icl_get_dpll with many if(EHL) statements. > > >+ } > >+ > >+ if (encoder->type == INTEL_OUTPUT_EDP) { > >+ DRM_DEBUG_KMS("Cannot use DPLL4 with EDP.\n"); > >+ return NULL; > >+ } > > this is already too late The idea was if we have EDP being used, then we first try to find if one of the combo PHY DPLLs are available to be used. If they are not, then we come here and return as we cannot use this one either. > > >+ > >+ min = max = DPLL_ID_ICL_MGPLL1; > >+ ret = icl_calc_mg_pll_state(crtc_state); > >+ if (!ret) { > >+ DRM_DEBUG_KMS("Could not calculate PLL state.\n"); > >+ return NULL; > > again... ret == 0 is success, not otherwise I'll send out a v4 with your suggestions soon. Thanks, Vivek > > Lucas De Marchi > > >+ } > >+ > >+ pll = intel_find_shared_dpll(crtc_state, min, max); > >+ if (!pll) { > >+ DRM_DEBUG_KMS("No PLL selected\n"); > >+ return NULL; > >+ } > >+ > >+ intel_reference_shared_dpll(pll, crtc_state); > >+ return pll; > >+} > >+ > > static bool mg_pll_get_hw_state(struct drm_i915_private *dev_priv, > > struct intel_shared_dpll *pll, > > struct intel_dpll_hw_state > > *hw_state) > >@@ -3115,6 +3165,13 @@ static void mg_pll_enable(struct > >drm_i915_private *dev_priv, > > i915_reg_t enable_reg = > > MG_PLL_ENABLE(icl_pll_id_to_tc_port(pll->info->id)); > > > >+ if (IS_ELKHARTLAKE(dev_priv) && > >+ (I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5 || > >+ I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6)) { > >+ DRM_ERROR("Cant enable DPLL4 when DC5 or DC6 are > >enabled\n"); > >+ return; > >+ } > >+ > > icl_pll_power_enable(dev_priv, pll, enable_reg); > > > > icl_mg_pll_write(dev_priv, pll); > >@@ -3249,12 +3306,13 @@ 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", &mg_pll_funcs, DPLL_ID_ICL_MGPLL1,0 }, > > { }, > > }; > > > > static const struct intel_dpll_mgr ehl_pll_mgr = { > > .dpll_info = ehl_plls, > >- .get_dpll = icl_get_dpll, > >+ .get_dpll = ehl_get_dpll, > > .dump_hw_state = icl_dump_hw_state, > > }; > > > >-- > >2.14.5 > >
On Mon, 8 Apr 2019 12:11:15 +0300 Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: Hi, > On Fri, Apr 05, 2019 at 04:33:30PM -0700, Vivek Kasireddy wrote: > > On Fri, 5 Apr 2019 21:39:11 +0300 > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > Hi Ville, > > > > > On Fri, Apr 05, 2019 at 09:33:56PM +0300, Ville Syrjälä wrote: > > > > On Fri, Apr 05, 2019 at 10:59:53AM -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 > > > > > > > > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > > > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> > > > > > Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_dpll_mgr.c | 60 > > > > > ++++++++++++++++++++++++++++++++++- 1 file changed, 59 > > > > > insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c > > > > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c index > > > > > e01c057ce50b..c3f0b9720c54 100644 --- > > > > > a/drivers/gpu/drm/i915/intel_dpll_mgr.c +++ > > > > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c @@ -2870,6 +2870,56 @@ > > > > > icl_get_dpll(struct intel_crtc_state *crtc_state, return pll; > > > > > } > > > > > > > > > > +static struct intel_shared_dpll * > > > > > +ehl_get_dpll(struct intel_crtc_state *crtc_state, > > > > > + struct intel_encoder *encoder) > > > > > +{ > > > > > + struct drm_i915_private *dev_priv = > > > > > to_i915(crtc_state->base.crtc->dev); > > > > > + struct intel_shared_dpll *pll; > > > > > + enum port port = encoder->port; > > > > > + enum intel_dpll_id min, max; > > > > > + bool ret; > > > > > + > > > > > + if (!intel_port_is_combophy(dev_priv, port)) { > > > > > + MISSING_CASE(port); > > > > > + return NULL; > > > > > + } > > > > > + > > > > > + min = DPLL_ID_ICL_DPLL0; > > > > > + max = DPLL_ID_ICL_DPLL1; > > > > > + ret = icl_calc_dpll_state(crtc_state, encoder); > > > > > + if (ret) { > > > > > + pll = intel_find_shared_dpll(crtc_state, min, > > > > > max); > > > > > + if (pll) { > > > > > + intel_reference_shared_dpll(pll, > > > > > crtc_state); > > > > > + return pll; > > > > > + } > > > > > + } else { > > > > > + DRM_DEBUG_KMS("Could not calculate PLL > > > > > state.\n"); > > > > > + } > > > > > + > > > > > + if (encoder->type == INTEL_OUTPUT_EDP) { > > > > > + DRM_DEBUG_KMS("Cannot use DPLL4 with > > > > > EDP.\n"); > > > > > + return NULL; > > > > > + } > > > > > + > > > > > + min = max = DPLL_ID_ICL_MGPLL1; > > > > > + ret = icl_calc_mg_pll_state(crtc_state); > > > > > + if (!ret) { > > > > > + DRM_DEBUG_KMS("Could not calculate PLL > > > > > state.\n"); > > > > > + return NULL; > > > > > + } > > > > > + > > > > > + pll = intel_find_shared_dpll(crtc_state, min, max); > > > > > + if (!pll) { > > > > > + DRM_DEBUG_KMS("No PLL selected\n"); > > > > > + return NULL; > > > > > + } > > > > > + > > > > > + intel_reference_shared_dpll(pll, crtc_state); > > > > > + return pll; > > > > > +} > > > > > + > > > > > static bool mg_pll_get_hw_state(struct drm_i915_private > > > > > *dev_priv, struct intel_shared_dpll *pll, > > > > > struct intel_dpll_hw_state > > > > > *hw_state) @@ -3115,6 +3165,13 @@ static void > > > > > mg_pll_enable(struct drm_i915_private *dev_priv, i915_reg_t > > > > > enable_reg = > > > > > MG_PLL_ENABLE(icl_pll_id_to_tc_port(pll->info->id)); > > > > > + if (IS_ELKHARTLAKE(dev_priv) && > > > > > + (I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5 || > > > > > + I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6)) { > > > > > + DRM_ERROR("Cant enable DPLL4 when DC5 or DC6 > > > > > are enabled\n"); > > > > > + return; > > > > > + } > > > > > > > > This looks like dead code, or we messed up earlier already (in > > > > which case it should just be some kind of assert IMO). > > Are you suggesting that I put this in a ->prepare() hook or much > > earlier in the atomic check/prepare phase? FYI, I am just adding a > > restriction mentioned in the spec concerning this DPLL. > > > > > > > > Does DMC handle other MG PLLs? I would have thought not, so if we > > > keep the assert then it should perhaps be unconditional. Hmm. But > > > aren't we still hodling the modeset power domain when this gets > > > called? If so this is definitely dead code. > > I am not sure whether DMC handles other MG PLLs but the spec > > explicitly says that DMC will not handle this PLL. Where do you > > suggest is an appropriate place to put this assert? > > DC5/6 enable probably is the best, if we even want the assert. And I'm > not convinced that we do since we don't have it for any other PLL > either. As I mentioned earlier, the spec says we cannot enable this DPLL for EHL if DC5 or DC6 is enabled. Do you think we should disable DC5/DC6 on EHL in order to enable this DPLL on EHL? Should the user/system integrator be making that decision via a module parameter? Thanks, Vivek > > > > > > > > > > > > > > Also what is this EHL thing anyway? I can't even find it in the > > > > spec. > > EHL = Elkhart Lake, a new Gen 11 platform. > > > > Thanks, > > Vivek > > > > > > > + > > > > > icl_pll_power_enable(dev_priv, pll, enable_reg); > > > > > > > > > > icl_mg_pll_write(dev_priv, pll); > > > > > @@ -3249,12 +3306,13 @@ 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", &mg_pll_funcs, DPLL_ID_ICL_MGPLL1,0 }, > > > > > { }, > > > > > }; > > > > > > > > > > static const struct intel_dpll_mgr ehl_pll_mgr = { > > > > > .dpll_info = ehl_plls, > > > > > - .get_dpll = icl_get_dpll, > > > > > + .get_dpll = ehl_get_dpll, > > > > > .dump_hw_state = icl_dump_hw_state, > > > > > }; > > > > > > > > > > -- > > > > > 2.14.5 > > > > > > > > > > _______________________________________________ > > > > > Intel-gfx mailing list > > > > > Intel-gfx@lists.freedesktop.org > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > -- > > > > Ville Syrjälä > > > > Intel > > > >
On Tue, Apr 09, 2019 at 05:00:44PM -0700, Vivek Kasireddy wrote: > On Mon, 8 Apr 2019 12:11:15 +0300 > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > Hi, > > > On Fri, Apr 05, 2019 at 04:33:30PM -0700, Vivek Kasireddy wrote: > > > On Fri, 5 Apr 2019 21:39:11 +0300 > > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > Hi Ville, > > > > > > > On Fri, Apr 05, 2019 at 09:33:56PM +0300, Ville Syrjälä wrote: > > > > > On Fri, Apr 05, 2019 at 10:59:53AM -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 > > > > > > > > > > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > > > > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> > > > > > > Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com> > > > > > > --- > > > > > > drivers/gpu/drm/i915/intel_dpll_mgr.c | 60 > > > > > > ++++++++++++++++++++++++++++++++++- 1 file changed, 59 > > > > > > insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c > > > > > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c index > > > > > > e01c057ce50b..c3f0b9720c54 100644 --- > > > > > > a/drivers/gpu/drm/i915/intel_dpll_mgr.c +++ > > > > > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c @@ -2870,6 +2870,56 @@ > > > > > > icl_get_dpll(struct intel_crtc_state *crtc_state, return pll; > > > > > > } > > > > > > > > > > > > +static struct intel_shared_dpll * > > > > > > +ehl_get_dpll(struct intel_crtc_state *crtc_state, > > > > > > + struct intel_encoder *encoder) > > > > > > +{ > > > > > > + struct drm_i915_private *dev_priv = > > > > > > to_i915(crtc_state->base.crtc->dev); > > > > > > + struct intel_shared_dpll *pll; > > > > > > + enum port port = encoder->port; > > > > > > + enum intel_dpll_id min, max; > > > > > > + bool ret; > > > > > > + > > > > > > + if (!intel_port_is_combophy(dev_priv, port)) { > > > > > > + MISSING_CASE(port); > > > > > > + return NULL; > > > > > > + } > > > > > > + > > > > > > + min = DPLL_ID_ICL_DPLL0; > > > > > > + max = DPLL_ID_ICL_DPLL1; > > > > > > + ret = icl_calc_dpll_state(crtc_state, encoder); > > > > > > + if (ret) { > > > > > > + pll = intel_find_shared_dpll(crtc_state, min, > > > > > > max); > > > > > > + if (pll) { > > > > > > + intel_reference_shared_dpll(pll, > > > > > > crtc_state); > > > > > > + return pll; > > > > > > + } > > > > > > + } else { > > > > > > + DRM_DEBUG_KMS("Could not calculate PLL > > > > > > state.\n"); > > > > > > + } > > > > > > + > > > > > > + if (encoder->type == INTEL_OUTPUT_EDP) { > > > > > > + DRM_DEBUG_KMS("Cannot use DPLL4 with > > > > > > EDP.\n"); > > > > > > + return NULL; > > > > > > + } > > > > > > + > > > > > > + min = max = DPLL_ID_ICL_MGPLL1; > > > > > > + ret = icl_calc_mg_pll_state(crtc_state); > > > > > > + if (!ret) { > > > > > > + DRM_DEBUG_KMS("Could not calculate PLL > > > > > > state.\n"); > > > > > > + return NULL; > > > > > > + } > > > > > > + > > > > > > + pll = intel_find_shared_dpll(crtc_state, min, max); > > > > > > + if (!pll) { > > > > > > + DRM_DEBUG_KMS("No PLL selected\n"); > > > > > > + return NULL; > > > > > > + } > > > > > > + > > > > > > + intel_reference_shared_dpll(pll, crtc_state); > > > > > > + return pll; > > > > > > +} > > > > > > + > > > > > > static bool mg_pll_get_hw_state(struct drm_i915_private > > > > > > *dev_priv, struct intel_shared_dpll *pll, > > > > > > struct intel_dpll_hw_state > > > > > > *hw_state) @@ -3115,6 +3165,13 @@ static void > > > > > > mg_pll_enable(struct drm_i915_private *dev_priv, i915_reg_t > > > > > > enable_reg = > > > > > > MG_PLL_ENABLE(icl_pll_id_to_tc_port(pll->info->id)); > > > > > > + if (IS_ELKHARTLAKE(dev_priv) && > > > > > > + (I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5 || > > > > > > + I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6)) { > > > > > > + DRM_ERROR("Cant enable DPLL4 when DC5 or DC6 > > > > > > are enabled\n"); > > > > > > + return; > > > > > > + } > > > > > > > > > > This looks like dead code, or we messed up earlier already (in > > > > > which case it should just be some kind of assert IMO). > > > Are you suggesting that I put this in a ->prepare() hook or much > > > earlier in the atomic check/prepare phase? FYI, I am just adding a > > > restriction mentioned in the spec concerning this DPLL. > > > > > > > > > > > Does DMC handle other MG PLLs? I would have thought not, so if we > > > > keep the assert then it should perhaps be unconditional. Hmm. But > > > > aren't we still hodling the modeset power domain when this gets > > > > called? If so this is definitely dead code. > > > I am not sure whether DMC handles other MG PLLs but the spec > > > explicitly says that DMC will not handle this PLL. Where do you > > > suggest is an appropriate place to put this assert? > > > > DC5/6 enable probably is the best, if we even want the assert. And I'm > > not convinced that we do since we don't have it for any other PLL > > either. > As I mentioned earlier, the spec says we cannot enable this DPLL for EHL > if DC5 or DC6 is enabled. Do you think we should disable DC5/DC6 on EHL > in order to enable this DPLL on EHL? Should the user/system integrator > be making that decision via a module parameter? If there is some way that could happen currently, then we need to prevent DC5/6 via some power domain stuff.
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c index e01c057ce50b..c3f0b9720c54 100644 --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c @@ -2870,6 +2870,56 @@ icl_get_dpll(struct intel_crtc_state *crtc_state, return pll; } +static struct intel_shared_dpll * +ehl_get_dpll(struct intel_crtc_state *crtc_state, + struct intel_encoder *encoder) +{ + struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); + struct intel_shared_dpll *pll; + enum port port = encoder->port; + enum intel_dpll_id min, max; + bool ret; + + if (!intel_port_is_combophy(dev_priv, port)) { + MISSING_CASE(port); + return NULL; + } + + min = DPLL_ID_ICL_DPLL0; + max = DPLL_ID_ICL_DPLL1; + ret = icl_calc_dpll_state(crtc_state, encoder); + if (ret) { + pll = intel_find_shared_dpll(crtc_state, min, max); + if (pll) { + intel_reference_shared_dpll(pll, crtc_state); + return pll; + } + } else { + DRM_DEBUG_KMS("Could not calculate PLL state.\n"); + } + + if (encoder->type == INTEL_OUTPUT_EDP) { + DRM_DEBUG_KMS("Cannot use DPLL4 with EDP.\n"); + return NULL; + } + + min = max = DPLL_ID_ICL_MGPLL1; + ret = icl_calc_mg_pll_state(crtc_state); + if (!ret) { + DRM_DEBUG_KMS("Could not calculate PLL state.\n"); + return NULL; + } + + pll = intel_find_shared_dpll(crtc_state, min, max); + if (!pll) { + DRM_DEBUG_KMS("No PLL selected\n"); + return NULL; + } + + intel_reference_shared_dpll(pll, crtc_state); + return pll; +} + static bool mg_pll_get_hw_state(struct drm_i915_private *dev_priv, struct intel_shared_dpll *pll, struct intel_dpll_hw_state *hw_state) @@ -3115,6 +3165,13 @@ static void mg_pll_enable(struct drm_i915_private *dev_priv, i915_reg_t enable_reg = MG_PLL_ENABLE(icl_pll_id_to_tc_port(pll->info->id)); + if (IS_ELKHARTLAKE(dev_priv) && + (I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5 || + I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6)) { + DRM_ERROR("Cant enable DPLL4 when DC5 or DC6 are enabled\n"); + return; + } + icl_pll_power_enable(dev_priv, pll, enable_reg); icl_mg_pll_write(dev_priv, pll); @@ -3249,12 +3306,13 @@ 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", &mg_pll_funcs, DPLL_ID_ICL_MGPLL1,0 }, { }, }; static const struct intel_dpll_mgr ehl_pll_mgr = { .dpll_info = ehl_plls, - .get_dpll = icl_get_dpll, + .get_dpll = ehl_get_dpll, .dump_hw_state = icl_dump_hw_state, };