Re: [PATCH 10/13] drm/i915: gather backlight information at setup

[Imre Deak <imre.deak@intel.com>]

On Thu, 2013-11-14 at 07:19 +0200, Jani Nikula wrote:
> On Wed, 13 Nov 2013, Imre Deak <imre.deak@intel.com> wrote:
> > On Fri, 2013-11-08 at 16:49 +0200, Jani Nikula wrote:
> >> Prepare for being able to use the information at enable.
> >> 
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_drv.h   |    2 ++
> >>  drivers/gpu/drm/i915/intel_panel.c |   68 +++++++++++++++++++++++++++++-------
> >>  2 files changed, 58 insertions(+), 12 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> index 7384a7b..6f2bc82 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -163,6 +163,8 @@ struct intel_panel {
> >>  		u32 level;
> >>  		u32 max;
> >>  		bool enabled;
> >> +		bool combination_mode;	/* gen 2/4 only */
> >> +		bool active_low_pwm;
> >>  		struct backlight_device *device;
> >>  	} backlight;
> >>  };
> >> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> >> index 3dd9f57d..0e8f0a3 100644
> >> --- a/drivers/gpu/drm/i915/intel_panel.c
> >> +++ b/drivers/gpu/drm/i915/intel_panel.c
> >> @@ -490,13 +490,14 @@ static u32 i9xx_get_backlight(struct intel_connector *connector)
> >>  {
> >>  	struct drm_device *dev = connector->base.dev;
> >>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >> +	struct intel_panel *panel = &connector->panel;
> >>  	u32 val;
> >>  
> >>  	val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK;
> >>  	if (INTEL_INFO(dev)->gen < 4)
> >>  		val >>= 1;
> >>  
> >> -	if (is_backlight_combination_mode(dev)) {
> >> +	if (panel->backlight.combination_mode) {
> >>  		u8 lbpc;
> >>  
> >>  		pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc);
> >> @@ -558,7 +559,7 @@ static void i9xx_set_backlight(struct intel_connector *connector, u32 level)
> >>  
> >>  	WARN_ON(panel->backlight.max == 0);
> >>  
> >> -	if (is_backlight_combination_mode(dev)) {
> >> +	if (panel->backlight.combination_mode) {
> >>  		u8 lbpc;
> >>  
> >>  		lbpc = level * 0xfe / panel->backlight.max + 1;
> >> @@ -966,46 +967,84 @@ static void intel_backlight_device_unregister(struct intel_connector *connector)
> >>   */
> >>  static int pch_setup_backlight(struct intel_connector *connector)
> >>  {
> >> +	struct drm_device *dev = connector->base.dev;
> >> +	struct drm_i915_private *dev_priv = dev->dev_private;
> >>  	struct intel_panel *panel = &connector->panel;
> >> -	u32 val;
> >> +	u32 cpu_ctl2, pch_ctl1, pch_ctl2, val;
> >>  
> >> -	panel->backlight.max = pch_get_max_backlight(connector);
> >> +	pch_ctl1 = I915_READ(BLC_PWM_PCH_CTL1);
> >> +	panel->backlight.active_low_pwm = pch_ctl1 & BLM_PCH_POLARITY;
> >> +
> >> +	pch_ctl2 = I915_READ(BLC_PWM_PCH_CTL2);
> >> +	panel->backlight.max = pch_ctl2 >> 16;
> >>  	if (!panel->backlight.max)
> >>  		return -ENODEV;
> >>  
> >>  	val = pch_get_backlight(connector);
> >>  	panel->backlight.level = intel_panel_compute_brightness(connector, val);
> >>  
> >> +	cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2);
> >> +	panel->backlight.enabled = (cpu_ctl2 & BLM_PWM_ENABLE) &&
> >> +		(pch_ctl1 & BLM_PCH_PWM_ENABLE) && panel->backlight.level != 0;
> >
> > Don't we need to check here for QUIRK_NO_PCH_PWM_ENABLE?
> 
> Up front, I think the quirk is bogus, and it breaks machines.
> 
> Even if *we* don't set BLM_PCH_PWM_ENABLE on quirked machines, we just
> expect the BIOS to have done it. 

Ok. I checked the commit adding the quirk, which only says: "Some
machines suffer from non-functional backlight controls if
BLM_PCH_PWM_ENABLE is set", so from that this wasn't clear.

> So for checking whether the backlight is enabled, this should be
accurate.

Ok, in that case the patch looks ok to me:

Reviewed-by: Imre Deak <imre.deak@intel.com>