Re: [Intel-gfx] [PATCH 2/5] drm/i915/psr: Kill scheduled work for Core platforms.
On Mon, 2018-02-26 at 15:12 -0800, Rodrigo Vivi wrote:
> On Fri, Feb 23, 2018 at 03:46:09PM -0800, Pandiyan, Dhinakaran wrote:
> > On Tue, 2018-02-13 at 15:26 -0800, Rodrigo Vivi wrote:
> > > It is a fact that scheduled work is now improved.
> > >
> > > But it is also a fact that on core platforms that shouldn't
> > > be needed. We only need to actually wait on VLV/CHV.
> > >
> > > The immediate enabling is actually not an issue for the
> > > HW perspective for core platforms that have HW tracking.
> > > HW will wait few identical idle frames before transitioning
> > > to actual psr active anyways.
> > >
> > > Note that this patch also remove the delayed activation
> > > on HSW and BDW introduced by commit 'd0ac896a477d
> > > ("drm/i915: Delay first PSR activation.")'. This was
> > > introduced to fix a blank screen on VLV/CHV and also
> > > masked some frozen screens on other core platforms.
> > > Probably the same that we are now properly hunting and fixing.
> > >
> > > Furthermore, if we stop using delayed activation on core
> > > platforms we will be able, on following up patches,
> > > to use available workarounds to make HW tracking properly
> > > exit PSR instead of the big nuke of disabling psr and
> > > re-enable on exit and activate respectively.
> > > At least on few reliable cases.
> > >
> > > Cc: Dhinakaran Pandiyan
> > > Signed-off-by: Rodrigo Vivi
> > > ---
> > > drivers/gpu/drm/i915/i915_debugfs.c | 14 +++---
> > > drivers/gpu/drm/i915/intel_psr.c| 27 +++
> > > 2 files changed, 26 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index da80ee16a3cf..541290c307c7 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -2522,18 +2522,18 @@ static int i915_edp_psr_status(struct seq_file
> > > *m, void *data)
> > > seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
> > > dev_priv->psr.busy_frontbuffer_bits);
> > >
> > > - if (timer_pending(_priv->psr.activate_timer))
> > > - seq_printf(m, "Activate scheduled: yes, in %dms\n",
> > > -
> > > jiffies_to_msecs(dev_priv->psr.activate_timer.expires - jiffies));
> > > - else
> > > - seq_printf(m, "Activate scheduled: no\n");
> > > -
> > > - if (HAS_DDI(dev_priv)) {
> > > + if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) {
> >
> > I don't get this change, it is better to retain HAS_DDI().
> >
> >
> > > if (dev_priv->psr.psr2_support)
> > > enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
> > > else
> > > enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
> > > } else {
> > > + if (timer_pending(_priv->psr.activate_timer))
> > > + seq_printf(m, "Activate scheduled: yes, in %dms\n",
> > > +
> > > jiffies_to_msecs(dev_priv->psr.activate_timer.expires - jiffies));
> > > + else
> > > + seq_printf(m, "Activate scheduled: no\n");
> > > +
> > > for_each_pipe(dev_priv, pipe) {
> > > enum transcoder cpu_transcoder =
> > > intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 826b480841ac..13409c6301e8 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -455,6 +455,8 @@ static void intel_psr_schedule(struct
> > > drm_i915_private *i915,
> > > {
> > > unsigned long next;
> > >
> > > + WARN_ON(!IS_VALLEYVIEW(i915) && !IS_CHERRYVIEW(i915));
> > > +
> > How about using only !(IS_VLV() || IS_CHV) in this file.
> >
> > I think this is a reasonable check to have, please add a return too.
> > WARN_ON(!(IS_VLV() || IS_CHV())
> > return;
> >
> > > lockdep_assert_held(>psr.lock);
> > >
> > > /*
> > > @@ -543,7 +545,7 @@ void intel_psr_enable(struct intel_dp *intel_dp,
> > > dev_priv->psr.enable_source(intel_dp, crtc_state);
> > > dev_priv->psr.enabled = intel_dp;
> > >
> > > - if (INTEL_GEN(dev_priv) >= 9) {
> > > + if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) {
> >
> > How about inverting this?
> >
> > if (IS_VLV() || IS_CHV())
> > intel_psr_schedule()
> > else
> > intel_psr_activate()
> >
> > is easier to follow IMO.
> >
> >
> > What is the reason to not use HAS_DDI() ?
>
> I believe HAS_DDI is not meaningful here. It is just a coincidence.
>
> maybe we could simplify everything with has_hw_tracking but also
> a coincidence in other cases...
>
> maybe create something meaninfull like VLV_PSR... :/
>
> no strong feelings actually...
>
Thanks for the clarification, IS_VLV() || IS_CHV() is good enough in
that case. Since you also have a comment
Re: [Intel-gfx] [PATCH 2/5] drm/i915/psr: Kill scheduled work for Core platforms.
On Fri, Feb 23, 2018 at 03:46:09PM -0800, Pandiyan, Dhinakaran wrote:
> On Tue, 2018-02-13 at 15:26 -0800, Rodrigo Vivi wrote:
> > It is a fact that scheduled work is now improved.
> >
> > But it is also a fact that on core platforms that shouldn't
> > be needed. We only need to actually wait on VLV/CHV.
> >
> > The immediate enabling is actually not an issue for the
> > HW perspective for core platforms that have HW tracking.
> > HW will wait few identical idle frames before transitioning
> > to actual psr active anyways.
> >
> > Note that this patch also remove the delayed activation
> > on HSW and BDW introduced by commit 'd0ac896a477d
> > ("drm/i915: Delay first PSR activation.")'. This was
> > introduced to fix a blank screen on VLV/CHV and also
> > masked some frozen screens on other core platforms.
> > Probably the same that we are now properly hunting and fixing.
> >
> > Furthermore, if we stop using delayed activation on core
> > platforms we will be able, on following up patches,
> > to use available workarounds to make HW tracking properly
> > exit PSR instead of the big nuke of disabling psr and
> > re-enable on exit and activate respectively.
> > At least on few reliable cases.
> >
> > Cc: Dhinakaran Pandiyan
> > Signed-off-by: Rodrigo Vivi
> > ---
> > drivers/gpu/drm/i915/i915_debugfs.c | 14 +++---
> > drivers/gpu/drm/i915/intel_psr.c| 27 +++
> > 2 files changed, 26 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index da80ee16a3cf..541290c307c7 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2522,18 +2522,18 @@ static int i915_edp_psr_status(struct seq_file *m,
> > void *data)
> > seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
> >dev_priv->psr.busy_frontbuffer_bits);
> >
> > - if (timer_pending(_priv->psr.activate_timer))
> > - seq_printf(m, "Activate scheduled: yes, in %dms\n",
> > -
> > jiffies_to_msecs(dev_priv->psr.activate_timer.expires - jiffies));
> > - else
> > - seq_printf(m, "Activate scheduled: no\n");
> > -
> > - if (HAS_DDI(dev_priv)) {
> > + if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) {
>
> I don't get this change, it is better to retain HAS_DDI().
>
>
> > if (dev_priv->psr.psr2_support)
> > enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
> > else
> > enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
> > } else {
> > + if (timer_pending(_priv->psr.activate_timer))
> > + seq_printf(m, "Activate scheduled: yes, in %dms\n",
> > +
> > jiffies_to_msecs(dev_priv->psr.activate_timer.expires - jiffies));
> > + else
> > + seq_printf(m, "Activate scheduled: no\n");
> > +
> > for_each_pipe(dev_priv, pipe) {
> > enum transcoder cpu_transcoder =
> > intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 826b480841ac..13409c6301e8 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -455,6 +455,8 @@ static void intel_psr_schedule(struct drm_i915_private
> > *i915,
> > {
> > unsigned long next;
> >
> > + WARN_ON(!IS_VALLEYVIEW(i915) && !IS_CHERRYVIEW(i915));
> > +
> How about using only !(IS_VLV() || IS_CHV) in this file.
>
> I think this is a reasonable check to have, please add a return too.
> WARN_ON(!(IS_VLV() || IS_CHV())
> return;
>
> > lockdep_assert_held(>psr.lock);
> >
> > /*
> > @@ -543,7 +545,7 @@ void intel_psr_enable(struct intel_dp *intel_dp,
> > dev_priv->psr.enable_source(intel_dp, crtc_state);
> > dev_priv->psr.enabled = intel_dp;
> >
> > - if (INTEL_GEN(dev_priv) >= 9) {
> > + if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) {
>
> How about inverting this?
>
> if (IS_VLV() || IS_CHV())
> intel_psr_schedule()
> else
> intel_psr_activate()
>
> is easier to follow IMO.
>
>
> What is the reason to not use HAS_DDI() ?
I believe HAS_DDI is not meaningful here. It is just a coincidence.
maybe we could simplify everything with has_hw_tracking but also
a coincidence in other cases...
maybe create something meaninfull like VLV_PSR... :/
no strong feelings actually...
>
> > intel_psr_activate(intel_dp);
> > } else {
> > /*
> > @@ -553,8 +555,6 @@ void intel_psr_enable(struct intel_dp *intel_dp,
> > * However on some platforms we face issues when first
> > * activation follows a modeset so quickly.
> > * - On VLV/CHV we get bank screen on
Re: [Intel-gfx] [PATCH 2/5] drm/i915/psr: Kill scheduled work for Core platforms.
On Tue, 2018-02-13 at 15:26 -0800, Rodrigo Vivi wrote:
> It is a fact that scheduled work is now improved.
>
> But it is also a fact that on core platforms that shouldn't
> be needed. We only need to actually wait on VLV/CHV.
>
> The immediate enabling is actually not an issue for the
> HW perspective for core platforms that have HW tracking.
> HW will wait few identical idle frames before transitioning
> to actual psr active anyways.
>
> Note that this patch also remove the delayed activation
> on HSW and BDW introduced by commit 'd0ac896a477d
> ("drm/i915: Delay first PSR activation.")'. This was
> introduced to fix a blank screen on VLV/CHV and also
> masked some frozen screens on other core platforms.
> Probably the same that we are now properly hunting and fixing.
>
> Furthermore, if we stop using delayed activation on core
> platforms we will be able, on following up patches,
> to use available workarounds to make HW tracking properly
> exit PSR instead of the big nuke of disabling psr and
> re-enable on exit and activate respectively.
> At least on few reliable cases.
>
> Cc: Dhinakaran Pandiyan
> Signed-off-by: Rodrigo Vivi
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 14 +++---
> drivers/gpu/drm/i915/intel_psr.c| 27 +++
> 2 files changed, 26 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index da80ee16a3cf..541290c307c7 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2522,18 +2522,18 @@ static int i915_edp_psr_status(struct seq_file *m,
> void *data)
> seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
> dev_priv->psr.busy_frontbuffer_bits);
>
> - if (timer_pending(_priv->psr.activate_timer))
> - seq_printf(m, "Activate scheduled: yes, in %dms\n",
> -
> jiffies_to_msecs(dev_priv->psr.activate_timer.expires - jiffies));
> - else
> - seq_printf(m, "Activate scheduled: no\n");
> -
> - if (HAS_DDI(dev_priv)) {
> + if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) {
I don't get this change, it is better to retain HAS_DDI().
> if (dev_priv->psr.psr2_support)
> enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
> else
> enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
> } else {
> + if (timer_pending(_priv->psr.activate_timer))
> + seq_printf(m, "Activate scheduled: yes, in %dms\n",
> +
> jiffies_to_msecs(dev_priv->psr.activate_timer.expires - jiffies));
> + else
> + seq_printf(m, "Activate scheduled: no\n");
> +
> for_each_pipe(dev_priv, pipe) {
> enum transcoder cpu_transcoder =
> intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 826b480841ac..13409c6301e8 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -455,6 +455,8 @@ static void intel_psr_schedule(struct drm_i915_private
> *i915,
> {
> unsigned long next;
>
> + WARN_ON(!IS_VALLEYVIEW(i915) && !IS_CHERRYVIEW(i915));
> +
How about using only !(IS_VLV() || IS_CHV) in this file.
I think this is a reasonable check to have, please add a return too.
WARN_ON(!(IS_VLV() || IS_CHV())
return;
> lockdep_assert_held(>psr.lock);
>
> /*
> @@ -543,7 +545,7 @@ void intel_psr_enable(struct intel_dp *intel_dp,
> dev_priv->psr.enable_source(intel_dp, crtc_state);
> dev_priv->psr.enabled = intel_dp;
>
> - if (INTEL_GEN(dev_priv) >= 9) {
> + if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) {
How about inverting this?
if (IS_VLV() || IS_CHV())
intel_psr_schedule()
else
intel_psr_activate()
is easier to follow IMO.
What is the reason to not use HAS_DDI() ?
> intel_psr_activate(intel_dp);
> } else {
> /*
> @@ -553,8 +555,6 @@ void intel_psr_enable(struct intel_dp *intel_dp,
>* However on some platforms we face issues when first
>* activation follows a modeset so quickly.
>* - On VLV/CHV we get bank screen on first activation
> - * - On HSW/BDW we get a recoverable frozen screen until
> - * next exit-activate sequence.
>*/
> intel_psr_schedule(dev_priv,
> intel_dp->panel_power_cycle_delay * 5);
> @@ -687,6 +687,8 @@ static void intel_psr_work(struct work_struct *work)
> struct drm_crtc *crtc = dp_to_dig_port(intel_dp)->base.base.crtc;
> enum pipe pipe =
