VLV is a whole different can of worms. Some parts of display are from before sanybridge, and it may have some special muxing on the backlight pins. I won't be able to give any feedback on how hardware behaves there. -----Original Message----- From: Taylor, Clinton A Sent: Tuesday, September 02, 2014 5:27 PM To: Runyan, Arthur J; Daniel Vetter Cc: Jani Nikula; Ville Syrjälä; Intel-gfx@xxxxxxxxxxxxxxxxxxxxx Subject: Re: [PATCH] drm/i915/dp: Backlight PWM enable before BL Enable assert On 08/27/2014 01:54 PM, Runyan, Arthur J wrote: > :-) We pulled most of the mobile functions from the bridge chip into the main chip, so that same backlight code might well have been there. > From sandybridge onwards I see that hardware will override the PWM signal to inactive (0 if backlight polarity is active high, 1 if active low) when PWM is disabled (PWM control bit 31 = 0). I don't have anything older than sandybridge to look at, and of course there could always be some hidden bug that prevents the override from kicking in. > Just enabling/disabling the PWM control bit will reproduce the issue on VLV. I have not tested on other platforms. Clint > -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@xxxxxxxx] On Behalf Of Daniel Vetter > Sent: Tuesday, August 26, 2014 2:52 AM > To: Runyan, Arthur J > Cc: Jani Nikula; Taylor, Clinton A; Ville Syrjälä; Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH] drm/i915/dp: Backlight PWM enable before BL Enable assert > > On Fri, Aug 22, 2014 at 05:12:25PM +0000, Runyan, Arthur J wrote: >> I'll check it out. I haven't heard any complaint about this before, but >> it may be one of those things that started back on i745 and never got >> documented. > > Only i855 and later started to have native lvds with all the surrounding > stuff, before there's only bridge chips that did all the magic. So won't > be quite _that_ old ;-) > > Cheers, Daniel > >> >> -----Original Message----- >> From: Jani Nikula [mailto:jani.nikula@xxxxxxxxxxxxxxx] >> Sent: Friday, August 22, 2014 6:07 AM >> To: Taylor, Clinton A; Ville Syrjälä; Runyan, Arthur J >> Cc: Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> Subject: Re: [PATCH] drm/i915/dp: Backlight PWM enable before BL Enable assert >> >> >> +Art >> >> On Thu, 21 Aug 2014, Clint Taylor <clinton.a.taylor@xxxxxxxxx> wrote: >>> There is also a need to add this delay when turning off the PWM enable >>> bit through intel_panel_disable_backlight(). If the PWM enable bit is >>> turned off while the PWM signal is high then the signal remains high. If >>> the bit is turned off when the signal is low the PWM will remain low. >>> Since we don't know the current state of the PWM signal we must set the >>> duty_cycle to 0, wait PWM_CYCLE_DELAY, and then turn off the enable bit. >> >> [citation needed] >> >> Really, I want this in the specs if this is true. >> >>> Actually it might be better if we never turn off the PWM enable bit in >>> intel_panel_disable_backlight() and we just use the duty cycle register >>> to control the PWM. >> >> Art, any feedback on these two? >> >> BR, >> Jani. >> >> >>> >>>> So I guess your approach is the simplest option here. >>>> >>>>> _intel_edp_backlight_on(intel_dp); >>>>> } >>>>> >>>>> @@ -4256,9 +4257,10 @@ intel_dp_init_panel_power_sequencer(struct drm_device *dev, >>>>> assign_final(t11_t12); >>>>> #undef assign_final >>>>> >>>>> +#define PWM_CYCLE_DELAY 5 >>>> >>>> Shoduln't this be calculated from the PWM frequqncy? Not sure what a PWM >>>> cycle here is exactly. Just one full period of the signal? >>>> >>> >>> The PWM cycle in this case turns out to be 1 full cycle of the PWM >>> waveform though it depends on which display core clock (128 or >>> 25Mhz(S0ix) is being used. Since we only care about the minimum elapsed >>> time to meet the panel specification a value of 5ms will work even >>> though more or less PWM cycles would take place before the BL_ENABLE is >>> asserted. I would prefer not to add complexity to switch between panel >>> timings for normal and S0ix display clock modes. How often does the >>> backlight get enabled while in S0ix? >>> >>>> Also would be nice to have a comment in the code explaining what this is >>>> and why we need to add it to the delay. >>> >>> Sure, As you may have noticed in other patches I really like comments. >>> >>>> >>>>> #define get_delay(field) (DIV_ROUND_UP(final.field, 10)) >>>>> intel_dp->panel_power_up_delay = get_delay(t1_t3); >>>>> - intel_dp->backlight_on_delay = get_delay(t8); >>>>> + intel_dp->backlight_on_delay = get_delay(t8) + PWM_CYCLE_DELAY; >>>>> intel_dp->backlight_off_delay = get_delay(t9); >>>>> intel_dp->panel_power_down_delay = get_delay(t10); >>>>> intel_dp->panel_power_cycle_delay = get_delay(t11_t12); >>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >>>>> index 3abc915..ad6fcc1 100644 >>>>> --- a/drivers/gpu/drm/i915/intel_drv.h >>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h >>>>> @@ -556,6 +556,7 @@ struct intel_dp { >>>>> bool want_panel_vdd; >>>>> unsigned long last_power_cycle; >>>>> unsigned long last_power_on; >>>>> + unsigned long last_backlight_on; >>>>> unsigned long last_backlight_off; >>>>> >>>>> struct notifier_block edp_notifier; >>>>> -- >>>>> 1.8.3.2 >>>>> >>>>> _______________________________________________ >>>>> Intel-gfx mailing list >>>>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >>>> >>> >>> _______________________________________________ >>> Intel-gfx mailing list >>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> -- >> Jani Nikula, Intel Open Source Technology Center >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
