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

[Jani Nikula]

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

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

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