[PATCH 1/2] drivers: i915: Fix BLC PWM register setup

[PATCH 1/2] drivers: i915: Fix BLC PWM register setup

[Simon Que]

There is an error in i915_read_blc_pwm_ctl, where the register values
are not being copied correctly.  BLC_PWM_CTL and BLC_PWM_CTL2 are
getting mixed up.  This patch fixes that so that saveBLC_PWM_CTL2 and
not saveBLC_PWM_CTL is copied to the BLC_PWM_CTL2 register.

Signed-off-by: Simon Que <sque@chromium.org>
---
 drivers/gpu/drm/i915/intel_panel.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index a9e0c7b..f15388c 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -141,8 +141,8 @@ static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
 			dev_priv->saveBLC_PWM_CTL2 = val;
 		} else if (val == 0) {
 			I915_WRITE(BLC_PWM_PCH_CTL2,
-				   dev_priv->saveBLC_PWM_CTL);
-			val = dev_priv->saveBLC_PWM_CTL;
+				   dev_priv->saveBLC_PWM_CTL2);
+			val = dev_priv->saveBLC_PWM_CTL2;
 		}
 	} else {
 		val = I915_READ(BLC_PWM_CTL);
-- 
1.7.2.3

[PATCH v3 2/2] drivers: i915: Default max backlight brightness value

[Simon Que]

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 <sque@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

Re: [PATCH v3 2/2] drivers: i915: Default max backlight brightness value

[Matthew Garrett]

I feel like I'm missing something here. Where's the firmware getting its 
initial value from?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH v3 2/2] drivers: i915: Default max backlight brightness value

[Simon Que]

[-- Attachment #1.1: Type: text/plain, Size: 554 bytes --]

On Tue, Nov 8, 2011 at 1:42 PM, Matthew Garrett <mjg59@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

[-- Attachment #1.2: Type: text/html, Size: 846 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/2] drivers: i915: Default max backlight brightness value

[Matthew Garrett]

On Tue, Nov 08, 2011 at 02:05:14PM -0800, Simon Que wrote:
> On Tue, Nov 8, 2011 at 1:42 PM, Matthew Garrett <mjg59@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 | mjg59@srcf.ucam.org

Re: [PATCH v3 2/2] drivers: i915: Default max backlight brightness value

[Simon Que]

[-- Attachment #1.1: Type: text/plain, Size: 1219 bytes --]

On Tue, Nov 8, 2011 at 2:10 PM, Matthew Garrett <mjg59@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 <mjg59@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.

[-- Attachment #1.2: Type: text/html, Size: 1716 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/2] drivers: i915: Default max backlight brightness value

[Matthew Garrett]

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

Re: [PATCH v3 2/2] drivers: i915: Default max backlight brightness value

[Simon Que]

[-- Attachment #1.1: Type: text/plain, Size: 795 bytes --]

On Tue, Nov 8, 2011 at 2:28 PM, Matthew Garrett <mjg59@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.

[-- Attachment #1.2: Type: text/html, Size: 1297 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/2] drivers: i915: Default max backlight brightness value

[Matthew Garrett]

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

Re: [PATCH v3 2/2] drivers: i915: Default max backlight brightness value

[Olof Johansson]

On Tue, Nov 8, 2011 at 2:47 PM, Matthew Garrett <mjg59@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

Re: [PATCH v3 2/2] drivers: i915: Default max backlight brightness value

[Matthew Garrett]

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

Re: [PATCH v3 2/2] drivers: i915: Default max backlight brightness value

[Olof Johansson]

Hi,

On Tue, Nov 8, 2011 at 3:11 PM, Matthew Garrett <mjg59@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

Re: [PATCH v3 2/2] drivers: i915: Default max backlight brightness value

[Bryan Freed]

On Tue, Nov 8, 2011 at 5:49 PM, Olof Johansson <olof@lixom.net> wrote:
> Hi,
>
> On Tue, Nov 8, 2011 at 3:11 PM, Matthew Garrett <mjg59@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
>