[Intel-gfx] [PATCH] drm/i915/dp: Backlight PWM enable before BL Enable assert

[Daniel Vetter]

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 at linux.intel.com] 
> Sent: Friday, August 22, 2014 6:07 AM
> To: Taylor, Clinton A; Ville Syrjälä; Runyan, Arthur J
> Cc: Intel-gfx at 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 at 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 at lists.freedesktop.org
> >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >>
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch