From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm/i915: Update PSR on resume. Date: Thu, 5 Jun 2014 11:15:29 +0200 Message-ID: <20140605091529.GT7416@phenom.ffwll.local> References: <1401234614-25220-1-git-send-email-rodrigo.vivi@gmail.com> <20140528125749.GR14841@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wi0-f176.google.com (mail-wi0-f176.google.com [209.85.212.176]) by gabe.freedesktop.org (Postfix) with ESMTP id E3CF16E926 for ; Thu, 5 Jun 2014 02:15:34 -0700 (PDT) Received: by mail-wi0-f176.google.com with SMTP id n15so9907711wiw.9 for ; Thu, 05 Jun 2014 02:15:34 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Rodrigo Vivi Cc: intel-gfx List-Id: intel-gfx@lists.freedesktop.org On Wed, Jun 04, 2014 at 12:17:14PM -0700, Rodrigo Vivi wrote: > On Wed, May 28, 2014 at 5:57 AM, Daniel Vetter wrote: > > > On Tue, May 27, 2014 at 04:50:14PM -0700, Rodrigo Vivi wrote: > > > Some registers set during setup might not be persistent after > > suspend/resume. > > > This was causing bugs for some people that was unable to get PSR entry > > state > > > after resume cycle. > > > > > > v2: Adding some comments and better commit message explaining why this > > is needed. > > > > > > Signed-off-by: Rodrigo Vivi > > > --- > > > drivers/gpu/drm/i915/i915_suspend.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_suspend.c > > b/drivers/gpu/drm/i915/i915_suspend.c > > > index 56785e8..1923708 100644 > > > --- a/drivers/gpu/drm/i915/i915_suspend.c > > > +++ b/drivers/gpu/drm/i915/i915_suspend.c > > > @@ -288,6 +288,12 @@ static void i915_restore_display(struct drm_device > > *dev) > > > I915_WRITE(PP_CONTROL, dev_priv->regfile.savePP_CONTROL); > > > } > > > > > > + /* Forcing a full init sequence after resume to make sure all > > > + * registers are properly set. Some might not be persistent after > > > + * suspend/resume cycle. */ > > > + dev_priv->psr.setup_done = false; > > > + intel_edp_psr_update(dev); > > > > No, crtc_enable should take care of this. There's more places (like after > > runtime pm) where the hw has potentially lost all register contents, so > > crtc_enabl is the right place for this. > > -Daniel > > > > crtc_enable takes care of the update, but not the setup part that is done > only once... > I do believe that only the setup_done = false is really needed here, but > doesn't heart to trigger the update right here > and fixes the bug... restore_display hurts. If there's some setup we need to do, we _really_ need to do it in crtc_enable. Splattering setup code all over the code is one of the prime sources of bugs we have wrt rpm, s/r and driver load. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch