Re: [PATCH 2/2] drm/i915: initialize backlight max from VBT

[Clint Taylor <clinton.a.taylor@intel.com>]

On 08/26/2015 12:58 AM, Jani Nikula wrote:
> Normally we determine the backlight PWM modulation frequency (which we
> also use as backlight max value) from the backlight registers at module
> load time, expecting the registers have been initialized by the BIOS. If
> this is not the case, we fail.
>
> The VBT contains the backlight modulation frequency in Hz. Add platform
> specific functions to convert the frequency in Hz to backlight PWM
> modulation frequency, and use them to initialize the backlight when the
> registers are not initialized by the BIOS.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h    |   2 +
>   drivers/gpu/drm/i915/i915_reg.h    |   1 +
>   drivers/gpu/drm/i915/intel_panel.c | 160 +++++++++++++++++++++++++++++++++++--
>   3 files changed, 156 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5f6fd0b71bc9..e9def31618b5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -665,6 +665,8 @@ struct drm_i915_display_funcs {
>   			      uint32_t level);
>   	void (*disable_backlight)(struct intel_connector *connector);
>   	void (*enable_backlight)(struct intel_connector *connector);
> +	uint32_t (*backlight_hz_to_pwm)(struct intel_connector *connector,
> +					uint32_t hz);
>   };
>
>   enum forcewake_domain_id {
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 71f7ba886fb1..5f07af41a600 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6316,6 +6316,7 @@ enum skl_disp_power_wells {
>   #define SOUTH_CHICKEN2		0xc2004
>   #define  FDI_MPHY_IOSFSB_RESET_STATUS	(1<<13)
>   #define  FDI_MPHY_IOSFSB_RESET_CTL	(1<<12)
> +#define  PWM_GRANULARITY		(1<<5)	/* LPT */

Shouldn't we be using the BLC_PWM_CTL version of the PWM_GRANULARITY 
bit on HSW and later?

>   #define  DPLS_EDP_PPS_FIX_DIS		(1<<0)
>
>   #define _FDI_RXA_CHICKEN         0xc200c
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index e2ab3f6ed022..edf523ff4610 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1212,10 +1212,125 @@ 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!
> + * HSW/BDW: This value represents the period of the PWM stream in clock periods
> + * multiplied by 128 (default increment) or 16 (alternate increment, selected in
> + * LPT SOUTH_CHICKEN2 register bit 5).
>    *
> - * XXX: Query mode clock or hardware clock and program PWM modulation frequency
> - * appropriately when it's 0. Use VBT and/or sane defaults.
> + * XXX: This only works when driving the PCH PWM. When driving the CPU PWM on
> + * the utility pin, the granularity needs to be determined by BLC_PWM_CTL bit
> + * 27.
> + */
> +static u32 hsw_hz_to_pwm(struct intel_connector *connector, u32 pwm_freq_hz)
> +{
> +	struct drm_device *dev = connector->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u32 mul, clock;
> +
> +	if (I915_READ(SOUTH_CHICKEN2) & PWM_GRANULARITY)
> +		mul = 16;

According to the HSW, BDW, and SKL BSPECs the increment is 8 and 128 
based on the PWM Granularity bit. The BDW and SKL ChromeOS reference 
designs uses the dedicated PWM pin and not the utility pin. So 
BLC_PWM_CTRL bit 27 should be used for PWM_GRANULARITY. We probably need 
logic to switch between PCH and CPU PWMs.

> +	else
> +		mul = 128;
> +
> +	if (dev_priv->pch_id == INTEL_PCH_LPT_DEVICE_ID_TYPE)
> +		clock = MHz(135); /* LPT:H */
> +	else
> +		clock = MHz(24); /* LPT:LP */
> +
> +	return clock / (pwm_freq_hz * mul);
> +}
> +
> +/*
> + * ILK/SNB/IVB: This value represents the period of the PWM stream in PCH
> + * display raw clocks multiplied by 128.
> + */
> +static u32 pch_hz_to_pwm(struct intel_connector *connector, u32 pwm_freq_hz)
> +{
> +	struct drm_device *dev = connector->base.dev;
> +	int clock = MHz(intel_pch_rawclk(dev));
> +
> +	return clock / (pwm_freq_hz * 128);
> +}
> +
> +/*
> + * Gen2: This field determines the number of time base events (display core
> + * clock frequency/32) in total for a complete cycle of modulated backlight
> + * control.
> + *
> + * Gen3: A time base event equals the display core clock ([DevPNV] HRAW clock)
> + * divided by 32.
> + */
> +static u32 i9xx_hz_to_pwm(struct intel_connector *connector, u32 pwm_freq_hz)
> +{
> +	struct drm_device *dev = connector->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int clock;
> +
> +	if (IS_PINEVIEW(dev))
> +		clock = intel_hrawclk(dev);
> +	else
> +		clock = 1000 * dev_priv->display.get_display_clock_speed(dev);
> +
> +	return clock / (pwm_freq_hz * 32);
> +}
> +
> +/*
> + * Gen4: This value represents the period of the PWM stream in display core
> + * clocks multiplied by 128.
> + */
> +static u32 i965_hz_to_pwm(struct intel_connector *connector, u32 pwm_freq_hz)
> +{
> +	struct drm_device *dev = connector->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int clock = 1000 * dev_priv->display.get_display_clock_speed(dev);
> +
> +	return clock / (pwm_freq_hz * 128);
> +}
> +
> +/*
> + * VLV: This value represents the period of the PWM stream in display core
> + * clocks ([DevCTG] 100MHz HRAW clocks) multiplied by 128 or 25MHz S0IX clocks

VLV and CHV use a 200 Mhz HRAW clock. Since patch 1 in the series moved 
intel_hrawclk() into intel_display.c lets use the returned value instead 
of the hardcoded value.

> + * multiplied by 16.
> + *
> + * XXX: Where is this selected???

This is currently selected in Bit 30 of CBR1_VLV. Of course intel_pm.c 
writes this register as 0x0 during init so it doesn't surprise me that 
this test would also be 0x0.

> + */
> +static u32 vlv_hz_to_pwm(struct intel_connector *connector, u32 pwm_freq_hz)
> +{
> +	if (1)
> +		return MHz(25) / (pwm_freq_hz * 16);

CHV actually uses a 19.2 MHz S0IX clock and since the MHz() macro 
doesn't accept floats we need to use KHz() macro.

> +	else
> +		return MHz(100) / (pwm_freq_hz * 128);
> +}

Here is the new function for the 3 previous issues

/*
  * VLV: This value represents the period of the PWM stream in display core
  * clocks ([DevCTG] 200MHz HRAW clocks) multiplied by 128 or 25MHz S0IX 
clocks
  * multiplied by 16. CHV uses a 19.2MHz S0IX clock.
  *
  */
static u32 vlv_hz_to_pwm(struct intel_connector *connector, u32 pwm_freq_hz)
{
	struct drm_device *dev = connector->base.dev;
	struct drm_i915_private *dev_priv = dev->dev_private;
	int clock;

	if ((I915_READ(CBR1_VLV) & CBR_PWM_CLOCK_MUX_SELECT) == 0)
		if (IS_CHERRYVIEW(dev)) {
			return KHz(19200) / (pwm_freq_hz * 16);
		}
		else {
			return MHz(25) / (pwm_freq_hz * 16);
		}
	else {
		clock = intel_hrawclk(dev);
		return MHz(clock) / (pwm_freq_hz * 128);
	}
}

> +
> +static u32 get_backlight_max_vbt(struct intel_connector *connector)
> +{
> +	struct drm_device *dev = connector->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u16 pwm_freq_hz = dev_priv->vbt.backlight.pwm_freq_hz;
> +	u32 pwm;
> +
> +	if (!pwm_freq_hz) {
> +		DRM_DEBUG_KMS("backlight frequency not specified in VBT\n");
> +		return 0;
> +	}
> +
> +	if (!dev_priv->display.backlight_hz_to_pwm) {
> +		DRM_DEBUG_KMS("backlight frequency setting from VBT currently not supported on this platform\n");
> +		return 0;
> +	}
> +
> +	pwm = dev_priv->display.backlight_hz_to_pwm(connector, pwm_freq_hz);
> +	if (!pwm) {
> +		DRM_DEBUG_KMS("backlight frequency conversion failed\n");
> +		return 0;
> +	}
> +
> +	DRM_DEBUG_KMS("backlight frequency %u Hz from VBT\n", pwm_freq_hz);
> +
> +	return pwm;
> +}
> +
> +/*
> + * Note: The setup hooks can't assume pipe is set!
>    */
>   static u32 get_backlight_min_vbt(struct intel_connector *connector)
>   {
> @@ -1255,6 +1370,10 @@ static int bdw_setup_backlight(struct intel_connector *connector, enum pipe unus
>
>   	pch_ctl2 = I915_READ(BLC_PWM_PCH_CTL2);
>   	panel->backlight.max = pch_ctl2 >> 16;
> +
> +	if (!panel->backlight.max)
> +		panel->backlight.max = get_backlight_max_vbt(connector);
> +
>   	if (!panel->backlight.max)
>   		return -ENODEV;
>
> @@ -1281,6 +1400,10 @@ static int pch_setup_backlight(struct intel_connector *connector, enum pipe unus
>
>   	pch_ctl2 = I915_READ(BLC_PWM_PCH_CTL2);
>   	panel->backlight.max = pch_ctl2 >> 16;
> +
> +	if (!panel->backlight.max)
> +		panel->backlight.max = get_backlight_max_vbt(connector);
> +
>   	if (!panel->backlight.max)
>   		return -ENODEV;
>
> @@ -1312,12 +1435,18 @@ static int i9xx_setup_backlight(struct intel_connector *connector, enum pipe unu
>   		panel->backlight.active_low_pwm = ctl & BLM_POLARITY_PNV;
>
>   	panel->backlight.max = ctl >> 17;
> -	if (panel->backlight.combination_mode)
> -		panel->backlight.max *= 0xff;
> +
> +	if (!panel->backlight.max) {
> +		panel->backlight.max = get_backlight_max_vbt(connector);
> +		panel->backlight.max >>= 1;
> +	}
>
>   	if (!panel->backlight.max)
>   		return -ENODEV;
>
> +	if (panel->backlight.combination_mode)
> +		panel->backlight.max *= 0xff;
> +
>   	panel->backlight.min = get_backlight_min_vbt(connector);
>
>   	val = i9xx_get_backlight(connector);
> @@ -1341,12 +1470,16 @@ static int i965_setup_backlight(struct intel_connector *connector, enum pipe unu
>
>   	ctl = I915_READ(BLC_PWM_CTL);
>   	panel->backlight.max = ctl >> 16;
> -	if (panel->backlight.combination_mode)
> -		panel->backlight.max *= 0xff;
> +
> +	if (!panel->backlight.max)
> +		panel->backlight.max = get_backlight_max_vbt(connector);
>
>   	if (!panel->backlight.max)
>   		return -ENODEV;
>
> +	if (panel->backlight.combination_mode)
> +		panel->backlight.max *= 0xff;
> +
>   	panel->backlight.min = get_backlight_min_vbt(connector);
>
>   	val = i9xx_get_backlight(connector);
> @@ -1386,6 +1519,10 @@ static int vlv_setup_backlight(struct intel_connector *connector, enum pipe pipe
>
>   	ctl = I915_READ(VLV_BLC_PWM_CTL(pipe));
>   	panel->backlight.max = ctl >> 16;
> +

the for_each_pipe() above this code hardcodes the upper 16 bits of 
VLV_BLC_PWM_CTL to 0xF42 (307.4 Hz) so this code never executes. 
removing the for_each_pipe block above these adds resolves this issue.

> +	if (!panel->backlight.max)
> +		panel->backlight.max = get_backlight_max_vbt(connector);
> +
>   	if (!panel->backlight.max)
>   		return -ENODEV;
>
> @@ -1412,6 +1549,10 @@ 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_FREQ1);
> +
> +	if (!panel->backlight.max)
> +		panel->backlight.max = get_backlight_max_vbt(connector);
> +
>   	if (!panel->backlight.max)
>   		return -ENODEV;
>
> @@ -1525,12 +1666,14 @@ void intel_panel_init_backlight_funcs(struct drm_device *dev)
>   		dev_priv->display.disable_backlight = pch_disable_backlight;
>   		dev_priv->display.set_backlight = bdw_set_backlight;
>   		dev_priv->display.get_backlight = bdw_get_backlight;
> +		dev_priv->display.backlight_hz_to_pwm = hsw_hz_to_pwm;

We need to create a new function for SKL, and BXT that doesn't use the 
utility pin. and yes I understand that this patch was submitted 20 
months ago. I'm investigating changes for BDW and SKL support.

>   	} else if (HAS_PCH_SPLIT(dev)) {
>   		dev_priv->display.setup_backlight = pch_setup_backlight;
>   		dev_priv->display.enable_backlight = pch_enable_backlight;
>   		dev_priv->display.disable_backlight = pch_disable_backlight;
>   		dev_priv->display.set_backlight = pch_set_backlight;
>   		dev_priv->display.get_backlight = pch_get_backlight;
> +		dev_priv->display.backlight_hz_to_pwm = pch_hz_to_pwm;
>   	} else if (IS_VALLEYVIEW(dev)) {
>   		if (dev_priv->vbt.has_mipi) {
>   			dev_priv->display.setup_backlight = pwm_setup_backlight;
> @@ -1544,6 +1687,7 @@ void intel_panel_init_backlight_funcs(struct drm_device *dev)
>   			dev_priv->display.disable_backlight = vlv_disable_backlight;
>   			dev_priv->display.set_backlight = vlv_set_backlight;
>   			dev_priv->display.get_backlight = vlv_get_backlight;
> +			dev_priv->display.backlight_hz_to_pwm = vlv_hz_to_pwm;
>   		}
>   	} else if (IS_GEN4(dev)) {
>   		dev_priv->display.setup_backlight = i965_setup_backlight;
> @@ -1551,12 +1695,14 @@ void intel_panel_init_backlight_funcs(struct drm_device *dev)
>   		dev_priv->display.disable_backlight = i965_disable_backlight;
>   		dev_priv->display.set_backlight = i9xx_set_backlight;
>   		dev_priv->display.get_backlight = i9xx_get_backlight;
> +		dev_priv->display.backlight_hz_to_pwm = i965_hz_to_pwm;
>   	} else {
>   		dev_priv->display.setup_backlight = i9xx_setup_backlight;
>   		dev_priv->display.enable_backlight = i9xx_enable_backlight;
>   		dev_priv->display.disable_backlight = i9xx_disable_backlight;
>   		dev_priv->display.set_backlight = i9xx_set_backlight;
>   		dev_priv->display.get_backlight = i9xx_get_backlight;
> +		dev_priv->display.backlight_hz_to_pwm = i9xx_hz_to_pwm;
>   	}
>   }
>
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx