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

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 at lists.freedesktop.org>
Cc: <nouveau at lists.freedesktop.org>
Cc: Benjamin Tissoires <benjamin.tissoires at redhat.com>
Cc: Peter Hutterer <peter.hutterer at who-t.net>
Signed-off-by: Lv Zheng <lv.zheng at 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

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 at lists.freedesktop.org>
> Cc: <nouveau at lists.freedesktop.org>
> Cc: Benjamin Tissoires <benjamin.tissoires at redhat.com>
> Cc: Peter Hutterer <peter.hutterer at who-t.net>
> Signed-off-by: Lv Zheng <lv.zheng at 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
>

Hi,
> From: Benjamin Tissoires [mailto:benjamin.tissoires at 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 at lists.freedesktop.org>
> > Cc: <nouveau at lists.freedesktop.org>
> > Cc: Benjamin Tissoires <benjamin.tissoires at redhat.com>
> > Cc: Peter Hutterer <peter.hutterer at who-t.net>
> > Signed-off-by: Lv Zheng <lv.zheng at 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