[Intel-gfx] [PATCH 2/2] drm/i915: initialize backlight max from VBT
Jani Nikula
jani.nikula at intel.com
Fri Sep 4 05:48:27 PDT 2015
On Thu, 27 Aug 2015, Clint Taylor <clinton.a.taylor at intel.com> wrote:
> 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 at 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?
Only when driving the backlight on the utility pin.
>
>> #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.
No, it's the other way around. BLC_PWM_CTL bit 27 is used for
granularity control when driving the utility pin. Yes, in theory we
would need logic to switch between PCH/CPU PWMs, but in practise we do
not support CPU PWM anywhere, and the last time I checked (probably like
a year or two ago, admittedly) there were no designs using the CPU
PWM. I've kind of decided to look at it when we get the first bug where
backlight doesn't work because the design uses the utility pin.
>
>> + 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
Thanks, this is the kind of review I like. :)
>
> /*
> * 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.
Ack, I was thinking of making this a separate patch.
>
>> + 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.
Okay, so I've gone through the backlight register specs a few times too
many, so I didn't do it this time. However, from memory, hsw, bdw and
skl PCH backlight works just the same. We could use the current bdw/skl
code for hsw just as well, we've just ended up letting the hardware send
the messages from the CPU registers to PCH; this is possible on LPT PCH
but no longer on SPT PCH. (See bdw_enable_backlight; LPT PCH *allows*
setting or not setting BLM_PCH_OVERRIDE_ENABLE, after that it's the
default.)
Again, we do not support CPU backlight anywhere on PCH split platforms,
it's just the passing of messages from CPU registers to PCH. We also
don't support backlight on the utility pin (there's patches to add this
to bxt, but that's different anyway).
---
I think I know too much about backlight. And by too much I mean that I
know enough to know I don't know nowhere near enough, and that thought
really scares me. I don't want to know more about backlight. I'd rather
regress to the blissful ignorance and the naïve misconception that
backlight should be easy.
BR,
Jani.
>
>> } 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;
>> }
>> }
>>
>>
>
--
Jani Nikula, Intel Open Source Technology Center
More information about the Intel-gfx
mailing list