Re: radeonfb i2c regression post-2.6.18.

Re: radeonfb i2c regression post-2.6.18.

[Benjamin Herrenschmidt]

On Sun, 2007-11-18 at 21:58 +0000, Roger Leigh wrote:
> Hi folks,
> 
> I'm using an Apple Cinema Display connected via DVI to an Apple Mac
> Mini with Radeon 9200 graphics. This used to work fine, but with
> kernels >= 2.6.19, the monitor powers off as soon as the framebuffer
> is initialised, making for a less than usable system. I've tested
> with 2.6.(19|2[012]), and I didn't see any related changes in 2.6.23.

   .../...

Can you try the patch from Jean that I pasted below and let us know if
it helps ? It looks like the releasing of the i2c lines may have been
done backward.

(Also, Michel, can you check if it fixes your other problem with this
code ? ie. your "hot crash")

Cheers,
Ben.

-------- Forwarded Message --------
From: Jean Delvare <khali@linux-fr.org>
To: linux-fbdev-devel@lists.sourceforge.net
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>, Antonino Daplas
<adaplas@gmail.com>
Subject: [PATCH] fb_ddc: Fix DDC lines quirk
Date: Sun, 18 Nov 2007 14:21:41 +0100

The code in fb_ddc_read() is said to be based on the implementation
of the radeon driver:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=fc5891c8a3ba284f13994d7bc1f1bfa8283982de

However, comparing the old radeon driver code with the new fb_ddc code
reveals some differences. Most notably, the I2C bus lines are held at
the end of the function, while the original code was releasing them
(as the comment above correctly says.)

There are a few other differences, which appear to be responsible for
read failures on my system. While tracing low-level I2C code in
i2c-algo-bit, I noticed that the initial attempt to read the EDID
always failed. It takes one retry for the read to succeed. As we are
about to remove this automatic retry property from i2c-algo-bit,
reading the EDID would really fail.

As a summary, the I2C lines quirk which is supposedly needed to read
EDID on some older monitors is currently breaking the (first) read on
all other monitors (and might not even work with older ones - did
anyone try since October 2006?)

After applying the patch below, which makes the code in fb_ddc_read()
really similar to what the radeon driver used to have, the first EDID
read succeeds again.

On top of that, as it appears that this code has been broken for one
year now and nobody seems to have complained, I'm curious if it makes
sense to keep this quirk in place. It makes the code more complex and
slower just for the sake of monitors which I guess nobody uses
anymore. Can't we just get rid of it?

Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
 drivers/video/fb_ddc.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

--- linux-2.6.24-rc3.orig/drivers/video/fb_ddc.c	2007-11-17 20:23:03.000000000 +0100
+++ linux-2.6.24-rc3/drivers/video/fb_ddc.c	2007-11-18 12:49:14.000000000 +0100
@@ -56,13 +56,12 @@ unsigned char *fb_ddc_read(struct i2c_ad
 	int i, j;
 
 	algo_data->setscl(algo_data->data, 1);
-	algo_data->setscl(algo_data->data, 0);
 
 	for (i = 0; i < 3; i++) {
 		/* For some old monitors we need the
 		 * following process to initialize/stop DDC
 		 */
-		algo_data->setsda(algo_data->data, 0);
+		algo_data->setsda(algo_data->data, 1);
 		msleep(13);
 
 		algo_data->setscl(algo_data->data, 1);
@@ -97,14 +96,15 @@ unsigned char *fb_ddc_read(struct i2c_ad
 		algo_data->setsda(algo_data->data, 1);
 		msleep(15);
 		algo_data->setscl(algo_data->data, 0);
+		algo_data->setsda(algo_data->data, 0);
 		if (edid)
 			break;
 	}
 	/* Release the DDC lines when done or the Apple Cinema HD display
 	 * will switch off
 	 */
-	algo_data->setsda(algo_data->data, 0);
-	algo_data->setscl(algo_data->data, 0);
+	algo_data->setsda(algo_data->data, 1);
+	algo_data->setscl(algo_data->data, 1);
 
 	return edid;
 }

Re: radeonfb i2c regression post-2.6.18.

[Roger Leigh]

[-- Attachment #1: Type: text/plain, Size: 1044 bytes --]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

> On Sun, 2007-11-18 at 21:58 +0000, Roger Leigh wrote:
>> Hi folks,
>> 
>> I'm using an Apple Cinema Display connected via DVI to an Apple Mac
>> Mini with Radeon 9200 graphics. This used to work fine, but with
>> kernels >= 2.6.19, the monitor powers off as soon as the framebuffer
>> is initialised, making for a less than usable system. I've tested
>> with 2.6.(19|2[012]), and I didn't see any related changes in 2.6.23.
>
>    .../...
>
> Can you try the patch from Jean that I pasted below and let us know if
> it helps ? It looks like the releasing of the i2c lines may have been
> done backward.

This patch fixes the problem.  The monitor stays powered on during the
switch to the framebuffer.


Thanks,
Roger

-- 
  .''`.  Roger Leigh
 : :' :  Debian GNU/Linux             http://people.debian.org/~rleigh/
 `. `'   Printing on GNU/Linux?       http://gutenprint.sourceforge.net/
   `-    GPG Public Key: 0x25BFB848   Please GPG sign your mail.

[-- Attachment #2: Type: application/pgp-signature, Size: 188 bytes --]

Re: radeonfb i2c regression post-2.6.18.

[Benjamin Herrenschmidt]

> > Can you try the patch from Jean that I pasted below and let us know if
> > it helps ? It looks like the releasing of the i2c lines may have been
> > done backward.
> 
> This patch fixes the problem.  The monitor stays powered on during the
> switch to the framebuffer.

Excellent ! That saves me having to test myself :-)

As far as I'm concerned, that's an Ack for the patch.

Ben.

Re: radeonfb i2c regression post-2.6.18.

[Michael Buesch]

On Wednesday 21 November 2007 20:58:09 Benjamin Herrenschmidt wrote:
> 
> > > Can you try the patch from Jean that I pasted below and let us know if
> > > it helps ? It looks like the releasing of the i2c lines may have been
> > > done backward.
> > 
> > This patch fixes the problem.  The monitor stays powered on during the
> > switch to the framebuffer.
> 
> Excellent ! That saves me having to test myself :-)
> 
> As far as I'm concerned, that's an Ack for the patch.

I'll test tomorrow if that also fixes my problem.
You'll hear from me.

-- 
Greetings Michael.

Re: radeonfb i2c regression post-2.6.18.

[Benjamin Herrenschmidt]

On Wed, 2007-11-21 at 21:13 +0100, Michael Buesch wrote:
> On Wednesday 21 November 2007 20:58:09 Benjamin Herrenschmidt wrote:
> > 
> > > > Can you try the patch from Jean that I pasted below and let us know if
> > > > it helps ? It looks like the releasing of the i2c lines may have been
> > > > done backward.
> > > 
> > > This patch fixes the problem.  The monitor stays powered on during the
> > > switch to the framebuffer.
> > 
> > Excellent ! That saves me having to test myself :-)
> > 
> > As far as I'm concerned, that's an Ack for the patch.
> 
> I'll test tomorrow if that also fixes my problem.
> You'll hear from me.

Thanks Michael !

Cheers,
Ben.

Re: radeonfb i2c regression post-2.6.18.

[Roger Leigh]

[-- Attachment #1: Type: text/plain, Size: 1129 bytes --]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

>> > Can you try the patch from Jean that I pasted below and let us know if
>> > it helps ? It looks like the releasing of the i2c lines may have been
>> > done backward.
>> 
>> This patch fixes the problem.  The monitor stays powered on during the
>> switch to the framebuffer.
>
> Excellent ! That saves me having to test myself :-)
>
> As far as I'm concerned, that's an Ack for the patch.

Fantastic, thanks!  I've copied this to Debian bugs 433236 and 426124
which were about this problem.

