Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1651698 > unrolled thread

[RFC PATCH v3 5/5] ACPI: button: Always notify kernel space using _LID returning value

Started byLv Zheng <lv.zheng@intel.com>
First post2017-05-27 03:40 +0200
Last post2017-05-31 11:10 +0200
Articles 3 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC PATCH v3 5/5] ACPI: button: Always notify kernel space using _LID returning value Lv Zheng <lv.zheng@intel.com> - 2017-05-27 03:40 +0200
    Re: [RFC PATCH v3 5/5] ACPI: button: Always notify kernel space  using _LID returning value Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-05-29 18:10 +0200
      RE: [RFC PATCH v3 5/5] ACPI: button: Always notify kernel space  using _LID returning value "Zheng, Lv" <lv.zheng@intel.com> - 2017-05-31 11:10 +0200

#1651698 — [RFC PATCH v3 5/5] ACPI: button: Always notify kernel space using _LID returning value

FromLv Zheng <lv.zheng@intel.com>
Date2017-05-27 03:40 +0200
Subject[RFC PATCH v3 5/5] ACPI: button: Always notify kernel space using _LID returning value
Message-ID<tLyRY-4YN-17@gated-at.bofh.it>
Both nouveau and i915, the only 2 kernel space lid notification listeners,
invoke acpi_lid_open() API to obtain _LID returning value instead of using
the notified value.

So this patch moves this logic from listeners to lid driver, always notify
kernel space listeners using _LID returning value.

This is a no-op cleanup, but facilitates administrators to configure to
notify kernel drivers with faked lid init states via command line
"button.lid_notify_init_state=Y".

Cc: <intel-gfx@lists.freedesktop.org>
Cc: <nouveau@lists.freedesktop.org>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/button.c             | 16 ++++++++++++++--
 drivers/gpu/drm/i915/intel_lvds.c |  2 +-
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 4abf8ae..e047d34 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -119,6 +119,9 @@ static u8 lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
 static unsigned long lid_report_interval __read_mostly = 500;
 module_param(lid_report_interval, ulong, 0644);
 MODULE_PARM_DESC(lid_report_interval, "Interval (ms) between lid key events");
+static bool lid_notify_init_state __read_mostly = false;
+module_param(lid_notify_init_state, bool, 0644);
+MODULE_PARM_DESC(lid_notify_init_state, "Notify init lid state to kernel drivers after boot/resume");
 
 /* --------------------------------------------------------------------------
                               FS Interface (/proc)
@@ -224,6 +227,15 @@ static void acpi_lid_notify_state(struct acpi_device *device,
 	if (state)
 		pm_wakeup_event(&device->dev, 0);
 
+	if (!lid_notify_init_state) {
+		/*
+		 * There are cases "state" is not a _LID return value, so
+		 * correct it before notification.
+		 */
+		if (!bios_notify &&
+		    lid_init_state != ACPI_BUTTON_LID_INIT_METHOD)
+			state = acpi_lid_evaluate_state(device);
+	}
 	acpi_lid_notifier_call(device, state);
 }
 
