Re: [PATCH v3 1/2] drm/i915/display: Add block_dc6_needed variable into intel_crtc

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Sep 18, 2024 at 05:53:37AM +0000, Hogander, Jouni wrote:
> On Tue, 2024-09-17 at 20:58 +0300, Ville Syrjälä wrote:
> > On Tue, Sep 17, 2024 at 09:35:59AM +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. For this purpose add new
> > > block_dc6_needed
> > > variable into intel_crtc. Check if eDP Panel Replay is possible and
> > > set the
> > > variable accordingly.
> > > 
> > > v3: check that encoder is dp
> > > v2: set/clear block_dc6_needed in intel_crtc_vblank_on/off
> > > 
> > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_crtc.c       | 17
> > > +++++++++++++++++
> > >  .../gpu/drm/i915/display/intel_display_types.h  |  7 +++++++
> > >  drivers/gpu/drm/i915/display/intel_psr.c        |  1 +
> > >  3 files changed, 25 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c
> > > b/drivers/gpu/drm/i915/display/intel_crtc.c
> > > index aed3853952be8..34a60b5b1e55b 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> > > @@ -24,6 +24,7 @@
> > >  #include "intel_display_irq.h"
> > >  #include "intel_display_trace.h"
> > >  #include "intel_display_types.h"
> > > +#include "intel_dp.h"
> > >  #include "intel_drrs.h"
> > >  #include "intel_dsi.h"
> > >  #include "intel_fifo_underrun.h"
> > > @@ -123,6 +124,20 @@ u32 intel_crtc_max_vblank_count(const struct
> > > intel_crtc_state *crtc_state)
> > >  void intel_crtc_vblank_on(const struct intel_crtc_state
> > > *crtc_state)
> > >  {
> > >         struct intel_crtc *crtc = to_intel_crtc(crtc_state-
> > > >uapi.crtc);
> > > +       struct intel_encoder *encoder;
> > > +
> > > +       for_each_encoder_on_crtc(crtc->base.dev, &crtc->base,
> > > encoder) {
> > > +               struct intel_dp *intel_dp;
> > > +
> > > +               if (!intel_encoder_is_dp(encoder))
> > > +                       continue;
> > > +
> > > +               intel_dp = enc_to_intel_dp(encoder);
> > > +
> > > +               if (intel_dp_is_edp(intel_dp) &&
> > > +                   CAN_PANEL_REPLAY(intel_dp))
> > > +                       crtc->block_dc6_needed = true;
> > > +       }
> > 
> > This could just a function provided by intel_psr.c so that
> > we don't have to to see any of the details.
> > 
> > Is there some reason this isn't simply looking at
> > crtc_state->has_panel_replay?
> 
> Is there intel_crtc_vblank_off/on cycle always when doing full mode
> set? If that is the case, then I think we can rely on crtc_state-
> >has_panel_replay: changes in Panel Replay mode always mean full mode
> set currently. How about fast mode set? Do we have vblank off/on cycle
> there?

No. vblank_off()/on() is only around full modesets.

> 
> Later if we move into activating/de-activating Panel Replay without
> full mode set I think we need to do something else.

I think we need a clear separation of the "logically enabled/possible"
vs. "currently active" states of PSR and panel replay. With that
we can just always enable this workaround whenever panel replay
was selected during the full modeset. Fastsets/plane updates
can then just activate/deactivate panel replay/PSR (*) as needed
due to more dynamic constraints (eg. planes going on/off) without
having to worry about this stuff.

(*) the activate/deactive should only toggle the single enable
    bit in the appropriate registers, nothing more

> E.g. reference
> count I had in previous version was trying to address this. To my
> opinion relying on CAN_PANEL_REPLAY could be good enough. That would
> cover all cases where blocking is needed. Drawback is that we are
> unnecessarily blocking it on certain cases. But that shouldn't matter
> as in these cases DC6 is blocked anyways by the HW. One example is
> panel supporting Panel Replay, but only PSR2 is allowed and VBI is
> enabled. What do you think?
> 
> BR,
> 
> Jouni Högander
> 
> > 
> > >  
> > >         assert_vblank_disabled(&crtc->base);
> > >         drm_crtc_set_max_vblank_count(&crtc->base,
> > > @@ -150,6 +165,8 @@ void intel_crtc_vblank_off(const struct
> > > intel_crtc_state *crtc_state)
> > >  
> > >         drm_crtc_vblank_off(&crtc->base);
> > >         assert_vblank_disabled(&crtc->base);
> > > +
> > > +       crtc->block_dc6_needed = false;
> > >  }
> > >  
> > >  struct intel_crtc_state *intel_crtc_state_alloc(struct intel_crtc
> > > *crtc)
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > index 000ab373c8879..df0c3eb750809 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > @@ -1413,6 +1413,13 @@ struct intel_crtc {
> > >  #ifdef CONFIG_DEBUG_FS
> > >         struct intel_pipe_crc pipe_crc;
> > >  #endif
> > > +
> > > +       /*
> > > +        * 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.
> > > +        */
> > 
> > We should point out the fact that panel replay turns the
> > link off only while in DC states. Otherwise I'm sure to
> > get confused by this again in the future.
> > 
> > > +       u8 block_dc6_needed;
> > 
> > That sounds a bit too generic perhaps. block_dc_for_vblank or
> > something?
> 
> Ok, I will change these.
> 
> BR,
> 
> Jouni Högander
> 
> > 
> > >  };
> > >  
> > >  struct intel_plane {
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index 4f29ac32ff85b..957f470b08fe8 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -35,6 +35,7 @@
> > >  #include "intel_cursor_regs.h"
> > >  #include "intel_ddi.h"
> > >  #include "intel_de.h"
> > > +#include "intel_display_irq.h"
> > >  #include "intel_display_types.h"
> > >  #include "intel_dp.h"
> > >  #include "intel_dp_aux.h"
> > > -- 
> > > 2.34.1
> > 
> 

-- 
Ville Syrjälä
Intel