| Message ID | 1401234614-25220-1-git-send-email-rodrigo.vivi@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > + > /* only restore FBC info on the platform that supports FBC*/ > intel_disable_fbc(dev); > > -- > 1.9.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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... > > > + > > /* only restore FBC info on the platform that supports FBC*/ > > intel_disable_fbc(dev); > > > > -- > > 1.9.0 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch >
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
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? 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 >
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
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); + /* only restore FBC info on the platform that supports FBC*/ intel_disable_fbc(dev);
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(+)