diff mbox series

[RFC,v2,09/12] gpiolib: Identify GPIO descriptor arrays with direct mapping

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

Commit Message

Janusz Krzysztofik Aug. 6, 2018, 10:29 p.m. UTC
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(-)

Comments

Linus Walleij Aug. 6, 2018, 11:29 p.m. UTC | #1
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
Janusz Krzysztofik Aug. 7, 2018, 4:50 p.m. UTC | #2
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
Boris Brezillon Aug. 7, 2018, 5:10 p.m. UTC | #3
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.
Boris Brezillon Aug. 7, 2018, 5:14 p.m. UTC | #4
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[];
>  };
Janusz Krzysztofik Aug. 7, 2018, 5:19 p.m. UTC | #5
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 mbox series

Patch

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[];
 };