Re: [PATCH v5 02/31] drm/i915: Don't register backlight when another backlight should be used (v2)

[Matthew Garrett]

On Thu, Oct 27, 2022 at 11:39:38AM +0200, Hans de Goede wrote:

> The *only* behavior which actually is new in 6.1 is the native GPU
> drivers now doing the equivalent of:
> 
>   if (acpi_video_get_backlight_type() != acpi_backlight_native)
>   return;
> 
> In their backlight register paths (i), which is causing the native
> backlight to disappear on your custom laptop setup and on Chromebooks
> (with the Chromebooks case being already solved I hope.).

It's causing the backlight control to vanish on any machine that isn't 
((acpi_video || vendor interface) || !acpi). Most machines that fall 
into that are either weird or Chromebooks or old, but there are machines 
that fall into that.

(I wrote https://mjg59.livejournal.com/127103.html over 12 years ago, so 
please do assume that I'm familiar with the complexities here :) )

> I agree this is a possible solution if this turns out to break more
> systems and there is no other easy/clean way to fix those. But I would
> greatly prefer to keep this change and stop the IMHO bad kernel behavior
> of "registering multiple backlight-devices for a single panel and then
> let userspace sort it out".

If we're not able to make a correct policy decision in the kernel then 
punting it to userland seems like the right thing to do? The kernel 
absolutely *should* make the right decision where it has enough 
information to do so, but in this case the code that's making that 
decision doesn't have the full set of information.

Re: [PATCH v5 02/31] drm/i915: Don't register backlight when another backlight should be used (v2)

[Matthew Garrett]

On Thu, Oct 27, 2022 at 10:51:45AM +0200, Hans de Goede wrote:

> In their backlight register paths and this has been present since
> circa 2015.
> 
> So both before and after my 6.1 refactor vendor is only preferred
> on devices which don't implement the ACPI video bus control method.

Sorry, yes, that's the case I meant.

> Just because a vendor interface is present does not mean that it will
> work. Unfortunately for none of the 3 main native/acpi_video/vendor
> backlight control methods the control method being present also guarantees
> that it will work. Which completely sucks, but it is the reality we
> have to deal with.

But traditionally that's been logic that we've encoded into the vendor 
drivers, which can take other factors into account when determining 
whether the exposed interface works. You've now discarded that 
knowledge. The only way you can maintain the degree of functionality 
that 6.0 had is to move that determination into core code, or 
alternatively support dynamic reattachment of backlight interfaces based 
on vendor drivers loading later. An alternative would be to just revert 
all of this, and instead only use this logic for the output property 
interface (which would still result in different outcomes, but only for 
userland that's choosing to use the new interface, so that's a different 
problem).

Re: [PATCH v5 02/31] drm/i915: Don't register backlight when another backlight should be used (v2)

[Matthew Garrett]

On Wed, Oct 26, 2022 at 11:59:28AM +0200, Hans de Goede wrote:

> Ok, so this is a local customization to what is already a custom BIOS
> for a custom motherboard. There is a lot of custom in that sentence and
> TBH at some point things might become too custom for them to be expected
> to work OOTB.

But it *did* work OOTB before. You broke it. I accept that I'm a 
ludicrously weird corner case here, but there are going to be other 
systems that are also affected by this.

> I'm afraid things are not that simple. I assume that with
> "if ACPI backlight control is expected to work" you mean don't
> use ACPI backlight control when (acpi_osi_is_win8() && native_available)
> evaluates to true because it is known to be broken on some of
> those systems because Windows 8 stopped using it ?

Correct.

> Unfortunately something similar applies to vendor interfaces,
> When Windows XP started using (and mandating for certification
> IIRC) ACPI backlight control, vendors still kept their own
> vendor specific EC/smbios/ACPI/WMI backlight interfaces around for
> a long long time, except they were often no longer tested.

The current situation (both before your patchset and with its current 
implementation) is that vendor is preferred to native, so if the vendor 
interface is present then we're already using it.

> > The 
> > problem you're dealing with is that the knowledge of whether or not 
> > there's a vendor interface isn't something the core kernel code knows 
> > about. What you're proposing here is effectively for us to expose 
> > additional information about whether or not there's a vendor interface 
> > in the system firmware, but since we're talking in some cases about 
> > hardware that's almost 20 years old, we're not realistically going to 
> > get those old machines fixed.
> 
> I don't understand why you keep talking about the old vendor interfaces,
> at least for the chromebook part of this thread the issue is that
> the i915 driver no longer registers the intel_backlight device which
> is a native device type, which is caused by the patch this email
> thread is about (and old vendor interfaces do not come into play
> at all here). So AFAICT this is a native vs acpi backlight control
> issue ?

I'm referring to your proposed patch that changed the default from 
backlight_vendor to backlight_native, which would fix my machine and 
Chromebooks but break anything that relies on the vendor interfaces.

> I really want to resolve your bug, but I still lack a lot of info,
> like what backlight interface you were actually using in 6.0 ?

Native.

> {
>  .callback = video_detect_force_video,
>  /* ThinkPad X201s */
>  .matches = {
> DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad X201s"),
> },
> },
> 
> will trigger.

In this case you'd break anyone else running the system who isn't using 
the hacked EC and different ACPI tables - obviously there's ways round 
this, but realistically since I'm (as far as I know) the only person in 
this situation it makes more sense for me to add a kernel parameter than 
carry around an exceedingly niche DMI quirk. I'm fine with that. But the 
point I'm trying to make is that the machines *are* telling you whether 
they'd prefer vendor or native, and you're not taking that into account 
in the video_detect code.

Re: [PATCH v5 02/31] drm/i915: Don't register backlight when another backlight should be used (v2)

[Matthew Garrett]

On Wed, Oct 26, 2022 at 01:27:25AM +0200, Hans de Goede wrote:

> this code should actually set the ACPI_VIDEO_BACKLIGHT flag:
> drivers/acpi/scan.c:
> 
> static acpi_status
> acpi_backlight_cap_match(acpi_handle handle, u32 level, void *context,
>   void **return_value)
> {
> long *cap = context;
> 
> if (acpi_has_method(handle, "_BCM") &&
> acpi_has_method(handle, "_BCL")) {
> acpi_handle_debug(handle, "Found generic backlight 
> support\n");
> *cap |= ACPI_VIDEO_BACKLIGHT;
> /* We have backlight support, no need to scan further */
> return AE_CTRL_TERMINATE;
> }
> return 0;
> }

Ah, yeah, my local tree no longer matches the upstream behaviour because 
I've hacked the EC firmware to remove the backlight trigger because it 
had an extremely poor brightness curve and also automatically changed it 
on AC events - as a result I removed the backlight code from the DSDT 
and just fell back to the native control. Like I said I'm a long way 
from the normal setup, but this did previously work.

The "right" logic here seems pretty simple: if ACPI backlight control is 
expected to work, use it. If it isn't, but there's a vendor interface, 
use it. If there's no vendor interface, use the native interface. The 
problem you're dealing with is that the knowledge of whether or not 
there's a vendor interface isn't something the core kernel code knows 
about. What you're proposing here is effectively for us to expose 
additional information about whether or not there's a vendor interface 
in the system firmware, but since we're talking in some cases about 
hardware that's almost 20 years old, we're not realistically going to 
get those old machines fixed. So, it feels like there's two choices:

1) Make a default policy decision, but then allow that decision to be 
altered later on (eg, when a vendor-specific platform driver has been 
loaded) - you've said this poses additional complexities.

2) Move the knowledge of whether or not there's a vendor interface into 
the core code. Basically take every platform driver that exposes a 
vendor interface, and move the detection code into the core.

I think any other approach is going to result in machines that 
previously worked no longer working (and you can't just make the 
vendor/native split dependent on the Coreboot DMI BIOS string, because 
there are some Coreboot platforms that implement the vendor interface 
for compatibility, and you also can't ask all Coreboot users to update 
their firmware to fix things)

Re: [PATCH v5 02/31] drm/i915: Don't register backlight when another backlight should be used (v2)

[Matthew Garrett]

On Tue, Oct 25, 2022 at 10:25:33PM +0200, Hans de Goede wrote:

> Having the native driver come and then go and be replaced
> with the vendor driver would also be quite inconvenient
> for these planned changes.

I understand that it would be inconvenient, but you've broken existing 
working setups.
 
> Can you perhaps explain a bit in what way your laptop
> is weird ?

It's a Chinese replacement motherboard for a Thinkpad X201, running my 
own port of Coreboot. Its DMI strings look like an actual Thinkpad in 
order to ensure that thinkpad_acpi can bind for hotkey suport, so it's 
hard to quirk. It'll actually be fixed by your proposed patch to fall 
back to native rather than vendor, but that patch will break any older 
machines that offer a vendor interface and don't have the native control 
hooked up (pretty sure at least the Thinkpad X40 falls into that 
category).

Re: [PATCH v5 02/31] drm/i915: Don't register backlight when another backlight should be used (v2)

[Matthew Garrett]

On Tue, Oct 25, 2022 at 08:50:54PM +0200, Hans de Goede wrote:

> That is a valid point, but keep in mind that this is only used on ACPI
> platforms and then only on devices with a builtin LCD panel and then
> only by GPU drivers which actually call acpi_video_get_backlight_type(),
> so e.g. not by all the ARM specific display drivers.
> 
> So I believe that Chromebooks quite likely are the only devices with
> this issue.

My laptop is, uh, weird, but it falls into this category.
 
> > I think for this to work correctly you need to have 
> > the infrastructure be aware of whether or not a vendor interface exists, 
> > which means having to handle cleanup if a vendor-specific module gets 
> > loaded later.
> 
> Getting rid of the whole ping-ponging of which backlight drivers
> get loaded during boot was actually one of the goals of the rework
> which landed in 6.1 this actually allowed us to remove some quirks
> because some hw/firmware did not like us changing our mind and
> switching backlight interfaces after first poking another one.
> So we definitely don't want to go back to the ping-pong thing.

Defaulting to native but then having a vendor driver be able to disable 
native drivers seems easiest? It shouldn't be a regression over the 
previous state of affairs since both drivers were being loaded already.

Re: [PATCH v5 02/31] drm/i915: Don't register backlight when another backlight should be used (v2)

[Matthew Garrett]

On Tue, Sep 27, 2022 at 01:04:52PM +0200, Hans de Goede wrote:

> So to fix this we need to make acpi_video_get_backlight_type()
> return native on the Acer Chromebook Spin 713.

Isn't the issue broader than that? Unless the platform is Windows 8 or 
later, we'll *always* (outside of some corner cases) return 
acpi_backlight_vendor if there's no ACPI video interface. This is broken 
for any platform that implements ACPI but relies on native video 
control, which is going to include a range of Coreboot platforms, not 
just Chromebooks. I think for this to work correctly you need to have 
the infrastructure be aware of whether or not a vendor interface exists, 
which means having to handle cleanup if a vendor-specific module gets 
loaded later.

Re: [PATCH 06/14] drm/cirrus: Use 32bpp by default

[Matthew Garrett]

On Wed, Aug 23, 2017 at 09:10:09PM +0530, Varad Gautam wrote:
> Hi Matthew,
> 
> On Sat, Aug 19, 2017 at 2:02 PM, Matthew Garrett  wrote:
> > On Fri, Aug 18, 2017 at 09:19:11PM +0530, Varad Gautam wrote:
> >> From: Stéphane Marchesin 
> >>
> >> initially reviewed for ChromiumOS at:
> >> https://chromium-review.googlesource.com/339093
> >> Signed-off-by: Stéphane Marchesin 
> >
> > 1280x1024x24 fits in 4MB, 1280x1024x32 doesn't. That seems like it's
> > going to be a visible change in behaviour.
> >
> 
> Right, 800x600 is the highest we can go for >24bpp, so we now switch
> to that instead of 1280x1024. fb creation fails in
> cirrus_check_framebuffer if the w*h*bpp doesn't fit, and plane updates
> get rejected when atomic is enabled later on.

So users who want 1280x1024 now have to change their configuration. Is 
that the intent? It seems odd to change user-visible behaviour for 
everyone just to fix ChromeOS in a VM.

-- 
Matthew Garrett | mj...@srcf.ucam.org
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 06/14] drm/cirrus: Use 32bpp by default

[Matthew Garrett]

On Fri, Aug 18, 2017 at 09:19:11PM +0530, Varad Gautam wrote:
> From: Stéphane Marchesin 
> 
> initially reviewed for ChromiumOS at:
> https://chromium-review.googlesource.com/339093
> Signed-off-by: Stéphane Marchesin 

1280x1024x24 fits in 4MB, 1280x1024x32 doesn't. That seems like it's 
going to be a visible change in behaviour.

-- 
Matthew Garrett | mj...@srcf.ucam.org
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

git pull] drm for v4.1-rc1

[Matthew Garrett]

On Thu, Apr 23, 2015 at 3:47 PM, Linus Torvalds
 wrote:

> I'm not sure why we want that IORESOURCE_ROM_SHADOW thing at all, but
> yes, if what this is all about is the magic video ROM at 0xc, then

It's used in the PCI layer to say "Read from 0xc rather than the
ROM BAR" and then ends up as a shorthand for "Was this the boot video
device" in various places because we're bad at software.

> There is no way to see that from the PCI device state, because as
> mentioned, quite often the "ROM" is entirely fake, and is not just
> some shadowed copy of a real underlying hardware ROM, but is
> fundamentally just a RAM image decompressed from some other source and
> then marked read-only.

If only - nvidias used to rewrite their image at runtime.

-- 
Matthew Garrett | matthew.garrett at coreos.com

git pull] drm for v4.1-rc1

[Matthew Garrett]

On Thu, Apr 23, 2015 at 2:30 PM, Bjorn Helgaas  wrote:

> Your i915 does not have a ROM BAR in hardware.  If the default video
> device has no ROM BAR, pci_fixup_video() sets IORESOURCE_ROM_SHADOW
> even though the resource flags are zero because the BAR itself doesn't
> exist.
>
> If IORESOURCE_ROM_SHADOW is set, pci_map_rom() assumes there's a
> shadow ROM image at 0xC.  Is there a shadow image even if the
> device itself doesn't have a ROM BAR?

On UEFI systems, there's no special reason to believe that there's
anything at 0xc - it depends on whether a CSM got loaded or not.
Everything is awful. So...

