Re: [Intel-gfx] [RFC v5] drm/i915/psr: Kill scheduled work for Core platforms.
On Wed, 2018-03-14 at 13:47 -0700, Rodrigo Vivi wrote:
> On Wed, Mar 14, 2018 at 01:24:13PM -0700, Pandiyan, Dhinakaran wrote:
> > On Tue, 2018-03-13 at 15:23 -0700, Rodrigo Vivi wrote:
> > > 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.
> > >
> > > v2:(DK): Remove unnecessary WARN_ONs and make some other
> > >VLV | CHV more readable.
> > > v3: Do it regardless the timer rework.
> > > v4: (DK/CI): Add VLV || CHV check on cancel work at psr_disable.
> > > v5: Kill remaining items and fully rework activation functions.
> > >
> > > Cc: Dhinakaran Pandiyan
> > > Signed-off-by: Rodrigo Vivi
> > > ---
> > > drivers/gpu/drm/i915/i915_debugfs.c | 20 +++---
> > > drivers/gpu/drm/i915/i915_drv.h | 3 +-
> > > drivers/gpu/drm/i915/intel_psr.c| 134
> > > ++--
> > > 3 files changed, 65 insertions(+), 92 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index 972014b2497d..7dfada863f9c 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -2567,15 +2567,14 @@ static int i915_edp_psr_status(struct seq_file
> > > *m, void *data)
> > > seq_printf(m, "Enabled: %s\n", yesno((bool)dev_priv->psr.enabled));
> > > seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
> > > dev_priv->psr.busy_frontbuffer_bits);
> > > - seq_printf(m, "Re-enable work scheduled: %s\n",
> > > -yesno(work_busy(_priv->psr.work.work)));
> > >
> > > - if (HAS_DDI(dev_priv)) {
> > > - 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 (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > > + bool scheduled;
> > > +
> > > + scheduled = work_busy(_priv->psr.vlv_activate_work.work);
> > > + seq_printf(m, "Re-enable work scheduled: %s\n",
> > > +yesno(scheduled));
> > > +
> > > for_each_pipe(dev_priv, pipe) {
> > > enum transcoder cpu_transcoder =
> > > intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> > > @@ -2594,6 +2593,11 @@ static int i915_edp_psr_status(struct seq_file *m,
> > > void *data)
> > >
> > > intel_display_power_put(dev_priv, power_domain);
> > > }
> > > + } else {
> > > + 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;
> > > }
> > >
> > > seq_printf(m, "Main link in standby mode: %s\n",
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 74b0e9d8ff62..409997f29e07 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -598,7 +598,7 @@ struct i915_psr {
> > > bool sink_support;
> > > struct intel_dp *enabled;
> > > bool active;
> > > - struct delayed_work work;
> > > + struct delayed_work vlv_activate_work;
> > > unsigned busy_frontbuffer_bits;
> > > bool psr2_support;
> > > bool aux_frame_sync;
> > > @@ -613,7 +613,6 @@ struct i915_psr {
> > > void (*disable_source)(struct intel_dp *,
> > > const struct intel_crtc_state *);
> > > void (*enable_sink)(struct intel_dp *);
> > > - void (*activate)(struct intel_dp *);
> > > void (*setup_vsc)(struct intel_dp *, const struct intel_crtc_state *);
> > > };
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 317cb4a12693..bc7b94a06417 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -311,24 +311,6 @@ static void vlv_psr_enable_source(struct intel_dp
> > > *intel_dp,
> > > VLV_EDP_PSR_ENABLE);
> > > }
> > >
Re: [Intel-gfx] [RFC v5] drm/i915/psr: Kill scheduled work for Core platforms.
On Wed, Mar 14, 2018 at 01:24:13PM -0700, Pandiyan, Dhinakaran wrote:
> On Tue, 2018-03-13 at 15:23 -0700, Rodrigo Vivi wrote:
> > 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.
> >
> > v2:(DK): Remove unnecessary WARN_ONs and make some other
> > VLV | CHV more readable.
> > v3: Do it regardless the timer rework.
> > v4: (DK/CI): Add VLV || CHV check on cancel work at psr_disable.
> > v5: Kill remaining items and fully rework activation functions.
> >
> > Cc: Dhinakaran Pandiyan
> > Signed-off-by: Rodrigo Vivi
> > ---
> > drivers/gpu/drm/i915/i915_debugfs.c | 20 +++---
> > drivers/gpu/drm/i915/i915_drv.h | 3 +-
> > drivers/gpu/drm/i915/intel_psr.c| 134
> > ++--
> > 3 files changed, 65 insertions(+), 92 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 972014b2497d..7dfada863f9c 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2567,15 +2567,14 @@ static int i915_edp_psr_status(struct seq_file *m,
> > void *data)
> > seq_printf(m, "Enabled: %s\n", yesno((bool)dev_priv->psr.enabled));
> > seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
> >dev_priv->psr.busy_frontbuffer_bits);
> > - seq_printf(m, "Re-enable work scheduled: %s\n",
> > - yesno(work_busy(_priv->psr.work.work)));
> >
> > - if (HAS_DDI(dev_priv)) {
> > - 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 (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > + bool scheduled;
> > +
> > + scheduled = work_busy(_priv->psr.vlv_activate_work.work);
> > + seq_printf(m, "Re-enable work scheduled: %s\n",
> > + yesno(scheduled));
> > +
> > for_each_pipe(dev_priv, pipe) {
> > enum transcoder cpu_transcoder =
> > intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> > @@ -2594,6 +2593,11 @@ static int i915_edp_psr_status(struct seq_file *m,
> > void *data)
> >
> > intel_display_power_put(dev_priv, power_domain);
> > }
> > + } else {
> > + 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;
> > }
> >
> > seq_printf(m, "Main link in standby mode: %s\n",
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 74b0e9d8ff62..409997f29e07 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -598,7 +598,7 @@ struct i915_psr {
> > bool sink_support;
> > struct intel_dp *enabled;
> > bool active;
> > - struct delayed_work work;
> > + struct delayed_work vlv_activate_work;
> > unsigned busy_frontbuffer_bits;
> > bool psr2_support;
> > bool aux_frame_sync;
> > @@ -613,7 +613,6 @@ struct i915_psr {
> > void (*disable_source)(struct intel_dp *,
> >const struct intel_crtc_state *);
> > void (*enable_sink)(struct intel_dp *);
> > - void (*activate)(struct intel_dp *);
> > void (*setup_vsc)(struct intel_dp *, const struct intel_crtc_state *);
> > };
> >
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 317cb4a12693..bc7b94a06417 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -311,24 +311,6 @@ static void vlv_psr_enable_source(struct intel_dp
> > *intel_dp,
> >VLV_EDP_PSR_ENABLE);
> > }
> >
> > -static void vlv_psr_activate(struct intel_dp *intel_dp)
> > -{
> > - struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > - struct drm_device *dev =
Re: [Intel-gfx] [RFC v5] drm/i915/psr: Kill scheduled work for Core platforms.
On Tue, 2018-03-13 at 15:23 -0700, Rodrigo Vivi wrote:
> 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.
>
> v2:(DK): Remove unnecessary WARN_ONs and make some other
>VLV | CHV more readable.
> v3: Do it regardless the timer rework.
> v4: (DK/CI): Add VLV || CHV check on cancel work at psr_disable.
> v5: Kill remaining items and fully rework activation functions.
>
> Cc: Dhinakaran Pandiyan
> Signed-off-by: Rodrigo Vivi
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 20 +++---
> drivers/gpu/drm/i915/i915_drv.h | 3 +-
> drivers/gpu/drm/i915/intel_psr.c| 134
> ++--
> 3 files changed, 65 insertions(+), 92 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 972014b2497d..7dfada863f9c 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2567,15 +2567,14 @@ static int i915_edp_psr_status(struct seq_file *m,
> void *data)
> seq_printf(m, "Enabled: %s\n", yesno((bool)dev_priv->psr.enabled));
> seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
> dev_priv->psr.busy_frontbuffer_bits);
> - seq_printf(m, "Re-enable work scheduled: %s\n",
> -yesno(work_busy(_priv->psr.work.work)));
>
> - if (HAS_DDI(dev_priv)) {
> - 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 (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> + bool scheduled;
> +
> + scheduled = work_busy(_priv->psr.vlv_activate_work.work);
> + seq_printf(m, "Re-enable work scheduled: %s\n",
> +yesno(scheduled));
> +
> for_each_pipe(dev_priv, pipe) {
> enum transcoder cpu_transcoder =
> intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> @@ -2594,6 +2593,11 @@ static int i915_edp_psr_status(struct seq_file *m,
> void *data)
>
> intel_display_power_put(dev_priv, power_domain);
> }
> + } else {
> + 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;
> }
>
> seq_printf(m, "Main link in standby mode: %s\n",
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 74b0e9d8ff62..409997f29e07 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -598,7 +598,7 @@ struct i915_psr {
> bool sink_support;
> struct intel_dp *enabled;
> bool active;
> - struct delayed_work work;
> + struct delayed_work vlv_activate_work;
> unsigned busy_frontbuffer_bits;
> bool psr2_support;
> bool aux_frame_sync;
> @@ -613,7 +613,6 @@ struct i915_psr {
> void (*disable_source)(struct intel_dp *,
> const struct intel_crtc_state *);
> void (*enable_sink)(struct intel_dp *);
> - void (*activate)(struct intel_dp *);
> void (*setup_vsc)(struct intel_dp *, const struct intel_crtc_state *);
> };
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 317cb4a12693..bc7b94a06417 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -311,24 +311,6 @@ static void vlv_psr_enable_source(struct intel_dp
> *intel_dp,
> VLV_EDP_PSR_ENABLE);
> }
>
> -static void vlv_psr_activate(struct intel_dp *intel_dp)
> -{
> - struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> - struct drm_device *dev = dig_port->base.base.dev;
> - struct drm_i915_private *dev_priv = to_i915(dev);
> - struct drm_crtc *crtc = dig_port->base.base.crtc;
> - enum pipe pipe = to_intel_crtc(crtc)->pipe;
> -
> - /*
> - * Let's
[Intel-gfx] [RFC v5] drm/i915/psr: Kill scheduled work for Core platforms.
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.
v2:(DK): Remove unnecessary WARN_ONs and make some other
VLV | CHV more readable.
v3: Do it regardless the timer rework.
v4: (DK/CI): Add VLV || CHV check on cancel work at psr_disable.
v5: Kill remaining items and fully rework activation functions.
Cc: Dhinakaran Pandiyan
Signed-off-by: Rodrigo Vivi
---
drivers/gpu/drm/i915/i915_debugfs.c | 20 +++---
drivers/gpu/drm/i915/i915_drv.h | 3 +-
drivers/gpu/drm/i915/intel_psr.c| 134 ++--
3 files changed, 65 insertions(+), 92 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index 972014b2497d..7dfada863f9c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2567,15 +2567,14 @@ static int i915_edp_psr_status(struct seq_file *m, void
*data)
seq_printf(m, "Enabled: %s\n", yesno((bool)dev_priv->psr.enabled));
seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
dev_priv->psr.busy_frontbuffer_bits);
- seq_printf(m, "Re-enable work scheduled: %s\n",
- yesno(work_busy(_priv->psr.work.work)));
- if (HAS_DDI(dev_priv)) {
- 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 (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
+ bool scheduled;
+
+ scheduled = work_busy(_priv->psr.vlv_activate_work.work);
+ seq_printf(m, "Re-enable work scheduled: %s\n",
+ yesno(scheduled));
+
for_each_pipe(dev_priv, pipe) {
enum transcoder cpu_transcoder =
intel_pipe_to_cpu_transcoder(dev_priv, pipe);
@@ -2594,6 +2593,11 @@ static int i915_edp_psr_status(struct seq_file *m, void
*data)
intel_display_power_put(dev_priv, power_domain);
}
+ } else {
+ 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;
}
seq_printf(m, "Main link in standby mode: %s\n",
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 74b0e9d8ff62..409997f29e07 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -598,7 +598,7 @@ struct i915_psr {
bool sink_support;
struct intel_dp *enabled;
bool active;
- struct delayed_work work;
+ struct delayed_work vlv_activate_work;
unsigned busy_frontbuffer_bits;
bool psr2_support;
bool aux_frame_sync;
@@ -613,7 +613,6 @@ struct i915_psr {
void (*disable_source)(struct intel_dp *,
const struct intel_crtc_state *);
void (*enable_sink)(struct intel_dp *);
- void (*activate)(struct intel_dp *);
void (*setup_vsc)(struct intel_dp *, const struct intel_crtc_state *);
};
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 317cb4a12693..bc7b94a06417 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -311,24 +311,6 @@ static void vlv_psr_enable_source(struct intel_dp
*intel_dp,
VLV_EDP_PSR_ENABLE);
}
-static void vlv_psr_activate(struct intel_dp *intel_dp)
-{
- struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
- struct drm_device *dev = dig_port->base.base.dev;
- struct drm_i915_private *dev_priv = to_i915(dev);
- struct drm_crtc *crtc = dig_port->base.base.crtc;
- enum pipe pipe = to_intel_crtc(crtc)->pipe;
-
- /*
-* Let's do the transition from PSR_state 1 (inactive) to
-* PSR_state 2 (transition to active - static frame transmission).
-* Then Hardware is responsible for the transition to
-
