| Message ID | 20180806222918.12644-10-jmkrzyszt@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,v2,01/12] mtd: rawnand: ams-delta: Assign mtd->dev.parent, not mtd->owner | expand |
Hi Janusz! On Tue, Aug 7, 2018 at 12:29 AM Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote: > Certain GPIO array lookup results may map directly to GPIO pins of a > single GPIO chip in hardware order. If that condition is recognized > and handled efficiently, significant performance gain of get/set array > functions may be possible. > > While processing a request for an array of GPIO descriptors, verify if > the descriptors just collected represent consecutive pins of a single > GPIO chip. Pass that information with the array to the caller so it > can benefit from enhanced performance as soon as bitmap based get/set > array functions which can make efficient use of that are available. > > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> (...) > This function returns a struct gpio_descs which contains an array of > -descriptors:: > +descriptors. It may also contain a valid descriptor of a single GPIO chip in > +case the array strictly matches pin hardware layout of the chip:: > > struct gpio_descs { > unsigned int ndescs; > struct gpio_desc *desc[]; > + struct gpio_chip *chip; This must be motivated: if the only purpose is to indicate to the consumer that all GPIOs are on the same chip, why not just have a bool all_on_same_chip; That you set to true if these are all on the same chip? Yours, Linus Walleij
Hi Linus, On Tuesday, August 7, 2018 1:29:43 AM CEST Linus Walleij wrote: > Hi Janusz! > > On Tue, Aug 7, 2018 at 12:29 AM Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote: > > > Certain GPIO array lookup results may map directly to GPIO pins of a > > single GPIO chip in hardware order. If that condition is recognized > > and handled efficiently, significant performance gain of get/set array > > functions may be possible. > > > > While processing a request for an array of GPIO descriptors, verify if > > the descriptors just collected represent consecutive pins of a single > > GPIO chip. Pass that information with the array to the caller so it > > can benefit from enhanced performance as soon as bitmap based get/set > > array functions which can make efficient use of that are available. > > > > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> > (...) > > This function returns a struct gpio_descs which contains an array of > > -descriptors:: > > +descriptors. It may also contain a valid descriptor of a single GPIO chip in > > +case the array strictly matches pin hardware layout of the chip:: > > > > struct gpio_descs { > > unsigned int ndescs; > > struct gpio_desc *desc[]; > > + struct gpio_chip *chip; > > This must be motivated: if the only purpose is to indicate to the consumer that > all GPIOs are on the same chip, why not just have a > > bool all_on_same_chip; > > That you set to true if these are all on the same chip? My approach would probably save one or two instructions per get/set call, but I'm not stuck to it and will be happy to find a better solution. How about folding the chip descriptor inside an additional structure, private to drivers, with internals not revealed to consumers? Thanks, Janusz
On Tue, 07 Aug 2018 18:50:22 +0200 Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote: > Hi Linus, > > On Tuesday, August 7, 2018 1:29:43 AM CEST Linus Walleij wrote: > > Hi Janusz! > > > > On Tue, Aug 7, 2018 at 12:29 AM Janusz Krzysztofik <jmkrzyszt@gmail.com> > wrote: > > > > > Certain GPIO array lookup results may map directly to GPIO pins of a > > > single GPIO chip in hardware order. If that condition is recognized > > > and handled efficiently, significant performance gain of get/set array > > > functions may be possible. > > > > > > While processing a request for an array of GPIO descriptors, verify if > > > the descriptors just collected represent consecutive pins of a single > > > GPIO chip. Pass that information with the array to the caller so it > > > can benefit from enhanced performance as soon as bitmap based get/set > > > array functions which can make efficient use of that are available. > > > > > > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> > > (...) > > > This function returns a struct gpio_descs which contains an array of > > > -descriptors:: > > > +descriptors. It may also contain a valid descriptor of a single GPIO > chip in > > > +case the array strictly matches pin hardware layout of the chip:: > > > > > > struct gpio_descs { > > > unsigned int ndescs; > > > struct gpio_desc *desc[]; > > > + struct gpio_chip *chip; > > > > This must be motivated: if the only purpose is to indicate to the consumer > that > > all GPIOs are on the same chip, why not just have a > > > > bool all_on_same_chip; > > > > That you set to true if these are all on the same chip? > > My approach would probably save one or two instructions per get/set call, but > I'm not stuck to it and will be happy to find a better solution. > > How about folding the chip descriptor inside an additional structure, private > to drivers, with internals not revealed to consumers? Or just get the chip from gpio_descs->desc[0]->gdev->chip when ->all_on_same_chip is true... That adds 2 dereferencing though.
On Tue, 7 Aug 2018 00:29:15 +0200 Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote: > Certain GPIO array lookup results may map directly to GPIO pins of a > single GPIO chip in hardware order. If that condition is recognized > and handled efficiently, significant performance gain of get/set array > functions may be possible. > > While processing a request for an array of GPIO descriptors, verify if > the descriptors just collected represent consecutive pins of a single > GPIO chip. Pass that information with the array to the caller so it > can benefit from enhanced performance as soon as bitmap based get/set > array functions which can make efficient use of that are available. > > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> > --- > Documentation/driver-api/gpio/consumer.rst | 4 +++- > drivers/gpio/gpiolib.c | 14 ++++++++++++++ > include/linux/gpio/consumer.h | 1 + > 3 files changed, 18 insertions(+), 1 deletion(-) > > diff --git a/Documentation/driver-api/gpio/consumer.rst b/Documentation/driver-api/gpio/consumer.rst > index aa03f389d41d..38a990b5f3b6 100644 > --- a/Documentation/driver-api/gpio/consumer.rst > +++ b/Documentation/driver-api/gpio/consumer.rst > @@ -109,11 +109,13 @@ For a function using multiple GPIOs all of those can be obtained with one call:: > enum gpiod_flags flags) > > This function returns a struct gpio_descs which contains an array of > -descriptors:: > +descriptors. It may also contain a valid descriptor of a single GPIO chip in > +case the array strictly matches pin hardware layout of the chip:: > > struct gpio_descs { > unsigned int ndescs; > struct gpio_desc *desc[]; > + struct gpio_chip *chip; chip is placed at the beginning of the struct in the real code, which is expected since putting it at the end won't work because of the desc[] declaration. ... > diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h > index 21ddbe440030..862ee027a02f 100644 > --- a/include/linux/gpio/consumer.h > +++ b/include/linux/gpio/consumer.h > @@ -22,6 +22,7 @@ struct gpio_desc; > * gpiod_get_array(). > */ > struct gpio_descs { > + struct gpio_chip *chip; > unsigned int ndescs; > struct gpio_desc *desc[]; > };
On Tuesday, August 7, 2018 7:14:20 PM CEST Boris Brezillon wrote: > On Tue, 7 Aug 2018 00:29:15 +0200 > Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote: > > > Certain GPIO array lookup results may map directly to GPIO pins of a > > single GPIO chip in hardware order. If that condition is recognized > > and handled efficiently, significant performance gain of get/set array > > functions may be possible. > > > > While processing a request for an array of GPIO descriptors, verify if > > the descriptors just collected represent consecutive pins of a single > > GPIO chip. Pass that information with the array to the caller so it > > can benefit from enhanced performance as soon as bitmap based get/set > > array functions which can make efficient use of that are available. > > > > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> > > --- > > Documentation/driver-api/gpio/consumer.rst | 4 +++- > > drivers/gpio/gpiolib.c | 14 ++++++++++++++ > > include/linux/gpio/consumer.h | 1 + > > 3 files changed, 18 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/driver-api/gpio/consumer.rst b/Documentation/driver-api/gpio/consumer.rst > > index aa03f389d41d..38a990b5f3b6 100644 > > --- a/Documentation/driver-api/gpio/consumer.rst > > +++ b/Documentation/driver-api/gpio/consumer.rst > > @@ -109,11 +109,13 @@ For a function using multiple GPIOs all of those can be obtained with one call:: > > enum gpiod_flags flags) > > > > This function returns a struct gpio_descs which contains an array of > > -descriptors:: > > +descriptors. It may also contain a valid descriptor of a single GPIO chip in > > +case the array strictly matches pin hardware layout of the chip:: > > > > struct gpio_descs { > > unsigned int ndescs; > > struct gpio_desc *desc[]; > > + struct gpio_chip *chip; > > chip is placed at the beginning of the struct in the real code, which > is expected since putting it at the end won't work because of the > desc[] declaration. Yes, I've already noticed that and will fix on next iteration, thanks. Janusz
diff --git a/Documentation/driver-api/gpio/consumer.rst b/Documentation/driver-api/gpio/consumer.rst index aa03f389d41d..38a990b5f3b6 100644 --- a/Documentation/driver-api/gpio/consumer.rst +++ b/Documentation/driver-api/gpio/consumer.rst @@ -109,11 +109,13 @@ For a function using multiple GPIOs all of those can be obtained with one call:: enum gpiod_flags flags) This function returns a struct gpio_descs which contains an array of -descriptors:: +descriptors. It may also contain a valid descriptor of a single GPIO chip in +case the array strictly matches pin hardware layout of the chip:: struct gpio_descs { unsigned int ndescs; struct gpio_desc *desc[]; + struct gpio_chip *chip; } The following function returns NULL instead of -ENOENT if no GPIOs have been diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index bdbfc95793e7..c50bcec6e2d7 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -4161,6 +4161,7 @@ struct gpio_descs *__must_check gpiod_get_array(struct device *dev, { struct gpio_desc *desc; struct gpio_descs *descs; + struct gpio_chip *chip; int count; count = gpiod_count(dev, con_id); @@ -4177,6 +4178,19 @@ struct gpio_descs *__must_check gpiod_get_array(struct device *dev, gpiod_put_array(descs); return ERR_CAST(desc); } + + /* + * Verify if the array qualifies for fast bitmap operations + * (single chip, pins in hardware order starting from 0) + * and mark the array with the chip descriptor if true. + */ + chip = gpiod_to_chip(desc); + if (descs->chip == NULL) + descs->chip = chip; + if (!IS_ERR(descs->chip) && (chip != descs->chip || + gpio_chip_hwgpio(desc) != descs->ndescs)) + descs->chip = ERR_PTR(-EINVAL); + descs->desc[descs->ndescs] = desc; descs->ndescs++; } diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h index 21ddbe440030..862ee027a02f 100644 --- a/include/linux/gpio/consumer.h +++ b/include/linux/gpio/consumer.h @@ -22,6 +22,7 @@ struct gpio_desc; * gpiod_get_array(). */ struct gpio_descs { + struct gpio_chip *chip; unsigned int ndescs; struct gpio_desc *desc[]; };
Certain GPIO array lookup results may map directly to GPIO pins of a single GPIO chip in hardware order. If that condition is recognized and handled efficiently, significant performance gain of get/set array functions may be possible. While processing a request for an array of GPIO descriptors, verify if the descriptors just collected represent consecutive pins of a single GPIO chip. Pass that information with the array to the caller so it can benefit from enhanced performance as soon as bitmap based get/set array functions which can make efficient use of that are available. Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> --- Documentation/driver-api/gpio/consumer.rst | 4 +++- drivers/gpio/gpiolib.c | 14 ++++++++++++++ include/linux/gpio/consumer.h | 1 + 3 files changed, 18 insertions(+), 1 deletion(-)