Re: [Intel-gfx] [PATCH 1/5] drm/i915/skl: Work around incorrect BIOS WRPLL PDIV programming

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Imre Deak <imre.deak@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 1/5] drm/i915/skl: Work around incorrect BIOS WRPLL PDIV programming
Date: Tue, 6 Oct 2020 02:37:58 +0300	[thread overview]
Message-ID: <20201005233758.GP6112@intel.com> (raw)
In-Reply-To: <20201005202605.GA1378377@ideak-desk.fi.intel.com>

On Mon, Oct 05, 2020 at 11:26:05PM +0300, Imre Deak wrote:
> On Mon, Oct 05, 2020 at 11:08:19PM +0300, Ville Syrjälä wrote:
> > On Sat, Oct 03, 2020 at 03:18:42AM +0300, Imre Deak wrote:
> > > The BIOS of at least one ASUS-Z170M system with an SKL I have programs
> > > the 101b WRPLL PDIV divider value, which is the encoding for PDIV=7 with
> > > bit#0 incorrectly set.
> > > 
> > > This happens with the
> > > 
> > > "3840x2160": 30 262750 3840 3888 3920 4000 2160 2163 2168 2191 0x48 0x9
> > > 
> > > HDMI mode (scaled from a 1024x768 src fb) set by BIOS and the
> > > 
> > > ref_clock=24000, dco_integer=383, dco_fraction=5802, pdiv=7, qdiv=1, kdiv=1
> > > 
> > > WRPLL parameters (assuming PDIV=7 was the intended setting). This
> > > corresponds to 262749 PLL frequency/port clock.
> > > 
> > > Later the driver sets the same mode for which it calculates the same
> > > dco_int/dco_frac/div WRPLL parameters (with the correct PDIV=7 encoding).
> > > 
> > > Based on the above, let's assume that PDIV=7 was intended and the HW
> > > just ignores bit#0 in the PDIV register field for this setting, treating
> > > 100b and 101b encodings the same way.
> > > 
> > > While at it add the MISSING_CASE() for the p0,p2 divider decodings.
> > > 
> > > v2: (Ville)
> > > - Add a define for the incorrect divider value.
> > > - Emit only a debug message when detecting the incorrect divider value.
> > > - Use fallthrough from the incorrect divider value case.
> > > - Add the MISSING_CASE()s.
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 14 ++++++++++++++
> > >  drivers/gpu/drm/i915/i915_reg.h               |  1 +
> > >  2 files changed, 15 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > > index e08684e34078..61cb558c60d1 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > > @@ -1602,12 +1602,26 @@ static int skl_ddi_wrpll_get_freq(struct drm_i915_private *i915,
> > >  	case DPLL_CFGCR2_PDIV_3:
> > >  		p0 = 3;
> > >  		break;
> > > +	default:
> > > +		if (p0 == DPLL_CFGCR2_PDIV_7_INVALID)
> > 
> > Why not just 'case DPLL_CFGCR2_PDIV_7_INVALID:' ?
> 
> So we can use fallthrough for both this one and the default case.

IMO trying to be fancy just makes the code harder to read.

> 
> > 
> > > +			/*
> > > +			 * Incorrect ASUS-Z170M BIOS setting, the HW seems to ignore bit#0,
> > > +			 * handling it the same way as PDIV_7.
> > > +			 */
> > > +			drm_dbg_kms(&i915->drm, "Invalid WRPLL PDIV divider value, fixing it.\n");
> > > +		else
> > > +			MISSING_CASE(p0);
> > > +
> > > +		fallthrough;
> > >  	case DPLL_CFGCR2_PDIV_7:
> > >  		p0 = 7;
> > >  		break;
> > >  	}
> > >  
> > >  	switch (p2) {
> > > +	default:
> > > +		MISSING_CASE(p2);
> > > +		fallthrough;
> > 
> > Is there a specific reason we fall through to the 5 and 7 cases for
> > bogus values?
> 
> Just to default to dividers that result in the minimum PLL freq.

I'd probably just set them to zero if they're bogus. Looks like
that should already give us warn and just return zero as the freq.

> 
> > 
> > >  	case DPLL_CFGCR2_KDIV_5:
> > >  		p2 = 5;
> > >  		break;
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 88c215cf97d4..d911583526db 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -10261,6 +10261,7 @@ enum skl_power_gate {
> > >  #define  DPLL_CFGCR2_PDIV_2 (1 << 2)
> > >  #define  DPLL_CFGCR2_PDIV_3 (2 << 2)
> > >  #define  DPLL_CFGCR2_PDIV_7 (4 << 2)
> > > +#define  DPLL_CFGCR2_PDIV_7_INVALID	(5 << 2)
> > >  #define  DPLL_CFGCR2_CENTRAL_FREQ_MASK	(3)
> > >  
> > >  #define DPLL_CFGCR1(id)	_MMIO_PIPE((id) - SKL_DPLL1, _DPLL1_CFGCR1, _DPLL2_CFGCR1)
> > > -- 
> > > 2.25.1
> > 
> > -- 
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx