Re: [PATCH 12/13] drm/i915: Manage HSW/BDW LCPLLs with the shared dpll interface

Conselvan De Oliveira, Ander
2016-03-08T11:11:55Z

On Tue, 2016-03-08 at 12:05 +0100, Maarten Lankhorst wrote:
> Op 26-02-16 om 14:54 schreef Ander Conselvan de Oliveira:
> > Manage the LCPLLs used with DisplayPort, so that all the HSW/BDW DPLLs
> > are managed by the shared dpll code.
> >
> > Signed-off-by: Ander Conselvan de Oliveira <
> > ander.conselvan.de.oliveira@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_ddi.c | 18 ++++----
> > drivers/gpu/drm/i915/intel_display.c | 35 ++++++++++++----
> > drivers/gpu/drm/i915/intel_dp.c | 23 +---------
> > drivers/gpu/drm/i915/intel_dp_mst.c | 4 --
> > drivers/gpu/drm/i915/intel_dpll_mgr.c | 79 +++++++++++++++++++++++++++++++-
> > ---
> > drivers/gpu/drm/i915/intel_dpll_mgr.h | 5 ++-
> > drivers/gpu/drm/i915/intel_drv.h | 1 -
> > 7 files changed, 110 insertions(+), 55 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index ad7888c..3cb9f36 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -992,17 +992,13 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
> > {
> > struct intel_shared_dpll *pll;
> >
> > - if (intel_encoder->type == INTEL_OUTPUT_HDMI ||
> > - intel_encoder->type == INTEL_OUTPUT_ANALOG) {
> > - pll = intel_get_shared_dpll(intel_crtc, crtc_state,
> > - intel_encoder);
> > - if (!pll)
> > - DRM_DEBUG_DRIVER("failed to find PLL for pipe
> > %c\n",
> > - pipe_name(intel_crtc->pipe));
> > - return pll;
> > - } else {
> > - return true;
> > - }
> > + pll = intel_get_shared_dpll(intel_crtc, crtc_state,
> > + intel_encoder);
> > + if (!pll)
> > + DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
> > + pipe_name(intel_crtc->pipe));
> > +
> > + return pll;
> > }
> Eventually the reliance on intel_encoder->type should end here, and based on
> the connector.

Do you mean that the parameter to get_shared_dpll() should be connector instead
of encoder?

> It would fix some kms tests, but that would be better to do in a future patch.
> > ...
> >
> > +static bool hsw_ddi_lcpll_get_hw_state(struct drm_i915_private *dev_priv,
> > + struct intel_shared_dpll *pll,
> > + struct intel_dpll_hw_state
> > *hw_state)
> > +{
> > + /*
> > + * LC PLL is kept enabled all the time since it drives CDCLK. The
> > + * state checker still expects it to be disabled when it is not
> > used
> > + * by any crtc. To avoid adding a case to LC PLL, just tell the
> > + * state checker what it expects.
> > + */
> > + if (pll->config.crtc_mask)
> > + return true;
> > + else
> > + return false;
> > +}
> Wouldn't it be better to return true or the real hardware state then, and set
> the ALWAYS_ON flag from the next patch?

That's exactly what v2 of this patch does. :)

Ander

> > +static const struct intel_shared_dpll_funcs hsw_ddi_lcpll_funcs = {
> > + .enable = hsw_ddi_lcpll_enable,
> > + .disable = hsw_ddi_lcpll_disable,
> > + .get_hw_state = hsw_ddi_lcpll_get_hw_state,
> > +};
> > +
> > struct skl_dpll_regs {
> > i915_reg_t ctl, cfgcr1, cfgcr2;
> > };
> > @@ -1559,9 +1617,12 @@ static const struct intel_dpll_mgr pch_pll_mgr = {
> > };
> >
> > static const struct dpll_info hsw_plls[] = {
> > - { "WRPLL 1", DPLL_ID_WRPLL1, &hsw_ddi_wrpll_funcs },
> > - { "WRPLL 2", DPLL_ID_WRPLL2, &hsw_ddi_wrpll_funcs },
> > - { "SPLL", DPLL_ID_SPLL, &hsw_ddi_spll_funcs },
> > + { "WRPLL 1", DPLL_ID_WRPLL1, &hsw_ddi_wrpll_funcs },
> > + { "WRPLL 2", DPLL_ID_WRPLL2, &hsw_ddi_wrpll_funcs },
> > + { "SPLL", DPLL_ID_SPLL, &hsw_ddi_spll_funcs },
> > + { "LCPLL 810", DPLL_ID_LCPLL_810, &hsw_ddi_lcpll_funcs },
> > + { "LCPLL 1350", DPLL_ID_LCPLL_1350, &hsw_ddi_lcpll_funcs },
> > + { "LCPLL 2700", DPLL_ID_LCPLL_2700, &hsw_ddi_lcpll_funcs },
> > { NULL, -1, NULL, },
> > };
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > index 82e53f5..872878e 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > @@ -49,13 +49,16 @@ enum intel_dpll_id {
> > DPLL_ID_WRPLL1 = 0,
> > DPLL_ID_WRPLL2 = 1,
> > DPLL_ID_SPLL = 2,
> > + DPLL_ID_LCPLL_810 = 3,
> > + DPLL_ID_LCPLL_1350 = 4,
> > + DPLL_ID_LCPLL_2700 = 5,
> >
> > /* skl */
> > DPLL_ID_SKL_DPLL1 = 0,
> > DPLL_ID_SKL_DPLL2 = 1,
> > DPLL_ID_SKL_DPLL3 = 2,
> > };
> > -#define I915_NUM_PLLS 3
> > +#define I915_NUM_PLLS 6
> >
> > struct intel_dpll_hw_state {
> > /* i9xx, pch plls */
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index c9e5030..18aa287 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1258,7 +1258,6 @@ void intel_edp_drrs_invalidate(struct drm_device *dev,
> > void intel_edp_drrs_flush(struct drm_device *dev, unsigned
> > frontbuffer_bits);
> > bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
> > struct intel_digital_port *port);
> > -void hsw_dp_set_ddi_pll_sel(struct intel_crtc_state *pipe_config);
> >
> > void
> > intel_dp_program_link_training_pattern(struct intel_dp *intel_dp,
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.