[Intel-gfx] [PATCH] drm/i915/bxt: fix incorrect BIOS backlight PWM frequency setting

[Jani Nikula]

On Thu, 10 Dec 2015, Imre Deak <imre.deak at 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 at 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);

-- 
Jani Nikula, Intel Open Source Technology Center