[PATCH] i915: Modify for pineview clock source

[PATCH] i915: Modify for pineview clock source

[bfreed]

The i915 driver normally assumes the video bios has configured several 
of the LVDS panel registers, and it just inherits the values. If the 
vbios has not run, several of these will need to be setup. 

intel_bios.c: default clock source selection on pineview to use the SSC source.

If these are not correct then although the panel looks ok, output from an HDMI
encoder (eg, Chrontel CH7036) will be incorrect. 

Signed-off-by: Mark Hayter <mdhayter@chromium.org>

Index: drivers/gpu/drm/i915/intel_bios.c
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 943bbad066af79d6fc62891014f15479213aa34e..e2f3629b46a028077aee214bbc323a1b3ca9ca8f 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -506,6 +506,14 @@ init_vbt_defaults(struct drm_i915_private *dev_priv)
 	dev_priv->int_tv_support = 1;
 	dev_priv->int_crt_support = 1;
 
+	if (IS_PINEVIEW(dev_priv->dev)) {
+		/* On Pineview flip default to use SSC (VBT can override) */
+		dev_priv->lvds_use_ssc = 1;
+		/* The ssc pin gets 100MHz downspread from timing gen */
+		dev_priv->lvds_ssc_freq = 100;
+	} else {
+		dev_priv->lvds_use_ssc = 0;
+	}
 	/* Set the Panel Power On/Off timings if uninitialized. */
 	if ((I915_READ(PP_ON_DELAYS) == 0) && (I915_READ(PP_OFF_DELAYS) == 0)) {
 		/* Set T2 to 40ms and T5 to 200ms */

Re: [PATCH] i915: Modify for pineview clock source

[Chris Wilson]

On Tue, 14 Dec 2010 11:40:30 -0800, bfreed@chromium.org wrote:
> The i915 driver normally assumes the video bios has configured several 
> of the LVDS panel registers, and it just inherits the values. If the 
> vbios has not run, several of these will need to be setup. 

I'm having problems applying these using git-am. Notably your base objects
are not in my tree.

I'm dubious about flipping the SSC bit just for PineView. On recent
platforms, I think using an SSC reference is prevalent.

Instead of hardcoding 100Mhz as the clock, I'd prefer to see those SSC
frequencies split out of the general definitions and shared with this
initialiser. Something like:
int intel_bios_ssc_frequency(struct drm_device *dev, bool high_speed)
{
   switch(INTEL_INFO(dev)->gen) {
   case 2: return high_speed ? 66 : 48;
   case 3:
   case 4: return high_speed ? 100 : 96;
   default: return high_speed ? 120 : 100;
   }
}
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] i915: Modify for pineview clock source

[Mark Hayter]

[-- Attachment #1.1: Type: text/plain, Size: 1860 bytes --]

[Again using correct address to get to list, sorry about that]

>
Chris,

>
I am happy to use the ssc frequency setting code and expand beyond pineview
(although I can't really test those configurations), but Im not sure what to
do about the high_speed flag. This would normally come from the BDB general
features ssc_freq flag. Since the code is to set defaults that will be used
in the absence of VBIOS/VBT would it be acceptable to just set high_speed to
1 when I call intel_bios_ssc_frequency to configure the speed, so that for
pineview it matches the original code? This would be correct for the
machines we know that do not have VBIOS and is otherwise as arbitrary as
using 0.

>
Thanks

> Mark

>
On Wed, Dec 15, 2010 at 6:52 AM, Chris Wilson <chris@chris-wilson.co.uk>wrote:

>
On Tue, 14 Dec 2010 11:40:30 -0800, bfreed@chromium.org wrote:
>
>> > The i915 driver normally assumes the video bios has configured several
>
>> > of the LVDS panel registers, and it just inherits the values. If the
>
>> > vbios has not run, several of these will need to be setup.
>
>>
> I'm having problems applying these using git-am. Notably your base objects
>
>> are not in my tree.
>
>>
> I'm dubious about flipping the SSC bit just for PineView. On recent
>
>> platforms, I think using an SSC reference is prevalent.
>
>>
> Instead of hardcoding 100Mhz as the clock, I'd prefer to see those SSC
>
>> frequencies split out of the general definitions and shared with this
>
>> initialiser. Something like:
>
>> int intel_bios_ssc_frequency(struct drm_device *dev, bool high_speed)
>
>> {
>
>>   switch(INTEL_INFO(dev)->gen) {
>
>>   case 2: return high_speed ? 66 : 48;
>
>>   case 3:
>
>>   case 4: return high_speed ? 100 : 96;
>
>>   default: return high_speed ? 120 : 100;
>
>>   }
>
>> }
>
>> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
>

[-- Attachment #1.2: Type: text/html, Size: 6734 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] i915: Modify for pineview clock source

[Dave Airlie]

> Chris,
>
> I am happy to use the ssc frequency setting code and expand beyond pineview
> (although I can't really test those configurations), but Im not sure what to
> do about the high_speed flag. This would normally come from the BDB general
> features ssc_freq flag. Since the code is to set defaults that will be used
> in the absence of VBIOS/VBT would it be acceptable to just set high_speed to
> 1 when I call intel_bios_ssc_frequency to configure the speed, so that for
> pineview it matches the original code? This would be correct for the
> machines we know that do not have VBIOS and is otherwise as arbitrary as
> using 0.

I have to say they are both seemingly arbitrary and pointless.

What happens if I say my pineview LVDS panel needs it the other way
and I don't have a VBIOS either? who wins?

Really the kernel should just fail if the parameter isn't available
from the VBIOS, and maybe you could add some sort of command line or
even openfirmware type table to store this info.

Having the driver making crappy decision because someone thought not
shipping a bios with the hw details in it is hardly what I call
engineering.

Dave.