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

[Jani Nikula <jani.nikula@intel.com>]

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

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