| Message ID | 56855dfc8f586ee486e0e0a9e26ba9f390d87dfc.1383920621.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2013-11-08 at 16:49 +0200, Jani Nikula wrote: > We now have the max backlight value cached. Use it. > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/intel_panel.c | 45 +++++++++++++++++++----------------- > 1 file changed, 24 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > index ed6b1ec..9a55b36 100644 > --- a/drivers/gpu/drm/i915/intel_panel.c > +++ b/drivers/gpu/drm/i915/intel_panel.c > @@ -436,9 +436,6 @@ static u32 vlv_get_max_backlight(struct intel_connector *connector) > return _vlv_get_max_backlight(dev, pipe); > } > > -/* XXX: query mode clock or hardware clock and program max PWM appropriately > - * when it's 0. > - */ > static u32 intel_panel_get_max_backlight(struct intel_connector *connector) > { > struct drm_device *dev = connector->base.dev; > @@ -466,15 +463,16 @@ static u32 intel_panel_compute_brightness(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; > + > + WARN_ON(panel->backlight.max == 0); Just to clarify that I understood this: we can't hit this any more since setup_backlight returns now -ENODEV if backlight.max == 0 and so we won't register the backlight device in that case. I assume then that during booting the BIOS always leaves us with a valid max PWM value in one of the PWM_CTL regs and it can only get zeroed due to suspend, which isn't a problem after patch 4. With this: Reviewed-by: Imre Deak <imre.deak@intel.com> > > if (i915_panel_invert_brightness < 0) > return val; > > if (i915_panel_invert_brightness > 0 || > dev_priv->quirks & QUIRK_INVERT_BRIGHTNESS) { > - u32 max = intel_panel_get_max_backlight(connector); > - if (max) > - return max - val; > + return panel->backlight.max - val; > } > > return val; > @@ -555,17 +553,15 @@ static void i9xx_set_backlight(struct intel_connector *connector, u32 level) > { > struct drm_device *dev = connector->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_panel *panel = &connector->panel; > u32 tmp, mask; > > + WARN_ON(panel->backlight.max == 0); > + > if (is_backlight_combination_mode(dev)) { > - u32 max = intel_panel_get_max_backlight(connector); > u8 lbpc; > > - /* we're screwed, but keep behaviour backwards compatible */ > - if (!max) > - max = 1; > - > - lbpc = level * 0xfe / max + 1; > + lbpc = level * 0xfe / panel->backlight.max + 1; > level /= lbpc; > pci_write_config_byte(dev->pdev, PCI_LBPC, lbpc); > } > @@ -620,13 +616,10 @@ void intel_panel_set_backlight(struct intel_connector *connector, u32 level, > > spin_lock_irqsave(&dev_priv->backlight_lock, flags); > > - freq = intel_panel_get_max_backlight(connector); > - if (!freq) { > - /* we are screwed, bail out */ > - goto out; > - } > + WARN_ON(panel->backlight.max == 0); > > - /* scale to hardware, but be careful to not overflow */ > + /* scale to hardware max, but be careful to not overflow */ > + freq = panel->backlight.max; > if (freq < max) > level = level * freq / max; > else > @@ -638,7 +631,7 @@ void intel_panel_set_backlight(struct intel_connector *connector, u32 level, > > if (panel->backlight.enabled) > intel_panel_actually_set_backlight(connector, level); > -out: > + > spin_unlock_irqrestore(&dev_priv->backlight_lock, flags); > } > > @@ -839,8 +832,13 @@ void intel_panel_enable_backlight(struct intel_connector *connector) > > spin_lock_irqsave(&dev_priv->backlight_lock, flags); > > + /* XXX: transitional, call to make sure freq is set */ > + intel_panel_get_max_backlight(connector); > + > + WARN_ON(panel->backlight.max == 0); > + > if (panel->backlight.level == 0) { > - panel->backlight.level = intel_panel_get_max_backlight(connector); > + panel->backlight.level = panel->backlight.max; > if (panel->backlight.device) > panel->backlight.device->props.brightness = > panel->backlight.level; > @@ -960,7 +958,12 @@ static void intel_backlight_device_unregister(struct intel_connector *connector) > } > #endif /* CONFIG_BACKLIGHT_CLASS_DEVICE */ > > -/* Note: The setup hooks can't assume pipe is set! */ > +/* > + * Note: The setup hooks can't assume pipe is set! > + * > + * XXX: Query mode clock or hardware clock and program PWM modulation frequency > + * appropriately when it's 0. Use VBT and/or sane defaults. > + */ > static int pch_setup_backlight(struct intel_connector *connector) > { > struct intel_panel *panel = &connector->panel;
On Wed, 13 Nov 2013, Imre Deak <imre.deak@intel.com> wrote: > On Fri, 2013-11-08 at 16:49 +0200, Jani Nikula wrote: >> We now have the max backlight value cached. Use it. >> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> --- >> drivers/gpu/drm/i915/intel_panel.c | 45 +++++++++++++++++++----------------- >> 1 file changed, 24 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c >> index ed6b1ec..9a55b36 100644 >> --- a/drivers/gpu/drm/i915/intel_panel.c >> +++ b/drivers/gpu/drm/i915/intel_panel.c >> @@ -436,9 +436,6 @@ static u32 vlv_get_max_backlight(struct intel_connector *connector) >> return _vlv_get_max_backlight(dev, pipe); >> } >> >> -/* XXX: query mode clock or hardware clock and program max PWM appropriately >> - * when it's 0. >> - */ >> static u32 intel_panel_get_max_backlight(struct intel_connector *connector) >> { >> struct drm_device *dev = connector->base.dev; >> @@ -466,15 +463,16 @@ static u32 intel_panel_compute_brightness(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; >> + >> + WARN_ON(panel->backlight.max == 0); > > Just to clarify that I understood this: we can't hit this any more since > setup_backlight returns now -ENODEV if backlight.max == 0 and so we > won't register the backlight device in that case. I assume then that > during booting the BIOS always leaves us with a valid max PWM value in > one of the PWM_CTL regs and it can only get zeroed due to suspend, which > isn't a problem after patch 4. With this: You've correctly understood what I meant. I do hope what I meant is the right thing to do! ;) Now that I think about it, there are hints that (at least on some platforms) a GPU reset might cause the registers to be reset too: commit 0b0b053a3949f5c467c3b3ba135d4c161f9fbd00 Author: Chris Wilson <chris@chris-wilson.co.uk> Date: Tue Nov 23 09:45:50 2010 +0000 drm/i915/panel: Restore saved value of BLC_PWM_CTL The big question is, are we okay with re-writing the potentially zeroed out registers at crtc enable time? The current upstream code would do that at reading the max backlight level, which happens also at adjusting the backlight. But there are no guarantees about anyone adjusting the backlight either. I don't understand our GPU reset enough to say for sure, but my gut feeling is that what I have in this series should be enough. BR, Jani. > > Reviewed-by: Imre Deak <imre.deak@intel.com> > >> >> if (i915_panel_invert_brightness < 0) >> return val; >> >> if (i915_panel_invert_brightness > 0 || >> dev_priv->quirks & QUIRK_INVERT_BRIGHTNESS) { >> - u32 max = intel_panel_get_max_backlight(connector); >> - if (max) >> - return max - val; >> + return panel->backlight.max - val; >> } >> >> return val; >> @@ -555,17 +553,15 @@ static void i9xx_set_backlight(struct intel_connector *connector, u32 level) >> { >> struct drm_device *dev = connector->base.dev; >> struct drm_i915_private *dev_priv = dev->dev_private; >> + struct intel_panel *panel = &connector->panel; >> u32 tmp, mask; >> >> + WARN_ON(panel->backlight.max == 0); >> + >> if (is_backlight_combination_mode(dev)) { >> - u32 max = intel_panel_get_max_backlight(connector); >> u8 lbpc; >> >> - /* we're screwed, but keep behaviour backwards compatible */ >> - if (!max) >> - max = 1; >> - >> - lbpc = level * 0xfe / max + 1; >> + lbpc = level * 0xfe / panel->backlight.max + 1; >> level /= lbpc; >> pci_write_config_byte(dev->pdev, PCI_LBPC, lbpc); >> } >> @@ -620,13 +616,10 @@ void intel_panel_set_backlight(struct intel_connector *connector, u32 level, >> >> spin_lock_irqsave(&dev_priv->backlight_lock, flags); >> >> - freq = intel_panel_get_max_backlight(connector); >> - if (!freq) { >> - /* we are screwed, bail out */ >> - goto out; >> - } >> + WARN_ON(panel->backlight.max == 0); >> >> - /* scale to hardware, but be careful to not overflow */ >> + /* scale to hardware max, but be careful to not overflow */ >> + freq = panel->backlight.max; >> if (freq < max) >> level = level * freq / max; >> else >> @@ -638,7 +631,7 @@ void intel_panel_set_backlight(struct intel_connector *connector, u32 level, >> >> if (panel->backlight.enabled) >> intel_panel_actually_set_backlight(connector, level); >> -out: >> + >> spin_unlock_irqrestore(&dev_priv->backlight_lock, flags); >> } >> >> @@ -839,8 +832,13 @@ void intel_panel_enable_backlight(struct intel_connector *connector) >> >> spin_lock_irqsave(&dev_priv->backlight_lock, flags); >> >> + /* XXX: transitional, call to make sure freq is set */ >> + intel_panel_get_max_backlight(connector); >> + >> + WARN_ON(panel->backlight.max == 0); >> + >> if (panel->backlight.level == 0) { >> - panel->backlight.level = intel_panel_get_max_backlight(connector); >> + panel->backlight.level = panel->backlight.max; >> if (panel->backlight.device) >> panel->backlight.device->props.brightness = >> panel->backlight.level; >> @@ -960,7 +958,12 @@ static void intel_backlight_device_unregister(struct intel_connector *connector) >> } >> #endif /* CONFIG_BACKLIGHT_CLASS_DEVICE */ >> >> -/* Note: The setup hooks can't assume pipe is set! */ >> +/* >> + * Note: The setup hooks can't assume pipe is set! >> + * >> + * XXX: Query mode clock or hardware clock and program PWM modulation frequency >> + * appropriately when it's 0. Use VBT and/or sane defaults. >> + */ >> static int pch_setup_backlight(struct intel_connector *connector) >> { >> struct intel_panel *panel = &connector->panel; > >
On Wed, Nov 13, 2013 at 10:39:43AM +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: > >> We now have the max backlight value cached. Use it. > >> > >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_panel.c | 45 +++++++++++++++++++----------------- > >> 1 file changed, 24 insertions(+), 21 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > >> index ed6b1ec..9a55b36 100644 > >> --- a/drivers/gpu/drm/i915/intel_panel.c > >> +++ b/drivers/gpu/drm/i915/intel_panel.c > >> @@ -436,9 +436,6 @@ static u32 vlv_get_max_backlight(struct intel_connector *connector) > >> return _vlv_get_max_backlight(dev, pipe); > >> } > >> > >> -/* XXX: query mode clock or hardware clock and program max PWM appropriately > >> - * when it's 0. > >> - */ > >> static u32 intel_panel_get_max_backlight(struct intel_connector *connector) > >> { > >> struct drm_device *dev = connector->base.dev; > >> @@ -466,15 +463,16 @@ static u32 intel_panel_compute_brightness(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; > >> + > >> + WARN_ON(panel->backlight.max == 0); > > > > Just to clarify that I understood this: we can't hit this any more since > > setup_backlight returns now -ENODEV if backlight.max == 0 and so we > > won't register the backlight device in that case. I assume then that > > during booting the BIOS always leaves us with a valid max PWM value in > > one of the PWM_CTL regs and it can only get zeroed due to suspend, which > > isn't a problem after patch 4. With this: > > You've correctly understood what I meant. I do hope what I meant is the > right thing to do! ;) > > Now that I think about it, there are hints that (at least on some > platforms) a GPU reset might cause the registers to be reset too: > > commit 0b0b053a3949f5c467c3b3ba135d4c161f9fbd00 > Author: Chris Wilson <chris@chris-wilson.co.uk> > Date: Tue Nov 23 09:45:50 2010 +0000 > > drm/i915/panel: Restore saved value of BLC_PWM_CTL > > The big question is, are we okay with re-writing the potentially zeroed > out registers at crtc enable time? The current upstream code would do > that at reading the max backlight level, which happens also at adjusting > the backlight. But there are no guarantees about anyone adjusting the > backlight either. > > I don't understand our GPU reset enough to say for sure, but my gut > feeling is that what I have in this series should be enough. Also not that nowadays we don't reinitialize the display stuff any more since it caused trouble. And it wasn't necessary either. So if we'd indeed need to readjust the backlight I think we'd have a bug report by now: commit 523bcb28c3fb92baecda6daa60560f67eb4a6177 Author: Daniel Vetter <daniel.vetter@ffwll.ch> Date: Fri Apr 27 15:17:46 2012 +0200 drm/i915: remove modeset reset from i915_reset Cheers, Daniel
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index ed6b1ec..9a55b36 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -436,9 +436,6 @@ static u32 vlv_get_max_backlight(struct intel_connector *connector) return _vlv_get_max_backlight(dev, pipe); } -/* XXX: query mode clock or hardware clock and program max PWM appropriately - * when it's 0. - */ static u32 intel_panel_get_max_backlight(struct intel_connector *connector) { struct drm_device *dev = connector->base.dev; @@ -466,15 +463,16 @@ static u32 intel_panel_compute_brightness(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; + + WARN_ON(panel->backlight.max == 0); if (i915_panel_invert_brightness < 0) return val; if (i915_panel_invert_brightness > 0 || dev_priv->quirks & QUIRK_INVERT_BRIGHTNESS) { - u32 max = intel_panel_get_max_backlight(connector); - if (max) - return max - val; + return panel->backlight.max - val; } return val; @@ -555,17 +553,15 @@ static void i9xx_set_backlight(struct intel_connector *connector, u32 level) { struct drm_device *dev = connector->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_panel *panel = &connector->panel; u32 tmp, mask; + WARN_ON(panel->backlight.max == 0); + if (is_backlight_combination_mode(dev)) { - u32 max = intel_panel_get_max_backlight(connector); u8 lbpc; - /* we're screwed, but keep behaviour backwards compatible */ - if (!max) - max = 1; - - lbpc = level * 0xfe / max + 1; + lbpc = level * 0xfe / panel->backlight.max + 1; level /= lbpc; pci_write_config_byte(dev->pdev, PCI_LBPC, lbpc); } @@ -620,13 +616,10 @@ void intel_panel_set_backlight(struct intel_connector *connector, u32 level, spin_lock_irqsave(&dev_priv->backlight_lock, flags); - freq = intel_panel_get_max_backlight(connector); - if (!freq) { - /* we are screwed, bail out */ - goto out; - } + WARN_ON(panel->backlight.max == 0); - /* scale to hardware, but be careful to not overflow */ + /* scale to hardware max, but be careful to not overflow */ + freq = panel->backlight.max; if (freq < max) level = level * freq / max; else @@ -638,7 +631,7 @@ void intel_panel_set_backlight(struct intel_connector *connector, u32 level, if (panel->backlight.enabled) intel_panel_actually_set_backlight(connector, level); -out: + spin_unlock_irqrestore(&dev_priv->backlight_lock, flags); } @@ -839,8 +832,13 @@ void intel_panel_enable_backlight(struct intel_connector *connector) spin_lock_irqsave(&dev_priv->backlight_lock, flags); + /* XXX: transitional, call to make sure freq is set */ + intel_panel_get_max_backlight(connector); + + WARN_ON(panel->backlight.max == 0); + if (panel->backlight.level == 0) { - panel->backlight.level = intel_panel_get_max_backlight(connector); + panel->backlight.level = panel->backlight.max; if (panel->backlight.device) panel->backlight.device->props.brightness = panel->backlight.level; @@ -960,7 +958,12 @@ static void intel_backlight_device_unregister(struct intel_connector *connector) } #endif /* CONFIG_BACKLIGHT_CLASS_DEVICE */ -/* Note: The setup hooks can't assume pipe is set! */ +/* + * Note: The setup hooks can't assume pipe is set! + * + * XXX: Query mode clock or hardware clock and program PWM modulation frequency + * appropriately when it's 0. Use VBT and/or sane defaults. + */ static int pch_setup_backlight(struct intel_connector *connector) { struct intel_panel *panel = &connector->panel;
We now have the max backlight value cached. Use it. Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/intel_panel.c | 45 +++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 21 deletions(-)