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

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