>   int pcibios_add_device(struct pci_dev *dev)
>   {
> if (dev-is-default-vga-device) {
>   dev->rom = 0xC;
>   dev->romlen = 0x2;
> }

I don't know what we want to do here. This is, at some level,
fundamentally wrong - however, it also wouldn't surprise me if this is
also the only copy of the video ROM we have on some UEFI systems,
especially since I believe that Windows 7 still required that there be
a legacy ROM it could use for bootloader modesetting on UEFI
platforms. So simply making this conditional on BIOS may break
existing machines. But if there *is* a ROM there then we should be
able to id it from the usual video ROM signature?

[PATCH 00/11] Enable gpu switching on the MacBook Pro

[Matthew Garrett]

On Tue, Apr 21, 2015 at 11:58:20AM +0200, Lukas Wunner wrote:

> On MBPs, the panel is not connected to one of the gpus, but to the
> gmux chip (the "handler" in vga_switcheroo speak). Initially the gmux
> is switched to the discrete gpu. The integrated i915 gpu is therefore
> unable to detect the LVDS (or eDP) connector on bootup and switching
> to the integrated gpu will lead to a black screen. The solution is
> to temporarily switch the DDC lines to the integrated gpu when reading
> the EDID, which the gmux is capable of. Lack of support for this in
> vga_switcheroo is pretty much the only obstacle to get gpu switching
> to work on MBPs.

My testing suggested that changing the DDC lines didn't change auxch, so 
this approach doesn't work for eDP. Have you found otherwise?

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

[PATCH] radeon: acquire BIOS via firmware subsystem if everything else failed

[Matthew Garrett]

On Tue, Nov 25, 2014 at 12:14:21PM -0500, Alex Deucher wrote:
> The vbios image is available via ACPI prior to the OS taking over.
> IIRC, Matthew Garret fixed up the bootloader to fetch the vbios image
> prior to loading Linux so that it could be accessed after the OS
> loaded.

EFI rather than ACPI, but yeah. In theory this should work fine if 
you're using the EFI entry point. I don't know whether the patches for 
linuxefi were ever accepted by grub upstream - if not, pushing those 
would make more sense.

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

[PATCH RFC 4/4] drm: link connectors to backlight devices

[Matthew Garrett]

One extreme case - apple_gmux needs to be mapped to both the internal and 
discrete gpu. The same may be true for some other platform drivers on multi-gpu 
systems.

Matthew Garrett | matthew.garrett at nebula.com
-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20140911/66ba7771/attachment.html>

[PATCH RFC 0/4] Linking DRM Connectors to Backlight Devices

[Matthew Garrett]

On Wed, 2014-09-10 at 17:54 +0200, David Herrmann wrote:

>  * User-space currently has a hard-time figuring out which backlight device to
>use, and which backlight device belongs to which display. So far, most
>systems only provide backlight-devices for internal displays, so figuring 
> out
>the connection is easy, but that might change with more capable external
>connectors.

The parent device of the backlight will be the correct display, if the
kernel has a meaningful way to determine that. We could do a better job
in the ACPI code than we currently do, but (unfortunately) that requires
us to know the ACPI IDs that each GPU vendor uses.

>If multiple backlights are available, the easiest solution is to simply 
> write
>to all of them and hope at least one of them works. This obviously fails if
>the devices interact badly, so it's not really a solution.

There's a pretty well-defined process for that, although it sucks - if
it's an LVDS or eDP display and there's a backlight of type "firmware",
use it. If there's no "firmware" backlight, but there is a "platform"
one, use that. Otherwise look for a "native" backlight that has the
output as a parent. libbacklight does all of this for you. 

>  * User-space needs root privileges to write to sysfs. There are no char-devs
>that can be used to control access-management to the backlight API, so /sys
>is the only interface available. So far, udev policy has been "/sys is
>read-only for non-root" and it's not going to change. char-devs are *THE*
>standard way to provide non-root user-space APIs, so use them!

Yeah, this is a problem.

> This series tries to solve this problem with a much simpler approach:
> Instead of moving backlights into DRM, we simply link DRM properties to a
> backlight device. That is, the kernel manages a link between a connector and a
> backlight device (or n backlight devices) which can be modified by udev in 
> case
> the kernel got it wrong (we don't want huge board-fixup-tables in the kernel).
> User-space can now use the simpl DRM API to manage backlights, and the kernel
> does not need any special driver code to make it work.

This doesn't really simplify userspace significantly - something's still
going to have to make the same policy decision as we do right now, and
the kernel isn't really the right place to do that. It does have the
benefit of allowing that policy decision to be made at boot time and
then allow that to be consumed by all later userspace, so there is
*some* benefit, but I think the "make unprivileged userspace possible"
argument is much more compelling.

-- 
Matthew Garrett 

[PATCH 1/2 v2] x86, ia64: Move EFI_FB vga_default_device() initialization to pci_vga_fixup()

[Matthew Garrett]

On Wed, 2014-06-25 at 00:55 +0200, Bruno Pr?mont wrote:

> With introduction of sysfb/simplefb/simpledrm efifb is getting obsolete
> while having native drivers for the GPU also makes selecting
> sysfb/efifb optional.
> 

Both look good to me.

Acked-by: Matthew Garrett 

-- 
Matthew Garrett 

[PATCH 11/11] apple_gmux: Wait for switch completion

[Matthew Garrett]

The GMUX doesn't appear to switch instantly, which can trigger problems in
panel detection and setup. Wait for an interrupt or 200msec, whichever comes
first.

Signed-off-by: Matthew Garrett 
---
 drivers/platform/x86/apple-gmux.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/drivers/platform/x86/apple-gmux.c 
b/drivers/platform/x86/apple-gmux.c
index 17f906d..6826ede 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -38,6 +38,7 @@ struct apple_gmux_data {
int gpe;
enum vga_switcheroo_client_id resume_client_id;
enum vga_switcheroo_state power_state;
+   struct completion switch_done;
struct completion powerchange_done;
 };

@@ -283,6 +284,8 @@ static int gmux_switch_ddc(enum vga_switcheroo_client_id id)

 static int gmux_switchto(enum vga_switcheroo_client_id id)
 {
+   reinit_completion(&apple_gmux_data->switch_done);
+
if (id == VGA_SWITCHEROO_IGD) {
gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 1);
gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DISPLAY, 2);
@@ -293,6 +296,11 @@ static int gmux_switchto(enum vga_switcheroo_client_id id)
gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_EXTERNAL, 3);
}

+   if (apple_gmux_data->gpe >= 0 &&
+   
!wait_for_completion_interruptible_timeout(&apple_gmux_data->switch_done,
+  msecs_to_jiffies(200)))
+   pr_warn("Timeout waiting for gmux GPU switch to complete\n");
+
return 0;
 }

@@ -401,6 +409,9 @@ static void gmux_notify_handler(acpi_handle device, u32 
value, void *context)
gmux_clear_interrupts(gmux_data);
gmux_enable_interrupts(gmux_data);

+   if (status & GMUX_INTERRUPT_STATUS_DISPLAY)
+   complete(&gmux_data->switch_done);
+
if (status & GMUX_INTERRUPT_STATUS_POWER)
complete(&gmux_data->powerchange_done);
 }
@@ -565,6 +576,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct 
pnp_device_id *id)
}

apple_gmux_data = gmux_data;
+   init_completion(&gmux_data->switch_done);
init_completion(&gmux_data->powerchange_done);
gmux_enable_interrupts(gmux_data);

-- 
1.8.5.3

[PATCH 10/11] apple-gmux: Indicate that driver supports changing of GPU power states

[Matthew Garrett]

The Apple GMUX can cut power to the discrete GPU, so should declare this
to the vga_switcheroo core.

Signed-off-by: Matthew Garrett 
---
 drivers/platform/x86/apple-gmux.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/platform/x86/apple-gmux.c 
b/drivers/platform/x86/apple-gmux.c
index e9b6d77..17f906d 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -359,6 +359,7 @@ static struct vga_switcheroo_handler gmux_handler = {
.switch_ddc = gmux_switch_ddc,
.power_state = gmux_set_power_state,
.get_client_id = gmux_get_client_id,
+   .handler_pm = true,
 };

 static inline void gmux_disable_interrupts(struct apple_gmux_data *gmux_data)
-- 
1.8.5.3

[PATCH 09/11] apple-gmux: Assign apple_gmux_data before registering

[Matthew Garrett]

Registering the handler after both GPUs will trigger a DDC switch for
connector reprobing. This will oops if apple_gmux_data hasn't already been
assigned. Reorder the code to do that.

Signed-off-by: Matthew Garrett 
---
 drivers/platform/x86/apple-gmux.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/apple-gmux.c 
b/drivers/platform/x86/apple-gmux.c
index 5594cbc..e9b6d77 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -563,18 +563,20 @@ static int gmux_probe(struct pnp_dev *pnp, const struct 
pnp_device_id *id)
gmux_data->gpe = -1;
}

+   apple_gmux_data = gmux_data;
+   init_completion(&gmux_data->powerchange_done);
+   gmux_enable_interrupts(gmux_data);
+
if (vga_switcheroo_register_handler(&gmux_handler)) {
ret = -ENODEV;
goto err_register_handler;
}

-   init_completion(&gmux_data->powerchange_done);
-   apple_gmux_data = gmux_data;
-   gmux_enable_interrupts(gmux_data);
-
return 0;

 err_register_handler:
+   gmux_disable_interrupts(gmux_data);
+   apple_gmux_data = NULL;
if (gmux_data->gpe >= 0)
acpi_disable_gpe(NULL, gmux_data->gpe);
 err_enable_gpe:
-- 
1.8.5.3

[PATCH 08/11] apple-gmux: Add support for the switch_ddc callback

[Matthew Garrett]

We can switch DDC pins in a way that ought (with luck) to work for LVDS.
This isn't sufficient for eDP, which is addressed in later patches.

Signed-off-by: Matthew Garrett 
---
 drivers/platform/x86/apple-gmux.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/drivers/platform/x86/apple-gmux.c 
b/drivers/platform/x86/apple-gmux.c
index b9429fb..5594cbc 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -271,6 +271,16 @@ static const struct backlight_ops gmux_bl_ops = {
.update_status = gmux_update_status,
 };

+static int gmux_switch_ddc(enum vga_switcheroo_client_id id)
+{
+   if (id == VGA_SWITCHEROO_IGD)
+   gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 1);
+   else
+   gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 2);
+
+   return 0;
+}
+
 static int gmux_switchto(enum vga_switcheroo_client_id id)
 {
if (id == VGA_SWITCHEROO_IGD) {
@@ -346,6 +356,7 @@ gmux_active_client(struct apple_gmux_data *gmux_data)

 static struct vga_switcheroo_handler gmux_handler = {
.switchto = gmux_switchto,
+   .switch_ddc = gmux_switch_ddc,
.power_state = gmux_set_power_state,
.get_client_id = gmux_get_client_id,
 };
-- 
1.8.5.3

[PATCH 07/11] vga_switcheroo: Reprobe old device on switching

[Matthew Garrett]

We need to force the previously active device to reprobe its connectors
when we switch in order to allow it to give up connectors that are no
longer in use.

Signed-off-by: Matthew Garrett 
---
 drivers/gpu/vga/vga_switcheroo.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index 199a111..d380158 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -429,6 +429,9 @@ static int vga_switchto_stage2(struct vga_switcheroo_client 
*new_client)
if (new_client->ops->reprobe)
new_client->ops->reprobe(new_client->pdev);

+   if (active->ops->reprobe)
+   active->ops->reprobe(active->pdev);
+
if (active->pwr_state == VGA_SWITCHEROO_ON)
vga_switchoff(active);

-- 
1.8.5.3

[PATCH 06/11] vga_switcheroo: Add enable() call to clients and permit deferral of dynamic PM

[Matthew Garrett]

We may not know whether the platform supports dynamic PM of GPUs until the
switcheroo handler is registered. Add an enable() callback for clients and
another entry point to switcheroo in order to permit them to set dynamic PM
appropriately.

Signed-off-by: Matthew Garrett 
---
 drivers/gpu/vga/vga_switcheroo.c | 17 +
 include/linux/vga_switcheroo.h   |  3 +++
 2 files changed, 20 insertions(+)

diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index c5e97d4..199a111 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -108,6 +108,9 @@ static void vga_switcheroo_enable(void)

client->id = ret;

+   if (client->ops->enable)
+   client->ops->enable(client->pdev);
+
if (!client->active && client->ops->reprobe_connectors) {
int old_id = (client->id == VGA_SWITCHEROO_IGD) ?
VGA_SWITCHEROO_DIS : VGA_SWITCHEROO_IGD;
@@ -670,6 +673,20 @@ static void vga_switcheroo_power_switch(struct pci_dev 
*pdev, enum vga_switchero
vgasr_priv.handler->power_state(client->id, state);
 }

+void vga_switcheroo_set_dynamic_support(struct pci_dev *pdev, bool dynamic)
+{
+   struct vga_switcheroo_client *client;
+
+   client = find_client_from_pci(&vgasr_priv.clients, pdev);
+   if (!client)
+   return;
+
+   client->driver_power_control = dynamic;
+
+   return;
+}
+EXPORT_SYMBOL(vga_switcheroo_set_dynamic_support);
+
 /* force a PCI device to a certain state - mainly to turn off audio clients */

 void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum 
vga_switcheroo_state dynamic)
diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
index 4bba934..5a483c6 100644
--- a/include/linux/vga_switcheroo.h
+++ b/include/linux/vga_switcheroo.h
@@ -44,6 +44,7 @@ struct vga_switcheroo_client_ops {
void (*reprobe)(struct pci_dev *dev);
void (*reprobe_connectors)(struct pci_dev *dev);
bool (*can_switch)(struct pci_dev *dev);
+   void (*enable)(struct pci_dev *dev);
 };

 #if defined(CONFIG_VGA_SWITCHEROO)
@@ -67,6 +68,7 @@ int vga_switcheroo_process_delayed_switch(void);

 int vga_switcheroo_get_client_state(struct pci_dev *dev);

+void vga_switcheroo_set_dynamic_support(struct pci_dev *pdev, bool dynamic);
 void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum 
vga_switcheroo_state dynamic);

 int vga_switcheroo_init_domain_pm_ops(struct device *dev, struct dev_pm_domain 
*domain);
@@ -94,6 +96,7 @@ static inline void vga_switcheroo_unregister_handler(void) {}
 static inline int vga_switcheroo_process_delayed_switch(void) { return 0; }
 static inline int vga_switcheroo_get_client_state(struct pci_dev *dev) { 
return VGA_SWITCHEROO_ON; }

+static inline void vga_switcheroo_set_dynamic_support(struct pci_dev *pdev, 
bool dynamic) {}
 static inline void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, 
enum vga_switcheroo_state dynamic) {}

 static inline int vga_switcheroo_init_domain_pm_ops(struct device *dev, struct 
dev_pm_domain *domain) { return -EINVAL; }
-- 
1.8.5.3

[PATCH 05/11] vga_switcheroo: Allow handlers to indicate that they can handle PM

[Matthew Garrett]

Most switcheroo setups attach power management to one of the GPUs. This is
not always the case, so provide a mechanism for handlers to declare that
they can change the power state of GPUs and permit drivers to obtain this
information.

Signed-off-by: Matthew Garrett 
---
 drivers/gpu/vga/vga_switcheroo.c | 8 
 include/linux/vga_switcheroo.h   | 5 +
 2 files changed, 13 insertions(+)

diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index 1a80b93..c5e97d4 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -825,6 +825,14 @@ struct edid *vga_switcheroo_get_edid(struct pci_dev *pdev)
 }
 EXPORT_SYMBOL(vga_switcheroo_get_edid);

+bool vga_switcheroo_handler_pm(void) {
+   if (!vgasr_priv.handler)
+   return false;
+
+   return vgasr_priv.handler->handler_pm;
+}
+EXPORT_SYMBOL(vga_switcheroo_handler_pm);
+
 static int __init vga_switcheroo_setup(char *str)
 {
if (!strcmp(str, "IGD"))
diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
index 07b07fc..4bba934 100644
--- a/include/linux/vga_switcheroo.h
+++ b/include/linux/vga_switcheroo.h
@@ -36,6 +36,7 @@ struct vga_switcheroo_handler {
   enum vga_switcheroo_state state);
int (*init)(void);
int (*get_client_id)(struct pci_dev *pdev);
+   bool handler_pm;
 };

 struct vga_switcheroo_client_ops {
@@ -76,6 +77,8 @@ u8 *vga_switcheroo_get_dpcd(struct pci_dev *pdev);
 int vga_switcheroo_set_edid(struct edid *edid);
 struct edid *vga_switcheroo_get_edid(struct pci_dev *pdev);

+bool vga_switcheroo_handler_pm(void);
+
 #else

 static inline void vga_switcheroo_unregister_client(struct pci_dev *dev) {}
@@ -101,5 +104,7 @@ static inline u8 *vga_switcheroo_get_dpcd(struct pci_dev 
*pdev) { return NULL };
 static inline int vga_switcheroo_set_edid(struct edid *edid) { return 0 };
 static inline struct edid *vga_switcheroo_get_edid(struct pci_dev *pdev) { 
return NULL };

+static inline bool vga_switcheroo_handler_pm(void) { return false; };
+
 #endif
 #endif /* _LINUX_VGA_SWITCHEROO_H_ */
-- 
1.8.5.3

[PATCH 04/11] vga_switcheroo: Allow stashing of panel data

[Matthew Garrett]

Not all MUXes allow us to connect the panel data channel to a GPU without
handing over the entire panel and triggering additional flickering during
boot. We only need to do this in order to probe for data that the first
GPU driver has already identified, so add some functions for stashing that
data in vga_switcheroo where it can be retrieved by the other driver later.

Signed-off-by: Matthew Garrett 
---
 drivers/gpu/vga/vga_switcheroo.c | 59 
 include/linux/vga_switcheroo.h   | 12 
 2 files changed, 71 insertions(+)

diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index 6d95626..1a80b93 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -17,6 +17,8 @@
  - switch_check  - check if the device is in a position to switch now
  */

+#include 
+
 #include 
 #include 
 #include 
@@ -39,6 +41,7 @@ struct vga_switcheroo_client {
int id;
bool active;
bool driver_power_control;
+   bool use_panel;
struct list_head list;
 };

@@ -56,6 +59,9 @@ struct vgasr_priv {
int registered_clients;
struct list_head clients;

+   struct edid *edid;
+   u8 *dpcd;
+
struct vga_switcheroo_handler *handler;
 };

@@ -107,7 +113,9 @@ static void vga_switcheroo_enable(void)
VGA_SWITCHEROO_DIS : VGA_SWITCHEROO_IGD;
if (vgasr_priv.handler->switch_ddc)
vgasr_priv.handler->switch_ddc(client->id);
+   client->use_panel = true;
client->ops->reprobe_connectors(client->pdev);
+   client->use_panel = false;
if (vgasr_priv.handler->switch_ddc)
vgasr_priv.handler->switch_ddc(old_id);
}
@@ -412,6 +420,9 @@ static int vga_switchto_stage2(struct vga_switcheroo_client 
*new_client)
if (ret)
goto restore_ddc;

+   new_client->use_panel = true;
+   active->use_panel = false;
+
if (new_client->ops->reprobe)
new_client->ops->reprobe(new_client->pdev);

@@ -766,6 +777,54 @@ int 
vga_switcheroo_init_domain_pm_optimus_hdmi_audio(struct device *dev, struct
 }
 EXPORT_SYMBOL(vga_switcheroo_init_domain_pm_optimus_hdmi_audio);

