| Message ID | 20240917063600.3086259-3-jouni.hogander@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Block DC6 on Vblank enable for Panel Replay | expand |
On Tue, Sep 17, 2024 at 09:36:00AM +0300, Jouni Högander wrote: > We need to block DC6 entry in case of Panel Replay as enabling VBI doesn't > prevent DC6 in case of Panel Replay. This causes problems if user-space is > polling for vblank events. > > Fix this by setting target DC state as DC_STATE_EN_UPTO_DC5 when both > source and sink are supporting eDP Panel Replay and VBI is enabled. > > v2: > - use READ_ONCE in intel_display_vblank_work > - use DC_STATE_DISABLE instead of DC_STATE_EN_UPTO_DC6 > - use intel_crtc->block_dc6_needed > > Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2296 > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > --- > .../gpu/drm/i915/display/intel_display_core.h | 2 ++ > .../gpu/drm/i915/display/intel_display_irq.c | 28 +++++++++++++++++++ > 2 files changed, 30 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h > index 0a711114ff2b4..0707bc2047931 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_core.h > +++ b/drivers/gpu/drm/i915/display/intel_display_core.h > @@ -457,6 +457,8 @@ struct intel_display { > /* For i915gm/i945gm vblank irq workaround */ > u8 vblank_enabled; > > + struct work_struct vblank_work; > + > u32 de_irq_mask[I915_MAX_PIPES]; > u32 pipestat_irq_mask[I915_MAX_PIPES]; > } irq; > diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c b/drivers/gpu/drm/i915/display/intel_display_irq.c > index 8f13f148c73e3..4bdc67e1baa31 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_irq.c > +++ b/drivers/gpu/drm/i915/display/intel_display_irq.c > @@ -1361,16 +1361,38 @@ static bool gen11_dsi_configure_te(struct intel_crtc *intel_crtc, > return true; > } > > +static void intel_display_vblank_work(struct work_struct *work) > +{ > + struct intel_display *display = > + container_of(work, typeof(*display), irq.vblank_work); > + struct drm_i915_private *i915 = to_i915(display->drm); > + u8 vblank_enabled = READ_ONCE(display->irq.vblank_enabled); Could be a bool since you don't use the numeric value for anything. Or could just not have a local variable since you only use it once anyway. > + > + /* > + * NOTE: intel_display_power_set_target_dc_state is used only by PSR > + * code for DC3CO handling. DC3CO target state is currently disabled in > + * PSR code. If DC3CO is taken into use we need take that into account > + * here as well. > + */ > + intel_display_power_set_target_dc_state(i915, vblank_enabled ? DC_STATE_DISABLE : > + DC_STATE_EN_UPTO_DC6); > +} > + > int bdw_enable_vblank(struct drm_crtc *_crtc) > { > struct intel_crtc *crtc = to_intel_crtc(_crtc); > + struct intel_display *display = to_intel_display(crtc); > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > enum pipe pipe = crtc->pipe; > unsigned long irqflags; > + u8 block_dc6_needed = READ_ONCE(crtc->block_dc6_needed); This doesn't really need the read once dance IMO since this will never change between vblank on/off. Feels like the introduction of that flag should also be part of this patch, and this should be the first patch, and the second patch would then just figure out when to set said flag. > > if (gen11_dsi_configure_te(crtc, true)) > return 0; > > + if (display->irq.vblank_enabled++ == 0 && block_dc6_needed) > + schedule_work(&display->irq.vblank_work); > + > spin_lock_irqsave(&dev_priv->irq_lock, irqflags); > bdw_enable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK); > spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > @@ -1436,6 +1458,7 @@ void ilk_disable_vblank(struct drm_crtc *crtc) > void bdw_disable_vblank(struct drm_crtc *_crtc) > { > struct intel_crtc *crtc = to_intel_crtc(_crtc); > + struct intel_display *display = to_intel_display(crtc); > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > enum pipe pipe = crtc->pipe; > unsigned long irqflags; > @@ -1446,6 +1469,9 @@ void bdw_disable_vblank(struct drm_crtc *_crtc) > spin_lock_irqsave(&dev_priv->irq_lock, irqflags); > bdw_disable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK); > spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > + > + if (--display->irq.vblank_enabled == 0) This one seems to be missing the block_dc6_needed check. > + schedule_work(&display->irq.vblank_work); > } > > void vlv_display_irq_reset(struct drm_i915_private *dev_priv) > @@ -1871,4 +1897,6 @@ void intel_display_irq_init(struct drm_i915_private *i915) > i915->display.irq.display_irqs_enabled = false; > > intel_hotplug_irq_init(i915); > + > + INIT_WORK(&i915->display.irq.vblank_work, intel_display_vblank_work); I'd probably also toss in a flush_work() at the end of intel_vblank_off() to make sure the work doesn't linger past its due date.
On Tue, 2024-09-17 at 21:15 +0300, Ville Syrjälä wrote: > On Tue, Sep 17, 2024 at 09:36:00AM +0300, Jouni Högander wrote: > > We need to block DC6 entry in case of Panel Replay as enabling VBI > > doesn't > > prevent DC6 in case of Panel Replay. This causes problems if user- > > space is > > polling for vblank events. > > > > Fix this by setting target DC state as DC_STATE_EN_UPTO_DC5 when > > both > > source and sink are supporting eDP Panel Replay and VBI is enabled. > > > > v2: > > - use READ_ONCE in intel_display_vblank_work > > - use DC_STATE_DISABLE instead of DC_STATE_EN_UPTO_DC6 > > - use intel_crtc->block_dc6_needed > > > > Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2296 > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > --- > > .../gpu/drm/i915/display/intel_display_core.h | 2 ++ > > .../gpu/drm/i915/display/intel_display_irq.c | 28 > > +++++++++++++++++++ > > 2 files changed, 30 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h > > b/drivers/gpu/drm/i915/display/intel_display_core.h > > index 0a711114ff2b4..0707bc2047931 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_core.h > > +++ b/drivers/gpu/drm/i915/display/intel_display_core.h > > @@ -457,6 +457,8 @@ struct intel_display { > > /* For i915gm/i945gm vblank irq workaround */ > > u8 vblank_enabled; > > > > + struct work_struct vblank_work; > > + > > u32 de_irq_mask[I915_MAX_PIPES]; > > u32 pipestat_irq_mask[I915_MAX_PIPES]; > > } irq; > > diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c > > b/drivers/gpu/drm/i915/display/intel_display_irq.c > > index 8f13f148c73e3..4bdc67e1baa31 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_irq.c > > +++ b/drivers/gpu/drm/i915/display/intel_display_irq.c > > @@ -1361,16 +1361,38 @@ static bool gen11_dsi_configure_te(struct > > intel_crtc *intel_crtc, > > return true; > > } > > > > +static void intel_display_vblank_work(struct work_struct *work) > > +{ > > + struct intel_display *display = > > + container_of(work, typeof(*display), > > irq.vblank_work); > > + struct drm_i915_private *i915 = to_i915(display->drm); > > + u8 vblank_enabled = READ_ONCE(display->irq.vblank_enabled); > > Could be a bool since you don't use the numeric value for anything. > Or could just not have a local variable since you only use it once > anyway. I will change this. > > > + > > + /* > > + * NOTE: intel_display_power_set_target_dc_state is used > > only by PSR > > + * code for DC3CO handling. DC3CO target state is currently > > disabled in > > + * PSR code. If DC3CO is taken into use we need take that > > into account > > + * here as well. > > + */ > > + intel_display_power_set_target_dc_state(i915, > > vblank_enabled ? DC_STATE_DISABLE : > > + DC_STATE_EN_UPTO_DC > > 6); > > +} > > + > > int bdw_enable_vblank(struct drm_crtc *_crtc) > > { > > struct intel_crtc *crtc = to_intel_crtc(_crtc); > > + struct intel_display *display = to_intel_display(crtc); > > struct drm_i915_private *dev_priv = to_i915(crtc- > > >base.dev); > > enum pipe pipe = crtc->pipe; > > unsigned long irqflags; > > + u8 block_dc6_needed = READ_ONCE(crtc->block_dc6_needed); > > This doesn't really need the read once dance IMO since this > will never change between vblank on/off. > > Feels like the introduction of that flag should also be part of > this patch, and this should be the first patch, and the second > patch would then just figure out when to set said flag. Ok, I will change this. > > > > > if (gen11_dsi_configure_te(crtc, true)) > > return 0; > > > > + if (display->irq.vblank_enabled++ == 0 && block_dc6_needed) > > + schedule_work(&display->irq.vblank_work); > > + > > spin_lock_irqsave(&dev_priv->irq_lock, irqflags); > > bdw_enable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK); > > spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > > @@ -1436,6 +1458,7 @@ void ilk_disable_vblank(struct drm_crtc > > *crtc) > > void bdw_disable_vblank(struct drm_crtc *_crtc) > > { > > struct intel_crtc *crtc = to_intel_crtc(_crtc); > > + struct intel_display *display = to_intel_display(crtc); > > struct drm_i915_private *dev_priv = to_i915(crtc- > > >base.dev); > > enum pipe pipe = crtc->pipe; > > unsigned long irqflags; > > @@ -1446,6 +1469,9 @@ void bdw_disable_vblank(struct drm_crtc > > *_crtc) > > spin_lock_irqsave(&dev_priv->irq_lock, irqflags); > > bdw_disable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK); > > spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > > + > > + if (--display->irq.vblank_enabled == 0) > > This one seems to be missing the block_dc6_needed check. Yes, that was left out as in version 2 block_dc6_needed was reference count. I will add it. > > > + schedule_work(&display->irq.vblank_work); > > } > > > > void vlv_display_irq_reset(struct drm_i915_private *dev_priv) > > @@ -1871,4 +1897,6 @@ void intel_display_irq_init(struct > > drm_i915_private *i915) > > i915->display.irq.display_irqs_enabled = false; > > > > intel_hotplug_irq_init(i915); > > + > > + INIT_WORK(&i915->display.irq.vblank_work, > > intel_display_vblank_work); > > I'd probably also toss in a flush_work() at the end of > intel_vblank_off() to make sure the work doesn't linger > past its due date. Ok, I will add that. BR, Jouni Högander >
diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h index 0a711114ff2b4..0707bc2047931 100644 --- a/drivers/gpu/drm/i915/display/intel_display_core.h +++ b/drivers/gpu/drm/i915/display/intel_display_core.h @@ -457,6 +457,8 @@ struct intel_display { /* For i915gm/i945gm vblank irq workaround */ u8 vblank_enabled; + struct work_struct vblank_work; + u32 de_irq_mask[I915_MAX_PIPES]; u32 pipestat_irq_mask[I915_MAX_PIPES]; } irq; diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c b/drivers/gpu/drm/i915/display/intel_display_irq.c index 8f13f148c73e3..4bdc67e1baa31 100644 --- a/drivers/gpu/drm/i915/display/intel_display_irq.c +++ b/drivers/gpu/drm/i915/display/intel_display_irq.c @@ -1361,16 +1361,38 @@ static bool gen11_dsi_configure_te(struct intel_crtc *intel_crtc, return true; } +static void intel_display_vblank_work(struct work_struct *work) +{ + struct intel_display *display = + container_of(work, typeof(*display), irq.vblank_work); + struct drm_i915_private *i915 = to_i915(display->drm); + u8 vblank_enabled = READ_ONCE(display->irq.vblank_enabled); + + /* + * NOTE: intel_display_power_set_target_dc_state is used only by PSR + * code for DC3CO handling. DC3CO target state is currently disabled in + * PSR code. If DC3CO is taken into use we need take that into account + * here as well. + */ + intel_display_power_set_target_dc_state(i915, vblank_enabled ? DC_STATE_DISABLE : + DC_STATE_EN_UPTO_DC6); +} + int bdw_enable_vblank(struct drm_crtc *_crtc) { struct intel_crtc *crtc = to_intel_crtc(_crtc); + struct intel_display *display = to_intel_display(crtc); struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); enum pipe pipe = crtc->pipe; unsigned long irqflags; + u8 block_dc6_needed = READ_ONCE(crtc->block_dc6_needed); if (gen11_dsi_configure_te(crtc, true)) return 0; + if (display->irq.vblank_enabled++ == 0 && block_dc6_needed) + schedule_work(&display->irq.vblank_work); + spin_lock_irqsave(&dev_priv->irq_lock, irqflags); bdw_enable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK); spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); @@ -1436,6 +1458,7 @@ void ilk_disable_vblank(struct drm_crtc *crtc) void bdw_disable_vblank(struct drm_crtc *_crtc) { struct intel_crtc *crtc = to_intel_crtc(_crtc); + struct intel_display *display = to_intel_display(crtc); struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); enum pipe pipe = crtc->pipe; unsigned long irqflags; @@ -1446,6 +1469,9 @@ void bdw_disable_vblank(struct drm_crtc *_crtc) spin_lock_irqsave(&dev_priv->irq_lock, irqflags); bdw_disable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK); spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); + + if (--display->irq.vblank_enabled == 0) + schedule_work(&display->irq.vblank_work); } void vlv_display_irq_reset(struct drm_i915_private *dev_priv) @@ -1871,4 +1897,6 @@ void intel_display_irq_init(struct drm_i915_private *i915) i915->display.irq.display_irqs_enabled = false; intel_hotplug_irq_init(i915); + + INIT_WORK(&i915->display.irq.vblank_work, intel_display_vblank_work); }
We need to block DC6 entry in case of Panel Replay as enabling VBI doesn't prevent DC6 in case of Panel Replay. This causes problems if user-space is polling for vblank events. Fix this by setting target DC state as DC_STATE_EN_UPTO_DC5 when both source and sink are supporting eDP Panel Replay and VBI is enabled. v2: - use READ_ONCE in intel_display_vblank_work - use DC_STATE_DISABLE instead of DC_STATE_EN_UPTO_DC6 - use intel_crtc->block_dc6_needed Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2296 Signed-off-by: Jouni Högander <jouni.hogander@intel.com> --- .../gpu/drm/i915/display/intel_display_core.h | 2 ++ .../gpu/drm/i915/display/intel_display_irq.c | 28 +++++++++++++++++++ 2 files changed, 30 insertions(+)