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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 25 Jun 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> 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.

Okay, I'll take your word for it. ;) Do you still want to pursue patch
1?  If not, please pick up the comments from there into this one, and
add the above as a comment in there too.

Thanks,
Jani.


>
> 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

-- 
Jani Nikula, Intel Open Source Technology Center