Re: [RFC 1/3] drm/i915/icl: Restructure ICL DPLL enable functionality

["Kulkarni, Vandita" <vandita.kulkarni@intel.com>]

> -----Original Message-----
> From: Nikula, Jani
> Sent: Wednesday, October 3, 2018 5:11 PM
> To: Ville Syrjälä <ville.syrjala@linux.intel.com>; Kulkarni, Vandita
> <vandita.kulkarni@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Zanoni, Paulo R
> <paulo.r.zanoni@intel.com>
> Subject: Re: [Intel-gfx] [RFC 1/3] drm/i915/icl: Restructure ICL DPLL enable
> functionality
> 
> On Wed, 03 Oct 2018, Jani Nikula <jani.nikula@intel.com> wrote:
> > On Wed, 03 Oct 2018, Jani Nikula <jani.nikula@intel.com> wrote:
> >> On Fri, 14 Sep 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >>> On Fri, Sep 14, 2018 at 12:24:12PM +0530, Vandita Kulkarni wrote:
> >>>> From: Madhav Chauhan <madhav.chauhan@intel.com>
> >>>>
> >>>> 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@intel.com>
> >>>> Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
> >>>> ---
> >>>>  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx