On Mon, 8 Apr 2019 12:11:15 +0300
Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> > > > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@xxxxxxxxx>
> > > > > Reviewed-by: Bob Paauwe <bob.j.paauwe@xxxxxxxxx>
> > > > > ---
> > > > > 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@xxxxxxxxxxxxxxxxxxxxx
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > >
> > > > --
> > > > Ville Syrjälä
> > > > Intel
> > >
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
