<div dir="ltr">would you be ok by a patch that doesn't trigger the psr_update but just set psr.setup_done = false on resume?<div><br></div><div>I don't want to do more setup than really needed.</div><div><br></div>
<div>Or you mean move even psr.setup_done to crtc_enable?</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Jun 5, 2014 at 2:15 AM, Daniel Vetter <span dir="ltr"><<a href="mailto:daniel@ffwll.ch" target="_blank">daniel@ffwll.ch</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Wed, Jun 04, 2014 at 12:17:14PM -0700, Rodrigo Vivi wrote:<br>
> On Wed, May 28, 2014 at 5:57 AM, Daniel Vetter <<a href="mailto:daniel@ffwll.ch">daniel@ffwll.ch</a>> wrote:<br>
><br>
> > On Tue, May 27, 2014 at 04:50:14PM -0700, Rodrigo Vivi wrote:<br>
> > > Some registers set during setup might not be persistent after<br>
> > suspend/resume.<br>
> > > This was causing bugs for some people that was unable to get PSR entry<br>
> > state<br>
> > > after resume cycle.<br>
> > ><br>
> > > v2: Adding some comments and better commit message explaining why this<br>
> > is needed.<br>
> > ><br>
> > > Signed-off-by: Rodrigo Vivi <<a href="mailto:rodrigo.vivi@gmail.com">rodrigo.vivi@gmail.com</a>><br>
> > > ---<br>
> > >  drivers/gpu/drm/i915/i915_suspend.c | 6 ++++++<br>
> > >  1 file changed, 6 insertions(+)<br>
> > ><br>
> > > diff --git a/drivers/gpu/drm/i915/i915_suspend.c<br>
> > b/drivers/gpu/drm/i915/i915_suspend.c<br>
> > > index 56785e8..1923708 100644<br>
> > > --- a/drivers/gpu/drm/i915/i915_suspend.c<br>
> > > +++ b/drivers/gpu/drm/i915/i915_suspend.c<br>
> > > @@ -288,6 +288,12 @@ static void i915_restore_display(struct drm_device<br>
> > *dev)<br>
> > >               I915_WRITE(PP_CONTROL, dev_priv->regfile.savePP_CONTROL);<br>
> > >       }<br>
> > ><br>
> > > +     /* Forcing a full init sequence after resume to make sure all<br>
> > > +     * registers are properly set. Some might not be persistent after<br>
> > > +     * suspend/resume cycle. */<br>
> > > +     dev_priv->psr.setup_done = false;<br>
> > > +     intel_edp_psr_update(dev);<br>
> ><br>
> > No, crtc_enable should take care of this. There's more places (like after<br>
> > runtime pm) where the hw has potentially lost all register contents, so<br>
> > crtc_enabl is the right place for this.<br>
> > -Daniel<br>
> ><br>
><br>
> crtc_enable takes care of the update, but not the setup part that is done<br>
> only once...<br>
> I do believe that only the setup_done = false is really needed here, but<br>
> doesn't heart to trigger the update right here<br>
> and fixes the bug...<br>
<br>
</div></div>restore_display hurts. If there's some setup we need to do, we _really_<br>
need to do it in crtc_enable. Splattering setup code all over the code is<br>
one of the prime sources of bugs we have wrt rpm, s/r and driver load.<br>
<span class="HOEnZb"><font color="#888888">-Daniel<br>
</font></span><div class="HOEnZb"><div class="h5">--<br>
Daniel Vetter<br>
Software Engineer, Intel Corporation<br>
<a href="tel:%2B41%20%280%29%2079%20365%2057%2048" value="+41793655748">+41 (0) 79 365 57 48</a> - <a href="http://blog.ffwll.ch" target="_blank">http://blog.ffwll.ch</a><br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div>Rodrigo Vivi</div><div>Blog: <a href="http://blog.vivi.eng.br" target="_blank">http://blog.vivi.eng.br</a></div><div> </div>
</div>