[PATCH] i915: Quirk out disconnected backlight

[PATCH] i915: Quirk out disconnected backlight

[Grant Likely]

Some platforms (for instance MacbookPros) have custom backlight drivers
and don't use the integrated i915 backlight control. This patch adds a
quirk to disable registering the intel backlight when unused on a
platform.

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.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: David Airlie <airlied@linux.ie>
Cc: Matthew Garrett <mjg@redhat.com>
Cc: David Woodhouse <dwmw2@infradead.org>
---
 drivers/gpu/drm/i915/i915_drv.h      |    1 +
 drivers/gpu/drm/i915/intel_display.c |   10 ++++++++++
 drivers/gpu/drm/i915/intel_panel.c   |    3 +++
 3 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 627fe35..48860a0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -349,6 +349,7 @@ enum intel_pch {
 #define QUIRK_PIPEA_FORCE (1<<0)
 #define QUIRK_LVDS_SSC_DISABLE (1<<1)
 #define QUIRK_INVERT_BRIGHTNESS (1<<2)
+#define QUIRK_NO_BACKLIGHT (2<<2)
 
 struct intel_fbdev;
 struct intel_fbc_work;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2dfa6cf..c8153cd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7088,6 +7088,13 @@ static void quirk_invert_brightness(struct drm_device *dev)
 	DRM_INFO("applying inverted panel brightness quirk\n");
 }
 
+static void quirk_no_backlight(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	dev_priv->quirks |= QUIRK_NO_BACKLIGHT;
+	DRM_INFO("applying no backlight quirk\n");
+}
+
 struct intel_quirk {
 	int device;
 	int subsystem_vendor;
@@ -7123,6 +7130,9 @@ static struct intel_quirk intel_quirks[] = {
 
 	/* Acer Aspire 5734Z must invert backlight brightness */
 	{ 0x2a42, 0x1025, 0x0459, quirk_invert_brightness },
+
+	/* Apple MacbookPro8,3 doesn't have a backlight */
+	{ 0x0126, 0x106b, 0x00de, quirk_no_backlight },
 };
 
 static void intel_init_quirks(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 3df4f5f..f116e2a 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -413,6 +413,9 @@ int intel_panel_setup_backlight(struct drm_device *dev)
 	struct backlight_properties props;
 	struct drm_connector *connector;
 
+	if (dev_priv->quirks & QUIRK_NO_BACKLIGHT)
+		return 0;
+
 	intel_panel_init_backlight(dev);
 
 	if (dev_priv->int_lvds_connector)
-- 
1.7.9.5

Re: [PATCH] i915: Quirk out disconnected backlight

[Chris Wilson]

On Fri, 14 Sep 2012 13:57:06 +0100, Grant Likely <grant.likely@secretlab.ca> wrote:
> Some platforms (for instance MacbookPros) have custom backlight drivers
> and don't use the integrated i915 backlight control. This patch adds a
> quirk to disable registering the intel backlight when unused on a
> platform.
> 
> 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 is informed throught the backlight/type property.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] i915: Quirk out disconnected backlight

[Grant Likely]

On Fri, Sep 14, 2012 at 2:01 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Fri, 14 Sep 2012 13:57:06 +0100, Grant Likely <grant.likely@secretlab.ca> wrote:
>> Some platforms (for instance MacbookPros) have custom backlight drivers
>> and don't use the integrated i915 backlight control. This patch adds a
>> quirk to disable registering the intel backlight when unused on a
>> platform.
>>
>> 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 is informed throught the backlight/type property.

Perhaps, but userspace (Ubuntu) isn't doing anything with it, and it
still remains that it makes no sense whatsoever to register a
backlight device that doesn't exist.

g.

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] i915: Quirk out disconnected backlight

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1283 bytes --]

On Fri, 2012-09-14 at 14:09 +0100, Grant Likely wrote:
> On Fri, Sep 14, 2012 at 2:01 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Fri, 14 Sep 2012 13:57:06 +0100, Grant Likely <grant.likely@secretlab.ca> wrote:
> >> Some platforms (for instance MacbookPros) have custom backlight drivers
> >> and don't use the integrated i915 backlight control. This patch adds a
> >> quirk to disable registering the intel backlight when unused on a
> >> platform.
> >>
> >> 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 is informed throught the backlight/type property.
> 
> Perhaps, but userspace (Ubuntu) isn't doing anything with it, and it
> still remains that it makes no sense whatsoever to register a
> backlight device that doesn't exist. 

Indeed. Userspace (well, gnome-settings-daemon) will use the backlight
provided by X, in preference to anything it finds
in /sys/class/backlight. So if the Intel one is present (and thus
exposed via X) then userspace will never bother with comparing types and
choosing the sanest backlight to use.

See https://bugzilla.redhat.com/show_bug.cgi?id=752595

-- 
dwmw2

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: [PATCH] i915: Quirk out disconnected backlight

[Grant Likely]

On Fri, Sep 14, 2012 at 2:12 PM, David Woodhouse <dwmw2@infradead.org> wrote:
> On Fri, 2012-09-14 at 14:09 +0100, Grant Likely wrote:
>> On Fri, Sep 14, 2012 at 2:01 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > On Fri, 14 Sep 2012 13:57:06 +0100, Grant Likely <grant.likely@secretlab.ca> wrote:
>> >> Some platforms (for instance MacbookPros) have custom backlight drivers
>> >> and don't use the integrated i915 backlight control. This patch adds a
>> >> quirk to disable registering the intel backlight when unused on a
>> >> platform.
>> >>
>> >> 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 is informed throught the backlight/type property.
>>
>> Perhaps, but userspace (Ubuntu) isn't doing anything with it, and it
>> still remains that it makes no sense whatsoever to register a
>> backlight device that doesn't exist.
>
> Indeed. Userspace (well, gnome-settings-daemon) will use the backlight
> provided by X, in preference to anything it finds
> in /sys/class/backlight. So if the Intel one is present (and thus
> exposed via X) then userspace will never bother with comparing types and
> choosing the sanest backlight to use.
>
> See https://bugzilla.redhat.com/show_bug.cgi?id=752595

In that bug you mention that the intel backlight sets a bogus max of
'1' when a backlight isn't present. I saw that too here. Here's the
offending code:

u32 intel_panel_get_max_backlight(struct drm_device *dev)
{
        struct drm_i915_private *dev_priv = dev->dev_private;
        u32 max;

        max = i915_read_blc_pwm_ctl(dev_priv);
        if (max == 0) {
                /* XXX add code here to query mode clock or hardware clock
                 * and program max PWM appropriately.
                 */
                pr_warn_once("fixme: max PWM is zero\n");
                return 1;
        }

I used a quirk in my patch, but I could instead change the driver to
bail here instead of trying to limp along.

g.

Re: [PATCH] i915: Quirk out disconnected backlight

[Chris Wilson]

On Fri, 14 Sep 2012 14:12:19 +0100, David Woodhouse <dwmw2@infradead.org> wrote:
> On Fri, 2012-09-14 at 14:09 +0100, Grant Likely wrote:
> > On Fri, Sep 14, 2012 at 2:01 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > On Fri, 14 Sep 2012 13:57:06 +0100, Grant Likely <grant.likely@secretlab.ca> wrote:
> > >> Some platforms (for instance MacbookPros) have custom backlight drivers
> > >> and don't use the integrated i915 backlight control. This patch adds a
> > >> quirk to disable registering the intel backlight when unused on a
> > >> platform.
> > >>
> > >> 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 is informed throught the backlight/type property.
> > 
> > Perhaps, but userspace (Ubuntu) isn't doing anything with it, and it
> > still remains that it makes no sense whatsoever to register a
> > backlight device that doesn't exist. 
> 
> Indeed. Userspace (well, gnome-settings-daemon) will use the backlight
> provided by X, in preference to anything it finds
> in /sys/class/backlight. So if the Intel one is present (and thus
> exposed via X) then userspace will never bother with comparing types and
> choosing the sanest backlight to use.
> 
> See https://bugzilla.redhat.com/show_bug.cgi?id=752595

And if you look at that bug, it starts off by complaining that a
workaround is required in order to use the intel_backlight. By the time
you hit the issue with apple_gmux, the upstream ddx already carried the
fix for a couple of months and even had it in a release. And more
recently we took a patch to allow the user to override which backlight is
preferred to handle the case of a broken platform/firmware interface
being selected instead of intel_backlight.

Userspace is indeed trying to do the right thing with the information
provided by the kernel. apple_gmux is not the only device with a phantom
PWM BLC, which is why the default preference is to use the
platform/firmware provided backlight interface, if any.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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 | mjg59@srcf.ucam.org

Re: [PATCH] i915: Quirk out disconnected backlight

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 587 bytes --]

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.

-- 
dwmw2

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

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 | mjg59@srcf.ucam.org

Re: [PATCH] i915: Quirk out disconnected backlight

[Jani Nikula]

On Fri, 14 Sep 2012, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Fri, Sep 14, 2012 at 2:12 PM, David Woodhouse <dwmw2@infradead.org> wrote:
>> See https://bugzilla.redhat.com/show_bug.cgi?id=752595
>
> In that bug you mention that the intel backlight sets a bogus max of
> '1' when a backlight isn't present. I saw that too here. Here's the
> offending code:
>
> u32 intel_panel_get_max_backlight(struct drm_device *dev)
> {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         u32 max;
>
>         max = i915_read_blc_pwm_ctl(dev_priv);
>         if (max == 0) {
>                 /* XXX add code here to query mode clock or hardware clock
>                  * and program max PWM appropriately.
>                  */
>                 pr_warn_once("fixme: max PWM is zero\n");
>                 return 1;
>         }
>
> I used a quirk in my patch, but I could instead change the driver to
> bail here instead of trying to limp along.

Hi Grant, please try v3.6-rc6 that does exactly that with:

commit 28dcc2d60cb570d9f549c329b2f51400553412a1
Author: Jani Nikula <jani.nikula@intel.com>
Date:   Mon Sep 3 16:25:12 2012 +0300

    drm/i915: do not expose a dysfunctional backlight interface to userspace

Does that fix it for you?


BR,
Jani.

Re: [PATCH] i915: Quirk out disconnected backlight

[Grant Likely]

On Mon, Sep 17, 2012 at 9:03 AM, Jani Nikula
<jani.nikula@linux.intel.com> wrote:
> Hi Grant, please try v3.6-rc6 that does exactly that with:
>
> commit 28dcc2d60cb570d9f549c329b2f51400553412a1
> Author: Jani Nikula <jani.nikula@intel.com>
> Date:   Mon Sep 3 16:25:12 2012 +0300
>
>     drm/i915: do not expose a dysfunctional backlight interface to userspace
>
> Does that fix it for you?

Yes, thanks.

g.