| Message ID | 1407518374-7886-1-git-send-email-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Aug 8, 2014 at 7:19 PM, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > From: Rodrigo Vivi <rodrigo.vivi@gmail.com> > > 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. > v3: Getting back old setup_done variable and move from resume to crtc_enable > as Daniel requested. > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> I'm confused ... whats the use of this? Afaict that's exactly what the code currently does. -Daniel
Well, this fix the issue Linus faced. Actually the issue I was aware of and trying to fix with this patch for a long time was reported by chromeos guys saying the psr wasn't propperly working after suspend/resume. They got the screen back but never got psr back again. The original patch fix suspend/resume issues with psr and I decided to keep the same and subject for reference of what the problem this fixes and what was the patch history, but changed the place for the full setup to crtc_enable per your recommendation. Thanks, Rodrigo. On Sat, Aug 9, 2014 at 12:40 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > On Fri, Aug 8, 2014 at 7:19 PM, Rodrigo Vivi <rodrigo.vivi@intel.com> > wrote: > > From: Rodrigo Vivi <rodrigo.vivi@gmail.com> > > > > 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. > > v3: Getting back old setup_done variable and move from resume to > crtc_enable > > as Daniel requested. > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > I'm confused ... whats the use of this? Afaict that's exactly what the > code currently does. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch >
On Mon, Aug 11, 2014 at 09:57:12AM -0700, Rodrigo Vivi wrote: > Well, this fix the issue Linus faced. > > Actually the issue I was aware of and trying to fix with this patch for a > long time was reported by chromeos guys saying the psr wasn't propperly > working after suspend/resume. They got the screen back but never got psr > back again. > > The original patch fix suspend/resume issues with psr and I decided to keep > the same and subject for reference of what the problem this fixes and what > was the patch history, but changed the place for the full setup to > crtc_enable per your recommendation. So if I read your patch correctly then we no do the initial psr setup in crtc_enable, but only after driver load/resume. Before we do it always when crtc_enable is called. That doesn't make sense. Also note that Linus said it's also sometimes broken when using X normally. Otoh if there's really something we need to in crtc_enable in addition to what we do already, then we _must_ do it always. Otherwise runtime pm (which is pretty much the same as system resume) will not work. -Daniel > > Thanks, > Rodrigo. > > > > On Sat, Aug 9, 2014 at 12:40 AM, Daniel Vetter <daniel.vetter@ffwll.ch> > wrote: > > > On Fri, Aug 8, 2014 at 7:19 PM, Rodrigo Vivi <rodrigo.vivi@intel.com> > > wrote: > > > From: Rodrigo Vivi <rodrigo.vivi@gmail.com> > > > > > > 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. > > > v3: Getting back old setup_done variable and move from resume to > > crtc_enable > > > as Daniel requested. > > > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > I'm confused ... whats the use of this? Afaict that's exactly what the > > code currently does. > > -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_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 125a83c..9ef8dfc 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -674,6 +674,7 @@ struct i915_psr { struct mutex lock; bool sink_support; bool source_ok; + bool setup_done; struct intel_dp *enabled; bool active; struct delayed_work work; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 89e0ac5..5f8e4f6 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3972,6 +3972,11 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) I915_WRITE(DSPCNTR(plane), DISPPLANE_GAMMA_ENABLE); POSTING_READ(DSPCNTR(plane)); + /* Forcing a full psr init sequence when enabling crtc to make sure all + * registers are properly set. Some might not be persistent after + * suspend/resume cycle. */ + dev_priv->psr.setup_done = false; + dev_priv->display.update_primary_plane(crtc, crtc->primary->fb, crtc->x, crtc->y); diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 34e3c47..5fde763 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1709,6 +1709,9 @@ static void intel_edp_psr_setup(struct intel_dp *intel_dp) struct drm_i915_private *dev_priv = dev->dev_private; struct edp_vsc_psr psr_vsc; + if (dev_priv->psr.setup_done) + return; + /* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */ memset(&psr_vsc, 0, sizeof(psr_vsc)); psr_vsc.sdp_header.HB0 = 0; @@ -1720,6 +1723,8 @@ static void intel_edp_psr_setup(struct intel_dp *intel_dp) /* Avoid continuous PSR exit by masking memup and hpd */ I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP | EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP); + + dev_priv->psr.setup_done = true; } static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp) @@ -1839,6 +1844,9 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp) WARN_ON(dev_priv->psr.active); lockdep_assert_held(&dev_priv->psr.lock); + /* Setup PSR once */ + intel_edp_psr_setup(intel_dp); + /* Enable PSR on the panel */ intel_edp_psr_enable_sink(intel_dp); @@ -1872,9 +1880,6 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp) dev_priv->psr.busy_frontbuffer_bits = 0; - /* Setup PSR once */ - intel_edp_psr_setup(intel_dp); - if (intel_edp_psr_match_conditions(intel_dp)) dev_priv->psr.enabled = intel_dp; mutex_unlock(&dev_priv->psr.lock);