+int vga_switcheroo_set_dpcd(u8 *dpcd)
+{
+   if (vgasr_priv.dpcd)
+   return -EEXIST;
+
+   vgasr_priv.dpcd = kmalloc(8, GFP_KERNEL);
+   memcpy(vgasr_priv.dpcd, dpcd, 8);
+   return 0;
+}
+EXPORT_SYMBOL(vga_switcheroo_set_dpcd);
+
+u8 *vga_switcheroo_get_dpcd(struct pci_dev *pdev)
+{
+   struct vga_switcheroo_client *client;
+   client = find_client_from_pci(&vgasr_priv.clients, pdev);
+
+   if (!client || !client->use_panel)
+   return NULL;
+
+   return vgasr_priv.dpcd;
+}
+EXPORT_SYMBOL(vga_switcheroo_get_dpcd);
+
+int vga_switcheroo_set_edid(struct edid *edid)
+{
+   int size = EDID_LENGTH * (1 + edid->extensions);
+
+   if (vgasr_priv.edid)
+   return -EEXIST;
+
+   vgasr_priv.edid = kmalloc(size, GFP_KERNEL);
+   memcpy(vgasr_priv.edid, edid, size);
+   return 0;
+}
+EXPORT_SYMBOL(vga_switcheroo_set_edid);
+
+struct edid *vga_switcheroo_get_edid(struct pci_dev *pdev)
+{
+   struct vga_switcheroo_client *client;
+   client = find_client_from_pci(&vgasr_priv.clients, pdev);
+
+   if (!client || !client->use_panel)
+   return NULL;
+
+   return vgasr_priv.edid;
+}
+EXPORT_SYMBOL(vga_switcheroo_get_edid);
+
 static int __init vga_switcheroo_setup(char *str)
 {
if (!strcmp(str, "IGD"))
diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
index bae62fd..07b07fc 100644
--- a/include/linux/vga_switcheroo.h
+++ b/include/linux/vga_switcheroo.h
@@ -10,6 +10,7 @@
 #ifndef _LINUX_VGA_SWITCHEROO_H_
 #define _LINUX_VGA_SWITCHEROO_H_

+#include 
 #include 

 struct pci_dev;
@@ -69,6 +70,12 @@ void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, 
enum vga_switcheroo

 int vga_switcheroo_init_domain_pm_ops(struct device *dev, struct dev_pm_domain 
*domain);
 int vga_switcheroo_init_domain_pm_optimus_hdmi_audio(struct device *dev, 
struct dev_pm_domain *domain);
+
+int vga_switcheroo_set_dpcd(u8 *dpcd);
+u8 *vga_switcheroo_get_dpcd(struct pci_dev *pdev);
+int vga_switcheroo_set_edid(struct edid *edid);
+struct edid *vga_switcheroo_get_edid(struct pci_dev *pdev);
+
 #else

 static inline void vga_switcheroo_unregister_client(struct pci_dev *dev) {}
@@ -89,5 +96,10 @@ static inline void vga_switcheroo_set_dynamic_switch(struct 
pci_dev *pdev, enum
 static inline int vga_switcheroo_init_domain_pm_ops(struct device *dev, struct 
dev_pm_domain *domain) { return -EINVAL; }
 static inline int vga_switcheroo_init_domain_pm_optimus_hdmi_audio(struct 
devi

[PATCH 03/11] vga_switcheroo: Add command line option

[Matthew Garrett]

Add a command line option in order to allow the user to provide a perferred
GPU at boot time.

Signed-off-by: Matthew Garrett 
---
 Documentation/kernel-parameters.txt |  5 +
 drivers/gpu/vga/vga_switcheroo.c| 29 +
 2 files changed, 34 insertions(+)

diff --git a/Documentation/kernel-parameters.txt 
b/Documentation/kernel-parameters.txt
index 30a8ad0d..6e6d505 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -3486,6 +3486,11 @@ bytes respectively. Such letter suffixes can also be 
entirely omitted.
This is actually a boot loader parameter; the value is
passed to the kernel using a special protocol.

+   vga_switcheroo= [HW] Switches the enabled GPU at boot time.
+   Format: { IGD | DIS }
+   IGD -- enable the Integrated GPU
+   DIS -- enable the Discrete GPU
+
vmalloc=nn[KMG] [KNL,BOOT] Forces the vmalloc area to have an exact
size of . This can be used to increase the
minimum size (128MB on x86). It can also be used to
diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index c06ad5f..6d95626 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -67,6 +67,11 @@ struct vgasr_priv {
 static int vga_switcheroo_debugfs_init(struct vgasr_priv *priv);
 static void vga_switcheroo_debugfs_fini(struct vgasr_priv *priv);

+static enum vga_switcheroo_client_id vga_switcheroo_default = -1;
+
+static int vga_switchto_stage1(struct vga_switcheroo_client *new_client);
+static int vga_switchto_stage2(struct vga_switcheroo_client *new_client);
+
 /* only one switcheroo per system */
 static struct vgasr_priv vgasr_priv = {
.clients = LIST_HEAD_INIT(vgasr_priv.clients),
@@ -107,6 +112,18 @@ static void vga_switcheroo_enable(void)
vgasr_priv.handler->switch_ddc(old_id);
}
}
+
+   list_for_each_entry(client, &vgasr_priv.clients, list) {
+   if (!client->active && client->id == vga_switcheroo_default) {
+   ret = vga_switchto_stage1(client);
+   if (ret)
+   printk(KERN_ERR "vga_switcheroo: switching 
failed stage 1 %d\n", ret);
+
+   ret = vga_switchto_stage2(client);
+   if (ret)
+   printk(KERN_ERR "vga_switcheroo: switching 
failed stage 2 %d\n", ret);
+   }
+   }
vga_switcheroo_debugfs_init(&vgasr_priv);
vgasr_priv.active = true;
 }
@@ -748,3 +765,15 @@ int 
vga_switcheroo_init_domain_pm_optimus_hdmi_audio(struct device *dev, struct
return -EINVAL;
 }
 EXPORT_SYMBOL(vga_switcheroo_init_domain_pm_optimus_hdmi_audio);
+
+static int __init vga_switcheroo_setup(char *str)
+{
+   if (!strcmp(str, "IGD"))
+   vga_switcheroo_default = VGA_SWITCHEROO_IGD;
+   else if (!strcmp(str, "DIS"))
+   vga_switcheroo_default = VGA_SWITCHEROO_DIS;
+
+   return 1;
+}
+
+__setup("vga_switcheroo=", vga_switcheroo_setup);
-- 
1.8.5.3

[PATCH 02/11] vga_switcheroo: Add support for reprobing connectors

[Matthew Garrett]

If display MUXing is handled by an external driver, the handler may not
initialise until some time after the graphics drivers originally bound. If
the driver is unable to detect some drivers unless appropriately MUXed, this
will cause problems. This patch adds a callback to allow switcheroo to
retrigger the driver's output probing.

Signed-off-by: Matthew Garrett 
---
 drivers/gpu/vga/vga_switcheroo.c | 10 ++
 include/linux/vga_switcheroo.h   |  1 +
 2 files changed, 11 insertions(+)

diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index dd1d587..c06ad5f 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -96,6 +96,16 @@ static void vga_switcheroo_enable(void)
return;

client->id = ret;
+
+   if (!client->active && client->ops->reprobe_connectors) {
+   int old_id = (client->id == VGA_SWITCHEROO_IGD) ?
+   VGA_SWITCHEROO_DIS : VGA_SWITCHEROO_IGD;
+   if (vgasr_priv.handler->switch_ddc)
+   vgasr_priv.handler->switch_ddc(client->id);
+   client->ops->reprobe_connectors(client->pdev);
+   if (vgasr_priv.handler->switch_ddc)
+   vgasr_priv.handler->switch_ddc(old_id);
+   }
}
vga_switcheroo_debugfs_init(&vgasr_priv);
vgasr_priv.active = true;
diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
index 37d6850..bae62fd 100644
--- a/include/linux/vga_switcheroo.h
+++ b/include/linux/vga_switcheroo.h
@@ -40,6 +40,7 @@ struct vga_switcheroo_handler {
 struct vga_switcheroo_client_ops {
void (*set_gpu_state)(struct pci_dev *dev, enum vga_switcheroo_state);
void (*reprobe)(struct pci_dev *dev);
+   void (*reprobe_connectors)(struct pci_dev *dev);
bool (*can_switch)(struct pci_dev *dev);
 };

-- 
1.8.5.3

[PATCH 01/11] vga_switcheroo: Add support for switching only the DDC

[Matthew Garrett]

From: Seth Forshee 

During graphics driver initialization its useful to be able to mux only
the DDC to the inactive client in order to read the EDID. Add a
switch_ddc callback to allow capable handlers to provide this
functionality, and add vga_switcheroo_switch_ddc() to allow DRM to mux
only the DDC.

Signed-off-by: Seth Forshee 
---
 drivers/gpu/vga/vga_switcheroo.c | 38 +-
 include/linux/vga_switcheroo.h   |  4 
 2 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index ec0ae2d..dd1d587 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -256,6 +256,28 @@ void vga_switcheroo_client_fb_set(struct pci_dev *pdev,
 }
 EXPORT_SYMBOL(vga_switcheroo_client_fb_set);

+int vga_switcheroo_switch_ddc(struct pci_dev *pdev)
+{
+   int ret = 0;
+   int id;
+
+   mutex_lock(&vgasr_mutex);
+
+   if (!vgasr_priv.handler) {
+   ret = -ENODEV;
+   goto out;
+   }
+
+   if (vgasr_priv.handler->switch_ddc) {
+   id = vgasr_priv.handler->get_client_id(pdev);
+   ret = vgasr_priv.handler->switch_ddc(id);
+   }
+out:
+   mutex_unlock(&vgasr_mutex);
+   return ret;
+}
+EXPORT_SYMBOL(vga_switcheroo_switch_ddc);
+
 static int vga_switcheroo_show(struct seq_file *m, void *v)
 {
struct vga_switcheroo_client *client;
@@ -353,9 +375,15 @@ static int vga_switchto_stage2(struct 
vga_switcheroo_client *new_client)
console_unlock();
}

+   if (vgasr_priv.handler->switch_ddc) {
+   ret = vgasr_priv.handler->switch_ddc(new_client->id);
+   if (ret)
+   return ret;
+   }
+
ret = vgasr_priv.handler->switchto(new_client->id);
if (ret)
-   return ret;
+   goto restore_ddc;

if (new_client->ops->reprobe)
new_client->ops->reprobe(new_client->pdev);
@@ -367,6 +395,14 @@ static int vga_switchto_stage2(struct 
vga_switcheroo_client *new_client)

new_client->active = true;
return 0;
+
+restore_ddc:
+   if (vgasr_priv.handler->switch_ddc) {
+   int id = (new_client->id == VGA_SWITCHEROO_IGD) ?
+   VGA_SWITCHEROO_DIS : VGA_SWITCHEROO_IGD;
+   vgasr_priv.handler->switch_ddc(id);
+   }
+   return ret;
 }

 static bool check_can_switch(void)
diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
index 502073a..37d6850 100644
--- a/include/linux/vga_switcheroo.h
+++ b/include/linux/vga_switcheroo.h
@@ -29,6 +29,7 @@ enum vga_switcheroo_client_id {
 };

 struct vga_switcheroo_handler {
+   int (*switch_ddc)(enum vga_switcheroo_client_id id);
int (*switchto)(enum vga_switcheroo_client_id id);
int (*power_state)(enum vga_switcheroo_client_id id,
   enum vga_switcheroo_state state);
@@ -54,6 +55,8 @@ int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
 void vga_switcheroo_client_fb_set(struct pci_dev *dev,
  struct fb_info *info);

+int vga_switcheroo_switch_ddc(struct pci_dev *pdev);
+
 int vga_switcheroo_register_handler(struct vga_switcheroo_handler *handler);
 void vga_switcheroo_unregister_handler(void);

@@ -71,6 +74,7 @@ static inline void vga_switcheroo_unregister_client(struct 
pci_dev *dev) {}
 static inline int vga_switcheroo_register_client(struct pci_dev *dev,
const struct vga_switcheroo_client_ops *ops, bool 
driver_power_control) { return 0; }
 static inline void vga_switcheroo_client_fb_set(struct pci_dev *dev, struct 
fb_info *info) {}
+static inline void vga_switcheroo_switch_ddc(struct pci_dev *pdev) { return 
NULL; }
 static inline int vga_switcheroo_register_handler(struct 
vga_switcheroo_handler *handler) { return 0; }
 static inline int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
const struct vga_switcheroo_client_ops *ops,
-- 
1.8.5.3

Improve Apple GMUX support on switcheroo

[Matthew Garrett]

The Apple GMUX switcheroo code doesn't really work that well - there's no
straightforward way to ensure that drivers are loaded in the correct order,
and no mechanism to probe displays without performing a full switch. This
patchset adds infrastructural support to switcheroo and functionality to
GMUX in order to fix things up. This won't actually *work* in its current
form - it needs additional patches to the GPU drivers, which are currently
in a somewhat hacky state.

-- 
Matthew Garrett | matthew.garrett at nebula.com

[PATCH] [ACPI] Default to using the native backlight control on Windows 8 systems

[Matthew Garrett]

The list of machines in the "Use native backlight" table is getting longer
and longer, which is a solid indication that we're doing something wrong.
Disable the ACPI backlight interface if the system claims to be Windows 8
or later.

Signed-off-by: Matthew Garrett 
---

Let's at least get this into -next for 3.16 and figure out whether the
drivers actually work now.

 drivers/acpi/video.c | 139 +--
 1 file changed, 1 insertion(+), 138 deletions(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 48c7e8a..21d2fc9 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -78,14 +78,6 @@ module_param(brightness_switch_enabled, bool, 0644);
 static bool allow_duplicates;
 module_param(allow_duplicates, bool, 0644);

-/*
- * For Windows 8 systems: used to decide if video module
- * should skip registering backlight interface of its own.
- */
-static int use_native_backlight_param = -1;
-module_param_named(use_native_backlight, use_native_backlight_param, int, 
0444);
-static bool use_native_backlight_dmi = false;
-
 static int register_count;
 static struct mutex video_list_lock;
 static struct list_head video_bus_head;
@@ -230,18 +222,9 @@ static int acpi_video_get_next_level(struct 
acpi_video_device *device,
 static int acpi_video_switch_brightness(struct acpi_video_device *device,
 int event);

-static bool acpi_video_use_native_backlight(void)
-{
-   if (use_native_backlight_param != -1)
-   return use_native_backlight_param;
-   else
-   return use_native_backlight_dmi;
-}
-
 static bool acpi_video_verify_backlight_support(void)
 {
-   if (acpi_osi_is_win8() && acpi_video_use_native_backlight() &&
-   backlight_device_registered(BACKLIGHT_RAW))
+   if (acpi_osi_is_win8() && backlight_device_registered(BACKLIGHT_RAW))
return false;
return acpi_video_backlight_support();
 }
@@ -405,12 +388,6 @@ static int __init video_set_bqc_offset(const struct 
dmi_system_id *d)
return 0;
 }

-static int __init video_set_use_native_backlight(const struct dmi_system_id *d)
-{
-   use_native_backlight_dmi = true;
-   return 0;
-}
-
 static struct dmi_system_id video_dmi_table[] __initdata = {
/*
 * Broken _BQC workaround 
http://bugzilla.kernel.org/show_bug.cgi?id=13121
@@ -455,120 +432,6 @@ static struct dmi_system_id video_dmi_table[] __initdata 
= {
DMI_MATCH(DMI_PRODUCT_NAME, "Aspire 7720"),
},
},
-   {
-.callback = video_set_use_native_backlight,
-.ident = "ThinkPad T430s",
-.matches = {
-   DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
-   DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad T430s"),
-   },
-   },
-   {
-.callback = video_set_use_native_backlight,
-.ident = "ThinkPad X230",
-.matches = {
-   DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
-   DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad X230"),
-   },
-   },
-   {
-   .callback = video_set_use_native_backlight,
-   .ident = "ThinkPad X1 Carbon",
-   .matches = {
-   DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
-   DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad X1 Carbon"),
-   },
-   },
-   {
-.callback = video_set_use_native_backlight,
-.ident = "Lenovo Yoga 13",
-.matches = {
-   DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
-   DMI_MATCH(DMI_PRODUCT_VERSION, "Lenovo IdeaPad Yoga 13"),
-   },
-   },
-   {
-.callback = video_set_use_native_backlight,
-.ident = "Dell Inspiron 7520",
-.matches = {
-   DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
-   DMI_MATCH(DMI_PRODUCT_VERSION, "Inspiron 7520"),
-   },
-   },
-   {
-.callback = video_set_use_native_backlight,
-.ident = "Acer Aspire 5733Z",
-.matches = {
-   DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
-   DMI_MATCH(DMI_PRODUCT_NAME, "Aspire 5733Z"),
-   },
-   },
-   {
-.callback = video_set_use_native_backlight,
-.ident = "Acer Aspire V5-431",
-.matches = {
-   DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
-   DMI_MATCH(DMI_PRODUCT_NAME, "Aspire V5-431"),
-   },
-   },
-   {
-   .callback = video_set_use_native_backlight,
-   .ident = "HP ProBook 4340s",
-   .matches = {
-   DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"),
-   DMI_MATCH(DMI_PRODUCT_VERSION, &q

Fixing the kernels backlight API

[Matthew Garrett]

On Thu, Feb 13, 2014 at 08:43:10PM +0100, Hans de Goede wrote:

> I can understand where Dave is coming from, from a kernel pov, so this
> might really be easier to just solve in userspace. I don't know if you've
> seen my very rough sketch of how this could work in userspace in my other
> mail. The idea would be to still make this show up on a specific connector,
> but only at the xrandr level, not at the drm level, and have the xserver
> talk to some kind of backlightd for this.  The backlightd API should of cource
> be generic enough that it can be used in wayland too.

Sure. You should probably take a look at libbacklight, which already has 
some amount of support for appropriate heuristics.

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

Fixing the kernels backlight API

[Matthew Garrett]

On Thu, Feb 13, 2014 at 02:41:53PM +0100, Hans de Goede wrote:

> I still believe we need to do better, but maybe that better needs to be done
> in userspace rather then in the kernel.

One option is to put it on the connector but provide some mechanism for 
userspace to define the relationship between platform/firmware 
interfaces and connectors. We ought to be able to tie connectors into 
the ACPI namespace and do a better job than we currently do with the 
association between backlight and connector. The bigger problem is 
figuring out how platform interfaces tie into things, and that's where 
heuristics are probably going to be involved.

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

[PATCH v3] ACPI / video: Add systems that should favor native backlight interface

[Matthew Garrett]

On Tue, 2014-01-21 at 13:32 +0800, Aaron Lu wrote:
> On 01/21/2014 11:17 AM, Matthew Garrett wrote:
> > We know that Windows 8 graphics drivers don't use the ACPI interface,
> > and that systems change their behaviour as a result, in some cases with
> > absolutely no way for the ACPI interface could possibly work. I haven't
> > seen any cases where that's obviously true for any non-Windows 8
> 
> Perhaps I'm not clear, I didn't mean non-Windows 8 systems will all favor
> GPU's interface, I just meant for one specific win7 laptop I could re-use
> the existing code to make the GPU's interface as the only one left. And to
> achieve this, the Win8 OSI check in acpi_video_verify_backlight_support
> has to be gone.

We could do that, but why do we think that's the correct fix? The plan
is to remove the native list entirely and do this for all Windows 8
systems, so the Win8 OSI check is the right thing to do.

-- 
Matthew Garrett 

[PATCH v3] ACPI / video: Add systems that should favor native backlight interface

[Matthew Garrett]

On Tue, 2014-01-21 at 10:24 +0800, Aaron Lu wrote:
> On 01/20/2014 09:34 PM, Matthew Garrett wrote:
> > On Mon, 2014-01-20 at 16:12 +0800, Aaron Lu wrote:
> > 
> >> 1 remove the win8 OSI check, I've seen win7 laptops that also needs to
> >>   have only the GPU interface left and checking win8 doesn't make much
> >>   sense now;
> > 
> > Are we sure that those aren't simply some other bug?
> 
> Well, the firmware on that laptop makes use of EC to do backlight
> control and the fact that the firmware interface doesn't work while the
> GPU's work seems to indicate that the backlight control circuit is not
> routed to EC. I think this is the same case as Win8 laptops.

We know that Windows 8 graphics drivers don't use the ACPI interface,
and that systems change their behaviour as a result, in some cases with
absolutely no way for the ACPI interface could possibly work. I haven't
seen any cases where that's obviously true for any non-Windows 8
systems. EC interfaces that don't work are often due to Linux leaving
the hardware in a state other than the one expected by the firmware. We
shouldn't assume that it's the same issue until we've investigated
further.

-- 
Matthew Garrett 

[PATCH v3] ACPI / video: Add systems that should favor native backlight interface

[Matthew Garrett]

On Mon, 2014-01-20 at 16:12 +0800, Aaron Lu wrote:

> 1 remove the win8 OSI check, I've seen win7 laptops that also needs to
>   have only the GPU interface left and checking win8 doesn't make much
>   sense now;

Are we sure that those aren't simply some other bug?

-- 
Matthew Garrett 

[PATCH v5 0/4] Fix Win8 backlight issue

[Matthew Garrett]

On Wed, 2013-10-16 at 01:33 +0200, Rafael J. Wysocki wrote:

> Since the next step will be to introduce a list of systems that need
> video.use_native_backlight=1 *and* don't break in that configuration, I don't
> see much point adding another Kconfig option for the default.

I'd still really prefer not to add such a list, because it almost
inevitably means that we'll never fix this problem properly.

-- 
Matthew Garrett 

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Matthew Garrett]

On Wed, 2013-09-11 at 13:29 +0300, Jani Nikula wrote:
> On Wed, 11 Sep 2013, Matthew Garrett  wrote:
> > On Wed, 2013-09-11 at 11:45 +0300, Jani Nikula wrote:
> >
> >> Before plunging forward, have you observed any difference between the
> >> boot modes? We have reports [1] that the backlight behaviour is
> >> different with UEFI vs. UEFI+CSM or legacy boot. So I'm wondering if the
> >> acpi_gbl_osi_data >= ACPI_OSI_WIN_8 check in patch 2/2 is the whole
> >> story.
> >> 
> >> Further, if we tell the BIOS we're Windows 8 to use the tested BIOS code
> >> paths, what guarantees do we have of UEFI+CSM or legacy boots working?
> >
> > We have no evidence of Windows behaving differently based on the exposed
> > firmware type.
> 
> By "behaving differently", do you mean internally adapting to the boot
> mode, or exhibiting different behaviour to the user?

As far as backlight control goes, both.

> We have evidence of the firmware behaving differently (VBT, backlight)
> based on the boot mode, all else being equal. We don't adapt to that,
> and we fail. I don't know if we should adapt, or do things differently
> altogether. I don't even know if Windows 8 works on all boot modes on
> the machines in question.

Sure, but Windows knows nothing about VBT or opregion-backed backlight
control. That's up to the Intel driver.

-- 
Matthew Garrett 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Matthew Garrett]

On Wed, 2013-09-11 at 11:45 +0300, Jani Nikula wrote:

> Before plunging forward, have you observed any difference between the
> boot modes? We have reports [1] that the backlight behaviour is
> different with UEFI vs. UEFI+CSM or legacy boot. So I'm wondering if the
> acpi_gbl_osi_data >= ACPI_OSI_WIN_8 check in patch 2/2 is the whole
> story.
> 
> Further, if we tell the BIOS we're Windows 8 to use the tested BIOS code
> paths, what guarantees do we have of UEFI+CSM or legacy boots working?

We have no evidence of Windows behaving differently based on the exposed
firmware type.

-- 
Matthew Garrett 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Matthew Garrett]

On Tue, 2013-09-10 at 17:21 +0300, Jani Nikula wrote:
> On Tue, 10 Sep 2013, Matthew Garrett  wrote:
> > On Tue, 2013-09-10 at 16:53 +0300, Jani Nikula wrote:
> >
> >> I think the parameter "Does the ACPI backlight interface work or not"
> >> belongs to the ACPI video driver.
> >
> > That depends on how Windows 8 works. If Windows 8 policy is handled by
> > the GPU drivers then it belongs in i915. If it's handled by the ACPI
> > code then it belongs in the ACPI code.
> 
> I fail to see the logic. Windows 8 policy dictates whether we can use
> the AML code or not. IMHO, ACPI code is in the best position to figure
> this out and quirk as necessary. It's the part that knows about Windows
> 8, not i915.

So if nvidia hardware uses the ACPI interface and Intel doesn't, we
should still quirk it in the ACPI driver?

-- 
Matthew Garrett 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Matthew Garrett]

On Tue, 2013-09-10 at 16:53 +0300, Jani Nikula wrote:

> I think the parameter "Does the ACPI backlight interface work or not"
> belongs to the ACPI video driver.

That depends on how Windows 8 works. If Windows 8 policy is handled by
the GPU drivers then it belongs in i915. If it's handled by the ACPI
code then it belongs in the ACPI code. But I have no way of determining
that, whereas you work for a company that produces a Windows 8 video
driver…

-- 
Matthew Garrett 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Matthew Garrett]

On Mon, 2013-09-09 at 17:21 +0200, Daniel Vetter wrote:

> If Win8 is as broken as we are I'm ok with the module option. It just
> sounded to me like right now we don't know of a way to make all machines
> somewhat happy, combined with the other pile of random backlight issues
> the assumption that we do something (maybe something a bit racy) that
> windows doesn't do isn't too far-fetched. So I'm not wary of the machines
> where the aml is busted for acpi_os=win8, but for the others where this
> broke stuff.

Windows 8 isn't broken because (as far as we can tell) the Intel drivers
under Windows 8 never use the ACPI backlight set function. Of course, it
would be nice to have that confirmed by Intel.

-- 
Matthew Garrett 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Backlight control only in the kernel?

[Matthew Garrett]

On Wed, 2013-08-07 at 09:44 +0200, Borislav Petkov wrote:

> Background is that on my x230, I needed to connect the
> Fn-Fx backlight hotkey presses to a script to write to
> /sys/class/backlight/intel_backlight/brightness because Fluxbox doesn't
> do that (and maybe doesn't have to).
> 
> So, without presuming any ACPI or backlight knowledge, can we make the
> backlight control work only in the kernel by connecting the hotkey
> presses to some backlight controlling interface which backlight-capable
> devices implement so that it works regardless of userspace environment?
> Even if the machine is not running X?

Not really. The ACPI driver is able to do this because the events and
the backlight are associated with the same device. We could potentially
make something work with GPU backlight drivers since their PCI device
should also be associated with the backlight device, but things get
complicated quickly - once ddcci support is hooked up you'll also have
kernel backlight devices for some external monitors, and now you need to
make a policy decision about which display should be controlled in
response to the keypress. One reasonable choice would be whichever
display contains the currently focused window, which is obviously out of
scope for the kernel.

So if we're going to do this then we need a generalised mechanism
in-kernel for connecting input devices to backlight devices, and we need
a mechanism for disabling it when userspace knows better. This sounds
like a lot of work for something that should really just be handled by
userspace already.

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

Re: Backlight control only in the kernel?

[Matthew Garrett]

On Wed, 2013-08-07 at 09:44 +0200, Borislav Petkov wrote:

> Background is that on my x230, I needed to connect the
> Fn-Fx backlight hotkey presses to a script to write to
> /sys/class/backlight/intel_backlight/brightness because Fluxbox doesn't
> do that (and maybe doesn't have to).
> 
> So, without presuming any ACPI or backlight knowledge, can we make the
> backlight control work only in the kernel by connecting the hotkey
> presses to some backlight controlling interface which backlight-capable
> devices implement so that it works regardless of userspace environment?
> Even if the machine is not running X?

Not really. The ACPI driver is able to do this because the events and
the backlight are associated with the same device. We could potentially
make something work with GPU backlight drivers since their PCI device
should also be associated with the backlight device, but things get
complicated quickly - once ddcci support is hooked up you'll also have
kernel backlight devices for some external monitors, and now you need to
make a policy decision about which display should be controlled in
response to the keypress. One reasonable choice would be whichever
display contains the currently focused window, which is obviously out of
scope for the kernel.

So if we're going to do this then we need a generalised mechanism
in-kernel for connecting input devices to backlight devices, and we need
a mechanism for disabling it when userspace knows better. This sounds
like a lot of work for something that should really just be handled by
userspace already.

-- 
Matthew Garrett | mj...@srcf.ucam.org
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[Update][PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Matthew Garrett]

On Wed, 2013-07-31 at 02:01 +0200, Rafael J. Wysocki wrote:

> (3) Fix i915 backlight control issues for all systems known to have them
> (that may take a while) and flip the defailt for that option to set when 
> we
> think we're ready.

Unfortunately I don't have any systems that reproduce problems here, so
I think Intel are going to have to take the lead on this one.

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

Re: [Update][PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Matthew Garrett]

On Wed, 2013-07-31 at 02:01 +0200, Rafael J. Wysocki wrote:

> (3) Fix i915 backlight control issues for all systems known to have them
> (that may take a while) and flip the defailt for that option to set when 
> we
> think we're ready.

Unfortunately I don't have any systems that reproduce problems here, so
I think Intel are going to have to take the lead on this one.

-- 
Matthew Garrett | mj...@srcf.ucam.org
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Update][PATCH] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Matthew Garrett]

On Tue, 2013-07-16 at 17:32 +0400, Igor Gnatenko wrote:
> Hmm. I found regression in user-space. In GNOME (maybe and other DEs) we no 
> longer see switch status of backlight.

Yeah, I can duplicate that. Rafael, we have to call
acpi_video_init_brightness() even if we're not going to initialise the
backlight - Thinkpads seem to use this as the trigger for enabling ACPI
notifications rather than handling it in firmware. This seems to do the
job:

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 01b1a25..71865f7 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -900,6 +900,9 @@ static void acpi_video_device_find_cap(struct
acpi_video_device *device)
device->cap._DDC = 1;
}
 
+   if (acpi_video_init_brightness(device))
+   return;
+
if (acpi_video_verify_backlight_support()) {
struct backlight_properties props;
struct pci_dev *pdev;
@@ -909,9 +912,6 @@ static void acpi_video_device_find_cap(struct
acpi_video_device *device)
static int count = 0;
char *name;
 
-   result = acpi_video_init_brightness(device);
-   if (result)
-   return;
name = kasprintf(GFP_KERNEL, "acpi_video%d", count);
if (!name)
        return;


-- 
Matthew Garrett | mj...@srcf.ucam.org
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[Update][PATCH] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Matthew Garrett]

On Tue, 2013-07-16 at 17:32 +0400, Igor Gnatenko wrote:
> Hmm. I found regression in user-space. In GNOME (maybe and other DEs) we no 
> longer see switch status of backlight.

Yeah, I can duplicate that. Rafael, we have to call
acpi_video_init_brightness() even if we're not going to initialise the
backlight - Thinkpads seem to use this as the trigger for enabling ACPI
notifications rather than handling it in firmware. This seems to do the
job:

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 01b1a25..71865f7 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -900,6 +900,9 @@ static void acpi_video_device_find_cap(struct
acpi_video_device *device)
device->cap._DDC = 1;
}

+   if (acpi_video_init_brightness(device))
+   return;
+
if (acpi_video_verify_backlight_support()) {
struct backlight_properties props;
struct pci_dev *pdev;
@@ -909,9 +912,6 @@ static void acpi_video_device_find_cap(struct
acpi_video_device *device)
static int count = 0;
char *name;

-   result = acpi_video_init_brightness(device);
-   if (result)
-   return;
name = kasprintf(GFP_KERNEL, "acpi_video%d", count);
if (!name)
        return;


-- 
Matthew Garrett | mjg59 at srcf.ucam.org

[PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Matthew Garrett]

On Tue, Jun 25, 2013 at 11:46:01PM +0200, Yves-Alexis Perez wrote:
> But if that introduce regressions, shouldn't workarounds be found then?
> Sorry if I keep repeating that but brightness keys handling in-kernel is
> quite a useful feature and losing it (because of the ?behave exactly
> like Windows 8 kernel? policy) is indeed a regression.

Your firmware behaves differently depending on whether the OS claims to 
be Windows 8 or not. We can't make that invisible.

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

[PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Matthew Garrett]

On Tue, Jun 25, 2013 at 11:30:37PM +0200, Yves-Alexis Perez wrote:
> On mar., 2013-06-25 at 22:14 +0100, Matthew Garrett wrote:
> > Which, as we've already established, you don't - Lenovo broke it. Your 
> > Thinkpad claims to have 100 available levels, and most of them don't 
> > work. The kernel has no way of knowing which levels work and which 
> > don't, so leaving this up to the kernel won't actually fix your system 
> > either.
> 
> I was referring to ?standardize the behaviour by leaving up to
> userspace?. A lot of thinkpads (for example) (all the pre-windows 8
> ones) have a perfectly working ACPI backlight interface.

And this patchset won't alter their behaviour.

> Also, if the kernel has no way of knowing which levels work, I fail to
> see how userspace can do better.

It can't. That's why this patchset disables the ACPI interface on 
Windows 8 systems.

> I understand that switching to intel_backlight instead of acpi_video0
> follows what Windows 8 recommends but for me it looks orthogonal to the
> fact ACPI methods now have some awkward (Lenovo) or broken (Dell). I
> mean, it's not the first time firmware people break some kernel
> behavior. I know it's usually not easy to contact them, but shouldn't
> those methods be fixed, instead of somehow blindly switching to graphic
> drivers?

No. The correct answer to all firmware issues is "Are we making the same 
firmware calls as the version of Windows that this hardware thinks it's 
running". If Windows 8 doesn't make these calls, we shouldn't make these 
calls.

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

[PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Matthew Garrett]

On Tue, Jun 25, 2013 at 11:10:11PM +0200, Yves-Alexis Perez wrote:
> On mar., 2013-06-25 at 21:54 +0100, Matthew Garrett wrote:
> > I agree, we should standardise the behaviour. And the only way we can 
> > standardise the behaviour is to leave it up to userspace.
> > 
> It's pretty clear we disagree on this and that my opinion won't really
> matter here. But letting userspace handle that just means broken
> functionality for those who have the chance (apparently) to have an ACPI
> backlight interface.

Which, as we've already established, you don't - Lenovo broke it. Your 
Thinkpad claims to have 100 available levels, and most of them don't 
work. The kernel has no way of knowing which levels work and which 
don't, so leaving this up to the kernel won't actually fix your system 
either.

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

[PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Matthew Garrett]

On Tue, Jun 25, 2013 at 10:43:57PM +0200, Yves-Alexis Perez wrote:
> On mar., 2013-06-25 at 17:08 +0100, Matthew Garrett wrote:
> > Right, the kernel has special-casing to hook the backlight keys up to 
> > the ACPI backlight control. This is an awful thing, because there's no 
> > way to detect this case other than parsing a single driver-specific 
> > module parameter.
> 
> I'm not sure what that means. To detect what case exactly? That the
> brightness is handled by video.ko?

That the kernel will automatically handle backlight key presses.

> > Could this functionality be duplicated across other backlight drivers? 
> > Not easily. The ACPI driver receives keypresses and performs backlight 
> > control. The i915 driver doesn't receive keypresses. We could easily tie 
> > certain keycodes into backlight events, but which backlight should they 
> > control? You're really starting to get into the kind of complex policy 
> > decision that's best left to userspace, which is where it should have 
> > been to begin with.
> > 
> Well, I get the reasoning, but I'm not sure I agree. That means
> userspace behavior is inconsistent depending on who does it
> (gnome-power-manager, gnome-setting-daemon, whatever), and it usually
> means there's nothing handling the brightness before those are running,
> not to mention people not running them because they don't run a full
> blown desktop environment (until someone starts thinking it's a good
> idea to handle brightness in systemd).

The behaviour is already inconsistent. If you have an ACPI backlight 
interface, hitting the keys makes your brightness change without any 
userspace help. If you don't, it doesn't.

> And in the end, the user just want the brightness keys to correctly
> handle the brightness, full stop. Having multiple brightness daemons
> using different policies on different hardware is a nightmare for the
> end user, imho. From a user point of view, having it handled all in the
> kernel works really pretty fine and is completely transparent (I have to
> admit that from that point of view, it was even better when it was
> handled by the EC but those times seem long gone).

I agree, we should standardise the behaviour. And the only way we can 
standardise the behaviour is to leave it up to userspace.

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

[PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Matthew Garrett]

On Tue, Jun 25, 2013 at 06:10:35PM +0200, Daniel Vetter wrote:

> Do we have any chances to amend this mistake by (gradually) phasing
> out support for the direct keypress forwarding in ACPI? Or at least
> some plans?

We could make the default value of brightness_switch_enabled a config 
variable and encourage distributions to flip their behaviour.

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

[PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Matthew Garrett]

On Sat, Jun 22, 2013 at 11:46:39PM +0200, Yves-Alexis Perez wrote:

> Before Linux support for acpi_osi("Windows 2012") (and when booting with
> acpi_osi="!Windows 2012"), brightness keys were handled by the kernel
> just fine, whether in console, in the display manager or in my desktop
> environment (Xfce). xfce4-power-manager just needs to be told that the
> brightness keys are already handled and it doesn't need to do anything.

Right, the kernel has special-casing to hook the backlight keys up to 
the ACPI backlight control. This is an awful thing, because there's no 
way to detect this case other than parsing a single driver-specific 
module parameter.

Could this functionality be duplicated across other backlight drivers? 
Not easily. The ACPI driver receives keypresses and performs backlight 
control. The i915 driver doesn't receive keypresses. We could easily tie 
certain keycodes into backlight events, but which backlight should they 
control? You're really starting to get into the kind of complex policy 
decision that's best left to userspace, which is where it should have 
been to begin with.

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

Re: [PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Matthew Garrett]

On Tue, Jun 25, 2013 at 11:46:01PM +0200, Yves-Alexis Perez wrote:
> But if that introduce regressions, shouldn't workarounds be found then?
> Sorry if I keep repeating that but brightness keys handling in-kernel is
> quite a useful feature and losing it (because of the “behave exactly
> like Windows 8 kernel” policy) is indeed a regression.

Your firmware behaves differently depending on whether the OS claims to 
be Windows 8 or not. We can't make that invisible.

-- 
Matthew Garrett | mj...@srcf.ucam.org
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Matthew Garrett]

On Tue, Jun 25, 2013 at 11:30:37PM +0200, Yves-Alexis Perez wrote:
> On mar., 2013-06-25 at 22:14 +0100, Matthew Garrett wrote:
> > Which, as we've already established, you don't - Lenovo broke it. Your 
> > Thinkpad claims to have 100 available levels, and most of them don't 
> > work. The kernel has no way of knowing which levels work and which 
> > don't, so leaving this up to the kernel won't actually fix your system 
> > either.
> 
> I was referring to “standardize the behaviour by leaving up to
> userspace”. A lot of thinkpads (for example) (all the pre-windows 8
> ones) have a perfectly working ACPI backlight interface.

And this patchset won't alter their behaviour.

> Also, if the kernel has no way of knowing which levels work, I fail to
> see how userspace can do better.

It can't. That's why this patchset disables the ACPI interface on 
Windows 8 systems.

> I understand that switching to intel_backlight instead of acpi_video0
> follows what Windows 8 recommends but for me it looks orthogonal to the
> fact ACPI methods now have some awkward (Lenovo) or broken (Dell). I
> mean, it's not the first time firmware people break some kernel
> behavior. I know it's usually not easy to contact them, but shouldn't
> those methods be fixed, instead of somehow blindly switching to graphic
> drivers?

No. The correct answer to all firmware issues is "Are we making the same 
firmware calls as the version of Windows that this hardware thinks it's 
running". If Windows 8 doesn't make these calls, we shouldn't make these 
calls.

-- 
Matthew Garrett | mj...@srcf.ucam.org
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Matthew Garrett]

On Tue, Jun 25, 2013 at 11:10:11PM +0200, Yves-Alexis Perez wrote:
> On mar., 2013-06-25 at 21:54 +0100, Matthew Garrett wrote:
> > I agree, we should standardise the behaviour. And the only way we can 
> > standardise the behaviour is to leave it up to userspace.
> > 
> It's pretty clear we disagree on this and that my opinion won't really
> matter here. But letting userspace handle that just means broken
> functionality for those who have the chance (apparently) to have an ACPI
> backlight interface.

Which, as we've already established, you don't - Lenovo broke it. Your 
Thinkpad claims to have 100 available levels, and most of them don't 
work. The kernel has no way of knowing which levels work and which 
don't, so leaving this up to the kernel won't actually fix your system 
either.

-- 
Matthew Garrett | mj...@srcf.ucam.org
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Matthew Garrett]

On Tue, Jun 25, 2013 at 10:43:57PM +0200, Yves-Alexis Perez wrote:
> On mar., 2013-06-25 at 17:08 +0100, Matthew Garrett wrote:
> > Right, the kernel has special-casing to hook the backlight keys up to 
> > the ACPI backlight control. This is an awful thing, because there's no 
> > way to detect this case other than parsing a single driver-specific 
> > module parameter.
> 
> I'm not sure what that means. To detect what case exactly? That the
> brightness is handled by video.ko?

That the kernel will automatically handle backlight key presses.

> > Could this functionality be duplicated across other backlight drivers? 
> > Not easily. The ACPI driver receives keypresses and performs backlight 
> > control. The i915 driver doesn't receive keypresses. We could easily tie 
> > certain keycodes into backlight events, but which backlight should they 
> > control? You're really starting to get into the kind of complex policy 
> > decision that's best left to userspace, which is where it should have 
> > been to begin with.
> > 
> Well, I get the reasoning, but I'm not sure I agree. That means
> userspace behavior is inconsistent depending on who does it
> (gnome-power-manager, gnome-setting-daemon, whatever), and it usually
> means there's nothing handling the brightness before those are running,
> not to mention people not running them because they don't run a full
> blown desktop environment (until someone starts thinking it's a good
> idea to handle brightness in systemd).

The behaviour is already inconsistent. If you have an ACPI backlight 
interface, hitting the keys makes your brightness change without any 
userspace help. If you don't, it doesn't.

> And in the end, the user just want the brightness keys to correctly
> handle the brightness, full stop. Having multiple brightness daemons
> using different policies on different hardware is a nightmare for the
> end user, imho. From a user point of view, having it handled all in the
> kernel works really pretty fine and is completely transparent (I have to
> admit that from that point of view, it was even better when it was
> handled by the EC but those times seem long gone).

I agree, we should standardise the behaviour. And the only way we can 
standardise the behaviour is to leave it up to userspace.

-- 
Matthew Garrett | mj...@srcf.ucam.org
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Matthew Garrett]

On Tue, Jun 25, 2013 at 06:10:35PM +0200, Daniel Vetter wrote:

> Do we have any chances to amend this mistake by (gradually) phasing
> out support for the direct keypress forwarding in ACPI? Or at least
> some plans?

We could make the default value of brightness_switch_enabled a config 
variable and encourage distributions to flip their behaviour.

-- 
Matthew Garrett | mj...@srcf.ucam.org
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Matthew Garrett]

On Sat, Jun 22, 2013 at 11:46:39PM +0200, Yves-Alexis Perez wrote:

> Before Linux support for acpi_osi("Windows 2012") (and when booting with
> acpi_osi="!Windows 2012"), brightness keys were handled by the kernel
> just fine, whether in console, in the display manager or in my desktop
> environment (Xfce). xfce4-power-manager just needs to be told that the
> brightness keys are already handled and it doesn't need to do anything.

Right, the kernel has special-casing to hook the backlight keys up to 
the ACPI backlight control. This is an awful thing, because there's no 
way to detect this case other than parsing a single driver-specific 
module parameter.

Could this functionality be duplicated across other backlight drivers? 
Not easily. The ACPI driver receives keypresses and performs backlight 
control. The i915 driver doesn't receive keypresses. We could easily tie 
certain keycodes into backlight events, but which backlight should they 
control? You're really starting to get into the kind of complex policy 
decision that's best left to userspace, which is where it should have 
been to begin with.

-- 
Matthew Garrett | mj...@srcf.ucam.org
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] i915: Don't provide ACPI backlight interface if firmware expects Windows 8

[Matthew Garrett]

On Sat, Jun 15, 2013 at 08:29:42PM +0200, Daniel Vetter wrote:

> Aside at the end: If the gnome tool indeed has its own backlight code and
> doesn't just use that as a fallback if the xrandr backligh property isn't
> available, then that's just a serious bug in gnome and should be fixed
> asap. But imo that's not something we should try to (nor do I see any way
> how to) work around in the kernel.

It's only used if there's no backlight property on the display.

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

[PATCH 3/3] i915: Don't provide ACPI backlight interface if firmware expects Windows 8

[Matthew Garrett]

On Sat, Jun 15, 2013 at 08:29:15PM +0800, Aaron Lu wrote:
> On 06/15/2013 12:19 PM, Matthew Garrett wrote:
> > The vendor will presumably have tested that backlight control works - if 
> > the GPU driver uses the ACPI interface and backlight control is broken, 
> > then the vendor would fix it.
> 
> I don't think GPU driver uses ACPI interface, that's why for some
> systems, ACPI interface is broken while the GPU's is not.

On systems where the ACPI interface is broken, the GPU driver clearly 
doesn't use the ACPI interface. As yet, we don't know if that's always 
true, though.

> > Sure, but it still requires the replacement of existing userspace. We 
> > need to fix the problem with existing userspace, too.
> 
> Yes. The problem is two way. First, some of the backlight interface
> privoder module provides a broken interface; Two, the userspace picked a
> broken interface while there are usable ones. For example, in the win8
> thinkpad case, the ACPI video driver provides broken interface and the
> GPU driver provides usable interface, but userspace choose ACPI video's
> interface. To make things complicated, if we make the broken interface
> disappear in ACPI video module, then the platform driver thinkpad_acpi
> will start to provide another broken interface. I have to say, solve it
> in the GPU code is a clever way, it's just a little weird to me for a
> broken interface to be blacklisted, we have to do it in another module,
> not the broken module itself.

The ACPI driver has no way of making this kind of policy decision. The 
GPU driver does.

> > No, I think we need to fix the bugs that currently require users to pass 
> > options.
> 
> For ACPI video driver, since it relies on firmware to do the right thing,
> if the functionality is broken, then it is, there is hardly anything we
> can do...(not always, we can quirk in some cases, but there are cases we
> can do nothing). In this case, we can:
> 1 Ask user to add acpi_backlight=vendor, so that ACPI video driver will
>   not create backlight interface and userspace will not pick it;
> 2 Add that model in DMI table in video_detect.c, eliminating the need for
>   that command line;
> 3 Add that model in DMI table in another module, let that module
>   unregister backlight interface provided by ACPI video as is done in
>   acer-wmi, asus-wmi, and i915 in this case;
> 3 Use the backlight section in xorg.conf to make X uses another
>   backlight interface. This may not work for distros that use
>   gsd-backlight-helper, which always prefers firmware.
> 
> Which one do you prefer?

I'd prefer to figure out how it works in Windows and implement it the 
same way.

> > How do these machines work under Windows? Until we know the answer to 
> > that, we don't know what the correct way to handle the problem is under 
> > Linux.
> 
> Do you mean we need to understand Windows behavior so that we can
> decide where to place the code to do the backlight management thing?
> So for win8 case, we know MS will use GPU driver to do backlight
> control, all this means to me is, ACPI video's and platform driver's
> interface are more likely broken on those systems, but it doesn't
> qualify why Linux' GPU driver should do the decision, it's not that the
> GPU driver can poke some GPU register and then decide oh, this model
> does not have working ACPI backlight interface. As this patch shows, we
> make the decision by OSI, which is not specific to GPU driver.

Sure it is - for all we know there's a value in the opregion space that 
tells the Intel GPU driver which interface we should be using. We don't 
know because Intel haven't released a version of the opregion driver 
newer than 2007. It may be that all Windows 8 GPU drivers use the GPU 
backlight control registers directly and never use any firmware 
interface, but right now we simply don't know that. All we can do is 
look at the existing cases we have and say that it appears that 
Intel graphics systems don't use the ACPI interface. Anything more than 
that requires more evidence.

> BTW, I don't think any module knows something better than another
> module, all quirk logic and DMI entry we currently have in Linux are
> got by user's feedback(bug reports), it doesn't seem to me some module
> is natrually the one that should make this decision. Or do I miss
> something?

The GPU driver makes the policy decision under Windows 8, so it's the 
natural place to make this decision on Windows 8 systems.

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

Re: [PATCH 3/3] i915: Don't provide ACPI backlight interface if firmware expects Windows 8

[Matthew Garrett]

On Sat, Jun 15, 2013 at 08:29:42PM +0200, Daniel Vetter wrote:

> Aside at the end: If the gnome tool indeed has its own backlight code and
> doesn't just use that as a fallback if the xrandr backligh property isn't
> available, then that's just a serious bug in gnome and should be fixed
> asap. But imo that's not something we should try to (nor do I see any way
> how to) work around in the kernel.

It's only used if there's no backlight property on the display.

-- 
Matthew Garrett | mj...@srcf.ucam.org
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] i915: Don't provide ACPI backlight interface if firmware expects Windows 8

[Matthew Garrett]

On Sat, Jun 15, 2013 at 08:29:15PM +0800, Aaron Lu wrote:
> On 06/15/2013 12:19 PM, Matthew Garrett wrote:
> > The vendor will presumably have tested that backlight control works - if 
> > the GPU driver uses the ACPI interface and backlight control is broken, 
> > then the vendor would fix it.
> 
> I don't think GPU driver uses ACPI interface, that's why for some
> systems, ACPI interface is broken while the GPU's is not.

On systems where the ACPI interface is broken, the GPU driver clearly 
doesn't use the ACPI interface. As yet, we don't know if that's always 
true, though.

> > Sure, but it still requires the replacement of existing userspace. We 
> > need to fix the problem with existing userspace, too.
> 
> Yes. The problem is two way. First, some of the backlight interface
> privoder module provides a broken interface; Two, the userspace picked a
> broken interface while there are usable ones. For example, in the win8
> thinkpad case, the ACPI video driver provides broken interface and the
> GPU driver provides usable interface, but userspace choose ACPI video's
> interface. To make things complicated, if we make the broken interface
> disappear in ACPI video module, then the platform driver thinkpad_acpi
> will start to provide another broken interface. I have to say, solve it
> in the GPU code is a clever way, it's just a little weird to me for a
> broken interface to be blacklisted, we have to do it in another module,
> not the broken module itself.

The ACPI driver has no way of making this kind of policy decision. The 
GPU driver does.

> > No, I think we need to fix the bugs that currently require users to pass 
> > options.
> 
> For ACPI video driver, since it relies on firmware to do the right thing,
> if the functionality is broken, then it is, there is hardly anything we
> can do...(not always, we can quirk in some cases, but there are cases we
> can do nothing). In this case, we can:
> 1 Ask user to add acpi_backlight=vendor, so that ACPI video driver will
>   not create backlight interface and userspace will not pick it;
> 2 Add that model in DMI table in video_detect.c, eliminating the need for
>   that command line;
> 3 Add that model in DMI table in another module, let that module
>   unregister backlight interface provided by ACPI video as is done in
>   acer-wmi, asus-wmi, and i915 in this case;
> 3 Use the backlight section in xorg.conf to make X uses another
>   backlight interface. This may not work for distros that use
>   gsd-backlight-helper, which always prefers firmware.
> 
> Which one do you prefer?

I'd prefer to figure out how it works in Windows and implement it the 
same way.

> > How do these machines work under Windows? Until we know the answer to 
> > that, we don't know what the correct way to handle the problem is under 
> > Linux.
> 
> Do you mean we need to understand Windows behavior so that we can
> decide where to place the code to do the backlight management thing?
> So for win8 case, we know MS will use GPU driver to do backlight
> control, all this means to me is, ACPI video's and platform driver's
> interface are more likely broken on those systems, but it doesn't
> qualify why Linux' GPU driver should do the decision, it's not that the
> GPU driver can poke some GPU register and then decide oh, this model
> does not have working ACPI backlight interface. As this patch shows, we
> make the decision by OSI, which is not specific to GPU driver.

Sure it is - for all we know there's a value in the opregion space that 
tells the Intel GPU driver which interface we should be using. We don't 
know because Intel haven't released a version of the opregion driver 
newer than 2007. It may be that all Windows 8 GPU drivers use the GPU 
backlight control registers directly and never use any firmware 
interface, but right now we simply don't know that. All we can do is 
look at the existing cases we have and say that it appears that 
Intel graphics systems don't use the ACPI interface. Anything more than 
that requires more evidence.

> BTW, I don't think any module knows something better than another
> module, all quirk logic and DMI entry we currently have in Linux are
> got by user's feedback(bug reports), it doesn't seem to me some module
> is natrually the one that should make this decision. Or do I miss
> something?

The GPU driver makes the policy decision under Windows 8, so it's the 
natural place to make this decision on Windows 8 systems.

-- 
Matthew Garrett | mj...@srcf.ucam.org
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] i915: Don't provide ACPI backlight interface if firmware expects Windows 8

[Matthew Garrett]

On Sat, 2013-06-15 at 09:26 +0800, Aaron Lu wrote:
> On 06/15/2013 01:29 AM, Matthew Garrett wrote: 
> > How would that work with existing userspace?
> 
> User space tool will need to be updated to use this as stated in the
> gist page, I've patches for gsd-backlight-helper and xorg-x11-drv-intel,
> for others we can add I think if the priority based solution is deemed
> useful.

Right, that's not a great solution.

> > We shouldn't export interfaces if we don't expect them to work.
> 
> It's not easy to decide if they work or not sometimes, e.g. I came
> across a system that claims win8 in ACPI table and has an Intel GPU,
> while its ACPI video interface also works. With this patch, the working
> ACPI video interface is removed, while with the priority based solution,
> the GPU's interface priority gets higher, but the ACPI video interface
> still stays.

Well, Windows 8 will only use the ACPI backlight interface if the GPU
driver decides to, right? So the logic for deciding whether to remove
the ACPI backlight control or not should be left up to the GPU. There's
no harm in refusing to expose a working method if there's another
working method, but there is harm in exposing a broken one and expecting
userspace to know the difference.

-- 
Matthew Garrett | mj...@srcf.ucam.org
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] i915: Don't provide ACPI backlight interface if firmware expects Windows 8

[Matthew Garrett]

On Fri, 2013-06-14 at 14:47 +0800, Aaron Lu wrote:

> What about a priority based solution? We can introduce a new field named
> priority to backlight_device and instead of calling another module's
> function like the unregister one here(which cause unnecessary module
> dependency), we only need to boost priority for its own interface. This
> field will be exported to sysfs, so user can change it during runtime
> too. And we can also introduce a new kernel command line as
> backlight.force_interface=raw/firmware/platform, to overcome the limited
> functionality provided by acpi_backlight=video/vendor, which does not
> involve GPU's interface.

How would that work with existing userspace?

> And we can place the quirk code in backlight layer instead of individual
> backlight functionality provider module. Suppose we have a backlight
> manager there, for all win8 systems, we can boost the raw type's
> priority on its registration, so no need to add code in
> intel/amd/etc./'s GPU driver code.

But we'd need to add code to every piece of userspace that currently
uses the backlight, right?

> With priority based solution, all backlight control interfaces stay,
> the priority field is an indication given by kernel to user space.

We shouldn't export interfaces if we don't expect them to work.

-- 
Matthew Garrett | mj...@srcf.ucam.org
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] i915: Don't provide ACPI backlight interface if firmware expects Windows 8

[Matthew Garrett]

On Sat, Jun 15, 2013 at 12:14:42PM +0800, Aaron Lu wrote:
> On 06/15/2013 09:38 AM, Matthew Garrett wrote:
> > Well, Windows 8 will only use the ACPI backlight interface if the GPU
> > driver decides to, right? So the logic for deciding whether to remove
> > the ACPI backlight control or not should be left up to the GPU. There's
> 
> I don't know this. From the document I googled, Microsoft suggests under
> win8, backlight should be controlled by the graphics driver for smooth
> brightness level change, instead of ACPI or other methods. So it is
> possible that OEM will not test the ACPI interface well and thus the
> interface is likely broken. I don't see why GPU driver has any better
> knowledge on which systems the firmware interface is broken or not.

The vendor will presumably have tested that backlight control works - if 
the GPU driver uses the ACPI interface and backlight control is broken, 
then the vendor would fix it.

> > no harm in refusing to expose a working method if there's another
> > working method, but there is harm in exposing a broken one and expecting
> > userspace to know the difference.
> 
> BTW, the proposed solution is not meant to solve win8 problems alone, it
> should make solving other problems easy and make individual backlight
> control interface provider module independent with each other such as
> the platform drivers will not need to check if ACPI video driver will
> control backlight or not and can always create backlight interface(its
> default priority is lower that ACPI video driver's so will not be taken
> by user space by default, showing the same behavior of the current code).

Sure, but it still requires the replacement of existing userspace. We 
need to fix the problem with existing userspace, too.

> The current acpi_backlight=video/vendor kernel command line is pretty
> misleading, for laptops that do not have vendor backlight interface,
> adding acpi_backlight=vendor actually makes the system using the GPU's
> interface. Some laptops are using this switch to work around problems in
> ACPI video driver and users think they are using vendor interface.
> That's why I think we need a new command line as the
> backlight.force_interface=raw/firmware/platform.

No, I think we need to fix the bugs that currently require users to pass 
options.

> Instead of letting individual driver to make decisions on which
> backlight interface this system should use(either in platform driver as
> we currently did, see acer-wmi and asus-wmi, or GPU driver as this case),
> I think we need a better and clear way to handle such things. For
> example, suppose an acer laptop: vendor does not support backlight, ACPI
> video's backlight interface is broken and GPU's works, to make it work,
> user will need to select acer-wmi module while this module does not have
> anything to do with the functionality, but simply because it serves as
> the backlight manager for acer laptops.

How do these machines work under Windows? Until we know the answer to 
that, we don't know what the correct way to handle the problem is under 
Linux.

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

[PATCH 3/3] i915: Don't provide ACPI backlight interface if firmware expects Windows 8

[Matthew Garrett]

On Sat, 2013-06-15 at 09:26 +0800, Aaron Lu wrote:
> On 06/15/2013 01:29 AM, Matthew Garrett wrote: 
> > How would that work with existing userspace?
> 
> User space tool will need to be updated to use this as stated in the
> gist page, I've patches for gsd-backlight-helper and xorg-x11-drv-intel,
> for others we can add I think if the priority based solution is deemed
> useful.

Right, that's not a great solution.

> > We shouldn't export interfaces if we don't expect them to work.
> 
> It's not easy to decide if they work or not sometimes, e.g. I came
> across a system that claims win8 in ACPI table and has an Intel GPU,
> while its ACPI video interface also works. With this patch, the working
> ACPI video interface is removed, while with the priority based solution,
> the GPU's interface priority gets higher, but the ACPI video interface
> still stays.

Well, Windows 8 will only use the ACPI backlight interface if the GPU
driver decides to, right? So the logic for deciding whether to remove
the ACPI backlight control or not should be left up to the GPU. There's
no harm in refusing to expose a working method if there's another
working method, but there is harm in exposing a broken one and expecting
userspace to know the difference.

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

Re: [PATCH 3/3] i915: Don't provide ACPI backlight interface if firmware expects Windows 8

[Matthew Garrett]

On Sat, Jun 15, 2013 at 12:14:42PM +0800, Aaron Lu wrote:
> On 06/15/2013 09:38 AM, Matthew Garrett wrote:
> > Well, Windows 8 will only use the ACPI backlight interface if the GPU
> > driver decides to, right? So the logic for deciding whether to remove
> > the ACPI backlight control or not should be left up to the GPU. There's
> 
> I don't know this. From the document I googled, Microsoft suggests under
> win8, backlight should be controlled by the graphics driver for smooth
> brightness level change, instead of ACPI or other methods. So it is
> possible that OEM will not test the ACPI interface well and thus the
> interface is likely broken. I don't see why GPU driver has any better
> knowledge on which systems the firmware interface is broken or not.

The vendor will presumably have tested that backlight control works - if 
the GPU driver uses the ACPI interface and backlight control is broken, 
then the vendor would fix it.

> > no harm in refusing to expose a working method if there's another
> > working method, but there is harm in exposing a broken one and expecting
> > userspace to know the difference.
> 
> BTW, the proposed solution is not meant to solve win8 problems alone, it
> should make solving other problems easy and make individual backlight
> control interface provider module independent with each other such as
> the platform drivers will not need to check if ACPI video driver will
> control backlight or not and can always create backlight interface(its
> default priority is lower that ACPI video driver's so will not be taken
> by user space by default, showing the same behavior of the current code).

Sure, but it still requires the replacement of existing userspace. We 
need to fix the problem with existing userspace, too.

> The current acpi_backlight=video/vendor kernel command line is pretty
> misleading, for laptops that do not have vendor backlight interface,
> adding acpi_backlight=vendor actually makes the system using the GPU's
> interface. Some laptops are using this switch to work around problems in
> ACPI video driver and users think they are using vendor interface.
> That's why I think we need a new command line as the
> backlight.force_interface=raw/firmware/platform.

No, I think we need to fix the bugs that currently require users to pass 
options.

> Instead of letting individual driver to make decisions on which
> backlight interface this system should use(either in platform driver as
> we currently did, see acer-wmi and asus-wmi, or GPU driver as this case),
> I think we need a better and clear way to handle such things. For
> example, suppose an acer laptop: vendor does not support backlight, ACPI
> video's backlight interface is broken and GPU's works, to make it work,
> user will need to select acer-wmi module while this module does not have
> anything to do with the functionality, but simply because it serves as
> the backlight manager for acer laptops.

How do these machines work under Windows? Until we know the answer to 
that, we don't know what the correct way to handle the problem is under 
Linux.

-- 
Matthew Garrett | mj...@srcf.ucam.org
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] i915: Don't provide ACPI backlight interface if firmware expects Windows 8

[Matthew Garrett]

On Fri, 2013-06-14 at 14:47 +0800, Aaron Lu wrote:

> What about a priority based solution? We can introduce a new field named
> priority to backlight_device and instead of calling another module's
> function like the unregister one here(which cause unnecessary module
> dependency), we only need to boost priority for its own interface. This
> field will be exported to sysfs, so user can change it during runtime
> too. And we can also introduce a new kernel command line as
> backlight.force_interface=raw/firmware/platform, to overcome the limited
> functionality provided by acpi_backlight=video/vendor, which does not
> involve GPU's interface.

How would that work with existing userspace?

> And we can place the quirk code in backlight layer instead of individual
> backlight functionality provider module. Suppose we have a backlight
> manager there, for all win8 systems, we can boost the raw type's
> priority on its registration, so no need to add code in
> intel/amd/etc./'s GPU driver code.

But we'd need to add code to every piece of userspace that currently
uses the backlight, right?

> With priority based solution, all backlight control interfaces stay,
> the priority field is an indication given by kernel to user space.

We shouldn't export interfaces if we don't expect them to work.

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

Re: [PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Matthew Garrett]

On Mon, 2013-06-10 at 13:59 +0200, Rafael J. Wysocki wrote:

> I happen to know that alternative solution to this problem is being worked on,
> so I'm going to wait until it is submitted and then we'll decide what to 
> merge.

Sure.

> I'm slightly concerned about unregistering ACPI backlight control for all
> systems declaring win8 support, even though it may actually work for them.

Right, that's why I think it's correct to leave it up to the graphics
drivers. It seems like they're the ones that make the policy
determination under Windows now. The policy as implemented here may not
be correct, but doing better would probably involve Intel letting us
know how their Windows driver behaves.

-- 
Matthew Garrett | mj...@srcf.ucam.org
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Matthew Garrett]

On Mon, 2013-06-10 at 13:59 +0200, Rafael J. Wysocki wrote:

> I happen to know that alternative solution to this problem is being worked on,
> so I'm going to wait until it is submitted and then we'll decide what to 
> merge.

Sure.

> I'm slightly concerned about unregistering ACPI backlight control for all
> systems declaring win8 support, even though it may actually work for them.

Right, that's why I think it's correct to leave it up to the graphics
drivers. It seems like they're the ones that make the policy
determination under Windows now. The policy as implemented here may not
be correct, but doing better would probably involve Intel letting us
know how their Windows driver behaves.

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

[PATCH 3/3] i915: Don't provide ACPI backlight interface if firmware expects Windows 8

[Matthew Garrett]

Windows 8 leaves backlight control up to individual graphics drivers rather
than making ACPI calls itself. There's plenty of evidence to suggest that
the Intel driver for Windows doesn't use the ACPI interface, including the
fact that it's broken on a bunch of machines when the OS claims to support
Windows 8. The simplest thing to do appears to be to disable the ACPI
backlight interface on these systems.

Signed-off-by: Matthew Garrett 
---
 drivers/gpu/drm/i915/i915_dma.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 3b315ba..23b6292 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1661,6 +1661,9 @@ int i915_driver_load(struct drm_device *dev, unsigned 
long flags)
/* Must be done after probing outputs */
intel_opregion_init(dev);
acpi_video_register();
+   /* Don't use ACPI backlight functions on Windows 8 platforms */
+   if (acpi_osi_version() >= ACPI_OSI_WIN_8)
+   acpi_video_backlight_unregister();
}

if (IS_GEN5(dev))
-- 
1.8.2.1

[PATCH 2/3] ACPICA: Add interface for getting latest OS version requested via _OSI

[Matthew Garrett]

Drivers may need to make policy decisions based on the OS that the firmware
believes it's interacting with. ACPI firmware will make a series of _OSI
calls, starting from the oldest OS version they support and ending with the
most recent. Add a function to return the last successful call so that
drivers know what the firmware's expecting.

Based on a patch by Seth Forshee 

Signed-off-by: Matthew Garrett 
Cc: Seth Forshee 
---
 drivers/acpi/acpica/aclocal.h | 13 -
 drivers/acpi/acpica/utxface.c | 19 +++
 include/acpi/acpixf.h | 22 ++
 3 files changed, 41 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/acpica/aclocal.h b/drivers/acpi/acpica/aclocal.h
index d5bfbd3..8a2f532 100644
--- a/drivers/acpi/acpica/aclocal.h
+++ b/drivers/acpi/acpica/aclocal.h
@@ -948,19 +948,6 @@ struct acpi_bit_register_info {

 /* Structs and definitions for _OSI support and I/O port validation */

-#define ACPI_OSI_WIN_2000   0x01
-#define ACPI_OSI_WIN_XP 0x02
-#define ACPI_OSI_WIN_XP_SP1 0x03
-#define ACPI_OSI_WINSRV_20030x04
-#define ACPI_OSI_WIN_XP_SP2 0x05
-#define ACPI_OSI_WINSRV_2003_SP10x06
-#define ACPI_OSI_WIN_VISTA  0x07
-#define ACPI_OSI_WINSRV_20080x08
-#define ACPI_OSI_WIN_VISTA_SP1  0x09
-#define ACPI_OSI_WIN_VISTA_SP2  0x0A
-#define ACPI_OSI_WIN_7  0x0B
-#define ACPI_OSI_WIN_8  0x0C
-
 #define ACPI_ALWAYS_ILLEGAL 0x00

 struct acpi_interface_info {
diff --git a/drivers/acpi/acpica/utxface.c b/drivers/acpi/acpica/utxface.c
index 6505774..c1638c3 100644
--- a/drivers/acpi/acpica/utxface.c
+++ b/drivers/acpi/acpica/utxface.c
@@ -389,6 +389,25 @@ ACPI_EXPORT_SYMBOL(acpi_install_interface_handler)

 /*
  *
+ * FUNCTION:acpi_osi_version
+ *
+ * PARAMETERS:  None
+ *
+ * RETURN:  Last OS version requested via _OSI
+ *
+ * DESCRIPTION: Returns the argument to the most recent _OSI query performed
+ * by the firmware
+ *
+ /
+u8 acpi_osi_version(void)
+{
+   return acpi_gbl_osi_data;
+}
+
+ACPI_EXPORT_SYMBOL(acpi_osi_version)
+
+/*
+ *
  * FUNCTION:acpi_check_address_range
  *
  * PARAMETERS:  space_id- Address space ID
diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
index 454881e..41d3ac1 100644
--- a/include/acpi/acpixf.h
+++ b/include/acpi/acpixf.h
@@ -82,6 +82,22 @@ extern u8 acpi_gbl_truncate_io_addresses;
 extern u8 acpi_gbl_disable_auto_repair;

 /*
+ * Values returned by acpi_osi_version()
+ */
+#define ACPI_OSI_WIN_2000   0x01
+#define ACPI_OSI_WIN_XP 0x02
+#define ACPI_OSI_WIN_XP_SP1 0x03
+#define ACPI_OSI_WINSRV_20030x04
+#define ACPI_OSI_WIN_XP_SP2 0x05
+#define ACPI_OSI_WINSRV_2003_SP10x06
+#define ACPI_OSI_WIN_VISTA  0x07
+#define ACPI_OSI_WINSRV_20080x08
+#define ACPI_OSI_WIN_VISTA_SP1  0x09
+#define ACPI_OSI_WIN_VISTA_SP2  0x0A
+#define ACPI_OSI_WIN_7  0x0B
+#define ACPI_OSI_WIN_8  0x0C
+
+/*
  * Hardware-reduced prototypes. All interfaces that use these macros will
  * be configured out of the ACPICA build if the ACPI_REDUCED_HARDWARE flag
  * is set to TRUE.
@@ -307,6 +323,12 @@ acpi_status acpi_install_notify_handler(acpi_handle 
device, u32 handler_type,
acpi_notify_handler handler,
void *context);

+#ifdef CONFIG_ACPI
+u8 acpi_osi_version(void);
+#else
+static inline u8 acpi_osi_version(void) { return 0; }
+#endif
+
 acpi_status
 acpi_remove_notify_handler(acpi_handle device,
   u32 handler_type, acpi_notify_handler handler);
-- 
1.8.2.1

[PATCH 1/3] acpi: video: add function to support unregister backlight interface

[Matthew Garrett]

From: "Lee, Chun-Yi" 

There have some situation we unregister whole acpi/video driver by downstream
driver just want to remove backlight control interface of acpi/video. It caues
we lost other functions of acpi/video, e.g. transfer acpi event to input event.

So, this patch add a new function, find_video_unregister_backlight, it provide
the interface let downstream driver can tell acpi/video to unregister backlight
interface of all acpi video devices. Then we can keep functions of acpi/video
but only remove backlight support.

Reference: bko#35622
https://bugzilla.kernel.org/show_bug.cgi?id=35622

v2: Also unregister cooling devices.

Tested-by: Andrzej Krentosz 
Cc: Zhang Rui 
Cc: Len Brown 
Cc: Rafael J. Wysocki 
Cc: Carlos Corbacho 
Cc: Matthew Garrett 
Cc: Dmitry Torokhov 
Cc: Corentin Chary 
Cc: Aaron Lu 
Cc: Thomas Renninger 
Signed-off-by: Lee, Chun-Yi 
---
 drivers/acpi/video.c | 93 
 include/acpi/video.h |  2 ++
 2 files changed, 89 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 5b32e15..da08ff7 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -43,6 +43,7 @@
 #include 
 #include 
 #include 
+#include 

 #define PREFIX "ACPI: "

@@ -86,6 +87,9 @@ module_param(allow_duplicates, bool, 0644);
 static bool use_bios_initial_backlight = 1;
 module_param(use_bios_initial_backlight, bool, 0644);

+static bool backlight_disable;
+module_param(backlight_disable, bool, 0644);
+
 static int register_count = 0;
 static int acpi_video_bus_add(struct acpi_device *device);
 static int acpi_video_bus_remove(struct acpi_device *device);
@@ -214,6 +218,8 @@ static const char device_decode[][30] = {
"UNKNOWN",
 };

+static DEFINE_MUTEX(backlight_mutex);
+
 static void acpi_video_device_notify(acpi_handle handle, u32 event, void 
*data);
 static void acpi_video_device_rebind(struct acpi_video_bus *video);
 static void acpi_video_device_bind(struct acpi_video_bus *video,
@@ -882,7 +888,7 @@ static void acpi_video_device_find_cap(struct 
acpi_video_device *device)
device->cap._DDC = 1;
}

-   if (acpi_video_backlight_support()) {
+   if (acpi_video_backlight_support() || backlight_disable) {
struct backlight_properties props;
struct pci_dev *pdev;
acpi_handle acpi_parent;
@@ -1083,15 +1089,22 @@ acpi_video_bus_get_one_device(struct acpi_device 
*device,
struct acpi_video_device *data;
struct acpi_video_device_attrib* attribute;

+   mutex_lock(&backlight_mutex);
+
status =
acpi_evaluate_integer(device->handle, "_ADR", NULL, &device_id);
/* Some device omits _ADR, we skip them instead of fail */
-   if (ACPI_FAILURE(status))
-   return 0;
+   if (ACPI_FAILURE(status)) {
+   status = 0;
+   goto out;
+   }

data = kzalloc(sizeof(struct acpi_video_device), GFP_KERNEL);
-   if (!data)
-   return -ENOMEM;
+   if (!data) {
+   status = -ENOMEM;
+   goto out;
+   }
+

strcpy(acpi_device_name(device), ACPI_VIDEO_DEVICE_NAME);
strcpy(acpi_device_class(device), ACPI_VIDEO_CLASS);
@@ -1156,6 +1169,8 @@ acpi_video_bus_get_one_device(struct acpi_device *device,
list_add_tail(&data->entry, &video->video_device_list);
mutex_unlock(&video->device_list_lock);

+out:
+   mutex_unlock(&backlight_mutex);
return status;
 }

@@ -1336,7 +1351,7 @@ acpi_video_switch_brightness(struct acpi_video_device 
*device, int event)
int result = -EINVAL;

/* no warning message if acpi_backlight=vendor is used */
-   if (!acpi_video_backlight_support())
+   if (!acpi_video_backlight_support() || backlight_disable)
return 0;

if (!device->brightness)
@@ -1869,6 +1884,72 @@ static int __init intel_opregion_present(void)
return opregion;
 }

+static acpi_status
+find_video_unregister_backlight(acpi_handle handle, u32 lvl, void *context,
+   void **rv)
+{
+   struct acpi_device *acpi_dev;
+   struct acpi_video_bus *video;
+   struct acpi_video_device *dev, *next;
+   acpi_status status = AE_OK;
+
+   mutex_lock(&backlight_mutex);
+
+   if (acpi_bus_get_device(handle, &acpi_dev))
+   goto out;
+
+   if (!acpi_match_device_ids(acpi_dev, video_device_ids)) {
+   video = acpi_driver_data(acpi_dev);
+
+   if (!video)
+   goto out;
+
+   acpi_video_bus_stop_devices(video);
+   mutex_lock(&video->device_list_lock);
+   list_for_each_entry_safe(dev, next, &video->video_device_list,
+   entry) {
+   

[PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Matthew Garrett]

Windows 8 introduced new policy for backlight control by pushing it out to
graphics drivers. This appears to have coincided with a range of vendors
adding Windows 8 checks to their backlight control code which trigger either
awkward behaviour (Lenovo) or complete brokenness (some Dells). The simplest
thing to do would be to just disable ACPI backlight control entirely if the
firmware indicates Windows 8 support, but it's entirely possible that
individual graphics drivers might still make use of the ACPI functionality in
preference to native control.

The first two patches in this series are picked from other patchesets aimed at
solving similar problems. The last simply unregisters ACPI backlight control
on Windows 8 systems when using an Intel GPU. Similar code could be added to
other drivers, but I'm reluctant to do so without further investigation as
to the behaviour of the vendor drivers under Windows.

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

[PATCH 1/3] acpi: video: add function to support unregister backlight interface

[Matthew Garrett]

From: "Lee, Chun-Yi" 

There have some situation we unregister whole acpi/video driver by downstream
driver just want to remove backlight control interface of acpi/video. It caues
we lost other functions of acpi/video, e.g. transfer acpi event to input event.

So, this patch add a new function, find_video_unregister_backlight, it provide
the interface let downstream driver can tell acpi/video to unregister backlight
interface of all acpi video devices. Then we can keep functions of acpi/video
but only remove backlight support.

Reference: bko#35622
https://bugzilla.kernel.org/show_bug.cgi?id=35622

v2: Also unregister cooling devices.

Tested-by: Andrzej Krentosz 
Cc: Zhang Rui 
Cc: Len Brown 
Cc: Rafael J. Wysocki 
Cc: Carlos Corbacho 
Cc: Matthew Garrett 
Cc: Dmitry Torokhov 
Cc: Corentin Chary 
Cc: Aaron Lu 
Cc: Thomas Renninger 
Signed-off-by: Lee, Chun-Yi 
---
 drivers/acpi/video.c | 93 
 include/acpi/video.h |  2 ++
 2 files changed, 89 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 5b32e15..da08ff7 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -43,6 +43,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #define PREFIX "ACPI: "
 
@@ -86,6 +87,9 @@ module_param(allow_duplicates, bool, 0644);
 static bool use_bios_initial_backlight = 1;
 module_param(use_bios_initial_backlight, bool, 0644);
 
+static bool backlight_disable;
+module_param(backlight_disable, bool, 0644);
+
 static int register_count = 0;
 static int acpi_video_bus_add(struct acpi_device *device);
 static int acpi_video_bus_remove(struct acpi_device *device);
@@ -214,6 +218,8 @@ static const char device_decode[][30] = {
"UNKNOWN",
 };
 
+static DEFINE_MUTEX(backlight_mutex);
+
 static void acpi_video_device_notify(acpi_handle handle, u32 event, void 
*data);
 static void acpi_video_device_rebind(struct acpi_video_bus *video);
 static void acpi_video_device_bind(struct acpi_video_bus *video,
@@ -882,7 +888,7 @@ static void acpi_video_device_find_cap(struct 
acpi_video_device *device)
device->cap._DDC = 1;
}
 
-   if (acpi_video_backlight_support()) {
+   if (acpi_video_backlight_support() || backlight_disable) {
struct backlight_properties props;
struct pci_dev *pdev;
acpi_handle acpi_parent;
@@ -1083,15 +1089,22 @@ acpi_video_bus_get_one_device(struct acpi_device 
*device,
struct acpi_video_device *data;
struct acpi_video_device_attrib* attribute;
 
+   mutex_lock(&backlight_mutex);
+
status =
acpi_evaluate_integer(device->handle, "_ADR", NULL, &device_id);
/* Some device omits _ADR, we skip them instead of fail */
-   if (ACPI_FAILURE(status))
-   return 0;
+   if (ACPI_FAILURE(status)) {
+   status = 0;
+   goto out;
+   }
 
data = kzalloc(sizeof(struct acpi_video_device), GFP_KERNEL);
-   if (!data)
-   return -ENOMEM;
+   if (!data) {
+   status = -ENOMEM;
+   goto out;
+   }
+
 
strcpy(acpi_device_name(device), ACPI_VIDEO_DEVICE_NAME);
strcpy(acpi_device_class(device), ACPI_VIDEO_CLASS);
@@ -1156,6 +1169,8 @@ acpi_video_bus_get_one_device(struct acpi_device *device,
list_add_tail(&data->entry, &video->video_device_list);
mutex_unlock(&video->device_list_lock);
 
+out:
+   mutex_unlock(&backlight_mutex);
return status;
 }
 
@@ -1336,7 +1351,7 @@ acpi_video_switch_brightness(struct acpi_video_device 
*device, int event)
int result = -EINVAL;
 
/* no warning message if acpi_backlight=vendor is used */
-   if (!acpi_video_backlight_support())
+   if (!acpi_video_backlight_support() || backlight_disable)
return 0;
 
if (!device->brightness)
@@ -1869,6 +1884,72 @@ static int __init intel_opregion_present(void)
return opregion;
 }
 
+static acpi_status
+find_video_unregister_backlight(acpi_handle handle, u32 lvl, void *context,
+   void **rv)
+{
+   struct acpi_device *acpi_dev;
+   struct acpi_video_bus *video;
+   struct acpi_video_device *dev, *next;
+   acpi_status status = AE_OK;
+
+   mutex_lock(&backlight_mutex);
+
+   if (acpi_bus_get_device(handle, &acpi_dev))
+   goto out;
+
+   if (!acpi_match_device_ids(acpi_dev, video_device_ids)) {
+   video = acpi_driver_data(acpi_dev);
+
+   if (!video)
+   goto out;
+
+   acpi_video_bus_stop_devices(video);
+   mutex_lock(&video->device_list_lock);
+   list_for_each_entry_safe(dev, next, &video->video_device_list,
+   entry) {
+   

[PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Matthew Garrett]

Windows 8 introduced new policy for backlight control by pushing it out to
graphics drivers. This appears to have coincided with a range of vendors
adding Windows 8 checks to their backlight control code which trigger either
awkward behaviour (Lenovo) or complete brokenness (some Dells). The simplest
thing to do would be to just disable ACPI backlight control entirely if the
firmware indicates Windows 8 support, but it's entirely possible that
individual graphics drivers might still make use of the ACPI functionality in
preference to native control.

The first two patches in this series are picked from other patchesets aimed at
solving similar problems. The last simply unregisters ACPI backlight control
on Windows 8 systems when using an Intel GPU. Similar code could be added to
other drivers, but I'm reluctant to do so without further investigation as
to the behaviour of the vendor drivers under Windows.

-- 
Matthew Garrett | mj...@srcf.ucam.org

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] i915: Don't provide ACPI backlight interface if firmware expects Windows 8

[Matthew Garrett]

Windows 8 leaves backlight control up to individual graphics drivers rather
than making ACPI calls itself. There's plenty of evidence to suggest that
the Intel driver for Windows doesn't use the ACPI interface, including the
fact that it's broken on a bunch of machines when the OS claims to support
Windows 8. The simplest thing to do appears to be to disable the ACPI
backlight interface on these systems.

Signed-off-by: Matthew Garrett 
---
 drivers/gpu/drm/i915/i915_dma.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 3b315ba..23b6292 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1661,6 +1661,9 @@ int i915_driver_load(struct drm_device *dev, unsigned 
long flags)
/* Must be done after probing outputs */
intel_opregion_init(dev);
acpi_video_register();
+   /* Don't use ACPI backlight functions on Windows 8 platforms */
+   if (acpi_osi_version() >= ACPI_OSI_WIN_8)
+   acpi_video_backlight_unregister();
}
 
if (IS_GEN5(dev))
-- 
1.8.2.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] ACPICA: Add interface for getting latest OS version requested via _OSI

[Matthew Garrett]

Drivers may need to make policy decisions based on the OS that the firmware
believes it's interacting with. ACPI firmware will make a series of _OSI
calls, starting from the oldest OS version they support and ending with the
most recent. Add a function to return the last successful call so that
drivers know what the firmware's expecting.

Based on a patch by Seth Forshee 

Signed-off-by: Matthew Garrett 
Cc: Seth Forshee 
---
 drivers/acpi/acpica/aclocal.h | 13 -
 drivers/acpi/acpica/utxface.c | 19 +++
 include/acpi/acpixf.h | 22 ++
 3 files changed, 41 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/acpica/aclocal.h b/drivers/acpi/acpica/aclocal.h
index d5bfbd3..8a2f532 100644
--- a/drivers/acpi/acpica/aclocal.h
+++ b/drivers/acpi/acpica/aclocal.h
@@ -948,19 +948,6 @@ struct acpi_bit_register_info {
 
 /* Structs and definitions for _OSI support and I/O port validation */
 
-#define ACPI_OSI_WIN_2000   0x01
-#define ACPI_OSI_WIN_XP 0x02
-#define ACPI_OSI_WIN_XP_SP1 0x03
-#define ACPI_OSI_WINSRV_20030x04
-#define ACPI_OSI_WIN_XP_SP2 0x05
-#define ACPI_OSI_WINSRV_2003_SP10x06
-#define ACPI_OSI_WIN_VISTA  0x07
-#define ACPI_OSI_WINSRV_20080x08
-#define ACPI_OSI_WIN_VISTA_SP1  0x09
-#define ACPI_OSI_WIN_VISTA_SP2  0x0A
-#define ACPI_OSI_WIN_7  0x0B
-#define ACPI_OSI_WIN_8  0x0C
-
 #define ACPI_ALWAYS_ILLEGAL 0x00
 
 struct acpi_interface_info {
diff --git a/drivers/acpi/acpica/utxface.c b/drivers/acpi/acpica/utxface.c
index 6505774..c1638c3 100644
--- a/drivers/acpi/acpica/utxface.c
+++ b/drivers/acpi/acpica/utxface.c
@@ -389,6 +389,25 @@ ACPI_EXPORT_SYMBOL(acpi_install_interface_handler)
 
 /*
  *
+ * FUNCTION:acpi_osi_version
+ *
+ * PARAMETERS:  None
+ *
+ * RETURN:  Last OS version requested via _OSI
+ *
+ * DESCRIPTION: Returns the argument to the most recent _OSI query performed
+ * by the firmware
+ *
+ /
+u8 acpi_osi_version(void)
+{
+   return acpi_gbl_osi_data;
+}
+
+ACPI_EXPORT_SYMBOL(acpi_osi_version)
+
+/*
+ *
  * FUNCTION:acpi_check_address_range
  *
  * PARAMETERS:  space_id- Address space ID
diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
index 454881e..41d3ac1 100644
--- a/include/acpi/acpixf.h
+++ b/include/acpi/acpixf.h
@@ -82,6 +82,22 @@ extern u8 acpi_gbl_truncate_io_addresses;
 extern u8 acpi_gbl_disable_auto_repair;
 
 /*
+ * Values returned by acpi_osi_version()
+ */
+#define ACPI_OSI_WIN_2000   0x01
+#define ACPI_OSI_WIN_XP 0x02
+#define ACPI_OSI_WIN_XP_SP1 0x03
+#define ACPI_OSI_WINSRV_20030x04
+#define ACPI_OSI_WIN_XP_SP2 0x05
+#define ACPI_OSI_WINSRV_2003_SP10x06
+#define ACPI_OSI_WIN_VISTA  0x07
+#define ACPI_OSI_WINSRV_20080x08
+#define ACPI_OSI_WIN_VISTA_SP1  0x09
+#define ACPI_OSI_WIN_VISTA_SP2  0x0A
+#define ACPI_OSI_WIN_7  0x0B
+#define ACPI_OSI_WIN_8  0x0C
+
+/*
  * Hardware-reduced prototypes. All interfaces that use these macros will
  * be configured out of the ACPICA build if the ACPI_REDUCED_HARDWARE flag
  * is set to TRUE.
@@ -307,6 +323,12 @@ acpi_status acpi_install_notify_handler(acpi_handle 
device, u32 handler_type,
acpi_notify_handler handler,
void *context);
 
+#ifdef CONFIG_ACPI
+u8 acpi_osi_version(void);
+#else
+static inline u8 acpi_osi_version(void) { return 0; }
+#endif
+
 acpi_status
 acpi_remove_notify_handler(acpi_handle device,
   u32 handler_type, acpi_notify_handler handler);
-- 
1.8.2.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/radeon: narrow scope of Apple re-POST hack

[Matthew Garrett]

On Wed, May 29, 2013 at 12:24:25PM -0400, alexdeucher at gmail.com wrote:
> From: Alex Deucher 
> 
> This narrows the scope of the apple re-POST hack added in:
> drm/radeon: re-POST the asic on Apple hardware when booted via EFI
> 
> That patch prevents UVD from working on macs when booted in EFI
> mode.  The original patch fixed macbook2,1 systems which were
> r5xx and hence have no UVD.  Limit the hack to those systems to
> prevent UVD breakage on newer systems.
> 
> Fixes:
> https://bugs.freedesktop.org/show_bug.cgi?id=63935
> 
> Cc: Matthew Garrett 
Acked-by: Matthew Garrett 

There's some potential for this to break some other Apple systems, but 
let's try it and see.

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

Re: [PATCH] drm/radeon: narrow scope of Apple re-POST hack

[Matthew Garrett]

On Wed, May 29, 2013 at 12:24:25PM -0400, alexdeuc...@gmail.com wrote:
> From: Alex Deucher 
> 
> This narrows the scope of the apple re-POST hack added in:
> drm/radeon: re-POST the asic on Apple hardware when booted via EFI
> 
> That patch prevents UVD from working on macs when booted in EFI
> mode.  The original patch fixed macbook2,1 systems which were
> r5xx and hence have no UVD.  Limit the hack to those systems to
> prevent UVD breakage on newer systems.
> 
> Fixes:
> https://bugs.freedesktop.org/show_bug.cgi?id=63935
> 
> Cc: Matthew Garrett 
Acked-by: Matthew Garrett 

There's some potential for this to break some other Apple systems, but 
let's try it and see.

-- 
Matthew Garrett | mj...@srcf.ucam.org
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 0/1] drm/i915: Allow specifying a minimum brightness level for sysfs control.

[Matthew Garrett]

On Wed, Mar 27, 2013 at 12:56:37PM +0100, Danny Baumann wrote:

> OK, I see. And there is user space depending on that behaviour? And
> again - how is user space supposed to know about the behavioral
> differences? Is it something like 'if type is raw, don't expect
> anything'?

"Do not rely upon 0 being visible".

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

Re: [PATCH 0/1] drm/i915: Allow specifying a minimum brightness level for sysfs control.

[Matthew Garrett]

On Wed, Mar 27, 2013 at 12:56:37PM +0100, Danny Baumann wrote:

> OK, I see. And there is user space depending on that behaviour? And
> again - how is user space supposed to know about the behavioral
> differences? Is it something like 'if type is raw, don't expect
> anything'?

"Do not rely upon 0 being visible".

-- 
Matthew Garrett | mj...@srcf.ucam.org
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 0/1] drm/i915: Allow specifying a minimum brightness level for sysfs control.

[Matthew Garrett]

On Tue, Mar 26, 2013 at 06:10:30PM +0100, Danny Baumann wrote:
> Am 26.03.2013 18:02, schrieb Matthew Garrett:
> >I'm not quite clear what you mean here. The behaviour of "0" isn't well
> >defined for the ACPI backlight driver - it's perfectly reasonable for it
> >to turn the backlight off entirely. Anything assuming that "0" is still
> >visible is broken.
> 
> Well, the ACPI spec says this (section B.5.2):
> 
> "
> The OEM may define the number 0 as "Zero brightness" that can mean
> to turn off the lighting (e.g. LCD panel backlight) in the device.
> This may be useful in the case of an output device that can still be
> viewed using only ambient light, for example, a transflective LCD.
> "
> 
> My interpretation of this is that the value 0 is supposed to still
> be visible. I'm pretty sure I saw a statement that 0 is supposed to
> mean "barely visible" somewhere, but can't find it at the moment.
> I'll search for the source of it.

I think that's a stretch - "This may be useful" isn't normative 
language, "The OEM may define" is. But even if we do assert it for the 
ACPI backlight, it's not true for other interfaces - zero backlight 
intensity is supposed to be screen off on Apple hardware, for instance.

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

[PATCH 0/1] drm/i915: Allow specifying a minimum brightness level for sysfs control.

[Matthew Garrett]

On Tue, Mar 26, 2013 at 12:48:44PM +0100, Danny Baumann wrote:
> This patch makes the behaviour of the intel_backlight backlight device
> consistent to e.g. acpi_videoX: When writing the value 0, the set brightness
> makes the panel content barely readable instead of turning the backlight off.
> This matches the expectations of user space (e.g. kde-workspace or the Intel
> X11 driver), which expects that it can use intel_backlight as a drop-in
> replacement for acpi_videoX.

I'm not quite clear what you mean here. The behaviour of "0" isn't well 
defined for the ACPI backlight driver - it's perfectly reasonable for it 
to turn the backlight off entirely. Anything assuming that "0" is still 
visible is broken.

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

Re: [PATCH 0/1] drm/i915: Allow specifying a minimum brightness level for sysfs control.

[Matthew Garrett]

On Tue, Mar 26, 2013 at 12:48:44PM +0100, Danny Baumann wrote:
> This patch makes the behaviour of the intel_backlight backlight device
> consistent to e.g. acpi_videoX: When writing the value 0, the set brightness
> makes the panel content barely readable instead of turning the backlight off.
> This matches the expectations of user space (e.g. kde-workspace or the Intel
> X11 driver), which expects that it can use intel_backlight as a drop-in
> replacement for acpi_videoX.

I'm not quite clear what you mean here. The behaviour of "0" isn't well 
defined for the ACPI backlight driver - it's perfectly reasonable for it 
to turn the backlight off entirely. Anything assuming that "0" is still 
visible is broken.

-- 
Matthew Garrett | mj...@srcf.ucam.org
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/1] drm/i915: Allow specifying a minimum brightness level for sysfs control.

[Matthew Garrett]

On Tue, Mar 26, 2013 at 06:10:30PM +0100, Danny Baumann wrote:
> Am 26.03.2013 18:02, schrieb Matthew Garrett:
> >I'm not quite clear what you mean here. The behaviour of "0" isn't well
> >defined for the ACPI backlight driver - it's perfectly reasonable for it
> >to turn the backlight off entirely. Anything assuming that "0" is still
> >visible is broken.
> 
> Well, the ACPI spec says this (section B.5.2):
> 
> "
> The OEM may define the number 0 as "Zero brightness" that can mean
> to turn off the lighting (e.g. LCD panel backlight) in the device.
> This may be useful in the case of an output device that can still be
> viewed using only ambient light, for example, a transflective LCD.
> "
> 
> My interpretation of this is that the value 0 is supposed to still
> be visible. I'm pretty sure I saw a statement that 0 is supposed to
> mean "barely visible" somewhere, but can't find it at the moment.
> I'll search for the source of it.

I think that's a stretch - "This may be useful" isn't normative 
language, "The OEM may define" is. But even if we do assert it for the 
ACPI backlight, it's not true for other interfaces - zero backlight 
intensity is supposed to be screen off on Apple hardware, for instance.

-- 
Matthew Garrett | mj...@srcf.ucam.org
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] i915: Quirk out disconnected backlight

[Matthew Garrett]

On Fri, Sep 14, 2012 at 03:29:14PM +0100, David Woodhouse wrote:
> On Fri, 2012-09-14 at 14:48 +0100, Matthew Garrett wrote:
> > On Fri, Sep 14, 2012 at 01:57:06PM +0100, Grant Likely wrote:
> > 
> > > Tested on MacbookPro8,3. Without this patch both the intel_backlight and
> > > gmux_backlight devices get registered and userspace doesn't know which
> > > it should use.
> > 
> > Userspace should be figuring out which one to use from the type field.
> 
> It only does that if it's using gsd-backlight-helper to poke
> at /sys/class/backlight directly. If X exposes a backlight, (as it does
> for the Intel backlight), then gsd will just use that.

Yeah, X should be doing the same. If it's not then it's broken. OTOH, I 
do agree that if we already know that we can't do anything with the 
backlight (as is clearly the case if the PWM field is 0) we should just 
disable it.

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

[PATCH] i915: Quirk out disconnected backlight

[Matthew Garrett]

On Fri, Sep 14, 2012 at 01:57:06PM +0100, Grant Likely wrote:

> Tested on MacbookPro8,3. Without this patch both the intel_backlight and
> gmux_backlight devices get registered and userspace doesn't know which
> it should use.

Userspace should be figuring out which one to use from the type field.

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

Re: [PATCH] i915: Quirk out disconnected backlight

[Matthew Garrett]

On Fri, Sep 14, 2012 at 03:29:14PM +0100, David Woodhouse wrote:
> On Fri, 2012-09-14 at 14:48 +0100, Matthew Garrett wrote:
> > On Fri, Sep 14, 2012 at 01:57:06PM +0100, Grant Likely wrote:
> > 
> > > Tested on MacbookPro8,3. Without this patch both the intel_backlight and
> > > gmux_backlight devices get registered and userspace doesn't know which
> > > it should use.
> > 
> > Userspace should be figuring out which one to use from the type field.
> 
> It only does that if it's using gsd-backlight-helper to poke
> at /sys/class/backlight directly. If X exposes a backlight, (as it does
> for the Intel backlight), then gsd will just use that.

Yeah, X should be doing the same. If it's not then it's broken. OTOH, I 
do agree that if we already know that we can't do anything with the 
backlight (as is clearly the case if the PWM field is 0) we should just 
disable it.

-- 
Matthew Garrett | mj...@srcf.ucam.org
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] i915: Quirk out disconnected backlight

[Matthew Garrett]

On Fri, Sep 14, 2012 at 01:57:06PM +0100, Grant Likely wrote:

> Tested on MacbookPro8,3. Without this patch both the intel_backlight and
> gmux_backlight devices get registered and userspace doesn't know which
> it should use.

Userspace should be figuring out which one to use from the type field.

-- 
Matthew Garrett | mj...@srcf.ucam.org
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] ACPI: export symbol acpi_get_table_with_size

[Matthew Garrett]

-- 
Matthew Garrett | mj...@srcf.ucam.org
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/4] ACPI: export symbol acpi_get_table_with_size

[Matthew Garrett]

On Tue, Aug 21, 2012 at 10:50:35AM -0400, Alex Deucher wrote:
> Any objections from the ACPI folks to this patch going into 3.6 and stable?

Looks good to me.

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

[PATCH 2/4] ACPI: export symbol acpi_get_table_with_size

[Matthew Garrett]

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

Re: [PATCH 2/4] ACPI: export symbol acpi_get_table_with_size

[Matthew Garrett]

On Tue, Aug 21, 2012 at 10:50:35AM -0400, Alex Deucher wrote:
> Any objections from the ACPI folks to this patch going into 3.6 and stable?

Looks good to me.

-- 
Matthew Garrett | mj...@srcf.ucam.org
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH 7/7] drm/pci: Defer initialization of secondary graphics devices until switcheroo is ready

[Matthew Garrett]

On Mon, Aug 20, 2012 at 11:24:44AM -0500, Seth Forshee wrote:

> I'm not sure how we support both of these cases without doing something
> more like what I originally proposed, i.e. registering the LVDS
> connector even if it doesn't look like a panel is attached. I still
> honestly favor that approach, although it does come with its own set of
> challenges.
> 
> The only other option I can come up with is to reprobe LVDS after
> switcheroo and add the connector at that time. I haven't investigated
> this option in detail, but at first glance it looks like there are at
> least some places where DRM isn't prepared to cope with adding
> connectors after initialization.

Well, one option is to identify whether the hardware is switcheroo 
capable without the use of the switcheroo driver. It looks like we can 
do that in the majority of cases - Apple is the only special case I can 
see, and that one's a fairly easy workaround.

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

[RFC PATCH 7/7] drm/pci: Defer initialization of secondary graphics devices until switcheroo is ready

[Matthew Garrett]

On Mon, Aug 20, 2012 at 10:56:33AM -0500, Seth Forshee wrote:
> On Mon, Aug 20, 2012 at 04:36:40PM +0100, Matthew Garrett wrote:
> > Won't this break the multiple cards with independent outputs case?
> 
> Yes, if they don't have a switcheroo handler. I only have experience
> with one such machine, which had optimus graphics. My recollection is
> that it did have a switcheroo handler, which was only capable of
> controlling power to the discrete card.

So if I have a desktop machine and install two graphics cards?

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

[RFC PATCH 7/7] drm/pci: Defer initialization of secondary graphics devices until switcheroo is ready

[Matthew Garrett]

On Mon, Aug 20, 2012 at 10:31:04AM -0500, Seth Forshee wrote:
> + /*
> +  * For secondary graphics devices shouldn't be initialized
> +  * until the handler and primary graphics device have been
> +  * registered with vga_switcheroo.
> +  *
> +  * FIXME: Is vga_default_device() reliable enough for this
> +  * purpose?
> +  *
> +  * FIXME: If vga_switcheroo is disabled secondary devices
> +  * never gets initialized. Is this okay? Maybe it is, since
> +  * we can't switch to the secondary GPU anyway.
> +  */

Won't this break the multiple cards with independent outputs case?

-- 
Matthew Garrett | mjg59 at srcf.ucam.org

Re: [RFC PATCH 7/7] drm/pci: Defer initialization of secondary graphics devices until switcheroo is ready

[Matthew Garrett]

On Mon, Aug 20, 2012 at 11:24:44AM -0500, Seth Forshee wrote:

> I'm not sure how we support both of these cases without doing something
> more like what I originally proposed, i.e. registering the LVDS
> connector even if it doesn't look like a panel is attached. I still
> honestly favor that approach, although it does come with its own set of
> challenges.
> 
> The only other option I can come up with is to reprobe LVDS after
> switcheroo and add the connector at that time. I haven't investigated
> this option in detail, but at first glance it looks like there are at
> least some places where DRM isn't prepared to cope with adding
> connectors after initialization.

Well, one option is to identify whether the hardware is switcheroo 
capable without the use of the switcheroo driver. It looks like we can 
do that in the majority of cases - Apple is the only special case I can 
see, and that one's a fairly easy workaround.

-- 
Matthew Garrett | mj...@srcf.ucam.org
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 7/7] drm/pci: Defer initialization of secondary graphics devices until switcheroo is ready

[Matthew Garrett]

On Mon, Aug 20, 2012 at 10:56:33AM -0500, Seth Forshee wrote:
> On Mon, Aug 20, 2012 at 04:36:40PM +0100, Matthew Garrett wrote:
> > Won't this break the multiple cards with independent outputs case?
> 
> Yes, if they don't have a switcheroo handler. I only have experience
> with one such machine, which had optimus graphics. My recollection is
> that it did have a switcheroo handler, which was only capable of
> controlling power to the discrete card.

So if I have a desktop machine and install two graphics cards?

-- 
Matthew Garrett | mj...@srcf.ucam.org
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 7/7] drm/pci: Defer initialization of secondary graphics devices until switcheroo is ready

[Matthew Garrett]

On Mon, Aug 20, 2012 at 10:31:04AM -0500, Seth Forshee wrote:
> + /*
> +  * For secondary graphics devices shouldn't be initialized
> +  * until the handler and primary graphics device have been
> +  * registered with vga_switcheroo.
> +  *
> +  * FIXME: Is vga_default_device() reliable enough for this
> +  * purpose?
> +  *
> +  * FIXME: If vga_switcheroo is disabled secondary devices
> +  * never gets initialized. Is this okay? Maybe it is, since
> +  * we can't switch to the secondary GPU anyway.
> +  */

Won't this break the multiple cards with independent outputs case?

-- 
Matthew Garrett | mj...@srcf.ucam.org
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel