diff mbox

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

Message ID 2962350.laXfENfrac@vostro.rjw.lan (mailing list archive)
State New, archived
Headers show

Commit Message

Rafael Wysocki July 5, 2013, 10:23 p.m. UTC 
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(+)

Comments

Aaron Lu July 6, 2013, 5:45 a.m. UTC | #1 
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.

Thanks,
Aaron

>  	} else {
>  		status = acpi_bus_get_device(graphics_handle, &tmp_dev);
>  		if (ACPI_FAILURE(status)) {
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Rafael Wysocki July 6, 2013, 1:33 p.m. UTC | #2 
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
diff mbox

Patch

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;
 	} else {
 		status = acpi_bus_get_device(graphics_handle, &tmp_dev);
 		if (ACPI_FAILURE(status)) {