Messages in this thread | | | Subject | Re: GPIO: Performance sensitive applications, gpiochip-level locking | From | Darren Hart <> | Date | Tue, 15 Oct 2013 15:10:24 -0700 |
| |
On Wed, 2013-10-16 at 00:02 +0200, Linus Walleij wrote: > On Mon, Oct 14, 2013 at 7:29 PM, Darren Hart <dvhart@linux.intel.com> wrote: > > > In the case of the gpio-sch driver, each operation for direction and > > value require a lock/unlock. There is no API in gpiolib to lock the chip > > as a whole and then make lockless calls. > > I don't see why the gpiolib should handle a lock? The lock in this > driver seems to be there for this type of read/modify/write sequence: > > spin_lock > val = inb() > val &= ~mask; > val |= set; > outb(val) > spin_unlock > > It's quite far away from the gpiochip as such ... In the case of ARM > we are now looking at implementing atomic read/modify/write calls > so we don't have to use any locks like this, so it's something that > is not going to be useful for everyone it seems.
Indeed. Thus the comment about being a no-op for some below. If it's a no-op for most, then this may very well be the wrong thing.
> > > We could do this for this > > specific driver, but it seems to me it would better to do so at the > > gpiolib layer. For some chips these operations might be no-ops, for > > others, like the gpio-sch chip, they could avoid the lock/unlock for > > every call and allow for some performance improvement. > > Yeah, we just need to figure out how to do that properly. > > > Full disclosure here, I don't yet know if the lock/unlock presents a > > performance bottleneck. I've asked the graphics driver developers to try > > with the existing API and see if it is adequate. > > OK seems like a good idea. You need a lot of GPIO > traffic for this to come into effect I believe, the cycles on the > io-port bus will be the major time consumer, right? Or are > these fast? > > > My thinking was more > > along the lines of: > > > > gpio_lock_chip(struct gpio_chip *chip) > > gpio_direction_input_locked(gpio) > > val = gpio_get_value_locked(gpio) > > ... > > gpio_direction_output_locked(gpio > > gpio_set_value_locked(gpio, val) > > ... > > gpio_unlock_chip(struct gpio_chip *chip) > > > > I like the possibility of your suggestion, but I wonder if it will be > > flexible enough. > > Argh, all these accessors with gpiod_* accesors already > being added this kernel cycle, it's going to be a *lot* > of duplicated APIs isn't it?
It is, and I don't like it. This also means anyone can call gpio_*_locked() and bypass the locking.... probably not a good plan. Fine for internal static implementations, but not for an exported API. I withdraw the suggestion :-)
> > But will the above be flexible? It's just some big anonymous > lock and doesn't encourage fine-grained locking. It's like a > "big GPIO lock" and that's maybe not desireable.
If we do anything, it should be more along the lines of what you suggested - but I still don't know if it will provide adequate flexibility. I'll wait to hear back from the graphics driver team, and if they can demonstrate this is a performance bottleneck, I'll come back with details.
Thanks! -- Darren Hart Intel Open Source Technology Center Yocto Project - Linux Kernel
| |