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@xxxxxxxxx> > > --- > > .../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 >