BTW, the framebuffer penguin logo looked a little wierd (low number of
colours, odd colours), though on my powerpc it has always looked odd
(wrong colours).  Could there be some endianness bug in the fblogo
code?  I'll check it with other video options when I next have a few
minutes.


Thanks again,
Roger

-- 
  .''`.  Roger Leigh
 : :' :  Debian GNU/Linux             http://people.debian.org/~rleigh/
 `. `'   Printing on GNU/Linux?       http://gutenprint.sourceforge.net/
   `-    GPG Public Key: 0x25BFB848   Please GPG sign your mail.

[-- Attachment #2: Type: application/pgp-signature, Size: 188 bytes --]

Re: radeonfb i2c regression post-2.6.18.

[Benjamin Herrenschmidt]

On Wed, 2007-11-21 at 23:56 +0000, Roger Leigh wrote:
> Fantastic, thanks!  I've copied this to Debian bugs 433236 and 426124
> which were about this problem.
> 
> BTW, the framebuffer penguin logo looked a little wierd (low number of
> colours, odd colours), though on my powerpc it has always looked odd
> (wrong colours).  Could there be some endianness bug in the fblogo
> code?  I'll check it with other video options when I next have a few
> minutes.

Strange, it's always been working fine for me.

Ben.

Re: radeonfb i2c regression post-2.6.18.

[Michael Buesch]

On Monday 19 November 2007 00:16:06 Benjamin Herrenschmidt wrote:
> (Also, Michel, can you check if it fixes your other problem with this
> code ? ie. your "hot crash")

> -------- Forwarded Message --------
> From: Jean Delvare <khali@linux-fr.org>
> To: linux-fbdev-devel@lists.sourceforge.net
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>, Antonino Daplas
> <adaplas@gmail.com>
> Subject: [PATCH] fb_ddc: Fix DDC lines quirk
> Date: Sun, 18 Nov 2007 14:21:41 +0100
> 
> The code in fb_ddc_read() is said to be based on the implementation
> of the radeon driver:
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=fc5891c8a3ba284f13994d7bc1f1bfa8283982de
> 
> However, comparing the old radeon driver code with the new fb_ddc code
> reveals some differences. Most notably, the I2C bus lines are held at
> the end of the function, while the original code was releasing them
> (as the comment above correctly says.)
> 
> There are a few other differences, which appear to be responsible for
> read failures on my system. While tracing low-level I2C code in
> i2c-algo-bit, I noticed that the initial attempt to read the EDID
> always failed. It takes one retry for the read to succeed. As we are
> about to remove this automatic retry property from i2c-algo-bit,
> reading the EDID would really fail.
> 
> As a summary, the I2C lines quirk which is supposedly needed to read
> EDID on some older monitors is currently breaking the (first) read on
> all other monitors (and might not even work with older ones - did
> anyone try since October 2006?)
> 
> After applying the patch below, which makes the code in fb_ddc_read()
> really similar to what the radeon driver used to have, the first EDID
> read succeeds again.
> 
> On top of that, as it appears that this code has been broken for one
> year now and nobody seems to have complained, I'm curious if it makes
> sense to keep this quirk in place. It makes the code more complex and
> slower just for the sake of monitors which I guess nobody uses
> anymore. Can't we just get rid of it?

This patch fixes my crash problem.
Thanks a lot guys!

> Signed-off-by: Jean Delvare <khali@linux-fr.org>

Acked-by: Michael Buesch <mb@bu3sch.de>

> ---
>  drivers/video/fb_ddc.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> --- linux-2.6.24-rc3.orig/drivers/video/fb_ddc.c	2007-11-17 20:23:03.000000000 +0100
> +++ linux-2.6.24-rc3/drivers/video/fb_ddc.c	2007-11-18 12:49:14.000000000 +0100
> @@ -56,13 +56,12 @@ unsigned char *fb_ddc_read(struct i2c_ad
>  	int i, j;
>  
>  	algo_data->setscl(algo_data->data, 1);
> -	algo_data->setscl(algo_data->data, 0);
>  
>  	for (i = 0; i < 3; i++) {
>  		/* For some old monitors we need the
>  		 * following process to initialize/stop DDC
>  		 */
> -		algo_data->setsda(algo_data->data, 0);
> +		algo_data->setsda(algo_data->data, 1);
>  		msleep(13);
>  
>  		algo_data->setscl(algo_data->data, 1);
> @@ -97,14 +96,15 @@ unsigned char *fb_ddc_read(struct i2c_ad
>  		algo_data->setsda(algo_data->data, 1);
>  		msleep(15);
>  		algo_data->setscl(algo_data->data, 0);
> +		algo_data->setsda(algo_data->data, 0);
>  		if (edid)
>  			break;
>  	}
>  	/* Release the DDC lines when done or the Apple Cinema HD display
>  	 * will switch off
>  	 */
> -	algo_data->setsda(algo_data->data, 0);
> -	algo_data->setscl(algo_data->data, 0);
> +	algo_data->setsda(algo_data->data, 1);
> +	algo_data->setscl(algo_data->data, 1);
>  
>  	return edid;
>  }
> 
> 
> 
> 
> 



-- 
Greetings Michael.

Re: radeonfb i2c regression post-2.6.18.

[Jean Delvare]

On Fri, 23 Nov 2007 17:00:52 +0100, Michael Buesch wrote:
> This patch fixes my crash problem.

Out of curiosity, what kind of crash was it? I admit that I can't see
how the code could crash.

-- 
Jean Delvare

Re: radeonfb i2c regression post-2.6.18.

[Benjamin Herrenschmidt]

On Fri, 2007-11-23 at 23:29 +0100, Jean Delvare wrote:
> On Fri, 23 Nov 2007 17:00:52 +0100, Michael Buesch wrote:
> > This patch fixes my crash problem.
> 
> Out of curiosity, what kind of crash was it? I admit that I can't see
> how the code could crash.

Really sneaky... apparently, keeping the i2c lines asserted on his
laptop model would drain enough current through the pullups (or the
chip) that the temperature will raise significantly, causing a thermal
shutdown if the machine was already warm.

A bit scary... looks to me that a pullup is a bit too weak somewhere on
the motherboard.

That also means that this fix should reduce power consumption on the
battery significantly on those machines as it must take quite a bit of
power to increase the temperature that significantly (either that, or
the heating part sits just next to the sensor).

Cheers,
Ben. 

Re: radeonfb i2c regression post-2.6.18.

[Jean Delvare]

On Sat, 24 Nov 2007 12:11:04 +1100, Benjamin Herrenschmidt wrote:
> 
> On Fri, 2007-11-23 at 23:29 +0100, Jean Delvare wrote:
> > On Fri, 23 Nov 2007 17:00:52 +0100, Michael Buesch wrote:
> > > This patch fixes my crash problem.
> > 
> > Out of curiosity, what kind of crash was it? I admit that I can't see
> > how the code could crash.
> 
> Really sneaky... apparently, keeping the i2c lines asserted on his
> laptop model would drain enough current through the pullups (or the
> chip) that the temperature will raise significantly, causing a thermal
> shutdown if the machine was already warm.
> 
> A bit scary... looks to me that a pullup is a bit too weak somewhere on
> the motherboard.
> 
> That also means that this fix should reduce power consumption on the
> battery significantly on those machines as it must take quite a bit of
> power to increase the temperature that significantly (either that, or
> the heating part sits just next to the sensor).

Wow, nasty. Then my patch really needs to go to Linus at the earliest.

Thanks,
-- 
Jean Delvare

Re: radeonfb i2c regression post-2.6.18.

[Michael Buesch]

On Friday 23 November 2007 23:29:28 Jean Delvare wrote:
> On Fri, 23 Nov 2007 17:00:52 +0100, Michael Buesch wrote:
> > This patch fixes my crash problem.
> 
> Out of curiosity, what kind of crash was it? I admit that I can't see
> how the code could crash.
> 

It's not the code that crashes. It's the hardware that turns off the machine.
It only happens if I boot the machine and only if it's hot at this
time. Some hardware will turn off the hardware two or three seconds
after the radeon driver was loaded.
It seems to be some overheating protection that's going crazy.

-- 
Greetings Michael.

Re: radeonfb i2c regression post-2.6.18.

[Jean Delvare]

On Sat, 24 Nov 2007 15:18:26 +0100, Michael Buesch wrote:
> On Friday 23 November 2007 23:29:28 Jean Delvare wrote:
> > Out of curiosity, what kind of crash was it? I admit that I can't see
> > how the code could crash.
> 
> It's not the code that crashes. It's the hardware that turns off the machine.
> It only happens if I boot the machine and only if it's hot at this
> time. Some hardware will turn off the hardware two or three seconds
> after the radeon driver was loaded.
> It seems to be some overheating protection that's going crazy.

Very strange indeed. Another possibility is that there is a hardware
monitoring chip connected to one of the Radeon adapter's I2C buses, and
that holding the I2C lines prevents reading from it, so whatever is
responsible for controlling the temperature prefers to play it safe and
shuts everything down. Somehow it seems more realistic than an actual
overheating (3 seconds is a very short period of time for that), but
we'd need the exact schematics of the hardware, and the details of the
thermal control system, to validate this theory.

Anyway, no need to worry anymore now that the bug is fixed :)

-- 
Jean Delvare

Re: radeonfb i2c regression post-2.6.18.

[Benjamin Herrenschmidt]

> Very strange indeed. Another possibility is that there is a hardware
> monitoring chip connected to one of the Radeon adapter's I2C buses, and
> that holding the I2C lines prevents reading from it, so whatever is
> responsible for controlling the temperature prefers to play it safe and
> shuts everything down. Somehow it seems more realistic than an actual
> overheating (3 seconds is a very short period of time for that), but
> we'd need the exact schematics of the hardware, and the details of the
> thermal control system, to validate this theory.
> 
> Anyway, no need to worry anymore now that the bug is fixed :)

