[PATCH] i915: Initialize panel timing registers if VBIOS did not.

[PATCH] i915: Initialize panel timing registers if VBIOS did not.

[Bryan Freed]

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

The time between start of the pixel clock and backlight enable is a basic
panel timing constraint.  If the Panel Power On/Off registers are found
to be 0, assume we are booting without VBIOS initialization and set these
registers to something reasonable.

Change-Id: Ibed6cc10d46bf52fd92e0beb25ae3525b5eef99d
Signed-off-by: Bryan Freed <bfreed@chromium.org>
---
 drivers/gpu/drm/i915/intel_bios.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c
b/drivers/gpu/drm/i915/intel_bios.c
index ad030ff..943bbad 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -505,6 +505,15 @@ init_vbt_defaults(struct drm_i915_private *dev_priv)
  /* general features */
  dev_priv->int_tv_support = 1;
  dev_priv->int_crt_support = 1;
+
+ /* 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 */
+ I915_WRITE(PP_ON_DELAYS, 0x019007d0);
+
+ /* Set T3 to 35ms and Tx to 200ms */
+ I915_WRITE(PP_OFF_DELAYS, 0x015e07d0);
+ }
 }

 /**
-- 
1.7.1

[-- Attachment #1.2: Type: text/html, Size: 2181 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: Initialize panel timing registers if VBIOS did not.

[Chris Wilson]

On Thu, 7 Oct 2010 17:05:46 -0700, Bryan Freed <bfreed@chromium.org> wrote:
> The time between start of the pixel clock and backlight enable is a basic
> panel timing constraint.  If the Panel Power On/Off registers are found
> to be 0, assume we are booting without VBIOS initialization and set these
> registers to something reasonable.

That looks cleaner. Obviously my only concern is what happens if we ever
see a second device that posts without setting up the registers. We may as
well compile in the VBT for every single manufactured device... Or we
could add a ROM table the manufacturers must include that provides the
necessary register values for their hardware. There must be some
replacement for the BIOS, a device description table at least?

Does any one have a strong "this will damage my hardware" objection? Are
the values safe enough for *any* device?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] i915: Initialize panel timing registers if VBIOS did not.

[Bryan Freed]

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

I took a look at several panel datasheets I found around here, and the one
issue some of them mentioned is that these timings (particularly T2 on
powerup and T3 on powerdown) are used to prevent latch-up.

That makes sense.  We do not want to drive pixels to a device that has 0V on
VDD.  This change sets a default of 40ms / 35ms (as per our panel of
interest) in the event we see both values at 0ms.  So this change can only
make things better (ie, latch-up less likely).

One "weakness" is that I do not explicitly check the T2 and T3 timings in
the PP_ON and PP_OFF registers.  I try to detect a lack of VBIOS
initialization by checking for 0 valued registers.  I rather think this is
the right approach.

The other timings in these registers are cosmetic.  For example, "do not
turn on the backlight before the panel is driving its pixels which is T5
after turning on the pixel clock."

bryan.

On Fri, Oct 8, 2010 at 2:58 AM, Chris Wilson <chris@chris-wilson.co.uk>wrote:

> On Thu, 7 Oct 2010 17:05:46 -0700, Bryan Freed <bfreed@chromium.org>
> wrote:
> > The time between start of the pixel clock and backlight enable is a basic
> > panel timing constraint.  If the Panel Power On/Off registers are found
> > to be 0, assume we are booting without VBIOS initialization and set these
> > registers to something reasonable.
>
> That looks cleaner. Obviously my only concern is what happens if we ever
> see a second device that posts without setting up the registers. We may as
> well compile in the VBT for every single manufactured device... Or we
> could add a ROM table the manufacturers must include that provides the
> necessary register values for their hardware. There must be some
> replacement for the BIOS, a device description table at least?
>
> Does any one have a strong "this will damage my hardware" objection? Are
> the values safe enough for *any* device?
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
>

[-- Attachment #1.2: Type: text/html, Size: 2490 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: Initialize panel timing registers if VBIOS did not.

[Jim Gettys]

On 10/08/2010 02:35 PM, Bryan Freed wrote:
> I took a look at several panel datasheets I found around here, and the one
> issue some of them mentioned is that these timings (particularly T2 on
> powerup and T3 on powerdown) are used to prevent latch-up.
>
> That makes sense.  We do not want to drive pixels to a device that has 0V on
> VDD.  This change sets a default of 40ms / 35ms (as per our panel of
> interest) in the event we see both values at 0ms.  So this change can only
> make things better (ie, latch-up less likely).

Yeah, there is a minimum frame rate you need to drive LCD's at, which is 
somewhat dependent on the details of the liquid crystal material itself; 
you really don't want them to crystallise fully.  I know from working 
with Mary Lou Jepson on the OLPC LCD panel that 25hz was fully safe for 
that technology (and the liquid crystals themselves weren't anything 
unusual in that panel).  She is entirely expert at LCD technology. 
Having some sanity check seems good and correlates with what I know, to 
handle the case of not being given any valid information.
			- Jim
>
> One "weakness" is that I do not explicitly check the T2 and T3 timings in
> the PP_ON and PP_OFF registers.  I try to detect a lack of VBIOS
> initialization by checking for 0 valued registers.  I rather think this is
> the right approach.
>
> The other timings in these registers are cosmetic.  For example, "do not
> turn on the backlight before the panel is driving its pixels which is T5
> after turning on the pixel clock."
>
> bryan.
>
> On Fri, Oct 8, 2010 at 2:58 AM, Chris Wilson<chris@chris-wilson.co.uk>wrote:
>
>> On Thu, 7 Oct 2010 17:05:46 -0700, Bryan Freed<bfreed@chromium.org>
>> wrote:
>>> The time between start of the pixel clock and backlight enable is a basic
>>> panel timing constraint.  If the Panel Power On/Off registers are found
>>> to be 0, assume we are booting without VBIOS initialization and set these
>>> registers to something reasonable.
>>
>> That looks cleaner. Obviously my only concern is what happens if we ever
>> see a second device that posts without setting up the registers. We may as
>> well compile in the VBT for every single manufactured device... Or we
>> could add a ROM table the manufacturers must include that provides the
>> necessary register values for their hardware. There must be some
>> replacement for the BIOS, a device description table at least?
>>
>> Does any one have a strong "this will damage my hardware" objection? Are
>> the values safe enough for *any* device?
>> -Chris
>>
>> --
>> Chris Wilson, Intel Open Source Technology Centre
>>
>
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] i915: Initialize panel timing registers if VBIOS did not.

[Chris Wilson]

On Fri, 08 Oct 2010 10:58:40 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Does any one have a strong "this will damage my hardware" objection? Are
> the values safe enough for *any* device?

Or perhaps a better question is: does anyone else feel confident enough
in these defaults to ack them? ;-) Reviewed-by and tested-by a bonus.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] i915: Initialize panel timing registers if VBIOS did not.

[Jesse Barnes]

On Fri, 08 Oct 2010 21:28:01 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Fri, 08 Oct 2010 10:58:40 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > Does any one have a strong "this will damage my hardware" objection? Are
> > the values safe enough for *any* device?
> 
> Or perhaps a better question is: does anyone else feel confident enough
> in these defaults to ack them? ;-) Reviewed-by and tested-by a bonus.

I can't vouch for any particular set of values, but Bryan's analysis
looks good to me; only a few of the values actually matter, and for
those having slightly longer delays should be safe.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] i915: Initialize panel timing registers if VBIOS did not.

[Olof Johansson]

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

On Fri, Oct 8, 2010 at 4:17 PM, Jesse Barnes <jbarnes@virtuousgeek.org>wrote:

> On Fri, 08 Oct 2010 21:28:01 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> > On Fri, 08 Oct 2010 10:58:40 +0100, Chris Wilson <
> chris@chris-wilson.co.uk> wrote:
> > > Does any one have a strong "this will damage my hardware" objection?
> Are
> > > the values safe enough for *any* device?
> >
> > Or perhaps a better question is: does anyone else feel confident enough
> > in these defaults to ack them? ;-) Reviewed-by and tested-by a bonus.
>
> I can't vouch for any particular set of values, but Bryan's analysis
> looks good to me; only a few of the values actually matter, and for
> those having slightly longer delays should be safe.
>

Sounds like a soft ack to me. :)

Chris, can you pick this up? Alternatively, guidance on how it should be
reworked to be acceptable would be appreciated.


Thanks,

-Olof

[-- Attachment #1.2: Type: text/html, Size: 1446 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: Initialize panel timing registers if VBIOS did not.

[Chris Wilson]

On Wed, 13 Oct 2010 16:00:44 -0500, Olof Johansson <olof@lixom.net> wrote:
> On Fri, Oct 8, 2010 at 4:17 PM, Jesse Barnes <jbarnes@virtuousgeek.org>wrote:
> > I can't vouch for any particular set of values, but Bryan's analysis
> > looks good to me; only a few of the values actually matter, and for
> > those having slightly longer delays should be safe.
> >
> 
> Sounds like a soft ack to me. :)

Close enough. I rearranged the code slightly so that the act of fixing up
registers was separated from the code who purpose is simply to parse the
VBIOS and pushed into drm-intel-staging.

If you could check it over for obvious mistakes as it will land in -next
just as soon as I've fixed up a troublesome merge with nouveau.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre