Re: [PATCH v2] drm/i915/bxt: Fix off-by-one error in Broxton PLL IDs

[Imre Deak <imre.deak@intel.com>]

On ti, 2016-03-15 at 09:50 +0200, Ander Conselvan De Oliveira wrote:
> On Mon, 2016-03-14 at 19:55 +0200, Imre Deak wrote:
> > After the commit below the Broxton PLL IDs had an off-by-one error,
> > so
> > fix this up. Also add a missing brace at intel_shared_dpll_init(),
> > it
> > happened to compile only due to the way the IS_BROXTON macro is
> > defined.
> > 
> > v2:
> > - remove debugging left-over
> > 
> > Fixes: a3c988ea068c ("drm/i915: Make SKL/KBL DPLL0 managed by the
> > shared dpll
> > code")
> > CC: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.
> > com>
> > CC: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c  | 6 +++---
> >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 8 ++++----
> >  2 files changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index a720628..3cbb02b 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9762,15 +9762,15 @@ static void bxt_get_ddi_pll(struct
> > drm_i915_private
> > *dev_priv,
> >  	switch (port) {
> >  	case PORT_A:
> >  		pipe_config->ddi_pll_sel = SKL_DPLL0;
> > -		id = DPLL_ID_SKL_DPLL1;
> > +		id = DPLL_ID_SKL_DPLL0;
> >  		break;
> >  	case PORT_B:
> >  		pipe_config->ddi_pll_sel = SKL_DPLL1;
> > -		id = DPLL_ID_SKL_DPLL2;
> > +		id = DPLL_ID_SKL_DPLL1;
> >  		break;
> >  	case PORT_C:
> >  		pipe_config->ddi_pll_sel = SKL_DPLL2;
> > -		id = DPLL_ID_SKL_DPLL3;
> > +		id = DPLL_ID_SKL_DPLL2;
> 
> Hmm, I think the mistake is that BXT relied on SKL DPLL ids in the
> first place.
> This is fine as a fix up, but maybe it would be better to just add
> separate ids
> for broxton too. Or even better, get rid of pll->id.

Yes, SKL_DPLLx and DPLL_ID_SKL_DPLLx that were defined originally had
confusingly different indexes already, not sure for the reason for
that. Removing 'id' if possible seems like a good idea.

> 
> >  		break;
> >  	default:
> >  		DRM_ERROR("Incorrect port type\n");
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > index 4b636c4..74d5aec 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > @@ -1706,9 +1706,9 @@ static const struct intel_dpll_mgr
> > skl_pll_mgr = {
> >  };
> >  
> >  static const struct dpll_info bxt_plls[] = {
> > -	{ "PORT PLL A", 0, &bxt_ddi_pll_funcs, 0 },
> > -	{ "PORT PLL B", 1, &bxt_ddi_pll_funcs, 0 },
> > -	{ "PORT PLL C", 2, &bxt_ddi_pll_funcs, 0 },
> > +	{ "PORT PLL A", DPLL_ID_SKL_DPLL0, &bxt_ddi_pll_funcs, 0
> > },
> > +	{ "PORT PLL B", DPLL_ID_SKL_DPLL1, &bxt_ddi_pll_funcs, 0
> > },
> > +	{ "PORT PLL C", DPLL_ID_SKL_DPLL2, &bxt_ddi_pll_funcs, 0
> > },
> >  	{ NULL, -1, NULL, },
> >  };
> >  
> > @@ -1726,7 +1726,7 @@ void intel_shared_dpll_init(struct drm_device
> > *dev)
> >  
> >  	if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev))
> >  		dpll_mgr = &skl_pll_mgr;
> > -	else if IS_BROXTON(dev)
> > +	else if (IS_BROXTON(dev))
> 
> Wow, really don't know how I managed to do this. Thanks for fixing.
> 
> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

Thanks for the review.

> 
> >  		dpll_mgr = &bxt_pll_mgr;
> >  	else if (HAS_DDI(dev))
> >  		dpll_mgr = &hsw_pll_mgr;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx