diff mbox series

input: gpio-keys - optimize wakeup sequence.

Message ID 899376598.3134980.1698299600677@mail-kr5-0.mail-kr5.knoxportal-kr-prod-blue.svc.cluster.local (mailing list archive)
State New
Headers show
Series input: gpio-keys - optimize wakeup sequence. | expand

Commit Message

Abhishek Kumar Singh Oct. 26, 2023, 5:53 a.m. UTC 
Dear Mr. Dmitry,
Greetings!


The patch removes unused many APIs call chain for every suspend/resume of the device 
if no key press event triggered.


There is a call back function gpio_keys_resume() called for
every suspend/resume of the device. and whenever this function called, it is
reading the status of the key. And gpio_keys_resume() API further calls the
below chain of API irrespective of key press event


APIs call chain:
static void gpio_keys_report_state(struct gpio_keys_drvdata *ddata)
static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
gpiod_get_value_cansleep(bdata->gpiod);
input_event(input, type, *bdata->code, state);
input_sync(input);


The patch avoid the above APIs call chain if there is no key press event triggered.
It will save the device computational resources, power resources and optimize the suspend/resume time


Signed-off-by: Abhishek Kumar Singh <abhi1.singh@samsung.com>


---
 linux-6.4.11/drivers/input/keyboard/gpio_keys.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)


---




Thanks and Regards,
Abhishek Kumar Singh
Sr. Chief Engineer, Samsung Electronics, Noida-India

Comments

Dmitry Torokhov Oct. 29, 2023, 2:11 a.m. UTC | #1 
Hi Abhishek,

On Thu, Oct 26, 2023 at 11:23:20AM +0530, Abhishek Kumar Singh wrote:
> Dear Mr. Dmitry,
> Greetings!
> 
> 
> The patch removes unused many APIs call chain for every suspend/resume of the device 
> if no key press event triggered.
> 
> 
> There is a call back function gpio_keys_resume() called for
> every suspend/resume of the device. and whenever this function called, it is
> reading the status of the key. And gpio_keys_resume() API further calls the
> below chain of API irrespective of key press event
> 
> 
> APIs call chain:
> static void gpio_keys_report_state(struct gpio_keys_drvdata *ddata)
> static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
> gpiod_get_value_cansleep(bdata->gpiod);
> input_event(input, type, *bdata->code, state);
> input_sync(input);
> 
> 
> The patch avoid the above APIs call chain if there is no key press event triggered.
> It will save the device computational resources, power resources and optimize the suspend/resume time

Unfortunately it also breaks the driver as button->value does not hold
the current state of the GPIO but rather set one via device tree so that
the driver can use that value when sending EV_ABS events. So with
typical GPIO-backed keys or buttons you change results in no events
reported on resume.

I also wonder what kind of measurements you did on improvements to
suspend/resume time with your change.

Thanks.
Abhishek Kumar Singh Nov. 7, 2023, 8:29 a.m. UTC | #2 
Dear Dmitry, 

Greetings!!!

Thank you so much for your response.

I checked in detailed again and observed the below points, please help to review 
and approve it.



There is ISR "gpio_keys_gpio_isr" which is called when the key state is 1 i.e. 
key pressed.
Therefore modified code will not impact on the existing driver code.

//For key pressed event:
<3>[  549.180072] I[0:      swapper/0:    0] gpio_keys_gpio_isr
<3>[  549.196126]  [1:    kworker/1:1:   78] gpio_keys_gpio_work_func
<3>[  549.196198]  [1:    kworker/1:1:   78] gpio-keys soc:gpio_keys: gpio_keys_gpio_report_event key = 115, value = 1



Performance:

I have calculated the differece between entry & exit time 
with modified and without modified code and observed that 
0.3ms extra computation time in current scenario in each entry/exit time.

Because below APIs will not be called in every resume functions:

1. static void gpio_keys_report_state(struct gpio_keys_drvdata *ddata)
2. static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
3. gpiod_get_value_cansleep(bdata->gpiod);
4. input_event(input, type, *bdata->code, state);
5. input_sync(input)

So we can save 0.3ms computation time, resume operations will faster and save battery as well.



With changes:

Line   311960: 07-18 16:50:09.359  root     0     0 I [0:   Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:20:37.573207725 UTC
Line   312626: 07-18 16:50:42.123  root     0     0 I [0:   Binder:699_1:20972] PM: gpio_keys_report_state enrty 2023-07-18 11:22:20.503579404 UTC
Line   312627: 07-18 16:50:42.123  root     0     0 I [0:   Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:22:20.503656644 UTC
Line   313301: 07-18 16:52:24.182  root     0     0 I [0:   Binder:699_1:20972] PM: gpio_keys_report_state enrty 2023-07-18 11:22:33.865626325 UTC
Line   313302: 07-18 16:52:24.182  root     0     0 I [0:   Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:22:33.865724502 UTC
Line   313572: 07-18 16:52:35.111  root     0     0 I [1:   Binder:699_1:20972] PM: gpio_keys_report_state enrty 2023-07-18 11:22:37.678468979 UTC
Line   313573: 07-18 16:52:35.111  root     0     0 I [1:   Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:22:37.678566167 UTC
Line   314209: 07-18 16:52:43.598  root     0     0 I [0:   Binder:699_1:20972] PM: gpio_keys_report_state enrty 2023-07-18 11:23:05.925340634 UTC
Line   314210: 07-18 16:52:43.598  root     0     0 I [0:   Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:23:05.925439384 UTC


Without changes:

Line   372095: 07-18 16:10:24.250  root     0     0 I [1:   Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:43:38.592548979 UTC
Line   372344: 07-18 16:13:45.439  root     0     0 I [0:   Binder:702_2:18137] PM: gpio_keys_report_state enrty 2023-07-18 10:44:11.589164226 UTC
Line   372346: 07-18 16:13:45.439  root     0     0 I [0:   Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:44:11.589514955 UTC
Line   372573: 07-18 16:14:13.414  root     0     0 I [0:   Binder:702_2:18137] PM: gpio_keys_report_state enrty 2023-07-18 10:44:22.606227138 UTC
Line   372575: 07-18 16:14:13.414  root     0     0 I [0:   Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:44:22.606490107 UTC
Line   372944: 07-18 16:14:26.732  root     0     0 I [1:   Binder:702_2:18137] PM: gpio_keys_report_state enrty 2023-07-18 10:44:29.024121927 UTC
Line   372946: 07-18 16:14:26.732  root     0     0 I [1:   Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:44:29.024528958 UTC
Line   373181: 07-18 16:14:30.790  root     0     0 I [0:   Binder:702_2:18137] PM: gpio_keys_report_state enrty 2023-07-18 10:44:30.904866770 UTC
Line   373183: 07-18 16:14:30.790  root     0     0 I [0:   Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:44:30.905126353 UTC




Thanks and Regards,
Abhishek Kumar Singh
Sr. Chief Engineer, Samsung Electronics, Noida-India


 
--------- Original Message ---------
Sender : dmitry.torokhov@gmail.com <dmitry.torokhov@gmail.com>
Date   : 2023-10-29 07:42 (GMT+5:30)
Title  : Re: [PATCH] input: gpio-keys - optimize wakeup sequence.
To : Abhishek Kumar Singh<abhi1.singh@samsung.com>
CC : robh@kernel.org<robh@kernel.org>, linux-input@vger.kernel.org<linux-input@vger.kernel.org>, linux-kernel@vger.kernel.org<linux-kernel@vger.kernel.org>, SRI-N IT Security<sri-n.itsec@samsung.com>
 
Hi Abhishek,

On Thu, Oct 26, 2023 at 11:23:20AM +0530, Abhishek Kumar Singh wrote:
> Dear Mr. Dmitry,
> Greetings!
> 
> 
> The patch removes unused many APIs call chain for every suspend/resume of the device 
> if no key press event triggered.
> 
> 
> There is a call back function gpio_keys_resume() called for
> every suspend/resume of the device. and whenever this function called, it is
> reading the status of the key. And gpio_keys_resume() API further calls the
> below chain of API irrespective of key press event
> 
> 
> APIs call chain:
> static void gpio_keys_report_state(struct gpio_keys_drvdata *ddata)
> static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
> gpiod_get_value_cansleep(bdata->gpiod);
> input_event(input, type, *bdata->code, state);
> input_sync(input);
> 
> 
> The patch avoid the above APIs call chain if there is no key press event triggered.
> It will save the device computational resources, power resources and optimize the suspend/resume time

Unfortunately it also breaks the driver as button->value does not hold
the current state of the GPIO but rather set one via device tree so that
the driver can use that value when sending EV_ABS events. So with
typical GPIO-backed keys or buttons you change results in no events
reported on resume.

I also wonder what kind of measurements you did on improvements to
suspend/resume time with your change.

Thanks.

-- 
Dmitry

 
Thanks and Regards,
Abhishek Kumar Singh Nov. 20, 2023, 3:59 a.m. UTC | #3 
Dear Dmitry, 

Greetings!!

Could you please help to review and update on this?


Thanks and Regards,
Abhishek Kumar Singh
Sr. Chief Engineer, 
Samsung Electronics, Noida-India

 
--------- Original Message ---------
Sender : Abhishek Kumar Singh <abhi1.singh@samsung.com>System Power Part /SRI-Noida/Samsung Electronics
Date   : 2023-11-07 13:59 (GMT+5:30)
Title  : Re[2]: [PATCH] input: gpio-keys - optimize wakeup sequence.
 
Dear Dmitry, 

Greetings!!!

Thank you so much for your response.

I checked in detailed again and observed the below points, please help to review 
and approve it.



There is ISR "gpio_keys_gpio_isr" which is called when the key state is 1 i.e. 
key pressed.
Therefore modified code will not impact on the existing driver code.

//For key pressed event:
<3>[  549.180072] I[0:      swapper/0:    0] gpio_keys_gpio_isr
<3>[  549.196126]  [1:    kworker/1:1:   78] gpio_keys_gpio_work_func
<3>[  549.196198]  [1:    kworker/1:1:   78] gpio-keys soc:gpio_keys: gpio_keys_gpio_report_event key = 115, value = 1



Performance:

I have calculated the differece between entry & exit time 
with modified and without modified code and observed that 
0.3ms extra computation time in current scenario in each entry/exit time.

Because below APIs will not be called in every resume functions:

1. static void gpio_keys_report_state(struct gpio_keys_drvdata *ddata)
2. static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
3. gpiod_get_value_cansleep(bdata->gpiod);
4. input_event(input, type, *bdata->code, state);
5. input_sync(input)

So we can save 0.3ms computation time, resume operations will faster and save battery as well.



With changes:

Line   311960: 07-18 16:50:09.359  root     0     0 I [0:   Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:20:37.573207725 UTC
Line   312626: 07-18 16:50:42.123  root     0     0 I [0:   Binder:699_1:20972] PM: gpio_keys_report_state enrty 2023-07-18 11:22:20.503579404 UTC
Line   312627: 07-18 16:50:42.123  root     0     0 I [0:   Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:22:20.503656644 UTC
Line   313301: 07-18 16:52:24.182  root     0     0 I [0:   Binder:699_1:20972] PM: gpio_keys_report_state enrty 2023-07-18 11:22:33.865626325 UTC
Line   313302: 07-18 16:52:24.182  root     0     0 I [0:   Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:22:33.865724502 UTC
Line   313572: 07-18 16:52:35.111  root     0     0 I [1:   Binder:699_1:20972] PM: gpio_keys_report_state enrty 2023-07-18 11:22:37.678468979 UTC
Line   313573: 07-18 16:52:35.111  root     0     0 I [1:   Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:22:37.678566167 UTC
Line   314209: 07-18 16:52:43.598  root     0     0 I [0:   Binder:699_1:20972] PM: gpio_keys_report_state enrty 2023-07-18 11:23:05.925340634 UTC
Line   314210: 07-18 16:52:43.598  root     0     0 I [0:   Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:23:05.925439384 UTC


Without changes:

Line   372095: 07-18 16:10:24.250  root     0     0 I [1:   Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:43:38.592548979 UTC
Line   372344: 07-18 16:13:45.439  root     0     0 I [0:   Binder:702_2:18137] PM: gpio_keys_report_state enrty 2023-07-18 10:44:11.589164226 UTC
Line   372346: 07-18 16:13:45.439  root     0     0 I [0:   Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:44:11.589514955 UTC
Line   372573: 07-18 16:14:13.414  root     0     0 I [0:   Binder:702_2:18137] PM: gpio_keys_report_state enrty 2023-07-18 10:44:22.606227138 UTC
Line   372575: 07-18 16:14:13.414  root     0     0 I [0:   Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:44:22.606490107 UTC
Line   372944: 07-18 16:14:26.732  root     0     0 I [1:   Binder:702_2:18137] PM: gpio_keys_report_state enrty 2023-07-18 10:44:29.024121927 UTC
Line   372946: 07-18 16:14:26.732  root     0     0 I [1:   Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:44:29.024528958 UTC
Line   373181: 07-18 16:14:30.790  root     0     0 I [0:   Binder:702_2:18137] PM: gpio_keys_report_state enrty 2023-07-18 10:44:30.904866770 UTC
Line   373183: 07-18 16:14:30.790  root     0     0 I [0:   Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:44:30.905126353 UTC




Thanks and Regards,
Abhishek Kumar Singh
Sr. Chief Engineer, Samsung Electronics, Noida-India


 
--------- Original Message ---------
Sender : dmitry.torokhov@gmail.com <dmitry.torokhov@gmail.com>
Date   : 2023-10-29 07:42 (GMT+5:30)
Title  : Re: [PATCH] input: gpio-keys - optimize wakeup sequence.
To : Abhishek Kumar Singh<abhi1.singh@samsung.com>
CC : robh@kernel.org<robh@kernel.org>, linux-input@vger.kernel.org<linux-input@vger.kernel.org>, linux-kernel@vger.kernel.org<linux-kernel@vger.kernel.org>, SRI-N IT Security<sri-n.itsec@samsung.com>
 
Hi Abhishek,

On Thu, Oct 26, 2023 at 11:23:20AM +0530, Abhishek Kumar Singh wrote:
> Dear Mr. Dmitry,
> Greetings!
> 
> 
> The patch removes unused many APIs call chain for every suspend/resume of the device 
> if no key press event triggered.
> 
> 
> There is a call back function gpio_keys_resume() called for
> every suspend/resume of the device. and whenever this function called, it is
> reading the status of the key. And gpio_keys_resume() API further calls the
> below chain of API irrespective of key press event
> 
> 
> APIs call chain:
> static void gpio_keys_report_state(struct gpio_keys_drvdata *ddata)
> static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
> gpiod_get_value_cansleep(bdata->gpiod);
> input_event(input, type, *bdata->code, state);
> input_sync(input);
> 
> 
> The patch avoid the above APIs call chain if there is no key press event triggered.
> It will save the device computational resources, power resources and optimize the suspend/resume time

Unfortunately it also breaks the driver as button->value does not hold
the current state of the GPIO but rather set one via device tree so that
the driver can use that value when sending EV_ABS events. So with
typical GPIO-backed keys or buttons you change results in no events
reported on resume.

I also wonder what kind of measurements you did on improvements to
suspend/resume time with your change.

Thanks.

-- 
Dmitry

 
Thanks and Regards,
  
 
Thanks and Regards,
diff mbox series

Patch

diff --git a/linux-6.4.11/drivers/input/keyboard/gpio_keys.c b/linux-6.4.11/drivers/input/keyboard/gpio_keys.c
index 13a8ba5..cd1609e 100644
--- a/linux-6.4.11/drivers/input/keyboard/gpio_keys.c
+++ b/linux-6.4.11/drivers/input/keyboard/gpio_keys.c
@@ -687,8 +687,12 @@  static void gpio_keys_report_state(struct gpio_keys_drvdata *ddata)
 
   for (i = 0; i < ddata->pdata->nbuttons; i++) {
     struct gpio_button_data *bdata = &ddata->data[i];
-    if (bdata->gpiod)
-      gpio_keys_gpio_report_event(bdata);
+    if (bdata->gpiod) {
+      struct gpio_keys_button *button = bdata->button;
+      if(button->value) {
+        gpio_keys_gpio_report_event(bdata);
+      }
+    }
   }
   input_sync(input);
 }