Re: [PATCH] drm/i915: Update PSR on resume.

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jun 10, 2014 at 08:11:41AM -0700, Rodrigo Vivi wrote:
> would you be ok by a patch that doesn't trigger the psr_update but just set
> psr.setup_done = false on resume?
> 
> I don't want to do more setup than really needed.
> 
> Or you mean move even psr.setup_done to crtc_enable?

Yeah, if there's some setup we need to do (e.g. figure out whether we can
do psr or set up the hw) we should do it in crtc_enable.

That's the only common function for modeset stuff shared between system
resume and runtime resume, and we'll loose register values in both cases.
-Daniel

> 
> 
> On Thu, Jun 5, 2014 at 2:15 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Wed, Jun 04, 2014 at 12:17:14PM -0700, Rodrigo Vivi wrote:
> > > On Wed, May 28, 2014 at 5:57 AM, Daniel Vetter <daniel@ffwll.ch> 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 <rodrigo.vivi@gmail.com>
> > > > > ---
> > > > >  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
> >
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br

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