From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jani Nikula Subject: Re: [PATCH 2/3] drm/i915: correct BLC vs PWM enable/disable ordering Date: Thu, 26 Jun 2014 10:58:37 +0300 Message-ID: <87y4wk9k5u.fsf@intel.com> References: <1396289637-1013-1-git-send-email-jbarnes@virtuousgeek.org> <1396289637-1013-2-git-send-email-jbarnes@virtuousgeek.org> <87mwd16uyv.fsf@intel.com> <20140625080236.7b8dae8c@jbarnes-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 8EF2E6E6F9 for ; Thu, 26 Jun 2014 00:58:45 -0700 (PDT) In-Reply-To: <20140625080236.7b8dae8c@jbarnes-desktop> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Jesse Barnes Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Wed, 25 Jun 2014, Jesse Barnes wrote: > On Wed, 25 Jun 2014 15:21:12 +0300 > Jani Nikula wrote: > >> On Mon, 31 Mar 2014, Jesse Barnes 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 >> > --- >> > 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