Actually, that's a possibility yes, though generally Apple put all temp.
monitoring chips elsewhere, it could well be the case.

Ben.

radeonfb i2c regression post-2.6.18.

[Roger Leigh]

[-- Attachment #1: Type: text/plain, Size: 1791 bytes --]

Hi folks,

I'm using an Apple Cinema Display connected via DVI to an Apple Mac
Mini with Radeon 9200 graphics.  This used to work fine, but with
kernels >= 2.6.19, the monitor powers off as soon as the framebuffer
is initialised, making for a less than usable system.  I've tested
with 2.6.(19|2[012]), and I didn't see any related changes in 2.6.23.

The breakage occured in this commit:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=7a45093b7caa9d3d5421274b4ba80fba5da17e19

Specifically, these lines:

-       /* Release the DDC lines when done or the Apple Cinema HD display
-        * will switch off
-        */
-       OUTREG(reg, INREG(reg) & ~(VGA_DDC_CLK_OUT_EN | VGA_DDC_DATA_OUT_EN));
-       (void)INREG(reg);
+       u8 *edid = fb_ddc_read(&rinfo->i2c[conn-1].adapter);

BenH later applied a fix:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=4f71c5de19c27f2198105d3b26b398494d5c353b

but I'm not sure if this is related to the above, or was for a
separate problem.  Either way, the monitor is still getting powered
off.

Due to the code reorganisation when the i2c stuff got moved into the
generic fbmon.c, I'm not sure where the workaround needs putting back
(if indeed this is where the problem lies).

I'm booting with video=ofonly to get a working system;
video=radeonfb:1680x1050-32@60 (with or without the additional
parameters) always results in a dead monitor as soon as the
framebuffer is set up.


Regards,
Roger

-- 
  .''`.  Roger Leigh
 : :' :  Debian GNU/Linux             http://people.debian.org/~rleigh/
 `. `'   Printing on GNU/Linux?       http://gutenprint.sourceforge.net/
   `-    GPG Public Key: 0x25BFB848   Please GPG sign your mail.

[-- Attachment #2: Type: application/pgp-signature, Size: 188 bytes --]