Re: [PATCH] drm/i915: Use SWF06 to figure out max cdclk for BDW

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Feb 16, 2016 at 07:13:05AM +0530, Thulasimani, Sivakumar wrote:
> 
> 
> On 2/12/2016 8:36 PM, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > Bspec tells us that we can allow cdclk up to 540Mhz on BDW ULX, or
> > up to 675 MHz on ULT, bu only if extra cooling is provided. There
> > don't seem to be any strap or VBT bits to tells us this however.
> >
> > But I did spot something potentially relevant in
> > VBIOS_GOP_Driver_SWF_Registers.pdf. Apparently VBIOS/GOP can pass
> > the max cdclk frequeny in SWF06 to the driver. Let's assume the firmware
> > knows what its doing and trust the max cdclk in SWF06 if it's higher
> > than the basic limit specified in Bspec. To avoid regressing anything
> > let's ignore SWF06 if it indicates a lower limit than Bspec.
> >
> > Cc: Clint Taylor <clinton.a.taylor@xxxxxxxxx>
> > Cc: "Thulasimani, Sivakumar" <sivakumar.thulasimani@xxxxxxxxx>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> >
> > I'm not at all sure if this is the right way to go about it. Sivakumar,
> > since you seem to have some ideas on this maybe you can have a look.
> > I'm not aware of any complaints so far that people can't get the cdclk
> > as high is they should on specific machines, so not sure if this is really
> > even needed.
> >
> > The other open question is what we should do if the VBIOS limit is
> > lower than what we'd expect based on BSpec. Should we still trust it?
> > Sadly we can't verify the SWF06 cdclk value in any way since it
> > only has two bits and we have four possible cdclk values.
> The value stored in SWF06 is the CD Clock VBIOS/GOP sees during boot. so
> it just backs up the CD Clock before it optimizes for the available LFP. 
> if we
> are trusting for higher value we should trust it for lower value too.
> The OEM might have did this to either reduce max resolution for cheaper
> products or might have removed some cooling mechanisms for thinner
> designs since we cannot say which of the reasons we should trust the
> lower value too.
> 
> now comes to tough part :(, this SWF06 is saved by VBIOS/GOP driver
> from intel alone(it is not programmed from VBT),
> since GOP driver can be implemented by anyone and
> if anyone implements their own GOP driver we cannot be sure if the
> value is valid or not. please check if we can check for "Intel GOP driver".
> And if "Intel GOP driver" was used during boot, we can trust the value
> 100%.  i am not sure how this can be done, so i would recommend
> trusting the values with clear debug messages as done below already.

We definitely need a way to validate the register value before we trust
it for lower values. I suppose we might be able to look at bits 31:16
since those should store the current cdclk "decimal" value. If that part
looks reasonable, we might be able to trust the "max cdclk" bits as well.

> 
> As mentioned in another thread, this needs to be done for SKL too.
> i dont have a SKL system so if no one else can make a patch for it
> i will have to share an untested patch for it :).

For SKL it seems a bit easier, since it apparently just stores the max
cdclk in 10.1 decimal fixed point in bits 10:0. So invalid/uninitialized
values would be easier to spot.

Hmm. 11 bits and 10.1 fixed point format, I wonder if it's really
decimal and not binary fixed point...

> >
> >   drivers/gpu/drm/i915/intel_display.c | 60 ++++++++++++++++++++++++++++--------
> >   1 file changed, 47 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 836bbdc239b6..1d70f6bf2934 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5407,21 +5407,55 @@ static void intel_update_max_cdclk(struct drm_device *dev)
> >   			dev_priv->max_cdclk_freq = 450000;
> >   		else
> >   			dev_priv->max_cdclk_freq = 337500;
> > -	} else if (IS_BROADWELL(dev))  {
> > +	} else if (IS_BROADWELL(dev)) {
> > +		int bios_max_cdclk_freq, max_cdclk_freq;
> > +
> >   		/*
> > -		 * FIXME with extra cooling we can allow
> > -		 * 540 MHz for ULX and 675 Mhz for ULT.
> > -		 * How can we know if extra cooling is
> > -		 * available? PCI ID, VTB, something else?
> > +		 * With extra cooling we can allow 540 MHz for
> > +		 * ULX and 675 Mhz for ULT. Assume VBIOS/GOP
> > +		 * passes that information in SWF06.
> >   		 */
> > -		if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT)
> > -			dev_priv->max_cdclk_freq = 450000;
> > -		else if (IS_BDW_ULX(dev))
> > -			dev_priv->max_cdclk_freq = 450000;
> > -		else if (IS_BDW_ULT(dev))
> > -			dev_priv->max_cdclk_freq = 540000;
> > -		else
> > -			dev_priv->max_cdclk_freq = 675000;
> > +		switch (I915_READ(SWF_ILK(0x06)) & 0x3) {
> > +		case 0:
> > +			bios_max_cdclk_freq = 450000;
> > +			break;
> > +		case 1:
> > +			bios_max_cdclk_freq = 540000;
> > +			break;
> > +		case 2:
> > +			bios_max_cdclk_freq = 337500;
> > +			break;
> > +		case 3:
> > +			bios_max_cdclk_freq = 675000;
> > +			break;
> > +		}
> > +
> > +		if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT) {
> > +			if (WARN_ON(bios_max_cdclk_freq != 450000))
> > +				bios_max_cdclk_freq = 450000;
> > +			max_cdclk_freq = 450000;
> > +		} else if (IS_BDW_ULX(dev_priv)) {
> > +			if (WARN_ON(bios_max_cdclk_freq > 540000))
> > +				bios_max_cdclk_freq = 540000;
> > +			max_cdclk_freq = 450000;
> > +		} else if (IS_BDW_ULT(dev_priv)) {
> > +			max_cdclk_freq = 540000;
> > +		} else {
> > +			max_cdclk_freq = 675000;
> > +		}
> > +
> > +		if (bios_max_cdclk_freq > max_cdclk_freq) {
> > +			DRM_DEBUG_KMS("VBIOS/GOP max cdclk (%d kHz) higher than basic limit (%d kHz), "
> > +				      "assuming extra cooling is present\n",
> > +				      bios_max_cdclk_freq, max_cdclk_freq);
> > +			max_cdclk_freq = bios_max_cdclk_freq;
> > +		} else if (bios_max_cdclk_freq < max_cdclk_freq) {
> > +			DRM_DEBUG_KMS("VBIOS/GOP max cdclk (%d kHz) lower than basic limit (%d kHz), "
> > +				      "ignoring it\n",
> > +				      bios_max_cdclk_freq, max_cdclk_freq);
> > +		}
> for the reasons mentioned above, i would favor trusting lower values too.
> 
> regards,
> Sivakumar
> > +
> > +		dev_priv->max_cdclk_freq = max_cdclk_freq;
> >   	} else if (IS_CHERRYVIEW(dev)) {
> >   		dev_priv->max_cdclk_freq = 320000;
> >   	} else if (IS_VALLEYVIEW(dev)) {

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux