On Tue, Feb 23, 2016 at 06:40:37PM +0530, Thulasimani, Sivakumar wrote:
>
>
> On 2/23/2016 4:35 PM, Ville Syrjälä wrote:
> > On Tue, Feb 23, 2016 at 04:22:01PM +0530, Thulasimani, Sivakumar wrote:
> >>
> >> On 2/16/2016 3:35 PM, Ville Syrjälä wrote:
> >>> 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.
> >> it seems the bits 31:16 are used only by VBIOS and not GOP, so that wont
> >> help.
> >> we need to come up with some other method to confirm the value or verify
> >> if intel gop is loaded. i will get back if i can find such a mechanism.
> > Oh, that's unfortunate. IIRC we use some other SWF register to check if
> > something already initialized things in the cdclk sanitation path.
> > Not sure if the same could be used here.
> i am not aware of any other SWF register used for cdclk related flow so
> cant help there.
I think the register was more of a canary and not directly related to
cdclk. And it was... SWF18. Hmm. Based on the spec that would indicate
some kind of initial pipe->connector mapping. So if there are no
displays connected, I suppose it might end up showing nothing either.
So yeah, probably not suitable for this stuff after all :(
> had a discussion with local folks and it seems like there is
> no easy way out atleast for BDW.
>
> SKL register using literal values is helpful in
> verifying against available set.
>
> my best guess would be to handle it for BDW is as follows
> if BIT0:1 == 0 && current_cdclk != 450MHz
> we are not in intel GOP driver so take the current clock as max
> if BIT0:1 == 0 && current_cdclk == 450MHz
> cant say which GOP driver was used, so limit to current clock
> as max
> if BIT0:1 != 0
> probability of some other component setting these two bits is
> very low :)
> so assume that we are in intel gop driver and trust the value.
> we have checks to limit clock for the SKU so it should not be a
> problem
> to trust the value in register.
>
>
> regards,
> Sivakumar
> >>>> 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...
> >> it uses the same value programmed in CDCLK_CTL register.
> >> i.e 01 0011 0011 1b = 308.57MHz
> > OK, so binary fixed point. Maybe I should file a bug against bspec to
> > remove this confusing "decimal" naming scheme from the cdclk stuff...
> >
> >> regards,
> >> Sivakumar
> >>>>> 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
