[PATCH] i2c-isch: Decrease delay in the loop checking the BUSY state of the bus

[PATCH] i2c-isch: Decrease delay in the loop checking the BUSY state of the bus

[Olivier Sobrie]

Generally it is not needed to wait for 1 msec, the SMBus get often ready
in less than 200 usecs.

msleep(1) can wait up to 20 msecs... It has a significant impact when
there is a burst of transactions on the bus.

Signed-off-by: Olivier Sobrie <olivier-Ui3EtX6WB9GzQB+pC5nmwQ@public.gmane.org>
---
 drivers/i2c/busses/i2c-isch.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-isch.c b/drivers/i2c/busses/i2c-isch.c
index 6561d27..f90a605 100644
--- a/drivers/i2c/busses/i2c-isch.c
+++ b/drivers/i2c/busses/i2c-isch.c
@@ -47,7 +47,7 @@
 #define SMBBLKDAT	(0x20 + sch_smba)
 
 /* Other settings */
-#define MAX_TIMEOUT	500
+#define MAX_RETRIES	5000
 
 /* I2C constants */
 #define SCH_QUICK		0x00
@@ -68,7 +68,7 @@ static int sch_transaction(void)
 {
 	int temp;
 	int result = 0;
-	int timeout = 0;
+	int retries = 0;
 
 	dev_dbg(&sch_adapter.dev, "Transaction (pre): CNT=%02x, CMD=%02x, "
 		"ADD=%02x, DAT0=%02x, DAT1=%02x\n", inb(SMBHSTCNT),
@@ -100,12 +100,12 @@ static int sch_transaction(void)
 	outb(inb(SMBHSTCNT) | 0x10, SMBHSTCNT);
 
 	do {
-		msleep(1);
+		usleep_range(100, 200);
 		temp = inb(SMBHSTSTS) & 0x0f;
-	} while ((temp & 0x08) && (timeout++ < MAX_TIMEOUT));
+	} while ((temp & 0x08) && (retries++ < MAX_RETRIES));
 
 	/* If the SMBus is still busy, we give up */
-	if (timeout > MAX_TIMEOUT) {
+	if (retries > MAX_RETRIES) {
 		dev_err(&sch_adapter.dev, "SMBus Timeout!\n");
 		result = -ETIMEDOUT;
 	}
-- 
1.7.5.4

Re: [PATCH] i2c-isch: Decrease delay in the loop checking the BUSY state of the bus

[Jean Delvare]

Salut Olivier,

On Fri, 20 Jan 2012 12:46:54 +0100, Olivier Sobrie wrote:
> Generally it is not needed to wait for 1 msec, the SMBus get often ready
> in less than 200 usecs.

The code change looks OK but the patch description not really. The loop
you're changing is waiting for command completion, it isn't checking
for bus business, regardless of what the comment in the code says. What
about:

i2c-isch: Decrease delay in command completion check loop

If this is OK with you I'll apply your patch with this description.

> msleep(1) can wait up to 20 msecs... It has a significant impact when
> there is a burst of transactions on the bus.

To be honest I didn't know about usleep_range(). Probably the same
change could apply to a number of polled SMBus controller drivers,
starting with i2c-i801. I'll give it a try...

Of course it's nowhere as good as switching the drivers to be
interrupt-driven. Did you check if you patch had any impact in terms of
CPU load? I'm also curious what happens on systems without high
resolution timer support, as apparently usleep_range() is implemented
in terms of these. I admit I am surprised that usleep_range() is
unconditionally available given that.

> Signed-off-by: Olivier Sobrie <olivier-Ui3EtX6WB9GzQB+pC5nmwQ@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-isch.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-isch.c b/drivers/i2c/busses/i2c-isch.c
> index 6561d27..f90a605 100644
> --- a/drivers/i2c/busses/i2c-isch.c
> +++ b/drivers/i2c/busses/i2c-isch.c
> @@ -47,7 +47,7 @@
>  #define SMBBLKDAT	(0x20 + sch_smba)
>  
>  /* Other settings */
> -#define MAX_TIMEOUT	500
> +#define MAX_RETRIES	5000
>  
>  /* I2C constants */
>  #define SCH_QUICK		0x00
> @@ -68,7 +68,7 @@ static int sch_transaction(void)
>  {
>  	int temp;
>  	int result = 0;
> -	int timeout = 0;
> +	int retries = 0;
>  
>  	dev_dbg(&sch_adapter.dev, "Transaction (pre): CNT=%02x, CMD=%02x, "
>  		"ADD=%02x, DAT0=%02x, DAT1=%02x\n", inb(SMBHSTCNT),
> @@ -100,12 +100,12 @@ static int sch_transaction(void)
>  	outb(inb(SMBHSTCNT) | 0x10, SMBHSTCNT);
>  
>  	do {
> -		msleep(1);
> +		usleep_range(100, 200);
>  		temp = inb(SMBHSTSTS) & 0x0f;
> -	} while ((temp & 0x08) && (timeout++ < MAX_TIMEOUT));
> +	} while ((temp & 0x08) && (retries++ < MAX_RETRIES));
>  
>  	/* If the SMBus is still busy, we give up */
> -	if (timeout > MAX_TIMEOUT) {
> +	if (retries > MAX_RETRIES) {
>  		dev_err(&sch_adapter.dev, "SMBus Timeout!\n");
>  		result = -ETIMEDOUT;
>  	}


-- 
Jean Delvare

Re: [PATCH] i2c-isch: Decrease delay in the loop checking the BUSY state of the bus

[Olivier Sobrie]

Salut Jean,

On Mon, Jan 23, 2012 at 04:26:20PM +0100, Jean Delvare wrote:
> On Fri, 20 Jan 2012 12:46:54 +0100, Olivier Sobrie wrote:
> > Generally it is not needed to wait for 1 msec, the SMBus get often ready
> > in less than 200 usecs.
> 
> The code change looks OK but the patch description not really. The loop
> you're changing is waiting for command completion, it isn't checking
> for bus business, regardless of what the comment in the code says. What
> about:
> 
> i2c-isch: Decrease delay in command completion check loop
> 
> If this is OK with you I'll apply your patch with this description.

It's OK for me. Sorry for the wrong description.
Indeed yours looks better !

> > msleep(1) can wait up to 20 msecs... It has a significant impact when
> > there is a burst of transactions on the bus.
> 
> To be honest I didn't know about usleep_range(). Probably the same
> change could apply to a number of polled SMBus controller drivers,
> starting with i2c-i801. I'll give it a try...

Indeed I saw there are a lot of msleep(1) in the i2c drivers.
As I only have an intel SMBus I cannot test it for others i2c busses.
I choose to use usleep_range() as the documentation located in
Documentation/timers/timers-howto.txt of the kernel tree says to use
this function in the case of a sleep between 10us and 20ms...

> Of course it's nowhere as good as switching the drivers to be
> interrupt-driven. Did you check if you patch had any impact in terms of
> CPU load? I'm also curious what happens on systems without high
> resolution timer support, as apparently usleep_range() is implemented
> in terms of these. I admit I am surprised that usleep_range() is
> unconditionally available given that.

I didn't check the CPU load. But I assume there will be no difference
in my case as the timer is generally fired only one time.
For info, I tested this change with a touchscreen device for which I've
to perform a lot of i2c_smbus_read_byte() to read touch data.
I'll have a look at the CPU load. By the way if you've a good idea how
to have relevant measures I'm interested in.

Concerning the system without hrtimers support, I just did a test and
the performances decrease! It introduces again a long delay... which is
not the case if I do a udelay(100)...

Thanks for your comments and have a nice day!

-- 
Olivier Sobrie

Re: [PATCH] i2c-isch: Decrease delay in the loop checking the BUSY state of the bus

[Jean Delvare]

On Tue, 24 Jan 2012 09:46:41 +0100, Olivier Sobrie wrote:
> On Mon, Jan 23, 2012 at 04:26:20PM +0100, Jean Delvare wrote:
> > The code change looks OK but the patch description not really. The loop
> > you're changing is waiting for command completion, it isn't checking
> > for bus business, regardless of what the comment in the code says. What
> > about:
> > 
> > i2c-isch: Decrease delay in command completion check loop
> > 
> > If this is OK with you I'll apply your patch with this description.
> 
> It's OK for me. Sorry for the wrong description.
> Indeed yours looks better !

OK, changed. I have queued your patch for kernel 3.4, it will be in
linux-next until then. If adjustments are needed, feel free to send an
update.

http://khali.linux-fr.org/devel/linux-3/jdelvare-i2c/i2c-isch-decrease-delay-in-command-completion-check-loop.patch

> > To be honest I didn't know about usleep_range(). Probably the same
> > change could apply to a number of polled SMBus controller drivers,
> > starting with i2c-i801. I'll give it a try...
> 
> Indeed I saw there are a lot of msleep(1) in the i2c drivers.
> As I only have an intel SMBus I cannot test it for others i2c busses.
> I choose to use usleep_range() as the documentation located in
> Documentation/timers/timers-howto.txt of the kernel tree says to use
> this function in the case of a sleep between 10us and 20ms...

And quite rightly so. Note that usleep_range() did not exist when most
of these drivers were originally written, so the use of msleep(1) is by
default and not a design decision. Using usleep_range() is certainly
the right thing to do in most of them these days (or convert them to
use interrupts, of course.)

> (...)
> I didn't check the CPU load. But I assume there will be no difference
> in my case as the timer is generally fired only one time.

In my own tests, the CPU load seems to increase slightly, most probably
because I use i2c_smbus_read_byte_data() which is a longer transaction
than yours so the timer is fired several times (2 or 3) per
transaction. If I change the range from (100, 200) to (250, 500) then
I'm almost back to the original CPU load figures, modulo measurement
noise, with almost the same speed benefit.

As the optimal range depends on the SMBus frequency and the transaction,
it seems a little difficult to optimize. We can settle on a per-driver
range, and anyone not happy with it will have to work on interrupt
support ;)

> For info, I tested this change with a touchscreen device for which I've
> to perform a lot of i2c_smbus_read_byte() to read touch data.
> I'll have a look at the CPU load. By the way if you've a good idea how
> to have relevant measures I'm interested in.

I am using the i2c-dev driver + i2cdump (from the i2c-tools package) on
an arbitrary SMBus slave on my SMBus. I'm measuring the time it takes
to dump all the register space:

# modprobe i2c-dev
# time for i in `seq 1 10` ; do i2cdump -y 8 0x2f b > /dev/null ; done

real	0m5.139s
user	0m0.016s
sys	0m0.118s

This was with the original i2c-i801 driver. "real" tells me how fast
register reads actually are (2560 reads in 5.139 s -> 2 ms/read on
average), and "sys" how much they cost in terms of CPU. "user" can be
ignored. With usleep_range(100, 200) I get:

real	0m1.448s
user	0m0.006s
sys	0m0.150s

So you can see it's much faster (0.57 ms/read) but costs more CPU. With
usleep_range(250, 500) I get:

real	0m1.587s
user	0m0.003s
sys	0m0.124s

That's 0.62 ms/read. And finally with usleep_range(400, 700) I get:

real	0m2.043s
user	0m0.007s
sys	0m0.118s

The speed/CPU tradeoff is visible, and I think I'll go with
usleep_range(250, 500).

Of course if you want more accurate measurements you want to do more
iterations and probably use better statistical analysis than the sum I
did.

> Concerning the system without hrtimers support, I just did a test and
> the performances decrease! It introduces again a long delay... which is
> not the case if I do a udelay(100)...

But udelay() is busy-waiting so it would have an unacceptable CPU cost
especially on large transactions. Question is, is usleep_range(100,
200) without hrtimers support slower than msleep(1)? If not then we're
fine. If it is slower then that would be a bug in the implementation of
usleep_range(), as it really shouldn't be each driver's job to check
for this individually.

-- 
Jean Delvare

Re: [PATCH] i2c-isch: Decrease delay in the loop checking the BUSY state of the bus

[Olivier Sobrie]

On Tue, Jan 24, 2012 at 10:58:38AM +0100, Jean Delvare wrote:
> OK, changed. I have queued your patch for kernel 3.4, it will be in
> linux-next until then. If adjustments are needed, feel free to send an
> update.
> 
> http://khali.linux-fr.org/devel/linux-3/jdelvare-i2c/i2c-isch-decrease-delay-in-command-completion-check-loop.patch

Ok thanks!

> > (...)
> > I didn't check the CPU load. But I assume there will be no difference
> > in my case as the timer is generally fired only one time.
> 
> In my own tests, the CPU load seems to increase slightly, most probably
> because I use i2c_smbus_read_byte_data() which is a longer transaction
> than yours so the timer is fired several times (2 or 3) per
> transaction. If I change the range from (100, 200) to (250, 500) then
> I'm almost back to the original CPU load figures, modulo measurement
> noise, with almost the same speed benefit.
> 
> As the optimal range depends on the SMBus frequency and the transaction,
> it seems a little difficult to optimize. We can settle on a per-driver
> range, and anyone not happy with it will have to work on interrupt
> support ;)
> 
> > For info, I tested this change with a touchscreen device for which I've
> > to perform a lot of i2c_smbus_read_byte() to read touch data.
> > I'll have a look at the CPU load. By the way if you've a good idea how
> > to have relevant measures I'm interested in.
> 
> I am using the i2c-dev driver + i2cdump (from the i2c-tools package) on
> an arbitrary SMBus slave on my SMBus. I'm measuring the time it takes
> to dump all the register space:
> 
> # modprobe i2c-dev
> # time for i in `seq 1 10` ; do i2cdump -y 8 0x2f b > /dev/null ; done
> 
> real	0m5.139s
> user	0m0.016s
> sys	0m0.118s
> 
> This was with the original i2c-i801 driver. "real" tells me how fast
> register reads actually are (2560 reads in 5.139 s -> 2 ms/read on
> average), and "sys" how much they cost in terms of CPU. "user" can be
> ignored. With usleep_range(100, 200) I get:
> 
> real	0m1.448s
> user	0m0.006s
> sys	0m0.150s
> 
> So you can see it's much faster (0.57 ms/read) but costs more CPU. With
> usleep_range(250, 500) I get:
> 
> real	0m1.587s
> user	0m0.003s
> sys	0m0.124s
> 
> That's 0.62 ms/read. And finally with usleep_range(400, 700) I get:
> 
> real	0m2.043s
> user	0m0.007s
> sys	0m0.118s
> 
> The speed/CPU tradeoff is visible, and I think I'll go with
> usleep_range(250, 500).
> 
> Of course if you want more accurate measurements you want to do more
> iterations and probably use better statistical analysis than the sum I
> did.

I performed the same test you did on my system and I observed this:

* msleep(1)
   real    0m 51.20s
   user    0m 0.29s
   sys     0m 0.00s

* usleep_range(100, 200)
   real    0m 1.46s
   user    0m 0.10s
   sys     0m 0.10s

* usleep_range(250, 500)
   real    0m 2.01s
   user    0m 0.05s
   sys     0m 0.25s

* usleep_range(50, 150)
   real    0m 1.43s
   user    0m 0.07s
   sys     0m 0.23s

I think usleep_range(100, 200) is the best compromise.

> > Concerning the system without hrtimers support, I just did a test and
> > the performances decrease! It introduces again a long delay... which is
> > not the case if I do a udelay(100)...
> 
> But udelay() is busy-waiting so it would have an unacceptable CPU cost
> especially on large transactions. Question is, is usleep_range(100,
> 200) without hrtimers support slower than msleep(1)? If not then we're
> fine. If it is slower then that would be a bug in the implementation of
> usleep_range(), as it really shouldn't be each driver's job to check
> for this individually.

I agree udelay() is not a good solution!
I did the test without hrtimers using usleep_range(100, 200) and got:
    real    0m 25.60s
    user    0m 0.30s
    sys     0m 0.00s
So that's not slower than msleep(1) in the case of no hrtimers.

-- 
Olivier Sobrie

Re: [PATCH] i2c-isch: Decrease delay in the loop checking the BUSY state of the bus

[Jean Delvare]

On Tue, 24 Jan 2012 15:07:50 +0100, Olivier Sobrie wrote:
> I performed the same test you did on my system and I observed this:
> 
> * msleep(1)
>    real    0m 51.20s
>    user    0m 0.29s
>    sys     0m 0.00s

I assume your kernel has HZ=100, that means 20 ms per transaction i.e.
2 jiffies, as expected from the msleep() implementation.

> 
> * usleep_range(100, 200)
>    real    0m 1.46s
>    user    0m 0.10s
>    sys     0m 0.10s
> 
> * usleep_range(250, 500)
>    real    0m 2.01s
>    user    0m 0.05s
>    sys     0m 0.25s

It's really curious that this can take more CPU time than
usleep_range(100, 200). I can't explain it.

> 
> * usleep_range(50, 150)
>    real    0m 1.43s
>    user    0m 0.07s
>    sys     0m 0.23s
> 
> I think usleep_range(100, 200) is the best compromise.

I agree.

> (...)
> I agree udelay() is not a good solution!
> I did the test without hrtimers using usleep_range(100, 200) and got:
>     real    0m 25.60s
>     user    0m 0.30s
>     sys     0m 0.00s
> So that's not slower than msleep(1) in the case of no hrtimers.

In fact that's exactly twice as fast as msleep(2), i.e. 1 jiffy per
transaction instead of 2. That's pretty good that there is a benefit
even without hrtimers support. Thanks for testing and reporting,
that's one less thing on my to-do list :)

I'll send a patch similar to yours for the i2c-i801 driver in a minute.
Thanks for showing us the way!

-- 
Jean Delvare