[Intel-gfx] [PATCH] drm/i915/bxt: Fix HDMI DPLL configuration

[Imre Deak]

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 at intel.com>
> > > > ---
> > > >  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