[PATCH v4] drivers: i915: Default backlight PWM frequency

[PATCH v4] drivers: i915: Default backlight PWM frequency

[Simon Que]

If the firmware did not initialize the backlight PWM registers, set up a
default PWM frequency of 200 Hz.  This is determined using the following
formula:

  freq = refclk / (128 * pwm_max)

The PWM register allows the max PWM value to be set.  So we want to use
the formula, where freq = 200:

  pwm_max = refclk / (128 * freq)

This patch will, in the case of missing PWM register initialization
values, look for the reference clock frequency.  Based on that, it sets
an appropriate max PWM value for a frequency of 200 Hz.

If no refclk frequency is found, the max PWM will be zero, which results
in no change to the PWM registers.

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

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index f15388c..dda5de2 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -32,6 +32,12 @@
 
 #define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
 
+/* These are used to calculate a reasonable default when firmware has not
+ * configured a maximum PWM frequency, with 200Hz as the current default target.
+ */
+#define DEFAULT_BACKLIGHT_PWM_FREQ   200
+#define BACKLIGHT_REFCLK_DIVISOR     128
+
 void
 intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
 		       struct drm_display_mode *adjusted_mode)
@@ -129,12 +135,32 @@ static int is_backlight_combination_mode(struct drm_device *dev)
 	return 0;
 }
 
+static void i915_set_default_max_backlight(struct drm_i915_private *dev_priv)
+{
+	u32 refclk_freq_mhz = 0;
+	u32 max_pwm;
+
+	if (HAS_PCH_SPLIT(dev_priv->dev))
+		refclk_freq_mhz = I915_READ(PCH_RAWCLK_FREQ) & RAWCLK_FREQ_MASK;
+	else if (dev_priv->lvds_use_ssc)
+		refclk_freq_mhz = dev_priv->lvds_ssc_freq;
+
+	max_pwm = refclk_freq_mhz * 1000000 /
+			(BACKLIGHT_REFCLK_DIVISOR * DEFAULT_BACKLIGHT_PWM_FREQ);
+
+	if (HAS_PCH_SPLIT(dev_priv->dev))
+		dev_priv->saveBLC_PWM_CTL2 = max_pwm << 16;
+	else if (IS_PINEVIEW(dev_priv->dev))
+		dev_priv->saveBLC_PWM_CTL = max_pwm << 17;
+	else
+		dev_priv->saveBLC_PWM_CTL = max_pwm << 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 */
 	if (HAS_PCH_SPLIT(dev_priv->dev)) {
 		val = I915_READ(BLC_PWM_PCH_CTL2);
 		if (dev_priv->saveBLC_PWM_CTL2 == 0) {
@@ -168,11 +194,11 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev)
 
 	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.
+		/* If backlight PWM registers have not been set, set them to
+		 * default backlight PWM settings.
 		 */
-		printk_once(KERN_WARNING "fixme: max PWM is zero.\n");
-		return 1;
+		i915_set_default_max_backlight(dev_priv);
+		max = i915_read_blc_pwm_ctl(dev_priv);
 	}
 
 	if (HAS_PCH_SPLIT(dev)) {
-- 
1.7.2.3

Re: [PATCH v4] drivers: i915: Default backlight PWM frequency

[Olof Johansson]

On Fri, Nov 11, 2011 at 02:12:58PM -0800, Simon Que wrote:
> If the firmware did not initialize the backlight PWM registers, set up a
> default PWM frequency of 200 Hz.  This is determined using the following
> formula:
> 
>   freq = refclk / (128 * pwm_max)
> 
> The PWM register allows the max PWM value to be set.  So we want to use
> the formula, where freq = 200:
> 
>   pwm_max = refclk / (128 * freq)
> 
> This patch will, in the case of missing PWM register initialization
> values, look for the reference clock frequency.  Based on that, it sets
> an appropriate max PWM value for a frequency of 200 Hz.
> 
> If no refclk frequency is found, the max PWM will be zero, which results
> in no change to the PWM registers.
> 
> Signed-off-by: Simon Que <sque@chromium.org>

Acked-by: Olof Johansson <olof@lixom.net>

Looks much better. I'm OK with this solution. Matthew?


-Olof

Re: [PATCH v4] drivers: i915: Default backlight PWM frequency

[Matthew Garrett]

On Fri, Nov 11, 2011 at 02:17:20PM -0800, Olof Johansson wrote:
> On Fri, Nov 11, 2011 at 02:12:58PM -0800, Simon Que wrote:
> > If the firmware did not initialize the backlight PWM registers, set up a
> > default PWM frequency of 200 Hz.  This is determined using the following
> > formula:
> > 
> >   freq = refclk / (128 * pwm_max)
> > 
> > The PWM register allows the max PWM value to be set.  So we want to use
> > the formula, where freq = 200:
> > 
> >   pwm_max = refclk / (128 * freq)
> > 
> > This patch will, in the case of missing PWM register initialization
> > values, look for the reference clock frequency.  Based on that, it sets
> > an appropriate max PWM value for a frequency of 200 Hz.
> > 
> > If no refclk frequency is found, the max PWM will be zero, which results
> > in no change to the PWM registers.
> > 
> > Signed-off-by: Simon Que <sque@chromium.org>
> 
> Acked-by: Olof Johansson <olof@lixom.net>
> 
> Looks much better. I'm OK with this solution. Matthew?

I'd still prefer this to come from the firmware in some way, but in the 
absence of the awesome let's go with the good.

Acked-by: Matthew Garrett <mjg@redhat.com>

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

Re: [PATCH v4] drivers: i915: Default backlight PWM frequency

[Takashi Iwai]

At Fri, 11 Nov 2011 14:12:58 -0800,
Simon Que wrote:
> 
> If the firmware did not initialize the backlight PWM registers, set up a
> default PWM frequency of 200 Hz.  This is determined using the following
> formula:
> 
>   freq = refclk / (128 * pwm_max)
> 
> The PWM register allows the max PWM value to be set.  So we want to use
> the formula, where freq = 200:
> 
>   pwm_max = refclk / (128 * freq)
> 
> This patch will, in the case of missing PWM register initialization
> values, look for the reference clock frequency.  Based on that, it sets
> an appropriate max PWM value for a frequency of 200 Hz.
> 
> If no refclk frequency is found, the max PWM will be zero, which results
> in no change to the PWM registers.
> 
> Signed-off-by: Simon Que <sque@chromium.org>
> ---
>  drivers/gpu/drm/i915/intel_panel.c |   38 ++++++++++++++++++++++++++++++-----
>  1 files changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index f15388c..dda5de2 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -32,6 +32,12 @@
>  
>  #define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
>  
> +/* These are used to calculate a reasonable default when firmware has not
> + * configured a maximum PWM frequency, with 200Hz as the current default target.
> + */
> +#define DEFAULT_BACKLIGHT_PWM_FREQ   200
> +#define BACKLIGHT_REFCLK_DIVISOR     128
> +
>  void
>  intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
>  		       struct drm_display_mode *adjusted_mode)
> @@ -129,12 +135,32 @@ static int is_backlight_combination_mode(struct drm_device *dev)
>  	return 0;
>  }
>  
> +static void i915_set_default_max_backlight(struct drm_i915_private *dev_priv)
> +{
> +	u32 refclk_freq_mhz = 0;
> +	u32 max_pwm;
> +
> +	if (HAS_PCH_SPLIT(dev_priv->dev))
> +		refclk_freq_mhz = I915_READ(PCH_RAWCLK_FREQ) & RAWCLK_FREQ_MASK;
> +	else if (dev_priv->lvds_use_ssc)
> +		refclk_freq_mhz = dev_priv->lvds_ssc_freq;
> +
> +	max_pwm = refclk_freq_mhz * 1000000 /
> +			(BACKLIGHT_REFCLK_DIVISOR * DEFAULT_BACKLIGHT_PWM_FREQ);
> +
> +	if (HAS_PCH_SPLIT(dev_priv->dev))
> +		dev_priv->saveBLC_PWM_CTL2 = max_pwm << 16;
> +	else if (IS_PINEVIEW(dev_priv->dev))
> +		dev_priv->saveBLC_PWM_CTL = max_pwm << 17;
> +	else
> +		dev_priv->saveBLC_PWM_CTL = max_pwm << 16;

Is the pineview case really correct?
The special handling for pineview in some places in intel_panel.c is
just for omitting the bit 0, IIRC.  It doesn't mean that the value is
twice larger.

BTW, this handling of bit 0 seems necessary not only for pineview but
for the older chips (gen < 4) in general, too, as being discussed in
another thread of LKML.  915GM hits the with problem of bit-0, for
example.


Takashi

Re: [PATCH v4] drivers: i915: Default backlight PWM frequency

[Daniel Vetter]

On Tue, Nov 15, 2011 at 07:47:58PM +0100, Takashi Iwai wrote:
> At Fri, 11 Nov 2011 14:12:58 -0800,
> Simon Que wrote:
> > 
> > If the firmware did not initialize the backlight PWM registers, set up a
> > default PWM frequency of 200 Hz.  This is determined using the following
> > formula:
> > 
> >   freq = refclk / (128 * pwm_max)
> > 
> > The PWM register allows the max PWM value to be set.  So we want to use
> > the formula, where freq = 200:
> > 
> >   pwm_max = refclk / (128 * freq)
> > 
> > This patch will, in the case of missing PWM register initialization
> > values, look for the reference clock frequency.  Based on that, it sets
> > an appropriate max PWM value for a frequency of 200 Hz.
> > 
> > If no refclk frequency is found, the max PWM will be zero, which results
> > in no change to the PWM registers.
> > 
> > Signed-off-by: Simon Que <sque@chromium.org>
> > ---
> >  drivers/gpu/drm/i915/intel_panel.c |   38 ++++++++++++++++++++++++++++++-----
> >  1 files changed, 32 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> > index f15388c..dda5de2 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -32,6 +32,12 @@
> >  
> >  #define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
> >  
> > +/* These are used to calculate a reasonable default when firmware has not
> > + * configured a maximum PWM frequency, with 200Hz as the current default target.
> > + */
> > +#define DEFAULT_BACKLIGHT_PWM_FREQ   200
> > +#define BACKLIGHT_REFCLK_DIVISOR     128
> > +
> >  void
> >  intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
> >  		       struct drm_display_mode *adjusted_mode)
> > @@ -129,12 +135,32 @@ static int is_backlight_combination_mode(struct drm_device *dev)
> >  	return 0;
> >  }
> >  
> > +static void i915_set_default_max_backlight(struct drm_i915_private *dev_priv)
> > +{
> > +	u32 refclk_freq_mhz = 0;
> > +	u32 max_pwm;
> > +
> > +	if (HAS_PCH_SPLIT(dev_priv->dev))
> > +		refclk_freq_mhz = I915_READ(PCH_RAWCLK_FREQ) & RAWCLK_FREQ_MASK;
> > +	else if (dev_priv->lvds_use_ssc)
> > +		refclk_freq_mhz = dev_priv->lvds_ssc_freq;
> > +
> > +	max_pwm = refclk_freq_mhz * 1000000 /
> > +			(BACKLIGHT_REFCLK_DIVISOR * DEFAULT_BACKLIGHT_PWM_FREQ);
> > +
> > +	if (HAS_PCH_SPLIT(dev_priv->dev))
> > +		dev_priv->saveBLC_PWM_CTL2 = max_pwm << 16;
> > +	else if (IS_PINEVIEW(dev_priv->dev))
> > +		dev_priv->saveBLC_PWM_CTL = max_pwm << 17;
> > +	else
> > +		dev_priv->saveBLC_PWM_CTL = max_pwm << 16;
> 
> Is the pineview case really correct?
> The special handling for pineview in some places in intel_panel.c is
> just for omitting the bit 0, IIRC.  It doesn't mean that the value is
> twice larger.
> 
> BTW, this handling of bit 0 seems necessary not only for pineview but
> for the older chips (gen < 4) in general, too, as being discussed in
> another thread of LKML.  915GM hits the with problem of bit-0, for
> example.

Do we still need this patch? If so, can you please address Takashi's
comment, on a quick check he seems to have a point.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48