Re: [PATCH] drm/i915/bxt: fix incorrect BIOS backlight PWM frequency setting

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

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx