[PATCH 1/2] drm/kms: Make i2c buses faster

[PATCH 1/2] drm/kms: Make i2c buses faster

[Jean Delvare]

A udelay value of 20 leads to an I2C bus running at only 25 kbps. I2C
devices can typically operate faster than this, 50 kbps should be fine
for all devices (and compliant devices can always stretch the clock if
needed.)

FWIW, the vast majority of framebuffer drivers set udelay to 10
already. So set it to 10 in DRM drivers too, this will make EDID block
reads faster. We might even lower the udelay value later if no problem
is reported.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Acked-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
Cc: Dave Airlie <airlied@gmail.com>
Cc: Keith Packard <keithp@keithp.com>
Cc: Alex Deucher <alexdeucher@gmail.com>
---
Already sent on: 2011-10-21.

 drivers/gpu/drm/i915/intel_i2c.c    |    2 +-
 drivers/gpu/drm/radeon/radeon_i2c.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

--- linux-3.3-rc1.orig/drivers/gpu/drm/i915/intel_i2c.c	2012-01-28 10:12:16.554480007 +0100
+++ linux-3.3-rc1/drivers/gpu/drm/i915/intel_i2c.c	2012-01-28 10:28:16.884668894 +0100
@@ -37,7 +37,7 @@
 
 /* Intel GPIO access functions */
 
-#define I2C_RISEFALL_TIME 20
+#define I2C_RISEFALL_TIME 10
 
 static inline struct intel_gmbus *
 to_intel_gmbus(struct i2c_adapter *i2c)
--- linux-3.3-rc1.orig/drivers/gpu/drm/radeon/radeon_i2c.c	2012-01-28 10:12:16.555480007 +0100
+++ linux-3.3-rc1/drivers/gpu/drm/radeon/radeon_i2c.c	2012-01-28 10:28:16.886668894 +0100
@@ -924,7 +924,7 @@ struct radeon_i2c_chan *radeon_i2c_creat
 		i2c->algo.bit.setscl = set_clock;
 		i2c->algo.bit.getsda = get_data;
 		i2c->algo.bit.getscl = get_clock;
-		i2c->algo.bit.udelay = 20;
+		i2c->algo.bit.udelay = 10;
 		/* vesa says 2.2 ms is enough, 1 jiffy doesn't seem to always
 		 * make this, 2 jiffies is a lot more reliable */
 		i2c->algo.bit.timeout = 2;

-- 
Jean Delvare
Suse L3

Re: [PATCH 1/2] drm/kms: Make i2c buses faster

[Alex Deucher]

On Sat, Jan 28, 2012 at 5:07 AM, Jean Delvare <jdelvare@suse.de> wrote:
> A udelay value of 20 leads to an I2C bus running at only 25 kbps. I2C
> devices can typically operate faster than this, 50 kbps should be fine
> for all devices (and compliant devices can always stretch the clock if
> needed.)
>
> FWIW, the vast majority of framebuffer drivers set udelay to 10
> already. So set it to 10 in DRM drivers too, this will make EDID block
> reads faster. We might even lower the udelay value later if no problem
> is reported.
>
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Acked-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
> Cc: Dave Airlie <airlied@gmail.com>
> Cc: Keith Packard <keithp@keithp.com>
> Cc: Alex Deucher <alexdeucher@gmail.com>

Reviewed-by: Alex Deucher <alexdeucher@gmail.com>

> ---
> Already sent on: 2011-10-21.
>
>  drivers/gpu/drm/i915/intel_i2c.c    |    2 +-
>  drivers/gpu/drm/radeon/radeon_i2c.c |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> --- linux-3.3-rc1.orig/drivers/gpu/drm/i915/intel_i2c.c 2012-01-28 10:12:16.554480007 +0100
> +++ linux-3.3-rc1/drivers/gpu/drm/i915/intel_i2c.c      2012-01-28 10:28:16.884668894 +0100
> @@ -37,7 +37,7 @@
>
>  /* Intel GPIO access functions */
>
> -#define I2C_RISEFALL_TIME 20
> +#define I2C_RISEFALL_TIME 10
>
>  static inline struct intel_gmbus *
>  to_intel_gmbus(struct i2c_adapter *i2c)
> --- linux-3.3-rc1.orig/drivers/gpu/drm/radeon/radeon_i2c.c      2012-01-28 10:12:16.555480007 +0100
> +++ linux-3.3-rc1/drivers/gpu/drm/radeon/radeon_i2c.c   2012-01-28 10:28:16.886668894 +0100
> @@ -924,7 +924,7 @@ struct radeon_i2c_chan *radeon_i2c_creat
>                i2c->algo.bit.setscl = set_clock;
>                i2c->algo.bit.getsda = get_data;
>                i2c->algo.bit.getscl = get_clock;
> -               i2c->algo.bit.udelay = 20;
> +               i2c->algo.bit.udelay = 10;
>                /* vesa says 2.2 ms is enough, 1 jiffy doesn't seem to always
>                 * make this, 2 jiffies is a lot more reliable */
>                i2c->algo.bit.timeout = 2;
>
> --
> Jean Delvare
> Suse L3

Re: [PATCH 1/2] drm/kms: Make i2c buses faster

[Keith Packard]

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

On Sat, 28 Jan 2012 11:07:09 +0100, Jean Delvare <jdelvare@suse.de> wrote:
> A udelay value of 20 leads to an I2C bus running at only 25 kbps. I2C
> devices can typically operate faster than this, 50 kbps should be fine
> for all devices (and compliant devices can always stretch the clock if
> needed.)
> 
> FWIW, the vast majority of framebuffer drivers set udelay to 10
> already. So set it to 10 in DRM drivers too, this will make EDID block
> reads faster. We might even lower the udelay value later if no problem
> is reported.

That runs the DDC at a whopping 50kbps, which is half of the maximum
rate specified in the DDC/CI standard. I don't know if we can count on
clock stretching (http://www.i2c-bus.org/clock-stretching/), but if so,
I don't know why we wouldn't just go to the standard 100kbps data rate
and be done with it.

Might be nice to see what frequency Windows uses for i2c; anyone want to
pull a vga cable apart and hook up a logic analyser?

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 bytes --]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/kms: Make i2c buses faster

[Jean Delvare]

Hi Keith,

Sorry for the late reply.

On Sunday 29 January 2012 02:26:25 am Keith Packard wrote:
> On Sat, 28 Jan 2012 11:07:09 +0100, Jean Delvare <jdelvare@suse.de>
> wrote:
> > A udelay value of 20 leads to an I2C bus running at only 25 kbps.
> > I2C devices can typically operate faster than this, 50 kbps should
> > be fine for all devices (and compliant devices can always stretch
> > the clock if needed.)
> >
> > FWIW, the vast majority of framebuffer drivers set udelay to 10
> > already. So set it to 10 in DRM drivers too, this will make EDID
> > block reads faster. We might even lower the udelay value later if
> > no problem is reported.
> 
> That runs the DDC at a whopping 50kbps, which is half of the maximum
> rate specified in the DDC/CI standard. I don't know if we can count
>  on clock stretching (http://www.i2c-bus.org/clock-stretching/), but
>  if so, I don't know why we wouldn't just go to the standard 100kbps
>  data rate and be done with it.

We may end up doing that. I wanted to play it safe for now as at least 
Alan Cox expressed concerns with increasing the speed of DDC buses. I 
don't share them, but being cautious can't hurt.

Clock stretching is optional, each slave is free to implement it or not. 
I very much doubt it is needed when reading and EDID though, even at 100 
kbps. Typically what takes time is writing to EEPROMs, but in general 
EEPROMs will buffer the write and simply stop responding to their slave 
addresses until done. This is why most EEPROMs have a documented write 
page size.

Displays must be interoperable by design, so I'd hope that every serious 
display maker would only use EEPROMs that can either cope with 100 kbps 
or do clock stretching as needed. I have no doubt crappy hardware 
exists, but I'd rather decrease the clock speed on repeated errors than 
default to a slow clock speed. Users with good hardware should get the 
best out of it.

> Might be nice to see what frequency Windows uses for i2c; anyone want
>  to pull a vga cable apart and hook up a logic analyser?

Can't do that, sorry. It would certainly be valuable if someone has the 
time, hardware and interest, however I don't think "Windows" uses a 
frequency, rather separate video drivers are likely to have their own 
implementation and speed (if nothing else, simply because recent video 
cards use hardware I2C engines rather than bit-banging.)

-- 
Jean Delvare
Suse L3

[PATCH 1/2] drm/kms: Make i2c buses faster

[Jean Delvare]

A udelay value of 20 leads to an I2C bus running at only 25 kbps. A
value of 40 as the nouveau driver has is even slower at 12.5 kbps. I2C
devices can typically operate faster than this, 50 kbps should be fine
for all devices (and compliant devices can always stretch the clock is
needed.)

FWIW, the vast majority of framebuffer drivers set udelay to 10
already. So set it to 10 in DRM drivers too, this will make EDID block
reads faster. We might even lower the udelay value later if no problem
is reported.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Cc: Eugeni Dodonov <eugeni@dodonov.net>
Cc: Dave Airlie <airlied@gmail.com>
Cc: Keith Packard <keithp@keithp.com>
Cc: Alex Deucher <alexdeucher@gmail.com>
---
 drivers/gpu/drm/i915/intel_i2c.c      |    2 +-
 drivers/gpu/drm/nouveau/nouveau_i2c.c |    2 +-
 drivers/gpu/drm/radeon/radeon_i2c.c   |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

--- linux-3.1-rc10.orig/drivers/gpu/drm/i915/intel_i2c.c	2011-07-22 04:17:23.000000000 +0200
+++ linux-3.1-rc10/drivers/gpu/drm/i915/intel_i2c.c	2011-10-20 14:59:11.000000000 +0200
@@ -36,7 +36,7 @@
 
 /* Intel GPIO access functions */
 
-#define I2C_RISEFALL_TIME 20
+#define I2C_RISEFALL_TIME 10
 
 static inline struct intel_gmbus *
 to_intel_gmbus(struct i2c_adapter *i2c)
--- linux-3.1-rc10.orig/drivers/gpu/drm/nouveau/nouveau_i2c.c	2011-07-22 04:17:23.000000000 +0200
+++ linux-3.1-rc10/drivers/gpu/drm/nouveau/nouveau_i2c.c	2011-10-20 15:14:36.000000000 +0200
@@ -217,7 +217,7 @@ nouveau_i2c_init(struct drm_device *dev,
 
 	if (entry->port_type < 6) {
 		i2c->adapter.algo_data = &i2c->bit;
-		i2c->bit.udelay = 40;
+		i2c->bit.udelay = 10;
 		i2c->bit.timeout = usecs_to_jiffies(5000);
 		i2c->bit.data = i2c;
 		ret = i2c_bit_add_bus(&i2c->adapter);
--- linux-3.1-rc10.orig/drivers/gpu/drm/radeon/radeon_i2c.c	2011-10-20 14:41:33.000000000 +0200
+++ linux-3.1-rc10/drivers/gpu/drm/radeon/radeon_i2c.c	2011-10-20 14:58:17.000000000 +0200
@@ -928,7 +928,7 @@ struct radeon_i2c_chan *radeon_i2c_creat
 		i2c->algo.bit.setscl = set_clock;
 		i2c->algo.bit.getsda = get_data;
 		i2c->algo.bit.getscl = get_clock;
-		i2c->algo.bit.udelay = 20;
+		i2c->algo.bit.udelay = 10;
 		/* vesa says 2.2 ms is enough, 1 jiffy doesn't seem to always
 		 * make this, 2 jiffies is a lot more reliable */
 		i2c->algo.bit.timeout = 2;

-- 
Jean Delvare
Suse L3

Re: [PATCH 1/2] drm/kms: Make i2c buses faster

[Alan Cox]

On Fri, 21 Oct 2011 09:08:30 +0200
Jean Delvare <jdelvare@suse.de> wrote:

> A udelay value of 20 leads to an I2C bus running at only 25 kbps. A
> value of 40 as the nouveau driver has is even slower at 12.5 kbps. I2C
> devices can typically operate faster than this, 50 kbps should be fine
> for all devices (and compliant devices can always stretch the clock is
> needed.)

That depends on your cable, drive and signal quality. It's not something
you just turn up because it seems a good idea. Reliability is MUCH more
important than shaving microseconds off monitor probe times.

Alan

Re: [PATCH 1/2] drm/kms: Make i2c buses faster

[Jean Delvare]

Hi Alan,

On Friday 21 October 2011 03:32:44 pm Alan Cox wrote:
> On Fri, 21 Oct 2011 09:08:30 +0200
> 
> Jean Delvare <jdelvare@suse.de> wrote:
> > A udelay value of 20 leads to an I2C bus running at only 25 kbps. A
> > value of 40 as the nouveau driver has is even slower at 12.5 kbps.
> > I2C devices can typically operate faster than this, 50 kbps should
> > be fine for all devices (and compliant devices can always stretch
> > the clock is needed.)
> 
> That depends on your cable, drive and signal quality. It's not
>  something you just turn up because it seems a good idea. Reliability
>  is MUCH more important than shaving microseconds off monitor probe
>  times.

We're talking milliseconds here, not microseconds. Namely 23 ms per 128-
byte EDID block for intel and radeon, 69 ms for nouveau.

I very much doubt that cable quality is an issue here. DDC is a very 
slow bus (even at the maximum speed of 100 kbps) compared to the video 
signal which is running through the other wires in the VGA or DDC cable. 
If you really reach the point where DDC becomes unreliable, I doubt the 
video signal is anywhere next to usable.

More importantly, my initial motivation for sending this patch is that 
it may help prevent problems due to the software nature of bit-banged 
I2C. A faster clock means shorter (time-wise) messages, which in turn 
means less risks to be disturbed by interrupts or CPU overload.

Last but not least, I can't believe that so many framebuffer drivers 
would have been using these settings for years if it caused trouble.

Does anyone know at which speed hardware I2C engines are running the DDC 
bus on various graphics cards?

-- 
Jean Delvare
Suse L3

Re: [PATCH 1/2] drm/kms: Make i2c buses faster

[Alex Deucher]

On Fri, Oct 21, 2011 at 10:16 AM, Jean Delvare <jdelvare@suse.de> wrote:
> Hi Alan,
>
> On Friday 21 October 2011 03:32:44 pm Alan Cox wrote:
>> On Fri, 21 Oct 2011 09:08:30 +0200
>>
>> Jean Delvare <jdelvare@suse.de> wrote:
>> > A udelay value of 20 leads to an I2C bus running at only 25 kbps. A
>> > value of 40 as the nouveau driver has is even slower at 12.5 kbps.
>> > I2C devices can typically operate faster than this, 50 kbps should
>> > be fine for all devices (and compliant devices can always stretch
>> > the clock is needed.)
>>
>> That depends on your cable, drive and signal quality. It's not
>>  something you just turn up because it seems a good idea. Reliability
>>  is MUCH more important than shaving microseconds off monitor probe
>>  times.
>
> We're talking milliseconds here, not microseconds. Namely 23 ms per 128-
> byte EDID block for intel and radeon, 69 ms for nouveau.
>
> I very much doubt that cable quality is an issue here. DDC is a very
> slow bus (even at the maximum speed of 100 kbps) compared to the video
> signal which is running through the other wires in the VGA or DDC cable.
> If you really reach the point where DDC becomes unreliable, I doubt the
> video signal is anywhere next to usable.
>
> More importantly, my initial motivation for sending this patch is that
> it may help prevent problems due to the software nature of bit-banged
> I2C. A faster clock means shorter (time-wise) messages, which in turn
> means less risks to be disturbed by interrupts or CPU overload.
>
> Last but not least, I can't believe that so many framebuffer drivers
> would have been using these settings for years if it caused trouble.
>
> Does anyone know at which speed hardware I2C engines are running the DDC
> bus on various graphics cards?

IIRC, we generally target the radeon hw i2c engines to run at 50 khz.

Alex

>
> --
> Jean Delvare
> Suse L3
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Re: [PATCH 1/2] drm/kms: Make i2c buses faster

[Jean Delvare]

Hi Alex,

On Friday 21 October 2011 08:05:48 pm Alex Deucher wrote:
> On Fri, Oct 21, 2011 at 10:16 AM, Jean Delvare <jdelvare@suse.de>
> > Does anyone know at which speed hardware I2C engines are running
> > the DDC bus on various graphics cards?
> 
> IIRC, we generally target the radeon hw i2c engines to run at 50 khz.

Then it doesn't seem unreasonable to try and achieve the same for bit-
banged I2C. That's exactly what my patch is doing.

-- 
Jean Delvare
Suse L3

Re: [PATCH 1/2] drm/kms: Make i2c buses faster

[Alex Deucher]

On Fri, Oct 21, 2011 at 3:29 PM, Jean Delvare <jdelvare@suse.de> wrote:
> Hi Alex,
>
> On Friday 21 October 2011 08:05:48 pm Alex Deucher wrote:
>> On Fri, Oct 21, 2011 at 10:16 AM, Jean Delvare <jdelvare@suse.de>
>> > Does anyone know at which speed hardware I2C engines are running
>> > the DDC bus on various graphics cards?
>>
>> IIRC, we generally target the radeon hw i2c engines to run at 50 khz.
>
> Then it doesn't seem unreasonable to try and achieve the same for bit-
> banged I2C. That's exactly what my patch is doing.

Seems fine to me then.  I don't know why we set it so low to begin
with, but I'm certainly not an i2c expert.

Alex

>
> --
> Jean Delvare
> Suse L3
>

Re: [PATCH 1/2] drm/kms: Make i2c buses faster

[Eugeni Dodonov]

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

On Sat, Oct 22, 2011 at 12:38, Alex Deucher <alexdeucher@gmail.com> wrote:

> On Fri, Oct 21, 2011 at 3:29 PM, Jean Delvare <jdelvare@suse.de> wrote:
> > Hi Alex,
> >
> > On Friday 21 October 2011 08:05:48 pm Alex Deucher wrote:
> >> On Fri, Oct 21, 2011 at 10:16 AM, Jean Delvare <jdelvare@suse.de>
> >> > Does anyone know at which speed hardware I2C engines are running
> >> > the DDC bus on various graphics cards?
> >>
> >> IIRC, we generally target the radeon hw i2c engines to run at 50 khz.
> >
> > Then it doesn't seem unreasonable to try and achieve the same for bit-
> > banged I2C. That's exactly what my patch is doing.
>
> Seems fine to me then.  I don't know why we set it so low to begin
> with, but I'm certainly not an i2c expert.
>

Seems fine for me as well.

Acked-by: Eugeni Dodonov <eugeni.dodonov@intel.com>

-- 
Eugeni Dodonov
<http://eugeni.dodonov.net/>

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

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel