Re: [PATCH] drm/i915: Instrument PSR parameter for possible quirks with link standby.

["Vivi, Rodrigo" <rodrigo.vivi@intel.com>]

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
> 
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx