| Message ID | 1449755537-8697-1-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 10 Dec 2015, Imre Deak <imre.deak@intel.com> wrote: > There seem to be BIOSes with that set an invalid PWM frequency, assuming the > wrong base frequency. This base frequency is 19.2MHz on BXT except for A > stepping where it's the current CD clock frequency. The BIOSes in question > don't take this difference into account and program the frequency > assuming it's based on the CD clock frequency even on B stepping. > > Since we don't care about A stepping any more, simply assume the 19.2MHz > everywhere and require a minimum of 100Hz PWM frequency, regard anything > below this as incorrect. In case we correct the frequency we also have to > disable fastboot, that being the simplest way to reprogram the backlight > settings. > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 5 +++++ > drivers/gpu/drm/i915/intel_drv.h | 1 + > drivers/gpu/drm/i915/intel_panel.c | 7 +++++++ > 3 files changed, 13 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index e9bb860..bdd8506 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -13271,6 +13271,11 @@ static void calc_watermark_data(struct drm_atomic_state *state) > } > } > > +void intel_disable_fastboot(void) > +{ > + i915.fastboot = 0; > +} > + > /** > * intel_atomic_check - validate state object > * @dev: drm device > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 76dfa28..e957494 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1057,6 +1057,7 @@ int intel_pch_rawclk(struct drm_device *dev); > int intel_hrawclk(struct drm_device *dev); > void intel_mark_busy(struct drm_device *dev); > void intel_mark_idle(struct drm_device *dev); > +void intel_disable_fastboot(void); > void intel_crtc_restore_mode(struct drm_crtc *crtc); > int intel_display_suspend(struct drm_device *dev); > void intel_encoder_destroy(struct drm_encoder *encoder); > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > index 72183a0..c598b6b 100644 > --- a/drivers/gpu/drm/i915/intel_panel.c > +++ b/drivers/gpu/drm/i915/intel_panel.c > @@ -1647,6 +1647,13 @@ bxt_setup_backlight(struct intel_connector *connector, enum pipe unused) > panel->backlight.active_low_pwm = pwm_ctl & BXT_BLC_PWM_POLARITY; > panel->backlight.max = > I915_READ(BXT_BLC_PWM_FREQ(panel->backlight.controller)); > + if (panel->backlight.hz_to_pwm && > + panel->backlight.max > panel->backlight.hz_to_pwm(connector, 100)) { > + DRM_INFO("Correcting innvalid BIOS backlight PWM frequency (%d), disabling fastboot\n", > + panel->backlight.max); > + intel_disable_fastboot(); > + panel->backlight.max = 0; > + } What a mess, again. This should be reported to the BIOS folks, they've probably written this against BXT A0. Maarten wants to kill off i915.fastboot again [1], and I need to fix the Chromebook backlight. This ties in a bit. The basic premise is the same; BIOS gave us a broken state. We should probably try to sanitize what we can, but there are limits to that. If the backlight code observes broken state, we'd need to call back to eDP code (which isn't necessarily ready yet because we're initializing) to switch off backlight in the power sequencer, reinit backlight, switch backlight back on in the power sequencer. This doesn't make sense. There are probably other scenarios where sanitization is too hard. I think your general approach of forcing a modeset and driver load in some cases is a good one. But I'd like to hear Maarten's comments on the implementation of it. BR, Jani. [1] http://mid.gmane.org/5666907C.2040501@linux.intel.com > > if (!panel->backlight.max) > panel->backlight.max = get_backlight_max_vbt(connector);
On to, 2015-12-10 at 17:00 +0200, Jani Nikula wrote: > On Thu, 10 Dec 2015, Imre Deak <imre.deak@intel.com> wrote: > > There seem to be BIOSes with that set an invalid PWM frequency, > > assuming the > > wrong base frequency. This base frequency is 19.2MHz on BXT except > > for A > > stepping where it's the current CD clock frequency. The BIOSes in > > question > > don't take this difference into account and program the frequency > > assuming it's based on the CD clock frequency even on B stepping. > > > > Since we don't care about A stepping any more, simply assume the > > 19.2MHz > > everywhere and require a minimum of 100Hz PWM frequency, regard > > anything > > below this as incorrect. In case we correct the frequency we also > > have to > > disable fastboot, that being the simplest way to reprogram the > > backlight > > settings. > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 5 +++++ > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > drivers/gpu/drm/i915/intel_panel.c | 7 +++++++ > > 3 files changed, 13 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index e9bb860..bdd8506 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -13271,6 +13271,11 @@ static void calc_watermark_data(struct > > drm_atomic_state *state) > > } > > } > > > > +void intel_disable_fastboot(void) > > +{ > > + i915.fastboot = 0; > > +} > > + > > /** > > * intel_atomic_check - validate state object > > * @dev: drm device > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > b/drivers/gpu/drm/i915/intel_drv.h > > index 76dfa28..e957494 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -1057,6 +1057,7 @@ int intel_pch_rawclk(struct drm_device *dev); > > int intel_hrawclk(struct drm_device *dev); > > void intel_mark_busy(struct drm_device *dev); > > void intel_mark_idle(struct drm_device *dev); > > +void intel_disable_fastboot(void); > > void intel_crtc_restore_mode(struct drm_crtc *crtc); > > int intel_display_suspend(struct drm_device *dev); > > void intel_encoder_destroy(struct drm_encoder *encoder); > > diff --git a/drivers/gpu/drm/i915/intel_panel.c > > b/drivers/gpu/drm/i915/intel_panel.c > > index 72183a0..c598b6b 100644 > > --- a/drivers/gpu/drm/i915/intel_panel.c > > +++ b/drivers/gpu/drm/i915/intel_panel.c > > @@ -1647,6 +1647,13 @@ bxt_setup_backlight(struct intel_connector > > *connector, enum pipe unused) > > panel->backlight.active_low_pwm = pwm_ctl & > > BXT_BLC_PWM_POLARITY; > > panel->backlight.max = > > I915_READ(BXT_BLC_PWM_FREQ(panel- > > >backlight.controller)); > > + if (panel->backlight.hz_to_pwm && > > + panel->backlight.max > panel- > > >backlight.hz_to_pwm(connector, 100)) { > > + DRM_INFO("Correcting innvalid BIOS backlight PWM > > frequency (%d), disabling fastboot\n", > > + panel->backlight.max); > > + intel_disable_fastboot(); > > + panel->backlight.max = 0; > > + } > > What a mess, again. This should be reported to the BIOS folks, > they've > probably written this against BXT A0. > > Maarten wants to kill off i915.fastboot again [1], and I need to fix > the > Chromebook backlight. This ties in a bit. > > The basic premise is the same; BIOS gave us a broken state. We should > probably try to sanitize what we can, but there are limits to that. > If > the backlight code observes broken state, we'd need to call back to > eDP > code (which isn't necessarily ready yet because we're initializing) > to > switch off backlight in the power sequencer, reinit backlight, switch > backlight back on in the power sequencer. This doesn't make sense. > There > are probably other scenarios where sanitization is too hard. > > I think your general approach of forcing a modeset and driver load in > some cases is a good one. But I'd like to hear Maarten's comments on > the > implementation of it. Note that this is also a problem without fastboot, since atm we'll just use the BIOS setting if it programmed the frequency. In fact fastboot is disabled now by default and I added the disabling of it only in case someone tries to enable it or it becomes enabled by default in the future. Forgot to mention all this in the commit message. --Imre
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e9bb860..bdd8506 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13271,6 +13271,11 @@ static void calc_watermark_data(struct drm_atomic_state *state) } } +void intel_disable_fastboot(void) +{ + i915.fastboot = 0; +} + /** * intel_atomic_check - validate state object * @dev: drm device diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 76dfa28..e957494 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1057,6 +1057,7 @@ int intel_pch_rawclk(struct drm_device *dev); int intel_hrawclk(struct drm_device *dev); void intel_mark_busy(struct drm_device *dev); void intel_mark_idle(struct drm_device *dev); +void intel_disable_fastboot(void); void intel_crtc_restore_mode(struct drm_crtc *crtc); int intel_display_suspend(struct drm_device *dev); void intel_encoder_destroy(struct drm_encoder *encoder); diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index 72183a0..c598b6b 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -1647,6 +1647,13 @@ bxt_setup_backlight(struct intel_connector *connector, enum pipe unused) panel->backlight.active_low_pwm = pwm_ctl & BXT_BLC_PWM_POLARITY; panel->backlight.max = I915_READ(BXT_BLC_PWM_FREQ(panel->backlight.controller)); + if (panel->backlight.hz_to_pwm && + panel->backlight.max > panel->backlight.hz_to_pwm(connector, 100)) { + DRM_INFO("Correcting innvalid BIOS backlight PWM frequency (%d), disabling fastboot\n", + panel->backlight.max); + intel_disable_fastboot(); + panel->backlight.max = 0; + } if (!panel->backlight.max) panel->backlight.max = get_backlight_max_vbt(connector);
There seem to be BIOSes with that set an invalid PWM frequency, assuming the wrong base frequency. This base frequency is 19.2MHz on BXT except for A stepping where it's the current CD clock frequency. The BIOSes in question don't take this difference into account and program the frequency assuming it's based on the CD clock frequency even on B stepping. Since we don't care about A stepping any more, simply assume the 19.2MHz everywhere and require a minimum of 100Hz PWM frequency, regard anything below this as incorrect. In case we correct the frequency we also have to disable fastboot, that being the simplest way to reprogram the backlight settings. Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 5 +++++ drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_panel.c | 7 +++++++ 3 files changed, 13 insertions(+)