Re: [PATCH] drm/i915/dp: Backlight PWM enable before BL Enable assert

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

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@ffwll.ch] 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@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [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@linux.intel.com]
>> Sent: Friday, August 22, 2014 6:07 AM
>> To: Taylor, Clinton A; Ville Syrjälä; Runyan, Arthur J
>> Cc: Intel-gfx@lists.freedesktop.org
>> Subject: Re: [Intel-gfx] [PATCH] drm/i915/dp: Backlight PWM enable before BL Enable assert
>>
>>
>> +Art
>>
>> On Thu, 21 Aug 2014, Clint Taylor <clinton.a.taylor@intel.com> 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@lists.freedesktop.org
>>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>>
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>> --
>> Jani Nikula, Intel Open Source Technology Center
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>