> -----Original Message-----
> From: Nikula, Jani
> Sent: Wednesday, October 3, 2018 5:11 PM
> To: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>; Kulkarni, Vandita
> <vandita.kulkarni@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Zanoni, Paulo R
> <paulo.r.zanoni@xxxxxxxxx>
> Subject: Re: [RFC 1/3] drm/i915/icl: Restructure ICL DPLL enable
> functionality
>
> On Wed, 03 Oct 2018, Jani Nikula <jani.nikula@xxxxxxxxx> wrote:
> > On Wed, 03 Oct 2018, Jani Nikula <jani.nikula@xxxxxxxxx> wrote:
> >> On Fri, 14 Sep 2018, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> >>> On Fri, Sep 14, 2018 at 12:24:12PM +0530, Vandita Kulkarni wrote:
> >>>> From: Madhav Chauhan <madhav.chauhan@xxxxxxxxx>
> >>>>
> >>>> In Gen11, DPLL 0 and 1 are shared between DDI and DSI.
> >>>> Most of the steps for enabling DPLL are common across DDI and DSI.
> >>>> This patch makes icl_dpll_enable() generic which will be used by
> >>>> all the encoders.
> >>>>
> >>>> Signed-off-by: Madhav Chauhan <madhav.chauhan@xxxxxxxxx>
> >>>> Signed-off-by: Vandita Kulkarni <vandita.kulkarni@xxxxxxxxx>
> >>>> ---
> >>>> drivers/gpu/drm/i915/intel_ddi.c | 12 ++++++++++++
> >>>> drivers/gpu/drm/i915/intel_dpll_mgr.c | 19 ++-----------------
> >>>> drivers/gpu/drm/i915/intel_dpll_mgr.h | 2 +-
> >>>> 3 files changed, 15 insertions(+), 18 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> >>>> b/drivers/gpu/drm/i915/intel_ddi.c
> >>>> index cd01a09..2942a24 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >>>> @@ -2810,6 +2810,18 @@ static void intel_ddi_clk_select(struct
> intel_encoder *encoder,
> >>>> mutex_lock(&dev_priv->dpll_lock);
> >>>>
> >>>> if (IS_ICELAKE(dev_priv)) {
> >>>> + enum intel_dpll_id id = pll->info->id;
> >>>> + i915_reg_t enable_reg = icl_pll_id_to_enable_reg(id);
> >>>> +
> >>>> + val = I915_READ(enable_reg);
> >>>> + val |= PLL_ENABLE;
> >>>> + I915_WRITE(enable_reg, val);
> >>>> +
> >>>> + /* TODO: wait times missing from the spec. */
> >>>> + if (intel_wait_for_register(dev_priv, enable_reg, PLL_LOCK,
> >>>> + PLL_LOCK, 5))
> >>>> + DRM_ERROR("PLL %d not locked\n", id);
> >>>> +
> >>>
> >>> I don't really see why this can't stay in the dpll mgr.
> >>
> >> Agreed, I think it should stay in DPLL manager.
> >>
> >> The thing is, DPLL enabling for DSI requires encoder specific steps
> >> in the middle of the sequence hidden in DPLL manager. It's not pretty
> >> to add that in DPLL manager.
> >>
> >> One approach might be to add encoder hooks to call from the right
> >> spot in the DPLL manager, "mid_pll_enable". It's annoying because it
> >> would have to happen in the middle of the platform specific DPLL
> >> manager
> >> pll->info->funcs->enable hook. We'd have to call the hooks from
> >> pll->info->funcs->platform
> >> specific code, or split those hooks in two. The former is ugly
> >> because it requires passing crtc to the pll enable hook. So I guess
> >> add a pll post enable hook.
> >>
> >> Below's some draft code to give an idea what I mean. You'd move the
> >> above hunk to the post hook instead.
> >>
> >> So then we'd add mid_pll_enable hooks and do the required magic in
> >> the DSI mid pll hook.
Thanks Jani, let me try this out.
Regards,
Vandita
> >
> > PS. And even with this I'm not yet sure if we can do the overall DPLL
> > enabling at the right spot wrt bspec DSI mode set sequence. *cringe*.
>
> Ville reminded me that we did have this idea of pushing pll enable calls down to
> encoders on DDI platforms. This would help, of course.
>
> BR,
> Jani.
>
>
> >
> > BR,
> > Jani.
> >
> >>
> >> Overall I'm starting to feel the appeal of driving modeset from
> >> encoders, with library style helpers provided from intel_display.c,
> >> instead of adding more and more encoder hooks to do stuff at specific
> >> places. But I digress.
> >>
> >> BR,
> >> Jani.
> >>
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> >> b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> >> index e6cac9225536..a4ca1b4a124c 100644
> >> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> >> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> >> @@ -191,6 +191,12 @@ void intel_enable_shared_dpll(struct intel_crtc
> >> *crtc)
> >>
> >> DRM_DEBUG_KMS("enabling %s\n", pll->info->name);
> >> pll->info->funcs->enable(dev_priv, pll);
> >> +
> >> + intel_encoders_mid_pll_enable(crtc); /* pipe_config, old_state? */
> >> +
> >> + if (pll->info->funcs->post_enable)
> >> + pll->info->funcs->post_enable(dev_priv, pll);
> >> +
> >> pll->on = true;
> >>
> >> out:
> >> @@ -3199,6 +3205,7 @@ static void icl_dump_hw_state(struct
> >> drm_i915_private *dev_priv,
> >>
> >> static const struct intel_shared_dpll_funcs icl_pll_funcs = {
> >> .enable = icl_pll_enable,
> >> + .post_enable = icl_pll_post_enable,
> >> .disable = icl_pll_disable,
> >> .get_hw_state = icl_pll_get_hw_state, }; diff --git
> >> a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> >> b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> >> index bf0de8a4dc63..eceeef3b8872 100644
> >> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> >> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> >> @@ -231,6 +231,16 @@ struct intel_shared_dpll_funcs {
> >> struct intel_shared_dpll *pll);
> >>
> >> /**
> >> + * @post_enable:
> >> + *
> >> + * Optional hook to call after encoder specific mid pll hooks have been
> >> + * called from intel_enable_shared_dpll().
> >> + */
> >> + void (*post_enable)(struct drm_i915_private *dev_priv,
> >> + struct intel_shared_dpll *pll);
> >> +
> >> +
> >> + /**
> >> * @disable:
> >> *
> >> * Hook for disabling the pll, called from
> >> intel_disable_shared_dpll()
>
> --
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