@@ -572,10 +584,10 @@ static int param_set_lid_init_state(const char *val, struct kernel_param *kp)
 
 	if (!strncmp(val, "open", sizeof("open") - 1)) {
 		lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
-		pr_info("Notify initial lid state as open\n");
+		pr_info("Notify initial lid state to users space as open and kernel drivers with _LID return value\n");
 	} else if (!strncmp(val, "method", sizeof("method") - 1)) {
 		lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
-		pr_info("Notify initial lid state with _LID return value\n");
+		pr_info("Notify initial lid state to user/kernel space with _LID return value\n");
 	} else if (!strncmp(val, "ignore", sizeof("ignore") - 1)) {
 		lid_init_state = ACPI_BUTTON_LID_INIT_IGNORE;
 		pr_info("Do not notify initial lid state\n");
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 9ca4dc4..8ca9080 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -548,7 +548,7 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
 	/* Don't force modeset on machines where it causes a GPU lockup */
 	if (dmi_check_system(intel_no_modeset_on_lid))
 		goto exit;
-	if (!acpi_lid_open()) {
+	if (!val) {
 		/* do modeset on next lid open event */
 		dev_priv->modeset_restore = MODESET_ON_LID_OPEN;
 		goto exit;
-- 
2.7.4

[toc] | [next] | [standalone]

#1652626 — Re: [RFC PATCH v3 5/5] ACPI: button: Always notify kernel space using _LID returning value

FromBenjamin Tissoires <benjamin.tissoires@redhat.com>
Date2017-05-29 18:10 +0200
SubjectRe: [RFC PATCH v3 5/5] ACPI: button: Always notify kernel space using _LID returning value
Message-ID<tMvoZ-2iD-5@gated-at.bofh.it>
In reply to#1651698
Hi Lv,

On May 27 2017 or thereabouts, Lv Zheng wrote:
> Both nouveau and i915, the only 2 kernel space lid notification listeners,
> invoke acpi_lid_open() API to obtain _LID returning value instead of using
> the notified value.
> 
> So this patch moves this logic from listeners to lid driver, always notify
> kernel space listeners using _LID returning value.
> 
> This is a no-op cleanup, but facilitates administrators to configure to
> notify kernel drivers with faked lid init states via command line
> "button.lid_notify_init_state=Y".
> 
> Cc: <intel-gfx@lists.freedesktop.org>
> Cc: <nouveau@lists.freedesktop.org>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Cc: Peter Hutterer <peter.hutterer@who-t.net>
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> ---
>  drivers/acpi/button.c             | 16 ++++++++++++++--
>  drivers/gpu/drm/i915/intel_lvds.c |  2 +-
>  2 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index 4abf8ae..e047d34 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -119,6 +119,9 @@ static u8 lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
>  static unsigned long lid_report_interval __read_mostly = 500;
>  module_param(lid_report_interval, ulong, 0644);
>  MODULE_PARM_DESC(lid_report_interval, "Interval (ms) between lid key events");
> +static bool lid_notify_init_state __read_mostly = false;
> +module_param(lid_notify_init_state, bool, 0644);
> +MODULE_PARM_DESC(lid_notify_init_state, "Notify init lid state to kernel drivers after boot/resume");
>  
>  /* --------------------------------------------------------------------------
>                                FS Interface (/proc)
> @@ -224,6 +227,15 @@ static void acpi_lid_notify_state(struct acpi_device *device,
>  	if (state)
>  		pm_wakeup_event(&device->dev, 0);
>  
> +	if (!lid_notify_init_state) {
> +		/*
> +		 * There are cases "state" is not a _LID return value, so
> +		 * correct it before notification.
> +		 */
> +		if (!bios_notify &&
> +		    lid_init_state != ACPI_BUTTON_LID_INIT_METHOD)
> +			state = acpi_lid_evaluate_state(device);
> +	}
>  	acpi_lid_notifier_call(device, state);
>  }
>  
> @@ -572,10 +584,10 @@ static int param_set_lid_init_state(const char *val, struct kernel_param *kp)
>  
>  	if (!strncmp(val, "open", sizeof("open") - 1)) {
>  		lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
> -		pr_info("Notify initial lid state as open\n");
> +		pr_info("Notify initial lid state to users space as open and kernel drivers with _LID return value\n");
>  	} else if (!strncmp(val, "method", sizeof("method") - 1)) {
>  		lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
> -		pr_info("Notify initial lid state with _LID return value\n");
> +		pr_info("Notify initial lid state to user/kernel space with _LID return value\n");
>  	} else if (!strncmp(val, "ignore", sizeof("ignore") - 1)) {
>  		lid_init_state = ACPI_BUTTON_LID_INIT_IGNORE;
>  		pr_info("Do not notify initial lid state\n");
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 9ca4dc4..8ca9080 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -548,7 +548,7 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
>  	/* Don't force modeset on machines where it causes a GPU lockup */
>  	if (dmi_check_system(intel_no_modeset_on_lid))
>  		goto exit;
> -	if (!acpi_lid_open()) {
> +	if (!val) {
>  		/* do modeset on next lid open event */
>  		dev_priv->modeset_restore = MODESET_ON_LID_OPEN;
>  		goto exit;

This last hunk should really be in its own patch because the intel GPU
folks would need to apply the rest of the series for their CI suite, and
also because there is no reason for this change to be alongside any
other acpi/button.c change.

Cheers,
Benjamin

> -- 
> 2.7.4
>

[toc] | [prev] | [next] | [standalone]

#1654023 — RE: [RFC PATCH v3 5/5] ACPI: button: Always notify kernel space using _LID returning value

From"Zheng, Lv" <lv.zheng@intel.com>
Date2017-05-31 11:10 +0200
SubjectRE: [RFC PATCH v3 5/5] ACPI: button: Always notify kernel space using _LID returning value
Message-ID<tN7ND-2Od-7@gated-at.bofh.it>
In reply to#1652626
Hi,

> From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
> Subject: Re: [RFC PATCH v3 5/5] ACPI: button: Always notify kernel space using _LID returning value
> 
> Hi Lv,
> 
> On May 27 2017 or thereabouts, Lv Zheng wrote:
> > Both nouveau and i915, the only 2 kernel space lid notification listeners,
> > invoke acpi_lid_open() API to obtain _LID returning value instead of using
> > the notified value.
> >
> > So this patch moves this logic from listeners to lid driver, always notify
> > kernel space listeners using _LID returning value.
> >
> > This is a no-op cleanup, but facilitates administrators to configure to
> > notify kernel drivers with faked lid init states via command line
> > "button.lid_notify_init_state=Y".
> >
> > Cc: <intel-gfx@lists.freedesktop.org>
> > Cc: <nouveau@lists.freedesktop.org>
> > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > Cc: Peter Hutterer <peter.hutterer@who-t.net>
> > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> > ---
> >  drivers/acpi/button.c             | 16 ++++++++++++++--
> >  drivers/gpu/drm/i915/intel_lvds.c |  2 +-
> >  2 files changed, 15 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> > index 4abf8ae..e047d34 100644
> > --- a/drivers/acpi/button.c
> > +++ b/drivers/acpi/button.c
> > @@ -119,6 +119,9 @@ static u8 lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
> >  static unsigned long lid_report_interval __read_mostly = 500;
> >  module_param(lid_report_interval, ulong, 0644);
> >  MODULE_PARM_DESC(lid_report_interval, "Interval (ms) between lid key events");
> > +static bool lid_notify_init_state __read_mostly = false;
> > +module_param(lid_notify_init_state, bool, 0644);
> > +MODULE_PARM_DESC(lid_notify_init_state, "Notify init lid state to kernel drivers after
> boot/resume");
> >
> >  /* --------------------------------------------------------------------------
> >                                FS Interface (/proc)
> > @@ -224,6 +227,15 @@ static void acpi_lid_notify_state(struct acpi_device *device,
> >  	if (state)
> >  		pm_wakeup_event(&device->dev, 0);
> >
> > +	if (!lid_notify_init_state) {
> > +		/*
> > +		 * There are cases "state" is not a _LID return value, so
> > +		 * correct it before notification.
> > +		 */
> > +		if (!bios_notify &&
> > +		    lid_init_state != ACPI_BUTTON_LID_INIT_METHOD)
> > +			state = acpi_lid_evaluate_state(device);
> > +	}
> >  	acpi_lid_notifier_call(device, state);
> >  }
> >
> > @@ -572,10 +584,10 @@ static int param_set_lid_init_state(const char *val, struct kernel_param *kp)
> >
> >  	if (!strncmp(val, "open", sizeof("open") - 1)) {
> >  		lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
> > -		pr_info("Notify initial lid state as open\n");
> > +		pr_info("Notify initial lid state to users space as open and kernel drivers with _LID
> return value\n");
> >  	} else if (!strncmp(val, "method", sizeof("method") - 1)) {
> >  		lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
> > -		pr_info("Notify initial lid state with _LID return value\n");
> > +		pr_info("Notify initial lid state to user/kernel space with _LID return value\n");
> >  	} else if (!strncmp(val, "ignore", sizeof("ignore") - 1)) {
> >  		lid_init_state = ACPI_BUTTON_LID_INIT_IGNORE;
> >  		pr_info("Do not notify initial lid state\n");
> > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > index 9ca4dc4..8ca9080 100644
> > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > @@ -548,7 +548,7 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
> >  	/* Don't force modeset on machines where it causes a GPU lockup */
> >  	if (dmi_check_system(intel_no_modeset_on_lid))
> >  		goto exit;
> > -	if (!acpi_lid_open()) {
> > +	if (!val) {
> >  		/* do modeset on next lid open event */
> >  		dev_priv->modeset_restore = MODESET_ON_LID_OPEN;
> >  		goto exit;
> 
> This last hunk should really be in its own patch because the intel GPU
> folks would need to apply the rest of the series for their CI suite, and
> also because there is no reason for this change to be alongside any
> other acpi/button.c change.

OK, I'll drop i915 related changes.
However I can see cleanup chances in button.c.
I feel I should at least do minimal tunings in button driver to allow future improvements.

Cheers,
Lv

> Cheers,
> Benjamin

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web