| Message ID | 20090817165743.28ac3e40@jbarnes-g45 (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Mon, Aug 17, 2009 at 04:57:43PM -0700, Jesse Barnes wrote: > +static int intel_get_backlight_brightness(struct backlight_device *bd) > +{ > + return bd->props.brightness; > +} This is what's called when the user reads actual_brightness, so should actually read the hardware rather than just returning the cached value. > + /* FIXME: check whether /sys/class/backlight is populated or not */ Calling acpi_video_backlight_support() will give you this for acpi. It's more complicated for the platform driver case, since that'll typically be loaded after us. We're going to need some sort of event generated and then unregister the drm backlight in response. > + backlight_device->props.max_brightness = > + dev_priv->blc_info.inverter_polarity ? 0 : 255; max_brightness is the maximum value that can be given to the interface, rather than the value at which the brightness is greatest. The polarity inversion stuff needs to be handled in the set function. I guess there's also an argument over what we do about the minimum being greater than 0 - we should probably scale to cope with that, otherwise UI is going to look weird.
On Tue, 18 Aug 2009 01:09:00 +0100 Matthew Garrett <mjg59@srcf.ucam.org> wrote: > On Mon, Aug 17, 2009 at 04:57:43PM -0700, Jesse Barnes wrote: > > > +static int intel_get_backlight_brightness(struct backlight_device > > *bd) +{ > > + return bd->props.brightness; > > +} > > This is what's called when the user reads actual_brightness, so > should actually read the hardware rather than just returning the > cached value. Ok. > > > + /* FIXME: check whether /sys/class/backlight is populated > > or not */ > > Calling acpi_video_backlight_support() will give you this for acpi. > It's more complicated for the platform driver case, since that'll > typically be loaded after us. We're going to need some sort of event > generated and then unregister the drm backlight in response. Ok cool, maybe the backlight class needs a notifier? (I think you talked about this awhile back...) > > > + backlight_device->props.max_brightness = > > + dev_priv->blc_info.inverter_polarity ? 0 : 255; > > max_brightness is the maximum value that can be given to the > interface, rather than the value at which the brightness is greatest. > The polarity inversion stuff needs to be handled in the set function. > I guess there's also an argument over what we do about the minimum > being greater than 0 Oh duh, yeah I probably would have noticed that had I dug out a test platform. ;) > - we should probably scale to cope with that, otherwise UI is going > to look weird. The VBIOS handles it by making anything below the min brightness equivalent to "lowest", which is probably fine. Scaling would work too though. Also, the PWM write is wrong for a range of 0-255, so I need some scaling anyway...
On Mon, Aug 17, 2009 at 05:52:28PM -0700, Jesse Barnes wrote: > > - we should probably scale to cope with that, otherwise UI is going > > to look weird. > > The VBIOS handles it by making anything below the min brightness > equivalent to "lowest", which is probably fine. Scaling would work too > though. Also, the PWM write is wrong for a range of 0-255, so I need > some scaling anyway... It seems that HAL / gnome-display-manager, or something in that area, wants to see only 8 different levels, otherwise you need to write a custom script for it to use. So be aware of this, I just changed the samsung-backlight driver to use 0 - 7 to keep from having to do this. thanks, greg k-h
On Tue, 2009-08-18 at 07:57 +0800, Jesse Barnes wrote: > On Mon, 17 Aug 2009 21:32:45 +0100 > Matthew Garrett <mjg59@srcf.ucam.org> wrote: > > > On Mon, Aug 17, 2009 at 10:09:09AM -0700, Greg KH wrote: > > > On Mon, Aug 17, 2009 at 06:52:37PM +0200, Matthias Hopf wrote: > > > > > So you want 2 different drivers controlling the same hardware? > > > That's a recipe for disaster :) > > > > samsung-laptop (in its current form) is controlling the same hardware > > that i915 is already. Implementing it properly involves parsing the > > BIOS tables to tell you the minimum acceptable value (driving the > > backlight controller outside its specced range probably isn't a good > > idea) and potentially also touching the MMIO backlight registers. I'd > > really recommend not merging this as a separate driver. It's slightly > > more work to do it in the drm driver, but it's also the only way to > > do it properly. > > Yeah, I mentioned that in another thread. This patch (untested) > illustrates the sort of thing I'd rather see. It integrates the > backlight detection and sysfs support into the i915 driver (as part of > the existing LVDS code), which should make it more reliable in the face > of the various machine types. It still needs to detect whether a > backlight device has been registered already, and the i2c code still > needs to be written (should be easy, but I'm not sure if I have a test > platform for it). IMO it is unnecessary to get such a complex interface. In fact this interface is useful only when there is neither acpi generic nor platform driver interface. At the same time the backlight section in VBIOS is not reliable. For example: On one box the length of backlight section is 21 and the lvds_panel_type is 13. In such case it is beyond the backlight section. Another issue is that the brightness level is so big when the native/combo mode is used. For example: 0-65535 for the native control method. The user will be confused by so many brightness levels. How about using the legacy control method? Thanks. > > -- > Jesse Barnes, Intel Open Source Technology Center > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 7537f57..2adad14 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -149,6 +149,11 @@ struct drm_i915_error_state { > struct timeval time; > }; > > +struct blc_funcs { > + int (*get)(struct drm_device *dev); > + void (*set)(struct drm_device *dev, int level); > +}; > + > typedef struct drm_i915_private { > struct drm_device *dev; > > @@ -212,6 +217,9 @@ typedef struct drm_i915_private { > struct drm_display_mode *panel_fixed_mode; > struct drm_display_mode *lfp_lvds_vbt_mode; /* if any */ > struct drm_display_mode *sdvo_lvds_vbt_mode; /* if any */ > + struct blc_struct blc_info; > + struct blc_funcs *backlight; > + struct backlight_device *backlight_device; /* for sysfs */ > > /* Feature bits from the VBIOS */ > unsigned int int_tv_support:1; > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c > index 300aee3..afc042b 100644 > --- a/drivers/gpu/drm/i915/intel_bios.c > +++ b/drivers/gpu/drm/i915/intel_bios.c > @@ -315,6 +315,27 @@ parse_driver_features(struct drm_i915_private *dev_priv, > dev_priv->edp_support = 1; > } > > +static void > +parse_lvds_backlight(struct drm_i915_private *dev_priv, struct bdb_header *bdb) > +{ > + struct bdb_lvds_options *lvds_options; > + struct bdb_lvds_backlight *backlight; > + int entry; > + > + lvds_options = find_section(bdb, BDB_LVDS_OPTIONS); > + backlight = find_section(bdb, BDB_LVDS_BACKLIGHT); > + if (!lvds_options || !backlight) > + return; > + > + /* Make sure we have a compatible layout */ > + if (backlight->blcstruct_size != sizeof(struct blc_struct)) > + return; > + > + entry = lvds_options->panel_type; > + memcpy(&dev_priv->blc_info, &backlight->panels[entry], > + sizeof(struct blc_struct)); > +} > + > /** > * intel_init_bios - initialize VBIOS settings & find VBT > * @dev: DRM device > @@ -366,6 +387,7 @@ intel_init_bios(struct drm_device *dev) > parse_sdvo_panel_data(dev_priv, bdb); > parse_sdvo_device_mapping(dev_priv, bdb); > parse_driver_features(dev_priv, bdb); > + parse_lvds_backlight(dev_priv, bdb); > > pci_unmap_rom(pdev, bios); > > diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h > index 0f8e5f6..5145151 100644 > --- a/drivers/gpu/drm/i915/intel_bios.h > +++ b/drivers/gpu/drm/i915/intel_bios.h > @@ -380,6 +380,48 @@ struct bdb_sdvo_lvds_options { > u8 panel_misc_bits_4; > } __attribute__((packed)); > > +#define BLC_INVERTER_TYPE_NONE 0 > +#define BLC_INVERTER_TYPE_I2C 1 > +#define BLC_INVERTER_TYPE_PWM 2 > + > +#define BLC_GPIO_NONE 0 > +#define BLC_GPIO_I2C 1 > +#define BLC_GPIO_CRT_DDC 2 > +#define BLC_GPIO_DVI_DDC 3 > +#define BLC_GPIO_SDVO_I2C 5 > + > +#define BLC_GMBUS_100KHZ 0 > +#define BLC_GMBUS_50KHZ 1 > +#define BLC_GMBUS_400KHZ 2 > +#define BLC_GMBUS_1MHZ 3 > + > +struct blc_struct { > + u8 inverter_type:2; > + u8 inverter_polarity:1; /* 1 means inverted (0 = max brightness) */ > + u8 gpio_pins:3; > + u8 gmbus_speed:2; > + u16 pwm_freq; /* in Hz */ > + u8 min_brightness; /* (0-255) */ > + u8 i2c_slave_addr; > + u8 i2c_cmd; > +} __attribute__((packed)); > + > +struct bdb_lvds_backlight { > + u8 blcstruct_size; > + struct blc_struct panels[16]; > +} __attribute__((packed)); > + > +struct bdb_lvds_power { > + u8 dpst_enabled:1; > + u8 pwr_prefs:3; > + u8 rsvd1:3; > + u8 als_enabled:1; > + u16 als_backlight1; > + u16 als_backlight2; > + u16 als_backlight3; > + u16 als_backlight4; > + u16 als_backlight5; > +} __attribute__((packed)); > > #define BDB_DRIVER_FEATURE_NO_LVDS 0 > #define BDB_DRIVER_FEATURE_INT_LVDS 1 > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c > index 3f445a8..75237c1 100644 > --- a/drivers/gpu/drm/i915/intel_lvds.c > +++ b/drivers/gpu/drm/i915/intel_lvds.c > @@ -27,6 +27,7 @@ > * Jesse Barnes <jesse.barnes@intel.com> > */ > > +#include <linux/backlight.h> > #include <linux/dmi.h> > #include <linux/i2c.h> > #include "drmP.h" > @@ -60,7 +61,7 @@ struct intel_lvds_priv { > * > * \param level backlight level, from 0 to intel_lvds_get_max_backlight(). > */ > -static void intel_lvds_set_backlight(struct drm_device *dev, int level) > +static void blc_pwm_set(struct drm_device *dev, int level) > { > struct drm_i915_private *dev_priv = dev->dev_private; > u32 blc_pwm_ctl, reg; > @@ -75,6 +76,30 @@ static void intel_lvds_set_backlight(struct drm_device *dev, int level) > (level << BACKLIGHT_DUTY_CYCLE_SHIFT))); > } > > +static int blc_pwm_get(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + u32 blc_pwm_ctl, reg; > + > + if (IS_IGDNG(dev)) > + reg = BLC_PWM_CPU_CTL; > + else > + reg = BLC_PWM_CTL; > + > + blc_pwm_ctl = I915_READ(reg) & ~BACKLIGHT_DUTY_CYCLE_MASK; > + > + return blc_pwm_ctl; > +} > + > +static void blc_i2c_set(struct drm_device *dev, int level) > +{ > +} > + > +static int blc_i2c_get(struct drm_device *dev) > +{ > + return 0; > +} > + > /** > * Returns the maximum level of the backlight duty cycle field. > */ > @@ -114,11 +139,7 @@ static void intel_lvds_set_power(struct drm_device *dev, bool on) > do { > pp_status = I915_READ(status_reg); > } while ((pp_status & PP_ON) == 0); > - > - intel_lvds_set_backlight(dev, dev_priv->backlight_duty_cycle); > } else { > - intel_lvds_set_backlight(dev, 0); > - > I915_WRITE(ctl_reg, I915_READ(ctl_reg) & > ~POWER_TARGET_ON); > do { > @@ -652,7 +673,11 @@ static int intel_lvds_get_modes(struct drm_connector *connector) > static void intel_lvds_destroy(struct drm_connector *connector) > { > struct intel_output *intel_output = to_intel_output(connector); > + struct drm_device *dev = connector->dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > > + if (dev_priv->backlight_device) > + backlight_device_unregister(dev_priv->backlight_device); > if (intel_output->ddc_bus) > intel_i2c_destroy(intel_output->ddc_bus); > drm_sysfs_connector_remove(connector); > @@ -856,6 +881,75 @@ static int intel_lid_present(void) > } > #endif > > +static int intel_get_backlight_brightness(struct backlight_device *bd) > +{ > + return bd->props.brightness; > +} > + > +static int intel_update_backlight_status(struct backlight_device *bd) > +{ > + struct drm_device *dev = bl_get_data(bd); > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + /* FIXME: check for FB blanking */ > + dev_priv->backlight->set(dev, bd->props.brightness); > + return 0; > +} > + > +static struct backlight_ops intel_backlight_ops = { > + .get_brightness = intel_get_backlight_brightness, > + .update_status = intel_update_backlight_status, > +}; > + > +/** > + * intel_lvds_backlight_init - find & initialize backlight devices > + * @dev: DRM device > + * > + * If we find a backlight controller (i.e. the backlight controller type > + * isn't BLC_INVERTER_TYPE_NONE) we set it up here. This either means > + * a PWM attached controller or an I2C attached controller. Control of the > + * backlight is exposed through sysfs, unless an ACPI or other platform > + * device has already been registered. > + */ > +static void intel_lvds_backlight_init(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct backlight_device *backlight_device; > + > + /* FIXME: check whether /sys/class/backlight is populated or not */ > + > + switch (dev_priv->blc_info.inverter_type) { > + case BLC_INVERTER_TYPE_I2C: > + dev_priv->backlight->get = blc_i2c_get; > + dev_priv->backlight->set = blc_i2c_set; > + break; > + case BLC_INVERTER_TYPE_PWM: > + dev_priv->backlight->get = blc_pwm_get; > + dev_priv->backlight->set = blc_pwm_set; > + break; > + default: > + DRM_DEBUG("backlight inverter not available\n"); > + goto out; > + } > + > + backlight_device = backlight_device_register("i915", &dev->pdev->dev, dev, > + &intel_backlight_ops); > + if (IS_ERR(backlight_device)) { > + DRM_ERROR("failed to register backlight device\n"); > + goto out; > + } > + > + backlight_device->props.max_brightness = > + dev_priv->blc_info.inverter_polarity ? 0 : 255; > + backlight_device->props.brightness = dev_priv->backlight->get(dev); > + backlight_device->props.power = FB_BLANK_UNBLANK; > + backlight_update_status(backlight_device); > + > + dev_priv->backlight_device = backlight_device; > +out: > + return; > +} > + > /** > * intel_lvds_init - setup LVDS connectors on this device > * @dev: drm device > @@ -1009,6 +1103,8 @@ void intel_lvds_init(struct drm_device *dev) > if (!dev_priv->panel_fixed_mode) > goto failed; > > + intel_lvds_backlight_init(dev); > + > out: > if (IS_IGDNG(dev)) { > u32 pwm; > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Aug 17, 2009 at 05:52:28PM -0700, Jesse Barnes wrote: > The VBIOS handles it by making anything below the min brightness > equivalent to "lowest", which is probably fine. Scaling would work too > though. Also, the PWM write is wrong for a range of 0-255, so I need > some scaling anyway... The problem with having them all at "lowest" is that you'll get a bunch of "low" brightness levels in the UI that all result in the same brightness. It's not a technical issue but it looks kind of ugly, so I think we need to set the zero point to the lowest value and reduce max_brightness accordingly.
On Mon, Aug 17, 2009 at 06:37:02PM -0700, Greg KH wrote: > It seems that HAL / gnome-display-manager, or something in that area, > wants to see only 8 different levels, otherwise you need to write a > custom script for it to use. So be aware of this, I just changed the > samsung-backlight driver to use 0 - 7 to keep from having to do this. The generic backlight addon should cope fine - I've got 0-10 here, and nouveau exposes something like 2^16 (gpm chooses a sensible number of steps in that case). Are you running an ancient version of hal?
On Tue, Aug 18, 2009 at 10:00:43AM +0800, ykzhao wrote: > Another issue is that the brightness level is so big when the > native/combo mode is used. For example: 0-65535 for the native control > method. The user will be confused by so many brightness levels. There's no problem with this for userspace. We don't show every single value to the user, but it's helpful if the driver exposes everything it can do. > How about using the legacy control method? If the firmware touches the BLC registers and we just touch the legacy registers then things will become very confused.
On Tue, 2009-08-18 at 10:05 +0800, Matthew Garrett wrote: > On Tue, Aug 18, 2009 at 10:00:43AM +0800, ykzhao wrote: > > > Another issue is that the brightness level is so big when the > > native/combo mode is used. For example: 0-65535 for the native control > > method. The user will be confused by so many brightness levels. > > There's no problem with this for userspace. We don't show every single > value to the user, but it's helpful if the driver exposes everything it > can do. > > > How about using the legacy control method? > > If the firmware touches the BLC registers and we just touch the legacy > registers then things will become very confused. Yes. If the brightness can be adjusted through two different interfaces, there exists the inconsistence. So we had better expose only one backlight interface under /sys/class/backlight. Thanks. >
On Tue, Aug 18, 2009 at 10:13:37AM +0800, ykzhao wrote: > Yes. If the brightness can be adjusted through two different interfaces, > there exists the inconsistence. So we had better expose only one > backlight interface under /sys/class/backlight. Right. So we can't just use the LBC. We need to check whether the BLC registers are also in use.
On Tue, 18 Aug 2009 03:01:22 +0100 Matthew Garrett <mjg59@srcf.ucam.org> wrote: > On Mon, Aug 17, 2009 at 05:52:28PM -0700, Jesse Barnes wrote: > > > The VBIOS handles it by making anything below the min brightness > > equivalent to "lowest", which is probably fine. Scaling would work > > too though. Also, the PWM write is wrong for a range of 0-255, so > > I need some scaling anyway... > > The problem with having them all at "lowest" is that you'll get a > bunch of "low" brightness levels in the UI that all result in the > same brightness. It's not a technical issue but it looks kind of > ugly, so I think we need to set the zero point to the lowest value > and reduce max_brightness accordingly. Sure, easy enough... Range exposed to the user will always be 0-max but internally we'll have min-max or max-min where min can be nonzero.
On Tue, 18 Aug 2009 10:00:43 +0800 ykzhao <yakui.zhao@intel.com> wrote: > On Tue, 2009-08-18 at 07:57 +0800, Jesse Barnes wrote: > > On Mon, 17 Aug 2009 21:32:45 +0100 > > Matthew Garrett <mjg59@srcf.ucam.org> wrote: > > > > > On Mon, Aug 17, 2009 at 10:09:09AM -0700, Greg KH wrote: > > > > On Mon, Aug 17, 2009 at 06:52:37PM +0200, Matthias Hopf wrote: > > > > > > > So you want 2 different drivers controlling the same hardware? > > > > That's a recipe for disaster :) > > > > > > samsung-laptop (in its current form) is controlling the same > > > hardware that i915 is already. Implementing it properly involves > > > parsing the BIOS tables to tell you the minimum acceptable value > > > (driving the backlight controller outside its specced range > > > probably isn't a good idea) and potentially also touching the > > > MMIO backlight registers. I'd really recommend not merging this > > > as a separate driver. It's slightly more work to do it in the drm > > > driver, but it's also the only way to do it properly. > > > > Yeah, I mentioned that in another thread. This patch (untested) > > illustrates the sort of thing I'd rather see. It integrates the > > backlight detection and sysfs support into the i915 driver (as part > > of the existing LVDS code), which should make it more reliable in > > the face of the various machine types. It still needs to detect > > whether a backlight device has been registered already, and the i2c > > code still needs to be written (should be easy, but I'm not sure if > > I have a test platform for it). > IMO it is unnecessary to get such a complex interface. In fact this > interface is useful only when there is neither acpi generic nor > platform driver interface. The interface is simple: it's just a /sys/class/backlight dir just like with ACPI or platform drivers. Yes, it's only useful where one of those is missing (and therefore mostly old machines) but we should still try to get it right... > At the same time the backlight section in VBIOS is not reliable. For > example: On one box the length of backlight section is 21 and the > lvds_panel_type is 13. In such case it is beyond the backlight > section. On recent platforms or old ones? If the platform already supports ACPI or a platform method, then the VBIOS data can be wrong and things will still work... > Another issue is that the brightness level is so big when the > native/combo mode is used. For example: 0-65535 for the native control > method. The user will be confused by so many brightness levels. User won't see them directly, so there should be no confusion. Brightness is generally exposed as a slider or a series of steps with the brightness keys. > How about using the legacy control method? Yes, we have to modify the legacy registers in some cases. But modifying them exclusively would be a problem...
On Tue, Aug 18, 2009 at 03:03:46AM +0100, Matthew Garrett wrote: > On Mon, Aug 17, 2009 at 06:37:02PM -0700, Greg KH wrote: > > > It seems that HAL / gnome-display-manager, or something in that area, > > wants to see only 8 different levels, otherwise you need to write a > > custom script for it to use. So be aware of this, I just changed the > > samsung-backlight driver to use 0 - 7 to keep from having to do this. > > The generic backlight addon should cope fine - I've got 0-10 here, and > nouveau exposes something like 2^16 (gpm chooses a sensible number of > steps in that case). Are you running an ancient version of hal? I didn't think I was, whatever is in Moblin 2.0 these days :) thanks, greg k-h
On Mon, Aug 17, 2009 at 07:21:50PM -0700, Greg KH wrote: > > The generic backlight addon should cope fine - I've got 0-10 here, and > > nouveau exposes something like 2^16 (gpm chooses a sensible number of > > steps in that case). Are you running an ancient version of hal? > > I didn't think I was, whatever is in Moblin 2.0 these days :) Are they running some custom backlight control app that hardcodes it? Limiting the maximum brightness is definitely the wrong thing to do here - any userspace that's failing to cope is more broken than our desktop code generally is.
On Tue, 2009-08-18 at 10:20 +0800, Jesse Barnes wrote: > On Tue, 18 Aug 2009 10:00:43 +0800 > ykzhao <yakui.zhao@intel.com> wrote: > > > On Tue, 2009-08-18 at 07:57 +0800, Jesse Barnes wrote: > > > On Mon, 17 Aug 2009 21:32:45 +0100 > > > Matthew Garrett <mjg59@srcf.ucam.org> wrote: > > > > > > > On Mon, Aug 17, 2009 at 10:09:09AM -0700, Greg KH wrote: > > > > > On Mon, Aug 17, 2009 at 06:52:37PM +0200, Matthias Hopf wrote: > > > > > > > > > So you want 2 different drivers controlling the same hardware? > > > > > That's a recipe for disaster :) > > > > > > > > samsung-laptop (in its current form) is controlling the same > > > > hardware that i915 is already. Implementing it properly involves > > > > parsing the BIOS tables to tell you the minimum acceptable value > > > > (driving the backlight controller outside its specced range > > > > probably isn't a good idea) and potentially also touching the > > > > MMIO backlight registers. I'd really recommend not merging this > > > > as a separate driver. It's slightly more work to do it in the drm > > > > driver, but it's also the only way to do it properly. > > > > > > Yeah, I mentioned that in another thread. This patch (untested) > > > illustrates the sort of thing I'd rather see. It integrates the > > > backlight detection and sysfs support into the i915 driver (as part > > > of the existing LVDS code), which should make it more reliable in > > > the face of the various machine types. It still needs to detect > > > whether a backlight device has been registered already, and the i2c > > > code still needs to be written (should be easy, but I'm not sure if > > > I have a test platform for it). > > IMO it is unnecessary to get such a complex interface. In fact this > > interface is useful only when there is neither acpi generic nor > > platform driver interface. > > The interface is simple: it's just a /sys/class/backlight dir just like > with ACPI or platform drivers. Yes, it's only useful where one of > those is missing (and therefore mostly old machines) but we should > still try to get it right... > > > At the same time the backlight section in VBIOS is not reliable. For > > example: On one box the length of backlight section is 21 and the > > lvds_panel_type is 13. In such case it is beyond the backlight > > section. > > On recent platforms or old ones? If the platform already supports ACPI > or a platform method, then the VBIOS data can be wrong and things will > still work... The two boxes are based on the g4x platform. And the acpi backlight interface can work well on one box. And there is no acpi backlight interface on another box. In fact I check the vbios dump on several other boxes and the length of backlight section is 21. So IMO the backlight section is not reliable and we can't use it to parse the backlight interface. > > > Another issue is that the brightness level is so big when the > > native/combo mode is used. For example: 0-65535 for the native control > > method. The user will be confused by so many brightness levels. > > User won't see them directly, so there should be no confusion. > Brightness is generally exposed as a slider or a series of steps with > the brightness keys. But this interface can be found in /sys/class/backlight/*. Maybe it is used to adjust the brightness by user. In such case too many brightness levels are exposed to user. > > > How about using the legacy control method? > > Yes, we have to modify the legacy registers in some cases. But > modifying them exclusively would be a problem... How can we modify the legacy registers in the two different paths if we expose only one interface to user space? Thanks >
On Aug 18, 09 03:01:22 +0100, Matthew Garrett wrote: > On Mon, Aug 17, 2009 at 05:52:28PM -0700, Jesse Barnes wrote: > > The VBIOS handles it by making anything below the min brightness > > equivalent to "lowest", which is probably fine. Scaling would work too > > though. Also, the PWM write is wrong for a range of 0-255, so I need > > some scaling anyway... > > The problem with having them all at "lowest" is that you'll get a bunch > of "low" brightness levels in the UI that all result in the same > brightness. It's not a technical issue but it looks kind of ugly, so I > think we need to set the zero point to the lowest value and reduce > max_brightness accordingly. IMHO the sanest would be to define 1 to be the minimum brightness and 0 to switch the light off completely. Another 2 cents Matthias
On Tue, Aug 18, 2009 at 11:37:17AM +0200, Matthias Hopf wrote: > IMHO the sanest would be to define 1 to be the minimum brightness and 0 > to switch the light off completely. There's the "power" field in the backlight class to turn it on or off, but 0 isn't well defined in terms of whether it's off or minimum brightness.
On Aug 18, 09 12:06:08 +0100, Matthew Garrett wrote: > On Tue, Aug 18, 2009 at 11:37:17AM +0200, Matthias Hopf wrote: > > IMHO the sanest would be to define 1 to be the minimum brightness and 0 > > to switch the light off completely. > There's the "power" field in the backlight class to turn it on or off, > but 0 isn't well defined in terms of whether it's off or minimum > brightness. I tried to use that on an hp netbook here, but somehow this had no effect. Anyway, I'd like to use the "0 is off, 1 is minimum" semantics in the RandR properties - we'd probably have to do some magic in the driver. power is only allowed to have 0 and 1 as values? Matthias
On Tue, Aug 18, 2009 at 01:52:03PM +0200, Matthias Hopf wrote: > I tried to use that on an hp netbook here, but somehow this had no > effect. It'll depend what interface is in use - for instance, ACPI typically won't let you disable the backlight completely. > Anyway, I'd like to use the "0 is off, 1 is minimum" semantics in the > RandR properties - we'd probably have to do some magic in the driver. In that case you'll have to add 1 to the values when dealing with most /sys/class/backlight implementations. > power is only allowed to have 0 and 1 as values? According to the header files, 0 is on, 1-3 are DPMS states, 4 is fully off. See include/linux/fb.h - it's the FB_BLANK_* defines.
On Tue, Aug 18, 2009 at 03:29:51AM +0100, Matthew Garrett wrote: > On Mon, Aug 17, 2009 at 07:21:50PM -0700, Greg KH wrote: > > > The generic backlight addon should cope fine - I've got 0-10 here, and > > > nouveau exposes something like 2^16 (gpm chooses a sensible number of > > > steps in that case). Are you running an ancient version of hal? > > > > I didn't think I was, whatever is in Moblin 2.0 these days :) > > Are they running some custom backlight control app that hardcodes it? > Limiting the maximum brightness is definitely the wrong thing to do > here - any userspace that's failing to cope is more broken than our > desktop code generally is. I really don't know, I'll poke around today and try to figure it out. thanks, greg k-h
On Tue, 18 Aug 2009 11:07:08 +0800 ykzhao <yakui.zhao@intel.com> wrote: > The two boxes are based on the g4x platform. And the acpi backlight > interface can work well on one box. And there is no acpi backlight > interface on another box. > In fact I check the vbios dump on several other boxes and the length > of backlight section is 21. So IMO the backlight section is not > reliable and we can't use it to parse the backlight interface. > None of the VBIOS dumps I have seem to have this sort of corruption; most have a length of 97, which is correct given the size of the backlight info struct we're using. But like I said, on 965+ platforms it doesn't matter, since they'll have OpRegion support (they're required to afaik). > > > Another issue is that the brightness level is so big when the > > > native/combo mode is used. For example: 0-65535 for the native > > > control method. The user will be confused by so many brightness > > > levels. > > > > User won't see them directly, so there should be no confusion. > > Brightness is generally exposed as a slider or a series of steps > > with the brightness keys. > But this interface can be found in /sys/class/backlight/*. Maybe it is > used to adjust the brightness by user. In such case too many > brightness levels are exposed to user. If the user is poking /sys/class/backlight directly, they should be technical enough to not be confused by large numbers. > > > How about using the legacy control method? > > > > Yes, we have to modify the legacy registers in some cases. But > > modifying them exclusively would be a problem... > How can we modify the legacy registers in the two different paths if > we expose only one interface to user space? If the legacy registers are active, they'll usually be used by firmware code to adjust the backlight quickly & easily. If the firmware adjusts them to 0, we may have to increase the value when we get a request through the backlight interface we export, or the brightness will remain at 0. In most other cases we'll only adjust the PWM value.
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 7537f57..2adad14 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -149,6 +149,11 @@ struct drm_i915_error_state { struct timeval time; }; +struct blc_funcs { + int (*get)(struct drm_device *dev); + void (*set)(struct drm_device *dev, int level); +}; + typedef struct drm_i915_private { struct drm_device *dev; @@ -212,6 +217,9 @@ typedef struct drm_i915_private { struct drm_display_mode *panel_fixed_mode; struct drm_display_mode *lfp_lvds_vbt_mode; /* if any */ struct drm_display_mode *sdvo_lvds_vbt_mode; /* if any */ + struct blc_struct blc_info; + struct blc_funcs *backlight; + struct backlight_device *backlight_device; /* for sysfs */ /* Feature bits from the VBIOS */ unsigned int int_tv_support:1; diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c index 300aee3..afc042b 100644 --- a/drivers/gpu/drm/i915/intel_bios.c +++ b/drivers/gpu/drm/i915/intel_bios.c @@ -315,6 +315,27 @@ parse_driver_features(struct drm_i915_private *dev_priv, dev_priv->edp_support = 1; } +static void +parse_lvds_backlight(struct drm_i915_private *dev_priv, struct bdb_header *bdb) +{ + struct bdb_lvds_options *lvds_options; + struct bdb_lvds_backlight *backlight; + int entry; + + lvds_options = find_section(bdb, BDB_LVDS_OPTIONS); + backlight = find_section(bdb, BDB_LVDS_BACKLIGHT); + if (!lvds_options || !backlight) + return; + + /* Make sure we have a compatible layout */ + if (backlight->blcstruct_size != sizeof(struct blc_struct)) + return; + + entry = lvds_options->panel_type; + memcpy(&dev_priv->blc_info, &backlight->panels[entry], + sizeof(struct blc_struct)); +} + /** * intel_init_bios - initialize VBIOS settings & find VBT * @dev: DRM device @@ -366,6 +387,7 @@ intel_init_bios(struct drm_device *dev) parse_sdvo_panel_data(dev_priv, bdb); parse_sdvo_device_mapping(dev_priv, bdb); parse_driver_features(dev_priv, bdb); + parse_lvds_backlight(dev_priv, bdb); pci_unmap_rom(pdev, bios); diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h index 0f8e5f6..5145151 100644 --- a/drivers/gpu/drm/i915/intel_bios.h +++ b/drivers/gpu/drm/i915/intel_bios.h @@ -380,6 +380,48 @@ struct bdb_sdvo_lvds_options { u8 panel_misc_bits_4; } __attribute__((packed)); +#define BLC_INVERTER_TYPE_NONE 0 +#define BLC_INVERTER_TYPE_I2C 1 +#define BLC_INVERTER_TYPE_PWM 2 + +#define BLC_GPIO_NONE 0 +#define BLC_GPIO_I2C 1 +#define BLC_GPIO_CRT_DDC 2 +#define BLC_GPIO_DVI_DDC 3 +#define BLC_GPIO_SDVO_I2C 5 + +#define BLC_GMBUS_100KHZ 0 +#define BLC_GMBUS_50KHZ 1 +#define BLC_GMBUS_400KHZ 2 +#define BLC_GMBUS_1MHZ 3 + +struct blc_struct { + u8 inverter_type:2; + u8 inverter_polarity:1; /* 1 means inverted (0 = max brightness) */ + u8 gpio_pins:3; + u8 gmbus_speed:2; + u16 pwm_freq; /* in Hz */ + u8 min_brightness; /* (0-255) */ + u8 i2c_slave_addr; + u8 i2c_cmd; +} __attribute__((packed)); + +struct bdb_lvds_backlight { + u8 blcstruct_size; + struct blc_struct panels[16]; +} __attribute__((packed)); + +struct bdb_lvds_power { + u8 dpst_enabled:1; + u8 pwr_prefs:3; + u8 rsvd1:3; + u8 als_enabled:1; + u16 als_backlight1; + u16 als_backlight2; + u16 als_backlight3; + u16 als_backlight4; + u16 als_backlight5; +} __attribute__((packed)); #define BDB_DRIVER_FEATURE_NO_LVDS 0 #define BDB_DRIVER_FEATURE_INT_LVDS 1 diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index 3f445a8..75237c1 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -27,6 +27,7 @@ * Jesse Barnes <jesse.barnes@intel.com> */ +#include <linux/backlight.h> #include <linux/dmi.h> #include <linux/i2c.h> #include "drmP.h" @@ -60,7 +61,7 @@ struct intel_lvds_priv { * * \param level backlight level, from 0 to intel_lvds_get_max_backlight(). */ -static void intel_lvds_set_backlight(struct drm_device *dev, int level) +static void blc_pwm_set(struct drm_device *dev, int level) { struct drm_i915_private *dev_priv = dev->dev_private; u32 blc_pwm_ctl, reg; @@ -75,6 +76,30 @@ static void intel_lvds_set_backlight(struct drm_device *dev, int level) (level << BACKLIGHT_DUTY_CYCLE_SHIFT))); } +static int blc_pwm_get(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + u32 blc_pwm_ctl, reg; + + if (IS_IGDNG(dev)) + reg = BLC_PWM_CPU_CTL; + else + reg = BLC_PWM_CTL; + + blc_pwm_ctl = I915_READ(reg) & ~BACKLIGHT_DUTY_CYCLE_MASK; + + return blc_pwm_ctl; +} + +static void blc_i2c_set(struct drm_device *dev, int level) +{ +} + +static int blc_i2c_get(struct drm_device *dev) +{ + return 0; +} + /** * Returns the maximum level of the backlight duty cycle field. */ @@ -114,11 +139,7 @@ static void intel_lvds_set_power(struct drm_device *dev, bool on) do { pp_status = I915_READ(status_reg); } while ((pp_status & PP_ON) == 0); - - intel_lvds_set_backlight(dev, dev_priv->backlight_duty_cycle); } else { - intel_lvds_set_backlight(dev, 0); - I915_WRITE(ctl_reg, I915_READ(ctl_reg) & ~POWER_TARGET_ON); do { @@ -652,7 +673,11 @@ static int intel_lvds_get_modes(struct drm_connector *connector) static void intel_lvds_destroy(struct drm_connector *connector) { struct intel_output *intel_output = to_intel_output(connector); + struct drm_device *dev = connector->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + if (dev_priv->backlight_device) + backlight_device_unregister(dev_priv->backlight_device); if (intel_output->ddc_bus) intel_i2c_destroy(intel_output->ddc_bus); drm_sysfs_connector_remove(connector); @@ -856,6 +881,75 @@ static int intel_lid_present(void) } #endif +static int intel_get_backlight_brightness(struct backlight_device *bd) +{ + return bd->props.brightness; +} + +static int intel_update_backlight_status(struct backlight_device *bd) +{ + struct drm_device *dev = bl_get_data(bd); + struct drm_i915_private *dev_priv = dev->dev_private; + + /* FIXME: check for FB blanking */ + dev_priv->backlight->set(dev, bd->props.brightness); + return 0; +} + +static struct backlight_ops intel_backlight_ops = { + .get_brightness = intel_get_backlight_brightness, + .update_status = intel_update_backlight_status, +}; + +/** + * intel_lvds_backlight_init - find & initialize backlight devices + * @dev: DRM device + * + * If we find a backlight controller (i.e. the backlight controller type + * isn't BLC_INVERTER_TYPE_NONE) we set it up here. This either means + * a PWM attached controller or an I2C attached controller. Control of the + * backlight is exposed through sysfs, unless an ACPI or other platform + * device has already been registered. + */ +static void intel_lvds_backlight_init(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + struct backlight_device *backlight_device; + + /* FIXME: check whether /sys/class/backlight is populated or not */ + + switch (dev_priv->blc_info.inverter_type) { + case BLC_INVERTER_TYPE_I2C: + dev_priv->backlight->get = blc_i2c_get; + dev_priv->backlight->set = blc_i2c_set; + break; + case BLC_INVERTER_TYPE_PWM: + dev_priv->backlight->get = blc_pwm_get; + dev_priv->backlight->set = blc_pwm_set; + break; + default: + DRM_DEBUG("backlight inverter not available\n"); + goto out; + } + + backlight_device = backlight_device_register("i915", &dev->pdev->dev, dev, + &intel_backlight_ops); + if (IS_ERR(backlight_device)) { + DRM_ERROR("failed to register backlight device\n"); + goto out; + } + + backlight_device->props.max_brightness = + dev_priv->blc_info.inverter_polarity ? 0 : 255; + backlight_device->props.brightness = dev_priv->backlight->get(dev); + backlight_device->props.power = FB_BLANK_UNBLANK; + backlight_update_status(backlight_device); + + dev_priv->backlight_device = backlight_device; +out: + return; +} + /** * intel_lvds_init - setup LVDS connectors on this device * @dev: drm device @@ -1009,6 +1103,8 @@ void intel_lvds_init(struct drm_device *dev) if (!dev_priv->panel_fixed_mode) goto failed; + intel_lvds_backlight_init(dev); + out: if (IS_IGDNG(dev)) { u32 pwm;