[Intel-gfx] [RFC 1/3] drm/i915/icl: Restructure ICL DPLL enable functionality

[Jani Nikula]

On Thu, 04 Oct 2018, "Kulkarni, Vandita" <vandita.kulkarni at intel.com> wrote:
>> -----Original Message-----
>> From: Nikula, Jani
>> Sent: Wednesday, October 3, 2018 5:11 PM
>> To: Ville Syrjälä <ville.syrjala at linux.intel.com>; Kulkarni, Vandita
>> <vandita.kulkarni at intel.com>
>> Cc: intel-gfx at lists.freedesktop.org; Zanoni, Paulo R
>> <paulo.r.zanoni at 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 at intel.com> wrote:
>> > On Wed, 03 Oct 2018, Jani Nikula <jani.nikula at intel.com> wrote:
>> >> On Fri, 14 Sep 2018, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
>> >>> On Fri, Sep 14, 2018 at 12:24:12PM +0530, Vandita Kulkarni wrote:
>> >>>> From: Madhav Chauhan <madhav.chauhan at 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 at intel.com>
>> >>>> Signed-off-by: Vandita Kulkarni <vandita.kulkarni at 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.

Like I said, I think Ville preferred pushing the pll enable calls down
to encoders on DDI platforms. In this case, we'd be able to better
control when and where the pll enable happens, and potentially split the
calls or add encoder specific parts into the dpll manager. (There
already exists some.)

BR,
Jani.


>
> 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

-- 
Jani Nikula, Intel Open Source Graphics Center