I'm currently working with a graphics driver that makes use of 2 GPIO
pins for EDID communication (clock and data). In order to coexist
peacefully with the driver for the GPIO chip, it must use gpiolib to
request the lines, set direction, and set values. This results in a
spinlock/unlock for every operation with this particular gpio driver.
It would be preferable to lock the resources once, perform the EDID
communication, then unlock the resources. The resources in this case are
the value and direction registers off the PCI GPIO base address register
which is shared with the other lines provided by the GPIO chip.
Is there a best practice for dealing with this kind of configuration?
If not, would it make sense to add optional gpiochip-level lock/unlock
and lockless direction and value operations to the gpiochip function
block?
Thanks,
--
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel
On Mon, Sep 30, 2013 at 10:29:26AM -0700, Darren Hart wrote:
> Is there a best practice for dealing with this kind of configuration?
> If not, would it make sense to add optional gpiochip-level lock/unlock
> and lockless direction and value operations to the gpiochip function
> block?
Another thing people keep suggesting for this is a block GPIO set
operation - something that will let you configure multiple GPIOs with a
single call. That maps nicely onto hardware which has a single register
for multiple GPIOs but there's been abstraction problems implementing
it.
On Mon, Sep 30, 2013 at 7:29 PM, Darren Hart <[email protected]> wrote:
> I'm currently working with a graphics driver that makes use of 2 GPIO
> pins for EDID communication (clock and data). In order to coexist
> peacefully with the driver for the GPIO chip, it must use gpiolib to
> request the lines, set direction, and set values. This results in a
> spinlock/unlock for every operation with this particular gpio driver.
Do you mean that this particular GPIO driver (which one?)
has a problem with this, or do you mean that there is something
in the gpiolib architecture that prevents you from augmenting
the GPIO driver to do what you want?
I can't see that we're taking any locks in the GPIOlib core.
> It would be preferable to lock the resources once, perform the EDID
> communication, then unlock the resources. The resources in this case are
> the value and direction registers off the PCI GPIO base address register
> which is shared with the other lines provided by the GPIO chip.
>
> Is there a best practice for dealing with this kind of configuration?
No.
> If not, would it make sense to add optional gpiochip-level lock/unlock
> and lockless direction and value operations to the gpiochip function
> block?
How do you imagine the API?
I can imagine something like:
gpio_bitbang_array(struct gpio_desc *desc, int value *, unsigned int values)
{
/* Fall all the way through to the driver */
}
Or even:
struct bitbang_entry {
unsigned int val;
unsigned int delay_after;
}
gpio_bitbang_array(struct gpio_desc *desc,
struct bitbang_entry **,
int entries);
In either case (for the rough sketches) the gpiolib core has to fall back to
iterating over the array and just using set_value() if the accelerated ops
are not supported by the driver.
Possibly things can be learned from other parts of the kernel here.
Yours,
Linus Walleij
On Fri, 2013-10-11 at 16:02 +0200, Linus Walleij wrote:
> On Mon, Sep 30, 2013 at 7:29 PM, Darren Hart <[email protected]> wrote:
>
> > I'm currently working with a graphics driver that makes use of 2 GPIO
> > pins for EDID communication (clock and data). In order to coexist
> > peacefully with the driver for the GPIO chip, it must use gpiolib to
> > request the lines, set direction, and set values. This results in a
> > spinlock/unlock for every operation with this particular gpio driver.
>
> Do you mean that this particular GPIO driver (which one?)
gpio-sch
> has a problem with this, or do you mean that there is something
> in the gpiolib architecture that prevents you from augmenting
> the GPIO driver to do what you want?
>
> I can't see that we're taking any locks in the GPIOlib core.
Right, this is going to be driver specific. Each driver, as I understand
it, is responsible for setting up any necessary locking since some may
share registers, others may not. However, if memory serves, a shared for
all pins for direction, enable, and value is not uncommon, and requires
a mutual exclusion mechanism.
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. 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.
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.
>
> > It would be preferable to lock the resources once, perform the EDID
> > communication, then unlock the resources. The resources in this case are
> > the value and direction registers off the PCI GPIO base address register
> > which is shared with the other lines provided by the GPIO chip.
> >
> > Is there a best practice for dealing with this kind of configuration?
>
> No.
>
> > If not, would it make sense to add optional gpiochip-level lock/unlock
> > and lockless direction and value operations to the gpiochip function
> > block?
>
> How do you imagine the API?
>
> I can imagine something like:
>
> gpio_bitbang_array(struct gpio_desc *desc, int value *, unsigned int values)
> {
> /* Fall all the way through to the driver */
> }
>
> Or even:
>
> struct bitbang_entry {
> unsigned int val;
> unsigned int delay_after;
> }
>
> gpio_bitbang_array(struct gpio_desc *desc,
> struct bitbang_entry **,
> int entries);
>
> In either case (for the rough sketches) the gpiolib core has to fall back to
> iterating over the array and just using set_value() if the accelerated ops
> are not supported by the driver.
>
> Possibly things can be learned from other parts of the kernel here.
Hrm... I hadn't considered something like that. 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.
Thanks,
Darren
>
> Yours,
> Linus Walleij
--
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel
On Mon, Oct 14, 2013 at 7:29 PM, Darren Hart <[email protected]> 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.
> 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?
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.
Yours,
Linus Walleij
On Wed, 2013-10-16 at 00:02 +0200, Linus Walleij wrote:
> On Mon, Oct 14, 2013 at 7:29 PM, Darren Hart <[email protected]> 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