| Message ID | 1449823782-2485-1-git-send-email-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2015-12-11 6:49 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>: > Link standby support has been deprecated with 'commit 89251b177 > ("drm/i915: PSR: deprecate link_standby support for core platforms.")' > > The reason for that is that main link in full off offers more power > savings and some platforms implementations on source side had known > bugs with link standby. I also read that for link_standby to work we need a workaround that involves single frame update support, but that's impossible on Haswell and would require 3 additional workarounds on Broadwell, which I'm assuming we don't implement yet. So why are we bringing this back if we know it won't work? > > However we don't know all panels out there and we don't fully rely > on the VBT information after the case found with the commit that > made us to deprecate link standby. Well, we kinda rely on the VBT. From what I see inside intel_psr_match_conditions(), if the VBT requests link standby mode and we're not VLV/CHV, we disable PSR (even if the user passes i915.enable_psr=2, which sounds like another problem). > > So, before enable PSR by default let's instrument the PSR parameter > in a way that we can identify different panels out there that might > require or work better with link standby mode. > > It is also useful to say that for backward compatibility I'm not > changing the meaning of this flag. So "0" still means disabled > and "1" means enabled with full support and maximum power savings. > > v2: Use positive value instead of negative for different operation mode > as suggested by Daniel. > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 5 +++++ > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_params.c | 7 ++++++- > drivers/gpu/drm/i915/intel_psr.c | 13 ++++++++++++- > 4 files changed, 24 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 24318b7..efe973b 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2567,6 +2567,10 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) > enabled = true; > } > } > + > + seq_printf(m, "Forcing main link standby: %s\n", > + yesno(dev_priv->psr.link_standby)); > + > seq_printf(m, "HW Enabled & Active bit: %s", yesno(enabled)); > > if (!HAS_DDI(dev)) > @@ -2587,6 +2591,7 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) > > seq_printf(m, "Performance_Counter: %u\n", psrperf); > } > + Bad chunk. > mutex_unlock(&dev_priv->psr.lock); > > intel_runtime_pm_put(dev_priv); > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 5edd393..de086f0 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -969,6 +969,7 @@ struct i915_psr { > unsigned busy_frontbuffer_bits; > bool psr2_support; > bool aux_frame_sync; > + bool link_standby; > }; > > enum intel_pch { > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c > index 835d609..6dd39f0 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -126,7 +126,12 @@ MODULE_PARM_DESC(enable_execlists, > "(-1=auto [default], 0=disabled, 1=enabled)"); > > module_param_named_unsafe(enable_psr, i915.enable_psr, int, 0600); > -MODULE_PARM_DESC(enable_psr, "Enable PSR (default: false)"); > +MODULE_PARM_DESC(enable_psr, "Enable PSR " > + "(0=disabled [default], 1=link-off maximum power-savings, 2=link-standby mode)" It's not clear which value should be tried by the user in case he wants to try PSR, but I don't think this matters, right? Also, on VLV/CHV, i915.enable_psr=1 will still use link standby mode (check vlv_psr_enable_sink()). So if we keep this patch, maybe we should do: 0 = disabled 1 = enabled (let the Kernel choose the link mode) 2 = force link off 3 = force link standby Although I'm not sure how we're supposed to proceed in case someone sets i915.enable_psr=2 on VLV with my suggestion. > + "In case you needed to force it on standby or disabled, please " > + "report PCI device ID, subsystem vendor and subsystem device ID " > + "to intel-gfx@lists.freedesktop.org, if your machine needs it. " > + "It will then be included in an upcoming module version."); > > module_param_named_unsafe(preliminary_hw_support, i915.preliminary_hw_support, int, 0600); > MODULE_PARM_DESC(preliminary_hw_support, > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index 9ccff30..bcc85fd 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -225,7 +225,12 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp) > (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT)); > } > > - drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, DP_PSR_ENABLE); > + if (dev_priv->psr.link_standby) > + drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, > + DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE); > + else > + drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, > + DP_PSR_ENABLE); > } > > static void vlv_psr_enable_source(struct intel_dp *intel_dp) > @@ -280,6 +285,9 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp) > if (IS_HASWELL(dev)) > val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES; > > + if (dev_priv->psr.link_standby) > + val |= EDP_PSR_LINK_STANDBY; > + > I915_WRITE(EDP_PSR_CTL, val | > max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT | > idle_frames << EDP_PSR_IDLE_FRAME_SHIFT | > @@ -763,6 +771,9 @@ void intel_psr_init(struct drm_device *dev) > dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ? > HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE; > > + if (i915.enable_psr == 2) > + dev_priv->psr.link_standby = true; > + > INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work); > mutex_init(&dev_priv->psr.lock); > } > -- > 2.4.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, 2015-12-11 at 17:55 -0200, Paulo Zanoni wrote: > 2015-12-11 6:49 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>: > > Link standby support has been deprecated with 'commit 89251b177 > > ("drm/i915: PSR: deprecate link_standby support for core > > platforms.")' > > > > The reason for that is that main link in full off offers more power > > savings and some platforms implementations on source side had known > > bugs with link standby. > > I also read that for link_standby to work we need a workaround that > involves single frame update support, but that's impossible on > Haswell > and would require 3 additional workarounds on Broadwell, which I'm > assuming we don't implement yet. So why are we bringing this back if > we know it won't work? I found a Skylake that although VBT doesn't tell anything about link standby the machine works better on link standby. But maybe if HSW || BDW we should just disable PSR at if link_standby needed and if SKL we can force one or another. > > > > > However we don't know all panels out there and we don't fully rely > > on the VBT information after the case found with the commit that > > made us to deprecate link standby. > > Well, we kinda rely on the VBT. From what I see inside > intel_psr_match_conditions(), if the VBT requests link standby mode > and we're not VLV/CHV, we disable PSR (even if the user passes > i915.enable_psr=2, which sounds like another problem). Oh, I had forgotten about this one. Let'me re-work that in a way that we continue relying VBT but provide a way to force one behaviour or another. > > > > > So, before enable PSR by default let's instrument the PSR parameter > > in a way that we can identify different panels out there that might > > require or work better with link standby mode. > > > > It is also useful to say that for backward compatibility I'm not > > changing the meaning of this flag. So "0" still means disabled > > and "1" means enabled with full support and maximum power savings. > > > > v2: Use positive value instead of negative for different operation > > mode > > as suggested by Daniel. > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/i915/i915_debugfs.c | 5 +++++ > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > drivers/gpu/drm/i915/i915_params.c | 7 ++++++- > > drivers/gpu/drm/i915/intel_psr.c | 13 ++++++++++++- > > 4 files changed, 24 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > b/drivers/gpu/drm/i915/i915_debugfs.c > > index 24318b7..efe973b 100644 > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > @@ -2567,6 +2567,10 @@ static int i915_edp_psr_status(struct > > seq_file *m, void *data) > > enabled = true; > > } > > } > > + > > + seq_printf(m, "Forcing main link standby: %s\n", > > + yesno(dev_priv->psr.link_standby)); > > + > > seq_printf(m, "HW Enabled & Active bit: %s", > > yesno(enabled)); > > > > if (!HAS_DDI(dev)) > > @@ -2587,6 +2591,7 @@ static int i915_edp_psr_status(struct > > seq_file *m, void *data) > > > > seq_printf(m, "Performance_Counter: %u\n", > > psrperf); > > } > > + > > Bad chunk. ops! > > > > mutex_unlock(&dev_priv->psr.lock); > > > > intel_runtime_pm_put(dev_priv); > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h > > index 5edd393..de086f0 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -969,6 +969,7 @@ struct i915_psr { > > unsigned busy_frontbuffer_bits; > > bool psr2_support; > > bool aux_frame_sync; > > + bool link_standby; > > }; > > > > enum intel_pch { > > diff --git a/drivers/gpu/drm/i915/i915_params.c > > b/drivers/gpu/drm/i915/i915_params.c > > index 835d609..6dd39f0 100644 > > --- a/drivers/gpu/drm/i915/i915_params.c > > +++ b/drivers/gpu/drm/i915/i915_params.c > > @@ -126,7 +126,12 @@ MODULE_PARM_DESC(enable_execlists, > > "(-1=auto [default], 0=disabled, 1=enabled)"); > > > > module_param_named_unsafe(enable_psr, i915.enable_psr, int, 0600); > > -MODULE_PARM_DESC(enable_psr, "Enable PSR (default: false)"); > > +MODULE_PARM_DESC(enable_psr, "Enable PSR " > > + "(0=disabled [default], 1=link-off maximum power > > -savings, 2=link-standby mode)" > > It's not clear which value should be tried by the user in case he > wants to try PSR, but I don't think this matters, right? > > Also, on VLV/CHV, i915.enable_psr=1 will still use link standby mode > (check vlv_psr_enable_sink()). So if we keep this patch, maybe we > should do: > 0 = disabled > 1 = enabled (let the Kernel choose the link mode) > 2 = force link off > 3 = force link standby Great idea! Thanks! > Although I'm not sure how we're supposed to proceed in case someone > sets i915.enable_psr=2 on VLV with my suggestion. force what user wants anyway... we warn these are unsafe options ;) > > > > + "In case you needed to force it on standby or > > disabled, please " > > + "report PCI device ID, subsystem vendor and > > subsystem device ID " > > + "to intel-gfx@lists.freedesktop.org, if your > > machine needs it. " > > + "It will then be included in an upcoming module > > version."); > > > > module_param_named_unsafe(preliminary_hw_support, > > i915.preliminary_hw_support, int, 0600); > > MODULE_PARM_DESC(preliminary_hw_support, > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > b/drivers/gpu/drm/i915/intel_psr.c > > index 9ccff30..bcc85fd 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -225,7 +225,12 @@ static void hsw_psr_enable_sink(struct > > intel_dp *intel_dp) > > (aux_clock_divider << > > DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT)); > > } > > > > - drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, > > DP_PSR_ENABLE); > > + if (dev_priv->psr.link_standby) > > + drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, > > + DP_PSR_ENABLE | > > DP_PSR_MAIN_LINK_ACTIVE); > > + else > > + drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, > > + DP_PSR_ENABLE); > > } > > > > static void vlv_psr_enable_source(struct intel_dp *intel_dp) > > @@ -280,6 +285,9 @@ static void hsw_psr_enable_source(struct > > intel_dp *intel_dp) > > if (IS_HASWELL(dev)) > > val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES; > > > > + if (dev_priv->psr.link_standby) > > + val |= EDP_PSR_LINK_STANDBY; > > + > > I915_WRITE(EDP_PSR_CTL, val | > > max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT | > > idle_frames << EDP_PSR_IDLE_FRAME_SHIFT | > > @@ -763,6 +771,9 @@ void intel_psr_init(struct drm_device *dev) > > dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ? > > HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE; > > > > + if (i915.enable_psr == 2) > > + dev_priv->psr.link_standby = true; > > + > > INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work); > > mutex_init(&dev_priv->psr.lock); > > } > > -- > > 2.4.3 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > >
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 24318b7..efe973b 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2567,6 +2567,10 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) enabled = true; } } + + seq_printf(m, "Forcing main link standby: %s\n", + yesno(dev_priv->psr.link_standby)); + seq_printf(m, "HW Enabled & Active bit: %s", yesno(enabled)); if (!HAS_DDI(dev)) @@ -2587,6 +2591,7 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) seq_printf(m, "Performance_Counter: %u\n", psrperf); } + mutex_unlock(&dev_priv->psr.lock); intel_runtime_pm_put(dev_priv); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 5edd393..de086f0 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -969,6 +969,7 @@ struct i915_psr { unsigned busy_frontbuffer_bits; bool psr2_support; bool aux_frame_sync; + bool link_standby; }; enum intel_pch { diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index 835d609..6dd39f0 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -126,7 +126,12 @@ MODULE_PARM_DESC(enable_execlists, "(-1=auto [default], 0=disabled, 1=enabled)"); module_param_named_unsafe(enable_psr, i915.enable_psr, int, 0600); -MODULE_PARM_DESC(enable_psr, "Enable PSR (default: false)"); +MODULE_PARM_DESC(enable_psr, "Enable PSR " + "(0=disabled [default], 1=link-off maximum power-savings, 2=link-standby mode)" + "In case you needed to force it on standby or disabled, please " + "report PCI device ID, subsystem vendor and subsystem device ID " + "to intel-gfx@lists.freedesktop.org, if your machine needs it. " + "It will then be included in an upcoming module version."); module_param_named_unsafe(preliminary_hw_support, i915.preliminary_hw_support, int, 0600); MODULE_PARM_DESC(preliminary_hw_support, diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index 9ccff30..bcc85fd 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -225,7 +225,12 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp) (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT)); } - drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, DP_PSR_ENABLE); + if (dev_priv->psr.link_standby) + drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, + DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE); + else + drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, + DP_PSR_ENABLE); } static void vlv_psr_enable_source(struct intel_dp *intel_dp) @@ -280,6 +285,9 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp) if (IS_HASWELL(dev)) val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES; + if (dev_priv->psr.link_standby) + val |= EDP_PSR_LINK_STANDBY; + I915_WRITE(EDP_PSR_CTL, val | max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT | idle_frames << EDP_PSR_IDLE_FRAME_SHIFT | @@ -763,6 +771,9 @@ void intel_psr_init(struct drm_device *dev) dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ? HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE; + if (i915.enable_psr == 2) + dev_priv->psr.link_standby = true; + INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work); mutex_init(&dev_priv->psr.lock); }
Link standby support has been deprecated with 'commit 89251b177 ("drm/i915: PSR: deprecate link_standby support for core platforms.")' The reason for that is that main link in full off offers more power savings and some platforms implementations on source side had known bugs with link standby. However we don't know all panels out there and we don't fully rely on the VBT information after the case found with the commit that made us to deprecate link standby. So, before enable PSR by default let's instrument the PSR parameter in a way that we can identify different panels out there that might require or work better with link standby mode. It is also useful to say that for backward compatibility I'm not changing the meaning of this flag. So "0" still means disabled and "1" means enabled with full support and maximum power savings. v2: Use positive value instead of negative for different operation mode as suggested by Daniel. Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 5 +++++ drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_params.c | 7 ++++++- drivers/gpu/drm/i915/intel_psr.c | 13 ++++++++++++- 4 files changed, 24 insertions(+), 2 deletions(-)