Re: i2c handling in nouveau driver

[Jean Delvare <jdelvare@suse.de>]

Hi Ben,

I'm sorry for the very very late reply. Please do not jump to the 
conclusion that I do not care - I do! Just I am very busy, just as you.

On Wednesday 11 January 2012 01:40:58 pm Ben Skeggs wrote:
> On Wed, 2012-01-11 at 11:17 +0100, Jean Delvare wrote:
> > In the commit message, you complain about the use of cond_resched()
> > in i2c-algo-bit. You might as well be right, maybe it should be
> > removed. It's been there pretty much forever (February 2002 at
> > least) and while I can understand why it was put there, I would
> > agree it isn't necessarily a good idea. Did you try removing it to
> > see if it solved your problem?
> 
> I don't disagree, and I held off a long time before finally
>  committing it to the nouveau repository due to not liking the
>  duplication.  My final decision to do it was admittedly due to
>  having very very limited time and a *lot* of other things to work
>  on.  And this solution worked now.
> 
> The fact i2c-algo-bit can't be used from an atomic context was
>  probably a convenient excuse I guess, but I did also figure there
>  were good reasons for it and there'd likely be push-back at
>  attempting to change that.  Again, due to having loads of other
>  things to do, I took the quick approach.

I don't think there actually is any good reason, and not only I wouldn't 
push back if someone wants to get rid of the call to cond_resched(), but 
I would even push for it to happen ;)

Now the question is, what do we replace it with? Nothing? cpu_relax()? 
yield()? udelay(<small value>)?

I guess yield() would have the same property of making the code not 
callable from atomic context. Not having anything might be harsh from a 
CPU and I/O load perspective, as it would be a tight loop lasting for 
several milliseconds. So this leaves us with cpu_relax() and udelay(). 
Any preference?

> (...)
> I'd be more than happy to have these issues (nvs300 and running from
>  an atomic context) solved in i2c-algo-bit, and will gladly revert
>  the nouveau changes once it is.  Let me know how I can help you with
>  that.

Incidentally I just sent a fix upstream for a bug reported by Ville 
Syrjälä in i2c-algo-bit, that would cause spurious timeouts on heavy CPU 
load. I am curious if it would have helped in your case. Please take a 
look:

http://marc.info/?l=linux-kernel&m=133182203301053&w=2

And if you think it could help, I would appreciate if you could give it 
a try with the nouveau driver and see if it actually does.

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