[Intel-gfx] [PATCH v3 2/2] drivers: i915: Default max backlight brightness value
Use 0x1000 as the default backlight PWM max value and period. This is
passed in as a module parameter to i915_drv and is used to program the
PWM registers. It can be set to other values based on the needs of each
system.
Signed-off-by: Simon Que s...@chromium.org
---
Matthew, this patch has been reworked to better address your concerns. It is
more clearly shown that it will only affect the systems that don't have the
proper register values to begin with.
drivers/gpu/drm/i915/i915_drv.c|3 +++
drivers/gpu/drm/i915/i915_drv.h|1 +
drivers/gpu/drm/i915/i915_reg.h|1 +
drivers/gpu/drm/i915/intel_panel.c | 13 +++--
4 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index eb91e2d..fd06ce8 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -70,6 +70,9 @@ module_param_named(vbt_sdvo_panel_type,
i915_vbt_sdvo_panel_type, int, 0600);
static bool i915_try_reset = true;
module_param_named(reset, i915_try_reset, bool, 0600);
+unsigned int i915_max_backlight = 0x1000;
+module_param_named(max_backlight, i915_max_backlight, bool, 0600);
+
static struct drm_driver driver;
extern int intel_agp_enabled;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1c44613..2a2b558 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -999,6 +999,7 @@ extern unsigned int i915_panel_use_ssc;
extern int i915_vbt_sdvo_panel_type;
extern unsigned int i915_enable_rc6;
extern unsigned int i915_enable_fbc;
+extern unsigned int i915_max_backlight;
extern int i915_suspend(struct drm_device *dev, pm_message_t state);
extern int i915_resume(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 5d5def7..a832028 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3275,6 +3275,7 @@
#define PWM_POLARITY_ACTIVE_HIGH2 (0 28)
#define BLC_PWM_PCH_CTL2 0xc8254
+#define BLC_PWM_PCH_FREQ_SHIFT 16
#define PCH_PP_STATUS 0xc7200
#define PCH_PP_CONTROL 0xc7204
diff --git a/drivers/gpu/drm/i915/intel_panel.c
b/drivers/gpu/drm/i915/intel_panel.c
index f15388c..a087f1c 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -133,10 +133,16 @@ static u32 i915_read_blc_pwm_ctl(struct drm_i915_private
*dev_priv)
{
u32 val;
- /* Restore the CTL value if it lost, e.g. GPU reset */
-
+ /* Restore the CTL value if it was lost, e.g. GPU reset */
+ /* Use the default PWM max value if none is available. */
+ /* Note that the default max value will only be used if there is no */
+ /* value already initialized in the PWM register by the BIOS and */
+ /* nothing saved in dev_priv. */
if (HAS_PCH_SPLIT(dev_priv-dev)) {
val = I915_READ(BLC_PWM_PCH_CTL2);
+ if (dev_priv-saveBLC_PWM_CTL2 == 0 val == 0)
+ dev_priv-saveBLC_PWM_CTL2 =
+ i915_max_backlight BLC_PWM_PCH_FREQ_SHIFT;
if (dev_priv-saveBLC_PWM_CTL2 == 0) {
dev_priv-saveBLC_PWM_CTL2 = val;
} else if (val == 0) {
@@ -146,6 +152,9 @@ static u32 i915_read_blc_pwm_ctl(struct drm_i915_private
*dev_priv)
}
} else {
val = I915_READ(BLC_PWM_CTL);
+ if (dev_priv-saveBLC_PWM_CTL == 0 val == 0)
+ dev_priv-saveBLC_PWM_CTL = i915_max_backlight
+BACKLIGHT_MODULATION_FREQ_SHIFT;
if (dev_priv-saveBLC_PWM_CTL == 0) {
dev_priv-saveBLC_PWM_CTL = val;
dev_priv-saveBLC_PWM_CTL2 = I915_READ(BLC_PWM_CTL2);
--
1.7.2.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 2/2] drivers: i915: Default max backlight brightness value
I feel like I'm missing something here. Where's the firmware getting its initial value from? -- Matthew Garrett | mj...@srcf.ucam.org ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 2/2] drivers: i915: Default max backlight brightness value
On Tue, Nov 8, 2011 at 1:42 PM, Matthew Garrett mj...@srcf.ucam.org wrote: I feel like I'm missing something here. Where's the firmware getting its initial value from? My understanding is that normally, the firmware's VBIOS can program the value of the PWM register. But if the firmware doesn't have the VBIOS initialization code, it can still provide an initial value using this new param, as part of the kernel command line. Either way, it's up to the person writing or selecting the firmware to decide what initial value to provide. Simon ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 2/2] drivers: i915: Default max backlight brightness value
On Tue, Nov 08, 2011 at 02:05:14PM -0800, Simon Que wrote: On Tue, Nov 8, 2011 at 1:42 PM, Matthew Garrett mj...@srcf.ucam.org wrote: I feel like I'm missing something here. Where's the firmware getting its initial value from? My understanding is that normally, the firmware's VBIOS can program the value of the PWM register. But if the firmware doesn't have the VBIOS initialization code, it can still provide an initial value using this new param, as part of the kernel command line. Either way, it's up to the person writing or selecting the firmware to decide what initial value to provide. So the firmware doesn't set up graphics itself, it's entirely up to the kernel? What hardware is this for? -- Matthew Garrett | mj...@srcf.ucam.org ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 2/2] drivers: i915: Default max backlight brightness value
On Tue, Nov 8, 2011 at 2:10 PM, Matthew Garrett mj...@srcf.ucam.org wrote: On Tue, Nov 08, 2011 at 02:05:14PM -0800, Simon Que wrote: On Tue, Nov 8, 2011 at 1:42 PM, Matthew Garrett mj...@srcf.ucam.org wrote: I feel like I'm missing something here. Where's the firmware getting its initial value from? My understanding is that normally, the firmware's VBIOS can program the value of the PWM register. But if the firmware doesn't have the VBIOS initialization code, it can still provide an initial value using this new param, as part of the kernel command line. Either way, it's up to the person writing or selecting the firmware to decide what initial value to provide. So the firmware doesn't set up graphics itself, it's entirely up to the kernel? What hardware is this for? This is for an x86-based Chromebook. Its firmware doesn't have the VBIOS support. Previously, we had our own backlight driver that also did a similar initialization. Now, we are trying to move away from that and use the upstream driver instead. However, we still don't have the firmware support, which is why we have this patch to provide the missing information. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 2/2] drivers: i915: Default max backlight brightness value
On Tue, Nov 08, 2011 at 02:27:04PM -0800, Simon Que wrote: This is for an x86-based Chromebook. Its firmware doesn't have the VBIOS support. Previously, we had our own backlight driver that also did a similar initialization. Now, we are trying to move away from that and use the upstream driver instead. However, we still don't have the firmware support, which is why we have this patch to provide the missing information. I'm still not clear on this. There is no video support in the firmware at all? What happens if the kernel fails to boot? -- Matthew Garrett | mj...@srcf.ucam.org ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 2/2] drivers: i915: Default max backlight brightness value
On Tue, Nov 8, 2011 at 2:28 PM, Matthew Garrett mj...@srcf.ucam.org wrote: On Tue, Nov 08, 2011 at 02:27:04PM -0800, Simon Que wrote: This is for an x86-based Chromebook. Its firmware doesn't have the VBIOS support. Previously, we had our own backlight driver that also did a similar initialization. Now, we are trying to move away from that and use the upstream driver instead. However, we still don't have the firmware support, which is why we have this patch to provide the missing information. I'm still not clear on this. There is no video support in the firmware at all? What happens if the kernel fails to boot ? There is a backup kernel partition that can be used for boot. Failing that, the system can boot from a recovery image over USB. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 2/2] drivers: i915: Default max backlight brightness value
On Tue, Nov 08, 2011 at 02:41:56PM -0800, Simon Que wrote: There is a backup kernel partition that can be used for boot. Failing that, the system can boot from a recovery image over USB. So absolutely no video code in the firmware at all? Ok. What I'd really rather see here is some generic way for platform-specific code to pass the correct value to the driver. You're already exposing various things via the chromeos-acpi code, can you use that to set things up sensibly? It's obviously possible to use the command line to transfer this information, it's just considered pretty poor style. -- Matthew Garrett | mj...@srcf.ucam.org ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 2/2] drivers: i915: Default max backlight brightness value
On Tue, Nov 8, 2011 at 2:47 PM, Matthew Garrett mj...@srcf.ucam.org wrote: On Tue, Nov 08, 2011 at 02:41:56PM -0800, Simon Que wrote: There is a backup kernel partition that can be used for boot. Failing that, the system can boot from a recovery image over USB. So absolutely no video code in the firmware at all? Ok. What I'd really rather see here is some generic way for platform-specific code to pass the correct value to the driver. You're already exposing various things via the chromeos-acpi code, can you use that to set things up sensibly? It's obviously possible to use the command line to transfer this information, it's just considered pretty poor style. How about a DMI table check that overrides whatever is setup (or not setup) from the video bios? We know exactly what platforms need this so that table would be easy to specify. I'm not sure how well this would fit into our platform layer code, it would be pretty nasty to have to export the default backlight variable from the i915 driver and modify it from there as well, and I'm sure noone wants to see any kind of chromeos-specific code paths in the 915 driver (myself included). -Olof ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 2/2] drivers: i915: Default max backlight brightness value
On Tue, Nov 08, 2011 at 03:02:00PM -0800, Olof Johansson wrote: How about a DMI table check that overrides whatever is setup (or not setup) from the video bios? We know exactly what platforms need this so that table would be easy to specify. dmi's horribly unscalable. It's much better to have a communication channel that doesn't require new code for new models of the same platform. I'm not sure how well this would fit into our platform layer code, it would be pretty nasty to have to export the default backlight variable from the i915 driver and modify it from there as well, and I'm sure noone wants to see any kind of chromeos-specific code paths in the 915 driver (myself included). Well right now this path is (effectively) chromeos-specific. Refactoring the code so we just have the register readback as a single information source and allow the existing platform-specific code to hook in would be conceptually cleaner. But then maybe this is grotesque over-engineering and we should just hack this case. -- Matthew Garrett | mj...@srcf.ucam.org ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 2/2] drivers: i915: Default max backlight brightness value
Hi, On Tue, Nov 8, 2011 at 3:11 PM, Matthew Garrett mj...@srcf.ucam.org wrote: On Tue, Nov 08, 2011 at 03:02:00PM -0800, Olof Johansson wrote: How about a DMI table check that overrides whatever is setup (or not setup) from the video bios? We know exactly what platforms need this so that table would be easy to specify. dmi's horribly unscalable. It's much better to have a communication channel that doesn't require new code for new models of the same platform. Yeah, agreed. I'm not sure how well this would fit into our platform layer code, it would be pretty nasty to have to export the default backlight variable from the i915 driver and modify it from there as well, and I'm sure noone wants to see any kind of chromeos-specific code paths in the 915 driver (myself included). Well right now this path is (effectively) chromeos-specific. Refactoring the code so we just have the register readback as a single information source and allow the existing platform-specific code to hook in would be conceptually cleaner. But then maybe this is grotesque over-engineering and we should just hack this case. Actually, I didn't look closely enough to the original patches from Simon for this and your original feedback. Looks to me like filling in the original block of 'XXX add code' fallback is the way to go here. Getting the raw pch clock isn't hard, and setting a reasonable value for the modulation frequency and the corresponding max duty cycle based on that should solve our problem. -Olof ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 2/2] drivers: i915: Default max backlight brightness value
On Tue, Nov 8, 2011 at 5:49 PM, Olof Johansson o...@lixom.net wrote: Hi, On Tue, Nov 8, 2011 at 3:11 PM, Matthew Garrett mj...@srcf.ucam.org wrote: On Tue, Nov 08, 2011 at 03:02:00PM -0800, Olof Johansson wrote: How about a DMI table check that overrides whatever is setup (or not setup) from the video bios? We know exactly what platforms need this so that table would be easy to specify. dmi's horribly unscalable. It's much better to have a communication channel that doesn't require new code for new models of the same platform. Yeah, agreed. I'm not sure how well this would fit into our platform layer code, it would be pretty nasty to have to export the default backlight variable from the i915 driver and modify it from there as well, and I'm sure noone wants to see any kind of chromeos-specific code paths in the 915 driver (myself included). Well right now this path is (effectively) chromeos-specific. Refactoring the code so we just have the register readback as a single information source and allow the existing platform-specific code to hook in would be conceptually cleaner. But then maybe this is grotesque over-engineering and we should just hack this case. I tried to make some progress on this today. By existing platform-specific code, are you referring to chromeos_acpi.c? I did not see a clean way to do this. We can easily add variables to chromeos_acpi, but how does one cleanly make use of them in intel_panel.c? Actually, I didn't look closely enough to the original patches from Simon for this and your original feedback. Looks to me like filling in the original block of 'XXX add code' fallback is the way to go here. Getting the raw pch clock isn't hard, and setting a reasonable value for the modulation frequency and the corresponding max duty cycle based on that should solve our problem. I think the original comment was a bit of a guess on how to proceed. Knowing internal clocks does not help us figure out what what the display limitations are, like what is the right frequency to set the backlight to. That is why this is typically set by VBIOS rather than calculated in the driver. Since we try to run without VBIOS or VBT, we get critical defaults from init_vbt_defaults(). Since this is our first critical register not touched by VBT processing, init_vbt_defaults() does not help much. The new native backlight driver in intel_panel.c detects this uninitialized register case but does not provide a default (other than a hopefully previously saved value) itself. The quick fix for chromebooks would be to use as the default what our old homespun native backlight driver hardwired into the register, 0x1000. Making that a module parameter allows us to put it in the kernel blob that our firmware parses to find command line parameters. And other BIOSes that do not run VBIOS (if there are any) can similarly provide their appropriate default. I understand module parameters are now frowned upon, but I do not see a cleaner solution. bryan. -Olof ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
