On Wed, 03 Oct 2018, Jani Nikula <jani.nikula@xxxxxxxxx> wrote:
> On Wed, 03 Oct 2018, Jani Nikula <jani.nikula@xxxxxxxxx> wrote:
>> On Fri, 14 Sep 2018, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
>>> On Fri, Sep 14, 2018 at 12:24:12PM +0530, Vandita Kulkarni wrote:
>>>> From: Madhav Chauhan <madhav.chauhan@xxxxxxxxx>
>>>>
>>>> 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@xxxxxxxxx>
>>>> Signed-off-by: Vandita Kulkarni <vandita.kulkarni@xxxxxxxxx>
>>>> ---
>>>> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
