[PATCH 0/3] Use acpi_video_unregister_backlight instead of acpi_video_unregister in platfrom backlight drivers

[PATCH 0/3] Use acpi_video_unregister_backlight instead of acpi_video_unregister in platfrom backlight drivers

[Hans de Goede]

Hi All,

I'm working on cleaning up the currently somewhat convoluted logic to
select which backlight interfaces to register on x86 systems, see:
http://lists.freedesktop.org/archives/dri-devel/2014-December/074687.html

For a rought outline (details will change in the actual patch-set).

These 3 patches are a preparation for that work, as the behavior of the
current code is not always consistent (it changes depending on module
loading order in some cases). These 3 patches remove this inconsistency
which in some cases may result in a behavior change.

This way the cleanup can have a consistent base to build upon, and I can
ensure that the cleanup itself does not cause any functional changes.

I hope to post a v1 of the actual cleanup patch-set in 1-2 weeks.

Regards,

Hans

[PATCH 1/3] apple_gmux: Use acpi_video_unregister_backlight instead of acpi_video_unregister

[Hans de Goede]

acpi_video_unregister() not only unregisters the acpi-video backlight
interface but also unregisters the acpi video bus event listener, causing
e.g. brightness hotkey presses to no longer generate keypress events.

The unregistering of the acpi video bus event listener usually is
undesirable, which by itself is a good reason to switch to
acpi_video_unregister_backlight().

Another problem with using acpi_video_unregister() rather then using
acpi_video_unregister_backlight() is that on systems with an intel video
opregion (most systems) whether or not the acpi video bus event listener
actually gets unregistered depends on module load ordering:

Scenario a:
1) acpi/video.ko gets loaded (*), does not do acpi_video_register as there
   is an intel opregion.
2) intel.ko gets loaded, calls acpi_video_register() which registers both
   the listener and the acpi backlight interface
3) apple-gmux.ko gets loaded, calls acpi_video_unregister() causing both
   the listener and the acpi backlight interface to unregister

Scenario b:
1) acpi/video.ko gets loaded (*), does not do acpi_video_register as there
   is an intel opregion.
2) apple-gmux.ko gets loaded, calls acpi_video_dmi_promote_vendor(),
   calls acpi_video_unregister(), which is a nop since acpi_video_register
   has not yet been called
2) intel.ko gets loaded, calls acpi_video_register() which registers
   the listener, but does not register the acpi backlight interface due to
   the call to the preciding call to acpi_video_dmi_promote_vendor()

*) acpi/video.ko always loads first as both other modules depend on it.

So we end up with or without an acpi video bus event listener depending
on module load ordering, not good.

Switching to using acpi_video_unregister_backlight() means that independ
of ordering we will always have an acpi video bus event listener fixing
this.

Note that this commit means that systems without an intel video opregion,
and systems which were hitting scenario a wrt module load ordering, are
now getting an acpi video bus event listener while before they were not!

On some systems this may cause the brightness hotkeys to start generating
keypresses while before they were not (good), while on other systems this
may cause the brightness hotkeys to generate multiple keypress events for
a single press (not so good). Since on most systems the acpi video bus is
the canonical source for brightness events I believe that the latter case
will needs to be handled on a case by case basis by filtering out the
duplicate keypresses at the other source for them.

Cc: Seth Forshee <seth.forshee@canonical.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/apple-gmux.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index 6808715..45032ce 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -551,7 +551,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 	 * Disable the other backlight choices.
 	 */
 	acpi_video_dmi_promote_vendor();
-	acpi_video_unregister();
+	acpi_video_unregister_backlight();
 	apple_bl_unregister();
 
 	gmux_data->power_state = VGA_SWITCHEROO_ON;
-- 
2.4.2

[PATCH 2/3] asus-wmi: Use acpi_video_unregister_backlight instead of acpi_video_unregister

[Hans de Goede]

acpi_video_unregister() not only unregisters the acpi-video backlight
interface but also unregisters the acpi video bus event listener, causing
e.g. brightness hotkey presses to no longer generate keypress events.

The unregistering of the acpi video bus event listener usually is
undesirable, which by itself is a good reason to switch to
acpi_video_unregister_backlight().

Another problem with using acpi_video_unregister() rather then using
acpi_video_unregister_backlight() is that on systems with an intel video
opregion (most systems) and a wmi_backlight_power quirk, whether or not
the acpi video bus event listener actually gets unregistered depends on
module load ordering:

Scenario a:
1) acpi/video.ko gets loaded (*), does not do acpi_video_register as there
   is an intel opregion.
2) intel.ko gets loaded, calls acpi_video_register() which registers both
   the listener and the acpi backlight interface
3) asus-wmi.ko gets loaded, calls acpi_video_unregister() causing both
   the listener and the acpi backlight interface to unregister

Scenario b:
1) acpi/video.ko gets loaded (*), does not do acpi_video_register as there
   is an intel opregion.
2) asus-wmi.ko gets loaded, calls acpi_video_dmi_promote_vendor(),
   calls acpi_video_unregister(), which is a nop since acpi_video_register
   has not yet been called
2) intel.ko gets loaded, calls acpi_video_register() which registers
   the listener, but does not register the acpi backlight interface due to
   the call to the preciding call to acpi_video_dmi_promote_vendor()

*) acpi/video.ko always loads first as both other modules depend on it.

So we end up with or without an acpi video bus event listener depending
on module load ordering, not good.

Switching to using acpi_video_unregister_backlight() means that independ
of ordering we will always have an acpi video bus event listener fixing
this.

Note that this commit means that systems without an intel video opregion,
and systems which were hitting scenario a wrt module load ordering, are
now getting an acpi video bus event listener while before they were not!

On some systems this may cause the brightness hotkeys to start generating
keypresses while before they were not (good), while on other systems this
may cause the brightness hotkeys to generate multiple keypress events for
a single press (not so good). Since on most systems the acpi video bus is
the canonical source for brightness events I believe that the latter case
will needs to be handled on a case by case basis by filtering out the
duplicate keypresses at the other source for them.

Cc: Corentin Chary <corentin.chary@gmail.com>
Cc: acpi4asus-user@lists.sourceforge.net
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/asus-wmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 7543a56..945145d 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -1777,7 +1777,7 @@ static int asus_wmi_add(struct platform_device *pdev)
 		acpi_video_dmi_promote_vendor();
 	if (!acpi_video_backlight_support()) {
 		pr_info("Disabling ACPI video driver\n");
-		acpi_video_unregister();
+		acpi_video_unregister_backlight();
 		err = asus_wmi_backlight_init(asus);
 		if (err && err != -ENODEV)
 			goto fail_backlight;
-- 
2.4.2

[PATCH 3/3] samsung-laptop: Use acpi_video_unregister_backlight instead of acpi_video_unregister

[Hans de Goede]

acpi_video_unregister() not only unregisters the acpi-video backlight
interface but also unregisters the acpi video bus event listener, causing
e.g. brightness hotkey presses to no longer generate keypress events.

The unregistering of the acpi video bus event listener usually is
undesirable, which by itself is a good reason to switch to
acpi_video_unregister_backlight().

Another problem with using acpi_video_unregister() rather then using
acpi_video_unregister_backlight() is that on systems with an intel video
opregion (most systems) and a broken_acpi_video quirk, whether or not
the acpi video bus event listener actually gets unregistered depends on
module load ordering:

Scenario a:
1) acpi/video.ko gets loaded (*), does not do acpi_video_register as there
   is an intel opregion.
2) intel.ko gets loaded, calls acpi_video_register() which registers both
   the listener and the acpi backlight interface
3) samsung-laptop.ko gets loaded, calls acpi_video_unregister() causing
   both the listener and the acpi backlight interface to unregister

