Re: [PATCH 2/3] drm/i915: correct BLC vs PWM enable/disable ordering

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Wed, 25 Jun 2014 15:21:12 +0300
Jani Nikula <jani.nikula@linux.intel.com> wrote:

> On Mon, 31 Mar 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > With the new checks in place, we can see we're doing things backwards,
> > so fix them up per the spec.
> >
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index b6f7087..d540fbe 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1273,8 +1273,6 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
> >  
> >  	DRM_DEBUG_KMS("Turn eDP power off\n");
> >  
> > -	edp_wait_backlight_off(intel_dp);
> > -
> 
> Please justify moving this wait to intel_edp_backlight_off. I thought
> the point of these wait calls is such that we'll only end up waiting
> when we really have to. If this is left as-is, we can do useful stuff
> *while* waiting (case in point, intel_dp_sink_dpms in intel_disable_dp).

The wait needs to happen between the BLC disable and the backlight
disable per the eDP timing spec.  We could put the disable into a
delayed work queue if you want to reclaim the time, but it should be
pretty small compared to a full panel power sequence.

The wait here looks like it was to prevent us from re-enabling the
backlight too quickly, but I don't have timing info for that; not sure
if there are specific requirements there or not.

Jesse

> Otherwise this looks good to me, although I didn't find proper
> explanations for everything. Do I understand correctly that the
> EDP_BLC_ENABLE bit in PP_CONTROL is basically the final switch for
> enabling/disabling the PWM signal to the eDP? So before this patch, we
> let the disabled PWM signal through to the panel?

Right, something like that.  Enabling PWM starts driving power to some
components, but the PP_CONTROL bit controls whether it actually gets
out to the panel meaningfully, and at least according to the scope
readouts we have, doing it in this order corrects the backward ordering
we saw.

-- 
Jesse Barnes, Intel Open Source Technology Center