From mboxrd@z Thu Jan 1 00:00:00 1970 From: Imre Deak Subject: Re: [PATCH 10/13] drm/i915: gather backlight information at setup Date: Thu, 14 Nov 2013 10:22:46 +0200 Message-ID: <1384417366.2455.5.camel@ideak-mobl> References: <9a3300d34d07703dec0a1ad076a4ecf57efa80b6.1383920621.git.jani.nikula@intel.com> <1384362099.25182.85.camel@intelbox> <87zjp77e79.fsf@intel.com> Reply-To: imre.deak@intel.com Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga03.intel.com (mga03.intel.com [143.182.124.21]) by gabe.freedesktop.org (Postfix) with ESMTP id 768C5FB271 for ; Thu, 14 Nov 2013 00:23:05 -0800 (PST) In-Reply-To: <87zjp77e79.fsf@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Jani Nikula Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Thu, 2013-11-14 at 07:19 +0200, Jani Nikula wrote: > On Wed, 13 Nov 2013, Imre Deak 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 > >> --- > >> 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