| Message ID | 1536908054-2176-2-git-send-email-vandita.kulkarni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable ICL DSI PLL | expand |
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. > if (port >= PORT_C) > I915_WRITE(DDI_CLK_SEL(port), > icl_pll_to_ddi_pll_sel(encoder, pll)); > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c > index e6cac92..36ed155 100644 > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c > @@ -2930,7 +2930,7 @@ static bool icl_calc_mg_pll_state(struct intel_crtc_state *crtc_state, > return pll; > } > > -static i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id) > +i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id) > { > switch (id) { > default: > @@ -3119,22 +3119,7 @@ static void icl_pll_enable(struct drm_i915_private *dev_priv, > default: > MISSING_CASE(id); > } > - > - /* > - * DVFS pre sequence would be here, but in our driver the cdclk code > - * paths should already be setting the appropriate voltage, hence we do > - * nothign here. > - */ > - > - val = I915_READ(enable_reg); > - val |= PLL_ENABLE; > - I915_WRITE(enable_reg, val); > - > - if (intel_wait_for_register(dev_priv, enable_reg, PLL_LOCK, PLL_LOCK, > - 1)) /* 600us actually. */ > - DRM_ERROR("PLL %d not locked\n", id); > - > - /* DVFS post sequence would be here. See the comment above. */ > + /* Encoder specific PLL enable steps are added in encoder file */ > } > > static void icl_pll_disable(struct drm_i915_private *dev_priv, > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h > index bf0de8a..9e89265 100644 > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h > @@ -345,5 +345,5 @@ void intel_dpll_dump_hw_state(struct drm_i915_private *dev_priv, > int icl_calc_dp_combo_pll_link(struct drm_i915_private *dev_priv, > uint32_t pll_id); > int cnl_hdmi_pll_ref_clock(struct drm_i915_private *dev_priv); > - > +i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id); > #endif /* _INTEL_DPLL_MGR_H_ */ > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> -----Original Message----- > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > Sent: Friday, September 14, 2018 9:37 PM > To: Kulkarni, Vandita <vandita.kulkarni@intel.com> > Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>; > Zanoni, Paulo R <paulo.r.zanoni@intel.com> > Subject: Re: [Intel-gfx] [RFC 1/3] drm/i915/icl: Restructure ICL DPLL enable > functionality > > 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. This was part of icl_pll_enable, which gets called for all hdmi, dp and mipi dsi. But for mipi dsi we have couple of more things to do before we enable pll. In order to keep it unchanged for other encoders , pll enable was moved here. Another way could be have an encoder check in icl_pll_enable function and do those extra steps for mipi dsi and then enable the pll. Please let me know what you think would be a better way to do. Thanks, Vandita > > > if (port >= PORT_C) > > I915_WRITE(DDI_CLK_SEL(port), > > icl_pll_to_ddi_pll_sel(encoder, pll)); diff --git > > a/drivers/gpu/drm/i915/intel_dpll_mgr.c > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c > > index e6cac92..36ed155 100644 > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c > > @@ -2930,7 +2930,7 @@ static bool icl_calc_mg_pll_state(struct > intel_crtc_state *crtc_state, > > return pll; > > } > > > > -static i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id) > > +i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id) > > { > > switch (id) { > > default: > > @@ -3119,22 +3119,7 @@ static void icl_pll_enable(struct drm_i915_private > *dev_priv, > > default: > > MISSING_CASE(id); > > } > > - > > - /* > > - * DVFS pre sequence would be here, but in our driver the cdclk code > > - * paths should already be setting the appropriate voltage, hence we do > > - * nothign here. > > - */ > > - > > - val = I915_READ(enable_reg); > > - val |= PLL_ENABLE; > > - I915_WRITE(enable_reg, val); > > - > > - if (intel_wait_for_register(dev_priv, enable_reg, PLL_LOCK, PLL_LOCK, > > - 1)) /* 600us actually. */ > > - DRM_ERROR("PLL %d not locked\n", id); > > - > > - /* DVFS post sequence would be here. See the comment above. */ > > + /* Encoder specific PLL enable steps are added in encoder file */ > > } > > > > static void icl_pll_disable(struct drm_i915_private *dev_priv, diff > > --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h > > b/drivers/gpu/drm/i915/intel_dpll_mgr.h > > index bf0de8a..9e89265 100644 > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h > > @@ -345,5 +345,5 @@ void intel_dpll_dump_hw_state(struct > > drm_i915_private *dev_priv, int icl_calc_dp_combo_pll_link(struct > drm_i915_private *dev_priv, > > uint32_t pll_id); > > int cnl_hdmi_pll_ref_clock(struct drm_i915_private *dev_priv); > > - > > +i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id); > > #endif /* _INTEL_DPLL_MGR_H_ */ > > -- > > 1.9.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel
On Wed, Sep 19, 2018 at 05:31:37PM +0000, Kulkarni, Vandita wrote: > > > > -----Original Message----- > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Sent: Friday, September 14, 2018 9:37 PM > > To: Kulkarni, Vandita <vandita.kulkarni@intel.com> > > Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>; > > Zanoni, Paulo R <paulo.r.zanoni@intel.com> > > Subject: Re: [Intel-gfx] [RFC 1/3] drm/i915/icl: Restructure ICL DPLL enable > > functionality > > > > 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. > This was part of icl_pll_enable, which gets called for all hdmi, dp and mipi dsi. > But for mipi dsi we have couple of more things to do before we enable pll. > > In order to keep it unchanged for other encoders , pll enable was moved here. > Another way could be have an encoder check in icl_pll_enable function and do those extra steps for mipi dsi and then enable the pll. > Please let me know what you think would be a better way to do. Hard to judge what is best without knowing what the differences are. > > Thanks, > Vandita > > > > > if (port >= PORT_C) > > > I915_WRITE(DDI_CLK_SEL(port), > > > icl_pll_to_ddi_pll_sel(encoder, pll)); diff --git > > > a/drivers/gpu/drm/i915/intel_dpll_mgr.c > > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c > > > index e6cac92..36ed155 100644 > > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c > > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c > > > @@ -2930,7 +2930,7 @@ static bool icl_calc_mg_pll_state(struct > > intel_crtc_state *crtc_state, > > > return pll; > > > } > > > > > > -static i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id) > > > +i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id) > > > { > > > switch (id) { > > > default: > > > @@ -3119,22 +3119,7 @@ static void icl_pll_enable(struct drm_i915_private > > *dev_priv, > > > default: > > > MISSING_CASE(id); > > > } > > > - > > > - /* > > > - * DVFS pre sequence would be here, but in our driver the cdclk code > > > - * paths should already be setting the appropriate voltage, hence we do > > > - * nothign here. > > > - */ > > > - > > > - val = I915_READ(enable_reg); > > > - val |= PLL_ENABLE; > > > - I915_WRITE(enable_reg, val); > > > - > > > - if (intel_wait_for_register(dev_priv, enable_reg, PLL_LOCK, PLL_LOCK, > > > - 1)) /* 600us actually. */ > > > - DRM_ERROR("PLL %d not locked\n", id); > > > - > > > - /* DVFS post sequence would be here. See the comment above. */ > > > + /* Encoder specific PLL enable steps are added in encoder file */ > > > } > > > > > > static void icl_pll_disable(struct drm_i915_private *dev_priv, diff > > > --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h > > > b/drivers/gpu/drm/i915/intel_dpll_mgr.h > > > index bf0de8a..9e89265 100644 > > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h > > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h > > > @@ -345,5 +345,5 @@ void intel_dpll_dump_hw_state(struct > > > drm_i915_private *dev_priv, int icl_calc_dp_combo_pll_link(struct > > drm_i915_private *dev_priv, > > > uint32_t pll_id); > > > int cnl_hdmi_pll_ref_clock(struct drm_i915_private *dev_priv); > > > - > > > +i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id); > > > #endif /* _INTEL_DPLL_MGR_H_ */ > > > -- > > > 1.9.1 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrjälä > > Intel
> -----Original Message----- > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > Sent: Wednesday, September 26, 2018 7:57 PM > To: Kulkarni, Vandita <vandita.kulkarni@intel.com> > Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>; > Zanoni, Paulo R <paulo.r.zanoni@intel.com> > Subject: Re: [Intel-gfx] [RFC 1/3] drm/i915/icl: Restructure ICL DPLL enable > functionality > > On Wed, Sep 19, 2018 at 05:31:37PM +0000, Kulkarni, Vandita wrote: > > > > > > > -----Original Message----- > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Sent: Friday, September 14, 2018 9:37 PM > > > To: Kulkarni, Vandita <vandita.kulkarni@intel.com> > > > Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani > > > <jani.nikula@intel.com>; Zanoni, Paulo R <paulo.r.zanoni@intel.com> > > > Subject: Re: [Intel-gfx] [RFC 1/3] drm/i915/icl: Restructure ICL > > > DPLL enable functionality > > > > > > 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. > > This was part of icl_pll_enable, which gets called for all hdmi, dp and mipi dsi. > > But for mipi dsi we have couple of more things to do before we enable pll. > > > > In order to keep it unchanged for other encoders , pll enable was moved here. > > Another way could be have an encoder check in icl_pll_enable function and do > those extra steps for mipi dsi and then enable the pll. > > Please let me know what you think would be a better way to do. > > Hard to judge what is best without knowing what the differences are. As per the bspec, The below two steps are the extra ones that we do before enabling the PLL 1. Configure both DSI_ESC_CLK_DIV and DPHY_ESC_CLK_DIV registers. Both registers must be programmed with the same value. 2. Read back both DPHY_ESC_CLK_DIV, ignoring the data value, this is to ensure write completed before the next step. And these steps below Configure DPCLKA_CFGCR0 to map the DPLL to the DDI/DSI. Make sure that the DDI clocks are not gated at this point. The DSI enabling sequence will gate the DDI clocks at a later point within the sequence. are done before pll enable in case of pll enable sequence for mipi dsi. Thanks Vandita > > > > > Thanks, > > Vandita > > > > > > > if (port >= PORT_C) > > > > I915_WRITE(DDI_CLK_SEL(port), > > > > icl_pll_to_ddi_pll_sel(encoder, pll)); diff --git > > > > a/drivers/gpu/drm/i915/intel_dpll_mgr.c > > > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c > > > > index e6cac92..36ed155 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c > > > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c > > > > @@ -2930,7 +2930,7 @@ static bool icl_calc_mg_pll_state(struct > > > intel_crtc_state *crtc_state, > > > > return pll; > > > > } > > > > > > > > -static i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id) > > > > +i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id) > > > > { > > > > switch (id) { > > > > default: > > > > @@ -3119,22 +3119,7 @@ static void icl_pll_enable(struct > > > > drm_i915_private > > > *dev_priv, > > > > default: > > > > MISSING_CASE(id); > > > > } > > > > - > > > > - /* > > > > - * DVFS pre sequence would be here, but in our driver the cdclk code > > > > - * paths should already be setting the appropriate voltage, hence we do > > > > - * nothign here. > > > > - */ > > > > - > > > > - val = I915_READ(enable_reg); > > > > - val |= PLL_ENABLE; > > > > - I915_WRITE(enable_reg, val); > > > > - > > > > - if (intel_wait_for_register(dev_priv, enable_reg, PLL_LOCK, PLL_LOCK, > > > > - 1)) /* 600us actually. */ > > > > - DRM_ERROR("PLL %d not locked\n", id); > > > > - > > > > - /* DVFS post sequence would be here. See the comment above. */ > > > > + /* Encoder specific PLL enable steps are added in encoder file > > > > +*/ > > > > } > > > > > > > > static void icl_pll_disable(struct drm_i915_private *dev_priv, > > > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h > > > > b/drivers/gpu/drm/i915/intel_dpll_mgr.h > > > > index bf0de8a..9e89265 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h > > > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h > > > > @@ -345,5 +345,5 @@ void intel_dpll_dump_hw_state(struct > > > > drm_i915_private *dev_priv, int icl_calc_dp_combo_pll_link(struct > > > drm_i915_private *dev_priv, > > > > uint32_t pll_id); > > > > int cnl_hdmi_pll_ref_clock(struct drm_i915_private *dev_priv); > > > > - > > > > +i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id); > > > > #endif /* _INTEL_DPLL_MGR_H_ */ > > > > -- > > > > 1.9.1 > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel
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()
On Wed, 03 Oct 2018, Jani Nikula <jani.nikula@intel.com> wrote: > 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. 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()
On Wed, 03 Oct 2018, Jani Nikula <jani.nikula@intel.com> wrote: > On Wed, 03 Oct 2018, Jani Nikula <jani.nikula@intel.com> wrote: >> 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. > > 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()
> -----Original Message----- > From: Nikula, Jani > Sent: Wednesday, October 3, 2018 5:11 PM > To: Ville Syrjälä <ville.syrjala@linux.intel.com>; Kulkarni, Vandita > <vandita.kulkarni@intel.com> > Cc: intel-gfx@lists.freedesktop.org; Zanoni, Paulo R > <paulo.r.zanoni@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@intel.com> wrote: > > On Wed, 03 Oct 2018, Jani Nikula <jani.nikula@intel.com> wrote: > >> 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 > >> 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. 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
On Thu, 04 Oct 2018, "Kulkarni, Vandita" <vandita.kulkarni@intel.com> wrote: >> -----Original Message----- >> From: Nikula, Jani >> Sent: Wednesday, October 3, 2018 5:11 PM >> To: Ville Syrjälä <ville.syrjala@linux.intel.com>; Kulkarni, Vandita >> <vandita.kulkarni@intel.com> >> Cc: intel-gfx@lists.freedesktop.org; Zanoni, Paulo R >> <paulo.r.zanoni@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@intel.com> wrote: >> > On Wed, 03 Oct 2018, Jani Nikula <jani.nikula@intel.com> wrote: >> >> 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 >> >> 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
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); + if (port >= PORT_C) I915_WRITE(DDI_CLK_SEL(port), icl_pll_to_ddi_pll_sel(encoder, pll)); diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c index e6cac92..36ed155 100644 --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c @@ -2930,7 +2930,7 @@ static bool icl_calc_mg_pll_state(struct intel_crtc_state *crtc_state, return pll; } -static i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id) +i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id) { switch (id) { default: @@ -3119,22 +3119,7 @@ static void icl_pll_enable(struct drm_i915_private *dev_priv, default: MISSING_CASE(id); } - - /* - * DVFS pre sequence would be here, but in our driver the cdclk code - * paths should already be setting the appropriate voltage, hence we do - * nothign here. - */ - - val = I915_READ(enable_reg); - val |= PLL_ENABLE; - I915_WRITE(enable_reg, val); - - if (intel_wait_for_register(dev_priv, enable_reg, PLL_LOCK, PLL_LOCK, - 1)) /* 600us actually. */ - DRM_ERROR("PLL %d not locked\n", id); - - /* DVFS post sequence would be here. See the comment above. */ + /* Encoder specific PLL enable steps are added in encoder file */ } static void icl_pll_disable(struct drm_i915_private *dev_priv, diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h index bf0de8a..9e89265 100644 --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h @@ -345,5 +345,5 @@ void intel_dpll_dump_hw_state(struct drm_i915_private *dev_priv, int icl_calc_dp_combo_pll_link(struct drm_i915_private *dev_priv, uint32_t pll_id); int cnl_hdmi_pll_ref_clock(struct drm_i915_private *dev_priv); - +i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id); #endif /* _INTEL_DPLL_MGR_H_ */