* [PATCH 0/4] PSR general improvements and stabilization. @ 2015-11-11 21:19 Rodrigo Vivi 2015-11-11 21:19 ` [PATCH 1/4] drm/i915: Force PSR exit when IRQ_HPD is detected on eDP Rodrigo Vivi ` (4 more replies) 0 siblings, 5 replies; 32+ messages in thread From: Rodrigo Vivi @ 2015-11-11 21:19 UTC (permalink / raw) To: intel-gfx; +Cc: Rodrigo Vivi Proceeding with the big series split here goes the general PSR improvements and stabilization work. There is no critical fix on this series although I believe it is good to have all of them before we can enable PSR back by default. Rodrigo Vivi (4): drm/i915: Force PSR exit when IRQ_HPD is detected on eDP. drm/i915: Remove duplicated dpcd write on hsw_psr_enable_sink. drm/i915: PSR: Let's rely more on frontbuffer tracking. drm/i915: PSR: Mask LPSP hw tracking back again. drivers/gpu/drm/i915/intel_dp.c | 5 ++- drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_psr.c | 67 ++++++++++++++++++++++++++-------------- 3 files changed, 48 insertions(+), 25 deletions(-) -- 2.4.3 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 1/4] drm/i915: Force PSR exit when IRQ_HPD is detected on eDP. 2015-11-11 21:19 [PATCH 0/4] PSR general improvements and stabilization Rodrigo Vivi @ 2015-11-11 21:19 ` Rodrigo Vivi 2015-11-16 16:55 ` Ville Syrjälä 2015-11-11 21:19 ` [PATCH 2/4] drm/i915: Remove duplicated dpcd write on hsw_psr_enable_sink Rodrigo Vivi ` (3 subsequent siblings) 4 siblings, 1 reply; 32+ messages in thread From: Rodrigo Vivi @ 2015-11-11 21:19 UTC (permalink / raw) To: intel-gfx; +Cc: Rodrigo Vivi According to VESA spec: "If a Source device receives and IRQ_HPD while in a PSR active state, and cannot identify what caused the IRQ_HPD to be generated, based on Sink device status registers, the Source device can take implementation-specific action. One such action can be to exit and then re-enter a PSR active state." Since we aren't checking for any sink status registers and we aren't looking for any other implementation-specific action, in case we receive any IRQ_HPD and psr is active let's force the exit and reschedule it back. Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 5 ++++- drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_psr.c | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 38 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 99b7f1d..71911b9 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4892,6 +4892,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) if (intel_dig_port->base.type != INTEL_OUTPUT_EDP) intel_dig_port->base.type = INTEL_OUTPUT_DISPLAYPORT; + else + intel_psr_irq_hpd(dev); if (long_hpd && intel_dig_port->base.type == INTEL_OUTPUT_EDP) { /* @@ -4900,8 +4902,9 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) * would end up in an endless cycle of * "vdd off -> long hpd -> vdd on -> detect -> vdd off -> ..." */ - DRM_DEBUG_KMS("ignoring long hpd on eDP port %c\n", + DRM_DEBUG_KMS("long hpd on eDP port %c\n", port_name(intel_dig_port->port)); + return IRQ_HANDLED; } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index e3794d3..95242e6 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1411,6 +1411,7 @@ void intel_psr_flush(struct drm_device *dev, void intel_psr_init(struct drm_device *dev); void intel_psr_single_frame_update(struct drm_device *dev, unsigned frontbuffer_bits); +void intel_psr_irq_hpd(struct drm_device *dev); /* intel_runtime_pm.c */ int intel_power_domains_init(struct drm_i915_private *); diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index 213581c..e5ae844 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -741,6 +741,39 @@ void intel_psr_flush(struct drm_device *dev, } /** + * intel_psr_irq_hpd - Let PSR aware of IRQ_HPD + * @dev: DRM device + * + * This function is called when IRQ_HPD is received on eDP. + */ +void intel_psr_irq_hpd(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + int delay_ms = HAS_DDI(dev) ? 100 : 500; + + mutex_lock(&dev_priv->psr.lock); + + /* + * According to VESA spec "If a Source device receives and IRQ_HPD + * while in a PSR active state, and cannot identify what caused the + * IRQ_HPD to be generated, based on Sink device status registers, + * the Source device can take implementation-specific action. + * One such action can be to exit and then re-enter a PSR active + * state." Since we aren't checking for any sink status registers + * and we aren't looking for any other implementation-specific + * action, in case we receive any IRQ_HPD and psr is active let's + * force the exit and reschedule it back. + */ + if (dev_priv->psr.active) { + intel_psr_exit(dev); + schedule_delayed_work(&dev_priv->psr.work, + msecs_to_jiffies(delay_ms)); + } + + mutex_unlock(&dev_priv->psr.lock); +} + +/** * intel_psr_init - Init basic PSR work and mutex. * @dev: DRM device * -- 2.4.3 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] drm/i915: Force PSR exit when IRQ_HPD is detected on eDP. 2015-11-11 21:19 ` [PATCH 1/4] drm/i915: Force PSR exit when IRQ_HPD is detected on eDP Rodrigo Vivi @ 2015-11-16 16:55 ` Ville Syrjälä 2015-11-18 19:19 ` [PATCH] " Rodrigo Vivi 0 siblings, 1 reply; 32+ messages in thread From: Ville Syrjälä @ 2015-11-16 16:55 UTC (permalink / raw) To: Rodrigo Vivi; +Cc: intel-gfx On Wed, Nov 11, 2015 at 01:19:58PM -0800, Rodrigo Vivi wrote: > According to VESA spec: "If a Source device receives and IRQ_HPD > while in a PSR active state, and cannot identify what caused the > IRQ_HPD to be generated, based on Sink device status registers, > the Source device can take implementation-specific action. > One such action can be to exit and then re-enter a PSR active > state." > > Since we aren't checking for any sink status registers and we > aren't looking for any other implementation-specific action, > in case we receive any IRQ_HPD and psr is active let's force > the exit and reschedule it back. > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 5 ++++- > drivers/gpu/drm/i915/intel_drv.h | 1 + > drivers/gpu/drm/i915/intel_psr.c | 33 +++++++++++++++++++++++++++++++++ > 3 files changed, 38 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 99b7f1d..71911b9 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4892,6 +4892,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) > > if (intel_dig_port->base.type != INTEL_OUTPUT_EDP) > intel_dig_port->base.type = INTEL_OUTPUT_DISPLAYPORT; > + else > + intel_psr_irq_hpd(dev); IRQ_HPD means short HPD. This one is now done for long HPD too. And in any case the placement is a bit weird since now the two branches here are totally unrelated. > > if (long_hpd && intel_dig_port->base.type == INTEL_OUTPUT_EDP) { > /* > @@ -4900,8 +4902,9 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) > * would end up in an endless cycle of > * "vdd off -> long hpd -> vdd on -> detect -> vdd off -> ..." > */ > - DRM_DEBUG_KMS("ignoring long hpd on eDP port %c\n", > + DRM_DEBUG_KMS("long hpd on eDP port %c\n", > port_name(intel_dig_port->port)); > + > return IRQ_HANDLED; > } > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index e3794d3..95242e6 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1411,6 +1411,7 @@ void intel_psr_flush(struct drm_device *dev, > void intel_psr_init(struct drm_device *dev); > void intel_psr_single_frame_update(struct drm_device *dev, > unsigned frontbuffer_bits); > +void intel_psr_irq_hpd(struct drm_device *dev); > > /* intel_runtime_pm.c */ > int intel_power_domains_init(struct drm_i915_private *); > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index 213581c..e5ae844 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -741,6 +741,39 @@ void intel_psr_flush(struct drm_device *dev, > } > > /** > + * intel_psr_irq_hpd - Let PSR aware of IRQ_HPD > + * @dev: DRM device > + * > + * This function is called when IRQ_HPD is received on eDP. > + */ > +void intel_psr_irq_hpd(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + int delay_ms = HAS_DDI(dev) ? 100 : 500; > + > + mutex_lock(&dev_priv->psr.lock); > + > + /* > + * According to VESA spec "If a Source device receives and IRQ_HPD > + * while in a PSR active state, and cannot identify what caused the > + * IRQ_HPD to be generated, based on Sink device status registers, > + * the Source device can take implementation-specific action. > + * One such action can be to exit and then re-enter a PSR active > + * state." Since we aren't checking for any sink status registers > + * and we aren't looking for any other implementation-specific > + * action, in case we receive any IRQ_HPD and psr is active let's > + * force the exit and reschedule it back. > + */ > + if (dev_priv->psr.active) { > + intel_psr_exit(dev); > + schedule_delayed_work(&dev_priv->psr.work, > + msecs_to_jiffies(delay_ms)); > + } > + > + mutex_unlock(&dev_priv->psr.lock); > +} > + > +/** > * intel_psr_init - Init basic PSR work and mutex. > * @dev: DRM device > * > -- > 2.4.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH] drm/i915: Force PSR exit when IRQ_HPD is detected on eDP. 2015-11-16 16:55 ` Ville Syrjälä @ 2015-11-18 19:19 ` Rodrigo Vivi 2015-11-23 21:29 ` Rodrigo Vivi 2015-12-01 18:56 ` Ville Syrjälä 0 siblings, 2 replies; 32+ messages in thread From: Rodrigo Vivi @ 2015-11-18 19:19 UTC (permalink / raw) To: intel-gfx; +Cc: Rodrigo Vivi According to VESA spec: "If a Source device receives and IRQ_HPD while in a PSR active state, and cannot identify what caused the IRQ_HPD to be generated, based on Sink device status registers, the Source device can take implementation-specific action. One such action can be to exit and then re-enter a PSR active state." Since we aren't checking for any sink status registers and we aren't looking for any other implementation-specific action, in case we receive any IRQ_HPD and psr is active let's force the exit and reschedule it back. v2: IRQ_HPD means short HPD so handle it correctly as Ville pointed out. Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 3 +++ drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_psr.c | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 37 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index bec443a..ca7a798 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -5004,6 +5004,9 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) goto mst_fail; } } else { + if (intel_dig_port->base.type == INTEL_OUTPUT_EDP) + intel_psr_irq_hpd(dev); + if (intel_dp->is_mst) { if (intel_dp_check_mst_status(intel_dp) == -EINVAL) goto mst_fail; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index ab5c147..1d61551 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1401,6 +1401,7 @@ void intel_psr_flush(struct drm_device *dev, void intel_psr_init(struct drm_device *dev); void intel_psr_single_frame_update(struct drm_device *dev, unsigned frontbuffer_bits); +void intel_psr_irq_hpd(struct drm_device *dev); /* intel_runtime_pm.c */ int intel_power_domains_init(struct drm_i915_private *); diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index bc5ea2a..465d36b 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -765,6 +765,39 @@ void intel_psr_flush(struct drm_device *dev, } /** + * intel_psr_irq_hpd - Let PSR aware of IRQ_HPD + * @dev: DRM device + * + * This function is called when IRQ_HPD is received on eDP. + */ +void intel_psr_irq_hpd(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + int delay_ms = HAS_DDI(dev) ? 100 : 500; + + mutex_lock(&dev_priv->psr.lock); + + /* + * According to VESA spec "If a Source device receives and IRQ_HPD + * while in a PSR active state, and cannot identify what caused the + * IRQ_HPD to be generated, based on Sink device status registers, + * the Source device can take implementation-specific action. + * One such action can be to exit and then re-enter a PSR active + * state." Since we aren't checking for any sink status registers + * and we aren't looking for any other implementation-specific + * action, in case we receive any IRQ_HPD and psr is active let's + * force the exit and reschedule it back. + */ + if (dev_priv->psr.active) { + intel_psr_exit(dev); + schedule_delayed_work(&dev_priv->psr.work, + msecs_to_jiffies(delay_ms)); + } + + mutex_unlock(&dev_priv->psr.lock); +} + +/** * intel_psr_init - Init basic PSR work and mutex. * @dev: DRM device * -- 2.4.3 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH] drm/i915: Force PSR exit when IRQ_HPD is detected on eDP. 2015-11-18 19:19 ` [PATCH] " Rodrigo Vivi @ 2015-11-23 21:29 ` Rodrigo Vivi 2015-12-01 18:56 ` Ville Syrjälä 1 sibling, 0 replies; 32+ messages in thread From: Rodrigo Vivi @ 2015-11-23 21:29 UTC (permalink / raw) To: Rodrigo Vivi, Ville Syrjälä; +Cc: intel-gfx Hi Ville, Is it better now? Could you please review this patch? Thanks, Rodrigo. On Wed, Nov 18, 2015 at 11:19 AM, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > According to VESA spec: "If a Source device receives and IRQ_HPD > while in a PSR active state, and cannot identify what caused the > IRQ_HPD to be generated, based on Sink device status registers, > the Source device can take implementation-specific action. > One such action can be to exit and then re-enter a PSR active > state." > > Since we aren't checking for any sink status registers and we > aren't looking for any other implementation-specific action, > in case we receive any IRQ_HPD and psr is active let's force > the exit and reschedule it back. > > v2: IRQ_HPD means short HPD so handle it correctly > as Ville pointed out. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 3 +++ > drivers/gpu/drm/i915/intel_drv.h | 1 + > drivers/gpu/drm/i915/intel_psr.c | 33 +++++++++++++++++++++++++++++++++ > 3 files changed, 37 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index bec443a..ca7a798 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -5004,6 +5004,9 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) > goto mst_fail; > } > } else { > + if (intel_dig_port->base.type == INTEL_OUTPUT_EDP) > + intel_psr_irq_hpd(dev); > + > if (intel_dp->is_mst) { > if (intel_dp_check_mst_status(intel_dp) == -EINVAL) > goto mst_fail; > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index ab5c147..1d61551 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1401,6 +1401,7 @@ void intel_psr_flush(struct drm_device *dev, > void intel_psr_init(struct drm_device *dev); > void intel_psr_single_frame_update(struct drm_device *dev, > unsigned frontbuffer_bits); > +void intel_psr_irq_hpd(struct drm_device *dev); > > /* intel_runtime_pm.c */ > int intel_power_domains_init(struct drm_i915_private *); > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index bc5ea2a..465d36b 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -765,6 +765,39 @@ void intel_psr_flush(struct drm_device *dev, > } > > /** > + * intel_psr_irq_hpd - Let PSR aware of IRQ_HPD > + * @dev: DRM device > + * > + * This function is called when IRQ_HPD is received on eDP. > + */ > +void intel_psr_irq_hpd(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + int delay_ms = HAS_DDI(dev) ? 100 : 500; > + > + mutex_lock(&dev_priv->psr.lock); > + > + /* > + * According to VESA spec "If a Source device receives and IRQ_HPD > + * while in a PSR active state, and cannot identify what caused the > + * IRQ_HPD to be generated, based on Sink device status registers, > + * the Source device can take implementation-specific action. > + * One such action can be to exit and then re-enter a PSR active > + * state." Since we aren't checking for any sink status registers > + * and we aren't looking for any other implementation-specific > + * action, in case we receive any IRQ_HPD and psr is active let's > + * force the exit and reschedule it back. > + */ > + if (dev_priv->psr.active) { > + intel_psr_exit(dev); > + schedule_delayed_work(&dev_priv->psr.work, > + msecs_to_jiffies(delay_ms)); > + } > + > + mutex_unlock(&dev_priv->psr.lock); > +} > + > +/** > * intel_psr_init - Init basic PSR work and mutex. > * @dev: DRM device > * > -- > 2.4.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Rodrigo Vivi Blog: http://blog.vivi.eng.br _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] drm/i915: Force PSR exit when IRQ_HPD is detected on eDP. 2015-11-18 19:19 ` [PATCH] " Rodrigo Vivi 2015-11-23 21:29 ` Rodrigo Vivi @ 2015-12-01 18:56 ` Ville Syrjälä 2015-12-01 19:44 ` Vivi, Rodrigo 1 sibling, 1 reply; 32+ messages in thread From: Ville Syrjälä @ 2015-12-01 18:56 UTC (permalink / raw) To: Rodrigo Vivi; +Cc: intel-gfx On Wed, Nov 18, 2015 at 11:19:06AM -0800, Rodrigo Vivi wrote: > According to VESA spec: "If a Source device receives and IRQ_HPD > while in a PSR active state, and cannot identify what caused the > IRQ_HPD to be generated, based on Sink device status registers, > the Source device can take implementation-specific action. > One such action can be to exit and then re-enter a PSR active > state." > > Since we aren't checking for any sink status registers and we > aren't looking for any other implementation-specific action, > in case we receive any IRQ_HPD and psr is active let's force > the exit and reschedule it back. > > v2: IRQ_HPD means short HPD so handle it correctly > as Ville pointed out. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> Do we have a device that needs this for something? Since it's totally implementation specific I would expect that something like VBT would tell us if we need to do something on random short HPDs from the panel. As far as the implementation goes, can't we just call intel_psr_flush() so that the frontbuffer tracking keeps in sync with the actual PSR state? > --- > drivers/gpu/drm/i915/intel_dp.c | 3 +++ > drivers/gpu/drm/i915/intel_drv.h | 1 + > drivers/gpu/drm/i915/intel_psr.c | 33 +++++++++++++++++++++++++++++++++ > 3 files changed, 37 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index bec443a..ca7a798 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -5004,6 +5004,9 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) > goto mst_fail; > } > } else { > + if (intel_dig_port->base.type == INTEL_OUTPUT_EDP) > + intel_psr_irq_hpd(dev); > + > if (intel_dp->is_mst) { > if (intel_dp_check_mst_status(intel_dp) == -EINVAL) > goto mst_fail; > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index ab5c147..1d61551 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1401,6 +1401,7 @@ void intel_psr_flush(struct drm_device *dev, > void intel_psr_init(struct drm_device *dev); > void intel_psr_single_frame_update(struct drm_device *dev, > unsigned frontbuffer_bits); > +void intel_psr_irq_hpd(struct drm_device *dev); > > /* intel_runtime_pm.c */ > int intel_power_domains_init(struct drm_i915_private *); > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index bc5ea2a..465d36b 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -765,6 +765,39 @@ void intel_psr_flush(struct drm_device *dev, > } > > /** > + * intel_psr_irq_hpd - Let PSR aware of IRQ_HPD > + * @dev: DRM device > + * > + * This function is called when IRQ_HPD is received on eDP. > + */ > +void intel_psr_irq_hpd(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + int delay_ms = HAS_DDI(dev) ? 100 : 500; > + > + mutex_lock(&dev_priv->psr.lock); > + > + /* > + * According to VESA spec "If a Source device receives and IRQ_HPD > + * while in a PSR active state, and cannot identify what caused the > + * IRQ_HPD to be generated, based on Sink device status registers, > + * the Source device can take implementation-specific action. > + * One such action can be to exit and then re-enter a PSR active > + * state." Since we aren't checking for any sink status registers > + * and we aren't looking for any other implementation-specific > + * action, in case we receive any IRQ_HPD and psr is active let's > + * force the exit and reschedule it back. > + */ > + if (dev_priv->psr.active) { > + intel_psr_exit(dev); > + schedule_delayed_work(&dev_priv->psr.work, > + msecs_to_jiffies(delay_ms)); > + } > + > + mutex_unlock(&dev_priv->psr.lock); > +} > + > +/** > * intel_psr_init - Init basic PSR work and mutex. > * @dev: DRM device > * > -- > 2.4.3 -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] drm/i915: Force PSR exit when IRQ_HPD is detected on eDP. 2015-12-01 18:56 ` Ville Syrjälä @ 2015-12-01 19:44 ` Vivi, Rodrigo 2015-12-02 9:42 ` Ville Syrjälä 0 siblings, 1 reply; 32+ messages in thread From: Vivi, Rodrigo @ 2015-12-01 19:44 UTC (permalink / raw) To: ville.syrjala; +Cc: intel-gfx On Tue, 2015-12-01 at 20:56 +0200, Ville Syrjälä wrote: > On Wed, Nov 18, 2015 at 11:19:06AM -0800, Rodrigo Vivi wrote: > > According to VESA spec: "If a Source device receives and IRQ_HPD > > while in a PSR active state, and cannot identify what caused the > > IRQ_HPD to be generated, based on Sink device status registers, > > the Source device can take implementation-specific action. > > One such action can be to exit and then re-enter a PSR active > > state." > > > > Since we aren't checking for any sink status registers and we > > aren't looking for any other implementation-specific action, > > in case we receive any IRQ_HPD and psr is active let's force > > the exit and reschedule it back. > > > > v2: IRQ_HPD means short HPD so handle it correctly > > as Ville pointed out. > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Do we have a device that needs this for something? hopefully not. and I'm glad that I never had a case with PSR that I needed it.... I was just trying to follow VESA spec a bit to avoid issues. I've seeing short pulses with some KBL, but also that came along with bad flickerings so PSR shouldn't be the worst problem when this short pulse on port A happens ;) > Since it's totally > implementation specific I would expect that something like VBT would > tell us if we need to do something on random short HPDs from the > panel. hm, I was more afraid from the Sink implementation side... if they were generating IRQ_HPD for some errors where they believe HW will send a frame update. > > As far as the implementation goes, can't we just call > intel_psr_flush() > so that the frontbuffer tracking keeps in sync with the actual PSR > state? that is a good idea... I believe when I first added this patch we weren't relying fully on flush being inactivate+flush, but now with all other patches merged I believe this would be better. But what are your overall opinion now? should I come with a v3 using the flush function or should we just ignore this patch? Thanks for your review and comments! > > > --- > > drivers/gpu/drm/i915/intel_dp.c | 3 +++ > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > drivers/gpu/drm/i915/intel_psr.c | 33 > > +++++++++++++++++++++++++++++++++ > > 3 files changed, 37 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > b/drivers/gpu/drm/i915/intel_dp.c > > index bec443a..ca7a798 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -5004,6 +5004,9 @@ intel_dp_hpd_pulse(struct intel_digital_port > > *intel_dig_port, bool long_hpd) > > goto mst_fail; > > } > > } else { > > + if (intel_dig_port->base.type == INTEL_OUTPUT_EDP) > > + intel_psr_irq_hpd(dev); > > + > > if (intel_dp->is_mst) { > > if (intel_dp_check_mst_status(intel_dp) == > > -EINVAL) > > goto mst_fail; > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > b/drivers/gpu/drm/i915/intel_drv.h > > index ab5c147..1d61551 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -1401,6 +1401,7 @@ void intel_psr_flush(struct drm_device *dev, > > void intel_psr_init(struct drm_device *dev); > > void intel_psr_single_frame_update(struct drm_device *dev, > > unsigned frontbuffer_bits); > > +void intel_psr_irq_hpd(struct drm_device *dev); > > > > /* intel_runtime_pm.c */ > > int intel_power_domains_init(struct drm_i915_private *); > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > b/drivers/gpu/drm/i915/intel_psr.c > > index bc5ea2a..465d36b 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -765,6 +765,39 @@ void intel_psr_flush(struct drm_device *dev, > > } > > > > /** > > + * intel_psr_irq_hpd - Let PSR aware of IRQ_HPD > > + * @dev: DRM device > > + * > > + * This function is called when IRQ_HPD is received on eDP. > > + */ > > +void intel_psr_irq_hpd(struct drm_device *dev) > > +{ > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + int delay_ms = HAS_DDI(dev) ? 100 : 500; > > + > > + mutex_lock(&dev_priv->psr.lock); > > + > > + /* > > + * According to VESA spec "If a Source device receives and > > IRQ_HPD > > + * while in a PSR active state, and cannot identify what > > caused the > > + * IRQ_HPD to be generated, based on Sink device status > > registers, > > + * the Source device can take implementation-specific > > action. > > + * One such action can be to exit and then re-enter a PSR > > active > > + * state." Since we aren't checking for any sink status > > registers > > + * and we aren't looking for any other implementation > > -specific > > + * action, in case we receive any IRQ_HPD and psr is > > active let's > > + * force the exit and reschedule it back. > > + */ > > + if (dev_priv->psr.active) { > > + intel_psr_exit(dev); > > + schedule_delayed_work(&dev_priv->psr.work, > > + msecs_to_jiffies(delay_ms)); > > + } > > + > > + mutex_unlock(&dev_priv->psr.lock); > > +} > > + > > +/** > > * intel_psr_init - Init basic PSR work and mutex. > > * @dev: DRM device > > * > > -- > > 2.4.3 > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] drm/i915: Force PSR exit when IRQ_HPD is detected on eDP. 2015-12-01 19:44 ` Vivi, Rodrigo @ 2015-12-02 9:42 ` Ville Syrjälä 2015-12-02 17:29 ` Vivi, Rodrigo 0 siblings, 1 reply; 32+ messages in thread From: Ville Syrjälä @ 2015-12-02 9:42 UTC (permalink / raw) To: Vivi, Rodrigo; +Cc: intel-gfx On Tue, Dec 01, 2015 at 07:44:00PM +0000, Vivi, Rodrigo wrote: > On Tue, 2015-12-01 at 20:56 +0200, Ville Syrjälä wrote: > > On Wed, Nov 18, 2015 at 11:19:06AM -0800, Rodrigo Vivi wrote: > > > According to VESA spec: "If a Source device receives and IRQ_HPD > > > while in a PSR active state, and cannot identify what caused the > > > IRQ_HPD to be generated, based on Sink device status registers, > > > the Source device can take implementation-specific action. > > > One such action can be to exit and then re-enter a PSR active > > > state." > > > > > > Since we aren't checking for any sink status registers and we > > > aren't looking for any other implementation-specific action, > > > in case we receive any IRQ_HPD and psr is active let's force > > > the exit and reschedule it back. > > > > > > v2: IRQ_HPD means short HPD so handle it correctly > > > as Ville pointed out. > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > Do we have a device that needs this for something? > > hopefully not. and I'm glad that I never had a case with PSR that I > needed it.... I was just trying to follow VESA spec a bit to avoid > issues. > > I've seeing short pulses with some KBL, but also that came along with > bad flickerings so PSR shouldn't be the worst problem when this short > pulse on port A happens ;) > > > Since it's totally > > implementation specific I would expect that something like VBT would > > tell us if we need to do something on random short HPDs from the > > panel. > > hm, I was more afraid from the Sink implementation side... if they were > generating IRQ_HPD for some errors where they believe HW will send a > frame update. > > > > > As far as the implementation goes, can't we just call > > intel_psr_flush() > > so that the frontbuffer tracking keeps in sync with the actual PSR > > state? > > that is a good idea... I believe when I first added this patch we > weren't relying fully on flush being inactivate+flush, but now with all > other patches merged I believe this would be better. > > But what are your overall opinion now? should I come with a v3 using > the flush function or should we just ignore this patch? Dunno. I suppose it can't do too much harm. Well, unless there's a sink that keeps signalling something unrelated to us that we don't understand, and thus would kick it out of PSR all the time. > > Thanks for your review and comments! > > > > > > --- > > > drivers/gpu/drm/i915/intel_dp.c | 3 +++ > > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > > drivers/gpu/drm/i915/intel_psr.c | 33 > > > +++++++++++++++++++++++++++++++++ > > > 3 files changed, 37 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > b/drivers/gpu/drm/i915/intel_dp.c > > > index bec443a..ca7a798 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -5004,6 +5004,9 @@ intel_dp_hpd_pulse(struct intel_digital_port > > > *intel_dig_port, bool long_hpd) > > > goto mst_fail; > > > } > > > } else { > > > + if (intel_dig_port->base.type == INTEL_OUTPUT_EDP) > > > + intel_psr_irq_hpd(dev); > > > + > > > if (intel_dp->is_mst) { > > > if (intel_dp_check_mst_status(intel_dp) == > > > -EINVAL) > > > goto mst_fail; > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > b/drivers/gpu/drm/i915/intel_drv.h > > > index ab5c147..1d61551 100644 > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > @@ -1401,6 +1401,7 @@ void intel_psr_flush(struct drm_device *dev, > > > void intel_psr_init(struct drm_device *dev); > > > void intel_psr_single_frame_update(struct drm_device *dev, > > > unsigned frontbuffer_bits); > > > +void intel_psr_irq_hpd(struct drm_device *dev); > > > > > > /* intel_runtime_pm.c */ > > > int intel_power_domains_init(struct drm_i915_private *); > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > b/drivers/gpu/drm/i915/intel_psr.c > > > index bc5ea2a..465d36b 100644 > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > @@ -765,6 +765,39 @@ void intel_psr_flush(struct drm_device *dev, > > > } > > > > > > /** > > > + * intel_psr_irq_hpd - Let PSR aware of IRQ_HPD > > > + * @dev: DRM device > > > + * > > > + * This function is called when IRQ_HPD is received on eDP. > > > + */ > > > +void intel_psr_irq_hpd(struct drm_device *dev) > > > +{ > > > + struct drm_i915_private *dev_priv = dev->dev_private; > > > + int delay_ms = HAS_DDI(dev) ? 100 : 500; > > > + > > > + mutex_lock(&dev_priv->psr.lock); > > > + > > > + /* > > > + * According to VESA spec "If a Source device receives and > > > IRQ_HPD > > > + * while in a PSR active state, and cannot identify what > > > caused the > > > + * IRQ_HPD to be generated, based on Sink device status > > > registers, > > > + * the Source device can take implementation-specific > > > action. > > > + * One such action can be to exit and then re-enter a PSR > > > active > > > + * state." Since we aren't checking for any sink status > > > registers > > > + * and we aren't looking for any other implementation > > > -specific > > > + * action, in case we receive any IRQ_HPD and psr is > > > active let's > > > + * force the exit and reschedule it back. > > > + */ > > > + if (dev_priv->psr.active) { > > > + intel_psr_exit(dev); > > > + schedule_delayed_work(&dev_priv->psr.work, > > > + msecs_to_jiffies(delay_ms)); > > > + } > > > + > > > + mutex_unlock(&dev_priv->psr.lock); > > > +} > > > + > > > +/** > > > * intel_psr_init - Init basic PSR work and mutex. > > > * @dev: DRM device > > > * > > > -- > > > 2.4.3 > > -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] drm/i915: Force PSR exit when IRQ_HPD is detected on eDP. 2015-12-02 9:42 ` Ville Syrjälä @ 2015-12-02 17:29 ` Vivi, Rodrigo 0 siblings, 0 replies; 32+ messages in thread From: Vivi, Rodrigo @ 2015-12-02 17:29 UTC (permalink / raw) To: ville.syrjala; +Cc: intel-gfx On Wed, 2015-12-02 at 11:42 +0200, Ville Syrjälä wrote: > On Tue, Dec 01, 2015 at 07:44:00PM +0000, Vivi, Rodrigo wrote: > > On Tue, 2015-12-01 at 20:56 +0200, Ville Syrjälä wrote: > > > On Wed, Nov 18, 2015 at 11:19:06AM -0800, Rodrigo Vivi wrote: > > > > According to VESA spec: "If a Source device receives and > > > > IRQ_HPD > > > > while in a PSR active state, and cannot identify what caused > > > > the > > > > IRQ_HPD to be generated, based on Sink device status registers, > > > > the Source device can take implementation-specific action. > > > > One such action can be to exit and then re-enter a PSR active > > > > state." > > > > > > > > Since we aren't checking for any sink status registers and we > > > > aren't looking for any other implementation-specific action, > > > > in case we receive any IRQ_HPD and psr is active let's force > > > > the exit and reschedule it back. > > > > > > > > v2: IRQ_HPD means short HPD so handle it correctly > > > > as Ville pointed out. > > > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > Do we have a device that needs this for something? > > > > hopefully not. and I'm glad that I never had a case with PSR that I > > needed it.... I was just trying to follow VESA spec a bit to avoid > > issues. > > > > I've seeing short pulses with some KBL, but also that came along > > with > > bad flickerings so PSR shouldn't be the worst problem when this > > short > > pulse on port A happens ;) > > > > > Since it's totally > > > implementation specific I would expect that something like VBT > > > would > > > tell us if we need to do something on random short HPDs from the > > > panel. > > > > hm, I was more afraid from the Sink implementation side... if they > > were > > generating IRQ_HPD for some errors where they believe HW will send > > a > > frame update. > > > > > > > > As far as the implementation goes, can't we just call > > > intel_psr_flush() > > > so that the frontbuffer tracking keeps in sync with the actual > > > PSR > > > state? > > > > that is a good idea... I believe when I first added this patch we > > weren't relying fully on flush being inactivate+flush, but now with > > all > > other patches merged I believe this would be better. > > > > But what are your overall opinion now? should I come with a v3 > > using > > the flush function or should we just ignore this patch? > > Dunno. I suppose it can't do too much harm. Well, unless there's a > sink > that keeps signalling something unrelated to us that we don't > understand, > and thus would kick it out of PSR all the time. yeah, good point... So I will hold this one for now if we need I can rework the v3 with flush and send again in the future. Thanks, Rodrigo. > > > > > Thanks for your review and comments! > > > > > > > > > --- > > > > drivers/gpu/drm/i915/intel_dp.c | 3 +++ > > > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > > > drivers/gpu/drm/i915/intel_psr.c | 33 > > > > +++++++++++++++++++++++++++++++++ > > > > 3 files changed, 37 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > > b/drivers/gpu/drm/i915/intel_dp.c > > > > index bec443a..ca7a798 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > @@ -5004,6 +5004,9 @@ intel_dp_hpd_pulse(struct > > > > intel_digital_port > > > > *intel_dig_port, bool long_hpd) > > > > goto mst_fail; > > > > } > > > > } else { > > > > + if (intel_dig_port->base.type == > > > > INTEL_OUTPUT_EDP) > > > > + intel_psr_irq_hpd(dev); > > > > + > > > > if (intel_dp->is_mst) { > > > > if > > > > (intel_dp_check_mst_status(intel_dp) == > > > > -EINVAL) > > > > goto mst_fail; > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > > b/drivers/gpu/drm/i915/intel_drv.h > > > > index ab5c147..1d61551 100644 > > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > > @@ -1401,6 +1401,7 @@ void intel_psr_flush(struct drm_device > > > > *dev, > > > > void intel_psr_init(struct drm_device *dev); > > > > void intel_psr_single_frame_update(struct drm_device *dev, > > > > unsigned frontbuffer_bits); > > > > +void intel_psr_irq_hpd(struct drm_device *dev); > > > > > > > > /* intel_runtime_pm.c */ > > > > int intel_power_domains_init(struct drm_i915_private *); > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > > b/drivers/gpu/drm/i915/intel_psr.c > > > > index bc5ea2a..465d36b 100644 > > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > > @@ -765,6 +765,39 @@ void intel_psr_flush(struct drm_device > > > > *dev, > > > > } > > > > > > > > /** > > > > + * intel_psr_irq_hpd - Let PSR aware of IRQ_HPD > > > > + * @dev: DRM device > > > > + * > > > > + * This function is called when IRQ_HPD is received on eDP. > > > > + */ > > > > +void intel_psr_irq_hpd(struct drm_device *dev) > > > > +{ > > > > + struct drm_i915_private *dev_priv = dev->dev_private; > > > > + int delay_ms = HAS_DDI(dev) ? 100 : 500; > > > > + > > > > + mutex_lock(&dev_priv->psr.lock); > > > > + > > > > + /* > > > > + * According to VESA spec "If a Source device receives > > > > and > > > > IRQ_HPD > > > > + * while in a PSR active state, and cannot identify > > > > what > > > > caused the > > > > + * IRQ_HPD to be generated, based on Sink device > > > > status > > > > registers, > > > > + * the Source device can take implementation-specific > > > > action. > > > > + * One such action can be to exit and then re-enter a > > > > PSR > > > > active > > > > + * state." Since we aren't checking for any sink > > > > status > > > > registers > > > > + * and we aren't looking for any other implementation > > > > -specific > > > > + * action, in case we receive any IRQ_HPD and psr is > > > > active let's > > > > + * force the exit and reschedule it back. > > > > + */ > > > > + if (dev_priv->psr.active) { > > > > + intel_psr_exit(dev); > > > > + schedule_delayed_work(&dev_priv->psr.work, > > > > + > > > > msecs_to_jiffies(delay_ms)); > > > > + } > > > > + > > > > + mutex_unlock(&dev_priv->psr.lock); > > > > +} > > > > + > > > > +/** > > > > * intel_psr_init - Init basic PSR work and mutex. > > > > * @dev: DRM device > > > > * > > > > -- > > > > 2.4.3 > > > > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 2/4] drm/i915: Remove duplicated dpcd write on hsw_psr_enable_sink. 2015-11-11 21:19 [PATCH 0/4] PSR general improvements and stabilization Rodrigo Vivi 2015-11-11 21:19 ` [PATCH 1/4] drm/i915: Force PSR exit when IRQ_HPD is detected on eDP Rodrigo Vivi @ 2015-11-11 21:19 ` Rodrigo Vivi 2015-11-12 22:46 ` [PATCH] " Rodrigo Vivi 2015-11-11 21:20 ` [PATCH 3/4] drm/i915: PSR: Let's rely more on frontbuffer tracking Rodrigo Vivi ` (2 subsequent siblings) 4 siblings, 1 reply; 32+ messages in thread From: Rodrigo Vivi @ 2015-11-11 21:19 UTC (permalink / raw) To: intel-gfx; +Cc: Rodrigo Vivi This is wrong since my commit (89251b17). The intention of that commit was to remove this one here that is also wrong anyway, but it was forgotten. Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/intel_psr.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index e5ae844..e5b3fce 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -172,9 +172,6 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp) aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0); - drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, - DP_PSR_ENABLE & ~DP_PSR_MAIN_LINK_ACTIVE); - /* Enable AUX frame sync at sink */ if (dev_priv->psr.aux_frame_sync) drm_dp_dpcd_writeb(&intel_dp->aux, -- 2.4.3 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH] drm/i915: Remove duplicated dpcd write on hsw_psr_enable_sink. 2015-11-11 21:19 ` [PATCH 2/4] drm/i915: Remove duplicated dpcd write on hsw_psr_enable_sink Rodrigo Vivi @ 2015-11-12 22:46 ` Rodrigo Vivi 2015-11-16 16:44 ` Paulo Zanoni 0 siblings, 1 reply; 32+ messages in thread From: Rodrigo Vivi @ 2015-11-12 22:46 UTC (permalink / raw) To: intel-gfx; +Cc: Rodrigo Vivi Commit (89251b17) intended to remove this line and let only one DP_PSR_EN_CFG set, but it was wrong and this call is now duplicated at the code. Also "& ~DP_PSR_MAIN_LINK_ACTIVE" doesn't do anything at all. It was like that since I introduced this call but probably the idea was to be informative and make clear statement that we were not using the link standby. So it is better to remove this one here and let the code a bit cleaner. v2: Improve commit message as requested by Paulo. Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Tested-by: Brian Norris <briannorris@chromium.org> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/intel_psr.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index e5ae844..e5b3fce 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -172,9 +172,6 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp) aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0); - drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, - DP_PSR_ENABLE & ~DP_PSR_MAIN_LINK_ACTIVE); - /* Enable AUX frame sync at sink */ if (dev_priv->psr.aux_frame_sync) drm_dp_dpcd_writeb(&intel_dp->aux, -- 2.4.3 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH] drm/i915: Remove duplicated dpcd write on hsw_psr_enable_sink. 2015-11-12 22:46 ` [PATCH] " Rodrigo Vivi @ 2015-11-16 16:44 ` Paulo Zanoni 0 siblings, 0 replies; 32+ messages in thread From: Paulo Zanoni @ 2015-11-16 16:44 UTC (permalink / raw) To: Rodrigo Vivi; +Cc: Intel Graphics Development 2015-11-12 20:46 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>: > Commit (89251b17) intended to remove this line and let only one > DP_PSR_EN_CFG set, but it was wrong and this call is now duplicated > at the code. > > Also "& ~DP_PSR_MAIN_LINK_ACTIVE" doesn't do anything at all. It > was like that since I introduced this call but probably the idea > was to be informative and make clear statement that we were not using > the link standby. So it is better to remove this one here and let > the code a bit cleaner. > > v2: Improve commit message as requested by Paulo. > Another side effect not mentioned in the commit message is that now we're flipping the DP_PSR_ENABLE bit a little later than we were before, but it looks like this is not a problem. Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Tested-by: Brian Norris <briannorris@chromium.org> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/intel_psr.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index e5ae844..e5b3fce 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -172,9 +172,6 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp) > > aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0); > > - drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, > - DP_PSR_ENABLE & ~DP_PSR_MAIN_LINK_ACTIVE); > - > /* Enable AUX frame sync at sink */ > if (dev_priv->psr.aux_frame_sync) > drm_dp_dpcd_writeb(&intel_dp->aux, > -- > 2.4.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Paulo Zanoni _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 3/4] drm/i915: PSR: Let's rely more on frontbuffer tracking. 2015-11-11 21:19 [PATCH 0/4] PSR general improvements and stabilization Rodrigo Vivi 2015-11-11 21:19 ` [PATCH 1/4] drm/i915: Force PSR exit when IRQ_HPD is detected on eDP Rodrigo Vivi 2015-11-11 21:19 ` [PATCH 2/4] drm/i915: Remove duplicated dpcd write on hsw_psr_enable_sink Rodrigo Vivi @ 2015-11-11 21:20 ` Rodrigo Vivi 2015-11-14 0:56 ` [PATCH] " Rodrigo Vivi 2015-11-11 21:20 ` [PATCH 4/4] drm/i915: PSR: Mask LPSP hw tracking back again Rodrigo Vivi 2015-11-24 17:12 ` [PATCH 0/4] PSR general improvements and stabilization Daniel Stone 4 siblings, 1 reply; 32+ messages in thread From: Rodrigo Vivi @ 2015-11-11 21:20 UTC (permalink / raw) To: intel-gfx; +Cc: Rodrigo Vivi Many reasons here: - Hardware tracking also has hidden corner cases - Frontbuffer tracking is mature and reliable now - Our sw exit by unseting bit 31 is really fast and reliable. Also frontbuffer tracking flush means invalidate and flush. So, let's rely more and do the proper meaning of flush for all cases without any workaround. Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/intel_psr.c | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index e5b3fce..b0e343c 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -711,25 +711,9 @@ void intel_psr_flush(struct drm_device *dev, frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe); dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits; - if (HAS_DDI(dev)) { - /* - * By definition every flush should mean invalidate + flush, - * however on core platforms let's minimize the - * disable/re-enable so we can avoid the invalidate when flip - * originated the flush. - */ - if (frontbuffer_bits && origin != ORIGIN_FLIP) - intel_psr_exit(dev); - } else { - /* - * On Valleyview and Cherryview we don't use hardware tracking - * so any plane updates or cursor moves don't result in a PSR - * invalidating. Which means we need to manually fake this in - * software for all flushes. - */ - if (frontbuffer_bits) - intel_psr_exit(dev); - } + /* By definition flush = invalidate + flush */ + if (frontbuffer_bits) + intel_psr_exit(dev); if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits) schedule_delayed_work(&dev_priv->psr.work, -- 2.4.3 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH] drm/i915: PSR: Let's rely more on frontbuffer tracking. 2015-11-11 21:20 ` [PATCH 3/4] drm/i915: PSR: Let's rely more on frontbuffer tracking Rodrigo Vivi @ 2015-11-14 0:56 ` Rodrigo Vivi 2015-11-16 18:57 ` Paulo Zanoni 0 siblings, 1 reply; 32+ messages in thread From: Rodrigo Vivi @ 2015-11-14 0:56 UTC (permalink / raw) To: intel-gfx; +Cc: Rodrigo Vivi The ultimate goal here is to remove the dependency we currently have on audio driver power to get PSR working. With audio driver runtime PM disabled the Hardware tracking believes graphics is fully active and prevent PSR Entry, or in other words continuously exit PSR. When we introduced PSR we let LPSP masked allowing us to get PSR independtly from the audio runtime PM. However in one of the attempts to get PSR enabled by default one user reported one specific case where he would miss screen updates if scrolling the firefox in a Gnome environment when i915 runtime pm was enabled. So for this specific case that (I could never create an i-g-t test case) we decided to remove the LPSP mask and let HW tracking taking care of this case. So, we started depending on audio driver again. An alternative solution that makes us indepent and also solve this case is to fully rely on our frontbuffer tracking that is really mature right now. From the frontbuffer tracking perspective the flush means invalidate and flush. But without this patch for HSW, BDW and SKL we just do the invalidate part when the flush wasn't originated by a page flip because we were trusting the HW tracking for the flip case, that is exactly the case with firefox scrolling on gnome. So let's rely more on frontbuffer tracking and do the invalidation regardless the origin as expected and for all platforms. v2: Improve commit message as suggested by Paulo. Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/intel_psr.c | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index e5b3fce..b0e343c 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -711,25 +711,9 @@ void intel_psr_flush(struct drm_device *dev, frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe); dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits; - if (HAS_DDI(dev)) { - /* - * By definition every flush should mean invalidate + flush, - * however on core platforms let's minimize the - * disable/re-enable so we can avoid the invalidate when flip - * originated the flush. - */ - if (frontbuffer_bits && origin != ORIGIN_FLIP) - intel_psr_exit(dev); - } else { - /* - * On Valleyview and Cherryview we don't use hardware tracking - * so any plane updates or cursor moves don't result in a PSR - * invalidating. Which means we need to manually fake this in - * software for all flushes. - */ - if (frontbuffer_bits) - intel_psr_exit(dev); - } + /* By definition flush = invalidate + flush */ + if (frontbuffer_bits) + intel_psr_exit(dev); if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits) schedule_delayed_work(&dev_priv->psr.work, -- 2.4.3 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH] drm/i915: PSR: Let's rely more on frontbuffer tracking. 2015-11-14 0:56 ` [PATCH] " Rodrigo Vivi @ 2015-11-16 18:57 ` Paulo Zanoni 2015-11-16 20:39 ` Vivi, Rodrigo 2015-11-18 19:21 ` [PATCH 1/2] " Rodrigo Vivi 0 siblings, 2 replies; 32+ messages in thread From: Paulo Zanoni @ 2015-11-16 18:57 UTC (permalink / raw) To: Rodrigo Vivi; +Cc: Intel Graphics Development 2015-11-13 22:56 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>: > The ultimate goal here is to remove the dependency we > currently have on audio driver power to get PSR working. > > With audio driver runtime PM disabled the Hardware tracking > believes graphics is fully active and prevent PSR Entry, or > in other words continuously exit PSR. > > When we introduced PSR we let LPSP masked allowing us > to get PSR independtly from the audio runtime PM. However > in one of the attempts to get PSR enabled by default one > user reported one specific case where he would miss > screen updates if scrolling the firefox in a Gnome > environment when i915 runtime pm was enabled. So for > this specific case that (I could never create an i-g-t > test case) we decided to remove the LPSP mask and > let HW tracking taking care of this case. So, we > started depending on audio driver again. Thanks for the better commit message, but I still think there's a huge gap between the paragraph above and the paragraph below. What is still not clear to me is: what is the relationship between the LPSP mask problem mentioned above and the automatic page flip tracking mentioned below? In other words: why not relying on the automatic HW tracking solves the bug? Also, did you do any measurements on how this patch affects PC state residency on the affected platforms? I expect to see some delta here. The patch itself looks fine. And we can always look into re-adding the HW piece after we get the current bugs sorted. With those two explained in the commit message: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > An alternative solution that makes us indepent and also > solve this case is to fully rely on our frontbuffer > tracking that is really mature right now. > > From the frontbuffer tracking perspective the flush means > invalidate and flush. But without this patch for HSW, BDW > and SKL we just do the invalidate part when the flush wasn't > originated by a page flip because we were trusting the HW > tracking for the flip case, that is exactly the case with > firefox scrolling on gnome. > > So let's rely more on frontbuffer tracking and do the > invalidation regardless the origin as expected and for > all platforms. > > v2: Improve commit message as suggested by Paulo. > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/intel_psr.c | 22 +++------------------- > 1 file changed, 3 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index e5b3fce..b0e343c 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -711,25 +711,9 @@ void intel_psr_flush(struct drm_device *dev, > frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe); > dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits; > > - if (HAS_DDI(dev)) { > - /* > - * By definition every flush should mean invalidate + flush, > - * however on core platforms let's minimize the > - * disable/re-enable so we can avoid the invalidate when flip > - * originated the flush. > - */ > - if (frontbuffer_bits && origin != ORIGIN_FLIP) > - intel_psr_exit(dev); > - } else { > - /* > - * On Valleyview and Cherryview we don't use hardware tracking > - * so any plane updates or cursor moves don't result in a PSR > - * invalidating. Which means we need to manually fake this in > - * software for all flushes. > - */ > - if (frontbuffer_bits) > - intel_psr_exit(dev); > - } > + /* By definition flush = invalidate + flush */ > + if (frontbuffer_bits) > + intel_psr_exit(dev); > > if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits) > schedule_delayed_work(&dev_priv->psr.work, > -- > 2.4.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Paulo Zanoni _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] drm/i915: PSR: Let's rely more on frontbuffer tracking. 2015-11-16 18:57 ` Paulo Zanoni @ 2015-11-16 20:39 ` Vivi, Rodrigo 2015-11-18 19:21 ` [PATCH 1/2] " Rodrigo Vivi 1 sibling, 0 replies; 32+ messages in thread From: Vivi, Rodrigo @ 2015-11-16 20:39 UTC (permalink / raw) To: przanoni; +Cc: intel-gfx On Mon, 2015-11-16 at 16:57 -0200, Paulo Zanoni wrote: > 2015-11-13 22:56 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>: > > The ultimate goal here is to remove the dependency we > > currently have on audio driver power to get PSR working. > > > > With audio driver runtime PM disabled the Hardware tracking > > believes graphics is fully active and prevent PSR Entry, or > > in other words continuously exit PSR. > > > > When we introduced PSR we let LPSP masked allowing us > > to get PSR independtly from the audio runtime PM. However > > in one of the attempts to get PSR enabled by default one > > user reported one specific case where he would miss > > screen updates if scrolling the firefox in a Gnome > > environment when i915 runtime pm was enabled. So for > > this specific case that (I could never create an i-g-t > > test case) we decided to remove the LPSP mask and > > let HW tracking taking care of this case. So, we > > started depending on audio driver again. > > Thanks for the better commit message, but I still think there's a > huge > gap between the paragraph above and the paragraph below. What is > still > not clear to me is: what is the relationship between the LPSP mask > problem mentioned above and the automatic page flip tracking > mentioned > below? In other words: why not relying on the automatic HW tracking > solves the bug? So, or we solve by fully relying on SW tracking or we solve by fully relying on HW tracking. The main disadvantage on the HW tracking is that we end up depending on audio driver. Since the SW tracking is more mature and covering all our use cases on Linux is better to rely more on the SW and implement the function as expected (i.e. flush = inactivate + flush.) > > Also, did you do any measurements on how this patch affects PC state > residency on the affected platforms? I expect to see some delta here. No, I didn't. Yes, it can affect the PC residency, but it depends a lot on the use case, the environment, etc. Although on the idle scenario that is the one that targets maximum PC residency we should see no difference. > > The patch itself looks fine. And we can always look into re-adding > the > HW piece after we get the current bugs sorted. > > With those two explained in the commit message: > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> I will prepare another v. Thanks! > > > > > An alternative solution that makes us indepent and also > > solve this case is to fully rely on our frontbuffer > > tracking that is really mature right now. > > > > From the frontbuffer tracking perspective the flush means > > invalidate and flush. But without this patch for HSW, BDW > > and SKL we just do the invalidate part when the flush wasn't > > originated by a page flip because we were trusting the HW > > tracking for the flip case, that is exactly the case with > > firefox scrolling on gnome. > > > > So let's rely more on frontbuffer tracking and do the > > invalidation regardless the origin as expected and for > > all platforms. > > > > v2: Improve commit message as suggested by Paulo. > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/i915/intel_psr.c | 22 +++------------------- > > 1 file changed, 3 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > b/drivers/gpu/drm/i915/intel_psr.c > > index e5b3fce..b0e343c 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -711,25 +711,9 @@ void intel_psr_flush(struct drm_device *dev, > > frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe); > > dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits; > > > > - if (HAS_DDI(dev)) { > > - /* > > - * By definition every flush should mean invalidate > > + flush, > > - * however on core platforms let's minimize the > > - * disable/re-enable so we can avoid the invalidate > > when flip > > - * originated the flush. > > - */ > > - if (frontbuffer_bits && origin != ORIGIN_FLIP) > > - intel_psr_exit(dev); > > - } else { > > - /* > > - * On Valleyview and Cherryview we don't use > > hardware tracking > > - * so any plane updates or cursor moves don't > > result in a PSR > > - * invalidating. Which means we need to manually > > fake this in > > - * software for all flushes. > > - */ > > - if (frontbuffer_bits) > > - intel_psr_exit(dev); > > - } > > + /* By definition flush = invalidate + flush */ > > + if (frontbuffer_bits) > > + intel_psr_exit(dev); > > > > if (!dev_priv->psr.active && !dev_priv > > ->psr.busy_frontbuffer_bits) > > schedule_delayed_work(&dev_priv->psr.work, > > -- > > 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 1/2] drm/i915: PSR: Let's rely more on frontbuffer tracking. 2015-11-16 18:57 ` Paulo Zanoni 2015-11-16 20:39 ` Vivi, Rodrigo @ 2015-11-18 19:21 ` Rodrigo Vivi 2015-11-18 19:27 ` Zanoni, Paulo R 1 sibling, 1 reply; 32+ messages in thread From: Rodrigo Vivi @ 2015-11-18 19:21 UTC (permalink / raw) To: intel-gfx; +Cc: Rodrigo Vivi The ultimate goal here is to remove the dependency we currently have on audio driver power to get PSR working. Since with audio driver runtime PM disabled the Hardware tracking believes graphics is fully active and prevent PSR Entry, or in other words continuously exit PSR. So, the idea is to transfer the PSR exit responsability from the HW tracking to the SW tracking (frontbuffer tracking), who is really mature right now. However with LPSP masked out there might be cases where we could miss exit from HW tracking since it can be relying on this, like a specific case reported at our mailing list who user reported he would miss screen updates if scrolling firefox in a Gnome environment when i915 runtimepm was enabled. So before masking out LPSP again to make us independent from the audio driver we need to make sure that all our cases are coverred from the frontbuffer tracking perspective, where the flush means invalidate and flush. Without this patch for HSW, BDW and SKL we just do the invalidate part when the flush wasn't originated by a page flip because we were trusting the HW tracking for the flip case. So let's rely more on frontbuffer tracking and do the invalidation regardless the origin as expected for all platforms. v2: Improve commit message as suggested by Paulo. v3: Another attempt to let commit message more clear. Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/intel_psr.c | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index e5b3fce..b0e343c 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -711,25 +711,9 @@ void intel_psr_flush(struct drm_device *dev, frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe); dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits; - if (HAS_DDI(dev)) { - /* - * By definition every flush should mean invalidate + flush, - * however on core platforms let's minimize the - * disable/re-enable so we can avoid the invalidate when flip - * originated the flush. - */ - if (frontbuffer_bits && origin != ORIGIN_FLIP) - intel_psr_exit(dev); - } else { - /* - * On Valleyview and Cherryview we don't use hardware tracking - * so any plane updates or cursor moves don't result in a PSR - * invalidating. Which means we need to manually fake this in - * software for all flushes. - */ - if (frontbuffer_bits) - intel_psr_exit(dev); - } + /* By definition flush = invalidate + flush */ + if (frontbuffer_bits) + intel_psr_exit(dev); if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits) schedule_delayed_work(&dev_priv->psr.work, -- 2.4.3 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] drm/i915: PSR: Let's rely more on frontbuffer tracking. 2015-11-18 19:21 ` [PATCH 1/2] " Rodrigo Vivi @ 2015-11-18 19:27 ` Zanoni, Paulo R 2015-11-19 11:16 ` Jani Nikula 0 siblings, 1 reply; 32+ messages in thread From: Zanoni, Paulo R @ 2015-11-18 19:27 UTC (permalink / raw) To: intel-gfx, Vivi, Rodrigo Em Qua, 2015-11-18 às 11:21 -0800, Rodrigo Vivi escreveu: > The ultimate goal here is to remove the dependency we > currently have on audio driver power to get PSR working. > Since with audio driver runtime PM disabled the Hardware tracking > believes graphics is fully active and prevent PSR Entry, or > in other words continuously exit PSR. > > So, the idea is to transfer the PSR exit responsability > from the HW tracking to the SW tracking (frontbuffer tracking), > who is really mature right now. > > However with LPSP masked out there might be cases where we could > miss exit from HW tracking since it can be relying on this, > like a specific case reported at our mailing list who > user reported he would miss screen updates if scrolling firefox > in a Gnome environment when i915 runtimepm was enabled. > > So before masking out LPSP again to make us independent from > the audio driver we need to make sure that all our cases > are coverred from the frontbuffer tracking perspective, > where the flush means invalidate and flush. > > Without this patch for HSW, BDW and SKL we just do the > invalidate part when the flush wasn't originated by a page flip > because we were trusting the HW tracking for the flip case. > > So let's rely more on frontbuffer tracking and do the > invalidation regardless the origin as expected for all platforms. > > v2: Improve commit message as suggested by Paulo. > > v3: Another attempt to let commit message more clear. > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> s/Cc/Reviewed-by/ > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/intel_psr.c | 22 +++------------------- > 1 file changed, 3 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > b/drivers/gpu/drm/i915/intel_psr.c > index e5b3fce..b0e343c 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -711,25 +711,9 @@ void intel_psr_flush(struct drm_device *dev, > frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe); > dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits; > > - if (HAS_DDI(dev)) { > - /* > - * By definition every flush should mean invalidate > + flush, > - * however on core platforms let's minimize the > - * disable/re-enable so we can avoid the invalidate > when flip > - * originated the flush. > - */ > - if (frontbuffer_bits && origin != ORIGIN_FLIP) > - intel_psr_exit(dev); > - } else { > - /* > - * On Valleyview and Cherryview we don't use > hardware tracking > - * so any plane updates or cursor moves don't result > in a PSR > - * invalidating. Which means we need to manually > fake this in > - * software for all flushes. > - */ > - if (frontbuffer_bits) > - intel_psr_exit(dev); > - } > + /* By definition flush = invalidate + flush */ > + if (frontbuffer_bits) > + intel_psr_exit(dev); > > if (!dev_priv->psr.active && !dev_priv- > >psr.busy_frontbuffer_bits) > schedule_delayed_work(&dev_priv->psr.work, _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] drm/i915: PSR: Let's rely more on frontbuffer tracking. 2015-11-18 19:27 ` Zanoni, Paulo R @ 2015-11-19 11:16 ` Jani Nikula 2015-11-19 11:24 ` Zanoni, Paulo R 0 siblings, 1 reply; 32+ messages in thread From: Jani Nikula @ 2015-11-19 11:16 UTC (permalink / raw) To: Zanoni, Paulo R, intel-gfx, Vivi, Rodrigo On Wed, 18 Nov 2015, "Zanoni, Paulo R" <paulo.r.zanoni@intel.com> wrote: > Em Qua, 2015-11-18 às 11:21 -0800, Rodrigo Vivi escreveu: >> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > s/Cc/Reviewed-by/ I don't think our patchwork is quite smart enough for that yet, so for future reference, please be write the whole thing. BR, Jani. -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] drm/i915: PSR: Let's rely more on frontbuffer tracking. 2015-11-19 11:16 ` Jani Nikula @ 2015-11-19 11:24 ` Zanoni, Paulo R 2015-11-19 12:03 ` Damien Lespiau 0 siblings, 1 reply; 32+ messages in thread From: Zanoni, Paulo R @ 2015-11-19 11:24 UTC (permalink / raw) To: intel-gfx, Vivi, Rodrigo, jani.nikula Em Qui, 2015-11-19 às 13:16 +0200, Jani Nikula escreveu: > On Wed, 18 Nov 2015, "Zanoni, Paulo R" <paulo.r.zanoni@intel.com> > wrote: > > Em Qua, 2015-11-18 às 11:21 -0800, Rodrigo Vivi escreveu: > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > s/Cc/Reviewed-by/ > > I don't think our patchwork is quite smart enough for that yet, so > for > future reference, please be write the whole thing. I completely forgot about this. Sorry. I guess that also means that if I say "If you do this and this and that, then I'll give you Reviewed-by: Paulo Zanoni <paulo.r.zanoni@inte l.com> it will mark the patch as reviewed even though it needs rework, right? I guess we just can't trust the PW automatic tag parsing since it can have either false positives or false negatives. Not until some major breakthroughs in AI. Is suggesting deprecating the use of emails as a way to handle patches still considered trolling? > > BR, > Jani. > > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] drm/i915: PSR: Let's rely more on frontbuffer tracking. 2015-11-19 11:24 ` Zanoni, Paulo R @ 2015-11-19 12:03 ` Damien Lespiau 0 siblings, 0 replies; 32+ messages in thread From: Damien Lespiau @ 2015-11-19 12:03 UTC (permalink / raw) To: Zanoni, Paulo R; +Cc: intel-gfx, Vivi, Rodrigo On Thu, Nov 19, 2015 at 11:24:22AM +0000, Zanoni, Paulo R wrote: > Em Qui, 2015-11-19 às 13:16 +0200, Jani Nikula escreveu: > > On Wed, 18 Nov 2015, "Zanoni, Paulo R" <paulo.r.zanoni@intel.com> > > wrote: > > > Em Qua, 2015-11-18 às 11:21 -0800, Rodrigo Vivi escreveu: > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > s/Cc/Reviewed-by/ > > > > I don't think our patchwork is quite smart enough for that yet, so > > for > > future reference, please be write the whole thing. > > I completely forgot about this. Sorry. > > I guess that also means that if I say "If you do this and this and > that, then I'll give you Reviewed-by: Paulo Zanoni <paulo.r.zanoni@inte > l.com> it will mark the patch as reviewed even though it needs rework, > right? I guess we just can't trust the PW automatic tag parsing since > it can have either false positives or false negatives. Not until some > major breakthroughs in AI. The Reviewed-by parsing is quite simple: it will match '^Reviewed-by:'. That means indenting the tag should be enough to bypass patchwork's accounting. That said, I think we need to extend the language we use for review and make patchwork understand it. For instance, being able to review several patches in one go could be done with: Patches 1-3,6: Reviewed-by: Damien Lespiau damien.lespiau@intel.com (See: https://github.com/dlespiau/patchwork/issues/27) > Is suggesting deprecating the use of emails as a way to handle patches > still considered trolling? No :) The way I see it is that it's easier to progressively enhance what we have to do what we want -- I'll be the first to say patchwork is very fragile today -- rather than switching to something totally different, probably closing the door to non-intel contributors and suddenly having two different systems for core DRM patch handling, among other things. Either way, we have to try to improve what we have. I took one path but it doesn't mean that was the best, someone else can always try to take another set of tools and convince/force people to use that. The goals are the important bit: test every patch before it's merged into -nightly, improve the developer experience and patch flow/tracking along the way. For the first goal, we are almost there (in terms of patchwork, the CI system, piglit, intel-gpu-tools, ... still need quite a bit for work): For instance on series #890, I've uploaded checkpatch.pl test results: http://patchwork.freedesktop.org/series/890/ I'll be deploying that checkpatch.pl test in the next week or so as an example of patchwork/test integration. QA is looking into that integration with BATs at the moment. Of course, email/notifications are part of the plan once happy enough with the tests. For the second goal, it's a long process of small improvements. We're really not there, but interesting things have been created already: I'm quite a fan of git-pw to retrieve series from the ml, for those series correctly parsed that is... Which leads me to the last thing: parsing things from the ml is fragile. The main problems are both people and to a lesser extend mailing-lists: using mails to carry patches does have problems, the vast majority of problems come from people though. People will get stuff wrong: attach patches to the wrong message-id, do things that are plain weird like suddenly attaching patch "2.4/10" as a way to say "oh actually insert a 11th patch in the middle there", typo the review tags, ... I think the last part is solvable, by having a tool wrapping git send-email to do the right things, or at least deterministic things, when sending patches and reviews. That's a bit blue sky stuff, I certainly would love to get there eventually though. Thinking about it, it's wouldn't actually be that complicated to have a start of such a tool, I've captured the first simple thoughts here: https://github.com/dlespiau/patchwork/issues/81 Oh well, it's a way longer email than the one I wanted to write. HTH, -- Damien _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 4/4] drm/i915: PSR: Mask LPSP hw tracking back again. 2015-11-11 21:19 [PATCH 0/4] PSR general improvements and stabilization Rodrigo Vivi ` (2 preceding siblings ...) 2015-11-11 21:20 ` [PATCH 3/4] drm/i915: PSR: Let's rely more on frontbuffer tracking Rodrigo Vivi @ 2015-11-11 21:20 ` Rodrigo Vivi 2015-11-16 19:27 ` Paulo Zanoni 2015-11-24 17:12 ` [PATCH 0/4] PSR general improvements and stabilization Daniel Stone 4 siblings, 1 reply; 32+ messages in thread From: Rodrigo Vivi @ 2015-11-11 21:20 UTC (permalink / raw) To: intel-gfx; +Cc: Rodrigo Vivi At the beginning it was masked to allow PSR at all. Than it got removed later by my commit 09108b90f040 ("drm/i915: PSR: Remove Low Power HW tracking mask.") in order to trying fixing one case reported at intel-gfx mailing list where we were missing screen updates when runtime_pm was enabled. However I verified that other patch that makes flush to force invalidate also fixes this issue by itself. commit 169de1316c1e ("drm/i915: PSR: Flush means invalidate + flush") Mainly now that we are relying more on frontbuffer tracking it is a good idea to mask this hw tracking again. But besides all this above it is important to hightligh that with LPSP unmasked we started seeing some screen freezings as reported at fd.o. v2: Update commit message since this patch by itself doesn't solve the bugzilla entries. Tested-by: Brian Norris <briannorris@chromium.org> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/intel_psr.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index b0e343c..b1b88d1 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -400,9 +400,14 @@ void intel_psr_enable(struct intel_dp *intel_dp) skl_psr_setup_su_vsc(intel_dp); } - /* Avoid continuous PSR exit by masking memup and hpd */ + /* + * Per Spec: Avoid continuous PSR exit by masking MEMUP and HPD. + * Also mask LPSP to avoid dependency on other drivers that + * might block runtime_pm besides preventing other hw tracking + * issues now we can rely on frontbuffer tracking. + */ I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP | - EDP_PSR_DEBUG_MASK_HPD); + EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP); /* Enable PSR on the panel */ hsw_psr_enable_sink(intel_dp); -- 2.4.3 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 4/4] drm/i915: PSR: Mask LPSP hw tracking back again. 2015-11-11 21:20 ` [PATCH 4/4] drm/i915: PSR: Mask LPSP hw tracking back again Rodrigo Vivi @ 2015-11-16 19:27 ` Paulo Zanoni 2015-11-18 0:01 ` Vivi, Rodrigo ` (2 more replies) 0 siblings, 3 replies; 32+ messages in thread From: Paulo Zanoni @ 2015-11-16 19:27 UTC (permalink / raw) To: Rodrigo Vivi; +Cc: Intel Graphics Development 2015-11-11 19:20 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>: > At the beginning it was masked to allow PSR at all. > Than it got removed later by my > commit 09108b90f040 ("drm/i915: PSR: Remove Low Power HW tracking mask.") > in order to trying fixing one case reported at intel-gfx mailing list > where we were missing screen updates when runtime_pm was enabled. > > However I verified that other patch that makes flush to force > invalidate also fixes this issue by itself. > commit 169de1316c1e ("drm/i915: PSR: Flush means invalidate + flush") > > Mainly now that we are relying more on frontbuffer tracking it is a > good idea to mask this hw tracking again. Up until this point you suggest that this patch is not a bug fix, but it also does not add any regression, and it's convenient since it stops us from depending on runtime PM. > > But besides all this above it is important to hightligh that with LPSP > unmasked we started seeing some screen freezings as reported at fd.o. But this sentence suggests the patch actually does fix bugs. But there are also no references to the fd.o bug. Does this patch fix any bug or not? > > v2: Update commit message since this patch by itself doesn't solve > the bugzilla entries. > > Tested-by: Brian Norris <briannorris@chromium.org> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/intel_psr.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index b0e343c..b1b88d1 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -400,9 +400,14 @@ void intel_psr_enable(struct intel_dp *intel_dp) > skl_psr_setup_su_vsc(intel_dp); > } > > - /* Avoid continuous PSR exit by masking memup and hpd */ > + /* > + * Per Spec: Avoid continuous PSR exit by masking MEMUP and HPD. > + * Also mask LPSP to avoid dependency on other drivers that > + * might block runtime_pm besides preventing other hw tracking > + * issues now we can rely on frontbuffer tracking. > + */ > I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP | > - EDP_PSR_DEBUG_MASK_HPD); > + EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP); > > /* Enable PSR on the panel */ > hsw_psr_enable_sink(intel_dp); > -- > 2.4.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Paulo Zanoni _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 4/4] drm/i915: PSR: Mask LPSP hw tracking back again. 2015-11-16 19:27 ` Paulo Zanoni @ 2015-11-18 0:01 ` Vivi, Rodrigo 2015-11-18 19:21 ` [PATCH 2/2] " Rodrigo Vivi 2015-11-18 21:49 ` Rodrigo Vivi 2 siblings, 0 replies; 32+ messages in thread From: Vivi, Rodrigo @ 2015-11-18 0:01 UTC (permalink / raw) To: przanoni; +Cc: intel-gfx On Mon, 2015-11-16 at 17:27 -0200, Paulo Zanoni wrote: > 2015-11-11 19:20 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>: > > At the beginning it was masked to allow PSR at all. > > Than it got removed later by my > > commit 09108b90f040 ("drm/i915: PSR: Remove Low Power HW tracking > > mask.") > > in order to trying fixing one case reported at intel-gfx mailing > > list > > where we were missing screen updates when runtime_pm was enabled. > > > > However I verified that other patch that makes flush to force > > invalidate also fixes this issue by itself. > > commit 169de1316c1e ("drm/i915: PSR: Flush means invalidate + > > flush") > > > > Mainly now that we are relying more on frontbuffer tracking it is a > > good idea to mask this hw tracking again. > > Up until this point you suggest that this patch is not a bug fix, but > it also does not add any regression, and it's convenient since it > stops us from depending on runtime PM. > > > > > But besides all this above it is important to hightligh that with > > LPSP > > unmasked we started seeing some screen freezings as reported at > > fd.o. > > But this sentence suggests the patch actually does fix bugs. But > there > are also no references to the fd.o bug. > > Does this patch fix any bug or not? Sorry, this was a confusion. The bug was fixed by another patch so nothing to worrie. I will update the commit message here. > > > > > v2: Update commit message since this patch by itself doesn't solve > > the bugzilla entries. > > > > Tested-by: Brian Norris <briannorris@chromium.org> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/i915/intel_psr.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > b/drivers/gpu/drm/i915/intel_psr.c > > index b0e343c..b1b88d1 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -400,9 +400,14 @@ void intel_psr_enable(struct intel_dp > > *intel_dp) > > skl_psr_setup_su_vsc(intel_dp); > > } > > > > - /* Avoid continuous PSR exit by masking memup and > > hpd */ > > + /* > > + * Per Spec: Avoid continuous PSR exit by masking > > MEMUP and HPD. > > + * Also mask LPSP to avoid dependency on other > > drivers that > > + * might block runtime_pm besides preventing other > > hw tracking > > + * issues now we can rely on frontbuffer tracking. > > + */ > > I915_WRITE(EDP_PSR_DEBUG_CTL(dev), > > EDP_PSR_DEBUG_MASK_MEMUP | > > - EDP_PSR_DEBUG_MASK_HPD); > > + EDP_PSR_DEBUG_MASK_HPD | > > EDP_PSR_DEBUG_MASK_LPSP); > > > > /* Enable PSR on the panel */ > > hsw_psr_enable_sink(intel_dp); > > -- > > 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 2/2] drm/i915: PSR: Mask LPSP hw tracking back again. 2015-11-16 19:27 ` Paulo Zanoni 2015-11-18 0:01 ` Vivi, Rodrigo @ 2015-11-18 19:21 ` Rodrigo Vivi 2015-11-18 19:29 ` Zanoni, Paulo R 2015-11-18 21:49 ` Rodrigo Vivi 2 siblings, 1 reply; 32+ messages in thread From: Rodrigo Vivi @ 2015-11-18 19:21 UTC (permalink / raw) To: intel-gfx; +Cc: Rodrigo Vivi When we introduced PSR we let LPSP masked allowing us to get PSR independently from the audio runtime PM. However in one of the attempts to get PSR enabled by default one user reported one specific case where he would miss screen updates if scrolling the firefox in a Gnome environment when i915 runtime pm was enabled. So for this specific case that (I could never create an i-g-t test case) we decided to remove the LPSP mask and let HW tracking taking care of this case. The mask got removed later by my commit 09108b90f04 ("drm/i915: PSR: Remove Low Power HW tracking mask.") So we started depending on audio driver again, what is bad. With previous commit "drm/i915: PSR: Let's rely more on frontbuffer tracking." we transfered the PSR exit responsability totally to SW frontbuffer tracking. So now can safelly shut off a bit the HW tracking, or at least this case that makes us to depend on other drivers. v2: Update commit message since this patch by itself doesn't solve the bugzilla entries. v3: Another attempt to improve commit message. Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Tested-by: Brian Norris <briannorris@chromium.org> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/intel_psr.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index b0e343c..b1b88d1 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -400,9 +400,14 @@ void intel_psr_enable(struct intel_dp *intel_dp) skl_psr_setup_su_vsc(intel_dp); } - /* Avoid continuous PSR exit by masking memup and hpd */ + /* + * Per Spec: Avoid continuous PSR exit by masking MEMUP and HPD. + * Also mask LPSP to avoid dependency on other drivers that + * might block runtime_pm besides preventing other hw tracking + * issues now we can rely on frontbuffer tracking. + */ I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP | - EDP_PSR_DEBUG_MASK_HPD); + EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP); /* Enable PSR on the panel */ hsw_psr_enable_sink(intel_dp); -- 2.4.3 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2] drm/i915: PSR: Mask LPSP hw tracking back again. 2015-11-18 19:21 ` [PATCH 2/2] " Rodrigo Vivi @ 2015-11-18 19:29 ` Zanoni, Paulo R 0 siblings, 0 replies; 32+ messages in thread From: Zanoni, Paulo R @ 2015-11-18 19:29 UTC (permalink / raw) To: intel-gfx, Vivi, Rodrigo Em Qua, 2015-11-18 às 11:21 -0800, Rodrigo Vivi escreveu: > When we introduced PSR we let LPSP masked allowing us to get PSR > independently from the audio runtime PM. However in one of the > attempts to get PSR enabled by default one user reported one specific > case where he would miss screen updates if scrolling the firefox in a > Gnome environment when i915 runtime pm was enabled. So for > this specific case that (I could never create an i-g-t test case) > we decided to remove the LPSP mask and let HW tracking taking care of > this case. The mask got removed later by my > commit 09108b90f04 ("drm/i915: PSR: Remove Low Power HW tracking > mask.") > > So we started depending on audio driver again, what is bad. > > With previous commit > "drm/i915: PSR: Let's rely more on frontbuffer tracking." > we transfered the PSR exit responsability totally to SW frontbuffer > tracking. So now can safelly shut off a bit the HW tracking, or > at least this case that makes us to depend on other drivers. > > v2: Update commit message since this patch by itself doesn't solve > the bugzilla entries. > > v3: Another attempt to improve commit message. > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> s/Cc/Reviewed-by/ > Tested-by: Brian Norris <briannorris@chromium.org> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/intel_psr.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > b/drivers/gpu/drm/i915/intel_psr.c > index b0e343c..b1b88d1 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -400,9 +400,14 @@ void intel_psr_enable(struct intel_dp *intel_dp) > skl_psr_setup_su_vsc(intel_dp); > } > > - /* Avoid continuous PSR exit by masking memup and > hpd */ > + /* > + * Per Spec: Avoid continuous PSR exit by masking > MEMUP and HPD. > + * Also mask LPSP to avoid dependency on other > drivers that > + * might block runtime_pm besides preventing other > hw tracking > + * issues now we can rely on frontbuffer tracking. > + */ > I915_WRITE(EDP_PSR_DEBUG_CTL(dev), > EDP_PSR_DEBUG_MASK_MEMUP | > - EDP_PSR_DEBUG_MASK_HPD); > + EDP_PSR_DEBUG_MASK_HPD | > EDP_PSR_DEBUG_MASK_LPSP); > > /* Enable PSR on the panel */ > hsw_psr_enable_sink(intel_dp); _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 2/2] drm/i915: PSR: Mask LPSP hw tracking back again. 2015-11-16 19:27 ` Paulo Zanoni 2015-11-18 0:01 ` Vivi, Rodrigo 2015-11-18 19:21 ` [PATCH 2/2] " Rodrigo Vivi @ 2015-11-18 21:49 ` Rodrigo Vivi 2015-11-23 21:52 ` Rodrigo Vivi 2 siblings, 1 reply; 32+ messages in thread From: Rodrigo Vivi @ 2015-11-18 21:49 UTC (permalink / raw) To: intel-gfx; +Cc: Rodrigo Vivi When we introduced PSR we let LPSP masked allowing us to get PSR independently from the audio runtime PM. However in one of the attempts to get PSR enabled by default one user reported one specific case where he would miss screen updates if scrolling the firefox in a Gnome environment when i915 runtime pm was enabled. So for this specific case that (I could never create an i-g-t test case) we decided to remove the LPSP mask and let HW tracking taking care of this case. The mask got removed later by my commit 09108b90f04 ("drm/i915: PSR: Remove Low Power HW tracking mask.") So we started depending on audio driver again, what is bad. With previous commit "drm/i915: PSR: Let's rely more on frontbuffer tracking." we transfered the PSR exit responsability totally to SW frontbuffer tracking. So now can safelly shut off a bit the HW tracking, or at least this case that makes us to depend on other drivers. v2: Update commit message since this patch by itself doesn't solve the bugzilla entries. v3: Another attempt to improve commit message. Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Tested-by: Brian Norris <briannorris@chromium.org> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/intel_psr.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index b0e343c..b1b88d1 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -400,9 +400,14 @@ void intel_psr_enable(struct intel_dp *intel_dp) skl_psr_setup_su_vsc(intel_dp); } - /* Avoid continuous PSR exit by masking memup and hpd */ + /* + * Per Spec: Avoid continuous PSR exit by masking MEMUP and HPD. + * Also mask LPSP to avoid dependency on other drivers that + * might block runtime_pm besides preventing other hw tracking + * issues now we can rely on frontbuffer tracking. + */ I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP | - EDP_PSR_DEBUG_MASK_HPD); + EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP); /* Enable PSR on the panel */ hsw_psr_enable_sink(intel_dp); -- 2.4.3 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2] drm/i915: PSR: Mask LPSP hw tracking back again. 2015-11-18 21:49 ` Rodrigo Vivi @ 2015-11-23 21:52 ` Rodrigo Vivi 2015-11-24 12:29 ` Daniel Vetter 0 siblings, 1 reply; 32+ messages in thread From: Rodrigo Vivi @ 2015-11-23 21:52 UTC (permalink / raw) To: Rodrigo Vivi, Daniel Vetter; +Cc: intel-gfx Hi Daniel Would you please consider merging patches 2,3 and 4 from this series that are ready to get merged? They don't depend on patch 1 that is under review yet. Thanks, On Wed, Nov 18, 2015 at 1:49 PM, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > When we introduced PSR we let LPSP masked allowing us to get PSR > independently from the audio runtime PM. However in one of the > attempts to get PSR enabled by default one user reported one specific > case where he would miss screen updates if scrolling the firefox in a > Gnome environment when i915 runtime pm was enabled. So for > this specific case that (I could never create an i-g-t test case) > we decided to remove the LPSP mask and let HW tracking taking care of > this case. The mask got removed later by my > commit 09108b90f04 ("drm/i915: PSR: Remove Low Power HW tracking mask.") > > So we started depending on audio driver again, what is bad. > > With previous commit > "drm/i915: PSR: Let's rely more on frontbuffer tracking." > we transfered the PSR exit responsability totally to SW frontbuffer > tracking. So now can safelly shut off a bit the HW tracking, or > at least this case that makes us to depend on other drivers. > > v2: Update commit message since this patch by itself doesn't solve > the bugzilla entries. > > v3: Another attempt to improve commit message. > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Tested-by: Brian Norris <briannorris@chromium.org> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/intel_psr.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index b0e343c..b1b88d1 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -400,9 +400,14 @@ void intel_psr_enable(struct intel_dp *intel_dp) > skl_psr_setup_su_vsc(intel_dp); > } > > - /* Avoid continuous PSR exit by masking memup and hpd */ > + /* > + * Per Spec: Avoid continuous PSR exit by masking MEMUP and HPD. > + * Also mask LPSP to avoid dependency on other drivers that > + * might block runtime_pm besides preventing other hw tracking > + * issues now we can rely on frontbuffer tracking. > + */ > I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP | > - EDP_PSR_DEBUG_MASK_HPD); > + EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP); > > /* Enable PSR on the panel */ > hsw_psr_enable_sink(intel_dp); > -- > 2.4.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Rodrigo Vivi Blog: http://blog.vivi.eng.br _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2] drm/i915: PSR: Mask LPSP hw tracking back again. 2015-11-23 21:52 ` Rodrigo Vivi @ 2015-11-24 12:29 ` Daniel Vetter 0 siblings, 0 replies; 32+ messages in thread From: Daniel Vetter @ 2015-11-24 12:29 UTC (permalink / raw) To: Rodrigo Vivi; +Cc: Daniel Vetter, intel-gfx, Rodrigo Vivi On Mon, Nov 23, 2015 at 01:52:37PM -0800, Rodrigo Vivi wrote: > Hi Daniel > > Would you please consider merging patches 2,3 and 4 from this series > that are ready to get merged? > They don't depend on patch 1 that is under review yet. Done. -Daniel > > Thanks, > > On Wed, Nov 18, 2015 at 1:49 PM, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > > When we introduced PSR we let LPSP masked allowing us to get PSR > > independently from the audio runtime PM. However in one of the > > attempts to get PSR enabled by default one user reported one specific > > case where he would miss screen updates if scrolling the firefox in a > > Gnome environment when i915 runtime pm was enabled. So for > > this specific case that (I could never create an i-g-t test case) > > we decided to remove the LPSP mask and let HW tracking taking care of > > this case. The mask got removed later by my > > commit 09108b90f04 ("drm/i915: PSR: Remove Low Power HW tracking mask.") > > > > So we started depending on audio driver again, what is bad. > > > > With previous commit > > "drm/i915: PSR: Let's rely more on frontbuffer tracking." > > we transfered the PSR exit responsability totally to SW frontbuffer > > tracking. So now can safelly shut off a bit the HW tracking, or > > at least this case that makes us to depend on other drivers. > > > > v2: Update commit message since this patch by itself doesn't solve > > the bugzilla entries. > > > > v3: Another attempt to improve commit message. > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Tested-by: Brian Norris <briannorris@chromium.org> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/i915/intel_psr.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > > index b0e343c..b1b88d1 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -400,9 +400,14 @@ void intel_psr_enable(struct intel_dp *intel_dp) > > skl_psr_setup_su_vsc(intel_dp); > > } > > > > - /* Avoid continuous PSR exit by masking memup and hpd */ > > + /* > > + * Per Spec: Avoid continuous PSR exit by masking MEMUP and HPD. > > + * Also mask LPSP to avoid dependency on other drivers that > > + * might block runtime_pm besides preventing other hw tracking > > + * issues now we can rely on frontbuffer tracking. > > + */ > > I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP | > > - EDP_PSR_DEBUG_MASK_HPD); > > + EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP); > > > > /* Enable PSR on the panel */ > > hsw_psr_enable_sink(intel_dp); > > -- > > 2.4.3 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Rodrigo Vivi > Blog: http://blog.vivi.eng.br -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 0/4] PSR general improvements and stabilization. 2015-11-11 21:19 [PATCH 0/4] PSR general improvements and stabilization Rodrigo Vivi ` (3 preceding siblings ...) 2015-11-11 21:20 ` [PATCH 4/4] drm/i915: PSR: Mask LPSP hw tracking back again Rodrigo Vivi @ 2015-11-24 17:12 ` Daniel Stone 2015-11-24 20:53 ` Vivi, Rodrigo 4 siblings, 1 reply; 32+ messages in thread From: Daniel Stone @ 2015-11-24 17:12 UTC (permalink / raw) To: Rodrigo Vivi; +Cc: intel-gfx Hi Rodrigo, On 11 November 2015 at 21:19, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > Proceeding with the big series split here goes the general PSR > improvements and stabilization work. > > There is no critical fix on this series although I believe it is > good to have all of them before we can enable PSR back by default. For the series: Tested-by: Daniel Stone <daniels@collabora.com> # SKL I haven't managed to get to BDW yet since that relies on IPS bits. Cheers, Daniel _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 0/4] PSR general improvements and stabilization. 2015-11-24 17:12 ` [PATCH 0/4] PSR general improvements and stabilization Daniel Stone @ 2015-11-24 20:53 ` Vivi, Rodrigo 2015-11-25 8:42 ` Daniel Vetter 0 siblings, 1 reply; 32+ messages in thread From: Vivi, Rodrigo @ 2015-11-24 20:53 UTC (permalink / raw) To: daniel; +Cc: intel-gfx On Tue, 2015-11-24 at 17:12 +0000, Daniel Stone wrote: > Hi Rodrigo, > > On 11 November 2015 at 21:19, Rodrigo Vivi <rodrigo.vivi@intel.com> > wrote: > > Proceeding with the big series split here goes the general PSR > > improvements and stabilization work. > > > > There is no critical fix on this series although I believe it is > > good to have all of them before we can enable PSR back by default. > > For the series: > Tested-by: Daniel Stone <daniels@collabora.com> # SKL Thank you very much for this and the aux one. > > I haven't managed to get to BDW yet since that relies on IPS bits. Oh, Daniel or Jani reverted the fastboot so PSR is just working well on BDW regardless that initialization rework. I'm still going to do the encoder fixup that Daniel suggest and accept your suggestions on the IPS one, but they shouldn't be blocking any of the PSR work anymore. > > Cheers, > Daniel _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 0/4] PSR general improvements and stabilization. 2015-11-24 20:53 ` Vivi, Rodrigo @ 2015-11-25 8:42 ` Daniel Vetter 0 siblings, 0 replies; 32+ messages in thread From: Daniel Vetter @ 2015-11-25 8:42 UTC (permalink / raw) To: Vivi, Rodrigo; +Cc: intel-gfx On Tue, Nov 24, 2015 at 08:53:43PM +0000, Vivi, Rodrigo wrote: > On Tue, 2015-11-24 at 17:12 +0000, Daniel Stone wrote: > > Hi Rodrigo, > > > > On 11 November 2015 at 21:19, Rodrigo Vivi <rodrigo.vivi@intel.com> > > wrote: > > > Proceeding with the big series split here goes the general PSR > > > improvements and stabilization work. > > > > > > There is no critical fix on this series although I believe it is > > > good to have all of them before we can enable PSR back by default. > > > > For the series: > > Tested-by: Daniel Stone <daniels@collabora.com> # SKL > > Thank you very much for this and the aux one. > > > > > I haven't managed to get to BDW yet since that relies on IPS bits. > > Oh, Daniel or Jani reverted the fastboot so PSR is just working well on > BDW regardless that initialization rework. > > I'm still going to do the encoder fixup that Daniel suggest and accept > your suggestions on the IPS one, but they shouldn't be blocking any of > the PSR work anymore. Yeah, my plan is to re-enable fastboot just in -nightly for better test coverage. Similar to how we enable psr/fbc and all that stuff in testcases, for better coverage. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2015-12-02 17:29 UTC | newest] Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-11-11 21:19 [PATCH 0/4] PSR general improvements and stabilization Rodrigo Vivi 2015-11-11 21:19 ` [PATCH 1/4] drm/i915: Force PSR exit when IRQ_HPD is detected on eDP Rodrigo Vivi 2015-11-16 16:55 ` Ville Syrjälä 2015-11-18 19:19 ` [PATCH] " Rodrigo Vivi 2015-11-23 21:29 ` Rodrigo Vivi 2015-12-01 18:56 ` Ville Syrjälä 2015-12-01 19:44 ` Vivi, Rodrigo 2015-12-02 9:42 ` Ville Syrjälä 2015-12-02 17:29 ` Vivi, Rodrigo 2015-11-11 21:19 ` [PATCH 2/4] drm/i915: Remove duplicated dpcd write on hsw_psr_enable_sink Rodrigo Vivi 2015-11-12 22:46 ` [PATCH] " Rodrigo Vivi 2015-11-16 16:44 ` Paulo Zanoni 2015-11-11 21:20 ` [PATCH 3/4] drm/i915: PSR: Let's rely more on frontbuffer tracking Rodrigo Vivi 2015-11-14 0:56 ` [PATCH] " Rodrigo Vivi 2015-11-16 18:57 ` Paulo Zanoni 2015-11-16 20:39 ` Vivi, Rodrigo 2015-11-18 19:21 ` [PATCH 1/2] " Rodrigo Vivi 2015-11-18 19:27 ` Zanoni, Paulo R 2015-11-19 11:16 ` Jani Nikula 2015-11-19 11:24 ` Zanoni, Paulo R 2015-11-19 12:03 ` Damien Lespiau 2015-11-11 21:20 ` [PATCH 4/4] drm/i915: PSR: Mask LPSP hw tracking back again Rodrigo Vivi 2015-11-16 19:27 ` Paulo Zanoni 2015-11-18 0:01 ` Vivi, Rodrigo 2015-11-18 19:21 ` [PATCH 2/2] " Rodrigo Vivi 2015-11-18 19:29 ` Zanoni, Paulo R 2015-11-18 21:49 ` Rodrigo Vivi 2015-11-23 21:52 ` Rodrigo Vivi 2015-11-24 12:29 ` Daniel Vetter 2015-11-24 17:12 ` [PATCH 0/4] PSR general improvements and stabilization Daniel Stone 2015-11-24 20:53 ` Vivi, Rodrigo 2015-11-25 8:42 ` Daniel Vetter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.