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)
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
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
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
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.
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.
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:
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.
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 userspaceDoes that fix it for you?
Yes, thanks.
g.
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.
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
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.
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.
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.
dri-devel@lists.freedesktop.org
-
Chris Wilson -
David Woodhouse -
Grant Likely -
Jani Nikula -
Matthew Garrett