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

Rafael J. Wysocki
2013-07-06T13:23:23Z

On Saturday, July 06, 2013 01:45:56 PM Aaron Lu wrote:
> On 07/06/2013 06:23 AM, Rafael J. Wysocki wrote:
> > On Friday, July 05, 2013 11:40:02 PM Rafael J. Wysocki wrote:
> >> On Friday, July 05, 2013 10:00:55 PM Rafael J. Wysocki wrote:
> >>> On Friday, July 05, 2013 02:20:14 PM Rafael J. Wysocki wrote:
> >>>> On Sunday, June 09, 2013 07:01:39 PM Matthew Garrett wrote:
> >>>>> 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 <matthew.garrett@nebula.com>
> >>>>> ---
> >>>>> 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))
> >>>>>
> >>>>
> >>>> Well, this causes build failures to happen when the ACPI video driver is
> >>>> modular and the graphics driver is not.
> >>>>
> >>>> I'm not sure how to resolve that, so suggestions are welcome.
> >>>
> >>> Actually, that happened with the radeon patch.
> >>>
> >>> That said, ACPI_OSI_WIN_8 doesn't make much sense for !CONFIG_ACPI, for
> >>> example.
> >>>
> >>> What about making acpi_video_register() do the quirk instead? We could add an
> >>> argument to it indicating whether or not quirks should be applied.
> >>
> >> Actually, I wonder what about the appended patch (on top of the Aaron's
> >> https://patchwork.kernel.org/patch/2812951/) instead of [1-3/3] from this series.
> >
> > Or even something as simple as this one.
> >
> > ---
> > drivers/acpi/video_detect.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > Index: linux-pm/drivers/acpi/video_detect.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/video_detect.c
> > +++ linux-pm/drivers/acpi/video_detect.c
> > @@ -203,6 +203,9 @@ long acpi_video_get_capabilities(acpi_ha
> > */
> >
> > dmi_check_system(video_detect_dmi_table);
> > +
> > + if (acpi_gbl_osi_data >= ACPI_OSI_WIN_8)
> > + acpi_video_support |= ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR;
>
> Then vendor driver(thinkpad_acpi) will step in and create a backlight
> interface for the system, which, unfortunately, is also broken for those
> win8 thinkpads.
>
> So we will need to do something in thinkpad_acpi to also not create
> backlight interface for these systems too.
>
> This actually doesn't feel bad to me, since the modules are blacklisting
> their own interfaces. The downside is of course, two quirk code exist.
>
> BTW, unregistering ACPI video's backlight interface in GPU driver doesn't
> have this problem since it made the platform driver think the ACPI video
> driver will control the backlight and then use the newly added API to
> remove ACPI video created backlight interface.

Yes, I know this.

I think I also know how to introduce that change in a slightly cleaner way.

I'll post a patch for comments later today or tomorrow.

Thanks,
Rafael

--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.