| Message ID | 1372441930-18280-1-git-send-email-rodrigo.vivi@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jun 28, 2013 at 02:52:10PM -0300, Rodrigo Vivi wrote: > PSR tracking engine in HSW doesn't detect automagically some directly copy area > operations through scanout so we will have to kick it manually and > reschedule it to come back to normal operation as soon as possible. Does force_exit correspond with any names/actions in the documentation? If not, care to consider intel_edp_psr_wakeup() as a less confusing name? -Chris
On Fri, Jun 28, 2013 at 2:57 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Fri, Jun 28, 2013 at 02:52:10PM -0300, Rodrigo Vivi wrote: >> PSR tracking engine in HSW doesn't detect automagically some directly copy area >> operations through scanout so we will have to kick it manually and >> reschedule it to come back to normal operation as soon as possible. > > Does force_exit correspond with any names/actions in the documentation? > If not, care to consider intel_edp_psr_wakeup() as a less confusing name? Yes, it correspond to name of setting 11 to bits 31:30 of 0x64860. Besides to be honest wakeup confuses me more. It seems ambiguous, I mean wake up from srd state (force exit) or wake up psr functionality (normal)? > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre -- Rodrigo Vivi Blog: http://blog.vivi.eng.br
2013/6/28 Rodrigo Vivi <rodrigo.vivi@gmail.com>: > PSR tracking engine in HSW doesn't detect automagically some directly copy area > operations through scanout so we will have to kick it manually and > reschedule it to come back to normal operation as soon as possible. > > v2: Before PSR Hook. Don't force it when busy yet. > v3/v4: Solved small conflict. > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 1 + > drivers/gpu/drm/i915/intel_dp.c | 59 ++++++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 4 +++ > 3 files changed, 64 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 833cc97..a19245b 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -1838,6 +1838,7 @@ > #define EDP_PSR_PERF_CNT_MASK 0xffffff > > #define EDP_PSR_DEBUG_CTL 0x64860 > +#define EDP_PSR_DEBUG_FORCE_EXIT (3<<30) Can you please explain why we're using the debug register here? Usually the debug register is only used for debugging, not as part of the normal operation. Why can't we do normal/full PSR exit/enter here using SRD_CTL bit 31 and the enable/disable sequences? What is different? > #define EDP_PSR_DEBUG_MASK_LPSP (1<<27) > #define EDP_PSR_DEBUG_MASK_MEMUP (1<<26) > #define EDP_PSR_DEBUG_MASK_HPD (1<<25) > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 68c81ab..5001fd9 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1366,6 +1366,48 @@ bool intel_edp_is_psr_enabled(struct drm_device *dev) > return I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE; > } > > +static void intel_edp_psr_delayed_normal_work(struct work_struct *__work) > +{ > + struct intel_dp *intel_dp = container_of(to_delayed_work(__work), > + struct intel_dp, > + edp_psr_delayed_normal_work); > + struct drm_device *dev = intel_dp_to_dev(intel_dp); > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + mutex_lock(&intel_dp->psr_exit_mutex); > + I915_WRITE(EDP_PSR_DEBUG_CTL, I915_READ(EDP_PSR_DEBUG_CTL) & > + ~EDP_PSR_DEBUG_FORCE_EXIT); > + mutex_unlock(&intel_dp->psr_exit_mutex); > +} > + > +void intel_edp_psr_force_exit(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_encoder *encoder; > + struct intel_dp *intel_dp = NULL; > + > + if (!intel_edp_is_psr_enabled(dev)) > + return; > + > + list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) > + if (encoder->type == INTEL_OUTPUT_EDP) > + intel_dp = enc_to_intel_dp(&encoder->base); > + > + if (!intel_dp) > + return; > + > + if (WARN_ON(!intel_dp->psr_setup_done)) > + return; > + > + mutex_lock(&intel_dp->psr_exit_mutex); > + I915_WRITE(EDP_PSR_DEBUG_CTL, I915_READ(EDP_PSR_DEBUG_CTL) | > + EDP_PSR_DEBUG_FORCE_EXIT); > + mutex_unlock(&intel_dp->psr_exit_mutex); > + > + schedule_delayed_work(&intel_dp->edp_psr_delayed_normal_work, > + msecs_to_jiffies(100)); Why 100ms? > +} > + > void intel_edp_psr_write_vsc(struct intel_dp *intel_dp, > struct edp_vsc_psr *vsc_psr) > { > @@ -1400,6 +1442,18 @@ void intel_edp_psr_write_vsc(struct intel_dp *intel_dp, > POSTING_READ(ctl_reg); > } > > +static void intel_edp_psr_setup(struct intel_dp *intel_dp) > +{ > + if (intel_dp->psr_setup_done) > + return; > + > + INIT_DELAYED_WORK(&intel_dp->edp_psr_delayed_normal_work, > + intel_edp_psr_delayed_normal_work); > + mutex_init(&intel_dp->psr_exit_mutex); > + > + intel_dp->psr_setup_done = true; > +} > + > static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp) > { > struct drm_device *dev = intel_dp_to_dev(intel_dp); > @@ -1544,6 +1598,9 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp) > > void intel_edp_psr_do_enable(struct intel_dp *intel_dp) > { > + /* Setup PSR once */ > + intel_edp_psr_setup(intel_dp); > + > /* Enable PSR on the panel */ > intel_edp_psr_enable_sink(intel_dp); > > @@ -3413,6 +3470,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > WARN(error, "intel_dp_i2c_init failed with error %d for port %c\n", > error, port_name(port)); > > + intel_dp->psr_setup_done = false; > + > if (!intel_edp_init_connector(intel_dp, intel_connector)) { > i2c_del_adapter(&intel_dp->adapter); > if (is_edp(intel_dp)) { > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 65f5001..3b2ccaa 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -497,6 +497,9 @@ struct intel_dp { > int backlight_on_delay; > int backlight_off_delay; > struct delayed_work panel_vdd_work; > + struct delayed_work edp_psr_delayed_normal_work; > + struct mutex psr_exit_mutex; > + bool psr_setup_done; > bool want_panel_vdd; > struct intel_connector *attached_connector; > }; > @@ -844,5 +847,6 @@ extern void intel_edp_psr_enable(struct intel_dp *intel_dp); > extern void intel_edp_psr_disable(struct intel_dp *intel_dp); > extern void intel_edp_psr_update(struct drm_device *dev); > extern bool intel_edp_is_psr_enabled(struct drm_device *dev); > +extern void intel_edp_psr_force_exit(struct drm_device *dev); > > #endif /* __INTEL_DRV_H__ */ > -- > 1.8.1.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Jul 8, 2013 at 10:03 AM, Paulo Zanoni <przanoni@gmail.com> wrote: > 2013/6/28 Rodrigo Vivi <rodrigo.vivi@gmail.com>: >> PSR tracking engine in HSW doesn't detect automagically some directly copy area >> operations through scanout so we will have to kick it manually and >> reschedule it to come back to normal operation as soon as possible. >> >> v2: Before PSR Hook. Don't force it when busy yet. >> v3/v4: Solved small conflict. >> >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> >> --- >> drivers/gpu/drm/i915/i915_reg.h | 1 + >> drivers/gpu/drm/i915/intel_dp.c | 59 ++++++++++++++++++++++++++++++++++++++++ >> drivers/gpu/drm/i915/intel_drv.h | 4 +++ >> 3 files changed, 64 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index 833cc97..a19245b 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -1838,6 +1838,7 @@ >> #define EDP_PSR_PERF_CNT_MASK 0xffffff >> >> #define EDP_PSR_DEBUG_CTL 0x64860 >> +#define EDP_PSR_DEBUG_FORCE_EXIT (3<<30) > > Can you please explain why we're using the debug register here? > Usually the debug register is only used for debugging, not as part of > the normal operation. Why can't we do normal/full PSR exit/enter here > using SRD_CTL bit 31 and the enable/disable sequences? What is > different? I understand that, but I didn't see a better way to nuke PSR forcing it to exit without using the DEBUG register. Please let me know if there is a better way of doing that. PSR functionality at haswell wasn't prepared to small directly rights to scanout. it tracks writes to some registers, mouse position updates, but doesn't track small writes to scanout. KDE and XDM do many small writes to scanout directly that doesn't trigger psr exit on haswell... in a long discussion with Chris we figure out that best place to force an psr exit was at busy_ioctl... and I really didn't want to disable it and enable on next plage flip or what ever because this can wait forever and this is the most efficient way of doing that in terms of power savings... Besides, debug register was already been used on enable anyway... EWe must to use it to disable memup tracking anyway, even if we fix hotplug, we will never stop using this debug register... ever... > > >> #define EDP_PSR_DEBUG_MASK_LPSP (1<<27) >> #define EDP_PSR_DEBUG_MASK_MEMUP (1<<26) >> #define EDP_PSR_DEBUG_MASK_HPD (1<<25) >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index 68c81ab..5001fd9 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -1366,6 +1366,48 @@ bool intel_edp_is_psr_enabled(struct drm_device *dev) >> return I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE; >> } >> >> +static void intel_edp_psr_delayed_normal_work(struct work_struct *__work) >> +{ >> + struct intel_dp *intel_dp = container_of(to_delayed_work(__work), >> + struct intel_dp, >> + edp_psr_delayed_normal_work); >> + struct drm_device *dev = intel_dp_to_dev(intel_dp); >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + >> + mutex_lock(&intel_dp->psr_exit_mutex); >> + I915_WRITE(EDP_PSR_DEBUG_CTL, I915_READ(EDP_PSR_DEBUG_CTL) & >> + ~EDP_PSR_DEBUG_FORCE_EXIT); >> + mutex_unlock(&intel_dp->psr_exit_mutex); >> +} >> + >> +void intel_edp_psr_force_exit(struct drm_device *dev) >> +{ >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + struct intel_encoder *encoder; >> + struct intel_dp *intel_dp = NULL; >> + >> + if (!intel_edp_is_psr_enabled(dev)) >> + return; >> + >> + list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) >> + if (encoder->type == INTEL_OUTPUT_EDP) >> + intel_dp = enc_to_intel_dp(&encoder->base); >> + >> + if (!intel_dp) >> + return; >> + >> + if (WARN_ON(!intel_dp->psr_setup_done)) >> + return; >> + >> + mutex_lock(&intel_dp->psr_exit_mutex); >> + I915_WRITE(EDP_PSR_DEBUG_CTL, I915_READ(EDP_PSR_DEBUG_CTL) | >> + EDP_PSR_DEBUG_FORCE_EXIT); >> + mutex_unlock(&intel_dp->psr_exit_mutex); >> + >> + schedule_delayed_work(&intel_dp->edp_psr_delayed_normal_work, >> + msecs_to_jiffies(100)); > > Why 100ms? because it works... > > >> +} >> + >> void intel_edp_psr_write_vsc(struct intel_dp *intel_dp, >> struct edp_vsc_psr *vsc_psr) >> { >> @@ -1400,6 +1442,18 @@ void intel_edp_psr_write_vsc(struct intel_dp *intel_dp, >> POSTING_READ(ctl_reg); >> } >> >> +static void intel_edp_psr_setup(struct intel_dp *intel_dp) >> +{ >> + if (intel_dp->psr_setup_done) >> + return; >> + >> + INIT_DELAYED_WORK(&intel_dp->edp_psr_delayed_normal_work, >> + intel_edp_psr_delayed_normal_work); >> + mutex_init(&intel_dp->psr_exit_mutex); >> + >> + intel_dp->psr_setup_done = true; >> +} >> + >> static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp) >> { >> struct drm_device *dev = intel_dp_to_dev(intel_dp); >> @@ -1544,6 +1598,9 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp) >> >> void intel_edp_psr_do_enable(struct intel_dp *intel_dp) >> { >> + /* Setup PSR once */ >> + intel_edp_psr_setup(intel_dp); >> + >> /* Enable PSR on the panel */ >> intel_edp_psr_enable_sink(intel_dp); >> >> @@ -3413,6 +3470,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, >> WARN(error, "intel_dp_i2c_init failed with error %d for port %c\n", >> error, port_name(port)); >> >> + intel_dp->psr_setup_done = false; >> + >> if (!intel_edp_init_connector(intel_dp, intel_connector)) { >> i2c_del_adapter(&intel_dp->adapter); >> if (is_edp(intel_dp)) { >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index 65f5001..3b2ccaa 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -497,6 +497,9 @@ struct intel_dp { >> int backlight_on_delay; >> int backlight_off_delay; >> struct delayed_work panel_vdd_work; >> + struct delayed_work edp_psr_delayed_normal_work; >> + struct mutex psr_exit_mutex; >> + bool psr_setup_done; >> bool want_panel_vdd; >> struct intel_connector *attached_connector; >> }; >> @@ -844,5 +847,6 @@ extern void intel_edp_psr_enable(struct intel_dp *intel_dp); >> extern void intel_edp_psr_disable(struct intel_dp *intel_dp); >> extern void intel_edp_psr_update(struct drm_device *dev); >> extern bool intel_edp_is_psr_enabled(struct drm_device *dev); >> +extern void intel_edp_psr_force_exit(struct drm_device *dev); >> >> #endif /* __INTEL_DRV_H__ */ >> -- >> 1.8.1.4 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Paulo Zanoni -- Rodrigo Vivi Blog: http://blog.vivi.eng.br
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 833cc97..a19245b 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -1838,6 +1838,7 @@ #define EDP_PSR_PERF_CNT_MASK 0xffffff #define EDP_PSR_DEBUG_CTL 0x64860 +#define EDP_PSR_DEBUG_FORCE_EXIT (3<<30) #define EDP_PSR_DEBUG_MASK_LPSP (1<<27) #define EDP_PSR_DEBUG_MASK_MEMUP (1<<26) #define EDP_PSR_DEBUG_MASK_HPD (1<<25) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 68c81ab..5001fd9 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1366,6 +1366,48 @@ bool intel_edp_is_psr_enabled(struct drm_device *dev) return I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE; } +static void intel_edp_psr_delayed_normal_work(struct work_struct *__work) +{ + struct intel_dp *intel_dp = container_of(to_delayed_work(__work), + struct intel_dp, + edp_psr_delayed_normal_work); + struct drm_device *dev = intel_dp_to_dev(intel_dp); + struct drm_i915_private *dev_priv = dev->dev_private; + + mutex_lock(&intel_dp->psr_exit_mutex); + I915_WRITE(EDP_PSR_DEBUG_CTL, I915_READ(EDP_PSR_DEBUG_CTL) & + ~EDP_PSR_DEBUG_FORCE_EXIT); + mutex_unlock(&intel_dp->psr_exit_mutex); +} + +void intel_edp_psr_force_exit(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_encoder *encoder; + struct intel_dp *intel_dp = NULL; + + if (!intel_edp_is_psr_enabled(dev)) + return; + + list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) + if (encoder->type == INTEL_OUTPUT_EDP) + intel_dp = enc_to_intel_dp(&encoder->base); + + if (!intel_dp) + return; + + if (WARN_ON(!intel_dp->psr_setup_done)) + return; + + mutex_lock(&intel_dp->psr_exit_mutex); + I915_WRITE(EDP_PSR_DEBUG_CTL, I915_READ(EDP_PSR_DEBUG_CTL) | + EDP_PSR_DEBUG_FORCE_EXIT); + mutex_unlock(&intel_dp->psr_exit_mutex); + + schedule_delayed_work(&intel_dp->edp_psr_delayed_normal_work, + msecs_to_jiffies(100)); +} + void intel_edp_psr_write_vsc(struct intel_dp *intel_dp, struct edp_vsc_psr *vsc_psr) { @@ -1400,6 +1442,18 @@ void intel_edp_psr_write_vsc(struct intel_dp *intel_dp, POSTING_READ(ctl_reg); } +static void intel_edp_psr_setup(struct intel_dp *intel_dp) +{ + if (intel_dp->psr_setup_done) + return; + + INIT_DELAYED_WORK(&intel_dp->edp_psr_delayed_normal_work, + intel_edp_psr_delayed_normal_work); + mutex_init(&intel_dp->psr_exit_mutex); + + intel_dp->psr_setup_done = true; +} + static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp) { struct drm_device *dev = intel_dp_to_dev(intel_dp); @@ -1544,6 +1598,9 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp) void intel_edp_psr_do_enable(struct intel_dp *intel_dp) { + /* Setup PSR once */ + intel_edp_psr_setup(intel_dp); + /* Enable PSR on the panel */ intel_edp_psr_enable_sink(intel_dp); @@ -3413,6 +3470,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, WARN(error, "intel_dp_i2c_init failed with error %d for port %c\n", error, port_name(port)); + intel_dp->psr_setup_done = false; + if (!intel_edp_init_connector(intel_dp, intel_connector)) { i2c_del_adapter(&intel_dp->adapter); if (is_edp(intel_dp)) { diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 65f5001..3b2ccaa 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -497,6 +497,9 @@ struct intel_dp { int backlight_on_delay; int backlight_off_delay; struct delayed_work panel_vdd_work; + struct delayed_work edp_psr_delayed_normal_work; + struct mutex psr_exit_mutex; + bool psr_setup_done; bool want_panel_vdd; struct intel_connector *attached_connector; }; @@ -844,5 +847,6 @@ extern void intel_edp_psr_enable(struct intel_dp *intel_dp); extern void intel_edp_psr_disable(struct intel_dp *intel_dp); extern void intel_edp_psr_update(struct drm_device *dev); extern bool intel_edp_is_psr_enabled(struct drm_device *dev); +extern void intel_edp_psr_force_exit(struct drm_device *dev); #endif /* __INTEL_DRV_H__ */
PSR tracking engine in HSW doesn't detect automagically some directly copy area operations through scanout so we will have to kick it manually and reschedule it to come back to normal operation as soon as possible. v2: Before PSR Hook. Don't force it when busy yet. v3/v4: Solved small conflict. Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> --- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_dp.c | 59 ++++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 4 +++ 3 files changed, 64 insertions(+)