| Message ID | 1364298525-4337-2-git-send-email-dannybaumann@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 26, 2013 at 12:48:45PM +0100, Danny Baumann wrote: > When controlling backlight devices via sysfs interface, user space > generally assumes the minimum level (0) still providing a brightness > that is usable by the user (probably due to the most commonly used > backlight device, acpi_videoX, behaving exactly like that). This doesn't > match the current behaviour of the intel_backlight control, though, as > the value 0 means 'backlight off'. > > In order to make intel_backlight consistent to other backlight devices, > introduce a module parameter that allows shifting the 0 level of the > intel_backlight device. It's expressed in percentages of the maximum > level. The default is 5, which provides a backlight level that is barely > readable. Setting it to 0 restores the old behaviour. > > Signed-off-by: Danny Baumann <dannybaumann@web.de> Thus far our assumption always was that the acpi backlight works better than the intel native backlight. So everything only uses the intel backlight if there's no other backlight driver by default. So if I should merge this as a general solution for Windows 8 machines not working properly, we first need to figure out what windows does on these machines and either disable the acpi backlight or adapt it. Adding more kernel options is not a viable solution for the backlight mess imo. -Daniel > --- > drivers/gpu/drm/i915/intel_panel.c | 48 ++++++++++++++++++++++++++++++++------ > 1 file changed, 41 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > index bee8cb6..5bad49d 100644 > --- a/drivers/gpu/drm/i915/intel_panel.c > +++ b/drivers/gpu/drm/i915/intel_panel.c > @@ -395,10 +395,35 @@ intel_panel_detect(struct drm_device *dev) > } > > #ifdef CONFIG_BACKLIGHT_CLASS_DEVICE > + > +static int i915_panel_min_brightness_percent = 5; > +MODULE_PARM_DESC(min_sysfs_brightness, "Minimum brightness percentage settable " > + "via sysfs. This adjusts the brightness level of the value '0' in the " > + "intel_backlight sysfs backlight interface."); > +module_param_named(min_sysfs_brightness, > + i915_panel_min_brightness_percent, int, 0400); > + > +static int intel_panel_min_brightness(struct drm_device *dev) > +{ > + int max; > + > + if (i915_panel_min_brightness_percent <= 0) > + return 0; > + > + max = intel_panel_get_max_backlight(dev); > + if (i915_panel_min_brightness_percent >= 100) > + return max; > + > + return max * i915_panel_min_brightness_percent / 100; > +} > + > static int intel_panel_update_status(struct backlight_device *bd) > { > struct drm_device *dev = bl_get_data(bd); > - intel_panel_set_backlight(dev, bd->props.brightness); > + int brightness = > + bd->props.brightness + intel_panel_min_brightness(dev); > + > + intel_panel_set_backlight(dev, brightness); > return 0; > } > > @@ -406,7 +431,10 @@ static int intel_panel_get_brightness(struct backlight_device *bd) > { > struct drm_device *dev = bl_get_data(bd); > struct drm_i915_private *dev_priv = dev->dev_private; > - return dev_priv->backlight_level; > + int brightness = > + dev_priv->backlight_level - intel_panel_min_brightness(dev); > + > + return brightness; > } > > static const struct backlight_ops intel_panel_bl_ops = { > @@ -419,16 +447,21 @@ int intel_panel_setup_backlight(struct drm_connector *connector) > struct drm_device *dev = connector->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > struct backlight_properties props; > + int max_brightness; > > intel_panel_init_backlight(dev); > > - memset(&props, 0, sizeof(props)); > - props.type = BACKLIGHT_RAW; > - props.max_brightness = _intel_panel_get_max_backlight(dev); > - if (props.max_brightness == 0) { > + max_brightness = _intel_panel_get_max_backlight(dev); > + if (max_brightness == 0) { > DRM_DEBUG_DRIVER("Failed to get maximum backlight value\n"); > return -ENODEV; > } > + > + memset(&props, 0, sizeof(props)); > + props.type = BACKLIGHT_RAW; > + props.max_brightness = > + max_brightness - intel_panel_min_brightness(dev); > + > dev_priv->backlight = > backlight_device_register("intel_backlight", > &connector->kdev, dev, > @@ -440,7 +473,8 @@ int intel_panel_setup_backlight(struct drm_connector *connector) > dev_priv->backlight = NULL; > return -ENODEV; > } > - dev_priv->backlight->props.brightness = intel_panel_get_backlight(dev); > + dev_priv->backlight->props.brightness = intel_panel_get_backlight(dev) > + - intel_panel_min_brightness(dev); > return 0; > } > > -- > 1.8.1.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Mar 26, 2013 at 04:13:13PM +0100, Daniel Vetter wrote: > On Tue, Mar 26, 2013 at 12:48:45PM +0100, Danny Baumann wrote: > > When controlling backlight devices via sysfs interface, user space > > generally assumes the minimum level (0) still providing a brightness > > that is usable by the user (probably due to the most commonly used > > backlight device, acpi_videoX, behaving exactly like that). This doesn't > > match the current behaviour of the intel_backlight control, though, as > > the value 0 means 'backlight off'. > > > > In order to make intel_backlight consistent to other backlight devices, > > introduce a module parameter that allows shifting the 0 level of the > > intel_backlight device. It's expressed in percentages of the maximum > > level. The default is 5, which provides a backlight level that is barely > > readable. Setting it to 0 restores the old behaviour. > > > > Signed-off-by: Danny Baumann <dannybaumann@web.de> > > Thus far our assumption always was that the acpi backlight works better > than the intel native backlight. So everything only uses the intel > backlight if there's no other backlight driver by default. There are machines, such as the pnv netbook on my desk, on which intel_backlight does nothing and the only control is through acpi_backlight0. Generalising expected behaviour based on one firmware implementation is fraught with danger. -Chris
Hi, > Thus far our assumption always was that the acpi backlight works better > than the intel native backlight. So everything only uses the intel > backlight if there's no other backlight driver by default. > > So if I should merge this as a general solution for Windows 8 machines not > working properly, we first need to figure out what windows does on these > machines and either disable the acpi backlight or adapt it. I fully agree to that. Making acpi_video control usable is preferable over using intel_backlight. As I lack the detail knowledge about the ACPI video stuff, I'd be great of one (or some) of you guys could look at the mentioned bug report ([1]) and comment on what the problem might be. I have added the basic needed information already and would be happy to provide any needed debugging info. > Adding more kernel options is not a viable solution for the backlight mess > imo. Actually I'm fine with hardcoding the percentage as well ;) I just figured it might make sense to make it controllable for special-case uses, while still making intel_backlight usable for the 'normal' use case. Regards, Danny [1] https://bugzilla.kernel.org/show_bug.cgi?id=55071
Hi, >> Thus far our assumption always was that the acpi backlight works better >> than the intel native backlight. So everything only uses the intel >> backlight if there's no other backlight driver by default. > > There are machines, such as the pnv netbook on my desk, on which > intel_backlight does nothing and the only control is through > acpi_backlight0. That's fine - but on my machine, (at least currently) it's the other way around. acpi_video[0|1] do nothing, while intel_backlight is the only method that works. This sucks (please also see my reply to Daniel), but it's a fact. > Generalising expected behaviour based on one firmware > implementation is fraught with danger. I'm not sure what you mean here. I interpret the statement as 'user space should treat acpi_videoX and intel_backlight differently'. Is this interpretation correct? If so, how is user space supposed to know how the respective backlight device will behave, as this behaviour might change at any point in time if there's no understanding in how it _should_ behave? What currently happens on my machine is that KDE completely turns off the backlight after the dim timeout, because it assumes that the value 0 will keep the backlight at a readable level (which would be the case if it used acpi_videoX because this behaviour is mandated by the spec there). I first thought of sending a patch to KDE, but I couldn't figure out how KDE is _supposed_ to correctly handle the situation, especially given that the actual sysfs interface used is abstracted away by Xrandr (and chosen by the Intel Xorg driver). With my patch, intel_backlight behaves in a way that roughly matches the behaviour mandated by the ACPI spec. Do you have a way in mind that allows fixing this in user space? Thanks, Danny
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index bee8cb6..5bad49d 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -395,10 +395,35 @@ intel_panel_detect(struct drm_device *dev) } #ifdef CONFIG_BACKLIGHT_CLASS_DEVICE + +static int i915_panel_min_brightness_percent = 5; +MODULE_PARM_DESC(min_sysfs_brightness, "Minimum brightness percentage settable " + "via sysfs. This adjusts the brightness level of the value '0' in the " + "intel_backlight sysfs backlight interface."); +module_param_named(min_sysfs_brightness, + i915_panel_min_brightness_percent, int, 0400); + +static int intel_panel_min_brightness(struct drm_device *dev) +{ + int max; + + if (i915_panel_min_brightness_percent <= 0) + return 0; + + max = intel_panel_get_max_backlight(dev); + if (i915_panel_min_brightness_percent >= 100) + return max; + + return max * i915_panel_min_brightness_percent / 100; +} + static int intel_panel_update_status(struct backlight_device *bd) { struct drm_device *dev = bl_get_data(bd); - intel_panel_set_backlight(dev, bd->props.brightness); + int brightness = + bd->props.brightness + intel_panel_min_brightness(dev); + + intel_panel_set_backlight(dev, brightness); return 0; } @@ -406,7 +431,10 @@ static int intel_panel_get_brightness(struct backlight_device *bd) { struct drm_device *dev = bl_get_data(bd); struct drm_i915_private *dev_priv = dev->dev_private; - return dev_priv->backlight_level; + int brightness = + dev_priv->backlight_level - intel_panel_min_brightness(dev); + + return brightness; } static const struct backlight_ops intel_panel_bl_ops = { @@ -419,16 +447,21 @@ int intel_panel_setup_backlight(struct drm_connector *connector) struct drm_device *dev = connector->dev; struct drm_i915_private *dev_priv = dev->dev_private; struct backlight_properties props; + int max_brightness; intel_panel_init_backlight(dev); - memset(&props, 0, sizeof(props)); - props.type = BACKLIGHT_RAW; - props.max_brightness = _intel_panel_get_max_backlight(dev); - if (props.max_brightness == 0) { + max_brightness = _intel_panel_get_max_backlight(dev); + if (max_brightness == 0) { DRM_DEBUG_DRIVER("Failed to get maximum backlight value\n"); return -ENODEV; } + + memset(&props, 0, sizeof(props)); + props.type = BACKLIGHT_RAW; + props.max_brightness = + max_brightness - intel_panel_min_brightness(dev); + dev_priv->backlight = backlight_device_register("intel_backlight", &connector->kdev, dev, @@ -440,7 +473,8 @@ int intel_panel_setup_backlight(struct drm_connector *connector) dev_priv->backlight = NULL; return -ENODEV; } - dev_priv->backlight->props.brightness = intel_panel_get_backlight(dev); + dev_priv->backlight->props.brightness = intel_panel_get_backlight(dev) + - intel_panel_min_brightness(dev); return 0; }
When controlling backlight devices via sysfs interface, user space generally assumes the minimum level (0) still providing a brightness that is usable by the user (probably due to the most commonly used backlight device, acpi_videoX, behaving exactly like that). This doesn't match the current behaviour of the intel_backlight control, though, as the value 0 means 'backlight off'. In order to make intel_backlight consistent to other backlight devices, introduce a module parameter that allows shifting the 0 level of the intel_backlight device. It's expressed in percentages of the maximum level. The default is 5, which provides a backlight level that is barely readable. Setting it to 0 restores the old behaviour. Signed-off-by: Danny Baumann <dannybaumann@web.de> --- drivers/gpu/drm/i915/intel_panel.c | 48 ++++++++++++++++++++++++++++++++------ 1 file changed, 41 insertions(+), 7 deletions(-)