Scenario b:
1) acpi/video.ko gets loaded (*), does not do acpi_video_register as there
   is an intel opregion.
2) samsung-laptop.ko gets loaded, calls acpi_video_dmi_promote_vendor(),
   calls acpi_video_unregister(), which is a nop since acpi_video_register
   has not yet been called
2) intel.ko gets loaded, calls acpi_video_register() which registers
   the listener, but does not register the acpi backlight interface due to
   the call to the preciding call to acpi_video_dmi_promote_vendor()

*) acpi/video.ko always loads first as both other modules depend on it.

So we end up with or without an acpi video bus event listener depending
on module load ordering, not good.

Switching to using acpi_video_unregister_backlight() means that independ
of ordering we will always have an acpi video bus event listener fixing
this.

Note that this commit means that systems without an intel video opregion,
and systems which were hitting scenario a wrt module load ordering, are
now getting an acpi video bus event listener while before they were not!

On some systems this may cause the brightness hotkeys to start generating
keypresses while before they were not (good), while on other systems this
may cause the brightness hotkeys to generate multiple keypress events for
a single press (not so good). Since on most systems the acpi video bus is
the canonical source for brightness events I believe that the latter case
will needs to be handled on a case by case basis by filtering out the
duplicate keypresses at the other source for them.

Cc: Corentin Chary <corentin.chary@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/samsung-laptop.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/samsung-laptop.c b/drivers/platform/x86/samsung-laptop.c
index 9e701b2..0df03e2 100644
--- a/drivers/platform/x86/samsung-laptop.c
+++ b/drivers/platform/x86/samsung-laptop.c
@@ -1730,14 +1730,14 @@ static int __init samsung_init(void)
 		samsung->handle_backlight = false;
 	} else if (samsung->quirks->broken_acpi_video) {
 		pr_info("Disabling ACPI video driver\n");
-		acpi_video_unregister();
+		acpi_video_unregister_backlight();
 	}
 
 	if (samsung->quirks->use_native_backlight) {
 		pr_info("Using native backlight driver\n");
 		/* Tell acpi-video to not handle the backlight */
 		acpi_video_dmi_promote_vendor();
-		acpi_video_unregister();
+		acpi_video_unregister_backlight();
 		/* And also do not handle it ourselves */
 		samsung->handle_backlight = false;
 	}
-- 
2.4.2

Re: [PATCH 0/3] Use acpi_video_unregister_backlight instead of acpi_video_unregister in platfrom backlight drivers

[Darren Hart]

On Mon, Jun 01, 2015 at 11:25:05AM +0200, Hans de Goede wrote:
> Hi All,
> 
> I'm working on cleaning up the currently somewhat convoluted logic to
> select which backlight interfaces to register on x86 systems, see:
> http://lists.freedesktop.org/archives/dri-devel/2014-December/074687.html
> 
> For a rought outline (details will change in the actual patch-set).
> 
> These 3 patches are a preparation for that work, as the behavior of the
> current code is not always consistent (it changes depending on module
> loading order in some cases). These 3 patches remove this inconsistency
> which in some cases may result in a behavior change.
> 
> This way the cleanup can have a consistent base to build upon, and I can
> ensure that the cleanup itself does not cause any functional changes.
> 
> I hope to post a v1 of the actual cleanup patch-set in 1-2 weeks.

That will likely mean we miss the 4.2 merge window and will target this for 4.3
(which is fine by me, but just so you know).

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 0/3] Use acpi_video_unregister_backlight instead of acpi_video_unregister in platfrom backlight drivers

[Hans de Goede]

Hi Darren,

On 01-06-15 19:41, Darren Hart wrote:
> On Mon, Jun 01, 2015 at 11:25:05AM +0200, Hans de Goede wrote:
>> Hi All,
>>
>> I'm working on cleaning up the currently somewhat convoluted logic to
>> select which backlight interfaces to register on x86 systems, see:
>> http://lists.freedesktop.org/archives/dri-devel/2014-December/074687.html
>>
>> For a rought outline (details will change in the actual patch-set).
>>
>> These 3 patches are a preparation for that work, as the behavior of the
>> current code is not always consistent (it changes depending on module
>> loading order in some cases). These 3 patches remove this inconsistency
>> which in some cases may result in a behavior change.
>>
>> This way the cleanup can have a consistent base to build upon, and I can
>> ensure that the cleanup itself does not cause any functional changes.
>>
>> I hope to post a v1 of the actual cleanup patch-set in 1-2 weeks.
>
> That will likely mean we miss the 4.2 merge window and will target this for 4.3
> (which is fine by me, but just so you know).

IMHO this patches are worthwhile to have by themselves, as said
they fix various inconsistencies, which is a good thing to fix
regardless.

I think that these going into 4.2, and the cleanup which depends on them
(but is other then that pretty much an orthogonal patch set) going into
4.3 is actually a good thing, because if people then experience any
problems due to my work on this, we can easily see if it is due to
the behavior change these 3 patches may introduce in some cases, or
if it is due to the actual cleanup causing a behavior change (which it
should not).

Regards,

Hans

Re: [PATCH 0/3] Use acpi_video_unregister_backlight instead of acpi_video_unregister in platfrom backlight drivers

[Jani Nikula]

On Mon, 01 Jun 2015, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi All,
>
> I'm working on cleaning up the currently somewhat convoluted logic to
> select which backlight interfaces to register on x86 systems, see:
> http://lists.freedesktop.org/archives/dri-devel/2014-December/074687.html
>
> For a rought outline (details will change in the actual patch-set).
>
> These 3 patches are a preparation for that work, as the behavior of the
> current code is not always consistent (it changes depending on module
> loading order in some cases). These 3 patches remove this inconsistency
> which in some cases may result in a behavior change.
>
> This way the cleanup can have a consistent base to build upon, and I can
> ensure that the cleanup itself does not cause any functional changes.

Thanks for doing this.

Slightly unrelated, but you'll end up calling:

acpi_video_unregister_backlight
	acpi_video_bus_unregister_backlight
		acpi_video_dev_unregister_backlight
			thermal_cooling_device_unregister

Uhm, err, what? What business does *that* have in the backlight
unregister call chain?! Should that be untangled from the mess too?

BR,
Jani.


>
> I hope to post a v1 of the actual cleanup patch-set in 1-2 weeks.
>
> Regards,
>
> Hans
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 0/3] Use acpi_video_unregister_backlight instead of acpi_video_unregister in platfrom backlight drivers

[Hans de Goede]

Hi,

On 02-06-15 11:59, Jani Nikula wrote:
> On Mon, 01 Jun 2015, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi All,
>>
>> I'm working on cleaning up the currently somewhat convoluted logic to
>> select which backlight interfaces to register on x86 systems, see:
>> http://lists.freedesktop.org/archives/dri-devel/2014-December/074687.html
>>
>> For a rought outline (details will change in the actual patch-set).
>>
>> These 3 patches are a preparation for that work, as the behavior of the
>> current code is not always consistent (it changes depending on module
>> loading order in some cases). These 3 patches remove this inconsistency
>> which in some cases may result in a behavior change.
>>
>> This way the cleanup can have a consistent base to build upon, and I can
>> ensure that the cleanup itself does not cause any functional changes.
>
> Thanks for doing this.
>
> Slightly unrelated, but you'll end up calling:
>
> acpi_video_unregister_backlight
> 	acpi_video_bus_unregister_backlight
> 		acpi_video_dev_unregister_backlight
> 			thermal_cooling_device_unregister
>
> Uhm, err, what? What business does *that* have in the backlight
> unregister call chain?!

I think the idea is that having the backlight on can cause the main board
to heat up as there is often a regulator / pwm for the backlight on the
main board. If that is turned off it may help to cool down the main board,
so the backlight is a cooling device in the sense that when it is turned
off it stops generating heat.

If this makes sense (*) then the cooling device stuff should maybe moved
from the acpi/video.c code to the backlight core code.

Or does the acpi-video code use some info from the BIOS to determine
whether or not to register a cooling device ?

Regards,

Hans




*) This is tricky, e.g. going low brightness may cause the main board
to heat up more then full brightness depending on how things are hooked
up.



  Should that be untangled from the mess too?
>
> BR,
> Jani.
>
>
>>
>> I hope to post a v1 of the actual cleanup patch-set in 1-2 weeks.
>>
>> Regards,
>>
>> Hans
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Re: [PATCH 0/3] Use acpi_video_unregister_backlight instead of acpi_video_unregister in platfrom backlight drivers

[Aaron Lu]

On Tue, Jun 02, 2015 at 12:59:10PM +0300, Jani Nikula wrote:
> On Mon, 01 Jun 2015, Hans de Goede <hdegoede@redhat.com> wrote:
> > Hi All,
> >
> > I'm working on cleaning up the currently somewhat convoluted logic to
> > select which backlight interfaces to register on x86 systems, see:
> > http://lists.freedesktop.org/archives/dri-devel/2014-December/074687.html
> >
> > For a rought outline (details will change in the actual patch-set).
> >
> > These 3 patches are a preparation for that work, as the behavior of the
> > current code is not always consistent (it changes depending on module
> > loading order in some cases). These 3 patches remove this inconsistency
> > which in some cases may result in a behavior change.
> >
> > This way the cleanup can have a consistent base to build upon, and I can
> > ensure that the cleanup itself does not cause any functional changes.
> 
> Thanks for doing this.
> 
> Slightly unrelated, but you'll end up calling:
> 
> acpi_video_unregister_backlight
> 	acpi_video_bus_unregister_backlight
> 		acpi_video_dev_unregister_backlight
> 			thermal_cooling_device_unregister
> 
> Uhm, err, what? What business does *that* have in the backlight
> unregister call chain?! Should that be untangled from the mess too?

I think it should be OK.
We currently will create a thermal cooling device for the ACPI backlight
device, its intention is that we can achieve some thermal purpose by
increasing or decreasing the backlight level(using the same set of
methods as the ACPI backlight one, though I'm not sure if it is being
used by anyone). When we are going to unregister the ACPI video
backlight interface, it most likely means that the interface doesn't
work so it doesn't make much sense to keep that thermal interface and
hence the thermal_cooling_device_unregister ends up being called here.

Regards,
Aaron

> 
> BR,
> Jani.
> 
> 
> >
> > I hope to post a v1 of the actual cleanup patch-set in 1-2 weeks.
> >
> > Regards,
> >
> > Hans
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 0/3] Use acpi_video_unregister_backlight instead of acpi_video_unregister in platfrom backlight drivers

[Aaron Lu]

On Tue, Jun 02, 2015 at 12:14:28PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 02-06-15 11:59, Jani Nikula wrote:
> >On Mon, 01 Jun 2015, Hans de Goede <hdegoede@redhat.com> wrote:
> >>Hi All,
> >>
> >>I'm working on cleaning up the currently somewhat convoluted logic to
> >>select which backlight interfaces to register on x86 systems, see:
> >>http://lists.freedesktop.org/archives/dri-devel/2014-December/074687.html
> >>
> >>For a rought outline (details will change in the actual patch-set).
> >>
> >>These 3 patches are a preparation for that work, as the behavior of the
> >>current code is not always consistent (it changes depending on module
> >>loading order in some cases). These 3 patches remove this inconsistency
> >>which in some cases may result in a behavior change.
> >>
> >>This way the cleanup can have a consistent base to build upon, and I can
> >>ensure that the cleanup itself does not cause any functional changes.
> >
> >Thanks for doing this.
> >
> >Slightly unrelated, but you'll end up calling:
> >
> >acpi_video_unregister_backlight
> >	acpi_video_bus_unregister_backlight
> >		acpi_video_dev_unregister_backlight
> >			thermal_cooling_device_unregister
> >
> >Uhm, err, what? What business does *that* have in the backlight
> >unregister call chain?!
> 
> I think the idea is that having the backlight on can cause the main board
> to heat up as there is often a regulator / pwm for the backlight on the
> main board. If that is turned off it may help to cool down the main board,
> so the backlight is a cooling device in the sense that when it is turned
> off it stops generating heat.

I think so.

> 
> If this makes sense (*) then the cooling device stuff should maybe moved
> from the acpi/video.c code to the backlight core code.

It's just a specific driver that provides thermal capability, no need to
move to the core code.

> 
> Or does the acpi-video code use some info from the BIOS to determine
> whether or not to register a cooling device ?

As long as we create the ACPI video backlight interface, we will create
the thermal cooling device. If we somehow knows that the ACPI methods to
adjust the backlight level doesn't work(a good hint is that the
acpi_video_unregister_backlight is called), we should make that cooling
device disappear.

Regards,
Aaron

> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> *) This is tricky, e.g. going low brightness may cause the main board
> to heat up more then full brightness depending on how things are hooked
> up.
> 
> 
> 
>  Should that be untangled from the mess too?
> >
> >BR,
> >Jani.
> >
> >
> >>
> >>I hope to post a v1 of the actual cleanup patch-set in 1-2 weeks.
> >>
> >>Regards,
> >>
> >>Hans
> >>_______________________________________________
> >>dri-devel mailing list
> >>dri-devel@lists.freedesktop.org
> >>http://lists.freedesktop.org/mailman/listinfo/dri-devel
> >

Re: [PATCH 0/3] Use acpi_video_unregister_backlight instead of acpi_video_unregister in platfrom backlight drivers

[Aaron Lu]

On Tue, Jun 02, 2015 at 10:33:50PM +0800, Aaron Lu wrote:
> On Tue, Jun 02, 2015 at 12:14:28PM +0200, Hans de Goede wrote:
> > 
> > If this makes sense (*) then the cooling device stuff should maybe moved
> > from the acpi/video.c code to the backlight core code.
> 
> It's just a specific driver that provides thermal capability, no need to
> move to the core code.

I didn't get this when replying and now I think it's doable.

Regards,
Aaron

Re: [PATCH 0/3] Use acpi_video_unregister_backlight instead of acpi_video_unregister in platfrom backlight drivers

[Darren Hart]

On Mon, Jun 01, 2015 at 11:25:05AM +0200, Hans de Goede wrote:
> Hi All,
> 
> I'm working on cleaning up the currently somewhat convoluted logic to
> select which backlight interfaces to register on x86 systems, see:
> http://lists.freedesktop.org/archives/dri-devel/2014-December/074687.html
> 
> For a rought outline (details will change in the actual patch-set).
> 
> These 3 patches are a preparation for that work, as the behavior of the
> current code is not always consistent (it changes depending on module
> loading order in some cases). These 3 patches remove this inconsistency
> which in some cases may result in a behavior change.
> 
> This way the cleanup can have a consistent base to build upon, and I can
> ensure that the cleanup itself does not cause any functional changes.
> 
> I hope to post a v1 of the actual cleanup patch-set in 1-2 weeks.

Letting these sit in my testing branch while awaiting at ack from Corentin on 2
of the 3.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 2/3] asus-wmi: Use acpi_video_unregister_backlight instead of acpi_video_unregister

[Corentin Chary]

