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

From: Ander Conselvan De Oliveira <conselvan2@gmail.com>
To: Imre Deak <imre.deak@intel.com>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2] drm/i915/bxt: Fix off-by-one error in Broxton PLL IDs
Date: Tue, 15 Mar 2016 09:50:21 +0200	[thread overview]
Message-ID: <1458028221.2949.6.camel@gmail.com> (raw)
In-Reply-To: <1457978134-12362-1-git-send-email-imre.deak@intel.com>

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.

>  		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>

>  		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