On ti, 2016-09-27 at 16:04 +0300, Ander Conselvan De Oliveira wrote:
> On Tue, 2016-09-27 at 14:26 +0300, Imre Deak wrote:
> > On ti, 2016-09-27 at 11:03 +0300, Ander Conselvan De Oliveira
> > wrote:
> > >
> > > On Mon, 2016-09-26 at 17:54 +0300, Imre Deak wrote:
> > > >
> > > > a277ca7dc01d should've been a no-functional-change commit, but
> > > > it
> > > > removed the initialization of the dpll_hw_state for HDMI
> > > > outputs,
> > > > resulting in state mismatches and a failed modeset with blank
> > > > screen. Fix this by reinstating the dpll_hw_state
> > > > initialization.
> > > >
> > > > Fixes: a277ca7dc01d ("drm/i915: Split bxt_ddi_pll_select()")
> > > > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > > > ---
> > > > drivers/gpu/drm/i915/intel_dpll_mgr.c | 21 ++++++++++++++++---
> > > > --
> > > > 1 file changed, 16 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > index c26d18a..e8bf838 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > @@ -1694,21 +1694,32 @@ bool bxt_ddi_dp_set_dpll_hw_state(int
> > > > clock,
> > > > return bxt_ddi_set_dpll_hw_state(clock, &clk_div,
> > > > dpll_hw_state);
> > > > }
> > > >
> > > > +bool bxt_ddi_hdmi_set_dpll_hw_state(struct intel_crtc
> > > > *intel_crtc,
> > > > + struct intel_crtc_state
> > > > *crtc_state,
> > > > + int clock,
> > > > + struct intel_dpll_hw_state
> > > > *dpll_hw_state)
> > > > +{
> > > > + struct bxt_clk_div clk_div = { };
> > > > +
> > > > + bxt_ddi_hdmi_pll_dividers(intel_crtc, crtc_state,
> > > > clock,
> > > > &clk_div);
> > > > +
> > > > + return bxt_ddi_set_dpll_hw_state(clock, &clk_div,
> > > > dpll_hw_state);
> > > > +}
> > > > +
> > > > static struct intel_shared_dpll *
> > > > bxt_get_dpll(struct intel_crtc *crtc,
> > > > struct intel_crtc_state *crtc_state,
> > > > struct intel_encoder *encoder)
> > > > {
> > > > - struct bxt_clk_div clk_div = {0};
> > > > - struct intel_dpll_hw_state dpll_hw_state = {0};
> > > > + struct intel_dpll_hw_state dpll_hw_state = { };
> > > > struct drm_i915_private *dev_priv = to_i915(crtc-
> > > > >
> > > > > base.dev);
> > > > struct intel_digital_port *intel_dig_port;
> > > > struct intel_shared_dpll *pll;
> > > > int i, clock = crtc_state->port_clock;
> > > >
> > > > - if (encoder->type == INTEL_OUTPUT_HDMI
> > > > - && !bxt_ddi_hdmi_pll_dividers(crtc, crtc_state,
> > > > - clock, &clk_div))
> > > > + if (encoder->type == INTEL_OUTPUT_HDMI &&
> > > > + !bxt_ddi_hdmi_set_dpll_hw_state(crtc, crtc_state,
> > > > clock,
> > > > + &dpll_hw_state))
> > > In my original patch there was just a straight call
> > > to bxt_ddi_set_dpll_hw_state() after this and the DP if condition
> > > (which also only calculated dividers).
> > That would require exporting bxt_ddi_dp_pll_dividers(), so I just
> > left
> > it as-is.
>
> Not necessarily, bxt_ddi_dp_set_dpll_hw_state() could still be
> exported for the
> caller.
Ok, that would've worked. I was thinking along having the same calls
from intel_ddi_get_link_dpll() and bxt_get_dpll() for DPLL setup
to keep things more unified.
> But the current solution is fine.
>
> An even better solution would be to have a somewhat platform
> independent entry
> point for force enabling a DPLL independent of modeset with a given
> clock for
> DP. The caller of bxt_ddi_dp_set_dpll_hw_state() will immediately
> enable the
> DPLL anyway, so it could instead just ask the DPLL code to do the
> right thing
> and enable the PLL and leave as much of the platform specific part
> out of the
> caller.
There is no user yet for intel_ddi_get_link_dpll(), but having a
platform independent way to turn DPLLs on/off makes sense to me.
--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
