[PATCH v3 2/2] drm/i915/display: Prevent DC6 while vblank is enabled for Panel Replay

[Hogander, Jouni]

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 at 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
>