Re: GPIO: Performance sensitive applications, gpiochip-level locking

[Darren Hart]

On Wed, 2013-10-16 at 00:02 +0200, Linus Walleij wrote:
> On Mon, Oct 14, 2013 at 7:29 PM, Darren Hart <dvhart@xxxxxxxxxxxxxxx> 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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/