On Mon, Jun 1, 2015 at 10:25 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> acpi_video_unregister() not only unregisters the acpi-video backlight
> interface but also unregisters the acpi video bus event listener, causing
> e.g. brightness hotkey presses to no longer generate keypress events.
>
> The unregistering of the acpi video bus event listener usually is
> undesirable, which by itself is a good reason to switch to
> acpi_video_unregister_backlight().
>
> Another problem with using acpi_video_unregister() rather then using
> acpi_video_unregister_backlight() is that on systems with an intel video
> opregion (most systems) and a wmi_backlight_power quirk, whether or not
> the acpi video bus event listener actually gets unregistered depends on
> module load ordering:
>
> Scenario a:
> 1) acpi/video.ko gets loaded (*), does not do acpi_video_register as there
>    is an intel opregion.
> 2) intel.ko gets loaded, calls acpi_video_register() which registers both
>    the listener and the acpi backlight interface
> 3) asus-wmi.ko gets loaded, calls acpi_video_unregister() causing both
>    the listener and the acpi backlight interface to unregister
>
> Scenario b:
> 1) acpi/video.ko gets loaded (*), does not do acpi_video_register as there
>    is an intel opregion.
> 2) asus-wmi.ko gets loaded, calls acpi_video_dmi_promote_vendor(),
>    calls acpi_video_unregister(), which is a nop since acpi_video_register
>    has not yet been called
> 2) intel.ko gets loaded, calls acpi_video_register() which registers
>    the listener, but does not register the acpi backlight interface due to
>    the call to the preciding call to acpi_video_dmi_promote_vendor()
>
> *) acpi/video.ko always loads first as both other modules depend on it.
>
> So we end up with or without an acpi video bus event listener depending
> on module load ordering, not good.
>
> Switching to using acpi_video_unregister_backlight() means that independ
> of ordering we will always have an acpi video bus event listener fixing
> this.
>
> Note that this commit means that systems without an intel video opregion,
> and systems which were hitting scenario a wrt module load ordering, are
> now getting an acpi video bus event listener while before they were not!
>
> On some systems this may cause the brightness hotkeys to start generating
> keypresses while before they were not (good), while on other systems this
> may cause the brightness hotkeys to generate multiple keypress events for
> a single press (not so good). Since on most systems the acpi video bus is
> the canonical source for brightness events I believe that the latter case
> will needs to be handled on a case by case basis by filtering out the
> duplicate keypresses at the other source for them.
>
> Cc: Corentin Chary <corentin.chary@gmail.com>
> Cc: acpi4asus-user@lists.sourceforge.net
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/platform/x86/asus-wmi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 7543a56..945145d 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -1777,7 +1777,7 @@ static int asus_wmi_add(struct platform_device *pdev)
>                 acpi_video_dmi_promote_vendor();
>         if (!acpi_video_backlight_support()) {
>                 pr_info("Disabling ACPI video driver\n");
> -               acpi_video_unregister();
> +               acpi_video_unregister_backlight();
>                 err = asus_wmi_backlight_init(asus);
>                 if (err && err != -ENODEV)
>                         goto fail_backlight;
> --
> 2.4.2
>

Acked-by: Corentin Chary <corentin.chary@gmail.com)

-- 
Corentin Chary
http://xf.iksaif.net

Re: [PATCH 3/3] samsung-laptop: Use acpi_video_unregister_backlight instead of acpi_video_unregister

[Corentin Chary]

On Mon, Jun 1, 2015 at 10:25 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> acpi_video_unregister() not only unregisters the acpi-video backlight
> interface but also unregisters the acpi video bus event listener, causing
> e.g. brightness hotkey presses to no longer generate keypress events.
>
> The unregistering of the acpi video bus event listener usually is
> undesirable, which by itself is a good reason to switch to
> acpi_video_unregister_backlight().
>
> Another problem with using acpi_video_unregister() rather then using
> acpi_video_unregister_backlight() is that on systems with an intel video
> opregion (most systems) and a broken_acpi_video quirk, whether or not
> the acpi video bus event listener actually gets unregistered depends on
> module load ordering:
>
> Scenario a:
> 1) acpi/video.ko gets loaded (*), does not do acpi_video_register as there
>    is an intel opregion.
> 2) intel.ko gets loaded, calls acpi_video_register() which registers both
>    the listener and the acpi backlight interface
> 3) samsung-laptop.ko gets loaded, calls acpi_video_unregister() causing
>    both the listener and the acpi backlight interface to unregister
>
> Scenario b:
> 1) acpi/video.ko gets loaded (*), does not do acpi_video_register as there
>    is an intel opregion.
> 2) samsung-laptop.ko gets loaded, calls acpi_video_dmi_promote_vendor(),
>    calls acpi_video_unregister(), which is a nop since acpi_video_register
>    has not yet been called
> 2) intel.ko gets loaded, calls acpi_video_register() which registers
>    the listener, but does not register the acpi backlight interface due to
>    the call to the preciding call to acpi_video_dmi_promote_vendor()
>
> *) acpi/video.ko always loads first as both other modules depend on it.
>
> So we end up with or without an acpi video bus event listener depending
> on module load ordering, not good.
>
> Switching to using acpi_video_unregister_backlight() means that independ
> of ordering we will always have an acpi video bus event listener fixing
> this.
>
> Note that this commit means that systems without an intel video opregion,
> and systems which were hitting scenario a wrt module load ordering, are
> now getting an acpi video bus event listener while before they were not!
>
> On some systems this may cause the brightness hotkeys to start generating
> keypresses while before they were not (good), while on other systems this
> may cause the brightness hotkeys to generate multiple keypress events for
> a single press (not so good). Since on most systems the acpi video bus is
> the canonical source for brightness events I believe that the latter case
> will needs to be handled on a case by case basis by filtering out the
> duplicate keypresses at the other source for them.
>
> Cc: Corentin Chary <corentin.chary@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/platform/x86/samsung-laptop.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/samsung-laptop.c b/drivers/platform/x86/samsung-laptop.c
> index 9e701b2..0df03e2 100644
> --- a/drivers/platform/x86/samsung-laptop.c
> +++ b/drivers/platform/x86/samsung-laptop.c
> @@ -1730,14 +1730,14 @@ static int __init samsung_init(void)
>                 samsung->handle_backlight = false;
>         } else if (samsung->quirks->broken_acpi_video) {
>                 pr_info("Disabling ACPI video driver\n");
> -               acpi_video_unregister();
> +               acpi_video_unregister_backlight();
>         }
>
>         if (samsung->quirks->use_native_backlight) {
>                 pr_info("Using native backlight driver\n");
>                 /* Tell acpi-video to not handle the backlight */
>                 acpi_video_dmi_promote_vendor();
> -               acpi_video_unregister();
> +               acpi_video_unregister_backlight();
>                 /* And also do not handle it ourselves */
>                 samsung->handle_backlight = false;
>         }
> --
> 2.4.2
>

Acked-by: Corentin Chary <corentin.chary@gmail.com)

-- 
Corentin Chary
http://xf.iksaif.net

Re: [PATCH 0/3] Use acpi_video_unregister_backlight instead of acpi_video_unregister in platfrom backlight drivers

[Darren Hart]

On Mon, Jun 01, 2015 at 11:25:05AM +0200, Hans de Goede wrote:
> Hi All,
> 
> I'm working on cleaning up the currently somewhat convoluted logic to
> select which backlight interfaces to register on x86 systems, see:
> http://lists.freedesktop.org/archives/dri-devel/2014-December/074687.html
> 
> For a rought outline (details will change in the actual patch-set).
> 
> These 3 patches are a preparation for that work, as the behavior of the
> current code is not always consistent (it changes depending on module
> loading order in some cases). These 3 patches remove this inconsistency
> which in some cases may result in a behavior change.
> 
> This way the cleanup can have a consistent base to build upon, and I can
> ensure that the cleanup itself does not cause any functional changes.

These are now available in for-next.

-- 
Darren Hart
Intel Open Source Technology Center