| Message ID | 1454925346-22479-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2/8/2016 3:25 PM, Daniel Vetter wrote: > eDP already restores PP state completely on it's own, we only need > this code for LVDS. Since it's more work to move this into the lvds > encoder properly just limit it to affected pch chips for now > (ibx&cpt/ppt). > > Cc: Jani Nikula <jani.nikula@intel.com> > Acked-by: Jani Nikula <jani.nikula@intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/i915/i915_suspend.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c > index a2aa09ce3202..7f6b050266a7 100644 > --- a/drivers/gpu/drm/i915/i915_suspend.c > +++ b/drivers/gpu/drm/i915/i915_suspend.c > @@ -43,8 +43,8 @@ static void i915_save_display(struct drm_device *dev) > else if (INTEL_INFO(dev)->gen <= 4 && IS_MOBILE(dev) && !IS_I830(dev)) > dev_priv->regfile.saveLVDS = I915_READ(LVDS); > > - /* Panel power sequencer */ > - if (HAS_PCH_SPLIT(dev)) { > + /* Panel power sequencer, only needed for LVDS */ > + if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) { > dev_priv->regfile.savePP_CONTROL = I915_READ(PCH_PP_CONTROL); > dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PCH_PP_ON_DELAYS); > dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PCH_PP_OFF_DELAYS); won't this code execute for eDP too ? Also it seems incorrect we are saving and restoring PP_CONTROL register too when we should have followed proper sequence delays for each bit in PP_CONTROL. This will end up bring up the panel before modeset in best case or damage the panel in the worst case. Sivakumar > @@ -78,8 +78,8 @@ static void i915_restore_display(struct drm_device *dev) > else if (INTEL_INFO(dev)->gen <= 4 && IS_MOBILE(dev) && !IS_I830(dev)) > I915_WRITE(LVDS, dev_priv->regfile.saveLVDS & mask); > > - /* Panel power sequencer */ > - if (HAS_PCH_SPLIT(dev)) { > + /* Panel power sequencer, only needed for LVDS */ > + if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) { > I915_WRITE(PCH_PP_ON_DELAYS, dev_priv->regfile.savePP_ON_DELAYS); > I915_WRITE(PCH_PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS); > I915_WRITE(PCH_PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR);
On Tue, 09 Feb 2016, "Thulasimani, Sivakumar" <sivakumar.thulasimani@intel.com> wrote: > On 2/8/2016 3:25 PM, Daniel Vetter wrote: >> eDP already restores PP state completely on it's own, we only need >> this code for LVDS. Since it's more work to move this into the lvds >> encoder properly just limit it to affected pch chips for now >> (ibx&cpt/ppt). >> >> Cc: Jani Nikula <jani.nikula@intel.com> >> Acked-by: Jani Nikula <jani.nikula@intel.com> >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >> --- >> drivers/gpu/drm/i915/i915_suspend.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c >> index a2aa09ce3202..7f6b050266a7 100644 >> --- a/drivers/gpu/drm/i915/i915_suspend.c >> +++ b/drivers/gpu/drm/i915/i915_suspend.c >> @@ -43,8 +43,8 @@ static void i915_save_display(struct drm_device *dev) >> else if (INTEL_INFO(dev)->gen <= 4 && IS_MOBILE(dev) && !IS_I830(dev)) >> dev_priv->regfile.saveLVDS = I915_READ(LVDS); >> >> - /* Panel power sequencer */ >> - if (HAS_PCH_SPLIT(dev)) { >> + /* Panel power sequencer, only needed for LVDS */ >> + if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) { >> dev_priv->regfile.savePP_CONTROL = I915_READ(PCH_PP_CONTROL); >> dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PCH_PP_ON_DELAYS); >> dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PCH_PP_OFF_DELAYS); > won't this code execute for eDP too ? Also it seems incorrect we are > saving and restoring PP_CONTROL register too when we should have > followed proper sequence delays for each bit in PP_CONTROL. This will > end up bring up the panel before modeset in best case or damage the > panel in the worst case. Exactly the reasons why we are trying to limit this code to fewer platforms! This is very fragile stuff though, I fear we might regress something (even if that something is currently working by coincidence) if we limit this further, in one go. I'd like to do this little by little, and see what breaks, if anything. With this patch, we'd already limit all of this hackery to cougarpoint and older, instead of all PCH split up to and including Skylake. If anyone wants to dig into LVDS power sequencing after that, I'm not opposed... BR, Jani. > > Sivakumar > >> @@ -78,8 +78,8 @@ static void i915_restore_display(struct drm_device *dev) >> else if (INTEL_INFO(dev)->gen <= 4 && IS_MOBILE(dev) && !IS_I830(dev)) >> I915_WRITE(LVDS, dev_priv->regfile.saveLVDS & mask); >> >> - /* Panel power sequencer */ >> - if (HAS_PCH_SPLIT(dev)) { >> + /* Panel power sequencer, only needed for LVDS */ >> + if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) { >> I915_WRITE(PCH_PP_ON_DELAYS, dev_priv->regfile.savePP_ON_DELAYS); >> I915_WRITE(PCH_PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS); >> I915_WRITE(PCH_PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR); >
On 2/9/2016 11:54 AM, Jani Nikula wrote: > On Tue, 09 Feb 2016, "Thulasimani, Sivakumar" <sivakumar.thulasimani@intel.com> wrote: >> On 2/8/2016 3:25 PM, Daniel Vetter wrote: >>> eDP already restores PP state completely on it's own, we only need >>> this code for LVDS. Since it's more work to move this into the lvds >>> encoder properly just limit it to affected pch chips for now >>> (ibx&cpt/ppt). >>> >>> Cc: Jani Nikula <jani.nikula@intel.com> >>> Acked-by: Jani Nikula <jani.nikula@intel.com> >>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_suspend.c | 8 ++++---- >>> 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c >>> index a2aa09ce3202..7f6b050266a7 100644 >>> --- a/drivers/gpu/drm/i915/i915_suspend.c >>> +++ b/drivers/gpu/drm/i915/i915_suspend.c >>> @@ -43,8 +43,8 @@ static void i915_save_display(struct drm_device *dev) >>> else if (INTEL_INFO(dev)->gen <= 4 && IS_MOBILE(dev) && !IS_I830(dev)) >>> dev_priv->regfile.saveLVDS = I915_READ(LVDS); >>> >>> - /* Panel power sequencer */ >>> - if (HAS_PCH_SPLIT(dev)) { >>> + /* Panel power sequencer, only needed for LVDS */ >>> + if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) { >>> dev_priv->regfile.savePP_CONTROL = I915_READ(PCH_PP_CONTROL); >>> dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PCH_PP_ON_DELAYS); >>> dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PCH_PP_OFF_DELAYS); >> won't this code execute for eDP too ? Also it seems incorrect we are >> saving and restoring PP_CONTROL register too when we should have >> followed proper sequence delays for each bit in PP_CONTROL. This will >> end up bring up the panel before modeset in best case or damage the >> panel in the worst case. > Exactly the reasons why we are trying to limit this code to fewer > platforms! This is very fragile stuff though, I fear we might regress > something (even if that something is currently working by coincidence) > if we limit this further, in one go. I'd like to do this little by > little, and see what breaks, if anything. > > With this patch, we'd already limit all of this hackery to cougarpoint > and older, instead of all PCH split up to and including Skylake. If > anyone wants to dig into LVDS power sequencing after that, I'm not > opposed... > > BR, > Jani. missed the part where this patch is reducing the platforms :). can we reduce this to systems with LVDS alone ? i am sure eDP will blankout if pps is not followed but have observed some LVDS panels working even without pps delays. Sivakumar > >> Sivakumar >> >>> @@ -78,8 +78,8 @@ static void i915_restore_display(struct drm_device *dev) >>> else if (INTEL_INFO(dev)->gen <= 4 && IS_MOBILE(dev) && !IS_I830(dev)) >>> I915_WRITE(LVDS, dev_priv->regfile.saveLVDS & mask); >>> >>> - /* Panel power sequencer */ >>> - if (HAS_PCH_SPLIT(dev)) { >>> + /* Panel power sequencer, only needed for LVDS */ >>> + if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) { >>> I915_WRITE(PCH_PP_ON_DELAYS, dev_priv->regfile.savePP_ON_DELAYS); >>> I915_WRITE(PCH_PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS); >>> I915_WRITE(PCH_PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR);
diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c index a2aa09ce3202..7f6b050266a7 100644 --- a/drivers/gpu/drm/i915/i915_suspend.c +++ b/drivers/gpu/drm/i915/i915_suspend.c @@ -43,8 +43,8 @@ static void i915_save_display(struct drm_device *dev) else if (INTEL_INFO(dev)->gen <= 4 && IS_MOBILE(dev) && !IS_I830(dev)) dev_priv->regfile.saveLVDS = I915_READ(LVDS); - /* Panel power sequencer */ - if (HAS_PCH_SPLIT(dev)) { + /* Panel power sequencer, only needed for LVDS */ + if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) { dev_priv->regfile.savePP_CONTROL = I915_READ(PCH_PP_CONTROL); dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PCH_PP_ON_DELAYS); dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PCH_PP_OFF_DELAYS); @@ -78,8 +78,8 @@ static void i915_restore_display(struct drm_device *dev) else if (INTEL_INFO(dev)->gen <= 4 && IS_MOBILE(dev) && !IS_I830(dev)) I915_WRITE(LVDS, dev_priv->regfile.saveLVDS & mask); - /* Panel power sequencer */ - if (HAS_PCH_SPLIT(dev)) { + /* Panel power sequencer, only needed for LVDS */ + if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) { I915_WRITE(PCH_PP_ON_DELAYS, dev_priv->regfile.savePP_ON_DELAYS); I915_WRITE(PCH_PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS); I915_WRITE(PCH_PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR);