On Fri, 04 Sep 2015, Jani Nikula <jani.nikula@xxxxxxxxx> wrote:
> On Thu, 27 Aug 2015, Clint Taylor <clinton.a.taylor@xxxxxxxxx> 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@xxxxxxxxx>
>>> ---
>>> 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.
Addition, for SKL/SPT we need to look at SOUTH_CHICKEN_1 bit 0.
BR,
Jani.
>
>>
>>> + 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
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
